1038811 - Implement Push API

Status: RESOLVED FIXED

(Core :: DOM: Push Subscriptions, defect)

Description

[Tyler Smith [:tysmith]]

with the new API

Comment 1

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Note that the spec hasn't been updated yet to reflect the discussed modifications - http://lists.w3.org/Archives/Public/public-webapps/2014AprJun/0223.html

Comment 2

[Tyler Smith [:tysmith]]

Attachment: API calls working for desktop

Just wanted a review.  Database calls need to be changed and permission prompt isn't implemented, but the desktop implementation works aside from that.

Comment 3

[Tyler Smith [:tysmith]]

Attachment: 1038811-newdbcalls.patch

Fixed the DB calls to account for only having one registration per origin.

Comment 4

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Before we do anything with this patch, please do the following:
File a bug blocking this bug that simply does a copy of the pristine push sources from dom/push to dom/push2. You'll have to rev UUIDs and contract IDs there to prevent collisions with dom/push. You'll also have to change the PushDB database name to not conflict with Push 1. Also add another pref dom.push2.enabled that will control this Push2 implementation. You'll then have to rebase the patches on top of that directory.

Comment 5

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8456317 [details] [diff] [review]
API calls working for desktop

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

::: dom/base/IndexedDBHelper.jsm
@@ +150,5 @@
>              failureCb("UnknownError");
>            }
>          }
>        };
> +

This change is not needed.

