Closed Bug 1470086 Opened 2 years ago Closed 2 years ago

Standardize Shader editor initialization

Categories

(DevTools Graveyard :: WebGL Shader Editor, enhancement)

enhancement
Not set
normal

Tracking

(firefox63 fixed)

RESOLVED FIXED
Firefox 63
Tracking Status
firefox63 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Per the following RFC:
  https://github.com/devtools-html/rfcs/issues/48
And in order to unblock Promise.jsm removal and especially allow landing bug 1388054, we should refactor shadereditor.js to stop being evaluated as a document script, but rather it be loaded as a module, via DevTools module loader.
Assignee: nobody → poirot.alex
Refactoring this fixes the test failure raised by bug 1388054 when switching defer from Promise.jsm to native promises:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=e85c416bad7841c494d115c25d831998d3b0c96c

Somehow, this small change made to defer should make it so that one `async` function used in shadereditor.js becomes a zombie during panel's destruction:
  https://searchfox.org/mozilla-central/search?q=async&case=false&regexp=false&path=shadereditor.js
Comment on attachment 8986712 [details]
Bug 1470086 - Refactor shader editor initializer.

https://reviewboard.mozilla.org/r/252002/#review258488

::: devtools/client/definitions.js:238
(Diff revision 1)
> +    const browserRequire = BrowserLoader({
> +      baseURI: "resource://devtools/client/shadereditor/",
> +      window: iframeWindow
> +    }).require;
> +    const { ShaderEditorPanel } = browserRequire("devtools/client/shadereditor/panel");
> +    return new ShaderEditorPanel(toolbox);

Here is the first main change.

Instead of loading `panel` modules via always-alive-singleton DevTools modules loader, we load it via a per-panel BrowserLoader.

There is no need to pass `iframeWindow` anymore to the panel class as it will be `window` global.

::: devtools/client/jar.mn:47
(Diff revision 1)
>      content/debugger/views/options-view.js (debugger/views/options-view.js)
>      content/debugger/views/stack-frames-view.js (debugger/views/stack-frames-view.js)
>      content/debugger/views/stack-frames-classic-view.js (debugger/views/stack-frames-classic-view.js)
>      content/debugger/views/filter-view.js (debugger/views/filter-view.js)
>      content/debugger/utils.js (debugger/utils.js)
> -    content/shadereditor/shadereditor.xul (shadereditor/shadereditor.xul)
> +    content/shadereditor/index.xul (shadereditor/index.xul)

Then shadereditor.xul is renamed to index.xul.
We may do this in one bug/patch across all tools.

::: devtools/client/shadereditor/index.xul:1
(Diff revision 1)
> +<?xml version="1.0"?>

I don't know why it appears as a new file, but this is just shadereditor.xul with the <script> tag for shadereditor.js removed.

