Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1857383002:
Refactor net predictor to use ResourceThrottles (Closed)
|
Created:
4 years, 8 months ago by Charlie Harrison Modified:
4 years, 6 months ago CC:
cbentzel+watch_chromium.org, chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefactor net predictor to use ResourceThrottles
The predictor currently hooks into ChromeNetworkDelegate for callbacks.
This patch reduces the number of moving parts by attaching a throttle for
resources coming into the RDHD.
BUG=601254
Committed: https://crrev.com/cb0f401524eb0009f583f8bb63959115013bf1a4
Cr-Commit-Position: refs/heads/master@{#396988}
Patch Set 1 #Patch Set 2 : bad dcheck #Patch Set 3 : #
Total comments: 24
Patch Set 4 : comments + code cleanup #
Total comments: 1
Patch Set 5 : more comment changes #
Total comments: 14
Patch Set 6 : mmenke@ review + additional test #
Total comments: 27
Patch Set 7 : Add another test, mmenke@ review #Patch Set 8 : Fix test #
Total comments: 14
Patch Set 9 : Comments + name changes #Patch Set 10 : rebase on #396550 #Messages
Total messages: 42 (11 generated)
Description was changed from ========== [WIP] Refactor net predictor to use ResourceThrottles BUG= ========== to ========== Refactor net predictor to use ResourceThrottles The predictor currently hooks into ChromeNetworkDelegate for callbacks. This patch reduces the number of moving parts by attaching a throttle for resources coming into the RDHD. BUG= ==========
csharrison@chromium.org changed reviewers: + mmenke@chromium.org
This can be your lowest priority review! I'd appreciate high level feedback on: - Exposing the predictor to ProfileIOData (feels wrong to me but we need it in RDHD). - Using resource throttles in general (limits the # of requests the predictor can see)
On 2016/04/06 21:05:35, csharrison wrote: > This can be your lowest priority review! > > I'd appreciate high level feedback on: > - Exposing the predictor to ProfileIOData (feels wrong to me but we need it in > RDHD). > - Using resource throttles in general (limits the # of requests the predictor > can see) Mind filing a bug for this? I think I'll have time to get to this tomorrow.
Description was changed from ========== Refactor net predictor to use ResourceThrottles The predictor currently hooks into ChromeNetworkDelegate for callbacks. This patch reduces the number of moving parts by attaching a throttle for resources coming into the RDHD. BUG= ========== to ========== Refactor net predictor to use ResourceThrottles The predictor currently hooks into ChromeNetworkDelegate for callbacks. This patch reduces the number of moving parts by attaching a throttle for resources coming into the RDHD. BUG=601254 ==========
https://codereview.chromium.org/1857383002/diff/40001/chrome/browser/profiles... File chrome/browser/profiles/profile_io_data.h (right): https://codereview.chromium.org/1857383002/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_io_data.h:249: virtual chrome_browser_net::Predictor* predictor(); As a virtual method, this should be named GetPredictor(). https://codereview.chromium.org/1857383002/diff/40001/chrome/browser/renderer... File chrome/browser/renderer_host/predictor_resource_throttle.cc (right): https://codereview.chromium.org/1857383002/diff/40001/chrome/browser/renderer... chrome/browser/renderer_host/predictor_resource_throttle.cc:17: void PredictorResourceThrottle::WillStartRequest(bool* defer) { Know this is just the old code, with the redirect stuff removed, but think you could beef up the comments a bit? It seems like a lot of code with little explanation. https://codereview.chromium.org/1857383002/diff/40001/chrome/browser/renderer... chrome/browser/renderer_host/predictor_resource_throttle.cc:18: GURL request_scheme_host( include gurl.h https://codereview.chromium.org/1857383002/diff/40001/chrome/browser/renderer... chrome/browser/renderer_host/predictor_resource_throttle.cc:22: // nit: Remove empty comment line - may make sense to have a blank line there. https://codereview.chromium.org/1857383002/diff/40001/chrome/browser/renderer... chrome/browser/renderer_host/predictor_resource_throttle.cc:29: content::ResourceType resource_type = info->GetResourceType(); include content/public/common/resource_type.h https://codereview.chromium.org/1857383002/diff/40001/chrome/browser/renderer... chrome/browser/renderer_host/predictor_resource_throttle.cc:30: bool is_main_frame = (resource_type == content::RESOURCE_TYPE_MAIN_FRAME); nit: Can just get rid of this bool, and inline the check below. https://codereview.chromium.org/1857383002/diff/40001/chrome/browser/renderer... chrome/browser/renderer_host/predictor_resource_throttle.cc:84: predictor_->timed_cache()->SetRecentlySeen(new_scheme_host); Note that all of this code changes behavior, since we aren't currently doing any of this on redirects. Not sure we should include this stuff in this CL, without tests. Do we have integration tests for any of the old code (It's clear from this CL we didn't have any unit tests)? https://codereview.chromium.org/1857383002/diff/40001/chrome/browser/renderer... File chrome/browser/renderer_host/predictor_resource_throttle.h (right): https://codereview.chromium.org/1857383002/diff/40001/chrome/browser/renderer... chrome/browser/renderer_host/predictor_resource_throttle.h:10: #include "net/url_request/url_request.h" Think these two can be forward declared. Same with RedirectInfo. https://codereview.chromium.org/1857383002/diff/40001/chrome/browser/renderer... chrome/browser/renderer_host/predictor_resource_throttle.h:12: class PredictorResourceThrottle : public content::ResourceThrottle { This should have some docs. https://codereview.chromium.org/1857383002/diff/40001/chrome/browser/renderer... chrome/browser/renderer_host/predictor_resource_throttle.h:12: class PredictorResourceThrottle : public content::ResourceThrottle { Hrm...The predictor itself is in net/, wonder if this should be as well. That does introduce a dependency on loader/ in net, but the predictor itself does depend on a fair number of things it probably shouldn't. Perhaps its' the predictor which doesn't belong in there. https://codereview.chromium.org/1857383002/diff/40001/chrome/browser/renderer... chrome/browser/renderer_host/predictor_resource_throttle.h:26: chrome_browser_net::Predictor* predictor_; DISALLOW_COPY_AND_ASSIGN, and include base/macros.h.
Thanks for the detailed review! I have one question about the redirects. https://codereview.chromium.org/1857383002/diff/40001/chrome/browser/renderer... File chrome/browser/renderer_host/predictor_resource_throttle.cc (right): https://codereview.chromium.org/1857383002/diff/40001/chrome/browser/renderer... chrome/browser/renderer_host/predictor_resource_throttle.cc:84: predictor_->timed_cache()->SetRecentlySeen(new_scheme_host); On 2016/04/07 at 22:12:49, mmenke wrote: > Note that all of this code changes behavior, since we aren't currently doing any of this on redirects. Not sure we should include this stuff in this CL, without tests. Do we have integration tests for any of the old code (It's clear from this CL we didn't have any unit tests)? Can you explain why the previous behavior does not track redirects? OnBeforeURLRequest is called for redirects. As far as I can tell it is keyed off URLRequest::Start. https://codereview.chromium.org/1857383002/diff/40001/chrome/browser/renderer... File chrome/browser/renderer_host/predictor_resource_throttle.h (right): https://codereview.chromium.org/1857383002/diff/40001/chrome/browser/renderer... chrome/browser/renderer_host/predictor_resource_throttle.h:12: class PredictorResourceThrottle : public content::ResourceThrottle { On 2016/04/07 at 22:12:50, mmenke wrote: > Hrm...The predictor itself is in net/, wonder if this should be as well. That does introduce a dependency on loader/ in net, but the predictor itself does depend on a fair number of things it probably shouldn't. Perhaps its' the predictor which doesn't belong in there. I agree that the predictor seems like it does not truly belong in net. I can look into moving it over here.
https://codereview.chromium.org/1857383002/diff/40001/chrome/browser/renderer... File chrome/browser/renderer_host/predictor_resource_throttle.cc (right): https://codereview.chromium.org/1857383002/diff/40001/chrome/browser/renderer... chrome/browser/renderer_host/predictor_resource_throttle.cc:84: predictor_->timed_cache()->SetRecentlySeen(new_scheme_host); On 2016/04/07 22:20:41, csharrison wrote: > On 2016/04/07 at 22:12:49, mmenke wrote: > > Note that all of this code changes behavior, since we aren't currently doing > any of this on redirects. Not sure we should include this stuff in this CL, > without tests. Do we have integration tests for any of the old code (It's clear > from this CL we didn't have any unit tests)? > > Can you explain why the previous behavior does not track redirects? > OnBeforeURLRequest is called for redirects. As far as I can tell it is keyed off > URLRequest::Start. Looks like you're right. Notice the "Only notify the delegate for the initial request." comment in URLRequest::Start. This was true for all of 3 weeks, at which point it was made untrue by the same person who added the comment. So now we've had that incorrect comment hanging around for 5 years. :( Also, ResourceHandler::OnWillStart is only called once per request, and since URLRequests are only started once from an external caller, I just assumed the methods were analogous. I had assumed redirects when through StartInternal, instead of Start (Which they used to do - not sure if I remembered the StartInternal call from 5 years ago or not, I may well have, since I was learning the code at that time).
I feel we should really take this opportunity to add some test coverage to the predictor_browsertests - at a minimum, test redirects (Same-site/cross-site), subframes (Which I should have suggested before), requests without a referrer (HTTPS making a CORS request to an HTTP page? Could also navigate or redirect from an HTTPS main frame to HTTP).
Yes! I am not planning on landing this without adding some tests. Thanks for the suggestions. I'm planning on doing a bit more with the predictor, so we really want to be sure that nothing breaks.
Can we revisit this one when you've got time? https://codereview.chromium.org/1857383002/diff/40001/chrome/browser/profiles... File chrome/browser/profiles/profile_io_data.h (right): https://codereview.chromium.org/1857383002/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_io_data.h:249: virtual chrome_browser_net::Predictor* predictor(); On 2016/04/07 22:12:49, mmenke wrote: > As a virtual method, this should be named GetPredictor(). Done. https://codereview.chromium.org/1857383002/diff/40001/chrome/browser/renderer... File chrome/browser/renderer_host/predictor_resource_throttle.cc (right): https://codereview.chromium.org/1857383002/diff/40001/chrome/browser/renderer... chrome/browser/renderer_host/predictor_resource_throttle.cc:17: void PredictorResourceThrottle::WillStartRequest(bool* defer) { On 2016/04/07 22:12:49, mmenke wrote: > Know this is just the old code, with the redirect stuff removed, but think you > could beef up the comments a bit? It seems like a lot of code with little > explanation. Yeah sure no problem. https://codereview.chromium.org/1857383002/diff/40001/chrome/browser/renderer... chrome/browser/renderer_host/predictor_resource_throttle.cc:18: GURL request_scheme_host( On 2016/04/07 22:12:49, mmenke wrote: > include gurl.h Done. https://codereview.chromium.org/1857383002/diff/40001/chrome/browser/renderer... chrome/browser/renderer_host/predictor_resource_throttle.cc:22: // On 2016/04/07 22:12:49, mmenke wrote: > nit: Remove empty comment line - may make sense to have a blank line there. Done. https://codereview.chromium.org/1857383002/diff/40001/chrome/browser/renderer... chrome/browser/renderer_host/predictor_resource_throttle.cc:29: content::ResourceType resource_type = info->GetResourceType(); On 2016/04/07 22:12:49, mmenke wrote: > include content/public/common/resource_type.h Done. https://codereview.chromium.org/1857383002/diff/40001/chrome/browser/renderer... chrome/browser/renderer_host/predictor_resource_throttle.cc:30: bool is_main_frame = (resource_type == content::RESOURCE_TYPE_MAIN_FRAME); On 2016/04/07 22:12:49, mmenke wrote: > nit: Can just get rid of this bool, and inline the check below. Done. https://codereview.chromium.org/1857383002/diff/40001/chrome/browser/renderer... File chrome/browser/renderer_host/predictor_resource_throttle.h (right): https://codereview.chromium.org/1857383002/diff/40001/chrome/browser/renderer... chrome/browser/renderer_host/predictor_resource_throttle.h:10: #include "net/url_request/url_request.h" On 2016/04/07 22:12:50, mmenke wrote: > Think these two can be forward declared. Same with RedirectInfo. Done. https://codereview.chromium.org/1857383002/diff/40001/chrome/browser/renderer... chrome/browser/renderer_host/predictor_resource_throttle.h:12: class PredictorResourceThrottle : public content::ResourceThrottle { On 2016/04/07 22:20:41, csharrison wrote: > On 2016/04/07 at 22:12:50, mmenke wrote: > > Hrm...The predictor itself is in net/, wonder if this should be as well. That > does introduce a dependency on loader/ in net, but the predictor itself does > depend on a fair number of things it probably shouldn't. Perhaps its' the > predictor which doesn't belong in there. > > I agree that the predictor seems like it does not truly belong in net. I can > look into moving it over here. I've added a TODO. https://codereview.chromium.org/1857383002/diff/40001/chrome/browser/renderer... chrome/browser/renderer_host/predictor_resource_throttle.h:12: class PredictorResourceThrottle : public content::ResourceThrottle { On 2016/04/07 22:12:50, mmenke wrote: > This should have some docs. Done. https://codereview.chromium.org/1857383002/diff/40001/chrome/browser/renderer... chrome/browser/renderer_host/predictor_resource_throttle.h:26: chrome_browser_net::Predictor* predictor_; On 2016/04/07 22:12:50, mmenke wrote: > DISALLOW_COPY_AND_ASSIGN, and include base/macros.h. Done. https://codereview.chromium.org/1857383002/diff/60001/chrome/browser/renderer... File chrome/browser/renderer_host/predictor_resource_throttle.h (right): https://codereview.chromium.org/1857383002/diff/60001/chrome/browser/renderer... chrome/browser/renderer_host/predictor_resource_throttle.h:22: // notifies the predictor of subframe / redirect requests to fire off Rewrite the paragraph.
Shockingly, I have no review backlog today.... So far. https://codereview.chromium.org/1857383002/diff/80001/chrome/browser/profiles... File chrome/browser/profiles/profile_io_data.cc (right): https://codereview.chromium.org/1857383002/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_io_data.cc:1343: chrome_browser_net::Predictor* ProfileIOData::GetPredictor() { Definition order should match declaration order. https://codereview.chromium.org/1857383002/diff/80001/chrome/browser/profiles... File chrome/browser/profiles/profile_io_data.h (right): https://codereview.chromium.org/1857383002/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_io_data.h:248: // non-OffTheRecord Profiles. Maybe replace "This is only implemented in non-OffTheRecord Profiles" with "Returns nullptr if there is no Predictor", optionally adding ", as is the case with OffTheRecord profiles." https://codereview.chromium.org/1857383002/diff/80001/chrome/browser/renderer... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/1857383002/diff/80001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:600: if (io_data->GetPredictor()) { I'd suggest avoiding the knowledge that we only create this throttle if ProfileIOData::GetPredictor returns non-NULL here. Instead I'd suggest: base::unique_ptr<content::ResourceThrottle> predictor_throttle = PredictorResourceThrottle::MaybeCreate(request, io_data); if (predictor_throttle) throttles->push_back(predictor_throttle.release()); While I don't see us ever adding more complexity to the logic here, it minimizes what this code needs to know about the throttle, and follows the pattern of most other logic in this method (Admittedly, other code here doesn't use smart pointers, but it should). https://codereview.chromium.org/1857383002/diff/80001/chrome/browser/renderer... File chrome/browser/renderer_host/predictor_resource_throttle.cc (right): https://codereview.chromium.org/1857383002/diff/80001/chrome/browser/renderer... chrome/browser/renderer_host/predictor_resource_throttle.cc:56: // Subresources for main frames usually get predicted when we detected the Verb tenses are awkward here ("detected" is past tense, "get predicted" is not). Also, avoid "we". Also note you mispelled predictions in the last line of this comment." Maybe "Subresources for main frames are predicted when navigation starts, in PredictorTabHelper. So only handle predictions for subframes here". Is there a reason for not handling subframes there? Or is it just that main frames have to be handled there, and this is the "default" spot to handle that logic? web contents_observer does have DidRedirectNavigation, so could probably put all making of predictions here, and all learning them here, so have all prediction making in one spot. https://codereview.chromium.org/1857383002/diff/80001/chrome/browser/renderer... chrome/browser/renderer_host/predictor_resource_throttle.cc:66: // Hook into redirect notifications to learn and initiate predictions based on "Hooks", "initiates", like the comment for the method above. https://codereview.chromium.org/1857383002/diff/80001/chrome/browser/renderer... chrome/browser/renderer_host/predictor_resource_throttle.cc:93: new_scheme_host, redirect_info.new_first_party_for_cookies); I don't think this should be called on non-main-frame / non-sub-frame resources. I don't think our tests are currently capable of catching that bug. :( https://codereview.chromium.org/1857383002/diff/80001/chrome/browser/renderer... File chrome/browser/renderer_host/predictor_resource_throttle.h (right): https://codereview.chromium.org/1857383002/diff/80001/chrome/browser/renderer... chrome/browser/renderer_host/predictor_resource_throttle.h:22: // referrer relationship, and populates the predictor's timed cache of ongoing relationship->relationships
PTAL. I also updated the logic so that we don't learn or predict any subresource redirect. That seems like a better policy we we don't actually perform predictions based on subresources. https://codereview.chromium.org/1857383002/diff/80001/chrome/browser/profiles... File chrome/browser/profiles/profile_io_data.cc (right): https://codereview.chromium.org/1857383002/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_io_data.cc:1343: chrome_browser_net::Predictor* ProfileIOData::GetPredictor() { On 2016/05/23 18:50:22, mmenke wrote: > Definition order should match declaration order. Done. https://codereview.chromium.org/1857383002/diff/80001/chrome/browser/profiles... File chrome/browser/profiles/profile_io_data.h (right): https://codereview.chromium.org/1857383002/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_io_data.h:248: // non-OffTheRecord Profiles. On 2016/05/23 18:50:22, mmenke wrote: > Maybe replace "This is only implemented in non-OffTheRecord Profiles" with > "Returns nullptr if there is no Predictor", optionally adding ", as is the case > with OffTheRecord profiles." Done. https://codereview.chromium.org/1857383002/diff/80001/chrome/browser/renderer... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/1857383002/diff/80001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:600: if (io_data->GetPredictor()) { On 2016/05/23 18:50:22, mmenke wrote: > I'd suggest avoiding the knowledge that we only create this throttle if > ProfileIOData::GetPredictor returns non-NULL here. Instead I'd suggest: > > base::unique_ptr<content::ResourceThrottle> predictor_throttle = > PredictorResourceThrottle::MaybeCreate(request, io_data); > if (predictor_throttle) > throttles->push_back(predictor_throttle.release()); > > While I don't see us ever adding more complexity to the logic here, it minimizes > what this code needs to know about the throttle, and follows the pattern of most > other logic in this method (Admittedly, other code here doesn't use smart > pointers, but it should). SGTM. Done. https://codereview.chromium.org/1857383002/diff/80001/chrome/browser/renderer... File chrome/browser/renderer_host/predictor_resource_throttle.cc (right): https://codereview.chromium.org/1857383002/diff/80001/chrome/browser/renderer... chrome/browser/renderer_host/predictor_resource_throttle.cc:56: // Subresources for main frames usually get predicted when we detected the On 2016/05/23 18:50:22, mmenke wrote: > Verb tenses are awkward here ("detected" is past tense, "get predicted" is not). > Also, avoid "we". Also note you mispelled predictions in the last line of this > comment." > > Maybe "Subresources for main frames are predicted when navigation starts, in > PredictorTabHelper. So only handle predictions for subframes here". Is there a > reason for not handling subframes there? Or is it just that main frames have to > be handled there, and this is the "default" spot to handle that logic? web > contents_observer does have DidRedirectNavigation, so could probably put all > making of predictions here, and all learning them here, so have all prediction > making in one spot. I think the tab helper was originally designed as an optimization, with all other logic residing here. Your suggestion makes sense, but I'd like to make that move when the PreconnectMore experiment is finished. That way, we can inject the subframe logic in DidStartNavigation without messing with the experiment. https://codereview.chromium.org/1857383002/diff/80001/chrome/browser/renderer... chrome/browser/renderer_host/predictor_resource_throttle.cc:66: // Hook into redirect notifications to learn and initiate predictions based on On 2016/05/23 18:50:22, mmenke wrote: > "Hooks", "initiates", like the comment for the method above. Done. https://codereview.chromium.org/1857383002/diff/80001/chrome/browser/renderer... chrome/browser/renderer_host/predictor_resource_throttle.cc:93: new_scheme_host, redirect_info.new_first_party_for_cookies); On 2016/05/23 18:50:22, mmenke wrote: > I don't think this should be called on non-main-frame / non-sub-frame resources. > I don't think our tests are currently capable of catching that bug. :( Wow you're right, even though this is the current behavior of the predictor. Added a test that catches this. https://codereview.chromium.org/1857383002/diff/80001/chrome/browser/renderer... File chrome/browser/renderer_host/predictor_resource_throttle.h (right): https://codereview.chromium.org/1857383002/diff/80001/chrome/browser/renderer... chrome/browser/renderer_host/predictor_resource_throttle.h:22: // referrer relationship, and populates the predictor's timed cache of ongoing On 2016/05/23 18:50:22, mmenke wrote: > relationship->relationships Done. https://codereview.chromium.org/1857383002/diff/100001/chrome/browser/net/pre... File chrome/browser/net/predictor_browsertest.cc (right): https://codereview.chromium.org/1857383002/diff/100001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:529: Will remove this line.
https://codereview.chromium.org/1857383002/diff/100001/chrome/browser/net/pre... File chrome/browser/net/predictor_browsertest.cc (right): https://codereview.chromium.org/1857383002/diff/100001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:83: Here's another extra line break for you to remove. Have to get them early, or they'll grow to encompass the entire chrome code base! https://codereview.chromium.org/1857383002/diff/100001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:817: redirector->ServeFilesFromSourceDirectory("chrome/test/data/"); I don't think we actually need a test server here, do we? We can just use the URL http://redirector/predictor/empty.js", right? https://codereview.chromium.org/1857383002/diff/100001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:829: // All requests to the redirector server should redirect to the -"the"? There are multiple embedded test servers here. https://codereview.chromium.org/1857383002/diff/100001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:857: EXPECT_EQ(1, observer()->CrossSitePreconnected()); Hrm...Should we make a subframe test that matches this? It's kind of nice to be able to look at the above test and say, "Aha, we aren't doing bonus preconnections on subresource redirects, but we are on subframe redirects". We do have subframe tests, and redirect tests already, though, I suppose... https://codereview.chromium.org/1857383002/diff/100001/chrome/browser/rendere... File chrome/browser/renderer_host/predictor_resource_throttle.cc (right): https://codereview.chromium.org/1857383002/diff/100001/chrome/browser/rendere... chrome/browser/renderer_host/predictor_resource_throttle.cc:54: predictor_->LearnAboutInitialNavigation(request_scheme_host); Not for this CL, but this is being called for non-navigation requests. Perhaps we should rename it. https://codereview.chromium.org/1857383002/diff/100001/chrome/browser/rendere... chrome/browser/renderer_host/predictor_resource_throttle.cc:54: predictor_->LearnAboutInitialNavigation(request_scheme_host); Also, worth noting there's a change in behavior here: This method was being called for all requests that go through the main URLRequestContext before. Now it's being called for all methods that go through the RDH. So what requests are those? Internal Chrome requests seem like the big one. Other requests are SDCH dictionary fetches (Which do have referrers, so could also affect per-navigation learning). There may be others (OCP requests?). https://codereview.chromium.org/1857383002/diff/100001/chrome/browser/rendere... chrome/browser/renderer_host/predictor_resource_throttle.cc:74: // based on the initial navigation. This comment assumes this is a navigation request, but we haven't made that check yet. I suggest combining this check with the one below (And doing it second, just because it makes logical sense, and it's marginally more expensive), and merging this comment with the one below. https://codereview.chromium.org/1857383002/diff/100001/chrome/browser/rendere... chrome/browser/renderer_host/predictor_resource_throttle.cc:99: if (new_scheme_host == original_scheme_host || new_scheme_host.is_empty()) Hrm...I kinda wonder if it's possible for "new_scheme_host" to be empty here. Oh well, not relevant for this CL. https://codereview.chromium.org/1857383002/diff/100001/chrome/browser/rendere... File chrome/browser/renderer_host/predictor_resource_throttle.h (right): https://codereview.chromium.org/1857383002/diff/100001/chrome/browser/rendere... chrome/browser/renderer_host/predictor_resource_throttle.h:25: // resource relationships. It notifies the predictor of all redirect and nit: Suggest removing "all" https://codereview.chromium.org/1857383002/diff/100001/chrome/browser/rendere... chrome/browser/renderer_host/predictor_resource_throttle.h:30: // requests. That is done on the UI thread in response to navigation callbacks nit: "off of main frame requests" -> "off of the initial main frame request (before any redirects)"
https://codereview.chromium.org/1857383002/diff/100001/chrome/browser/rendere... File chrome/browser/renderer_host/predictor_resource_throttle.cc (right): https://codereview.chromium.org/1857383002/diff/100001/chrome/browser/rendere... chrome/browser/renderer_host/predictor_resource_throttle.cc:54: predictor_->LearnAboutInitialNavigation(request_scheme_host); On 2016/05/24 20:27:34, mmenke wrote: > Also, worth noting there's a change in behavior here: This method was being > called for all requests that go through the main URLRequestContext before. Now > it's being called for all methods that go through the RDH. > > So what requests are those? Internal Chrome requests seem like the big one. > Other requests are SDCH dictionary fetches (Which do have referrers, so could > also affect per-navigation learning). There may be others (OCP requests?). Worth noting that SDCH dictionary requests do not set referrer, anyways, and even if it did, it may be the referring resource, rather than the top level resource, so we may miss them, anyways....unless the top level resource is the one that refers to the SDCH dictionary.
Thanks! https://codereview.chromium.org/1857383002/diff/100001/chrome/browser/net/pre... File chrome/browser/net/predictor_browsertest.cc (right): https://codereview.chromium.org/1857383002/diff/100001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:83: On 2016/05/24 20:27:34, mmenke wrote: > Here's another extra line break for you to remove. Have to get them early, or > they'll grow to encompass the entire chrome code base! Done. https://codereview.chromium.org/1857383002/diff/100001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:529: On 2016/05/23 21:13:32, csharrison wrote: > Will remove this line. Done. https://codereview.chromium.org/1857383002/diff/100001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:817: redirector->ServeFilesFromSourceDirectory("chrome/test/data/"); On 2016/05/24 20:27:34, mmenke wrote: > I don't think we actually need a test server here, do we? We can just use the > URL http://redirector/predictor/empty.js%22, right? Right. Done. https://codereview.chromium.org/1857383002/diff/100001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:829: // All requests to the redirector server should redirect to the On 2016/05/24 20:27:34, mmenke wrote: > -"the"? There are multiple embedded test servers here. Done. https://codereview.chromium.org/1857383002/diff/100001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:857: EXPECT_EQ(1, observer()->CrossSitePreconnected()); On 2016/05/24 20:27:34, mmenke wrote: > Hrm...Should we make a subframe test that matches this? It's kind of nice to be > able to look at the above test and say, "Aha, we aren't doing bonus > preconnections on subresource redirects, but we are on subframe redirects". We > do have subframe tests, and redirect tests already, though, I suppose... Done. I also fixed this test up a little. https://codereview.chromium.org/1857383002/diff/100001/chrome/browser/rendere... File chrome/browser/renderer_host/predictor_resource_throttle.cc (right): https://codereview.chromium.org/1857383002/diff/100001/chrome/browser/rendere... chrome/browser/renderer_host/predictor_resource_throttle.cc:54: predictor_->LearnAboutInitialNavigation(request_scheme_host); On 2016/05/24 20:27:34, mmenke wrote: > Not for this CL, but this is being called for non-navigation requests. Perhaps > we should rename it. Added a TODO. https://codereview.chromium.org/1857383002/diff/100001/chrome/browser/rendere... chrome/browser/renderer_host/predictor_resource_throttle.cc:54: predictor_->LearnAboutInitialNavigation(request_scheme_host); On 2016/05/24 21:39:31, mmenke wrote: > On 2016/05/24 20:27:34, mmenke wrote: > > Also, worth noting there's a change in behavior here: This method was being > > called for all requests that go through the main URLRequestContext before. > Now > > it's being called for all methods that go through the RDH. > > > > So what requests are those? Internal Chrome requests seem like the big one. > > Other requests are SDCH dictionary fetches (Which do have referrers, so could > > also affect per-navigation learning). There may be others (OCP requests?). > > Worth noting that SDCH dictionary requests do not set referrer, anyways, and > even if it did, it may be the referring resource, rather than the top level > resource, so we may miss them, anyways....unless the top level resource is the > one that refers to the SDCH dictionary. Okay thanks for the explanation. It seems like this is not a terrible change in behavior. In any case, there's a tradeoff for preresolving user facing urls vs internal urls. It's possible this change could be better for user experience, even (more user facing results in the top ten startup list). https://codereview.chromium.org/1857383002/diff/100001/chrome/browser/rendere... chrome/browser/renderer_host/predictor_resource_throttle.cc:74: // based on the initial navigation. On 2016/05/24 20:27:34, mmenke wrote: > This comment assumes this is a navigation request, but we haven't made that > check yet. I suggest combining this check with the one below (And doing it > second, just because it makes logical sense, and it's marginally more > expensive), and merging this comment with the one below. Done. https://codereview.chromium.org/1857383002/diff/100001/chrome/browser/rendere... chrome/browser/renderer_host/predictor_resource_throttle.cc:99: if (new_scheme_host == original_scheme_host || new_scheme_host.is_empty()) On 2016/05/24 20:27:34, mmenke wrote: > Hrm...I kinda wonder if it's possible for "new_scheme_host" to be empty here. > Oh well, not relevant for this CL. Acknowledged. https://codereview.chromium.org/1857383002/diff/100001/chrome/browser/rendere... File chrome/browser/renderer_host/predictor_resource_throttle.h (right): https://codereview.chromium.org/1857383002/diff/100001/chrome/browser/rendere... chrome/browser/renderer_host/predictor_resource_throttle.h:25: // resource relationships. It notifies the predictor of all redirect and On 2016/05/24 20:27:34, mmenke wrote: > nit: Suggest removing "all" Done. https://codereview.chromium.org/1857383002/diff/100001/chrome/browser/rendere... chrome/browser/renderer_host/predictor_resource_throttle.h:30: // requests. That is done on the UI thread in response to navigation callbacks On 2016/05/24 20:27:34, mmenke wrote: > nit: "off of main frame requests" -> "off of the initial main frame request > (before any redirects)" Done.
Quick response. Hope to sign off on this today, but may not happen until tomorrow. https://codereview.chromium.org/1857383002/diff/100001/chrome/browser/rendere... File chrome/browser/renderer_host/predictor_resource_throttle.cc (right): https://codereview.chromium.org/1857383002/diff/100001/chrome/browser/rendere... chrome/browser/renderer_host/predictor_resource_throttle.cc:54: predictor_->LearnAboutInitialNavigation(request_scheme_host); On 2016/05/24 21:43:22, csharrison wrote: > On 2016/05/24 21:39:31, mmenke wrote: > > On 2016/05/24 20:27:34, mmenke wrote: > > > Also, worth noting there's a change in behavior here: This method was being > > > called for all requests that go through the main URLRequestContext before. > > Now > > > it's being called for all methods that go through the RDH. > > > > > > So what requests are those? Internal Chrome requests seem like the big one. > > > > Other requests are SDCH dictionary fetches (Which do have referrers, so > could > > > also affect per-navigation learning). There may be others (OCP requests?). > > > > Worth noting that SDCH dictionary requests do not set referrer, anyways, and > > even if it did, it may be the referring resource, rather than the top level > > resource, so we may miss them, anyways....unless the top level resource is the > > one that refers to the SDCH dictionary. > > Okay thanks for the explanation. It seems like this is not a terrible change in > behavior. > > In any case, there's a tradeoff for preresolving user facing urls vs internal > urls. It's possible this change could be better for user experience, even (more > user facing results in the top ten startup list). Most internal requests won't have referrers, the question is just the pseudo-internal requests, which may set referrer - I'm not sure there are any, though.
https://codereview.chromium.org/1857383002/diff/100001/chrome/browser/rendere... File chrome/browser/renderer_host/predictor_resource_throttle.cc (right): https://codereview.chromium.org/1857383002/diff/100001/chrome/browser/rendere... chrome/browser/renderer_host/predictor_resource_throttle.cc:54: predictor_->LearnAboutInitialNavigation(request_scheme_host); On 2016/05/25 14:57:48, mmenke wrote: > On 2016/05/24 21:43:22, csharrison wrote: > > On 2016/05/24 21:39:31, mmenke wrote: > > > On 2016/05/24 20:27:34, mmenke wrote: > > > > Also, worth noting there's a change in behavior here: This method was > being > > > > called for all requests that go through the main URLRequestContext before. > > > > Now > > > > it's being called for all methods that go through the RDH. > > > > > > > > So what requests are those? Internal Chrome requests seem like the big > one. > > > > > > Other requests are SDCH dictionary fetches (Which do have referrers, so > > could > > > > also affect per-navigation learning). There may be others (OCP > requests?). > > > > > > Worth noting that SDCH dictionary requests do not set referrer, anyways, and > > > even if it did, it may be the referring resource, rather than the top level > > > resource, so we may miss them, anyways....unless the top level resource is > the > > > one that refers to the SDCH dictionary. > > > > Okay thanks for the explanation. It seems like this is not a terrible change > in > > behavior. > > > > In any case, there's a tradeoff for preresolving user facing urls vs internal > > urls. It's possible this change could be better for user experience, even > (more > > user facing results in the top ten startup list). > > Most internal requests won't have referrers, the question is just the > pseudo-internal requests, which may set referrer - I'm not sure there are any, > though. Gotcha. "LearnAboutInitialNavigation" doesn't care about referrers though.
https://codereview.chromium.org/1857383002/diff/100001/chrome/browser/rendere... File chrome/browser/renderer_host/predictor_resource_throttle.cc (right): https://codereview.chromium.org/1857383002/diff/100001/chrome/browser/rendere... chrome/browser/renderer_host/predictor_resource_throttle.cc:54: predictor_->LearnAboutInitialNavigation(request_scheme_host); On 2016/05/25 15:01:08, csharrison wrote: > On 2016/05/25 14:57:48, mmenke wrote: > > On 2016/05/24 21:43:22, csharrison wrote: > > > On 2016/05/24 21:39:31, mmenke wrote: > > > > On 2016/05/24 20:27:34, mmenke wrote: > > > > > Also, worth noting there's a change in behavior here: This method was > > being > > > > > called for all requests that go through the main URLRequestContext > before. > > > > > > Now > > > > > it's being called for all methods that go through the RDH. > > > > > > > > > > So what requests are those? Internal Chrome requests seem like the big > > one. > > > > > > > > Other requests are SDCH dictionary fetches (Which do have referrers, so > > > could > > > > > also affect per-navigation learning). There may be others (OCP > > requests?). > > > > > > > > Worth noting that SDCH dictionary requests do not set referrer, anyways, > and > > > > even if it did, it may be the referring resource, rather than the top > level > > > > resource, so we may miss them, anyways....unless the top level resource is > > the > > > > one that refers to the SDCH dictionary. > > > > > > Okay thanks for the explanation. It seems like this is not a terrible change > > in > > > behavior. > > > > > > In any case, there's a tradeoff for preresolving user facing urls vs > internal > > > urls. It's possible this change could be better for user experience, even > > (more > > > user facing results in the top ten startup list). > > > > Most internal requests won't have referrers, the question is just the > > pseudo-internal requests, which may set referrer - I'm not sure there are any, > > though. > > Gotcha. "LearnAboutInitialNavigation" doesn't care about referrers though. The rest of this code does, however... If there are requests that set referrer and don't go through ResourceLoader, they would have affected the predictor before, but after this CL, they won't. There certainly are requests that don't set referrer and don't go through ResourceLoader. Those did result in calling LearnAboutInitialNavigation before, and now they won't, so that is a change. Hrm...Predictor's comments indicate initial_observer_ is only intended for startup work, but I don't see it being deleted anywhere. Is that a bug, or am I missing something?
https://codereview.chromium.org/1857383002/diff/100001/chrome/browser/rendere... File chrome/browser/renderer_host/predictor_resource_throttle.cc (right): https://codereview.chromium.org/1857383002/diff/100001/chrome/browser/rendere... chrome/browser/renderer_host/predictor_resource_throttle.cc:54: predictor_->LearnAboutInitialNavigation(request_scheme_host); On 2016/05/25 15:21:20, mmenke wrote: > On 2016/05/25 15:01:08, csharrison wrote: > > On 2016/05/25 14:57:48, mmenke wrote: > > > On 2016/05/24 21:43:22, csharrison wrote: > > > > On 2016/05/24 21:39:31, mmenke wrote: > > > > > On 2016/05/24 20:27:34, mmenke wrote: > > > > > > Also, worth noting there's a change in behavior here: This method was > > > being > > > > > > called for all requests that go through the main URLRequestContext > > before. > > > > > > > > Now > > > > > > it's being called for all methods that go through the RDH. > > > > > > > > > > > > So what requests are those? Internal Chrome requests seem like the > big > > > one. > > > > > > > > > > Other requests are SDCH dictionary fetches (Which do have referrers, > so > > > > could > > > > > > also affect per-navigation learning). There may be others (OCP > > > requests?). > > > > > > > > > > Worth noting that SDCH dictionary requests do not set referrer, anyways, > > and > > > > > even if it did, it may be the referring resource, rather than the top > > level > > > > > resource, so we may miss them, anyways....unless the top level resource > is > > > the > > > > > one that refers to the SDCH dictionary. > > > > > > > > Okay thanks for the explanation. It seems like this is not a terrible > change > > > in > > > > behavior. > > > > > > > > In any case, there's a tradeoff for preresolving user facing urls vs > > internal > > > > urls. It's possible this change could be better for user experience, even > > > (more > > > > user facing results in the top ten startup list). > > > > > > Most internal requests won't have referrers, the question is just the > > > pseudo-internal requests, which may set referrer - I'm not sure there are > any, > > > though. > > > > Gotcha. "LearnAboutInitialNavigation" doesn't care about referrers though. > > The rest of this code does, however... > > If there are requests that set referrer and don't go through ResourceLoader, > they would have affected the predictor before, but after this CL, they won't. > > There certainly are requests that don't set referrer and don't go through > ResourceLoader. Those did result in calling LearnAboutInitialNavigation before, > and now they won't, so that is a change. > > Hrm...Predictor's comments indicate initial_observer_ is only intended for > startup work, but I don't see it being deleted anywhere. Is that a bug, or am I > missing something? And Charles pointed out to me that this is not an issue - initial_observer_ only checks the first x "navigations" on startup.
LGTM https://codereview.chromium.org/1857383002/diff/140001/chrome/browser/net/pre... File chrome/browser/net/predictor_browsertest.cc (right): https://codereview.chromium.org/1857383002/diff/140001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:844: GURL redirecting_url = cross_site_test_server()->GetURL(base::StringPrintf( The URL doesn't redirect, it requests a subresource that does. Maybe redirect_requesting_url? url_with_cors_subresources? https://codereview.chromium.org/1857383002/diff/140001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:851: int navigation_preconnects = observer()->CrossSitePreconnected(); Should we call WaitForAcceptedConnectionsOnUI? Looks like a couple other tests aren't doing that before checking preconnects. https://codereview.chromium.org/1857383002/diff/140001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:887: WaitForAcceptedConnectionsOnUI? https://codereview.chromium.org/1857383002/diff/140001/chrome/browser/rendere... File chrome/browser/renderer_host/predictor_resource_throttle.cc (right): https://codereview.chromium.org/1857383002/diff/140001/chrome/browser/rendere... chrome/browser/renderer_host/predictor_resource_throttle.cc:46: // requests, and to learn referrer relationships. I don't think either of these method-level comments adds any value - they're basically covered by the class description. https://codereview.chromium.org/1857383002/diff/140001/chrome/browser/rendere... chrome/browser/renderer_host/predictor_resource_throttle.cc:76: // prediction dispatching to the tab helper. Having a TODO in the middle of two informational comments can be confusing. Maybe move this to the header, below the class description? Or just below both other comments here. https://codereview.chromium.org/1857383002/diff/140001/chrome/browser/rendere... chrome/browser/renderer_host/predictor_resource_throttle.cc:99: if (new_scheme_host == original_scheme_host || new_scheme_host.is_empty()) BUG: We're comparing a non-canonicalized URL with a canonicalized one. I'd suggest keeping it as-is for now, but think the whole canonicalization thing here requires more thought. https://codereview.chromium.org/1857383002/diff/140001/chrome/browser/rendere... File chrome/browser/renderer_host/predictor_resource_throttle.h (right): https://codereview.chromium.org/1857383002/diff/140001/chrome/browser/rendere... chrome/browser/renderer_host/predictor_resource_throttle.h:32: // in predictor_tab_helper.cc. nit: Can merge lines 32 and 31
csharrison@chromium.org changed reviewers: + sky@chromium.org
+sky for OWNERS, PTAL! https://codereview.chromium.org/1857383002/diff/140001/chrome/browser/net/pre... File chrome/browser/net/predictor_browsertest.cc (right): https://codereview.chromium.org/1857383002/diff/140001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:844: GURL redirecting_url = cross_site_test_server()->GetURL(base::StringPrintf( On 2016/05/25 18:16:04, mmenke wrote: > The URL doesn't redirect, it requests a subresource that does. Maybe > redirect_requesting_url? url_with_cors_subresources? Done. https://codereview.chromium.org/1857383002/diff/140001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:851: int navigation_preconnects = observer()->CrossSitePreconnected(); On 2016/05/25 18:16:04, mmenke wrote: > Should we call WaitForAcceptedConnectionsOnUI? Looks like a couple other tests > aren't doing that before checking preconnects. I don't think we should here. We stopped blocking requests to navigate to the cross site url, so it's not clear how many sockets will be accepted at this point. In testing, it shows that 3 sockets are accepted after the navigation finishes. The other tests which don't wait have similar issues (e.g. don't block the cross site test server). https://codereview.chromium.org/1857383002/diff/140001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:887: On 2016/05/25 18:16:04, mmenke wrote: > WaitForAcceptedConnectionsOnUI? We do the check after checking observer()->CrossSitePreconnected(). This is because the observer will receive a preconnect notification before the load event, so it will be guaranteed to happen at this point. Also, it will give better error messages on failure. https://codereview.chromium.org/1857383002/diff/140001/chrome/browser/rendere... File chrome/browser/renderer_host/predictor_resource_throttle.cc (right): https://codereview.chromium.org/1857383002/diff/140001/chrome/browser/rendere... chrome/browser/renderer_host/predictor_resource_throttle.cc:46: // requests, and to learn referrer relationships. On 2016/05/25 18:16:04, mmenke wrote: > I don't think either of these method-level comments adds any value - they're > basically covered by the class description. Done. https://codereview.chromium.org/1857383002/diff/140001/chrome/browser/rendere... chrome/browser/renderer_host/predictor_resource_throttle.cc:76: // prediction dispatching to the tab helper. On 2016/05/25 18:16:04, mmenke wrote: > Having a TODO in the middle of two informational comments can be confusing. > Maybe move this to the header, below the class description? Or just below both > other comments here. Done. https://codereview.chromium.org/1857383002/diff/140001/chrome/browser/rendere... chrome/browser/renderer_host/predictor_resource_throttle.cc:99: if (new_scheme_host == original_scheme_host || new_scheme_host.is_empty()) On 2016/05/25 18:16:04, mmenke wrote: > BUG: We're comparing a non-canonicalized URL with a canonicalized one. I'd > suggest keeping it as-is for now, but think the whole canonicalization thing > here requires more thought. Added a note. https://codereview.chromium.org/1857383002/diff/140001/chrome/browser/rendere... File chrome/browser/renderer_host/predictor_resource_throttle.h (right): https://codereview.chromium.org/1857383002/diff/140001/chrome/browser/rendere... chrome/browser/renderer_host/predictor_resource_throttle.h:32: // in predictor_tab_helper.cc. On 2016/05/25 18:16:04, mmenke wrote: > nit: Can merge lines 32 and 31 Done.
On 2016/05/25 18:45:16, csharrison wrote: > +sky for OWNERS, PTAL! Make sure you tell people what you're asking them to review. It's often not immediately obvious from looking at the modified files. [sky]: Please review chrome/browser/renderer_host/. I'm an owner of the rest.
On 2016/05/25 19:23:57, mmenke wrote: > On 2016/05/25 18:45:16, csharrison wrote: > > +sky for OWNERS, PTAL! > > Make sure you tell people what you're asking them to review. It's often not > immediately obvious from looking at the modified files. > > [sky]: Please review chrome/browser/renderer_host/. I'm an owner of the rest. Oops sorry about that! Thanks for the correction, mmenke@.
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/1857383002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1857383002/160001
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/1857383002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1857383002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
sky@ friendly ping
LGTM
Thanks!
The CQ bit was checked by csharrison@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/1857383002/#ps180001 (title: "rebase on #396550")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1857383002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1857383002/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Refactor net predictor to use ResourceThrottles The predictor currently hooks into ChromeNetworkDelegate for callbacks. This patch reduces the number of moving parts by attaching a throttle for resources coming into the RDHD. BUG=601254 ========== to ========== Refactor net predictor to use ResourceThrottles The predictor currently hooks into ChromeNetworkDelegate for callbacks. This patch reduces the number of moving parts by attaching a throttle for resources coming into the RDHD. BUG=601254 Committed: https://crrev.com/cb0f401524eb0009f583f8bb63959115013bf1a4 Cr-Commit-Position: refs/heads/master@{#396988} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/cb0f401524eb0009f583f8bb63959115013bf1a4 Cr-Commit-Position: refs/heads/master@{#396988} |
