Closed
Bug 836050
Opened 8 years ago
Closed 8 years ago
Make Element::GetAttr/HasAttr/AttrValueIs non-virtual
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla21
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
Details
Attachments
(3 files)
6.30 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
7.48 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
8.68 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
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?
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.
![]() |
Assignee | |
Comment 2•8 years ago
|
||
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•8 years ago
|
||
(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.
Comment 5•8 years ago
|
||
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.
![]() |
Assignee | |
Updated•8 years ago
|
Summary: Make Element::GetAttr non-virtual → Make Element::GetAttr/HasAttr/AttrValueIs non-virtual
![]() |
Assignee | |
Comment 6•8 years ago
|
||
Attachment #708173 -
Flags: review?(bugs)
![]() |
Assignee | |
Comment 7•8 years ago
|
||
Attachment #708174 -
Flags: review?(bugs)
![]() |
Assignee | |
Comment 8•8 years ago
|
||
Attachment #708176 -
Flags: review?(bugs)
![]() |
Assignee | |
Updated•8 years ago
|
Assignee: nobody → bzbarsky
Whiteboard: [need review]
Updated•8 years ago
|
Attachment #708173 -
Flags: review?(bugs) → review+
Comment 9•8 years ago
|
||
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 10•8 years ago
|
||
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+
![]() |
Assignee | |
Comment 11•8 years ago
|
||
> 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.
![]() |
Assignee | |
Comment 12•8 years ago
|
||
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•8 years ago
|
||
Non-inline nsIContent::GetAttr sounds ok to me.
![]() |
Assignee | |
Comment 14•8 years ago
|
||
Actually, I may be able to do what nsINode::HasAttributes does (non-inline decl, inline impl). Will try that.
![]() |
Assignee | |
Comment 15•8 years ago
|
||
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
![]() |
Assignee | |
Comment 16•8 years ago
|
||
And I had totally forgotten to rev the IIDs. Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/0b427bfd720d to address that.
Comment 17•8 years ago
|
||
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?"
![]() |
Assignee | |
Comment 18•8 years ago
|
||
Uh.... this built locally for me on Mac. What the hell?
![]() |
Assignee | |
Comment 19•8 years ago
|
||
OK, so I think I'm going to just fully out-of-line those methods, and screw the performance wins...
![]() |
Assignee | |
Comment 20•8 years ago
|
||
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•8 years ago
|
||
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
Closed: 8 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>
![]() |
Assignee | |
Comment 23•8 years ago
|
||
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.
Comment 25•8 years ago
|
||
How about nsIContentInlines.h?
![]() |
Assignee | |
Comment 26•8 years ago
|
||
> 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.
![]() |
Assignee | |
Comment 27•8 years ago
|
||
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
Updated•2 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•