Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2874053003:
Add checks for Feature Policy to the mojo Permission Service (Closed)
|
Created:
3 years, 7 months ago by raymes Modified:
3 years, 6 months ago CC:
chfremer+watch_chromium.org, chrome-apps-syd-reviews_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, mlamouri+watch-permissions_chromium.org, nasko+codewatch_chromium.org, raymes+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd checks for Feature Policy to the mojo Permission Service
This adds Feature Policy checks to the mojo permission service and
corresponding tests. Because multiple permissions can be requested at the same
time, part of a request can be answered synchronously (by feature policy) and
the other part asynchronously (by quering the PermissionManager). Thus we need
to track the partially answered request in the PendingRequest class.
Feature policy checks are done here (in content) as they will form part of the
specifications for various features and thus it is best if other embedders of
content also benefit from these checks.
BUG=689802
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2874053003
Cr-Commit-Position: refs/heads/master@{#475832}
Committed: https://chromium.googlesource.com/chromium/src/+/09bcca49766516c2b8a685420b7b075f39350500
Patch Set 1 #Patch Set 2 : FP #
Total comments: 1
Patch Set 3 : FP #Patch Set 4 : FP #Patch Set 5 : FP #Patch Set 6 : FP #Patch Set 7 : Add checks for Feature Policy to the mojo Permission Service #
Total comments: 12
Patch Set 8 : Add checks for Feature Policy to the mojo Permission Service #
Depends on Patchset: Messages
Total messages: 37 (28 generated)
Description was changed from ========== Add checks for Feature Policy to the mojo Permission Service This adds Feature Policy checks to the mojo permission service and corresponding tests. Although most permissions that have a corresponding feature policy currently go through the Permission Service, these checks happen in the renderer process. So it's also necessary to check the feature policy at the point that features are used in the browser process. But most browser-side checks happen inside chrome/ and we need to ensure that FP checks happen inside content too such that content embedders get them. This is why these checks are implemented here. In the long term, having a separate permission service where these checks lived would be ideal, which would be queried from both chrome/ and content/. BUG=689802 ========== to ========== Add checks for Feature Policy to the mojo Permission Service This adds Feature Policy checks to the mojo permission service and corresponding tests. Although most permissions that have a corresponding feature policy currently go through the Permission Service, these checks happen in the renderer process. So it's also necessary to check the feature policy at the point that features are used in the browser process. But most browser-side checks happen inside chrome/ and we need to ensure that FP checks happen inside content too such that content embedders get them. This is why these checks are implemented here. In the long term, having a separate permission service where these checks lived would be ideal, which would be queried from both chrome/ and content/. BUG=689802 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
raymes@chromium.org changed reviewers: + timloh@chromium.org
ping :)
lgtm https://codereview.chromium.org/2874053003/diff/20001/content/browser/permiss... File content/browser/permissions/permission_service_impl_unittest.cc (right): https://codereview.chromium.org/2874053003/diff/20001/content/browser/permiss... content/browser/permissions/permission_service_impl_unittest.cc:173: EXPECT_EQ(2u, result.size()); Maybe also have a test of a single permission which is denied by feature policy.
The CQ bit was checked by raymes@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...
Description was changed from ========== Add checks for Feature Policy to the mojo Permission Service This adds Feature Policy checks to the mojo permission service and corresponding tests. Although most permissions that have a corresponding feature policy currently go through the Permission Service, these checks happen in the renderer process. So it's also necessary to check the feature policy at the point that features are used in the browser process. But most browser-side checks happen inside chrome/ and we need to ensure that FP checks happen inside content too such that content embedders get them. This is why these checks are implemented here. In the long term, having a separate permission service where these checks lived would be ideal, which would be queried from both chrome/ and content/. BUG=689802 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Add checks for Feature Policy to the mojo Permission Service This adds Feature Policy checks to the mojo permission service and corresponding tests. Because multiple permissions can be requested at the same time, part of a request can be answered synchronously (by feature policy) and the other part asynchronously (by quering the PermissionManager). Thus we need to track the partially answered request in the PendingRequest class. Feature policy checks are done here (in content) as they will form part of the specifications for various features and thus it is best if other embedders of content also benefit from these checks. BUG=689802 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by raymes@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by raymes@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from timloh@chromium.org Link to the patchset: https://codereview.chromium.org/2874053003/#ps100001 (title: "FP")
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2898653004 Patch 60001). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
The CQ bit was checked by raymes@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: This issue passed the CQ dry run.
The CQ bit was checked by raymes@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...
raymes@chromium.org changed reviewers: + mlamouri@chromium.org
mlamouri: PTAL, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2874053003/diff/120001/content/browser/permis... File content/browser/permissions/permission_service_impl.cc (right): https://codereview.chromium.org/2874053003/diff/120001/content/browser/permis... content/browser/permissions/permission_service_impl.cc:140: int id() { return id_; } nit: `const` https://codereview.chromium.org/2874053003/diff/120001/content/browser/permis... content/browser/permissions/permission_service_impl.cc:143: size_t RequestSize() { return types_.size(); } ditto https://codereview.chromium.org/2874053003/diff/120001/content/browser/permis... content/browser/permissions/permission_service_impl.cc:170: std::vector<PermissionStatus> results_; Did you consider: `std::vector<base::Optional<PermissionStatus>>`. I think it would be cleaner because you wouldn't need to keep track of whether things have been set. https://codereview.chromium.org/2874053003/diff/120001/content/browser/permis... content/browser/permissions/permission_service_impl.cc:296: for (size_t i = 0; i < request->RequestSize(); ++i) { Can you add a comment on top of this loop explaining what you are doing. Maybe move the comment inside it too. It took me longer than I wished to understand how you were able to avoid re-writing the same indexes. It can be easy to miss that you are looping on the `request`. https://codereview.chromium.org/2874053003/diff/120001/content/browser/permis... content/browser/permissions/permission_service_impl.cc:304: DCHECK(partial_result.end() == partial_result_it); Would DCHECK_EQ work here? https://codereview.chromium.org/2874053003/diff/120001/content/browser/permis... File content/browser/permissions/permission_service_impl_unittest.cc (right): https://codereview.chromium.org/2874053003/diff/120001/content/browser/permis... content/browser/permissions/permission_service_impl_unittest.cc:38: ~TestPermissionManager() override {} nit: `= default`
The CQ bit was checked by raymes@chromium.org to run a CQ dry run
Thanks Mounir! https://codereview.chromium.org/2874053003/diff/120001/content/browser/permis... File content/browser/permissions/permission_service_impl.cc (right): https://codereview.chromium.org/2874053003/diff/120001/content/browser/permis... content/browser/permissions/permission_service_impl.cc:140: int id() { return id_; } On 2017/05/30 09:38:22, mlamouri wrote: > nit: `const` Done. https://codereview.chromium.org/2874053003/diff/120001/content/browser/permis... content/browser/permissions/permission_service_impl.cc:143: size_t RequestSize() { return types_.size(); } On 2017/05/30 09:38:22, mlamouri wrote: > ditto Done. https://codereview.chromium.org/2874053003/diff/120001/content/browser/permis... content/browser/permissions/permission_service_impl.cc:170: std::vector<PermissionStatus> results_; On 2017/05/30 09:38:22, mlamouri wrote: > Did you consider: > `std::vector<base::Optional<PermissionStatus>>`. I think it would be cleaner > because you wouldn't need to keep track of whether things have been set. Good idea. I tried this but it meant having to transform the array at the end into a non-optional version to run the callback with. That didn't seem ideal, so I just left it as-is. https://codereview.chromium.org/2874053003/diff/120001/content/browser/permis... content/browser/permissions/permission_service_impl.cc:296: for (size_t i = 0; i < request->RequestSize(); ++i) { On 2017/05/30 09:38:22, mlamouri wrote: > Can you add a comment on top of this loop explaining what you are doing. Maybe > move the comment inside it too. It took me longer than I wished to understand > how you were able to avoid re-writing the same indexes. It can be easy to miss > that you are looping on the `request`. Thanks - I tried to make the comment clearer. https://codereview.chromium.org/2874053003/diff/120001/content/browser/permis... content/browser/permissions/permission_service_impl.cc:304: DCHECK(partial_result.end() == partial_result_it); On 2017/05/30 09:38:22, mlamouri wrote: > Would DCHECK_EQ work here? Unfortunately it fails to compile I think because the iterators don't translate to strings: error: no matching function for call to 'MakeCheckOpValueString' https://codereview.chromium.org/2874053003/diff/120001/content/browser/permis... File content/browser/permissions/permission_service_impl_unittest.cc (right): https://codereview.chromium.org/2874053003/diff/120001/content/browser/permis... content/browser/permissions/permission_service_impl_unittest.cc:38: ~TestPermissionManager() override {} On 2017/05/30 09:38:22, mlamouri wrote: > nit: `= default` Done.
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 raymes@chromium.org
The CQ bit was checked by raymes@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from timloh@chromium.org, mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/2874053003/#ps140001 (title: "Add checks for Feature Policy to the mojo Permission Service")
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": 140001, "attempt_start_ts": 1496211607932510,
"parent_rev": "8438d67abe62bc1a7545cc60ec203d3bcee7f5a0", "commit_rev":
"09bcca49766516c2b8a685420b7b075f39350500"}
Message was sent while issue was closed.
Description was changed from ========== Add checks for Feature Policy to the mojo Permission Service This adds Feature Policy checks to the mojo permission service and corresponding tests. Because multiple permissions can be requested at the same time, part of a request can be answered synchronously (by feature policy) and the other part asynchronously (by quering the PermissionManager). Thus we need to track the partially answered request in the PendingRequest class. Feature policy checks are done here (in content) as they will form part of the specifications for various features and thus it is best if other embedders of content also benefit from these checks. BUG=689802 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Add checks for Feature Policy to the mojo Permission Service This adds Feature Policy checks to the mojo permission service and corresponding tests. Because multiple permissions can be requested at the same time, part of a request can be answered synchronously (by feature policy) and the other part asynchronously (by quering the PermissionManager). Thus we need to track the partially answered request in the PendingRequest class. Feature policy checks are done here (in content) as they will form part of the specifications for various features and thus it is best if other embedders of content also benefit from these checks. BUG=689802 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2874053003 Cr-Commit-Position: refs/heads/master@{#475832} Committed: https://chromium.googlesource.com/chromium/src/+/09bcca49766516c2b8a685420b7b... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/09bcca49766516c2b8a685420b7b... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
