1464433 - Simplify nsINode::GetElementById

Status: RESOLVED FIXED

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

Description

[Emilio Cobos Álvarez (:emilio)]

It's not exposed on other nodes, so it has no reason to live in nsINode. Moving this would make it faster and easier to understand.

There was a time where it needed to live there because QuerySelector used to live there, and they shared FindMatchingElementsWithId, but that's no longer the case.

The setup here can be simplified, and afterwards we can optimize GetElementById in shadow trees.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Not sure what to optimize for ShadowTrees. Document and ShadowRoot use already the same mechanism for ID tracking.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Err, for some reason I thought they used nsINode::GetElementById, nvm. We can cleanup the nsINode version though.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #2)
> Err, for some reason I thought they used nsINode::GetElementById, nvm. We
> can cleanup the nsINode version though.

And that version _can_ be optimized for elements in shadow trees, though looking at stuff I guess it only matters for <svg> elements.

Comment 4

[rahul0379]

Hi Emilio,

As I understand, GetElementById is not needed in nsINode(is this a file?)
I don't understand what shadow trees are. And what we need to optimize.
Can you name the files we're working with here?

Comment 5

[Emilio Cobos Álvarez (:emilio)]

(In reply to rahul0379 from comment #4)
> Hi Emilio,
> 
> As I understand, GetElementById is not needed in nsINode(is this a file?)
> I don't understand what shadow trees are. And what we need to optimize.
> Can you name the files we're working with here?

So, it is needed, because it's reused from DocumentFragment and SVGSVGElement. We can cleanup a bit of it though.

The code lives here: https://searchfox.org/mozilla-central/rev/bf4def01bf8f6ff0d18f02f2d7e9efc73e12c63f/dom/base/nsINode.cpp#2524

That's the only caller of FindMatchingElementsWithId, and it passes some constant arguments, so that can be simplified:

 * The 'bool onlyFirstMatch' template parameter of that function can be removed.
 * The T& parameter could be converted to ElementHolder, but it's easier to just make that function return 'Element*', since we're never going to return more than one now.
 * The argument 'SelectorMatchInfo* aMatchInfo' can also be removed, since it's always null.

With that, the function would end up looking:

static Element*
FindMatchingElementWithId(const nsAString& aId, nsINode* aRoot)
{
  // ...
}

After that, we can handle shadow trees, but that can be another bug.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Oh, and ElementHolder of course should be removed in the process.

Comment 7

[rahul0379]

1. ElementHolder is being used only by 'GetElementByID'(in /dom/base at least). Should we remove the whole struct itself?

2. Since FindMatchingElementWithId is now returning Element*, the statement in GetElementById will simply become,

    return FindMatchingElementsWithId(aId, this, nullptr, holder);

3. `Element* mElement` will initialize an empty element right?
Because I'm simply returning it if `elements` is empty:

  Element* mElement;
  const nsTArray<Element*>* elements = aRoot->OwnerDoc()->GetAllElementsForId(aId);
  if (!elements) {
    // Nothing to do; we're done
    return mElement;
  }

4. Also, since `onlyFirstMatch` was always true, we'll be returning only the first match that we find(if any).

Comment 8

[rahul0379]

Ah, my bad, for second point the statement will become

    return FindMatchingElementsWithId(aId, this);

Comment 9

[Emilio Cobos Álvarez (:emilio)]

(In reply to rahul0379 from comment #7)
> 1. ElementHolder is being used only by 'GetElementByID'(in /dom/base at
> least). Should we remove the whole struct itself?

Yes, it can be removed.

> 2. Since FindMatchingElementWithId is now returning Element*, the statement
> in GetElementById will simply become,
> 
>     return FindMatchingElementsWithId(aId, this, nullptr, holder);

As you pointed out it should become FindMatchingElementsWithId(aId, this);

But since that function will only return one element, we should rename it to FindMatchingElementWithId as well. That can be a different patch.

> 3. `Element* mElement` will initialize an empty element right?
> Because I'm simply returning it if `elements` is empty:
> 
>   Element* mElement;
>   const nsTArray<Element*>* elements =
> aRoot->OwnerDoc()->GetAllElementsForId(aId);
>   if (!elements) {
>     // Nothing to do; we're done
>     return mElement;
>   }

No, that's wrong, that's returning an uninitialized pointer which is wrong, you need to return null:

  const nsTArray<Element*>* elements = aRoot->OwnerDoc()->GetAllElementsForId(aId);
  if (!elements) {
    // Nothing to do; we're done
    return nullptr;
  }

> 4. Also, since `onlyFirstMatch` was always true, we'll be returning only the first match that we find(if any).

Right, you'd need to turn the loop into something like:

  for (Element* element : *elements) {
    if (!aRoot->IsElement() ||
        (element != aRoot &&
           nsContentUtils::ContentIsDescendantOf(element, aRoot))) {
      return element;
    }
  }

Comment 10

[rahul0379]

I've changed the name to `FindMatchingElementWithId`, if that is fine. If you say, I can rename it back to 'FindMatchingElementsWithId' and submit a patch.

Comment 11

[rahul0379]

Also, I was thinking whether the empty check for `elements`(in 3rd point) is really necessary. But that depends on the return type of aRoot->OwnerDoc()->GetAllElementsForId(aId).

Comment 12

[Emilio Cobos Álvarez (:emilio)]

FindMatchingElementWithId sounds great :).

And yes, the null-check of `elements` is necessary, because that function can and does return null.

Comment 13

[rahul0379]

Attachment: bug1464433.patch - Cleaning nsINode::GetElementById

Hi Emilio, I've attached a patch. Please review.

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8980864 [details] [diff] [review]
bug1464433.patch - Cleaning nsINode::GetElementById

Review of attachment 8980864 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great!

::: dom/base/nsINode.cpp
@@ +2436,5 @@
>  };
>  } // namespace
>  
> +// Given an id, find first element with that id under aRoot.
> +// If none found, return nullptr.

Please keep the comment about aRoot needing to be in the document, for now.

Comment 15

[rahul0379]

Attachment: bug1464433.patch - Cleanup nsINode::GetElementById

Added the comment.

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8980883 [details] [diff] [review]
bug1464433.patch - Cleanup nsINode::GetElementById

Review of attachment 8980883 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, thanks a lot!

Comment 17

[Emilio Cobos Álvarez (:emilio)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=98def9a7c9801b757525ba8c92641dec9d4830b7

Comment 18

[Emilio Cobos Álvarez (:emilio)]

(Ignore the failures which are because of other failures in my queue).

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2263ea88eb06
Cleaning up nsINode::GetElementById. r=emilio

Comment 20

[Guilherme Lima]

The inbound -> central merge didn't mark the bugs as fixed (https://hg.mozilla.org/mozilla-central/rev/6b9076ac236c).

Comment 21

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/2263ea88eb06

Comment 22

[Cosmin Sabou [:CosminS]]

Sorry about that Gullherme. It seems I didn't run Bugherder for the inbound to central merge and two times for the autoland one. Will double check in the future. Thanks for pointing this out.