Reject permissonState calls if userVisible is not present as parameter

Reject permissonState calls if userVisibleOnly is not present as parameter

Since silent push is not implemented in chrome it makes more sense to fail with NotSupportedError than deny it.

BUG=479246

[Miguel Garcia, 2015-05-07 15:14:22]

Patchset #1 (id:1) has been deleted

[Miguel Garcia, 2015-05-07 15:15:21]

miguelg@chromium.org changed reviewers:
+ peter@chromium.org

[Peter Beverloo, 2015-05-07 15:28:22]

Thanks for the patch. It's rather... unpolished.

Comments on the CL's description:
  - Please clarify that this is Chrome-specific behavior.
  - s/userVisible/userVisibleOnly/
  - s/NotSupported/NotSupportedError/

https://codereview.chromium.org/1136513002/diff/20001/LayoutTests/http/tests/...
File
LayoutTests/http/tests/push_messaging/permission-state-denied-in-document.html
(right):

https://codereview.chromium.org/1136513002/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/push_messaging/permission-state-denied-in-document.html:20:
return serviceWorkerRegistration.pushManager.permissionState({userVisibleOnly :
true});
nit: no space before the colon

https://codereview.chromium.org/1136513002/diff/20001/LayoutTests/http/tests/...
File
LayoutTests/http/tests/push_messaging/permission-state-denied-in-service-worker.html
(right):

https://codereview.chromium.org/1136513002/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/push_messaging/permission-state-denied-in-service-worker.html:24:
workerInfo.port.postMessage({command: 'permissionState', arg: {userVisibleOnly:
true}});
nit: s/arg/options/

https://codereview.chromium.org/1136513002/diff/20001/LayoutTests/http/tests/...
File
LayoutTests/http/tests/push_messaging/permission-state-exception-in-document.html
(right):

https://codereview.chromium.org/1136513002/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/push_messaging/permission-state-exception-in-document.html:4:
<title>Push API: permissionState fails with NotSupported if userVisible is not
used</title>
NotSupported -> NotSupportedError; userVisible -> userVisibleOnly. In the test's
description as well.

https://codereview.chromium.org/1136513002/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/push_messaging/permission-state-exception-in-document.html:20:
// If running manually, clear permissions before running this test.
Blink indents with four spaces.

https://codereview.chromium.org/1136513002/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/push_messaging/permission-state-exception-in-document.html:26:
assert_equals(error.name, "NotSupportedError");
nit: Please be consistent with ' vs. "

https://codereview.chromium.org/1136513002/diff/20001/LayoutTests/http/tests/...
File
LayoutTests/http/tests/push_messaging/permission-state-exception-in-service-worker.html
(right):

https://codereview.chromium.org/1136513002/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/push_messaging/permission-state-exception-in-service-worker.html:4:
<title>Push API: permissionState when called in service worker fails with
NotSupported if userVisible is not used</title>
dito re: NotSupported(Error) and userVisible(Only)

https://codereview.chromium.org/1136513002/diff/20001/LayoutTests/http/tests/...
File
LayoutTests/http/tests/push_messaging/permission-state-granted-in-document.html
(right):

https://codereview.chromium.org/1136513002/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/push_messaging/permission-state-granted-in-document.html:20:
return serviceWorkerRegistration.pushManager.permissionState({userVisibleOnly :
true});
nit: no space before the colon.

https://codereview.chromium.org/1136513002/diff/20001/LayoutTests/http/tests/...
File
LayoutTests/http/tests/push_messaging/permission-state-prompt-in-document.html
(right):

https://codereview.chromium.org/1136513002/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/push_messaging/permission-state-prompt-in-document.html:21:
return serviceWorkerRegistration.pushManager.permissionState({userVisibleOnly :
true});
dito re: space before the colon.

https://codereview.chromium.org/1136513002/diff/20001/LayoutTests/http/tests/...
File
LayoutTests/http/tests/push_messaging/resources/instrumentation-service-worker.js
(right):

