Closed Bug 1250002 Opened 4 years ago Closed 4 years ago

about:debugging load React components using BrowserLoader

Categories

(DevTools :: about:debugging, defect)

47 Branch
defect
Not set

Tracking

(firefox47 fixed)

RESOLVED FIXED
Firefox 47
Tracking Status
firefox47 --- fixed

People

(Reporter: jdescottes, Assigned: jdescottes)

References

Details

Attachments

(2 files, 2 obsolete files)

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.
Depends on: 1249585, 1245029
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Attached patch bug1250002.wip.patch (obsolete) — Splinter Review
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 ?
Attachment #8721812 - Flags: feedback?(poirot.alex)
Blocks: 1207997
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.
Attachment #8721812 - Flags: feedback?(poirot.alex) → feedback+
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.
(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.
(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 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/
Attachment #8721992 - Flags: review?(jryans)
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.
(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.
Attached patch bug1250002.part1.wip.patch (obsolete) — Splinter Review
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.
Attachment #8722713 - Flags: feedback?(jryans)
Comment on attachment 8722713 [details] [diff] [review]
bug1250002.part1.wip.patch

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

Looks correct to me, thanks.
Attachment #8722713 - Flags: feedback?(jryans) → review+
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/
Attachment #8721992 - Flags: review?(jryans)
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+ !
Attachment #8721992 - Flags: review?(jryans) → review+
Attachment #8722713 - Attachment is obsolete: true
Attachment #8721812 - Attachment is obsolete: true
https://reviewboard.mozilla.org/r/35819/#review33005

Already reviewed as bugzilla attachment
Attachment #8721992 - Flags: review?(jryans)
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 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/
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
Attachment #8723234 - Flags: review?(poirot.alex) → review+
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 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/
Attachment #8721992 - Flags: review?(jryans)
Try is green, pushing to fx-team.
https://hg.mozilla.org/mozilla-central/rev/5c0b8b63d2a8
https://hg.mozilla.org/mozilla-central/rev/3ac7627f1cf7
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.