1022003 - Remove ObjectWrapper.jsm

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Bobby Holley (:bholley)]

We deprecated ObjectWrapper.jsm, and now there are only three consumers of it in the tree. One of them doesn't use it at all, and the other just uses it for getObjectKind.

We should move getObjectKind into a some other kind of utils JSM, and remove ObjectWrapper.jsm.

Comment 1

[:Gijs (he/him)]

Did you have a place in mind where to put getObjectKind? Just directly on Cu, or somewhere else?

Comment 2

[Bobby Holley (:bholley)]

(In reply to :Gijs Kruitbosch from comment #1)
> Did you have a place in mind where to put getObjectKind? Just directly on
> Cu, or somewhere else?

No idea. It doesn't need to be on Cu certainly - it's trivially implementable in JS. Is there some sort of JSM dumping ground for utils-y things? I'm just saying that we don't need a whole global for this thing.

Comment 3

[:Gijs (he/him)]

(In reply to Bobby Holley (:bholley) from comment #2)
> (In reply to :Gijs Kruitbosch from comment #1)
> > Did you have a place in mind where to put getObjectKind? Just directly on
> > Cu, or somewhere else?
> 
> No idea. It doesn't need to be on Cu certainly - it's trivially
> implementable in JS. Is there some sort of JSM dumping ground for utils-y
> things?

Not that I know of.

> I'm just saying that we don't need a whole global for this thing.


I think the simplest thing might be to add a method to SettingsDB, considering that that's essentially where the two sole consumers live right now (and SettingsManager can call into SettingsDB).

Comment 4

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #3)
> (In reply to Bobby Holley (:bholley) from comment #2)
> > (In reply to :Gijs Kruitbosch from comment #1)
> > > Did you have a place in mind where to put getObjectKind? Just directly on
> > > Cu, or somewhere else?
> > 
> > No idea. It doesn't need to be on Cu certainly - it's trivially
> > implementable in JS. Is there some sort of JSM dumping ground for utils-y
> > things?
> 
> Not that I know of.

Well, the closest I can think of are Services and XPCOMUtils, but both of them actually have a decently-specified purpose/domain, and this method isn't a clear fit for either.

Comment 5

[:Gijs (he/him)]

Attachment: remove ObjectWrapper.jsm,

So here's what I think we should do... Try push at https://tbpl.mozilla.org/?tree=Try&rev=39d9decfed95

Comment 6

[Bobby Holley (:bholley)]

Comment on attachment 8437730 [details] [diff] [review]
remove ObjectWrapper.jsm,

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

r=bholley with that fixed.

::: dom/base/ObjectWrapper.jsm
@@ -33,5 @@
> -    } else if (aObject instanceof Ci.nsIDOMFile) {
> -      return "file";
> -    } else if (aObject instanceof Ci.nsIDOMBlob) {
> -      return "blob";
> -    } else if (aObject instanceof Date) {

Oh wow, this was totally broken - cross-global instanceof doesn't work for pure ES objects. So unless the Date object actually came from this scope (which it almost certainly didn't), this would never return "date".

Can you fix up the remaining consumers to do the constructor.name check?

Comment 7

[Gregor Wagner [:gwagner]]

Comment on attachment 8437730 [details] [diff] [review]
remove ObjectWrapper.jsm,

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

I thought we are using it in more places. Thanks for fixing!

Comment 8

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #5)
> Created attachment 8437730 [details] [diff] [review]
> remove ObjectWrapper.jsm,
> 
> So here's what I think we should do... Try push at
> https://tbpl.mozilla.org/?tree=Try&rev=39d9decfed95

Well, that was dumb. Let's try with actually removing the Cu.import call...

https://tbpl.mozilla.org/?tree=Try&rev=4849f975f577

Comment 9

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/bb9ca4ddc79e

Marco, can you add this one as well, please? :-)

Comment 10

[Marco Mucci [:MarcoM]]

Added to Iteration 33.1

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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