superreview requested: [Bug 857464] Push Notifications - Desktop support : [Attachment 744173] Sandbox + system messages

[bugzill...@mozilla.org]

Doug Turner (:dougt) <doug....@gmail.com> has asked Jonas Sicking (:sicking)
<jo...@sicking.cc> for superreview:
Bug 857464: Push Notifications - Desktop support
https://bugzilla.mozilla.org/show_bug.cgi?id=857464

Attachment 744173: Sandbox + system messages
https://bugzilla.mozilla.org/attachment.cgi?id=744173&action=edit

[bugzill...@mozilla.org]

Nikhil Marathe <nsm.n...@gmail.com> has canceled Doug Turner (:dougt)
<doug....@gmail.com>'s request for superreview:

Bug 857464: Push Notifications - Desktop support
https://bugzilla.mozilla.org/show_bug.cgi?id=857464

Attachment 744173: Sandbox + system messages
https://bugzilla.mozilla.org/attachment.cgi?id=744173&action=edit

------- Additional Comments from Nikhil Marathe <nsm.n...@gmail.com>
Fixes issues and nits raised by Fabrice and Mounir.

I've kept the Sandbox in identity for now, it would be cleaner to file a
separate bug to move it into toolkit/modules.

I've also moved prefs into a separate attachment with RELEASE_BUILD guards.

[bugzill...@mozilla.org]

Nikhil Marathe <nsm.n...@gmail.com> has asked Jonas Sicking (:sicking)
<jo...@sicking.cc> for superreview:

Bug 857464: Push Notifications - Desktop support
https://bugzilla.mozilla.org/show_bug.cgi?id=857464

Attachment 747909: Sandbox + system messages
https://bugzilla.mozilla.org/attachment.cgi?id=747909&action=edit

[bugzill...@mozilla.org]

Nikhil Marathe <nsm.n...@gmail.com> has asked Jonas Sicking (:sicking)
<jo...@sicking.cc> for superreview:
Bug 857464: Push Notifications - Desktop support
https://bugzilla.mozilla.org/show_bug.cgi?id=857464

Attachment 747913: Sandbox + system messages
https://bugzilla.mozilla.org/attachment.cgi?id=747913&action=edit

[bugzill...@mozilla.org]

Nikhil Marathe <nsm.n...@gmail.com> has canceled Nikhil Marathe
<nsm.n...@gmail.com>'s request for superreview:

Bug 857464: Push Notifications - Desktop support
https://bugzilla.mozilla.org/show_bug.cgi?id=857464

Attachment 747909: Sandbox + system messages
https://bugzilla.mozilla.org/attachment.cgi?id=747909&action=edit

[bugzill...@mozilla.org]

Nikhil Marathe <nsm.n...@gmail.com> has canceled Nikhil Marathe
<nsm.n...@gmail.com>'s request for superreview:
Bug 857464: Push Notifications - Desktop support
https://bugzilla.mozilla.org/show_bug.cgi?id=857464

Attachment 747913: Sandbox + system messages
https://bugzilla.mozilla.org/attachment.cgi?id=747913&action=edit

------- Additional Comments from Nikhil Marathe <nsm.n...@gmail.com>

Pass on the absolute URI derived from newURI to the service, otherwise relative
URLs will fail on push notification.

Remove this._origin, this._pageURI is enough.

[bugzill...@mozilla.org]

Nikhil Marathe <nsm.n...@gmail.com> has asked Jonas Sicking (:sicking)

<jo...@sicking.cc> for superreview:

Bug 857464: Push Notifications - Desktop support
https://bugzilla.mozilla.org/show_bug.cgi?id=857464

Attachment 748378: Sandbox + system messages
https://bugzilla.mozilla.org/attachment.cgi?id=748378&action=edit

[bugzill...@mozilla.org]

Nikhil Marathe <nsm.n...@gmail.com> has canceled Nikhil Marathe

<nsm.n...@gmail.com>'s request for superreview:

Bug 857464: Push Notifications - Desktop support
https://bugzilla.mozilla.org/show_bug.cgi?id=857464

Attachment 748378: Sandbox + system messages
https://bugzilla.mozilla.org/attachment.cgi?id=748378&action=edit


------- Additional Comments from Nikhil Marathe <nsm.n...@gmail.com>

This patch moves out much of the system messages changes into bug 868322.

Added support for prompt on desktop.
Added registerPage() calls to get system messages working.
Changed DOMRequest errors to use DOM spec error names.
Removed fakeSuccess and instead fail with SecurityError.

