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

Issue 2908133003: Split GetPermissionStatus into GetPermissionStatusForFrame/Worker

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 months, 3 weeks ago by raymes
Modified:
4 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
Trybot results: Sign in to try more bots
Commit queue not available (can’t edit this change).

Messages

Total messages: 60 (38 generated)
raymes
Sorry this looks large, but only because of the changes needed for tests, etc.
4 months, 3 weeks ago (2017-05-30 03:21:54 UTC) #5
raymes
4 months, 3 weeks 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 ...
4 months, 3 weeks 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; ...
4 months, 3 weeks 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 ...
4 months, 2 weeks ago (2017-06-01 02:54:40 UTC) #18
benwells
lgtm
4 months, 2 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 ...
4 months, 2 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 ...
4 months, 2 weeks ago (2017-06-07 06:40:47 UTC) #41
kinuko
lgtm
4 months, 2 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 ...
4 months, 2 weeks ago (2017-06-07 10:15:25 UTC) #45
alokp
chromecast/ lgtm
4 months, 2 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 ...
4 months, 2 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: > ...
4 months, 1 week 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 ...
4 months, 1 week 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" ...
4 months, 1 week ago (2017-06-08 13:35:52 UTC) #53
Sami
headless/ lgtm, thanks for adding the test.
4 months, 1 week ago (2017-06-08 13:54:01 UTC) #54
iclelland
background_sync lgtm
4 months, 1 week 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 ...
4 months, 1 week 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 ...
4 months, 1 week 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: > ...
4 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 ...
4 months ago (2017-06-19 06:55:47 UTC) #59
mlamouri (slow - plz ping)
4 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 :)
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 81bcdb8aa