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

Issue 2908133003: Split GetPermissionStatus into GetPermissionStatusForFrame/Worker

Created:
3 years, 6 months ago by raymes
Modified:
3 years, 6 months 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.
3 years, 6 months ago (2017-05-30 03:21:54 UTC) #5
raymes
3 years, 6 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 ...
3 years, 6 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; ...
3 years, 6 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 ...
3 years, 6 months ago (2017-06-01 02:54:40 UTC) #18
benwells
lgtm
3 years, 6 months 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 ...
3 years, 6 months 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 ...
3 years, 6 months ago (2017-06-07 06:40:47 UTC) #41
kinuko
lgtm
3 years, 6 months 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 ...
3 years, 6 months ago (2017-06-07 10:15:25 UTC) #45
alokp
chromecast/ lgtm
3 years, 6 months 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 ...
3 years, 6 months 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: > ...
3 years, 6 months 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 ...
3 years, 6 months 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" ...
3 years, 6 months ago (2017-06-08 13:35:52 UTC) #53
Sami
headless/ lgtm, thanks for adding the test.
3 years, 6 months ago (2017-06-08 13:54:01 UTC) #54
iclelland
background_sync lgtm
3 years, 6 months 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 ...
3 years, 6 months 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 ...
3 years, 6 months 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: > ...
3 years, 6 months 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 ...
3 years, 6 months ago (2017-06-19 06:55:47 UTC) #59
mlamouri (slow - plz ping)
3 years, 6 months 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