Last Comment Bug 764400 - Move various nsIContent methods to nsINode
: Move various nsIContent methods to nsINode
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: mozilla16
Assigned To: :Aryeh Gregor (away until August 15)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-13 08:02 PDT by :Aryeh Gregor (away until August 15)
Modified: 2012-06-22 09:16 PDT (History)
6 users (show)
ayg: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch part 1, v1 -- Move various nsIContent methods to nsINode (4.06 KB, patch)
2012-06-13 08:33 PDT, :Aryeh Gregor (away until August 15)
mounir: review+
Details | Diff | Review
Patch part 2, v1 -- Improve Tag() comment, fix whitespace (1.32 KB, patch)
2012-06-13 08:37 PDT, :Aryeh Gregor (away until August 15)
mounir: review+
Details | Diff | Review
Partial backout patch (4.00 KB, patch)
2012-06-21 08:15 PDT, :Aryeh Gregor (away until August 15)
mounir: review+
Details | Diff | Review

Description :Aryeh Gregor (away until August 15) 2012-06-13 08:02:21 PDT
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 1 :Aryeh Gregor (away until August 15) 2012-06-13 08:33:13 PDT
Created attachment 632708 [details] [diff] [review]
Patch part 1, v1 -- Move various nsIContent methods to nsINode
Comment 2 :Aryeh Gregor (away until August 15) 2012-06-13 08:37:13 PDT
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
Comment 3 :Ms2ger 2012-06-14 01:08:09 PDT
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 4 Mounir Lamouri (:mounir) 2012-06-18 11:38:06 PDT
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.
Comment 5 Mounir Lamouri (:mounir) 2012-06-18 11:38:14 PDT
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.
Comment 6 :Aryeh Gregor (away until August 15) 2012-06-19 01:25:28 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2a44838954a
Comment 7 Peter Van der Beken [:peterv] 2012-06-19 04:13:06 PDT
This doesn't all make sense IMO, especially putting IsHTML/IsSVG/IsXUL/IsMathML on documents.
Comment 8 Peter Van der Beken [:peterv] 2012-06-19 04:16:01 PDT
And won't|((nsINode*)doc)->IsHTML()| now return a different response from |((nsIDocument*)doc)->IsHTML()| :-/.
Comment 9 :Aryeh Gregor (away until August 15) 2012-06-19 05:40:17 PDT
(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.
Comment 10 Mike Hommey [:glandium] 2012-06-19 15:22:30 PDT
This landing likely caused these regressions:
https://groups.google.com/forum/?fromgroups#!topic/mozilla.dev.tree-management/vKh0F7QTrYg
Comment 11 :Aryeh Gregor (away until August 15) 2012-06-20 00:37:28 PDT
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?
Comment 12 Mike Hommey [:glandium] 2012-06-20 01:02:06 PDT
(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".
Comment 13 Ed Morley [:emorley] 2012-06-20 02:23:52 PDT
https://hg.mozilla.org/mozilla-central/rev/e2a44838954a
Comment 14 :Aryeh Gregor (away until August 15) 2012-06-20 04:41:56 PDT
(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?
Comment 15 Mike Hommey [:glandium] 2012-06-20 04:51:15 PDT
(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.
Comment 16 :Aryeh Gregor (away until August 15) 2012-06-20 07:15:32 PDT
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.
Comment 17 Peter Van der Beken [:peterv] 2012-06-20 08:12:22 PDT
I'd rather see the Is* methods moved back to nsIContent, to avoid the confusion I pointed out in comment 7 and comment 8.
Comment 18 Peter Van der Beken [:peterv] 2012-06-20 08:14:38 PDT
For example, I worry about people writing

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

and not realizing that this will always be false.
Comment 19 Mounir Lamouri (:mounir) 2012-06-20 08:17:51 PDT
I should have seen that during the review... Aryeh, could you backout the patch and attach another one only moving Tag()?
Comment 20 :Aryeh Gregor (away until August 15) 2012-06-20 08:30:30 PDT
(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?
Comment 21 Mounir Lamouri (:mounir) 2012-06-20 08:40:58 PDT
(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 22 :Aryeh Gregor (away until August 15) 2012-06-21 08:15:38 PDT
Created attachment 635320 [details] [diff] [review]
Partial backout patch

New try: https://tbpl.mozilla.org/?tree=Try&rev=ec1b99f7c8cc
Comment 23 Mounir Lamouri (:mounir) 2012-06-21 08:58:43 PDT
Comment on attachment 635320 [details] [diff] [review]
Partial backout patch

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

r=me
Comment 24 :Aryeh Gregor (away until August 15) 2012-06-22 00:42:28 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9dec2d6c445
Comment 25 Ed Morley [:emorley] 2012-06-22 09:16:33 PDT
https://hg.mozilla.org/mozilla-central/rev/c9dec2d6c445

Note You need to log in before you can comment on or make changes to this bug.