Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2217683002:
Allow embedder to use custom ResourceRequestAllowedNotifier (Closed)
DescriptionAllow embedder to use custom ResourceRequestAllowedNotifier
By allowing custom ResourceRequestAllowedNotifier, embedder
has the flexibility to set custom criteria when the fetch is
allowed or not allowed.
BUG=634458
Review-Url: https://codereview.chromium.org/2217683002
Cr-Commit-Position: refs/heads/master@{#454739}
Committed: https://chromium.googlesource.com/chromium/src/+/3d708d4ee896db39e60f4de411851d5d85dea9b1
Patch Set 1 #
Total comments: 13
Patch Set 2 : Allow embedder to use custom ResourceRequestAllowedNotifier #Patch Set 3 : Allow embedder to use custom ResourceRequestAllowedNotifier #
Total comments: 5
Patch Set 4 : Allow embedder to use custom ResourceRequestAllowedNotifier #Patch Set 5 : Allow embedder to use custom ResourceRequestAllowedNotifier #
Messages
Total messages: 32 (15 generated)
guptaag@amazon.com changed reviewers: + dbeam@chromium.org
Hi Dan, PTAL Thanks, Ashish
/cc rsesek@: thoughts on this? simpler ways to accomplish these unit tests? https://codereview.chromium.org/2217683002/diff/1/components/web_resource/web... File components/web_resource/web_resource_service.cc (right): https://codereview.chromium.org/2217683002/diff/1/components/web_resource/web... components/web_resource/web_resource_service.cc:127: return; nit: this is currently 3 space indent, but should be 2 https://codereview.chromium.org/2217683002/diff/1/components/web_resource/web... components/web_resource/web_resource_service.cc:128: } nit: no curlies https://codereview.chromium.org/2217683002/diff/1/components/web_resource/web... File components/web_resource/web_resource_service.h (right): https://codereview.chromium.org/2217683002/diff/1/components/web_resource/web... components/web_resource/web_resource_service.h:69: std::unique_ptr<ResourceRequestAllowedNotifier> notifier); 4\s indent instead of 2\s (add 2 more spaces to the beginning of this line) https://codereview.chromium.org/2217683002/diff/1/components/web_resource/web... File components/web_resource/web_resource_service_unittest.cc (right): https://codereview.chromium.org/2217683002/diff/1/components/web_resource/web... components/web_resource/web_resource_service_unittest.cc:38: }; nit: please omit these optional semi-colons (;) https://codereview.chromium.org/2217683002/diff/1/components/web_resource/web... components/web_resource/web_resource_service_unittest.cc:50: state_ = state; nit: call SetState()? https://codereview.chromium.org/2217683002/diff/1/components/web_resource/web... components/web_resource/web_resource_service_unittest.cc:100: "0"); can you run `git cl format` on this changelist? i'll automatically reshuffle your code in chromium-approved style so we don't have to talk about indent nits and things https://codereview.chromium.org/2217683002/diff/1/components/web_resource/web... components/web_resource/web_resource_service_unittest.cc:113: new TestResourceRequestAllowedNotifier(local_state_, NULL); indent off https://codereview.chromium.org/2217683002/diff/1/components/web_resource/web... components/web_resource/web_resource_service_unittest.cc:113: new TestResourceRequestAllowedNotifier(local_state_, NULL); please use nullptr (instead of NULL) in new code https://codereview.chromium.org/2217683002/diff/1/components/web_resource/web... components/web_resource/web_resource_service_unittest.cc:125: return test_web_resource_service_->fetch_scheduled_; can we just make a const accessor for this instead? maybe make it protected and then in this derived class up the visibility to public? class X { protected: Y get_y const () { return y_; } }; class TestingX { public: using get_y; } TestingX().get_y(); // now allowed by compiler https://codereview.chromium.org/2217683002/diff/1/components/web_resource/web... components/web_resource/web_resource_service_unittest.cc:134: const WebResourceService::ErrorCallback& error_callback) { indent off https://codereview.chromium.org/2217683002/diff/1/components/web_resource/web... components/web_resource/web_resource_service_unittest.cc:140: } no curlies https://codereview.chromium.org/2217683002/diff/1/components/web_resource/web... components/web_resource/web_resource_service_unittest.cc:154: TestingPrefServiceSimple* local_state_; can you initialize |local_state_| and |test_web_resource_service_| to nullptr? yes, even though you're setting them in SetUp(). https://codereview.chromium.org/2217683002/diff/1/components/web_resource/web... components/web_resource/web_resource_service_unittest.cc:178: } nit: \n here
rsesek@chromium.org changed reviewers: + rsesek@chromium.org
This should be reviewed by someone in //components/variations/OWNERS too.
On 2016/08/08 18:15:34, Dan Beam wrote: > /cc rsesek@: thoughts on this? simpler ways to accomplish these unit tests? > > https://codereview.chromium.org/2217683002/diff/1/components/web_resource/web... > File components/web_resource/web_resource_service.cc (right): > > https://codereview.chromium.org/2217683002/diff/1/components/web_resource/web... > components/web_resource/web_resource_service.cc:127: return; > nit: this is currently 3 space indent, but should be 2 > > https://codereview.chromium.org/2217683002/diff/1/components/web_resource/web... > components/web_resource/web_resource_service.cc:128: } > nit: no curlies > > https://codereview.chromium.org/2217683002/diff/1/components/web_resource/web... > File components/web_resource/web_resource_service.h (right): > > https://codereview.chromium.org/2217683002/diff/1/components/web_resource/web... > components/web_resource/web_resource_service.h:69: > std::unique_ptr<ResourceRequestAllowedNotifier> notifier); > 4\s indent instead of 2\s (add 2 more spaces to the beginning of this line) > > https://codereview.chromium.org/2217683002/diff/1/components/web_resource/web... > File components/web_resource/web_resource_service_unittest.cc (right): > > https://codereview.chromium.org/2217683002/diff/1/components/web_resource/web... > components/web_resource/web_resource_service_unittest.cc:38: }; > nit: please omit these optional semi-colons (;) > > https://codereview.chromium.org/2217683002/diff/1/components/web_resource/web... > components/web_resource/web_resource_service_unittest.cc:50: state_ = state; > nit: call SetState()? > > https://codereview.chromium.org/2217683002/diff/1/components/web_resource/web... > components/web_resource/web_resource_service_unittest.cc:100: "0"); > can you run `git cl format` on this changelist? i'll automatically reshuffle > your code in chromium-approved style so we don't have to talk about indent nits > and things > > https://codereview.chromium.org/2217683002/diff/1/components/web_resource/web... > components/web_resource/web_resource_service_unittest.cc:113: new > TestResourceRequestAllowedNotifier(local_state_, NULL); > indent off > > https://codereview.chromium.org/2217683002/diff/1/components/web_resource/web... > components/web_resource/web_resource_service_unittest.cc:113: new > TestResourceRequestAllowedNotifier(local_state_, NULL); > please use nullptr (instead of NULL) in new code > > https://codereview.chromium.org/2217683002/diff/1/components/web_resource/web... > components/web_resource/web_resource_service_unittest.cc:125: return > test_web_resource_service_->fetch_scheduled_; > can we just make a const accessor for this instead? maybe make it protected and > then in this derived class up the visibility to public? > > class X { > protected: > Y get_y const () { return y_; } > }; > > class TestingX { > public: > using get_y; > } > > TestingX().get_y(); // now allowed by compiler > > https://codereview.chromium.org/2217683002/diff/1/components/web_resource/web... > components/web_resource/web_resource_service_unittest.cc:134: const > WebResourceService::ErrorCallback& error_callback) { > indent off > > https://codereview.chromium.org/2217683002/diff/1/components/web_resource/web... > components/web_resource/web_resource_service_unittest.cc:140: } > no curlies > > https://codereview.chromium.org/2217683002/diff/1/components/web_resource/web... > components/web_resource/web_resource_service_unittest.cc:154: > TestingPrefServiceSimple* local_state_; > can you initialize |local_state_| and |test_web_resource_service_| to nullptr? > yes, even though you're setting them in SetUp(). > > https://codereview.chromium.org/2217683002/diff/1/components/web_resource/web... > components/web_resource/web_resource_service_unittest.cc:178: } > nit: \n here Thanks for reviewing. Executed `git cl format` to fix formatting issues. Removed curly brackets as suggested.
looks pretty good to me. are you in src/AUTHORS? if not, you need to add yourself and/or sign a CLA. please wait for rsesek@ to review (or say he doesn't need to) and get a components/variations reviewer https://codereview.chromium.org/2217683002/diff/40001/components/web_resource... File components/web_resource/web_resource_service.h (right): https://codereview.chromium.org/2217683002/diff/40001/components/web_resource... components/web_resource/web_resource_service.h:107: // before fetching starts so that next fetch is scheduled. This is nit: wrap to 80 character lines https://codereview.chromium.org/2217683002/diff/40001/components/web_resource... File components/web_resource/web_resource_service_unittest.cc (right): https://codereview.chromium.org/2217683002/diff/40001/components/web_resource... components/web_resource/web_resource_service_unittest.cc:39: }; nit: no need for these ; after methods
On 2016/08/19 02:14:33, Dan Beam wrote: > looks pretty good to me. are you in src/AUTHORS? if not, you need to add > yourself and/or sign a CLA. > > please wait for rsesek@ to review (or say he doesn't need to) and get a > components/variations reviewer I'll defer to a components/variations OWNER since I don't have a vested interested in this code anymore.
On 2016/08/19 02:14:33, Dan Beam wrote: > looks pretty good to me. are you in src/AUTHORS? if not, you need to add > yourself and/or sign a CLA. > > please wait for rsesek@ to review (or say he doesn't need to) and get a > components/variations reviewer > > https://codereview.chromium.org/2217683002/diff/40001/components/web_resource... > File components/web_resource/web_resource_service.h (right): > > https://codereview.chromium.org/2217683002/diff/40001/components/web_resource... > components/web_resource/web_resource_service.h:107: // before fetching starts so > that next fetch is scheduled. This is > nit: wrap to 80 character lines > > https://codereview.chromium.org/2217683002/diff/40001/components/web_resource... > File components/web_resource/web_resource_service_unittest.cc (right): > > https://codereview.chromium.org/2217683002/diff/40001/components/web_resource... > components/web_resource/web_resource_service_unittest.cc:39: }; > nit: no need for these ; after methods Hi Dan, Have addressed suggested nits. Please recommend components/variations reviewer so that this codereview can be completed. Thanks, Ashish
On 2016/08/24 17:21:28, Robert Sesek wrote: > On 2016/08/19 02:14:33, Dan Beam wrote: > > looks pretty good to me. are you in src/AUTHORS? if not, you need to add > > yourself and/or sign a CLA. > > > > please wait for rsesek@ to review (or say he doesn't need to) and get a > > components/variations reviewer > > I'll defer to a components/variations OWNER since I don't have a vested > interested in this code anymore. Hi Robert, Thanks for your time. Please recommend components/variations reviewer so that this codereview can be completed. -Ashish
On 2016/08/24 17:21:28, Robert Sesek wrote: > On 2016/08/19 02:14:33, Dan Beam wrote: > > looks pretty good to me. are you in src/AUTHORS? if not, you need to add > > yourself and/or sign a CLA. > > > > please wait for rsesek@ to review (or say he doesn't need to) and get a > > components/variations reviewer > > I'll defer to a components/variations OWNER since I don't have a vested > interested in this code anymore. Just to add yes I'm added to src/AUTHORS.
Description was changed from ========== Allow embedder to use custom ResourceRequestAllowedNotifier By allowing custom ResourceRequestAllowedNotifier, embedder has the flexibility to set custom criteria when the fetch is allowed or not allowed. BUG=634458 ========== to ========== Allow embedder to use custom ResourceRequestAllowedNotifier By allowing custom ResourceRequestAllowedNotifier, embedder has the flexibility to set custom criteria when the fetch is allowed or not allowed. BUG=634458 ==========
guptaag@amazon.com changed reviewers: + asvitkine@chromium.org - rsesek@chromium.org
guptaag@amazon.com changed reviewers: + jwd@chromium.org - asvitkine@chromium.org
Dan Beam: Please review the latest patch. jwd@: Please review latest patch as suggested by Robert Sesek. Thanks, Ashish https://codereview.chromium.org/2217683002/diff/40001/components/web_resource... File components/web_resource/web_resource_service.h (right): https://codereview.chromium.org/2217683002/diff/40001/components/web_resource... components/web_resource/web_resource_service.h:107: // before fetching starts so that next fetch is scheduled. This is On 2016/08/19 02:14:32, Dan Beam wrote: > nit: wrap to 80 character lines >> Wrapped within 80 character. https://codereview.chromium.org/2217683002/diff/40001/components/web_resource... File components/web_resource/web_resource_service_unittest.cc (right): https://codereview.chromium.org/2217683002/diff/40001/components/web_resource... components/web_resource/web_resource_service_unittest.cc:39: }; On 2016/08/19 02:14:32, Dan Beam wrote: > nit: no need for these ; after methods >> Removed ; from methods https://codereview.chromium.org/2217683002/diff/40001/components/web_resource... components/web_resource/web_resource_service_unittest.cc:39: }; On 2016/08/19 02:14:32, Dan Beam wrote: > nit: no need for these ; after methods Done.
This seems fine from the variations point of view, as it doesn't affect how we depend on this component. So LGTM for that aspect. Our ownership doesn't cover the changed files though.
lgtm
i will mention here, though, that the only chromium consumer of this code is plugins so we may move this code out of its own component soon. it was previously used to download promotional stuff, but that is no longer the case.
The CQ bit was checked by guptaag@amazon.com
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
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 guptaag@amazon.com 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 guptaag@amazon.com
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org, jwd@chromium.org Link to the patchset: https://codereview.chromium.org/2217683002/#ps80001 (title: "Allow embedder to use custom ResourceRequestAllowedNotifier")
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": 80001, "attempt_start_ts": 1488590203817760,
"parent_rev": "4eb5d8b165e857c75ad3b10bcc4fb4a07f248577", "commit_rev":
"3d708d4ee896db39e60f4de411851d5d85dea9b1"}
Message was sent while issue was closed.
Description was changed from ========== Allow embedder to use custom ResourceRequestAllowedNotifier By allowing custom ResourceRequestAllowedNotifier, embedder has the flexibility to set custom criteria when the fetch is allowed or not allowed. BUG=634458 ========== to ========== Allow embedder to use custom ResourceRequestAllowedNotifier By allowing custom ResourceRequestAllowedNotifier, embedder has the flexibility to set custom criteria when the fetch is allowed or not allowed. BUG=634458 Review-Url: https://codereview.chromium.org/2217683002 Cr-Commit-Position: refs/heads/master@{#454739} Committed: https://chromium.googlesource.com/chromium/src/+/3d708d4ee896db39e60f4de41185... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/3d708d4ee896db39e60f4de41185... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
