Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2915173002:
Return empty Budget API buckets if there is no notification permission (Closed)
|
Created:
3 years, 6 months ago by Peter Beverloo Modified:
3 years, 6 months ago Reviewers:
johnme CC:
chromium-reviews, picksi1 Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionReturn empty Budget API buckets if there is no notification permission
This change only affects the GetBudget() method that we're
experimenting with as part of an Origin Trial.
BUG=710809
Review-Url: https://codereview.chromium.org/2915173002
Cr-Commit-Position: refs/heads/master@{#476804}
Committed: https://chromium.googlesource.com/chromium/src/+/9c993f78366636eb3b4b4f0a3749a52862c96ee5
Patch Set 1 #Patch Set 2 : Return empty Budget API buckets if there is no notification permission #
Total comments: 4
Patch Set 3 : comments #
Messages
Total messages: 23 (16 generated)
The CQ bit was checked by peter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Return empty Budget API buckets if there is no notification permission BUG= ========== to ========== Return empty Budget API buckets if there is no notification permission This change only affects the GetBudget() method that we're experimenting with as part of an Origin Trial. BUG=710809 ==========
The CQ bit was checked by peter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
peter@chromium.org changed reviewers: + johnme@chromium.org
+johnme, PTAL, this is for M60
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2915173002/diff/20001/chrome/browser/budget_s... File chrome/browser/budget_service/budget_service_impl.cc (right): https://codereview.chromium.org/2915173002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_service_impl.cc:56: content::BrowserContext* context = host->GetBrowserContext(); Nit: just do `Profile* profile = Profile::FromBrowserContext(host->GetBrowserContext());` here since we're in chrome/, that fits on one line, and you unconditionally convert it to a Profile* later anyway. https://codereview.chromium.org/2915173002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_service_impl.cc:67: if (permission_manager->GetPermissionStatus( It's encouraged to use PermissionManager::GetPermissionStatusForFrame instead in case feature policy has disabled the feature for a particular frame, though I guess that's moot since this can be accessed from a SW. I'm surprised there isn't a WebFeaturePolicyFeature for disabling Service Workers since they can be used to bypass other feature policy entries :-s In a similar vein, it's a little odd to pass an empty GURL for embedder origin, though I guess for now NotificationPermissionContext mostly ignores that (https://crbug.com/416894).
Thanks John! https://codereview.chromium.org/2915173002/diff/20001/chrome/browser/budget_s... File chrome/browser/budget_service/budget_service_impl.cc (right): https://codereview.chromium.org/2915173002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_service_impl.cc:56: content::BrowserContext* context = host->GetBrowserContext(); On 2017/06/02 16:06:35, johnme wrote: > Nit: just do `Profile* profile = > Profile::FromBrowserContext(host->GetBrowserContext());` here since we're in > chrome/, that fits on one line, and you unconditionally convert it to a Profile* > later anyway. Done. https://codereview.chromium.org/2915173002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_service_impl.cc:67: if (permission_manager->GetPermissionStatus( On 2017/06/02 16:06:35, johnme wrote: > It's encouraged to use PermissionManager::GetPermissionStatusForFrame instead in > case feature policy has disabled the feature for a particular frame, though I > guess that's moot since this can be accessed from a SW. I'm surprised there > isn't a WebFeaturePolicyFeature for disabling Service Workers since they can be > used to bypass other feature policy entries :-s > > In a similar vein, it's a little odd to pass an empty GURL for embedder origin, > though I guess for now NotificationPermissionContext mostly ignores that > (https://crbug.com/416894). This is inherent to how Service Workers work, part unfortunate, part reality. I don't think we'll want to change this?
The CQ bit was checked by peter@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from johnme@chromium.org Link to the patchset: https://codereview.chromium.org/2915173002/#ps40001 (title: "comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by peter@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1496432611537780,
"parent_rev": "15e1023261ee539b7d0037c2bb57596b48ca3ae3", "commit_rev":
"9c993f78366636eb3b4b4f0a3749a52862c96ee5"}
Message was sent while issue was closed.
Description was changed from ========== Return empty Budget API buckets if there is no notification permission This change only affects the GetBudget() method that we're experimenting with as part of an Origin Trial. BUG=710809 ========== to ========== Return empty Budget API buckets if there is no notification permission This change only affects the GetBudget() method that we're experimenting with as part of an Origin Trial. BUG=710809 Review-Url: https://codereview.chromium.org/2915173002 Cr-Commit-Position: refs/heads/master@{#476804} Committed: https://chromium.googlesource.com/chromium/src/+/9c993f78366636eb3b4b4f0a3749... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/9c993f78366636eb3b4b4f0a3749... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
