940541 - Convert to Promise.jsm in the shader editor

Status: RESOLVED FIXED

(DevTools Graveyard :: WebGL Shader Editor, defect, P2)

Comment 1

[Brandon Benvie [:benvie]]

Attachment: promise-shadereditor.patch

My initial work on this.

Comment 2

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 940541

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.

Comment 3

[Brandon Benvie [:benvie]]

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.

Comment 4

[Brandon Benvie [:benvie]]

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

Comment 5

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

Will review this asap!

Comment 6

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 940541

Added fixes from Benvie's changes

Comment 7

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

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?

Comment 8

[Brandon Benvie [:benvie]]

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

Do not ever again speak such terrible thoughts!

Comment 9

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 940541

* 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

Comment 10

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 940541

Added r+'s from benvie, vp

Comment 11

[Ryan VanderMeulen [:RyanVM]]

This is bitrotted. Please rebase.

Comment 12

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 940541

Fixed bitrotted patch

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/14d1050e721b

Comment 14

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/ef4c8f6e3ef7 for mostly-Windows bustage - one Mac, https://tbpl.mozilla.org/php/getParsedLog.php?id=31349410&tree=Fx-Team, so far, but Windows says

https://tbpl.mozilla.org/php/getParsedLog.php?id=31351844&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=31350187&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=31351437&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=31350235&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=31349529&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=31352713&tree=Fx-Team

that it's thoroughly unhappy with it.

Comment 15

[Phil Ringnalda (:philor)]

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.

Comment 16

[Brandon Benvie [:benvie]]

I'm taking this over since I have a machine for Windows dev.

Comment 17

[Brandon Benvie [:benvie]]

Attachment: promise-shadereditor.patch

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

Comment 18

[Brandon Benvie [:benvie]]

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b86d25a3c048