Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2655663004:
Introduce content-side Feature Policy object and maintain in parallel with renderer policy. (Closed)
|
Created:
3 years, 10 months ago by iclelland Modified:
3 years, 9 months ago CC:
chromium-reviews, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, dglazkov+blink, darin-cc_chromium.org, blink-reviews, blink-reviews-api_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntroduce content-side Feature Policy object and maintain in parallel with renderer policy.
This is part 3 of 5 in the effort to move the FeaturePolicy implementation
into the content layer, which will facilitate use of the framework for browser-
based policy decisions.
See the other CLs in this series:
[1/5] https://codereview.chromium.org/2648423002/ (Rename classes)
[2/5] https://codereview.chromium.org/2654873004/ (Centralize content-side code)
[3/5] (This CL) (Maintain parallel FP in browser)
[4/5] https://codereview.chromium.org/2651883008/ (Use content/ FP in blink)
[5/5] https://codereview.chromium.org/2656533004/ (Remove unused blink code.)
BUG=685195
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2655663004
Cr-Commit-Position: refs/heads/master@{#449444}
Committed: https://chromium.googlesource.com/chromium/src/+/2c79efe295ea0cd234b3e13b44369fdba046280e
Patch Set 1 #Patch Set 2 : Add DISALLOW_COPY_AND_ASSIGN macro to FP class #Patch Set 3 : Rebase #
Total comments: 35
Patch Set 4 : Addressing review comments #
Total comments: 5
Patch Set 5 : Move FeaturePolicy to RenderFrameHostImpl #
Total comments: 8
Patch Set 6 : Simplify FP mirroring logic; move most of it to RFHI #
Total comments: 23
Patch Set 7 : Responding to review comments #
Total comments: 6
Patch Set 8 : Review nits #
Total comments: 2
Patch Set 9 : Add comments for public enum members #Messages
Total messages: 64 (43 generated)
Description was changed from ========== Introduce content-side FP object and maintain in parallel with renderer FP BUG= ========== to ========== Introduce content-side FP object and maintain in parallel with renderer FP BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by iclelland@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: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by iclelland@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 ========== Introduce content-side FP object and maintain in parallel with renderer FP BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Introduce content-side Feature Policy object and maintain in parallel with renderer policy. This is part 3 of 5 in the effort to move the FeaturePolicy implementation into the content layer, which will facilitate use of the framework for browser- based policy decisions. See the other CLs in this series: [1/5] https://codereview.chromium.org/2648423002/ (Rename classes) [2/5] https://codereview.chromium.org/2654873004/ (Centralize content-side code) [3/5] (This CL) (Maintain parallel FP in browser) [4/5] https://codereview.chromium.org/2651883008/ (Use content/ FP in blink) [5/5] https://codereview.chromium.org/2656533004/ (Remove unused blink code.) BUG=685195 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
iclelland@chromium.org changed reviewers: + raymes@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
+r raymes, PTAL -- this is starting to get to the meat of the change now :)
The CQ bit was checked by iclelland@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.
lgtm with a bunch of nits. Thanks! https://codereview.chromium.org/2655663004/diff/40001/content/browser/frame_h... File content/browser/frame_host/frame_tree_node.cc (right): https://codereview.chromium.org/2655663004/diff/40001/content/browser/frame_h... content/browser/frame_host/frame_tree_node.cc:274: } optional: alternatively: FeaturePolicy* parent_policy = parent() ? parent()->feature_policy_.get() : nullptr; ... https://codereview.chromium.org/2655663004/diff/40001/content/browser/frame_h... File content/browser/frame_host/frame_tree_node.h (right): https://codereview.chromium.org/2655663004/diff/40001/content/browser/frame_h... content/browser/frame_host/frame_tree_node.h:20: #include "content/common/feature_policy/feature_policy.h" nit: I think we can forward declare this instead? https://codereview.chromium.org/2655663004/diff/40001/content/common/feature_... File content/common/feature_policy/feature_policy.cc (right): https://codereview.chromium.org/2655663004/diff/40001/content/common/feature_... content/common/feature_policy/feature_policy.cc:56: // Extracts a Whitelist from a ParsedFeaturePolicyDeclaration nit: . at end of sentence. https://codereview.chromium.org/2655663004/diff/40001/content/common/feature_... content/common/feature_policy/feature_policy.cc:86: FeaturePolicy::Whitelist::Whitelist() : matches_all_origins_(false) {} nit: newline after https://codereview.chromium.org/2655663004/diff/40001/content/common/feature_... content/common/feature_policy/feature_policy.cc:117: DCHECK(feature_list_.count(feature)); Instead of count we tend to use ContainsKey from base/stl_util.h https://codereview.chromium.org/2655663004/diff/40001/content/common/feature_... content/common/feature_policy/feature_policy.cc:120: if (!inherited_policies_.at(feature)) { nit: (here and below) We tend to use [] instead of at (since we don't have exceptions in Chrome anyway). https://codereview.chromium.org/2655663004/diff/40001/content/common/feature_... content/common/feature_policy/feature_policy.cc:123: if (whitelists_.count(feature)) { nit: in this case it's probably slightly better to use find() and then you can reuse the iterator to check what the value is. https://codereview.chromium.org/2655663004/diff/40001/content/common/feature_... content/common/feature_policy/feature_policy.cc:157: : origin_(origin), feature_list_(feature_list) {} nit: newline https://codereview.chromium.org/2655663004/diff/40001/content/common/feature_... content/common/feature_policy/feature_policy.cc:159: : origin_(origin), feature_list_(getDefaultFeatureList()) {} nit: newline https://codereview.chromium.org/2655663004/diff/40001/content/common/feature_... content/common/feature_policy/feature_policy.cc:182: // TODO: See if this should use lazy_instance instead nit: remove this, the way you're doing it is probably more appropriate here :) https://codereview.chromium.org/2655663004/diff/40001/content/common/feature_... File content/common/feature_policy/feature_policy.h (right): https://codereview.chromium.org/2655663004/diff/40001/content/common/feature_... content/common/feature_policy/feature_policy.h:82: // NOTE: These types are used for replication frame state between processes. nit: replication->replicating https://codereview.chromium.org/2655663004/diff/40001/content/common/feature_... content/common/feature_policy/feature_policy.h:83: // They exist only because we can't transfer WebVectors directly over IPC. nit: this comment is probably wrong now https://codereview.chromium.org/2655663004/diff/40001/content/common/feature_... content/common/feature_policy/feature_policy.h:105: class Whitelist final { It's a bit unfortunate that ParsedFeaturePolicyDeclaration is so similar to Whitelist but I don't see a strictly better way to share code, since the struct is used for IPC :( https://codereview.chromium.org/2655663004/diff/40001/content/common/feature_... content/common/feature_policy/feature_policy.h:162: url::Origin origin); nit: can probably use a const reference here https://codereview.chromium.org/2655663004/diff/40001/content/common/feature_... content/common/feature_policy/feature_policy.h:166: url::Origin origin) const; nit: can probably use a const reference here https://codereview.chromium.org/2655663004/diff/40001/content/common/feature_... File content/common/feature_policy/feature_policy_unittest.cc (right): https://codereview.chromium.org/2655663004/diff/40001/content/common/feature_... content/common/feature_policy/feature_policy_unittest.cc:13: #define ORIGIN_C "https://example.org/" nit: these can probably be inlined now https://codereview.chromium.org/2655663004/diff/40001/content/common/feature_... content/common/feature_policy/feature_policy_unittest.cc:38: static_cast<int>(blink::WebFeaturePolicyFeature::LAST_FEATURE) + 1); nit: newlines between https://codereview.chromium.org/2655663004/diff/40001/content/common/feature_... content/common/feature_policy/feature_policy_unittest.cc:59: url::Origin origin) { nit: const ref https://codereview.chromium.org/2655663004/diff/40001/content/common/feature_... content/common/feature_policy/feature_policy_unittest.cc:72: The parsing tests have (understandably) been removed. Will they get added somewhere else?
The CQ bit was checked by iclelland@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...
https://codereview.chromium.org/2655663004/diff/40001/content/browser/frame_h... File content/browser/frame_host/frame_tree_node.cc (right): https://codereview.chromium.org/2655663004/diff/40001/content/browser/frame_h... content/browser/frame_host/frame_tree_node.cc:274: } On 2017/02/01 22:42:20, raymes wrote: > optional: alternatively: > FeaturePolicy* parent_policy = parent() ? parent()->feature_policy_.get() : > nullptr; > ... Done. https://codereview.chromium.org/2655663004/diff/40001/content/browser/frame_h... File content/browser/frame_host/frame_tree_node.h (right): https://codereview.chromium.org/2655663004/diff/40001/content/browser/frame_h... content/browser/frame_host/frame_tree_node.h:20: #include "content/common/feature_policy/feature_policy.h" On 2017/02/01 22:42:21, raymes wrote: > nit: I think we can forward declare this instead? Done. https://codereview.chromium.org/2655663004/diff/40001/content/common/feature_... File content/common/feature_policy/feature_policy.cc (right): https://codereview.chromium.org/2655663004/diff/40001/content/common/feature_... content/common/feature_policy/feature_policy.cc:56: // Extracts a Whitelist from a ParsedFeaturePolicyDeclaration On 2017/02/01 22:42:21, raymes wrote: > nit: . at end of sentence. Done. https://codereview.chromium.org/2655663004/diff/40001/content/common/feature_... content/common/feature_policy/feature_policy.cc:86: FeaturePolicy::Whitelist::Whitelist() : matches_all_origins_(false) {} On 2017/02/01 22:42:21, raymes wrote: > nit: newline after Done. https://codereview.chromium.org/2655663004/diff/40001/content/common/feature_... content/common/feature_policy/feature_policy.cc:120: if (!inherited_policies_.at(feature)) { On 2017/02/01 22:42:21, raymes wrote: > nit: (here and below) We tend to use [] instead of at (since we don't have > exceptions in Chrome anyway). I don't think I can use [], since it returns a nonconst reference (potentially assignable), and I can't do that in a const method. https://codereview.chromium.org/2655663004/diff/40001/content/common/feature_... content/common/feature_policy/feature_policy.cc:123: if (whitelists_.count(feature)) { On 2017/02/01 22:42:21, raymes wrote: > nit: in this case it's probably slightly better to use find() and then you can > reuse the iterator to check what the value is. That's a good idea, thanks. https://codereview.chromium.org/2655663004/diff/40001/content/common/feature_... content/common/feature_policy/feature_policy.cc:157: : origin_(origin), feature_list_(feature_list) {} On 2017/02/01 22:42:21, raymes wrote: > nit: newline Done. https://codereview.chromium.org/2655663004/diff/40001/content/common/feature_... content/common/feature_policy/feature_policy.cc:159: : origin_(origin), feature_list_(getDefaultFeatureList()) {} On 2017/02/01 22:42:21, raymes wrote: > nit: newline Done. https://codereview.chromium.org/2655663004/diff/40001/content/common/feature_... content/common/feature_policy/feature_policy.cc:182: // TODO: See if this should use lazy_instance instead On 2017/02/01 22:42:21, raymes wrote: > nit: remove this, the way you're doing it is probably more appropriate here :) Done, thanks for confirming :) https://codereview.chromium.org/2655663004/diff/40001/content/common/feature_... File content/common/feature_policy/feature_policy.h (right): https://codereview.chromium.org/2655663004/diff/40001/content/common/feature_... content/common/feature_policy/feature_policy.h:105: class Whitelist final { On 2017/02/01 22:42:21, raymes wrote: > It's a bit unfortunate that ParsedFeaturePolicyDeclaration is so similar to > Whitelist but I don't see a strictly better way to share code, since the struct > is used for IPC :( Agreed. https://codereview.chromium.org/2655663004/diff/40001/content/common/feature_... content/common/feature_policy/feature_policy.h:162: url::Origin origin); On 2017/02/01 22:42:21, raymes wrote: > nit: can probably use a const reference here Done. https://codereview.chromium.org/2655663004/diff/40001/content/common/feature_... content/common/feature_policy/feature_policy.h:166: url::Origin origin) const; On 2017/02/01 22:42:21, raymes wrote: > nit: can probably use a const reference here Done. https://codereview.chromium.org/2655663004/diff/40001/content/common/feature_... File content/common/feature_policy/feature_policy_unittest.cc (right): https://codereview.chromium.org/2655663004/diff/40001/content/common/feature_... content/common/feature_policy/feature_policy_unittest.cc:13: #define ORIGIN_C "https://example.org/" On 2017/02/01 22:42:21, raymes wrote: > nit: these can probably be inlined now Done. https://codereview.chromium.org/2655663004/diff/40001/content/common/feature_... content/common/feature_policy/feature_policy_unittest.cc:38: static_cast<int>(blink::WebFeaturePolicyFeature::LAST_FEATURE) + 1); On 2017/02/01 22:42:21, raymes wrote: > nit: newlines between Done. https://codereview.chromium.org/2655663004/diff/40001/content/common/feature_... content/common/feature_policy/feature_policy_unittest.cc:59: url::Origin origin) { On 2017/02/01 22:42:21, raymes wrote: > nit: const ref Done. https://codereview.chromium.org/2655663004/diff/40001/content/common/feature_... content/common/feature_policy/feature_policy_unittest.cc:72: On 2017/02/01 22:42:21, raymes wrote: > The parsing tests have (understandably) been removed. Will they get added > somewhere else? Parsing tests still live in 3p/WK/Source/platform/feature_policy/FeaturePolicyTest.cpp, with the parsing code. If that code gets moved into content as well, then the tests will move with it. (These tests assume the correctness of the parsing code, and just declare the ParsedFeaturePolicyDeclaration structs inline)
still lgtm, thanks :)
iclelland@chromium.org changed reviewers: + pfeldman@chromium.org
Thanks, raymes! +r pfeldman, could you PTAL as OWNER?
pfeldman@chromium.org changed reviewers: + creis@chromium.org, nasko@chromium.org
https://codereview.chromium.org/2655663004/diff/60001/content/browser/frame_h... File content/browser/frame_host/frame_tree_node.h (right): https://codereview.chromium.org/2655663004/diff/60001/content/browser/frame_h... content/browser/frame_host/frame_tree_node.h:356: std::unique_ptr<FeaturePolicy> feature_policy_; I'd talk to someone from the site isolation team on whether this is the right place for it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
iclelland@chromium.org changed reviewers: + alexmos@chromium.org
Thanks, pfeldman. +alexmos for site-isolation input https://codereview.chromium.org/2655663004/diff/60001/content/browser/frame_h... File content/browser/frame_host/frame_tree_node.h (right): https://codereview.chromium.org/2655663004/diff/60001/content/browser/frame_h... content/browser/frame_host/frame_tree_node.h:356: std::unique_ptr<FeaturePolicy> feature_policy_; On 2017/02/03 18:04:26, pfeldman wrote: > I'd talk to someone from the site isolation team on whether this is the right > place for it. Thanks -- alexmos, do you have any idea whether this is right? The policy *header* is already being replicated between frames (on the replication_state_ member); this is for the independently-constructed policy in the browser process, which should exist regardless of site-isolation. It may make more sense to put this on the render_frame_host, for instance, as it is the browser's view of the policy which should exist in the renderer. (We plan to query this policy in the browser process when that's the right place to make decisions, like for permission-backed features)
https://codereview.chromium.org/2655663004/diff/60001/content/browser/frame_h... File content/browser/frame_host/frame_tree_node.h (right): https://codereview.chromium.org/2655663004/diff/60001/content/browser/frame_h... content/browser/frame_host/frame_tree_node.h:356: std::unique_ptr<FeaturePolicy> feature_policy_; On 2017/02/03 19:26:37, iclelland wrote: > On 2017/02/03 18:04:26, pfeldman wrote: > > I'd talk to someone from the site isolation team on whether this is the right > > place for it. > > Thanks -- alexmos, do you have any idea whether this is right? The policy > *header* is already being replicated between frames (on the replication_state_ > member); this is for the independently-constructed policy in the browser > process, which should exist regardless of site-isolation. > > It may make more sense to put this on the render_frame_host, for instance, as it > is the browser's view of the policy which should exist in the renderer. (We plan > to query this policy in the browser process when that's the right place to make > decisions, like for permission-backed features) I think it might make more sense to put this on the RenderFrameHost. The main issue is this will be incorrect if you will ever need to check feature policy on a non-current RFH (i.e., pending RFH [for cross-process navigation that hasn't yet committed] or pending delete RFH [running unload handler after another RFH has committed a navigation and became current for this FTN]). We've had this issue with URLs and origins (see issues 590034, 590035, 663740), and ended up storing both of those on RFHI. (The origin is still also kept in the FRS for replication, but it should be avoided for origin checks unless you really know what you're doing). Seems like at least some of the features controlled by feature policy could be utilized in unload handlers (right?), so you probably have to worry about this.
The CQ bit was checked by iclelland@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: linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)
https://codereview.chromium.org/2655663004/diff/60001/content/browser/frame_h... File content/browser/frame_host/frame_tree_node.h (right): https://codereview.chromium.org/2655663004/diff/60001/content/browser/frame_h... content/browser/frame_host/frame_tree_node.h:356: std::unique_ptr<FeaturePolicy> feature_policy_; On 2017/02/03 22:37:16, alexmos wrote: > On 2017/02/03 19:26:37, iclelland wrote: > > On 2017/02/03 18:04:26, pfeldman wrote: > > > I'd talk to someone from the site isolation team on whether this is the > right > > > place for it. > > > > Thanks -- alexmos, do you have any idea whether this is right? The policy > > *header* is already being replicated between frames (on the replication_state_ > > member); this is for the independently-constructed policy in the browser > > process, which should exist regardless of site-isolation. > > > > It may make more sense to put this on the render_frame_host, for instance, as > it > > is the browser's view of the policy which should exist in the renderer. (We > plan > > to query this policy in the browser process when that's the right place to > make > > decisions, like for permission-backed features) > > I think it might make more sense to put this on the RenderFrameHost. The main > issue is this will be incorrect if you will ever need to check feature policy on > a non-current RFH (i.e., pending RFH [for cross-process navigation that hasn't > yet committed] or pending delete RFH [running unload handler after another RFH > has committed a navigation and became current for this FTN]). We've had this > issue with URLs and origins (see issues 590034, 590035, 663740), and ended up > storing both of those on RFHI. (The origin is still also kept in the FRS for > replication, but it should be avoided for origin checks unless you really know > what you're doing). Seems like at least some of the features controlled by > feature policy could be utilized in unload handlers (right?), so you probably > have to worry about this. Thanks, Alex, I appreciate the insight. I've moved the actual policy to the RFHI, with getter/setter methods. In the CL that actually starts using this from the browser, I'll be adding a boolean IsFeatureEnabled() method to the RFH public interface.
https://codereview.chromium.org/2655663004/diff/60001/content/browser/frame_h... File content/browser/frame_host/frame_tree_node.h (right): https://codereview.chromium.org/2655663004/diff/60001/content/browser/frame_h... content/browser/frame_host/frame_tree_node.h:356: std::unique_ptr<FeaturePolicy> feature_policy_; On 2017/02/06 18:32:33, iclelland wrote: > On 2017/02/03 22:37:16, alexmos wrote: > > On 2017/02/03 19:26:37, iclelland wrote: > > > On 2017/02/03 18:04:26, pfeldman wrote: > > > > I'd talk to someone from the site isolation team on whether this is the > > right > > > > place for it. > > > > > > Thanks -- alexmos, do you have any idea whether this is right? The policy > > > *header* is already being replicated between frames (on the > replication_state_ > > > member); this is for the independently-constructed policy in the browser > > > process, which should exist regardless of site-isolation. > > > > > > It may make more sense to put this on the render_frame_host, for instance, > as > > it > > > is the browser's view of the policy which should exist in the renderer. (We > > plan > > > to query this policy in the browser process when that's the right place to > > make > > > decisions, like for permission-backed features) > > > > I think it might make more sense to put this on the RenderFrameHost. The main > > issue is this will be incorrect if you will ever need to check feature policy > on > > a non-current RFH (i.e., pending RFH [for cross-process navigation that hasn't > > yet committed] or pending delete RFH [running unload handler after another RFH > > has committed a navigation and became current for this FTN]). We've had this > > issue with URLs and origins (see issues 590034, 590035, 663740), and ended up > > storing both of those on RFHI. (The origin is still also kept in the FRS for > > replication, but it should be avoided for origin checks unless you really know > > what you're doing). Seems like at least some of the features controlled by > > feature policy could be utilized in unload handlers (right?), so you probably > > have to worry about this. > > Thanks, Alex, I appreciate the insight. I've moved the actual policy to the > RFHI, with getter/setter methods. In the CL that actually starts using this from > the browser, I'll be adding a boolean IsFeatureEnabled() method to the RFH > public interface. Yes, that sounds good to me. https://codereview.chromium.org/2655663004/diff/80001/content/browser/frame_h... File content/browser/frame_host/frame_tree_node.cc (right): https://codereview.chromium.org/2655663004/diff/80001/content/browser/frame_h... content/browser/frame_host/frame_tree_node.cc:289: current_frame_host()->GetFeaturePolicy()->SetHeaderPolicy(parsed_header); Could we move this call to RFHI::OnDidSetFeaturePolicyHeader, where we can directly call SetHeaderPolicy on the RFHI's feature_policy_? I'm realizing also that RFHI::OnDidSetFeaturePolicyHeader might be vulnerable to the race from https://bugs.chromium.org/p/chromium/issues/detail?id=686188 (another cross-process navigation could commit just before we receive the DidSetFeaturePolicyHeader IPC, making the receiving RFH pending deletion. In that case it probably doesn't make sense to update the FTN header, but I'll deal with that separately, as it affects other IPCs as well.) https://codereview.chromium.org/2655663004/diff/80001/content/browser/frame_h... content/browser/frame_host/frame_tree_node.cc:296: current_frame_host()->SetFeaturePolicy( Similarly, it might be safer to call this on the right |render_frame_host| directly from the ResetFeaturePolicy call site in NavigatorImpl::DidNavigate. Currently, it looks like that function might call DidNavigateFrame either before or after the call to ResetFeaturePolicy, depending on |oopifs_possible|, and DidNavigateFrame is what eventually calls CommitPending and updates current_frame_host() (i.e., you might be grabbing the wrong one here). https://codereview.chromium.org/2655663004/diff/80001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/2655663004/diff/80001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:593: void SetFeaturePolicy(std::unique_ptr<FeaturePolicy> feature_policy); These will need a comment.
https://codereview.chromium.org/2655663004/diff/80001/content/browser/frame_h... File content/browser/frame_host/frame_tree_node.cc (right): https://codereview.chromium.org/2655663004/diff/80001/content/browser/frame_h... content/browser/frame_host/frame_tree_node.cc:296: current_frame_host()->SetFeaturePolicy( On 2017/02/07 00:21:35, alexmos wrote: > Similarly, it might be safer to call this on the right |render_frame_host| > directly from the ResetFeaturePolicy call site in NavigatorImpl::DidNavigate. > Currently, it looks like that function might call DidNavigateFrame either before > or after the call to ResetFeaturePolicy, depending on |oopifs_possible|, and > DidNavigateFrame is what eventually calls CommitPending and updates > current_frame_host() (i.e., you might be grabbing the wrong one here). I can definitely do that -- and it brings up the issue that the current origin isn't well defined at this point; the new origin has been set current in the FTR, but not committed in the RFH. What do you think of moving this to near the end of NavigatorImpl::DidNavigate, after the new origin is committed, or actually into RFHI::set_last_committed_origin?
https://codereview.chromium.org/2655663004/diff/80001/content/browser/frame_h... File content/browser/frame_host/frame_tree_node.cc (right): https://codereview.chromium.org/2655663004/diff/80001/content/browser/frame_h... content/browser/frame_host/frame_tree_node.cc:296: current_frame_host()->SetFeaturePolicy( On 2017/02/07 15:47:37, iclelland wrote: > On 2017/02/07 00:21:35, alexmos wrote: > > Similarly, it might be safer to call this on the right |render_frame_host| > > directly from the ResetFeaturePolicy call site in NavigatorImpl::DidNavigate. > > Currently, it looks like that function might call DidNavigateFrame either > before > > or after the call to ResetFeaturePolicy, depending on |oopifs_possible|, and > > DidNavigateFrame is what eventually calls CommitPending and updates > > current_frame_host() (i.e., you might be grabbing the wrong one here). > > I can definitely do that -- and it brings up the issue that the current origin > isn't well defined at this point; the new origin has been set current in the > FTR, but not committed in the RFH. What do you think of moving this to near the > end of NavigatorImpl::DidNavigate, after the new origin is committed, or > actually into RFHI::set_last_committed_origin? Right, at the point where ResetFeaturePolicy is called you have to be careful in that the FTN origin (from the replication state) has been updated, but the RFHI origin hasn't. All the work that updates the replication state (FTN::SetCurrentOrigin, ResetFeaturePolicy, ResetContentSecurityPolicy) needs to happen before DidNavigateFrame, which send that replication state to the new proxy that replaces the old RF that we just navigated from. So that can't be moved after set_last_committed_origin, as that comes after DidNavigateFrame. But I think calling SetFeaturePolicy() after the set_last_committed_origin() in DidNavigate makes sense -- that will kind of go with the existing flow that first we update the things stored on replication state, then commit the new RFH with DidNavigateFrame, then update the things stored on RFHI. Or you could just pass the new origin (|params.origin|) to it directly if you wanted to keep all feature policy calls together. Having it inside RFHI::set_last_committed_origin seems a bit more weird, as feature policy doesn't seem much related to updating the origin.
The CQ bit was checked by iclelland@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...
https://codereview.chromium.org/2655663004/diff/80001/content/browser/frame_h... File content/browser/frame_host/frame_tree_node.cc (right): https://codereview.chromium.org/2655663004/diff/80001/content/browser/frame_h... content/browser/frame_host/frame_tree_node.cc:289: current_frame_host()->GetFeaturePolicy()->SetHeaderPolicy(parsed_header); On 2017/02/07 00:21:35, alexmos wrote: > Could we move this call to RFHI::OnDidSetFeaturePolicyHeader, where we can > directly call SetHeaderPolicy on the RFHI's feature_policy_? > > I'm realizing also that RFHI::OnDidSetFeaturePolicyHeader might be vulnerable to > the race from https://bugs.chromium.org/p/chromium/issues/detail?id=686188 > (another cross-process navigation could commit just before we receive the > DidSetFeaturePolicyHeader IPC, making the receiving RFH pending deletion. In > that case it probably doesn't make sense to update the FTN header, but I'll deal > with that separately, as it affects other IPCs as well.) Done -- you're absolutely right that it doesn't make sense to call back to the RFHI here, since that's what called this method in the first place. https://codereview.chromium.org/2655663004/diff/80001/content/browser/frame_h... content/browser/frame_host/frame_tree_node.cc:296: current_frame_host()->SetFeaturePolicy( On 2017/02/07 23:15:15, alexmos wrote: > On 2017/02/07 15:47:37, iclelland wrote: > > On 2017/02/07 00:21:35, alexmos wrote: > > > Similarly, it might be safer to call this on the right |render_frame_host| > > > directly from the ResetFeaturePolicy call site in > NavigatorImpl::DidNavigate. > > > Currently, it looks like that function might call DidNavigateFrame either > > before > > > or after the call to ResetFeaturePolicy, depending on |oopifs_possible|, and > > > DidNavigateFrame is what eventually calls CommitPending and updates > > > current_frame_host() (i.e., you might be grabbing the wrong one here). > > > > I can definitely do that -- and it brings up the issue that the current origin > > isn't well defined at this point; the new origin has been set current in the > > FTR, but not committed in the RFH. What do you think of moving this to near > the > > end of NavigatorImpl::DidNavigate, after the new origin is committed, or > > actually into RFHI::set_last_committed_origin? > > Right, at the point where ResetFeaturePolicy is called you have to be careful in > that the FTN origin (from the replication state) has been updated, but the RFHI > origin hasn't. All the work that updates the replication state > (FTN::SetCurrentOrigin, ResetFeaturePolicy, ResetContentSecurityPolicy) needs to > happen before DidNavigateFrame, which send that replication state to the new > proxy that replaces the old RF that we just navigated from. So that can't be > moved after set_last_committed_origin, as that comes after DidNavigateFrame. > > But I think calling SetFeaturePolicy() after the set_last_committed_origin() in > DidNavigate makes sense -- that will kind of go with the existing flow that > first we update the things stored on replication state, then commit the new RFH > with DidNavigateFrame, then update the things stored on RFHI. That's what I've done in the latest patch. > Or you could just > pass the new origin (|params.origin|) to it directly if you wanted to keep all > feature policy calls together. I'd considered that, but I think this makes more sense; it's easier to see what the ResetFeaturePolicy method does, and there's no question about what you need to pass to it. https://codereview.chromium.org/2655663004/diff/80001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/2655663004/diff/80001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:593: void SetFeaturePolicy(std::unique_ptr<FeaturePolicy> feature_policy); On 2017/02/07 00:21:35, alexmos wrote: > These will need a comment. Done -- the new methods in this file have comments.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks, the current approach of keeping the feature policy in the RFHI looks good. Just a few other minor comments below, and I didn't look at the unit tests (are those just transferred from FeaturePolicyTest.cpp?) https://codereview.chromium.org/2655663004/diff/100001/content/browser/frame_... File content/browser/frame_host/frame_tree_node.cc (right): https://codereview.chromium.org/2655663004/diff/100001/content/browser/frame_... content/browser/frame_host/frame_tree_node.cc:21: #include "content/common/feature_policy/feature_policy.h" This is no longer needed, right? https://codereview.chromium.org/2655663004/diff/100001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2655663004/diff/100001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:3366: const FeaturePolicy* RenderFrameHostImpl::GetFeaturePolicy() { This is a simple getter, so it could just go inlined into the header as get_feature_policy(). https://codereview.chromium.org/2655663004/diff/100001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/2655663004/diff/100001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:594: const FeaturePolicy* GetFeaturePolicy(); nit: no const (see a note in https://www.chromium.org/developers/content-module/content-api about this) https://codereview.chromium.org/2655663004/diff/100001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:1155: std::unique_ptr<FeaturePolicy> feature_policy_; Currently, seems like this will be null from the time the frame is created until its first navigation commits. I see that this is the current behavior in Blink also, so just curious about why we wouldn't want to initialize the feature policy to something at frame creation time so it's always non-null (so callers don't have to potentially null-check GetFeaturePolicy()). I guess we won't know the origin until the commit, though on the other hand, I think Blink considers a new subframe's initial blank document to be in its parent's origin before commit. https://codereview.chromium.org/2655663004/diff/100001/content/common/feature... File content/common/feature_policy/feature_policy.cc (right): https://codereview.chromium.org/2655663004/diff/100001/content/common/feature... content/common/feature_policy/feature_policy.cc:18: std::string feature_name, nit: const std::string& ? https://codereview.chromium.org/2655663004/diff/100001/content/common/feature... content/common/feature_policy/feature_policy.cc:123: return false; nit: {} not necessary for single-line if and body (also for the if below) https://codereview.chromium.org/2655663004/diff/100001/content/common/feature... content/common/feature_policy/feature_policy.cc:142: return IsFeatureEnabledForOrigin(feature, origin_); This brings an interesting point. For unique origins, if the same instance of a unique origin is passed to SecurityOrigin::isSameSchemeHostPort, it will return true (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/webor...), but url::Origin would return false (https://cs.chromium.org/chromium/src/url/origin.cc?l=161). So if origin_ is unique, it wouldn't pass the check in line 135, whereas it would in Blink. I don't know if this matters for you in practice (e.g., in sandboxed iframes), but mentioning it just in case. https://codereview.chromium.org/2655663004/diff/100001/content/common/feature... content/common/feature_policy/feature_policy.cc:172: std::unique_ptr<FeaturePolicy> newPolicy = nit: new_policy https://codereview.chromium.org/2655663004/diff/100001/content/common/feature... content/common/feature_policy/feature_policy.cc:188: FeatureList, defaultFeatureList, nit: defaultFeatureList -> default_feature_list https://codereview.chromium.org/2655663004/diff/100001/content/common/feature... File content/common/feature_policy/feature_policy.h (right): https://codereview.chromium.org/2655663004/diff/100001/content/common/feature... content/common/feature_policy/feature_policy.h:187: static const FeatureList& getDefaultFeatureList(); nit: capitalize get https://codereview.chromium.org/2655663004/diff/100001/content/common/feature... content/common/feature_policy/feature_policy.h:202: const FeatureList& feature_list_; Just curious, why is this not shared across all feature policies?
The CQ bit was checked by iclelland@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...
> Thanks, the current approach of keeping the feature policy in the RFHI looks > good. Just a few other minor comments below, and I didn't look at the unit > tests (are those just transferred from FeaturePolicyTest.cpp?) Yep, those are transferred almost verbatim. The only change made was that, because we're not doing the string parsing in content/, we just build up the pre-parsed structs for headers and then test the policies that get generated from those. (The parsing logic, ensuring that the structs are generated correctly, is still tested. Those tests just haven't moved from Blink) https://codereview.chromium.org/2655663004/diff/100001/content/browser/frame_... File content/browser/frame_host/frame_tree_node.cc (right): https://codereview.chromium.org/2655663004/diff/100001/content/browser/frame_... content/browser/frame_host/frame_tree_node.cc:21: #include "content/common/feature_policy/feature_policy.h" On 2017/02/09 05:59:50, alexmos wrote: > This is no longer needed, right? It's still used for the definition of ParsedFeaturePolicyHeader, but that ends up being included transitively through frame_tree_node.h -> frame_replication_state.h -> feature_policy.h. The normal thing here would be to leave this include in, and forward-declare in frame_tree_node.h, but it's actually a using-declaration, so it's going to be awkward to do that :( I've just removed the include from this file, to at least be consistent. https://codereview.chromium.org/2655663004/diff/100001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2655663004/diff/100001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:3366: const FeaturePolicy* RenderFrameHostImpl::GetFeaturePolicy() { On 2017/02/09 05:59:50, alexmos wrote: > This is a simple getter, so it could just go inlined into the header as > get_feature_policy(). Done. https://codereview.chromium.org/2655663004/diff/100001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/2655663004/diff/100001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:594: const FeaturePolicy* GetFeaturePolicy(); On 2017/02/09 05:59:50, alexmos wrote: > nit: no const (see a note in > https://www.chromium.org/developers/content-module/content-api about this) Done. (I thought this only applied to headers in content/public, ie. render_frame_host.h, but I'm fine with this here as well. Returning a nonconst object allows callers to modify the policy, but I don't have any reason to think that people would accidentally do that.) https://codereview.chromium.org/2655663004/diff/100001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:1155: std::unique_ptr<FeaturePolicy> feature_policy_; On 2017/02/09 05:59:50, alexmos wrote: > Currently, seems like this will be null from the time the frame is created until > its first navigation commits. I see that this is the current behavior in Blink > also, so just curious about why we wouldn't want to initialize the feature > policy to something at frame creation time so it's always non-null (so callers > don't have to potentially null-check GetFeaturePolicy()). I guess we won't know > the origin until the commit, though on the other hand, I think Blink considers a > new subframe's initial blank document to be in its parent's origin before > commit. Acknowledged. I'll see about doing an explicit pre-navigation-commit init of the policy in a future CL; it will clear up any other call sites. https://codereview.chromium.org/2655663004/diff/100001/content/common/feature... File content/common/feature_policy/feature_policy.cc (right): https://codereview.chromium.org/2655663004/diff/100001/content/common/feature... content/common/feature_policy/feature_policy.cc:18: std::string feature_name, On 2017/02/09 05:59:50, alexmos wrote: > nit: const std::string& ? Done. https://codereview.chromium.org/2655663004/diff/100001/content/common/feature... content/common/feature_policy/feature_policy.cc:123: return false; On 2017/02/09 05:59:51, alexmos wrote: > nit: {} not necessary for single-line if and body (also for the if below) Done. https://codereview.chromium.org/2655663004/diff/100001/content/common/feature... content/common/feature_policy/feature_policy.cc:142: return IsFeatureEnabledForOrigin(feature, origin_); On 2017/02/09 05:59:50, alexmos wrote: > This brings an interesting point. For unique origins, if the same instance of a > unique origin is passed to SecurityOrigin::isSameSchemeHostPort, it will return > true > (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/webor...), > but url::Origin would return false > (https://cs.chromium.org/chromium/src/url/origin.cc?l=161). So if origin_ is > unique, it wouldn't pass the check in line 135, whereas it would in Blink. I > don't know if this matters for you in practice (e.g., in sandboxed iframes), but > mentioning it just in case. Yes, I discovered that yesterday, troubleshooting the next CL in the series (https://codereview.chromium.org/2651883008/). I've a call with mkwst to talk about the semantics of unique/opaque origins in this case this afternoon. I think the solution is to change the API slightly for the most common case (where the only origin we care about is the frame's one) and explicitly check the origin for identity with that, even if it's unique. That doesn't help *every* case, but it at least allows the default policy to work in those frames. There are certainly tricky issues around this, though -- there's no way to convert a unique SecurityOrigin to a url::Origin in a way that preserves its identity :( I've created crbug.com/690520 to ensure that this doesn't get forgotten. https://codereview.chromium.org/2655663004/diff/100001/content/common/feature... content/common/feature_policy/feature_policy.cc:172: std::unique_ptr<FeaturePolicy> newPolicy = On 2017/02/09 05:59:51, alexmos wrote: > nit: new_policy Thanks; need a better converter from blink style :) https://codereview.chromium.org/2655663004/diff/100001/content/common/feature... content/common/feature_policy/feature_policy.cc:188: FeatureList, defaultFeatureList, On 2017/02/09 05:59:50, alexmos wrote: > nit: defaultFeatureList -> default_feature_list Done. https://codereview.chromium.org/2655663004/diff/100001/content/common/feature... File content/common/feature_policy/feature_policy.h (right): https://codereview.chromium.org/2655663004/diff/100001/content/common/feature... content/common/feature_policy/feature_policy.h:187: static const FeatureList& getDefaultFeatureList(); On 2017/02/09 05:59:51, alexmos wrote: > nit: capitalize get Done. https://codereview.chromium.org/2655663004/diff/100001/content/common/feature... content/common/feature_policy/feature_policy.h:202: const FeatureList& feature_list_; On 2017/02/09 05:59:51, alexmos wrote: > Just curious, why is this not shared across all feature policies? Outside of tests, it is shared across all of them. It's only explicitly held here so that we can construct test policies without reference to any real concrete features. For example, there are no "default-off" features currently defined, but it's still useful to be able to construct a policy with such features, to test the framework itself.
LGTM % nits https://codereview.chromium.org/2655663004/diff/100001/content/common/feature... File content/common/feature_policy/feature_policy.h (right): https://codereview.chromium.org/2655663004/diff/100001/content/common/feature... content/common/feature_policy/feature_policy.h:202: const FeatureList& feature_list_; On 2017/02/09 17:16:18, iclelland wrote: > On 2017/02/09 05:59:51, alexmos wrote: > > Just curious, why is this not shared across all feature policies? > > Outside of tests, it is shared across all of them. It's only explicitly held > here so that we can construct test policies without reference to any real > concrete features. For example, there are no "default-off" features currently > defined, but it's still useful to be able to construct a policy with such > features, to test the framework itself. Acknowledged. I was just thinking about whether this could be static - not sure if the tests need to instantiate multiple policies with differing feature lists, but regardless, this matches the Blink side today, so any changes would be for another CL. https://codereview.chromium.org/2655663004/diff/120001/content/common/feature... File content/common/feature_policy/feature_policy.cc (right): https://codereview.chromium.org/2655663004/diff/120001/content/common/feature... content/common/feature_policy/feature_policy.cc:18: const std::string feature_name, nit: still missing the & :) https://codereview.chromium.org/2655663004/diff/120001/content/common/feature... File content/common/feature_policy/feature_policy_unittest.cc (right): https://codereview.chromium.org/2655663004/diff/120001/content/common/feature... content/common/feature_policy/feature_policy_unittest.cc:60: url::Origin originA_ = url::Origin(GURL("https://example.com/")); nit: origin_a_, etc. https://codereview.chromium.org/2655663004/diff/120001/content/common/feature... content/common/feature_policy/feature_policy_unittest.cc:67: FeaturePolicy::FeatureList featureList_; nit: feature_list_
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 iclelland@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...
iclelland@chromium.org changed reviewers: + rbyers@chromium.org
Thanks, Alex! +r rbyers for OWNER stamp on 3p/WK/public/platform https://codereview.chromium.org/2655663004/diff/120001/content/common/feature... File content/common/feature_policy/feature_policy.cc (right): https://codereview.chromium.org/2655663004/diff/120001/content/common/feature... content/common/feature_policy/feature_policy.cc:18: const std::string feature_name, On 2017/02/09 19:11:18, alexmos wrote: > nit: still missing the & :) Oops. Thanks, done properly now. https://codereview.chromium.org/2655663004/diff/120001/content/common/feature... File content/common/feature_policy/feature_policy_unittest.cc (right): https://codereview.chromium.org/2655663004/diff/120001/content/common/feature... content/common/feature_policy/feature_policy_unittest.cc:60: url::Origin originA_ = url::Origin(GURL("https://example.com/")); On 2017/02/09 19:11:18, alexmos wrote: > nit: origin_a_, etc. Done. https://codereview.chromium.org/2655663004/diff/120001/content/common/feature... content/common/feature_policy/feature_policy_unittest.cc:67: FeaturePolicy::FeatureList featureList_; On 2017/02/09 19:11:18, alexmos wrote: > nit: feature_list_ Done.
public/ LGTM with nit https://codereview.chromium.org/2655663004/diff/140001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebFeaturePolicy.h (right): https://codereview.chromium.org/2655663004/diff/140001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebFeaturePolicy.h:14: enum class WebFeaturePolicyFeature { nit: we generally try to write detailed comments for (new) public APIs. Could you add a comment saying where to look for the precise definition of each of these (eg. should these map 1:1 to the spec, and/or is there some other definition inside blink that should be considered authoritative)? Or perhaps add detailed comments here as the canonical definition of what exactly each of these features refers to?
The CQ bit was checked by iclelland@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...
https://codereview.chromium.org/2655663004/diff/140001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebFeaturePolicy.h (right): https://codereview.chromium.org/2655663004/diff/140001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebFeaturePolicy.h:14: enum class WebFeaturePolicyFeature { On 2017/02/09 20:40:14, Rick Byers wrote: > nit: we generally try to write detailed comments for (new) public APIs. Could > you add a comment saying where to look for the precise definition of each of > these (eg. should these map 1:1 to the spec, and/or is there some other > definition inside blink that should be considered authoritative)? Or perhaps > add detailed comments here as the canonical definition of what exactly each of > these features refers to? Done, thanks!
The CQ bit was checked by iclelland@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org, alexmos@chromium.org, rbyers@chromium.org Link to the patchset: https://codereview.chromium.org/2655663004/#ps160001 (title: "Add comments for public enum members")
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": 160001, "attempt_start_ts": 1486675782079720,
"parent_rev": "57fd39f44a3af5474f75cca7bd9870d4e26fc6d2", "commit_rev":
"2c79efe295ea0cd234b3e13b44369fdba046280e"}
Message was sent while issue was closed.
Description was changed from ========== Introduce content-side Feature Policy object and maintain in parallel with renderer policy. This is part 3 of 5 in the effort to move the FeaturePolicy implementation into the content layer, which will facilitate use of the framework for browser- based policy decisions. See the other CLs in this series: [1/5] https://codereview.chromium.org/2648423002/ (Rename classes) [2/5] https://codereview.chromium.org/2654873004/ (Centralize content-side code) [3/5] (This CL) (Maintain parallel FP in browser) [4/5] https://codereview.chromium.org/2651883008/ (Use content/ FP in blink) [5/5] https://codereview.chromium.org/2656533004/ (Remove unused blink code.) BUG=685195 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Introduce content-side Feature Policy object and maintain in parallel with renderer policy. This is part 3 of 5 in the effort to move the FeaturePolicy implementation into the content layer, which will facilitate use of the framework for browser- based policy decisions. See the other CLs in this series: [1/5] https://codereview.chromium.org/2648423002/ (Rename classes) [2/5] https://codereview.chromium.org/2654873004/ (Centralize content-side code) [3/5] (This CL) (Maintain parallel FP in browser) [4/5] https://codereview.chromium.org/2651883008/ (Use content/ FP in blink) [5/5] https://codereview.chromium.org/2656533004/ (Remove unused blink code.) BUG=685195 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2655663004 Cr-Commit-Position: refs/heads/master@{#449444} Committed: https://chromium.googlesource.com/chromium/src/+/2c79efe295ea0cd234b3e13b4436... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/2c79efe295ea0cd234b3e13b4436... |
