Bug 81573 - Add support for [ArrayClass] and use that on NodeList
: Add support for [ArrayClass] and use that on NodeList
Status: REOPENED
: WebKit
HTML DOM
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
: http://dom.spec.whatwg.org/#interface...
: WebExposed
: 81588 83093 83766
:
  Show dependency treegraph
 
Reported: 2012-03-19 15:37 PST by
Modified: 2013-07-08 09:56 PST (History)


Attachments
Patch (38.43 KB, patch)
2012-03-19 16:54 PST, Erik Arvidsson
no flags Review Patch | Details | Formatted Diff | Diff
Patch (33.71 KB, patch)
2012-04-05 15:04 PST, Erik Arvidsson
no flags Review Patch | Details | Formatted Diff | Diff
Patch (34.21 KB, patch)
2012-04-05 15:59 PST, Erik Arvidsson
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (6.58 MB, application/zip)
2012-04-05 17:09 PST, WebKit Review Bot
no flags Details
Archive of layout-test-results from ec2-cr-linux-03 (6.55 MB, application/zip)
2012-04-05 17:57 PST, WebKit Review Bot
no flags Details
Patch (37.00 KB, patch)
2012-04-05 18:06 PST, Erik Arvidsson
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (37.02 KB, patch)
2012-04-11 15:30 PST, Erik Arvidsson
no flags Review Patch | Details | Formatted Diff | Diff
Patch (39.87 KB, patch)
2012-10-24 15:04 PST, Erik Arvidsson
no flags Review Patch | Details | Formatted Diff | Diff
Patch (29.54 KB, patch)
2012-10-25 08:44 PST, Erik Arvidsson
no flags Review Patch | Details | Formatted Diff | Diff
Patch (29.54 KB, patch)
2012-10-25 13:32 PST, Erik Arvidsson
no flags Review Patch | Details | Formatted Diff | Diff
Patch (29.71 KB, patch)
2012-10-26 12:27 PST, Erik Arvidsson
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-03-19 15:37:27 PST
DOM4 specs NodeList as having Array.prototype in its prototype chain.

