Bug 52609 - Add navigator.registerProtocolHandler behind a flag.
Summary: Add navigator.registerProtocolHandler behind a flag.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-01-17 21:35 PST by James Kozianski
Modified: 2011-02-03 22:32 PST (History)
12 users (show)

See Also:


Attachments
Patch (29.41 KB, patch)
2011-01-17 21:52 PST, James Kozianski
no flags Details | Formatted Diff | Diff
Patch (50.93 KB, patch)
2011-01-19 18:54 PST, James Kozianski
no flags Details | Formatted Diff | Diff
Patch (39.17 KB, patch)
2011-01-23 16:41 PST, James Kozianski
no flags Details | Formatted Diff | Diff
Patch (39.09 KB, patch)
2011-01-24 15:43 PST, James Kozianski
no flags Details | Formatted Diff | Diff
Patch (39.09 KB, patch)
2011-01-24 20:48 PST, James Kozianski
no flags Details | Formatted Diff | Diff
Patch (39.19 KB, patch)
2011-01-24 22:02 PST, James Kozianski
no flags Details | Formatted Diff | Diff
Patch (39.43 KB, patch)
2011-01-27 20:17 PST, James Kozianski
no flags Details | Formatted Diff | Diff
Patch (39.42 KB, patch)
2011-01-30 15:22 PST, James Kozianski
no flags Details | Formatted Diff | Diff
Patch (35.87 KB, patch)
2011-01-31 14:56 PST, James Kozianski
no flags Details | Formatted Diff | Diff
Patch (36.98 KB, patch)
2011-02-03 19:33 PST, James Kozianski
no flags Details | Formatted Diff | Diff
Patch (36.95 KB, patch)
2011-02-03 20:04 PST, James Kozianski
no flags Details | Formatted Diff | Diff
Patch (36.99 KB, patch)
2011-02-03 20:15 PST, James Kozianski
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Kozianski 2011-01-17 21:35:27 PST
Add navigator.registerProtocolHandler behind a flag.
Comment 1 James Kozianski 2011-01-17 21:52:42 PST
Created attachment 79241 [details]
Patch
Comment 2 Adam Barth 2011-01-17 22:06:24 PST
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 3 David Levin 2011-01-17 22:40:05 PST
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 4 James Kozianski 2011-01-19 18:37:07 PST
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.
Comment 5 James Kozianski 2011-01-19 18:54:44 PST
Created attachment 79538 [details]
Patch
Comment 6 WebKit Review Bot 2011-01-19 19:26:45 PST
Attachment 79538 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7526239
Comment 7 James Kozianski 2011-01-19 19:54:50 PST
(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).
Comment 8 David Levin 2011-01-20 14:31:57 PST
(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 9 David Levin 2011-01-20 14:32:30 PST
Comment on attachment 79538 [details]
Patch

r- for build breaks (plus this should have some layout tests).
Comment 10 James Kozianski 2011-01-23 16:34:00 PST
(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.
Comment 11 James Kozianski 2011-01-23 16:41:02 PST
Created attachment 79883 [details]
Patch
Comment 12 David Levin 2011-01-23 23:03:00 PST
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.
Comment 13 James Kozianski 2011-01-24 15:43:28 PST
Created attachment 79983 [details]
Patch
Comment 14 James Kozianski 2011-01-24 15:44:57 PST
(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.
Comment 15 WebKit Review Bot 2011-01-24 15:52:18 PST
Attachment 79983 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7645054
Comment 16 James Kozianski 2011-01-24 15:57:58 PST
Comment on attachment 79983 [details]
Patch

Investigating gtk and efl breakages.
Comment 17 Early Warning System Bot 2011-01-24 15:58:06 PST
Attachment 79983 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7528307
Comment 18 WebKit Review Bot 2011-01-24 16:16:31 PST
Attachment 79983 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7598327
Comment 19 James Kozianski 2011-01-24 20:48:40 PST
Created attachment 80011 [details]
Patch
Comment 20 James Kozianski 2011-01-24 22:02:11 PST
Created attachment 80022 [details]
Patch
Comment 21 James Kozianski 2011-01-24 22:13:47 PST
Hm, I can't figure out why it won't apply.. Are there any common causes of that apart from being out of sync?
Comment 22 James Kozianski 2011-01-24 22:22:36 PST
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.
Comment 23 James Kozianski 2011-01-27 20:17:34 PST
Created attachment 80408 [details]
Patch
Comment 24 James Kozianski 2011-01-27 20:25:39 PST
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.
Comment 25 WebKit Review Bot 2011-01-27 21:02:01 PST
Attachment 80408 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7582410
Comment 26 James Kozianski 2011-01-30 15:22:32 PST
Created attachment 80604 [details]
Patch
Comment 27 Dimitri Glazkov (Google) 2011-01-30 18:23:50 PST
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.
Comment 28 Ojan Vafai 2011-01-30 18:44:37 PST
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.
Comment 29 David Levin 2011-01-31 10:51:35 PST
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.
Comment 30 Eric Seidel (no email) 2011-01-31 12:13:58 PST
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. :)
Comment 31 Ojan Vafai 2011-01-31 12:42:38 PST
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.
Comment 32 David Levin 2011-01-31 13:11:31 PST
(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.
Comment 33 Eric Seidel (no email) 2011-01-31 13:34:34 PST
(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)!
Comment 34 James Kozianski 2011-01-31 14:37:24 PST
(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?
Comment 35 James Kozianski 2011-01-31 14:52:41 PST
(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.
Comment 36 Dimitri Glazkov (Google) 2011-01-31 14:53:11 PST
(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?
Comment 37 James Kozianski 2011-01-31 14:56:10 PST
Created attachment 80681 [details]
Patch
Comment 38 James Kozianski 2011-01-31 15:00:45 PST
(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 39 Dimitri Glazkov (Google) 2011-01-31 15:34:47 PST
Comment on attachment 80681 [details]
Patch

ok.
Comment 40 WebKit Commit Bot 2011-02-01 00:19:44 PST
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
Comment 41 James Kozianski 2011-02-03 19:33:15 PST
Created attachment 81174 [details]
Patch
Comment 42 James Kozianski 2011-02-03 20:04:21 PST
Created attachment 81177 [details]
Patch
Comment 43 James Kozianski 2011-02-03 20:15:12 PST
Created attachment 81179 [details]
Patch
Comment 44 WebKit Commit Bot 2011-02-03 21:31:40 PST
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
Comment 45 David Levin 2011-02-03 21:45:41 PST
I'm landing this.
Comment 46 David Levin 2011-02-03 22:32:44 PST
Committed as http://trac.webkit.org/changeset/77607.