Closed Bug 963234 Opened 10 years ago Closed 10 years ago

Notifications are broken in mulet

Categories

(Firefox OS Graveyard :: General, defect)

defect
Not set
normal

Tracking

(tracking-b2g:backlog)

RESOLVED FIXED
2.0 S1 (9may)
tracking-b2g backlog

People

(Reporter: ochameau, Assigned: gerard-majax)

References

Details

(Whiteboard: [systemsfe], [priority])

Attachments

(2 files, 13 obsolete files)

46 bytes, text/x-github-pull-request
alive
: review+
mikehenrty
: review+
Details | Review
28.79 KB, patch
gerard-majax
: review+
Details | Diff | Splinter Review
That's because b2g/components/AlertsService.js gets the top level window,
and then interacts directly with AlertsHelper object.
That's the only one code I've seen so far using direct references to JS objects within shell.js. Ideally we would only send events to shell.js, via the observer service, or something similar.
That's one way to make it work on the mulet, but there is some alternatives.
One alternative, that would introduce a bigger patch, is to move this AlertsHelper to the xpcom.
Most other components listen to mozContentEvent and send mozChromeEvent from the xpcom/jsm.
Attachment #8364562 - Flags: review?(fabrice)
Comment on attachment 8364562 [details] [diff] [review]
abstract AlertsServices.js from shell.js

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

Alex, I think I like the "move this AlertsHelper to the xpcom" option better.
Attachment #8364562 - Flags: review?(fabrice)
Assignee: poirot.alex → nobody
Since I've been playing with notifications for a while now, taking. In which XPCOM should we move this ? AlertsService ?
Assignee: nobody → lissyx+mozillians
Whiteboard: [systemfe]
Whiteboard: [systemfe] → [systemsfe]
We should move everything alerts-related from shell.js to b2g/components/AlertsService.js

Then I tend to think the only necessary thing you will need by doing that,
is using the SystemApp.jsm component I implemented in bug 963239 (which is in the git branch).
To replace shell.send*Event()/shell.contentBrowser.addEventListener by SystemApp.send*Event()/SystemApp.addEventListener().

Once you have a patch, I can either cherry-pick it from your branch, or you can open a pull request so that I include it in the git branch. That to ease rebasing the branch over last upstream master.
Target Milestone: --- → 1.4 S3 (14mar)
blocking-b2g: --- → backlog
Whiteboard: [systemsfe] → [systemsfe], [priority]
Target Milestone: 1.4 S3 (14mar) → 1.4 S4 (28mar)
Target Milestone: 1.4 S4 (28mar) → 1.4 S5 (11apr)
Target Milestone: 1.4 S5 (11apr) → 1.4 S6 (25apr)
Status: NEW → ASSIGNED
This has been easier than expected. I have a patch that complies to fabrice's suggestions and that is working fine on B2G Desktop. Let's confirm on device tomorrow :).
(In reply to Alexandre LISSY :gerard-majax from comment #5)
> This has been easier than expected. I have a patch that complies to
> fabrice's suggestions and that is working fine on B2G Desktop. Let's confirm
> on device tomorrow :).

