Convert to Promise.jsm in the shader editor

RESOLVED FIXED in Firefox 28

Status

defect
P2
normal
RESOLVED FIXED
6 years ago
a month ago

People

(Reporter: bbenvie, Assigned: bbenvie)

Tracking

(Blocks 1 bug)

Trunk
Firefox 28
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

6 years ago
Posted patch promise-shadereditor.patch (obsolete) — Splinter Review
My initial work on this.
Assignee: nobody → jsantell
Posted patch 940541 (obsolete) — Splinter Review
Again, first big patch (in addition to NetMonitor with Promise.jsm) refactoring, so give it a good eye over.

Some hacks working around "program-linked" events as they fire synchronously back-to-back, which caused issues with async resolutions, so some ugliness there.
Attachment #8335586 - Attachment is obsolete: true
Attachment #8337187 - Flags: review?(vporof)
Attachment #8337187 - Flags: review?(bbenvie)
(Assignee)

Comment 3

6 years ago
Comment on attachment 8337187 [details] [diff] [review]
940541

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

Excellente! r+ with some small changes.

::: browser/devtools/shadereditor/shadereditor.js
@@ +34,5 @@
> +  CONTAINER_READY: "ShaderEditor:ContainerReady",
> +
> +  // When onTabNavigated resets the UI
> +  UI_RESET: "ShaderEditor:UIReset",
> +  

Nit: whitespace

@@ +291,5 @@
>  
> +    getShaders()
> +      .then(getSources)
> +      .then(showSources)
> +      .then(() => window.emit(EVENTS.CONTAINER_READY));

