1250002 - about:debugging load React components using BrowserLoader

Status: RESOLVED FIXED

(DevTools :: about:debugging, defect)

Description

[Julian Descottes [:jdescottes]]

Follow up to Bug 1245029.

Using BrowserLoader & require to load react components would allow a direct access to the global window object. The window object is needed in AboutDebuggingApp to listen to hash changes and in AddonsControl.

Comment 1

[Julian Descottes [:jdescottes]]

Attachment: bug1250002.wip.patch

In this commit, all react components are loaded using require.
Inside the components themselves, for now I use exclusively require to load other dependencies. As you commented on a previous bug, we are losing some laziness.

I marked all the spots where a lazy require could still be used with the comment "// CHECK-LAZY".

One way to get back lazyRequireGetter & lazyImporter here is to import Cu from "chrome", and use it to import the loader. This is a bit verbose and I assume it also has a cost.

How do you think we should balance this ?

Comment 2

[Alexandre Poirot [:ochameau]]

Comment on attachment 8721812 [details] [diff] [review]
bug1250002.wip.patch

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

So BrowserLoader doesn't expose the magic "loader" in globals?
Note that this isn't using any chrome privileges or any special trick.
This is all based on Object.defineProperty()
  http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/loader/XPCOMUtils.jsm#178
  http://mxr.mozilla.org/mozilla-central/source/devtools/shared/Loader.jsm#294

But anyway, you should look closely, case by case, which dependency is really important to be loaded lazily.
Most React components isn't really worth loading lazily as they are going to be used anyway...
But some chrome/jsm/modules would be worth being loaded lazily as commented inline.

::: devtools/client/aboutdebugging/components/aboutdebugging.js
@@ +10,5 @@
> +
> +const React = require("devtools/client/shared/vendor/react");
> +const AddonsTab = React.createFactory(require("./addons-tab").AddonsTab);
> +const TabMenu = React.createFactory(require("./tab-menu").TabMenu);
> +const WorkersTab = React.createFactory(require("./workers-tab").WorkersTab);

These imports should be lazy loaded, otherwise we are going to load the world (all tabs) even if we just open just one tab.

::: devtools/client/aboutdebugging/components/addons-controls.js
@@ +12,5 @@
>  
> +const React = require("devtools/client/shared/vendor/react");
> +
> +// CHECK-LAZY
> +const { AddonManager } = require("resource://gre/modules/AddonManager.jsm");

This is going to be used anyway?

::: devtools/client/aboutdebugging/components/addons-tab.js
@@ +6,5 @@
>  
>  "use strict";
>  
> +// CHECK-LAZY
> +const { AddonManager } = require("resource://gre/modules/AddonManager.jsm");

Same?

::: devtools/client/aboutdebugging/components/target.js
@@ +15,5 @@
> +const { TargetFactory } = require("devtools/client/framework/target");
> +// CHECK-LAZY
> +const { gDevTools } = require("devtools/client/framework/devtools");
> +// CHECK-LAZY
> +const { Toolbox } = require("devtools/client/framework/toolbox");

See case by case which one is going to be use in any case and which one isn't.
BrowserToolboxProcess for example should be loaded lazily.

Comment 3

[Alexandre Poirot [:ochameau]]

Also. Please try to land your patches as soon as they are r+. It would help reading your patch. For example, components/aboutdebugging.js doesn't exists yet on fx-team and to better understand your patch, I have to look into various bugs.
On another way to soften this is to use mozreview that allows to see more than just the diff file.

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Alexandre Poirot [:ochameau] from comment #2)
> Comment on attachment 8721812 [details] [diff] [review]
> bug1250002.wip.patch
> 
> Review of attachment 8721812 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So BrowserLoader doesn't expose the magic "loader" in globals?
> Note that this isn't using any chrome privileges or any special trick.
> This is all based on Object.defineProperty()
>  
> http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/loader/XPCOMUtils.
> jsm#178
>  
> http://mxr.mozilla.org/mozilla-central/source/devtools/shared/Loader.jsm#294
> 
> But anyway, you should look closely, case by case, which dependency is
> really important to be loaded lazily.
> Most React components isn't really worth loading lazily as they are going to
> be used anyway...
> But some chrome/jsm/modules would be worth being loaded lazily as commented
> inline.

Seems like we could adapt BrowserLoader to expose `loader` and lazy methods if we want to.

Comment 5

[Julian Descottes [:jdescottes]]

