Call isValidProtocol() function first in order to remove duplicated protocol checking in NavigatorC…

Call isValidProtocol() function first in order to remove duplicated protocol checking in NavigatorContentUtils

isValidProtocol() can check if protocl contains colon. If we call isValidProtocol() first in verifyProtocolHandlerScheme(), we're able to remove unnecessary protocol checking logic.

TEST=navigatorcontentutils
BUG=none

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176894

[gyuyoung-inactive, 2014-06-23 13:38:23]

Add haraken to reviewer.

[haraken, 2014-06-23 13:47:17]

https://codereview.chromium.org/349193002/diff/1/Source/modules/navigatorcont...
File Source/modules/navigatorcontentutils/NavigatorContentUtils.cpp (left):

https://codereview.chromium.org/349193002/diff/1/Source/modules/navigatorcont...
Source/modules/navigatorcontentutils/NavigatorContentUtils.cpp:126:
exceptionState.throwDOMException(SyntaxError, "The scheme '" + scheme + "'
contains colon.");

After this CL, this exception is merged with the SyntaxError at line 113 (in the
new code). Is this intentional? Shall we add a test for this?

https://codereview.chromium.org/349193002/diff/1/Source/modules/navigatorcont...
File Source/modules/navigatorcontentutils/NavigatorContentUtils.cpp (right):

https://codereview.chromium.org/349193002/diff/1/Source/modules/navigatorcont...
Source/modules/navigatorcontentutils/NavigatorContentUtils.cpp:113:
exceptionState.throwDOMException(SyntaxError, "The scheme '" + scheme + "' is
not valid protocol");

Shouldn't this be a SecurityError instead of a DOMException? Before this CL,
this was treated as a SecurityError.

[gyuyoung-inactive, 2014-06-24 05:09:56]

https://codereview.chromium.org/349193002/diff/1/Source/modules/navigatorcont...
File Source/modules/navigatorcontentutils/NavigatorContentUtils.cpp (left):

https://codereview.chromium.org/349193002/diff/1/Source/modules/navigatorcont...
Source/modules/navigatorcontentutils/NavigatorContentUtils.cpp:126:
exceptionState.throwDOMException(SyntaxError, "The scheme '" + scheme + "'
contains colon.");
On 2014/06/23 13:47:16, haraken wrote:
> 
> After this CL, this exception is merged with the SyntaxError at line 113 (in
the
> new code). Is this intentional? Shall we add a test for this?

Yes, I already added a test for this in r175104. If you think I need to add more
test for this, please let me know.

https://src.chromium.org/viewvc/blink?view=rev&revision=175104

https://codereview.chromium.org/349193002/diff/1/Source/modules/navigatorcont...
File Source/modules/navigatorcontentutils/NavigatorContentUtils.cpp (right):

https://codereview.chromium.org/349193002/diff/1/Source/modules/navigatorcont...
Source/modules/navigatorcontentutils/NavigatorContentUtils.cpp:113:
exceptionState.throwDOMException(SyntaxError, "The scheme '" + scheme + "' is
not valid protocol");
On 2014/06/23 13:47:16, haraken wrote:
> 
> Shouldn't this be a SecurityError instead of a DOMException? Before this CL,
> this was treated as a SecurityError.

Yes, SecurityError looks proper in this exception. Fixed.

[haraken, 2014-06-24 05:12:29]

On 2014/06/24 05:09:56, gyuyoung wrote:
>
https://codereview.chromium.org/349193002/diff/1/Source/modules/navigatorcont...
> File Source/modules/navigatorcontentutils/NavigatorContentUtils.cpp (left):
> 
>
https://codereview.chromium.org/349193002/diff/1/Source/modules/navigatorcont...
> Source/modules/navigatorcontentutils/NavigatorContentUtils.cpp:126:
> exceptionState.throwDOMException(SyntaxError, "The scheme '" + scheme + "'
> contains colon.");
> On 2014/06/23 13:47:16, haraken wrote:
> > 
> > After this CL, this exception is merged with the SyntaxError at line 113 (in
> the
> > new code). Is this intentional? Shall we add a test for this?
> 
> Yes, I already added a test for this in r175104. If you think I need to add
more
> test for this, please let me know.
> 
> https://src.chromium.org/viewvc/blink?view=rev&revision=175104
> 
>
https://codereview.chromium.org/349193002/diff/1/Source/modules/navigatorcont...
> File Source/modules/navigatorcontentutils/NavigatorContentUtils.cpp (right):
> 
>
https://codereview.chromium.org/349193002/diff/1/Source/modules/navigatorcont...
> Source/modules/navigatorcontentutils/NavigatorContentUtils.cpp:113:
> exceptionState.throwDOMException(SyntaxError, "The scheme '" + scheme + "' is
> not valid protocol");
> On 2014/06/23 13:47:16, haraken wrote:
> > 
> > Shouldn't this be a SecurityError instead of a DOMException? Before this CL,
> > this was treated as a SecurityError.
> 
> Yes, SecurityError looks proper in this exception. Fixed.

Does the change of error types align with Firefox and the spec?

[gyuyoung-inactive, 2014-06-24 05:17:45]

On 2014/06/24 05:12:29, haraken wrote:
> On 2014/06/24 05:09:56, gyuyoung wrote:
> >
>
https://codereview.chromium.org/349193002/diff/1/Source/modules/navigatorcont...
> > File Source/modules/navigatorcontentutils/NavigatorContentUtils.cpp (left):
> > 
> >
>
https://codereview.chromium.org/349193002/diff/1/Source/modules/navigatorcont...
> > Source/modules/navigatorcontentutils/NavigatorContentUtils.cpp:126:
> > exceptionState.throwDOMException(SyntaxError, "The scheme '" + scheme + "'
> > contains colon.");
> > On 2014/06/23 13:47:16, haraken wrote:
> > > 
> > > After this CL, this exception is merged with the SyntaxError at line 113
(in
> > the
> > > new code). Is this intentional? Shall we add a test for this?
> > 
> > Yes, I already added a test for this in r175104. If you think I need to add
> more
> > test for this, please let me know.
> > 
> > https://src.chromium.org/viewvc/blink?view=rev&revision=175104
> > 
> >
>
https://codereview.chromium.org/349193002/diff/1/Source/modules/navigatorcont...
> > File Source/modules/navigatorcontentutils/NavigatorContentUtils.cpp (right):
> > 
> >
>
https://codereview.chromium.org/349193002/diff/1/Source/modules/navigatorcont...
> > Source/modules/navigatorcontentutils/NavigatorContentUtils.cpp:113:
> > exceptionState.throwDOMException(SyntaxError, "The scheme '" + scheme + "'
is
> > not valid protocol");
> > On 2014/06/23 13:47:16, haraken wrote:
> > > 
> > > Shouldn't this be a SecurityError instead of a DOMException? Before this
CL,
> > > this was treated as a SecurityError.
> > 
> > Yes, SecurityError looks proper in this exception. Fixed.
> 
> Does the change of error types align with Firefox and the spec?


According to the spec, if there is any problem in protocol, spec mentions
security error should happen.
http://www.whatwg.org/specs/web-apps/current-work/#custom-handlers

"Throws a SecurityError exception if the user agent blocks the registration
(this might happen if trying to register as a handler for "http", for
instance)."

"If the registerProtocolHandler() method is invoked with a scheme that is
neither a whitelisted scheme nor a scheme whose value starts with the substring
"web+" and otherwise contains only lowercase ASCII letters, and whose length is
at least five characters (including the "web+" prefix), the user agent must
throw a SecurityError exception."


And, let me check firefox behavior soon.

[gyuyoung-inactive, 2014-06-24 08:07:24]

On 2014/06/24 05:17:45, gyuyoung wrote:

> And, let me check firefox behavior soon.

When running below command on Firefox's web console tool, Firefox generates
"permission denied exception". It looks Firefox prohibits to call the
"registerProtocolHandler()" from non-permitted user.

  navigator.registerProtocolHandler("mailto",
                                   
"https://mail.google.com/mail/?extsrc=mailto&url=%s",
                                    "Gmail");


AFAIK, protocols supported by Firefox are already registered as built-in. User
only can enable/disable registered protocol via "about:config" page. To verify
this behavior, I may need to dig/modify Firefox's source code. Do you think we
should verify what error Firefox generates ?

[haraken, 2014-06-24 08:14:46]

On 2014/06/24 08:07:24, gyuyoung wrote:
> On 2014/06/24 05:17:45, gyuyoung wrote:
> 
> > And, let me check firefox behavior soon.
> 
> When running below command on Firefox's web console tool, Firefox generates
> "permission denied exception". It looks Firefox prohibits to call the
> "registerProtocolHandler()" from non-permitted user.
> 
>   navigator.registerProtocolHandler("mailto",
>                                    
> "https://mail.google.com/mail/?extsrc=mailto&url=%s",
>                                     "Gmail");
> 
> 
> AFAIK, protocols supported by Firefox are already registered as built-in. User
> only can enable/disable registered protocol via "about:config" page. To verify
> this behavior, I may need to dig/modify Firefox's source code. Do you think we
> should verify what error Firefox generates ?

You don't need to investigate the code :) What matters is what behavior is
exposed to users.

Let me make sure if I'm understanding correctly:

- The spec says that we should have a way to register a new protocol handler.
- The spec says that we should raise a SecurityError for an invalid protocol.
- In Firefox, there is no way to register a new protocol handler, and thus no
way to check the error type.
- IE doesn't support protocol handlers.

Am I understanding correctly?

[gyuyoung-inactive, 2014-06-24 08:20:40]

On 2014/06/24 08:14:46, haraken wrote:
> On 2014/06/24 08:07:24, gyuyoung wrote:
> > On 2014/06/24 05:17:45, gyuyoung wrote:
> > 
> > > And, let me check firefox behavior soon.
> > 
> > When running below command on Firefox's web console tool, Firefox generates
> > "permission denied exception". It looks Firefox prohibits to call the
> > "registerProtocolHandler()" from non-permitted user.
> > 
> >   navigator.registerProtocolHandler("mailto",
> >                                    
> > "https://mail.google.com/mail/?extsrc=mailto&url=%s",
> >                                     "Gmail");
> > 
> > 
> > AFAIK, protocols supported by Firefox are already registered as built-in.
User
> > only can enable/disable registered protocol via "about:config" page. To
verify
> > this behavior, I may need to dig/modify Firefox's source code. Do you think
we
> > should verify what error Firefox generates ?
> 
> You don't need to investigate the code :) What matters is what behavior is
> exposed to users.
> 
> Let me make sure if I'm understanding correctly:
> 
> - The spec says that we should have a way to register a new protocol handler.
> - The spec says that we should raise a SecurityError for an invalid protocol.
> - In Firefox, there is no way to register a new protocol handler, and thus no
> way to check the error type.
> - IE doesn't support protocol handlers.
> 
> Am I understanding correctly?

Although I may need to check if there is other methods to use
"registerProtocolHandler()" in Firefox further, AFAIK, you're understanding
current status.

[gyuyoung-inactive, 2014-06-24 22:15:54]

On 2014/06/24 08:20:40, gyuyoung wrote:
> On 2014/06/24 08:14:46, haraken wrote:
> > On 2014/06/24 08:07:24, gyuyoung wrote:
> > > On 2014/06/24 05:17:45, gyuyoung wrote:
> > > 
> > > > And, let me check firefox behavior soon.
> > > 
> > > When running below command on Firefox's web console tool, Firefox
generates
> > > "permission denied exception". It looks Firefox prohibits to call the
> > > "registerProtocolHandler()" from non-permitted user.
> > > 
> > >   navigator.registerProtocolHandler("mailto",
> > >                                    
> > > "https://mail.google.com/mail/?extsrc=mailto&url=%s",
> > >                                     "Gmail");
> > > 
> > > 
> > > AFAIK, protocols supported by Firefox are already registered as built-in.
> User
> > > only can enable/disable registered protocol via "about:config" page. To
> verify
> > > this behavior, I may need to dig/modify Firefox's source code. Do you
think
> we
> > > should verify what error Firefox generates ?
> > 
> > You don't need to investigate the code :) What matters is what behavior is
> > exposed to users.
> > 
> > Let me make sure if I'm understanding correctly:
> > 
> > - The spec says that we should have a way to register a new protocol
handler.
> > - The spec says that we should raise a SecurityError for an invalid
protocol.
> > - In Firefox, there is no way to register a new protocol handler, and thus
no
> > way to check the error type.
> > - IE doesn't support protocol handlers.
> > 
> > Am I understanding correctly?
> 
> Although I may need to check if there is other methods to use
> "registerProtocolHandler()" in Firefox further, AFAIK, you're understanding
> current status.

Kentaro, any comment ?

[haraken, 2014-06-24 23:18:42]

On 2014/06/24 22:15:54, gyuyoung wrote:
> On 2014/06/24 08:20:40, gyuyoung wrote:
> > On 2014/06/24 08:14:46, haraken wrote:
> > > On 2014/06/24 08:07:24, gyuyoung wrote:
> > > > On 2014/06/24 05:17:45, gyuyoung wrote:
> > > > 
> > > > > And, let me check firefox behavior soon.
> > > > 
> > > > When running below command on Firefox's web console tool, Firefox
> generates
> > > > "permission denied exception". It looks Firefox prohibits to call the
> > > > "registerProtocolHandler()" from non-permitted user.
> > > > 
> > > >   navigator.registerProtocolHandler("mailto",
> > > >                                    
> > > > "https://mail.google.com/mail/?extsrc=mailto&url=%s",
> > > >                                     "Gmail");
> > > > 
> > > > 
> > > > AFAIK, protocols supported by Firefox are already registered as
built-in.
> > User
> > > > only can enable/disable registered protocol via "about:config" page. To
> > verify
> > > > this behavior, I may need to dig/modify Firefox's source code. Do you
> think
> > we
> > > > should verify what error Firefox generates ?
> > > 
> > > You don't need to investigate the code :) What matters is what behavior is
> > > exposed to users.
> > > 
> > > Let me make sure if I'm understanding correctly:
> > > 
> > > - The spec says that we should have a way to register a new protocol
> handler.
> > > - The spec says that we should raise a SecurityError for an invalid
> protocol.
> > > - In Firefox, there is no way to register a new protocol handler, and thus
> no
> > > way to check the error type.
> > > - IE doesn't support protocol handlers.
> > > 
> > > Am I understanding correctly?
> > 
> > Although I may need to check if there is other methods to use
> > "registerProtocolHandler()" in Firefox further, AFAIK, you're understanding
> > current status.
> 
> Kentaro, any comment ?

If the description I wrote in #7 is correct, LGTM.

[gyuyoung-inactive, 2014-06-24 23:32:07]

The CQ bit was checked by gyuyoung.kim@samsung.com

[commit-bot: I haz the power, 2014-06-24 23:32:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/gyuyoung.kim@samsung.com/349193002/20001

[commit-bot: I haz the power, 2014-06-25 00:35:53]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/13440)

[commit-bot: I haz the power, 2014-06-25 01:26:58]

Change committed as 176894