836050 - Make Element::GetAttr/HasAttr/AttrValueIs non-virtual

Status: RESOLVED FIXED

Description

[Boris Zbarsky [:bzbarsky, bz on IRC]]

There are two ways to do this:

1)  Get rid of nsIContent::GetAttr and fix all consumers to check IsElement() and call AsElement() as needed.

2)  Have a non-virtual GetAttr on nsIContent that checks IsElement() and calls the Element version itself as needed.

Thoughts on which is better?

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I think having a non-virtual GetAttr on both nsIContent and Element might work. Where the one on nsIContent does an IsElement check and then casts and forwards to the Element one.

Comment 2

[Boris Zbarsky [:bzbarsky, bz on IRC]]

I would definitely work.  The question is whether it's better than an API that makes consumers think about whether they should be storing nsIContent or Element.

Comment 3

[Trevor Saunders (:tbsaunde)]

(In reply to Boris Zbarsky (:bz) from comment #2)
> I would definitely work.  The question is whether it's better than an API
> that makes consumers think about whether they should be storing nsIContent
> or Element.

So, I half way suspect a11y is a special case here, but there's a pretty good though somewhat sad reason Accessible::mContent is nsIContent* not Element* which is that TextLeafAccessible needs to be able to point at a text node.  For all other accessibles I iirc mContent is either null or actually an element.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I don't believe that the saved IsElement() check is worth the hassle of the extra source code. In cases where performance does matter, we can always do directly to Element::GetAttr where we can do that faster.

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

I think it would be somewhat nice to put it only on Element, but there's a lot of callers that have an nsIContent, IIRC.

Comment 6

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Attachment: part 1. Make Element::GetAttr non-virtual.

Comment 7

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Attachment: part 2. Make Element::GetAttr non-virtual.

Comment 8

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Attachment: part 3. Make Element::AttrValueIs non-virtual.

Comment 9

[Olli Pettay [:smaug]]

Comment on attachment 708174 [details] [diff] [review]
part 2.  Make Element::GetAttr non-virtual.

This patch is about HasAttr, not GetAttr.
But ok.

Comment 10

[Olli Pettay [:smaug]]

Comment on attachment 708176 [details] [diff] [review]
part 3.  Make Element::AttrValueIs non-virtual.

Update IIDs of nsIContent and Element.

Comment 11

[Boris Zbarsky [:bzbarsky, bz on IRC]]

> Update IIDs of nsIContent and Element.

Good catch, thanks.

I pushed this to try, after building locally, and of course it builds here but not on try.... some followup patches coming up to fix the try redness once I get it all fixed.

Comment 12

[Boris Zbarsky [:bzbarsky, bz on IRC]]

OK, so the problem is that including nsIContent.h but not Element.h warns with this patch about an inline method being used (even though it's not actually being used!) but not defined.

Which in warnings-as-errors directories turns the buildred.

This is being hit by tons of places in the tree.  I can sprinkle Element.h includes, or I could just make the nsIContent version not inline...  Callers who care about performance should be calling the Element version directly.

Comment 13

[Olli Pettay [:smaug]]

Non-inline nsIContent::GetAttr sounds ok to me.

Comment 14

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Actually, I may be able to do what nsINode::HasAttributes does (non-inline decl, inline impl).  Will try that.

Comment 15

[Boris Zbarsky [:bzbarsky, bz on IRC]]

That works.  Pushed:

   https://hg.mozilla.org/integration/mozilla-inbound/rev/79e6231558d7
   https://hg.mozilla.org/integration/mozilla-inbound/rev/67fbd02a0122
   https://hg.mozilla.org/integration/mozilla-inbound/rev/e172a78270e6

Comment 16

[Boris Zbarsky [:bzbarsky, bz on IRC]]

And I had totally forgotten to rev the IIDs.  Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/0b427bfd720d to address that.

Comment 17

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/21cbd89537b5 - both Mac and b2g want to know the same thing, "what is this nsIContent::AttrValueIs of which you speak?"

Comment 18

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Uh.... this built locally for me on Mac.  What the hell?

Comment 19

[Boris Zbarsky [:bzbarsky, bz on IRC]]

OK, so I think I'm going to just fully out-of-line those methods, and screw the performance wins...

Comment 20

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Did that, and relanded:

   https://hg.mozilla.org/integration/mozilla-inbound/rev/8b4c02455736
   https://hg.mozilla.org/integration/mozilla-inbound/rev/2ea8ee91cea8
   https://hg.mozilla.org/integration/mozilla-inbound/rev/cd110c95073d

(iid rev in third changeset now).

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8b4c02455736
https://hg.mozilla.org/mozilla-central/rev/2ea8ee91cea8
https://hg.mozilla.org/mozilla-central/rev/cd110c95073d

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Could you put inline implementations in nsIContent.h if you #include Element.h after the nsIContent declaration?

I.e, make nsIContent.h be


class nsIContent { ... }
#include Element.h
<inline impl>

Comment 23

[Boris Zbarsky [:bzbarsky, bz on IRC]]

I believe I could, yes.

Are we happy including Element.h from nsIContent.h like that?

Comment 24

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I wouldn't mind that no.


What I really want to avoid is people writing code all over which cast nsIContent to Element just to save a few cycles.

Comment 25

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

How about nsIContentInlines.h?

Comment 26

[Boris Zbarsky [:bzbarsky, bz on IRC]]

> How about nsIContentInlines.h?

That has the same issue as comment 12, no?  That being "sprinkle over the whole tree".

Jonas, what we generally want is for people to be using Element when they know they'll have an element.  I agree they shouldn't be randomly casting just because.

Comment 27

[Boris Zbarsky [:bzbarsky, bz on IRC]]

OK, I tried that.  It works ok for things that include nsIContent, but for things that include DocumentFragment.h, we get fail.

The reason for that is that DocumentFragment inherits from FragmentOrElement (and includes it) which inherits from nsIContent (and includes it).  And then we get:

In file included from ../../dist/include/mozilla/dom/DocumentFragment.h:10:
In file included from ../../dist/include/mozilla/dom/FragmentOrElement.h:18:
In file included from ../../dist/include/nsIContent.h:923:
../../dist/include/mozilla/dom/Element.h:127:24: error: expected class name
class Element : public FragmentOrElement