Mounir: throw NS_ERROR_DOM* does not work in JS because the DOM error codes are
not exposed to Components.results in JS.

[bugzill...@mozilla.org]

Mounir Lamouri (:mounir) <mou...@lamouri.fr> has asked Jonas Sicking (:sicking)
<jo...@sicking.cc> for superreview:

Bug 857464: Push Notifications - Desktop support
https://bugzilla.mozilla.org/show_bug.cgi?id=857464

Attachment 751937: Desktop push.
https://bugzilla.mozilla.org/attachment.cgi?id=751937&action=edit


------- Additional Comments from Mounir Lamouri (:mounir) <mou...@lamouri.fr>
Review of attachment 751937:
-----------------------------------------------------------------

r=me with my comments applied and the register() method signature discussed a
bit more.

I guess Doug need to review this code too. You should also ask a browser/ peer
to review the changes in nsBrowserGlue.js and browser.properties. You should
also ask a ui-review for the strings in browser.properties.

::: browser/locales/en-US/chrome/browser/browser.properties
@@ +300,5 @@
> +push.alwaysAllow=Always allow
> +push.alwaysAllow.accesskey=A
> +push.neverAllow=Never allow
> +push.neverAllow.accesskey=N
> +push.allowFromSite=Do you want to allow push notifications from %S?

You should ask ui-review for that I believe.
FWIW, I wonder why the accesskey for "allowForSession" is lowercase while the
other a uppercase. It might change nothing though. Also, I think the
description will never be understood by a real human being: I doubt most our
users have any idea what "push notifications" are and what would be the
consequences of allowing them.

::: dom/interfaces/push/nsIDOMPushManager.idl
@@ +21,5 @@
> *
> * On success, the DOMRequest's result field will be a string URL. This
URL
> * is the endpoint that can be contacted to wake up the application.
> */
> + nsIDOMDOMRequest register([optional] in ACString wakeupURL);

I'm not terribly convinced that we should bind the system message mechanism to
every API that will require it. I'm also not a super fan of having two
different method signatures, one for application context and one for browser
context. I will not r- the patch for that but I would appreciate some
discussions before pushing this forward.

::: dom/permission/PermissionPromptService.js
@@ +71,5 @@
> let type = aRequest.access !== "unused" ? aRequest.type + "-" +
aRequest.access
> : aRequest.type;
> let perm =
> permissionManager.testExactPermissionFromPrincipal(aRequest.principal,
type);
> + dump(type + " " + perm);

I guess this debug code should go away.

