1308510 - Baldr: default to non-experimental text format

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

Since the baseline we need to provide is the linear text format, I think that should be the default thing we display in the the debugger view.  Later, I think we can hook the Beautify button to render the more readable infix experimental text format we're currently displaying.

Comment 1

[Luke Wagner [:luke]]

In looking at the trivial patch for this I noticed that BinaryToText() doesn't produce a GeneratedSourceMap.  Yury, I assume we need GeneratedSourceMap for the debugger integration?  How hard would this be to add to WasmBinaryToText()?  It seems like we'll need this eventually since we have to support the the linear format as a baseline.

Comment 2

[Yury Delendik (:yury)]

(In reply to Luke Wagner [:luke] from comment #1)
> In looking at the trivial patch for this I noticed that BinaryToText()
> doesn't produce a GeneratedSourceMap.  Yury, I assume we need
> GeneratedSourceMap for the debugger integration?  How hard would this be to
> add to WasmBinaryToText()?  It seems like we'll need this eventually since
> we have to support the the linear format as a baseline.

Not hard. I planned to do it in the nearest future. If you want I can take the entire bug since bulk of work will be generate source map change.

Comment 3

[Luke Wagner [:luke]]

Oh yes, that would be much appreciated, thank you!

Comment 4

[Yury Delendik (:yury)]

Attachment: Bug 1308510 - Default to non-experimental text format.

Review commit: https://reviewboard.mozilla.org/r/85566/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/85566/

Comment 5

[Yury Delendik (:yury)]

Attachment: Bug 1308510 - Disable highlight for non-experimental WebAssembly format.

Review commit: https://reviewboard.mozilla.org/r/85568/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/85568/

Comment 6

[James Long (:jlongster)]

Comment on attachment 8800686 [details]
Bug 1308510 - Disable highlight for non-experimental WebAssembly format.

https://reviewboard.mozilla.org/r/85568/#review84226

Look good, r+ if you remove the change to the bundle.

::: devtools/client/debugger/new/bundle.js:39198
(Diff revision 1)
>  	    var contentType = sourceText.get("contentType");
>  	
>  	    if (contentType.includes("javascript")) {
>  	      this.editor.setMode({ name: "javascript" });
>  	    } else if (contentType === "text/wasm") {
> -	      this.editor.setMode({ name: "wasm" });
> +	      this.editor.setMode({ name: "text" });

Don't update the bundle; I just pushed out an update and this'll conflict. It's not the end of the world if you change it, as it'll just be overwritten when we update it, but it's good to try to keep the commit history clean.

Thanks for the opening a PR on github! We will merge it there and push a bundle update later this week.

Comment 7

[Luke Wagner [:luke]]

Comment on attachment 8800685 [details]
Bug 1308510 - Default to non-experimental text format.

https://reviewboard.mozilla.org/r/85566/#review84250

Thanks!

::: js/src/asmjs/WasmCode.cpp:720
(Diff revision 1)
>              return nullptr;
>  
> -        if (!BinaryToExperimentalText(cx, bytes.begin(), bytes.length(), buffer,
> +        if (!BinaryToText(cx, bytes.begin(), bytes.length(), buffer, maybeSourceMap_.get())) {
> -                                      ExperimentalTextFormatting(), maybeSourceMap_.get())) {
>              return nullptr;
>          }

nit: no { } now that single-line

Comment 8

[Yury Delendik (:yury)]

Comment on attachment 8800685 [details]
Bug 1308510 - Default to non-experimental text format.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85566/diff/1-2/

Comment 9

[Yury Delendik (:yury)]

Comment on attachment 8800686 [details]
Bug 1308510 - Disable highlight for non-experimental WebAssembly format.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85568/diff/1-2/

Comment 10

[Jeff Walden [:Waldo]]

(In reply to James Long (:jlongster) from comment #6)
> >  	    } else if (contentType === "text/wasm") {

Is this actually implying a MIME type of text/wasm?  Wouldn't application/wasm have been more appropriate?  After all, application/javascript is the preferred type for JS, not text/javascript.

Comment 11

[Yury Delendik (:yury)]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #10)
> (In reply to James Long (:jlongster) from comment #6)
> > >  	    } else if (contentType === "text/wasm") {
> 
> Is this actually implying a MIME type of text/wasm?  Wouldn't
> application/wasm have been more appropriate?  After all,
> application/javascript is the preferred type for JS, not text/javascript.

There is no spec for mime types for WebAssembly and its text format (see https://github.com/WebAssembly/spec/issues/357). Actually, 'application/wasm' is more appropriate for binary format. Text format is not fully define yet.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b9d6455cb170
Default to non-experimental text format. r=luke
https://hg.mozilla.org/integration/autoland/rev/f134bd918a45
Disable highlight for non-experimental WebAssembly format. r=jlongster

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/b9d6455cb170
https://hg.mozilla.org/mozilla-central/rev/f134bd918a45