Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2449323005:
predictors: Basic browsertest checks predictor learning. (Closed)
Descriptionpredictors: Basic browsertest checks predictor learning.
First stage for implementing ResourcePrefetchPredictor browsertests is to check
that the prefetch database is really filled after some navigations in browser.
Browsertest class provides convenient way to declare expected resources in the
test and then checks expectations internally that allows to write readable
tests.
BUG=650253
Committed: https://crrev.com/e636e3bcc172f220dfadfd2121d238c9908185bb
Cr-Commit-Position: refs/heads/master@{#429966}
Patch Set 1 #
Total comments: 16
Patch Set 2 : Ignore PlzNavigate tests + hardcode net priorities. #Patch Set 3 : rebase #Patch Set 4 : Naming alignement + nits. #Patch Set 5 : Reuse URLRequestSummary for ResourceSummary + Defaults. #
Total comments: 4
Patch Set 6 : Revert changes about defaults. #
Total comments: 38
Patch Set 7 : Various fixes. #Patch Set 8 : Break the wall of usings. #Patch Set 9 : Get rid of mime type. #
Messages
Total messages: 46 (22 generated)
alexilin@chromium.org changed reviewers: + lizeb@chromium.org, pasko@chromium.org
I was studying and playing with browsertests for ResourcePrefetchPredictor alone for a long time and now I want you to check this out. I have several ideas what to test else (for example, iframes, loading resources from script, etc.) and your suggestions is welcomed also.
The CQ bit was checked by lizeb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! The overall structure seems good, I still have to take a closer look though. https://codereview.chromium.org/2449323005/diff/1/chrome/browser/predictors/r... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2449323005/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:58: net::RequestPriority TypeToPriority(content::ResourceType type) { I'm not really sure about the mapping of resource type to priorities. Where do you take this from?
https://codereview.chromium.org/2449323005/diff/1/chrome/browser/predictors/r... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2449323005/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:58: net::RequestPriority TypeToPriority(content::ResourceType type) { On 2016/11/02 09:56:42, Benoit L wrote: > I'm not really sure about the mapping of resource type to priorities. > Where do you take this from? I'm not sure too. Probably it would be better not to check priority at all or check only when we're expecting prefetch (then it should be net::LOW priority). I've found mapping in WebKit. https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/fetch/Res... There is a long way to transform blink::ResourceLoadPriority at first to blink::WebURLRequest::Priority and then to net::RequestPriority.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2449323005/diff/1/chrome/browser/predictors/r... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2449323005/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:58: net::RequestPriority TypeToPriority(content::ResourceType type) { On 2016/11/02 10:09:14, alexilin wrote: > On 2016/11/02 09:56:42, Benoit L wrote: > > I'm not really sure about the mapping of resource type to priorities. > > Where do you take this from? > > I'm not sure too. Probably it would be better not to check priority at all or > check only when we're expecting prefetch (then it should be net::LOW priority). > I've found mapping in WebKit. > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/fetch/Res... > There is a long way to transform blink::ResourceLoadPriority at first to > blink::WebURLRequest::Priority and then to net::RequestPriority. The actual priority logic is indeed more complex. For instance for scripts in <head>, it's going to be different. We can probably hardcode the priorities for the various resources in the test files, with a comment explaining why the value is set this way. It makes it easier to update whenever the priorities heuristics change as well. wdyt?
https://codereview.chromium.org/2449323005/diff/1/chrome/browser/predictors/r... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2449323005/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:58: net::RequestPriority TypeToPriority(content::ResourceType type) { On 2016/11/02 12:23:12, Benoit L wrote: > On 2016/11/02 10:09:14, alexilin wrote: > > On 2016/11/02 09:56:42, Benoit L wrote: > > > I'm not really sure about the mapping of resource type to priorities. > > > Where do you take this from? > > > > I'm not sure too. Probably it would be better not to check priority at all or > > check only when we're expecting prefetch (then it should be net::LOW > priority). > > I've found mapping in WebKit. > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/fetch/Res... > > There is a long way to transform blink::ResourceLoadPriority at first to > > blink::WebURLRequest::Priority and then to net::RequestPriority. > > The actual priority logic is indeed more complex. For instance for scripts in > <head>, it's going to be different. > We can probably hardcode the priorities for the various resources in the test > files, with a comment explaining why the value is set this way. > > It makes it easier to update whenever the priorities heuristics change as well. > wdyt? I'm not sure that I understood you correctly because the priorities are already hardcoded. I suppose you want to define a net priority for each resource individually. Anyway, PTAL at new patchset. Could be there any problems with dynamically assigned priorities that will make tests flaky? I mean, can priorities change from run to run?
The CQ bit was checked by alexilin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2449323005/diff/1/chrome/browser/predictors/r... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2449323005/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:38: struct ResourceSummary { This largely duplicates ResourcePrefetchPredictor::URLRequestSummary. Is the duplication necessary? If so, can you align more closely the two classes? (same naming, for instance). If not, please unify a bit. https://codereview.chromium.org/2449323005/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:45: version(0) {} Doesn't seem to be used in this CL beyond the Etag (which can be set another way). I'm fine with keeping this in this CL if it's going to be used shortly. Otherwise please remove it. https://codereview.chromium.org/2449323005/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:54: bool no_store; tiny nit: I quite like "is_X" for boolean variables. https://codereview.chromium.org/2449323005/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:80: message_loop_runner_(new content::MessageLoopRunner), nit: content::MessageLoopRunner(). That's safer. https://codereview.chromium.org/2449323005/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:184: std::unique_ptr<BasicHttpResponse> http_response(new BasicHttpResponse); nit: () Or better: MakeUnique https://codereview.chromium.org/2449323005/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:189: http_response->AddCustomHeader("Cache-Control", "no-store"); It's possible to call this several times, good. Then no_store and has_validators are not mutually exclusive.
The CQ bit was checked by alexilin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2449323005/diff/1/chrome/browser/predictors/r... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2449323005/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:38: struct ResourceSummary { On 2016/11/02 14:03:18, Benoit L wrote: > This largely duplicates ResourcePrefetchPredictor::URLRequestSummary. > > Is the duplication necessary? > If so, can you align more closely the two classes? (same naming, for instance). > If not, please unify a bit. These structures have mismatched fields. ResourceSummary: content, no_store, version URLRequestSummary: navigation_id, redirect_url I think the same naming will be best option. https://codereview.chromium.org/2449323005/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:45: version(0) {} On 2016/11/02 14:03:18, Benoit L wrote: > Doesn't seem to be used in this CL beyond the Etag True > (which can be set another way). Suggestions? I want to be able to invalidate resource in test. > I'm fine with keeping this in this CL if it's going to be used shortly. > Otherwise please remove it. Well, I don't use all flags in tests yet, but I'm going to write more tests. https://codereview.chromium.org/2449323005/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:54: bool no_store; On 2016/11/02 14:03:18, Benoit L wrote: > tiny nit: I quite like "is_X" for boolean variables. Done. https://codereview.chromium.org/2449323005/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:80: message_loop_runner_(new content::MessageLoopRunner), On 2016/11/02 14:03:18, Benoit L wrote: > nit: content::MessageLoopRunner(). > That's safer. Done. https://codereview.chromium.org/2449323005/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:184: std::unique_ptr<BasicHttpResponse> http_response(new BasicHttpResponse); On 2016/11/02 14:03:18, Benoit L wrote: > nit: () > > Or better: MakeUnique Done. https://codereview.chromium.org/2449323005/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:189: http_response->AddCustomHeader("Cache-Control", "no-store"); On 2016/11/02 14:03:18, Benoit L wrote: > It's possible to call this several times, good. > Then no_store and has_validators are not mutually exclusive. Is it good or bad? I had DCHECK checked that no_store and {has_validators, always_revalidate} are mutually exclusive but I deleted it. Mix of these headers isn't forbidden in RFC I guess. So we can check the behavior of the browser in this case.
- Reuse URLRequestSummary in ResourceSummary struct - Defaults for net::ResourcePriority
Thanks! Sorry, last minute changes, but after that, I think it's all good. https://codereview.chromium.org/2449323005/diff/80001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2449323005/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:41: // Corresponds to blink::typeToPrioriity function. typo: typeToPriority. https://codereview.chromium.org/2449323005/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:42: net::RequestPriority GetDefaultPriorityForType(content::ResourceType type) { Per offline discussion, I think it's simpler to hardcode the priority as was done in the previous patch. Sorry for the misunderstanding.
Revert hardcoding changes. https://codereview.chromium.org/2449323005/diff/80001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2449323005/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:41: // Corresponds to blink::typeToPrioriity function. On 2016/11/02 17:35:47, Benoit L wrote: > typo: typeToPriority. Done. https://codereview.chromium.org/2449323005/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:42: net::RequestPriority GetDefaultPriorityForType(content::ResourceType type) { On 2016/11/02 17:35:47, Benoit L wrote: > Per offline discussion, I think it's simpler to hardcode the priority as was > done in the previous patch. > > Sorry for the misunderstanding. Never mind. Done.
The CQ bit was checked by alexilin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm, thank you.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
toplevel comments: 1. the general approach is good (modulo custom request handler, see below) 2. I am worried about re-prioritization in resource scheduler that may make tests flaky. It probably does not affect the simple test we have in this change, but may be a problem on more complex tests. Have you guys thought of listening network priority changes (URLRequestInterceptor may be of help, not sure) and ASSERT-ing expected behavior? This could allow resource scheduler code to require changes on the side of predictors/ (where we are owners), which would be a neat discovery mechanism for cases where our priority-related assumptions got violated. Not for this change, of course. https://codereview.chromium.org/2449323005/diff/100001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://codereview.chromium.org/2449323005/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.h:166: friend class ResourcePrefetchPredictorBrowserTest; not necessary for this change, please add when it is necessary, at which point we will discuss how too many friends can be avoided https://codereview.chromium.org/2449323005/diff/100001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2449323005/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:47: // Helper class to track and allow waiting for ResourcePrefetchPredictor events. not only waiting, but also verifying that a PageRequestSummary is learned as expected. That would be a valuable additions to the comment https://codereview.chromium.org/2449323005/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:55: message_loop_runner_(new content::MessageLoopRunner()), using content::MessageLoopRunner without calling QuitClosure() is confusing because base::RunLoop is sufficient to solve the problem. Please change to base::RunLoop. https://codereview.chromium.org/2449323005/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:59: ~ResourcePrefetchPredictorTestObserver() override {} is this necessary? does not add to readability https://codereview.chromium.org/2449323005/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:61: // ResourcePrefetchPredictor::Observer // TestObserver implementation. https://codereview.chromium.org/2449323005/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:124: bool has_validators = true, the style guide "somewhat discourages" function overloading and default arguments, see: https://google.github.io/styleguide/cppguide.html#Function_Overloading Once the amount of arguments reaches ~7, maintaining the right order of parameters becomes very error-prone: there are no compiler-enforced checks to verify that all these booleans/ints/pointers are passed the way it was intended. Since these default arguments are not used yet, let's omit them at the moment, this would be simpler. Later, makes sense to consider having a function to register a ResourceSummary an return it. If an individual test cares about some particular parts of the ResourceSummary, they can overwrite that particular default value of that particular member. Another option is o have multiple functions under different names to do a similar thing with different set of arguments (just like the style guide suggests). Throwing in lots of default args is not a good option https://codereview.chromium.org/2449323005/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:152: std::unique_ptr<HttpResponse> HandleRequest(const HttpRequest& request) { A few concerns with manually handling requests in this class: 1. it is a bit manual :) The test server can load files and serve custom response headers, see for example: https://codesearch.chromium.org/chromium/src/chrome/test/data/prerender/image... 2. sharing the state of the test fixture (resources_, predictor_, what-may-come-later) with the test server makes it easy to overlook problems that happen in reality by intercepting/rewriting all the request-response meat. Proposal: rely on default handlers as much as possible. That would be a path to the least surprise. If we need something fancy we can still have handlers for those and return nullptr if the path does not belong to them, the test server will just try the next handlers and then the default handlers, see: https://codesearch.chromium.org/chromium/src/net/test/embedded_test_server/em... This proposal is, of course, a tradeoff, for example, it may lead to creating more files and more custom headers, happy to discuss. https://codereview.chromium.org/2449323005/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:183: std::map<std::string, size_t> visit_count_; This value gets updated, but the test only observes the value 1. It is not obvious to me how other tests will handle multiple navigations and whether it would add much value there to check for visit count. It is preferable to introduce things on as-needed basis, otherwise we risk forgetting to remove dead code it the future reviews find other ways to perform the same checks. https://codereview.chromium.org/2449323005/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:186: IN_PROC_BROWSER_TEST_F(ResourcePrefetchPredictorBrowserTest, ) { name of the test missing, suggest: ResourcePrefetchPredictorBrowserTest.Simple https://codereview.chromium.org/2449323005/diff/100001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_test_util.h (right): https://codereview.chromium.org/2449323005/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_test_util.h:43: const std::vector<ResourcePrefetchPredictor::URLRequestSummary>& Add #include <vector> for vector<> [build/include_what_you_use] [4]
Top level response to pasko@: What kind of re-prioritization could we encounter? Can prority be accurately determined at compile-time? I don't have answers for these questions yet. For current test it looks correct. What did you mean by "ASSERT-ing expected behavior"? We have resource scheduler not only on predictors/ side. https://codereview.chromium.org/2449323005/diff/100001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://codereview.chromium.org/2449323005/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.h:166: friend class ResourcePrefetchPredictorBrowserTest; On 2016/11/02 19:37:01, pasko wrote: > not necessary for this change, please add when it is necessary, at which point > we will discuss how too many friends can be avoided Thanks for the pointing, we don't need it anymore since we check all resources in the observer. (I used private caches for checking earlier). Remove it for now, we'll try to avoid to add this friendship in further CLs. https://codereview.chromium.org/2449323005/diff/100001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2449323005/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:47: // Helper class to track and allow waiting for ResourcePrefetchPredictor events. On 2016/11/02 19:37:02, pasko wrote: > not only waiting, but also verifying that a PageRequestSummary is learned as > expected. That would be a valuable additions to the comment That's why I don't like comments: they become stale too fast. Done. https://codereview.chromium.org/2449323005/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:55: message_loop_runner_(new content::MessageLoopRunner()), On 2016/11/02 19:37:01, pasko wrote: > using content::MessageLoopRunner without calling QuitClosure() is confusing > because base::RunLoop is sufficient to solve the problem. Please change to > base::RunLoop. MessageLoopRunner does additional work internally in comparison with base::RunLoop. Don't we need it? I've thought that using MessageLoopRunner is common pattern for this kind of test (even without calling QuitClosure). Btw, base::RunLoop contains QuitClosure() method too, so this is not an argument. I'm not very familiar with this message running infrastructure yet, so it'd be great if you explain me details when we should use MessageLoopRunner instead of RunLoop. https://codereview.chromium.org/2449323005/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:59: ~ResourcePrefetchPredictorTestObserver() override {} On 2016/11/02 19:37:02, pasko wrote: > is this necessary? does not add to readability Done. https://codereview.chromium.org/2449323005/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:61: // ResourcePrefetchPredictor::Observer On 2016/11/02 19:37:02, pasko wrote: > // TestObserver implementation. Done. Btw, I've seen alike comments with the word "implementation" and without, e.g. "//content::WebContentsObserver:". https://codereview.chromium.org/2449323005/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:124: bool has_validators = true, On 2016/11/02 19:37:01, pasko wrote: > the style guide "somewhat discourages" function overloading and default > arguments, see: > > https://google.github.io/styleguide/cppguide.html#Function_Overloading > > Once the amount of arguments reaches ~7, maintaining the right order of > parameters becomes very error-prone: there are no compiler-enforced checks to > verify that all these booleans/ints/pointers are passed the way it was intended. > > Since these default arguments are not used yet, let's omit them at the moment, > this would be simpler. > > Later, makes sense to consider having a function to register a ResourceSummary > an return it. If an individual test cares about some particular parts of the > ResourceSummary, they can overwrite that particular default value of that > particular member. > > Another option is o have multiple functions under different names to do a > similar thing with different set of arguments (just like the style guide > suggests). > > Throwing in lots of default args is not a good option Many of the tests for ResourcePrefetchPredictors contains functions like this. For example, look at Create* functions. I agree that they don't looks very good, but on other hand these are for tests and objects could be constructed directly so it's just for convenience. Anyway, I think that the idea of returning ResourceSummary is good in this case. https://codereview.chromium.org/2449323005/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:152: std::unique_ptr<HttpResponse> HandleRequest(const HttpRequest& request) { On 2016/11/02 19:37:02, pasko wrote: > A few concerns with manually handling requests in this class: > > 1. it is a bit manual :) The test server can load files and serve custom > response headers, see for example: > https://codesearch.chromium.org/chromium/src/chrome/test/data/prerender/image... > > 2. sharing the state of the test fixture (resources_, predictor_, > what-may-come-later) with the test server makes it easy to overlook problems > that happen in reality by intercepting/rewriting all the request-response meat. > > Proposal: rely on default handlers as much as possible. That would be a path to > the least surprise. > > If we need something fancy we can still have handlers for those and return > nullptr if the path does not belong to them, the test server will just try the > next handlers and then the default handlers, see: > > https://codesearch.chromium.org/chromium/src/net/test/embedded_test_server/em... > > This proposal is, of course, a tradeoff, for example, it may lead to creating > more files and more custom headers, happy to discuss. I don't see anything special or wrong in manual handling. Default request handler use the same functions for setting headers and other stuff as I do. https://cs.chromium.org/chromium/src/net/test/embedded_test_server/request_ha... Creation of special header file for each subresource looks more sophisticated than just add headers in accordance with function parameters in code. For example, I could iterate over all possible combinations of parameters programmatically with this method. Default handler has also the way to pass custom headers via query parameter but it also doesn't look nice. From other side, I'm not sure that we have to check predictor behavior depending on cache-control directives but I find that it'd be nice. Embedded HTTP server provides a neat and simple way to customize its behavior. I don't see anything surprising in custom request handler. I know that it's possible to return nullptr if I don't want to handle some request and I already use it. https://codereview.chromium.org/2449323005/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:183: std::map<std::string, size_t> visit_count_; On 2016/11/02 19:37:01, pasko wrote: > This value gets updated, but the test only observes the value 1. It is not > obvious to me how other tests will handle multiple navigations and whether it > would add much value there to check for visit count. > > It is preferable to introduce things on as-needed basis, otherwise we risk > forgetting to remove dead code it the future reviews find other ways to perform > the same checks. In future tests we definitely will navigate to the same url more than once to check that prefetch really works. And in this way we also could check that prefetch doesn't affect history namely that we count a visit exactly once regardless of whether or not the prefetch was. https://codereview.chromium.org/2449323005/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:186: IN_PROC_BROWSER_TEST_F(ResourcePrefetchPredictorBrowserTest, ) { On 2016/11/02 19:37:01, pasko wrote: > name of the test missing, suggest: ResourcePrefetchPredictorBrowserTest.Simple Wow, how does it even work? Yeah, I see GTest uses this name only as parameter of ## macro operator (and #). Done. https://codereview.chromium.org/2449323005/diff/100001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_test_util.h (right): https://codereview.chromium.org/2449323005/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_test_util.h:43: const std::vector<ResourcePrefetchPredictor::URLRequestSummary>& On 2016/11/02 19:37:02, pasko wrote: > Add #include <vector> for vector<> [build/include_what_you_use] [4] Done.
please upload your latest changes
Sorry! Changes are updated now.
https://codereview.chromium.org/2449323005/diff/100001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2449323005/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:36: using net::test_server::BasicHttpResponse; the walls of 'using' statements are not adding too much convenience, especially the ones that are used only once (like the all the net::test_server::* here), but add a theoretical risk of causing name conflicts in future changes. Please minimize the amount of type aliases to only leave those that are saving a lot of line space. https://codereview.chromium.org/2449323005/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:55: message_loop_runner_(new content::MessageLoopRunner()), On 2016/11/03 13:49:09, alexilin wrote: > On 2016/11/02 19:37:01, pasko wrote: > > using content::MessageLoopRunner without calling QuitClosure() is confusing > > because base::RunLoop is sufficient to solve the problem. Please change to > > base::RunLoop. > > MessageLoopRunner does additional work internally in comparison with > base::RunLoop. Don't we need it? > I've thought that using MessageLoopRunner is common pattern for this kind of > test (even without calling QuitClosure). > Btw, base::RunLoop contains QuitClosure() method too, so this is not an > argument. > I'm not very familiar with this message running infrastructure yet, so it'd be > great if you explain me details when we should use MessageLoopRunner instead of > RunLoop. Frankly, this code keeps changing and I lost track, so not an expert here either. My conclusion is based on these observations: 0. RunLoop solves the problem just as elegantly 1. RunLoop is a commonly recognized pattern, used ~20x more than MessageLoopRunner 2. MessageLoopRunner uses RunLoop under the scenes with extra 4x of state to care about 3. chrome/browser/predictors/ is using RunLoop already, but MessageLoopRunner is a new dependency https://codereview.chromium.org/2449323005/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:61: // ResourcePrefetchPredictor::Observer On 2016/11/03 13:49:08, alexilin wrote: > On 2016/11/02 19:37:02, pasko wrote: > > // TestObserver implementation. > > Done. > Btw, I've seen alike comments with the word "implementation" and without, e.g. > "//content::WebContentsObserver:". me too, also 'blah methods.'. What we do in cases like this, is aiming at being more consistent with the code closely around. Your variant looks more consistent with chrome/browser/ and chrome/browser/predictors: $ git grep '// .*::[A-Za-z]* implementation[.]$' chrome/browser/ | wc -l 442 $ git grep '// .*::[A-Za-z]*:$' chrome/browser/ | wc -l 1310 So then I would suggest changing it to: // TestObserver: sorry about the previous misleading suggestion https://codereview.chromium.org/2449323005/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:124: bool has_validators = true, On 2016/11/03 13:49:09, alexilin wrote: > On 2016/11/02 19:37:01, pasko wrote: > > the style guide "somewhat discourages" function overloading and default > > arguments, see: > > > > https://google.github.io/styleguide/cppguide.html#Function_Overloading > > > > Once the amount of arguments reaches ~7, maintaining the right order of > > parameters becomes very error-prone: there are no compiler-enforced checks to > > verify that all these booleans/ints/pointers are passed the way it was > intended. > > > > Since these default arguments are not used yet, let's omit them at the moment, > > this would be simpler. > > > > Later, makes sense to consider having a function to register a ResourceSummary > > an return it. If an individual test cares about some particular parts of the > > ResourceSummary, they can overwrite that particular default value of that > > particular member. > > > > Another option is o have multiple functions under different names to do a > > similar thing with different set of arguments (just like the style guide > > suggests). > > > > Throwing in lots of default args is not a good option > > Many of the tests for ResourcePrefetchPredictors contains functions like this. Yes. I did not have a chance to object at the time they were introduced. > For example, look at Create* functions. I agree that they don't looks very good, > but on other hand these are for tests and objects could be constructed directly > so it's just for convenience. > > Anyway, I think that the idea of returning ResourceSummary is good in this case. Looks nicer indeed. https://codereview.chromium.org/2449323005/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:152: std::unique_ptr<HttpResponse> HandleRequest(const HttpRequest& request) { On 2016/11/03 13:49:08, alexilin wrote: > On 2016/11/02 19:37:02, pasko wrote: > > A few concerns with manually handling requests in this class: > > > > 1. it is a bit manual :) The test server can load files and serve custom > > response headers, see for example: > > > https://codesearch.chromium.org/chromium/src/chrome/test/data/prerender/image... > > > > 2. sharing the state of the test fixture (resources_, predictor_, > > what-may-come-later) with the test server makes it easy to overlook problems > > that happen in reality by intercepting/rewriting all the request-response > meat. > > > > Proposal: rely on default handlers as much as possible. That would be a path > to > > the least surprise. > > > > If we need something fancy we can still have handlers for those and return > > nullptr if the path does not belong to them, the test server will just try the > > next handlers and then the default handlers, see: > > > > > https://codesearch.chromium.org/chromium/src/net/test/embedded_test_server/em... > > > > This proposal is, of course, a tradeoff, for example, it may lead to creating > > more files and more custom headers, happy to discuss. > > I don't see anything special or wrong in manual handling. > Default request handler use the same functions for setting headers and other > stuff as I do. > https://cs.chromium.org/chromium/src/net/test/embedded_test_server/request_ha... > Creation of special header file for each subresource looks more sophisticated > than just add headers in accordance with function parameters in code. For > example, I could iterate over all possible combinations of parameters > programmatically with this method. Default handler has also the way to pass > custom headers via query parameter but it also doesn't look nice. What do you mean by 'nice' in this case? > From other side, I'm not sure that we have to check predictor behavior depending > on cache-control directives but I find that it'd be nice. > > Embedded HTTP server provides a neat and simple way to customize its behavior. I > don't see anything surprising in custom request handler. > I know that it's possible to return nullptr if I don't want to handle some > request and I already use it. Having flexibility is a good point. I do not object to flexibility when we need it, just asking to use more standard methods of solving the problem for non-sophisticated cases. Less coupling of the server side and the test fixture is also probably good, since we may want to introduce other test fixtures with the same resource files, then we would want to avoid duplicating the logic in this function. With the way I proposed it would be easier to say what a test does by looking at html and headers. As usual, this favors reading the code at the cost of less convenient writing, which is a good tradeoff in most cases, because the code is being read many more times than it is being written. https://codereview.chromium.org/2449323005/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:158: std::unique_ptr<BasicHttpResponse> http_response = nit: since the type should be obvious: auto http_response = ... https://codereview.chromium.org/2449323005/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:183: std::map<std::string, size_t> visit_count_; On 2016/11/03 13:49:09, alexilin wrote: > On 2016/11/02 19:37:01, pasko wrote: > > This value gets updated, but the test only observes the value 1. It is not > > obvious to me how other tests will handle multiple navigations and whether it > > would add much value there to check for visit count. > > > > It is preferable to introduce things on as-needed basis, otherwise we risk > > forgetting to remove dead code it the future reviews find other ways to > perform > > the same checks. > > In future tests we definitely will navigate to the same url more than once to > check that prefetch really works. > And in this way we also could check that prefetch doesn't affect history namely > that we count a visit exactly once regardless of whether or not the prefetch > was. I see, makes sense, thanks.
I'm still thinking that the custom request handler isn't evil and will help to create readable tests. But it's up to pasko@. https://codereview.chromium.org/2449323005/diff/100001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2449323005/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:36: using net::test_server::BasicHttpResponse; On 2016/11/03 16:23:13, pasko wrote: > the walls of 'using' statements are not adding too much convenience, especially > the ones that are used only once (like the all the net::test_server::* here), > but add a theoretical risk of causing name conflicts in future changes. Please > minimize the amount of type aliases to only leave those that are saving a lot of > line space. Done. https://codereview.chromium.org/2449323005/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:55: message_loop_runner_(new content::MessageLoopRunner()), On 2016/11/03 16:23:12, pasko wrote: > On 2016/11/03 13:49:09, alexilin wrote: > > On 2016/11/02 19:37:01, pasko wrote: > > > using content::MessageLoopRunner without calling QuitClosure() is confusing > > > because base::RunLoop is sufficient to solve the problem. Please change to > > > base::RunLoop. > > > > MessageLoopRunner does additional work internally in comparison with > > base::RunLoop. Don't we need it? > > I've thought that using MessageLoopRunner is common pattern for this kind of > > test (even without calling QuitClosure). > > Btw, base::RunLoop contains QuitClosure() method too, so this is not an > > argument. > > I'm not very familiar with this message running infrastructure yet, so it'd be > > great if you explain me details when we should use MessageLoopRunner instead > of > > RunLoop. > > Frankly, this code keeps changing and I lost track, so not an expert here > either. My conclusion is based on these observations: > 0. RunLoop solves the problem just as elegantly > 1. RunLoop is a commonly recognized pattern, used ~20x more than > MessageLoopRunner > 2. MessageLoopRunner uses RunLoop under the scenes with extra 4x of state to > care about > 3. chrome/browser/predictors/ is using RunLoop already, but MessageLoopRunner is > a new dependency Acknowledged. https://codereview.chromium.org/2449323005/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:61: // ResourcePrefetchPredictor::Observer On 2016/11/03 16:23:13, pasko wrote: > On 2016/11/03 13:49:08, alexilin wrote: > > On 2016/11/02 19:37:02, pasko wrote: > > > // TestObserver implementation. > > > > Done. > > Btw, I've seen alike comments with the word "implementation" and without, e.g. > > "//content::WebContentsObserver:". > > me too, also 'blah methods.'. What we do in cases like this, is aiming at being > more consistent with the code closely around. Your variant looks more consistent > with chrome/browser/ and chrome/browser/predictors: > > $ git grep '// .*::[A-Za-z]* implementation[.]$' chrome/browser/ | wc -l > 442 > $ git grep '// .*::[A-Za-z]*:$' chrome/browser/ | wc -l > 1310 > > So then I would suggest changing it to: > // TestObserver: > > sorry about the previous misleading suggestion Actually, your suggestion wasn't completely misleading because I had stale class name in comment (ResourcePrefetchPredictor::Observer) https://codereview.chromium.org/2449323005/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:124: bool has_validators = true, On 2016/11/03 16:23:12, pasko wrote: > On 2016/11/03 13:49:09, alexilin wrote: > > On 2016/11/02 19:37:01, pasko wrote: > > > the style guide "somewhat discourages" function overloading and default > > > arguments, see: > > > > > > https://google.github.io/styleguide/cppguide.html#Function_Overloading > > > > > > Once the amount of arguments reaches ~7, maintaining the right order of > > > parameters becomes very error-prone: there are no compiler-enforced checks > to > > > verify that all these booleans/ints/pointers are passed the way it was > > intended. > > > > > > Since these default arguments are not used yet, let's omit them at the > moment, > > > this would be simpler. > > > > > > Later, makes sense to consider having a function to register a > ResourceSummary > > > an return it. If an individual test cares about some particular parts of the > > > ResourceSummary, they can overwrite that particular default value of that > > > particular member. > > > > > > Another option is o have multiple functions under different names to do a > > > similar thing with different set of arguments (just like the style guide > > > suggests). > > > > > > Throwing in lots of default args is not a good option > > > > Many of the tests for ResourcePrefetchPredictors contains functions like this. > > Yes. I did not have a chance to object at the time they were introduced. > > > For example, look at Create* functions. I agree that they don't looks very > good, > > but on other hand these are for tests and objects could be constructed > directly > > so it's just for convenience. > > > > Anyway, I think that the idea of returning ResourceSummary is good in this > case. > > Looks nicer indeed. Acknowledged. https://codereview.chromium.org/2449323005/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:152: std::unique_ptr<HttpResponse> HandleRequest(const HttpRequest& request) { > > Creation of special header file for each subresource looks more sophisticated > > than just add headers in accordance with function parameters in code. For > > example, I could iterate over all possible combinations of parameters > > programmatically with this method. Default handler has also the way to pass > > custom headers via query parameter but it also doesn't look nice. > > What do you mean by 'nice' in this case? Pardon, I had in mind "expected_body" parameter but it has different meaning. > Having flexibility is a good point. I do not object to flexibility when we need > it, just asking to use more standard methods of solving the problem for > non-sophisticated cases. Less coupling of the server side and the test fixture > is also probably good, since we may want to introduce other test fixtures with > the same resource files, then we would want to avoid duplicating the logic in > this function. 1. It's not possible to test redirects with default handler so we'll anyway have to write a custom one. 2. For many resources I don't interested in theirs content. I just want to get HTTP_OK response on it (probably with custom headers). 3. Coupling of the server side and the test fixture allows to control server and test expectations from one place. It leads to less code and more concentration on the real problem. 4. The custom handler allows to create infinite number of "resources" with different names and the same content (which not matters) programmatically. Name matters in that case because it's a key for predictor and cache. Of course, we can have some manipulations with files from test instead of it but why? > With the way I proposed it would be easier to say what a test does by looking at > html and headers. As usual, this favors reading the code at the cost of less > convenient writing, which is a good tradeoff in most cases, because the code is > being read many more times than it is being written. I don't think that my custom handler is so sophisticated. I admit that it could be surprising to see image.png link in .html file but not to see image.png in the directory. But there already are browsertests that use custom handlers. Anyway, we need to come to a final decision. Your suggestion was to use default handler and create separate files for each resource + for each custom header (if needed). If you insist I'll implement it. https://codereview.chromium.org/2449323005/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:158: std::unique_ptr<BasicHttpResponse> http_response = On 2016/11/03 16:23:12, pasko wrote: > nit: since the type should be obvious: auto http_response = ... Done. https://codereview.chromium.org/2449323005/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:183: std::map<std::string, size_t> visit_count_; On 2016/11/03 16:23:13, pasko wrote: > On 2016/11/03 13:49:09, alexilin wrote: > > On 2016/11/02 19:37:01, pasko wrote: > > > This value gets updated, but the test only observes the value 1. It is not > > > obvious to me how other tests will handle multiple navigations and whether > it > > > would add much value there to check for visit count. > > > > > > It is preferable to introduce things on as-needed basis, otherwise we risk > > > forgetting to remove dead code it the future reviews find other ways to > > perform > > > the same checks. > > > > In future tests we definitely will navigate to the same url more than once to > > check that prefetch really works. > > And in this way we also could check that prefetch doesn't affect history > namely > > that we count a visit exactly once regardless of whether or not the prefetch > > was. > > I see, makes sense, thanks. Acknowledged.
OK, let's have the in-process custom handler, as long as it is not too complicated. lgtm with a suggestion to slightly simplify the test logic around mime types Thank you! https://codereview.chromium.org/2449323005/diff/100001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2449323005/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:152: std::unique_ptr<HttpResponse> HandleRequest(const HttpRequest& request) { On 2016/11/03 18:42:12, alexilin wrote: > > > Creation of special header file for each subresource looks more > sophisticated > > > than just add headers in accordance with function parameters in code. For > > > example, I could iterate over all possible combinations of parameters > > > programmatically with this method. Default handler has also the way to pass > > > custom headers via query parameter but it also doesn't look nice. > > > > What do you mean by 'nice' in this case? > > Pardon, I had in mind "expected_body" parameter but it has different meaning. > > > Having flexibility is a good point. I do not object to flexibility when we > need > > it, just asking to use more standard methods of solving the problem for > > non-sophisticated cases. Less coupling of the server side and the test fixture > > is also probably good, since we may want to introduce other test fixtures with > > the same resource files, then we would want to avoid duplicating the logic in > > this function. > > 1. It's not possible to test redirects with default handler so we'll anyway have > to write a custom one. What makes you say that? How about: chrome/test/data/prerender/image-redirect.png.mock-http-headers https://codereview.chromium.org/2449323005/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:190: "image/png"); let's try to remove setting the mime types returned by the server, if that breaks somewhere, it would be interesting to know why, otherwise just less code. Testing mime type recognition on this end-to-end test level is not a goal, it should be covered by the predictor unittest for ResourcePrefetchPredictor::IsHandledResourceType().
The CQ bit was checked by alexilin@chromium.org to run a CQ dry run
Ready to commit after green try bots. https://codereview.chromium.org/2449323005/diff/100001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2449323005/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:152: std::unique_ptr<HttpResponse> HandleRequest(const HttpRequest& request) { On 2016/11/04 15:05:34, pasko wrote: > On 2016/11/03 18:42:12, alexilin wrote: > > > > Creation of special header file for each subresource looks more > > sophisticated > > > > than just add headers in accordance with function parameters in code. For > > > > example, I could iterate over all possible combinations of parameters > > > > programmatically with this method. Default handler has also the way to > pass > > > > custom headers via query parameter but it also doesn't look nice. > > > > > > What do you mean by 'nice' in this case? > > > > Pardon, I had in mind "expected_body" parameter but it has different meaning. > > > > > Having flexibility is a good point. I do not object to flexibility when we > > need > > > it, just asking to use more standard methods of solving the problem for > > > non-sophisticated cases. Less coupling of the server side and the test > fixture > > > is also probably good, since we may want to introduce other test fixtures > with > > > the same resource files, then we would want to avoid duplicating the logic > in > > > this function. > > > > 1. It's not possible to test redirects with default handler so we'll anyway > have > > to write a custom one. > > What makes you say that? How about: > > chrome/test/data/prerender/image-redirect.png.mock-http-headers Oh, I didn't know that HTTP response code is a part of the headers in this universe. My bad, sorry. https://codereview.chromium.org/2449323005/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:190: "image/png"); On 2016/11/04 15:05:34, pasko wrote: > let's try to remove setting the mime types returned by the server, if that > breaks somewhere, it would be interesting to know why, otherwise just less code. > Testing mime type recognition on this end-to-end test level is not a goal, it > should be covered by the predictor unittest for > ResourcePrefetchPredictor::IsHandledResourceType(). Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/11/04 16:07:53, alexilin wrote: > Ready to commit after green try bots. > > https://codereview.chromium.org/2449323005/diff/100001/chrome/browser/predict... > File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc > (right): > > https://codereview.chromium.org/2449323005/diff/100001/chrome/browser/predict... > chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:152: > std::unique_ptr<HttpResponse> HandleRequest(const HttpRequest& request) { > On 2016/11/04 15:05:34, pasko wrote: > > On 2016/11/03 18:42:12, alexilin wrote: > > > > > Creation of special header file for each subresource looks more > > > sophisticated > > > > > than just add headers in accordance with function parameters in code. > For > > > > > example, I could iterate over all possible combinations of parameters > > > > > programmatically with this method. Default handler has also the way to > > pass > > > > > custom headers via query parameter but it also doesn't look nice. > > > > > > > > What do you mean by 'nice' in this case? > > > > > > Pardon, I had in mind "expected_body" parameter but it has different > meaning. > > > > > > > Having flexibility is a good point. I do not object to flexibility when we > > > need > > > > it, just asking to use more standard methods of solving the problem for > > > > non-sophisticated cases. Less coupling of the server side and the test > > fixture > > > > is also probably good, since we may want to introduce other test fixtures > > with > > > > the same resource files, then we would want to avoid duplicating the logic > > in > > > > this function. > > > > > > 1. It's not possible to test redirects with default handler so we'll anyway > > have > > > to write a custom one. > > > > What makes you say that? How about: > > > > chrome/test/data/prerender/image-redirect.png.mock-http-headers > > Oh, I didn't know that HTTP response code is a part of the headers in this > universe. > My bad, sorry. > > https://codereview.chromium.org/2449323005/diff/100001/chrome/browser/predict... > chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:190: > "image/png"); > On 2016/11/04 15:05:34, pasko wrote: > > let's try to remove setting the mime types returned by the server, if that > > breaks somewhere, it would be interesting to know why, otherwise just less > code. > > Testing mime type recognition on this end-to-end test level is not a goal, it > > should be covered by the predictor unittest for > > ResourcePrefetchPredictor::IsHandledResourceType(). > > Done. still lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by pasko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pasko@chromium.org Link to the patchset: https://codereview.chromium.org/2449323005/#ps160001 (title: "Get rid of mime type.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== predictors: Basic browsertest checks predictor learning. First stage for implementing ResourcePrefetchPredictor browsertests is to check that the prefetch database is really filled after some navigations in browser. Browsertest class provides convenient way to declare expected resources in the test and then checks expectations internally that allows to write readable tests. BUG=650253 ========== to ========== predictors: Basic browsertest checks predictor learning. First stage for implementing ResourcePrefetchPredictor browsertests is to check that the prefetch database is really filled after some navigations in browser. Browsertest class provides convenient way to declare expected resources in the test and then checks expectations internally that allows to write readable tests. BUG=650253 Committed: https://crrev.com/e636e3bcc172f220dfadfd2121d238c9908185bb Cr-Commit-Position: refs/heads/master@{#429966} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/e636e3bcc172f220dfadfd2121d238c9908185bb Cr-Commit-Position: refs/heads/master@{#429966}
Message was sent while issue was closed.
A revert of this CL (patchset #9 id:160001) has been created in https://codereview.chromium.org/2470143007/ by mathp@chromium.org. The reason for reverting is: Created a flaky test: first occurrence at https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%.... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
