----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126189/#review88913 -----------------------------------------------------------
Most of this looks good, but I'm not sure about the "2 components" parts of it. www.mydomain.co.uk would lead to "co.uk" ? I'd suggest not limiting to two, it's unrelated to the bug you're fixing anyway - David Faure On Nov. 27, 2015, 11:14 p.m., Michael Pyne wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/126189/ > ----------------------------------------------------------- > > (Updated Nov. 27, 2015, 11:14 p.m.) > > > Review request for KDE Frameworks and David Faure. > > > Bugs: 355508 > https://bugs.kde.org/show_bug.cgi?id=355508 > > > Repository: kcoreaddons > > > Description > ------- > > 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. > > 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"'. > > I also modified the API docs to reflect the change. I will add the > appropriate @since based on when I get a Ship It! ;) > > > Diffs > ----- > > autotests/kaboutdatatest.cpp 96b3a13 > src/lib/kaboutdata.h e9fc56b > src/lib/kaboutdata.cpp de19e6f > > Diff: https://git.reviewboard.kde.org/r/126189/diff/ > > > Testing > ------- > > All tests pass in kaboutdatatest, including the extra test cases added to > test https:// URLs specifically. > > I also had hacked in some qDebug()s and ran with several KDE apps, all of > which generated reasonable organization domains. > > > Thanks, > > Michael Pyne > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
