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

Issue 1690043004: Use GURLs instead of patterns in SetContentSetting() in geolocation and push_messaging (Closed)

Created:
4 years, 10 months ago by lshang
Modified:
4 years, 9 months ago
CC:
chromium-reviews, mlamouri+watch-geolocation_chromium.org, Peter Beverloo, johnme+watch_chromium.org, mvanouwerkerk+watch_chromium.org, Michael van Ouwerkerk, mlamouri+watch-permissions_chromium.org, harkness+watch_chromium.org, chrome-apps-syd-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@scoping_set_content_setting
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Use GURLs instead of patterns in SetContentSetting() in geolocation and push_messaging Change SetContentSetting() to use SetContentSettingDefaultScope() which takes GURLs instead of patterns as parameters. BUG=551747 Committed: https://crrev.com/4477efb2301f507f2335b1d1920a708008b72050 Cr-Commit-Position: refs/heads/master@{#380037}

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : #

Total comments: 7

Patch Set 4 : addressing reivew comments #

Patch Set 5 : rebase #

Patch Set 6 : remove branch in PushMessagingPermissionContextTest::SetContentSetting() after we add GetPatternsFr… #

Patch Set 7 : rebase #

Patch Set 8 : remove CustomScope due to depending CL update #

Unified diffs Side-by-side diffs Delta from patch set Stats (+62 lines, -87 lines) Patch
M chrome/browser/geolocation/geolocation_browsertest.cc View 1 2 3 4 4 chunks +9 lines, -15 lines 0 comments Download
M chrome/browser/geolocation/geolocation_settings_state_unittest.cc View 1 2 2 chunks +20 lines, -30 lines 0 comments Download
M chrome/browser/push_messaging/push_messaging_browsertest.cc View 1 2 3 4 5 6 7 6 chunks +24 lines, -32 lines 0 comments Download
M chrome/browser/push_messaging/push_messaging_permission_context_unittest.cc View 1 2 3 4 5 1 chunk +9 lines, -10 lines 0 comments Download

Depends on Patchset:

Messages

Total messages: 23 (10 generated)
lshang
PTAL, thanks!
4 years, 10 months ago (2016-02-17 06:20:21 UTC) #4
Michael van Ouwerkerk
https://codereview.chromium.org/1690043004/diff/40001/chrome/browser/geolocation/geolocation_browsertest.cc File chrome/browser/geolocation/geolocation_browsertest.cc (right): https://codereview.chromium.org/1690043004/diff/40001/chrome/browser/geolocation/geolocation_browsertest.cc#newcode29 chrome/browser/geolocation/geolocation_browsertest.cc:29: #include "components/content_settings/core/common/content_settings_pattern.h" nit: delete this https://codereview.chromium.org/1690043004/diff/40001/chrome/browser/push_messaging/push_messaging_permission_context_unittest.cc File chrome/browser/push_messaging/push_messaging_permission_context_unittest.cc (right): ...
4 years, 10 months ago (2016-02-17 11:19:27 UTC) #6
lshang
Thanks for your review, Michael!:-) Could you please PTAL again? https://codereview.chromium.org/1690043004/diff/40001/chrome/browser/geolocation/geolocation_browsertest.cc File chrome/browser/geolocation/geolocation_browsertest.cc (right): https://codereview.chromium.org/1690043004/diff/40001/chrome/browser/geolocation/geolocation_browsertest.cc#newcode29 ...
4 years, 10 months ago (2016-02-18 00:23:53 UTC) #7
Michael van Ouwerkerk
lgtm but please wait for raymes to also take a look https://codereview.chromium.org/1690043004/diff/40001/chrome/browser/push_messaging/push_messaging_permission_context_unittest.cc File chrome/browser/push_messaging/push_messaging_permission_context_unittest.cc (right): ...
4 years, 10 months ago (2016-02-18 09:42:44 UTC) #8
lshang
Raymes, I removed the branch here since we added code to migrate old settings. Can ...
4 years, 9 months ago (2016-03-01 02:55:03 UTC) #9
raymes
lgtm - because these are just tests we don't need to migrate the settings that ...
4 years, 9 months ago (2016-03-01 05:53:47 UTC) #10
lshang
+OWNER johnme@, please review changes in: chrome/browser/push_messaging/push_messaging_browsertest.cc chrome/browser/push_messaging/push_messaging_permission_context_unittest.cc thanks!
4 years, 9 months ago (2016-03-04 00:28:51 UTC) #13
lshang
johnme@, kindly ping:-)
4 years, 9 months ago (2016-03-07 23:39:49 UTC) #14
johnme
lgtm, thanks! push_messaging_browsertest seems unchanged, and I see that this fixes a bug in PushMessagingPermissionContextTest::SetContentSetting, ...
4 years, 9 months ago (2016-03-08 13:17:39 UTC) #15
lshang
On 2016/03/08 13:17:39, johnme wrote: > lgtm, thanks! push_messaging_browsertest seems unchanged, and I see that ...
4 years, 9 months ago (2016-03-08 23:57:23 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1690043004/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1690043004/140001
4 years, 9 months ago (2016-03-09 01:47:05 UTC) #19
commit-bot: I haz the power
Committed patchset #8 (id:140001)
4 years, 9 months ago (2016-03-09 02:35:35 UTC) #21
commit-bot: I haz the power
4 years, 9 months ago (2016-03-09 02:36:44 UTC) #23
Message was sent while issue was closed.
Patchset 8 (id:??) landed as
https://crrev.com/4477efb2301f507f2335b1d1920a708008b72050
Cr-Commit-Position: refs/heads/master@{#380037}

Powered by Google App Engine
This is Rietveld 408576698