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

Issue 2908133003: Split GetPermissionStatus into GetPermissionStatusForFrame/Worker

Created:
11 months ago by raymes
Modified:
10 months, 1 week ago
CC:
awdf+watch_chromium.org, chasej+watch_chromium.org, chrome-apps-syd-reviews_chromium.org, chromium-reviews, darin-cc_chromium.org, einbinder+watch-test-runner_chromium.org, iclelland+watch_chromium.org, jam, jkarlin+watch_chromium.org, jochen+watch_chromium.org, johnme+watch_chromium.org, mcgreevy, mlamouri+watch-notifications_chromium.org, mlamouri+watch-content_chromium.org, mlamouri+watch-test-runner_chromium.org, mlamouri+watch-permissions_chromium.org, Peter Beverloo, raymes+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Split GetPermissionStatus into GetPermissionStatusForFrame/Worker Currently we only expose PermissionManager::GetPermissionStatus to content, which returns the permission status for a given origin/embedding origin pair. However, when a request is coming from a frame, the RenderFrameHost is needed to do certain permission checks (e.g. checking the Feature Policy and checking Android permissions, crbug.com/703498). In other cases, the request is coming from a worker, which does not have a frame associated with it. This CL splits GetPermissionStatus into GetPermissionStatusForFrame, GetPermissionStatusForWorker. The former takes a RenderFrameHost as an argument while the latter just takes an origin. BUG=703498

Patch Set 1 #

Patch Set 2 : Split GetPermissionStatus into GetPermissionStatusForFrame/Worker #

Patch Set 3 : Split GetPermissionStatus into GetPermissionStatusForFrame/Worker #

Patch Set 4 : Split GetPermissionStatus into GetPermissionStatusForFrame/Worker #

Patch Set 5 : Split GetPermissionStatus into GetPermissionStatusForFrame/Worker #

Patch Set 6 : Split GetPermissionStatus into GetPermissionStatusForFrame/Worker #

Total comments: 19

Patch Set 7 : Split GetPermissionStatus into GetPermissionStatusForFrame/Worker #

Patch Set 8 : Split GetPermissionStatus into GetPermissionStatusForFrame/Worker #

Total comments: 5

Patch Set 9 : Split GetPermissionStatus into GetPermissionStatusForFrame/Worker #

Patch Set 10 : Split GetPermissionStatus into GetPermissionStatusForFrame/Worker #

Total comments: 9
Unified diffs Side-by-side diffs Delta from patch set Stats (+350 lines, -191 lines) Patch
M android_webview/browser/aw_permission_manager.h View 1 2 3 4 5 1 chunk +6 lines, -3 lines 0 comments Download
M android_webview/browser/aw_permission_manager.cc View 1 2 3 4 5 6 2 chunks +15 lines, -3 lines 0 comments Download
M chrome/browser/budget_service/budget_service_impl.cc View 1 2 3 4 5 6 7 2 chunks +5 lines, -3 lines 1 comment Download
M chrome/browser/geolocation/geolocation_permission_context_unittest.cc View 1 2 3 4 5 14 chunks +80 lines, -72 lines 0 comments Download
M chrome/browser/permissions/permission_manager.h View 1 chunk +6 lines, -3 lines 1 comment Download
M chrome/browser/permissions/permission_manager.cc View 1 2 3 4 5 6 7 2 chunks +32 lines, -9 lines 1 comment Download
M chrome/browser/permissions/permission_manager_unittest.cc View 1 2 3 4 5 6 7 8 5 chunks +31 lines, -10 lines 0 comments Download
M chromecast/browser/cast_permission_manager.h View 1 2 3 4 1 chunk +6 lines, -3 lines 0 comments Download
M chromecast/browser/cast_permission_manager.cc View 1 2 3 4 1 chunk +11 lines, -3 lines 0 comments Download
M content/browser/background_sync/background_sync_manager.cc View 1 chunk +2 lines, -2 lines 1 comment Download
M content/browser/background_sync/background_sync_manager_unittest.cc View 3 chunks +5 lines, -5 lines 0 comments Download
M content/browser/background_sync/background_sync_service_impl_unittest.cc View 1 chunk +1 line, -1 line 0 comments Download
M content/browser/permissions/permission_service_impl.cc View 1 2 3 4 5 6 1 chunk +7 lines, -5 lines 0 comments Download
M content/browser/permissions/permission_service_impl_unittest.cc View 1 2 3 4 5 6 2 chunks +5 lines, -4 lines 0 comments Download
M content/public/browser/permission_manager.h View 1 chunk +12 lines, -7 lines 5 comments Download
M content/shell/browser/layout_test/layout_test_notification_manager.cc View 1 2 3 4 5 6 1 chunk +4 lines, -3 lines 0 comments Download
M content/shell/browser/layout_test/layout_test_permission_manager.h View 2 chunks +12 lines, -4 lines 0 comments Download
M content/shell/browser/layout_test/layout_test_permission_manager.cc View 2 chunks +30 lines, -12 lines 0 comments Download
M content/shell/browser/layout_test/layout_test_push_messaging_service.h View 1 chunk +3 lines, -3 lines 0 comments Download
M content/shell/browser/layout_test/layout_test_push_messaging_service.cc View 1 2 3 4 5 6 1 chunk +9 lines, -4 lines 0 comments Download
M content/shell/browser/shell_permission_manager.h View 1 chunk +7 lines, -4 lines 0 comments Download
M content/shell/browser/shell_permission_manager.cc View 1 2 3 4 5 6 2 chunks +28 lines, -16 lines 0 comments Download
M content/test/mock_permission_manager.h View 1 chunk +7 lines, -3 lines 0 comments Download
M headless/lib/browser/headless_permission_manager.h View 1 chunk +6 lines, -3 lines 0 comments Download
M headless/lib/browser/headless_permission_manager.cc View 1 2 3 4 5 6 7 8 9 1 chunk +12 lines, -3 lines 0 comments Download
M headless/lib/headless_browser_browsertest.cc View 1 chunk +8 lines, -3 lines 0 comments Download

