Unregister with push service and SW database when permission is lost.

Unregister with push service and SW database when permission is lost.

This CL makes the PushService a ContentSettings observer and check
for the subscriber's origin permission when a ContentSettings change
might have changed it.

BUG=443228
Committed: https://crrev.com/8082d50a6306b1b93619a16836579044c562b98e
Cr-Commit-Position: refs/heads/master@{#316799}

[mlamouri (slow - plz ping), 2015-02-16 19:16:01]

mlamouri@chromium.org changed reviewers:
+ avi@chromium.org, johnme@chromium.org

[mlamouri (slow - plz ping), 2015-02-16 19:16:02]

johnme@, could you review changes in:
  chrome/browser/services/gcm/push_messaging_application_id.cc
  chrome/browser/services/gcm/push_messaging_application_id.h
  chrome/browser/services/gcm/push_messaging_browsertest.cc
  chrome/browser/services/gcm/push_messaging_service_impl.cc
  chrome/browser/services/gcm/push_messaging_service_impl.h

avi@, could you review changes in:
  content/public/browser/push_messaging_service.cc
  content/public/browser/push_messaging_service.h

Thanks :)

[johnme, 2015-02-16 20:27:45]

lgtm with nits - thanks :)

https://codereview.chromium.org/930083002/diff/20001/chrome/browser/services/...
File chrome/browser/services/gcm/push_messaging_browsertest.cc (right):

https://codereview.chromium.org/930083002/diff/20001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_browsertest.cc:888: // The Push
servire should not unsubcribe |origin| because at no point it was
servire -> service

https://codereview.chromium.org/930083002/diff/20001/chrome/browser/services/...
File chrome/browser/services/gcm/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/930083002/diff/20001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:135: // In some
tests, we might end up with |profile_| being null at that point.
that -> this

https://codereview.chromium.org/930083002/diff/20001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:265:
Unregister(id.app_id_guid, true /* retry */, UnregisterCallback());;
;; -> ;

https://codereview.chromium.org/930083002/diff/20001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:268:
PushMessagingService::ClearPushRegistrationID(
We should probably also call this from the
PUSH_DELIVERY_STATUS_PERMISSION_DENIED case of DeliverMessageCallback (even
though hopefully that case will be pretty rare after this patch).

https://codereview.chromium.org/930083002/diff/20001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:275:
profile_->GetHostContentSettingsMap()->AddObserver(this);
Maybe add a DCHECK(profile) to emphasize that only valid profiles should be
passed to SetProfileForTesting?

https://codereview.chromium.org/930083002/diff/20001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:665: callback.Run(
2-space indent within if please

https://codereview.chromium.org/930083002/diff/20001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:709:
content::PUSH_UNREGISTRATION_STATUS_SUCCESS_WAS_NOT_REGISTERED);
4-space indent for wrap please

https://codereview.chromium.org/930083002/diff/20001/chrome/test/data/push_me...
File chrome/test/data/push_messaging/push_test.js (right):

https://codereview.chromium.org/930083002/diff/20001/chrome/test/data/push_me...
chrome/test/data/push_messaging/push_test.js:144: subscription ?
sendResultToTest('true - registered')
    sendResultToTest(subscription ? 'true - registered'
                                  : 'false - not registered');

https://codereview.chromium.org/930083002/diff/20001/content/public/browser/p...
File content/public/browser/push_messaging_service.cc (right):

https://codereview.chromium.org/930083002/diff/20001/content/public/browser/p...
content/public/browser/push_messaging_service.cc:19: "push_registration_id";
Please add an extern declaration for the one in
content/browser/push_messaging/push_messaging_message_filter.cc in
content/browser/push_messaging/push_messaging_message_filter.h, and #include
that file here (DEPS allows that) instead of redefining this.

