Bug 52609 – Add navigator.registerProtocolHandler behind a flag.

• Component: New Bugs
• Status: RESOLVED FIXED

[James Kozianski]

Add navigator.registerProtocolHandler behind a flag.

[James Kozianski]

Created attachment 79241 [details]
Patch

[Adam Barth]

Comment on attachment 79241 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=79241&action=review

> Source/WebCore/page/Navigator.cpp:190
> +    newURL.remove(index, sizeof(token) / sizeof(token[0]));

sizeof(token) - 1, right?

[David Levin]

Comment on attachment 79241 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=79241&action=review

I think you'll also need to put it behind a runtime flag for chromium but that can (and likely should be saved) for another patch (to keep this one smaller).

> Source/WebCore/ChangeLog:12
> +        behind a flag so as not to break JS feature detection.

Hurray!

> Source/WebCore/ChangeLog:14
> +        No new tests. (OOPS!)

This should either point out the test or explain why none is possible.

> Source/WebCore/page/ChromeClient.h:139
> +        virtual void registerProtocolHandler(const String&, const String&, const String&, const String&) { }

I think this should be = 0 and be behind the if ENABLE (and have the parameter names filled in).

> Source/WebCore/page/Navigator.cpp:28
> +#include "ExceptionCode.h"

Out of order.

> Source/WebCore/page/Navigator.cpp:178
> +    // The specification requires that it is a SYNTAX_ERR if the the "%s" token

"the the"

>> Source/WebCore/page/Navigator.cpp:190
>> +    newURL.remove(index, sizeof(token) / sizeof(token[0]));
> 
> sizeof(token) - 1, right?

Also use WTF_ARRAY_LENGTH (if possible). (It may not be possible due to the variable begin a static const local.)

> Source/WebCore/page/Navigator.h:66
> +        void registerProtocolHandler(const String& scheme, const String& url, const String& title, ExceptionCode&);

This appears to be indented too far.

> Source/WebKit/mac/Configurations/FeatureDefines.xcconfig:130
> +FEATURE_DEFINES = $(ENABLE_LINK_PREFETCH) $(ENABLE_ACCELERATED_2D_CANVAS) $(ENABLE_3D_CANVAS) $(ENABLE_3D_RENDERING) $(ENABLE_BLOB) $(ENABLE_CHANNEL_MESSAGING) $(ENABLE_CLIENT_BASED_GEOLOCATION) $(ENABLE_DATABASE) $(ENABLE_DATAGRID) $(ENABLE_DATALIST) $(ENABLE_DEVICE_ORIENTATION) $(ENABLE_DIRECTORY_UPLOAD) $(ENABLE_DOM_STORAGE) $(ENABLE_EVENTSOURCE) $(ENABLE_FILTERS) $(ENABLE_FILE_SYSTEM) $(ENABLE_FULLSCREEN_API) $(ENABLE_GEOLOCATION) $(ENABLE_ICONDATABASE) $(ENABLE_IMAGE_RESIZER) $(ENABLE_INDEXED_DATABASE) $(ENABLE_INPUT_SPEECH) $(ENABLE_JAVASCRIPT_DEBUGGER) $(ENABLE_MATHML) $(ENABLE_METER_TAG) $(ENABLE_NOTIFICATIONS) $(ENABLE_OFFLINE_WEB_APPLICATIONS) $(ENABLE_PROGRESS_TAG) $(ENABLE_REGISTER_PROTOCOL_HANDLER) $(ENABLE_SHARED_WORKERS) $(ENABLE_SVG) $(ENABLE_SVG_ANIMATION) $(ENABLE_SVG_AS_IMAGE) $(ENABLE_SVG_DOM_OBJC_BINDINGS) $(ENABLE_SVG_FONTS) $(ENABLE_SVG_FOREIGN_OBJECT) $(ENABLE_SVG_USE) $(ENABLE_VIDEO) $(ENABLE_WEB_AUDIO) $(ENABLE_WEB_SOCKETS) $(ENABLE_WEB_TIMING) $(ENABLE_WML) $(ENABLE_WORKERS) $(ENABLE_XHTMLMP) $(ENABLE_XPATH) $(ENABLE_XSLT);

It looks like something went wrong here. This diff doesn't look right.

[James Kozianski]

Comment on attachment 79241 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=79241&action=review

>> Source/WebCore/ChangeLog:14
>> +        No new tests. (OOPS!)
> 
> This should either point out the test or explain why none is possible.

Done. (Is something behind behind a compile flag a valid reason for not having a layout test?)

