Open Bug 1389922 Opened 5 years ago Updated 3 years ago

Align document.all with spec

Categories

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

enhancement

Tracking

()

People

(Reporter: ayg, Unassigned)

Details

Attachments

(3 files)

There are a bunch of small issues in our implementation.
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 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 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+
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.
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.
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
Attachment #8896738 - Flags: review- → review?(bugs)
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)
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 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.
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.
I added a comment in the latest version, is that good?  If not, please let me know what sort of comment you want.
Hopefully someone else will be willing to pick this up.
Assignee: ayg → nobody
Status: ASSIGNED → NEW
Priority: -- → P3
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.