The default bug view has changed. See this FAQ.

Move various nsIContent methods to nsINode

RESOLVED FIXED in mozilla16

Status

()

Core
DOM
--
enhancement
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ayg, Assigned: ayg)

Tracking

Trunk
mozilla16
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

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.
Created attachment 632708 [details] [diff] [review]
Patch part 1, v1 -- Move various nsIContent methods to nsINode
Attachment #632708 - Flags: review?(mounir)
Created attachment 632712 [details] [diff] [review]
Patch part 2, v1 -- Improve Tag() comment, fix whitespace

While I'm here . . .

Try: https://tbpl.mozilla.org/?tree=Try&rev=dce0ae217c71
Attachment #632712 - Flags: review?(mounir)
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.
This landing likely caused these regressions:
https://groups.google.com/forum/?fromgroups#!topic/mozilla.dev.tree-management/vKh0F7QTrYg
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
Last Resolved: 5 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.
Created attachment 635320 [details] [diff] [review]
Partial backout patch

New try: https://tbpl.mozilla.org/?tree=Try&rev=ec1b99f7c8cc
Attachment #635320 - Flags: review?(mounir)
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+
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9dec2d6c445
https://hg.mozilla.org/mozilla-central/rev/c9dec2d6c445
You need to log in before you can comment on or make changes to this bug.