Nevermind, this was completely broken on OOP builds
Depends on: 1000337
Moving AlertsHelper out of shell.js
Attachment #8364562 - Attachment is obsolete: true
Attachment #8411830 - Flags: feedback?(poirot.alex)
As far as I could test, it works on device and in b2g desktop. I still have to address the SystemAppProxy.addEventListener() issue and finish implementing showAlertNotification().
After a careful look at other codes using SystemAppProxy, it was just the content-start observer that helped :)
Attachment #8411830 - Attachment is obsolete: true
Attachment #8411830 - Flags: feedback?(poirot.alex)
Attachment #8411961 - Flags: feedback?(poirot.alex)
The whole point of _pendingListeners in SystemAppProxy is to prevent having to wait for content-start...
so there is something wrong with this list!
Attachment #8411961 - Flags: feedback?(poirot.alex) → feedback+
(In reply to Alexandre Poirot (:ochameau) from comment #10)
> The whole point of _pendingListeners in SystemAppProxy is to prevent having
> to wait for content-start...
> so there is something wrong with this list!

Okay, I'll continue investigating this. From what I saw yesterday, the content being used when performing the failing addEventListener() was in a strange shape: innerWidth/innerHeight were 0, for example.
Asking feedback for the removal of content-start.
Attachment #8411961 - Attachment is obsolete: true
Attachment #8412449 - Flags: feedback?(poirot.alex)
> $ git revert 05f96a18fa3479e3e37232aed41ae466258f3f21
> [detached HEAD 1adff20] Revert "Bug 963234 - abstract AlertsServices.js from shell.js r=fabrice"
>  2 files changed, 7 insertions(+), 19 deletions(-)
> $ git cherry-pick bug963234 
> [detached HEAD 6848f99] Bug 963234 - Moving AlertsHelper out of shell.js
>  5 files changed, 275 insertions(+), 206 deletions(-)
>  create mode 100644 b2g/components/AlertsHelper.jsm

So far, it works as expected.
Every bits should be in place now.
Attachment #8412449 - Attachment is obsolete: true
Attachment #8412449 - Flags: feedback?(poirot.alex)
Attachment #8412568 - Attachment is obsolete: true
It looks like try is being green: https://tbpl.mozilla.org/?tree=Try&rev=ceee271dbd2c
Attachment #8412643 - Flags: review?(poirot.alex)
Attachment #8412643 - Flags: review?(fabrice)
Comment on attachment 8412643 [details] [diff] [review]
0001-Bug-963234-Moving-AlertsHelper-out-of-shell.js.patch

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

::: b2g/components/AlertsHelper.jsm
@@ +38,5 @@
> +let AlertsHelper = {
> +
> +  _listeners: {},
> +
> +  init: function alert_init() {

here and everywhere else: no need to name functions anymore.

@@ +42,5 @@
> +  init: function alert_init() {
> +    ppmm.addMessageListener('app-notification-send', this);
> +    ppmm.addMessageListener('alert-notification-send', this);
> +    ppmm.addMessageListener('alert-notification-close', this);
> +    SystemAppProxy.addEventListener('mozContentEvent', this);

these are never removed. They should be in an xpcom-shutdown observer for instance.

@@ +55,5 @@
> +      case kNotificationClick:
> +      case kNotificationClose:
> +        this.handleNotificationEvent(detail);
> +        break;
> +    }

so, we're getting all the mozContentEvent(s) ? Can we switch to use a NotificationEvent instead?

@@ +121,5 @@
> +
> +  registerAppListener: function alert_registerAppListener(uid, listener) {
> +    this._listeners[uid] = listener;
> +
> +    let app = DOMApplicationRegistry.getAppByManifestURL(listener.manifestURL);

using directly DOMApplicationRegistry is discouraged. Switch to the AppsService if possible.

@@ +181,5 @@
> +    // If we have a manifest URL, get the icon and title from the manifest
> +    // to prevent spoofing.
> +    let app = DOMApplicationRegistry.getAppByManifestURL(manifestURL);
> +    DOMApplicationRegistry.getManifestFor(manifestURL).then((aManifest) => {
> +      let helper = new ManifestHelper(aManifest, app.origin);

you can use manifestURL instead of app.origin and not do the getAppByManifestURL() call.
Attachment #8412643 - Flags: review?(fabrice) → review-
(In reply to Fabrice Desré [:fabrice] from comment #17)
> Comment on attachment 8412643 [details] [diff] [review]
> 0001-Bug-963234-Moving-AlertsHelper-out-of-shell.js.patch
> 
> Review of attachment 8412643 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: b2g/components/AlertsHelper.jsm
> @@ +38,5 @@
> > +let AlertsHelper = {
> > +
> > +  _listeners: {},
> > +
> > +  init: function alert_init() {
> 
> here and everywhere else: no need to name functions anymore.

Fixed.

> 
> @@ +42,5 @@
> > +  init: function alert_init() {
> > +    ppmm.addMessageListener('app-notification-send', this);
> > +    ppmm.addMessageListener('alert-notification-send', this);
> > +    ppmm.addMessageListener('alert-notification-close', this);
> > +    SystemAppProxy.addEventListener('mozContentEvent', this);
> 
> these are never removed. They should be in an xpcom-shutdown observer for
> instance.

Right, fixed.

> 
> @@ +55,5 @@
> > +      case kNotificationClick:
> > +      case kNotificationClose:
> > +        this.handleNotificationEvent(detail);
> > +        break;
> > +    }
> 
> so, we're getting all the mozContentEvent(s) ? Can we switch to use a
> NotificationEvent instead?

This is something Alexandre pointed at but was not sure as far as I remember. It means we have to change it also on Gaia side, but if you think it's better, then let's do it :).

And yes, I saw that we get all mozContentEvent that are sent.
Addressing a first set of remarks.
Attachment #8412643 - Attachment is obsolete: true
Attachment #8412643 - Flags: review?(poirot.alex)
Gaia pull request to handle the new mozNotificationEvent.
Depends on: 997949
(In reply to Fabrice Desré [:fabrice] from comment #17)
> Comment on attachment 8412643 [details] [diff] [review]
> 0001-Bug-963234-Moving-AlertsHelper-out-of-shell.js.patch
> 
> Review of attachment 8412643 [details] [diff] [review]:
> -----------------------------------------------------------------
> 

[...]

> 
> @@ +121,5 @@
> > +
> > +  registerAppListener: function alert_registerAppListener(uid, listener) {
> > +    this._listeners[uid] = listener;
> > +
> > +    let app = DOMApplicationRegistry.getAppByManifestURL(listener.manifestURL);
> 
> using directly DOMApplicationRegistry is discouraged. Switch to the
> AppsService if possible.
> 
> @@ +181,5 @@
> > +    // If we have a manifest URL, get the icon and title from the manifest
> > +    // to prevent spoofing.
> > +    let app = DOMApplicationRegistry.getAppByManifestURL(manifestURL);
> > +    DOMApplicationRegistry.getManifestFor(manifestURL).then((aManifest) => {
> > +      let helper = new ManifestHelper(aManifest, app.origin);
> 
> you can use manifestURL instead of app.origin and not do the
> getAppByManifestURL() call.

Both are fixed, but I had to expose getManifestFor() in AppsService.
Updated patch that fixes all of the remarks.
Attachment #8413175 - Attachment is obsolete: true
Attachment #8413646 - Flags: review?(poirot.alex)
Attachment #8413646 - Flags: review?(fabrice)
Moving registerFrame earlier, as suggested on IRC.
Attachment #8413646 - Attachment is obsolete: true
Attachment #8413646 - Flags: review?(poirot.alex)
Attachment #8413646 - Flags: review?(fabrice)
Attachment #8413771 - Flags: review?(poirot.alex)
Attachment #8413771 - Flags: review?(fabrice)
Comment on attachment 8413176 [details] [review]
Link to Github https://github.com/mozilla-b2g/gaia/pull/18709

This is the Gaia part patch that enables the use of mozNotificationEvent.
Attachment #8413176 - Flags: review?(poirot.alex)
Attachment #8413176 - Flags: review?(fabrice)
Comment on attachment 8413176 [details] [review]
Link to Github https://github.com/mozilla-b2g/gaia/pull/18709

I'm not reviewer for such gaia patch.
I think you will have to keep supporting both old and new events and may be cleanup after a bit, otherwise you will have hard time landing these patches without breaking tests!
Attachment #8413176 - Flags: review?(poirot.alex)
(In reply to Alexandre Poirot (:ochameau) from comment #26)
> Comment on attachment 8413176 [details] [review]
> Link to Github https://github.com/mozilla-b2g/gaia/pull/18709
> 
> I'm not reviewer for such gaia patch.
> I think you will have to keep supporting both old and new events and may be
> cleanup after a bit, otherwise you will have hard time landing these patches
> without breaking tests!

Obviously. There was a thread regarding this on dev-b2g back in november 2013, and one conclusion I just recovered was that the majority of people were in favor of:
 1. land a first gaia patch that disable the tests which will get broken
 2. land the gecko change
 3. once gecko is fjxed, re-enable the tests.
Target Milestone: 1.4 S6 (25apr) → 2.0 S1 (9may)
Comment on attachment 8413771 [details] [diff] [review]
0001-Bug-963234-Moving-AlertsHelper-out-of-shell.js.patch

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

I took a brief look at alert modification, but the SystemAppProxy modification looks good (Thanks having tracked down this issue!).
Works great on device and mulet when having both gecko/gaia patches.
Same thing than gaia patch, I'm not reviewer of this code.

::: b2g/components/AlertsHelper.jsm
@@ +97,5 @@
> +      topic = 'alertclickcallback';
> +    } else if (detail.type === kNotificationShow) {
> +      topic = 'alertshow';
> +    } else {
> +      /* desktop-notification-close */

nit: I would have put kNotificationClose instead of string value

@@ +145,5 @@
> +    this._listeners[uid] = listener;
> +
> +    let manifest = appsService.getManifestFor(listener.manifestURL);
> +    if (manifest) {
> +      let helper = new ManifestHelper(manifest, listener.manifestURL);

ManifestHelper expects the origin as second argument, but I don't know how much it can break it.

::: b2g/components/FxAccountsMgmtService.jsm
@@ +51,5 @@
>      Services.obs.addObserver(this, ONLOGIN_NOTIFICATION, false);
>      Services.obs.addObserver(this, ONVERIFIED_NOTIFICATION, false);
>      Services.obs.addObserver(this, ONLOGOUT_NOTIFICATION, false);
> +    SystemAppProxy.addEventListener("mozFxAccountsContentEvent",
> +                                        FxAccountsMgmtService);

nit: indentation.
Attachment #8413771 - Flags: review?(poirot.alex) → feedback+
Addressing nits from feedback+ from :ochameau.
Attachment #8413771 - Attachment is obsolete: true
Attachment #8413771 - Flags: review?(fabrice)
Attachment #8413893 - Flags: review?(fabrice)
Attachment #8413893 - Flags: feedback+
Comment on attachment 8413893 [details] [diff] [review]
0001-Bug-963234-Moving-AlertsHelper-out-of-shell.js.patch

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

::: b2g/components/AlertsHelper.jsm
@@ +11,5 @@
> +const Cc = Components.classes;
> +
> +Cu.import('resource://gre/modules/XPCOMUtils.jsm');
> +Cu.import('resource://gre/modules/Services.jsm');
> +Cu.import("resource://gre/modules/AppsUtils.jsm");

unused.

@@ +35,5 @@
> +}
> +
> +const kNotificationShow  = "desktop-notification-show";
> +const kNotificationClick = "desktop-notification-click";
> +const kNotificationClose = "desktop-notification-close";

double quotes here, single quotes elsewhere... We use double quotes in chrome JS in general.

@@ +76,5 @@
> +      case kNotificationShow:
> +      case kNotificationClick:
> +      case kNotificationClose:
> +        this.handleNotificationEvent(detail);
> +        break;

nit: you could log the default case of getting an unexpected event type.

@@ +143,5 @@
> +
> +  registerAppListener: function(uid, listener) {
> +    this._listeners[uid] = listener;
> +
> +    let manifest = appsService.getManifestFor(listener.manifestURL);

getManifestFor() returns a Promis<manifest>. How does that code work?

::: dom/apps/src/AppsService.js
@@ +40,5 @@
>    },
>  
> +  getManifestFor: function getManifestFor(aManifestURL) {
> +    debug("getManifestFor(" + aManifestURL + ")");
> +    return DOMApplicationRegistry.getManifestFor(aManifestURL);

that will only work in the parent process, but nothing prevents this code to run in the child.

::: dom/interfaces/apps/nsIAppsService.idl
@@ +21,5 @@
>  interface nsIAppsService : nsISupports
>  {
>    mozIApplication getAppByManifestURL(in DOMString manifestURL);
>  
> +  nsISupports getManifestFor(in DOMString manifestURL);

document what the nsISupports actually is.
Attachment #8413893 - Flags: review?(fabrice) → review-
Comment on attachment 8413176 [details] [review]
Link to Github https://github.com/mozilla-b2g/gaia/pull/18709

Looks fine, but you need to ask review to a system app peer.
Attachment #8413176 - Flags: review?(fabrice)
(In reply to Fabrice Desré [:fabrice] from comment #30)
> Comment on attachment 8413893 [details] [diff] [review]
> 0001-Bug-963234-Moving-AlertsHelper-out-of-shell.js.patch
> 
> Review of attachment 8413893 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: b2g/components/AlertsHelper.jsm
> @@ +11,5 @@
> > +const Cc = Components.classes;
> > +
> > +Cu.import('resource://gre/modules/XPCOMUtils.jsm');
> > +Cu.import('resource://gre/modules/Services.jsm');
> > +Cu.import("resource://gre/modules/AppsUtils.jsm");
> 
> unused.

Which one ? We need AppsUtils for ManifestHelper

> 
> @@ +35,5 @@
> > +}
> > +
> > +const kNotificationShow  = "desktop-notification-show";
> > +const kNotificationClick = "desktop-notification-click";
> > +const kNotificationClose = "desktop-notification-close";
> 
> double quotes here, single quotes elsewhere... We use double quotes in
> chrome JS in general.

Sorry, fixed.

> 
> @@ +76,5 @@
> > +      case kNotificationShow:
> > +      case kNotificationClick:
> > +      case kNotificationClose:
> > +        this.handleNotificationEvent(detail);
> > +        break;
> 
> nit: you could log the default case of getting an unexpected event type.

Done.

> 
> @@ +143,5 @@
> > +
> > +  registerAppListener: function(uid, listener) {
> > +    this._listeners[uid] = listener;
> > +
> > +    let manifest = appsService.getManifestFor(listener.manifestURL);
> 
> getManifestFor() returns a Promis<manifest>. How does that code work?

I'm as surprised as you, at first I used it as a Promise and I kept getting errors about lacking .then(). I'll investigate what happens, thanks for confirming that we should be getting a Promise.

> 
> ::: dom/apps/src/AppsService.js
> @@ +40,5 @@
> >    },
> >  
> > +  getManifestFor: function getManifestFor(aManifestURL) {
> > +    debug("getManifestFor(" + aManifestURL + ")");
> > +    return DOMApplicationRegistry.getManifestFor(aManifestURL);
> 
> that will only work in the parent process, but nothing prevents this code to
> run in the child.

Should we make it work in the child or protect against child calling us?

> 
> ::: dom/interfaces/apps/nsIAppsService.idl
> @@ +21,5 @@
> >  interface nsIAppsService : nsISupports
> >  {
> >    mozIApplication getAppByManifestURL(in DOMString manifestURL);
> >  
> > +  nsISupports getManifestFor(in DOMString manifestURL);
> 
> document what the nsISupports actually is.

Done.
Depends on: 1003063
(In reply to Alexandre LISSY :gerard-majax from comment #32)
> (In reply to Fabrice Desré [:fabrice] from comment #30)
> > Comment on attachment 8413893 [details] [diff] [review]

[...]

> > @@ +143,5 @@
> > > +
> > > +  registerAppListener: function(uid, listener) {
> > > +    this._listeners[uid] = listener;
> > > +
> > > +    let manifest = appsService.getManifestFor(listener.manifestURL);
> > 
> > getManifestFor() returns a Promis<manifest>. How does that code work?
> 
> I'm as surprised as you, at first I used it as a Promise and I kept getting
> errors about lacking .then(). I'll investigate what happens, thanks for
> confirming that we should be getting a Promise.

It seems that returning a jsval instead of nsISupports makes it behaving as a Promise as expected.
Updated patch that should fix all previous remarks.
Attachment #8413893 - Attachment is obsolete: true
Attachment #8414384 - Flags: review?(poirot.alex)
Attachment #8414384 - Flags: review?(fabrice)
Fixing some style. For some reason, I get a failure in gaia marionette test: apps/system/test/marionette/desktop_notification_test.js
Attachment #8414384 - Attachment is obsolete: true
Attachment #8414384 - Flags: review?(poirot.alex)
Attachment #8414384 - Flags: review?(fabrice)
Comment on attachment 8414404 [details] [diff] [review]
0001-Bug-963234-Moving-AlertsHelper-out-of-shell.js-r-fab.patch

Gaia integration tests now passes correctly locally, and all breakages seems to be gone also on Travis.
Attachment #8414404 - Flags: review?(poirot.alex)
Attachment #8414404 - Flags: review?(fabrice)
Comment on attachment 8413176 [details] [review]
Link to Github https://github.com/mozilla-b2g/gaia/pull/18709

Alive and Michael, I'd like that you review the Gaia part of those changes. I'm adding mozNotificationEvent handling with the current mozChromeEvent. The plan is that this way, Gaia will work as expected during the transition. Bug 1003063 is already filed to take care of removing mozChromeEvent, hence we will just have to want for its PR to be travis green.
Attachment #8413176 - Flags: review?(mhenretty)
Attachment #8413176 - Flags: review?(alive)
(In reply to Alexandre LISSY :gerard-majax from comment #37)
> Comment on attachment 8413176 [details] [review]
> Link to Github https://github.com/mozilla-b2g/gaia/pull/18709
> 
> Alive and Michael, I'd like that you review the Gaia part of those changes.
> I'm adding mozNotificationEvent handling with the current mozChromeEvent.
> The plan is that this way, Gaia will work as expected during the transition.
> Bug 1003063 is already filed to take care of removing mozChromeEvent, hence
> we will just have to want for its PR to be travis green.

Gaia integration tests should be fixed and passing before and after the Gecko patch: I checked on my system and everything seems to be fine.
Comment on attachment 8414404 [details] [diff] [review]
0001-Bug-963234-Moving-AlertsHelper-out-of-shell.js-r-fab.patch

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

r- just for the changes to getManifestFor().

::: b2g/components/AlertsHelper.jsm
@@ +197,5 @@
> +    // If we have a manifest URL, get the icon and title from the manifest
> +    // to prevent spoofing.
> +    appsService.getManifestFor(manifestURL).then((manifest) => {
> +      let helper = new ManifestHelper(manifest, manifestURL);
> +      send(helper.name, helper.iconURLForSize(128));

nit: use a constant defined at the beginning of this file instead of '128'

::: dom/apps/src/AppsService.js
@@ +40,5 @@
>    },
>  
> +  getManifestFor: function getManifestFor(aManifestURL) {
> +    debug("getManifestFor(" + aManifestURL + ")");
> +    return DOMApplicationRegistry.getManifestFor(aManifestURL);

returning null for the promise will lead to weird errors. I would be ok with either throwing or returning a rejected promise when called in a child process.

::: dom/apps/src/AppsServiceChild.jsm
@@ +75,5 @@
>  
> +  getManifestFor: function getManifestFor(aManifestURL) {
> +    debug("getManifestFor() not yet supported on child!");
> +    return null;
> +  },

remove that one.

::: dom/interfaces/apps/nsIAppsService.idl
@@ +22,5 @@
>  {
>    mozIApplication getAppByManifestURL(in DOMString manifestURL);
>  
>    /**
> +   * Returns a Promise for the manifest for a given manifestURL.

Add: * This is only supported in the parent process.
Attachment #8414404 - Flags: review?(fabrice) → review-
Comment on attachment 8413176 [details] [review]
Link to Github https://github.com/mozilla-b2g/gaia/pull/18709

Sorry, messed up events: mozChromeEvent/mozContentEvent should become mozChromeNotificationEvent/mozContentNotificationEvent rather than just mozNotificationEvent.
Attachment #8413176 - Flags: review?(mhenretty)
Attachment #8413176 - Flags: review?(alive)
Comment on attachment 8414404 [details] [diff] [review]
0001-Bug-963234-Moving-AlertsHelper-out-of-shell.js-r-fab.patch

Sorry, messed up events: mozChromeEvent/mozContentEvent should become mozChromeNotificationEvent/mozContentNotificationEvent rather than just mozNotificationEvent.
Attachment #8414404 - Flags: review?(poirot.alex)
Attachment #8414404 - Flags: review-
Updating patch: fixing latest comments reported by fabrice and changing mozChrome/mozContent events in a saner manner. PR has been updated accordingly.

Pending try: https://tbpl.mozilla.org/?tree=Try&rev=53412d4455c9
Pending travis: https://travis-ci.org/mozilla-b2g/gaia/builds/24081668
Attachment #8414404 - Attachment is obsolete: true
Comment on attachment 8413176 [details] [review]
Link to Github https://github.com/mozilla-b2g/gaia/pull/18709

Making use of {mozChrome,mozContent}NotificationEvent all tests are passing, travis is green.
Attachment #8413176 - Flags: review?(mhenretty)
Attachment #8413176 - Flags: review?(alive)
Attachment #8415066 - Flags: review?(mhenretty)
Attachment #8415066 - Flags: review?(fabrice)
Comment on attachment 8415066 [details] [diff] [review]
0001-Bug-963234-Moving-AlertsHelper-out-of-shell.js-r-fab.patch

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

r=me with nit fixed.

::: dom/apps/src/AppsService.js
@@ +48,5 @@
> +    } else {
> +      deferred.reject(
> +        new Error("Calling getManifestFor() from child is not supported"));
> +    }
> +    return deferred.promise;

why not just return DOMApplicationRegistry.getManifestFor(aManifestURL) when in the parent and create the new promise that we reject only in the child?

::: dom/interfaces/apps/nsIAppsService.idl
@@ +23,5 @@
>    mozIApplication getAppByManifestURL(in DOMString manifestURL);
>  
>    /**
> +   * Returns a Promise for the manifest for a given manifestURL.
> +   * This is only supported in the parent process.

: the promise will be rejected in content processes.
Attachment #8415066 - Flags: review?(fabrice) → review+
Comment on attachment 8413176 [details] [review]
Link to Github https://github.com/mozilla-b2g/gaia/pull/18709

Nice interim solution.
Attachment #8413176 - Flags: review?(mhenretty) → review+
Attachment #8415066 - Flags: review?(mhenretty) → review+
Thanks, gaia part merged: https://github.com/mozilla-b2g/gaia/commit/1b2f9f11c5cd3a39d1c677190fe9bffa059569bf

I'll address latest fabrice comment, update the patch and mark it as checkin needed :)
Blocks: 1003063
No longer depends on: 1003063
Fixing latest nits reported by fabrice.
Attachment #8415066 - Attachment is obsolete: true
Attachment #8415844 - Flags: review+
Try is green, except one unrelated test https://tbpl.mozilla.org/?tree=Try&rev=e75f17f6c243
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/caa1de36d1ad
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Blocks: 1037371
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.