Closed Bug 1398354 Opened 7 years ago Closed 6 years ago

Update document.all/HTMLAllCollection legacy caller

Categories

(Core :: DOM: Core & HTML, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: d, Assigned: bzbarsky)

References

Details

(Keywords: dev-doc-complete)

Attachments

(3 files)

The spec changed a while back to some reasonable intersection semantics, I believe in https://github.com/whatwg/html/commit/93cc395912de3a9824fdf35e8d9395f7ddb7c003 . While making an editorial change recently ( https://github.com/whatwg/html/commit/0679448c59cb4cd69019be1e1f35fee1bcbc9c66 ), we added tests for that, which Gecko fails some of: - https://github.com/w3c/web-platform-tests/pull/7254 - http://w3c-test.org/html/infrastructure/common-dom-interfaces/collections/htmlallcollection.html The spec should be implementable by just updating the IDL and continuing to use legacycaller, even though the spec no longer does. https://html.spec.whatwg.org/multipage/common-dom-interfaces.html#htmlallcollection
Thanks (as always) for a great bug report. We'll try to get to this some time soon.
Priority: -- → P2
Adding ddn, just in case anything needs updating on our end.
Keywords: dev-doc-needed
There are a few legacy caller tests that Firefox fails, but no other browser fails: legacy caller with "array index property name" legacy caller with "array index property name" as number legacy caller with no argument I'm not sure what the first two are, but at least the final one seems to involve behavior that was mentioned in the pull request in comment 0.
I believe to match the spec we need to do a few things: 1) Implement the new spec semantics for HTMLAllCollection.item. Those are pretty annoying, of course; they involve converting numbers to strings and then back. :( 2) Make the legacycaller just be item(). We could fix that third test mentioned in comment 3 by making the legacycaller arg optional now, I guess. But we might as well just do the whole silly thing. :(
Do we have JSAPI for testing https://tc39.github.io/ecma262/#array-index with (char16_t,length) pair or some equivalent? Or even with a JS::Value?
Flags: needinfo?(jwalden+bmo)
Flags: needinfo?(jwalden+bmo)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Comment on attachment 9017820 [details] [diff] [review] part 1. Expose StringIsArrayIndex taking a char pointer in jsfriendapi Review of attachment 9017820 [details] [diff] [review]: ----------------------------------------------------------------- Add a jsapi-test with obvious simple passing cases and failing cases in the obvious extreme and edge-case ways. ::: js/src/builtin/Array.cpp @@ +265,5 @@ > { > AutoCheckCannotGC nogc; > return str->hasLatin1Chars() > + ? ::StringIsArrayIndexHelper(str->latin1Chars(nogc), str->length(), indexp) > + : ::StringIsArrayIndexHelper(str->twoByteChars(nogc), str->length(), indexp); Get rid of the :: qualifications now that they're unnecessary. @@ +278,5 @@ > + > +JS_FRIEND_API(bool) > +js::StringIsArrayIndex(const char* str, uint32_t length, uint32_t* indexp) > +{ > + AutoCheckCannotGC nogc; Don't need the nogc in these.
Attachment #9017820 - Flags: review?(jwalden+bmo) → review+
Attachment #9017821 - Flags: review?(continuation) → review+
Attached patch jsapi testsSplinter Review
Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/8e41c385edb9 part 1. Expose StringIsArrayIndex taking a char pointer in jsfriendapi. r=waldo https://hg.mozilla.org/integration/mozilla-inbound/rev/344b7e258ea5 part 2. Update document.all item() and legacycaller to new spec semantics. r=mccr8
Backed out 2 changesets (Bug 1398354) for spidermonkey and hazard bustages at js/src/jsapi-tests/testStringIsArrayIndex.cpp Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/a42a9fc0c5f4bc801873a3803ffefbe0761c9b59 Failure push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=344b7e258ea5d73fdd919a6ed6459189874c3533 Failure log: Spidermonkey: https://treeherder.mozilla.org/logviewer.html#?job_id=206277319&repo=mozilla-inbound&lineNumber=2683 Hazard: https://treeherder.mozilla.org/logviewer.html#?job_id=206277276&repo=mozilla-inbound&lineNumber=3121 Build bustage: https://treeherder.mozilla.org/logviewer.html#?job_id=206277248&repo=mozilla-inbound&lineNumber=33947 task 2018-10-18T03:17:16.650Z] --- check_spidermonkey_style.py expected output [task 2018-10-18T03:17:16.650Z] +++ check_spidermonkey_style.py actual output [task 2018-10-18T03:17:16.650Z] @@ -1,3 +1,6 @@ [task 2018-10-18T03:17:16.650Z] +js/src/jsapi-tests/testStringIsArrayIndex.cpp:10:11: error: [task 2018-10-18T03:17:16.650Z] + "jsapi-tests/tests.h" should be included after <stdio.h> [task 2018-10-18T03:17:16.650Z] + [task 2018-10-18T03:17:16.650Z] js/src/tests/style/BadIncludes.h:3: error: [task 2018-10-18T03:17:16.650Z] the file includes itself [task 2018-10-18T03:17:16.650Z] [task 2018-10-18T03:17:16.650Z] TEST-UNEXPECTED-FAIL | check_spidermonkey_style.py | actual output does not match expected output; diff is above. [task 2018-10-18T03:17:16.650Z] TEST-UNEXPECTED-FAIL | check_spidermonkey_style.py | Hint: If the problem is that you renamed a header, and many #includes are no longer in alphabetical order, commit your work and then try `check_spidermonkey_style.py --fixup`. You need to commit first because --fixup modifies your files in place. [task 2018-10-18T03:17:16.654Z] Traceback (most recent call last): [task 2018-10-18T03:17:16.654Z] File "/usr/lib/python2.7/runpy.py", line 162, in _run_module_as_main [task 2018-10-18T03:17:16.654Z] "__main__", fname, loader, pkg_name) [task 2018-10-18T03:17:16.654Z] File "/usr/lib/python2.7/runpy.py", line 72, in _run_code [task 2018-10-18T03:17:16.654Z] exec code in run_globals [task 2018-10-18T03:17:16.654Z] File "/builds/worker/workspace/build/src/python/mozbuild/mozbuild/action/file_generate.py", line 110, in <module> [task 2018-10-18T03:17:16.654Z] sys.exit(main(sys.argv[1:])) [task 2018-10-18T03:17:16.654Z] File "/builds/worker/workspace/build/src/python/mozbuild/mozbuild/action/file_generate.py", line 70, in main [task 2018-10-18T03:17:16.654Z] ret = module.__dict__[method](output, *args.additional_arguments, **kwargs) [task 2018-10-18T03:17:16.654Z] File "/builds/worker/workspace/build/src/config/run_spidermonkey_checks.py", line 15, in main [task 2018-10-18T03:17:16.654Z] raise Exception(script + " failed") [task 2018-10-18T03:17:16.654Z] Exception: /builds/worker/workspace/build/src/config/check_spidermonkey_style.py failed [task 2018-10-18T03:17:16.660Z] backend.mk:20: recipe for target '.deps/spidermonkey_checks.stub' failed [task 2018-10-18T03:17:16.660Z] make[3]: *** [.deps/spidermonkey_checks.stub] Error 1 [task 2018-10-18T03:17:16.660Z] make[3]: Leaving directory '/builds/worker/workspace/build/src/obj-spider/js/src/build' [task 2018-10-18T03:17:16.660Z] /builds/worker/workspace/build/src/config/recurse.mk:101: recipe for target 'js/src/build/misc' failed [task 2018-10-18T03:17:16.660Z] make[2]: *** [js/src/build/misc] Error 2 [task 2018-10-18T03:17:16.660Z] make[2]: Leaving directory '/builds/worker/workspace/build/src/obj-spider' [task 2018-10-18T03:17:16.661Z] /builds/worker/workspace/build/src/config/recurse.mk:32: recipe for target 'misc' failed [task 2018-10-18T03:17:16.661Z] make[1]: *** [misc] Error 2 [task 2018-10-18T03:17:16.661Z] make[1]: Leaving directory '/builds/worker/workspace/build/src/obj-spider' [task 2018-10-18T03:17:16.661Z] /builds/worker/workspace/build/src/config/rules.mk:431: recipe for target 'default' failed [task 2018-10-18T03:17:16.661Z] make: *** [default] Error 2
Flags: needinfo?(bzbarsky)
Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b7142afba712 part 1. Expose StringIsArrayIndex taking a char pointer in jsfriendapi. r=waldo https://hg.mozilla.org/integration/mozilla-inbound/rev/59e0564b7435 part 2. Update document.all item() and legacycaller to new spec semantics. r=mccr8
That's what I get for adding a test after the try run.
Flags: needinfo?(bzbarsky)
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Note to docs team: I've added a note to the Fx 64 rel notes: https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/64#APIs I'm not convinced we really need to bother documenting this (we don't currently), but I could be convinced otherwise.
I'm leaving this; let me know if you think we need documentation on this.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: