1146519 - Persist simulator configurations in WebIDE

Status: RESOLVED FIXED

(DevTools Graveyard :: WebIDE, defect)

Description

[Jan Keromnes [:janx]]

With bug 1090949, Simulators will be configurable in WebIDE.

However, any configuration will be temporary and values will reset to defaults after a restart. To improve this, configurations need to be persisted.

Comment 1

[Jan Keromnes [:janx]]

Attachment: Persist simulator configurations in WebIDE.

WIP (misses a test).

Comment 2

[Jan Keromnes [:janx]]

Attachment: Persist simulator configurations in WebIDE.

Comment 3

[Jan Keromnes [:janx]]

Comment on attachment 8627762 [details] [diff] [review]
Persist simulator configurations in WebIDE.

Ryan, please have a look. I added a test to ensure saving/loading works.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=713bc18105ab

Comment 4

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

Comment on attachment 8627762 [details] [diff] [review]
Persist simulator configurations in WebIDE.

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

Task.async w/ function* would make a lot of these functions easier to read.  Then you can easily yield on promises.

Seems to work well, just some style things I'd like changed.

::: browser/devtools/webide/modules/simulators.js
@@ +27,5 @@
> +    if (this._loadingPromise) {
> +      return this._loadingPromise;
> +    }
> +
> +    this._loadingPromise = asyncStorage.getItem("simulators").then(value => {

With Task.async, this would look like:

this._loadingPromise = asyncStorage.getItem("simulators");
yield this._loadingPromise;
<contents of then block without explicit then>

@@ +62,5 @@
> +  },
> +
> +  // Add default simulators to the list for each new (unused) addon.
> +  _addUnusedAddons() {
> +    return Simulators.findSimulatorAddons().then(addons => {

Use Task.async / yield here.

@@ +73,5 @@
> +  },
> +
> +  // Save the current list of configurations.
> +  _save() {
> +    return this._load().then(() => {

Use Task.async / yield here.

@@ +88,3 @@
>    // List all available simulators.
>    findSimulators() {
> +    return this._load().then(() => {

Use Task.async / yield here.

@@ +130,5 @@
> +    }
> +  },
> +
> +  // Add a new simulator to the list. Caution: `simulator.name` may be modified.
> +  // @return Promise to added simulator.

DevTools style uses /** */ comment blocks when describing the purpose, arguments, and return values of a function.

::: browser/devtools/webide/test/test_simulators.html
@@ +331,5 @@
>            yield nextTick();
>  
>            is(findAll(".runtime-panel-item-simulator").length, 0, "Last simulator was removed");
>  
> +          yield Simulators._save();

Should the simulator storage be cleared at the end of test to avoid (since we try to reset to known state between tests)?

Comment 5

[Jan Keromnes [:janx]]

Attachment: Persist simulator configurations in WebIDE.

All nits addressed, round #2.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=11b3f2b80225

Comment 6

[Jan Keromnes [:janx]]

Sheriffs, please land this patch along with bug 1156834 (that other patch is included in the try push).

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/2e72154c2c40

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/2e72154c2c40