Closed
Bug 783149
Opened 12 years ago
Closed 12 years ago
System Message API - Buffer messages in shell.js until the system app has started
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla18
People
(Reporter: airpingu, Assigned: airpingu)
References
Details
(Whiteboard: [LOE:M], [qa-])
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
fabrice
:
review+
|
Details | Diff | Splinter Review |
Please see https://bugzilla.mozilla.org/show_bug.cgi?id=777226#c1 for the motivation and https://bugzilla.mozilla.org/show_bug.cgi?id=777226#c4 for the possible solution.
Assignee | ||
Updated•12 years ago
|
Whiteboard: [LOE:M]
Assignee | ||
Comment 1•12 years ago
|
||
(In reply to Gene Lian [:gene] from comment #0)
> Please see https://bugzilla.mozilla.org/show_bug.cgi?id=777226#c1 for the
> motivation and https://bugzilla.mozilla.org/show_bug.cgi?id=777226#c4 for
> the possible solution.
Oops... shell.js cannot receive the window "load" event (why?!). Need to find another way to detect the system apps have been started or not.
Fabrice, do you have any good idea?
Assignee | ||
Comment 2•12 years ago
|
||
(In reply to Gene Lian [:gene] from comment #1)
> (In reply to Gene Lian [:gene] from comment #0)
> > Please see https://bugzilla.mozilla.org/show_bug.cgi?id=777226#c1 for the
> > motivation and https://bugzilla.mozilla.org/show_bug.cgi?id=777226#c4 for
> > the possible solution.
>
> Oops... shell.js cannot receive the window "load" event (why?!). Need to
> find another way to detect the system apps have been started or not.
Hmm... mozbrowserloadend might probably work. I'll give it a try tomorrow. Time to home...
Assignee | ||
Comment 3•12 years ago
|
||
Hi Fabrice,
I've fixed this one. ;) Could you please have a review on it when you're available? Thank you very much! The following summarizes what I've done in this patch:
1. Let shell listen to the |mozbrowserloadend| event so that we can have chance to detect if the homescreen app has been completely loaded.
2. However, we would get multiple |mozbrowserloadend| events coming from different apps (like sms, dialer... etc) during power-up. Therefore, we have to check if the event comes from the homescreen one. We can do so by checking |evt.target.dataset.frameOrigin| (inspired by https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/window_manager.js#L701).
3. During power-up, we will buffer the system messages (ex, "alarm" messages) until the homescreen app has been loaded. However, there is one exception: we shouldn't buffer the "activity" messages since one of it is just being fired for turning on the homescreen app (i.e. if we buffer that we will never get the |mozbrowserloadend| event for homescreen).
-----
Btw, I discovered another weird thing (not related to this issue). During power-up, the following "activity" message for opening homescreen app would be fired twice:
{"uri":"app://homescreen.gaiamobile.org/index.html","manifest":"app://homescreen.gaiamobile.org/manifest.webapp","type":"activity","target":{"filters":{"type":"application/x-application-list"},"href":"app://homescreen.gaiamobile.org/index.html"}}
Is that a potential bug?
Attachment #653034 -
Flags: review?(fabrice)
Comment 4•12 years ago
|
||
Comment on attachment 653034 [details] [diff] [review]
Patch
Review of attachment 653034 [details] [diff] [review]:
-----------------------------------------------------------------
::: b2g/chrome/content/shell.js
@@ +280,5 @@
> + if (!('frameType' in dataset) || dataset.frameType !== 'window')
> + return;
> +
> + if (!this.isHomescreenLoaded &&
> + dataset.frameOrigin === 'app://homescreen.gaiamobile.org') {
We can't harcode the homescreen url.
@@ +281,5 @@
> + return;
> +
> + if (!this.isHomescreenLoaded &&
> + dataset.frameOrigin === 'app://homescreen.gaiamobile.org') {
> + this.sendEvent(window, 'HomescreenLoaded');
why send another event here and not call the handler directly?
@@ +653,5 @@
> });
> });
>
> +window.addEventListener('HomescreenLoaded', function ss_onHomescreenLoaded() {
> + // Send and clean up the buffered system messages
nit: whitespace at the end of the line
Attachment #653034 -
Flags: review?(fabrice) → review-
Assignee | ||
Comment 5•12 years ago
|
||
(In reply to Fabrice Desré [:fabrice] from comment #4)
> Comment on attachment 653034 [details] [diff] [review]
> Patch
>
> Review of attachment 653034 [details] [diff] [review]:
> -----------------------------------------------------------------
Thank you for the reviews! :) Some quick questions as below:
>
> ::: b2g/chrome/content/shell.js
> @@ +280,5 @@
> > + if (!('frameType' in dataset) || dataset.frameType !== 'window')
> > + return;
> > +
> > + if (!this.isHomescreenLoaded &&
> > + dataset.frameOrigin === 'app://homescreen.gaiamobile.org') {
>
> We can't harcode the homescreen url.
May we just compare this in window_manager.js and send a customized event from there?
>
> @@ +281,5 @@
> > + return;
> > +
> > + if (!this.isHomescreenLoaded &&
> > + dataset.frameOrigin === 'app://homescreen.gaiamobile.org') {
> > + this.sendEvent(window, 'HomescreenLoaded');
>
> why send another event here and not call the handler directly?
Yeap... I'm wondering this too. However, I have to this, otherwise it won't work properly. This is inspired by the similar thing at http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#273. Anyway, this logic could be removed if you agree with the solution of sending customized event from window_manager.js.
Assignee | ||
Comment 6•12 years ago
|
||
Hi Fabrice!
Ping on this bug and the above question. Or may we just use a 10 seconds delay before sending "alarm"-specific system message from shell.js? Need your suggestions, please. :)
Comment 7•12 years ago
|
||
I'm not a big fan on relying on the homescreen to send the "I'm ready" event, and gecko only knows about launching the system app. Using a delay may be an acceptable tradeoff.
Vivien, do you have a magic idea for this one?
Assignee | ||
Comment 8•12 years ago
|
||
I vote for the 10-seconds-delayed approach. It's very hard from Gecko to get Gaia's dynamic loading status if we have to assume the platform should always be independent from applications in theory.
Upload a new patch for this if we cannot find a proper solution. ;)
Attachment #653034 -
Attachment is obsolete: true
Attachment #655928 -
Flags: review?(fabrice)
Comment 9•12 years ago
|
||
Comment on attachment 655928 [details] [diff] [review]
Patch, V2
Review of attachment 655928 [details] [diff] [review]:
-----------------------------------------------------------------
r=me with the nit addressed.
::: b2g/chrome/content/shell.js
@@ +393,5 @@
> // Listen for system messages and relay them to Gaia.
> Services.obs.addObserver(function onSystemMessage(subject, topic, data) {
> let msg = JSON.parse(data);
> + // Buffer alarm messages until the content starts to load for 10 seconds.
> + if (shell.needBufferSysMsgs && msg.type === 'alarm') {
I don't see any reason to make that specific to 'alarm' messages.
Attachment #655928 -
Flags: review?(fabrice) → review+
Assignee | ||
Comment 10•12 years ago
|
||
(In reply to Fabrice Desré [:fabrice] from comment #9)
> Comment on attachment 655928 [details] [diff] [review]
> Patch, V2
>
> Review of attachment 655928 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> r=me with the nit addressed.
>
> ::: b2g/chrome/content/shell.js
> @@ +393,5 @@
> > // Listen for system messages and relay them to Gaia.
> > Services.obs.addObserver(function onSystemMessage(subject, topic, data) {
> > let msg = JSON.parse(data);
> > + // Buffer alarm messages until the content starts to load for 10 seconds.
> > + if (shell.needBufferSysMsgs && msg.type === 'alarm') {
>
> I don't see any reason to make that specific to 'alarm' messages.
I did this because there are some 'activity' messages sent when powering-up as below:
{"uri":"app://homescreen.gaiamobile.org/index.html","manifest":"app://homescreen.gaiamobile.org/manifest.webapp","type":"activity","target":{"filters":{"type":"application/x-application-list"},"href":"app://homescreen.gaiamobile.org/index.html"}}
This seems to open the homescreen app. Right? I guess we cannot buffer that; otherwise, it would delay our homescreen app to show up, which could not be a good experience. Also, the alarm messages must be fired after them. How about changing the condition to be:
if (shell.needBufferSysMsgs && msg.type !== 'activity') {
Does that sound reasonable to you?
Comment 11•12 years ago
|
||
Yes, let's go with !== 'activity'. We'll revisit that later if new kind of messages need to not be cached.
Assignee | ||
Comment 12•12 years ago
|
||
Thanks for the review!
Attachment #655928 -
Attachment is obsolete: true
Attachment #656728 -
Flags: review?(fabrice)
Updated•12 years ago
|
Attachment #656728 -
Flags: review?(fabrice) → review+
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Comment 13•12 years ago
|
||
Flags: in-testsuite-
Keywords: checkin-needed
Comment 14•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Assignee | ||
Updated•12 years ago
|
Summary: System Message Handler API - Buffer messages in shell.js until the system app has started → System Message API - Buffer messages in shell.js until the system app has started
Updated•12 years ago
|
Whiteboard: [LOE:M] → [LOE:M], [qa-]
Assignee | ||
Comment 15•12 years ago
|
||
Recently, we have a new "system-message-listener-ready" customized event sent from system app, which sounds exactly what we want to solve this issue. Better to use that. ;)
Comment 16•12 years ago
|
||
I stand by what I said in comment #7 - I'd rather not rely on gaia sending us something.
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•