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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: airpingu, Assigned: airpingu)

References

Details

(Whiteboard: [LOE:M], [qa-])

Attachments

(1 file, 2 obsolete files)

Assignee: nobody → clian
Blocks: 777226
Whiteboard: [LOE:M]
(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?
(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...
Attached patch Patch (obsolete) (deleted) — Splinter Review
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 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-
(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.
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. :)
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?
Attached patch Patch, V2 (obsolete) (deleted) — Splinter Review
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 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+
(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?
Yes, let's go with !== 'activity'. We'll revisit that later if new kind of messages need to not be cached.
Attached patch Patch, V2.1 (deleted) — Splinter Review
Thanks for the review!
Attachment #655928 - Attachment is obsolete: true
Attachment #656728 - Flags: review?(fabrice)
Attachment #656728 - Flags: review?(fabrice) → review+
Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Blocks: system-message-api
No longer blocks: 777226
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
Blocks: 777226
Whiteboard: [LOE:M] → [LOE:M], [qa-]
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. ;)
I stand by what I said in comment #7 - I'd rather not rely on gaia sending us something.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: