1255630 - Add an experimental badge to wasm sources in the sources list

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P1)

Description

[Shu-yu Guo [:shu]]

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.

Comment 1

[Shu-yu Guo [:shu]]

It would be nice to have a quick-and-dirty version for GDC, by 3/14.

Comment 2

[Shu-yu Guo [:shu]]

Marking this as leave-open and NIing Helen to figure out a better longer term UX.

Comment 3

[James Long (:jlongster)]

I ran out of time on Friday to finish a prototype, but I'm going to try to have something by Monday.

Comment 4

[James Long (:jlongster)]

Attachment: 1255630.patch

Does this message look right? I'll get someone else to check out the CSS changes too

Comment 5

[James Long (:jlongster)]

Attachment: Screen Shot 2016-03-15 at 1.44.38 PM.png

the badge. if you hover over it there is a tooltip that says "This is an experimental feature"

Comment 6

[James Long (:jlongster)]

To test go here: http://rfrn.org/~shu/wasm-test.html and click the button to add a wasm source

Comment 7

[Brian Grinstead [:bgrins]]

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

Comment 8

[James Long (:jlongster)]

(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 9

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/7afcc6cf189d

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7afcc6cf189d

Comment 11

[James Long (:jlongster)]

Any reason this wasn't marked fixed?

Comment 12

[James Long (:jlongster)]

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.

Comment 13

[Brian Grinstead [:bgrins]]

Resolving and clearing needinfo.  Further discussion should move into the bug James mentions in Comment 12 (although he didn't say the bug #).

Comment 14

[James Long (:jlongster)]

Sorry, was mentioned in the response to the review, but it's bug 1257238