Closed Bug 786361 Opened 8 years ago Closed 8 years ago

Prevent a startup race waiting for the DOM application registry to load

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-basecamp:+)

RESOLVED FIXED
blocking-basecamp +

People

(Reporter: fabrice, Assigned: fabrice)

Details

(Whiteboard: [WebAPI:P1], [qa-])

Attachments

(1 file, 4 obsolete files)

Attached patch patch (obsolete) — Splinter Review
Tested by Etienne on an opt build.
Attachment #656095 - Flags: review?(21)
OS: Linux → Gonk
Hardware: x86_64 → ARM
Attachment #656095 - Flags: review?(21)
Assignee: nobody → fabrice
blocking-basecamp: --- → +
Attached patch patch v2 (obsolete) — Splinter Review
Marshall, can you test it on your device config?
Attachment #656095 - Attachment is obsolete: true
Attached patch patch v2 (obsolete) — Splinter Review
I forgot to qref a small change.
Attachment #659401 - Attachment is obsolete: true
Whiteboard: [WebAPI:P1]
Attached patch patch v3 (obsolete) — Splinter Review
Updated patch to deal with non-existent directories properly.
Attachment #659402 - Attachment is obsolete: true
Attachment #659880 - Flags: review?(marshall)
Comment on attachment 659880 [details] [diff] [review]
patch v3

Canceling review for now. This doesn't work well on device, since we end up with other races instead.
Attachment #659880 - Flags: review?(marshall)
Attached patch patch v4Splinter Review
This patch takes a slightly different approach: we still load the system app as soon as possible to get the lock screen up without delay, but fire a "webapps-registry-ready" event to the system app.

The PR https://github.com/mozilla-b2g/gaia/pull/4575 uses that event to launch the homescreen.
Attachment #659880 - Attachment is obsolete: true
Attachment #660166 - Flags: review?(marshall)
Comment on attachment 660166 [details] [diff] [review]
patch v4

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

r+ with some minor nits addressed :)

::: b2g/chrome/content/shell.js
@@ +422,5 @@
>  }, "fullscreen-origin-change", false);
>  
> +Services.obs.addObserver(function onWebappsReady(subject, topic, data) {
> +  shell.sendChromeEvent({ type: "webapps-registry-ready" });
> +}, 'webapps-registry-ready', false);

minor nit: different quotes used here and one line above. This file isn't particularly consistent, but one block of code should be at least :)

::: dom/apps/src/Webapps.jsm
@@ +192,5 @@
> +        let app = this.webapps[aResult.id];
> +        let manifest = aResult.manifest;
> +        this._registerSystemMessages(manifest, app);
> +        this._registerActivities(manifest, app);
> +      }).bind(this));

nit: could we save a generated function here by using:

aResults.forEach(function() { .. }, this) ?
Attachment #660166 - Flags: review?(marshall) → review+
Comment on attachment 660166 [details] [diff] [review]
patch v4

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

::: dom/apps/src/Webapps.jsm
@@ -178,5 @@
> -  _processManifestForId: function(aId) {
> -    let app = this.webapps[aId];
> -    this._readManifests([{ id: aId }], (function registerManifest(aResult) {
> -      let manifest = aResult[0].manifest;
> -      app.name = manifest.name;

This line appears to have been dropped, which will break the fix for bug 784805
With nits fixed and Dave's app.name back in:

https://hg.mozilla.org/integration/mozilla-inbound/rev/f04c43188e47
Whiteboard: [WebAPI:P1] → [WebAPI:P1], [qa-]
https://hg.mozilla.org/mozilla-central/rev/f04c43188e47
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.