Stub out wasm source files in the Debugger

RESOLVED FIXED in Firefox 48

Status

()

Firefox
Developer Tools: Debugger
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jsantell, Unassigned)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(1 attachment)

Once initial wasm support lands (mid-Q1?, no bug AFAIK), we need to stub out support for this in the debugger, so that we just display the source item in the debugger -- we don't need to display anything in the source code panel (a message maybe indicating that this is bleeding edge and coming soon?), but just basic groundwork for eventual support.
(Reporter)

Updated

2 years ago
Blocks: 1247142

Updated

2 years ago
Depends on: 1254893

Comment 1

2 years ago
Created attachment 8728913 [details] [diff] [review]
Give wasm Debugger.Sources the text/wasm mimetype and use their .text property.
Attachment #8728913 - Flags: review?(ejpbruel)
Comment on attachment 8728913 [details] [diff] [review]
Give wasm Debugger.Sources the text/wasm mimetype and use their .text property.

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

Easy patch. I have a few questions, but otherwise looks good to go.

::: devtools/server/actors/source.js
@@ +338,4 @@
>        if (this.source &&
>            this.source.text !== "[no source]" &&
>            this._contentType &&
> +          (this._contentType.indexOf('javascript') !== -1 ||

Nit: I know the line above it also didn't do it, but I prefer using "" over '' for strings.

::: devtools/server/actors/utils/TabSources.js
@@ +306,5 @@
>          if (url.indexOf("Scratchpad/") === 0 ||
>              url.indexOf("javascript:") === 0 ||
>              url === "debugger eval code") {
>            spec.contentType = "text/javascript";
> +        } else if (aSource.introductionType === "wasm") {

Gah, yet another special case here :-(

My understanding is that introductionType is meant to indicate *what* introduced this particular source, be it an event handler, an eval statement, the Function constructor, etc. Within that light, does it make sense to consider wasm as another thing that can introduce a source?

Assuming you've already thought that through, how about moving this if branch to the top, and adding a comment like 'if this source was introduced by wasm, assume its MIME type is type/wasm. Otherwise, try to obtain the mime type from the URL'.
Attachment #8728913 - Flags: review?(ejpbruel) → review+

Comment 3

2 years ago
(In reply to Eddy Bruel [:ejpbruel] from comment #2)
> Comment on attachment 8728913 [details] [diff] [review]
> Give wasm Debugger.Sources the text/wasm mimetype and use their .text
> property.
> 
> Review of attachment 8728913 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Easy patch. I have a few questions, but otherwise looks good to go.
> 
> ::: devtools/server/actors/utils/TabSources.js
> @@ +306,5 @@
> >          if (url.indexOf("Scratchpad/") === 0 ||
> >              url.indexOf("javascript:") === 0 ||
> >              url === "debugger eval code") {
> >            spec.contentType = "text/javascript";
> > +        } else if (aSource.introductionType === "wasm") {
> 
> Gah, yet another special case here :-(
> 
> My understanding is that introductionType is meant to indicate *what*
> introduced this particular source, be it an event handler, an eval
> statement, the Function constructor, etc. Within that light, does it make
> sense to consider wasm as another thing that can introduce a source?
> 

This is really a philosophical question. Wasm code doesn't have "sources" at all, they have to be synthesized. In that sense, these sources are "introduced by" wasm. Originally I had an additional .format property on Debugger.Source that returned "js" or "wasm", but I felt that was redundant. This isn't set in stone, but I see no compelling argument to changing it for the initial landing.

> Assuming you've already thought that through, how about moving this if
> branch to the top, and adding a comment like 'if this source was introduced
> by wasm, assume its MIME type is type/wasm. Otherwise, try to obtain the
> mime type from the URL'.

Sure, will do.

Comment 4

2 years ago
A nit on the review about "assume its content type". It isn't really an assumption like the JS cases, because having an intro type of "wasm" means we know exactly what type its source text should be treated as.

Updated

2 years ago
Blocks: 1255630

Comment 5

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d89e927c2b7b
https://hg.mozilla.org/mozilla-central/rev/d89e927c2b7b
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
You need to log in before you can comment on or make changes to this bug.