Expose isValidProtocol() in KURL.h.
Created attachment 82710 [details] Patch
The implementation of isValidProtocol() in KURLGoogle.cpp is basically a copy/paste of the one in KURL.cpp - I'm not sure if this is kosher from a code duplication or plagiarism point of view (the function is quite trivial and any reasonable implementation would be isomorphic to the original) so any guidance on this matter would be appreciated.
Comment on attachment 82710 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=82710&action=review > Source/WebCore/ChangeLog:7 > + Expose isValidProtocol() in KURL.h. > + https://bugs.webkit.org/show_bug.cgi?id=54594 > + Why?
(In reply to comment #3) > (From update of attachment 82710 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=82710&action=review > > > Source/WebCore/ChangeLog:7 > > + Expose isValidProtocol() in KURL.h. > > + https://bugs.webkit.org/show_bug.cgi?id=54594 > > + > > Why? For general use, but in particular because I want to check for invalid schemes being used in calls to navigator.registerProtocolHandler (see bug 54438). Sorry, I'll add that to the ChangeLog.
Created attachment 82714 [details] Patch
CCing URL folks
Should we rename this to isValidProtocolName() when exposing? Or even isValidSchemeName()?
Comment on attachment 82714 [details] Patch This seems tricky. How flexible should this be? Should this allow leading/trailing spaces? Why is this only implemented for the Google side? is "a" a valid scheme? Seems this says it is. Can we test this?
(In reply to comment #8) > (From update of attachment 82714 [details]) > This seems tricky. How flexible should this be? Should this allow leading/trailing spaces? Why is this only implemented for the Google side? is "a" a valid scheme? Seems this says it is. Can we test this? There is already an implementation in KURL.cpp of which this is just a copy (see my above comment). And I think the most intuitive behaviour for this function would be to validate exactly what the spec says at http://tools.ietf.org/html/rfc3986#section-3.1. As for testing, this function isn't exposed to the browser directly (it is used internally by KURL.cpp), but in my other patch (bug 54438) which causes navigator.registerProtocolHandler() to use this function to verify protocols passed to it, I add test cases that exercise this function directly.
(In reply to comment #7) > Should we rename this to isValidProtocolName() when exposing? Or even isValidSchemeName()? I think its current name is the most consistent - the current public interface for KURL uses 'protocol' by itself (protocolIs(), setProtocol(), etc). Also, it could be argued that in the context of a URL it is implied that we are referring to the name of a protocol.
> I think its current name is the most consistent - the current public interface for KURL uses 'protocol' by itself (protocolIs(), setProtocol(), etc). OK. > Also, it could be argued that in the context of a URL it is implied that we are referring to the name of a protocol. This I don't agree with. KURL knows a lot about semantics of URLs (even default ports!), so it's not always about names.
(In reply to comment #11) > > Also, it could be argued that in the context of a URL it is implied that we are referring to the name of a protocol. > > This I don't agree with. KURL knows a lot about semantics of URLs (even default ports!), so it's not always about names. True, that's a good point.
Comment on attachment 82714 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=82714&action=review > Source/WebCore/platform/KURLGoogle.cpp:136 > +bool isValidProtocol(const String& protocol) This is missing a comment from the KURL.cpp version. May as well make the copy-paste exact. Also, can you put a comment explaining that this is copy-pasted from there?
(In reply to comment #13) > (From update of attachment 82714 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=82714&action=review > > > Source/WebCore/platform/KURLGoogle.cpp:136 > > +bool isValidProtocol(const String& protocol) > > This is missing a comment from the KURL.cpp version. May as well make the copy-paste exact. Also, can you put a comment explaining that this is copy-pasted from there? Done.
Created attachment 84917 [details] Patch
dave_levin have been working to remove duplicated code in KURL.cpp and KURLGoogle.cpp. I noticed this comment: + // NOTE This is a copy of the function in KURL.cpp. Maybe we can move this function outside the ifdef so we don't have to copy/paste it?
(In reply to comment #16) > dave_levin have been working to remove duplicated code in KURL.cpp and KURLGoogle.cpp. I noticed this comment: > > + // NOTE This is a copy of the function in KURL.cpp. > > Maybe we can move this function outside the ifdef so we don't have to copy/paste it? Yes, please do (see the bottom of KURL.cpp). (It seems silly to duplicate code between these two files for things that are identical so I got rid of some. Let's not add more.)
Comment on attachment 84917 [details] Patch Per above comment.
(In reply to comment #17) > (In reply to comment #16) > > dave_levin have been working to remove duplicated code in KURL.cpp and KURLGoogle.cpp. I noticed this comment: > > > > + // NOTE This is a copy of the function in KURL.cpp. > > > > Maybe we can move this function outside the ifdef so we don't have to copy/paste it? > > Yes, please do (see the bottom of KURL.cpp). (It seems silly to duplicate code between these two files for things that are identical so I got rid of some. Let's not add more.) Right, of course. Done.
Created attachment 84991 [details] Patch
Comment on attachment 84991 [details] Patch Compile errors.
(In reply to comment #21) > (From update of attachment 84991 [details]) > Compile errors. Hm. This is made a bit more complicated by the fact that the duplicated versions are identical in code, but because they are in different files they get linked with different static inline methods (see isSchemeChar() and isSchemeFirstChar() in KURL.cpp and KURLGoogle.cpp). What would be the best way to move forward here?
(In reply to comment #22) > (In reply to comment #21) > > (From update of attachment 84991 [details] [details]) > > Compile errors. > > Hm. This is made a bit more complicated by the fact that the duplicated versions are identical in code, but because they are in different files they get linked with different static inline methods (see isSchemeChar() and isSchemeFirstChar() in KURL.cpp and KURLGoogle.cpp). > > What would be the best way to move forward here? If needed, you can do separate (duplicate) methods.
> If needed, you can do separate (duplicate) methods. Okay, I've revived the old patch as per our chat.
Comment on attachment 84917 [details] Patch So sad but it seems we need to do this. If you get a better idea, feel free to submit another patch. Something to consider is moving those inline functions to KURL.h Anyway r+ on this.
Comment on attachment 84917 [details] Patch Clearing flags on attachment: 84917 Committed r80566: <http://trac.webkit.org/changeset/80566>
All reviewed patches have been landed. Closing bug.