::: dom/push/src/Push.js
@@ +168,5 @@
> + },
> + allow: function() {
> + // Add push permissions to the wakeup URL.
> + Services.perms.add(wakeupURI, 'push',
Ci.nsIPermissionManager.ALLOW_ACTION,
> + Ci.nsIPermissionManager.EXPIRE_NEVER);

I believe this should be done by nsIContentPermissionPrompt, shouldn't it? I'm
afraid you might end up over-writting session permission with a always
permission here.

::: dom/push/src/PushService.jsm
@@ +505,5 @@
> this._requestTimeout = prefs.get("requestTimeout");
>
> this._udpPort = prefs.get("udp.port");
>
> this._db.getAllChannelIDs(

nit: a comment saying what this method is doing would be pretty awesome.

@@ -506,5 @@
> -
> - // At this point, profile-change-net-teardown has already fired, so the
> - // WebSocket has been closed with NS_ERROR_ABORT (if it was up) and will

> - // try to reconnect. Stop the timer.
> - this._stopAlarm();

Is that chunk here on purpose?

@@ +1265,5 @@
> + return;
> + }
> + // If endpoint was created by a web page, only allow pages with the
same
> + // origin permission to delete the endpoint.
> + else if (!record.manifestURL) {

No need for "else" here.

@@ +1274,5 @@
> + " is trying to delete endpoint set by " + recordPageURI.spec);

> + try {
> + Services.scriptSecurityManager.checkSameOriginURI(callerPageURI,
> + recordPageURI,
> + false);

If the call is developer initiated, having true as a third parameter would be
better so developers can debug more easily.

[bugzill...@mozilla.org]

Nikhil Marathe <nsm.n...@gmail.com> has asked Jonas Sicking (:sicking)

<jo...@sicking.cc> for superreview:
Bug 857464: Push Notifications - Desktop support
https://bugzilla.mozilla.org/show_bug.cgi?id=857464

Attachment 762384: Desktop push.
https://bugzilla.mozilla.org/attachment.cgi?id=762384&action=edit

------- Additional Comments from Nikhil Marathe <nsm.n...@gmail.com>

Changed to use { worker: 'script.js' } for desktop.

[bugzill...@mozilla.org]

Nikhil Marathe <nsm.n...@gmail.com> has canceled Mounir Lamouri (:mounir)
<mou...@lamouri.fr>'s request for superreview:

Bug 857464: Push Notifications - Desktop support
https://bugzilla.mozilla.org/show_bug.cgi?id=857464

Attachment 751937: Desktop push.
https://bugzilla.mozilla.org/attachment.cgi?id=751937&action=edit

[bugzill...@mozilla.org]

Nikhil Marathe <nsm.n...@gmail.com> has canceled Nikhil Marathe
<nsm.n...@gmail.com>'s request for superreview:

Bug 857464: Push Notifications - Desktop support
https://bugzilla.mozilla.org/show_bug.cgi?id=857464

Attachment 762384: Desktop push.
https://bugzilla.mozilla.org/attachment.cgi?id=762384&action=edit

------- Additional Comments from Nikhil Marathe <nsm.n...@gmail.com>

Carrying forward dougt's r+.

Add |{page:}| and |{eventPage:}| support to register()

[bugzill...@mozilla.org]

Nikhil Marathe <nsm.n...@gmail.com> has asked Jonas Sicking (:sicking)

<jo...@sicking.cc> for superreview:

Bug 857464: Push Notifications - Desktop support
https://bugzilla.mozilla.org/show_bug.cgi?id=857464

Attachment 764487: Desktop push with event page support
https://bugzilla.mozilla.org/attachment.cgi?id=764487&action=edit

[bugzill...@mozilla.org]

Nikhil Marathe <nsm.n...@gmail.com> has asked Jonas Sicking (:sicking)
<jo...@sicking.cc> for superreview:
Bug 857464: Push Notifications - Desktop support
https://bugzilla.mozilla.org/show_bug.cgi?id=857464

Attachment 765071: Desktop push with event page support
https://bugzilla.mozilla.org/attachment.cgi?id=765071&action=edit

------- Additional Comments from Nikhil Marathe <nsm.n...@gmail.com>

Moved browser changes into bug 885093.

navigator.push is now null in private browsing mode. This should stay this way
until SimplePush is modified to use multiple IndexedDBs. Filed 885108.

[bugzill...@mozilla.org]

Nikhil Marathe <nsm.n...@gmail.com> has canceled Nikhil Marathe

<nsm.n...@gmail.com>'s request for superreview:

Bug 857464: Push Notifications - Desktop support
https://bugzilla.mozilla.org/show_bug.cgi?id=857464

Attachment 764487: Desktop push with event page support
https://bugzilla.mozilla.org/attachment.cgi?id=764487&action=edit

[bugzill...@mozilla.org]

Jonas Sicking (:sicking) <jo...@sicking.cc> has granted Nikhil Marathe

<nsm.n...@gmail.com>'s request for superreview:
Bug 857464: Push Notifications - Desktop support
https://bugzilla.mozilla.org/show_bug.cgi?id=857464

Attachment 765071: Desktop push with event page support
https://bugzilla.mozilla.org/attachment.cgi?id=765071&action=edit

------- Additional Comments from Jonas Sicking (:sicking) <jo...@sicking.cc>
Review of attachment 765071:
-----------------------------------------------------------------

sr=me assuming you've hooked this up to the webapps-clear-data notification?

::: dom/push/src/Push.js
@@ +160,5 @@
> + type = 'eventpage';
> + url = aOptions.eventPage;
> + } else {
> + type = 'page';
> + url = this._pageURI.spec;

I think we should remove this default and simply throw an exception here. It's
probably the less common case to actually want a page to pop up in response an
incoming push message, then to run some logic and only open a tab after that.
So making that the default doesn't seem good.

[bugzill...@mozilla.org]

Nikhil Marathe [:nsm] <nsm.n...@gmail.com> has granted superreview:

Bug 857464: Push Notifications - Desktop support
https://bugzilla.mozilla.org/show_bug.cgi?id=857464

Attachment 774193: Desktop push with event page support
https://bugzilla.mozilla.org/attachment.cgi?id=774193&action=edit


------- Additional Comments from Nikhil Marathe [:nsm] <nsm.n...@gmail.com>
Rebased on changes from 867632 and 889984