::: dom/push/src/Push.js
@@ +52,5 @@
>      debug("init()");
>  
> +    if (PrivateBrowsingUtils.isWindowPrivate(aWindow)) {
> +      debug("Push is disabled in private browsing mode.");
> +      return null;

Components can no longer return anything except undefined. We should continue here and move this check to when any actual method calls are attempted. The purpose of isPermission() in this spec is for these kind of things. The new workflow is that the app would call isPermission() and if we are in private browsing, we should deny permission.

Attempts to call register/unregister/isRegistered() should all fail if permission is not granted.

@@ +71,5 @@
> +      let perm = Services.perms.testExactPermissionFromPrincipal(principal,
> +                                                                 "push");
> +      if (perm != Ci.nsIPermissionManager.ALLOW_ACTION) {
> +        debug("App does not have push permission");
> +        return null;

Again, can't return null. And we should expose the interface but not allow calls.

@@ +91,5 @@
>        "PushService:Register:KO",
>        "PushService:Unregister:OK",
>        "PushService:Unregister:KO",
> +      "PushService:Isregistered:OK",
> +      "PushService:Isregistered:KO"

Nit: Purely for stylistic reasons, can we call this IsRegistered?

@@ +100,5 @@
>    },
>  
>    receiveMessage: function(aMessage) {
>      debug("receiveMessage()");
>      let request = this.getRequest(aMessage.data.requestID);

Since resolver replaces request, this can be gotten rid of.

@@ +133,5 @@
>          debug("NOT IMPLEMENTED! receiveMessage for " + aMessage.name);
>      }
>    },
>  
> +  register: function(aOptions) {

Please add a comment saying aOptions are unused for now.

@@ +138,2 @@
>      debug("register()");
>      let req = this.createRequest();

Unnecessary since Promise has superseded it. Applies to all methods.

@@ +138,4 @@
>      debug("register()");
>      let req = this.createRequest();
> +    let promise = this.createPromise((resolve, reject) => {
> +    

Trailing whitespace.

@@ +148,1 @@
>  

We should check permission somewhere here and reject the promise.

@@ +148,3 @@
>  
> +      let resolverID = this.getPromiseResolverId({resolve:resolve, reject:reject})
> +    

trailing WS

@@ +196,5 @@
> +    return promise;
> +
> +  },
> +
> +  hasPermission: function() {

You have to use the nsIContentPermissionPrompt infrastructure here.

::: dom/push/src/PushService.jsm
@@ +45,5 @@
>  // Set debug first so that all debugging actually works.
>  gDebuggingEnabled = prefs.get("debug");
>  
>  const kPUSHDB_DB_NAME = "push";
> +const kPUSHDB_DB_VERSION = 2; // Change this if the IndexedDB format changes

We can't change the version, but will have to use a new db to allow apps to continue using the older Push API for a while. Could you file a bug on a deprecation path for the older API, and look into how Push 1 & 2 could operate over the same websocket? (If possible). Or is it possible to land the http2 changes before this ships and modify the old Push to use it under the hood, by internally multiplexing the apps' multiple channels on a single channel?

@@ +70,5 @@
>  
>    upgradeSchema: function(aTransaction, aDb, aOldVersion, aNewVersion) {
>      debug("PushDB.upgradeSchema()")
>  
> +    aDb.deleteObjectStore(kPUSHDB_STORE_NAME)

Why is this required?

@@ +81,5 @@
>  
>      // index to fetch records per manifest, so we can identify endpoints
>      // associated with an app. Since an app can have multiple endpoints
>      // uniqueness cannot be enforced
>      objectStore.createIndex("manifestURL", "manifestURL", { unique: false });

Since we only support 1 registration per manifest, unique can be true right?

@@ +83,5 @@
>      // associated with an app. Since an app can have multiple endpoints
>      // uniqueness cannot be enforced
>      objectStore.createIndex("manifestURL", "manifestURL", { unique: false });
> +
> +    objectStore.createIndex("pageURL", "pageURL", { unique: false });

same here.

@@ +651,5 @@
>     * networks not supporting UDP. So the websocket should only be shutdown if
>     * onServerClose indicates UDP wakeup.  If WS is closed due to socket error,
>     * _reconnectAfterBackoff() is called.  The retry alarm is started and when
>     * it times out, beginWSSetup() is called again.
> +   * 

trailing ws

@@ +879,1 @@
>  

These changes went away again in bug 1026599. You should rebase on that once that bug lands on m-c.

@@ +1565,5 @@
> +          this._onRegistrationsSuccess.bind(this, aPageRecord, resolve),
> +          function(error) { reject('Database error') } 
> +        );
> +      }
> +      else if (aPageRecord.pageURL) {

For pages, you should extract the origin and save on that. Otherwise every page on the same origin will have its own entry. In fact some of this cruft can be cleaned up if we can stop treating manifest URLs specially and just use the concept of an origin. Asking baku about this.

@@ +1590,5 @@
>      pushRecords.forEach(function(pushRecord) {
> +      if(pushRecord.manifestURL == aPageRecord.manifestURL 
> +        && pushRecord.pageURL == aPageRecord.pageURL){
> +          registrations.push({
> +              __exposedProps__: { pushEndpoint: 'r', version: 'r' },

Ah. we should really stop using exposedProps. The right thing to do here would be to define a PushRegistration webidl interface as discussed in the spec plan. PushService would simply pass JSON to the PushRegistrationManager instance, which would create a PushRegistration from it and resolve the Promise with it.

::: dom/webidl/PushManager.webidl
@@ +9,2 @@
>   JSImplementation="@mozilla.org/push/PushManager;1",
> + //CheckPermissions="push",

This is ok for testing, but since you will be landing a prompt you want to land with this uncommented :)

@@ +11,2 @@
>   Pref="services.push.enabled"]
>  interface PushManager {

This is renamed to PushRegistrationManager. Also rename the file.

Comment 6

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

baku,

I believe you've written several APIs where you had to extract a viable 'origin' to associate things with. What is the minimum information that should be stored to uniquely identify origins across both apps and web pages? AFAIK, if it's an app we use the manifest url as the origin, otherwise we use the 'domain' as the origin. Is that all we need to store or do we also store the appId and stuff?

Comment 7

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8456383 [details] [diff] [review]
1038811-newdbcalls.patch

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

General direction is fine, but needs fixing about resolving origins properly and safely from the PushRegistrationManager.

::: dom/push/src/PushService.jsm
@@ +45,5 @@
>  // Set debug first so that all debugging actually works.
>  gDebuggingEnabled = prefs.get("debug");
>  
>  const kPUSHDB_DB_NAME = "push";
> +const kPUSHDB_DB_VERSION = 3; // Change this if the IndexedDB format changes

You don't need to change this since '2' hasn't landed and so no one in the world is using it. So you can just stick to 2, although we'll just use a different DB for backwards compat, so this can just be 1.

@@ +77,5 @@
>                                              { keyPath: "channelID" });
>  
> +    // The URL will either be a manifest URL or a page URL depending
> +    // on who is using push.  Only one endpoint per app/page.
> +    objectStore.createIndex("URL", "URL", { unique: true });

I'd call this origin. Appropriate renames throughout this file.

Comment 8

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

If we intend to land and enable Push 2 (the per spec Push API) while also keeping Push 1 available on B2G for a few more (how many?) releases, we can't just copy dom/push out into dom/push2 and have two of them working side by side. This will just lead to a device where both old and new apps are used having to maintain 2 websockets and 2 PushServices, neither of which is ideal with resource constraints.

I'd like to propose the following migration path:

Land the DOM facing Push 2 changes as dom/push/src/Push2.js or whatever. Since it is exposed on a different property on navigator (pushRegistrationManager vs push), the web-facing compat issues are non-existent.

The PushService stays the way it is for the most part, i.e. as far as the PushService is concerned, a single app can have multiple registrations.
The PushService will need the following changes:
- Upgrade the database to allow store and lookup by 'origin' instead of purely manifest URLs. I believe we can treat manifestURLs as the origin in the app case.
- Write a new set of receiveMessage handlers to interact with the Push 2 DOM API, that present the facade of there only being one registration per origin by adding the appropriate DB checks. (Tyler, so don't land the database changes yet!)
- On receiving a push, the service should be able to decide if it is an old style or new style app and launch the page or spawn a serviceworker accordingly.

This raises the question of what we should do when HTTP2 is ready to be enabled. Can we seamlessly migrate to it on the backend, without old style apps realizing any change? That is they continue to have multiple registrations even on HTTP2. Ben, since you have been working on the protocol, you might have the best idea here.

Comment 9

[Ben Brittain (:bbrittain)]

I don't really see a way to seamlessly migrate to webpush, the protocols are rather different. For example, webpush has no guarantees of atomicity in the message id, it also includes data as primary part of the protocol.

I think the best course of action would be to keep both services around for a release, with a notice of deprecation for simplepush. I don't really see how the new push2 api would work with the old simplepush setup, but it may be possible. It would probably be best to keep both apis around and distinct.


also, keep in mind that webpush has not yet been standardized. We should probably wait till after the IETF so as to see what they say.

Comment 10

[Tyler Smith [:tysmith]]

Attachment: Everything is here

Comment 11

[Tyler Smith [:tysmith]]

Sorry Nikhil, I didn't see your review.  Not everything is there, I just got the prompt/permission stuff working.  I'll start working on the changes you suggested.

Comment 12

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

:bbrittain accidentally canceled baku's ni? (comment 6)

Comment 13

[Tyler Smith [:tysmith]]

Attachment: 1038811-all-rev.patch

revision of all patch.  Has dual compat.

Comment 14

[Tyler Smith [:tysmith]]

Attachment: 1038811-all-rev.patch

Comment 15

[Tyler Smith [:tysmith]]

Attachment: 1038811-all-rev.patch

Comment 16

[Andrea Marchesini [:baku]]

> I believe you've written several APIs where you had to extract a viable
> 'origin' to associate things with. What is the minimum information that
> should be stored to uniquely identify origins across both apps and web
> pages? AFAIK, if it's an app we use the manifest url as the origin,
> otherwise we use the 'domain' as the origin. Is that all we need to store or
> do we also store the appId and stuff?

Yes, something like this:

  uint16_t appStatus = aPrincipal->GetAppStatus();
  if (appStatus == nsIPrincipal::APP_STATUS_NOT_INSTALLED) {
    nsContentUtils::GetUTFOrigin(aPrincipal, origin);
    ...
  } else {
   nsCOMPtr<nsIAppsService> appsService = ...
   appsService->GetManifestURLByLocalId(aPrincipal->GetAppId(), origin);

Comment 17

[Tyler Smith [:tysmith]]

Attachment: newapiexposure.patch

new Webpush stuff living in dom/webpush.  Webpush only currently implemented for Desktop.

Comment 18

[Tyler Smith [:tysmith]]

Attachment: newapiexposure.patch

I overlooked deleting something.

Comment 19

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8480815 [details] [diff] [review]
newapiexposure.patch

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

I've mostly glossed over the service itself since much of that code is from the original. Once the new patch brings the API return values etc. up to the latest spec, I'll look at it again.

Also please add tests for this API as another patch before landing this. Since the API is exposed to webpages, you can just use simple mochitests.

::: browser/components/nsBrowserGlue.js
@@ +1963,5 @@
> +    this._showPrompt(aRequest, message, "push", actions, "push",
> +                     null, null);
> +
> +  },
> +

You'll also need review from a browser peer for these changes.

::: browser/components/preferences/aboutPermissions.js
@@ +334,5 @@
>      let value = (aValue != this.DENY);
>      Services.prefs.setBoolPref("full-screen-api.enabled", value);
>    },
> +  get push() {
> +    if (!Services.prefs.getBoolPref("dom.push.userEnabled")) {

I think you misunderstood userEnabled. all.js documents its purpose. You want just enabled here.

@@ +341,5 @@
> +    return this.UNKNOWN;
> +  },
> +  set push(aValue) {
> +    let value = (aValue != this.DENY);
> +    Services.prefs.setBoolPref("dom.push.userEnabled", value);

ditto.

::: browser/locales/en-US/chrome/browser/browser.properties
@@ +355,5 @@
>  
> +# Push Notifications
> +push.allowForSession=Allow for Session
> +push.allowForSession.accesskey=S
> +push.alwaysAllow=Always Allow Notifications

Push Notifications to disambiguate between the notification API and this.

::: dom/base/IndexedDBHelper.jsm
@@ +72,4 @@
>      };
>      req.onerror = function (aEvent) {
>        if (DEBUG) debug("Failed to open database: " + self.dbName);
> +      debug(aEvent.target.error.name);

This entire file should be reverted to the original.

::: dom/moz.build
@@ +62,4 @@
>      'messages',
>      'power',
>      'push',
> +    'webpush',

Please add comments explaining which one is old and which one is new.

::: dom/webidl/PushManager.webidl
@@ +7,4 @@
>  [NoInterfaceObject,
>   NavigatorProperty="push",
>   JSImplementation="@mozilla.org/push/PushManager;1",
> + //CheckPermissions="push",

Revert this change.

If webpush uses the same permission name, you will have to change that.

::: dom/webidl/WebPushManager.webidl
@@ +3,5 @@
> +* License, v. 2.0. If a copy of the MPL was not distributed with this file,
> +* You can obtain one at http://mozilla.org/MPL/2.0/.
> +*/
> +
> +[NoInterfaceObject,

Why the NoInterfaceObject?

@@ +8,5 @@
> + Exposed=Window,
> + JSImplementation="@mozilla.org/webpush/WebPushManager;1",
> + //CheckPermissions="push",
> + Pref="dom.push.enabled"]
> +interface WebPushManager {

Please clarify from the spec what this interface is to be called. This will appear as an interface on the window object, and hence needs to have the right name per spec. You also want to modify dom/tests/mochitest/general/test_interfaces.html and make sure that test passes.

https://github.com/w3c/push-api/pull/13

@@ +12,5 @@
> +interface WebPushManager {
> +    Promise <ScalarValueString> register(optional any aOptions);
> +    Promise <void> unregister();
> +    Promise <boolean> isRegistered();
> +    Promise <ScalarValueString> hasPermission();

All of these have changed their return types
https://github.com/w3c/push-api/pull/23

::: dom/webpush/moz.build
@@ +2,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +DIRS += ['src']

Keep the structure flat by moving everything in src to this dir.

::: dom/webpush/src/WebPush.js
@@ +70,5 @@
> +
> +      this._callerIsWebPage = true;
> +    } else {
> +      this._callerIsWebPage = false;
> +    }

Could you have a DOM peer review this principal stuff after describing in a comment on the bug (when you ask for review) under what conditions webpush should be disabled.

@@ +88,5 @@
> +
> +  receiveMessage: function(aMessage) {
> +    debug("receiveMessage()");
> +    let resolver = this.takePromiseResolver(aMessage.data.resolverID);
> +    let json = aMessage.data;

Swap these lines and just use takePromiseResolver(json.resolveID);

@@ +142,5 @@
> +  unregister: function() {
> +    debug("unregister()");
> +    let promise = this.createPromise((resolve, reject) => {
> +      let resolverID = this.getPromiseResolverId({resolve:resolve, reject:reject});
> +      this._promptPermission(resolverID, "WebPush:Unregister", reject)

Do we need to prompt for permission for this?

@@ +168,5 @@
> +        case Ci.nsIPermissionManager.ALLOW_ACTION: resolve('granted');
> +                      break;
> +        case Ci.nsIPermissionManager.DENY_ACTION: resolve('denied');
> +          break;
> +        case Ci.nsIPermissionManager.UNKNOWN_ACTION: resolve('needask');

Style nit: Move the resolve to a newline in the above cases.

@@ +170,5 @@
> +        case Ci.nsIPermissionManager.DENY_ACTION: resolve('denied');
> +          break;
> +        case Ci.nsIPermissionManager.UNKNOWN_ACTION: resolve('needask');
> +          break;
> +        default: reject('derp');

derp? :) If none of these cases are true (which is nearing impossible to happen), just resolve with denied.

@@ +181,5 @@
> +    debug('setScope');
> +    this._scope = scope;
> +  },
> +
> +  _promptPermission: function(resolverID, requestString, reject){

s/requestString/message ?

@@ +183,5 @@
> +  },
> +
> +  _promptPermission: function(resolverID, requestString, reject){
> +    if(!this._callerIsWebPage){
> +      reject('Not implemented for Mobile yet!')

s/Mobile/apps/

@@ +211,5 @@
> +        window: this._window,
> +        allow: () => {
> +          this._cpmm.sendAsyncMessage(requestString, {
> +              resolverID: resolverID,
> +              scope: this._scope

Ensure that scope is a non-empty string, otherwise fail.

::: dom/webpush/src/WebPushService.jsm
@@ +41,5 @@
> +// Set debug first so that all debugging actually works.
> +gDebuggingEnabled = prefs.get("debug");
> +
> +const kWEBPUSHDB_DB_NAME = "webpush";
> +const kWEBPUSHDB_DB_VERSION = 2; // Change this if the IndexedDB format changes

Why 2? new db, so v1.

@@ +70,5 @@
> +    aDb.deleteObjectStore(kWEBPUSHDB_STORE_NAME);
> +    let objectStore = aDb.createObjectStore(kWEBPUSHDB_STORE_NAME,
> +                                            { keyPath: "channelID" });
> +
> +    // Each push registration is tied to a specific scope belonging to 

trailing whitespace here and

@@ +72,5 @@
> +                                            { keyPath: "channelID" });
> +
> +    // Each push registration is tied to a specific scope belonging to 
> +    // a ServiceWorkerRegistration.  Only one push registration is allowed
> +    // per ServiceWorkerRegistration, so uniqueness can be enforced.  

here

@@ +345,5 @@
> +
> +        // Only remove push registrations for apps.
> +        if (data.browserOnly) {
> +          return;
> +        }

Since push2 does not work with apps right now, please go over this once and ensure that it works. You may have to listen to a different topic for normal web pages.

@@ +1275,5 @@
> +                }
> +              );
> +          }
> +        },
> +        function(error){

Fat arrow?

::: dom/workers/ServiceWorkerRegistration.cpp
@@ +42,4 @@
>    MOZ_ASSERT(aWindow);
>    MOZ_ASSERT(aWindow->IsInnerWindow());
>  
> +  SetIsDOMBinding();

This is called by DETH's ctor.

@@ +198,5 @@
>  
> +already_AddRefed<WebPushManager>
> +ServiceWorkerRegistration::GetPushRegistrationManager(ErrorResult& aRv)
> +{
> +  if(mPushRegistrationManager == nullptr){

Nit: Style is |if<space>(<condition>)<space> {|

@@ +209,5 @@
> +    GlobalObject global(cx, globalJs);
> +
> +    JS::Rooted<JSObject*> jsImplObj(cx);
> +    nsCOMPtr<nsPIDOMWindow> window =
> +    ConstructJSImplementation(cx, "@mozilla.org/webpush/WebPushManager;1", global, &jsImplObj, aRv);

Nit: Indentation and wrapping to 80 chars.

::: dom/workers/ServiceWorkerRegistration.h
@@ +13,5 @@
> +#include "jsapi.h"
> +
> +#include "mozilla/dom/WebPushManagerBinding.h"
> +#include "mozilla/dom/BindingDeclarations.h"
> +#include "mozilla/dom/BindingUtils.h"

None of these includes are required in the header, they can all be replaced by forward declarations. afaik you only need to fwd declare the binding class.

@@ +15,5 @@
> +#include "mozilla/dom/WebPushManagerBinding.h"
> +#include "mozilla/dom/BindingDeclarations.h"
> +#include "mozilla/dom/BindingUtils.h"
> +
> +#include "nsDebug.h"

Extra?

@@ +78,5 @@
> +  // Something that we can feed into the Func webidl property
> +  static bool
> +  AlwaysFalse(JSContext*, JSObject*){
> +    return false;
> +  }

Please have a more push relevant name for this function. Name the arguments even if they are unused.

Comment 20

[Tyler Smith [:tysmith]]

Attachment: Patch1apiexposure.patch

Comment 21

[Tyler Smith [:tysmith]]

Attachment: Patch2pushevent.patch

Comment 22

[Tyler Smith [:tysmith]]

Sorry for being late.

Comment 23

[Tyler Smith [:tysmith]]

Attachment: Patch1apiexposure.patch

Minor fix

Comment 25

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8489540 [details] [diff] [review]
Patch1apiexposure.patch

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

Please upload interdiffs from this point on along with the changed patch since this patch is pretty big!

0. Please fix both js and c++ to comply with the gecko style guide. I have marked some differences individually, but there are others.
1. Get a browser peer review on stuff in browser/
2. Please add tests for API exposure when proper prefs are enabled.
3. Could you also post a try run?
4. Various package-manifest.in files for desktop, android and b2g require entries. See how older push does it.
5. Ask for r? from bz on ServiceWorkerRegistration.cpp.
6. all.js has a lot of extra changes.
7. Add a note to all the webidl files stating the origin, similar to http://dxr.mozilla.org/mozilla-central/source/dom/webidl/ChildNode.webidl#6.
8. File a followup to implement the service using HTTP/2 and make it dependent on bug 1024730.
9. Please go over the spec algorithms and ensure our implementation rejects with appropriate DOMException instances (not strings!).
10. WebPush.js does not implement getRegistration().
11. Please remove stuff from the service that has no analog in PushRegistrationManager due to spec changes.
12. I have assumed that the majority of the push service code related to timeouts and websockets is unchanged relative to the older push and not reviewed it. Please correct me if that is not the case and I'll review it.

::: browser/components/nsBrowserGlue.js
@@ +215,4 @@
>    },
>  #endif
>  
> +  // nsIObserver implementation

We usually avoid removing trailing whitespace of unrelated code since it interferes with blame. Here and in the rest of this file.
Feel free to file a relevant bug though and fix it there.

@@ +1845,4 @@
>     */
>    _showPrompt: function CPP_showPrompt(aRequest, aMessage, aPermission, aActions,
>                                         aNotificationId, aAnchorId, aOptions) {
> +

Nit: extra line.

@@ +1937,4 @@
>      }
>    },
>  
> +  _promptPush : function(aRequest){

nit: space between ) and {

::: browser/components/preferences/aboutPermissions.xul
@@ +38,4 @@
>                     onselect="AboutPermissions.onSitesListSelect(event);">
>          <richlistitem id="all-sites-item"
>                        class="site"
> +                      value="&sites.allSites;"/>

Nit: unrelated trailing whitespace

::: dom/push/PushService.jsm
@@ +1340,4 @@
>  
>      this._updatePushRecord(record)
>        .then(
> +        () => {

Nit: Changes in this file are not relevant to Push 2, so remove these changes from the patch when landing.

::: dom/webidl/PushRegistration.webidl
@@ +3,5 @@
> +* License, v. 2.0. If a copy of the MPL was not distributed with this file,
> +* You can obtain one at http://mozilla.org/MPL/2.0/.
> +*/
> +
> +[Exposed=Window,

Please hide this behind the dom.push.enabled pref.

@@ +4,5 @@
> +* You can obtain one at http://mozilla.org/MPL/2.0/.
> +*/
> +
> +[Exposed=Window,
> + Constructor(DOMString pushEndpoint, DOMString pushRegistrationId),

Exposing a webidl constructor means client JS can create instances of this. The spec does not mention it, nor does it seem to be required.

@@ +7,5 @@
> +[Exposed=Window,
> + Constructor(DOMString pushEndpoint, DOMString pushRegistrationId),
> + JSImplementation="@mozilla.org/webpush/PushRegistration;1"]
> +interface PushRegistration {
> +    readonly    attribute DOMString pushEndpoint;

Nit: too many spaces between readonly and attribute.

@@ +8,5 @@
> + Constructor(DOMString pushEndpoint, DOMString pushRegistrationId),
> + JSImplementation="@mozilla.org/webpush/PushRegistration;1"]
> +interface PushRegistration {
> +    readonly    attribute DOMString pushEndpoint;
> +    readonly    attribute DOMString pushRegistrationId;

ditto.

::: dom/webidl/PushRegistrationManager.webidl
@@ +8,5 @@
> + JSImplementation="@mozilla.org/webpush/PushRegistrationManager;1",
> + Pref="dom.push.enabled"]
> +interface PushRegistrationManager {
> +
> +    Promise<PushRegistration> register (optional any aOptions);

Nit: no space between method name and (. Here and below.

@@ +9,5 @@
> + Pref="dom.push.enabled"]
> +interface PushRegistrationManager {
> +
> +    Promise<PushRegistration> register (optional any aOptions);
> +    Promise<DOMString>        unregister ();

unregister() returns a Promise<PushRegistration>. I'm perfectly ok with landing this as is with a comment and bug number for the followup that will change this.

@@ +11,5 @@
> +
> +    Promise<PushRegistration> register (optional any aOptions);
> +    Promise<DOMString>        unregister ();
> +    Promise<PushRegistration> getRegistration ();
> +    Promise <ScalarValueString> hasPermission();

removed.

::: dom/webpush/PushRegistration.js
@@ +14,5 @@
> +
> +PushRegistration.prototype = {
> +
> +  init: function(aWindow) {
> +    this.window = aWindow;

Seems unused.

@@ +24,5 @@
> +  },
> +
> +  classID:          Components.ID(PUSHREGISTRATION_CID),
> +  contractID:       "@mozilla.org/webpush/PushRegistration;1",
> +  QueryInterface: XPCOMUtils.generateQI([Components.interfaces.nsISupportsWeakReference])

nsISupportsWeakReference isn't required. only Ci.nsISupports.

::: dom/webpush/WebPush.js
@@ +21,5 @@
> +Cu.import("resource://gre/modules/DOMRequestHelper.jsm");
> +Cu.import("resource://gre/modules/AppsUtils.jsm");
> +Cu.import("resource://gre/modules/PrivateBrowsingUtils.jsm");
> +
> +const WEBPUSH_CID = Components.ID("{cde1d019-beef-4044-b141-65fb4fb7a245}");

(offtopic) you got a word in the UUID :)

@@ +50,5 @@
> +    // pref will require a reload of the app/page, which seems acceptable.
> +    gDebuggingEnabled = Services.prefs.getBoolPref("dom.push.debug");
> +    debug("init()");
> +
> +    this._window = aWindow;

DOMRequestIpcHelper will set this below.

@@ +54,5 @@
> +    this._window = aWindow;
> +
> +    if (PrivateBrowsingUtils.isWindowPrivate(aWindow)) {
> +      debug("WebPush is disabled in private browsing mode.");
> +      return null;

may not return null. This check should be moved to register/unregister. register() should pretend permission was denied while unregister() should pretend NotFoundError when in private browsing mode.

@@ +60,5 @@
> +
> +    let appsService = Cc["@mozilla.org/AppsService;1"]
> +                        .getService(Ci.nsIAppsService);
> +
> +    let principal = this._principal = aWindow.document.nodePrincipal;

The new correct way to obtain the principal is getWebIDLCallerPrincipal(). https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Determining_the_principal_of_the_caller_that_invoked_the_WebIDL_API

@@ +100,5 @@
> +
> +    switch (aMessage.name) {
> +      case "WebPushService:Register:OK":
> +        var pushRegistration = new
> +          this._window.PushRegistration(json.pushEndpoint, "what am I?");

nit: move the new to the next line.

pushRegistrationId is wrong.

@@ +107,5 @@
> +      case "WebPushService:Register:KO":
> +        resolver.reject(json.error);
> +        break;
> +      case "WebPushService:Unregister:OK":
> +        resolver.resolve("Unregister was successful.")

UA dependent literal strings are not a good idea. Resolve with the pushEndpoint for now, and in the followup to be spec compliant, fix to use PushRegistration.

@@ +112,5 @@
> +        break;
> +      case "WebPushService:Unregister:KO":
> +        resolver.reject(json.error);
> +        break;
> +      case "WebPushService:IsRegistered:OK":

Message obsoleted by spec changes.

@@ +115,5 @@
> +        break;
> +      case "WebPushService:IsRegistered:OK":
> +        resolver.resolve(json.isRegistered);
> +        break;
> +      case "WebPushService:IsRegistered:KO":

Message obsoleted by spec changes.

@@ +128,5 @@
> +  register: function(aOptions) {
> +    debug("register()");
> +    let promise = this.createPromise((resolve, reject) => {
> +
> +      if (!Services.prefs.getBoolPref("dom.push.enabled")) {

The PushRegistrationManager interface will never be exposed if dom.push.enabled is false. This check is redundant.

@@ +148,5 @@
> +    let promise = this.createPromise((resolve, reject) => {
> +      let resolverID = this.getPromiseResolverId({resolve:resolve, reject:reject});
> +      this._cpmm.sendAsyncMessage("WebPush:Unregister", {
> +              resolverID: resolverID,
> +              scope: this._scope

Please check scope is valid (non-empty string).

@@ +155,5 @@
> +
> +    return promise;
> +  },
> +
> +  isRegistered: function() {

obsolete.

@@ +166,5 @@
> +
> +    return promise;
> +  },
> +
> +  hasPermission: function() {

obsolete.

@@ +194,5 @@
> +  },
> +
> +  _promptPermission: function(resolverID, message, reject){
> +    if(!this._callerIsWebPage){
> +      reject('Not implemented for apps yet!')

reject with a valid DOMException. Ideally PushRegistrationManager itself should not be exposed in app contexts. You can use Func= with checks in C++ to enforce these requirements.

@@ +196,5 @@
> +  _promptPermission: function(resolverID, message, reject){
> +    if(!this._callerIsWebPage){
> +      reject('Not implemented for apps yet!')
> +    }
> +    else{

use early return above and move everything out of the else block.

@@ +210,5 @@
> +      if(!types)
> +        reject(types);
> +      let promptType = {
> +        type: 'push',
> +        access: "unused",

Nit: Decide between single and double quotes :)

@@ +221,5 @@
> +        principal: this._principal,
> +        window: this._window,
> +        allow: () => {
> +          if(!this._scope){
> +            reject("This shouldn't happen.");

reject with a DOMException that does not reveal what went wrong internally, since this is clearly something that should never ever happen. AbortError is acceptable. debug() with a more useful error message.

@@ +231,5 @@
> +              scope: this._scope
> +            });
> +        },
> +        cancel: () => {
> +          reject('SecurityError.');

reject with a DOMException

::: dom/webpush/WebPushService.jsm
@@ +1432,5 @@
> +          else{
> +              aMessageManager.sendAsyncMessage("WebPushService:Unregister:OK", {
> +                resolverID: aSWRegistrationRecord.resolverID
> +              });
> +          }

unregister() is not spec compliant.

@@ +1446,5 @@
> +  },
> +
> +  _getRegistration: function(aSWRegistrationRecord) {
> +    debug("_getRegistration()");
> +    let promise = new Promise((resolve, reject) => {

Is this using the DOM Promise or Promise.jsm promise? Promise.jsm does not implement promises in this way. I'm surprised this hasn't failed in testing yet.

::: dom/workers/ServiceWorkerRegistration.cpp
@@ +299,5 @@
>  
> +already_AddRefed<PushRegistrationManager>
> +ServiceWorkerRegistration::GetPushRegistrationManager(ErrorResult& aRv)
> +{
> +  if (mPushRegistrationManager == nullptr) {

Nit: just |if (!mPushRegistrationManager) {|

@@ +310,5 @@
> +    GlobalObject global(cx, globalJs);
> +
> +    JS::Rooted<JSObject*> jsImplObj(cx);
> +    nsCOMPtr<nsPIDOMWindow> window =
> +    ConstructJSImplementation(cx, "@mozilla.org/webpush/PushRegistrationManager;1",

Nit: Indent 2 spaces.

@@ +311,5 @@
> +
> +    JS::Rooted<JSObject*> jsImplObj(cx);
> +    nsCOMPtr<nsPIDOMWindow> window =
> +    ConstructJSImplementation(cx, "@mozilla.org/webpush/PushRegistrationManager;1",
> +      global, &jsImplObj, aRv);

Nit: These arguments should align with the opening ( in the previous line.

::: dom/workers/ServiceWorkerRegistration.h
@@ +9,5 @@
>  
>  #include "mozilla/DOMEventTargetHelper.h"
>  #include "mozilla/dom/ServiceWorkerCommon.h"
> +#include "mozilla/dom/ScriptSettings.h"
> +#include "jsapi.h"

These includes aren't required in the header as far as i can tell.

@@ +67,3 @@
>  
>    // DOMEventTargethelper
>    virtual void DisconnectFromOwner() MOZ_OVERRIDE;

Nit: Please add a newline here.

@@ +72,5 @@
> +
> +  // Something that we can feed into the Func webidl property to ensure that
> +  // SetScope is never exposed to the user.
> +  static bool
> +  WebPushMethodHider(JSContext* unusedContext, JSObject* unusedObject){

Nit: Space between ) and {.

Comment 26

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8489533 [details] [diff] [review]
Patch2pushevent.patch

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

Please modify to stick to gecko coding style.
try run + test_interfaces changes as required.
otherwise looks good with the following changes.

::: dom/interfaces/base/nsIServiceWorkerManager.idl
@@ +17,4 @@
>    [noscript] void UnregisterFailed();
>  };
>  
> +[scriptable, uuid(79ad5b57-d65d-46d3-b5e9-32d27e16052d)]

change the UUID since a new method was added.

::: dom/webidl/PushEvent.webidl
@@ +1,4 @@
> +/* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/.

Notice of where this idl came from.

@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/.
> + */
> +
> +[Constructor(DOMString type, optional PushEventInit eventInitDict), Exposed=ServiceWorker]

Please put the Exposed= on a new line.

::: dom/webpush/PushRegistration.js
@@ +8,4 @@
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  
>  const PUSHREGISTRATION_CID = Components.ID("{e0c9919f-1482-2b5f-b20c-fd9d4f0fe7da}");
> +const PUSHMESSAGE_CID = Components.ID("{41a7064a-391b-11e4-aa5b-53391d5d46b0}");

None of the changes in this file are needed.

::: dom/webpush/WebPush.js
@@ +136,4 @@
>          return;
>        }
>  
> +      let resolverID = this.getPromiseResolverId({resolve:resolve, reject:reject});

these changes aren't needed either.

::: dom/webpush/WebPushService.jsm
@@ +26,5 @@
>  Cu.import("resource://gre/modules/Promise.jsm");
>  Cu.importGlobalProperties(["indexedDB"]);
>  
> +var swm = Components.classes["@mozilla.org/serviceworkers/manager;1"]
> +                    .getService(Ci.nsIServiceWorkerManager);

You can acquire this reference just where you need it rather than keep it around globally. Also, we use let, not var.

@@ +1129,5 @@
> +    let makeURI = function(aURL, aOriginCharset, aBaseURI) {
> +      var ioService = Components.classes["@mozilla.org/network/io-service;1"]
> +                      .getService(Components.interfaces.nsIIOService);
> +      return ioService.newURI(aURL, aOriginCharset, aBaseURI);
> +    }

unused.

@@ +1145,5 @@
>          aPushRecord.version = aLatestVersion;
> +
> +        let ioService = Components.classes["@mozilla.org/network/io-service;1"]
> +                      .getService(Components.interfaces.nsIIOService);
> +        let uri = ioService.newURI(aPushRecord.scope, null, null);

Just Services.io.newURI can replace this :)

@@ +1151,5 @@
> +        // If permission has been revoked, trash the message.
> +        if(Services.perms.testExactPermission(uri, "push") != Ci.nsIPermissionManager.ALLOW_ACTION)
> +          return;
> +
> +        swm.sendPushEvent(aPushRecord.scope, JSON.stringify({data: "data placeholder"}));

use empty string for now as the data.

::: dom/workers/ServiceWorkerManager.cpp
@@ +877,5 @@
>  };
>  
> +class SendPushEventRunnable MOZ_FINAL : public WorkerRunnable
> +{
> +  nsMainThreadPtrHandle<ServiceWorkerRegistrationInfo> mRegistration;

unused.

@@ +878,5 @@
>  
> +class SendPushEventRunnable MOZ_FINAL : public WorkerRunnable
> +{
> +  nsMainThreadPtrHandle<ServiceWorkerRegistrationInfo> mRegistration;
> +  PushEventInit mPei;

nsString (see below).

@@ +884,5 @@
> +public:
> +  SendPushEventRunnable(
> +    WorkerPrivate* aWorkerPrivate,
> +    const nsMainThreadPtrHandle<ServiceWorkerRegistrationInfo>& aRegistration,
> +    const PushEventInit& aPei)

This will be replaced by a string anyway, but this is dangerous since you are keeping a reference to a stack allocated PushEventInit that is on a separate thread and could go away before this runnable runs.

@@ +921,5 @@
> +NS_IMETHODIMP
> +ServiceWorkerManager::SendPushEvent(const nsACString& scope, const nsAString& data){
> +  AssertIsOnMainThread();
> +
> +  nsCString aScope;

conventionally the arguments are a-prefixed and these are without :)

@@ +933,5 @@
> +  nsRefPtr<ServiceWorkerDomainInfo> aDomainInfo = swm->GetDomainInfo(aScope);
> +
> +  if(!aDomainInfo){
> +    // There is no registered ServiceWorker for this domain
> +    return NS_ERROR_DOM_SECURITY_ERR;

this doesn't seem like a security error. Just a NS_ERROR_NOT_AVAILABLE or NS_ERROR_FAILURE is fine.

@@ +940,5 @@
> +  nsRefPtr<ServiceWorkerRegistrationInfo> aServiceWorkerRegistrationInfo = aDomainInfo->GetRegistration(aScope);
> +
> +  if(!aServiceWorkerRegistrationInfo){
> +    // There is no registered ServiceWorker for this particular scope.
> +    return NS_ERROR_DOM_SECURITY_ERR;

ditto.

@@ +962,5 @@
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  PushEventInit pei;

Just pass the nsString to the runnable and create this in the runnable on the worker thread.

Comment 27

[Tyler Smith [:tysmith]]

Attachment: Patch2pushevent.patch

Comment 28

[Tyler Smith [:tysmith]]

Attachment: patch1interdiff.patch

Comment 29

[Tyler Smith [:tysmith]]

Attachment: bz.patch

Could I get a review on these changes to ServiceWorkerRegistrations?  Patch 1 is pretty large so I put all the things relevant to ServiceWorkerRegistrations in this patch, and any changes you suggest I'll put into the interdiff.

Comment 30

[Tyler Smith [:tysmith]]

Attachment: gsharp.patch

Same situation as above.  I made some changes to browser to handle permissions for the new Push API.  Can you look over them?

Comment 31

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8491834 [details] [diff] [review]
bz.patch

>+    mPushRegistrationManager->SetScope(mScope, aRv);
>+    if (aRv.Failed()) {

Then you need to null out mPushRegistrationManager, right?  Otherwise the next time you'll return the one that SetScope failed on.

>+  nsRefPtr<PushRegistrationManager> ret = mPushRegistrationManager;
>+  return ret.forget();

Why not just have this method return PushRegistrationManager* and return mPushRegistrationManager?

>+  // Something that we can feed into the Func webidl property to ensure that
>+  // SetScope is never exposed to the user.

Chromeonly is not good enough?  I guess this is fine.

r=me with the above fixed.

Comment 32

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8491837 [details] [diff] [review]
gsharp.patch

Could you move this patch to a Firefox::General bug? Much easier to track separately.

I imagine this completely replaces bug 885093? There is some discussion there that may be relevant. The primary concern in that bug was making it clear what this permission actually grants, we may need some UX input on that. A concise summary of how this stuff works at a high level would be useful in that new bug.

Comment 33

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8491796 [details] [diff] [review]
patch1interdiff.patch

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

Almost there. I'd like to see a full patch (not just an interdiff) once more before we land some of this.

::: b2g/installer/package-manifest.in
@@ +557,4 @@
>  @BINPATH@/components/Push.js
>  @BINPATH@/components/Push.manifest
>  @BINPATH@/components/PushServiceLauncher.js
> +@BINPATH@/components/WebPush.js

Add a comment here stating these are Push 2 files since the proximity may be confusing.

::: browser/components/nsBrowserGlue.js
@@ +215,4 @@
>    },
>  #endif
>  
> +  // nsIObserver implementation 

Your editor has introduced trailing whitespace in this file.

::: browser/components/preferences/aboutPermissions.xul
@@ +38,4 @@
>                     onselect="AboutPermissions.onSitesListSelect(event);">
>          <richlistitem id="all-sites-item"
>                        class="site"
> +                      value="&sites.allSites;"/>                

trailing ws

@@ +212,4 @@
>          </vbox>
>        </hbox>
>  
> +      <!-- IndexedDB Storage -->  

same here

::: browser/locales/en-US/chrome/browser/browser.properties
@@ +401,4 @@
>  tabview.title=%S - Group Your Tabs
>  # LOCALIZATION NOTE (tabview.moveToUnnamedGroup.label): Semicolon-separated list of plural forms.
>  # See: http://developer.mozilla.org/en/docs/Localization_and_Plurals
> +# #1 is the page title of the first tab in the unnamed group, 

trailing ws

::: dom/webidl/PushRegistrationManager.webidl
@@ +14,5 @@
>  
> +    Promise<PushRegistration>     register (optional any aOptions);
> +    Promise<PushRegistration>     unregister ();
> +    Promise<PushRegistration>     getRegistration ();
> +    Promise<PushPermissionStatus> hasPermission ();

no space between function name and ()

@@ +21,3 @@
>      // but we don't want it exposed to client JS.  WebPushMethodHider
>      // always returns false.
>      [Func="ServiceWorkerRegistration::WebPushMethodHider"] void setScope(DOMString scope);

newline after annotation please.

::: dom/webpush/WebPush.js
@@ +54,4 @@
>      let appsService = Cc["@mozilla.org/AppsService;1"]
>                          .getService(Ci.nsIAppsService);
>  
> +    // This the canonical reason to do this, but it's not working for reasons that

s/reason/way. And if it's not working we don't want to land it :) use the older style.

@@ +103,3 @@
>      switch (aMessage.name) {
>        case "WebPushService:Register:OK":
> +        pushRegistration = this._window.PushRegistration._create(

where is _create defined? Can't find it in this patch or the older one.

@@ +111,5 @@
>          break;
>        case "WebPushService:Unregister:OK":
> +        pushRegistration = this._window.PushRegistration._create(
> +          this._window, reg);
> +        resolver.resolve(pushRegistration);

You can collapse the Register:OK, Unregister:OK and GetRegistration:OK cases since they are the same code.

@@ +117,5 @@
>        case "WebPushService:Unregister:KO":
>          resolver.reject(json.error);
>          break;
> +      case "WebPushService:GetRegistration:OK":
> +        pushRegistration = this._window.PushRegistration._create(

Move initialization of pushRegistration outside the switch, it's the same everywhere.

@@ +145,5 @@
>    unregister: function() {
>      debug("unregister()");
>      let promise = this.createPromise((resolve, reject) => {
> +      if(!this._scope) {
> +        reject();

reject with a valid error.

@@ +203,4 @@
>  
> +    if (PrivateBrowsingUtils.isWindowPrivate(this._window)) {
> +      debug("WebPush is disabled in private browsing mode.");
> +      reject("temp");

valid error.

@@ +210,5 @@
> +    let permPrompt = Cc["@mozilla.org/content-permission/prompt;1"]
> +                       .createInstance(Ci.nsIContentPermissionPrompt);
> +
> +    if (!permPrompt) {
> +      reject("SecurityError");

|reject(new this._window.DOMError("PermissionDeniedError"))|

@@ +216,5 @@
>      }
> +
> +    let types = Cc["@mozilla.org/array;1"].createInstance(Ci.nsIMutableArray);
> +    if(!types)
> +      reject(types);

don't expose  internals. reject everything with PermissionDeniedError here.

@@ +229,5 @@
> +      types: types,
> +      principal: this._principal,
> +      window: this._window,
> +      allow: () => {
> +        if(!this._scope){

shouldn't the scope have been set a long time ago? So can you check this right at the beginning of _promptPermission.

@@ +242,5 @@
> +          });
> +      },
> +      cancel: () => {
> +        // temporary
> +        let = "SecurityError";

syntax error?
Also, use DOMError. you can create one by using the |new window.DOMError("SecurityError")| form.

Comment 34

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Notifications on ServiceWorkers are not a hard blocker, but we definitely need them to allow devs to do anything useful.

Comment 35

[Doug Turner (:dougt)]

wip here: https://github.com/dougt/gecko-dev/tree/dougt_push_api_hacking

Comment 36

[Doug Turner (:dougt)]

Attachment: test.html

Comment 37

[Doug Turner (:dougt)]

Attachment: serviceworker.js

Comment 38

[Doug Turner (:dougt)]

curl -X PUT -v <endpoint>  # should basically work for now.

Comment 39

[Jonathan McKay]

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #34)
> Notifications on ServiceWorkers are not a hard blocker, but we definitely
> need them to allow devs to do anything useful.

As far as I can tell, push makes the effort needed to implement service workers justified due to its reengagement potential.

Comment 40

[Doug Turner (:dougt)]

Attachment: Push Notifications - Firefox front end changes for preferences, and permission notification

Comment 41

[Doug Turner (:dougt)]

Attachment: Push Notifications - WebIDL changes

Comment 42

[Doug Turner (:dougt)]

Attachment: Push Notifications - Push implementation changes

Comment 43

[Doug Turner (:dougt)]

Attachment: Push Notifications - Tests

Comment 44

[Doug Turner (:dougt)]

Attachment: Push Notifications - ServiceWorker changes, push event implementation,

Comment 45

[Doug Turner (:dougt)]

Comment on attachment 8582852 [details] [diff] [review]
Push Notifications - Firefox front end changes for preferences, and permission notification

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

The Push- png's are a copy of Geolocation.  I'm blocking on madhava for ux resourcing.

::: browser/app/profile/firefox.js
@@ +1880,5 @@
>  // Enable the readinglist engine by default.
>  pref("readinglist.scheduler.enabled", true);
>  pref("readinglist.server", "https://readinglist.services.mozilla.com/v1");
> +
> +pref("dom.serviceWorkers.enabled", true);

:)

Comment 46

[Martin Thomson [:mt:]]

(In reply to Doug Turner (:dougt) from comment #45)
>
> > +pref("dom.serviceWorkers.enabled", true);
> 
> :)

a=dougt

I've all the necessary pieces for the crypto piece ready, modulo any changes I have to make to deal with changes to the format.  I'm aware of the need for two small-ish changes.

https://github.com/martinthomson/webpush-client

Comment 47

[Doug Turner (:dougt)]

excellent mr. thomson. I'm expecting a pr from Dragana.

Comment 48

[Matthew N. [:MattN]]

Comment on attachment 8582852 [details] [diff] [review]
Push Notifications - Firefox front end changes for preferences, and permission notification

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

Awesome! Looking forward to this landing.

r=me assuming the strings and icons get UX approval.

::: browser/app/profile/firefox.js
@@ -1880,5 @@
>  #endif
>  
> -// Enable ReadingList browser UI by default.
> -pref("browser.readinglist.enabled", true);
> -pref("browser.readinglist.sidebarEverOpened", false);

I'm guessing you didn't mean to delete these.

@@ +1883,5 @@
> +
> +pref("dom.serviceWorkers.enabled", true);
> +
> +pref("dom.push.enabled", true);
> +pref("dom.push.debug", true);

Do we want debug true by default?

I think most of the prefs should be defined in all.js and only have some defaults overridden in firefox.js or you may end up breaking other applications that use Gecko (e.g. FxOS) if these prefs aren't defined there. The prefs in all.js should probably be moved out of this front-end patch so the reviewer for the backend can review the values.

::: browser/components/nsBrowserGlue.js
@@ +2391,5 @@
> +                    learnMoreURL: Services.urlFormatter.formatURLPref("browser.push.warning.infoURL"),
> +                  };
> +
> +    this._showPrompt(aRequest, message, "push", actions, "push",
> +                     "geo-notification-icon", options);

This should be "push-notification-icon"

::: browser/themes/linux/browser.css
@@ +1288,5 @@
>  #geo-notification-icon {
>    list-style-image: url(chrome://browser/skin/Geolocation-16.png);
>  }
>  
> +.push-notification-icon,

IIRC the class is only used for the old notification bars (that geolocation once used) so I think you don't need this line (but please confirm the doorhanger anchor icons still work).

@@ +1290,5 @@
>  }
>  
> +.push-notification-icon,
> +#push-notification-icon {
> +  list-style-image: url(chrome://browser/skin/Push-16.png);

Nowadays this stuff could probably be moved to themes/shared/ since it's the same across platforms but that's for a separate bug. I just filed bug 1147281 for this.

::: browser/themes/linux/jar.mn
@@ +33,5 @@
>    skin/classic/browser/fullscreen-darknoise.png
>    skin/classic/browser/Geolocation-16.png
>    skin/classic/browser/Geolocation-64.png
> +  skin/classic/browser/Push-16.png
> +  skin/classic/browser/Push-64.png

Note that if the UI designer ends up providing a single cross-platform icon you should put it in browser/themes/shared/ and reference it like others in this file with something like: (../shared/Push-16.png)

::: browser/themes/osx/browser.css
@@ +3726,5 @@
>      list-style-image: url(chrome://browser/skin/[e-mail]);
>    }
>  }
>  
> +.push-notification-icon,

Same as my comment on Linux

@@ +3731,5 @@
> +#push-notification-icon {
> +  list-style-image: url(chrome://browser/skin/Push-16.png);
> +}
> +@media (min-resolution: 2dppx) {
> +  .push-notification-icon,

here too

::: browser/themes/windows/browser.css
@@ +2299,5 @@
>  #geo-notification-icon {
>    list-style-image: url(chrome://browser/skin/Geolocation-16.png);
>  }
>  
> +.push-notification-icon,

Same as my comment on Linux

Comment 49

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8582855 [details] [diff] [review]
Push Notifications - WebIDL changes

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

r- because I'm concerned how backwards compat with old push is maintained when PushManager.webidl had its properties changed?

::: dom/interfaces/base/nsIServiceWorkerManager.idl
@@ +18,5 @@
>    [noscript] void UnregisterSucceeded(in bool aState);
>    [noscript] void UnregisterFailed();
>  };
>  
> +[scriptable, builtinclass, uuid(F7A76F97-B866-466F-9EF4-A6272BF765C7)]

Nit: sorry to be a prick, but could you use a lowercase UUID.

::: dom/webidl/PushMessageData.webidl
@@ +6,5 @@
> + * The origin of this IDL file is
> + * https://w3c.github.io/push-api/
> + */
> +
> +typedef object Any;

Please remove (see below)

@@ +7,5 @@
> + * https://w3c.github.io/push-api/
> + */
> +
> +typedef object Any;
> +// todo get this removed : NoInterfaceObject,

I guess there is approval https://github.com/w3c/push-api/issues/123
so you could just get rid of this comment.

@@ +13,5 @@
> +interface PushMessageData
> +{
> +    ArrayBuffer arrayBuffer();
> +    Blob        blob();
> +    Any         json();

The Push spec seems to be confused about this. The type name is "Any" but the webidl keyword is "any" which is a sort of 'primitive type' and can just be used here.
Although, now that I think about it, maybe this should be replaced with JSON. I filed https://github.com/w3c/push-api/issues/133

::: dom/webidl/PushSubscription.webidl
@@ +1,5 @@
> +/* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> +* License, v. 2.0. If a copy of the MPL was not distributed with this file,
> +* You can obtain one at http://mozilla.org/MPL/2.0/.
> +*/

Notice for this and other files with appropriate links.
"The origin for this IDL file is:
https://w3c.github.io/push-api/#idl-def-PushSubscription"

::: dom/webidl/ServiceWorkerRegistration.webidl
@@ +23,5 @@
>    // event
>    attribute EventHandler onupdatefound;
> +
> +  [Throws]
> +  readonly attribute PushManager pushManager;

Please use the "partial..." syntax exactly as in the spec at the end of this file.

Comment 50

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8582858 [details] [diff] [review]
Push Notifications - ServiceWorker changes, push event implementation,

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

::: dom/workers/ServiceWorkerEvents.cpp
@@ +377,5 @@
>  
> +
> +PushMessageData::PushMessageData(const nsAString& aData)
> +{
> +  mData = aData;

Up to before '{' please.

@@ +391,5 @@
> +void
> +PushMessageData::Json(JSContext* cx, JS::MutableHandle<JSObject*> aRetval)
> +{
> +  //todo
> +   NS_ABORT();

Copy code from FetchBody<Derived>::ContinueConsumeBody when you need it in the followup. :)

::: dom/workers/ServiceWorkerEvents.h
@@ +222,5 @@
>  };
>  
> +
> +class PushMessageData final : public nsISupports,
> +                                  public nsWrapperCache

Nit: indentation.

@@ +244,5 @@
> +  void Text(nsAString& aData);
> +  void ArrayBuffer(JSContext* cx, JS::MutableHandle<JSObject*> aRetval);
> +  mozilla::dom::File* Blob();
> +
> +  PushMessageData(const nsAString& aData);

prefix explicit otherwise build failures with our clang static checks.

@@ +255,5 @@
> +{
> +  nsString mData;
> +  nsMainThreadPtrHandle<ServiceWorker> mServiceWorker;
> +
> +

Nit: extra newline

::: dom/workers/ServiceWorkerManager.cpp
@@ +1463,5 @@
> +  {
> +    AssertIsOnMainThread();
> +    MOZ_ASSERT(aWorkerPrivate);
> +    mData = aData;
> +    mServiceWorker = aServiceWorker;

Could you bump these two initializations to before the '{'

@@ +1467,5 @@
> +    mServiceWorker = aServiceWorker;
> +  }
> +
> +  bool
> +  WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate)

Nit: override at the end.

@@ +1471,5 @@
> +  WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate)
> +  {
> +    MOZ_ASSERT(aCx);
> +    MOZ_ASSERT(aWorkerPrivate);
> +    MOZ_ASSERT(aWorkerPrivate->IsServiceWorker());

This assertion should be moved to the constructor.

@@ +1476,5 @@
> +    GlobalObject globalObj(aCx, aWorkerPrivate->GlobalScope()->GetWrapper());
> +
> +    PushEventInit pei;
> +
> +    nsString optData(mData);

unused.

@@ +1481,5 @@
> +    pei.mData.Construct(mData);
> +    pei.mBubbles = false;
> +    pei.mCancelable = true;
> +
> +    ErrorResult rv;

Nit: ErrorResult result. It is too easy to confuse with nsresult.

@@ +1509,5 @@
> +      : WorkerRunnable(aWorkerPrivate, WorkerThreadModifyBusyCount)
> +  {
> +    AssertIsOnMainThread();
> +    MOZ_ASSERT(aWorkerPrivate);
> +    mServiceWorker = aServiceWorker;

Same as above.

@@ +1513,5 @@
> +    mServiceWorker = aServiceWorker;
> +  }
> +
> +  bool
> +  WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate)

override.

@@ +1517,5 @@
> +  WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate)
> +  {
> +    MOZ_ASSERT(aCx);
> +    MOZ_ASSERT(aWorkerPrivate);
> +    MOZ_ASSERT(aWorkerPrivate->IsServiceWorker());

Same as above.

@@ +1525,5 @@
> +    ExtendableEventInit init;
> +    init.mBubbles = false;
> +    init.mCancelable = true;
> +    nsRefPtr<ExtendableEvent> event =
> +      ExtendableEvent::Constructor(target, NS_LITERAL_STRING("pushsubscriptionchange"), init);

Doesn't need to be an ExtendableEvent according to the spec. Just a simple event, so target->DispatchTrustedEvent(NS_LITERAL_STRING("pushsubscriptionchange")) will do it.

@@ +1548,5 @@
> +
> +  nsRefPtr<SendPushEventRunnable> r =
> +    new SendPushEventRunnable(serviceWorker->GetWorkerPrivate(), aData, serviceWorkerHandle);
> +
> +  AutoJSAPI api;

Nit: always jsapi.

@@ +1570,5 @@
> +
> +  nsRefPtr<SendPushSubscriptionChangeEventRunnable> r =
> +    new SendPushSubscriptionChangeEventRunnable(serviceWorker->GetWorkerPrivate(), serviceWorkerHandle);
> +
> +  AutoJSAPI api;

jsapi.

::: dom/workers/ServiceWorkerManager.h
@@ +347,5 @@
>      return reg.forget();
>    }
>  
> +  already_AddRefed<ServiceWorkerRegistrationInfo>
> +  GetRegistration(const nsACString& aScope) const

There is already one above. If this is just about nsCString/nsACString, please change the argument of the one above

@@ +425,5 @@
>                             WhichServiceWorker aWhichWorker,
>                             nsISupports** aServiceWorker);
>  
> +  already_AddRefed<ServiceWorker>
> +  GetOrCreateServiceWorkerFromScope(const nsACString& aScope);

this never gets, always creates. for consistency please call it CreateServiceWorkerForScope

::: dom/workers/ServiceWorkerRegistration.cpp
@@ +26,5 @@
>  namespace mozilla {
>  namespace dom {
>  
>  NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED(ServiceWorkerRegistration)
>  NS_INTERFACE_MAP_END_INHERITING(DOMEventTargetHelper)

Around this point, add mPushManager to the cycle collection list.

@@ +294,5 @@
>  }
>  
> +already_AddRefed<PushManager>
> +ServiceWorkerRegistration::GetPushManager(ErrorResult& aRv)
> +{

Call |AssertOnMainThread();| here because of PushManager issue below.

@@ +304,5 @@
> +      return nullptr;
> +    }
> +
> +    AutoJSAPI jsapi;
> +    jsapi.Init(globalObject);

This can fail.

if (NS_WARN_IF(!jsapi.Init(globalObject)) {
  throw and return.
}

@@ +310,5 @@
> +
> +    JS::RootedObject globalJs(cx, globalObject->GetGlobalJSObject());
> +    GlobalObject global(cx, globalJs);
> +
> +    printf("asdfasdfasdf -- JS component about to be created.\n");

Remove this and others.

@@ +315,5 @@
> +
> +
> +    JS::Rooted<JSObject*> jsImplObj(cx);
> +    nsCOMPtr<nsIGlobalObject> unused = ConstructJSImplementation(cx, "@mozilla.org/push/PushManager;1",
> +                              global, &jsImplObj, aRv);

This is going to fail miserably on workers when we expose ServiceWorkerRegistration on workers (Bug 1131327). Could you file a follow-up and add a comment with the bug number here?

The easy solution is that for workers we create a C++ PushManagerProxy that routes calls to their equivalents on the main thread (see DataStore comparison in dom/workers/DataStore.{h,cpp} and dom/datastore)

Comment 51

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

I just(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #49)
> Comment on attachment 8582855 [details] [diff] [review]
> Push Notifications - WebIDL changes
> 
> Review of attachment 8582855 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- because I'm concerned how backwards compat with old push is maintained
> when PushManager.webidl had its properties changed?

Ok, I just read disapproval on bug 1040219. I change the r- to a dropped review.

Comment 52

[Doug Turner (:dougt)]

Thanks Matt for the review:

> IIRC the class is only used for the old notification bars (that geolocation once used) so I think you don't need this line (but please confirm the doorhanger anchor icons still work).

I will file a follow up here to clean up all of the *notification-icon classes.


Also, I am going to turn off push by default so that we can land it 'under a pref'.  We can then clean up the FE (like the icons) later.  Make sense?

Comment 53

[Doug Turner (:dougt)]

(follow up bug for the notification class style change: https://bugzilla.mozilla.org/show_bug.cgi?id=1147981_

Comment 54

[Doug Turner (:dougt)]

nsm,

> This assertion should be moved to the constructor.

Not sure why that would be?  I am ensuring that the incoming arguments are what I expect.  Shouldn't we be doing this in other place?

Comment 55

[Matthew N. [:MattN]]

(In reply to Doug Turner (:dougt) from comment #52)
> Thanks Matt for the review:
> 
> > IIRC the class is only used for the old notification bars (that geolocation once used) so I think you don't need this line (but please confirm the doorhanger anchor icons still work).
> 
> I will file a follow up here to clean up all of the *notification-icon
> classes.

OK, I would prefer that you don't add to the confusion though (not sure if you were planning to).

> Also, I am going to turn off push by default so that we can land it 'under a
> pref'.  We can then clean up the FE (like the icons) later.  Make sense?

I think that's fine as long as you consider string freeze.

Comment 56

[Doug Turner (:dougt)]

Attachment: Push Notifications - Firefox front end changes for preferences, and permission notification

Comment 57

[Doug Turner (:dougt)]

Attachment: Push Notifications - WebIDL changes

Comment 58

[Doug Turner (:dougt)]

Attachment: Push Notifications - Push implementation changes

Comment 59

[Doug Turner (:dougt)]

Attachment: Push Notifications - Tests

Comment 60

[Doug Turner (:dougt)]

Attachment: Push Notifications - ServiceWorker changes, push event implementation,

Comment 61

[Matthew N. [:MattN]]

Comment on attachment 8584176 [details] [diff] [review]
Push Notifications - Firefox front end changes for preferences, and permission notification

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

r=me if you move most/all of the prefs from firefox.js to all.js.

(Quoting Matthew N. [:MattN] from comment #48)
> @@ +1883,5 @@
> > +
> > +pref("dom.serviceWorkers.enabled", true);
> > +
> > +pref("dom.push.enabled", true);
> > +pref("dom.push.debug", true);
> 
> I think most of the prefs should be defined in all.js and only have some
> defaults overridden in firefox.js or you may end up breaking other
> applications that use Gecko (e.g. FxOS) if these prefs aren't defined there.
> The prefs in all.js should probably be moved out of this front-end patch so
> the reviewer for the backend can review the values.

It doesn't seem like the above comment was addressed.

(In reply to Doug Turner (:dougt) from comment #52)
> We can then clean up the FE (like the icons) later.

If you delete the change to browser.xul (ideally with the related CSS) then we will fallback to a default notification icon that you can use in the meantime instead of landing incorrect temporary images.

::: browser/app/profile/firefox.js
@@ +1879,5 @@
>  
>  // Enable ReadingList browser UI by default.
>  pref("browser.readinglist.enabled", true);
>  pref("browser.readinglist.sidebarEverOpened", false);
> +

Nit: remove this newline

::: browser/themes/linux/browser.css
@@ +1288,5 @@
>  #geo-notification-icon {
>    list-style-image: url(chrome://browser/skin/Geolocation-16.png);
>  }
>  
> +.push-notification-icon,

> IIRC the class is only used for the old notification bars (that geolocation
> once used) so I think you don't need this line (but please confirm the
> doorhanger anchor icons still work).

See comment 55. Most of the existing rulesets don't have both the class and id rule (only some that previous had notification bars) so I don't think we should add a selector which we can easily know isn't needed.

Comment 62

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

(In reply to Doug Turner (:dougt) from comment #54)
> nsm,
> 
> > This assertion should be moved to the constructor.
> 
> Not sure why that would be?  I am ensuring that the incoming arguments are
> what I expect.  Shouldn't we be doing this in other place?

There are assertions all over the place in worker code to make sure that a worker runnable only runs on the worker it was originally constructed with. It would be nicer to fail at site of dispatch.

Comment 63

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8584177 [details] [diff] [review]
Push Notifications - WebIDL changes

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

::: dom/webidl/PushManager.webidl
@@ +12,5 @@
> +    Promise<PushSubscription>     subscribe();
> +    Promise<PushSubscription?>    getSubscription();
> +    Promise<PushPermissionStatus> hasPermission();
> +
> +    // We need the a setter in the bindings so that the C++ can use it,

Nit: the a -> the

::: dom/webidl/ServiceWorkerRegistration.webidl
@@ +25,5 @@
>  };
> +
> +partial interface ServiceWorkerRegistration {
> +  [Throws]
> +  readonly attribute PushManager pushManager;

This should be hidden behind a pref, right?
It is currently hidden only because serviceWorkers are disabled.

Comment 64

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8584180 [details] [diff] [review]
Push Notifications - ServiceWorker changes, push event implementation,

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

r=me with event name fixed and other minor changes.

::: dom/workers/ServiceWorkerEvents.cpp
@@ +390,5 @@
> +
> +void
> +PushMessageData::Json(JSContext* cx, JS::MutableHandle<JSObject*> aRetval)
> +{
> +  //todo

Is there a bug on file for this and others? Please put the bug number here too.

::: dom/workers/ServiceWorkerManager.cpp
@@ +1476,5 @@
> +    GlobalObject globalObj(aCx, aWorkerPrivate->GlobalScope()->GetWrapper());
> +
> +    PushEventInit pei;
> +
> +    nsString optData(mData);

Unused.

@@ +1481,5 @@
> +    pei.mData.Construct(mData);
> +    pei.mBubbles = false;
> +    pei.mCancelable = true;
> +
> +    ErrorResult rv;

Nit: ErrorResult result;

@@ +1521,5 @@
> +    MOZ_ASSERT(aWorkerPrivate->IsServiceWorker());
> +
> +    nsRefPtr<EventTarget> target = do_QueryObject(aWorkerPrivate->GlobalScope());
> +
> +    nsContentUtils::DispatchTrustedEvent(nullptr, target,

There is a simpler form - target->DispatchTrustedEvent(NS_LITERAL_STRING("..."));

@@ +1522,5 @@
> +
> +    nsRefPtr<EventTarget> target = do_QueryObject(aWorkerPrivate->GlobalScope());
> +
> +    nsContentUtils::DispatchTrustedEvent(nullptr, target,
> +                                         NS_LITERAL_STRING("DeviceMotionEvent"),

Huh? :)

::: dom/workers/ServiceWorkerRegistration.cpp
@@ +317,5 @@
> +
> +    JS::RootedObject globalJs(cx, globalObject->GetGlobalJSObject());
> +    GlobalObject global(cx, globalJs);
> +
> +// TODO: bug 1148117.  This will fail when swr is exposed on workers

Nit: Indentation.

Comment 65

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8584179 [details] [diff] [review]
Push Notifications - Tests

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

::: dom/push/test/frame.html
@@ +7,5 @@
> +
> +function waitOnPushMessage(pushSubscription)
> +{
> +  var p = new Promise(function(res, rej) {
> +      navigator.serviceWorker.onmessage = function(e) {

Nit: Indentation starting here to end of function. 2 spaces.

::: dom/push/test/push-server.sjs
@@ +15,5 @@
> +  let xhr =  Cc["@mozilla.org/xmlextras/xmlhttprequest;1"].createInstance(Ci.nsIXMLHttpRequest);
> +  xhr.open("PUT", params);
> +  xhr.send();
> +  xhr.onload = function(e) {
> +      debug("xhr : " + this.status);

Nit: indentation from this line onwards.

::: dom/push/test/test_has_permissions.html
@@ +5,5 @@
> +
> +Any copyright is dedicated to the Public Domain.
> +http://creativecommons.org/licenses/publicdomain/
> +
> +TODO MOZ_DISABLE_NONLOCAL_CONNECTIONS

We can't ship the tests without this fixed right?

@@ +44,5 @@
> +  var p = new Promise(function(res, rej) {
> +    swr.pushManager.hasPermission().then(
> +      function(status) {
> +        debug("status: " + status);
> +        ok(true, "hasPermission() returned a status");

can we assert status == denied?

::: dom/push/test/test_register.html
@@ +61,5 @@
> +
> +var registration;
> +
> +function start() {
> +  return navigator.serviceWorker.register("worker.js" + "?" + (Math.random()), {scope: "."})

Why are you using random identifiers if the same worker is used for all tests? Every test can register (after the previous unregistration) and if the worker is already the active worker, the unregister will just be cancelled.

@@ +82,5 @@
> +        ok(false, "could not register for push notification");
> +        res(null);
> +      }
> +    );
> +  });

You don't need the bounding Promise, just:

return swr.pushManager.subscribe().then(...) will work due to the way then() automatically returns a Promise.

@@ +93,5 @@
> +  return pushSubscription.unsubscribe();
> +}
> +
> +function waitForPushNotification(pushSubscription) {
> +  var p = controlledFrame.contentWindow.waitOnPushMessage(pushSubscription);

Instead of passing the subscription to the frame, do this:

var p = controlledFrame.contentWindow.waitOnPushMessage();
sendPushToPushServer(pushSubscription.endpoint);
return p.then(function() {
  return pushSubscription;
});

::: dom/push/test/worker.js
@@ +7,5 @@
> +        if (event instanceof PushEvent &&
> +            event.data instanceof PushMessageData &&
> +            event.data.text().length > 0) {
> +
> +            console.log("------>   data.text(): " + event.data.text());

Please remove this.

Comment 66

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8584178 [details] [diff] [review]
Push Notifications - Push implementation changes

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

There are issues with delivering messages to SWs that need follow up bugs before this can get r+ed.

::: dom/push/Push.js
@@ +63,5 @@
> +    return this._pushEndpoint;
> +  },
> +
> +  get subscriptionId() {
> +    return "The twins of Mammon quarrelled.";

empty string?

@@ +84,5 @@
> +
> +    return this.createPromise(promiseInit);
> +  },
> +
> +receiveMessage: function(aMessage) {

Nit: indentation.

@@ +166,5 @@
> +
> +    debug("Existing permission " + permValue);
> +
> +    if (permValue == Ci.nsIPermissionManager.ALLOW_ACTION) {
> +        aAllowCallback();

Nit: indentation.

@@ +277,5 @@
>  
> +  getSubscription: function() {
> +    debug("getSubscription()" + this._scope);
> +
> +// TODO please tell me that I don't need to bind every function. ugh

Fat arrow syntax is now allowed in Firefox code.

@@ +305,5 @@
> +    debug("getSubscription()" + this._scope);
> +
> +    let promiseInit = function(resolve, reject) {
> +      let resolverId = this.getPromiseResolverId({resolve: resolve,
> +                                                  reject: reject });

This isn't needed since there is no IPC here.

@@ +317,5 @@
> +      } else if (permission == Ci.nsIPermissionManager.DENY_ACTION) {
> +        pushPermissionStatus = "denied";
> +      }
> +
> +      // TODO.  can I resolve from here?

Yes you can.

@@ +318,5 @@
> +        pushPermissionStatus = "denied";
> +      }
> +
> +      // TODO.  can I resolve from here?
> +      let resolver = this.takePromiseResolver(resolverId);

Not needed.

@@ +325,5 @@
> +    }.bind(this);
> +
> +    return this.createPromise(promiseInit)
> +
> +  },

Take everything that is inside promiseInit, move it to the hasPermission scope. then as the final line:

return Promise.resolve(pushPermissionStatus);

::: dom/push/PushService.jsm
@@ +375,2 @@
>          if (data.browserOnly) {
>            return;

Don't we always want to remove now?

@@ +377,5 @@
>          }
>  
>          let appsService = Cc["@mozilla.org/AppsService;1"]
>                              .getService(Ci.nsIAppsService);
> +        let scope = appsService.getScopeByLocalId(data.appId);

I don't think this works ;) There is no getScopeByLocalId. Need a way to go from manifest url to 'all scopes registered for push in this app'

@@ +1285,4 @@
>        }
>  
> +      // TODO -- test needed.
> +      let swm = Cc["@mozilla.org/serviceworkers/manager;1"].getService(Ci.nsIServiceWorkerManager);

This won't work on e10s and on b2g. The nsIServiceWorkerManager running in the child (and on b2g, in the appropriate child, which may involve launching an app) has to be notified. This will require using '<browser remote="true">' and a frame script at the minimum.

@@ +1287,5 @@
> +      // TODO -- test needed.
> +      let swm = Cc["@mozilla.org/serviceworkers/manager;1"].getService(Ci.nsIServiceWorkerManager);
> +      for (let scope in wakeupTable) {
> +        wakeupTable[scope].forEach(function(pageURL) {
> +          swm.sendPushEvent(aPushRecord.scope);

sendPushSubscriptionChangeEvent.

@@ +1323,5 @@
> +    }
> +
> +// TODO Data
> +
> +    let swm = Cc["@mozilla.org/serviceworkers/manager;1"].getService(Ci.nsIServiceWorkerManager);

again, has to be in the child.

@@ +1431,5 @@
>      let record = {
>        channelID: data.channelID,
>        pushEndpoint: data.pushEndpoint,
>        pageURL: aPageRecord.pageURL,
> +      scope: aPageRecord.scope,

For apps we are likely to continue to need manifestURL so we no which app to launch.

@@ +1546,5 @@
> +    let registration = null;
> +
> +    if (pushRecord) {
> +      registration = {
> +        __exposedProps__: { pushEndpoint: 'r', version: 'r' },

exposedProps not needed any more since we use PushSubscription to expose data to content.

Comment 67

[Doug Turner (:dougt)]

> We can't ship the tests without this fixed right?

No, but working with releng on a solution.  Expect that these todo's be removed before we land.

Comment 68

[Doug Turner (:dougt)]

nsm:

> We can't ship the tests without this fixed right?

No, but working with releng on a solution.  Expect that these todo's be removed before we land.

> Why are you using random identifiers if the same worker is used for all tests? Every test can register (after the previous unregistration) and if the worker is already the active worker, the unregister will just be cancelled.

Copy and paste.  Recall that bug whereby Sw was aggressively cacheing?...

> empty string?

The subscription id is something we need to figure out soon.  Right now, we don't need one but Chrome does.  Follow up bug has be filed and I will comment in code.

> this.takePromiseResolver

Using this is the best way to hold onto the resolve/reject methods when using the message manager.  I cleaned up hasPermission() because it doesn't need to use the messsage manager.  I also cleaned the code up around subscribe and getSubscription to not use promiseInit.  That makes the code read better.

> This won't work on e10s and on b2g

Yup.  That's next.

> getScopeByLocalId

Yup. Also site preferences!

Comment 69

[Doug Turner (:dougt)]

Attachment: 0004-Bug-1038811-Push-Notifications-Move-old-push-to-simp.patch

Comment 70

[Doug Turner (:dougt)]

FirefoxOS follow up bug: 1151180

Comment 71

[Doug Turner (:dougt)]

regarding clearing all push notifications from "Clear Recent History" in Firefox, it will be easier when we have an xpcom interface to push.  Kit is working on that.  What that interface is available, we will call that from: browser/base/content/sanitize.js

Comment 72

[Doug Turner (:dougt)]

Attachment: 0010-Bug-1038811-Push-Notifications-Update-to-push-change.patch

Comment 73

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8588279 [details] [diff] [review]
0004-Bug-1038811-Push-Notifications-Move-old-push-to-simp.patch

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

::: configure.in
@@ +7547,4 @@
>  AC_SUBST(MOZ_B2G_CAMERA)
>  
>  dnl ========================================================
> +dnl = Enable Support for SimplePush (Gonk usually)

Add a note that this will disable webpush (since the moz.build only picks one of them).

::: dom/webidl/moz.build
@@ +660,5 @@
> +         'SimplePushManager.webidl'
> +    ]
> +else:
> +    WEBIDL_FILES += [
> +    ]

Just |pass|

Comment 74

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8588490 [details] [diff] [review]
0010-Bug-1038811-Push-Notifications-Update-to-push-change.patch

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

::: dom/push/PushService.jsm
@@ +496,5 @@
>      if (!prefs.get("enabled"))
>          return null;
>  
> +    var globalMM = Cc["@mozilla.org/globalmessagemanager;1"]
> +               .getService(Ci.nsIMessageListenerManager);

this should be nsIFrameScriptLoader

@@ +1287,5 @@
>        }
>  
>        // TODO -- test needed.  E10s support needed.
> +
> +      let globalMM = Cc['@mozilla.org/globalmessagemanager;1'].getService(Ci.nsIFrameScriptLoader);

And this and below should be nsIMessageListenerManager.

Comment 75

[Doug Turner (:dougt)]

Attachment: a

Comment 76

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8591033 [details] [diff] [review]
a

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

This looks good to me, but I'll pass to ahal to double-check.

Comment 77

[Andrew Halberstadt [:ahal]]

Comment on attachment 8591033 [details] [diff] [review]
a

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

Lgtm, with one nit.

::: dom/push/test/mochitest.ini
@@ +1,2 @@
>  [DEFAULT]
> +tags = push

nit: remove tags. Setting subsuite removes the tests from the default run, so this will just lead to confusion when people run |mach mochitest --tag push| and nothing happens.

Comment 78

[Johnny Stenback (:jst)]

Comment on attachment 8584177 [details] [diff] [review]
Push Notifications - WebIDL changes

sr=jst, please add newlines to the end of the new files.

Comment 79

[Doug Turner (:dougt)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/raw-rev/4b63edbf1c12
remote:   https://hg.mozilla.org/integration/mozilla-inbound/raw-rev/1dd7cc876cd1
remote:   https://hg.mozilla.org/integration/mozilla-inbound/raw-rev/b277f825bc83
remote:   https://hg.mozilla.org/integration/mozilla-inbound/raw-rev/a33d054b75de
remote:   https://hg.mozilla.org/integration/mozilla-inbound/raw-rev/f75774fac6e2
remote:   https://hg.mozilla.org/integration/mozilla-inbound/raw-rev/7c9ebfb16e72
remote:   https://hg.mozilla.org/integration/mozilla-inbound/raw-rev/5b87a28c50c0

Comment 80

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4b63edbf1c12
https://hg.mozilla.org/mozilla-central/rev/1dd7cc876cd1
https://hg.mozilla.org/mozilla-central/rev/b277f825bc83
https://hg.mozilla.org/mozilla-central/rev/a33d054b75de
https://hg.mozilla.org/mozilla-central/rev/f75774fac6e2
https://hg.mozilla.org/mozilla-central/rev/7c9ebfb16e72
https://hg.mozilla.org/mozilla-central/rev/5b87a28c50c0

Comment 81

[Bobby Holley (:bholley)]

Hey Doug,

This landing added __exposedProps__, which is verboten, see [1]. If you explain why it's necessary, I'll tell you how to fix it. :-)

[1] https://groups.google.com/forum/#!msg/mozilla.dev.platform/yNKS1Z6UYQo/qloD9G0AdkcJ

Comment 82

[Doug Turner (:dougt)]

Yeah.  So this is in the old FirefoxOS code that we moved from dom/push to dom/simplepush.  I did fix the web exposed __exposedProps__ here:

https://hg.mozilla.org/mozilla-central/rev/a33d054b75de#l4.658

I do want to rm -rf simplepush as soon as possible.

Comment 83

[Bobby Holley (:bholley)]

Oh I see - the corresponding removal was in another changeset. I guess that's fine then, especially since simplepush is going away.

Comment 84

[Dragana Damjanovic [:dragana]]

In http://mxr.mozilla.org/mozilla-central/source/dom/push/PushService.jsm#1292

for (let scope in wakeupTable) {
  wakeupTable[scope].forEach(function(pageURL) {
    globalMM.broadcastAsyncMessage('pushsubscriptionchanged', aPushRecord.scope);
  });
}

it should be 
globalMM.broadcastAsyncMessage('pushsubscriptionchanged', scope);
I have not look at broadcastAsyncMessage, but name broadcast suggest that this should be 
for (let scope in wakeupTable) {
  globalMM.broadcastAsyncMessage('pushsubscriptionchanged', scope);
}

Comment 85

[Doug Turner (:dougt)]

Thanks Dragana.  Good find.  No tests for this right now as the comment:

http://mxr.mozilla.org/mozilla-central/source/dom/push/PushService.jsm#1287

says! :)

please file a new bug.

Comment 86

[Lina Butler [:lina]]

Nice catch, Dragana! I fixed this up in https://bugzilla.mozilla.org/attachment.cgi?id=8594228&action=diff, but the new tests caused IndexedDB thrashing. Can split it out (without the xpcshell tests) into a separate patch if that makes things easier.

Comment 87

[:gerard-majax]

Strangely, in bug 1159046 comment 37 I have no PushServiceLauncher.js being triggered on B2G, making SimplePush not working. Loading by hand from WebIDE makes it working again.

MOZ_SIMPLEPUSH=1 is present in objdir-gecko/config.status

Extracted omni.ja defines app-startup on PushServiceLauncher:
> gecko/omni $ grep -R 'PushServiceLauncher' 
> components/components.manifest:component {4b8caa3b-3c58-4f3c-a7f5-7bd9cb24c11d} PushServiceLauncher.js
> components/components.manifest:category app-startup PushServiceLauncher service,@mozilla.org/push/ServiceLauncher;1

But I cannot find PushServiceLauncher in omni.ja:
> gecko/omni$ find . | grep PushService
> ./chrome/toolkit/content/global/PushServiceChildPreload.js
> ./modules/PushService.jsm
> gecko/omni$