Last Comment Bug 763283 - Add nsINode::AsContent, like nsINode::AsElement
: Add nsINode::AsContent, like nsINode::AsElement
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: 834487 763861 802995 835314
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-10 03:54 PDT by :Aryeh Gregor (away until August 15)
Modified: 2013-01-28 05:42 PST (History)
4 users (show)
ayg: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch part 1 v1 -- Add new nsINode::AsContent() and nsINode::IsContent() methods (1.64 KB, patch)
2012-06-10 04:30 PDT, :Aryeh Gregor (away until August 15)
mounir: review-
Details | Diff | Review
Patch part 2 v1 -- Use nsINode::AsContent() in editor/ (11.77 KB, patch)
2012-06-10 04:34 PDT, :Aryeh Gregor (away until August 15)
no flags Details | Diff | Review
Patch part 1 v2 -- Add new nsINode::AsContent() and nsINode::IsContent() methods (1.67 KB, patch)
2012-06-11 02:20 PDT, :Aryeh Gregor (away until August 15)
mounir: review+
Details | Diff | Review
Patch part 2 v2 -- Use nsINode::AsContent() in editor/ (19.19 KB, patch)
2012-06-11 02:22 PDT, :Aryeh Gregor (away until August 15)
ehsan: review+
Details | Diff | Review

Description :Aryeh Gregor (away until August 15) 2012-06-10 03:54:45 PDT
This would save a lot of QIs.
Comment 1 :Aryeh Gregor (away until August 15) 2012-06-10 04:30:38 PDT
Created attachment 631732 [details] [diff] [review]
Patch part 1 v1 -- Add new nsINode::AsContent() and nsINode::IsContent() methods

I opted to make AsContent() just return null when !IsContent(), not assert like AsElement(), because that makes it match do_QueryInterface more closely.  When updating code to use AsContent(), I found lots of stuff like

  nsCOMPtr<nsIContent> content = do_QueryInterface(Something());
  NS_ENSURE_STATE(content);

which would have to become

  NS_ENSURE_STATE(Something()->IsContent());
  nsCOMPtr<nsIContent> content = Something()->AsContent();

which involves an extra call to Something(), which might itself be long.  I don't particularly see why this is superior anyway.
Comment 2 :Aryeh Gregor (away until August 15) 2012-06-10 04:34:09 PDT
Created attachment 631734 [details] [diff] [review]
Patch part 2 v1 -- Use nsINode::AsContent() in editor/

Naturally, since this is editor/, I also found some QI's from dom::Element to nsIContent.  Got rid of those too!  The only bug this patch should be able to introduce is extra null dereferences, but I think every specific change here is probably safe.

https://tbpl.mozilla.org/?tree=Try&rev=9fe68050f2f8
Comment 3 :Ms2ger 2012-06-10 06:45:28 PDT
I think making AsContent() and AsElement() inconsistent is going to cause a lot of bugs :/
Comment 4 :Ms2ger 2012-06-10 07:29:19 PDT
Also, note that you can't just replace

  nsCOMPtr<nsIContent> content = do_QueryInterface(foo);

with

  nsCOMPtr<nsIContent> content = foo->AsContent();

