Closed Bug 1399090 Opened 7 years ago Closed 7 years ago

Responsive design toolbox button force loading many RDM dependencies

Categories

(DevTools :: Responsive Design Mode, enhancement)

enhancement
Not set
normal

Tracking

(firefox57 fixed)

RESOLVED FIXED
Firefox 57
Tracking Status
firefox57 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

https://perfht.ml/2w4zbUp
When inserting RDM icon in the toolbox toolbar, we end up loading responsivedesign.jsm which takes 3.9ms / 1.2% of overall JS computing in parent process.
With this patch we go down to 0.5ms / 0.2% for loading responsivedesign.jsm:
  https://perfht.ml/2w577A1
This patch moves most of
  devtools/client/responsivedesign/responsivedesign.jsm
to
  devtools/client/responsivedesign/responsivedesign-old.js

as it is all related to the old RDM.
Then, I added lazy loading in devtools/client/responsive.html/manager.js
so that we end up only loading a very small responsivedesign.jsm and only manager.js (and none of its deps).
Assignee: nobody → poirot.alex
Comment on attachment 8907043 [details]
Bug 1399090 - Lazy load responsive design manager modules.

https://reviewboard.mozilla.org/r/178768/#review183992

Thanks, looks like a good cleanup and perf improvement! :)

It seems like we could go further though, maybe in a follow up bug / patch.  Is there any reason for the new vs. old switch to be a JSM?  Can we convert it to a regular module instead?

I also noticed some references to RDM in `browser.js` that seem like dead code now that DevTools manages menus and shortcuts:

http://searchfox.org/mozilla-central/rev/51eae084534f522a502e4b808484cde8591cb502/browser/base/content/browser.js#8467-8477

(Perhaps the Scratchpad part is dead as well.)

::: devtools/client/responsivedesign/responsivedesign-old.js:1
(Diff revision 2)
> -"use strict";
> -
> -const Cu = Components.utils;
> -
> -const { loader, require } = Cu.import("resource://devtools/shared/Loader.jsm", {});
>  const { LocalizationHelper } = require("devtools/shared/l10n");

New file needs license header.

::: devtools/client/responsivedesign/responsivedesign-old.js:130
(Diff revision 2)
>      }
>    })
>  };
>  
>  EventEmitter.decorate(Manager);
> +exports.Manager = Manager;

Rename the `Manager` object to `ResponsiveUIManager` in the new file.  This way both new and old use the same name.  (I only changed to `Manager` when adding the new / old switch because of some ESLint rule.)

::: devtools/client/responsivedesign/responsivedesign-old.js:259
(Diff revision 2)
>  
>      this.showNewUINotification();
>  
>      // Notify that responsive mode is on.
>      this._telemetry.toolOpened("responsive");
> -    ResponsiveUIManager.emit("on", { tab: this.tab });
> +    Manager.emit("on", { tab: this.tab });

These renames can all be reverted with the name change above.
Attachment #8907043 - Flags: review?(jryans) → review+
Blocks: 1399449
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #6)
> Comment on attachment 8907043 [details]
> Bug 1399090 - Lazy load responsive design manager modules.
> 
> https://reviewboard.mozilla.org/r/178768/#review183992
> 
> Thanks, looks like a good cleanup and perf improvement! :)
> 
> It seems like we could go further though, maybe in a follow up bug / patch. 
> Is there any reason for the new vs. old switch to be a JSM?  Can we convert
> it to a regular module instead?

Opened bug 1399449.

> I also noticed some references to RDM in `browser.js` that seem like dead
> code now that DevTools manages menus and shortcuts:
> 
> http://searchfox.org/mozilla-central/rev/
> 51eae084534f522a502e4b808484cde8591cb502/browser/base/content/browser.
> js#8467-8477
> 
> (Perhaps the Scratchpad part is dead as well.)

That's bug 1250832, I think I will proceed with all these cleanups early during 58 cycle.
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/665aadca9d5f
Lazy load responsive design manager modules. r=jryans
https://hg.mozilla.org/mozilla-central/rev/665aadca9d5f
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.