Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2683413003:
Introduce AsyncDocumentSubresourceFilter. (Closed)
|
Created:
3 years, 10 months ago by pkalinnikov Modified:
3 years, 10 months ago CC:
blundell+watchlist_chromium.org, chromium-reviews, darin-cc_chromium.org, droger+watchlist_chromium.org, jam, melandory, sdefresne+watchlist_chromium.org, subresource-filter-reviews_chromium.org, jochen (gone - plz use gerrit) Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntroduce AsyncDocumentSubresourceFilter.
AsyncDocumentSubresourceFilter (ADSF) is an asynchronous wrapper around
DocumentSubresourceFilter (DSF).
When ADSF is constructed, it creates an ADSF::Core and initializes it
asynchronously on a SequencedTaskRunner using the supplied initialization
|params| and a VerifiedRuleset. The task runner and VerifiedRuleset are taken
from VerifiedRuleset::Handle provided to the constructor. ADSF::Core remains
owned by ADSF, but lives on (and is accessed on) the task runner.
During ADSF::Core initialization an ActivationState for the current frame is
calculated, and then reported back via |activation_state_callback| on the task
runner associated with the ADSF creator's thread (normally, the UI thread). If
MemoryMappedRuleset is not present or malformed, then a default ActivationState
is reported (with ActivationLevel equal to DISABLED).
Although the full initialization of ADSF/ADSF::Core is asynchronous, the
properties of SequencedTaskRunner allow the ADSF instance to be immediately
used after construction.
The key purpose of ADSF is providing decisions on whether to allow loading
subframes for the current frame. The GetLoadPolicyForSubdocument async method
serves this purpose, and boils down to calling the GetLoadPolicy method of DSF
on the task runner and returning the result to the caller's task runner.
BUG=637415
Review-Url: https://codereview.chromium.org/2683413003
Cr-Commit-Position: refs/heads/master@{#451648}
Committed: https://chromium.googlesource.com/chromium/src/+/3e809d1f60ee19d1021842752704717001f1e0b4
Patch Set 1 #
Total comments: 14
Patch Set 2 : Address initial comments from engedy@. #Patch Set 3 : Add unittests. #Patch Set 4 : Add first_disallowed_load_callback to ADSF. #
Total comments: 6
Patch Set 5 : Distinguish main-frame and subframe ADSF initialization params. #
Total comments: 7
Patch Set 6 : Address comments from csharrison@. #
Total comments: 45
Patch Set 7 : Address more comments from engedy@. #
Total comments: 4
Patch Set 8 : Address comments from jochen@. #Patch Set 9 : Fix build. #Patch Set 10 : Fix blink tests build. #Patch Set 11 : Fix fix blink test build. #
Total comments: 4
Patch Set 12 : Rebase and clean up. #Patch Set 13 : Rebase. #
Messages
Total messages: 99 (69 generated)
The CQ bit was checked by pkalinnikov@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...
pkalinnikov@chromium.org changed reviewers: + engedy@chromium.org
Balazs, please take an initial look. I will add tests a bit later.
Some initial comments. https://codereview.chromium.org/2683413003/diff/1/components/subresource_filt... File components/subresource_filter/content/browser/async_document_subresource_filter.h (right): https://codereview.chromium.org/2683413003/diff/1/components/subresource_filt... components/subresource_filter/content/browser/async_document_subresource_filter.h:21: // ADSF is an asynchronous version of DocumentSubresourceFilter. Normally it Phrasing nit: An asynchronous wrapper around DocumentSubresourceFilter. <blank line> It is accessed on the UI thread, and owns ... https://codereview.chromium.org/2683413003/diff/1/components/subresource_filt... components/subresource_filter/content/browser/async_document_subresource_filter.h:30: // computing ActivationState for a document, and creating a corresponding nit: and for https://codereview.chromium.org/2683413003/diff/1/components/subresource_filt... components/subresource_filter/content/browser/async_document_subresource_filter.h:46: // the first disallowed load. Called at most once, on |task_runner|. Hang on, calling this on the |task_runner| doesn't sound right. https://codereview.chromium.org/2683413003/diff/1/components/subresource_filt... components/subresource_filter/content/browser/async_document_subresource_filter.h:57: // |ruleset_handle|. The instance remains owned by |this| object, but living nit: but lives on (and is accessed on) https://codereview.chromium.org/2683413003/diff/1/components/subresource_filt... components/subresource_filter/content/browser/async_document_subresource_filter.h:60: // Reports ActivationState via |activation_state_callback| on the current nit: Once the ActivationState for the current frame is calculated, it is reported back ... https://codereview.chromium.org/2683413003/diff/1/components/subresource_filt... components/subresource_filter/content/browser/async_document_subresource_filter.h:70: // Computes LoadPolicy on a |task_runner| and returns it back to the calling nit: Determines the LoadPolicy for the given |subdocument_url| load asynchronously on the |task_runner| ... https://codereview.chromium.org/2683413003/diff/1/components/subresource_filt... components/subresource_filter/content/browser/async_document_subresource_filter.h:98: // Can return nullptr even after initialization in case DSF could not be nit: Let's try to avoid these non-standard abbreviations.
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 pkalinnikov@chromium.org to run a CQ dry run
Addressed initial comments. https://codereview.chromium.org/2683413003/diff/1/components/subresource_filt... File components/subresource_filter/content/browser/async_document_subresource_filter.h (right): https://codereview.chromium.org/2683413003/diff/1/components/subresource_filt... components/subresource_filter/content/browser/async_document_subresource_filter.h:21: // ADSF is an asynchronous version of DocumentSubresourceFilter. Normally it On 2017/02/10 14:31:59, engedy wrote: > Phrasing nit: > > An asynchronous wrapper around DocumentSubresourceFilter. > <blank line> > It is accessed on the UI thread, and owns ... Done. https://codereview.chromium.org/2683413003/diff/1/components/subresource_filt... components/subresource_filter/content/browser/async_document_subresource_filter.h:30: // computing ActivationState for a document, and creating a corresponding On 2017/02/10 14:31:58, engedy wrote: > nit: and for Done. https://codereview.chromium.org/2683413003/diff/1/components/subresource_filt... components/subresource_filter/content/browser/async_document_subresource_filter.h:46: // the first disallowed load. Called at most once, on |task_runner|. On 2017/02/10 14:31:59, engedy wrote: > Hang on, calling this on the |task_runner| doesn't sound right. Now the call is redirected to the current thread that creates AsyncDocumentSubresourceFilter. https://codereview.chromium.org/2683413003/diff/1/components/subresource_filt... components/subresource_filter/content/browser/async_document_subresource_filter.h:57: // |ruleset_handle|. The instance remains owned by |this| object, but living On 2017/02/10 14:31:58, engedy wrote: > nit: but lives on (and is accessed on) Done. https://codereview.chromium.org/2683413003/diff/1/components/subresource_filt... components/subresource_filter/content/browser/async_document_subresource_filter.h:60: // Reports ActivationState via |activation_state_callback| on the current On 2017/02/10 14:31:59, engedy wrote: > nit: Once the ActivationState for the current frame is calculated, it is > reported back ... Done. https://codereview.chromium.org/2683413003/diff/1/components/subresource_filt... components/subresource_filter/content/browser/async_document_subresource_filter.h:70: // Computes LoadPolicy on a |task_runner| and returns it back to the calling On 2017/02/10 14:31:59, engedy wrote: > nit: Determines the LoadPolicy for the given |subdocument_url| load > asynchronously on the |task_runner| ... Done. https://codereview.chromium.org/2683413003/diff/1/components/subresource_filt... components/subresource_filter/content/browser/async_document_subresource_filter.h:98: // Can return nullptr even after initialization in case DSF could not be On 2017/02/10 14:31:58, engedy wrote: > nit: Let's try to avoid these non-standard abbreviations. How about this?
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 pkalinnikov@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 pkalinnikov@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...
PTAL.
csharrison@chromium.org changed reviewers: + csharrison@chromium.org
Quick request for comment https://codereview.chromium.org/2683413003/diff/60001/components/subresource_... File components/subresource_filter/content/browser/async_document_subresource_filter.h (right): https://codereview.chromium.org/2683413003/diff/60001/components/subresource_... components/subresource_filter/content/browser/async_document_subresource_filter.h:38: struct InitializationParams { From looking at the members, it is not immediately clear how we should construct a ADSF for the main frame. Do we let the parent_document_origin be url::Origin(), should the parent_activation_level be the page activation level? A comment explicitly addressing main frame ADSFs would be nice.
https://codereview.chromium.org/2683413003/diff/60001/components/subresource_... File components/subresource_filter/content/browser/async_document_subresource_filter.h (right): https://codereview.chromium.org/2683413003/diff/60001/components/subresource_... components/subresource_filter/content/browser/async_document_subresource_filter.h:38: struct InitializationParams { On 2017/02/13 14:15:45, Charlie Harrison wrote: > From looking at the members, it is not immediately clear how we should construct > a ADSF for the main frame. Do we let the parent_document_origin be > url::Origin(), should the parent_activation_level be the page activation level? > > A comment explicitly addressing main frame ADSFs would be nice. Quick answer: Your interpretation is right. |parent_activation_state| should have both bool's false, and |activation_level| set to either ENABLED/DRYRUN if filtering is activated for the main frame, or DISABLED if not activated. You can simply use InitializationParams constructor with URL and |parent_activation_level| as a shortcut. Will add a comment shortly.
https://codereview.chromium.org/2683413003/diff/60001/components/subresource_... File components/subresource_filter/content/browser/async_document_subresource_filter.h (right): https://codereview.chromium.org/2683413003/diff/60001/components/subresource_... components/subresource_filter/content/browser/async_document_subresource_filter.h:38: struct InitializationParams { On 2017/02/13 14:22:50, pkalinnikov wrote: > On 2017/02/13 14:15:45, Charlie Harrison wrote: > > From looking at the members, it is not immediately clear how we should > construct > > a ADSF for the main frame. Do we let the parent_document_origin be > > url::Origin(), should the parent_activation_level be the page activation > level? > > > > A comment explicitly addressing main frame ADSFs would be nice. > > Quick answer: Your interpretation is right. |parent_activation_state| should > have both bool's false, and |activation_level| set to either ENABLED/DRYRUN if > filtering is activated for the main frame, or DISABLED if not activated. You can > simply use InitializationParams constructor with URL and > |parent_activation_level| as a shortcut. > > Will add a comment shortly. Oh, and don't forget to set the |measure_performance| bit of the |parent_activation_state| appropriately (see the ShouldMeasurePerformanceForPageLoad function in content/browser and its usage). Would you like to have this bit in the shortcut constructor?
https://codereview.chromium.org/2683413003/diff/60001/components/subresource_... File components/subresource_filter/content/browser/async_document_subresource_filter.h (right): https://codereview.chromium.org/2683413003/diff/60001/components/subresource_... components/subresource_filter/content/browser/async_document_subresource_filter.h:38: struct InitializationParams { On 2017/02/13 14:27:39, pkalinnikov wrote: > On 2017/02/13 14:22:50, pkalinnikov wrote: > > On 2017/02/13 14:15:45, Charlie Harrison wrote: > > > From looking at the members, it is not immediately clear how we should > > construct > > > a ADSF for the main frame. Do we let the parent_document_origin be > > > url::Origin(), should the parent_activation_level be the page activation > > level? > > > > > > A comment explicitly addressing main frame ADSFs would be nice. > > > > Quick answer: Your interpretation is right. |parent_activation_state| should > > have both bool's false, and |activation_level| set to either ENABLED/DRYRUN if > > filtering is activated for the main frame, or DISABLED if not activated. You > can > > simply use InitializationParams constructor with URL and > > |parent_activation_level| as a shortcut. > > > > Will add a comment shortly. > > Oh, and don't forget to set the |measure_performance| bit of the > |parent_activation_state| appropriately (see the > ShouldMeasurePerformanceForPageLoad function in content/browser and its usage). > Would you like to have this bit in the shortcut constructor? Sure that sounds useful. It seems like the shortcut constructor should not be missing anything that is required by non-test callers. We probably don't want to miss setting the measure_perf bool in production code so let's make it almost impossible to.
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 pkalinnikov@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...
PTAL again. Charlie, feel free to review the whole CL. https://codereview.chromium.org/2683413003/diff/60001/components/subresource_... File components/subresource_filter/content/browser/async_document_subresource_filter.h (right): https://codereview.chromium.org/2683413003/diff/60001/components/subresource_... components/subresource_filter/content/browser/async_document_subresource_filter.h:38: struct InitializationParams { On 2017/02/13 14:33:58, Charlie Harrison wrote: > On 2017/02/13 14:27:39, pkalinnikov wrote: > > On 2017/02/13 14:22:50, pkalinnikov wrote: > > > On 2017/02/13 14:15:45, Charlie Harrison wrote: > > > > From looking at the members, it is not immediately clear how we should > > > construct > > > > a ADSF for the main frame. Do we let the parent_document_origin be > > > > url::Origin(), should the parent_activation_level be the page activation > > > level? > > > > > > > > A comment explicitly addressing main frame ADSFs would be nice. > > > > > > Quick answer: Your interpretation is right. |parent_activation_state| should > > > have both bool's false, and |activation_level| set to either ENABLED/DRYRUN > if > > > filtering is activated for the main frame, or DISABLED if not activated. You > > can > > > simply use InitializationParams constructor with URL and > > > |parent_activation_level| as a shortcut. > > > > > > Will add a comment shortly. > > > > Oh, and don't forget to set the |measure_performance| bit of the > > |parent_activation_state| appropriately (see the > > ShouldMeasurePerformanceForPageLoad function in content/browser and its > usage). > > Would you like to have this bit in the shortcut constructor? > > Sure that sounds useful. It seems like the shortcut constructor should not be > missing anything that is required by non-test callers. We probably don't want to > miss setting the measure_perf bool in production code so let's make it almost > impossible to. Does it look good to you now?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Generally this CL lgtm, thanks for the quick responses https://codereview.chromium.org/2683413003/diff/60001/components/subresource_... File components/subresource_filter/content/browser/async_document_subresource_filter.h (right): https://codereview.chromium.org/2683413003/diff/60001/components/subresource_... components/subresource_filter/content/browser/async_document_subresource_filter.h:38: struct InitializationParams { On 2017/02/13 15:26:49, pkalinnikov wrote: > On 2017/02/13 14:33:58, Charlie Harrison wrote: > > On 2017/02/13 14:27:39, pkalinnikov wrote: > > > On 2017/02/13 14:22:50, pkalinnikov wrote: > > > > On 2017/02/13 14:15:45, Charlie Harrison wrote: > > > > > From looking at the members, it is not immediately clear how we should > > > > construct > > > > > a ADSF for the main frame. Do we let the parent_document_origin be > > > > > url::Origin(), should the parent_activation_level be the page activation > > > > level? > > > > > > > > > > A comment explicitly addressing main frame ADSFs would be nice. > > > > > > > > Quick answer: Your interpretation is right. |parent_activation_state| > should > > > > have both bool's false, and |activation_level| set to either > ENABLED/DRYRUN > > if > > > > filtering is activated for the main frame, or DISABLED if not activated. > You > > > can > > > > simply use InitializationParams constructor with URL and > > > > |parent_activation_level| as a shortcut. > > > > > > > > Will add a comment shortly. > > > > > > Oh, and don't forget to set the |measure_performance| bit of the > > > |parent_activation_state| appropriately (see the > > > ShouldMeasurePerformanceForPageLoad function in content/browser and its > > usage). > > > Would you like to have this bit in the shortcut constructor? > > > > Sure that sounds useful. It seems like the shortcut constructor should not be > > missing anything that is required by non-test callers. We probably don't want > to > > miss setting the measure_perf bool in production code so let's make it almost > > impossible to. > > Does it look good to you now? Yep!
A few nits and one thing I caught using this class in unit tests. https://codereview.chromium.org/2683413003/diff/80001/components/subresource_... File components/subresource_filter/content/browser/async_document_subresource_filter.cc (right): https://codereview.chromium.org/2683413003/diff/80001/components/subresource_... components/subresource_filter/content/browser/async_document_subresource_filter.cc:131: filter_.emplace(url::Origin(params.document_url), activation_state, ruleset, Can you wrap this in a if (activation_state.activation_level != ActivationLevel::DISABLED)? Otherwise, if we try to initialize an ADSF with a main frame whose page isn't activated, we'll DCHECK in DocumentSubresourceFilter's constructor. If we don't want to allow creating a main frame ADSF with page-level activation disabled, let's not include the activation level bool in the "main frame" initialization param constructor. Also, a test that goes through this path would be appreciated. https://codereview.chromium.org/2683413003/diff/80001/components/subresource_... File components/subresource_filter/content/browser/async_document_subresource_filter_unittest.cc (right): https://codereview.chromium.org/2683413003/diff/80001/components/subresource_... components/subresource_filter/content/browser/async_document_subresource_filter_unittest.cc:77: std::unique_ptr<VerifiedRulesetDealer::Handle> dealer_handle_; #include <memory> https://codereview.chromium.org/2683413003/diff/80001/components/subresource_... components/subresource_filter/content/browser/async_document_subresource_filter_unittest.cc:79: DISALLOW_COPY_AND_ASSIGN(AsyncDocumentSubresourceFilterTest); #include "base/macros.h"
The CQ bit was checked by pkalinnikov@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...
Addressed. Thanks! https://codereview.chromium.org/2683413003/diff/80001/components/subresource_... File components/subresource_filter/content/browser/async_document_subresource_filter.cc (right): https://codereview.chromium.org/2683413003/diff/80001/components/subresource_... components/subresource_filter/content/browser/async_document_subresource_filter.cc:131: filter_.emplace(url::Origin(params.document_url), activation_state, ruleset, On 2017/02/13 20:07:50, Charlie Harrison wrote: > Can you wrap this in a if (activation_state.activation_level != > ActivationLevel::DISABLED)? > > > Otherwise, if we try to initialize an ADSF with a main frame whose page isn't > activated, we'll DCHECK in DocumentSubresourceFilter's constructor. > > If we don't want to allow creating a main frame ADSF with page-level activation > disabled, let's not include the activation level bool in the "main frame" > initialization param constructor. > > Also, a test that goes through this path would be appreciated. Just like with DocumentSubresourceFilter, let's not try creating an ADSF with DISABLED activation level in the first place. For this purpose I added DCHECKs here and in couple places above. As for tests, there is no need to exercise this path any more, because it's forbidden. We can't completely eliminate ActivationLevel from initialization param constructor, because there should still be an option between ENABLED and DRYRUN. I could to an |is_dry_run| bool instead, but this seems less future-proof. Does this sound good? https://codereview.chromium.org/2683413003/diff/80001/components/subresource_... File components/subresource_filter/content/browser/async_document_subresource_filter_unittest.cc (right): https://codereview.chromium.org/2683413003/diff/80001/components/subresource_... components/subresource_filter/content/browser/async_document_subresource_filter_unittest.cc:77: std::unique_ptr<VerifiedRulesetDealer::Handle> dealer_handle_; On 2017/02/13 20:07:50, Charlie Harrison wrote: > #include <memory> Done. https://codereview.chromium.org/2683413003/diff/80001/components/subresource_... components/subresource_filter/content/browser/async_document_subresource_filter_unittest.cc:79: DISALLOW_COPY_AND_ASSIGN(AsyncDocumentSubresourceFilterTest); On 2017/02/13 20:07:50, Charlie Harrison wrote: > #include "base/macros.h" Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2683413003/diff/80001/components/subresource_... File components/subresource_filter/content/browser/async_document_subresource_filter.cc (right): https://codereview.chromium.org/2683413003/diff/80001/components/subresource_... components/subresource_filter/content/browser/async_document_subresource_filter.cc:131: filter_.emplace(url::Origin(params.document_url), activation_state, ruleset, On 2017/02/14 09:31:07, pkalinnikov wrote: > On 2017/02/13 20:07:50, Charlie Harrison wrote: > > Can you wrap this in a if (activation_state.activation_level != > > ActivationLevel::DISABLED)? > > > > > > Otherwise, if we try to initialize an ADSF with a main frame whose page isn't > > activated, we'll DCHECK in DocumentSubresourceFilter's constructor. > > > > If we don't want to allow creating a main frame ADSF with page-level > activation > > disabled, let's not include the activation level bool in the "main frame" > > initialization param constructor. > > > > Also, a test that goes through this path would be appreciated. > > Just like with DocumentSubresourceFilter, let's not try creating an ADSF with > DISABLED activation level in the first place. For this purpose I added DCHECKs > here and in couple places above. > > As for tests, there is no need to exercise this path any more, because it's > forbidden. > > We can't completely eliminate ActivationLevel from initialization param > constructor, because there should still be an option between ENABLED and DRYRUN. > I could to an |is_dry_run| bool instead, but this seems less future-proof. > > Does this sound good? I prefer just DCHECKing the activation state is not DISABLED, rather than using an opaque bool. What you have now lgtm
Looks great, thanks! LGTM % one or two comments, and some nits and wordsmithing. https://codereview.chromium.org/2683413003/diff/100001/components/subresource... File components/subresource_filter/content/browser/async_document_subresource_filter.cc (right): https://codereview.chromium.org/2683413003/diff/100001/components/subresource... components/subresource_filter/content/browser/async_document_subresource_filter.cc:53: VerifiedRuleset::Handle* ruleset_handle, Based on how Charles is planning to use this, have you thought about taking a unique_ptr<VR::Handle> here? https://codereview.chromium.org/2683413003/diff/100001/components/subresource... components/subresource_filter/content/browser/async_document_subresource_filter.cc:60: std::move(first_disallowed_load_callback)) { nit: #include <utility> for std::move. https://codereview.chromium.org/2683413003/diff/100001/components/subresource... components/subresource_filter/content/browser/async_document_subresource_filter.cc:87: DocumentSubresourceFilter* filter = core->filter(); nit: Do you expect it to have a measurable performance impact if I really wanted to save some lines by just inlining core->filter()? Same below. https://codereview.chromium.org/2683413003/diff/100001/components/subresource... components/subresource_filter/content/browser/async_document_subresource_filter.cc:104: auto report_disallowed_load = [](AsyncDocumentSubresourceFilter::Core* core) { nit: How would it look if we inlined this lambda (provided it works with base::Bind)? https://codereview.chromium.org/2683413003/diff/100001/components/subresource... components/subresource_filter/content/browser/async_document_subresource_filter.cc:130: const MemoryMappedRuleset* ruleset = verified_ruleset->Get(); nit: Same here? https://codereview.chromium.org/2683413003/diff/100001/components/subresource... components/subresource_filter/content/browser/async_document_subresource_filter.cc:140: base::OnceClosure()); Let's add a comment to explain why this callback is never used. https://codereview.chromium.org/2683413003/diff/100001/components/subresource... File components/subresource_filter/content/browser/async_document_subresource_filter.h (right): https://codereview.chromium.org/2683413003/diff/100001/components/subresource... components/subresource_filter/content/browser/async_document_subresource_filter.h:25: // It is accessed on the UI thread and owns an ADSF::Core living on a dedicated nit: Given that the existence of Core is an implementation details that clients of this class might not necessarily be interested in, how about mentioning DSF in client-facing comments? https://codereview.chromium.org/2683413003/diff/100001/components/subresource... components/subresource_filter/content/browser/async_document_subresource_filter.h:36: // computing ActivationState for a (sub-)document, and for creating a nit: I'd suggest swapping the two parts, so that the semantic meaning comes first: Encapsulates the parameters needed for computing the frame-level ActivationState appropriate for the frame this ASDF is created for. These parameters are posted to ADSF::Core during its initialization. https://codereview.chromium.org/2683413003/diff/100001/components/subresource... components/subresource_filter/content/browser/async_document_subresource_filter.h:41: // Creates parameters for a main-frame |document|. |activation_level| should nit: How about phrasing this an its sibling like so: Takes parameters needed for calculating the main-frame ActivationState, that is: the main-frame |document| URL, the page-level |activation_level|, and whether or not to |measure_performance|. https://codereview.chromium.org/2683413003/diff/100001/components/subresource... components/subresource_filter/content/browser/async_document_subresource_filter.h:60: GURL document_url; nit: #include "url/gurl.h" https://codereview.chromium.org/2683413003/diff/100001/components/subresource... components/subresource_filter/content/browser/async_document_subresource_filter.h:70: // |ruleset_handle|. The instance remains owned by |this| object, but lives on nit: s/instance/core/ https://codereview.chromium.org/2683413003/diff/100001/components/subresource... components/subresource_filter/content/browser/async_document_subresource_filter.h:111: // Holds a DocumentSubresourceFilter (after Initialization, in case there is a Comment nit: // Holds a DocumentSubresourceFilter that is created in a deferred manner in Initialize(), provided there is a valid ruleset to work with. https://codereview.chromium.org/2683413003/diff/100001/components/subresource... components/subresource_filter/content/browser/async_document_subresource_filter.h:114: // Initially holds an empty filter, allowing this object to be created on the nit: What do you think about moving this paragraph (with minimal adjustments) to the comment above the ASDF above, so that the reader is not left in suspense wondering about the purpose of the Core? https://codereview.chromium.org/2683413003/diff/100001/components/subresource... File components/subresource_filter/content/browser/async_document_subresource_filter_unittest.cc (right): https://codereview.chromium.org/2683413003/diff/100001/components/subresource... components/subresource_filter/content/browser/async_document_subresource_filter_unittest.cc:10: #include "base/bind.h" nit: #include "base/bind_helpers.h" https://codereview.chromium.org/2683413003/diff/100001/components/subresource... components/subresource_filter/content/browser/async_document_subresource_filter_unittest.cc:33: "child2.com", proto::ACTIVATION_TYPE_GENERICBLOCK, nit: How about whilelisted.subframe.com and example.com for readability? https://codereview.chromium.org/2683413003/diff/100001/components/subresource... components/subresource_filter/content/browser/async_document_subresource_filter_unittest.cc:35: rules.push_back(testing::CreateSuffixRule("block-this.html")); nit: disallowed.html https://codereview.chromium.org/2683413003/diff/100001/components/subresource... components/subresource_filter/content/browser/async_document_subresource_filter_unittest.cc:66: return std::unique_ptr<VerifiedRuleset::Handle>( nit: Can we use MakeUnique for this? https://codereview.chromium.org/2683413003/diff/100001/components/subresource... components/subresource_filter/content/browser/async_document_subresource_filter_unittest.cc:76: testing::TestRulesetCreator test_ruleset_creator_; nit: Can there be any tasks left in these task runners that hold file handles to test rulesets? If so, we should move line 76 and 77 to the first place, so that the task runners are pumped out, all potential file handles are closed, and temp directories can be cleaned up. https://codereview.chromium.org/2683413003/diff/100001/components/subresource... components/subresource_filter/content/browser/async_document_subresource_filter_unittest.cc:94: void Expect(ActivationState expected_state) const { nit: How about making the expectations in the tests more specific by naming this ExpectReceivedOnce? Same below. https://codereview.chromium.org/2683413003/diff/100001/components/subresource... components/subresource_filter/content/browser/async_document_subresource_filter_unittest.cc:105: ActivationState activation_state_; nit: last_activation_state_; https://codereview.chromium.org/2683413003/diff/100001/components/subresource... components/subresource_filter/content/browser/async_document_subresource_filter_unittest.cc:148: AsyncDocumentSubresourceFilter::LoadPolicy load_policy_; nit: last_load_policy_;
The CQ bit was checked by pkalinnikov@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...
Addressed, thanks! engedy@: The are couple of questions. PTAL. csharrison@: There is one question for you about the the usage of the handle. PTAL. https://codereview.chromium.org/2683413003/diff/100001/components/subresource... File components/subresource_filter/content/browser/async_document_subresource_filter.cc (right): https://codereview.chromium.org/2683413003/diff/100001/components/subresource... components/subresource_filter/content/browser/async_document_subresource_filter.cc:53: VerifiedRuleset::Handle* ruleset_handle, On 2017/02/14 15:13:13, engedy wrote: > Based on how Charles is planning to use this, have you thought about taking a > unique_ptr<VR::Handle> here? Charles, can you please tell how you are going to use ruleset handles? My intention was: Let us have a single ruleset handle (which is created as soon as the main-frame throttle sees a possibility to use it in the close future, and which then lives throughout the whole page load unless the throttle sees that it won't actually be used), and use it for all the subframes to create ADSFs. PRO: all the subframes get the same version of the ruleset. Your proposal seems different, because it transfers ownership from handle to ADSF. And the solution when we grab a new handle for each subframe doesn't have the above mentioned PRO. What do you think? https://codereview.chromium.org/2683413003/diff/100001/components/subresource... components/subresource_filter/content/browser/async_document_subresource_filter.cc:60: std::move(first_disallowed_load_callback)) { On 2017/02/14 15:13:13, engedy wrote: > nit: #include <utility> for std::move. Done. https://codereview.chromium.org/2683413003/diff/100001/components/subresource... components/subresource_filter/content/browser/async_document_subresource_filter.cc:87: DocumentSubresourceFilter* filter = core->filter(); On 2017/02/14 15:13:13, engedy wrote: > nit: Do you expect it to have a measurable performance impact if I really wanted > to save some lines by just inlining core->filter()? Same below. Funny thing is that it doesn't save the line, because the one below becomes too long and gets split :) I don't see other benefits, so I propose to stick with the current code. Same concerns the lambda below. https://codereview.chromium.org/2683413003/diff/100001/components/subresource... components/subresource_filter/content/browser/async_document_subresource_filter.cc:104: auto report_disallowed_load = [](AsyncDocumentSubresourceFilter::Core* core) { On 2017/02/14 15:13:13, engedy wrote: > nit: How would it look if we inlined this lambda (provided it works with > base::Bind)? Okay, this doesn't look too bad :) https://codereview.chromium.org/2683413003/diff/100001/components/subresource... components/subresource_filter/content/browser/async_document_subresource_filter.cc:130: const MemoryMappedRuleset* ruleset = verified_ruleset->Get(); On 2017/02/14 15:13:13, engedy wrote: > nit: Same here? Done. https://codereview.chromium.org/2683413003/diff/100001/components/subresource... components/subresource_filter/content/browser/async_document_subresource_filter.cc:140: base::OnceClosure()); On 2017/02/14 15:13:13, engedy wrote: > Let's add a comment to explain why this callback is never used. Done. https://codereview.chromium.org/2683413003/diff/100001/components/subresource... File components/subresource_filter/content/browser/async_document_subresource_filter.h (right): https://codereview.chromium.org/2683413003/diff/100001/components/subresource... components/subresource_filter/content/browser/async_document_subresource_filter.h:25: // It is accessed on the UI thread and owns an ADSF::Core living on a dedicated On 2017/02/14 15:13:13, engedy wrote: > nit: Given that the existence of Core is an implementation details that clients > of this class might not necessarily be interested in, how about mentioning DSF > in client-facing comments? Done. https://codereview.chromium.org/2683413003/diff/100001/components/subresource... components/subresource_filter/content/browser/async_document_subresource_filter.h:36: // computing ActivationState for a (sub-)document, and for creating a On 2017/02/14 15:13:14, engedy wrote: > nit: I'd suggest swapping the two parts, so that the semantic meaning comes > first: > > Encapsulates the parameters needed for computing the frame-level ActivationState > appropriate for the frame this ASDF is created for. These parameters are posted > to ADSF::Core during its initialization. Done. https://codereview.chromium.org/2683413003/diff/100001/components/subresource... components/subresource_filter/content/browser/async_document_subresource_filter.h:41: // Creates parameters for a main-frame |document|. |activation_level| should On 2017/02/14 15:13:14, engedy wrote: > nit: How about phrasing this an its sibling like so: > > Takes parameters needed for calculating the main-frame ActivationState, that is: > the main-frame |document| URL, the page-level |activation_level|, and whether or > not to |measure_performance|. Done. https://codereview.chromium.org/2683413003/diff/100001/components/subresource... components/subresource_filter/content/browser/async_document_subresource_filter.h:60: GURL document_url; On 2017/02/14 15:13:14, engedy wrote: > nit: #include "url/gurl.h" Done. https://codereview.chromium.org/2683413003/diff/100001/components/subresource... components/subresource_filter/content/browser/async_document_subresource_filter.h:70: // |ruleset_handle|. The instance remains owned by |this| object, but lives on On 2017/02/14 15:13:14, engedy wrote: > nit: s/instance/core/ Done. https://codereview.chromium.org/2683413003/diff/100001/components/subresource... components/subresource_filter/content/browser/async_document_subresource_filter.h:111: // Holds a DocumentSubresourceFilter (after Initialization, in case there is a On 2017/02/14 15:13:14, engedy wrote: > Comment nit: > > // Holds a DocumentSubresourceFilter that is created in a deferred manner in > Initialize(), provided there is a valid ruleset to work with. Done. https://codereview.chromium.org/2683413003/diff/100001/components/subresource... components/subresource_filter/content/browser/async_document_subresource_filter.h:114: // Initially holds an empty filter, allowing this object to be created on the On 2017/02/14 15:13:14, engedy wrote: > nit: What do you think about moving this paragraph (with minimal adjustments) to > the comment above the ASDF above, so that the reader is not left in suspense > wondering about the purpose of the Core? Done. https://codereview.chromium.org/2683413003/diff/100001/components/subresource... File components/subresource_filter/content/browser/async_document_subresource_filter_unittest.cc (right): https://codereview.chromium.org/2683413003/diff/100001/components/subresource... components/subresource_filter/content/browser/async_document_subresource_filter_unittest.cc:10: #include "base/bind.h" On 2017/02/14 15:13:14, engedy wrote: > nit: #include "base/bind_helpers.h" Done. https://codereview.chromium.org/2683413003/diff/100001/components/subresource... components/subresource_filter/content/browser/async_document_subresource_filter_unittest.cc:33: "child2.com", proto::ACTIVATION_TYPE_GENERICBLOCK, On 2017/02/14 15:13:14, engedy wrote: > nit: How about http://whilelisted.subframe.com and http://example.com for readability? Done. https://codereview.chromium.org/2683413003/diff/100001/components/subresource... components/subresource_filter/content/browser/async_document_subresource_filter_unittest.cc:35: rules.push_back(testing::CreateSuffixRule("block-this.html")); On 2017/02/14 15:13:14, engedy wrote: > nit: disallowed.html Done. https://codereview.chromium.org/2683413003/diff/100001/components/subresource... components/subresource_filter/content/browser/async_document_subresource_filter_unittest.cc:66: return std::unique_ptr<VerifiedRuleset::Handle>( On 2017/02/14 15:13:14, engedy wrote: > nit: Can we use MakeUnique for this? Done. https://codereview.chromium.org/2683413003/diff/100001/components/subresource... components/subresource_filter/content/browser/async_document_subresource_filter_unittest.cc:76: testing::TestRulesetCreator test_ruleset_creator_; On 2017/02/14 15:13:14, engedy wrote: > nit: Can there be any tasks left in these task runners that hold file handles to > test rulesets? If so, we should move line 76 and 77 to the first place, so that > the task runners are pumped out, all potential file handles are closed, and temp > directories can be cleaned up. We have RunUntilIdle() in TearDown(), but still let's do this permutation to be on the safe side. https://codereview.chromium.org/2683413003/diff/100001/components/subresource... components/subresource_filter/content/browser/async_document_subresource_filter_unittest.cc:94: void Expect(ActivationState expected_state) const { On 2017/02/14 15:13:14, engedy wrote: > nit: How about making the expectations in the tests more specific by naming this > ExpectReceivedOnce? Same below. Done. https://codereview.chromium.org/2683413003/diff/100001/components/subresource... components/subresource_filter/content/browser/async_document_subresource_filter_unittest.cc:105: ActivationState activation_state_; On 2017/02/14 15:13:14, engedy wrote: > nit: last_activation_state_; Done. https://codereview.chromium.org/2683413003/diff/100001/components/subresource... components/subresource_filter/content/browser/async_document_subresource_filter_unittest.cc:148: AsyncDocumentSubresourceFilter::LoadPolicy load_policy_; On 2017/02/14 15:13:14, engedy wrote: > nit: last_load_policy_; Done.
https://codereview.chromium.org/2683413003/diff/100001/components/subresource... File components/subresource_filter/content/browser/async_document_subresource_filter.cc (right): https://codereview.chromium.org/2683413003/diff/100001/components/subresource... components/subresource_filter/content/browser/async_document_subresource_filter.cc:53: VerifiedRuleset::Handle* ruleset_handle, On 2017/02/14 18:47:13, pkalinnikov wrote: > On 2017/02/14 15:13:13, engedy wrote: > > Based on how Charles is planning to use this, have you thought about taking a > > unique_ptr<VR::Handle> here? > > Charles, can you please tell how you are going to use ruleset handles? > > My intention was: Let us have a single ruleset handle (which is created as soon > as the main-frame throttle sees a possibility to use it in the close future, and > which then lives throughout the whole page load unless the throttle sees that it > won't actually be used), and use it for all the subframes to create ADSFs. PRO: > all the subframes get the same version of the ruleset. > > Your proposal seems different, because it transfers ownership from handle to > ADSF. And the solution when we grab a new handle for each subframe doesn't have > the above mentioned PRO. > > What do you think? Your solution sounds good to me. I was thinking that the page-level structure would get a single ruleset *dealer* handle, out of which all ASDFs would generate their handles. Wouldn't this achieve the same pro? In any case I think I prefer your approach because generating the handle's is not free and generates extra initialization tasks on the blocking task queue.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Still LGTM, thanks! See replies below. https://codereview.chromium.org/2683413003/diff/100001/components/subresource... File components/subresource_filter/content/browser/async_document_subresource_filter.cc (right): https://codereview.chromium.org/2683413003/diff/100001/components/subresource... components/subresource_filter/content/browser/async_document_subresource_filter.cc:53: VerifiedRuleset::Handle* ruleset_handle, On 2017/02/14 19:31:06, Charlie Harrison wrote: > On 2017/02/14 18:47:13, pkalinnikov wrote: > > On 2017/02/14 15:13:13, engedy wrote: > > > Based on how Charles is planning to use this, have you thought about taking > a > > > unique_ptr<VR::Handle> here? > > > > Charles, can you please tell how you are going to use ruleset handles? > > > > My intention was: Let us have a single ruleset handle (which is created as > soon > > as the main-frame throttle sees a possibility to use it in the close future, > and > > which then lives throughout the whole page load unless the throttle sees that > it > > won't actually be used), and use it for all the subframes to create ADSFs. > PRO: > > all the subframes get the same version of the ruleset. > > > > Your proposal seems different, because it transfers ownership from handle to > > ADSF. And the solution when we grab a new handle for each subframe doesn't > have > > the above mentioned PRO. > > > > What do you think? > > Your solution sounds good to me. I was thinking that the page-level structure > would get a single ruleset *dealer* handle, out of which all ASDFs would > generate their handles. Wouldn't this achieve the same pro? In any case I think > I prefer your approach because generating the handle's is not free and generates > extra initialization tasks on the blocking task queue. Currently the verification state is persisted on the VRDealer, so there should be only one instance of that per browser process to avoid multiple verifications taking place. This is also more consistent with the dealer being a per-process singleton in the renderers. Other than that, the only important thing is that we should not have any VR::Handle instances created unless there is a good chance of page-level activation. If we can nicely maintain this shared instance on the page level, I'm fine with that. I agree that both using the same version of the ruleset across all frames in page, as well as avoiding initializing per-frame VR::Handles are nice, but neither of them are hard constraints. https://codereview.chromium.org/2683413003/diff/100001/components/subresource... components/subresource_filter/content/browser/async_document_subresource_filter.cc:87: DocumentSubresourceFilter* filter = core->filter(); On 2017/02/14 18:47:13, pkalinnikov wrote: > On 2017/02/14 15:13:13, engedy wrote: > > nit: Do you expect it to have a measurable performance impact if I really > wanted > > to save some lines by just inlining core->filter()? Same below. > > Funny thing is that it doesn't save the line, because the one below becomes too > long and gets split :) > I don't see other benefits, so I propose to stick with the current code. Same > concerns the lambda below. Fair enough. ;-)
The CQ bit was checked by pkalinnikov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from csharrison@chromium.org Link to the patchset: https://codereview.chromium.org/2683413003/#ps120001 (title: "Address more comments from engedy@.")
The CQ bit was unchecked by pkalinnikov@chromium.org
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 AsyncDocumentSubresourceFilter. BUG=637415 ========== to ========== Introduce AsyncDocumentSubresourceFilter. AsyncDocumentSubresourceFilter (ADSF) is an asynchronous wrapper around DocumentSubresourceFilter (DSF). When ADSF is constructed, it creates an ADSF::Core and initializes it asynchronously on a SequencedTaskRunner using the supplied initialization |params| and a VerifiedRuleset. The task runner and VerifiedRuleset are taken from VerifiedRuleset::Handle provided to the constructor. ADSF::Core remains owned by ADSF, but lives on (and is accessed on) the task runner. During ADSF::Core initialization an ActivationState for the current frame is calculated, and then reported back via |activation_state_callback| on the task runner associated with the ADSF creator's thread (normally, the UI thread). If MemoryMappedRuleset is not present or malformed, then a default ActivationState is reported (with ActivationLevel equal to DISABLED). Although the full initialization of ADSF/ADSF::Core is asynchronous, the properties of SequencedTaskRunner allow the ADSF instance to be immediately used after construction. The key purpose of ADSF is providing decisions on whether to allow loading subframes for the current frame. The GetLoadPolicyForSubdocument async method serves this purpose, and boils down to calling the same-named method of DSF on the task runner and returning the result to the caller's task runner. BUG=637415 ==========
pkalinnikov@chromium.org changed reviewers: + jochen@chromium.org
Jochen, please review the added DEPS from "third_party/WebKit/public/platform".
https://codereview.chromium.org/2683413003/diff/120001/components/subresource... File components/subresource_filter/content/browser/DEPS (right): https://codereview.chromium.org/2683413003/diff/120001/components/subresource... components/subresource_filter/content/browser/DEPS:5: "+third_party/WebKit/public/platform", please only depend on individual headers https://codereview.chromium.org/2683413003/diff/120001/components/subresource... File components/subresource_filter/content/browser/async_document_subresource_filter.h (right): https://codereview.chromium.org/2683413003/diff/120001/components/subresource... components/subresource_filter/content/browser/async_document_subresource_filter.h:19: #include "third_party/WebKit/public/platform/WebDocumentSubresourceFilter.h" you can only include POD and enum headers from the browser
The CQ bit was checked by pkalinnikov@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...
Jochen, PTAL again. https://codereview.chromium.org/2683413003/diff/120001/components/subresource... File components/subresource_filter/content/browser/DEPS (right): https://codereview.chromium.org/2683413003/diff/120001/components/subresource... components/subresource_filter/content/browser/DEPS:5: "+third_party/WebKit/public/platform", On 2017/02/15 10:32:10, jochen wrote: > please only depend on individual headers How about this? https://codereview.chromium.org/2683413003/diff/120001/components/subresource... File components/subresource_filter/content/browser/async_document_subresource_filter.h (right): https://codereview.chromium.org/2683413003/diff/120001/components/subresource... components/subresource_filter/content/browser/async_document_subresource_filter.h:19: #include "third_party/WebKit/public/platform/WebDocumentSubresourceFilter.h" On 2017/02/15 10:32:11, jochen wrote: > you can only include POD and enum headers from the browser Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by pkalinnikov@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_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by pkalinnikov@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_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by pkalinnikov@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.
On 2017/02/15 11:47:40, pkalinnikov wrote: > Jochen, PTAL again. > > https://codereview.chromium.org/2683413003/diff/120001/components/subresource... > File components/subresource_filter/content/browser/DEPS (right): > > https://codereview.chromium.org/2683413003/diff/120001/components/subresource... > components/subresource_filter/content/browser/DEPS:5: > "+third_party/WebKit/public/platform", > On 2017/02/15 10:32:10, jochen wrote: > > please only depend on individual headers > > How about this? > > https://codereview.chromium.org/2683413003/diff/120001/components/subresource... > File > components/subresource_filter/content/browser/async_document_subresource_filter.h > (right): > > https://codereview.chromium.org/2683413003/diff/120001/components/subresource... > components/subresource_filter/content/browser/async_document_subresource_filter.h:19: > #include "third_party/WebKit/public/platform/WebDocumentSubresourceFilter.h" > On 2017/02/15 10:32:11, jochen wrote: > > you can only include POD and enum headers from the browser > > Done. Jochen, friendly ping to take a look. Please notice that there is no #include DEP any more. However, there is a typedef chain leading through c/s_f/content/common/ to a WebKit enum (which is already in DEPS for content/common). WDYT?
https://codereview.chromium.org/2683413003/diff/200001/components/subresource... File components/subresource_filter/content/common/document_subresource_filter.h (right): https://codereview.chromium.org/2683413003/diff/200001/components/subresource... components/subresource_filter/content/common/document_subresource_filter.h:24: #include "third_party/WebKit/public/platform/WebDocumentSubresourceFilterLoadPolicy.h" please change subresource_filter/content/common/DEPS to enumerate individual webkit headers, instead of allowing blanket inclusion. also in common/ we can only depend on POD and enum headers. https://codereview.chromium.org/2683413003/diff/200001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebDocumentSubresourceFilterLoadPolicy.h (right): https://codereview.chromium.org/2683413003/diff/200001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebDocumentSubresourceFilterLoadPolicy.h:11: Allow, please prefix enum constants with k
Patchset #12 (id:220001) has been deleted
The CQ bit was checked by pkalinnikov@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 AsyncDocumentSubresourceFilter. AsyncDocumentSubresourceFilter (ADSF) is an asynchronous wrapper around DocumentSubresourceFilter (DSF). When ADSF is constructed, it creates an ADSF::Core and initializes it asynchronously on a SequencedTaskRunner using the supplied initialization |params| and a VerifiedRuleset. The task runner and VerifiedRuleset are taken from VerifiedRuleset::Handle provided to the constructor. ADSF::Core remains owned by ADSF, but lives on (and is accessed on) the task runner. During ADSF::Core initialization an ActivationState for the current frame is calculated, and then reported back via |activation_state_callback| on the task runner associated with the ADSF creator's thread (normally, the UI thread). If MemoryMappedRuleset is not present or malformed, then a default ActivationState is reported (with ActivationLevel equal to DISABLED). Although the full initialization of ADSF/ADSF::Core is asynchronous, the properties of SequencedTaskRunner allow the ADSF instance to be immediately used after construction. The key purpose of ADSF is providing decisions on whether to allow loading subframes for the current frame. The GetLoadPolicyForSubdocument async method serves this purpose, and boils down to calling the same-named method of DSF on the task runner and returning the result to the caller's task runner. BUG=637415 ========== to ========== Introduce AsyncDocumentSubresourceFilter. AsyncDocumentSubresourceFilter (ADSF) is an asynchronous wrapper around DocumentSubresourceFilter (DSF). When ADSF is constructed, it creates an ADSF::Core and initializes it asynchronously on a SequencedTaskRunner using the supplied initialization |params| and a VerifiedRuleset. The task runner and VerifiedRuleset are taken from VerifiedRuleset::Handle provided to the constructor. ADSF::Core remains owned by ADSF, but lives on (and is accessed on) the task runner. During ADSF::Core initialization an ActivationState for the current frame is calculated, and then reported back via |activation_state_callback| on the task runner associated with the ADSF creator's thread (normally, the UI thread). If MemoryMappedRuleset is not present or malformed, then a default ActivationState is reported (with ActivationLevel equal to DISABLED). Although the full initialization of ADSF/ADSF::Core is asynchronous, the properties of SequencedTaskRunner allow the ADSF instance to be immediately used after construction. The key purpose of ADSF is providing decisions on whether to allow loading subframes for the current frame. The GetLoadPolicyForSubdocument async method serves this purpose, and boils down to calling the GetLoadPolicy method of DSF on the task runner and returning the result to the caller's task runner. BUG=637415 ==========
pkalinnikov@chromium.org changed reviewers: - jochen@chromium.org
Balazs, can you make a final round? Jochen, I managed to get rid of the dependency here and in common/ (see https://codereview.chromium.org/2697363005/). Since this CL doesn't add new DEPS any more, I am moving you to CC. Thank you for forcing this refactoring. https://codereview.chromium.org/2683413003/diff/200001/components/subresource... File components/subresource_filter/content/common/document_subresource_filter.h (right): https://codereview.chromium.org/2683413003/diff/200001/components/subresource... components/subresource_filter/content/common/document_subresource_filter.h:24: #include "third_party/WebKit/public/platform/WebDocumentSubresourceFilterLoadPolicy.h" On 2017/02/16 14:46:21, jochen wrote: > please change subresource_filter/content/common/DEPS to enumerate individual > webkit headers, instead of allowing blanket inclusion. > > also in common/ we can only depend on POD and enum headers. Now common/ doesn't depend on blink. https://codereview.chromium.org/2683413003/diff/200001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebDocumentSubresourceFilterLoadPolicy.h (right): https://codereview.chromium.org/2683413003/diff/200001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebDocumentSubresourceFilterLoadPolicy.h:11: Allow, On 2017/02/16 14:46:21, jochen wrote: > please prefix enum constants with k I will make it in a separate fixup CL.
Guys, please disregard patchsets 8-11.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for cleaning up the dependencies, Pavel. LGTM.
The CQ bit was checked by pkalinnikov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from csharrison@chromium.org Link to the patchset: https://codereview.chromium.org/2683413003/#ps240001 (title: "Rebase and clean up.")
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
Failed to apply patch for
components/subresource_filter/content/browser/BUILD.gn:
While running git apply --index -p1;
error: patch failed: components/subresource_filter/content/browser/BUILD.gn:4
error: components/subresource_filter/content/browser/BUILD.gn: patch does not
apply
Patch: components/subresource_filter/content/browser/BUILD.gn
Index: components/subresource_filter/content/browser/BUILD.gn
diff --git a/components/subresource_filter/content/browser/BUILD.gn
b/components/subresource_filter/content/browser/BUILD.gn
index
b55d108e2ae7fed52f9105790931ab16ef56b35b..7daa674b518469f7ea074b36ad5ca8dbed146edd
100644
--- a/components/subresource_filter/content/browser/BUILD.gn
+++ b/components/subresource_filter/content/browser/BUILD.gn
@@ -4,6 +4,8 @@
static_library("browser") {
sources = [
+ "async_document_subresource_filter.cc",
+ "async_document_subresource_filter.h",
"content_ruleset_service_delegate.cc",
"content_ruleset_service_delegate.h",
"content_subresource_filter_driver.cc",
@@ -33,6 +35,7 @@ static_library("browser") {
source_set("unit_tests") {
testonly = true
sources = [
+ "async_document_subresource_filter_unittest.cc",
"content_ruleset_service_delegate_unittest.cc",
"content_subresource_filter_driver_factory_unittest.cc",
"verified_ruleset_dealer_unittest.cc",
The CQ bit was checked by pkalinnikov@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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by pkalinnikov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from engedy@chromium.org, csharrison@chromium.org Link to the patchset: https://codereview.chromium.org/2683413003/#ps260001 (title: "Rebase.")
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": 260001, "attempt_start_ts": 1487609580439170,
"parent_rev": "7957c30c9151965b069f469b51cd97d3349232e8", "commit_rev":
"3e809d1f60ee19d1021842752704717001f1e0b4"}
Message was sent while issue was closed.
Description was changed from ========== Introduce AsyncDocumentSubresourceFilter. AsyncDocumentSubresourceFilter (ADSF) is an asynchronous wrapper around DocumentSubresourceFilter (DSF). When ADSF is constructed, it creates an ADSF::Core and initializes it asynchronously on a SequencedTaskRunner using the supplied initialization |params| and a VerifiedRuleset. The task runner and VerifiedRuleset are taken from VerifiedRuleset::Handle provided to the constructor. ADSF::Core remains owned by ADSF, but lives on (and is accessed on) the task runner. During ADSF::Core initialization an ActivationState for the current frame is calculated, and then reported back via |activation_state_callback| on the task runner associated with the ADSF creator's thread (normally, the UI thread). If MemoryMappedRuleset is not present or malformed, then a default ActivationState is reported (with ActivationLevel equal to DISABLED). Although the full initialization of ADSF/ADSF::Core is asynchronous, the properties of SequencedTaskRunner allow the ADSF instance to be immediately used after construction. The key purpose of ADSF is providing decisions on whether to allow loading subframes for the current frame. The GetLoadPolicyForSubdocument async method serves this purpose, and boils down to calling the GetLoadPolicy method of DSF on the task runner and returning the result to the caller's task runner. BUG=637415 ========== to ========== Introduce AsyncDocumentSubresourceFilter. AsyncDocumentSubresourceFilter (ADSF) is an asynchronous wrapper around DocumentSubresourceFilter (DSF). When ADSF is constructed, it creates an ADSF::Core and initializes it asynchronously on a SequencedTaskRunner using the supplied initialization |params| and a VerifiedRuleset. The task runner and VerifiedRuleset are taken from VerifiedRuleset::Handle provided to the constructor. ADSF::Core remains owned by ADSF, but lives on (and is accessed on) the task runner. During ADSF::Core initialization an ActivationState for the current frame is calculated, and then reported back via |activation_state_callback| on the task runner associated with the ADSF creator's thread (normally, the UI thread). If MemoryMappedRuleset is not present or malformed, then a default ActivationState is reported (with ActivationLevel equal to DISABLED). Although the full initialization of ADSF/ADSF::Core is asynchronous, the properties of SequencedTaskRunner allow the ADSF instance to be immediately used after construction. The key purpose of ADSF is providing decisions on whether to allow loading subframes for the current frame. The GetLoadPolicyForSubdocument async method serves this purpose, and boils down to calling the GetLoadPolicy method of DSF on the task runner and returning the result to the caller's task runner. BUG=637415 Review-Url: https://codereview.chromium.org/2683413003 Cr-Commit-Position: refs/heads/master@{#451648} Committed: https://chromium.googlesource.com/chromium/src/+/3e809d1f60ee19d1021842752704... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:260001) as https://chromium.googlesource.com/chromium/src/+/3e809d1f60ee19d1021842752704... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
