Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2004453002:
Add a Dns preresolve interface in //content (Closed)
|
Created:
4 years, 7 months ago by Charlie Harrison Modified:
4 years, 6 months ago CC:
cbentzel+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, jam, Julia Tuttle, loading-reviews_chromium.org, Randy Smith (Not in Mondays) Base URL:
https://chromium.googlesource.com/chromium/src.git@predictor_dns_browsertest Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a Dns preresolve interface in //content
This patch adds a preresolve interface in //content, with the sole
consumer being the predictor in //chrome/browser/net. This is in
preparation for sending resource hints from Blink to //content.
The requests are routed through the ResourceHintsController, which
is scoped to the RDHI. The controller keeps track of each request until
completion, and calls a consumer-supplied callback.
Note: this change also makes all dns preresolves by the predictor use
a lower priority (net::IDLE) than the priority they had previously been
using (net::DEFAULT_PRIORITY).
TBR=kinuko@chromium.org
BUG=610750, 614350
Committed: https://crrev.com/f848daf2b342548c8ec90b21396097339688819a
Cr-Commit-Position: refs/heads/master@{#397250}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Fix comment and add necessary include #
Total comments: 10
Patch Set 3 : kinuko@ review #Patch Set 4 : Changed interface to accept ResourceContexts #
Total comments: 8
Patch Set 5 : safe callbacks #
Total comments: 2
Patch Set 6 : remove controller class #
Total comments: 8
Patch Set 7 : achuithb@ review: cleaned up predictor of NULLs and simplified auth_prewarmer getting signin Resour… #
Total comments: 2
Patch Set 8 : null checks => DCHECKs #Patch Set 9 : rebase on 396447 #Patch Set 10 : Fix error with base::Passed #
Total comments: 6
Patch Set 11 : eroman@ review. use base::Owned instead of changine single_request_host_resolver #
Total comments: 2
Patch Set 12 : Move AddressList to unique_ptr #
Total comments: 2
Patch Set 13 : this time just use base::Owned for everything #Patch Set 14 : rebase on #397108 #
Total comments: 1
Patch Set 15 : Comment change #
Messages
Total messages: 65 (22 generated)
Description was changed from ========== Add a Dns preresolve interface in //content This patch adds a preresolve interface in //content, with the sole consumer being the predictor in //chrome/browser/net. This is in preparation for sending resource hints from Blink to //content. BUG=610750 ========== to ========== Add a Dns preresolve interface in //content This patch adds a preresolve interface in //content, with the sole consumer being the predictor in //chrome/browser/net. This is in preparation for sending resource hints from Blink to //content. Note: this change also makes all dns preresolves by the predictor use a lower priority (net::IDLE) than the priority they had previously been using (net::DEFAULT_PRIORITY). BUG=610750 ==========
Description was changed from ========== Add a Dns preresolve interface in //content This patch adds a preresolve interface in //content, with the sole consumer being the predictor in //chrome/browser/net. This is in preparation for sending resource hints from Blink to //content. Note: this change also makes all dns preresolves by the predictor use a lower priority (net::IDLE) than the priority they had previously been using (net::DEFAULT_PRIORITY). BUG=610750 ========== to ========== Add a Dns preresolve interface in //content This patch adds a preresolve interface in //content, with the sole consumer being the predictor in //chrome/browser/net. This is in preparation for sending resource hints from Blink to //content. The requests are routed through the ResourceHintsController, which is scoped to the RDHI. The controller keeps track of each request until completion, and calls a consumer-supplied callback. Note: this change also makes all dns preresolves by the predictor use a lower priority (net::IDLE) than the priority they had previously been using (net::DEFAULT_PRIORITY). BUG=610750 ==========
csharrison@chromium.org changed reviewers: + kinuko@chromium.org, rdsmith@chromium.org
PTAL. Note that the interface into //content is very simplified (no sending priorities, etc.). I think this is okay. In a follow up patch, I will move all the preconnect / preresolves in blink to use this interface via mojo. https://codereview.chromium.org/2004453002/diff/1/content/browser/loader/reso... File content/browser/loader/resource_hints_impl.h (right): https://codereview.chromium.org/2004453002/diff/1/content/browser/loader/reso... content/browser/loader/resource_hints_impl.h:18: // Scoped to the ResourceDispatcherHostImpl. I've added a more detailed comment here in the next patch.
https://codereview.chromium.org/2004453002/diff/20001/content/browser/loader/... File content/browser/loader/resource_hints_impl.cc (right): https://codereview.chromium.org/2004453002/diff/20001/content/browser/loader/... content/browser/loader/resource_hints_impl.cc:68: if (!dispatcher_host) When could this happen?? https://codereview.chromium.org/2004453002/diff/20001/content/browser/loader/... File content/browser/loader/resource_hints_impl.h (right): https://codereview.chromium.org/2004453002/diff/20001/content/browser/loader/... content/browser/loader/resource_hints_impl.h:21: class ResourceHintsController { I feel this class could be in its own header and cc file (e.g. resource_hints_controller.{h,cc} https://codereview.chromium.org/2004453002/diff/20001/content/browser/loader/... content/browser/loader/resource_hints_impl.h:30: struct DnsRequest { nit: the definition of this struct could be in .cc https://codereview.chromium.org/2004453002/diff/20001/content/browser/loader/... content/browser/loader/resource_hints_impl.h:31: DnsRequest(net::HostResolver* resolver); nit: explicit https://codereview.chromium.org/2004453002/diff/20001/content/browser/loader/... content/browser/loader/resource_hints_impl.h:40: std::set<DnsRequest*> pending_dns_requests_; std::set<std::unique_ptr<DnsRequest>> ?
Please take another look. Responded to comments and changed the interface to use ResourceContexts. https://codereview.chromium.org/2004453002/diff/20001/content/browser/loader/... File content/browser/loader/resource_hints_impl.cc (right): https://codereview.chromium.org/2004453002/diff/20001/content/browser/loader/... content/browser/loader/resource_hints_impl.cc:68: if (!dispatcher_host) On 2016/05/23 14:07:26, kinuko wrote: > When could this happen?? Well, this is a raw function, so it could conceivably be called after RDH tears down. I've turned it into a DCHECK. https://codereview.chromium.org/2004453002/diff/20001/content/browser/loader/... File content/browser/loader/resource_hints_impl.h (right): https://codereview.chromium.org/2004453002/diff/20001/content/browser/loader/... content/browser/loader/resource_hints_impl.h:21: class ResourceHintsController { On 2016/05/23 14:07:26, kinuko wrote: > I feel this class could be in its own header and cc file (e.g. > resource_hints_controller.{h,cc} Done. https://codereview.chromium.org/2004453002/diff/20001/content/browser/loader/... content/browser/loader/resource_hints_impl.h:30: struct DnsRequest { On 2016/05/23 14:07:26, kinuko wrote: > nit: the definition of this struct could be in .cc Done. https://codereview.chromium.org/2004453002/diff/20001/content/browser/loader/... content/browser/loader/resource_hints_impl.h:31: DnsRequest(net::HostResolver* resolver); On 2016/05/23 14:07:26, kinuko wrote: > nit: explicit Done. https://codereview.chromium.org/2004453002/diff/20001/content/browser/loader/... content/browser/loader/resource_hints_impl.h:40: std::set<DnsRequest*> pending_dns_requests_; On 2016/05/23 14:07:26, kinuko wrote: > std::set<std::unique_ptr<DnsRequest>> ? Is there an easy way to do this with unique pointers? I think the closest approximation I could do is std::map<DnsRequest*, std::unique_ptr<DnsRequest>>. The problem is that we need to lookup a DnsRequest* on OnResolveComplete. Actually, let me try passing ownership to the callback. Then I don't even think we need a container.
cc juliatuttle@ FHI (not a review request; I think this is mostly a routing issue, not a DNS issue).
Generally looking good, I have a few questions about object lifetime... https://codereview.chromium.org/2004453002/diff/60001/chrome/browser/net/pred... File chrome/browser/net/predictor.cc (right): https://codereview.chromium.org/2004453002/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor.cc:1084: base::Bind(&Predictor::OnLookupFinished, base::Unretained(this), url)); Who guarantees the callback is called while |this| is alive? https://codereview.chromium.org/2004453002/diff/60001/content/browser/loader/... File content/browser/loader/resource_hints_controller.cc (right): https://codereview.chromium.org/2004453002/diff/60001/content/browser/loader/... content/browser/loader/resource_hints_controller.cc:41: base::Unretained(this), base::Passed(std::move(request)), ditto, how is it guaranteed that calling OnResolveComplete on |this| is always safe?
Thanks for the detailed review, I think both those callbacks were a bit problematic and I didn't think through the one in the predictor because it just duplicated behavior. I'll try to amend the patch with some hopefully clean solutions. https://codereview.chromium.org/2004453002/diff/60001/chrome/browser/net/pred... File chrome/browser/net/predictor.cc (right): https://codereview.chromium.org/2004453002/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor.cc:1084: base::Bind(&Predictor::OnLookupFinished, base::Unretained(this), url)); On 2016/05/24 09:15:27, kinuko wrote: > Who guarantees the callback is called while |this| is alive? Good question. The semantics shouldn't be changed from the previous version of the code, and in fact we do see a small amount of crashes for OnLookupFinished today. I filed crbug.com/614350 that hopefully this patch will fix. I'm not sure what the best way to fix it is. Possibly we could add an intermediate object scoped to the callback that is notified of predictor destruction. I'd hate to add that after we've removed the LookupRequest here though :(. https://codereview.chromium.org/2004453002/diff/60001/content/browser/loader/... File content/browser/loader/resource_hints_controller.cc (right): https://codereview.chromium.org/2004453002/diff/60001/content/browser/loader/... content/browser/loader/resource_hints_controller.cc:37: net::HostResolver::RequestInfo resolve_info(net::HostPortPair::FromURL(url)); Make sure we set this to be speculative. https://codereview.chromium.org/2004453002/diff/60001/content/browser/loader/... content/browser/loader/resource_hints_controller.cc:41: base::Unretained(this), base::Passed(std::move(request)), On 2016/05/24 09:15:27, kinuko wrote: > ditto, how is it guaranteed that calling OnResolveComplete on |this| is always > safe? I think it might not be. The host resolver is destroyed when the IO thread is destroyed, but the RDHI is Shutdown() right before that. I can add some safety by picking this off the RDHI global.
https://codereview.chromium.org/2004453002/diff/60001/chrome/browser/net/pred... File chrome/browser/net/predictor.cc (right): https://codereview.chromium.org/2004453002/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor.cc:1084: base::Bind(&Predictor::OnLookupFinished, base::Unretained(this), url)); On 2016/05/24 13:14:22, csharrison wrote: > On 2016/05/24 09:15:27, kinuko wrote: > > Who guarantees the callback is called while |this| is alive? > > Good question. The semantics shouldn't be changed from the previous version of > the code, and in fact we do see a small amount of crashes for OnLookupFinished > today. I filed crbug.com/614350 that hopefully this patch will fix. > > I'm not sure what the best way to fix it is. Possibly we could add an > intermediate object scoped to the callback that is notified of predictor > destruction. I'd hate to add that after we've removed the LookupRequest here > though :(. If we just want to disallow running the callback when |this| gets destructed we could possibly just use weak ptr here (and in the other place too)..? (Note that I'm naively writing this without looking into the detailed object relationship around this code)
Description was changed from ========== Add a Dns preresolve interface in //content This patch adds a preresolve interface in //content, with the sole consumer being the predictor in //chrome/browser/net. This is in preparation for sending resource hints from Blink to //content. The requests are routed through the ResourceHintsController, which is scoped to the RDHI. The controller keeps track of each request until completion, and calls a consumer-supplied callback. Note: this change also makes all dns preresolves by the predictor use a lower priority (net::IDLE) than the priority they had previously been using (net::DEFAULT_PRIORITY). BUG=610750 ========== to ========== Add a Dns preresolve interface in //content This patch adds a preresolve interface in //content, with the sole consumer being the predictor in //chrome/browser/net. This is in preparation for sending resource hints from Blink to //content. The requests are routed through the ResourceHintsController, which is scoped to the RDHI. The controller keeps track of each request until completion, and calls a consumer-supplied callback. Note: this change also makes all dns preresolves by the predictor use a lower priority (net::IDLE) than the priority they had previously been using (net::DEFAULT_PRIORITY). BUG=610750,614350 ==========
Thanks. I've updated the description to track 614350, as hopefully we'll fix that crasher with this patch. https://codereview.chromium.org/2004453002/diff/60001/chrome/browser/net/pred... File chrome/browser/net/predictor.cc (right): https://codereview.chromium.org/2004453002/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor.cc:1084: base::Bind(&Predictor::OnLookupFinished, base::Unretained(this), url)); On 2016/05/24 14:38:34, kinuko wrote: > On 2016/05/24 13:14:22, csharrison wrote: > > On 2016/05/24 09:15:27, kinuko wrote: > > > Who guarantees the callback is called while |this| is alive? > > > > Good question. The semantics shouldn't be changed from the previous version of > > the code, and in fact we do see a small amount of crashes for OnLookupFinished > > today. I filed crbug.com/614350 that hopefully this patch will fix. > > > > I'm not sure what the best way to fix it is. Possibly we could add an > > intermediate object scoped to the callback that is notified of predictor > > destruction. I'd hate to add that after we've removed the LookupRequest here > > though :(. > > If we just want to disallow running the callback when |this| gets destructed we > could possibly just use weak ptr here (and in the other place too)..? (Note > that I'm naively writing this without looking into the detailed object > relationship around this code) That works :) I scanned through the ownership and it all looks good. The only thing we mutate in OnLookupFinished is the results_ map which should be destructed fine when the Predictor goes away. Thanks for the suggestion. https://codereview.chromium.org/2004453002/diff/60001/content/browser/loader/... File content/browser/loader/resource_hints_controller.cc (right): https://codereview.chromium.org/2004453002/diff/60001/content/browser/loader/... content/browser/loader/resource_hints_controller.cc:41: base::Unretained(this), base::Passed(std::move(request)), On 2016/05/24 13:14:22, csharrison wrote: > On 2016/05/24 09:15:27, kinuko wrote: > > ditto, how is it guaranteed that calling OnResolveComplete on |this| is always > > safe? > > I think it might not be. The host resolver is destroyed when the IO thread is > destroyed, but the RDHI is Shutdown() right before that. I can add some safety > by picking this off the RDHI global. Actually, we don't actually need the controller here, just the callback. I've changed this to be a static method.
csharrison@chromium.org changed reviewers: + achuith@chromium.org
+achuith@chromium.org: Can you look at the changes to auth_prewarmer? I had to modify the interface into //content slightly.
lgtm
On 2016/05/24 20:36:20, achuithb wrote: > lgtm To be clear, only looked at auth_prewarmer, please get an lgtm for the CL as a whole.
https://codereview.chromium.org/2004453002/diff/80001/content/browser/loader/... File content/browser/loader/resource_hints_controller.h (right): https://codereview.chromium.org/2004453002/diff/80001/content/browser/loader/... content/browser/loader/resource_hints_controller.h:31: int result); It looks both DnsRequest and OnResolveComplete can simply be a local struct / method in an anon space in .cc? ...and at this point I feel we might not even need this Controller class at all?
https://codereview.chromium.org/2004453002/diff/80001/content/browser/loader/... File content/browser/loader/resource_hints_controller.h (right): https://codereview.chromium.org/2004453002/diff/80001/content/browser/loader/... content/browser/loader/resource_hints_controller.h:31: int result); On 2016/05/25 02:07:04, kinuko wrote: > It looks both DnsRequest and OnResolveComplete can simply be a local struct / > method in an anon space in .cc? > > ...and at this point I feel we might not even need this Controller class at all? Yupp, I removed the class in the latest CL revision.
achuithb@, just verifying that you also looked at the other files in chromeos/?
https://codereview.chromium.org/2004453002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/login/auth/auth_prewarmer.cc (right): https://codereview.chromium.org/2004453002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/login/auth/auth_prewarmer.cc:99: base::Bind(&content::PreconnectUrl, login::GetSigninResourceContext(), Why not ProfileHelper::GetSigninProfile()->GetResourceContext() here? https://codereview.chromium.org/2004453002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/login/helper.h (right): https://codereview.chromium.org/2004453002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/login/helper.h:114: content::ResourceContext* GetSigninResourceContext(); Do we need this? https://codereview.chromium.org/2004453002/diff/100001/chrome/browser/net/pre... File chrome/browser/net/predictor.cc (right): https://codereview.chromium.org/2004453002/diff/100001/chrome/browser/net/pre... chrome/browser/net/predictor.cc:110: user_prefs_(NULL), Could you please replace NULLs with nullptr in this file? Also move initialization to in-class in the header instead of in the ctor?
Thanks for the second pass. https://codereview.chromium.org/2004453002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/login/auth/auth_prewarmer.cc (right): https://codereview.chromium.org/2004453002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/login/auth/auth_prewarmer.cc:99: base::Bind(&content::PreconnectUrl, login::GetSigninResourceContext(), On 2016/05/25 21:17:10, achuithb wrote: > Why not > ProfileHelper::GetSigninProfile()->GetResourceContext() here? That's much better. Done. https://codereview.chromium.org/2004453002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/login/helper.h (right): https://codereview.chromium.org/2004453002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/login/helper.h:114: content::ResourceContext* GetSigninResourceContext(); On 2016/05/25 21:17:10, achuithb wrote: > Do we need this? Nope, I've changed the auth_prewarmer code to use ProfileHelper. https://codereview.chromium.org/2004453002/diff/100001/chrome/browser/net/pre... File chrome/browser/net/predictor.cc (right): https://codereview.chromium.org/2004453002/diff/100001/chrome/browser/net/pre... chrome/browser/net/predictor.cc:110: user_prefs_(NULL), On 2016/05/25 21:17:10, achuithb wrote: > Could you please replace NULLs with nullptr in this file? > > Also move initialization to in-class in the header instead of in the ctor? Replaced all NULLs with nullptr. I don't think header initialization is explicitly part of the style guide, mind if I punt on that one?
https://codereview.chromium.org/2004453002/diff/100001/chrome/browser/net/pre... File chrome/browser/net/predictor.cc (right): https://codereview.chromium.org/2004453002/diff/100001/chrome/browser/net/pre... chrome/browser/net/predictor.cc:110: user_prefs_(NULL), On 2016/05/25 21:36:19, csharrison wrote: > On 2016/05/25 21:17:10, achuithb wrote: > > Could you please replace NULLs with nullptr in this file? > > > > Also move initialization to in-class in the header instead of in the ctor? > > Replaced all NULLs with nullptr. I don't think header initialization is > explicitly part of the style guide, mind if I punt on that one? I'm not an owner here, so it's up to you if you want to do it. Here's the discussion: https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/zqB-DySA4V0 https://chromium-cpp.appspot.com/ (Look for Non-static class member initializers)
https://codereview.chromium.org/2004453002/diff/100001/chrome/browser/net/pre... File chrome/browser/net/predictor.cc (right): https://codereview.chromium.org/2004453002/diff/100001/chrome/browser/net/pre... chrome/browser/net/predictor.cc:110: user_prefs_(NULL), On 2016/05/25 21:54:32, achuithb wrote: > On 2016/05/25 21:36:19, csharrison wrote: > > On 2016/05/25 21:17:10, achuithb wrote: > > > Could you please replace NULLs with nullptr in this file? > > > > > > Also move initialization to in-class in the header instead of in the ctor? > > > > Replaced all NULLs with nullptr. I don't think header initialization is > > explicitly part of the style guide, mind if I punt on that one? > > I'm not an owner here, so it's up to you if you want to do it. > > Here's the discussion: > https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/zqB-DySA4V0 > > https://chromium-cpp.appspot.com/ > (Look for Non-static class member initializers) Thanks for the link. I'd prefer not to in this case due to the members here that need complex initialization.
lgtm https://codereview.chromium.org/2004453002/diff/120001/content/browser/loader... File content/browser/loader/resource_hints_impl.cc (right): https://codereview.chromium.org/2004453002/diff/120001/content/browser/loader... content/browser/loader/resource_hints_impl.cc:87: return net::ERR_FAILED; This could happen.. in some tests? DCHECK's not sufficient?
Thanks! https://codereview.chromium.org/2004453002/diff/120001/content/browser/loader... File content/browser/loader/resource_hints_impl.cc (right): https://codereview.chromium.org/2004453002/diff/120001/content/browser/loader... content/browser/loader/resource_hints_impl.cc:87: return net::ERR_FAILED; On 2016/05/25 23:25:18, kinuko wrote: > This could happen.. in some tests? DCHECK's not sufficient? I was thinking of just relaxing the constraints of the public interface. I don't think any consumers provide null contexts, so I'll just change these back to DCHECKs.
Thanks! https://codereview.chromium.org/2004453002/diff/120001/content/browser/loader... File content/browser/loader/resource_hints_impl.cc (right): https://codereview.chromium.org/2004453002/diff/120001/content/browser/loader... content/browser/loader/resource_hints_impl.cc:87: return net::ERR_FAILED; On 2016/05/25 23:25:18, kinuko wrote: > This could happen.. in some tests? DCHECK's not sufficient? I was thinking of just relaxing the constraints of the public interface. I don't think any consumers provide null contexts, so I'll just change these back to DCHECKs.
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2004453002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2004453002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2004453002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2004453002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
csharrison@chromium.org changed reviewers: + eroman@chromium.org - rdsmith@chromium.org
+eroman, for overall review and OWNERS for chrome/browser/net. moving rdsmith to cc. Looks like Win bots are consistently reporting errors in various test suites. Unfortunately the stack traces aren't very helpful. I'll continue digging.
It seems like the win errors were from using the DnsRequest after it was base::Passed into the callback. I've fixed this in a clean way by relaxing the constraints to SingleRequestHostResolver to accept null addresses if the caller doesn't really care. eroman@, does this look ok?
https://codereview.chromium.org/2004453002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/login/auth/auth_prewarmer.cc (right): https://codereview.chromium.org/2004453002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/login/auth/auth_prewarmer.cc:100: ProfileHelper::GetSigninProfile()->GetResourceContext(), url, why this change? (i.e. as opposed to just removing base::RetainedRef()) (should be the same currently, but barring more information why not re-use the existing abstraction of this class, GetRequestContext() ?) https://codereview.chromium.org/2004453002/diff/180001/chrome/browser/net/pre... File chrome/browser/net/predictor.h (right): https://codereview.chromium.org/2004453002/diff/180001/chrome/browser/net/pre... chrome/browser/net/predictor.h:436: // Queue hostname for resolution. If queueing was done, return the pointer nit: Queue --> Queues (feel free to save that for another CL) https://codereview.chromium.org/2004453002/diff/180001/content/browser/loader... File content/browser/loader/resource_hints_impl.cc (right): https://codereview.chromium.org/2004453002/diff/180001/content/browser/loader... content/browser/loader/resource_hints_impl.cc:86: resolve_info, net::IDLE, nullptr, I would rather not introduce the concept of a null result int SingleRequestHostResolver. In part because I want to remove SingleRequestHostResolver [1], so any deviation from the HostResolver API will just cause problems during that migration. Instead how about binding the result lifetime to the callback, as in: AddressList* addresses = new AddressList; return resolver->Resolve( resolve_info, net::IDLE, addresses, base::Bind(&OnResolveComplete, base::Passed(base::WrapUnique(resolver)), base::Owned(addresses), callback) Kind of annoying to express this pattern with a std::unique_ptr<> + base::Passed() because of undefined argument evaluation order of C++, so for the example above just did the raw pointer version. [1] https://bugs.chromium.org/p/chromium/issues/detail?id=486264
Thanks for the review. https://codereview.chromium.org/2004453002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/login/auth/auth_prewarmer.cc (right): https://codereview.chromium.org/2004453002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/login/auth/auth_prewarmer.cc:100: ProfileHelper::GetSigninProfile()->GetResourceContext(), url, On 2016/05/31 20:45:27, eroman wrote: > why this change? (i.e. as opposed to just removing base::RetainedRef()) > > (should be the same currently, but barring more information why not re-use the > existing abstraction of this class, GetRequestContext() ?) Talked to mmenke@ about it and he said ResourceContext is the better abstraction than URLRequestContextGetter for this. Also, I have some plans to use the ResourceContext to delegate back up to the ProfileIOData in //chrome RDHD, which seems to be the common pattern there. https://codereview.chromium.org/2004453002/diff/180001/chrome/browser/net/pre... File chrome/browser/net/predictor.h (right): https://codereview.chromium.org/2004453002/diff/180001/chrome/browser/net/pre... chrome/browser/net/predictor.h:436: // Queue hostname for resolution. If queueing was done, return the pointer On 2016/05/31 20:45:27, eroman wrote: > nit: Queue --> Queues (feel free to save that for another CL) Done. https://codereview.chromium.org/2004453002/diff/180001/content/browser/loader... File content/browser/loader/resource_hints_impl.cc (right): https://codereview.chromium.org/2004453002/diff/180001/content/browser/loader... content/browser/loader/resource_hints_impl.cc:86: resolve_info, net::IDLE, nullptr, On 2016/05/31 20:45:27, eroman wrote: > I would rather not introduce the concept of a null result int > SingleRequestHostResolver. > > In part because I want to remove SingleRequestHostResolver [1], so any deviation > from the HostResolver API will just cause problems during that migration. > > Instead how about binding the result lifetime to the callback, as in: > > AddressList* addresses = new AddressList; > > return resolver->Resolve( > resolve_info, net::IDLE, addresses, > base::Bind(&OnResolveComplete, base::Passed(base::WrapUnique(resolver)), > base::Owned(addresses), callback) > > Kind of annoying to express this pattern with a std::unique_ptr<> + > base::Passed() because of undefined argument evaluation order of C++, so for the > example above just did the raw pointer version. > > [1] https://bugs.chromium.org/p/chromium/issues/detail?id=486264 Done. Thanks for the suggestion.
lgtm https://codereview.chromium.org/2004453002/diff/200001/content/browser/loader... File content/browser/loader/resource_hints_impl.cc (right): https://codereview.chromium.org/2004453002/diff/200001/content/browser/loader... content/browser/loader/resource_hints_impl.cc:30: // that information. Note that |addresses| will be destroyed when this or alternately if you want it to be self-documenting make it of type std::unique_ptr<net::AddressList> -- although then the construction is a bit uglier, but maybe worth the clarity.
Description was changed from ========== Add a Dns preresolve interface in //content This patch adds a preresolve interface in //content, with the sole consumer being the predictor in //chrome/browser/net. This is in preparation for sending resource hints from Blink to //content. The requests are routed through the ResourceHintsController, which is scoped to the RDHI. The controller keeps track of each request until completion, and calls a consumer-supplied callback. Note: this change also makes all dns preresolves by the predictor use a lower priority (net::IDLE) than the priority they had previously been using (net::DEFAULT_PRIORITY). BUG=610750,614350 ========== to ========== Add a Dns preresolve interface in //content This patch adds a preresolve interface in //content, with the sole consumer being the predictor in //chrome/browser/net. This is in preparation for sending resource hints from Blink to //content. The requests are routed through the ResourceHintsController, which is scoped to the RDHI. The controller keeps track of each request until completion, and calls a consumer-supplied callback. Note: this change also makes all dns preresolves by the predictor use a lower priority (net::IDLE) than the priority they had previously been using (net::DEFAULT_PRIORITY). TBR=kinuko@chromium.org BUG=610750,614350 ==========
Thanks! Moving kinuko@ to TBR for the small changes to fix the base::Passed bug. https://codereview.chromium.org/2004453002/diff/200001/content/browser/loader... File content/browser/loader/resource_hints_impl.cc (right): https://codereview.chromium.org/2004453002/diff/200001/content/browser/loader... content/browser/loader/resource_hints_impl.cc:30: // that information. Note that |addresses| will be destroyed when this On 2016/05/31 22:09:56, eroman wrote: > or alternately if you want it to be self-documenting make it of type > std::unique_ptr<net::AddressList> -- although then the construction is a bit > uglier, but maybe worth the clarity. Done.
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2004453002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2004453002/220001
https://codereview.chromium.org/2004453002/diff/220001/content/browser/loader... File content/browser/loader/resource_hints_impl.cc (right): https://codereview.chromium.org/2004453002/diff/220001/content/browser/loader... content/browser/loader/resource_hints_impl.cc:89: new net::SingleRequestHostResolver(resource_context->GetHostResolver()); style: a different approach is used for |resolver| and address. I would say either use std::unique_ptr<> for both, or use raw pointers + base::Owned() for both, but not a mix of the two. I think after reading this, I prefer the naked pointer + base::Owned() for readability, but up to you.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by csharrison@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from achuith@chromium.org, kinuko@chromium.org, eroman@chromium.org Link to the patchset: https://codereview.chromium.org/2004453002/#ps220001 (title: "Move AddressList to unique_ptr")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2004453002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2004453002/220001
The CQ bit was unchecked by csharrison@chromium.org
https://codereview.chromium.org/2004453002/diff/220001/content/browser/loader... File content/browser/loader/resource_hints_impl.cc (right): https://codereview.chromium.org/2004453002/diff/220001/content/browser/loader... content/browser/loader/resource_hints_impl.cc:89: new net::SingleRequestHostResolver(resource_context->GetHostResolver()); On 2016/05/31 23:03:28, eroman wrote: > style: a different approach is used for |resolver| and address. > > I would say either use std::unique_ptr<> for both, or use raw pointers + > base::Owned() for both, but not a mix of the two. > > I think after reading this, I prefer the naked pointer + base::Owned() for > readability, but up to you. Yeah I think I agree with you. I'll put up another patch in a little (might have to wait until thurs.
Alright I've changed both cases to use base::Owned.
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2004453002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2004453002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2004453002/diff/260001/content/browser/loader... File content/browser/loader/resource_hints_impl.cc (right): https://codereview.chromium.org/2004453002/diff/260001/content/browser/loader... content/browser/loader/resource_hints_impl.cc:25: // Note that |request| and |addresses| will be deleted when this function nit: How about phrasing this as: the lifetime of "request" and "address" is managed by the caller. (Strictly speaking their lifetime is tied to that of the callback -- the callback could be invoked multiple times and the deletion happens just once in its dtor.)
The CQ bit was checked by csharrison@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org, achuith@chromium.org, eroman@chromium.org Link to the patchset: https://codereview.chromium.org/2004453002/#ps280001 (title: "Comment change")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2004453002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2004453002/280001
Message was sent while issue was closed.
Description was changed from ========== Add a Dns preresolve interface in //content This patch adds a preresolve interface in //content, with the sole consumer being the predictor in //chrome/browser/net. This is in preparation for sending resource hints from Blink to //content. The requests are routed through the ResourceHintsController, which is scoped to the RDHI. The controller keeps track of each request until completion, and calls a consumer-supplied callback. Note: this change also makes all dns preresolves by the predictor use a lower priority (net::IDLE) than the priority they had previously been using (net::DEFAULT_PRIORITY). TBR=kinuko@chromium.org BUG=610750,614350 ========== to ========== Add a Dns preresolve interface in //content This patch adds a preresolve interface in //content, with the sole consumer being the predictor in //chrome/browser/net. This is in preparation for sending resource hints from Blink to //content. The requests are routed through the ResourceHintsController, which is scoped to the RDHI. The controller keeps track of each request until completion, and calls a consumer-supplied callback. Note: this change also makes all dns preresolves by the predictor use a lower priority (net::IDLE) than the priority they had previously been using (net::DEFAULT_PRIORITY). TBR=kinuko@chromium.org BUG=610750,614350 ==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== Add a Dns preresolve interface in //content This patch adds a preresolve interface in //content, with the sole consumer being the predictor in //chrome/browser/net. This is in preparation for sending resource hints from Blink to //content. The requests are routed through the ResourceHintsController, which is scoped to the RDHI. The controller keeps track of each request until completion, and calls a consumer-supplied callback. Note: this change also makes all dns preresolves by the predictor use a lower priority (net::IDLE) than the priority they had previously been using (net::DEFAULT_PRIORITY). TBR=kinuko@chromium.org BUG=610750,614350 ========== to ========== Add a Dns preresolve interface in //content This patch adds a preresolve interface in //content, with the sole consumer being the predictor in //chrome/browser/net. This is in preparation for sending resource hints from Blink to //content. The requests are routed through the ResourceHintsController, which is scoped to the RDHI. The controller keeps track of each request until completion, and calls a consumer-supplied callback. Note: this change also makes all dns preresolves by the predictor use a lower priority (net::IDLE) than the priority they had previously been using (net::DEFAULT_PRIORITY). TBR=kinuko@chromium.org BUG=610750,614350 Committed: https://crrev.com/f848daf2b342548c8ec90b21396097339688819a Cr-Commit-Position: refs/heads/master@{#397250} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/f848daf2b342548c8ec90b21396097339688819a Cr-Commit-Position: refs/heads/master@{#397250} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
