Closed
Bug 1232014
Opened 9 years ago
Closed 8 years ago
Stub out wasm source files in the Debugger
Categories
(DevTools :: Debugger, defect)
DevTools
Debugger
Tracking
(firefox48 fixed)
RESOLVED
FIXED
Firefox 48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: jsantell, Unassigned)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
3.14 KB,
patch
|
ejpbruel
:
review+
|
Details | Diff | Splinter Review |
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.
Comment 1•8 years ago
|
||
Attachment #8728913 -
Flags: review?(ejpbruel)
Comment 2•8 years ago
|
||
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•8 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•8 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.
Comment 6•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d89e927c2b7b
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•