>> Source/WebCore/page/ChromeClient.h:139
>> +        virtual void registerProtocolHandler(const String&, const String&, const String&, const String&) { }
> 
> I think this should be = 0 and be behind the if ENABLE (and have the parameter names filled in).

Done (I added this to all the implementors of ChromeClient I could find, too).

>> Source/WebCore/page/Navigator.cpp:28
>> +#include "ExceptionCode.h"
> 
> Out of order.

Fixed.

>> Source/WebCore/page/Navigator.cpp:178
>> +    // The specification requires that it is a SYNTAX_ERR if the the "%s" token
> 
> "the the"

Fixed.

>>> Source/WebCore/page/Navigator.cpp:190
>>> +    newURL.remove(index, sizeof(token) / sizeof(token[0]));
>> 
>> sizeof(token) - 1, right?
> 
> Also use WTF_ARRAY_LENGTH (if possible). (It may not be possible due to the variable begin a static const local.)

Yep, you're right, Adam.

And WTF_ARRAY_LENGTH does work here, so I've changed it to WTF_ARRAY_LENGTH() - 1.

>> Source/WebCore/page/Navigator.h:66
>> +        void registerProtocolHandler(const String& scheme, const String& url, const String& title, ExceptionCode&);
> 
> This appears to be indented too far.

Fixed.

[James Kozianski]

Created attachment 79538 [details]
Patch

[WebKit Review Bot]

Attachment 79538 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7526239

[James Kozianski]

