Push API: Prevent DCHECK when clear permissions after clear SW DB

Push API: Prevent DCHECK when clear permissions after clear SW DB

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

BUG=464328
Committed: https://crrev.com/6a27f154a404aafa80aef26f733ce429fad38dee
Cr-Commit-Position: refs/heads/master@{#320657}

[johnme, 2015-03-12 20:04:03]

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

[mlamouri (slow - plz ping), 2015-03-13 16:25:49]

https://codereview.chromium.org/990283002/diff/80001/chrome/browser/push_mess...
File chrome/browser/push_messaging/push_messaging_browsertest.cc (right):

https://codereview.chromium.org/990283002/diff/80001/chrome/browser/push_mess...
chrome/browser/push_messaging/push_messaging_browsertest.cc:1060: // DCHECK
occurred (crbug.com/464328).
Are you going to fix that bug with that CL?

Also, please https://crbug.com/...

https://codereview.chromium.org/990283002/diff/80001/chrome/browser/push_mess...
File chrome/browser/push_messaging/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/990283002/diff/80001/chrome/browser/push_mess...
chrome/browser/push_messaging/push_messaging_service_impl.cc:653:
unregister_callback.Run(GCMClient::INVALID_PARAMETER);
Is there any chance you could handle that case where you've added the comment?
Even me with a diff, I wondered why you made that check here. Someone looking at
the entire file would have no clue.

https://codereview.chromium.org/990283002/diff/80001/chrome/browser/push_mess...
chrome/browser/push_messaging/push_messaging_service_impl.cc:749: // are cleared
for the origin (crbug.com/464328). In that case Unregister will
This CL is fixing the bug you list. If someone care enough, that person will
check git blame.

https://codereview.chromium.org/990283002/diff/80001/chrome/browser/push_mess...
chrome/browser/push_messaging/push_messaging_service_impl.cc:751: //
TODO(johnme): Auto-unregister before SW DB is cleared (crbug.com/402458).
https://crbug.com/....

[johnme, 2015-03-13 17:21:12]

Addressed/replied to Mounir's review comments - thanks :)

https://codereview.chromium.org/990283002/diff/80001/chrome/browser/push_mess...
File chrome/browser/push_messaging/push_messaging_browsertest.cc (right):

https://codereview.chromium.org/990283002/diff/80001/chrome/browser/push_mess...
chrome/browser/push_messaging/push_messaging_browsertest.cc:1060: // DCHECK
occurred (crbug.com/464328).
On 2015/03/13 16:25:49, Mounir Lamouri wrote:
> Are you going to fix that bug with that CL?
> 
> Also, please https://crbug.com/...

Yes, this CL fixes that bug. I've changed the comment to:

"
// TODO(johnme): Get this test running on Android, which has a different
// codepath due to sender_id being required for unregistering there."

https://codereview.chromium.org/990283002/diff/80001/chrome/browser/push_mess...
File chrome/browser/push_messaging/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/990283002/diff/80001/chrome/browser/push_mess...
chrome/browser/push_messaging/push_messaging_service_impl.cc:653:
unregister_callback.Run(GCMClient::INVALID_PARAMETER);
On 2015/03/13 16:25:49, Mounir Lamouri wrote:
> Is there any chance you could handle that case where you've added the comment?
> Even me with a diff, I wondered why you made that check here. Someone looking
at
> the entire file would have no clue.

I want to re-use the logic for getting the PMAI, calling DeleteFromDisk, then
calling DecreasePushRegistrationCount. It would be possible to duplicate those
in UnregisterBecausePermissionRevoked, but I'd also have to duplicate the
comments explaining why it's okish to just delete the mapping, which seemed a
bit excessive.

I've added a "// UnregisterBecausePermissionRevoked sometimes calls us with an
empty one." comment here; does that make it clear enough?

https://codereview.chromium.org/990283002/diff/80001/chrome/browser/push_mess...
chrome/browser/push_messaging/push_messaging_service_impl.cc:749: // are cleared
for the origin (crbug.com/464328). In that case Unregister will
On 2015/03/13 16:25:49, Mounir Lamouri wrote:
> This CL is fixing the bug you list. If someone care enough, that person will
> check git blame.

Done.

https://codereview.chromium.org/990283002/diff/80001/chrome/browser/push_mess...
chrome/browser/push_messaging/push_messaging_service_impl.cc:751: //
TODO(johnme): Auto-unregister before SW DB is cleared (crbug.com/402458).
On 2015/03/13 16:25:49, Mounir Lamouri wrote:
> https://crbug.com/....

Done (does your editor linkify links, but only if they start with http(s) ?).

[mlamouri (slow - plz ping), 2015-03-13 17:32:13]

https://codereview.chromium.org/990283002/diff/80001/chrome/browser/push_mess...
File chrome/browser/push_messaging/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/990283002/diff/80001/chrome/browser/push_mess...
chrome/browser/push_messaging/push_messaging_service_impl.cc:653:
unregister_callback.Run(GCMClient::INVALID_PARAMETER);
On 2015/03/13 at 17:21:12, johnme wrote:
> On 2015/03/13 16:25:49, Mounir Lamouri wrote:
> > Is there any chance you could handle that case where you've added the
comment?
> > Even me with a diff, I wondered why you made that check here. Someone
looking at
> > the entire file would have no clue.
> 
> I want to re-use the logic for getting the PMAI, calling DeleteFromDisk, then
calling DecreasePushRegistrationCount. It would be possible to duplicate those
in UnregisterBecausePermissionRevoked, but I'd also have to duplicate the
comments explaining why it's okish to just delete the mapping, which seemed a
bit excessive.
> 
> I've added a "// UnregisterBecausePermissionRevoked sometimes calls us with an
empty one." comment here; does that make it clear enough?

It's clearer but I would suggest to not do that. If you want to re-use that
logic later, move that check there later. As of now, that check has no place
here and moving it when it will be needed will have a low cost. Unless you
already have a CL ready with which this check there makes sense?

https://codereview.chromium.org/990283002/diff/80001/chrome/browser/push_mess...
chrome/browser/push_messaging/push_messaging_service_impl.cc:751: //
TODO(johnme): Auto-unregister before SW DB is cleared (crbug.com/402458).
On 2015/03/13 at 17:21:12, johnme wrote:
> On 2015/03/13 16:25:49, Mounir Lamouri wrote:
> > https://crbug.com/....
> 
> Done (does your editor linkify links, but only if they start with http(s) ?).

I have no idea what my editors would do but "crbug.com/xxxx" isn't really an
URL. The scheme is required so it will be better for everyone to have it (easier
if you use a browser that wouldn't autocomplete and easier if your editor has a
grep using http?://).

[mlamouri (slow - plz ping), 2015-03-13 17:43:00]

Hmm, sorry about that. The code review UI gives me 3 lines of context. I thought
your condition check was some kind of early return. There are actually quite a
lot of lines before...

lgtm

[johnme, 2015-03-13 17:43:54]

The CQ bit was checked by johnme@chromium.org

[commit-bot: I haz the power, 2015-03-13 17:44:15]

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

[commit-bot: I haz the power, 2015-03-13 18:22:14]

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

[commit-bot: I haz the power, 2015-03-13 18:22:15]

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

[johnme, 2015-03-14 14:36:14]

The CQ bit was checked by johnme@chromium.org

[johnme, 2015-03-14 14:36:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/990283002/#ps120001
(title: "Fix typo")

[commit-bot: I haz the power, 2015-03-14 14:36:26]

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

[commit-bot: I haz the power, 2015-03-14 16:42:41]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-03-14 16:43:33]

Patchset 7 (id:??) landed as
https://crrev.com/6a27f154a404aafa80aef26f733ce429fad38dee
Cr-Commit-Position: refs/heads/master@{#320657}