1567245 - [jsdbg2] Debugger::wrapVariantReferent has many unnecessary type parameters

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P2)

Description

[Jim Blandy :jimb]

In the review for a patch on bug 1564170 ("Give Debugger.Script instances their own NativeObject subclass, DebuggerScript"), jorendorff suggested the following cleanup:

I'm guessing the ReferentVariant parameter here is fully determined by Wrapper; please remove it as a template parameter and make it a member typedef instead, Wrapper::ReferentVariant.

(Actually I bet all 3 of the others are fully determined by Referent. This could be a lot nicer. Oh well!)

Comment 1

[Jim Blandy :jimb]

Attachment: Bug 1567245: Tighten value types for DebuggerWeakMap. r?jorendorff

Give DebuggerWeakMap a Wrapper type parameter to use as the WeakMap value type
instead of plain JSObject*. Then, supply a specific wrapper class type for each
DebuggerWeakMap instantiation.

No intended change in visible behavior.

Comment 2

[Jim Blandy :jimb]

Attachment: Bug 1567245: Rename DebuggerWeakMap type parameter 'UnbarrieredKey' to 'Referent'. r?jorendorff

Technically, DebuggerWeakMap could be used for any sort of cross-compartment
mapping. But we actually only use it for looking up Debugger reflection objects,
so it is more informative to call the key and value types 'referent' and
'wrapper'.

No intended change in visible behavior.

Depends on D38539

Comment 3

[Jim Blandy :jimb]

Attachment: Bug 1567245: Let Debugger::wrapVariantReferent consult DebuggerScript and DebuggerSource for their referent variant types. r?jorendorff

Give DebuggerScript and DebuggerSource a member type that spells out the variant
type appropriate for their referents. This allows wrapVariantReferent to simply
consult the wrapper for the referent variant type instead of demanding it as a
type parameter.

No intended change in visible behavior.

Depends on D38540

Comment 4

[Jim Blandy :jimb]

Attachment: Bug 1567245: Factor out pointer from DebuggerWeakMap Referent type parameter. r?jorendorff

It's more convenient for a DebuggerWeakMap's Wrapper type parameter to be the
pointed-to type, not the pointer type, since we'll be consulting its member
types. Then, it's a bit more consistent to also use the pointed-to type for
Referent.

Depends on D38541

Comment 5

[Jim Blandy :jimb]

Attachment: Bug 1567245: Tighten type of Debugger::generatorFrames and Debugger::sources. r?jorendorff

For generatorFrames, this doesn't matter, but for sources, subsequent patches
need a more precise Referent type than simply 'JSObject'.

Any type used as a weak map key needs a MovableCellHasher instantiation, so add
the necessary ones to Barrier.cpp. Since the list is getting long, sort it.

Depends on D38542

Comment 6

[Jim Blandy :jimb]

Attachment: Bug 1567245: Have Debugger::wrapVariantReferent obtain the wrapper and referent types from the DebuggerWeakMap. r?jorendorff

Finally, once we know the type of the map we're consulting, that tells us all
the other types we need to know: certainly the referent and wrapper types, but
also the referent variant type, via the wrapper.

DebuggerWeakMap does need new member types to tell us what its Referent and
Wrapper types are. Rename the type parameters to avoid shadowing.

Since the map type can be inferred from the call, we no longer need to pass any
type parameters at all when we call the innermost wrapVariantReferent overload.

Depends on D38543

Comment 7

[Jim Blandy :jimb]

This isn't actually that complicated; I just ran into too many verbose errors when I tried to do it all in one step, and doing it in tiny steps was easier. Hopefully it'll make the review easier as well.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/95f206dd2dae
Tighten value types for DebuggerWeakMap. r=jorendorff
https://hg.mozilla.org/integration/autoland/rev/c659f1359db5
Rename DebuggerWeakMap type parameter 'UnbarrieredKey' to 'Referent'. r=jorendorff
https://hg.mozilla.org/integration/autoland/rev/61d30888957a
Let Debugger::wrapVariantReferent consult DebuggerScript and DebuggerSource for their referent variant types. r=jorendorff
https://hg.mozilla.org/integration/autoland/rev/f8a3b8b9c876
Factor out pointer from DebuggerWeakMap Referent type parameter. r=jorendorff
https://hg.mozilla.org/integration/autoland/rev/661d131593dd
Tighten type of Debugger::generatorFrames and Debugger::sources. r=jorendorff
https://hg.mozilla.org/integration/autoland/rev/708ce1c9a53f
Have Debugger::wrapVariantReferent obtain the wrapper and referent types from the DebuggerWeakMap. r=jorendorff

Comment 9

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/95f206dd2dae
https://hg.mozilla.org/mozilla-central/rev/c659f1359db5
https://hg.mozilla.org/mozilla-central/rev/61d30888957a
https://hg.mozilla.org/mozilla-central/rev/f8a3b8b9c876
https://hg.mozilla.org/mozilla-central/rev/661d131593dd
https://hg.mozilla.org/mozilla-central/rev/708ce1c9a53f