Bug 81573 – Add support for [ArrayClass] and use that on NodeList

• Status: RESOLVED
• Resolution: INVALID
• Component: DOM

[Erik Arvidsson]

DOM4 specs NodeList as having Array.prototype in its prototype chain.

Patch coming...

[Erik Arvidsson]

Created attachment 132720 [details]
Patch

[Adam Barth]

Comment on attachment 132720 [details]
Patch

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).

[Adam Barth]

BTW, I'm super excited that we're making this change.

[Erik Arvidsson]

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.

[Adam Barth]

> 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.

[Erik Arvidsson]

(In reply to comment #2)
> (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.

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.

[WebKit Review Bot]

Comment on attachment 132720 [details]
Patch

Attachment 132720 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11995039

[Erik Arvidsson]

Created attachment 135914 [details]
Patch

[WebKit Review Bot]

Comment on attachment 135914 [details]
Patch

Attachment 135914 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12338485

[Erik Arvidsson]

Created attachment 135922 [details]
Patch

[WebKit Review Bot]

Comment on attachment 135922 [details]
Patch

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

[WebKit Review Bot]

Created attachment 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

[WebKit Review Bot]

Comment on attachment 135922 [details]
Patch

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

[WebKit Review Bot]

Created attachment 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

[Erik Arvidsson]

Created attachment 135951 [details]
Patch

[Adam Barth]

Comment on attachment 135951 [details]
Patch

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.

[Erik Arvidsson]

(In reply to comment #16)
> (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!
>

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

[WebKit Review Bot]

Comment on attachment 135951 [details]
Patch

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

[Erik Arvidsson]

Created attachment 136767 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 136767 [details]
Patch for landing

Clearing flags on attachment: 136767

Committed r113931: <http://trac.webkit.org/changeset/113931>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Philippe Normand]

(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?

[Adam Barth]

I'm not sure.  Hopefully arv knows.

[Philippe Normand]

(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.

[Csaba Osztrogonác]

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)

[Erik Arvidsson]

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.

[Erik Arvidsson]

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.

[Filip Pizlo]

Guys,

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

-F

[Erik Arvidsson]

(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

[Filip Pizlo]

Reopen pending commit of blocking work.

[Filip Pizlo]

(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.

[Antti Koivisto]

Note that this patch also seems to be a significant performance regression in Dromaeo:

It was landed here:

http://webkit-perf.appspot.com/graph.html#tests=[[45012,2001,32196]]&sel=1334260354892.0234,1334314425296.1956,164285.7142857143,200000&displayrange=30&datatype=running

And rolled out here:

http://webkit-perf.appspot.com/graph.html#tests=[[45012,2001,32196]]&sel=1334375766830.9153,1334399422632.7405,164285.7142857143,200000&displayrange=7&datatype=running

Please don't reland before fixing performance.

[Erik Arvidsson]

What is the policy for fixing incorrect behavior if it reduces performance?

[Ojan Vafai]

(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. :(

[Kentaro Hara]

(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.

[Erik Arvidsson]

(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.

[Kentaro Hara]

(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.

[Ryosuke Niwa]

(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.

[Antti Koivisto]

(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).

[Ryosuke Niwa]

(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.

[Antti Koivisto]

Closing this. If this causes google sites to break then a few million other pages are going to break too.

[Erik Arvidsson]

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.

[Ryosuke Niwa]

(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?

[Cameron McCormack (:heycam)]

What was the specific breakage here?  Is it something fundamental, or something we could try and work around in the spec?

[Erik Arvidsson]

(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 )

[Eric Seidel (no email)]

Should this still be open?

[Erik Arvidsson]

(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.

[Erik Arvidsson]

Created attachment 170480 [details]
Patch

[Erik Arvidsson]

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.

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

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.

[Dirk Pranke]

Comment on attachment 170480 [details]
Patch

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 ...

[Antti Koivisto]

Comment on attachment 170480 [details]
Patch

I don't see any new information why this would be ok this time.

[Erik Arvidsson]

(In reply to comment #53)
> (From update of attachment 170480 [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).

[Erik Arvidsson]

Created attachment 170660 [details]
Patch

[Erik Arvidsson]

Comment on attachment 170480 [details]
Patch

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.

[Antti Koivisto]

(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.

[Ojan Vafai]

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.

[Antti Koivisto]

(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?

[Adam Barth]

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.

[Erik Arvidsson]

Created attachment 170723 [details]
Patch

[WebKit Review Bot]

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.

[Erik Arvidsson]

(In reply to comment #62)
> 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.

This is not part of my change.

[Antti Koivisto]

(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?

[Adam Barth]

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?

[Erik Arvidsson]

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?

[Antti Koivisto]

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.

[Erik Arvidsson]

(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.

[Ojan Vafai]

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.

[Erik Arvidsson]

Created attachment 170983 [details]
Patch

[Adam Barth]

> 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.

[Ryosuke Niwa]

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.

[Erik Arvidsson]

(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?

[Ojan Vafai]

(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).

[Ryosuke Niwa]

(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.

[Erik Arvidsson]

(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.

[Ryosuke Niwa]

(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

[Ryosuke Niwa]

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.

[Erik Arvidsson]

(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.

[Anne van Kesteren]

This is no longer a thing in the standard.

[Antti Koivisto]

Too much Stop Energy.

[Anne van Kesteren]

I'm glad Pepperidge Farm remembers. <3