[webkit-reviews] review granted: [Bug 29651] [HTML5] Add a way for an application to register as a protocol handler. : [Attachment 42270] Proposed implementation of window.navigator.registerProtocolHandler and registerContentHandler.

[bugzilla-daemon at webkit.org]

Dmitry Titov <dimich at chromium.org> has granted B. Green <brg at chromium.org>'s
request for review:
Bug 29651: [HTML5] Add a way for an application to register as a protocol
handler.
https://bugs.webkit.org/show_bug.cgi?id=29651

Attachment 42270: Proposed implementation of
window.navigator.registerProtocolHandler and registerContentHandler.
https://bugs.webkit.org/attachment.cgi?id=42270&action=review

------- Additional Comments from Dmitry Titov <dimich at chromium.org>
> Index: WebCore/page/Navigator.cpp
> Index: LayoutTests/fast/dom/navigator-detached-no-crash.html
> +	 } catch(err) {
> +	   // navigator.registerXXX will throw on invalid input.
> +	   strings.push("navigator."+p+"() threw err "+err);

I'd change comment to explicitly indicate that exception is an expected result
here.


> Index: LayoutTests/fast/dom/registerContentHandler.html

> +    var succeeded = false;
> +    try {
> +	   window.navigator.registerContentHandler(protocol, "invalid protocol
%s", "title");
> +    } catch(e) {
> +	   if('SECURITY_ERR' == e.name) {
> +	       succeeded = true;
> +	   };
> +    };
> +
> +    if(succeeded == true) {
> +	   debug('Pass: Invalid protocol "' + protocol + '" threw SECURITY_ERR
exception.');
> +    } else {
> +	   debug('Fail: Invalid protocol "' + protocol + '" allowed.');
> +    };

I think by removal of a 'succeeded' the test could be smaller and more
readable.

> +if(succeeded == true) {
> +    debug('Pass: Valid call succeeded.');
> +} else {
> +    debug('Fail: Invalid call did not succeed.');

typo, should be "Fail: Valid call did not succeed."


Overall looks good. r=me.
I'll make the changes above on landing.