Open
Bug 1389922
Opened 7 years ago
Updated 2 years ago
Align document.all with spec
Categories
(Core :: DOM: Core & HTML, enhancement, P3)
Core
DOM: Core & HTML
Tracking
()
NEW
People
(Reporter: ayg, Unassigned)
Details
Attachments
(3 files)
There are a bunch of small issues in our implementation.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 4•7 years ago
|
||
mozreview-review |
Comment on attachment 8896739 [details] Bug 1389922 - Return new object each time from document.all; https://reviewboard.mozilla.org/r/168028/#review173648 ::: dom/html/HTMLAllCollection.cpp:170 (Diff revision 1) > return; > } > > - nsContentList* docAllList = GetDocumentAllList(aID); > - if (!docAllList) { > + nsCOMPtr<nsIAtom> id = NS_Atomize(aID); > + RefPtr<nsContentList> docAllList = > + new nsContentList(mDocument, DocAllResultMatch, nullptr, nullptr, true, id); This is too slow, I'm pretty sure. ('new' is slow, and nsContentLists are slow) Since I assume the common case is to return Element and not nsContentList, could we keep nsContentLists internally and use them to check if there are more than 1 entries, and if not, just return element? Or do something else which avoids all the very heavy Length() flushing and allocation
Attachment #8896739 -
Flags: review?(bugs) → review-
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8896738 [details] Bug 1389922 - Align HTMLAllCollection.item()/legacy caller with spec; https://reviewboard.mozilla.org/r/168026/#review173652 ::: dom/html/HTMLAllCollection.cpp:76 (Diff revision 1) > + if (aStr[i] == '0' && (i != 0 || aStr.Length() != 1)) { > + // Leading 0, invalid > + return UINT32_MAX; > + } > + uint32_t origVal = val; > + val = val*10 + aStr[i] - '0'; Missing space before and after * ::: dom/html/HTMLAllCollection.cpp:82 (Diff revision 1) > + if (val < origVal) { > + // Overflow, invalid array index > + return UINT32_MAX; > + } > + } > + return val; Am I missing something here, or can you return uin32_t - 1 as val. Per spec that isn't valid. "If i is equal to 2^32 − 1, then return false." (I don't quite understand why the webidl spec has that limitation)
Attachment #8896738 -
Flags: review?(bugs) → review-
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8896737 [details] Bug 1389922 - Update HTMLAllCollection IDL to spec; https://reviewboard.mozilla.org/r/168024/#review173664 ::: testing/web-platform/meta/html/dom/interfaces.html.ini:373 (Diff revision 1) > expected: FAIL > > [HTMLAllCollection interface: calling namedItem(DOMString) on document.all with too few arguments must throw TypeError] > expected: FAIL > > + [HTMLAllCollection interface: document.all must inherit property "item" with the proper type (3)] Ok, so this will be fixed in another commit, so this patch shouldn't land without those fixes.
Attachment #8896737 -
Flags: review?(bugs) → review+
Reporter | ||
Comment 7•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8896737 [details] Bug 1389922 - Update HTMLAllCollection IDL to spec; https://reviewboard.mozilla.org/r/168024/#review173664 > Ok, so this will be fixed in another commit, so this patch shouldn't land without those fixes. This is a bug in the test, we behave correctly. I submitted a PR upstream to fix the test bug, so this expected fail will go away (along with a bunch of others in this file) when the PR is accepted and we pick up the change. I don't want to commit the test fix on the Mozilla side because I want it to be reviewed upstream, because my patch changes testharness.js and idlharness.js and I'm not sure that's the best way to do it. So I think adding an expected fail for now is correct.
Reporter | ||
Comment 8•7 years ago
|
||
mozreview-review |
Comment on attachment 8896738 [details] Bug 1389922 - Align HTMLAllCollection.item()/legacy caller with spec; https://reviewboard.mozilla.org/r/168026/#review173962 ::: dom/html/HTMLAllCollection.cpp:82 (Diff revision 1) > + if (val < origVal) { > + // Overflow, invalid array index > + return UINT32_MAX; > + } > + } > + return val; This function returns UINT32_MAX if the value is not valid, so no special check is needed. If it's UINT32_MAX, it's invalid and we'll return UINT32_MAX, which the caller will interpret as signaling an invalid value. I also don't understand this restriction. A comment in the spec would be nice.
Reporter | ||
Comment 9•7 years ago
|
||
mozreview-review |
Comment on attachment 8896738 [details] Bug 1389922 - Align HTMLAllCollection.item()/legacy caller with spec; https://reviewboard.mozilla.org/r/168026/#review173964 ::: dom/html/HTMLAllCollection.cpp:82 (Diff revision 1) > + if (val < origVal) { > + // Overflow, invalid array index > + return UINT32_MAX; > + } > + } > + return val; I filed an issue on WebIDL to explain the 2^32 - 1 thing, incidentally: https://github.com/heycam/webidl/issues/409
Reporter | ||
Updated•7 years ago
|
Attachment #8896738 -
Flags: review- → review?(bugs)
Comment 10•7 years ago
|
||
mozreview-review |
Comment on attachment 8896738 [details] Bug 1389922 - Align HTMLAllCollection.item()/legacy caller with spec; https://reviewboard.mozilla.org/r/168026/#review173972 I think the webidl spec issue should be fixed first, or clarified. ::: dom/html/HTMLAllCollection.cpp:82 (Diff revision 1) > + if (val < origVal) { > + // Overflow, invalid array index > + return UINT32_MAX; > + } > + } > + return val; I have no idea why the issue has been dropped.
Attachment #8896738 -
Flags: review?(bugs)
Reporter | ||
Comment 11•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8896738 [details] Bug 1389922 - Align HTMLAllCollection.item()/legacy caller with spec; https://reviewboard.mozilla.org/r/168026/#review173972 In the issue, it's explained that JavaScript arrays have a maximum length of 2^32 - 1, so the maximum index is 2^32 - 2. I don't know why an array can't have 2^32 elements, but it can't, apparently. > I have no idea why the issue has been dropped. I don't know how to explain more clearly. UINT32_MAX is invalid, so what would you like me to write? if (val == UINT32_MAX) { return UINT32_MAX; } return val; This is the same as just "return val;", which is what I have.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 16•7 years ago
|
||
mozreview-review |
Comment on attachment 8896739 [details] Bug 1389922 - Return new object each time from document.all; https://reviewboard.mozilla.org/r/168028/#review173986 ::: dom/html/HTMLAllCollection.cpp:197 (Diff revision 2) > if (docAllList->Item(1, true)) { > aFound = true; > - aResult.SetValue().SetAsHTMLCollection() = docAllList; > + // We need to return a different value for each call per spec > + nsCOMPtr<nsIAtom> id = NS_Atomize(aID); > + aResult.SetValue().SetAsHTMLCollection() = > + new nsContentList(mDocument, DocAllResultMatch, nullptr, nullptr, true, I thought we could maybe avoid the object creation here if docAllList's refcount is 1, but I don't know if that's a good idea in practice.
Comment 17•7 years ago
|
||
Ahaa, UINT32_MAX isn't 2^32, but 2^32 - 1, silly me. But please add a comment there, so that whoever is reading the code and webidl spec can easily see the mapping.
Reporter | ||
Comment 18•7 years ago
|
||
I added a comment in the latest version, is that good? If not, please let me know what sort of comment you want.
Reporter | ||
Comment 19•7 years ago
|
||
Hopefully someone else will be willing to pick this up.
Assignee: ayg → nobody
Status: ASSIGNED → NEW
Updated•7 years ago
|
Priority: -- → P3
Assignee | ||
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•