Add navigator.registerProtocolHandler behind a flag.
Created attachment 79241 [details] Patch
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?
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.
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.
Created attachment 79538 [details] Patch
Attachment 79538 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7526239
(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).
(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.
Comment on attachment 79538 [details] Patch r- for build breaks (plus this should have some layout tests).
(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.
Created attachment 79883 [details] Patch
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.
Created attachment 79983 [details] Patch
(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.
Attachment 79983 [details] did not build on gtk: Build output: http://queues.webkit.org/results/7645054
Comment on attachment 79983 [details] Patch Investigating gtk and efl breakages.
Attachment 79983 [details] did not build on qt: Build output: http://queues.webkit.org/results/7528307
Attachment 79983 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7598327
Created attachment 80011 [details] Patch
Created attachment 80022 [details] Patch
Hm, I can't figure out why it won't apply.. Are there any common causes of that apart from being out of sync?
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.
Created attachment 80408 [details] Patch
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.
Attachment 80408 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7582410
Created attachment 80604 [details] Patch
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.
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.
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.
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. :)
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.
(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.
(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)!
(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?
(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.
(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?
Created attachment 80681 [details] Patch
(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 :-)
Comment on attachment 80681 [details] Patch ok.
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
Created attachment 81174 [details] Patch
Created attachment 81177 [details] Patch
Created attachment 81179 [details] Patch
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
I'm landing this.
Committed as http://trac.webkit.org/changeset/77607.