<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/126189/">https://git.reviewboard.kde.org/r/126189/</a>
</td>
</tr>
</table>
<br />
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for KDE Frameworks and David Faure.</div>
<div>By Michael Pyne.</div>
<p style="color: grey;"><i>Updated Dec. 2, 2015, 3:51 a.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Changes</h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">- Change the domain stripping behavior to better match original behavior (the original code could have stripped the domain to a single component, I have it set to keep at least 2... should it be different?).
- Rebased against latest HEAD
- kaboutdatatest still passes</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://bugs.kde.org/show_bug.cgi?id=355508">355508</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kcoreaddons
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This commit permits URLs such as "https://www.foo.org" (i.e. with non-http schemas), which bug 355508 points out is something we don't currently support, and expands the test suite to ensure http and https URLs give the same behavior when deriving desktop file names and organization domains.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The existing code seems to operate the stripping the first URL host component (e.g. www.foo.org -> foo.org) for the purposes of generating an organization domain or desktop file name. I'm not sure if that was intentional or not, but I found it easier to just limit to two components instead (e.g. www.product.foo.org -> foo.org). Everything else should be pretty straightforward for review I think... much of the code change is simply because I tried to fix by making the current code scheme-independent instead of just adding a '|| scheme == "https"'.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I also modified the API docs to reflect the change. I will add the appropriate @since based on when I get a Ship It! ;)</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">All tests pass in kaboutdatatest, including the extra test cases added to test https:// URLs specifically.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I also had hacked in some qDebug()s and ran with several KDE apps, all of which generated reasonable organization domains.</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> (updated)</h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>src/lib/kaboutdata.h <span style="color: grey">(e9fc56b)</span></li>
<li>src/lib/kaboutdata.cpp <span style="color: grey">(de19e6f)</span></li>
<li>autotests/kaboutdatatest.cpp <span style="color: grey">(96b3a13)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/126189/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>