[webkit-reviews] review granted: [Bug 105590] Add functions for detecting top level and high level domain names : [Attachment 181405] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Jeffrey Pfau <jeffrey at endrift.com>'s
request for review:
Bug 105590: Add functions for detecting top level and high level domain names
https://bugs.webkit.org/show_bug.cgi?id=105590

Attachment 181405: Patch
https://bugs.webkit.org/attachment.cgi?id=181405&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=181405&action=review


A patch that adds a function, currently unused by WebKit except in its own unit
tests, is unusual for the WebKit project. Is there some simple use of this that
could be incorporated with the patch so the code would be both added and used?

> Source/WebCore/ChangeLog:21
> +	   (WebCore):

Please remove bogus lines like this one from change log and/or fix the script
so it doesn’t add them.

> Source/WebCore/DerivedSources.make:1076
> +SuffixList.cpp : make-suffix-list.py platform/SuffixList.in
> +	python $^

How does the script know where to write its output? Normally we would have to
pass $@ to the script too.

> Source/WebCore/platform/PublicSuffix.cpp:40
> +String childStateName(const SubstateTuple*);
> +
> +String childStateName(const SubstateTuple* tuple)

This is an anti-pattern. If the function is used outside this file, then it
should be declared in a header file. If it’s not used outside the file its
definition should be marked “static” and so we would not need a separate
declaration.

> Source/WebCore/platform/PublicSuffix.cpp:44
> +    return String(&publicSuffixStrings[tuple->nameOffset]);

It seems wasteful to construct a String just to return a null-terminated C
string. Is there some reason we have to wrap these in String objects? If we
just want to compare with a String as is done in the searchForState function we
can do that without doing memory allocation.

> Source/WebCore/platform/PublicSuffix.cpp:87
> +    for (size_t nextDot = UINT_MAX, previousDot = domain.reverseFind('.',
UINT_MAX); nextDot != notFound; nextDot = previousDot, previousDot =
domain.reverseFind('.', nextDot - 1)) {

The use of UINT_MAX here is strange. The type of nextDot is not unsigned, so
UINT_MAX is the wrong maximum. I would have suggested using
numeric_limits<size_t>::max(),

For reverseFind, I think you should just omit the second argument, because it
starts from the end of the string by default. But also, we could probably
rewrite the loop so the start code and next code didn’t have to repeat the
reverseFind.

> Source/WebCore/platform/PublicSuffix.cpp:91
> +	       substate = searchForState(state, "*");

Seems unfortunate that this will construct a String containing the single
character "*" repeatedly an in an inefficient way.

> Tools/ChangeLog:12
> +	   (TestWebKitAPI):

Again, bogus line that should be deleted.