1210517 - Create nsVariant directly rather than via do_CreateInstance()

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

The goal here is to leave creation stuff mostly for JS, so we can
convert it entirely over to a non-threadsafe cycle-collected version
without breaking any existing C++ users.

I didn't do this for a remaining use in nsGlobalWindow.h to avoid
including nsVariant.h all over the place.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: Create nsVariant directly rather than via do_CreateInstance().

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: Create nsVariant directly rather than via do_CreateInstance().

The goal here is to leave creation stuff mostly for JS, so we can
convert it entirely over to a non-threadsafe cycle-collected version
without breaking any existing C++ users.

I didn't do this for a remaining use in nsGlobalWindow.h to avoid
including nsVariant.h all over the place.

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=83e2e0f866a3

Comment 3

[Nathan Froyd [:froydnj]]

Comment on attachment 8669137 [details] [diff] [review]
Create nsVariant directly rather than via do_CreateInstance().

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

We should probably just shuffle DialogValueHolder and friends around from nsGlobalWindow.h to nsGlobalWindow.cpp so we can alter that final instance.  Followup bug?

::: dom/base/nsContentAreaDragDrop.cpp
@@ +726,5 @@
>                              const nsAString& aFlavor,
>                              const nsAString& aData,
>                              nsIPrincipal* aPrincipal)
>  {
> +  nsCOMPtr<nsIWritableVariant> variant = new nsVariant();

I have a slight preference for using nsRefPtr<nsVariant> in cases like these, so that we're using concrete types whenever possible.  Ditto for all instances below.

::: dom/events/DataTransfer.cpp
@@ +434,5 @@
>  void
>  DataTransfer::SetData(const nsAString& aFormat, const nsAString& aData,
>                        ErrorResult& aRv)
>  {
> +  nsCOMPtr<nsIWritableVariant> variant = new nsVariant();

I *think* that making this allocation switch from fallible to infallible (along with all the others) is OK, because the mechanism underneath do_CreateInstance probably didn't properly check for fallibility anyway.  (Which makes me wonder if we should make it do that...)

Comment 4

[Andrew McCreight [:mccr8]]

I filed bug 1212148 for the use in nsGlobalWindow.h and I replaced all of the nsCOMPtr<> with nsRefPtr<>. There's a lot of funky single platform code in the patch so I'm going to try an all-platform build run to make sure nothing is broken, given all of the problems I've had lately with broken builds in other patches.

Comment 5

[Andrew McCreight [:mccr8]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=734a970c6cf6
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea2d0d9c982e

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fa0dfcebda22

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fa0dfcebda22