Element::FromNode should exist.

RESOLVED FIXED in Firefox 62

Status

()

defect
RESOLVED FIXED
Last year
4 months ago

People

(Reporter: emilio, Assigned: emilio)

Tracking

(Blocks 1 bug)

unspecified
mozilla62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(1 attachment)

I missed this in a couple of the latest patches I've done.
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.
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 ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0bd6e3f079f1
followup: Fix an error while addressing review comments. r=me on a CLOSED TREE
(In reply to Pulsebot from comment #5)
> Pushed by ecoal95@gmail.com:
> 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 :)
https://hg.mozilla.org/mozilla-central/rev/6417e4379b41
https://hg.mozilla.org/mozilla-central/rev/0bd6e3f079f1
Status: NEW → RESOLVED
Closed: Last year
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.