Alexandre Poirot [:ochameau] Assignee
	Description • 11 years ago

In the Mulet, b2g's shell.html document isn't the top level chrome window anymore. Instead, it becomes a document loaded in a regular firefox tab.

So that, all code similar to this one are going to fail on the Mulet:
  let browser = Services.wm.getMostRecentWindow("navigator:browser");
  if (browser && browser.shell) {
    browser.shell.sendChromeEvent(event);

We should make it easy to support both environmenet and ease communicating with the system app when you are in a platform code.

	Alexandre Poirot [:ochameau] Assignee
	Comment 1 • 11 years ago

Implements "SystemApp.jsm" to abtract and ease interacting with the system app from platform code.

This jsm exposes only the core features, the platform code needs to interact with gaia via the system app: send and receive events.
It factors out some code related to pending events, duplicated between shell.js, UpdatesPrompt and WebappsUpdater.
It then makes other codepaths stronger by always have the pending event support.
It may also fix the random errors when Services.vm.getMostRecentWindow(navigator:browser).shell.contentBrowser was undefined!
All components trying to listen for content events seem to have all faced scenarios where
the system app iframe wasn't created yet, while they were already trying to listen for events.
In this little helper we can also have "pending listeners", to prevent any such startup race.

fabrice, before I start testing this patch extensively, can you confirm that this sounds good to you?

	[:fabrice] Fabrice Desré
	Comment 2 • 11 years ago

Comment on attachment 8364602 [details] [diff] [review]
patch

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

::: b2g/components/ActivitiesGlue.js
@@ +13,5 @@
>  
> +XPCOMUtils.defineLazyGetter(this, 'SystemApp', function() {
> +  Cu.import('resource://gre/modules/SystemApp.jsm');
> +  return SystemApp;
> +});

There's XPCOMUtils.defineLazyModuleGetter that does that for you already.

	Jan Keromnes [:janx]
	Comment 4 • 11 years ago

Since you're touching `sendCustomEvent()`, could you please make it return the event, so that senders can verify if `event.isDefaultPrevented()` after sending it?

	Alexandre Poirot [:ochameau] Assignee
	Comment 6 • 11 years ago

Rebased and fixed some issues discovered when testing on device.

	Alexandre Poirot [:ochameau] Assignee
	Comment 7 • 11 years ago

Add a mochitest against SystemApp.jsm

	Alexandre Poirot [:ochameau] Assignee
	Comment 8 • 11 years ago

https://tbpl.mozilla.org/?tree=Try&rev=a8bb0309a92d

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)
	Comment 10 • 11 years ago

Hi @Alexandre, previously I add some test cases that use getMostRecentWindow(navigator:browser) [1]. You might need to modify it as well. :)

[1] http://dxr.mozilla.org/mozilla-central/source/b2g/chrome/content/test/mochitest/RecordingStatusChromeScript.js#20

	Alexandre Poirot [:ochameau] Assignee
	Comment 11 • 11 years ago

