Closed
Bug 1255630
Opened 8 years ago
Closed 8 years ago
Add an experimental badge to wasm sources in the sources list
Categories
(DevTools :: Debugger, defect, P1)
DevTools
Debugger
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: shu, Assigned: jlong)
References
Details
Attachments
(2 files)
After bug 1254893 and bug 1232014 land, wasm sources should "just work" and show up in the debugger tab. We should add a badge or disclaimer somewhere saying this is all very experimental, however, as many features, like setting breakpoints, do not work when viewing a wasm source.
Reporter | ||
Comment 1•8 years ago
|
||
It would be nice to have a quick-and-dirty version for GDC, by 3/14.
Flags: needinfo?(jlong)
Reporter | ||
Comment 2•8 years ago
|
||
Marking this as leave-open and NIing Helen to figure out a better longer term UX.
Flags: needinfo?(hholmes)
Reporter | ||
Updated•8 years ago
|
Keywords: leave-open
Reporter | ||
Updated•8 years ago
|
Updated•8 years ago
|
Component: Developer Tools → Developer Tools: Debugger
Updated•8 years ago
|
Priority: -- → P2
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → jlong
Flags: needinfo?(jlong)
Assignee | ||
Comment 3•8 years ago
|
||
I ran out of time on Friday to finish a prototype, but I'm going to try to have something by Monday.
Assignee | ||
Comment 4•8 years ago
|
||
Does this message look right? I'll get someone else to check out the CSS changes too
Attachment #8730822 -
Flags: review?(shu)
Assignee | ||
Comment 5•8 years ago
|
||
the badge. if you hover over it there is a tooltip that says "This is an experimental feature"
Assignee | ||
Updated•8 years ago
|
Attachment #8730822 -
Flags: review?(bgrinstead)
Assignee | ||
Updated•8 years ago
|
Priority: P2 → P1
Assignee | ||
Comment 6•8 years ago
|
||
To test go here: http://rfrn.org/~shu/wasm-test.html and click the button to add a wasm source
Comment 7•8 years ago
|
||
Comment on attachment 8730822 [details] [diff] [review] 1255630.patch Review of attachment 8730822 [details] [diff] [review]: ----------------------------------------------------------------- seems generally ok, but the test page starts to throw errors when trying to set a bp on either source once I do 'make me a wasm'. Known issue? TypeError: ({}) does not refer to a JS script Stack trace: getScriptsByURLAndLine@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/utils/ScriptStore.js:167:1 getScriptsBySourceActorAndLine@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/utils/ScriptStore.js:91:12 ::: devtools/client/debugger/content/views/sources-view.js @@ +241,5 @@ > contents.setAttribute("flex", "1"); > contents.setAttribute("tooltiptext", unicodeUrl); > > + if (aSource.introductionType === "wasm") { > + const wasm = document.createElement("vbox"); Using box instead of vbox seems more correct (even though I know the icon is pos:absolute so nbd) @@ +242,5 @@ > contents.setAttribute("tooltiptext", unicodeUrl); > > + if (aSource.introductionType === "wasm") { > + const wasm = document.createElement("vbox"); > + wasm.className = "dbg-wasm-item"; Don't need this class name - it isn't used anywhere @@ +243,5 @@ > > + if (aSource.introductionType === "wasm") { > + const wasm = document.createElement("vbox"); > + wasm.className = "dbg-wasm-item"; > + const icon = document.createElement("div"); You'll need to do createElementNS if you want an HTML div. But I don't think it needs to be, so this should be just a 'box' so it's more clear that it's in XUL ns @@ +249,5 @@ > + icon.className = "icon"; > + wasm.appendChild(icon); > + wasm.appendChild(contents); > + > + contents = wasm; I was afraid that this might break some child css selectors but there don't seem to be any. Or that some js would be counting on dom ordering. Is there a plan / bug to remove this warning? We should make sure this code and CSS gets cleaned out at some point ::: devtools/client/themes/debugger.css @@ +29,5 @@ > padding: 2px 0px; > } > > +.dbg-wasm-item .icon { > + content: " "; Why `content`? Presumably this used to be ::before but isn't anymore. @@ +39,5 @@ > + background-position: -24px -24px; > + width: 10px; > + height: 10px; > + position: absolute; > + margin-left: -15px; This should be margin-inline-start
Attachment #8730822 -
Flags: review?(bgrinstead) → review+
Reporter | ||
Updated•8 years ago
|
Attachment #8730822 -
Flags: review?(shu) → review+
Assignee | ||
Comment 8•8 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #7) > Comment on attachment 8730822 [details] [diff] [review] > 1255630.patch > > Review of attachment 8730822 [details] [diff] [review]: > ----------------------------------------------------------------- > > seems generally ok, but the test page starts to throw errors when trying to > set a bp on either source once I do 'make me a wasm'. Known issue? > > TypeError: ({}) does not refer to a JS script > Stack trace: > getScriptsByURLAndLine@resource://gre/modules/commonjs/toolkit/loader.js -> > resource://devtools/server/actors/utils/ScriptStore.js:167:1 > getScriptsBySourceActorAndLine@resource://gre/modules/commonjs/toolkit/ > loader.js -> resource://devtools/server/actors/utils/ScriptStore.js:91:12 Yes, very little works. Basically just showing the text. That's why we want to put this badge on it. > > ::: devtools/client/debugger/content/views/sources-view.js > @@ +241,5 @@ > > contents.setAttribute("flex", "1"); > > contents.setAttribute("tooltiptext", unicodeUrl); > > > > + if (aSource.introductionType === "wasm") { > > + const wasm = document.createElement("vbox"); > > Using box instead of vbox seems more correct (even though I know the icon is > pos:absolute so nbd) Ah cool, done > > @@ +242,5 @@ > > contents.setAttribute("tooltiptext", unicodeUrl); > > > > + if (aSource.introductionType === "wasm") { > > + const wasm = document.createElement("vbox"); > > + wasm.className = "dbg-wasm-item"; > > Don't need this class name - it isn't used anywhere This is the class used in CSS to style the icon > > @@ +243,5 @@ > > > > + if (aSource.introductionType === "wasm") { > > + const wasm = document.createElement("vbox"); > > + wasm.className = "dbg-wasm-item"; > > + const icon = document.createElement("div"); > > You'll need to do createElementNS if you want an HTML div. But I don't > think it needs to be, so this should be just a 'box' so it's more clear that > it's in XUL ns I switched it to `box`; I actually wanted a XUL element for the tooltip. (and to stay consistent with things around it) > > @@ +249,5 @@ > > + icon.className = "icon"; > > + wasm.appendChild(icon); > > + wasm.appendChild(contents); > > + > > + contents = wasm; > > I was afraid that this might break some child css selectors but there don't > seem to be any. Or that some js would be counting on dom ordering. > > Is there a plan / bug to remove this warning? We should make sure this code > and CSS gets cleaned out at some point > Just filed bug 1257238 > ::: devtools/client/themes/debugger.css > @@ +29,5 @@ > > padding: 2px 0px; > > } > > > > +.dbg-wasm-item .icon { > > + content: " "; > > Why `content`? Presumably this used to be ::before but isn't anymore. Yep! Removed > > @@ +39,5 @@ > > + background-position: -24px -24px; > > + width: 10px; > > + height: 10px; > > + position: absolute; > > + margin-left: -15px; > > This should be margin-inline-start Done
Comment 10•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7afcc6cf189d
Assignee | ||
Comment 11•8 years ago
|
||
Any reason this wasn't marked fixed?
Assignee | ||
Comment 12•8 years ago
|
||
Oh, it had leave-open. I filed another bug to remove this and I say we close this and use that bug for the longer-term solution.
Keywords: leave-open
Comment 13•8 years ago
|
||
Resolving and clearing needinfo. Further discussion should move into the bug James mentions in Comment 12 (although he didn't say the bug #).
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: needinfo?(hholmes)
Resolution: --- → FIXED
Assignee | ||
Comment 14•8 years ago
|
||
Sorry, was mentioned in the response to the review, but it's bug 1257238
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•