| Summary: | JSDOMIterator forEach should support second optional parameter | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | youenn fablet <youennf> | ||||
| Component: | WebCore JavaScript | Assignee: | youenn fablet <youennf> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | bfulgham, cdumez, jiewen_tan | ||||
| Priority: | P2 | ||||||
| Version: | WebKit Nightly Build | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=131443 | ||||||
| Attachments: |
|
||||||
|
Description
youenn fablet
2016-06-22 02:36:14 PDT
Created attachment 281830 [details]
Patch
I am working on confirming that this fixes yahoo.jp. Thanks! 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) (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? (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 :/ (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 (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? (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. > 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.
(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. (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. (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. (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. (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. (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. (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. 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. 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? 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. 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.. 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.
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. Comment on attachment 281830 [details] Patch Clearing flags on attachment: 281830 Committed r202334: <http://trac.webkit.org/changeset/202334> All reviewed patches have been landed. Closing bug. |