1465478 - Element::FromNode should exist.

Status: RESOLVED FIXED

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

Description

[Emilio Cobos Álvarez (:emilio)]

I missed this in a couple of the latest patches I've done.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1465478: Introduce Element::FromNode.

And use it in a couple places I noticed.

Review commit: https://reviewboard.mozilla.org/r/247942/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/247942/

Comment 2

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

Comment on attachment 8981914 [details]
Bug 1465478: Introduce Element::FromNode.

https://reviewboard.mozilla.org/r/247942/#review254172

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);

ditto.
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.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6417e4379b41
Introduce Element::FromNode. r=smaug

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0bd6e3f079f1
followup: Fix an error while addressing review comments. r=me on a CLOSED TREE

Comment 6

[Emilio Cobos Álvarez (:emilio)]

(In reply to Pulsebot from comment #5)
> Pushed by [e-mail]:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/0bd6e3f079f1
> followup: Fix an error while addressing review comments. r=me on a CLOSED
> TREE

I definitely prefer auto* to how that line ended up too, fwiw :)

Comment 7

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/6417e4379b41
https://hg.mozilla.org/mozilla-central/rev/0bd6e3f079f1