::: devtools/client/shadereditor/moz.build:8
(Diff revision 1)
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  DevToolsModules(
> -    'panel.js'
> +    'panel.js',
> +    'shadereditor.js'

The script loaded via script tag is now a module, so it is loaded from resource:// url instead of chrome://. (We have to load html/xul from chrome in order to be privileged, but they can load script from any scheme)

::: devtools/client/shadereditor/panel.js:49
(Diff revision 1)
> -        this.panelWin.gToolbox = this._toolbox;
> -        this.panelWin.gTarget = this.target;
> -        this.panelWin.gFront = new WebGLFront(this.target.client, this.target.form);
> -        return this.panelWin.startupShaderEditor();
> -      })
> -      .then(() => {
> +    this.EventsHandler = new EventsHandler();
> +    this.ShadersEditorsView = new ShadersEditorsView();
> +    await this.ShadersListView.initialize(this._toolbox, this.ShadersEditorsView);
> +    await this.EventsHandler.initialize(this, this._toolbox, this.target, this.front,
> +                                        this.ShadersListView);
> +    await this.ShadersEditorsView.initialize(this, this.ShadersListView);

This change is very specific to shadereditor.js.

May be some other tools are using singletons (like EventsHandler, ShaderEditorsView, ...) and globals (like gFront, gToolbox). But I feel like it doesn't fit modules behavior. Ideally each of these (EventsHandler, ShaderEditorsView and ShadersListView) would be in distinct modules and so won't be able to shared globals like gFront, gToolbox.
So I converted them to instanciable classes, receiving toolbox and front as arguments.

We may iterate over this by moving these classes to individual modules.

::: devtools/client/shadereditor/shadereditor.js
(Diff revision 1)
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  "use strict";
>  
> -/* exported startupShaderEditor, shutdownShaderEditor */
> -
> -const {require} = ChromeUtils.import("resource://devtools/shared/Loader.jsm", {});

This file loaded as script tag no longer need to create a BrowserLoaded as it is now done from definitions.js

::: devtools/client/shadereditor/shadereditor.js
(Diff revision 1)
>    require("devtools/client/shared/widgets/view-helpers");
>  
> -// Use privileged promise in panel documents to prevent having them to freeze
> -// during toolbox destruction. See bug 1402779.
> -// eslint-disable-next-line no-unused-vars
> -const Promise = require("Promise");

We no longer need to override document's `Promise` symbol as this is now a Sandbox one that isn't tied to document lifecycle.

::: devtools/client/shadereditor/shadereditor.js:126
(Diff revision 1)
>        $("#reload-notice").hidden = true;
>        $("#waiting-notice").hidden = false;
>      }
>  
>      $("#content").hidden = true;
> -    window.emit(EVENTS.UI_RESET);
> +    this.panel.emit(EVENTS.UI_RESET);

It is no longer relevant to emit events on the window object. It feels hacky to decorate window object with EventEmitter API.
So instead of emitting on the window, we now emit on the panel, that is easily accessible from tests.

::: devtools/client/shadereditor/test/browser_se_bfcache.js:9
(Diff revision 1)
>  /**
>   * Tests if the shader editor works with bfcache.
>   */
>  async function ifWebGLSupported() {
>    const { target, panel } = await initShaderEditor(SIMPLE_CANVAS_URL);
> -  const { gFront, $, EVENTS, ShadersListView, ShadersEditorsView } = panel.panelWin;
> +  const { front, $, EVENTS, ShadersListView, ShadersEditorsView } = panel;

Finally, we go through all the tests to find the necessary symbols on `panel` rather than `panelWin`.
Your changes make sense to me! :)

Perhaps you can tweak the patch upload to record a file move for index.xul?
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #4)
> Perhaps you can tweak the patch upload to record a file move for index.xul?

Done, and asked for review from Julian.
Comment on attachment 8986712 [details]
Bug 1470086 - Refactor shader editor initializer.

https://reviewboard.mozilla.org/r/252002/#review258802

This looks good, thanks for the refactor! Just one small issue + the usual questions :)

One point still unclear for me: what is the role of panel.js vs shadereditor.js and how do we know what should go into each.
I'm fine with keeping panel.js as a separated file, but we should really try to make it consistent between panels. From what you did here, the shortlist seems to be:
- implement Panel API: open(), destroy(), target (getter)
- create/destroy necessary fronts
- create/destroy UI
- expose some objects for tests
- emits "ready"/"destroyed"

Can we try to stick to that for other panels as well?