Patch coming...
------- Comment #1 From 2012-03-19 16:54:41 PST -------
Created an attachment (id=132720) [details]
Patch
------- Comment #2 From 2012-03-19 17:34:34 PST -------
(From update of attachment 132720 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=132720&action=review

> Source/WebCore/bindings/scripts/IDLAttributes.txt:22
> +ArrayClass

Please update https://trac.webkit.org/wiki/WebKitIDL once this patch lands.

> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:629
> +    // Bail out if fetching failed.

These comments are sort of silly.  Maybe remove them above too?

> Source/WebCore/bindings/v8/V8DOMWindowShell.h:80
> +    // WARNING: Call |installHiddenPrototypeObjects| only on fresh contexts!

Can we ASSERT that these hidden property names don't already exist?  That might catch someone trying to call this function too often.

> Source/WebCore/bindings/v8/V8DOMWrapper.cpp:123
> +        v8::Local<v8::Value> prototypeValue = value->Get(v8::String::NewSymbol("prototype"));

Is this get observable with __defineGetter__ ?

> Source/WebCore/bindings/v8/V8DOMWrapper.cpp:127
> +                prototypeValue.As<v8::Object>()->SetPrototype(arrayPrototype);

I'm slightly unclear how often this function gets called.  Do we need to worry about overwriting user changes to the prototype chain?

> LayoutTests/fast/dom/NodeList/node-list-array-class.html:10
> +var nodeList = document.childNodes;
> +shouldBe('nodeList.__proto__', 'NodeList.prototype');
> +shouldBe('nodeList.__proto__.__proto__', 'Array.prototype');
> +
> +shouldBe('NodeList.prototype.__proto__', 'Array.prototype');

Can you add some tests about what happens if script tries to mutate these prototype chains (e.g., before any objects are constructed, etc).
------- Comment #3 From 2012-03-19 17:35:20 PST -------
BTW, I'm super excited that we're making this change.
------- Comment #4 From 2012-03-19 17:39:34 PST -------
I think this is only called once and there is no way to get in before this is called. However, I'll convince myself and/or add a test.
------- Comment #5 From 2012-03-19 17:55:55 PST -------
> I think this is only called once and there is no way to get in before this is called. However, I'll convince myself and/or add a test.

Thanks.  Everything else in the patch looks good.
------- Comment #6 From 2012-03-19 19:35:20 PST -------
(In reply to comment #2)
> (From update of attachment 132720 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=132720&action=review
> 
> > Source/WebCore/bindings/scripts/IDLAttributes.txt:22
> > +ArrayClass
> 
> Please update https://trac.webkit.org/wiki/WebKitIDL once this patch lands.

Will do

> > Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:629
> > +    // Bail out if fetching failed.
> 
> These comments are sort of silly.  Maybe remove them above too?

Agreed. I removed them.

> > Source/WebCore/bindings/v8/V8DOMWindowShell.h:80
> > +    // WARNING: Call |installHiddenPrototypeObjects| only on fresh contexts!
> 
> Can we ASSERT that these hidden property names don't already exist?  That might catch someone trying to call this function too often.

Added asserts

> > Source/WebCore/bindings/v8/V8DOMWrapper.cpp:123
> > +        v8::Local<v8::Value> prototypeValue = value->Get(v8::String::NewSymbol("prototype"));
> 
> Is this get observable with __defineGetter__ ?

No, the prototype is not configurable so you cannot add a getter.

Object.defineProperty(NodeList, 'prototype', {...})  // Uncaught TypeError: Cannot redefine property: prototype

> > Source/WebCore/bindings/v8/V8DOMWrapper.cpp:127
> > +                prototypeValue.As<v8::Object>()->SetPrototype(arrayPrototype);
> 
> I'm slightly unclear how often this function gets called.  Do we need to worry about overwriting user changes to the prototype chain?

This gets called once per context (I instrumented the code.)

> > LayoutTests/fast/dom/NodeList/node-list-array-class.html:10
> > +var nodeList = document.childNodes;
> > +shouldBe('nodeList.__proto__', 'NodeList.prototype');
> > +shouldBe('nodeList.__proto__.__proto__', 'Array.prototype');
> > +
> > +shouldBe('NodeList.prototype.__proto__', 'Array.prototype');
> 
> Can you add some tests about what happens if script tries to mutate these prototype chains (e.g., before any objects are constructed, etc).

Good catch. This patch breaks writable __proto__ of NodeList.prototype when using V8. I'll have to track this down later this week.
------- Comment #7 From 2012-03-19 21:05:02 PST -------
(From update of attachment 132720 [details])
Attachment 132720 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11995039
------- Comment #8 From 2012-04-05 15:04:09 PST -------
Created an attachment (id=135914) [details]
Patch
------- Comment #9 From 2012-04-05 15:44:13 PST -------
(From update of attachment 135914 [details])
Attachment 135914 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12338485
------- Comment #10 From 2012-04-05 15:59:50 PST -------
Created an attachment (id=135922) [details]
Patch
------- Comment #11 From 2012-04-05 17:09:09 PST -------
(From update of attachment 135922 [details])
Attachment 135922 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12338538

New failing tests:
inspector/console/console-dir.html
inspector/console/command-line-api.html
inspector/console/console-format-collections.html
------- Comment #12 From 2012-04-05 17:09:15 PST -------
Created an attachment (id=135941) [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
------- Comment #13 From 2012-04-05 17:57:40 PST -------
(From update of attachment 135922 [details])
Attachment 135922 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12340585

New failing tests:
inspector/console/console-dir.html
inspector/console/command-line-api.html
inspector/console/console-format-collections.html
------- Comment #14 From 2012-04-05 17:57:46 PST -------
Created an attachment (id=135948) [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
------- Comment #15 From 2012-04-05 18:06:51 PST -------
Created an attachment (id=135951) [details]
Patch
------- Comment #16 From 2012-04-06 15:02:46 PST -------
(From update of attachment 135951 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=135951&action=review

This looks great.  I'm glad that we're making this change.  Don't forget to update the IDL documentation.  Thanks!

> Source/WebCore/bindings/v8/V8HiddenPropertyName.h:40
>      V(objectPrototype) \
> +    V(arrayPrototype) \

I don't think we're using these anymore.  We can probably just delete them now that we're using V8BindingPerContextData.
------- Comment #17 From 2012-04-06 15:07:36 PST -------
(In reply to comment #16)
> (From update of attachment 135951 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=135951&action=review
> 
> This looks great.  I'm glad that we're making this change.  Don't forget to update the IDL documentation.  Thanks!
>

Will do


> > Source/WebCore/bindings/v8/V8HiddenPropertyName.h:40
> >      V(objectPrototype) \
> > +    V(arrayPrototype) \
> 
> I don't think we're using these anymore.  We can probably just delete them now that we're using V8BindingPerContextData.

Removed
------- Comment #18 From 2012-04-11 13:39:05 PST -------
(From update of attachment 135951 [details])
Rejecting attachment 135951 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
orm/chromium/inspector/console/command-line-api-expected.txt
patching file LayoutTests/platform/chromium/inspector/console/console-dir-expected.txt
patching file LayoutTests/platform/chromium/inspector/console/console-format-collections-expected.txt
patching file LayoutTests/platform/mac/fast/dom/prototype-inheritance-2-expected.txt

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

Full output: http://queues.webkit.org/results/12384969
------- Comment #19 From 2012-04-11 15:30:21 PST -------
Created an attachment (id=136767) [details]
Patch for landing
------- Comment #20 From 2012-04-11 18:00:48 PST -------
(From update of attachment 136767 [details])
Clearing flags on attachment: 136767

Committed r113931: <http://trac.webkit.org/changeset/113931>
------- Comment #21 From 2012-04-11 18:01:11 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #22 From 2012-04-12 00:03:20 PST -------
(In reply to comment #21)
> All reviewed patches have been landed.  Closing bug.

This patch broke 4 tests on Mac/GTK/Qt. First failing GTK build:

http://build.webkit.org/results/GTK%20Linux%2064-bit%20Release/r113931%20(22099)/results.html

It seems some tests could be rebaseline but some also clearly fail. Any advice Erik or Adam?
------- Comment #23 From 2012-04-12 00:45:52 PST -------
I'm not sure.  Hopefully arv knows.
------- Comment #24 From 2012-04-12 03:27:53 PST -------
(In reply to comment #23)
> I'm not sure.  Hopefully arv knows.

I'm rolling it out for now. Sorry! Hopefully indeed Erik will be able to reland it soon.
------- Comment #25 From 2012-04-12 03:57:25 PST -------
It broke not only 4 tests, but 8:
http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Release/r113956%20%2836095%29/results.html

(all fails apart from fast/dom/Window/window-properties.html)
------- Comment #26 From 2012-04-12 08:04:04 PST -------
Sigh, these just needed rebaselines. Sorry for not communicating this better. 

All platforms that build with JSC will have expectations that fail due to the JSC bug this depends on.
------- Comment #27 From 2012-04-12 14:19:05 PST -------
The ChangLogs got messed up so this did not get closed automatically.

http://trac.webkit.org/changeset/114036

This probably still needs rebaseline on non Chromium/Apple ports.
------- Comment #28 From 2012-04-13 12:26:23 PST -------
Guys,

This broke GMail in JSC: https://bugs.webkit.org/show_bug.cgi?id=83787

-F
------- Comment #29 From 2012-04-13 12:29:20 PST -------
(In reply to comment #28)
> Guys,
> 
> This broke GMail in JSC: https://bugs.webkit.org/show_bug.cgi?id=83787
> 
> -F

Marking fixed again and moving the discussion to the relevant JSC bug: 81588
------- Comment #30 From 2012-04-13 14:39:30 PST -------
Reopen pending commit of blocking work.
------- Comment #31 From 2012-04-13 14:40:17 PST -------
(In reply to comment #29)
> (In reply to comment #28)
> > Guys,
> > 
> > This broke GMail in JSC: https://bugs.webkit.org/show_bug.cgi?id=83787
> > 
> > -F
> 
> Marking fixed again and moving the discussion to the relevant JSC bug: 81588

No.

In the future, don't land changes that depend on JSC changes that haven't landed.

I have reopened this bug, and will close it when the world is right again.
------- Comment #33 From 2012-04-20 16:11:02 PST -------
What is the policy for fixing incorrect behavior if it reduces performance?
------- Comment #34 From 2012-04-20 16:22:21 PST -------
(In reply to comment #33)
> What is the policy for fixing incorrect behavior if it reduces performance?

There's no official policy. It's rare that this is actually the tradeoff we have to make. When we do, it's on a case-by-case basis. Usually, there are ways to make the performance work...it just may be painful figuring it out. :(
------- Comment #35 From 2012-04-20 16:23:23 PST -------
(In reply to comment #33)
> What is the policy for fixing incorrect behavior if it reduces performance?

I think that it depends on how urgent the fix is. If it is a security bug, we might allow the performance regression temporarily. Otherwise, we might want to fix the performance issue first.
------- Comment #36 From 2012-04-20 16:30:00 PST -------
(In reply to comment #35)
> I think that it depends on how urgent the fix is. If it is a security bug, we might allow the performance regression temporarily. Otherwise, we might want to fix the performance issue first.

I know others don't agree with me on this one but my opinion is that we should have the correct behavior first and then based on real data (not micro benchmarks like dromaeo) identify the performance bottle necks.

Still, I don't see why this would slow down dromeao so I will look into this before trying to landing this again.

Some update on the status of this. I have fixed the Closure Library bug that cause gmail and other google products to fail. This will of course take some time to trickle out to production servers. My current plan is to wait for at least the reported site breakages to get fixed before trying to land this again.
------- Comment #37 From 2012-04-20 16:39:21 PST -------
(In reply to comment #36)
> Still, I don't see why this would slow down dromeao so I will look into this before trying to landing this again.

Sounds like a good idea. I think we should first identify the performance bottleneck. (At least we might want to avoid landing the patch without knowing what the performance issue is.) If the performance issue is easy to fix, let's fix it first. Otherwise, let's discuss at the time.
------- Comment #38 From 2012-04-20 22:30:56 PST -------
(In reply to comment #36)
> (In reply to comment #35)
> > I think that it depends on how urgent the fix is. If it is a security bug, we might allow the performance regression temporarily. Otherwise, we might want to fix the performance issue first.
> 
> I know others don't agree with me on this one but my opinion is that we should have the correct behavior first and then based on real data (not micro benchmarks like dromaeo) identify the performance bottle necks.

Yes, I agree.
------- Comment #39 From 2012-04-20 23:56:11 PST -------
(In reply to comment #36)
> Some update on the status of this. I have fixed the Closure Library bug that cause gmail and other google products to fail. This will of course take some time to trickle out to production servers. My current plan is to wait for at least the reported site breakages to get fixed before trying to land this again.

So what you are saying here is that this change breaks the web. Sounds like the only course of action is to fix the spec (which is just an editors draft, there is no "incorrect behavior" here).
------- Comment #40 From 2012-04-21 00:13:07 PST -------
(In reply to comment #39)
> (In reply to comment #36)
> > Some update on the status of this. I have fixed the Closure Library bug that cause gmail and other google products to fail. This will of course take some time to trickle out to production servers. My current plan is to wait for at least the reported site breakages to get fixed before trying to land this again.
> 
> So what you are saying here is that this change breaks the web. Sounds like the only course of action is to fix the spec (which is just an editors draft, there is no "incorrect behavior" here).

Yeah, I'm afraid there might be other websites that depend on the current behavior.
------- Comment #41 From 2012-04-21 00:14:35 PST -------
Closing this. If this causes google sites to break then a few million other pages are going to break too.
------- Comment #42 From 2012-04-21 10:43:08 PST -------
I'm well aware that this is just an editor's draft but products broke due to a bug in closure library. I agree that this one is risky but the benefits are huge. People have personally thanked me for doing this.

Antti, next time don't close things if you don't know the issue.
------- Comment #43 From 2012-04-21 12:16:40 PST -------
(In reply to comment #42)
> I'm well aware that this is just an editor's draft but products broke due to a bug in closure library. I agree that this one is risky but the benefits are huge. People have personally thanked me for doing this.

Why makes us think this change won't break the Web?
------- Comment #44 From 2012-06-29 01:27:52 PST -------
What was the specific breakage here?  Is it something fundamental, or something we could try and work around in the spec?
------- Comment #45 From 2012-06-29 09:44:17 PST -------
(In reply to comment #44)
> What was the specific breakage here?  Is it something fundamental, or something we could try and work around in the spec?

The problem is that there is code out there that do instanceof Array checks but then use methods that work differently if the object is a real array or not. The case that broke most Google properties was Array.prototype.concat.

The Google code has been fixed and my plan is to try to land this again to get some more real world feedback. It is very likely that it will not be feasible to do this until we make changes to ES. We have have some ideas (Allen mostly) about how we should not use nominal typing to trigger the array behaviors for concat (Item 5 at http://wiki.ecmascript.org/doku.php?id=strawman:es5_internal_nominal_typing&s=concat )
------- Comment #46 From 2012-10-11 19:00:10 PST -------
Should this still be open?
------- Comment #47 From 2012-10-12 06:46:02 PST -------
(In reply to comment #46)
> Should this still be open?

Yes. [ArrayClass] is still in the spec for NodeList.

It is something that improves the DOM API a lot and a lot of people want this.

My plan was to try this again so we should keep this open.
------- Comment #48 From 2012-10-24 15:04:09 PST -------
Created an attachment (id=170480) [details]
Patch
------- Comment #49 From 2012-10-24 15:06:15 PST -------
This new patch puts this under a runtime feature flag.

Dimitri, do you mind reviewing the runtime feature code?

Adam: The binding stuff got even simpler than the last time you r+ this.
------- Comment #50 From 2012-10-24 15:08:51 PST -------
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
------- Comment #51 From 2012-10-24 15:09:23 PST -------
Attachment 170480 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
LayoutTests/platform/qt/TestExpectations:192:  Path does not exist.  [test/expectations] [5]
LayoutTests/platform/efl-wk1/TestExpectations:315:  Path does not exist.  [test/expectations] [5]
Total errors found: 2 in 45 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #52 From 2012-10-24 15:59:35 PST -------
(From update of attachment 170480 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=170480&action=review

> LayoutTests/ChangeLog:46
> +        * platform/wk2/TestExpectations:

You don't actually need to suppress this in every TestExpectations file; most ports use a cascade of files, so you only need to update the "base" variant of each port, e.g., platform/efl/TestExpectations, platform/mac, platform/qt, platform/win, platform/win-cairo ...
------- Comment #53 From 2012-10-25 01:27:06 PST -------
(From update of attachment 170480 [details])
I don't see any new information why this would be ok this time.
------- Comment #54 From 2012-10-25 07:06:23 PST -------
(In reply to comment #53)
> (From update of attachment 170480 [details] [details])
> I don't see any new information why this would be ok this time.

This patch uses a runtime flag so that we can gather more information.

If you check previous discussions, one of the major JS libraries had a bug that has been fixed. Another benefit to doing more testing is to see if there are any other things that might needs to be fixed in ES (like concat which was found the first time around).
------- Comment #55 From 2012-10-25 08:44:58 PST -------
Created an attachment (id=170660) [details]
Patch
------- Comment #56 From 2012-10-25 08:45:30 PST -------
(From update of attachment 170480 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=170480&action=review

>> LayoutTests/ChangeLog:46
>> +        * platform/wk2/TestExpectations:
> 
> You don't actually need to suppress this in every TestExpectations file; most ports use a cascade of files, so you only need to update the "base" variant of each port, e.g., platform/efl/TestExpectations, platform/mac, platform/qt, platform/win, platform/win-cairo ...

Done.
------- Comment #57 From 2012-10-25 10:24:55 PST -------
(In reply to comment #54)
> If you check previous discussions, one of the major JS libraries had a bug that has been fixed. Another benefit to doing more testing is to see if there are any other things that might needs to be fixed in ES (like concat which was found the first time around).

You say there are versions of a major JS library that break with this change. Surely all instances haven't suddenly disappeared from the web. I don't see how we could make this behavior change any time soon. We don't want dead code in WebKit.
------- Comment #58 From 2012-10-25 10:42:26 PST -------
My understanding per the recent webkit-dev thread is that one preferred way of making potentially backwards incompatible changes is to try shipping it and see what break. Chromium would like to try turning this on again and seeing what the fallout is in out experimental channels.

This seems like reasonable way forward here unless we want to give up on this change entirely, which would be really sad IMO.

The JS library that broke (Closure?) I believe only broke if you happened to call a specific method, so there's a non-trivial chance that this change will stick this time since the sites we know to have broken last time have been updated.
------- Comment #59 From 2012-10-25 11:33:48 PST -------
(In reply to comment #58)
> My understanding per the recent webkit-dev thread is that one preferred way of making potentially backwards incompatible changes is to try shipping it and see what break. Chromium would like to try turning this on again and seeing what the fallout is in out experimental channels.

This was tested already. It broke a bunch of Google sites via a popular framework. I can't think of any more convincing evidence that this is not a safe change. Any overriding evidence would need to be pretty extraordinary.

> This seems like reasonable way forward here unless we want to give up on this change entirely, which would be really sad IMO.

That and the spec change seem like the right way to go. Maybe it would be better to have a new API instead of trying to retrofit a new behavior to the old one?
------- Comment #60 From 2012-10-25 13:14:39 PST -------
I think you all are talking past each other a bit.

> This was tested already. It broke a bunch of Google sites via a popular framework.

Ojan wrote:

> The sites we know to have broken last time have been updated.

That makes it seem like it might be worth trying again.

The change might or might not stick this time, but I don't see the harm in shipping it to Chrome's Canary channel now that the previous breakages have been fixed.  One of the goals of the Canary channel is to let us try changes like this.  Once we get data from that experiment, we can either roll out the change or enable it everywhere.
------- Comment #61 From 2012-10-25 13:32:06 PST -------
Created an attachment (id=170723) [details]
Patch
------- Comment #62 From 2012-10-25 13:35:02 PST -------
Attachment 170723 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
LayoutTests/platform/qt/TestExpectations:2477:  Path does not exist.  [test/expectations] [5]
Total errors found: 1 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #63 From 2012-10-25 13:49:13 PST -------
(In reply to comment #62)
> Attachment 170723 [details] [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
> LayoutTests/platform/qt/TestExpectations:2477:  Path does not exist.  [test/expectations] [5]
> Total errors found: 1 in 31 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

This is not part of my change.
------- Comment #64 From 2012-10-25 13:50:19 PST -------
(In reply to comment #60)
> I think you all are talking past each other a bit.

I don't think so.

> > This was tested already. It broke a bunch of Google sites via a popular framework.
> 
> Ojan wrote:
> 
> > The sites we know to have broken last time have been updated.
> 
> That makes it seem like it might be worth trying again.
> 
> The change might or might not stick this time, but I don't see the harm in shipping it to Chrome's Canary channel now that the previous breakages have been fixed.  One of the goals of the Canary channel is to let us try changes like this.  Once we get data from that experiment, we can either roll out the change or enable it everywhere.

I just can't see what kind of data could convincingly prove that this change broke several google sites and nothing else in the web. That seems exceedingly unlikely. Wouldn't it make more sense to start thinking alternative approaches?
------- Comment #65 From 2012-10-25 15:20:26 PST -------
I'm sorry, but I must not understand your point of view correctly.  My understanding of what has happened so far is the following:

1) We tried landing this patch before, and we ran into compatibility problems.
2) We evangelized the web sites to resolve the compatibility problems.
3) We'd now like to try again to see whether there are other problems.

It doesn't seem unreasonable to try (3) now that (2) is done.  It's possible that (3) will work, and it's possible (3) won't work, but that doesn't mean we shouldn't try.

Am I misunderstanding?
------- Comment #66 From 2012-10-26 06:44:33 PST -------
1. Fixing NodeLists is one of the highest priority bugs from web developers so it would be a shame to give up so easily.
2. Without having this, under a flag, in a real browser, it will be hard to know if there are other buggy code that depends on NodeLists not being arrays. Having this feedback is crucial for fixing these things in ES.
3. The known culprit (Closure Library) that broke lots of sites have been fixed.

Antti, are you still opposed to landing this under a runtime flag for Chromium?
------- Comment #67 From 2012-10-26 07:04:42 PST -------
I'm not opposed to landing it per se, I just don't see the point. WebKit does not accept web API changes that cause significant breakage and require evangelizing without extremely good reason (security generally). That the breakage here was first noticed on google sites that the the patch authors were able to get changed was a pure coincidence. I can't see how any further testing could prove this change safe for the rest of the web. Considering this it seems to me that everyones time would be better spent finding alternative solutions.
------- Comment #68 From 2012-10-26 07:19:34 PST -------
(In reply to comment #67)
> I'm not opposed to landing it per se, I just don't see the point. WebKit does not accept web API changes that cause significant breakage and require evangelizing without extremely good reason (security generally). That the breakage here was first noticed on google sites that the the patch authors were able to get changed was a pure coincidence. I can't see how any further testing could prove this change safe for the rest of the web. Considering this it seems to me that everyones time would be better spent finding alternative solutions.

This is about developer productivity and making the platform more consistent for future generations.

We are also looking for other ways to make the platform more consistent.
------- Comment #69 From 2012-10-26 12:14:04 PST -------
We believe this solution to be enough better than any alternatives that we'd really like to be sure before abandoning it. Sounds like there's enough resolution to move forward (i.e. no firm objections).

Arv, mind uploading a patch that passes the EWS bots? abarth or haraken should probably review the bindings changes.
------- Comment #70 From 2012-10-26 12:27:37 PST -------
Created an attachment (id=170983) [details]
Patch
------- Comment #71 From 2012-10-26 12:31:50 PST -------
> Considering this it seems to me that everyones time would be better spent finding alternative solutions.

Given that this is an open source project, arv gets to decide how he would like to spend his time.  I understand that this change isn't a high priority for you, but, for whatever reason, arv is interested in vesting his time here.
------- Comment #72 From 2012-11-03 18:12:05 PST -------
Has this been discussed on webkit-dev recently ? I believe that this change has significant enough Web exposed behavior change that we want to announce it there before landing it. Better yet, we should make getElementsBy* use HTMLCollection instead of NodeList to minimize the potential compat. problems. I have merged implementations of those two interfaces enough that making the transition should be easy now.
------- Comment #73 From 2012-11-05 11:38:55 PST -------
(In reply to comment #72)
> Has this been discussed on webkit-dev recently ? I believe that this change has significant enough Web exposed behavior change that we want to announce it there before landing it. Better yet, we should make getElementsBy* use HTMLCollection instead of NodeList to minimize the potential compat. problems. I have merged implementations of those two interfaces enough that making the transition should be easy now.

wb

Given that this is going to be off by default, do you still think it is worth having a discussion on webkit-dev?
------- Comment #74 From 2012-11-05 11:43:06 PST -------
(In reply to comment #72)
> Better yet, we should make getElementsBy* use HTMLCollection instead of NodeList to minimize the potential compat. problems.

The whole point of this change is to be able to use array methods on the return values of these APIs. Although, this could be the next thing we try if we can't get this patch working. Then, at least other methods that return NodeLists could inherit from Array (e.g. querySelectorAll/findAll).
------- Comment #75 From 2012-11-05 11:46:39 PST -------
(In reply to comment #74)
> (In reply to comment #72)
> > Better yet, we should make getElementsBy* use HTMLCollection instead of NodeList to minimize the potential compat. problems.
> 
> The whole point of this change is to be able to use array methods on the return values of these APIs. Although, this could be the next thing we try if we can't get this patch working. Then, at least other methods that return NodeLists could inherit from Array (e.g. querySelectorAll/findAll).

That makes no sense. As I understand, there is a significant compat. issue with using NodeList for getElementsBy* functions reported by Mozilla so we must move onto to use HTMLCollection.
------- Comment #76 From 2012-11-05 11:50:36 PST -------
(In reply to comment #75)
> (In reply to comment #74)
> > (In reply to comment #72)
> > > Better yet, we should make getElementsBy* use HTMLCollection instead of NodeList to minimize the potential compat. problems.
> > 
> > The whole point of this change is to be able to use array methods on the return values of these APIs. Although, this could be the next thing we try if we can't get this patch working. Then, at least other methods that return NodeLists could inherit from Array (e.g. querySelectorAll/findAll).
> 
> That makes no sense. As I understand, there is a significant compat. issue with using NodeList for getElementsBy* functions reported by Mozilla so we must move onto to use HTMLCollection.

Whether we use HTMLCollection or NodeList for getElementsBy* is off topic. Once/if we can use [ArrayClass] on NodeList I expect all list like interfaces to follow suite.
------- Comment #77 From 2012-11-05 11:52:03 PST -------
(In reply to comment #76)
> Whether we use HTMLCollection or NodeList for getElementsBy* is off topic. Once/if we can use [ArrayClass] on NodeList I expect all list like interfaces to follow suite.

I don’t think we’re doing that: https://www.w3.org/Bugs/Public/show_bug.cgi?id=16490
------- Comment #78 From 2012-11-05 11:59:00 PST -------
If the goal here is to make Array’s methods available on return values of getElementsBy* then that’s incompatible with Web as reported by Mozilla (see http://lists.w3.org/Archives/Public/www-dom/2012JanMar/0008.html and related posts) and we should close this bug with WONTFIX.

If not, then we must first change the return values of getElementsBy* to HTMLCollection before making this change; at least before we get rid of the runtime flag.
------- Comment #79 From 2012-11-05 12:11:06 PST -------
(In reply to comment #78)
> If the goal here is to make Array’s methods available on return values of getElementsBy* then that’s incompatible with Web as reported by Mozilla (see http://lists.w3.org/Archives/Public/www-dom/2012JanMar/0008.html and related posts) and we should close this bug with WONTFIX.
> 
> If not, then we must first change the return values of getElementsBy* to HTMLCollection before making this change; at least before we get rid of the runtime flag.

We can still do it for querySelectorAll (and findAll) which are what modern js uses.