Closed Bug 1567245 Opened 9 months ago Closed 8 months ago

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

Categories

(Core :: JavaScript Engine, task, P2)

task

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox70 --- fixed

People

(Reporter: jimb, Assigned: jimb)

References

(Blocks 2 open bugs, Regressed 1 open bug)

Details

(Whiteboard: [debugger-mvp])

Attachments

(6 files)

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!)

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.

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

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

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

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

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

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.

Assignee: nobody → jimb
Blocks: dbg-70
Status: NEW → ASSIGNED
Whiteboard: [debugger-mvp]
Priority: -- → P2
Regressions: 1572562
Pushed by jblandy@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.