Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(130)

Issue 893793005: Check if there is a SW controlling the start page for app banner. (Closed)

Created:
5 years, 10 months ago by benwells
Modified:
5 years, 10 months ago
Reviewers:
falken, gone, no sievers
CC:
chrome-apps-syd-reviews_chromium.org, kinuko, nhiroki
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

This exposes a way to see if there is a Service Worker controlling two URLs. This is used for the app banner manager to determine if it should show a banner for the current page. BUG=452825 Committed: https://crrev.com/f23c9142c04fc5d10e9164294d61095ff578b4a8 Cr-Commit-Position: refs/heads/master@{#315024}

Patch Set 1 #

Patch Set 2 : Change interface, use it #

Patch Set 3 : Finish off chrome side #

Patch Set 4 : Tidy up #

Patch Set 5 : Rebase #

Total comments: 3

Patch Set 6 : Android compile #

Total comments: 11

Patch Set 7 : Feedback #

Total comments: 2

Patch Set 8 : Rename #

Patch Set 9 : Updated comment #

Total comments: 6

Patch Set 10 : Rebase and naming / comment tweaks #

Unified diffs Side-by-side diffs Delta from patch set Stats (+117 lines, -14 lines) Patch
M chrome/browser/android/banners/app_banner_manager.h View 1 2 3 4 5 6 7 8 9 3 chunks +10 lines, -0 lines 0 comments Download
M chrome/browser/android/banners/app_banner_manager.cc View 1 2 3 4 5 6 7 8 9 3 chunks +35 lines, -13 lines 0 comments Download
M content/browser/service_worker/service_worker_context_wrapper.h View 1 2 3 4 5 6 7 2 chunks +10 lines, -0 lines 0 comments Download
M content/browser/service_worker/service_worker_context_wrapper.cc View 1 2 3 4 5 6 7 3 chunks +47 lines, -0 lines 0 comments Download
M content/public/browser/service_worker_context.h View 1 2 3 4 5 6 7 8 9 3 chunks +15 lines, -1 line 0 comments Download

Messages

