The apps' registration can happen at any time like updating apps after booting up. SystemMessageInternal needs to know when the activities starts to do that at run-time, so that it won't have chances to send the system messages to the wrong pages.

Hi Vivien and Fabrice, Not sure who is the expert of this part, so temporally assign this to you guys. Could either of you take a look on this patch? Thanks a lot! Summary: 1. We need to send an observer message "webapps-registry-start" to SystemMessageInternal, so that the SystemMessageInternal will buffer all the system messages until the apps' registration is redone (i.e. until receiving "webapps-registry-ready"). 2. I don't see any reason why we need a flag |this.allActivitiesSent|. Tentatively clean it up. Please correct me if I'm wrong. 3. Not every part needs |this._saveApps()| so I didn't put it in the |this.notifyAppsRegistryReady()|.

Comment on attachment 671382 [details] [diff] [review] Patch Review of attachment 671382 [details] [diff] [review]: ----------------------------------------------------------------- Leaving the review to fabrice since it is the expert here :)

(In reply to Gene Lian [:gene] from comment #1) > Created attachment 671382 [details] [diff] [review] > Patch > > Hi Vivien and Fabrice, > > Not sure who is the expert of this part, so temporally assign this to you > guys. Could either of you take a look on this patch? Thanks a lot! > > Summary: > > 1. We need to send an observer message "webapps-registry-start" to > SystemMessageInternal, so that the SystemMessageInternal will buffer all the > system messages until the apps' registration is redone (i.e. until receiving > "webapps-registry-ready"). Hm, ok. I understand why you need that, but we also have another listener for webapps-registry-ready (http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#501) and we need to make sure that calling it several times is ok. > 2. I don't see any reason why we need a flag |this.allActivitiesSent|. > Tentatively clean it up. Please correct me if I'm wrong. yep, you can't change that. The activities registration is async and we need that flag to prevent race conditions. > 3. Not every part needs |this._saveApps()| so I didn't put it in the > |this.notifyAppsRegistryReady()|. I need to take a closer look for this one, but at first sight I would rather keep it in notifyAppsRegistryReady(). This is not too costly.

(In reply to Fabrice Desré [:fabrice] from comment #3) > > 1. We need to send an observer message "webapps-registry-start" to > > SystemMessageInternal, so that the SystemMessageInternal will buffer all the > > system messages until the apps' registration is redone (i.e. until receiving > > "webapps-registry-ready"). > > Hm, ok. I understand why you need that, but we also have another listener > for webapps-registry-ready > (http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell. > js#501) and we need to make sure that calling it several times is ok. Thanks for the reminder! So far, Gaia only listens to the "webapps-registry-ready" event *one time* for initialization and removes the listener immediately [1], so I think there should be no harm if we fire multiple times. I think I can fire another "webapps-registry-start" event if Gaia really needs it in the future to know the time period for apps' registration. [1] https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/applications.js#L32 > > > 2. I don't see any reason why we need a flag |this.allActivitiesSent|. > > Tentatively clean it up. Please correct me if I'm wrong. > > yep, you can't change that. The activities registration is async and we need > that flag to prevent race conditions. All right. I understand it better :) I'll also add this flag in other places wherever we call |this._registerActivities()|. > > > 3. Not every part needs |this._saveApps()| so I didn't put it in the > > |this.notifyAppsRegistryReady()|. > > I need to take a closer look for this one, but at first sight I would rather > keep it in notifyAppsRegistryReady(). This is not too costly. OK. I'll put |this._saveApps()| in |this.notifyAppsRegistryReady()|.

Please see comment #4. Thanks Fabrice for the review! :)

Comment on attachment 671713 [details] [diff] [review] Patch V2 Review of attachment 671713 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comment addressed, and I'd also like to see a try run of this patch. ::: dom/apps/src/Webapps.jsm @@ +1000,4 @@ > #ifdef MOZ_SYS_MSG > + this.activitiesToRegister = 0; > + this.activitiesRegistered = 0; > + this.allActivitiesSent = false; So, we now have three places where we do these initializations. That's very error prone, please put them in a common helper function.

Addressing comment 6. Thanks Fabrice for the review.

Try server: https://tbpl.mozilla.org/?tree=Try&rev=99276143e5e8 (all green! magic!)

(In reply to Gene Lian [:gene] from comment #8) > Try server: https://tbpl.mozilla.org/?tree=Try&rev=99276143e5e8 (all green! > magic!) Sorry, not everything is green but should still be fine. ;)

https://hg.mozilla.org/integration/mozilla-inbound/rev/357778ffa801 Should this have a test?

(In reply to Ryan VanderMeulen from comment #10) > https://hg.mozilla.org/integration/mozilla-inbound/rev/357778ffa801 > > Should this have a test? No, thanks!

Please specify this by bb+ because it needs to be checked in into Aurora.

Blocks blockers.

(In reply to Gene Lian [:gene] from comment #13) > Please specify this by bb+ because it needs to be checked in into Aurora. Just to clarify: you don't need basecamp-blocking+ to push in m-a. You can also get approval without this.

Thanks Ryan, Fabrice, Chris and Mounir for your drive-by supports and comments. Glad to know everything was perfectly done after the weekend. :)