Closed Bug 1146519 Opened 5 years ago Closed 5 years ago

Persist simulator configurations in WebIDE

Categories

(DevTools :: WebIDE, defect)

defect
Not set

Tracking

(firefox42 fixed)

RESOLVED FIXED
Firefox 42
Tracking Status
firefox42 --- fixed

People

(Reporter: janx, Assigned: janx)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Depends on: 1146521
Blocks: 1156834
WIP (misses a test).
Assignee: nobody → janx
Status: NEW → ASSIGNED
Attachment #8622539 - Attachment is obsolete: true
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
Attachment #8627762 - Flags: review?(jryans)
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)?
Attachment #8627762 - Flags: review?(jryans) → feedback+
Attachment #8628832 - Flags: review?(jryans)
Sheriffs, please land this patch along with bug 1156834 (that other patch is included in the try push).
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2e72154c2c40
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.