Reject the permissionState promise with NotSupported Exception (2/3)

Reject the permissionState promise with NotSupported Exception if userVisibleOnly is false (2/3)

This change starts using the new onError flow and checks if user visible is
supported. It does that by introducing a new method to the PushMessagingService that can be overriden by other vendors if they need to.

1) https://codereview.chromium.org/1130233003/
2) This CL
3) Cleanup CL where the empty onError method is removed and two new Layout tests
   are added so the exception is actually tested

BUG=479246
Committed: https://crrev.com/c03ef37c3c9bd8f8d099c42f5b74963d66db53cc
Cr-Commit-Position: refs/heads/master@{#329655}

[Miguel Garcia, 2015-05-11 11:51:34]

Patchset #1 (id:1) has been deleted

[Miguel Garcia, 2015-05-11 12:00:24]

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

[Miguel Garcia, 2015-05-11 12:00:24]

Depends on https://codereview.chromium.org/1130233003/

[Peter Beverloo, 2015-05-11 12:56:07]

lgtm % comments

https://codereview.chromium.org/1133083002/diff/20001/content/child/push_mess...
File content/child/push_messaging/push_provider.cc (right):

https://codereview.chromium.org/1133083002/diff/20001/content/child/push_mess...
content/child/push_messaging/push_provider.cc:294: "supported.";
Capitalization - it's a sentence.

https://codereview.chromium.org/1133083002/diff/20001/content/child/push_mess...
content/child/push_messaging/push_provider.cc:296: error_message = "";
No need to set it to an empty string - the constructor already implies this.

https://codereview.chromium.org/1133083002/diff/20001/content/public/browser/...
File content/public/browser/push_messaging_service.h (right):

https://codereview.chromium.org/1133083002/diff/20001/content/public/browser/...
content/public/browser/push_messaging_service.h:80: // Returns true if silent
push messages are accepted
Punctuation - it's a sentence. Also, "true" is implied since it's a boolean, and
"silent" in the comment vs. "non visible" in the method name may not be obvious
for the occasional reader.

What about:

  // Returns whether subscriptions that do not mandate user visible UI upon
  // receiving a push message are supported. Influences permission request and
  // permission check behaviour.

https://codereview.chromium.org/1133083002/diff/20001/content/public/browser/...
content/public/browser/push_messaging_service.h:81: virtual bool
SupportNonVisibleMessages();
nit: This should be pure virtual, like the rest of the non-static API.

The overrides should be added in:
 - /chrome/browser/push_messaging/push_messaging_service_impl.h
 - /content/shell/browser/layout_test/layout_test_push_messaging_service.h

[Miguel Garcia, 2015-05-11 13:50:39]

https://codereview.chromium.org/1133083002/diff/20001/content/child/push_mess...
File content/child/push_messaging/push_provider.cc (right):

https://codereview.chromium.org/1133083002/diff/20001/content/child/push_mess...
content/child/push_messaging/push_provider.cc:294: "supported.";
On 2015/05/11 12:56:07, Peter Beverloo wrote:
> Capitalization - it's a sentence.

Done.

https://codereview.chromium.org/1133083002/diff/20001/content/child/push_mess...
content/child/push_messaging/push_provider.cc:296: error_message = "";
On 2015/05/11 12:56:07, Peter Beverloo wrote:
> No need to set it to an empty string - the constructor already implies this.

Done.

https://codereview.chromium.org/1133083002/diff/20001/content/public/browser/...
File content/public/browser/push_messaging_service.h (right):

https://codereview.chromium.org/1133083002/diff/20001/content/public/browser/...
content/public/browser/push_messaging_service.h:80: // Returns true if silent
push messages are accepted
On 2015/05/11 12:56:07, Peter Beverloo wrote:
> Punctuation - it's a sentence. Also, "true" is implied since it's a boolean,
and
> "silent" in the comment vs. "non visible" in the method name may not be
obvious
> for the occasional reader.
> 
> What about:
> 
>   // Returns whether subscriptions that do not mandate user visible UI upon
>   // receiving a push message are supported. Influences permission request and
>   // permission check behaviour.

Done.

https://codereview.chromium.org/1133083002/diff/20001/content/public/browser/...
content/public/browser/push_messaging_service.h:81: virtual bool
SupportNonVisibleMessages();
On 2015/05/11 12:56:07, Peter Beverloo wrote:
> nit: This should be pure virtual, like the rest of the non-static API.
> 
> The overrides should be added in:
>  - /chrome/browser/push_messaging/push_messaging_service_impl.h
>  - /content/shell/browser/layout_test/layout_test_push_messaging_service.h

Done.

[Miguel Garcia, 2015-05-12 08:47:42]

The CQ bit was checked by miguelg@chromium.org

[Miguel Garcia, 2015-05-12 08:47:42]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org
Link to the patchset: https://codereview.chromium.org/1133083002/#ps40001
(title: " ")

[commit-bot: I haz the power, 2015-05-12 08:48:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1133083002/40001

[commit-bot: I haz the power, 2015-05-12 08:54:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-12 08:54:59]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Miguel Garcia, 2015-05-12 09:03:35]

miguelg@chromium.org changed reviewers:
+ jam@chromium.org, mkwst@chromium.org

[Miguel Garcia, 2015-05-12 09:03:36]

+Mike for shell/* files
+jam for content/* files

Thanks!

[Miguel Garcia, 2015-05-12 09:07:00]

On 2015/05/12 09:03:36, Miguel Garcia wrote:
> +Mike for shell/* files
> +jam for content/* files
> 
> Thanks!

Mike, can you also rubber stamp the IPC change for security purposes?

[Mike West, 2015-05-12 09:17:56]

Adding the enum to the IPC LGTM.

[Mike West, 2015-05-12 09:21:26]

On 2015/05/12 at 09:17:56, Mike West wrote:
> Adding the enum to the IPC LGTM.

shell/ LGTM2. Sorry I missed that request.

[Miguel Garcia, 2015-05-12 09:22:58]

Thanks Mike!

On Tue, May 12, 2015 at 10:21 AM, <mkwst@chromium.org> wrote:

> On 2015/05/12 at 09:17:56, Mike West wrote:
>
>> Adding the enum to the IPC LGTM.
>>
>
> shell/ LGTM2. Sorry I missed that request.
>
> https://codereview.chromium.org/1133083002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Miguel Garcia, 2015-05-12 11:15:20]

miguelg@chromium.org changed reviewers:
+ avi@chromium.org
- jam@chromium.org

[Miguel Garcia, 2015-05-12 11:15:21]

I was pointed out that Avi is closer to London time zone wise so swapping
reviewers for content/

[Mike West, 2015-05-12 11:28:18]

Note that there's a PushMessaging test that's failing, and seems related.

[Miguel Garcia, 2015-05-12 11:29:26]

Indeed, I am looking into it.

On Tue, May 12, 2015 at 12:28 PM, <mkwst@chromium.org> wrote:

> Note that there's a PushMessaging test that's failing, and seems related.
>
> https://codereview.chromium.org/1133083002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Miguel Garcia, 2015-05-13 09:22:27]

On 2015/05/12 11:29:26, Miguel Garcia wrote:
> Indeed, I am looking into it.
> 
> On Tue, May 12, 2015 at 12:28 PM, <mailto:mkwst@chromium.org> wrote:
> 
> > Note that there's a PushMessaging test that's failing, and seems related.
> >
> > https://codereview.chromium.org/1133083002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Friendly ping Avi. I just need your LGTM on a one liner in 
content/public/browser/push_messaging_service.h


Thanks

[Miguel Garcia, 2015-05-13 14:57:39]

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

[Miguel Garcia, 2015-05-13 14:57:39]

I found out Avi is in an summit. Jam, could you have a look?

Thanks

[Avi (use Gerrit), 2015-05-13 15:44:33]

lgtm

No, I'm not in a summit, just missed this CL. Please ping me if I don't reply.

https://codereview.chromium.org/1133083002/diff/60001/content/child/push_mess...
File content/child/push_messaging/push_provider.cc (right):

https://codereview.chromium.org/1133083002/diff/60001/content/child/push_mess...
content/child/push_messaging/push_provider.cc:272:
callbacks->onError(web_error.release());
This bothers me because the definition of onError doesn't explain that it takes
ownership of the error. Why doesn't the Blink API use something like OwnPtr to
indicate the semantics here? :(

[Miguel Garcia, 2015-05-13 15:45:24]

On 2015/05/13 15:44:33, Avi wrote:
> lgtm
> 
> No, I'm not in a summit, just missed this CL. Please ping me if I don't reply.
> 
>
https://codereview.chromium.org/1133083002/diff/60001/content/child/push_mess...
> File content/child/push_messaging/push_provider.cc (right):
> 
>
https://codereview.chromium.org/1133083002/diff/60001/content/child/push_mess...
> content/child/push_messaging/push_provider.cc:272:
> callbacks->onError(web_error.release());
> This bothers me because the definition of onError doesn't explain that it
takes
> ownership of the error. Why doesn't the Blink API use something like OwnPtr to
> indicate the semantics here? :(

Thanks Avi!

[Miguel Garcia, 2015-05-13 15:45:28]

The CQ bit was checked by miguelg@chromium.org

[Miguel Garcia, 2015-05-13 15:45:28]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1133083002/#ps60001
(title: "Fix failing test")

[commit-bot: I haz the power, 2015-05-13 15:46:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1133083002/60001

[Miguel Garcia, 2015-05-13 15:50:40]

On 2015/05/13 15:46:31, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1133083002/60001

Oh and I will totally look at changing the blink piece to make this clearer

[commit-bot: I haz the power, 2015-05-13 16:37:23]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2015-05-13 16:38:04]

Patchset 3 (id:??) landed as
https://crrev.com/c03ef37c3c9bd8f8d099c42f5b74963d66db53cc
Cr-Commit-Position: refs/heads/master@{#329655}