https://codereview.chromium.org/1136513002/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/push_messaging/resources/instrumentation-service-worker.js:16:
var arguments = event.data.arg || {}
s/(arguments|arg)/options/

https://codereview.chromium.org/1136513002/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/push_messaging/resources/instrumentation-service-worker.js:16:
var arguments = event.data.arg || {}
Please also pass the |options| argument to subscribe() on line 27. Even if we
don't use it yet, we'll most likely want to at some point.

https://codereview.chromium.org/1136513002/diff/20001/Source/modules/push_mes...
File Source/modules/push_messaging/PushManager.cpp (right):

https://codereview.chromium.org/1136513002/diff/20001/Source/modules/push_mes...
Source/modules/push_messaging/PushManager.cpp:97: return
ScriptPromise::rejectWithDOMException(scriptState,
DOMException::create(NotSupportedError, "non-visible push messages are not
supported."));
What about: "Chrome does not yet support push subscriptions that do not enable
the userVisibleOnly option."

This (a) indicates that it's Chrome-specific behavior, not per the spec, and (b)
refers to the userVisibleOnly option, which is a term for developers to work
towards enabling this.

[Miguel Garcia, 2015-05-07 19:40:46]

https://chromiumcodereview.appspot.com/1136513002/diff/20001/LayoutTests/http...
File
LayoutTests/http/tests/push_messaging/permission-state-denied-in-document.html
(right):

https://chromiumcodereview.appspot.com/1136513002/diff/20001/LayoutTests/http...
LayoutTests/http/tests/push_messaging/permission-state-denied-in-document.html:20:
return serviceWorkerRegistration.pushManager.permissionState({userVisibleOnly :
true});
On 2015/05/07 15:28:21, Peter Beverloo wrote:
> nit: no space before the colon

Done.

https://chromiumcodereview.appspot.com/1136513002/diff/20001/LayoutTests/http...
File
LayoutTests/http/tests/push_messaging/permission-state-denied-in-service-worker.html
(right):

https://chromiumcodereview.appspot.com/1136513002/diff/20001/LayoutTests/http...
LayoutTests/http/tests/push_messaging/permission-state-denied-in-service-worker.html:24:
workerInfo.port.postMessage({command: 'permissionState', arg: {userVisibleOnly:
true}});
On 2015/05/07 15:28:21, Peter Beverloo wrote:
> nit: s/arg/options/

Done.

https://chromiumcodereview.appspot.com/1136513002/diff/20001/LayoutTests/http...
File
LayoutTests/http/tests/push_messaging/permission-state-exception-in-document.html
(right):

https://chromiumcodereview.appspot.com/1136513002/diff/20001/LayoutTests/http...
LayoutTests/http/tests/push_messaging/permission-state-exception-in-document.html:4:
<title>Push API: permissionState fails with NotSupported if userVisible is not
used</title>
On 2015/05/07 15:28:21, Peter Beverloo wrote:
> NotSupported -> NotSupportedError; userVisible -> userVisibleOnly. In the
test's
> description as well.

Done.

https://chromiumcodereview.appspot.com/1136513002/diff/20001/LayoutTests/http...
LayoutTests/http/tests/push_messaging/permission-state-exception-in-document.html:20:
// If running manually, clear permissions before running this test.
On 2015/05/07 15:28:21, Peter Beverloo wrote:
> Blink indents with four spaces.

Done.

https://chromiumcodereview.appspot.com/1136513002/diff/20001/LayoutTests/http...
LayoutTests/http/tests/push_messaging/permission-state-exception-in-document.html:26:
assert_equals(error.name, "NotSupportedError");
On 2015/05/07 15:28:21, Peter Beverloo wrote:
> nit: Please be consistent with ' vs. "

Done.

https://chromiumcodereview.appspot.com/1136513002/diff/20001/LayoutTests/http...
File
LayoutTests/http/tests/push_messaging/permission-state-exception-in-service-worker.html
(right):

https://chromiumcodereview.appspot.com/1136513002/diff/20001/LayoutTests/http...
LayoutTests/http/tests/push_messaging/permission-state-exception-in-service-worker.html:4:
<title>Push API: permissionState when called in service worker fails with
NotSupported if userVisible is not used</title>
On 2015/05/07 15:28:21, Peter Beverloo wrote:
> dito re: NotSupported(Error) and userVisible(Only)

Done.

https://chromiumcodereview.appspot.com/1136513002/diff/20001/LayoutTests/http...
File
LayoutTests/http/tests/push_messaging/permission-state-granted-in-document.html
(right):

https://chromiumcodereview.appspot.com/1136513002/diff/20001/LayoutTests/http...
LayoutTests/http/tests/push_messaging/permission-state-granted-in-document.html:20:
return serviceWorkerRegistration.pushManager.permissionState({userVisibleOnly :
true});
On 2015/05/07 15:28:21, Peter Beverloo wrote:
> nit: no space before the colon.

Done.

https://chromiumcodereview.appspot.com/1136513002/diff/20001/LayoutTests/http...
File
LayoutTests/http/tests/push_messaging/permission-state-prompt-in-document.html
(right):

https://chromiumcodereview.appspot.com/1136513002/diff/20001/LayoutTests/http...
LayoutTests/http/tests/push_messaging/permission-state-prompt-in-document.html:21:
return serviceWorkerRegistration.pushManager.permissionState({userVisibleOnly :
true});
On 2015/05/07 15:28:21, Peter Beverloo wrote:
> dito re: space before the colon.

Done.

https://chromiumcodereview.appspot.com/1136513002/diff/20001/LayoutTests/http...
File
LayoutTests/http/tests/push_messaging/resources/instrumentation-service-worker.js
(right):

https://chromiumcodereview.appspot.com/1136513002/diff/20001/LayoutTests/http...
LayoutTests/http/tests/push_messaging/resources/instrumentation-service-worker.js:16:
var arguments = event.data.arg || {}
On 2015/05/07 15:28:21, Peter Beverloo wrote:
> s/(arguments|arg)/options/

Done.

https://chromiumcodereview.appspot.com/1136513002/diff/20001/LayoutTests/http...
LayoutTests/http/tests/push_messaging/resources/instrumentation-service-worker.js:16:
var arguments = event.data.arg || {}
On 2015/05/07 15:28:21, Peter Beverloo wrote:
> Please also pass the |options| argument to subscribe() on line 27. Even if we
> don't use it yet, we'll most likely want to at some point.

Done.

https://chromiumcodereview.appspot.com/1136513002/diff/20001/Source/modules/p...
File Source/modules/push_messaging/PushManager.cpp (right):

https://chromiumcodereview.appspot.com/1136513002/diff/20001/Source/modules/p...
Source/modules/push_messaging/PushManager.cpp:97: return
ScriptPromise::rejectWithDOMException(scriptState,
DOMException::create(NotSupportedError, "non-visible push messages are not
supported."));
I'd prefer to avoid "yet" since it's not clear exactly what's our timeline and
conditions for silent push.

I will go with "Chrome does not support push subscriptions that do not enable
the userVisibleOnly option."


On 2015/05/07 15:28:21, Peter Beverloo wrote:
> What about: "Chrome does not yet support push subscriptions that do not enable
> the userVisibleOnly option."
> 
> This (a) indicates that it's Chrome-specific behavior, not per the spec, and
(b)
> refers to the userVisibleOnly option, which is a term for developers to work
> towards enabling this.

[Peter Beverloo, 2015-05-07 22:39:04]

Let's chat about this tomorrow.

Tests look fine, but I would very much prefer the check in PushManager.cpp to
move to //content/ code (possibly even //chrome/). Let's find out how involved
it would be to do so.

[Peter Beverloo, 2015-05-08 12:17:50]

I do not think we should have this code in Blink.

The decision to support push subscriptions which do not result in notifications
is carefully and consistently made in the //chrome layer in the Push Messaging
implementation. We do this both for permission requests[1] and for enforcing the
UX requirements on incoming messages[2].

There is no reason why a //content embedder could not decide to support silent
push. For me, that's where I mentally draw the line between //chrome and
//content - us not supporting silent push is solely a UA decision. The
specification includes the "userVisibleOnly" key exactly to enable this
decision.

This CL unconditionally says no in Blink to UA-specific behavior that's in its
entirely controlled by and implemented in //chrome. It's the wrong layer.

Rather, while I acknowledge that it'll be a fair share of more work, the TODO on
the first line of PushMessagingServiceImpl::GetPermissionStatus() should be
addressed. I'm happy to help figuring out the right way of doing that, since
it's not entirely straightforward :-). There's a week left before branch point,
plenty of time.

Finally, the Permissions API seems to be doing this already for the
"userVisible" property (which has yet to be renamed?), which I disagree with as
well. I would like to note  that no Push Messaging feature owners were involved
in the review of that CL[4].


[1] The registration methods in
chrome/browser/push_messaging/push_messaging_service_impl.cc.
[2] DeliverMessageCallback() in
chrome/browser/push_messaging/push_messaging_service_impl.cc.
[3] query() in Source/modules/permissions/Permissions.cpp.
[4] https://codereview.chromium.org/1045283002

[Miguel Garcia, 2015-05-08 13:38:17]

miguelg@chromium.org changed reviewers:
+ johnme@chromium.org, mvanouwerkerk@chromium.org

[Miguel Garcia, 2015-05-08 13:38:17]

+John and Michael

As Peter mentioned in the previous comment we didn't manage to reach a
conclusion on this so opening it up to other OWNERS.

A summary of the conversation for your consideration

pros/condition of keeping it as coded:
It's a 3 line change instead of possibly 50 to 100 just to plumb this
everywhere. Which we then have to maintain forever.
If any other vendor were to request this to be changed because they want to do
something different before we ship silent push we will agree and even contribute
to have it changed.
The permissions API does the same thing. In fact we do it in a fair amount of
places like WebAudio or ServiceWorkers
https://cs.corp.google.com/#search/&q=rejectWithDOMException%5C(%20file:%5Esr...

We should probably change the exception so that it does not say Chrome (but we
probably want to do this anyway).

cons: 
As Peter says there is a theoretical possibility of a other embedder wanting to
do something else so it is in fact  a small layering violation.

[Peter Beverloo, 2015-05-08 14:08:02]

On 2015/05/08 13:38:17, Miguel Garcia wrote:
> pros/condition of keeping it as coded:
> It's a 3 line change instead of possibly 50 to 100 just to plumb this
> everywhere. Which we then have to maintain forever.

That's a consequence of Chrome's layering. We're upholding a certain quality in
this regard with our Push Messaging implementation, and you're proposing to
lower this quality because it's easier. I definitely disagree with that.

Additionally, we would only have to maintain this code until we support silent
push. You will find that the added complexity of supporting this is relatively
minor.

> If any other vendor were to request this to be changed because they want to do
> something different before we ship silent push we will agree and even
contribute
> to have it changed.

We have shown due diligence in maintaining proper layering between //chrome and
//content thus far, regardless of the plans of Opera, Samsung or any other
external vendor.

Their decisions are not ours to make, and I definitely agree that we should not
preemptively accommodate for their theoretical decisions where they diverge from
ours. This is not an example of that, however: *we* make the entire decision in
//chrome and are thus accommodating for our own needs.

In a situation where we support silent push, but external parties do not, then
we should raise this with them, understand their concerns, and potentially agree
for them to take over maintenance of that code.

> The permissions API does the same thing.

I already replied to that - it shouldn't.

> In fact we do it in a fair amount of
> places like WebAudio or ServiceWorkers
>
https://cs.corp.google.com/#search/&q=rejectWithDOMException%5C(%20file:%5Esr...

A vast amount of logic of the Web Audio API and Service Workers is implemented
in Blink. The Push Messaging code in Blink is mostly plumbing. There's an
important difference there - they may not support features because the
interfaces are not yet implemented in Blink, whereas in our case the reasoning
lives in //chrome.

However, I'm sure there are other places in Blink, like the Permission API,
where we do. Are you really arguing that it's OK to break layering because other
features do?

> We should probably change the exception so that it does not say Chrome (but we
> probably want to do this anyway).

I don't feel strongly about the text of the exception :-). My main point there
is that I would like it to be clear that not supporting silent push is a
decision made by Chrome as a browser, not by the specification. That makes a
difference for Web developers, because other implementations (notably Firefox)
will support it.

[Michael van Ouwerkerk, 2015-05-08 14:32:07]

I agree with Peter on this. It's a layering violation, and we try to avoid doing
that, and have put significant effort into this in the past.

[Miguel Garcia, 2015-05-08 14:33:45]

On 2015/05/08 14:32:07, Michael van Ouwerkerk wrote:
> I agree with Peter on this. It's a layering violation, and we try to avoid
doing
> that, and have put significant effort into this in the past.

me sad. I just discovered that John is not an owner so I guess I am going to
write all the plumbing anyway. Totally disagree with this.

[Peter Beverloo, 2015-05-08 14:38:14]

On 2015/05/08 14:33:45, Miguel Garcia wrote:
> On 2015/05/08 14:32:07, Michael van Ouwerkerk wrote:
> > I agree with Peter on this. It's a layering violation, and we try to avoid
> doing
> > that, and have put significant effort into this in the past.
> 
> I just discovered that John is not an owner

Err, fixed in https://codereview.chromium.org/1129373004.

[johnme, 2015-05-08 15:48:22]

https://codereview.chromium.org/1136513002/diff/40001/Source/modules/push_mes...
File Source/modules/push_messaging/PushManager.cpp (right):

https://codereview.chromium.org/1136513002/diff/40001/Source/modules/push_mes...
Source/modules/push_messaging/PushManager.cpp:97: return
ScriptPromise::rejectWithDOMException(scriptState,
DOMException::create(NotSupportedError,
To change the subject (since I think Peter and Michael have covered layering),
why does this throw an exception when you access permissionState()? That's
incompatible with developers that base their implementation on the standardized
Push API, and the unexpected exception could break their entire website.
Wouldn't it be better to return "denied" instead (and possibly log a
once-per-pageload message to the console if we want to be helpful)?

[johnme, 2015-05-08 15:57:07]

https://codereview.chromium.org/1136513002/diff/40001/Source/modules/push_mes...
File Source/modules/push_messaging/PushManager.cpp (right):

https://codereview.chromium.org/1136513002/diff/40001/Source/modules/push_mes...
Source/modules/push_messaging/PushManager.cpp:97: return
ScriptPromise::rejectWithDOMException(scriptState,
DOMException::create(NotSupportedError,
On 2015/05/08 15:48:22, johnme wrote:
> To change the subject (since I think Peter and Michael have covered layering),
> why does this throw an exception when you access permissionState()? That's
> incompatible with developers that base their implementation on the
standardized
> Push API, and the unexpected exception could break their entire website.
> Wouldn't it be better to return "denied" instead (and possibly log a
> once-per-pageload message to the console if we want to be helpful)?

Oh, it's rejecting the promise with an exception, rather than throwing one.
That's not quite so bad, but it might still cause developers to trip up, since
whilst the spec allows the promise to be rejected, it doesn't list any
circumstances under which this would happen, and it's hard to imagine any good
reasons for a UA to do so either.

[Miguel Garcia, 2015-05-08 21:00:32]

https://chromiumcodereview.appspot.com/1136513002/diff/40001/Source/modules/p...
File Source/modules/push_messaging/PushManager.cpp (right):

https://chromiumcodereview.appspot.com/1136513002/diff/40001/Source/modules/p...
Source/modules/push_messaging/PushManager.cpp:97: return
ScriptPromise::rejectWithDOMException(scriptState,
DOMException::create(NotSupportedError,
Yes, it feels more accurate than just denying. That's also what the permission
API does.


On 2015/05/08 15:57:07, johnme wrote:
> On 2015/05/08 15:48:22, johnme wrote:
> > To change the subject (since I think Peter and Michael have covered
layering),
> > why does this throw an exception when you access permissionState()? That's
> > incompatible with developers that base their implementation on the
> standardized
> > Push API, and the unexpected exception could break their entire website.
> > Wouldn't it be better to return "denied" instead (and possibly log a
> > once-per-pageload message to the console if we want to be helpful)?
> 
> Oh, it's rejecting the promise with an exception, rather than throwing one.
> That's not quite so bad, but it might still cause developers to trip up, since
> whilst the spec allows the promise to be rejected, it doesn't list any
> circumstances under which this would happen, and it's hard to imagine any good
> reasons for a UA to do so either.

[Miguel Garcia, 2015-05-11 09:40:53]

Patchset #3 (id:60001) has been deleted

[johnme, 2015-05-11 12:31:01]

On 2015/05/08 21:00:32, Miguel Garcia wrote:
>
https://chromiumcodereview.appspot.com/1136513002/diff/40001/Source/modules/p...
> File Source/modules/push_messaging/PushManager.cpp (right):
> 
>
https://chromiumcodereview.appspot.com/1136513002/diff/40001/Source/modules/p...
> Source/modules/push_messaging/PushManager.cpp:97: return
> ScriptPromise::rejectWithDOMException(scriptState,
> DOMException::create(NotSupportedError,
> 
> On 2015/05/08 15:57:07, johnme wrote:
> > On 2015/05/08 15:48:22, johnme wrote:
> > > To change the subject (since I think Peter and Michael have covered
> layering),
> > > why does this throw an exception when you access permissionState()? That's
> > > incompatible with developers that base their implementation on the
> > standardized
> > > Push API, and the unexpected exception could break their entire website.
> > > Wouldn't it be better to return "denied" instead (and possibly log a
> > > once-per-pageload message to the console if we want to be helpful)?
> > 
> > Oh, it's rejecting the promise with an exception, rather than throwing one.
> > That's not quite so bad, but it might still cause developers to trip up,
since
> > whilst the spec allows the promise to be rejected, it doesn't list any
> > circumstances under which this would happen, and it's hard to imagine any
good
> > reasons for a UA to do so either.
>
> Yes, it feels more accurate than just denying. That's also what the permission
> API does.

It may be more accurate, but it's not compatible with other UAs, which will
never
reject this promise. Even though the Push spec technically allows it, that's
just a
spec bug, and the spec should disallow rejecting the promise.

This is very different from the Permissions API. That's a generic API to deal
with many
different permissions, and it only rejects if it it's an unknown permission
name.
Here, the presence of the permissionState() method means that by definition it's
a
known permission name (since the permission name is implicit in the
permissionState() API), and we should never reject the promise.

[Michael van Ouwerkerk, 2015-06-23 16:15:56]

mvanouwerkerk@chromium.org changed reviewers:
- mvanouwerkerk@chromium.org

[mlamouri (slow - plz ping), 2015-07-24 13:25:14]

mlamouri@chromium.org changed reviewers:
+ mlamouri@chromium.org

[mlamouri (slow - plz ping), 2015-07-24 13:25:14]

Should this be closed?