Bug 38566 – [WTFURL] Add a class to represent the segments of a URL

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Barth]

[WTFURL] Add a class to represent the segments of a URL

[Adam Barth]

Created attachment 55090 [details]
Patch

[Maciej Stachowiak]

Comment on attachment 55090 [details]
Patch


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.

[Adam Barth]

> 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.

The string isn't available here (because it's stored separately from the segmentation), so we can't actually assert that here.

> 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.

I've addressed the remainder.  I'll be more agressive about making the comments more WebKit-like in future patches.

[Adam Barth]

Created attachment 55172 [details]
Patch

[Adam Barth]

Created attachment 55189 [details]
patch w/ update license block

[Maciej Stachowiak]

Comment on attachment 55189 [details]
patch w/ update license block

r=me

[Adam Barth]

Comment on attachment 55189 [details]
patch w/ update license block

Thanks!

[WebKit Commit Bot]

Comment on attachment 55189 [details]
patch w/ update license block

Clearing flags on attachment: 55189

Committed r58874: <http://trac.webkit.org/changeset/58874>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.