Bug 54594 – Expose isValidProtocol() in KURL.h.

• Component: Platform
• Status: RESOLVED FIXED

[James Kozianski]

Expose isValidProtocol() in KURL.h.

[James Kozianski]

Created attachment 82710 [details]
Patch

[James Kozianski]

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.

[Adam Barth]

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?

[James Kozianski]

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

[James Kozianski]

Created attachment 82714 [details]
Patch

[Adam Barth]

CCing URL folks

[Alexey Proskuryakov]

Should we rename this to isValidProtocolName() when exposing? Or even isValidSchemeName()?

[Eric Seidel (no email)]

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?

[James Kozianski]

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

[James Kozianski]

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

[Alexey Proskuryakov]

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

[James Kozianski]

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

[Ojan Vafai]

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?

[James Kozianski]

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

[James Kozianski]

Created attachment 84917 [details]
Patch

[Adam Barth]

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?

[David Levin]

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

[David Levin]

Comment on attachment 84917 [details]
Patch

Per above comment.

[James Kozianski]

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

[James Kozianski]

Created attachment 84991 [details]
Patch

[James Kozianski]

Comment on attachment 84991 [details]
Patch

Compile errors.

[James Kozianski]

(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?

[David Levin]

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

[James Kozianski]

> If needed, you can do separate (duplicate) methods.

Okay, I've revived the old patch as per our chat.

[David Levin]

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.

[WebKit Commit Bot]

Comment on attachment 84917 [details]
Patch

Clearing flags on attachment: 84917

Committed r80566: <http://trac.webkit.org/changeset/80566>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.