Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(4302)

Issue 2675293003: Push API: Don't wait for network when unsubscribing (Closed)

Created:
2 years, 7 months ago by johnme
Modified:
2 years, 7 months ago
Reviewers:
Peter Beverloo, jwd
CC:
chromium-reviews, Peter Beverloo, johnme+watch_chromium.org, harkness+watch_chromium.org, asvitkine+watch_chromium.org, zea+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Push API: Don't wait for network when unsubscribing pushSubscription.unsubscribe() used to delete local state then hang for up to 10 minutes until we managed to tell GCM servers about the unsubscription (the 10 minutes is due to exponential backoff retries on desktop, if the device is offline or on a flaky connection). So unsubscribing offline would never resolve the Promise if the tab/Service Worker was closed before 10 minutes, as is common, however the unsubscription would in fact have succeeded to the extent that subscribe()/getSubscription() correctly appear unsubscribed, and messages are no longer be delivered. This patch resolves the unsubscribe() promise as soon as the local state has been deleted, without waiting for network requests to GCM servers. Since we no longer use the results of those network requests directly, UMA logging is added to track them. To allow testing offline behavior, FakeGCMProfileService can now provide a coarse simulation of being offline, and I refactored it a little to simplify things. BUG=669095, 448500 Review-Url: https://codereview.chromium.org/2675293003 Cr-Commit-Position: refs/heads/master@{#449343} Committed: https://chromium.googlesource.com/chromium/src/+/ef71bd0bc3e889890de79958ae65052029d429f1

Patch Set 1 #

Total comments: 18

Patch Set 2 : Address peter's review comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+304 lines, -216 lines) Patch
M chrome/browser/gcm/fake_gcm_profile_service.h View 1 4 chunks +15 lines, -22 lines 0 comments Download
M chrome/browser/gcm/fake_gcm_profile_service.cc View 1 5 chunks +118 lines, -97 lines 0 comments Download
M chrome/browser/push_messaging/push_messaging_browsertest.cc View 15 chunks +53 lines, -11 lines 0 comments Download
M chrome/browser/push_messaging/push_messaging_service_impl.h View 3 chunks +4 lines, -5 lines 0 comments Download
M chrome/browser/push_messaging/push_messaging_service_impl.cc View 4 chunks +47 lines, -68 lines 0 comments Download
M components/gcm_driver/gcm_client.h View 1 2 chunks +5 lines, -1 line 0 comments Download
M components/gcm_driver/gcm_client_impl.cc View 1 2 chunks +9 lines, -5 lines 0 comments Download
M components/gcm_driver/instance_id/instance_id.h View 1 1 chunk +12 lines, -7 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 3 chunks +41 lines, -0 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 16 (8 generated)
johnme
2 years, 7 months ago (2017-02-06 18:34:49 UTC) #3
Peter Beverloo
SGTM, no stamp yet due to unused FailAfterMaxBackoff logic https://codereview.chromium.org/2675293003/diff/1/chrome/browser/gcm/fake_gcm_profile_service.cc File chrome/browser/gcm/fake_gcm_profile_service.cc (right): https://codereview.chromium.org/2675293003/diff/1/chrome/browser/gcm/fake_gcm_profile_service.cc#newcode131 chrome/browser/gcm/fake_gcm_profile_service.cc:131: ...
2 years, 7 months ago (2017-02-07 18:11:08 UTC) #4
johnme
Addressed review comments - thanks :) https://codereview.chromium.org/2675293003/diff/1/chrome/browser/gcm/fake_gcm_profile_service.cc File chrome/browser/gcm/fake_gcm_profile_service.cc (right): https://codereview.chromium.org/2675293003/diff/1/chrome/browser/gcm/fake_gcm_profile_service.cc#newcode131 chrome/browser/gcm/fake_gcm_profile_service.cc:131: base::Bind(&CustomFakeGCMDriver::DoRegister, base::Unretained(this), On ...
2 years, 7 months ago (2017-02-08 14:14:37 UTC) #5
Peter Beverloo
lgtm
2 years, 7 months ago (2017-02-08 15:35:22 UTC) #6
johnme
jwd: please approve added histograms - thanks!
2 years, 7 months ago (2017-02-08 15:40:33 UTC) #10
jwd
lgtm
2 years, 7 months ago (2017-02-09 15:50:30 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2675293003/20001
2 years, 7 months ago (2017-02-09 16:31:36 UTC) #13
commit-bot: I haz the power
2 years, 7 months ago (2017-02-09 17:46:36 UTC) #16
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/ef71bd0bc3e889890de79958ae65...

Powered by Google App Engine
This is Rietveld 408576698