Create nsVariant directly rather than via do_CreateInstance()

RESOLVED FIXED in Firefox 44

Status

()

Core
XPCOM
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

unspecified
mozilla44
Points:
---

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
Created attachment 8668601 [details] [diff] [review]
Create nsVariant directly rather than via do_CreateInstance().
(Assignee)

Updated

2 years ago
Assignee: nobody → continuation
Blocks: 931283
(Assignee)

Updated

2 years ago
Summary: Create nsVariant directly rather than via do_CreateInstance(). → Create nsVariant directly rather than via do_CreateInstance()
(Assignee)

Comment 2

2 years ago
Created attachment 8669137 [details] [diff] [review]
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
Attachment #8669137 - Flags: review?(nfroyd)
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...)
Attachment #8669137 - Flags: review?(nfroyd) → review+
(Assignee)

Updated

2 years ago
Attachment #8668601 - Attachment is obsolete: true
(Assignee)

Comment 4

2 years ago
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.
(Assignee)

Comment 5

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

Comment 6

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa0dfcebda22
https://hg.mozilla.org/mozilla-central/rev/fa0dfcebda22
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.