1248619 - Save / restore RDM workspace when toggling

Status: VERIFIED FIXED

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

Description

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

The existing RDM tool saves the current viewport size when you exit RDM.

We should preserve the current RDM workspace (set of viewports, etc.) and restore them when the tool opens again.

At the very least, the size should be restored to maintain parity with the existing tool.

Comment 1

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

The way I imagine this working is roughly:

* Every so often (after X seconds or something) we would persist the state of Redux store in RDM using the asyncStorage module
* The next time you enter RDM, we retrieve that state from storage and pass it as the `initialState` to `createStore`[1]

This is kind of a quick and dirty approach because it does not re-run any side effects that might be triggered when perform each UI actions one after another.  Instead it just sets the state immediately.  A more complex variant would instead record a _log_ of all dispatched actions so that the transition of each action can be replayed correctly, but something like that is likely to be quite slow and is really meant more for debugging.

We may need to filter out certain parts of state if there are things that don't restore correctly.  I think we should at least be able to get the basics like viewport size working this way.

There are all kinds of ways to build on top of this later on: we could save different workspaces per site that you work on, for example.

For now, we just want the base of restoring settings (as many as we can do easily) when you re-enter.

[1]: http://redux.js.org/docs/api/createStore.html

Comment 2

[Jaideep Bhoosreddy [:jbhoosreddy]]

Attachment: 1248619.patch [WIP]

Hello Ryan. As per your previous comment on this bug, I passed the viewports as initial state to the store. And I verified that the object is correctly applied to the store. Ideally, the RDM should load with the viewports applied, but it is not loading, and I'm trying to debug why that is happening. Could you give me some pointers on how to proceed?

Comment 3

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

Comment on attachment 8767323 [details] [diff] [review]
1248619.patch [WIP]

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

I've added a few comments that should get you further along, but there are more issues to look into at startup, such as device loading tries to re-run, etc.

