Simplify nsINode::GetElementById

RESOLVED FIXED in Firefox 62

Status

()

enhancement
RESOLVED FIXED
11 months ago
a month ago

People

(Reporter: emilio, Assigned: rahul0379)

Tracking

unspecified
mozilla62
Points:
---

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

11 months ago
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

11 months ago
Not sure what to optimize for ShadowTrees. Document and ShadowRoot use already the same mechanism for ID tracking.
(Reporter)

Comment 2

11 months ago
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
(Reporter)

Updated

11 months ago
No longer blocks: 1426493
(Reporter)

Comment 3

11 months ago
(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

11 months ago
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)
(Reporter)

Comment 5

11 months ago
(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)
(Reporter)

Comment 6

11 months ago
Oh, and ElementHolder of course should be removed in the process.
(Assignee)

Comment 7

11 months ago
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

11 months ago
Ah, my bad, for second point the statement will become

    return FindMatchingElementsWithId(aId, this);
(Reporter)

Comment 9

11 months ago
(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

11 months ago
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

11 months ago
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).
(Reporter)

Comment 12

11 months ago
FindMatchingElementWithId sounds great :).

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

Comment 13

11 months ago
Hi Emilio, I've attached a patch. Please review.
Attachment #8980864 - Flags: review?(emilio)
(Reporter)

Comment 14

11 months ago
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

11 months ago
Added the comment.
Attachment #8980864 - Attachment is obsolete: true
Attachment #8980883 - Flags: review?(emilio)
(Reporter)

Comment 16

11 months ago
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+
(Reporter)

Comment 18

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

Comment 19

11 months ago
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2263ea88eb06
Cleaning up nsINode::GetElementById. r=emilio

Comment 20

11 months ago
The inbound -> central merge didn't mark the bugs as fixed (https://hg.mozilla.org/mozilla-central/rev/6b9076ac236c).
Flags: needinfo?(csabou)

Comment 21

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2263ea88eb06
Status: NEW → RESOLVED
Last Resolved: 11 months ago
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
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.