1271654 - Implement a C++ interface for Debugger.Script 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 DebuggerScript class.

Comment 2

[Eddy Bruel [:ejpbruel]]

Attachment: DebuggerScript_getDisplayName does not need to wrap its result.

Hi Jim. I know we should be reluctant about changing the semantics of the debugger API in this patch series, but if I'm right, this patch just removes redundant code.

The displayName of a JSFunction is a JSAtom. According to jandem, because all atoms live in the same zone, they don't have to be copied by a call to cx->compartment()->wrap (unlike JSStrings, which can live in different zones).

There is precedent for this change too: we already don't wrap the result in DebuggerObject::displayName, which accesses the same property.

All in all, the change seems small enough to me that it's ok to land this in a separate patch. Let me know if you agree.

Comment 3

[Jim Blandy :jimb]

Comment on attachment 8770488 [details] [diff] [review]
Implement a DebuggerScript class.

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

Looks good.

::: js/src/vm/Debugger.cpp
@@ +5122,5 @@
>  
>  /*** Debugger.Script *****************************************************************************/
>  
> +/* static */ NativeObject*
> +DebuggerScript::initClass(JSContext* cx, HandleObject debugCtor, HandleObject obj)

Couldn't this return a DebuggerScript*? The prototype does use the right class.

@@ +5137,5 @@
> +
> +    return scriptProto;
> +}
> +
> +class DebuggerScriptSetPrivateMatcher

If we made this class private to DebuggerScript, it might make things a little less verbose. You'd put:

    class SetPrivateMatcher;

inside the definition of class DebuggerScript in Debugger.h, change this to

    class DebuggerScript::SetPrivateMatcher { ... };

and then change DebuggerScript::create to just say:

    SetPrivateMatcher matcher(&script);

Just a suggestion.

Comment 4

[Eddy Bruel [:ejpbruel]]

Attachment: Implement a C++ interface for DebuggerScript.displayName.

Comment 5

[Eddy Bruel [:ejpbruel]]

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

Comment 6

[Eddy Bruel [:ejpbruel]]

Attachment: Implement a C++ interface for DebuggerScript.startLine.

Comment 7

[Eddy Bruel [:ejpbruel]]

Attachment: Implement a C++ interface for DebuggerScript.lineCount.

Comment 8

[Eddy Bruel [:ejpbruel]]

Attachment: Implement a C++ interface for DebuggerScript.displayName (v2).

Fixed a minor (and rather daft) bug in this patch.

Comment 9

[Eddy Bruel [:ejpbruel]]

Attachment: Implement a C++ interface for DebuggerScript.source.

Note that this patch depends on the patch in bug 1271655.

Comment 10

[Eddy Bruel [:ejpbruel]]

Attachment: Implement a C++ interface for DebuggerScript.sourceStart.

Comment 11

[Eddy Bruel [:ejpbruel]]

Attachment: Implement a C++ interface for DebuggerScript.sourceLength.

Comment 12

[Eddy Bruel [:ejpbruel]]

Attachment: Implement a C++ interface for DebuggerScript.global.

Comment 13

[Eddy Bruel [:ejpbruel]]

Attachment: Implement a C++ interface for DebuggerScript.format.

Comment 14

[Jim Blandy :jimb]

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

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

::: js/src/vm/Debugger.cpp
@@ +5560,3 @@
>  
> +    if (!DebuggerScript::requireScript(cx, script))
> +        return false;

Okay, now I see that the displayName patch introduces THIS_DEBUGGER_SCRIPT and DebuggerScript::requireScript, so I'll review that first.

Comment 15

[Jim Blandy :jimb]

Comment on attachment 8815684 [details] [diff] [review]
Implement a C++ interface for DebuggerScript.displayName (v2).

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

Okay, skipping the wrap makes sense to me. I like the requireScript check, better than the existing DebuggerScript_checkThis.

Comment 16

[Jim Blandy :jimb]

To be useful as a C++ API, DebuggerScript will need `isJavaScript` and `isWasm` methods.

Comment 17

[Jim Blandy :jimb]

Comment on attachment 8815393 [details] [diff] [review]
Implement a C++ interface for DebuggerScript.startLine.

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

::: js/src/vm/Debugger.cpp
@@ +5585,5 @@
> +
> +    if (!DebuggerScript::requireScript(cx, script))
> +        return false;
> +
> +    args.rval().setNumber(uint32_t(script->startLine()));

This cast doesn't do anything, does it?

Comment 18

[Jim Blandy :jimb]

Comment on attachment 8815394 [details] [diff] [review]
Implement a C++ interface for DebuggerScript.lineCount.

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

::: js/src/vm/Debugger.cpp
@@ +5606,2 @@
>  
> +    args.rval().setNumber(uint32_t(script->lineCount()));

This cast doesn't do anything, that I can see.

Comment 19

[Jim Blandy :jimb]

Comment on attachment 8816112 [details] [diff] [review]
Implement a C++ interface for DebuggerScript.format.

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

::: js/src/vm/Debugger.cpp
@@ +5737,5 @@
>  
> +    JSString* str;
> +    switch (script->format()) {
> +      case DebuggerScriptFormat::JS:
> +        str = cx->names().js; 

Space at end of line.

@@ +6814,5 @@
>      JS_PSG("source", DebuggerScript::sourceGetter, 0),
>      JS_PSG("sourceStart", DebuggerScript::sourceStartGetter, 0),
>      JS_PSG("sourceLength", DebuggerScript::sourceLengthGetter, 0),
>      JS_PSG("global", DebuggerScript::globalGetter, 0),
> +    JS_PSG("format", DebuggerScript::formatGetter, 0),

This is a new JS-visible API, so it needs tests and documentation.

::: js/src/vm/Debugger.h
@@ +1552,5 @@
>      static MOZ_MUST_USE bool getErrorReport(JSContext* cx, HandleObject maybeError,
>                                              JSErrorReport*& report);
>  };
>  
> +enum class DebuggerScriptFormat {

Make this a public member type, DebuggerScript::Format?

Comment 20

[Jim Blandy :jimb]

I'm going to work on getting this landed.