tidy up nsImageBoxFrame::Init a little bit

RESOLVED FIXED in Firefox 40

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

unspecified
mozilla40
Points:
---

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(1 attachment)

We don't need to use manual NS_{ADDREF,RELEASE} here when nsRefPtr will
do that for us.  The manual QueryInterface invocation only serves to do
an expensive static_cast and increase the refcount; we can forget our
reference into mListener to achieve the same effect.
Created attachment 8586816 [details] [diff] [review]
tidy up nsImageBoxFrame::Init a little bit

We don't need to use manual NS_{ADDREF,RELEASE} here when nsRefPtr will
do that for us.  The manual QueryInterface invocation only serves to do
an expensive static_cast and increase the refcount; we can forget our
reference into mListener to achieve the same effect.
Comment on attachment 8586816 [details] [diff] [review]
tidy up nsImageBoxFrame::Init a little bit

git-bz weirdly wouldn't let me set the reviewer flag while attaching the patch, so here you go.
Attachment #8586816 - Flags: review?(dholbert)
Comment on attachment 8586816 [details] [diff] [review]
tidy up nsImageBoxFrame::Init a little bit


> nsImageBoxFrame::Init(nsIContent*       aContent,
>                       nsContainerFrame* aParent,
>                       nsIFrame*         aPrevInFlow)
> {
>   if (!mListener) {
>-    nsImageBoxListener *listener = new nsImageBoxListener();
>-    NS_ADDREF(listener);
>+    nsRefPtr<nsImageBoxListener> listener = new nsImageBoxListener();
>     listener->SetFrame(this);
>-    listener->QueryInterface(NS_GET_IID(imgINotificationObserver), getter_AddRefs(mListener));
>-    NS_RELEASE(listener);
>+    mListener = listener.forget();

Can we simplify this further, to just:
  mListener = new nsImageBoxListener();
  listener->SetFrame(this);

or does that not work for some reason?

(If nsImageBoxListener did anything non-trivial, then reversing the order like this might be problematic. But it's just a one-liner, "mFrame = frame":
http://mxr.mozilla.org/mozilla-central/source/layout/xul/nsImageBoxFrame.h?rev=8bc7561d7557#32
)

(Also -- side note -- I wonder if mListener should really just be a nsRefPtr<nsImageBoxListener>. We already reinterpret_cast it to nsImageBoxListener in one spot, so there's no mystery / abstraction about what the underlying type is. No need to fix that up as part of this bug; but if you feel like doing so, great!)
(In reply to Daniel Holbert [:dholbert] from comment #3)
> Can we simplify this further, to just:
>   mListener = new nsImageBoxListener();
>   listener->SetFrame(this);
> 
> or does that not work for some reason?

ohh right, that doesn't work because mListener is an interface-pointer, whereas SetFrame is declared on the actual concrete class. Never mind about that, then.
Attachment #8586816 - Flags: review?(dholbert) → review+
(In reply to Daniel Holbert [:dholbert] from comment #3)
> (Also -- side note -- I wonder if mListener should really just be a
> nsRefPtr<nsImageBoxListener>. We already reinterpret_cast it to
> nsImageBoxListener in one spot, so there's no mystery / abstraction about
> what the underlying type is. No need to fix that up as part of this bug; but
> if you feel like doing so, great!)

We pass it out to something(s) in nsContentUtils (maybe other places?) that require the interface class, but I should think that the compiler would take care of the necessary casts there.  I'll file a followup for that.
Or pass the frame to the constructor?
https://hg.mozilla.org/mozilla-central/rev/e99e71fa5089
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.