Bug 159020 – JSDOMIterator forEach should support second optional parameter

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[youenn fablet]

The second optional parameter should be the thisValue when calling the callback.

[youenn fablet]

Created attachment 281830 [details]
Patch

[Chris Dumez]

I am working on confirming that this fixes yahoo.jp.

[youenn fablet]

Thanks!

[Chris Dumez]

Hmm, yahoo.co.jp is still broken with this patch but the JS error is different:

[Error] TypeError: undefined is not an object (evaluating 'o.removeClass')
	(anonymous function) (mtop-page-top.js:662:1513)
	f (mtop-page-top.js:14:886)

[youenn fablet]

(In reply to comment #4)
> Hmm, yahoo.co.jp is still broken with this patch but the JS error is
> different:
> 
> [Error] TypeError: undefined is not an object (evaluating 'o.removeClass')
> 	(anonymous function) (mtop-page-top.js:662:1513)
> 	f (mtop-page-top.js:14:886)

Do you have a link to mtop-page-top.js?

[Chris Dumez]

(In reply to comment #5)
> (In reply to comment #4)
> > Hmm, yahoo.co.jp is still broken with this patch but the JS error is
> > different:
> > 
> > [Error] TypeError: undefined is not an object (evaluating 'o.removeClass')
> > 	(anonymous function) (mtop-page-top.js:662:1513)
> > 	f (mtop-page-top.js:14:886)
> 
> Do you have a link to mtop-page-top.js?

Full URL:
http://i.yimg.jp/images/mtop/4.5.38/js/standalone/mtop-page-top.js

Note that commenting out the "iterable<Node>;" in NodeList.idl does not seem to help either :/

[Chris Dumez]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > Hmm, yahoo.co.jp is still broken with this patch but the JS error is
> > > different:
> > > 
> > > [Error] TypeError: undefined is not an object (evaluating 'o.removeClass')
> > > 	(anonymous function) (mtop-page-top.js:662:1513)
> > > 	f (mtop-page-top.js:14:886)
> > 
> > Do you have a link to mtop-page-top.js?
> 
> Full URL:
> http://i.yimg.jp/images/mtop/4.5.38/js/standalone/mtop-page-top.js
> 
> Note that commenting out the "iterable<Node>;" in NodeList.idl does not seem
> to help either :/

Commenting out "iterable<FontFace>;" on FontFaceSet does not unbreak yahoo.co.jp either :( So I believe I have commented out all the iterables and the site is still broken.

It is possible that:
1. There are several bugs and the iterable one hid another one that landed later on.
2. The patch for iterable support changed the behavior of code not strictly related to iterable

[youenn fablet]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > Hmm, yahoo.co.jp is still broken with this patch but the JS error is
> > > different:
> > > 
> > > [Error] TypeError: undefined is not an object (evaluating 'o.removeClass')
> > > 	(anonymous function) (mtop-page-top.js:662:1513)
> > > 	f (mtop-page-top.js:14:886)
> > 
> > Do you have a link to mtop-page-top.js?
> 
> Full URL:
> http://i.yimg.jp/images/mtop/4.5.38/js/standalone/mtop-page-top.js
> 
> Note that commenting out the "iterable<Node>;" in NodeList.idl does not seem
> to help either :/

Is the page working if only forEach property is removed from JSNodeListPrototypeTableValues in JSNodeList.cpp?

[Chris Dumez]

(In reply to comment #8)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > (In reply to comment #4)
> > > > Hmm, yahoo.co.jp is still broken with this patch but the JS error is
> > > > different:
> > > > 
> > > > [Error] TypeError: undefined is not an object (evaluating 'o.removeClass')
> > > > 	(anonymous function) (mtop-page-top.js:662:1513)
> > > > 	f (mtop-page-top.js:14:886)
> > > 
> > > Do you have a link to mtop-page-top.js?
> > 
> > Full URL:
> > http://i.yimg.jp/images/mtop/4.5.38/js/standalone/mtop-page-top.js
> > 
> > Note that commenting out the "iterable<Node>;" in NodeList.idl does not seem
> > to help either :/
> 
> Is the page working if only forEach property is removed from
> JSNodeListPrototypeTableValues in JSNodeList.cpp?

Not sure I understand the question, I commented out the whole "iterable<Node>;" in NodeList so there is no forEach property in JSNodeList.cpp already.

[youenn fablet]

> Not sure I understand the question, I commented out the whole
> "iterable<Node>;" in NodeList so there is no forEach property in
> JSNodeList.cpp already.

iterable<Node> also triggers methods like keys, values which may or may not be used in the code.

[Chris Dumez]

(In reply to comment #10)
> > Not sure I understand the question, I commented out the whole
> > "iterable<Node>;" in NodeList so there is no forEach property in
> > JSNodeList.cpp already.
> 
> iterable<Node> also triggers methods like keys, values which may or may not
> be used in the code.

Hmm, ok but the site was working before <http://trac.webkit.org/changeset/200619> where you added iterable<> on NodeList.

[youenn fablet]

(In reply to comment #11)
> (In reply to comment #10)
> > > Not sure I understand the question, I commented out the whole
> > > "iterable<Node>;" in NodeList so there is no forEach property in
> > > JSNodeList.cpp already.
> > 
> > iterable<Node> also triggers methods like keys, values which may or may not
> > be used in the code.
> 
> Hmm, ok but the site was working before
> <http://trac.webkit.org/changeset/200619> where you added iterable<> on
> NodeList.

Can you describe what means "the site is working" vs "the site is not working"?
That might help me trying to investigate further the issue on my local machine.

[Chris Dumez]

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > > Not sure I understand the question, I commented out the whole
> > > > "iterable<Node>;" in NodeList so there is no forEach property in
> > > > JSNodeList.cpp already.
> > > 
> > > iterable<Node> also triggers methods like keys, values which may or may not
> > > be used in the code.
> > 
> > Hmm, ok but the site was working before
> > <http://trac.webkit.org/changeset/200619> where you added iterable<> on
> > NodeList.
> 
> Can you describe what means "the site is working" vs "the site is not
> working"?
> That might help me trying to investigate further the issue on my local
> machine.

This was explained on the other bug. The site content at the bottom of the page no longer loads, we just see a spinner. You may be able to reproduce in a simulator.

[Chris Dumez]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #6)
> > > (In reply to comment #5)
> > > > (In reply to comment #4)
> > > > > Hmm, yahoo.co.jp is still broken with this patch but the JS error is
> > > > > different:
> > > > > 
> > > > > [Error] TypeError: undefined is not an object (evaluating 'o.removeClass')
> > > > > 	(anonymous function) (mtop-page-top.js:662:1513)
> > > > > 	f (mtop-page-top.js:14:886)
> > > > 
> > > > Do you have a link to mtop-page-top.js?
> > > 
> > > Full URL:
> > > http://i.yimg.jp/images/mtop/4.5.38/js/standalone/mtop-page-top.js
> > > 
> > > Note that commenting out the "iterable<Node>;" in NodeList.idl does not seem
> > > to help either :/
> > 
> > Is the page working if only forEach property is removed from
> > JSNodeListPrototypeTableValues in JSNodeList.cpp?
> 
> Not sure I understand the question, I commented out the whole
> "iterable<Node>;" in NodeList so there is no forEach property in
> JSNodeList.cpp already.

I tried dropping just the forEach property and it did not help.

[youenn fablet]

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > (In reply to comment #10)
> > > > > Not sure I understand the question, I commented out the whole
> > > > > "iterable<Node>;" in NodeList so there is no forEach property in
> > > > > JSNodeList.cpp already.
> > > > 
> > > > iterable<Node> also triggers methods like keys, values which may or may not
> > > > be used in the code.
> > > 
> > > Hmm, ok but the site was working before
> > > <http://trac.webkit.org/changeset/200619> where you added iterable<> on
> > > NodeList.
> > 
> > Can you describe what means "the site is working" vs "the site is not
> > working"?
> > That might help me trying to investigate further the issue on my local
> > machine.
> 
> This was explained on the other bug. The site content at the bottom of the
> page no longer loads, we just see a spinner. You may be able to reproduce in
> a simulator.

I tried on mini browser this morning but was not sure what to search for.
I will try again.

[Chris Dumez]

(In reply to comment #15)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > (In reply to comment #11)
> > > > (In reply to comment #10)
> > > > > > Not sure I understand the question, I commented out the whole
> > > > > > "iterable<Node>;" in NodeList so there is no forEach property in
> > > > > > JSNodeList.cpp already.
> > > > > 
> > > > > iterable<Node> also triggers methods like keys, values which may or may not
> > > > > be used in the code.
> > > > 
> > > > Hmm, ok but the site was working before
> > > > <http://trac.webkit.org/changeset/200619> where you added iterable<> on
> > > > NodeList.
> > > 
> > > Can you describe what means "the site is working" vs "the site is not
> > > working"?
> > > That might help me trying to investigate further the issue on my local
> > > machine.
> > 
> > This was explained on the other bug. The site content at the bottom of the
> > page no longer loads, we just see a spinner. You may be able to reproduce in
> > a simulator.
> 
> I tried on mini browser this morning but was not sure what to search for.
> I will try again.

FYI, I am doing more testing. It seems I can no longer reproduce for some reason. Trying to figure out what I changed.

[Chris Dumez]

(In reply to comment #15)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > (In reply to comment #11)
> > > > (In reply to comment #10)
> > > > > > Not sure I understand the question, I commented out the whole
> > > > > > "iterable<Node>;" in NodeList so there is no forEach property in
> > > > > > JSNodeList.cpp already.
> > > > > 
> > > > > iterable<Node> also triggers methods like keys, values which may or may not
> > > > > be used in the code.
> > > > 
> > > > Hmm, ok but the site was working before
> > > > <http://trac.webkit.org/changeset/200619> where you added iterable<> on
> > > > NodeList.
> > > 
> > > Can you describe what means "the site is working" vs "the site is not
> > > working"?
> > > That might help me trying to investigate further the issue on my local
> > > machine.
> > 
> > This was explained on the other bug. The site content at the bottom of the
> > page no longer loads, we just see a spinner. You may be able to reproduce in
> > a simulator.
> 
> I tried on mini browser this morning but was not sure what to search for.
> I will try again.

This is on iOS with the mobile site.

[Chris Dumez]

So I cannot reproduce the bottom of the site not loading anymore. However, I can confirm that the forEach() JS exceptions are present without the patch attached to this bug and that they're gone with the patch.

Therefore, I think yahoo.co.jp does require the support for the second parameter on forEach and we should land your patch.

[Chris Dumez]

Comment on attachment 281830 [details]
Patch

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

> LayoutTests/fast/dom/nodeListIterator.html:51
> +                shouldBe('thisValue', 'window');

Looking at the spec (http://heycam.github.io/webidl/#es-forEach), shouldn't this be undefined rather than window?

[Chris Dumez]

Comment on attachment 281830 [details]
Patch

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

>> LayoutTests/fast/dom/nodeListIterator.html:51
>> +                shouldBe('thisValue', 'window');
> 
> Looking at the spec (http://heycam.github.io/webidl/#es-forEach), shouldn't this be undefined rather than window?

Referring to step 4.4 Invoke callback with thisArg (or undefined, if the argument was not supplied) as the callback this value and value, key and O as its arguments.

[Chris Dumez]

Comment on attachment 281830 [details]
Patch

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

>>> LayoutTests/fast/dom/nodeListIterator.html:51
>>> +                shouldBe('thisValue', 'window');
>> 
>> Looking at the spec (http://heycam.github.io/webidl/#es-forEach), shouldn't this be undefined rather than window?
> 
> Referring to step 4.4 Invoke callback with thisArg (or undefined, if the argument was not supplied) as the callback this value and value, key and O as its arguments.

Hmm, Chrome seems to agree with you though..

[Chris Dumez]

Comment on attachment 281830 [details]
Patch

Nevermind, I am mis-interpreting the spec. You do call JSC::call() with undefined as thisValue in this case.

[Chris Dumez]

Note that on Chrome, a few checks are failing:
FAIL iterator.next().value should be [object HTMLDivElement] (of type object). Was 0 (of type number).
FAIL iterator.next().value should be [object HTMLOListElement] (of type object). Was 1 (of type number).

I have checked yet who is right.

[Chris Dumez]

Comment on attachment 281830 [details]
Patch

Clearing flags on attachment: 281830

Committed r202334: <http://trac.webkit.org/changeset/202334>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.