(In reply to Alexandre Poirot [:ochameau] from comment #3)
> Also. Please try to land your patches as soon as they are r+. It would help
> reading your patch. For example, components/aboutdebugging.js doesn't exists
> yet on fx-team and to better understand your patch, I have to look into
> various bugs.
> On another way to soften this is to use mozreview that allows to see more
> than just the diff file.

I can't land patches for now :( so I depend on a sheriff to pick-up the checkin-needed keyword. 
But I'll push to mozreview from now on, thanks for the tip.

(In reply to Alexandre Poirot [:ochameau] from comment #2)
> Comment on attachment 8721812 [details] [diff] [review]
> bug1250002.wip.patch
> 
> Review of attachment 8721812 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So BrowserLoader doesn't expose the magic "loader" in globals?
> Note that this isn't using any chrome privileges or any special trick.
> This is all based on Object.defineProperty()
>  
> http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/loader/XPCOMUtils.
> jsm#178
>  
> http://mxr.mozilla.org/mozilla-central/source/devtools/shared/Loader.jsm#294
> 

For now it's not exposed. Doing so is pretty easy, just need to add it to the globals object at https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/browser-loader.js#109 . I will file a separate bug for this. In the meantime, I propose to simply load { loader } on the current global in initializer.js :

const { loader } = Components.utils.import("resource://devtools/shared/Loader.jsm", this);

> But anyway, you should look closely, case by case, which dependency is
> really important to be loaded lazily.
> Most React components isn't really worth loading lazily as they are going to
> be used anyway...
> But some chrome/jsm/modules would be worth being loaded lazily as commented
> inline.
> 
> ::: devtools/client/aboutdebugging/components/aboutdebugging.js
> @@ +10,5 @@
> > +
> > +const React = require("devtools/client/shared/vendor/react");
> > +const AddonsTab = React.createFactory(require("./addons-tab").AddonsTab);
> > +const TabMenu = React.createFactory(require("./tab-menu").TabMenu);
> > +const WorkersTab = React.createFactory(require("./workers-tab").WorkersTab);
> 
> These imports should be lazy loaded, otherwise we are going to load the
> world (all tabs) even if we just open just one tab.

Right. Since we will soon use factories (actually included them by mistake in the patch you reviewed), for the lazy loading to be effective we will probably need some additional helper. We would like to create the factory only when needed. Can do that in the next patch though.

> 
> ::: devtools/client/aboutdebugging/components/addons-controls.js
> @@ +12,5 @@
> >  
> > +const React = require("devtools/client/shared/vendor/react");
> > +
> > +// CHECK-LAZY
> > +const { AddonManager } = require("resource://gre/modules/AddonManager.jsm");
> 
> This is going to be used anyway?

Only if the user clicks on the button to add an add-on so it could be interesting to lazy load.

> 
> ::: devtools/client/aboutdebugging/components/addons-tab.js
> @@ +6,5 @@
> >  
> >  "use strict";
> >  
> > +// CHECK-LAZY
> > +const { AddonManager } = require("resource://gre/modules/AddonManager.jsm");
> 

Yes you're right it's used when the component is mounted, which should always happen if the file is loaded.

> 
> ::: devtools/client/aboutdebugging/components/target.js
> @@ +15,5 @@
> > +const { TargetFactory } = require("devtools/client/framework/target");
> > +// CHECK-LAZY
> > +const { gDevTools } = require("devtools/client/framework/devtools");
> > +// CHECK-LAZY
> > +const { Toolbox } = require("devtools/client/framework/toolbox");
> 
> See case by case which one is going to be use in any case and which one
> isn't.
> BrowserToolboxProcess for example should be loaded lazily.

All of them are only used when clicking on the debug button, (a different set depending on the category we are in). IMO all of them should be treated in the same way, which means lazily loaded here. Unless BrowserToolboxProcess is different for some reason ?

Comment 6

[Julian Descottes [:jdescottes]]

Attachment: MozReview Request: Bug 1250002 - part1: add loader to globals exposed by require from browser-loader;r=jryans

Review commit: https://reviewboard.mozilla.org/r/35819/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35819/

Comment 7

[Julian Descottes [:jdescottes]]

Comment on attachment 8721992 [details]
MozReview Request: Bug 1250002 - part1: add loader to globals exposed by require from browser-loader;r=jryans

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35819/diff/1-2/

Comment 8

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8721992 [details]
MozReview Request: Bug 1250002 - part1: add loader to globals exposed by require from browser-loader;r=jryans

https://reviewboard.mozilla.org/r/35819/#review32653

::: devtools/client/shared/browser-loader.js:129
(Diff revision 2)
> +      // Allow modules to use the DevtoolsLoader lazy loading helpers.

Nit: DevToolsLoader

::: devtools/client/shared/browser-loader.js:134
(Diff revision 2)
> +        lazyRequireGetter: devtools.lazyRequireGetter,

This one doesn't seem right: we need to use BrowserLoader's require, not the require from the outer / "regular" DevToolsLoader.  So, you'll probably need to reimplement this one.

Comment 9

[Julian Descottes [:jdescottes]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #8)
> Comment on attachment 8721992 [details]
> MozReview Request: Bug 1250002 - part1: add loader to globals exposed by
> require from browser-loader;r=jryans
> 
> https://reviewboard.mozilla.org/r/35819/#review32653
> 
> ::: devtools/client/shared/browser-loader.js:129
> (Diff revision 2)
> > +      // Allow modules to use the DevtoolsLoader lazy loading helpers.
> 
> Nit: DevToolsLoader
> 
> ::: devtools/client/shared/browser-loader.js:134
> (Diff revision 2)
> > +        lazyRequireGetter: devtools.lazyRequireGetter,
> 
> This one doesn't seem right: we need to use BrowserLoader's require, not the
> require from the outer / "regular" DevToolsLoader.  So, you'll probably need
> to reimplement this one.

Thanks for the review!

You're right, for some reason I thought we would like to have both a "DevToolsLoader" lazyRequireGetter and a "BrowserLoader" lazyRequireGetter (to be implemented later). Once we are inside a module, lazyRequireGetter should simply rely on the appropriate require. In the same way that require automatically maps to BrowserLoader.require or DevToolsLoader.require depending on the module's context.

In any case, the BrowserLoader's require falls back to the DevToolsLoader require when we specify a path that is outside of its baseURI.

Comment 10

[Julian Descottes [:jdescottes]]

Attachment: bug1250002.part1.wip.patch

Ryan: Before going back to review, can you give me preliminary feedback on the patch here? 

Adding the lazyRequireGetter in the current BrowserLoader felt ugly, because: `lazyRequireGetter` needs to rely on `require`, but `require` is only created after we passed `lazyRequireGetter` as an argument property for loaders.Loader. 

I decided to rely on a class here because I feel it makes it easier to understand/maintain. Tried to keep the changes to a minimum, let me know what you think.

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8722713 [details] [diff] [review]
bug1250002.part1.wip.patch

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

Looks correct to me, thanks.

Comment 12

[Julian Descottes [:jdescottes]]

Comment on attachment 8721992 [details]
MozReview Request: Bug 1250002 - part1: add loader to globals exposed by require from browser-loader;r=jryans

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35819/diff/2-3/

Comment 13

[Julian Descottes [:jdescottes]]

Attachment: MozReview Request: Bug 1250002 - part2: about:debugging switch to browserLoader;r=ochameau

Review commit: https://reviewboard.mozilla.org/r/36415/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36415/

Comment 14

[Julian Descottes [:jdescottes]]

Comment on attachment 8721992 [details]
MozReview Request: Bug 1250002 - part1: add loader to globals exposed by require from browser-loader;r=jryans

Thanks for the review! Missed the feedback? -> review+ ...
Carrying over r+ !

Comment 15

[Julian Descottes [:jdescottes]]

https://reviewboard.mozilla.org/r/35819/#review33005

Already reviewed as bugzilla attachment

Comment 16

[Julian Descottes [:jdescottes]]

Comment on attachment 8721992 [details]
MozReview Request: Bug 1250002 - part1: add loader to globals exposed by require from browser-loader;r=jryans

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35819/diff/3-4/

Comment 17

[Julian Descottes [:jdescottes]]

Comment on attachment 8723234 [details]
MozReview Request: Bug 1250002 - part2: about:debugging switch to browserLoader;r=ochameau

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36415/diff/1-2/

Comment 18

[Julian Descottes [:jdescottes]]

Try failures were most likely linked to a rebase on top of a bad revision. 
Rebased again, try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=7924df3d6025

Comment 19

[Alexandre Poirot [:ochameau]]

Comment on attachment 8723234 [details]
MozReview Request: Bug 1250002 - part2: about:debugging switch to browserLoader;r=ochameau

https://reviewboard.mozilla.org/r/36415/#review33111

Looks good.

::: devtools/client/aboutdebugging/components/aboutdebugging.js:12
(Diff revision 2)
> -  "devtools/client/aboutdebugging/components/addons-tab", true);
> +const TabMenu = require("./tab-menu").TabMenu;

const { TabMenu } = require("./tab-menu");

::: devtools/client/aboutdebugging/components/addons-tab.js:13
(Diff revision 2)
> -  "resource://gre/modules/AddonManager.jsm");
> +const AddonsControls = require("./addons-controls").AddonsControls;

const { AddonsControls } = require("./addons-controls");

::: devtools/client/aboutdebugging/components/tab-menu.js:10
(Diff revision 2)
> -  "devtools/client/shared/vendor/react");
> +const TabMenuEntry = require("./tab-menu-entry").TabMenuEntry;

const { TabMenuEntry } = require("./tab-menu-entry");

::: devtools/client/aboutdebugging/components/target-list.js:12
(Diff revision 2)
> -  "devtools/client/aboutdebugging/components/target", true);
> +const Target = require("./target").Target;

const { Target } = require("./target");

::: devtools/client/aboutdebugging/components/workers-tab.js:13
(Diff revision 2)
> -loader.lazyRequireGetter(this, "TabHeader",
> +const TabHeader = require("./tab-header").TabHeader;

const { TargetList } = require("./target-list");
const { TabHeader } = require("./tab-header");

Comment 20

[Julian Descottes [:jdescottes]]

Comment on attachment 8723234 [details]
MozReview Request: Bug 1250002 - part2: about:debugging switch to browserLoader;r=ochameau

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36415/diff/2-3/

Comment 21

[Julian Descottes [:jdescottes]]

Try is green, pushing to fx-team.

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/5c0b8b63d2a8
https://hg.mozilla.org/integration/fx-team/rev/3ac7627f1cf7

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5c0b8b63d2a8
https://hg.mozilla.org/mozilla-central/rev/3ac7627f1cf7