Add nsINode::AsContent, like nsINode::AsElement

RESOLVED FIXED in mozilla16

Status

()

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

People

(Reporter: ayg, Assigned: ayg)

Tracking

(Depends on: 1 bug)

Trunk
mozilla16
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

This would save a lot of QIs.
Flags: in-testsuite-
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.
Attachment #631732 - Flags: review?(mounir)
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
Attachment #631734 - Flags: review?(ehsan)
I think making AsContent() and AsElement() inconsistent is going to cause a lot of bugs :/
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 :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

5 years ago
If the methods can return null, they should be called GetAsContent/GetAsElement
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.
Attachment #631732 - Flags: review?(mounir) → review-
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?
Attachment #631732 - Attachment is obsolete: true
Attachment #631834 - Flags: review?(mounir)
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
Attachment #631734 - Attachment is obsolete: true
Attachment #631734 - Flags: review?(ehsan)
Attachment #631835 - Flags: review?(ehsan)
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.
Attachment #631834 - Flags: review?(mounir) → review+
(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.
(new try with extra third patch: https://tbpl.mozilla.org/?tree=Try&rev=b478bb9077ce)
Attachment #631835 - Flags: review?(ehsan) → review+
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)
(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
Target Milestone: --- → mozilla16
(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?
(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.
Depends on: 763861
https://hg.mozilla.org/mozilla-central/rev/d625424ed3ed
https://hg.mozilla.org/mozilla-central/rev/f15e5762f237
https://hg.mozilla.org/mozilla-central/rev/adfd72bd9d39
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Depends on: 802995
Depends on: 835314
Depends on: 834487
You need to log in before you can comment on or make changes to this bug.