Closed Bug 867101 Opened 7 years ago Closed 7 years ago

Finish fixing implicit conversion to already_AddRefed

Categories

(Core :: General, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: ayg, Assigned: ayg)

References

Details

Attachments

(1 file)

This is for all the little odds and ends that don't need their own bugs.  Patch will be attached once I get a green try run with the bug 859817 patch.
Flags: in-testsuite-
Attached patch PatchSplinter Review
Green try: https://tbpl.mozilla.org/?tree=Try&rev=824d1dd7f6fa

Feedback from roc requested for the gfx/thebes/gfxWindowsPlatform.cpp change.  It seems like this codepath was previously returning an object with refcount zero, which this patch fixes.  Is that right?  (If so, it's exactly the sort of bug this is meant to fix!)


Other remarks:

NULL to nullptr changes: Our OS X builders don't implicitly convert NULL to nullptr_t, so they need these changes.  Other platforms seem happy either way.

content/base/src/nsDocument.cpp: Doesn't use .downcast() as requested by Ehsan in bug 859817 comment 17.  The InternalDOMEvent() thing is awkward, but I don't see a nicer way to do it.

gfx/thebes/gfxWindowsSurface.cpp: Hmm, I should get rid of the intermediate asurf variable here.

toolkit/components/places/nsPlacesMacros.h: To explain the comment a bit more, Database::~Database does MOZ_ASSERT(gDatabase) to ensure there aren't multiple singletons constructed, so without the "ret = nullptr;" we get an assert failure at "return ret;".

widget/cocoa/nsChildView.mm: Aghkhhpfthb.

widget/windows/nsIdleServiceWin.h: The way I changed this is inconsistent from the way I changed the other ones.  Probably I should make them match this, to keep the downcast as close as possible to the source of the variable.

xpcom/base/nsCycleCollector.cpp: See bug 860886.  This should be fixed properly, but it would be a bit annoying, so I didn't.
Attachment #750999 - Flags: review?(Ms2ger)
Attachment #750999 - Flags: feedback?(roc)
Comment on attachment 750999 [details] [diff] [review]
Patch

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

That is a heinous bug!
Attachment #750999 - Flags: feedback?(roc) → feedback+
Comment on attachment 750999 [details] [diff] [review]
Patch

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

Love it.

::: content/base/src/nsDocument.cpp
@@ +4762,5 @@
>                    nullptr, mDefaultElementType, getter_AddRefs(content));
>    if (rv.Failed()) {
>      return nullptr;
>    }
> +  return dont_AddRef(content.forget().get()->AsElement());

I'd like a followup filed to make CreateElem and NS_NewElement return an Element.

@@ +6909,5 @@
>    rv =
>      nsEventDispatcher::CreateEvent(const_cast<nsIDocument*>(this),
>                                     presContext, nullptr, aEventType,
>                                     getter_AddRefs(ev));
> +  return ev ? dont_AddRef(ev.forget().get()->InternalDOMEvent()) : nullptr;

And one for this too.

::: gfx/thebes/gfxWindowsSurface.cpp
@@ +178,5 @@
>      if (!isurf)
>          return nullptr;
>  
>      nsRefPtr<gfxASurface> asurf = gfxASurface::Wrap(isurf);
> +    return asurf.forget().downcast<gfxImageSurface>();

Feel free to get rid of asurf, yes.

::: uriloader/exthandler/mac/nsOSHelperAppService.mm
@@ +314,2 @@
>    if (!mimeInfoMac)
>      return nullptr;

While you're here, remove this check

::: xpcom/base/nsCycleCollector.cpp
@@ +1668,5 @@
>            NS_IF_RELEASE(logFile);
>            return nullptr;
>          }
>  
> +        return dont_AddRef(logFile);

Add a comment why this needs to be special.
Attachment #750999 - Flags: review?(Ms2ger) → review+
Depends on: 874838
(In reply to :Ms2ger from comment #3)
> @@ +6909,5 @@
> >    rv =
> >      nsEventDispatcher::CreateEvent(const_cast<nsIDocument*>(this),
> >                                     presContext, nullptr, aEventType,
> >                                     getter_AddRefs(ev));
> > +  return ev ? dont_AddRef(ev.forget().get()->InternalDOMEvent()) : nullptr;
> 
> And one for this too.

What should the followup ask to do?  Make nsDOMEvent subclass nsIDOMEvent?  Make nsEventDispatcher::CreateEvent return an nsDOMEvent?
Flags: needinfo?(Ms2ger)
The latter, please.
Flags: needinfo?(Ms2ger)
Depends on: 874842
https://hg.mozilla.org/mozilla-central/rev/0f04bc2cf586
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.