Closed Bug 764400 Opened 8 years ago Closed 8 years ago

Move various nsIContent methods to nsINode

Categories

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

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: ayg, Assigned: ayg)

Details

Attachments

(3 files)

I regularly find myself wanting to use Tag() on an nsINode.  I figure that if I'm moving that, I can move a bunch of other mNodeInfo-based methods too, because why not.  These will all crash if you call them on an uninitialized document, but so will NodeName etc., so fair's fair.
Comment on attachment 632708 [details] [diff] [review]
Patch part 1, v1 -- Move various nsIContent methods to nsINode

Review of attachment 632708 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/public/nsINode.h
@@ +509,5 @@
> +  {
> +    return mNodeInfo;
> +  }
> +
> +  inline bool IsInNamespace(PRInt32 aNamespace) const {

While you're here, can you move the { to the next line for these, and remove the 'inline'? (That's implicit if you define them like this.)
Comment on attachment 632708 [details] [diff] [review]
Patch part 1, v1 -- Move various nsIContent methods to nsINode

Review of attachment 632708 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the nits Ms2ger suggested.
Attachment #632708 - Flags: review?(mounir) → review+
Comment on attachment 632712 [details] [diff] [review]
Patch part 2, v1 -- Improve Tag() comment, fix whitespace

Review of attachment 632712 [details] [diff] [review]:
-----------------------------------------------------------------

Please merge this with the previous patch when pushing, there is no need to split them.
Attachment #632712 - Flags: review?(mounir) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2a44838954a
Flags: in-testsuite-
Target Milestone: --- → mozilla16
This doesn't all make sense IMO, especially putting IsHTML/IsSVG/IsXUL/IsMathML on documents.
And won't|((nsINode*)doc)->IsHTML()| now return a different response from |((nsIDocument*)doc)->IsHTML()| :-/.
(In reply to Peter Van der Beken [:peterv] from comment #7)
> This doesn't all make sense IMO, especially putting
> IsHTML/IsSVG/IsXUL/IsMathML on documents.

Well, they'll just always return false.  I guess you're right that this is unintuitive, though, because you'd expect them to mean something else for documents.  I think something like IsLink would be fine on nsINode instead of nsIContent even though it will always return false for documents, because it saves you from having to check that the node is content.  But not with this implementation of IsHTML etc., you're right.

(In reply to Peter Van der Beken [:peterv] from comment #8)
> And won't|((nsINode*)doc)->IsHTML()| now return a different response from
> |((nsIDocument*)doc)->IsHTML()| :-/.

Yes, you're right.  Quite possibly that was a bad idea.

I'm all for moving those back if there are objections.  I really just wanted Tag(), TBH.  Maybe I should have only moved that to start with.
How could this have caused performance regressions?  What did it do that would cause the compiler to generate any different code than it would have otherwise?
(In reply to Aryeh Gregor from comment #11)
> How could this have caused performance regressions?  What did it do that
> would cause the compiler to generate any different code than it would have
> otherwise?

You removed "inline".
https://hg.mozilla.org/mozilla-central/rev/e2a44838954a
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
(In reply to Mike Hommey [:glandium] from comment #12)
> You removed "inline".

(In reply to :Ms2ger from comment #3)
> While you're here, can you move the { to the next line for these, and remove
> the 'inline'? (That's implicit if you define them like this.)

Are some compilers not following the spec here or something?
(In reply to Aryeh Gregor from comment #14)
> (In reply to Mike Hommey [:glandium] from comment #12)
> > You removed "inline".
> 
> (In reply to :Ms2ger from comment #3)
> > While you're here, can you move the { to the next line for these, and remove
> > the 'inline'? (That's implicit if you define them like this.)
> 
> Are some compilers not following the spec here or something?

As mentioned on irc, maybe it's just moving stuff around that makes gcc not inline some instances.
FTR: I'm happy to write/submit/push any partial backout patches people want (except that I want to keep Tag() on nsINode).  But I don't have a strong opinion here on what exactly to do and don't think it's my call to make.  Mounir reviewed the patch, so for my part, I won't write any backouts without his agreement.  (Except if peterv or jst wanted me to do it without asking him, I guess, since they're owners rather than peers.)

If it turns out gcc is really confused by moving methods around, maybe we could trick it by redeclaring the same method(s) on nsIContent.
I'd rather see the Is* methods moved back to nsIContent, to avoid the confusion I pointed out in comment 7 and comment 8.
For example, I worry about people writing

if (node->NodeType() == nsIDOMNode::DOCUMENT_NODE && node->IsHTML())

and not realizing that this will always be false.
I should have seen that during the review... Aryeh, could you backout the patch and attach another one only moving Tag()?
(In reply to Mounir Lamouri (:mounir) from comment #19)
> I should have seen that during the review... Aryeh, could you backout the
> patch and attach another one only moving Tag()?

Sure, I'll do that when I have a minute (probably tomorrow).  Should I keep the "inline" keyword removed, or re-add it?
(In reply to Aryeh Gregor from comment #20)
> (In reply to Mounir Lamouri (:mounir) from comment #19)
> > I should have seen that during the review... Aryeh, could you backout the
> > patch and attach another one only moving Tag()?
> 
> Sure, I'll do that when I have a minute (probably tomorrow).  Should I keep
> the "inline" keyword removed, or re-add it?

AFAIK, adding "inline" in a method being defined in the class declaration is useless because inlining is implicit here. However, it seems that I might be wrong. Glandium should have more insights.
Anyhow, don't change that keyword for methods you don't move.
Comment on attachment 635320 [details] [diff] [review]
Partial backout patch

Review of attachment 635320 [details] [diff] [review]:
-----------------------------------------------------------------

r=me
Attachment #635320 - Flags: review?(mounir) → review+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.