Messages

Total messages: 60 (38 generated)
raymes
Sorry this looks large, but only because of the changes needed for tests, etc.
11 months ago (2017-05-30 03:21:54 UTC) #5
raymes
11 months ago (2017-05-30 03:37:51 UTC) #6
benwells
We should chat about this tomorrow. After seeing this in practice I'm unsure of it ...
11 months ago (2017-05-30 06:49:13 UTC) #13
benwells
Thanks for chatting offline with me :) https://codereview.chromium.org/2908133003/diff/90001/android_webview/browser/aw_permission_manager.cc File android_webview/browser/aw_permission_manager.cc (right): https://codereview.chromium.org/2908133003/diff/90001/android_webview/browser/aw_permission_manager.cc#newcode513 android_webview/browser/aw_permission_manager.cc:513: return PermissionStatus::DENIED; ...
11 months ago (2017-05-31 01:48:10 UTC) #16
raymes
https://codereview.chromium.org/2908133003/diff/90001/android_webview/browser/aw_permission_manager.cc File android_webview/browser/aw_permission_manager.cc (right): https://codereview.chromium.org/2908133003/diff/90001/android_webview/browser/aw_permission_manager.cc#newcode513 android_webview/browser/aw_permission_manager.cc:513: return PermissionStatus::DENIED; On 2017/05/31 01:48:10, benwells wrote: > On ...
10 months, 4 weeks ago (2017-06-01 02:54:40 UTC) #18
benwells
lgtm
10 months, 4 weeks ago (2017-06-01 03:34:23 UTC) #24
raymes
+OWNERS boliu: android_webview johnme: chrome/browser/budget_service alokp: chromecast iclelland: content/browser/background_sync mlamouri: content/browser/permissions kinuko: content/public/browser/permission_manager.h and content/test/mock_permission_manager.h ...
10 months, 3 weeks ago (2017-06-07 05:47:09 UTC) #38
Mike West
https://codereview.chromium.org/2908133003/diff/210001/content/shell/browser/shell_permission_manager.cc File content/shell/browser/shell_permission_manager.cc (right): https://codereview.chromium.org/2908133003/diff/210001/content/shell/browser/shell_permission_manager.cc#newcode25 content/shell/browser/shell_permission_manager.cc:25: // tests. Could you add the default grant to ...
10 months, 3 weeks ago (2017-06-07 06:40:47 UTC) #41
kinuko
lgtm
10 months, 3 weeks ago (2017-06-07 07:25:34 UTC) #44
Sami
https://codereview.chromium.org/2908133003/diff/210001/headless/lib/browser/headless_permission_manager.cc File headless/lib/browser/headless_permission_manager.cc (right): https://codereview.chromium.org/2908133003/diff/210001/headless/lib/browser/headless_permission_manager.cc#newcode62 headless/lib/browser/headless_permission_manager.cc:62: return blink::mojom::PermissionStatus::DENIED; Could you explain why this is DENIED ...
10 months, 3 weeks ago (2017-06-07 10:15:25 UTC) #45
alokp
chromecast/ lgtm
10 months, 3 weeks ago (2017-06-07 16:54:00 UTC) #46
boliu
On 2017/06/07 05:47:09, raymes wrote: > +OWNERS > > boliu: android_webview lgtm > johnme: chrome/browser/budget_service ...
10 months, 3 weeks ago (2017-06-07 17:02:55 UTC) #47
raymes
https://codereview.chromium.org/2908133003/diff/210001/content/shell/browser/shell_permission_manager.cc File content/shell/browser/shell_permission_manager.cc (right): https://codereview.chromium.org/2908133003/diff/210001/content/shell/browser/shell_permission_manager.cc#newcode25 content/shell/browser/shell_permission_manager.cc:25: // tests. On 2017/06/07 06:40:47, Mike West wrote: > ...
10 months, 3 weeks ago (2017-06-07 23:03:35 UTC) #51
Mike West
On 2017/06/07 at 23:03:35, raymes wrote: > https://codereview.chromium.org/2908133003/diff/210001/content/shell/browser/shell_permission_manager.cc > File content/shell/browser/shell_permission_manager.cc (right): > > https://codereview.chromium.org/2908133003/diff/210001/content/shell/browser/shell_permission_manager.cc#newcode25 ...
10 months, 3 weeks ago (2017-06-08 04:58:37 UTC) #52
mlamouri (slow - plz ping)
https://codereview.chromium.org/2908133003/diff/250001/content/public/browser/permission_manager.h File content/public/browser/permission_manager.h (right): https://codereview.chromium.org/2908133003/diff/250001/content/public/browser/permission_manager.h#newcode67 content/public/browser/permission_manager.h:67: virtual blink::mojom::PermissionStatus GetPermissionStatusForFrame( Can we drop "ForFrame" and "ForWorkers" ...
10 months, 3 weeks ago (2017-06-08 13:35:52 UTC) #53
Sami
headless/ lgtm, thanks for adding the test.
10 months, 3 weeks ago (2017-06-08 13:54:01 UTC) #54
iclelland
background_sync lgtm
10 months, 3 weeks ago (2017-06-08 13:57:14 UTC) #55
raymes
Thanks Mounir https://codereview.chromium.org/2908133003/diff/250001/content/public/browser/permission_manager.h File content/public/browser/permission_manager.h (right): https://codereview.chromium.org/2908133003/diff/250001/content/public/browser/permission_manager.h#newcode67 content/public/browser/permission_manager.h:67: virtual blink::mojom::PermissionStatus GetPermissionStatusForFrame( On 2017/06/08 13:35:52, mlamouri ...
10 months, 2 weeks ago (2017-06-12 03:24:17 UTC) #56
johnme
Sorry, I was OOO last week. Got a few questions about this. https://codereview.chromium.org/2908133003/diff/250001/chrome/browser/budget_service/budget_service_impl.cc File chrome/browser/budget_service/budget_service_impl.cc ...
10 months, 2 weeks ago (2017-06-12 10:18:20 UTC) #57
mlamouri (slow - plz ping)
https://codereview.chromium.org/2908133003/diff/210001/headless/lib/browser/headless_permission_manager.cc File headless/lib/browser/headless_permission_manager.cc (right): https://codereview.chromium.org/2908133003/diff/210001/headless/lib/browser/headless_permission_manager.cc#newcode62 headless/lib/browser/headless_permission_manager.cc:62: return blink::mojom::PermissionStatus::DENIED; On 2017/06/07 at 23:03:35, raymes wrote: > ...
10 months, 2 weeks ago (2017-06-15 13:02:08 UTC) #58
raymes
mlamouri, johnme: thanks for the feedback! I think these are valid concerns so I'm going ...
10 months, 1 week ago (2017-06-19 06:55:47 UTC) #59
mlamouri (slow - plz ping)
10 months, 1 week ago (2017-06-19 12:28:18 UTC) #60
On 2017/06/19 at 06:55:47, raymes wrote:
> mlamouri, johnme: thanks for the feedback! I think these are valid concerns so
I'm going to avoid the ForWorker method for now and just have a
GetPermissionStatusForFrame and GetPermissionStatus (I haven't had a chance to
make the changes to the CL yet).

Thanks for addressing the feedback. I'm not sure where the chromium style guide
stands on this but if overloading is something we can do, I would recommend
having the same name for the methods and clearly say in the documentation (ie.
header) which one should be used (ie. use the one with RFH when possible).
Again, this is bikeshed, nitpick so feel free to ignore :)

Powered by Google App Engine
This is Rietveld 408576698