Simplify nsINode::GetElementById

RESOLVED FIXED in Firefox 62

Status

()

enhancement
RESOLVED FIXED
Last year
3 months ago

People

(Reporter: emilio, Assigned: rahul0379)

Tracking

unspecified
mozilla62
Points:
---

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
Not sure what to optimize for ShadowTrees. Document and ShadowRoot use already the same mechanism for ID tracking.
Err, for some reason I thought they used nsINode::GetElementById, nvm. We can cleanup the nsINode version though.
Summary: Move GetElementByID to DocumentOrShadowRoot. → Simplify nsINode::GetElementById
No longer blocks: 1426493
(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.
Assignee

Comment 4

Last year
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?
Flags: needinfo?(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.
Flags: needinfo?(emilio)
Oh, and ElementHolder of course should be removed in the process.
Assignee

Comment 7

Last year
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).
Flags: needinfo?(emilio)
Assignee

Comment 8

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

    return FindMatchingElementsWithId(aId, this);
(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;
    }
  }
Flags: needinfo?(emilio)
Assignee

Comment 10

Last year
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.
Assignee

Comment 11

Last year
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).
FindMatchingElementWithId sounds great :).

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

Comment 13

Last year
Hi Emilio, I've attached a patch. Please review.
Attachment #8980864 - Flags: review?(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.
Attachment #8980864 - Flags: review?(emilio) → review+
Assignee

Comment 15

Last year
Added the comment.
Attachment #8980864 - Attachment is obsolete: true
Attachment #8980883 - Flags: review?(emilio)
Comment on attachment 8980883 [details] [diff] [review]
bug1464433.patch - Cleanup nsINode::GetElementById

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

LGTM, thanks a lot!
Attachment #8980883 - Flags: review?(emilio) → review+
(Ignore the failures which are because of other failures in my queue).

Comment 19

Last year
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2263ea88eb06
Cleaning up nsINode::GetElementById. r=emilio
The inbound -> central merge didn't mark the bugs as fixed (https://hg.mozilla.org/mozilla-central/rev/6b9076ac236c).
Flags: needinfo?(csabou)

Comment 21

Last year
bugherder
https://hg.mozilla.org/mozilla-central/rev/2263ea88eb06
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
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.
Flags: needinfo?(csabou)
Assignee: nobody → rahul0379
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.