[WTFURL] Add a class to represent the segments of a URL
Created attachment 55090 [details] Patch
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.
> 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.
Created attachment 55172 [details] Patch
Created attachment 55189 [details] patch w/ update license block
Comment on attachment 55189 [details] patch w/ update license block r=me
Comment on attachment 55189 [details] patch w/ update license block Thanks!
Comment on attachment 55189 [details] patch w/ update license block Clearing flags on attachment: 55189 Committed r58874: <http://trac.webkit.org/changeset/58874>
All reviewed patches have been landed. Closing bug.