Total messages: 37 (13 generated)
benwells
falken: early review for the SW stuff which I don't think will change. The app ...
5 years, 10 months ago (2015-02-04 09:34:16 UTC) #2
benwells
+dfalcantara for c/b/android/banners
5 years, 10 months ago (2015-02-04 15:20:40 UTC) #4
gone
https://chromiumcodereview.appspot.com/893793005/diff/100001/chrome/browser/android/banners/app_banner_manager.h File chrome/browser/android/banners/app_banner_manager.h (right): https://chromiumcodereview.appspot.com/893793005/diff/100001/chrome/browser/android/banners/app_banner_manager.h#newcode151 chrome/browser/android/banners/app_banner_manager.h:151: // completed. What does "may" mean in this context? ...
5 years, 10 months ago (2015-02-04 17:28:02 UTC) #6
benwells
https://chromiumcodereview.appspot.com/893793005/diff/100001/chrome/browser/android/banners/app_banner_manager.h File chrome/browser/android/banners/app_banner_manager.h (right): https://chromiumcodereview.appspot.com/893793005/diff/100001/chrome/browser/android/banners/app_banner_manager.h#newcode151 chrome/browser/android/banners/app_banner_manager.h:151: // completed. On 2015/02/04 17:28:01, dfalcantara wrote: > What ...
5 years, 10 months ago (2015-02-04 17:35:36 UTC) #7
benwells
On 2015/02/04 17:35:36, benwells wrote: > https://chromiumcodereview.appspot.com/893793005/diff/100001/chrome/browser/android/banners/app_banner_manager.h > File chrome/browser/android/banners/app_banner_manager.h (right): > > https://chromiumcodereview.appspot.com/893793005/diff/100001/chrome/browser/android/banners/app_banner_manager.h#newcode151 > ...
5 years, 10 months ago (2015-02-04 17:36:16 UTC) #8
gone
Banners, lgtm. https://chromiumcodereview.appspot.com/893793005/diff/100001/chrome/browser/android/banners/app_banner_manager.h File chrome/browser/android/banners/app_banner_manager.h (right): https://chromiumcodereview.appspot.com/893793005/diff/100001/chrome/browser/android/banners/app_banner_manager.h#newcode151 chrome/browser/android/banners/app_banner_manager.h:151: // completed. On 2015/02/04 17:35:36, benwells wrote: ...
5 years, 10 months ago (2015-02-04 17:40:10 UTC) #9
benwells
+sievers as content/public owner
5 years, 10 months ago (2015-02-04 17:49:33 UTC) #12
no sievers
https://chromiumcodereview.appspot.com/893793005/diff/120001/content/browser/service_worker/service_worker_context_wrapper.cc File content/browser/service_worker/service_worker_context_wrapper.cc (right): https://chromiumcodereview.appspot.com/893793005/diff/120001/content/browser/service_worker/service_worker_context_wrapper.cc#newcode335 content/browser/service_worker/service_worker_context_wrapper.cc:335: const CheckHasSameServiceWorkerCallback& callback) { This is a bit odd: ...
5 years, 10 months ago (2015-02-04 19:46:05 UTC) #13
benwells
https://codereview.chromium.org/893793005/diff/120001/content/browser/service_worker/service_worker_context_wrapper.cc File content/browser/service_worker/service_worker_context_wrapper.cc (right): https://codereview.chromium.org/893793005/diff/120001/content/browser/service_worker/service_worker_context_wrapper.cc#newcode335 content/browser/service_worker/service_worker_context_wrapper.cc:335: const CheckHasSameServiceWorkerCallback& callback) { On 2015/02/04 19:46:05, sievers wrote: ...
5 years, 10 months ago (2015-02-05 02:09:25 UTC) #14
falken
https://codereview.chromium.org/893793005/diff/120001/content/browser/service_worker/service_worker_context_wrapper.cc File content/browser/service_worker/service_worker_context_wrapper.cc (right): https://codereview.chromium.org/893793005/diff/120001/content/browser/service_worker/service_worker_context_wrapper.cc#newcode295 content/browser/service_worker/service_worker_context_wrapper.cc:295: ServiceWorkerUtils::ScopeMatches(registration->pattern(), other_url); There might be a bug here depending ...
5 years, 10 months ago (2015-02-05 10:26:11 UTC) #15
benwells
https://codereview.chromium.org/893793005/diff/120001/content/browser/service_worker/service_worker_context_wrapper.cc File content/browser/service_worker/service_worker_context_wrapper.cc (right): https://codereview.chromium.org/893793005/diff/120001/content/browser/service_worker/service_worker_context_wrapper.cc#newcode295 content/browser/service_worker/service_worker_context_wrapper.cc:295: ServiceWorkerUtils::ScopeMatches(registration->pattern(), other_url); On 2015/02/05 10:26:11, falken wrote: > There ...
5 years, 10 months ago (2015-02-05 12:38:29 UTC) #16
kinuko
(Reg: callback threading) https://codereview.chromium.org/893793005/diff/120001/content/browser/service_worker/service_worker_context_wrapper.cc File content/browser/service_worker/service_worker_context_wrapper.cc (right): https://codereview.chromium.org/893793005/diff/120001/content/browser/service_worker/service_worker_context_wrapper.cc#newcode335 content/browser/service_worker/service_worker_context_wrapper.cc:335: const CheckHasSameServiceWorkerCallback& callback) { > > ...
5 years, 10 months ago (2015-02-05 14:24:24 UTC) #17
falken
lgtm https://codereview.chromium.org/893793005/diff/140001/content/public/browser/service_worker_context.h File content/public/browser/service_worker_context.h (right): https://codereview.chromium.org/893793005/diff/140001/content/public/browser/service_worker_context.h#newcode92 content/public/browser/service_worker_context.h:92: virtual void CheckHasSameServiceWorker( This name is a bit ...
5 years, 10 months ago (2015-02-05 15:46:46 UTC) #18
benwells
https://codereview.chromium.org/893793005/diff/140001/content/public/browser/service_worker_context.h File content/public/browser/service_worker_context.h (right): https://codereview.chromium.org/893793005/diff/140001/content/public/browser/service_worker_context.h#newcode92 content/public/browser/service_worker_context.h:92: virtual void CheckHasSameServiceWorker( On 2015/02/05 15:46:46, falken wrote: > ...
5 years, 10 months ago (2015-02-05 17:45:08 UTC) #20
no sievers
On 2015/02/05 10:26:11, falken wrote: > > I chatted a bit offline with Ben, I ...
5 years, 10 months ago (2015-02-05 19:47:32 UTC) #21
benwells
On 2015/02/05 19:47:32, sievers wrote: > On 2015/02/05 10:26:11, falken wrote: > > > > ...
5 years, 10 months ago (2015-02-05 20:01:45 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/893793005/180001
5 years, 10 months ago (2015-02-05 20:03:04 UTC) #25
commit-bot: I haz the power
Failed to apply patch for chrome/browser/android/banners/app_banner_manager.cc: While running git apply --index -3 -p1; error: patch ...
5 years, 10 months ago (2015-02-05 21:27:20 UTC) #27
nhiroki
Drive-by-comment: CheckHasServiceWorker() seems to do multiple things and its behavior wouldn't be straightforward. Could you ...
5 years, 10 months ago (2015-02-06 05:36:34 UTC) #30
nhiroki
On 2015/02/05 12:38:29, benwells wrote: > https://codereview.chromium.org/893793005/diff/120001/content/browser/service_worker/service_worker_context_wrapper.cc > File content/browser/service_worker/service_worker_context_wrapper.cc (right): > > https://codereview.chromium.org/893793005/diff/120001/content/browser/service_worker/service_worker_context_wrapper.cc#newcode295 > ...
5 years, 10 months ago (2015-02-06 05:55:07 UTC) #31
benwells
https://codereview.chromium.org/893793005/diff/180001/chrome/browser/android/banners/app_banner_manager.cc File chrome/browser/android/banners/app_banner_manager.cc (right): https://codereview.chromium.org/893793005/diff/180001/chrome/browser/android/banners/app_banner_manager.cc#newcode165 chrome/browser/android/banners/app_banner_manager.cc:165: void AppBannerManager::OnDidCheckHasServiceWorker(bool has_same) { On 2015/02/06 05:36:33, nhiroki (very ...
5 years, 10 months ago (2015-02-06 12:24:59 UTC) #33
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/893793005/200001
5 years, 10 months ago (2015-02-06 12:25:22 UTC) #35
commit-bot: I haz the power
Committed patchset #10 (id:200001)
5 years, 10 months ago (2015-02-06 14:09:12 UTC) #36
commit-bot: I haz the power
5 years, 10 months ago (2015-02-06 14:10:47 UTC) #37
Message was sent while issue was closed.
Patchset 10 (id:??) landed as
https://crrev.com/f23c9142c04fc5d10e9164294d61095ff578b4a8
Cr-Commit-Position: refs/heads/master@{#315024}

Powered by Google App Engine
This is Rietveld 408576698