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

RESOLVED FIXED in mozilla21

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

unspecified
mozilla21
x86
Mac OS X
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

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?
Depends on: 835219
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.
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.
(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.
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.
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.
Summary: Make Element::GetAttr non-virtual → Make Element::GetAttr/HasAttr/AttrValueIs non-virtual
Created attachment 708173 [details] [diff] [review]
part 1.  Make Element::GetAttr non-virtual.
Attachment #708173 - Flags: review?(bugs)
Created attachment 708174 [details] [diff] [review]
part 2.  Make Element::GetAttr non-virtual.
Attachment #708174 - Flags: review?(bugs)
Created attachment 708176 [details] [diff] [review]
part 3.  Make Element::AttrValueIs non-virtual.
Attachment #708176 - Flags: review?(bugs)
Assignee: nobody → bzbarsky
Whiteboard: [need review]
Attachment #708173 - Flags: review?(bugs) → review+
Comment on attachment 708174 [details] [diff] [review]
part 2.  Make Element::GetAttr non-virtual.

This patch is about HasAttr, not GetAttr.
But ok.
Attachment #708174 - Flags: review?(bugs) → review+
Comment on attachment 708176 [details] [diff] [review]
part 3.  Make Element::AttrValueIs non-virtual.

Update IIDs of nsIContent and Element.
Attachment #708176 - Flags: review?(bugs) → review+
> 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.
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.
Non-inline nsIContent::GetAttr sounds ok to me.
Actually, I may be able to do what nsINode::HasAttributes does (non-inline decl, inline impl).  Will try that.
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
Flags: in-testsuite-
Whiteboard: [need review]
Target Milestone: --- → mozilla21
And I had totally forgotten to rev the IIDs.  Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/0b427bfd720d to address that.
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?"
Uh.... this built locally for me on Mac.  What the hell?
OK, so I think I'm going to just fully out-of-line those methods, and screw the performance wins...
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).
https://hg.mozilla.org/mozilla-central/rev/8b4c02455736
https://hg.mozilla.org/mozilla-central/rev/2ea8ee91cea8
https://hg.mozilla.org/mozilla-central/rev/cd110c95073d
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
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>
I believe I could, yes.

Are we happy including Element.h from nsIContent.h like that?
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.
How about nsIContentInlines.h?
> 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.
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
You need to log in before you can comment on or make changes to this bug.