[webkit-reviews] review denied: [Bug 38566] [WTFURL] Add a class to represent the segments of a URL : [Attachment 55090] Patch

[bugzilla-daemon at webkit.org]

Maciej Stachowiak <mjs at apple.com> has denied Adam Barth <abarth at webkit.org>'s
request for review:
Bug 38566: [WTFURL] Add a class to represent the segments of a URL
https://bugs.webkit.org/show_bug.cgi?id=38566

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

------- Additional Comments from Maciej Stachowiak <mjs at apple.com>

JavaScriptCore/wtf/url/src/URLSegments.h:88
 +	//	  *Ref: 20		     20
Perhaps "Ref" should be called "Fragment" to match the RFC naming.

JavaScriptCore/wtf/url/src/URLSegments.h:108
 +	// Host name.
This comment doesn't seem to add any value.

JavaScriptCore/wtf/url/src/URLSegments.h:111
 +	// Port number.
This comment doesn't seem to add any value.

The other com,ments do convey some information, but they seem needlessly
verbose. It seems like there is a common pattern of excluding delimiters from
the range, and having -1 length if the delimiter character for that component
is missing. Perhaps we could just say this once, instead of separately for
every component.

JavaScriptCore/wtf/url/src/URLSegments.h:53
 +	// The default constructor is sufficient for the components.
This comment doesn't add any value.

JavaScriptCore/wtf/url/src/URLSegments.h:41
 +	// Identifies different components.
This comment doesn't add any value.

JavaScriptCore/wtf/url/src/URLSegments.h:130
 +	URLComponent ref;
ref should be fragment (again)

Please address
JavaScriptCore/wtf/url/src/URLSegments.cpp:58
 +	    current = scheme.end() + 1; // Advance over the ':' at the end of
the scheme.
If this code expects to be advancing past a specific character, perhaps an
ASSERT would be a better way to document that than a comment.

JavaScriptCore/wtf/url/src/URLSegments.cpp:63
 +	    current = username.end() + 1; // Advance over the '@' or ':' at the
end.
If this code expects to be advancing past a specific character, perhaps an
ASSERT would be a better way to document that than a comment.


JavaScriptCore/wtf/url/src/URLSegments.cpp:69
 +	    current = password.end() + 1; // Advance over the '@' at the end.
If this code expects to be advancing past a specific character, perhaps an
ASSERT would be a better way to document that than a comment.


JavaScriptCore/wtf/url/src/URLSegments.cpp:80
 +		return port.begin() - 1; // Back over delimiter.
If this code expects to be advancing past a specific character, perhaps an
ASSERT would be a better way to document that than a comment.


JavaScriptCore/wtf/url/src/URLSegments.cpp:94
 +		return query.begin() - 1; // Back over delimiter.
If this code expects to be advancing past a specific character, perhaps an
ASSERT would be a better way to document that than a comment.


JavaScriptCore/wtf/url/src/URLSegments.cpp:102
 +		return ref.begin(); // Back over delimiter.
If this code expects to be advancing past a specific character, perhaps an
ASSERT would be a better way to document that than a comment.


JavaScriptCore/wtf/url/src/URLSegments.cpp:54
 +	// There will be some characters after the scheme like "://" and we
don't
I don't see anything in this code that seems specific to advancing pass ://, it
seems to just look at all the component ranges in turn.


r- for now to address the above issues. None of them are showstoppers, but it
seems like there are enough different comments to warrant a new patch version.