(In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #10)
Thanks for the report, I updated the patch.

	Alexandre Poirot [:ochameau] Assignee
	Comment 12 • 11 years ago

https://tbpl.mozilla.org/?tree=Try&rev=95f4b11dfbd3

	Alexandre Poirot [:ochameau] Assignee
	Comment 13 • 11 years ago

Rebased patch, still waiting on bug 965257 before proceeding with this patch.
https://tbpl.mozilla.org/?tree=Try&rev=5670f35f5879

	Alexandre Poirot [:ochameau] Assignee
	Comment 14 • 10 years ago

Just rebased.
https://tbpl.mozilla.org/?tree=Try&rev=4e12fa2c9fe1

	Alexandre Poirot [:ochameau] Assignee
	Comment 15 • 10 years ago

Uses Cu.cloneInto instead of ObjectWrapper.

	Alexandre Poirot [:ochameau] Assignee
	Comment 16 • 10 years ago

Comment on attachment 8396443 [details] [diff] [review]
patch

This patch touches various code, do not hesitate to flag anyone else for review.

It basically exposes a new JSM, SystemApp which allows to easily
listen for events on the system app as well as dispatch events on it.
It has be to be used everywhere in platform code in order to ensure b2g to work on the mulet.

I'll post on dev-b2g about this once reviewed.

Fabrice, I'm asking vivien to review this patch as I already have other patches pending you for review. So I'm trying to balance reviews here, but do not hesitate to review this one as well.

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 17 • 10 years ago

Comment on attachment 8396443 [details] [diff] [review]
patch

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

It's a nice clean up! I would like to enforce a bit the policy about how to use mozChromeEvent/mozContentEvent and abstract that so people will not mess up with Gaia and creates tons of new events however.

In the future it would also be nice if the |id| check for a message that is supposed to be a return value of a chrome event could be done into this helper. That would make the code more robust.

::: b2g/chrome/content/shell.js
@@ +30,5 @@
>  
>  Cu.import('resource://gre/modules/DownloadsAPI.jsm');
>  
> +XPCOMUtils.defineLazyModuleGetter(this, "SystemApp",
> +                                  "resource://gre/modules/SystemApp.jsm");

Can we give it a better name, something like SystemAppProxy.jsm ?

@@ +672,5 @@
>  #endif
>  
>        Services.obs.notifyObservers(null, "browser-ui-startup-complete", "");
>  
> +      SystemApp.setIsLoaded();

SystemAppProxy.setIsReady();

::: b2g/chrome/content/test/mochitest/RecordingStatusChromeScript.js
@@ +2,5 @@
>     http://creativecommons.org/publicdomain/zero/1.0/ */
>  
>  const { classes: Cc, interfaces: Ci, results: Cr, utils: Cu } = Components;
>  const { Services } = Cu.import('resource://gre/modules/Services.jsm');
> +const { SystemApp } = Cu.import("resource://gre/modules/SystemApp.jsm");

s/"/'

::: b2g/components/FxAccountsMgmtService.jsm
@@ +36,2 @@
>    _onFulfill: function(aMsgId, aData) {
> +    SystemApp.sendCustomEvent("mozFxAccountsChromeEvent", {

Those should not be allowed to send a customEvent. The dirty mozChromeEvent/mozContentEvent hack is not intended to start to have tons of custom events.

Same comments for the others.

Sorry to put that on your plate but since you're refactoring the mechanism to discuss from Gecko to the System App it seems a good time to enforce some practices.

::: b2g/components/FxAccountsUIGlue.js
@@ +29,3 @@
>      let id = uuidgen.generateUUID().toString();
>  
> +    SystemApp.addEventListener("mozFxAccountsRPContentEvent",

Same comment than the other FXAccountMgmtService file.

::: b2g/components/SystemApp.jsm
@@ +21,5 @@
> +  // either the system app one, or a top level xul window one,
> +  // if the system app isn't loaded yet.
> +  get navigator() {
> +    return this._frame ? this._frame.contentWindow.navigator
> +                       : Services.wm.getMostRecentWindow('navigator:browser').navigator;

In which case do we want to return the system app navigator object instead of the top xul level window ? The system app can miss some permissions (ok that's unlikely).

@@ +22,5 @@
> +  // if the system app isn't loaded yet.
> +  get navigator() {
> +    return this._frame ? this._frame.contentWindow.navigator
> +                       : Services.wm.getMostRecentWindow('navigator:browser').navigator;
> +  },

More generally I don't think this method should be part of this module. People should use an XPCOM if they want to access the mozApps database or the ICC status. That's fine for the purpose of this patch but can you open a followup to remove the navigator calls inside the xpcom components ?

@@ +38,5 @@
> +  },
> +
> +  // To call when it is ready to receive events
> +  setIsLoaded: function () {
> +    this._isLoaded = true;

Maybe you want to throw if someone call setIsReady when the module is already ready ?

@@ +51,5 @@
> +  /*
> +   * Common way to send an event to the system app.
> +   *
> +   * // In gecko code:
> +   *   SystemApp.sendCustomEvent('foo', { data: 'bar' });

Seems like there is a mix of ' and " in this file :P

@@ +57,5 @@
> +   *   window.addEventListener('foo', function (event) {
> +   *     event.details == 'bar'
> +   *   });
> +   */
> +  sendCustomEvent: function systemApp_sendCustomEvent(type, details) {

People should not really be able to specify a custom event name. Specific types should be passed into the |type| field of the event detail.

Can you enforce the 'mozChromeEvent' name here and remove the type parameters. Then you can also remove the sendCustomEvent method and rename sendCustomEvent method to dispatchEvent.

@@ +65,5 @@
> +    // queue events until someone calls setIsLoaded
> +    if (!this._isLoaded || !content) {
> +      this._pendingChromeEvents.push([type, details]);
> +      return null;
> +    }

nit: add a line break after this block since this block will return. That makes it easier to see.

@@ +77,5 @@
> +    } else {
> +      payload = details ? Cu.cloneInto(details, content) : {};
> +    }
> +
> +    event.initCustomEvent(type, true, true, payload);

I don't think any of those events are really cancelable. Let turn the third parameter to false.

@@ +92,5 @@
> +  // Listen for dom events on the system app
> +  addEventListener: function systemApp_addEventListener() {
> +    let content = this._frame ? this._frame.contentWindow : null;
> +    if (!content) {
> +      Cu.reportError("Trying to add a system app event listener while the app is still loading");

Is it really an error ? Not sure this message won't just be pain in the ass.

@@ +97,5 @@
> +      this._pendingListeners.push(arguments);
> +      return false;
> +    }
> +
> +    content.addEventListener.apply(content, arguments);

Can we abstract the mozContentEvent name here ? In theory people should not listen anything else than mozContentEvent and I would be happy if this is enforced by the helper.

It also seems like the fx accounts code does not do that... That's bad. The type of events likely needs to lived into a |type| field of event.detail.

@@ +105,5 @@
> +  removeEventListener: function systemApp_removeEventListener() {
> +    let content = this._frame ? this._frame.contentWindow : null;
> +    if (content) {
> +      content.removeEventListener.apply(content, arguments);
> +    }

This function does not seems to remove something from the pendingListeners. So if someone had a listener then decide to remove it or some reasons it will still be in the pendingListeners list.

We can likely abstract the 'mozContentEvent' type parameters as well.

::: b2g/components/test/mochitest/systemapp_helper.js
@@ +65,5 @@
> +    });
> +  },
> +
> +  function earlyEvents() {
> +    // Immediatly try to send events

s/Immediatly/Immediately

::: b2g/components/test/mochitest/test_systemapp.html
@@ +4,5 @@
> +https://bugzilla.mozilla.org/show_bug.cgi?id=963239
> +-->
> +<head>
> +  <meta charset="utf-8">
> +  <title>Permission Prompt Test</title>

Permission prompt test ? :)

	[:fabrice] Fabrice Desré
	Comment 18 • 10 years ago

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #17)

> It's a nice clean up! I would like to enforce a bit the policy about how to
> use mozChromeEvent/mozContentEvent and abstract that so people will not mess
> up with Gaia and creates tons of new events however.

I guess we'll need to discuss that outside of this bug, but I disagree that using just mozChromeEvent is good. It already caused us performance issues with app launch time because we had > 20 listeners for this event and the one dealing with apps launching was one of the last ones. That meant ~30ms lost for nothing.

	Alexandre Poirot [:ochameau] Assignee
	Comment 19 • 10 years ago

Addressed most comments.
I avoided modifying the actual implementation of each b2g components.
Modifying the way these components communicate with the system app
requires a lot of work (gecko and gaia patches, that can potentially break each other).

For example, ContentPermissionPrompt.js with its sendToBrowserWindow helper function,
doesn't listen for any explicit mozContentEvent with one precise type,
but rather filter by id...

So the suggestion to listen for one precise type of mozContentEvent
doesn't really work as-is with this component.

Similar kind of complexity also happens with fx account code.

Whereas I can understand such cleanup, it really has nothing to do with the Mulet,
and such refactoring should be made by each component owner,
which know exatly how to properly test all this code.

Also, instead of forcing a refactoring by only exposing a clean API,
we should first expose what we think is a better way to communicate with the system app,
and then refactor each component one by one.

	Alexandre Poirot [:ochameau] Assignee
	Comment 20 • 10 years ago

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #17)
> ::: b2g/chrome/content/shell.js
> @@ +30,5 @@
> >  
> >  Cu.import('resource://gre/modules/DownloadsAPI.jsm');
> >  
> > +XPCOMUtils.defineLazyModuleGetter(this, "SystemApp",
> > +                                  "resource://gre/modules/SystemApp.jsm");
> 
> Can we give it a better name, something like SystemAppProxy.jsm ?

Done

> ::: b2g/components/FxAccountsMgmtService.jsm
> @@ +36,2 @@
> >    _onFulfill: function(aMsgId, aData) {
> > +    SystemApp.sendCustomEvent("mozFxAccountsChromeEvent", {
> 
> Those should not be allowed to send a customEvent. The dirty
> mozChromeEvent/mozContentEvent hack is not intended to start to have tons of
> custom events.
> 
> Same comments for the others.
> 
> Sorry to put that on your plate but since you're refactoring the mechanism
> to discuss from Gecko to the System App it seems a good time to enforce some
> practices.
> 

Renamed to SystemApp._sendCustomEvent, to at least say it better to use sendChromeEvent.

> ::: b2g/components/SystemApp.jsm
> @@ +21,5 @@
> > +  // either the system app one, or a top level xul window one,
> > +  // if the system app isn't loaded yet.
> > +  get navigator() {
> > +    return this._frame ? this._frame.contentWindow.navigator
> > +                       : Services.wm.getMostRecentWindow('navigator:browser').navigator;
> 
> In which case do we want to return the system app navigator object instead
> of the top xul level window ? The system app can miss some permissions (ok
> that's unlikely).

It's not really important, I switched back all call site to use navigator:browser as before.

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 21 • 10 years ago

(In reply to Fabrice Desré [:fabrice] NOT READING BUGMAIL from comment #18)
> (In reply to Vivien Nicolas (:vingtetun) (:21) from comment #17)
> 
> > It's a nice clean up! I would like to enforce a bit the policy about how to
> > use mozChromeEvent/mozContentEvent and abstract that so people will not mess
> > up with Gaia and creates tons of new events however.
> 
> I guess we'll need to discuss that outside of this bug, but I disagree that
> using just mozChromeEvent is good. It already caused us performance issues
> with app launch time because we had > 20 listeners for this event and the
> one dealing with apps launching was one of the last ones. That meant ~30ms
> lost for nothing.

That should be fixeable directly in the system app if we apply the same rule. 
We could have one mozChromeListener in the system app that redispatch those events as custom events with different names.

But instead of having multiple places that are dispatching tons of different events we have one place that we control and may be able to clean at some point.

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 22 • 10 years ago

Comment on attachment 8400778 [details] [diff] [review]
patch

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

r+ with SystemAppProxy.removeEventListener removing events from pendings.

::: b2g/chrome/content/payment.js
@@ +239,5 @@
>      // In order to avoid race conditions, we wait for the UI to notify that
>      // it has successfully closed the payment flow and has recovered the
>      // caller app, before notifying the parent process to fire the success
>      // or error event over the DOMRequest.
> +    SystemAppProxy.addEventListener(kClosePaymentFlowEvent,

mozContentEvent ?

::: b2g/components/SystemAppProxy.jsm
@@ +12,5 @@
> +this.EXPORTED_SYMBOLS = ['SystemAppProxy'];
> +
> +let SystemAppProxy = {
> +  _frame: null,
> +  _isLoaded: false,

s/_isLoaded/_isReady/

@@ +13,5 @@
> +
> +let SystemAppProxy = {
> +  _frame: null,
> +  _isLoaded: false,
> +  _pendingChromeEvents: [],

s/_pendingChromeEvents/_pendingEvents/

@@ +100,5 @@
> +  removeEventListener: function systemApp_removeEventListener() {
> +    let content = this._frame ? this._frame.contentWindow : null;
> +    if (content) {
> +      content.removeEventListener.apply(content, arguments);
> +    }

You want to iterate over the pending events in the case where there is no content.

::: b2g/components/test/mochitest/systemapp_helper.js
@@ +1,2 @@
> +const Cu = Components.utils;
> +

Can you add a test to check that removeEventListener removes events from the pending lists as well ?

	Alexandre Poirot [:ochameau] Assignee
	Comment 23 • 10 years ago

Addressed last review, also fixed some indentation and unnecessary modification made to updateprompt and webappsupdater.

https://tbpl.mozilla.org/?tree=Try&rev=0209a7ea7064

	Alexandre Poirot [:ochameau] Assignee
	Comment 24 • 10 years ago

Fixed two failing tests.
One typo _pendingListener instead of _pendingListeners,
and I had to modify fxaccount xpcshell test.

https://tbpl.mozilla.org/?tree=Try&rev=3283878575e1

	Alexandre Poirot [:ochameau] Assignee
	Comment 25 • 10 years ago

Manually tested:
  Activities, by changing the wallpaper
  ContentPermissions, by getting the prompt in Here maps and correct location
  FxAccount, with Test FxA app
  SignIn, with UI test
  Webapps, Got two hosted and one packaged app being updated with correct notification

Poorly tested:
  Payment, Don't know how to test, blocked on pin code question in the in-app payment test app.
  Update, Don't know how to test.

	Alexandre Poirot [:ochameau] Assignee
	Comment 26 • 10 years ago

Try is green!

	Ryan VanderMeulen [:RyanVM]
	Comment 27 • 10 years ago

https://hg.mozilla.org/integration/b2g-inbound/rev/c50c4788f313

	Ryan VanderMeulen [:RyanVM]
	Comment 28 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/c50c4788f313

	Yuan Xulei [:yxl] (Leave from May 18)
	Comment 29 • 10 years ago

Gaia keyboard needs sendChromeEvent when running in Firefox Nightly. This bug broke that. Could you make SystemApp.jsm available on Firefox Nightly as well?

See bug 996528 for details.

	Alexandre Poirot [:ochameau] Assignee
	Comment 30 • 10 years ago

Let's discuss about that in bug 996528