(In reply to comment #3)
> (From update of attachment 79241 [details])
> > Source/WebKit/mac/Configurations/FeatureDefines.xcconfig:130
> > +FEATURE_DEFINES = $(ENABLE_LINK_PREFETCH) $(ENABLE_ACCELERATED_2D_CANVAS) $(ENABLE_3D_CANVAS) $(ENABLE_3D_RENDERING) $(ENABLE_BLOB) $(ENABLE_CHANNEL_MESSAGING) $(ENABLE_CLIENT_BASED_GEOLOCATION) $(ENABLE_DATABASE) $(ENABLE_DATAGRID) $(ENABLE_DATALIST) $(ENABLE_DEVICE_ORIENTATION) $(ENABLE_DIRECTORY_UPLOAD) $(ENABLE_DOM_STORAGE) $(ENABLE_EVENTSOURCE) $(ENABLE_FILTERS) $(ENABLE_FILE_SYSTEM) $(ENABLE_FULLSCREEN_API) $(ENABLE_GEOLOCATION) $(ENABLE_ICONDATABASE) $(ENABLE_IMAGE_RESIZER) $(ENABLE_INDEXED_DATABASE) $(ENABLE_INPUT_SPEECH) $(ENABLE_JAVASCRIPT_DEBUGGER) $(ENABLE_MATHML) $(ENABLE_METER_TAG) $(ENABLE_NOTIFICATIONS) $(ENABLE_OFFLINE_WEB_APPLICATIONS) $(ENABLE_PROGRESS_TAG) $(ENABLE_REGISTER_PROTOCOL_HANDLER) $(ENABLE_SHARED_WORKERS) $(ENABLE_SVG) $(ENABLE_SVG_ANIMATION) $(ENABLE_SVG_AS_IMAGE) $(ENABLE_SVG_DOM_OBJC_BINDINGS) $(ENABLE_SVG_FONTS) $(ENABLE_SVG_FOREIGN_OBJECT) $(ENABLE_SVG_USE) $(ENABLE_VIDEO) $(ENABLE_WEB_AUDIO) $(ENABLE_WEB_SOCKETS) $(ENABLE_WEB_TIMING) $(ENABLE_WML) $(ENABLE_WORKERS) $(ENABLE_XHTMLMP) $(ENABLE_XPATH) $(ENABLE_XSLT);
> 
> It looks like something went wrong here. This diff doesn't look right.

Is there anything specific wrong with it you can point out? To my eye it looks similar to other diffs for this line (such as those in bug 49272).

[David Levin]

(In reply to comment #7)
> (In reply to comment #3)
> > (From update of attachment 79241 [details] [details])
> > > Source/WebKit/mac/Configurations/FeatureDefines.xcconfig:130
> > It looks like something went wrong here. This diff doesn't look right.
> 
> Is there anything specific wrong with it you can point out? To my eye it looks similar to other diffs for this line (such as those in bug 49272).

It looks fine in the latest patch.


(In reply to comment #4)
> (From update of attachment 79241 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=79241&action=review
> 
> >> Source/WebCore/ChangeLog:14
> >> +        No new tests. (OOPS!)
> > 
> > This should either point out the test or explain why none is possible.
> 
> Done. (Is something behind behind a compile flag a valid reason for not having a layout test?)

Not being testable is a valid reason but this is testable. It is exposed to js code.

I would add tests. You can add a few to test error conditions.  You can add the test to Skipped files for platforms that don't have it enabled.

> 
> >> Source/WebCore/page/ChromeClient.h:139
> >> +        virtual void registerProtocolHandler(const String&, const String&, const String&, const String&) { }
> > 
> > I think this should be = 0 and be behind the if ENABLE (and have the parameter names filled in).
> 
> Done (I added this to all the implementors of ChromeClient I could find, too).

I'd avoid that actually. Since this is behind a flag, if a platform enables that flag, then the compile error will let them know exactly what they need to implement to make it work.

[David Levin]

Comment on attachment 79538 [details]
Patch

r- for build breaks (plus this should have some layout tests).

[James Kozianski]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #3)
> > > (From update of attachment 79241 [details] [details] [details])
> > > > Source/WebKit/mac/Configurations/FeatureDefines.xcconfig:130
> > > It looks like something went wrong here. This diff doesn't look right.
> > 
> > Is there anything specific wrong with it you can point out? To my eye it looks similar to other diffs for this line (such as those in bug 49272).
> 
> It looks fine in the latest patch.
> 
> 
> (In reply to comment #4)
> > (From update of attachment 79241 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=79241&action=review
> > 
> > >> Source/WebCore/ChangeLog:14
> > >> +        No new tests. (OOPS!)
> > > 
> > > This should either point out the test or explain why none is possible.
> > 
> > Done. (Is something behind behind a compile flag a valid reason for not having a layout test?)
> 
> Not being testable is a valid reason but this is testable. It is exposed to js code.
> 
> I would add tests. You can add a few to test error conditions.  You can add the test to Skipped files for platforms that don't have it enabled.

Cool. Done.

> 
> > 
> > >> Source/WebCore/page/ChromeClient.h:139
> > >> +        virtual void registerProtocolHandler(const String&, const String&, const String&, const String&) { }
> > > 
> > > I think this should be = 0 and be behind the if ENABLE (and have the parameter names filled in).
> > 
> > Done (I added this to all the implementors of ChromeClient I could find, too).
> 
> I'd avoid that actually. Since this is behind a flag, if a platform enables that flag, then the compile error will let them know exactly what they need to implement to make it work.

Yeah, good point.

[James Kozianski]

Created attachment 79883 [details]
Patch

[David Levin]

Comment on attachment 79883 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=79883&action=review

Seems fine. I noted a few nits below.

The biggest problem is that this patch doesn't seem to apply (see all of the purple ews statuses).

> Source/WebCore/loader/EmptyClients.h:148
> +        virtual void registerProtocolHandler(const String&, const String&, const String&, const String&) { }

indented too far.

> Source/WebKit/chromium/public/WebViewClient.h:327
> +      const WebString& baseUrl, const WebString& url, const WebString& title);

Align with ( from previous line.

[James Kozianski]

Created attachment 79983 [details]
Patch

[James Kozianski]

(In reply to comment #12)
> (From update of attachment 79883 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=79883&action=review
> 
> Seems fine. I noted a few nits below.
> 
> The biggest problem is that this patch doesn't seem to apply (see all of the purple ews statuses).

Hm, I think I needed to sync this patch. Hopefully it'll pass now.

> 
> > Source/WebCore/loader/EmptyClients.h:148
> > +        virtual void registerProtocolHandler(const String&, const String&, const String&, const String&) { }
> 
> indented too far.

Done.

> 
> > Source/WebKit/chromium/public/WebViewClient.h:327
> > +      const WebString& baseUrl, const WebString& url, const WebString& title);
> 
> Align with ( from previous line.

Done.

[WebKit Review Bot]

Attachment 79983 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7645054

[James Kozianski]

Comment on attachment 79983 [details]
Patch

Investigating gtk and efl breakages.

[Early Warning System Bot]

Attachment 79983 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7528307

[WebKit Review Bot]

Attachment 79983 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7598327

[James Kozianski]

Created attachment 80011 [details]
Patch

[James Kozianski]

Created attachment 80022 [details]
Patch

[James Kozianski]

Hm, I can't figure out why it won't apply.. Are there any common causes of that apart from being out of sync?

[James Kozianski]

It seems to be choking on the vsprops files. The patch that gets uploaded has unix-style line endings throughout, but the vsprops files have dos-style line endings.

My local copy of the files have dos-style line endings, so maybe I've got some weird git flag that is messing things up. I'll investigate more later.

[James Kozianski]

Created attachment 80408 [details]
Patch

[James Kozianski]

It turns out it wasn't applying just because the test_expectations.txt and Skipped files were changing too quickly and getting out of sync. I tried to svn-apply it myself but forgot the --force option, so I thought it was failing from CRLF errors :-)

It should be all good now.

[WebKit Review Bot]

Attachment 80408 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7582410

[James Kozianski]

Created attachment 80604 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 80604 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=80604&action=review

Mostly style nits and the bit about xcconfig synchronization.

> LayoutTests/fast/dom/registerProtocolHandler.html:1
> +<html>

Typically test file names are dash-separated.html

> LayoutTests/fast/dom/registerProtocolHandler.html:8
> + }

Indentation oopsie.

> LayoutTests/fast/dom/registerProtocolHandler.html:10
> +function debug(str) {

Function braces start on new line (here and elsewhere).

> LayoutTests/fast/dom/registerProtocolHandler.html:16
> +if(window.navigator.registerProtocolHandler) {

Don't need extra braces for single-line statements (here and elsewhere).

> Source/WebKit/mac/Configurations/FeatureDefines.xcconfig:126
> +FEATURE_DEFINES = $(ENABLE_LINK_PREFETCH) $(ENABLE_ACCELERATED_2D_CANVAS) $(ENABLE_WEBGL) $(ENABLE_3D_RENDERING) $(ENABLE_BLOB) $(ENABLE_CHANNEL_MESSAGING) $(ENABLE_CLIENT_BASED_GEOLOCATION) $(ENABLE_DATABASE) $(ENABLE_DATAGRID) $(ENABLE_DATALIST) $(ENABLE_DEVICE_ORIENTATION) $(ENABLE_DIRECTORY_UPLOAD) $(ENABLE_DOM_STORAGE) $(ENABLE_EVENTSOURCE) $(ENABLE_FILTERS) $(ENABLE_FILE_SYSTEM) $(ENABLE_FULLSCREEN_API) $(ENABLE_GEOLOCATION) $(ENABLE_ICONDATABASE) $(ENABLE_IMAGE_RESIZER) $(ENABLE_INDEXED_DATABASE) $(ENABLE_INPUT_SPEECH) $(ENABLE_JAVASCRIPT_DEBUGGER) $(ENABLE_MATHML) $(ENABLE_METER_TAG) $(ENABLE_NOTIFICATIONS) $(ENABLE_OFFLINE_WEB_APPLICATIONS) $(ENABLE_PROGRESS_TAG) $(ENABLE_REGISTER_PROTOCOL_HANDLER) $(ENABLE_SHARED_WORKERS) $(ENABLE_SVG) $(ENABLE_SVG_ANIMATION) $(ENABLE_SVG_AS_IMAGE) $(ENABLE_SVG_DOM_OBJC_BINDINGS) $(ENABLE_SVG_FONTS) $(ENABLE_SVG_FOREIGN_OBJECT) $(ENABLE_SVG_USE) $(ENABLE_VIDEO) $(ENABLE_WEB_AUDIO) $(ENABLE_WEB_SOCKETS) $(ENABLE_WEB_TIMING) $(ENABLE_WML) $(ENABLE_WORKERS) $(ENABLE_XHTMLMP) $(ENABLE_XPATH) $(ENABLE_XSLT);

See the comment at the top of the file about keeping this value in sync with all other FeatureDefines.xcconfig files.

[Ojan Vafai]

View in context: https://bugs.webkit.org/attachment.cgi?id=80604&action=review

Few more nits in addition to dimitri's.

> LayoutTests/fast/dom/registerProtocolHandler.html:22
> +// Test that the API regects secured protocols.

s/regects/rejects. also, this comment is kind of useless. it's pretty clear from the variable name what's going on.

> LayoutTests/fast/dom/registerProtocolHandler.html:31
> +        if('SECURITY_ERR' == e.name) {
> +            succeeded = true;
> +        };

no brackets on one-line ifs. and no semicolon after if-statement curly braces. also, you could just write this as:
succeeded = 'SECURITY_ERR' == e.name;

> LayoutTests/fast/dom/registerProtocolHandler.html:38
> +    if(succeeded == true) {
> +        debug('Pass: Invalid protocol "' + protocol + '" threw SECURITY_ERR exception.');
> +    } else {
> +        debug('Fail: Invalid protocol "' + protocol + '" allowed.');
> +    };

nit: webkit style is to leave out the curly braces. also, there shouldn't be a semicolon on line  38.

> LayoutTests/fast/dom/registerProtocolHandler.html:41
> +// Test that the API correctly parses the url for '%s'

useless comment. also, doesn't actually match the urls it's checking.

> LayoutTests/fast/dom/registerProtocolHandler.html:50
> +        if('SYNTAX_ERR' == e.name) {
> +            succeeded = true;
> +        };

ditto: above

> LayoutTests/fast/dom/registerProtocolHandler.html:57
> +    if(succeeded == true) {
> +        debug('Pass: Invalid url "' + url + '" threw SYNTAX_ERR exception.');
> +    } else {
> +        debug('Fail: Invalid url "' + url + '" allowed.');
> +    };

ditto above

> LayoutTests/fast/dom/registerProtocolHandler.html:72
> +if(succeeded == true) {
> +  debug('Pass: Valid call succeeded.');
> +} else {
> +  debug('Fail: Invalid call did not succeed.');
> +};

ditto re: braces

> LayoutTests/platform/chromium/test_expectations.txt:3044
> +BUGCR11359 : fast/dom/registerProtocolHandler.html = TEXT

Standard webkit practice is to just check in the failing result rather than to skip it on every platform.

[David Levin]

fwiw, from what I've seen the standard coding guidelines are not enforced for layout tests.

Indeed, it has been argued (by ap@ to my memory) that diversity in coding styles potentially allows for better testing to ensure that there aren't weird parse errors that would be overlooked with such uniformity.

[Eric Seidel (no email)]

Parsing tests belong as parsing tests.  Not as an excuse for code style diversity.

That said, I haven't looked at this patch at all. :)

[Ojan Vafai]

They don't need to be enforced at all cost, but where possible it makes sense to me to enforce them as we would with any other code. Using a shared style in layout tests has all the same benefits it does for non-test code. This is probably a topic for webkit-dev@ though.

[David Levin]

(In reply to comment #31)
> They don't need to be enforced at all cost, but where possible it makes sense to me to enforce them as we would with any other code. Using a shared style in layout tests has all the same benefits it does for non-test code. 

(In reply to comment #30)
> Parsing tests belong as parsing tests.  Not as an excuse for code style diversity.

Interesting. I explicitly have avoided enforcing style on layout tests due to this. I should probably change that.

[Eric Seidel (no email)]

(In reply to comment #32)
> (In reply to comment #31)
> > They don't need to be enforced at all cost, but where possible it makes sense to me to enforce them as we would with any other code. Using a shared style in layout tests has all the same benefits it does for non-test code. 
> 
> (In reply to comment #30)
> > Parsing tests belong as parsing tests.  Not as an excuse for code style diversity.
> 
> Interesting. I explicitly have avoided enforcing style on layout tests due to this. I should probably change that.

I don't really know what our javascript/layout-test style looks like.  Seems we should define one if we want to enforce it. :)  Things like { on the same line as an if, and 4-space indent, and descriptive variable names seem common throughout all languages that WebKit uses.  But AFAIK we don't have any explicit style guides for layout tests/html/js yet (but probably should)!

[James Kozianski]

(In reply to comment #27)
> (From update of attachment 80604 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80604&action=review
> 
> Mostly style nits and the bit about xcconfig synchronization.
> 
> > LayoutTests/fast/dom/registerProtocolHandler.html:1
> > +<html>
> 
> Typically test file names are dash-separated.html

Fixed.

> 
> > LayoutTests/fast/dom/registerProtocolHandler.html:8
> > + }
> 
> Indentation oopsie.

Fixed.

> 
> > LayoutTests/fast/dom/registerProtocolHandler.html:10
> > +function debug(str) {
> 
> Function braces start on new line (here and elsewhere).

Fixed.

> 
> > LayoutTests/fast/dom/registerProtocolHandler.html:16
> > +if(window.navigator.registerProtocolHandler) {
> 
> Don't need extra braces for single-line statements (here and elsewhere).

Fixed.

> 
> > Source/WebKit/mac/Configurations/FeatureDefines.xcconfig:126
> > +FEATURE_DEFINES = $(ENABLE_LINK_PREFETCH) $(ENABLE_ACCELERATED_2D_CANVAS) $(ENABLE_WEBGL) $(ENABLE_3D_RENDERING) $(ENABLE_BLOB) $(ENABLE_CHANNEL_MESSAGING) $(ENABLE_CLIENT_BASED_GEOLOCATION) $(ENABLE_DATABASE) $(ENABLE_DATAGRID) $(ENABLE_DATALIST) $(ENABLE_DEVICE_ORIENTATION) $(ENABLE_DIRECTORY_UPLOAD) $(ENABLE_DOM_STORAGE) $(ENABLE_EVENTSOURCE) $(ENABLE_FILTERS) $(ENABLE_FILE_SYSTEM) $(ENABLE_FULLSCREEN_API) $(ENABLE_GEOLOCATION) $(ENABLE_ICONDATABASE) $(ENABLE_IMAGE_RESIZER) $(ENABLE_INDEXED_DATABASE) $(ENABLE_INPUT_SPEECH) $(ENABLE_JAVASCRIPT_DEBUGGER) $(ENABLE_MATHML) $(ENABLE_METER_TAG) $(ENABLE_NOTIFICATIONS) $(ENABLE_OFFLINE_WEB_APPLICATIONS) $(ENABLE_PROGRESS_TAG) $(ENABLE_REGISTER_PROTOCOL_HANDLER) $(ENABLE_SHARED_WORKERS) $(ENABLE_SVG) $(ENABLE_SVG_ANIMATION) $(ENABLE_SVG_AS_IMAGE) $(ENABLE_SVG_DOM_OBJC_BINDINGS) $(ENABLE_SVG_FONTS) $(ENABLE_SVG_FOREIGN_OBJECT) $(ENABLE_SVG_USE) $(ENABLE_VIDEO) $(ENABLE_WEB_AUDIO) $(ENABLE_WEB_SOCKETS) $(ENABLE_WEB_TIMING) $(ENABLE_WML) $(ENABLE_WORKERS) $(ENABLE_XHTMLMP) $(ENABLE_XPATH) $(ENABLE_XSLT);
> 
> See the comment at the top of the file about keeping this value in sync with all other FeatureDefines.xcconfig files.

Yep, I think I've amended all the required files (all FeatureDefines.xccode, build-webkit, and FeatureDefines.vsprops and FeatureDefinesCairo.vsprops). Did you mention this because you saw that I missed something?

[James Kozianski]

(In reply to comment #28)
> View in context: https://bugs.webkit.org/attachment.cgi?id=80604&action=review
> 
> Few more nits in addition to dimitri's.
> 
> > LayoutTests/fast/dom/registerProtocolHandler.html:22
> > +// Test that the API regects secured protocols.
> 
> s/regects/rejects. also, this comment is kind of useless. it's pretty clear from the variable name what's going on.

Done.

> 
> > LayoutTests/fast/dom/registerProtocolHandler.html:31
> > +        if('SECURITY_ERR' == e.name) {
> > +            succeeded = true;
> > +        };
> 
> no brackets on one-line ifs. and no semicolon after if-statement curly braces. also, you could just write this as:
> succeeded = 'SECURITY_ERR' == e.name;

Done.

> 
> > LayoutTests/fast/dom/registerProtocolHandler.html:38
> > +    if(succeeded == true) {
> > +        debug('Pass: Invalid protocol "' + protocol + '" threw SECURITY_ERR exception.');
> > +    } else {
> > +        debug('Fail: Invalid protocol "' + protocol + '" allowed.');
> > +    };
> 
> nit: webkit style is to leave out the curly braces. also, there shouldn't be a semicolon on line  38.

Done.

> 
> > LayoutTests/fast/dom/registerProtocolHandler.html:41
> > +// Test that the API correctly parses the url for '%s'
> 
> useless comment. also, doesn't actually match the urls it's checking.

Removed.

> 
> > LayoutTests/fast/dom/registerProtocolHandler.html:50
> > +        if('SYNTAX_ERR' == e.name) {
> > +            succeeded = true;
> > +        };
> 
> ditto: above

Done.

> 
> > LayoutTests/fast/dom/registerProtocolHandler.html:57
> > +    if(succeeded == true) {
> > +        debug('Pass: Invalid url "' + url + '" threw SYNTAX_ERR exception.');
> > +    } else {
> > +        debug('Fail: Invalid url "' + url + '" allowed.');
> > +    };
> 
> ditto above

Done.

> 
> > LayoutTests/fast/dom/registerProtocolHandler.html:72
> > +if(succeeded == true) {
> > +  debug('Pass: Valid call succeeded.');
> > +} else {
> > +  debug('Fail: Invalid call did not succeed.');
> > +};
> 
> ditto re: braces

Done.

> 
> > LayoutTests/platform/chromium/test_expectations.txt:3044
> > +BUGCR11359 : fast/dom/registerProtocolHandler.html = TEXT
> 
> Standard webkit practice is to just check in the failing result rather than to skip it on every platform.

Done.

[Dimitri Glazkov (Google)]

(In reply to comment #34)
> (In reply to comment #27)
> > (From update of attachment 80604 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=80604&action=review
> > 
> > Mostly style nits and the bit about xcconfig synchronization.
> > 
> > > LayoutTests/fast/dom/registerProtocolHandler.html:1
> > > +<html>
> > 
> > Typically test file names are dash-separated.html
> 
> Fixed.
> 
> > 
> > > LayoutTests/fast/dom/registerProtocolHandler.html:8
> > > + }
> > 
> > Indentation oopsie.
> 
> Fixed.
> 
> > 
> > > LayoutTests/fast/dom/registerProtocolHandler.html:10
> > > +function debug(str) {
> > 
> > Function braces start on new line (here and elsewhere).
> 
> Fixed.
> 
> > 
> > > LayoutTests/fast/dom/registerProtocolHandler.html:16
> > > +if(window.navigator.registerProtocolHandler) {
> > 
> > Don't need extra braces for single-line statements (here and elsewhere).
> 
> Fixed.
> 
> > 
> > > Source/WebKit/mac/Configurations/FeatureDefines.xcconfig:126
> > > +FEATURE_DEFINES = $(ENABLE_LINK_PREFETCH) $(ENABLE_ACCELERATED_2D_CANVAS) $(ENABLE_WEBGL) $(ENABLE_3D_RENDERING) $(ENABLE_BLOB) $(ENABLE_CHANNEL_MESSAGING) $(ENABLE_CLIENT_BASED_GEOLOCATION) $(ENABLE_DATABASE) $(ENABLE_DATAGRID) $(ENABLE_DATALIST) $(ENABLE_DEVICE_ORIENTATION) $(ENABLE_DIRECTORY_UPLOAD) $(ENABLE_DOM_STORAGE) $(ENABLE_EVENTSOURCE) $(ENABLE_FILTERS) $(ENABLE_FILE_SYSTEM) $(ENABLE_FULLSCREEN_API) $(ENABLE_GEOLOCATION) $(ENABLE_ICONDATABASE) $(ENABLE_IMAGE_RESIZER) $(ENABLE_INDEXED_DATABASE) $(ENABLE_INPUT_SPEECH) $(ENABLE_JAVASCRIPT_DEBUGGER) $(ENABLE_MATHML) $(ENABLE_METER_TAG) $(ENABLE_NOTIFICATIONS) $(ENABLE_OFFLINE_WEB_APPLICATIONS) $(ENABLE_PROGRESS_TAG) $(ENABLE_REGISTER_PROTOCOL_HANDLER) $(ENABLE_SHARED_WORKERS) $(ENABLE_SVG) $(ENABLE_SVG_ANIMATION) $(ENABLE_SVG_AS_IMAGE) $(ENABLE_SVG_DOM_OBJC_BINDINGS) $(ENABLE_SVG_FONTS) $(ENABLE_SVG_FOREIGN_OBJECT) $(ENABLE_SVG_USE) $(ENABLE_VIDEO) $(ENABLE_WEB_AUDIO) $(ENABLE_WEB_SOCKETS) $(ENABLE_WEB_TIMING) $(ENABLE_WML) $(ENABLE_WORKERS) $(ENABLE_XHTMLMP) $(ENABLE_XPATH) $(ENABLE_XSLT);
> > 
> > See the comment at the top of the file about keeping this value in sync with all other FeatureDefines.xcconfig files.
> 
> Yep, I think I've amended all the required files (all FeatureDefines.xccode, build-webkit, and FeatureDefines.vsprops and FeatureDefinesCairo.vsprops). Did you mention this because you saw that I missed something?

JavaScriptCore one?

[James Kozianski]

Created attachment 80681 [details]
Patch

[James Kozianski]

(In reply to comment #36)
> (In reply to comment #34)
> > (In reply to comment #27)
> > > (From update of attachment 80604 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=80604&action=review
> > > 
> > > Mostly style nits and the bit about xcconfig synchronization.
> > > 
> > > > LayoutTests/fast/dom/registerProtocolHandler.html:1
> > > > +<html>
> > > 
> > > Typically test file names are dash-separated.html
> > 
> > Fixed.
> > 
> > > 
> > > > LayoutTests/fast/dom/registerProtocolHandler.html:8
> > > > + }
> > > 
> > > Indentation oopsie.
> > 
> > Fixed.
> > 
> > > 
> > > > LayoutTests/fast/dom/registerProtocolHandler.html:10
> > > > +function debug(str) {
> > > 
> > > Function braces start on new line (here and elsewhere).
> > 
> > Fixed.
> > 
> > > 
> > > > LayoutTests/fast/dom/registerProtocolHandler.html:16
> > > > +if(window.navigator.registerProtocolHandler) {
> > > 
> > > Don't need extra braces for single-line statements (here and elsewhere).
> > 
> > Fixed.
> > 
> > > 
> > > > Source/WebKit/mac/Configurations/FeatureDefines.xcconfig:126
> > > > +FEATURE_DEFINES = $(ENABLE_LINK_PREFETCH) $(ENABLE_ACCELERATED_2D_CANVAS) $(ENABLE_WEBGL) $(ENABLE_3D_RENDERING) $(ENABLE_BLOB) $(ENABLE_CHANNEL_MESSAGING) $(ENABLE_CLIENT_BASED_GEOLOCATION) $(ENABLE_DATABASE) $(ENABLE_DATAGRID) $(ENABLE_DATALIST) $(ENABLE_DEVICE_ORIENTATION) $(ENABLE_DIRECTORY_UPLOAD) $(ENABLE_DOM_STORAGE) $(ENABLE_EVENTSOURCE) $(ENABLE_FILTERS) $(ENABLE_FILE_SYSTEM) $(ENABLE_FULLSCREEN_API) $(ENABLE_GEOLOCATION) $(ENABLE_ICONDATABASE) $(ENABLE_IMAGE_RESIZER) $(ENABLE_INDEXED_DATABASE) $(ENABLE_INPUT_SPEECH) $(ENABLE_JAVASCRIPT_DEBUGGER) $(ENABLE_MATHML) $(ENABLE_METER_TAG) $(ENABLE_NOTIFICATIONS) $(ENABLE_OFFLINE_WEB_APPLICATIONS) $(ENABLE_PROGRESS_TAG) $(ENABLE_REGISTER_PROTOCOL_HANDLER) $(ENABLE_SHARED_WORKERS) $(ENABLE_SVG) $(ENABLE_SVG_ANIMATION) $(ENABLE_SVG_AS_IMAGE) $(ENABLE_SVG_DOM_OBJC_BINDINGS) $(ENABLE_SVG_FONTS) $(ENABLE_SVG_FOREIGN_OBJECT) $(ENABLE_SVG_USE) $(ENABLE_VIDEO) $(ENABLE_WEB_AUDIO) $(ENABLE_WEB_SOCKETS) $(ENABLE_WEB_TIMING) $(ENABLE_WML) $(ENABLE_WORKERS) $(ENABLE_XHTMLMP) $(ENABLE_XPATH) $(ENABLE_XSLT);
> > > 
> > > See the comment at the top of the file about keeping this value in sync with all other FeatureDefines.xcconfig files.
> > 
> > Yep, I think I've amended all the required files (all FeatureDefines.xccode, build-webkit, and FeatureDefines.vsprops and FeatureDefinesCairo.vsprops). Did you mention this because you saw that I missed something?
> 
> JavaScriptCore one?

Oh no, that one is in there :-)

[Dimitri Glazkov (Google)]

Comment on attachment 80681 [details]
Patch

ok.

[WebKit Commit Bot]

Comment on attachment 80681 [details]
Patch

Rejecting attachment 80681 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-7', 'apply-..." exit_code: 2

Last 500 characters of output:
unk #2 FAILED at 152.
2 out of 2 hunks FAILED -- saving rejects to file WebKitLibraries/win/tools/vsprops/FeatureDefines.vsprops.rej
patching file WebKitLibraries/win/tools/vsprops/FeatureDefinesCairo.vsprops
Hunk #1 FAILED at 9.
Hunk #2 FAILED at 152.
2 out of 2 hunks FAILED -- saving rejects to file WebKitLibraries/win/tools/vsprops/FeatureDefinesCairo.vsprops.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Dimitri Glazkov', u'--..." exit_code: 1

Full output: http://queues.webkit.org/results/7685486

[James Kozianski]

Created attachment 81174 [details]
Patch

[James Kozianski]

Created attachment 81177 [details]
Patch

[James Kozianski]

Created attachment 81179 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 81179 [details]
Patch

Rejecting attachment 81179 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-4', 'apply-..." exit_code: 2

Last 500 characters of output:
.vsprops
Hunk #1 FAILED at 9.
Hunk #2 FAILED at 152.
2 out of 2 hunks FAILED -- saving rejects to file WebKitLibraries/win/tools/vsprops/FeatureDefines.vsprops.rej
patching file WebKitLibraries/win/tools/vsprops/FeatureDefinesCairo.vsprops
Hunk #1 FAILED at 9.
Hunk #2 FAILED at 152.
2 out of 2 hunks FAILED -- saving rejects to file WebKitLibraries/win/tools/vsprops/FeatureDefinesCairo.vsprops.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/7692753

[David Levin]

I'm landing this.

[David Levin]

Committed as http://trac.webkit.org/changeset/77607.