910955 - Implement a live WebGL shader editor

Status: RESOLVED FIXED

(DevTools Graveyard :: WebGL Shader Editor, defect)

Comment 1

[Victor Porof [:vporof][:vp]]

Attachment: webgl-frontend.patch

Still needs some more tests, which I haven't attached yet.

Comment 2

[Victor Porof [:vporof][:vp]]

Attachment: webgl-fix-toolbox-test.patch

Make sure 'browser_toolbox_options.js' doesn't assume all tools are enabled while testing.

Comment 3

[Victor Porof [:vporof][:vp]]

Looks good so far: https://tbpl.mozilla.org/?tree=Try&rev=84ee2ad67ab4

Comment 4

[Victor Porof [:vporof][:vp]]

Comment on attachment 812676 [details] [diff] [review]
webgl-frontend.patch

Rob practically begged me to review this.

Comment 5

[Victor Porof [:vporof][:vp]]

Comment on attachment 813193 [details] [diff] [review]
webgl-fix-toolbox-test.patch

And this.

Comment 6

[Victor Porof [:vporof][:vp]]

Comment on attachment 813193 [details] [diff] [review]
webgl-fix-toolbox-test.patch

Probably no longer needed.

Comment 7

[Dave Camp (:dcamp)]

Comment on attachment 812676 [details] [diff] [review]
webgl-frontend.patch

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

Nice and easy-to-understand.

::: browser/devtools/shadereditor/panel.js
@@ +20,5 @@
> +
> +exports.ShaderEditorPanel = ShaderEditorPanel;
> +
> +ShaderEditorPanel.prototype = {
> +  open: function() {

Any chance of open being called twice?  Should we save the init promise and return it again in that case?

@@ +29,5 @@
> +      targetPromise = this.target.makeRemote();
> +    } else {
> +      targetPromise = promise.resolve(this.target);
> +    }
> +

target.makeRemote() will do the right thing if the target has already been made remote, you shouldn't need this if statement.

@@ +42,5 @@
> +        this.emit("ready");
> +        return this;
> +      })
> +      .then(null, function onError(aReason) {
> +        Cu.reportError("ShaderEditorPanel open failed. " +

Why not console.error?

::: browser/devtools/shadereditor/shadereditor.js
@@ +191,5 @@
> +        fs: fragmentShaderText
> +      });
> +    }
> +
> +    getShaders().then(getSources).then(showSources).then(null, Cu.reportError);

Same console.error question

Comment 8

[Victor Porof [:vporof][:vp]]

Attachment: webgl-frontend-tests.patch

I felt a bit uneasy landing this as is, so I added a dozen more tests to be sure things won't regress in the future.

Comment 9

[Victor Porof [:vporof][:vp]]

Try: https://tbpl.mozilla.org/?tree=Try&rev=8090f88e78c1

Comment 10

[Victor Porof [:vporof][:vp]]

https://hg.mozilla.org/integration/fx-team/rev/3097d7118a18

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3097d7118a18

Comment 12

[Florian Bender]

Is there a particular reason this is not mentioned in the Fx 27 release notes?

Comment 13

[Victor Porof [:vporof][:vp]]

It never came up. I'd be more than happy if someone added it.

Comment 14

[Florian Bender]

For relnote, remember that the Shader Editor is hidden by default thus needs to be enabled in the dev tools preferences.

(I was reading a post on TNW[1] that wondered about the missing Shader Editor in the relnotes, speculating it may not be ready for beta/release. Maybe somebody can reach out to them?)

Comment 15

[Victor Porof [:vporof][:vp]]

(In reply to Florian Bender from comment #14)
> For relnote, remember that the Shader Editor is hidden by default thus needs
> to be enabled in the dev tools preferences.
> 
> (I was reading a post on TNW[1] that wondered about the missing Shader
> Editor in the relnotes, speculating it may not be ready for beta/release.
> Maybe somebody can reach out to them?)

The Shader Editor is hidden by default ("disabled" is a too strong word) because most Web content out there doesn't contain WebGL, and horizontal estate in the Toolbox tabbar isn't cheap, so we're avoiding showing a tab which won't be primary to web developers.

Web developers who regularly deal with WebGL can choose show the tab via the Options panel in the Toolbox.

We have bug 937725 which plans to more gracefully deal with this situation.

I'm not sure if adding a relnote would make more sense after that bug is fixed, or now, with a "can be shown via the Toolbox options tab" addendum. Either way is fine by me, but bug 937725 probably won't be getting attention very soon.

Comment 16

[Alfred Kayser]

The Shader Editor is very nice, and is worthy of a nice little icon for itself instead of using the styleeditor icon.

Comment 17

[Victor Porof [:vporof][:vp]]

Thanks! :)
Bug 938290.

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Firefox 27 has been released without this in the release notes.
Maybe we could add this in the release notes when it will be displayed by default.