Closed Bug 1255630 Opened 4 years ago Closed 4 years ago

Add an experimental badge to wasm sources in the sources list

Categories

(DevTools :: Debugger, defect, P1)

defect

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.
It would be nice to have a quick-and-dirty version for GDC, by 3/14.
Flags: needinfo?(jlong)
Marking this as leave-open and NIing Helen to figure out a better longer term UX.
Flags: needinfo?(hholmes)
Keywords: leave-open
Depends on: 1254893, 1232014
Component: Developer Tools → Developer Tools: Debugger
Priority: -- → P2
Assignee: nobody → jlong
Flags: needinfo?(jlong)
I ran out of time on Friday to finish a prototype, but I'm going to try to have something by Monday.
Attached patch 1255630.patchSplinter Review
Does this message look right? I'll get someone else to check out the CSS changes too
Attachment #8730822 - Flags: review?(shu)
the badge. if you hover over it there is a tooltip that says "This is an experimental feature"
Attachment #8730822 - Flags: review?(bgrinstead)
Priority: P2 → P1
To test go here: http://rfrn.org/~shu/wasm-test.html and click the button to add a wasm source
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+
Attachment #8730822 - Flags: review?(shu) → review+
(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
Any reason this wasn't marked fixed?
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
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: 4 years ago
Flags: needinfo?(hholmes)
Resolution: --- → FIXED
Sorry, was mentioned in the response to the review, but it's bug 1257238
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.