1321831 - Delay device loading when opening RDM

Status: RESOLVED FIXED

(DevTools :: Responsive Design Mode, defect, P1)

Description

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

Loading the device list already happens async, but it appears to still cause delays in open RDM anyway.

Let's explicitly move it after startup completes.

Comment 1

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

All times are measured after toggling RDM a few times first to warm up caches, etc.

Before change: RDM open takes 530ms - 660ms
After change:  RDM open takes 230ms - 380ms

Comment 2

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

Attachment: Bug 1321831 - Delay loading RDM devices to allow faster open.

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

Comment 3

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5915bddf6536fe4260fce6ab1bdfd3aa5702a2a3

Comment 4

[Alexandre Poirot [:ochameau]]

Comment on attachment 8822558 [details]
Bug 1321831 - Delay loading RDM devices to allow faster open.

https://reviewboard.mozilla.org/r/101418/#review102192

Looks good, I just have some comments related to tests.

::: devtools/client/responsive.html/index.js:34
(Diff revision 1)
>  const { changeDisplayPixelRatio } = require("./actions/display-pixel-ratio");
>  const { addViewport, resizeViewport } = require("./actions/viewports");
>  const { loadDevices } = require("./actions/devices");
>  
> +// Exposed for use by tests
> +window.ReactTestUtils = addons.TestUtils;

nit: I'm wondering if tests should do:
toolWindow.require("...react").addons.TestUtils?

::: devtools/client/responsive.html/test/browser/browser_mouse_resize.js:12
(Diff revision 1)
>  
>  addRDMTask(TEST_URL, function* ({ ui, manager }) {
> -  ok(ui, "An instance of the RDM should be attached to the tab.");
> +  let store = ui.toolWindow.store;
> +
> +  // Wait until the viewport has been added
> +  yield waitUntilState(store, state => state.viewports.length == 1);

I'm still new to this codebase, but shouldn't this wait be done within addRDMTask? (And may be also the wait to device list load)

Comment 5

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

Comment on attachment 8822558 [details]
Bug 1321831 - Delay loading RDM devices to allow faster open.

https://reviewboard.mozilla.org/r/101418/#review102192

> nit: I'm wondering if tests should do:
> toolWindow.require("...react").addons.TestUtils?

Yes, that works too, though I do need to expose `require` on the window for it to be accessible.  Wasn't sure which was better, but I think I'll change to what you're suggesting since it's a bit more flexible.

> I'm still new to this codebase, but shouldn't this wait be done within addRDMTask? (And may be also the wait to device list load)

Yes, possibly...  Originally I wasn't sure what each test would need to wait for, but it is common to at least wait for the viewport.

I think I'll keep waiting a bit on this for now, but it's probably a good idea.

Comment 6

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

Comment on attachment 8822558 [details]
Bug 1321831 - Delay loading RDM devices to allow faster open.

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/876ba9916117
Delay loading RDM devices to allow faster open. r=ochameau

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/876ba9916117

Comment 9

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

Comment on attachment 8822558 [details]
Bug 1321831 - Delay loading RDM devices to allow faster open.

Approval Request Comment
[Feature/Bug causing the regression]: Slow loading time in new Responsive Design Mode (first enabled in 52)
[User impact if declined]: Loading time will remain slow
[Is this code covered by automated tests?]: Yes, tests were updated for this change
[Has the fix been verified in Nightly?]: Tested locally
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Only affects UI of RDM
[String changes made/needed]: None

Comment 10

[Julien Cristau [:jcristau]]

Comment on attachment 8822558 [details]
Bug 1321831 - Delay loading RDM devices to allow faster open.

faster open of devtools RDM, aurora52+

Comment 11

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/2c312ad91c1b