You should keep the trailing `.then(null, Cu.reportError).

@@ +370,3 @@
>     */
>    setText: function(sources) {
> +    let view = this;

Shouldn't need to rebind `this` since you're using arrow functions.

@@ +424,3 @@
>     */
>    _toggleListeners: function(flag) {
> +    let view = this;

Same thing with rebinding `this`.

@@ +508,5 @@
>        return e.lineMatch && e.textMatch;
>      }
>      function sanitizeValidMatches(e) {
>        return {
> +        // Drivers might yield confusing line numbers under some obscure

lol

::: browser/devtools/shadereditor/test/browser_se_bfcache.js
@@ +16,5 @@
>    let navigated = navigate(target, MULTIPLE_CONTEXTS_URL);
> +  let [secondProgram, thirdProgram] = yield promise.all([
> +    once(gFront, "program-linked"),
> +    once(gFront, "program-linked")
> +  ]);

Use getProgramsFromWebGLFront here?

::: browser/devtools/shadereditor/test/browser_se_editors-error-gutter.js
@@ +21,2 @@
>    vsEditor.replaceText("vec3", { line: 7, ch: 22 }, { line: 7, ch: 26 });
> +  let [_, vertError] = yield onceSpread(panel.panelWin, EVENTS.SHADER_COMPILED);

Kind of a nit: let [, vertError] = ...

::: browser/devtools/shadereditor/test/browser_se_programs-blackbox.js
@@ +16,5 @@
>    reload(target);
> +  let [firstProgramActor, secondProgramActor] = yield promise.all([
> +    once(gFront, "program-linked"),
> +    once(gFront, "program-linked")
> +  ]);

Use getProgramsFromWebGLFront here?

::: browser/devtools/shadereditor/test/browser_se_programs-highlight.js
@@ +17,5 @@
> +
> +  let [firstProgramActor, secondProgramActor] = yield promise.all([
> +    once(gFront, "program-linked"),
> +    once(gFront, "program-linked")
> +  ]);

Use getProgramsFromWebGLFront here?

::: browser/devtools/shadereditor/test/browser_se_programs-list.js
@@ +38,4 @@
>  
> +  yield programsReady.promise;
> +
> +  let [firstProgramActor, secondProgramActor] = actors;

Use getProgramsFromWebGLFront here?

::: browser/devtools/shadereditor/test/browser_se_shaders-edit-02.js
@@ +21,2 @@
>    vsEditor.replaceText("vec3", { line: 7, ch: 22 }, { line: 7, ch: 26 });
> +  let [_, error] = yield onceSpread(panel.panelWin, EVENTS.SHADER_COMPILED);

Same thing with all the `_`'s here.

::: browser/devtools/shadereditor/test/browser_se_shaders-edit-03.js
@@ +14,5 @@
> +
> +  yield promise.all([
> +    once(gFront, "program-linked"),
> +    once(gFront, "program-linked")
> +  ]);

Use getProgramsFromWebGLFront here?

::: browser/devtools/shadereditor/test/head.js
@@ +137,5 @@
> +// Hack around `once`, as that only resolves to a single (first) argument
> +// and discards the rest. `onceSpread` is similar, except resolves to an
> +// array of all of the arguments in the handler. These should be consolidated
> +// into the same function, but many tests will need to be changed.
> +function onceSpread (aTarget, aEvent) {

Nit: no space between function name and parenthesis.

@@ +281,5 @@
> +// Due to `program-linked` events firing synchronously, we cannot
> +// just yield/chain them together, as then we miss all actors after the
> +// first event since they're fired consecutively. This allows us to capture
> +// all actors and returns an array containing them.
> +function getProgramsFromWebGLFront (front, count) {

Nit: no space between function name and parenthesis.
Attachment #8337187 - Flags: review?(bbenvie) → review+
Will review this asap!
Posted patch 940541 (obsolete) — Splinter Review
Added fixes from Benvie's changes
Attachment #8337187 - Attachment is obsolete: true
Attachment #8337187 - Flags: review?(vporof)
Attachment #8337952 - Flags: review?(vporof)
(Assignee)

Updated

6 years ago
Status: NEW → ASSIGNED
Comment on attachment 8337952 [details] [diff] [review]
940541

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

r+ with comments addressed.
Sweet patch!

::: browser/devtools/shadereditor/shadereditor.js
@@ +30,5 @@
>    // When a shader's source was edited and compiled via the editor.
> +  SHADER_COMPILED: "ShaderEditor:ShaderCompiled",
> +
> +  // When onProgramSelect completes loading a container
> +  CONTAINER_READY: "ShaderEditor:ContainerReady",

CONTAINER_READY isn't terribly descriptive, and I don't think it's necessary. Why wouldn't the other event (SOURCES_SHOWN) work in tests?

@@ +32,5 @@
> +
> +  // When onProgramSelect completes loading a container
> +  CONTAINER_READY: "ShaderEditor:ContainerReady",
> +
> +  // When onTabNavigated resets the UI

Let's avoid implementation details in these event descriptions.
"When a tab navigation resets the UI" is better imo.

@@ +36,5 @@
> +  // When onTabNavigated resets the UI
> +  UI_RESET: "ShaderEditor:UIReset",
> +
> +  // When the editor's error markers are all removed
> +  EDITOR_CLEANED: "ShaderEditor:EditorCleaned"

How about naming this EDITOR_ERROR_MARKERS_REMOVED then? EDITOR_CLEANED makes me think the text was removed.

@@ +129,5 @@
>          // Reset UI.
>          ShadersListView.empty();
> +        ShadersEditorsView.setText({ vs: "", fs: "" }).then(() => {
> +          $("#content").hidden = true;
> +          window.emit(EVENTS.UI_RESET);

If promises will ever become sync again (please no, but bare with me a bit), this won't work properly anymore.

How about doing a task spawn, yielding, doing everything else, and emitting the event at the end?

@@ +380,5 @@
> +        this._getEditor("vs").then(e => setTextAndClearHistory(e, sources.vs)),
> +        this._getEditor("fs").then(e => setTextAndClearHistory(e, sources.fs))
> +      ]);
> +    }).then(() => this._toggleListeners("on"))
> +      .then(() => window.emit(EVENTS.SOURCES_SHOWN, sources));

Using Task.spawn would make all of this look nicer here.

@@ +395,4 @@
>     */
>    _getEditor: function(type) {
>      if ($("#content").hidden) {
> +      return promise.reject(new Error("Shader Editor is still loading."));

..a better error message would be "Shader Editor is still waiting for a WebGL context to be created", since the content is also hidden after the target navigates.

::: browser/devtools/shadereditor/test/browser_se_editors-contents.js
@@ +15,5 @@
>  
>    let vsEditor = yield ShadersEditorsView._getEditor("vs");
>    let fsEditor = yield ShadersEditorsView._getEditor("fs");
>  
> +  yield once(panel.panelWin, EVENTS.CONTAINER_READY);

The sources might have been shown by now. Better to start waiting for the event earlier.

yield promise.all([
  once(gFront, "program-linked"),
  once(panel.panelWin, EVENTS.CONTAINER_READY)
]);

..with the minor addendum that EVENTS.CONTAINER_READY is probably unnecessary (see my earlier comment).

Same goes for all the other tests. The event might have already fired, in which case this yield would never finish.

::: browser/devtools/shadereditor/test/browser_se_navigation.js
@@ +39,5 @@
>    let navigated = once(target, "will-navigate");
>    navigate(target, "about:blank");
>  
>    yield navigating;
> +  yield once(panel.panelWin, EVENTS.UI_RESET);

Should probably wait for both events at the same time. yield promise.all([... ]);

::: browser/devtools/shadereditor/test/browser_se_programs-list.js
@@ +22,5 @@
> +  let [firstProgramActor, secondProgramActor] = yield getPrograms(gFront, 2, (actors) => {
> +    // Fired upon each actor addition, we want to check only
> +    // after the first actor has been added so we can test state
> +    if (actors.length === 1)
> +      checkFirstProgram();

Nit: can you have a checkSecondProgram here for consistency?
Attachment #8337952 - Flags: review?(vporof) → review+
(Assignee)

Comment 8

6 years ago
(In reply to Victor Porof [:vp] from comment #7)
> If promises will ever become sync again

Do not ever again speak such terrible thoughts!
Posted patch 940541 (obsolete) — Splinter Review
* Removed the CONTAINER_READY event -- was one of the first things I added, and is redundant with the SOURCES_SHOWN event at this point.
* Fixed up some nits, added Task spawns in there.
* Added the `checkSecondProgram` like you mentioned
* Where the SOURCES_SHOWN event is waiting, along with program-links, they've been combined into a promise.all so they're all listening immediately
Attachment #8337952 - Attachment is obsolete: true
Attachment #8339553 - Flags: review+
Posted patch 940541 (obsolete) — Splinter Review
Added r+'s from benvie, vp
Attachment #8339553 - Attachment is obsolete: true
Attachment #8339555 - Flags: review+
Attachment #8339555 - Attachment is patch: true
This is bitrotted. Please rebase.
Keywords: checkin-needed
Posted patch 940541 (obsolete) — Splinter Review
Fixed bitrotted patch
Attachment #8339555 - Attachment is obsolete: true
Attachment #8341262 - Flags: review+
https://hg.mozilla.org/integration/fx-team/rev/14d1050e721b
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://tbpl.mozilla.org/?tree=Try&rev=f86c9d5f0059, try: -b do -p all -u all[x64] -t none - pretty sure we don't run the shadereditor tests on Linux ("Skipping test because WebGL isn't supported"), so that try run with only tests on Linux64 wouldn't have actually run the affected tests, even if someone would have retriggered the debug browser-chrome that hit an infra failure before it got started.
(Assignee)

Comment 16

5 years ago
I'm taking this over since I have a machine for Windows dev.
Assignee: jsantell → bbenvie
Whiteboard: [fixed-in-fx-team]
(Assignee)

Comment 17

5 years ago
This should resolve the failures. By default, webgl canvas contexts do not retain their buffer outside of the requestAnimationFrame callback. Calling `context.readPixels(...)` outside of the rAF callback just results in black pixels. With sync promises the call to readPixels always happened during rAF, and with async promises it never does. The solution is to change all the initial places when a webgl context is retrieved (in the .html test file) to have an additional option: `canvas.getContext("webgl", { preserveDrawingBuffer : true })`. preserveDrawingBuffer removes the requirement that readPixels happen inside rAF.

https://tbpl.mozilla.org/?tree=Try&rev=290e9e03cf2e
Attachment #8341262 - Attachment is obsolete: true
Attachment #8343392 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/b86d25a3c048
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 28
Depends on: 947544

Updated

5 years ago
Blocks: 996671

Updated

10 months ago
Product: Firefox → DevTools

Updated

a month ago
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.