Closed Bug 1465478 Opened 2 years ago Closed 2 years ago

Element::FromNode should exist.


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

Not set



Tracking Status
firefox62 --- fixed


(Reporter: emilio, Assigned: emilio)


(Blocks 1 open bug)



(1 file)

I missed this in a couple of the latest patches I've done.
Comment on attachment 8981914 [details]
Bug 1465478: Introduce Element::FromNode.

Use proper types, not auto*, then r+

::: accessible/generic/HyperTextAccessible.cpp:337
(Diff revision 1)
>   *     So, if you want to use this for other purpose, you might need to check
>   *     ancestors too.
>   */
>  static nsIContent* GetElementAsContentOf(nsINode* aNode)
>  {
> -  if (aNode->IsElement()) {
> +  if (auto* element = Element::FromNode(aNode)) {

auto doesn't really make the code any easier to read here. But I guess I could live with auto in this case

::: dom/base/nsDocument.cpp:3806
(Diff revision 1)
>  {
>  #ifdef DEBUG
>    for (const nsINode* node = &aSubtreeRoot;
>         node;
>         node = node->GetNextNode(&aSubtreeRoot)) {
> -    if (!node->IsElement()) {
> +    auto* element = Element::FromNode(node);

I wonder why you decided to use auto*.
Use of auto* after all requires more thinking than just using Element*. With auto* one needs to explicitly think what FromNode returns.
Same also elsewhere.
No need to change, but I'd like to understand why to use auto* and not Element*

::: dom/base/nsDocument.cpp:6127
(Diff revision 1)
>    uint32_t length = nodeList->Length();
>    bool universalMatch = aAttrValue.EqualsLiteral("*");
>    for (uint32_t i = 0; i < length; ++i) {
> -    nsIContent* current = nodeList->Item(i);
> +    auto* current = Element::FromNode(nodeList->Item(i));

... or actually. I changed my mind a bit.
I do prefer explicit Element* everywhere.
Use of auto* just reduces the reading speed.
Whoever is checking the type of the variable when reading code later in the method, can't just read
Foo* foo, but needs to read auto* foo = SomeMethodWhichSeemsToReturnObjectOfSomeType.
Attachment #8981914 - Flags: review?(bugs) → review+
To be honest, I disagree with that. I find:

  Element* element = Element::FromNode(mChildren.ChildAt(i - 1));

Harder to read than:

  auto* element = Element::FromNode(mChildren.ChildAt(i - 1));

And longer, too, but that's less of an issue. I find that the type to the left doesn't really say anything that isn't present in the rest of the code.

But anyway, I'll change it, sounds fine.
Pushed by
followup: Fix an error while addressing review comments. r=me on a CLOSED TREE
(In reply to Pulsebot from comment #5)
> Pushed by
> followup: Fix an error while addressing review comments. r=me on a CLOSED

I definitely prefer auto* to how that line ended up too, fwiw :)
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.