Closed Bug 1308510 Opened 5 years ago Closed 5 years ago

Baldr: default to non-experimental text format

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: luke, Assigned: yury)

References

Details

Attachments

(2 files)

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.
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.
Flags: needinfo?(ydelendik)
(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.
Flags: needinfo?(ydelendik)
Oh yes, that would be much appreciated, thank you!
Assignee: luke → ydelendik
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.
Attachment #8800686 - Flags: review?(jlong) → review+
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
Attachment #8800685 - Flags: review?(luke) → review+
Keywords: checkin-needed
See Also: → 1285976, 1280471
(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.
(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.
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.