because the latter will crash if foo is null.
Comment 5 :Aryeh Gregor (away until August 15) 2012-06-10 12:06:03 PDT
(In reply to :Ms2ger from comment #3)
> I think making AsContent() and AsElement() inconsistent is going to cause a
> lot of bugs :/

I'd say to change AsElement().  I could attach a patch to do that.

(In reply to :Ms2ger from comment #4)
> Also, note that you can't just replace
> 
>   nsCOMPtr<nsIContent> content = do_QueryInterface(foo);
> 
> with
> 
>   nsCOMPtr<nsIContent> content = foo->AsContent();
> 
> because the latter will crash if foo is null.

(In reply to Aryeh Gregor from comment #2)
> The only bug this patch should be
> able to introduce is extra null dereferences, but I think every specific
> change here is probably safe.

There were one or two that aren't totally obvious and that I should have looked more closely at, that.
Comment 6 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-06-10 12:14:59 PDT
If the methods can return null, they should be called GetAsContent/GetAsElement
Comment 7 Mounir Lamouri (:mounir) 2012-06-10 13:01:38 PDT
Comment on attachment 631732 [details] [diff] [review]
Patch part 1 v1 -- Add new nsINode::AsContent() and nsINode::IsContent() methods

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

I think you should have AsContent() behaving like AsElement(). That would be consistent and more efficient regarding performance because IsContent() will not have to be called if the caller knows the node is a content.
However, MOZ_ASSERT() should probably be used instead of NS_ASSERTION.
Comment 8 :Aryeh Gregor (away until August 15) 2012-06-11 02:20:47 PDT
Created attachment 631834 [details] [diff] [review]
Patch part 1 v2 -- Add new nsINode::AsContent() and nsINode::IsContent() methods

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #7)
> I think you should have AsContent() behaving like AsElement(). That would be
> consistent and more efficient regarding performance because IsContent() will
> not have to be called if the caller knows the node is a content.
> However, MOZ_ASSERT() should probably be used instead of NS_ASSERTION.

Okay, done.  Should I change AsElement() to MOZ_ASSERT() also in another patch, for consistency?
Comment 9 :Aryeh Gregor (away until August 15) 2012-06-11 02:22:13 PDT
Created attachment 631835 [details] [diff] [review]
Patch part 2 v2 -- Use nsINode::AsContent() in editor/

Rewritten for the change in part one.  It forces the logic to be somewhat more explicit, which may or may not be a good thing but is certainly a bit longer.

Try: https://tbpl.mozilla.org/?tree=Try&rev=b967be93be7a
Comment 10 Mounir Lamouri (:mounir) 2012-06-11 02:24:35 PDT
Comment on attachment 631834 [details] [diff] [review]
Patch part 1 v2 -- Add new nsINode::AsContent() and nsINode::IsContent() methods

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

You can open a new bug to change AsElement() for consistency or even just change that in that patch.

r+ either way.
Comment 11 :Aryeh Gregor (away until August 15) 2012-06-11 03:44:23 PDT
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #10)
> You can open a new bug to change AsElement() for consistency or even just
> change that in that patch.
> 
> r+ either way.

I'll write a separate, third patch with r+ from you and push it as part of this bug, if that's okay with you.
Comment 12 :Aryeh Gregor (away until August 15) 2012-06-11 03:45:23 PDT
(new try with extra third patch: https://tbpl.mozilla.org/?tree=Try&rev=b478bb9077ce)
Comment 13 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-06-11 07:41:45 PDT
Comment on attachment 631834 [details] [diff] [review]
Patch part 1 v2 -- Add new nsINode::AsContent() and nsINode::IsContent() methods

Do we have spare bits in nsINode? If so, could we add a flag to
tell if node is content so that we wouldn't have to use virtual 
IsNodeOfType call.

(Also, { goes to the next line after method declaration)
Comment 14 :Aryeh Gregor (away until August 15) 2012-06-11 08:08:39 PDT
(In reply to Olli Pettay [:smaug] from comment #13)
> (Also, { goes to the next line after method declaration)

Oops -- fixed.  I copy-pasted from Element.h, which also is missing a newline there.  I'll fix that in patch three while I'm there.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d625424ed3ed
https://hg.mozilla.org/integration/mozilla-inbound/rev/f15e5762f237
https://hg.mozilla.org/integration/mozilla-inbound/rev/adfd72bd9d39
Comment 15 :Ehsan Akhgari (out sick) 2012-06-11 10:31:37 PDT
(In reply to Olli Pettay [:smaug] from comment #13)
> Comment on attachment 631834 [details] [diff] [review]
> Patch part 1 v2 -- Add new nsINode::AsContent() and nsINode::IsContent()
> methods
> 
> Do we have spare bits in nsINode? If so, could we add a flag to
> tell if node is content so that we wouldn't have to use virtual 
> IsNodeOfType call.

Yeah, we do have a lot of room in nsINode::BooleanFlag.

Aryeh, can you file a new bug for this please?
Comment 16 :Aryeh Gregor (away until August 15) 2012-06-12 02:38:30 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #15)
> Yeah, we do have a lot of room in nsINode::BooleanFlag.
> 
> Aryeh, can you file a new bug for this please?

Bug 763861.

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