Closed Bug 910955 Opened 7 years ago Closed 7 years ago

Implement a live WebGL shader editor

Categories

(DevTools Graveyard :: WebGL Shader Editor, defect)

defect
Not set
normal

Tracking

(relnote-firefox -)

RESOLVED FIXED
Firefox 27
Tracking Status
relnote-firefox --- -

People

(Reporter: vporof, Assigned: vporof)

References

Details

(Keywords: feature, relnote, Whiteboard: [qa-])

Attachments

(2 files, 1 obsolete file)

No description provided.
Depends on: 910953
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Still needs some more tests, which I haven't attached yet.
Attachment #812676 - Flags: review?(dcamp)
Attached patch webgl-fix-toolbox-test.patch (obsolete) — Splinter Review
Make sure 'browser_toolbox_options.js' doesn't assume all tools are enabled while testing.
Attachment #813193 - Flags: review?(dcamp)
Comment on attachment 812676 [details] [diff] [review]
webgl-frontend.patch

Rob practically begged me to review this.
Attachment #812676 - Flags: review?(dcamp) → review?(rcampbell)
Comment on attachment 813193 [details] [diff] [review]
webgl-fix-toolbox-test.patch

And this.
Attachment #813193 - Flags: review?(dcamp) → review?(rcampbell)
Attachment #812676 - Flags: review?(rcampbell) → review?(dcamp)
Comment on attachment 813193 [details] [diff] [review]
webgl-fix-toolbox-test.patch

Probably no longer needed.
Attachment #813193 - Flags: review?(rcampbell)
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
Attachment #812676 - Flags: review?(dcamp) → review+
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.
Attachment #813193 - Attachment is obsolete: true
Attachment #817803 - Flags: review?(dcamp)
Attachment #817803 - Flags: review?(dcamp) → review+
Blocks: 929916
Blocks: 930928
Component: Developer Tools → Developer Tools: WebGL Shader Editor
https://hg.mozilla.org/mozilla-central/rev/3097d7118a18
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 27
Keywords: feature
Depends on: 938290
Is there a particular reason this is not mentioned in the Fx 27 release notes?
Flags: needinfo?(vporof)
It never came up. I'd be more than happy if someone added it.
Flags: needinfo?(vporof)
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?)
Keywords: relnote
(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.
The Shader Editor is very nice, and is worthy of a nice little icon for itself instead of using the styleeditor icon.
Thanks! :)
Bug 938290.
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.
relnote-firefox: ? → ---
Flags: in-testsuite+
Whiteboard: [qa-]
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.