Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2670543002:
Introduce VerifiedRuleset and its async Handle. (Closed)
|
Created:
3 years, 10 months ago by pkalinnikov Modified:
3 years, 10 months ago CC:
chromium-reviews, jam, darin-cc_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntroduce VerifiedRuleset and its async Handle.
VerifiedRuleset (VR) holds a reference to MemoryMappedRuleset, initially empty,
and provides access to it. It lives on the same SequencedTaskRunner as a
VerifiedRulesetDealer (VRD) owned by a VRD::Handle, which is used to create a
VR::Handle. VR is owned, asynchronously accessed and destroyed by a VR::Handle,
its UI-thread counterpart.
The VR will usually live on a blocking pool thread because this is where a VRD
would normally live.
BUG=637415
Review-Url: https://codereview.chromium.org/2670543002
Cr-Commit-Position: refs/heads/master@{#447782}
Committed: https://chromium.googlesource.com/chromium/src/+/f2774bfe725f433421242e887269aa093bfe405a
Patch Set 1 #
Total comments: 15
Patch Set 2 : Address comments; fix memory leak in tests. #
Messages
Total messages: 24 (16 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
Initial PTAL.
pkalinnikov@chromium.org changed reviewers: + melandory@chromium.org
+Tanja to reviewers.
LGTM % minor comments. Thanks! https://codereview.chromium.org/2670543002/diff/1/components/subresource_filt... File components/subresource_filter/content/browser/verified_ruleset_dealer.h (right): https://codereview.chromium.org/2670543002/diff/1/components/subresource_filt... components/subresource_filter/content/browser/verified_ruleset_dealer.h:71: base::SequencedTaskRunner* task_runner() { return task_runner_; } Let's add a comment here: // Returns the |task_runner| on which the VerifiedRulesetDealer, as well as the MemoryMappedRulesets it creates, should be accessed. https://codereview.chromium.org/2670543002/diff/1/components/subresource_filt... components/subresource_filter/content/browser/verified_ruleset_dealer.h:91: // Holds a strong reference to MemoryMappedRuleset, and provides acceess to it. nit: Could you please explain very briefly why this class is needed, i.e. that this is an initially empty holder for a MemoryMappedRuleset, it can be created on the UI thread synchronously, hence allowing the Handle to be created synchronously and be immediately used by clients on the UI thread, while the MemoryMappedRuleset is retrieved on the |task_runner| in a deferred manner. https://codereview.chromium.org/2670543002/diff/1/components/subresource_filt... components/subresource_filter/content/browser/verified_ruleset_dealer.h:99: // Returns nullptr iff the ruleset is not initialized or is CORRUPT. Comment nit: // Can return nullptr even after initialization in case no ruleset is available, or if the ruleset is corrupted. https://codereview.chromium.org/2670543002/diff/1/components/subresource_filt... components/subresource_filter/content/browser/verified_ruleset_dealer.h:100: const MemoryMappedRuleset* Get() const; There seems to be quite some precedence for using ThreadChecker in inlined getters defined in the header file, let's do that, given that this is really just a getter. https://codereview.chromium.org/2670543002/diff/1/components/subresource_filt... components/subresource_filter/content/browser/verified_ruleset_dealer.h:112: // The UI-thread handle that owns a VerifiedRuleset living on a dedicated nit: Please update comments to point out that this will use the same |task_runner| which is used by the VerifiedRulesetDealer. https://codereview.chromium.org/2670543002/diff/1/components/subresource_filt... components/subresource_filter/content/browser/verified_ruleset_dealer.h:123: // Posts a task to |task_runner| to access the owned VerifiedRuleset via an Please extend comment to mimic the final version of its counterpart for VRD. https://codereview.chromium.org/2670543002/diff/1/components/subresource_filt... File components/subresource_filter/content/browser/verified_ruleset_dealer_unittest.cc (right): https://codereview.chromium.org/2670543002/diff/1/components/subresource_filt... components/subresource_filter/content/browser/verified_ruleset_dealer_unittest.cc:402: std::unique_ptr<VerifiedRuleset::Handle> NewRulesetHandle() { nit: CreateNewRulesetHandle() to make this start with a verb.
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_...)
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...
Thanks. Will update the CL description shortly. https://codereview.chromium.org/2670543002/diff/1/components/subresource_filt... File components/subresource_filter/content/browser/verified_ruleset_dealer.h (right): https://codereview.chromium.org/2670543002/diff/1/components/subresource_filt... components/subresource_filter/content/browser/verified_ruleset_dealer.h:71: base::SequencedTaskRunner* task_runner() { return task_runner_; } On 2017/02/01 13:30:32, engedy wrote: > Let's add a comment here: > > // Returns the |task_runner| on which the VerifiedRulesetDealer, as well as the > MemoryMappedRulesets it creates, should be accessed. Done. https://codereview.chromium.org/2670543002/diff/1/components/subresource_filt... components/subresource_filter/content/browser/verified_ruleset_dealer.h:91: // Holds a strong reference to MemoryMappedRuleset, and provides acceess to it. On 2017/02/01 13:30:32, engedy wrote: > nit: Could you please explain very briefly why this class is needed, i.e. that > this is an initially empty holder for a MemoryMappedRuleset, it can be created > on the UI thread synchronously, hence allowing the Handle to be created > synchronously and be immediately used by clients on the UI thread, while the > MemoryMappedRuleset is retrieved on the |task_runner| in a deferred manner. Done. https://codereview.chromium.org/2670543002/diff/1/components/subresource_filt... components/subresource_filter/content/browser/verified_ruleset_dealer.h:99: // Returns nullptr iff the ruleset is not initialized or is CORRUPT. On 2017/02/01 13:30:32, engedy wrote: > Comment nit: > > // Can return nullptr even after initialization in case no ruleset is available, > or if the ruleset is corrupted. Done. https://codereview.chromium.org/2670543002/diff/1/components/subresource_filt... components/subresource_filter/content/browser/verified_ruleset_dealer.h:100: const MemoryMappedRuleset* Get() const; On 2017/02/01 13:30:32, engedy wrote: > There seems to be quite some precedence for using ThreadChecker in inlined > getters defined in the header file, let's do that, given that this is really > just a getter. Done. https://codereview.chromium.org/2670543002/diff/1/components/subresource_filt... components/subresource_filter/content/browser/verified_ruleset_dealer.h:112: // The UI-thread handle that owns a VerifiedRuleset living on a dedicated On 2017/02/01 13:30:32, engedy wrote: > nit: Please update comments to point out that this will use the same > |task_runner| which is used by the VerifiedRulesetDealer. Done. https://codereview.chromium.org/2670543002/diff/1/components/subresource_filt... components/subresource_filter/content/browser/verified_ruleset_dealer.h:123: // Posts a task to |task_runner| to access the owned VerifiedRuleset via an On 2017/02/01 13:30:32, engedy wrote: > Please extend comment to mimic the final version of its counterpart for VRD. Done. Side effect of working on changes in parallel :) https://codereview.chromium.org/2670543002/diff/1/components/subresource_filt... File components/subresource_filter/content/browser/verified_ruleset_dealer_unittest.cc (right): https://codereview.chromium.org/2670543002/diff/1/components/subresource_filt... components/subresource_filter/content/browser/verified_ruleset_dealer_unittest.cc:402: std::unique_ptr<VerifiedRuleset::Handle> NewRulesetHandle() { On 2017/02/01 13:30:32, engedy wrote: > nit: CreateNewRulesetHandle() to make this start with a verb. How about just CreateRulesetHandle()?
https://codereview.chromium.org/2670543002/diff/1/components/subresource_filt... File components/subresource_filter/content/browser/verified_ruleset_dealer_unittest.cc (right): https://codereview.chromium.org/2670543002/diff/1/components/subresource_filt... components/subresource_filter/content/browser/verified_ruleset_dealer_unittest.cc:402: std::unique_ptr<VerifiedRuleset::Handle> NewRulesetHandle() { On 2017/02/01 17:32:13, pkalinnikov wrote: > On 2017/02/01 13:30:32, engedy wrote: > > nit: CreateNewRulesetHandle() to make this start with a verb. > > How about just CreateRulesetHandle()? Fine too, just wanted a verb in it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Introduce VerifiedRuleset and its async Handle. BUG=637415 ========== to ========== Introduce VerifiedRuleset and its async Handle. VerifiedRuleset (VR) holds a reference to MemoryMappedRuleset, initially empty, and provides access to it. It lives on the same SequencedTaskRunner as a VerifiedRulesetDealer (VRD) owned by a VRD::Handle, which is used to create a VR::Handle. VR is owned, asynchronously accessed and destroyed by a VR::Handle, its UI-thread counterpart. The VR will usually live on a blocking pool thread because this is where a VRD would normally live. BUG=637415 ==========
Description was changed from ========== Introduce VerifiedRuleset and its async Handle. VerifiedRuleset (VR) holds a reference to MemoryMappedRuleset, initially empty, and provides access to it. It lives on the same SequencedTaskRunner as a VerifiedRulesetDealer (VRD) owned by a VRD::Handle, which is used to create a VR::Handle. VR is owned, asynchronously accessed and destroyed by a VR::Handle, its UI-thread counterpart. The VR will usually live on a blocking pool thread because this is where a VRD would normally live. BUG=637415 ========== to ========== Introduce VerifiedRuleset and its async Handle. VerifiedRuleset (VR) holds a reference to MemoryMappedRuleset, initially empty, and provides access to it. It lives on the same SequencedTaskRunner as a VerifiedRulesetDealer (VRD) owned by a VRD::Handle, which is used to create a VR::Handle. VR is owned, asynchronously accessed and destroyed by a VR::Handle, its UI-thread counterpart. The VR will usually live on a blocking pool thread because this is where a VRD would normally live. BUG=637415 ==========
On 2017/02/01 17:36:05, engedy wrote: > https://codereview.chromium.org/2670543002/diff/1/components/subresource_filt... > File > components/subresource_filter/content/browser/verified_ruleset_dealer_unittest.cc > (right): > > https://codereview.chromium.org/2670543002/diff/1/components/subresource_filt... > components/subresource_filter/content/browser/verified_ruleset_dealer_unittest.cc:402: > std::unique_ptr<VerifiedRuleset::Handle> NewRulesetHandle() { > On 2017/02/01 17:32:13, pkalinnikov wrote: > > On 2017/02/01 13:30:32, engedy wrote: > > > nit: CreateNewRulesetHandle() to make this start with a verb. > > > > How about just CreateRulesetHandle()? > > Fine too, just wanted a verb in it. lgtm
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 Link to the patchset: https://codereview.chromium.org/2670543002/#ps20001 (title: "Address comments; fix memory leak in tests.")
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": 20001, "attempt_start_ts": 1486054646382470,
"parent_rev": "1876ab4afa78c0dfae46359e449db1f07f1f8f1c", "commit_rev":
"f2774bfe725f433421242e887269aa093bfe405a"}
Message was sent while issue was closed.
Description was changed from ========== Introduce VerifiedRuleset and its async Handle. VerifiedRuleset (VR) holds a reference to MemoryMappedRuleset, initially empty, and provides access to it. It lives on the same SequencedTaskRunner as a VerifiedRulesetDealer (VRD) owned by a VRD::Handle, which is used to create a VR::Handle. VR is owned, asynchronously accessed and destroyed by a VR::Handle, its UI-thread counterpart. The VR will usually live on a blocking pool thread because this is where a VRD would normally live. BUG=637415 ========== to ========== Introduce VerifiedRuleset and its async Handle. VerifiedRuleset (VR) holds a reference to MemoryMappedRuleset, initially empty, and provides access to it. It lives on the same SequencedTaskRunner as a VerifiedRulesetDealer (VRD) owned by a VRD::Handle, which is used to create a VR::Handle. VR is owned, asynchronously accessed and destroyed by a VR::Handle, its UI-thread counterpart. The VR will usually live on a blocking pool thread because this is where a VRD would normally live. BUG=637415 Review-Url: https://codereview.chromium.org/2670543002 Cr-Commit-Position: refs/heads/master@{#447782} Committed: https://chromium.googlesource.com/chromium/src/+/f2774bfe725f433421242e887269... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/f2774bfe725f433421242e887269... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