https://codereview.chromium.org/930083002/diff/20001/content/public/browser/p...
content/public/browser/push_messaging_service.cc:68:
scoped_refptr<ServiceWorkerContextWrapper> service_worker_context) {
Nit: Please swap the argument order, for consistency with other methods.

[mlamouri (slow - plz ping), 2015-02-16 20:45:12]

New patchsets have been uploaded after l-g-t-m from johnme@chromium.org

[mlamouri (slow - plz ping), 2015-02-16 20:46:03]

Comments applied.

avi@, could you PTAL at the content/public/ changes?

https://codereview.chromium.org/930083002/diff/20001/chrome/browser/services/...
File chrome/browser/services/gcm/push_messaging_browsertest.cc (right):

https://codereview.chromium.org/930083002/diff/20001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_browsertest.cc:888: // The Push
servire should not unsubcribe |origin| because at no point it was
On 2015/02/16 at 20:27:44, johnme wrote:
> servire -> service

Done.

https://codereview.chromium.org/930083002/diff/20001/chrome/browser/services/...
File chrome/browser/services/gcm/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/930083002/diff/20001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:135: // In some
tests, we might end up with |profile_| being null at that point.
On 2015/02/16 at 20:27:44, johnme wrote:
> that -> this

Done.

https://codereview.chromium.org/930083002/diff/20001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:265:
Unregister(id.app_id_guid, true /* retry */, UnregisterCallback());;
On 2015/02/16 at 20:27:45, johnme wrote:
> ;; -> ;

Surprised this didn't trigger a warning.

https://codereview.chromium.org/930083002/diff/20001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:268:
PushMessagingService::ClearPushRegistrationID(
On 2015/02/16 at 20:27:45, johnme wrote:
> We should probably also call this from the
PUSH_DELIVERY_STATUS_PERMISSION_DENIED case of DeliverMessageCallback (even
though hopefully that case will be pretty rare after this patch).

I think we should consider having the service doing the PushRegistrationID
clearance. I do not think it's part of that patch though.

https://codereview.chromium.org/930083002/diff/20001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:275:
profile_->GetHostContentSettingsMap()->AddObserver(this);
On 2015/02/16 at 20:27:45, johnme wrote:
> Maybe add a DCHECK(profile) to emphasize that only valid profiles should be
passed to SetProfileForTesting?

Not very useful given that it will crash anyway and this is testing code.

https://codereview.chromium.org/930083002/diff/20001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:665: callback.Run(
On 2015/02/16 at 20:27:45, johnme wrote:
> 2-space indent within if please

Done.

https://codereview.chromium.org/930083002/diff/20001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:709:
content::PUSH_UNREGISTRATION_STATUS_SUCCESS_WAS_NOT_REGISTERED);
On 2015/02/16 at 20:27:45, johnme wrote:
> 4-space indent for wrap please

Done.

https://codereview.chromium.org/930083002/diff/20001/chrome/test/data/push_me...
File chrome/test/data/push_messaging/push_test.js (right):

https://codereview.chromium.org/930083002/diff/20001/chrome/test/data/push_me...
chrome/test/data/push_messaging/push_test.js:144: subscription ?
sendResultToTest('true - registered')
On 2015/02/16 at 20:27:45, johnme wrote:
> sendResultToTest(subscription ? 'true - registered'
>                                   : 'false - not registered');

Done.

https://codereview.chromium.org/930083002/diff/20001/content/public/browser/p...
File content/public/browser/push_messaging_service.cc (right):

https://codereview.chromium.org/930083002/diff/20001/content/public/browser/p...
content/public/browser/push_messaging_service.cc:19: "push_registration_id";
On 2015/02/16 at 20:27:45, johnme wrote:
> Please add an extern declaration for the one in
content/browser/push_messaging/push_messaging_message_filter.cc in
content/browser/push_messaging/push_messaging_message_filter.h, and #include
that file here (DEPS allows that) instead of redefining this.

Done.

https://codereview.chromium.org/930083002/diff/20001/content/public/browser/p...
content/public/browser/push_messaging_service.cc:68:
scoped_refptr<ServiceWorkerContextWrapper> service_worker_context) {
On 2015/02/16 at 20:27:45, johnme wrote:
> Nit: Please swap the argument order, for consistency with other methods.

I guess PushMessagingRouter isn't consistent with this code ;) (I started
writing that code there.)

Done.

[mlamouri (slow - plz ping), 2015-02-16 20:46:45]

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

[mlamouri (slow - plz ping), 2015-02-16 20:46:46]

+bauerb@ for push_messaging_service_impl.cc and the tests changes given the use
of ContentSettings.

[mlamouri (slow - plz ping), 2015-02-16 20:46:58]

mlamouri@chromium.org changed required reviewers:
+ avi@chromium.org, johnme@chromium.org

[Bernhard Bauer, 2015-02-16 22:41:05]

https://codereview.chromium.org/930083002/diff/60001/chrome/browser/services/...
File chrome/browser/services/gcm/push_messaging_browsertest.cc (right):

https://codereview.chromium.org/930083002/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_browsertest.cc:720: GURL origin =
https_server()->GetURL("").GetOrigin();
Use an empty std::string() constructor instead of a string literal; it saves
some instructions.

https://codereview.chromium.org/930083002/diff/60001/chrome/browser/services/...
File chrome/browser/services/gcm/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/930083002/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:259: //
|primary_pattern| might be invalid or it does matches the origin, thus
This sentence doesn't parse.

[mlamouri (slow - plz ping), 2015-02-16 23:34:36]

bauerb@ review comments applied.

https://codereview.chromium.org/930083002/diff/60001/chrome/browser/services/...
File chrome/browser/services/gcm/push_messaging_browsertest.cc (right):

https://codereview.chromium.org/930083002/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_browsertest.cc:720: GURL origin =
https_server()->GetURL("").GetOrigin();
On 2015/02/16 at 22:41:05, Bernhard Bauer wrote:
> Use an empty std::string() constructor instead of a string literal; it saves
some instructions.

Done.

https://codereview.chromium.org/930083002/diff/60001/chrome/browser/services/...
File chrome/browser/services/gcm/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/930083002/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:259: //
|primary_pattern| might be invalid or it does matches the origin, thus
On 2015/02/16 at 22:41:05, Bernhard Bauer wrote:
> This sentence doesn't parse.

Wrote something else.

[Bernhard Bauer, 2015-02-17 08:46:08]

Thanks, LGTM!

[mlamouri (slow - plz ping), 2015-02-17 15:28:58]

New patchsets have been uploaded after l-g-t-m from bauerb@chromium.org

[Avi (use Gerrit), 2015-02-17 16:21:38]

lgtm

[mlamouri (slow - plz ping), 2015-02-17 16:50:52]

New patchsets have been uploaded after l-g-t-m from avi@chromium.org

[mlamouri (slow - plz ping), 2015-02-17 16:51:40]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2015-02-17 16:52:26]

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

[commit-bot: I haz the power, 2015-02-17 17:57:33]

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

[commit-bot: I haz the power, 2015-02-17 17:57:35]

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

[mlamouri (slow - plz ping), 2015-02-18 00:37:59]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2015-02-18 00:39:29]

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

[commit-bot: I haz the power, 2015-02-18 00:51:00]

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

[commit-bot: I haz the power, 2015-02-18 00:51:01]

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

[mlamouri (slow - plz ping), 2015-02-18 10:20:32]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2015-02-18 10:21:00]

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

[commit-bot: I haz the power, 2015-02-18 11:20:25]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-02-18 11:21:05]

Patchset 8 (id:??) landed as
https://crrev.com/8082d50a6306b1b93619a16836579044c562b98e
Cr-Commit-Position: refs/heads/master@{#316799}