::: devtools/client/shadereditor/panel.js:43
(Diff revision 2)
> -    } else {
> -      targetPromise = promise.resolve(this.target);
>      }
>  
> -    return targetPromise
> -      .then(() => {
> +    this.front = new WebGLFront(this.target.client, this.target.form);
> +    this.ShadersListView = new ShadersListView();

The usual convention is have this.myClass = new MyClass(); 

Any reason to keep the first letter uppercase here? We can address that later of course. (same comment applies to other properties and arguments in this review)

::: devtools/client/shadereditor/panel.js:46
(Diff revision 2)
> -        this.panelWin.gFront = new WebGLFront(this.target.client, this.target.form);
> -        return this.panelWin.startupShaderEditor();
> -      })
> -      .then(() => {
> +    await this.ShadersListView.initialize(this._toolbox, this.ShadersEditorsView);
> +    await this.EventsHandler.initialize(this, this._toolbox, this.target, this.front,
> +                                        this.ShadersListView);
> +    await this.ShadersEditorsView.initialize(this, this.ShadersListView);

We don't care here because this is the shader editor, but in theory we should 

await Promise.all([
  this.ShadersListView.initialize(...),
  ...
])

::: devtools/client/shadereditor/shadereditor.js:85
(Diff revision 2)
>    /**
>     * Remove events emitted by the current tab target.
>     */
> -  destroy: function() {
> -    gToolbox.off("host-changed", this._onHostChanged);
> -    gTarget.off("will-navigate", this._onTabWillNavigate);
> +  destroy() {
> +    this.toolbox.off("host-changed", this._onHostChanged);
> +    this.toolbox.off("will-navigate", this._onTabWillNavigate);

should be this.target
Attachment #8986712 - Flags: review?(jdescottes) → review+
(In reply to Julian Descottes [:jdescottes][:julian] from comment #7)

These three items are things that are obvious to be made from Panel:

> - implement Panel API: open(), destroy(), target (getter)

I'm not sure target is important? I think it ended up under "DevToolsPanelAPI" by mistake.

> - expose some objects for tests

I think this is just a workaround to avoid complex refactorings.
We may as well fetch symbols via require() from tests.
But it isn't easy to get access to the browserRequire of each panel.
May be if we make that easier, it would be a suggested way to fetch test objects?
Having said that, it may not cover all the usecases, you may still want to access variables on the Panel instance.

> - emits "ready"/"destroyed"

There is also "reloaded". I'm wondering what it means for the inspector?
Inspector becomes InspectorPanel? And so, the InspectorPanel class becomes gigantic?
Should Panel classes only do things specified in this list, or can they do whatever else in addition?


But it is harder to say that the two following points have to be made in Panel:

> - create/destroy necessary fronts

See bug 1453093, where the front is instanciated in a "accessibility-startup" module.
It is similar to what we do for inspector/highlighter/performance/... and all these fronts/clients
that we instanciate before the panel are displayed.
Now, we may say: "if the front isn't already created before panel opening, it should be done in panel class".

> - create/destroy UI

Here, Shader Editor is simple, we may be able to merge shader-editor.js's classes into panel.js.
But I don't think this is perfect. Ideally, Panel class would only instanciate one or some classes and may be destroy them (for me destroy path should be inexistant, but well, we have corner cases that justify it sometimes).
For shader editor, I would move all EventsHandler, ShadersListView and ShaderEditorsView classes into their own file/module and let panel.js instanciate them.
I imagine the current patch aligns with this goal. There is just this shader-editor.js module, including a bunch of random classes.

> Can we try to stick to that for other panels as well?
> 
> ::: devtools/client/shadereditor/panel.js:43
> (Diff revision 2)
> > -    } else {
> > -      targetPromise = promise.resolve(this.target);
> >      }
> >  
> > -    return targetPromise
> > -      .then(() => {
> > +    this.front = new WebGLFront(this.target.client, this.target.form);
> > +    this.ShadersListView = new ShadersListView();
> 
> The usual convention is have this.myClass = new MyClass(); 
> 
> Any reason to keep the first letter uppercase here? We can address that
> later of course. (same comment applies to other properties and arguments in
> this review)

No, it was just to avoid renaming all the variable names in shader-editor.js.
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e973dc385fd1
Refactor shader editor initializer. r=jdescottes
Backed out changeset e973dc385fd1 (bug 1470086) for devtools failures on browser_se_editors-contents.js

Backout: https://hg.mozilla.org/integration/autoland/rev/47fe84b3f3f7455959c0f6c1994a9c3e7f144bbe

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=e973dc385fd119f568cf9bb81cd5d9aaf09f9dde&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=success&selectedJob=184894149

Failure log: toolbar>https://treeherder.mozilla.org/logviewer.html#?job_id=184894149&repo=autoland&lineNumber=14047

04:43:57     INFO - TEST-START | devtools/client/shadereditor/test/browser_se_editors-contents.js
04:43:57     INFO - GECKO(843) | ++DOCSHELL 0x12685b800 == 2 [pid = 846] [id = {9aa410e6-f092-9e4a-b8b9-9d1e449c2693}]
04:43:57     INFO - GECKO(843) | ++DOMWINDOW == 3 (0x115740c00) [pid = 846] [serial = 3] [outer = 0x0]
04:43:57     INFO - GECKO(843) | ++DOMWINDOW == 4 (0x126610800) [pid = 846] [serial = 4] [outer = 0x115740c00]
04:43:57     INFO - GECKO(843) | [Child 846, Main Thread] WARNING: site security information will not be persisted: file /builds/worker/workspace/build/src/security/manager/ssl/nsSiteSecurityService.cpp, line 553
04:43:58     INFO - GECKO(843) | --DOCSHELL 0x12028b000 == 7 [pid = 843] [id = {39d0a257-f6b0-824a-b899-583a6ec02c8c}]
04:43:58     INFO - GECKO(843) | --DOCSHELL 0x12a392800 == 6 [pid = 843] [id = {5b433e65-bcc8-7740-bd6f-ee9b28055ac1}]
04:43:58     INFO - GECKO(843) | --DOCSHELL 0x12a728000 == 5 [pid = 843] [id = {dae74014-4a5c-d644-8097-052c6c6bcb91}]
04:43:58     INFO - GECKO(843) | --DOMWINDOW == 18 (0x12a4ede00) [pid = 843] [serial = 6] [outer = 0x0] [url = about:blank]
04:43:58     INFO - GECKO(843) | ++DOMWINDOW == 5 (0x1268b2400) [pid = 846] [serial = 5] [outer = 0x115740c00]
04:43:58     INFO - GECKO(843) | [Child 846, Main Thread] WARNING: NS_ENSURE_TRUE(nsContentUtils::IsJavascriptMIMEType(type)) failed: file /builds/worker/workspace/build/src/dom/script/ScriptLoader.cpp, line 1282
04:43:58     INFO - GECKO(843) | [Child 846, Main Thread] WARNING: NS_ENSURE_TRUE(nsContentUtils::IsJavascriptMIMEType(type)) failed: file /builds/worker/workspace/build/src/dom/script/ScriptLoader.cpp, line 1282
04:43:59     INFO - GECKO(843) | ++DOCSHELL 0x120270800 == 6 [pid = 843] [id = {5c294cba-fc23-cc43-af5f-07d15216f282}]
04:43:59     INFO - GECKO(843) | ++DOMWINDOW == 19 (0x10b25fa00) [pid = 843] [serial = 20] [outer = 0x0]
04:43:59     INFO - GECKO(843) | ++DOMWINDOW == 20 (0x10b282000) [pid = 843] [serial = 21] [outer = 0x10b25fa00]
04:43:59     INFO - GECKO(843) | ++DOMWINDOW == 21 (0x11d94f800) [pid = 843] [serial = 22] [outer = 0x10b25fa00]
04:43:59     INFO - GECKO(843) | ++DOCSHELL 0x1202bb800 == 7 [pid = 843] [id = {00294aec-9199-ab44-8774-9431700631f9}]
04:43:59     INFO - GECKO(843) | ++DOMWINDOW == 22 (0x10b25fc00) [pid = 843] [serial = 23] [outer = 0x0]
04:43:59     INFO - GECKO(843) | ++DOMWINDOW == 23 (0x11d98b000) [pid = 843] [serial = 24] [outer = 0x10b25fc00]
04:44:00     INFO - GECKO(843) | ++DOMWINDOW == 6 (0x12920b400) [pid = 846] [serial = 6] [outer = 0x115740c00]
04:44:00     INFO - GECKO(843) | [Child 846, Main Thread] WARNING: NS_ENSURE_TRUE(nsContentUtils::IsJavascriptMIMEType(type)) failed: file /builds/worker/workspace/build/src/dom/script/ScriptLoader.cpp, line 1282
04:44:00     INFO - GECKO(843) | [Child 846, Main Thread] WARNING: NS_ENSURE_TRUE(nsContentUtils::IsJavascriptMIMEType(type)) failed: file /builds/worker/workspace/build/src/dom/script/ScriptLoader.cpp, line 1282
04:44:01     INFO - GECKO(843) | --DOMWINDOW == 6 (0x116d58000) [pid = 844] [serial = 2] [outer = 0x0] [url = about:blank]
04:44:01     INFO - GECKO(843) | --DOMWINDOW == 5 (0x11f0e5400) [pid = 844] [serial = 6] [outer = 0x0] [url = about:blank]
04:44:01     INFO - GECKO(843) | ++DOCSHELL 0x12a730000 == 8 [pid = 843] [id = {c16c208f-e2a2-a441-a968-0258bf72c62d}]
04:44:01     INFO - GECKO(843) | ++DOMWINDOW == 24 (0x12a4eee00) [pid = 843] [serial = 25] [outer = 0x0]
04:44:01     INFO - GECKO(843) | ++DOCSHELL 0x12b10d000 == 9 [pid = 843] [id = {79c27e87-b257-ca42-992d-712f378f8f93}]
04:44:01     INFO - GECKO(843) | ++DOMWINDOW == 25 (0x12a4efc00) [pid = 843] [serial = 26] [outer = 0x0]
04:44:01     INFO - GECKO(843) | ++DOMWINDOW == 26 (0x12c657400) [pid = 843] [serial = 27] [outer = 0x12a4eee00]
04:44:01     INFO - GECKO(843) | ++DOMWINDOW == 27 (0x12eb07c00) [pid = 843] [serial = 28] [outer = 0x12a4efc00]
04:44:01     INFO - GECKO(843) | --DOCSHELL 0x11374b800 == 0 [pid = 847] [id = {53d423e7-436f-1c4e-b929-cc2e60b3491e}]
04:44:02     INFO - GECKO(843) | --DOCSHELL 0x11f41b000 == 1 [pid = 846] [id = {7dc26944-1273-d241-bf0d-f2a092584767}]
04:44:02     INFO - TEST-INFO | started process screencapture
04:44:03     INFO - TEST-INFO | screencapture: exit 0
04:44:03     INFO - Buffered messages logged at 04:43:57
04:44:03     INFO - Initializing a shader editor pane.
04:44:03     INFO - Adding a new tab with URL: http://example.com/browser/devtools/client/shadereditor/test/doc_simple-canvas.html
04:44:03     INFO - Buffered messages logged at 04:43:58
04:44:03     INFO - Console message: [JavaScript Warning: "Use of nsIFile in content process is deprecated." {file: "resource://gre/modules/FileUtils.jsm" line: 170}]
04:44:03     INFO - Tab added and finished loading
04:44:03     INFO - Buffered messages logged at 04:44:00
04:44:03     INFO - Waiting for event: 'navigate' on TabTarget:[object XULElement].
04:44:03     INFO - Waiting for event: 'program-linked' on [Front for webgl/server1.conn1.child1/webglActor6].
04:44:03     INFO - Waiting for event: 'ShaderEditor:SourcesShown' on [object Object].
04:44:03     INFO - Buffered messages logged at 04:44:01
04:44:03     INFO - Got event: 'program-linked' on [Front for webgl/server1.conn1.child1/webglActor6].
04:44:03     INFO - Got event: 'navigate' on TabTarget:[object XULElement].
04:44:03     INFO - Buffered messages logged at 04:44:02
04:44:03     INFO - Got event: 'ShaderEditor:SourcesShown' on [object Object].
04:44:03     INFO - Buffered messages finished
04:44:03     INFO - TEST-UNEXPECTED-FAIL | devtools/client/shadereditor/test/browser_se_editors-contents.js | Got an error: ShadersEditorsView is undefined
Flags: needinfo?(poirot.alex)
I forgot to remove the capitals from tests and that this panel tests are only run on MacOS...
Flags: needinfo?(poirot.alex)
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4cdfe6546b58
Refactor shader editor initializer. r=jdescottes
https://hg.mozilla.org/mozilla-central/rev/4cdfe6546b58
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.