Closed Bug 1321831 Opened 5 years ago Closed 5 years ago

Delay device loading when opening RDM

Categories

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

49 Branch
defect

Tracking

(firefox51 unaffected, firefox52 fixed, firefox53 fixed)

RESOLVED FIXED
Firefox 53
Tracking Status
firefox51 --- unaffected
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: jryans, Assigned: jryans)

References

(Blocks 1 open bug)

Details

(Whiteboard: [rdm-v2])

Attachments

(1 file)

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.
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 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)
Attachment #8822558 - Flags: review?(poirot.alex) → review+
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.
Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/876ba9916117
Delay loading RDM devices to allow faster open. r=ochameau
https://hg.mozilla.org/mozilla-central/rev/876ba9916117
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
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
Attachment #8822558 - Flags: approval-mozilla-aurora?
Comment on attachment 8822558 [details]
Bug 1321831 - Delay loading RDM devices to allow faster open.

faster open of devtools RDM, aurora52+
Attachment #8822558 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.