1271655 - Implement a C++ interface for Debugger.Source instances.

Status: NEW

(Core :: JavaScript Engine, defect, P3)

Description

[Eddy Bruel [:ejpbruel]]

This bug is part of the effort to implement a C++ interface for the debugger API. See the parent bug 1271641 for more context.

Comment 1

[Eddy Bruel [:ejpbruel]]

Attachment: Implement a Debugger.Source class.

Comment 2

[Eddy Bruel [:ejpbruel]]

Attachment: Implement a C++ interface for DebuggerSource.text.

Comment 3

[Eddy Bruel [:ejpbruel]]

Attachment: Implement a C++ interface for DebuggerSource.text.

That was the wrong patch.

Comment 4

[Eddy Bruel [:ejpbruel]]

Attachment: Implement a C++ interface for DebuggerSource.url.

Comment 5

[Jim Blandy :jimb]

Could we make DebuggerSource_{trace,construct,check,checkThis} into DebuggerSource::X, too?

Comment 6

[Jim Blandy :jimb]

Oh, `construct` is already in there...

Comment 7

[Jim Blandy :jimb]

Comment on attachment 8815687 [details] [diff] [review]
Implement a Debugger.Source class.

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

Looks good!

::: js/src/vm/Debugger.cpp
@@ +6844,5 @@
>      ReturnType match(Handle<WasmInstanceObject*> instance) { obj_->setPrivateGCThing(instance); }
>  };
>  
> +/* static */ NativeObject*
> +DebuggerSource::initClass(JSContext* cx, HandleObject debugCtor, HandleObject obj)

These argument names are reversed in Debugger.h.

Calls to various initClass methods presently in Debugger.cpp:

    frameProto = DebuggerFrame::initClass(cx, debugCtor, obj);
    objectProto = DebuggerObject::initClass(cx, obj, debugCtor);
    envProto = DebuggerEnvironment::initClass(cx, debugCtor, obj);

This is not a recipe for happiness.

@@ +6856,5 @@
> +
> +    if (!sourceProto)
> +        return nullptr;
> +
> +    return sourceProto;

I understand we have an "early return" rule, but I think it'd be okay to just "return sourceProto".

@@ +6880,5 @@
>  Debugger::newDebuggerSource(JSContext* cx, Handle<DebuggerSourceReferent> referent)
>  {
>      assertSameCompartment(cx, object.get());
>  
> +    RootedNativeObject debugger(cx, object);

I would move this to the top, and then use it in the assertSameCompartment, and in the JSSLOT_DEBUG_SOURCE_PROTO fetch.

@@ +6936,5 @@
>  {
>      JSObject* thisobj = NonNullObject(cx, thisv);
>      if (!thisobj)
>          return nullptr;
> +    if (thisobj->getClass() != &DebuggerSource::class_) {

You can just say `class_` here.

Comment 8

[Jim Blandy :jimb]

Comment on attachment 8816128 [details] [diff] [review]
Implement a C++ interface for DebuggerSource.text.

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

::: js/src/vm/Debugger.cpp
@@ +6965,5 @@
> +    }
> +
> +    Rooted<DebuggerSourceReferent> referent(cx, source->referent());
> +    DebuggerSourceGetTextMatcher matcher(cx);
> +    result.set(referent.match(matcher));

Can't this call to referent.match fail? You'll end up returning true with `result` and the JSSLOT_DEBUGSOURCE_TEXT slot set to nullptr.

If I'm right that this is a problem, then we'll need a test case to catch it.

Comment 9

[Jim Blandy :jimb]

Comment on attachment 8816148 [details] [diff] [review]
Implement a C++ interface for DebuggerSource.url.

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

Looks good to me.

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Moving to p3 because no activity for at least 1 year(s).
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information