::: devtools/client/responsive.html/index.js
@@ +153,5 @@
> +  store.subscribe(throttle(() => {
> +    const state = store.getState();
> +    const viewports = state.viewports;
> +    if (viewports && viewports.length > 0) {
> +      asyncStorage.setItem("rdm.viewports", viewports).then(() => {

As mentioned in store, you need to store the complete state.

::: devtools/client/responsive.html/store.js
@@ +21,5 @@
>      history = [];
>      shouldLog = true;
>    }
> +  const viewports = yield asyncStorage.getItem("rdm.viewports")
> +    .then(viewports => viewports);

This `then` line doesn't seem to do anything.

@@ +27,4 @@
>    let store = createStore({
>      log: shouldLog,
> +    history,
> +  })(combineReducers(reducers), {viewports});

The initial state of the store has either be "complete" (it must can all the properties full store would have) or nothing at all.  So, you can't store and set just the viewports.

It's possible we'll need to do some more tricks to initialize data outside of viewports the something reasonable at startup.  But to get started with this approach for the moment, set the full state.

Also, createStore expects the initialState to be undefined on the first run, but getItem will give back null.  You'll need to force to undefined for an falsy value.

Comment 4

[Jaideep Bhoosreddy [:jbhoosreddy]]

Attachment: 1248619.patch

Apparently using intialState with combineReducers causes some issues due to scoping of the initialState for each reducer. Corollary is that the initialState is applied to the store when createStore but it is not properly scoped for each reducer. If my understanding is correct.
http://stackoverflow.com/questions/33749759/read-stores-initial-state-in-redux-reducer
Also, it seems that partial state can be provided in initialState. It is applied to the store and should be applied to the appropriate reducer.

In any case, I was unable to apply the initialState of the store to the viewports reducer. So I tried another approach without initialState in createStore, which passes the cachedViewport when dispatching addViewport action. It works well without the resizing glitch we saw before.

Comment 5

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

Comment on attachment 8771790 [details] [diff] [review]
1248619.patch

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

Hmm.  We can fall back to something like this if we must, but I would like to try restoring more of the RDM state than just the viewport size, so that we have a general save / restore ability, rather than special casing specific bits of data.

I don't think I follow your analysis of the StackOverflow answer you linked to.  It seems to say that you can provide data for either some or all of the reducers when using `combineReducers`, so it's actually more flexible than I imagined (I assumed you'd need specify all of them).

Did you attempt to save the entire state and restore the entire state like I mentioned?  It's not clear to me what "I was unable to apply the initialState of the store to the viewports reducer" means.

Comment 6

[Jaideep Bhoosreddy [:jbhoosreddy]]

I did follow your instructions and attempt to save and restore the entire state (not just viewports) while creating the store. But I could not resolve the issue that the RDM viewport would not open, as it was supposed to, after the store was created with initialState.
I will look into it again, but if you understand why that is happening, do let me know.

Comment 8

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 1: Refactor and restore the reload condition settings in RDM. [1.0]

Comment 10

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 2: Refactor and remove the deviceType prop from viewports reducer [1.0]

Comment 11

[Razvan Caliman [:rcaliman]]

Comment on attachment 9009380 [details] [diff] [review]
Part 1: Refactor and restore the reload condition settings in RDM. [1.0]

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

As discussed over messaging, there's potential to replace the prop drilling for the handlers which only dispatch an action to the store (most `onToggle...` props) with a more concise, direct reference to the actions themselves, either via `mapDispatchToProps` or an alternative solution. Some ideas here: https://gist.github.com/heygrady/c6c17fc7cbdd978f93a746056f618552

But since we haven't settled on an approach yet, this code may go in as-is and we'll refactor later. Just leaving this here as a note for future reference.

::: devtools/client/responsive.html/reducers/ui.js
@@ +68,5 @@
> +      enabled : !ui.reloadOnTouchSimulation;
> +
> +    Services.prefs.setBoolPref(RELOAD_ON_TOUCH_SIMULATION, reloadOnTouchSimulation);
> +
> +    return Object.assign({}, ui, {

This sort of return statement can become a one-liner:

```
return { ...ui, reloadOnTouchSimulation };
```

Just a style preference. You're free to keep as is.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e089ee7f512
Part 1: Refactor and restore the reload condition settings in RDM. r=rcaliman

Comment 14

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/5e089ee7f512

Comment 15

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 2: Restore the previous device state in RDM. [1.0]

Comment 16

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 2: Restore the previous device state in RDM. [2.0]

Comment 17

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 2: Restore the previous device state in RDM. [3.0]

Comment 18

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 3: Restore the previous viewport size, user agent, display pixel ratio and touch simultation properties. [1.0]

Comment 19

[Razvan Caliman [:rcaliman]]

Comment on attachment 9015171 [details] [diff] [review]
Part 2: Restore the previous device state in RDM. [3.0]

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

::: devtools/client/responsive.html/actions/viewports.js
@@ +35,5 @@
>     * Change the viewport device.
>     */
>    changeDevice(id, device, deviceType) {
> +    return async function(dispatch) {
> +      await asyncStorage.setItem("devtools.responsive.deviceState",

Nit: I wonder whether we should guard this call to asyncStorage. It should work most of the time. But if, for any reason, the IndexedDB backing it throws an error, this will prevent changing devices in RDM. Given that asyncStorage is only used to enhance the experience by saving device state to restore, perhaps it should be allowed to fail gracefully if needed.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae15dbcedd8a
Part 2: Restore the previous device state in RDM. r=rcaliman
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb2a9e0b7538
Part 3: Restore the previous viewport size, user agent, display pixel ratio and touch simultation properties. r=rcaliman

Comment 21

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb0c2bcc9bc6
Backed out 2 changesets for devtools/client/responsive.html/test/unit/test_change_device.js failures

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/63551d28d39a
Part 2: Restore the previous device state in RDM. r=rcaliman
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d1391ec87f2
Part 3: Restore the previous viewport size, user agent, display pixel ratio and touch simultation properties. r=rcaliman

Comment 23

[Bogdan Tara[:bogdan_tara | bogdant]]

I backed out this earlier for devtools/client/responsive.html/test/unit/test_change_device.js failures.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=fb2a9e0b7538881fbe1c526d961338108ca4c967

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/fb0c2bcc9bc616cc20d193969e0f4c14c7166600

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=204301908&repo=mozilla-inbound&lineNumber=1932

[task 2018-10-09T19:28:09.739Z] 19:28:09     INFO -  TEST-START | devtools/client/responsive.html/test/unit/test_change_device.js
[task 2018-10-09T19:28:10.200Z] 19:28:10  WARNING -  TEST-UNEXPECTED-FAIL | devtools/client/responsive.html/test/unit/test_change_device.js | xpcshell return code: 0
[task 2018-10-09T19:28:10.201Z] 19:28:10     INFO -  TEST-INFO took 457ms
[task 2018-10-09T19:28:10.202Z] 19:28:10     INFO -  >>>>>>>
[task 2018-10-09T19:28:10.204Z] 19:28:10     INFO -  PID 10489 | [10489, Main Thread] WARNING: Couldn't get the user appdata directory. Crash events may not be produced.: file /builds/worker/workspace/build/src/toolkit/crashreporter/nsExceptionHandler.cpp, line 2733
[task 2018-10-09T19:28:10.205Z] 19:28:10     INFO -  PID 10489 | JavaScript strict warning: resource://devtools/shared/Loader.jsm, line 222: ReferenceError: reference to undefined property "name"
[task 2018-10-09T19:28:10.207Z] 19:28:10     INFO -  (xpcshell/head.js) | test MAIN run_test pending (1)
[task 2018-10-09T19:28:10.208Z] 19:28:10     INFO -  (xpcshell/head.js) | test run_next_test 0 pending (2)
[task 2018-10-09T19:28:10.210Z] 19:28:10     INFO -  (xpcshell/head.js) | test MAIN run_test finished (2)
[task 2018-10-09T19:28:10.211Z] 19:28:10     INFO -  running event loop
[task 2018-10-09T19:28:10.212Z] 19:28:10     INFO -  "CONSOLE_MESSAGE: (warn) [JavaScript Warning: "ReferenceError: reference to undefined property "name"" {file: "resource://devtools/shared/Loader.jsm" line: 222}]"
[task 2018-10-09T19:28:10.214Z] 19:28:10     INFO -  devtools/client/responsive.html/test/unit/test_change_device.js | Starting
[task 2018-10-09T19:28:10.215Z] 19:28:10     INFO -  (xpcshell/head.js) | test pending (2)
[task 2018-10-09T19:28:10.217Z] 19:28:10     INFO -  PID 10489 | console.log: "[DISPATCH] action type:" "ADD_DEVICE_TYPE"
[task 2018-10-09T19:28:10.218Z] 19:28:10     INFO -  PID 10489 | console.log: "[DISPATCH] action type:" "ADD_DEVICE"
[task 2018-10-09T19:28:10.220Z] 19:28:10     INFO -  PID 10489 | console.log: "[DISPATCH] action type:" "ADD_VIEWPORT"
[task 2018-10-09T19:28:10.220Z] 19:28:10     INFO -  TEST-PASS | devtools/client/responsive.html/test/unit/test_change_device.js |  - Default device is unselected - "" == ""
[task 2018-10-09T19:28:10.222Z] 19:28:10  WARNING -  TEST-UNEXPECTED-FAIL | devtools/client/responsive.html/test/unit/test_change_device.js |  - Changed to Firefox OS Flame device - "" == "Firefox OS Flame"
[task 2018-10-09T19:28:10.226Z] 19:28:10     INFO -  /builds/worker/workspace/build/tests/xpcshell/tests/devtools/client/responsive.html/test/unit/test_change_device.js:null:40
[task 2018-10-09T19:28:10.227Z] 19:28:10     INFO -  exiting test
[task 2018-10-09T19:28:10.229Z] 19:28:10     INFO -  (xpcshell/head.js) | test run_next_test 0 finished (2)
[task 2018-10-09T19:28:10.230Z] 19:28:10     INFO -  Unexpected exception NS_ERROR_ABORT:
[task 2018-10-09T19:28:10.231Z] 19:28:10     INFO -  _abort_failed_test@/builds/worker/workspace/build/tests/xpcshell/head.js:746:9
[task 2018-10-09T19:28:10.232Z] 19:28:10     INFO -  do_report_result@/builds/worker/workspace/build/tests/xpcshell/head.js:853:5
[task 2018-10-09T19:28:10.232Z] 19:28:10     INFO -  Assert<@/builds/worker/workspace/build/tests/xpcshell/head.js:55:5
[task 2018-10-09T19:28:10.232Z] 19:28:10     INFO -  proto.report@resource://testing-common/Assert.jsm:214:5
[task 2018-10-09T19:28:10.233Z] 19:28:10     INFO -  equal@resource://testing-common/Assert.jsm:246:3
[task 2018-10-09T19:28:10.233Z] 19:28:10     INFO -  @/builds/worker/workspace/build/tests/xpcshell/tests/devtools/client/responsive.html/test/unit/test_change_device.js:40:3
[task 2018-10-09T19:28:10.234Z] 19:28:10     INFO -  async*run_next_test/_run_next_test/<@/builds/worker/workspace/build/tests/xpcshell/head.js:1441:22
[task 2018-10-09T19:28:10.234Z] 19:28:10     INFO -  async*_run_next_test@/builds/worker/workspace/build/tests/xpcshell/head.js:1441:10
[task 2018-10-09T19:28:10.234Z] 19:28:10     INFO -  run@/builds/worker/workspace/build/tests/xpcshell/head.js:692:9
[task 2018-10-09T19:28:10.234Z] 19:28:10     INFO -  _do_main@/builds/worker/workspace/build/tests/xpcshell/head.js:219:3
[task 2018-10-09T19:28:10.235Z] 19:28:10     INFO -  _execute_test@/builds/worker/workspace/build/tests/xpcshell/head.js:533:5
[task 2018-10-09T19:28:10.235Z] 19:28:10     INFO -  @-e:1:1
[task 2018-10-09T19:28:10.235Z] 19:28:10     INFO -  exiting test

Comment 24

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/63551d28d39a
https://hg.mozilla.org/mozilla-central/rev/2d1391ec87f2

Comment 26

[Oana Botisan, Desktop QA]

I managed to reproduce the issue using an older version of Nightly (2018-09-17) on Windows 10 x64 by using these steps: 
1. In one tab I opened the developer tools and enable RDM. 
2. I changed the width and height of the RDM view. 
3. Open another tab and opened RDM again.

I retested everything using latest Nightly on Windows 10 x64, Ubuntu 16.04 x 64 and macOS 10.13. The bug is not reproducing anymore. Even after a restart the RDM keeps the changed dimensions.

But I did notice something else. 
If in one of the tabs you change the dimensions again and then switch to the second tab, the second tab doesn't automatically change dimensions after refresh. You have to disable the RDM and enable it once again. Is that expected behaviour?

Comment 27

[Leho Kraav (:macmaN @lkraav)]

FF64b4 loses "rotate viewport" info on RDM re-entry

CURRENT

* enter RDM
* select some RDM preset, like "Laptop HiDPI" 900x1440 (weird portrait resolution, https://github.com/mozilla/simulated-devices/issues/29)
* rotate viewport, 1440x900
* exit RDM
* re-enter RDM
* RDM has lost "rotate viewport" result, we're back at 900x1440

EXPECTED

* RDM re-enters at 1440x900

NOTES

Still better than before, though (y)

Comment 29

[Martin Balfanz [:mbalfanz]]

Leho, I filed bug 1503902 for this. Thank you for bringing this up!

Comment 30

[Irene Smith]

I can see the proper behavior (at least the remembering which device was chosen) in Nightly, currently 65.0a1, but not in 64 as it says above. I have Developer Edition 64.0b6 and it does not remember which device was chosen. Does that mean this feature has moved to 65?

Comment 31

[Martin Balfanz [:mbalfanz]]

Irene, saving preferences incl. selected device is part of 64. Also, it works for me as expected in Firefox Developer Edition 64.0b6.

It would be great if you can test again and, potentially, post STR if it doesn't work for you.

Comment 32

[Irene Smith]

> Irene, saving preferences incl. selected device is part of 64. Also, it
> works for me as expected in Firefox Developer Edition 64.0b6.
> 
> It would be great if you can test again and, potentially, post STR if it
> doesn't work for you.

I don't know why this was not working yesterday. I have not even reset my system since then. But I can no longer reproduce the problem. Can I confirm which settings should be saved between sessions?

Is it only device right now or are things like the throttling supposed to be saved as well? From what I am seeing, only the device choice is saved right now, not the connection throttling or the orientation. Is there something else that is not visible that I should be mentioning?

And thanks for your help on this.(In reply to Martin Balfanz [:mbalfanz] from comment #31)

Comment 33

[Irene Smith]

Added this note to the Responsive Design page:

Note: The device you select when in Responsive Design Mode will be saved between sessions.

I know other work is planned, so this will most likely get updated in the near future.

This is the release notes paragraph:

Responsive Design Mode device selection is now saved between sessions (bug 1248619).

Comment 34

[Gabriel Luong [:gl] (ΦωΦ)]

(In reply to Oana Botisan from comment #26)
> I managed to reproduce the issue using an older version of Nightly
> (2018-09-17) on Windows 10 x64 by using these steps: 
> 1. In one tab I opened the developer tools and enable RDM. 
> 2. I changed the width and height of the RDM view. 
> 3. Open another tab and opened RDM again.
> 
> I retested everything using latest Nightly on Windows 10 x64, Ubuntu 16.04 x
> 64 and macOS 10.13. The bug is not reproducing anymore. Even after a restart
> the RDM keeps the changed dimensions.
> 
> But I did notice something else. 
> If in one of the tabs you change the dimensions again and then switch to the
> second tab, the second tab doesn't automatically change dimensions after
> refresh. You have to disable the RDM and enable it once again. Is that
> expected behaviour?

Yes. The current behavior currently does not account for different tabs.

Comment 36

[Rares Doghi, Desktop QA]

I can Confirm this issue as Fixed in Firefox 64.0, Nightly 66.0a1 (2018-12-19) and Beta 65.0b5.