Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1881463003:
Add a browsertest suite for net predictor (Closed)
|
Created:
4 years, 8 months ago by Charlie Harrison Modified:
4 years, 7 months ago Reviewers:
mmenke 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. |
DescriptionAdd a browsertest suite for net predictor
This patch adds end to end tests for the predictor. These tests span multiple
navigations, asserting that host relationships are learned, and that
subsequent navigations will preconnect accurately.
BUG=602396
Committed: https://crrev.com/a7b1611d80ee455b77a8144a4afcb2248b854a55
Cr-Commit-Position: refs/heads/master@{#393682}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : Test overhaul #Patch Set 4 : Add a "forgetting" test + fix typo #
Total comments: 53
Patch Set 5 : mmenke@ review #
Total comments: 35
Patch Set 6 : Add synchronization RunLoops (hope to fix Win7 errors) #Patch Set 7 : Fix really silly hang add dead simple preconnect tests #Patch Set 8 : Add locks around {set_}preconnect_enabled() #Patch Set 9 : Refactor, disable preconnect + intercept request for determinism #
Total comments: 54
Patch Set 10 : Responded to minor comments #Patch Set 11 : Kill sockets on both ends #
Total comments: 77
Patch Set 12 : Responded to mmenke@, simplified things (always block requests, etc) #
Total comments: 16
Patch Set 13 : mmenke@ review #Patch Set 14 : EXPECT that we get the right number of accepted connections after waiting #Messages
Total messages: 44 (7 generated)
Description was changed from ========== [WIP] Add tests for net's predictor BUG= ========== to ========== [WIP] Add tests for net's predictor BUG=602396 ==========
Description was changed from ========== [WIP] Add tests for net's predictor BUG=602396 ========== to ========== Add a browsertest suite for net predictor This patch adds end to end tests for the predictor. These tests span multiple navigations, asserting that host relationships are learned, and that subsequent navigations will preconnect accurately. BUG=602396 ==========
csharrison@chromium.org changed reviewers: + mmenke@chromium.org
Hey Matt, here's the initial batch of browser tests for the predictor. Some notes: - I ended up intercepting all favicon.ico requests. I could circle back and do something better, but I think the tests are pretty solid without having to worry about this additional complexity. - The "forget" test keeps navigating until a prediction is forgotten. It takes us 2 navigations of re-learning before we stop preconnecting. This is possibly too aggressive. - Related to the "forget" test, I was curious why we don't see massive amounts of preconnects in about:dns. I tested this with cnn.com and found that entries get evicted pretty frequently. This might be an improvement for the predictor if we find that brand new entries often evict recently used entries. Thanks for helping out with this!
Probably won't get back to you until tomorrow. https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... File chrome/browser/net/predictor_browsertest.cc (right): https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:319: target_test_server()->GetURL("/favicon.ico"))); Do we still learn from these requests? Just wondering if there's still a possible race here.
https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... File chrome/browser/net/predictor_browsertest.cc (right): https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:319: target_test_server()->GetURL("/favicon.ico"))); On 2016/04/13 at 17:50:23, mmenke wrote: > Do we still learn from these requests? Just wondering if there's still a possible race here. I think we do still learn from these. They are a hint for self-preconnection, which I realize now I don't have tests for. There might be a race in that case :/. There may be some way to detect the favicon loading in Javascript.
I talked with Steven about this and we came to a few tentative solutions to the favicon thing: 1. Make the test harness a favicon observer: https://code.google.com/p/chromium/codesearch#chromium/src/components/favicon... 2. Use a method similar to the InfiniteResponse in embedded_test_server_unittests (i.e. somehow give the request handler some pointer to the test harness that we can post a task to.) 3. Improve the embedded_test_server_connection_listener to know about http connections, not just sockets. I think (3) seems like the best (improve embedded test server infrastructure). WDYT? I may be able to do this in a followup CL with same-server preconnect tests.
Some comments on your test fixture - still want to dig through all the test cases today, but wanted to get at least some comments out in the meantime. Sorry for slowness after asking you to do this - not heavily booked on reviews this week, just still recovering from last week. https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... File chrome/browser/net/predictor.cc (right): https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor.cc:471: } nit: Don't use braces on two-line ifs. https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... File chrome/browser/net/predictor.h (right): https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor.h:313: void set_preconnect_enabled(bool preconnect_enabled) { set_preconnect_enabled_for_test There's a clang check that _for_test / ForTest methods aren't called in production, I believe. https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor.h:315: } This doesn't seem threadsafe - this is accessed on both the IO and UI threads. Unfortunately, the test fixture adds a command line switch to disable preconnects (They break our really bad spawned test server). I don't suppose we can just remove that argument in SetUpCommandLine (Or whatever the test fixture method is called) https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... File chrome/browser/net/predictor_browsertest.cc (right): https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:82: base::AutoLock lock(lock_); This can be below the rv <= 0 check https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:102: size_t read_sockets = 0; Why did you remove the lock? If this coincides with a AcceptedSocket on the server thread, where we add a socket to sockets_, we're in trouble. Running at the same time as ReadFromSocket would result in a race, and threadsanitizer may whine about it, even if it is a benign race. https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:221: class NotFoundRequestInterceptor : public net::URLRequestInterceptor { Give this a public constructor and destructor, and a private DISALLOW_COPY_AND_ASSIGN. https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:231: true); URLRequestTestJobs are weird. Can we just use URLRequestFailedJob? https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:254: void OnPreconnectUrl(const GURL& original_url, I don't think these should be part of the test fixture - they seem very out of place in the tests of the renderer-initiated preconnects. Think it's best just making a separate class implementing PredictorObserver. https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:257: int count) override { These are called on the IO thread, but you're accessing preconnected_target_ and preconnected_source_ on the UI thread. Should either add locks around them or make these PostTask over to the UI thread. Could make them atomic, but I think that's overkill, except in production code. Atomics are in base::subtle for a reason. https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:261: preconnected_source_ = std::max(preconnected_source_, count); nit: Use braces in if/else (Style varies in Chrome, but generally in the network stack, we use them for all if-elses). https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:261: preconnected_source_ = std::max(preconnected_source_, count); If it's neither of those URLs, should we fail? https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:261: preconnected_source_ = std::max(preconnected_source_, count); If we're preconnecting some other URL, should we fail? https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:269: } What if they aren't equal? Should we do something then? https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:300: predictor()->SetObserver(this); Calling this on the UI thread after the IOThread has already been started seems like a bad idea - we should post a task to do it over to the IO thread. https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:315: // sockets to the test server. nit avoid "we" in comments. https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:388: int num_non_cors, Think we should rename cors/non_cors to match the test server names (currently target_test_server/embedded_test_server). See comment on second test server name further down. https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:391: std::string url = base::StringPrintf( Not a url - this is a path. As-is, the GetURL(url) line is particularly confusing. https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:434: std::unique_ptr<net::EmbeddedTestServer> target_test_server_; Think this needs a comment. I suggest calling it something more general - like maybe secondary_test_server or cross_site_test_server. If it were only redirects, "target" would make sense, but it's not. https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:442: NavigateToCrossSiteHTMLURL(0, 1, ""); Suggest you should add comments with the variable names for the first two arguments on all these tests, to make it clearer what's going on.
https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... File chrome/browser/net/predictor_browsertest.cc (right): https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:388: int num_non_cors, Oh...The fact that non_cors does not mean same site is confusing, at least to people as unfamiliar with the fetch spec as I am. https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:457: NavigateToCrossSiteHTMLURL(1, 1, ""); Why do we care so much about the cors vs. non-cors cases? https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:483: // logic could probably be removed. Oh wow...that's funky. Could we have a test where the host names don't match? Two ways to do it: localhost vs 127.0.0.1, or add mock host names - there's a mock host resolver somewhere in the BrowserTest fixture you can add rules to, though I think by default, it may map everything to 127.0.0.1 anyways. https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:527: EXPECT_EQ(4, preconnected_target_); How about also having a test where you just re-navigate the subframe? https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:531: // <meta name="referrer" content="never"> Is this really desired behavior, or just testing an implementation fluke, to be updated once that fluke is fixed? https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:557: // time. This gives us (2 + .33)*.66 + .33 + 1 ~= 2.87. nit: --we (x3). Well, one's an "us".
I tried to address all comments (except the set_preconnect_enabled_for_test). Otherwise this is ready for another review, though the Windows 7 failures look real and hard to debug :/ https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... File chrome/browser/net/predictor.cc (right): https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor.cc:471: } On 2016/04/15 at 15:44:02, mmenke wrote: > nit: Don't use braces on two-line ifs. Done. https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... File chrome/browser/net/predictor.h (right): https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor.h:313: void set_preconnect_enabled(bool preconnect_enabled) { On 2016/04/15 at 15:44:02, mmenke wrote: > set_preconnect_enabled_for_test > > There's a clang check that _for_test / ForTest methods aren't called in production, I believe. Done. https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor.h:315: } On 2016/04/15 at 15:44:02, mmenke wrote: > This doesn't seem threadsafe - this is accessed on both the IO and UI threads. > > Unfortunately, the test fixture adds a command line switch to disable preconnects (They break our really bad spawned test server). I don't suppose we can just remove that argument in SetUpCommandLine (Or whatever the test fixture method is called) Hmmm. I can try that in another CL to see if anything breaks. We can always selectively add the command line for breaking tests. I'll leave this as is for now with the expectation that I'll find a better solution later. What would you think about adding another flag to override this one? https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... File chrome/browser/net/predictor_browsertest.cc (right): https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:82: base::AutoLock lock(lock_); On 2016/04/15 at 15:44:03, mmenke wrote: > This can be below the rv <= 0 check Done. https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:102: size_t read_sockets = 0; On 2016/04/15 at 15:44:02, mmenke wrote: > Why did you remove the lock? If this coincides with a AcceptedSocket on the server thread, where we add a socket to sockets_, we're in trouble. Running at the same time as ReadFromSocket would result in a race, and threadsanitizer may whine about it, even if it is a benign race. This was a careless mistake probably from rearranging code. My apologies. Fixed. https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:221: class NotFoundRequestInterceptor : public net::URLRequestInterceptor { On 2016/04/15 at 15:44:03, mmenke wrote: > Give this a public constructor and destructor, and a private DISALLOW_COPY_AND_ASSIGN. Done. https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:231: true); On 2016/04/15 at 15:44:02, mmenke wrote: > URLRequestTestJobs are weird. Can we just use URLRequestFailedJob? Sure. I'm not sure what the best error is, but ABORTED holds a certain favor in my heart. https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:254: void OnPreconnectUrl(const GURL& original_url, On 2016/04/15 at 15:44:02, mmenke wrote: > I don't think these should be part of the test fixture - they seem very out of place in the tests of the renderer-initiated preconnects. Think it's best just making a separate class implementing PredictorObserver. Fair enough, done. https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:257: int count) override { On 2016/04/15 at 15:44:02, mmenke wrote: > These are called on the IO thread, but you're accessing preconnected_target_ and preconnected_source_ on the UI thread. Should either add locks around them or make these PostTask over to the UI thread. Could make them atomic, but I think that's overkill, except in production code. Atomics are in base::subtle for a reason. I added locks. https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:261: preconnected_source_ = std::max(preconnected_source_, count); On 2016/04/15 15:44:02, mmenke wrote: > nit: Use braces in if/else (Style varies in Chrome, but generally in the > network stack, we use them for all if-elses). Done. https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:261: preconnected_source_ = std::max(preconnected_source_, count); On 2016/04/15 15:44:03, mmenke wrote: > If we're preconnecting some other URL, should we fail? Yeah. Done. https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:261: preconnected_source_ = std::max(preconnected_source_, count); On 2016/04/15 15:44:03, mmenke wrote: > If it's neither of those URLs, should we fail? Yupp, done. https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:269: } On 2016/04/15 15:44:02, mmenke wrote: > What if they aren't equal? Should we do something then? I've added failure conditions. https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:300: predictor()->SetObserver(this); On 2016/04/15 15:44:03, mmenke wrote: > Calling this on the UI thread after the IOThread has already been started seems > like a bad idea - we should post a task to do it over to the IO thread. Done. The tests now call "InstallPredictorObserver" which posts to the IO thread. https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:315: // sockets to the test server. On 2016/04/15 15:44:03, mmenke wrote: > nit avoid "we" in comments. Done. Removed all "we"s in comments in this file. https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:388: int num_non_cors, On 2016/04/15 20:27:17, mmenke wrote: > Oh...The fact that non_cors does not mean same site is confusing, at least to > people as unfamiliar with the fetch spec as I am. Ok, I will update with a comment. https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:388: int num_non_cors, On 2016/04/15 15:44:02, mmenke wrote: > Think we should rename cors/non_cors to match the test server names (currently > target_test_server/embedded_test_server). See comment on second test server > name further down. So actually the non cors perform a fetch() to the target test server with mode: 'no-cors', which performs the fetch with an "opaque" response. So both end up going to the target test server. I think this might be a little unnecessary as a core part of the test harness. https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:391: std::string url = base::StringPrintf( On 2016/04/15 15:44:03, mmenke wrote: > Not a url - this is a path. As-is, the GetURL(url) line is particularly > confusing. Done. https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:434: std::unique_ptr<net::EmbeddedTestServer> target_test_server_; On 2016/04/15 15:44:02, mmenke wrote: > Think this needs a comment. I suggest calling it something more general - like > maybe secondary_test_server or cross_site_test_server. If it were only > redirects, "target" would make sense, but it's not. Done. Renamed to "cross_site_test_server". https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:442: NavigateToCrossSiteHTMLURL(0, 1, ""); On 2016/04/15 15:44:03, mmenke wrote: > Suggest you should add comments with the variable names for the first two > arguments on all these tests, to make it clearer what's going on. Done. https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:457: NavigateToCrossSiteHTMLURL(1, 1, ""); On 2016/04/15 at 20:27:17, mmenke wrote: > Why do we care so much about the cors vs. non-cors cases? It was originally because these separate into privacy-mode pools and non privacy mode pools. My tests didn't really use that fact so I think I'm going to remove the distinction. We can add specialized tests later to test the privacy mode functionality (like the existing tests for link rel). https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:483: // logic could probably be removed. On 2016/04/15 20:27:16, mmenke wrote: > Oh wow...that's funky. Could we have a test where the host names don't match? > Two ways to do it: localhost vs 127.0.0.1, or add mock host names - there's a > mock host resolver somewhere in the BrowserTest fixture you can add rules to, > though I think by default, it may map everything to 127.0.0.1 anyways. Added a test using localhost. https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:527: EXPECT_EQ(4, preconnected_target_); On 2016/04/15 20:27:17, mmenke wrote: > How about also having a test where you just re-navigate the subframe? Hm, so this test classifies the subframe as "subresource". It seems like you want a test that tests that subframe -> subresource prediction works. I've added a simple test that does this. https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:531: // <meta name="referrer" content="never"> On 2016/04/15 20:27:17, mmenke wrote: > Is this really desired behavior, or just testing an implementation fluke, to be > updated once that fluke is fixed? Well, the referrer tag is pretty architecturally important for the predictor, so it makes sense to test its limitations. Ideally we will lose this restriction. https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:557: // time. This gives us (2 + .33)*.66 + .33 + 1 ~= 2.87. On 2016/04/15 20:27:17, mmenke wrote: > nit: --we (x3). Well, one's an "us". Done.
Apologies this patch is getting pretty huge.
I added a WaitForConnectionsAccepted(size_t num_sockets) method to the connection listener in the hope that it fixes the Windows7 bots failure. That is, assuming it's due to raciness and not something more sinister. We'll see what the bots say. If this still doesn't work, I can spin up a windows machine for local debugging.
Some more comments on the test fixture. https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... File chrome/browser/net/predictor.h (right): https://codereview.chromium.org/1881463003/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor.h:315: } On 2016/04/20 12:36:42, csharrison wrote: > On 2016/04/15 at 15:44:02, mmenke wrote: > > This doesn't seem threadsafe - this is accessed on both the IO and UI threads. > > > > Unfortunately, the test fixture adds a command line switch to disable > preconnects (They break our really bad spawned test server). I don't suppose we > can just remove that argument in SetUpCommandLine (Or whatever the test fixture > method is called) > > Hmmm. I can try that in another CL to see if anything breaks. We can always > selectively add the command line for breaking tests. We can't really selectively add the flag - every single test that uses the embedded test server for a single navigation/resource will run into this. > I'll leave this as is for now with the expectation that I'll find a better > solution later. > > What would you think about adding another flag to override this one? I think adding another flag is a worse solution. https://codereview.chromium.org/1881463003/diff/80001/chrome/browser/net/pred... File chrome/browser/net/predictor.h (right): https://codereview.chromium.org/1881463003/diff/80001/chrome/browser/net/pred... chrome/browser/net/predictor.h:80: const GURL& target_url) {} Pre-existing issue, but this file should forward declare GURL - or actually, probably include it, since one method returns a GURL. https://codereview.chromium.org/1881463003/diff/80001/chrome/browser/net/pred... chrome/browser/net/predictor.h:312: // Used only for testing. Maybe add "Overrides command line flag to disable preconnect, which is added the browser test fixture." https://codereview.chromium.org/1881463003/diff/80001/chrome/browser/net/pred... File chrome/browser/net/predictor_browsertest.cc (right): https://codereview.chromium.org/1881463003/diff/80001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:115: void ClearSockets() { ClearSocketCounts? The external API is all about socket counts, tracking sockets themselves is more of an implementation detail. https://codereview.chromium.org/1881463003/diff/80001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:234: std::string headers(kHeaders, arraysize(kHeaders)); Above two lines aren't used. https://codereview.chromium.org/1881463003/diff/80001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:245: CrossSitePredictorObserver(const GURL source_host, const GURL cross_site_host) nit: const GURL& (x2) https://codereview.chromium.org/1881463003/diff/80001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:245: CrossSitePredictorObserver(const GURL source_host, const GURL cross_site_host) include gurl.h https://codereview.chromium.org/1881463003/diff/80001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:249: same_site_learned_(0), I don't think we ever use same_site_learned_? The cookie requests make it inherently racy, unless we always wait for them. https://codereview.chromium.org/1881463003/diff/80001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:264: NOTREACHED() << "Preconnected" << original_url Should use ADD_FAILURE() instead (A gtest macro that's similar to EXPECT_TRUE(false), instead of something similar to DCHECK(false)) https://codereview.chromium.org/1881463003/diff/80001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:278: } else if (referring_url != cross_site_host_) { What if target_url is unexpected? Can we just do: else if (referring_url != cross_site_host_ || (target_url == cross_site_host_ && target_url == source_host_)) Or maybe simpler, get rid of the else, and check if either URL is unexpected. We also don't expect cross_site_host_ to have a target URL of source_host_, do we? https://codereview.chromium.org/1881463003/diff/80001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:279: NOTREACHED() << "Learned " << referring_url << " => " << target_url ADD_FAILURE() https://codereview.chromium.org/1881463003/diff/80001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:323: mutable base::Lock lock_; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/1881463003/diff/80001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:353: std::unique_ptr<net::test_server::HttpResponse> HandleRequest( This method name should be clearer, and may need a comment. https://codereview.chromium.org/1881463003/diff/80001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:355: if (request.GetURL().path() != "/") Suggest taking this as an argument, then can have a better description of just what's doing on in the RegisterRequestHandler line, and think the code will be a little clearer. https://codereview.chromium.org/1881463003/diff/80001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:361: "Location", cross_site_test_server()->GetURL("/title1.html").spec()); Calling into the cross_site_test_server on the other test server's thread makes me nervous. Can we just make this method static, and pass in the GURL? We will need to start the cross_site_test_server before we register this handler, but not a big deal, I think. https://codereview.chromium.org/1881463003/diff/80001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:466: cross_site_test_server()->base_url())); Suggest just creating the observer_ in InstallPredictorObserver. Otherwise, have to reason about whether it's guaranteed to have been created before accessing it on the UI thread. https://codereview.chromium.org/1881463003/diff/80001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:472: // Navigate to an html file and request async fetches. Wait until the fetches Suggest "Navigate to an html file and request async fetches" -> "Navigate to an html file on embedded_test_server and tell it to request |num_cors| |num_cors| resources from the cross_site_test_server. Then waits for those requests to complete." https://codereview.chromium.org/1881463003/diff/80001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:487: browser()->tab_strip_model()->GetActiveWebContents(), "sendFetches()", Maybe sendFetches -> startFetchesAndWaitForReply? Open to other ideas, just want a name that makes it cleaerer this does more than send the requests. https://codereview.chromium.org/1881463003/diff/80001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:500: static void AddUrlInterceptor(const GURL url) { GURL&
Matt, can you take another look? https://codereview.chromium.org/1881463003/diff/80001/chrome/browser/net/pred... File chrome/browser/net/predictor.h (right): https://codereview.chromium.org/1881463003/diff/80001/chrome/browser/net/pred... chrome/browser/net/predictor.h:80: const GURL& target_url) {} On 2016/04/22 16:29:25, mmenke wrote: > Pre-existing issue, but this file should forward declare GURL - or actually, > probably include it, since one method returns a GURL. Done. https://codereview.chromium.org/1881463003/diff/80001/chrome/browser/net/pred... File chrome/browser/net/predictor_browsertest.cc (right): https://codereview.chromium.org/1881463003/diff/80001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:115: void ClearSockets() { On 2016/04/22 16:29:26, mmenke wrote: > ClearSocketCounts? The external API is all about socket counts, tracking > sockets themselves is more of an implementation detail. Done. https://codereview.chromium.org/1881463003/diff/80001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:234: std::string headers(kHeaders, arraysize(kHeaders)); On 2016/04/22 16:29:25, mmenke wrote: > Above two lines aren't used. Done. https://codereview.chromium.org/1881463003/diff/80001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:245: CrossSitePredictorObserver(const GURL source_host, const GURL cross_site_host) On 2016/04/22 16:29:25, mmenke wrote: > nit: const GURL& (x2) Done. https://codereview.chromium.org/1881463003/diff/80001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:245: CrossSitePredictorObserver(const GURL source_host, const GURL cross_site_host) On 2016/04/22 16:29:25, mmenke wrote: > include gurl.h Done. https://codereview.chromium.org/1881463003/diff/80001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:249: same_site_learned_(0), On 2016/04/22 16:29:25, mmenke wrote: > I don't think we ever use same_site_learned_? The cookie requests make it > inherently racy, unless we always wait for them. Removed. If we want to add tests for them we can later. https://codereview.chromium.org/1881463003/diff/80001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:264: NOTREACHED() << "Preconnected" << original_url On 2016/04/22 16:29:25, mmenke wrote: > Should use ADD_FAILURE() instead (A gtest macro that's similar to > EXPECT_TRUE(false), instead of something similar to DCHECK(false)) Done. https://codereview.chromium.org/1881463003/diff/80001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:278: } else if (referring_url != cross_site_host_) { On 2016/04/22 16:29:25, mmenke wrote: > What if target_url is unexpected? Can we just do: > > else if (referring_url != cross_site_host_ || (target_url == cross_site_host_ && > target_url == source_host_)) > > Or maybe simpler, get rid of the else, and check if either URL is unexpected. > We also don't expect cross_site_host_ to have a target URL of source_host_, do > we? The code in your comment is a bit unclear but I think I get your gist. There are three possibilities: source => target source => source target => target I've updated the if statement to reference these states, hopefully making it a bit more clear. https://codereview.chromium.org/1881463003/diff/80001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:279: NOTREACHED() << "Learned " << referring_url << " => " << target_url On 2016/04/22 16:29:25, mmenke wrote: > ADD_FAILURE() Done. https://codereview.chromium.org/1881463003/diff/80001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:323: mutable base::Lock lock_; On 2016/04/22 16:29:25, mmenke wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/1881463003/diff/80001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:353: std::unique_ptr<net::test_server::HttpResponse> HandleRequest( On 2016/04/22 16:29:25, mmenke wrote: > This method name should be clearer, and may need a comment. Updated the name to "RedirectForPathPathHandler". https://codereview.chromium.org/1881463003/diff/80001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:355: if (request.GetURL().path() != "/") On 2016/04/22 16:29:25, mmenke wrote: > Suggest taking this as an argument, then can have a better description of just > what's doing on in the RegisterRequestHandler line, and think the code will be a > little clearer. Done. https://codereview.chromium.org/1881463003/diff/80001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:361: "Location", cross_site_test_server()->GetURL("/title1.html").spec()); On 2016/04/22 16:29:25, mmenke wrote: > Calling into the cross_site_test_server on the other test server's thread makes > me nervous. Can we just make this method static, and pass in the GURL? We will > need to start the cross_site_test_server before we register this handler, but > not a big deal, I think. Done. https://codereview.chromium.org/1881463003/diff/80001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:466: cross_site_test_server()->base_url())); On 2016/04/22 16:29:25, mmenke wrote: > Suggest just creating the observer_ in InstallPredictorObserver. Otherwise, > have to reason about whether it's guaranteed to have been created before > accessing it on the UI thread. Done. https://codereview.chromium.org/1881463003/diff/80001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:472: // Navigate to an html file and request async fetches. Wait until the fetches On 2016/04/22 16:29:25, mmenke wrote: > Suggest "Navigate to an html file and request async fetches" -> "Navigate to an > html file on embedded_test_server and tell it to request |num_cors| |num_cors| > resources from the cross_site_test_server. Then waits for those requests to > complete." Done. https://codereview.chromium.org/1881463003/diff/80001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:487: browser()->tab_strip_model()->GetActiveWebContents(), "sendFetches()", On 2016/04/22 16:29:25, mmenke wrote: > Maybe sendFetches -> startFetchesAndWaitForReply? Open to other ideas, just > want a name that makes it cleaerer this does more than send the requests. SGTM. Done. https://codereview.chromium.org/1881463003/diff/80001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:500: static void AddUrlInterceptor(const GURL url) { On 2016/04/22 16:29:25, mmenke wrote: > GURL& Done.
Actually, looks like the win failures are still occurring. I must have mistaken the "trigger browsertest" success with the actual test result.
On 2016/04/28 19:08:42, csharrison wrote: > Actually, looks like the win failures are still occurring. I must have mistaken > the "trigger browsertest" success with the actual test result. Please ping me once you get things working under Windows, and I'll take a look then. Not rushing you, just making clear that I consider this "not in my court" at the moment.
Look at those nice green bots :) So, I mentioned briefly what I've done to make this work (selectively disabling preconnect/requests to keep cross site test server clean).
Despite my slowness (And my many comments). I'm pretty happy with this in its current state. I do want to see if we can get rid of the behavior where we start with the predictor disabled, and have a lot of minor comments, but seems reasonable. https://codereview.chromium.org/1881463003/diff/160001/chrome/browser/net/pre... File chrome/browser/net/predictor_browsertest.cc (right): https://codereview.chromium.org/1881463003/diff/160001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:50: net::URLRequestJob* EmptyRequestJob(net::URLRequest* request, CreateEmptyBodyRequestJob? The method itself isn't the job, and ERR_EMPTY_RESPONSE means no headers, either. https://codereview.chromium.org/1881463003/diff/160001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:56: "Expires: Thu, 1 Jan 2100 20:00:00 GMT\n" I don't think we need the expires header? https://codereview.chromium.org/1881463003/diff/160001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:60: std::string(kPlainTextHeaders, arraysize(kPlainTextHeaders)), "", true); std::string(kPlainTextHeaders, arraysize(kPlainTextHeaders)) can be replaced with "kPlainTextHeaders", since the string doesn't have any nulls in it, and it null terminated. https://codereview.chromium.org/1881463003/diff/160001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:63: net::URLRequestJob* NotFoundRequestJob(net::URLRequest* request, CreateFailingRequestJob? (It's not a 404 response, or an ERR_NOT_FOUND response, as this name impies) https://codereview.chromium.org/1881463003/diff/160001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:65: return new net::URLRequestFailedJob(request, delegate, net::ERR_ABORTED); ERR_ABORTED is a magical error code, that net shouldn't really be returning (Though it does, in some obscure cases). Can we just use ERR_FAILED? If this has to be ERR_ABORTED due to predictor logic, that should be found. https://codereview.chromium.org/1881463003/diff/160001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:139: // The UI thread will wait for exactly |n| items in soeckts_. nit: |sockets_| https://codereview.chromium.org/1881463003/diff/160001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:193: // If accept_n_ > 0, then the object is waiting for |accept_n_| sockets to be nit: |accept_n_| https://codereview.chromium.org/1881463003/diff/160001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:278: // port of the URL must match the port given in the constructor. Update comment. https://codereview.chromium.org/1881463003/diff/160001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:285: : callback_(job_callback), port_(port) {} suggest a clearer name for both the argument and the member variable. maybe create_job_callback_(create_job_callback) https://codereview.chromium.org/1881463003/diff/160001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:295: } nit: Blank ine between methods and member variables. https://codereview.chromium.org/1881463003/diff/160001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:298: int port_; nit: both these can be const https://codereview.chromium.org/1881463003/diff/160001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:299: DISALLOW_COPY_AND_ASSIGN(MatchingPortRequestInterceptor); nit: Blank line before DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/1881463003/diff/160001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:302: class CrossSitePredictorObserver Add description https://codereview.chromium.org/1881463003/diff/160001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:358: int cross_site_learned() const { These should use the BigSlowFunctionNamingScheme because of the lock https://codereview.chromium.org/1881463003/diff/160001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:381: mutable base::Lock lock_; suggest not making this mutable, and getting rid of the consts - think the mutable costs us more than the consts get us, in terms of cleanliness. https://codereview.chromium.org/1881463003/diff/160001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:382: DISALLOW_COPY_AND_ASSIGN(CrossSitePredictorObserver); nit: Blank line before DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/1881463003/diff/160001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:397: void StartInterceptingCrossSite() { Should have thread-safety DCHECKs on all of these (DCHECK_CURRENTLY_ON(BrowserThread::IO)) https://codereview.chromium.org/1881463003/diff/160001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:401: cross_site_test_server()->GetURL("/").EffectiveIntPort(), The test server's GetURL method isn't guaranteed to be threadsafe. Suggest just making this method static, and taking the URL as an argument. https://codereview.chromium.org/1881463003/diff/160001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:404: void StopInterceptingCrossSite() { Suggest you make this static, per above comment. https://codereview.chromium.org/1881463003/diff/160001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:419: } nit: Suggest a blank line between methods. https://codereview.chromium.org/1881463003/diff/160001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:419: } Suggest putting these 4 methods below SetUpOnIOThread - that way, all the set up during test fixture creation stuff is together, at the top. https://codereview.chromium.org/1881463003/diff/160001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:437: std::unique_ptr<net::test_server::HttpResponse> RedirectForPathHandler( This should be static. Actually, could even just be moved out of the test fixture. https://codereview.chromium.org/1881463003/diff/160001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:447: return std::move(response); Is the std::move needed here? May be because it's a unique_ptr<net::test_server::BasicHttpResponse> and you're returning it as a unique_ptr<net::test_server::HttpResponse>, but I'm not sure. https://codereview.chromium.org/1881463003/diff/160001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:563: void NavigateToCrossSiteHTMLURL(int num_cors, const char* file_suffix) { nit: HtmlUrl (A lot of class and methods capitalize both words, but camel case is now the preferred style). https://codereview.chromium.org/1881463003/diff/160001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:579: static void AddUrlInterceptor(const GURL& url) { This method name doesn't make the behavior clear. AddFailRequestInterceptor? https://codereview.chromium.org/1881463003/diff/160001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:579: static void AddUrlInterceptor(const GURL& url) { I guess this is needed because failed requests aren't learned by the predictor? Where's that logic? ChromeNetworkDelegate::OnBeforeURLRequest calls WitnessURLRequest, which happens before requests make it to the interceptors. https://codereview.chromium.org/1881463003/diff/160001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:629: // no races and the connections are deterministic. Could we just wait until we see the expected number of sockets from a "clean slate" state, and then flush them? Turning the predictor off and then on just seems a bit unusual from the usual case, from the predictor's standpoint. Coincidentally, it also gives us coverage of the number of preconnects we make in the no-knowledge state. https://codereview.chromium.org/1881463003/diff/160001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:1140: IN_PROC_BROWSER_TEST_F(PredictorBrowserTest, SimplePreconnectOne) { Suggest putting these first (Think that putting the more basic tests before the ones that depend on it can help with picking the best test to start debugging when you run into a regression).
On 2016/05/06 19:54:02, mmenke wrote: > Despite my slowness (And my many comments). I'm pretty happy with this in its > current state. I do want to see if we can get rid of the behavior where we > start with the predictor disabled, and have a lot of minor comments, but seems > reasonable. Oh, and about the disabled thing - I don't suppose we can jsut wait to see the number of expected preconnects for the initial navigation, since real requests shouldn't make it to the socket layer?
The current patchset keeps the predictor disabling behavior. I'm nervous enabling preconnect, waiting until the sockets are connected, then flushing them will be difficult to implement. The previous test infra failed actually get the test in a clean slate position after the cross site server had any sockets connected. Granted, the previous patches sent the actual requests too (no interceptor), so it might be a different story. I don't quite see how it will be different though. https://codereview.chromium.org/1881463003/diff/160001/chrome/browser/net/pre... File chrome/browser/net/predictor_browsertest.cc (right): https://codereview.chromium.org/1881463003/diff/160001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:50: net::URLRequestJob* EmptyRequestJob(net::URLRequest* request, On 2016/05/06 19:54:01, mmenke wrote: > CreateEmptyBodyRequestJob? The method itself isn't the job, and > ERR_EMPTY_RESPONSE means no headers, either. Done. https://codereview.chromium.org/1881463003/diff/160001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:56: "Expires: Thu, 1 Jan 2100 20:00:00 GMT\n" On 2016/05/06 19:54:01, mmenke wrote: > I don't think we need the expires header? Done. https://codereview.chromium.org/1881463003/diff/160001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:60: std::string(kPlainTextHeaders, arraysize(kPlainTextHeaders)), "", true); On 2016/05/06 19:54:00, mmenke wrote: > std::string(kPlainTextHeaders, arraysize(kPlainTextHeaders)) can be replaced > with "kPlainTextHeaders", since the string doesn't have any nulls in it, and it > null terminated. Done. https://codereview.chromium.org/1881463003/diff/160001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:63: net::URLRequestJob* NotFoundRequestJob(net::URLRequest* request, On 2016/05/06 19:54:01, mmenke wrote: > CreateFailingRequestJob? (It's not a 404 response, or an ERR_NOT_FOUND > response, as this name impies) Done. https://codereview.chromium.org/1881463003/diff/160001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:65: return new net::URLRequestFailedJob(request, delegate, net::ERR_ABORTED); On 2016/05/06 19:54:01, mmenke wrote: > ERR_ABORTED is a magical error code, that net shouldn't really be returning > (Though it does, in some obscure cases). Can we just use ERR_FAILED? > > If this has to be ERR_ABORTED due to predictor logic, that should be found. Done. https://codereview.chromium.org/1881463003/diff/160001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:139: // The UI thread will wait for exactly |n| items in soeckts_. On 2016/05/06 19:54:01, mmenke wrote: > nit: |sockets_| Done. https://codereview.chromium.org/1881463003/diff/160001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:193: // If accept_n_ > 0, then the object is waiting for |accept_n_| sockets to be On 2016/05/06 19:54:01, mmenke wrote: > nit: |accept_n_| Done. https://codereview.chromium.org/1881463003/diff/160001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:278: // port of the URL must match the port given in the constructor. On 2016/05/06 19:54:01, mmenke wrote: > Update comment. Done. https://codereview.chromium.org/1881463003/diff/160001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:285: : callback_(job_callback), port_(port) {} On 2016/05/06 19:54:01, mmenke wrote: > suggest a clearer name for both the argument and the member variable. maybe > create_job_callback_(create_job_callback) Done. https://codereview.chromium.org/1881463003/diff/160001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:295: } On 2016/05/06 19:54:01, mmenke wrote: > nit: Blank ine between methods and member variables. Done. https://codereview.chromium.org/1881463003/diff/160001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:298: int port_; On 2016/05/06 19:54:00, mmenke wrote: > nit: both these can be const Done. https://codereview.chromium.org/1881463003/diff/160001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:299: DISALLOW_COPY_AND_ASSIGN(MatchingPortRequestInterceptor); On 2016/05/06 19:54:01, mmenke wrote: > nit: Blank line before DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/1881463003/diff/160001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:302: class CrossSitePredictorObserver On 2016/05/06 19:54:01, mmenke wrote: > Add description Done. https://codereview.chromium.org/1881463003/diff/160001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:358: int cross_site_learned() const { On 2016/05/06 19:54:00, mmenke wrote: > These should use the BigSlowFunctionNamingScheme because of the lock Done. https://codereview.chromium.org/1881463003/diff/160001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:381: mutable base::Lock lock_; On 2016/05/06 19:54:00, mmenke wrote: > suggest not making this mutable, and getting rid of the consts - think the > mutable costs us more than the consts get us, in terms of cleanliness. Done. https://codereview.chromium.org/1881463003/diff/160001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:382: DISALLOW_COPY_AND_ASSIGN(CrossSitePredictorObserver); On 2016/05/06 19:54:01, mmenke wrote: > nit: Blank line before DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/1881463003/diff/160001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:397: void StartInterceptingCrossSite() { On 2016/05/06 19:54:00, mmenke wrote: > Should have thread-safety DCHECKs on all of these > (DCHECK_CURRENTLY_ON(BrowserThread::IO)) Done. Put it on the most relevant methods. https://codereview.chromium.org/1881463003/diff/160001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:401: cross_site_test_server()->GetURL("/").EffectiveIntPort(), On 2016/05/06 19:54:01, mmenke wrote: > The test server's GetURL method isn't guaranteed to be threadsafe. Suggest just > making this method static, and taking the URL as an argument. Done. https://codereview.chromium.org/1881463003/diff/160001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:404: void StopInterceptingCrossSite() { On 2016/05/06 19:54:00, mmenke wrote: > Suggest you make this static, per above comment. Done. https://codereview.chromium.org/1881463003/diff/160001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:419: } On 2016/05/06 19:54:01, mmenke wrote: > nit: Suggest a blank line between methods. Done. https://codereview.chromium.org/1881463003/diff/160001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:419: } On 2016/05/06 19:54:01, mmenke wrote: > Suggest putting these 4 methods below SetUpOnIOThread - that way, all the set up > during test fixture creation stuff is together, at the top. Done. https://codereview.chromium.org/1881463003/diff/160001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:437: std::unique_ptr<net::test_server::HttpResponse> RedirectForPathHandler( On 2016/05/06 19:54:02, mmenke wrote: > This should be static. Actually, could even just be moved out of the test > fixture. Done. https://codereview.chromium.org/1881463003/diff/160001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:447: return std::move(response); On 2016/05/06 19:54:01, mmenke wrote: > Is the std::move needed here? May be because it's a > unique_ptr<net::test_server::BasicHttpResponse> and you're returning it as a > unique_ptr<net::test_server::HttpResponse>, but I'm not sure. Yeah it's needed. https://codereview.chromium.org/1881463003/diff/160001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:563: void NavigateToCrossSiteHTMLURL(int num_cors, const char* file_suffix) { On 2016/05/06 19:54:00, mmenke wrote: > nit: HtmlUrl (A lot of class and methods capitalize both words, but camel case > is now the preferred style). Done. https://codereview.chromium.org/1881463003/diff/160001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:579: static void AddUrlInterceptor(const GURL& url) { On 2016/05/06 19:54:01, mmenke wrote: > I guess this is needed because failed requests aren't learned by the predictor? > Where's that logic? ChromeNetworkDelegate::OnBeforeURLRequest calls > WitnessURLRequest, which happens before requests make it to the interceptors. Actually, I think this could use the regular interceptor. This is an artifact of previous patches. favicon is self-learned, so we don't interfere with cross site learning. We only need to avoid connecting a socket. https://codereview.chromium.org/1881463003/diff/160001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:1140: IN_PROC_BROWSER_TEST_F(PredictorBrowserTest, SimplePreconnectOne) { On 2016/05/06 19:54:00, mmenke wrote: > Suggest putting these first (Think that putting the more basic tests before the > ones that depend on it can help with picking the best test to start debugging > when you run into a regression). Done.
On 2016/05/09 19:47:05, csharrison wrote: > The current patchset keeps the predictor disabling behavior. > > I'm nervous enabling preconnect, waiting until the sockets are connected, then > flushing them will be difficult to implement. The previous test infra failed > actually get the test in a clean slate position after the cross site server had > any sockets connected. > > Granted, the previous patches sent the actual requests too (no interceptor), so > it might be a different story. I don't quite see how it will be different > though. The difference is that we now know exactly how many connection attempts should be made. With no favicon request, no possible races between preconnects and actual connections, I don't think there will be any mysterious counting mismatches, if we just wait for how many ever preconnects we expect (0, for the first navigation? Do we even expect any connections, the first time?) Another suggested test: Navigate to the same test URL 3 times, to make sure we update existing preconnect information correctly.
On 2016/05/11 16:37:10, mmenke wrote: > On 2016/05/09 19:47:05, csharrison wrote: > > The current patchset keeps the predictor disabling behavior. > > > > I'm nervous enabling preconnect, waiting until the sockets are connected, then > > flushing them will be difficult to implement. The previous test infra failed > > actually get the test in a clean slate position after the cross site server > had > > any sockets connected. > > > > Granted, the previous patches sent the actual requests too (no interceptor), > so > > it might be a different story. I don't quite see how it will be different > > though. > > The difference is that we now know exactly how many connection attempts should > be made. With no favicon request, no possible races between preconnects and > actual connections, I don't think there will be any mysterious counting > mismatches, if we just wait for how many ever preconnects we expect (0, for the > first navigation? Do we even expect any connections, the first time?) > > Another suggested test: Navigate to the same test URL 3 times, to make sure we > update existing preconnect information correctly. Could also do a test with two different cross-origin servers.
On 2016/05/11 16:37:48, mmenke wrote: > On 2016/05/11 16:37:10, mmenke wrote: > > On 2016/05/09 19:47:05, csharrison wrote: > > > The current patchset keeps the predictor disabling behavior. > > > > > > I'm nervous enabling preconnect, waiting until the sockets are connected, > then > > > flushing them will be difficult to implement. The previous test infra failed > > > actually get the test in a clean slate position after the cross site server > > had > > > any sockets connected. > > > > > > Granted, the previous patches sent the actual requests too (no interceptor), > > so > > > it might be a different story. I don't quite see how it will be different > > > though. > > > > The difference is that we now know exactly how many connection attempts should > > be made. With no favicon request, no possible races between preconnects and > > actual connections, I don't think there will be any mysterious counting > > mismatches, if we just wait for how many ever preconnects we expect (0, for > the > > first navigation? Do we even expect any connections, the first time?) > > > > Another suggested test: Navigate to the same test URL 3 times, to make sure > we > > update existing preconnect information correctly. > > Could also do a test with two different cross-origin servers. (Fine to punt on those two suggested tests, or leave them for another CL - this one is already pretty big)
Punted on the tests, but I've fixed the tests to work on Windows again by killing sockets on both ends. I'll update the bug with nice-to-have tests to do asynchronously with later work.
Didn't quite make it through all the tests https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... File chrome/browser/net/predictor.h (right): https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor.h:305: bool preconnect_enabled() const { Naming scheme should be changed, and method de-inlined since we're grabbing a lock. https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor.h:305: bool preconnect_enabled() const { optional: Could use an atomic instead of a lock here. Doesn't really matter, I suppose. (Naming scheme should still change, even if we switch) https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor.h:316: void set_preconnect_enabled_for_test(bool preconnect_enabled) { Again, naming scheme should be changed, and should be de-inlined. https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor.h:581: bool preconnect_enabled_; Should mention it's protected by the lock. Or make it atomic, per above suggestions. https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor.h:614: mutable base::Lock lock_; Should mention what this protects, and could even call it preconnect_enabled_lock_. https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... File chrome/browser/net/predictor_browsertest.cc (right): https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:150: // The UI thread will wait for exactly |n| items in sockets_. nit: sockets_ https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:152: DCHECK_CURRENTLY_ON(BrowserThread::UI); DCHECK(!run_loop_); https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:161: base::Unretained(this))); Why do we need to do this on the IO thread? Can't we call CheckAccepted() directly? Could even check if "sockets_.size() == n" right after getting the lock, and if so, exit without spinning the loop. https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:171: void CheckAccepted() { ASSERT_TRUE(lock_.AssertAcquired()); https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:171: void CheckAccepted() { Should probably document some of these. https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:173: if (accept_n_ == sockets_.size()) { nit: Early return is generally preferred, unless it's likely to lead to bugs (i.e. if new code is added after the if, it's likely you'll want it to run in both cases) https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:212: base::RunLoop* accept_n_loop_; Should make it clearer what's protected by the lock, and what isn't. Maybe put everything protected by it in a row, with a comment. ... https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:212: base::RunLoop* accept_n_loop_; Optional: Suggest making this a scoped_ptr and creating it + resetting it in the new wait function. It seems to be a more common patter (And saves one whole line of code!) https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:301: net::NetworkDelegate*)> nit: include base/callback.h and base/bind.h https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:316: net::NetworkDelegate*)> nit: Suggest a typedef at the start of the class. Doesn't really save us any lines of code, but it's a bit prettier. https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:345: cross_site_preconnected_ = std::max(cross_site_preconnected_, count); nit: #include <algorithm> https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:349: ADD_FAILURE() << "Preconnected" << original_url nit: "Preconnected" -> "Preconnected " https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:350: << " When should only be preconnecting the source host: " nit: When -> when https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:370: << " When should only be learning the source host: " nit: when https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:400: GURL cross_site_host_; nit: const (x2) https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:406: base::Lock lock_; Should document what this lock protects. https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:406: base::Lock lock_; nit: Weird to have a blank line between cross_site_learned_ and cross_site_preconnected_, but not one before the lock. https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:470: base::Bind(&CreateEmptyBodyRequestJob)))); Do we actually need to do this? As you said, they're all for the main site, anyways. https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:474: // two test servers. Should also document what this method actually does (Something along the lines of "Intercepts all requests to the specified host and returns a response with an empty body. Needed to prevent requests from actually going to the test server, to avoid any races related to socket accounting." https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:478: "http", url.host(), "http" -> url.scheme()? Doesn't really matter, but since we're taking a URL, seems like we should be using it. https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:485: net::URLRequestFilter::GetInstance()->RemoveHostnameHandler("http", "http" -> url.scheme() https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:494: cross_site_test_server()->GetURL("/"))); nit: GetURL("/") -> base_url() https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:502: cross_site_test_server()->GetURL("/"))); base_url() https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:601: base::RunLoop wait_loop; nit: run_loop? Just for consistency https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:642: std::unique_ptr<ConnectionListener> cross_site_connection_listener_; include <memory> https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:651: scoped_refptr<base::SingleThreadTaskRunner> task_runner_; include base/memory/ref_counted.h https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:659: connection_listener_->WaitForAcceptedConnectionsOnUI(1u); Hrm...Should we make WaitForAcceptedConnectionsOnUI have an EXPECT failure if we see too many sockets? As long as we make sure real cross-site requests don't make it to the socket pools, think there are no concerns about real requests racing with preconnects. https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:704: // and disable preconnects to the cross site test server. This allows the test We don't disable preconnects any more. https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:717: InstallPredictorObserver(); Should we call these two in the test fixture itself? Don't think they hurt for tests that don't use them, and we call them on a number of tests. https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:720: StopInterceptingCrossSiteOnUI(); Do we need to stop intercepting? We're just interested in preconnects, and these could confuse things (WaitForAcceptedConnectionsOnUI(4u) could succeed, even if we don't preconnect - admittedly, we also check with the observer, but I'm paranoid). https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:743: // embedded test servers have the same host_piece, an extra preconnect is host_piece -> host name? https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:772: // The redirect causes two preconnects for the initial navigation. This isn't a redirect - it's a (subframe) navigation. https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:793: IN_PROC_BROWSER_TEST_F(PredictorBrowserTest, SubframeInitiatesPreconnects) { I'm not following what this test does. If we go to <same_site>/predictor/predictor_cross_site, and then to a data URL that contains <same_site>/title1.html, why do we preconnect to cross site at all?
Responded to initial comments. Thanks for such a detailed review :) https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... File chrome/browser/net/predictor.h (right): https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor.h:305: bool preconnect_enabled() const { On 2016/05/12 16:30:23, mmenke wrote: > optional: Could use an atomic instead of a lock here. Doesn't really matter, I > suppose. (Naming scheme should still change, even if we switch) Kept the lock for simplicity. https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor.h:305: bool preconnect_enabled() const { On 2016/05/12 16:30:23, mmenke wrote: > Naming scheme should be changed, and method de-inlined since we're grabbing a > lock. Done. https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor.h:316: void set_preconnect_enabled_for_test(bool preconnect_enabled) { On 2016/05/12 16:30:22, mmenke wrote: > Again, naming scheme should be changed, and should be de-inlined. Done. https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor.h:581: bool preconnect_enabled_; On 2016/05/12 16:30:23, mmenke wrote: > Should mention it's protected by the lock. Or make it atomic, per above > suggestions. Done. https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor.h:614: mutable base::Lock lock_; On 2016/05/12 16:30:23, mmenke wrote: > Should mention what this protects, and could even call it > preconnect_enabled_lock_. Done. https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... File chrome/browser/net/predictor_browsertest.cc (right): https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:150: // The UI thread will wait for exactly |n| items in sockets_. On 2016/05/12 16:30:23, mmenke wrote: > nit: sockets_ Done. https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:152: DCHECK_CURRENTLY_ON(BrowserThread::UI); On 2016/05/12 16:30:24, mmenke wrote: > DCHECK(!run_loop_); Done. https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:161: base::Unretained(this))); On 2016/05/12 16:30:23, mmenke wrote: > Why do we need to do this on the IO thread? Can't we call CheckAccepted() > directly? Could even check if "sockets_.size() == n" right after getting the > lock, and if so, exit without spinning the loop. I basically wanted to unconditionally loop, instead of having two paths with one that waits. I've made it a bit more reasonable without adding a thread hop. https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:171: void CheckAccepted() { On 2016/05/12 16:30:23, mmenke wrote: > ASSERT_TRUE(lock_.AssertAcquired()); Done. https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:171: void CheckAccepted() { On 2016/05/12 16:30:23, mmenke wrote: > Should probably document some of these. Done. https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:173: if (accept_n_ == sockets_.size()) { On 2016/05/12 16:30:23, mmenke wrote: > nit: Early return is generally preferred, unless it's likely to lead to bugs > (i.e. if new code is added after the if, it's likely you'll want it to run in > both cases) Done. https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:212: base::RunLoop* accept_n_loop_; On 2016/05/12 16:30:24, mmenke wrote: > Should make it clearer what's protected by the lock, and what isn't. Maybe put > everything protected by it in a row, with a comment. > ... Done. https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:212: base::RunLoop* accept_n_loop_; On 2016/05/12 16:30:24, mmenke wrote: > Optional: Suggest making this a scoped_ptr and creating it + resetting it in > the new wait function. It seems to be a more common patter (And saves one whole > line of code!) Done. https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:212: base::RunLoop* accept_n_loop_; On 2016/05/12 16:30:24, mmenke wrote: > Optional: Suggest making this a scoped_ptr and creating it + resetting it in > the new wait function. It seems to be a more common patter (And saves one whole > line of code!) I don't like this because we'd need to protect the "reset()" by reacquiring the lock after the runloop is done. Unless I'm misunderstanding you. https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:301: net::NetworkDelegate*)> On 2016/05/12 16:30:25, mmenke wrote: > nit: include base/callback.h and base/bind.h Done. https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:316: net::NetworkDelegate*)> On 2016/05/12 16:30:24, mmenke wrote: > nit: Suggest a typedef at the start of the class. Doesn't really save us any > lines of code, but it's a bit prettier. Arguable... but I'll do it :) https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:345: cross_site_preconnected_ = std::max(cross_site_preconnected_, count); On 2016/05/12 16:30:23, mmenke wrote: > nit: #include <algorithm> Done. https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:349: ADD_FAILURE() << "Preconnected" << original_url On 2016/05/12 16:30:24, mmenke wrote: > nit: "Preconnected" -> "Preconnected " Done. https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:350: << " When should only be preconnecting the source host: " On 2016/05/12 16:30:24, mmenke wrote: > nit: When -> when Done. https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:370: << " When should only be learning the source host: " On 2016/05/12 16:30:25, mmenke wrote: > nit: when Done. https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:400: GURL cross_site_host_; On 2016/05/12 16:30:24, mmenke wrote: > nit: const (x2) Done. https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:406: base::Lock lock_; On 2016/05/12 16:30:25, mmenke wrote: > nit: Weird to have a blank line between cross_site_learned_ and > cross_site_preconnected_, but not one before the lock. Done. https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:406: base::Lock lock_; On 2016/05/12 16:30:24, mmenke wrote: > Should document what this lock protects. Done. https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:470: base::Bind(&CreateEmptyBodyRequestJob)))); On 2016/05/12 16:30:23, mmenke wrote: > Do we actually need to do this? As you said, they're all for the main site, > anyways. Done. https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:474: // two test servers. On 2016/05/12 16:30:23, mmenke wrote: > Should also document what this method actually does (Something along the lines > of "Intercepts all requests to the specified host and returns a response with an > empty body. Needed to prevent requests from actually going to the test server, > to avoid any races related to socket accounting." Done. https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:478: "http", url.host(), On 2016/05/12 16:30:24, mmenke wrote: > "http" -> url.scheme()? Doesn't really matter, but since we're taking a URL, > seems like we should be using it. Done. https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:485: net::URLRequestFilter::GetInstance()->RemoveHostnameHandler("http", On 2016/05/12 16:30:24, mmenke wrote: > "http" -> url.scheme() Done. https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:494: cross_site_test_server()->GetURL("/"))); On 2016/05/12 16:30:24, mmenke wrote: > nit: GetURL("/") -> base_url() Done. https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:502: cross_site_test_server()->GetURL("/"))); On 2016/05/12 16:30:23, mmenke wrote: > base_url() Done. https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:601: base::RunLoop wait_loop; On 2016/05/12 16:30:24, mmenke wrote: > nit: run_loop? Just for consistency Done. https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:642: std::unique_ptr<ConnectionListener> cross_site_connection_listener_; On 2016/05/12 16:30:23, mmenke wrote: > include <memory> Done. https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:651: scoped_refptr<base::SingleThreadTaskRunner> task_runner_; On 2016/05/12 16:30:24, mmenke wrote: > include base/memory/ref_counted.h Done. https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:659: connection_listener_->WaitForAcceptedConnectionsOnUI(1u); On 2016/05/12 16:30:24, mmenke wrote: > Hrm...Should we make WaitForAcceptedConnectionsOnUI have an EXPECT failure if we > see too many sockets? As long as we make sure real cross-site requests don't > make it to the socket pools, think there are no concerns about real requests > racing with preconnects. Yeah, good idea. That will force tests to be careful about killing sockets, etc. https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:704: // and disable preconnects to the cross site test server. This allows the test On 2016/05/12 16:30:24, mmenke wrote: > We don't disable preconnects any more. Done. https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:717: InstallPredictorObserver(); On 2016/05/12 16:30:24, mmenke wrote: > Should we call these two in the test fixture itself? Don't think they hurt for > tests that don't use them, and we call them on a number of tests. The observer EXPECTS certain URLs, so the one test we use localhost fails. We can special case that test though. Per your comment below, I'm fine moving these to the setup. https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:720: StopInterceptingCrossSiteOnUI(); On 2016/05/12 16:30:24, mmenke wrote: > Do we need to stop intercepting? We're just interested in preconnects, and > these could confuse things (WaitForAcceptedConnectionsOnUI(4u) could succeed, > even if we don't preconnect - admittedly, we also check with the observer, but > I'm paranoid). Actually, you're right. That simplifies things :) https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:743: // embedded test servers have the same host_piece, an extra preconnect is On 2016/05/12 16:30:23, mmenke wrote: > host_piece -> host name? Done. https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:772: // The redirect causes two preconnects for the initial navigation. On 2016/05/12 16:30:24, mmenke wrote: > This isn't a redirect - it's a (subframe) navigation. Done. https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:793: IN_PROC_BROWSER_TEST_F(PredictorBrowserTest, SubframeInitiatesPreconnects) { On 2016/05/12 16:30:24, mmenke wrote: > I'm not following what this test does. If we go to > <same_site>/predictor/predictor_cross_site, and then to a data URL that contains > <same_site>/title1.html, why do we preconnect to cross site at all? This test is essentially the same as CrossSiteSimplePredictionAfterOneNavigation, except the second navigation is the same site in an iframe instead of a main frame. Because the first navigation finds the relationship between same site and cross site, when the same-site shows up again (even in a subframe), we navigate. I've updated the comment to be a little clearer.
ttps://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... File chrome/browser/net/predictor_browsertest.cc (right): https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:793: IN_PROC_BROWSER_TEST_F(PredictorBrowserTest, SubframeInitiatesPreconnects) { On 2016/05/12 20:17:05, csharrison wrote: > On 2016/05/12 16:30:24, mmenke wrote: > > I'm not following what this test does. If we go to > > <same_site>/predictor/predictor_cross_site, and then to a data URL that > contains > > <same_site>/title1.html, why do we preconnect to cross site at all? > > This test is essentially the same as > CrossSiteSimplePredictionAfterOneNavigation, except the second navigation is the > same site in an iframe instead of a main frame. > > Because the first navigation finds the relationship between same site and cross > site, when the same-site shows up again (even in a subframe), we navigate. > > I've updated the comment to be a little clearer. Ahh, I didn't notice that the NavigateToURL call in the previous test used embedded_test_server()...So...erm, does the predictor only learn URLs on a per-domain basis, rather than a per-URL one? I hadn't realized that (Or maybe I had, but then I forgot).
On 2016/05/12 20:33:39, mmenke wrote: > ttps://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... > File chrome/browser/net/predictor_browsertest.cc (right): > > https://codereview.chromium.org/1881463003/diff/200001/chrome/browser/net/pre... > chrome/browser/net/predictor_browsertest.cc:793: > IN_PROC_BROWSER_TEST_F(PredictorBrowserTest, SubframeInitiatesPreconnects) { > On 2016/05/12 20:17:05, csharrison wrote: > > On 2016/05/12 16:30:24, mmenke wrote: > > > I'm not following what this test does. If we go to > > > <same_site>/predictor/predictor_cross_site, and then to a data URL that > > contains > > > <same_site>/title1.html, why do we preconnect to cross site at all? > > > > This test is essentially the same as > > CrossSiteSimplePredictionAfterOneNavigation, except the second navigation is > the > > same site in an iframe instead of a main frame. > > > > Because the first navigation finds the relationship between same site and > cross > > site, when the same-site shows up again (even in a subframe), we navigate. > > > > I've updated the comment to be a little clearer. > > Ahh, I didn't notice that the NavigateToURL call in the previous test used > embedded_test_server()...So...erm, does the predictor only learn URLs on a > per-domain basis, rather than a per-URL one? I hadn't realized that (Or maybe I > had, but then I forgot). Yeah, it's domain rather than per url. That's why we always navigation to /title1 to generate the preconnect.
LGTM https://codereview.chromium.org/1881463003/diff/220001/chrome/browser/net/pre... File chrome/browser/net/predictor_browsertest.cc (right): https://codereview.chromium.org/1881463003/diff/220001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:8: #include <memory> nit: blank line between C and C++ headers. https://codereview.chromium.org/1881463003/diff/220001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:157: void WaitForAcceptedConnectionsOnUI(size_t n) { Suggest replacing "n" with |connections| or |num_connections| everywhere - "n" seems to violate the style guide's rules about naming. Same with accept_n_ and accept_n_loop_ https://codereview.chromium.org/1881463003/diff/220001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:169: // Will have quit if CheckAccepted finds the correct number of accepted This sounds a bit awkward. Maybe "Will have quit if" -> "Will quit when"? https://codereview.chromium.org/1881463003/diff/220001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:221: // IO and UI thread. Members declared after the lock are protected by it. All the above members? I think you mean all the below members, except task_runner_. https://codereview.chromium.org/1881463003/diff/220001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:669: connection_listener_->WaitForAcceptedConnectionsOnUI(1u); On 2016/05/12 20:17:07, csharrison wrote: > On 2016/05/12 16:30:24, mmenke wrote: > > Hrm...Should we make WaitForAcceptedConnectionsOnUI have an EXPECT failure if > we > > see too many sockets? As long as we make sure real cross-site requests don't > > make it to the socket pools, think there are no concerns about real requests > > racing with preconnects. > > Yeah, good idea. That will force tests to be careful about killing sockets, etc. I don't think you did this (See quoted text)? https://codereview.chromium.org/1881463003/diff/220001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:744: StopInterceptingCrossSiteOnUI(); Why is this needed? Think it needs a comment. Or update the test description to say when intercepting is a problem here. Are you just disabling it because it's not needed for this particular test? https://codereview.chromium.org/1881463003/diff/220001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:866: EXPECT_EQ(0u, cross_site_connection_listener_->GetAcceptedSocketCount()); Do we even need both this and the above test? Seems like you can just slap in a check for SameSitePreconnected above the NavigateToURL, and the above test is pretty much a subset of this one.
Thanks! Let me know specifically regarding the WaitForAcceptedConnectionsOnUI change. https://codereview.chromium.org/1881463003/diff/220001/chrome/browser/net/pre... File chrome/browser/net/predictor_browsertest.cc (right): https://codereview.chromium.org/1881463003/diff/220001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:8: #include <memory> On 2016/05/13 at 20:57:46, mmenke wrote: > nit: blank line between C and C++ headers. Done. https://codereview.chromium.org/1881463003/diff/220001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:157: void WaitForAcceptedConnectionsOnUI(size_t n) { On 2016/05/13 at 20:57:46, mmenke wrote: > Suggest replacing "n" with |connections| or |num_connections| everywhere - "n" seems to violate the style guide's rules about naming. Same with accept_n_ and accept_n_loop_ Done. https://codereview.chromium.org/1881463003/diff/220001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:169: // Will have quit if CheckAccepted finds the correct number of accepted On 2016/05/13 at 20:57:46, mmenke wrote: > This sounds a bit awkward. Maybe "Will have quit if" -> "Will quit when"? The wording was awkward because I guess I was trying to explain that the run loop is allowed to quit synchronously (before it is even Run()), i.e. there is no race condition. I'll reword. https://codereview.chromium.org/1881463003/diff/220001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:221: // IO and UI thread. Members declared after the lock are protected by it. On 2016/05/13 at 20:57:46, mmenke wrote: > All the above members? I think you mean all the below members, except task_runner_. Done. https://codereview.chromium.org/1881463003/diff/220001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:669: connection_listener_->WaitForAcceptedConnectionsOnUI(1u); On 2016/05/13 at 20:57:46, mmenke wrote: > On 2016/05/12 20:17:07, csharrison wrote: > > On 2016/05/12 16:30:24, mmenke wrote: > > > Hrm...Should we make WaitForAcceptedConnectionsOnUI have an EXPECT failure if > > we > > > see too many sockets? As long as we make sure real cross-site requests don't > > > make it to the socket pools, think there are no concerns about real requests > > > racing with preconnects. > > > > Yeah, good idea. That will force tests to be careful about killing sockets, etc. > > I don't think you did this (See quoted text)? I added "DCHECK_GE(num_connections, sockets_.size())" after we acquire the lock in WaitForAcceptedConnectionsOnUI. Did I misunderstand you? https://codereview.chromium.org/1881463003/diff/220001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:744: StopInterceptingCrossSiteOnUI(); On 2016/05/13 at 20:57:46, mmenke wrote: > Why is this needed? Think it needs a comment. Or update the test description to say when intercepting is a problem here. Are you just disabling it because it's not needed for this particular test? This is just a sanity check that the interceptor doesn't mess with the predictor logic. We lose the ability to introspect at the socket layer, but we can still verify that the predictor itself is doing it's job by inspecting at the observer layer. Updated the comment to be clearer here. https://codereview.chromium.org/1881463003/diff/220001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:866: EXPECT_EQ(0u, cross_site_connection_listener_->GetAcceptedSocketCount()); On 2016/05/13 at 20:57:46, mmenke wrote: > Do we even need both this and the above test? Seems like you can just slap in a check for SameSitePreconnected above the NavigateToURL, and the above test is pretty much a subset of this one. Right. Done.
https://codereview.chromium.org/1881463003/diff/220001/chrome/browser/net/pre... File chrome/browser/net/predictor_browsertest.cc (right): https://codereview.chromium.org/1881463003/diff/220001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:669: connection_listener_->WaitForAcceptedConnectionsOnUI(1u); On 2016/05/13 21:27:53, csharrison wrote: > On 2016/05/13 at 20:57:46, mmenke wrote: > > On 2016/05/12 20:17:07, csharrison wrote: > > > On 2016/05/12 16:30:24, mmenke wrote: > > > > Hrm...Should we make WaitForAcceptedConnectionsOnUI have an EXPECT failure > if > > > we > > > > see too many sockets? As long as we make sure real cross-site requests > don't > > > > make it to the socket pools, think there are no concerns about real > requests > > > > racing with preconnects. > > > > > > Yeah, good idea. That will force tests to be careful about killing sockets, > etc. > > > > I don't think you did this (See quoted text)? > > I added "DCHECK_GE(num_connections, sockets_.size())" after we acquire the lock > in WaitForAcceptedConnectionsOnUI. Did I misunderstand you? Yes. I meant *after* spinning the RunLoop (I admit, I missed the DCHECK - was looking for an EXPECT)
On 2016/05/13 21:30:40, mmenke wrote: > https://codereview.chromium.org/1881463003/diff/220001/chrome/browser/net/pre... > File chrome/browser/net/predictor_browsertest.cc (right): > > https://codereview.chromium.org/1881463003/diff/220001/chrome/browser/net/pre... > chrome/browser/net/predictor_browsertest.cc:669: > connection_listener_->WaitForAcceptedConnectionsOnUI(1u); > On 2016/05/13 21:27:53, csharrison wrote: > > On 2016/05/13 at 20:57:46, mmenke wrote: > > > On 2016/05/12 20:17:07, csharrison wrote: > > > > On 2016/05/12 16:30:24, mmenke wrote: > > > > > Hrm...Should we make WaitForAcceptedConnectionsOnUI have an EXPECT > failure > > if > > > > we > > > > > see too many sockets? As long as we make sure real cross-site requests > > don't > > > > > make it to the socket pools, think there are no concerns about real > > requests > > > > > racing with preconnects. > > > > > > > > Yeah, good idea. That will force tests to be careful about killing > sockets, > > etc. > > > > > > I don't think you did this (See quoted text)? > > > > I added "DCHECK_GE(num_connections, sockets_.size())" after we acquire the > lock > > in WaitForAcceptedConnectionsOnUI. Did I misunderstand you? > > Yes. I meant *after* spinning the RunLoop (I admit, I missed the DCHECK - was > looking for an EXPECT) Worth noting we'd have to grab the mutex again to check it
Thanks for the clarification https://codereview.chromium.org/1881463003/diff/220001/chrome/browser/net/pre... File chrome/browser/net/predictor_browsertest.cc (right): https://codereview.chromium.org/1881463003/diff/220001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:669: connection_listener_->WaitForAcceptedConnectionsOnUI(1u); On 2016/05/13 at 21:30:40, mmenke wrote: > On 2016/05/13 21:27:53, csharrison wrote: > > On 2016/05/13 at 20:57:46, mmenke wrote: > > > On 2016/05/12 20:17:07, csharrison wrote: > > > > On 2016/05/12 16:30:24, mmenke wrote: > > > > > Hrm...Should we make WaitForAcceptedConnectionsOnUI have an EXPECT failure > > if > > > > we > > > > > see too many sockets? As long as we make sure real cross-site requests > > don't > > > > > make it to the socket pools, think there are no concerns about real > > requests > > > > > racing with preconnects. > > > > > > > > Yeah, good idea. That will force tests to be careful about killing sockets, > > etc. > > > > > > I don't think you did this (See quoted text)? > > > > I added "DCHECK_GE(num_connections, sockets_.size())" after we acquire the lock > > in WaitForAcceptedConnectionsOnUI. Did I misunderstand you? > > Yes. I meant *after* spinning the RunLoop (I admit, I missed the DCHECK - was looking for an EXPECT) Ah okay that makes sense. I think we can just EXPECT_EQ here.
On 2016/05/13 21:40:39, csharrison wrote: > Thanks for the clarification > > https://codereview.chromium.org/1881463003/diff/220001/chrome/browser/net/pre... > File chrome/browser/net/predictor_browsertest.cc (right): > > https://codereview.chromium.org/1881463003/diff/220001/chrome/browser/net/pre... > chrome/browser/net/predictor_browsertest.cc:669: > connection_listener_->WaitForAcceptedConnectionsOnUI(1u); > On 2016/05/13 at 21:30:40, mmenke wrote: > > On 2016/05/13 21:27:53, csharrison wrote: > > > On 2016/05/13 at 20:57:46, mmenke wrote: > > > > On 2016/05/12 20:17:07, csharrison wrote: > > > > > On 2016/05/12 16:30:24, mmenke wrote: > > > > > > Hrm...Should we make WaitForAcceptedConnectionsOnUI have an EXPECT > failure > > > if > > > > > we > > > > > > see too many sockets? As long as we make sure real cross-site > requests > > > don't > > > > > > make it to the socket pools, think there are no concerns about real > > > requests > > > > > > racing with preconnects. > > > > > > > > > > Yeah, good idea. That will force tests to be careful about killing > sockets, > > > etc. > > > > > > > > I don't think you did this (See quoted text)? > > > > > > I added "DCHECK_GE(num_connections, sockets_.size())" after we acquire the > lock > > > in WaitForAcceptedConnectionsOnUI. Did I misunderstand you? > > > > Yes. I meant *after* spinning the RunLoop (I admit, I missed the DCHECK - was > looking for an EXPECT) > > Ah okay that makes sense. I think we can just EXPECT_EQ here. Still LGTM
On 2016/05/13 at 21:42:12, mmenke wrote: > On 2016/05/13 21:40:39, csharrison wrote: > > Thanks for the clarification > > > > https://codereview.chromium.org/1881463003/diff/220001/chrome/browser/net/pre... > > File chrome/browser/net/predictor_browsertest.cc (right): > > > > https://codereview.chromium.org/1881463003/diff/220001/chrome/browser/net/pre... > > chrome/browser/net/predictor_browsertest.cc:669: > > connection_listener_->WaitForAcceptedConnectionsOnUI(1u); > > On 2016/05/13 at 21:30:40, mmenke wrote: > > > On 2016/05/13 21:27:53, csharrison wrote: > > > > On 2016/05/13 at 20:57:46, mmenke wrote: > > > > > On 2016/05/12 20:17:07, csharrison wrote: > > > > > > On 2016/05/12 16:30:24, mmenke wrote: > > > > > > > Hrm...Should we make WaitForAcceptedConnectionsOnUI have an EXPECT > > failure > > > > if > > > > > > we > > > > > > > see too many sockets? As long as we make sure real cross-site > > requests > > > > don't > > > > > > > make it to the socket pools, think there are no concerns about real > > > > requests > > > > > > > racing with preconnects. > > > > > > > > > > > > Yeah, good idea. That will force tests to be careful about killing > > sockets, > > > > etc. > > > > > > > > > > I don't think you did this (See quoted text)? > > > > > > > > I added "DCHECK_GE(num_connections, sockets_.size())" after we acquire the > > lock > > > > in WaitForAcceptedConnectionsOnUI. Did I misunderstand you? > > > > > > Yes. I meant *after* spinning the RunLoop (I admit, I missed the DCHECK - was > > looking for an EXPECT) > > > > Ah okay that makes sense. I think we can just EXPECT_EQ here. > > Still LGTM W00t. Thank you for your patience with this patch.
The CQ bit was checked by csharrison@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1881463003/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1881463003/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-simulator on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by csharrison@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1881463003/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1881463003/260001
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Add a browsertest suite for net predictor This patch adds end to end tests for the predictor. These tests span multiple navigations, asserting that host relationships are learned, and that subsequent navigations will preconnect accurately. BUG=602396 ========== to ========== Add a browsertest suite for net predictor This patch adds end to end tests for the predictor. These tests span multiple navigations, asserting that host relationships are learned, and that subsequent navigations will preconnect accurately. BUG=602396 Committed: https://crrev.com/a7b1611d80ee455b77a8144a4afcb2248b854a55 Cr-Commit-Position: refs/heads/master@{#393682} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/a7b1611d80ee455b77a8144a4afcb2248b854a55 Cr-Commit-Position: refs/heads/master@{#393682} |
