Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2397943004:
predictors: Use redirect data in prefetch. (Closed)
Descriptionpredictors: Use redirect data in prefetch.
The data about redirects now is used for populate subresource URLs in
ResourcePrefetchPredictor::GetPrefetchData when main page doesn't have its own
subresources.
BUG=631966
Committed: https://crrev.com/a5d0b1f24b9e4a3ae59081a33e83a9e9daf0d005
Cr-Commit-Position: refs/heads/master@{#424447}
Patch Set 1 : . #
Total comments: 12
Patch Set 2 : Refactor GetPrefetchData, get rid of redirects sort. #
Total comments: 17
Patch Set 3 : Renaming and comments. #
Total comments: 15
Patch Set 4 : Add comments + std::max_element. #
Total comments: 4
Patch Set 5 : Add tests. Modify priorities in GetPrefetchData. #
Total comments: 1
Patch Set 6 : Nit. #
Total comments: 2
Patch Set 7 : Non-empty check. #
Messages
Total messages: 34 (12 generated)
Patchset #1 (id:1) has been deleted
alexilin@chromium.org changed reviewers: + lizeb@chromium.org, pasko@chromium.org
Hello, It's time to use collected redirect data for the prefetch. I'm a little bit concerned about absence of tests for StartPrefetching function. WDYT?
https://codereview.chromium.org/2397943004/diff/20001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2397943004/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:563: bool ResourcePrefetchPredictor::GetFinalRedirect( The scoring code is duplicated, and it even seems that you don't really need the scoring, since you're walking all the redirects here anyway. If so, can you simplify the code?
Comment for Benoit. https://codereview.chromium.org/2397943004/diff/20001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2397943004/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:563: bool ResourcePrefetchPredictor::GetFinalRedirect( On 2016/10/07 09:52:18, Benoit L wrote: > The scoring code is duplicated, and it even seems that you don't really need the > scoring, since you're walking all the redirects here anyway. > > If so, can you simplify the code? No, I'm not walking all the redirects here, where did you find it? I pick first redirect from the list, with highest score. But you are right about code duplication. We have different notions of score and confidence in case of resources but they are the same in this case. Probably it will be even better to don't sort redirects and just iterate over all collection here each time. WDYT?
On 2016/10/06 15:41:56, alexilin wrote: > Hello, > > It's time to use collected redirect data for the prefetch. > I'm a little bit concerned about absence of tests for StartPrefetching function. > WDYT? Discussed offline and concluded: * testing new individual functions should be done here * in a separate change (or set of changes) we are going to introduce browsertests that would end-to-end check learned and prefetched resources.
https://codereview.chromium.org/2397943004/diff/20001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://codereview.chromium.org/2397943004/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:246: // Returns true if the URL-based table contains PrefetchData that can be not fully true, since the |urls| could be non-empty and hence it would return true even if the table contains no relevant data. We should put a DCHECK(urls->empty()) in the beginning of the implementation of every function like this. https://codereview.chromium.org/2397943004/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:252: // Returns true if the host-based table contains PrefetchData that can be used It is preferable to write 'iff' in this case (iff == if and only if) https://codereview.chromium.org/2397943004/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:264: bool GetFinalRedirect(const std::string& first_redirect, this seems not to require any state of the object. In this case it should be either static or should move to anonymous namespace in the .cc file. Now, if we want to test it, we have two options: 1. public bool GetFinalRedirectForTesting(...) // calls .cc:GetFinalRedirect(), more preferable by me 2. private static bool GetFinalRedirect(...) // with FRIEND_TEST_ALL_PREFIXES .. which we have many. https://codereview.chromium.org/2397943004/diff/20001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor_tables.cc (right): https://codereview.chromium.org/2397943004/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:378: (data.number_of_hits() + data.number_of_misses()); If this is not covered by any tests, it would be nice to create once: populate redirect tables, read back the redirects and check for their order.
Dear reviewers, I've almost rewritten this CL. Changes: - Scan all redirects while populating to find the best one instead of sort them while recording. - Refactor GetPrefetchData: merge GetURLPrefetchData, GetHostPrefetchData and PopulatePrefetcherRequest into the one function PopulatePrefetcherRequest. - Make GetFinalRedirect static. - Fix "true iff resources was added" invariant in PopulatePrefetchRequest. - Fix comments. Tests for GetPrefetchData, PopulatePrefetcherRequest and GetFinalRedirect functions are in progress yet. https://codereview.chromium.org/2397943004/diff/20001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2397943004/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:563: bool ResourcePrefetchPredictor::GetFinalRedirect( On 2016/10/07 10:49:07, alexilin wrote: > On 2016/10/07 09:52:18, Benoit L wrote: > > The scoring code is duplicated, and it even seems that you don't really need > the > > scoring, since you're walking all the redirects here anyway. > > > > If so, can you simplify the code? > > No, I'm not walking all the redirects here, where did you find it? > I pick first redirect from the list, with highest score. > But you are right about code duplication. > We have different notions of score and confidence in case of resources but they > are the same in this case. > > Probably it will be even better to don't sort redirects and just iterate over > all collection here each time. > > WDYT? Scan all the redirects here instead of sort while recording. Thanks for the tip. https://codereview.chromium.org/2397943004/diff/20001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://codereview.chromium.org/2397943004/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:246: // Returns true if the URL-based table contains PrefetchData that can be On 2016/10/07 11:42:43, pasko wrote: > not fully true, since the |urls| could be non-empty and hence it would return > true even if the table contains no relevant data. > > We should put a DCHECK(urls->empty()) in the beginning of the implementation of > every function like this. Yeah, excellent observation! Nevertheless, I think it's strange and non-intuitive invariant for this function so I'll just check if any element was added to |urls|. https://codereview.chromium.org/2397943004/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:252: // Returns true if the host-based table contains PrefetchData that can be used On 2016/10/07 11:42:43, pasko wrote: > It is preferable to write 'iff' in this case (iff == if and only if) Done. https://codereview.chromium.org/2397943004/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:264: bool GetFinalRedirect(const std::string& first_redirect, On 2016/10/07 11:42:43, pasko wrote: > this seems not to require any state of the object. In this case it should be > either static or should move to anonymous namespace in the .cc file. > > Now, if we want to test it, we have two options: > 1. public bool GetFinalRedirectForTesting(...) // calls .cc:GetFinalRedirect(), > more preferable by me > 2. private static bool GetFinalRedirect(...) // with FRIEND_TEST_ALL_PREFIXES .. > which we have many. Implemented the second one because "we have many". Done. https://codereview.chromium.org/2397943004/diff/20001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor_tables.cc (right): https://codereview.chromium.org/2397943004/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:378: (data.number_of_hits() + data.number_of_misses()); On 2016/10/07 11:42:43, pasko wrote: > If this is not covered by any tests, it would be nice to create once: populate > redirect tables, read back the redirects and check for their order. I've just realized that we don't have to sort redirects because we need only the best one for populating prefetch. It could be done by single pass through the array instead of (potentially) n*log(n) sorting. So I removed this code.
overall looking good, a few random comments that came into mind, .. and will wait for tests https://codereview.chromium.org/2397943004/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2397943004/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:241: const RedirectStat& best_redirect = std::max_element? https://codereview.chromium.org/2397943004/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://codereview.chromium.org/2397943004/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:227: // Returns true if |redirect_data_map| contains confident redirect endpoint nit: iff https://codereview.chromium.org/2397943004/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:228: // for |first_redirect| and assign it to the |final_redirect|. nit: s/assign/assigns/ https://codereview.chromium.org/2397943004/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:228: // for |first_redirect| and assign it to the |final_redirect|. nit: first_redirect is not a good name, because a redirect may refer to two URLs and other data like request/response headers, and this is just a string. Suggestion: |entry_point|. https://codereview.chromium.org/2397943004/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:230: RedirectDataMap* redirect_data_map, if the object (RedirectDataMap) is not modified by the function, it should be passed as const reference (or by value). This would be according to the style guide: https://google.github.io/styleguide/cppguide.html#Function_Parameter_Ordering https://google.github.io/styleguide/cppguide.html#Reference_Arguments We do not always follow it (historical reasons?), but for new code it is strongly recommended. https://codereview.chromium.org/2397943004/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:231: std::string* final_redirect); nit: |redirect_endpoint| sounds more intuitive (feel free to discard/object if it does not work for you this way) https://codereview.chromium.org/2397943004/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:257: PrefetchDataMap* data_map, const reference here too
https://codereview.chromium.org/2397943004/diff/20001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2397943004/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:563: bool ResourcePrefetchPredictor::GetFinalRedirect( On 2016/10/07 13:09:29, alexilin wrote: > On 2016/10/07 10:49:07, alexilin wrote: > > On 2016/10/07 09:52:18, Benoit L wrote: > > > The scoring code is duplicated, and it even seems that you don't really need > > the > > > scoring, since you're walking all the redirects here anyway. > > > > > > If so, can you simplify the code? > > > > No, I'm not walking all the redirects here, where did you find it? > > I pick first redirect from the list, with highest score. > > But you are right about code duplication. > > We have different notions of score and confidence in case of resources but > they > > are the same in this case. > > > > Probably it will be even better to don't sort redirects and just iterate over > > all collection here each time. > > > > WDYT? > > Scan all the redirects here instead of sort while recording. Thanks for the tip. Sorry, wasn't clear enough. Thanks for reading between the lines, and implementing it anyway.
Patchset #3 (id:60001) has been deleted
Answer Egor's comments. Tests are still in progress... https://codereview.chromium.org/2397943004/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2397943004/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:241: const RedirectStat& best_redirect = On 2016/10/07 13:44:27, pasko wrote: > std::max_element? Well, it will lead to small overhead : we'll have to compute confidence for current best element each time. WDYT? https://codereview.chromium.org/2397943004/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://codereview.chromium.org/2397943004/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:227: // Returns true if |redirect_data_map| contains confident redirect endpoint On 2016/10/07 13:44:27, pasko wrote: > nit: iff Done. https://codereview.chromium.org/2397943004/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:228: // for |first_redirect| and assign it to the |final_redirect|. On 2016/10/07 13:44:27, pasko wrote: > nit: first_redirect is not a good name, because a redirect may refer to two URLs > and other data like request/response headers, and this is just a string. > Suggestion: |entry_point|. Done. https://codereview.chromium.org/2397943004/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:230: RedirectDataMap* redirect_data_map, On 2016/10/07 13:44:27, pasko wrote: > if the object (RedirectDataMap) is not modified by the function, it should be > passed as const reference (or by value). > > This would be according to the style guide: > > https://google.github.io/styleguide/cppguide.html#Function_Parameter_Ordering > > https://google.github.io/styleguide/cppguide.html#Reference_Arguments > > We do not always follow it (historical reasons?), but for new code it is > strongly recommended. Ok, thanks! I know this rule but I was confused by surrounding code and by the fact that obtaining const reference from unique_ptr looks ugly. (A few moments later) Ah, I forgot about unique_ptr::operator*. Not so ugly now. https://codereview.chromium.org/2397943004/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:231: std::string* final_redirect); On 2016/10/07 13:44:27, pasko wrote: > nit: |redirect_endpoint| sounds more intuitive > > (feel free to discard/object if it does not work for you this way) Done. https://codereview.chromium.org/2397943004/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:257: PrefetchDataMap* data_map, On 2016/10/07 13:44:27, pasko wrote: > const reference here too Done.
https://codereview.chromium.org/2397943004/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2397943004/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:241: const RedirectStat& best_redirect = On 2016/10/07 14:29:05, alexilin wrote: > On 2016/10/07 13:44:27, pasko wrote: > > std::max_element? > > Well, it will lead to small overhead : we'll have to compute confidence for > current best element each time. WDYT? Did you mean we would re-compute the confidence 2 times for the best element (but only once for all other elements)? I did not think about it, so let me think as I am writing this. I am a fan of not doing premature optimization. We have numerous bigger inefficiencies sprinkled densely around the code, each one of them would be 1000x slower than these 3 floating point operations. So it seems unlikely that this optimization would turn out to be user-visible. The benefits of having 5 lines of straightforward code instead of ~20 lines look attractive to me. We are also fans of boring code: even though it is less fun to write, it is a lot easier to maintain. So yeah, using std::max_element looks better. https://codereview.chromium.org/2397943004/diff/80001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2397943004/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:541: if (use_url_data && add a comment above: // Attempt to fetch URLs based on the incoming host/URL. https://codereview.chromium.org/2397943004/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:549: std::string redirect_endpoint; Add a comment 1 line above saying: // Fallback to fetching URLs based on a redirect endpoint for hist/URL. https://codereview.chromium.org/2397943004/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:555: *url_table_cache_, urls)) if the condition takes multiple lines, it requires braces (used to be chromium style, but I cannot find a reference easily - good for consistency) https://codereview.chromium.org/2397943004/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:558: if (use_host_data && PopulatePrefetcherRequest(redirect_endpoint_url.host(), Using URL-based redirects for host-based prefetch has non-obvious benefits. Can you explain the motivation? (we might have discussed it, and I just forgot) At least it seems that host-based redirects should be tried first. Because of this the code looks less symmetric and harder to read.
Egor answers too fast, no time for tests... https://codereview.chromium.org/2397943004/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2397943004/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:241: const RedirectStat& best_redirect = On 2016/10/07 15:42:44, pasko wrote: > On 2016/10/07 14:29:05, alexilin wrote: > > On 2016/10/07 13:44:27, pasko wrote: > > > std::max_element? > > > > Well, it will lead to small overhead : we'll have to compute confidence for > > current best element each time. WDYT? > > Did you mean we would re-compute the confidence 2 times for the best element > (but only once for all other elements)? > > I did not think about it, so let me think as I am writing this. I am a fan of > not doing premature optimization. We have numerous bigger inefficiencies > sprinkled densely around the code, each one of them would be 1000x slower than > these 3 floating point operations. So it seems unlikely that this optimization > would turn out to be user-visible. > > The benefits of having 5 lines of straightforward code instead of ~20 lines look > attractive to me. We are also fans of boring code: even though it is less fun to > write, it is a lot easier to maintain. > > So yeah, using std::max_element looks better. I meant that std::max_element accepts comparer as an argument so the confidence will be computed for two arguments (for current element and current best) for each element in list. I've got your message, thanks. Done. https://codereview.chromium.org/2397943004/diff/80001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2397943004/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:541: if (use_url_data && On 2016/10/07 15:42:44, pasko wrote: > add a comment above: > // Attempt to fetch URLs based on the incoming host/URL. Done. https://codereview.chromium.org/2397943004/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:549: std::string redirect_endpoint; On 2016/10/07 15:42:44, pasko wrote: > Add a comment 1 line above saying: > // Fallback to fetching URLs based on a redirect endpoint for hist/URL. Done. https://codereview.chromium.org/2397943004/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:555: *url_table_cache_, urls)) On 2016/10/07 15:42:44, pasko wrote: > if the condition takes multiple lines, it requires braces (used to be chromium > style, but I cannot find a reference easily - good for consistency) Oh, I've thought that this rule is applied only for single multiline statement under condition. Good to know, thanks! Quote from https://google.github.io/styleguide/cppguide.html#Conditionals "In general, curly braces are not required for single-line statements, but they are allowed if you like them; conditional or loop statements with complex conditions or statements may be more readable with curly braces. Some projects require that an if must always always have an accompanying brace." So actually this rule isn't obligatory. https://codereview.chromium.org/2397943004/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:558: if (use_host_data && PopulatePrefetcherRequest(redirect_endpoint_url.host(), On 2016/10/07 15:42:45, pasko wrote: > Using URL-based redirects for host-based prefetch has non-obvious benefits. Can > you explain the motivation? (we might have discussed it, and I just forgot) > > At least it seems that host-based redirects should be tried first. > > Because of this the code looks less symmetric and harder to read. It wasn't discussed yet so this is only my opinion. Well, URL-based redirect seemingly should have more accurate stats than host-based URL. Since we are confident in redirect endpoint, this redirect will lead us to a web-page that user will likely see. Then why shouldn't we try host-base prefetch in this case? Redirects and resources tables are not connected actually. I'm not sure that we should try host-based redirects first for the same reason that we don't try host-based resources first: it looks like URL data is more "accurate". Moreover, if we have URL-based redirect for some URL then in most cases we'll have host-based redirect for this URL. Hence there won't be any sense to try URL-based redirects after host-based.
I am generally slow to respond, I guess it's just this change was super-exciting :) https://codereview.chromium.org/2397943004/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2397943004/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:241: const RedirectStat& best_redirect = On 2016/10/07 16:33:44, alexilin wrote: > On 2016/10/07 15:42:44, pasko wrote: > > On 2016/10/07 14:29:05, alexilin wrote: > > > On 2016/10/07 13:44:27, pasko wrote: > > > > std::max_element? > > > > > > Well, it will lead to small overhead : we'll have to compute confidence for > > > current best element each time. WDYT? > > > > Did you mean we would re-compute the confidence 2 times for the best element > > (but only once for all other elements)? > > > > I did not think about it, so let me think as I am writing this. I am a fan of > > not doing premature optimization. We have numerous bigger inefficiencies > > sprinkled densely around the code, each one of them would be 1000x slower than > > these 3 floating point operations. So it seems unlikely that this optimization > > would turn out to be user-visible. > > > > The benefits of having 5 lines of straightforward code instead of ~20 lines > look > > attractive to me. We are also fans of boring code: even though it is less fun > to > > write, it is a lot easier to maintain. > > > > So yeah, using std::max_element looks better. > > I meant that std::max_element accepts comparer as an argument so the confidence > will be computed for two arguments (for current element and current best) for > each element in list. Ah, sorry, you are right, this has a comparator, hence evaluating twice each time. I did not realize that. Still not overly heavy. https://codereview.chromium.org/2397943004/diff/80001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2397943004/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:555: *url_table_cache_, urls)) On 2016/10/07 16:33:44, alexilin wrote: > On 2016/10/07 15:42:44, pasko wrote: > > if the condition takes multiple lines, it requires braces (used to be chromium > > style, but I cannot find a reference easily - good for consistency) > > Oh, I've thought that this rule is applied only for single multiline statement > under condition. Good to know, thanks! > > Quote from > https://google.github.io/styleguide/cppguide.html#Conditionals > "In general, curly braces are not required for single-line statements, but they > are allowed if you like them; conditional or loop statements with complex > conditions or statements may be more readable with curly braces. Some projects > require that an if must always always have an accompanying brace." > > So actually this rule isn't obligatory. I was curious ... because I actually prefer always-braces. The chromium style guide used to have this line: "Omit {} for one-line conditional or loop bodies", but it was removed 4 years ago without notice .. (I just traced it, rev#126 of [1] - pkasting@), somewhat later there is a conversation on chromium-dev@ [2] where he expresses his opinion: """ there are files and submodules within Chrome that require braces on all conditionals; if you're modifying code in those, follow the prevailing practice. Most code does not use braces on one-line conditionals, and I am not a huge fan of either changing it all or just instating a new rule, because the amount of code affected is so large -- probably larger than with any other rule change we've ever discussed. """ so indeed putting or not-putting braces is not obligatory, hence, as usual we should aim at consistency with the current file/module. In any case, this is a bit orthogonal, multi-line statements require braces :) [1] https://sites.google.com/a/chromium.org/dev/developers/coding-style [2] https://groups.google.com/a/chromium.org/d/msg/chromium-dev/bf385RfI_JQ/bON68... https://codereview.chromium.org/2397943004/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:558: if (use_host_data && PopulatePrefetcherRequest(redirect_endpoint_url.host(), On 2016/10/07 16:33:44, alexilin wrote: > On 2016/10/07 15:42:45, pasko wrote: > > Using URL-based redirects for host-based prefetch has non-obvious benefits. > Can > > you explain the motivation? (we might have discussed it, and I just forgot) > > > > At least it seems that host-based redirects should be tried first. > > > > Because of this the code looks less symmetric and harder to read. > > It wasn't discussed yet so this is only my opinion. > Well, URL-based redirect seemingly should have more accurate stats than > host-based URL. Since we are confident in redirect endpoint, this redirect will > lead us to a web-page that user will likely see. Then why shouldn't we try > host-base prefetch in this case? Redirects and resources tables are not > connected actually. > > I'm not sure that we should try host-based redirects first for the same reason > that we don't try host-based resources first: it looks like URL data is more > "accurate". Moreover, if we have URL-based redirect for some URL then in most > cases we'll have host-based redirect for this URL. Hence there won't be any > sense to try URL-based redirects after host-based. Im, interesting and reasonable. Theoretically .. it could be that the data for this particular URL does not have many datapoints and hence has some randomness, and the host data is more precise. In other words, I can see cases when your proposal is good and when it is bad, but I don't have a sense of what is more likely. Perhaps we need a whiteboard for that .. or a blackboard. So, in presence of no clarity of the impact of this, I would prefer the code to be simpler and more symmetric towards URL/host, it would be easier to review/modify and perform data analysis if those two systems are more independent.
Code looks fine, I don't have any comments on it (aside from agreeing with pasko's). Only one comment regarding the way we get the data, but it's perfectly fine to disagree, or to postpone it to a forthcoming CL. https://codereview.chromium.org/2397943004/diff/100001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2397943004/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:536: // Attempt to fetch URLs based on the incoming URL/host. I'm not sure that's the policy we want, though I'm not sure that the policy change is something for this CL either, and what's implemented here makes sense. Let's take the case of a mobile redirect, i.e. foo.com -> m.foo.com. Let's assume that this redirect happens on mobile only, and that the user has used "request desktop site" at least a few times, or that the target website doesn't redirect the user to the mobile site for some pages. Then we have data for both the mobile and the non-mobile version, even though the mobile site is much more relevant.
Hi folks, I've added tests to cover all new functions. Also I've accepted yours suggestions about GetPrefetchData function. I'm ready for yours grammar-nazi comments. https://codereview.chromium.org/2397943004/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2397943004/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:241: const RedirectStat& best_redirect = On 2016/10/07 17:18:54, pasko wrote: > On 2016/10/07 16:33:44, alexilin wrote: > > On 2016/10/07 15:42:44, pasko wrote: > > > On 2016/10/07 14:29:05, alexilin wrote: > > > > On 2016/10/07 13:44:27, pasko wrote: > > > > > std::max_element? > > > > > > > > Well, it will lead to small overhead : we'll have to compute confidence > for > > > > current best element each time. WDYT? > > > > > > Did you mean we would re-compute the confidence 2 times for the best element > > > (but only once for all other elements)? > > > > > > I did not think about it, so let me think as I am writing this. I am a fan > of > > > not doing premature optimization. We have numerous bigger inefficiencies > > > sprinkled densely around the code, each one of them would be 1000x slower > than > > > these 3 floating point operations. So it seems unlikely that this > optimization > > > would turn out to be user-visible. > > > > > > The benefits of having 5 lines of straightforward code instead of ~20 lines > > look > > > attractive to me. We are also fans of boring code: even though it is less > fun > > to > > > write, it is a lot easier to maintain. > > > > > > So yeah, using std::max_element looks better. > > > > I meant that std::max_element accepts comparer as an argument so the > confidence > > will be computed for two arguments (for current element and current best) for > > each element in list. > > Ah, sorry, you are right, this has a comparator, hence evaluating twice each > time. I did not realize that. Still not overly heavy. > Done. https://codereview.chromium.org/2397943004/diff/80001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2397943004/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:555: *url_table_cache_, urls)) On 2016/10/07 17:18:54, pasko wrote: > On 2016/10/07 16:33:44, alexilin wrote: > > On 2016/10/07 15:42:44, pasko wrote: > > > if the condition takes multiple lines, it requires braces (used to be > chromium > > > style, but I cannot find a reference easily - good for consistency) > > > > Oh, I've thought that this rule is applied only for single multiline statement > > under condition. Good to know, thanks! > > > > Quote from > > https://google.github.io/styleguide/cppguide.html#Conditionals > > "In general, curly braces are not required for single-line statements, but > they > > are allowed if you like them; conditional or loop statements with complex > > conditions or statements may be more readable with curly braces. Some projects > > require that an if must always always have an accompanying brace." > > > > So actually this rule isn't obligatory. > > I was curious ... because I actually prefer always-braces. > > The chromium style guide used to have this line: > "Omit {} for one-line conditional or loop bodies", but it was removed 4 years > ago without notice .. (I just traced it, rev#126 of [1] - pkasting@), somewhat > later there is a conversation on chromium-dev@ [2] where he expresses his > opinion: > > """ > there are files and submodules within Chrome that require braces on all > conditionals; if you're modifying code in those, follow the prevailing practice. > Most code does not use braces on one-line conditionals, and I am not a huge fan > of either changing it all or just instating a new rule, because the amount of > code affected is so large -- probably larger than with any other rule change > we've ever discussed. > """ > > so indeed putting or not-putting braces is not obligatory, hence, as usual we > should aim at consistency with the current file/module. > > In any case, this is a bit orthogonal, multi-line statements require braces :) > > [1] https://sites.google.com/a/chromium.org/dev/developers/coding-style > > [2] > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/bf385RfI_JQ/bON68... Benoit has already forced me to remove brackets in one-line if statement. So I'm between two fires with you guys. Done. https://codereview.chromium.org/2397943004/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:558: if (use_host_data && PopulatePrefetcherRequest(redirect_endpoint_url.host(), On 2016/10/07 17:18:54, pasko wrote: > On 2016/10/07 16:33:44, alexilin wrote: > > On 2016/10/07 15:42:45, pasko wrote: > > > Using URL-based redirects for host-based prefetch has non-obvious benefits. > > Can > > > you explain the motivation? (we might have discussed it, and I just forgot) > > > > > > At least it seems that host-based redirects should be tried first. > > > > > > Because of this the code looks less symmetric and harder to read. > > > > It wasn't discussed yet so this is only my opinion. > > Well, URL-based redirect seemingly should have more accurate stats than > > host-based URL. Since we are confident in redirect endpoint, this redirect > will > > lead us to a web-page that user will likely see. Then why shouldn't we try > > host-base prefetch in this case? Redirects and resources tables are not > > connected actually. > > > > I'm not sure that we should try host-based redirects first for the same reason > > that we don't try host-based resources first: it looks like URL data is more > > "accurate". Moreover, if we have URL-based redirect for some URL then in most > > cases we'll have host-based redirect for this URL. Hence there won't be any > > sense to try URL-based redirects after host-based. > > Im, interesting and reasonable. > > Theoretically .. it could be that the data for this particular URL does not have > many datapoints and hence has some randomness, and the host data is more > precise. In other words, I can see cases when your proposal is good and when it > is bad, but I don't have a sense of what is more likely. Perhaps we need a > whiteboard for that .. or a blackboard. > > So, in presence of no clarity of the impact of this, I would prefer the code to > be simpler and more symmetric towards URL/host, it would be easier to > review/modify and perform data analysis if those two systems are more > independent. Done. https://codereview.chromium.org/2397943004/diff/100001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2397943004/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:536: // Attempt to fetch URLs based on the incoming URL/host. On 2016/10/10 15:56:04, Benoit L wrote: > I'm not sure that's the policy we want, though I'm not sure that the policy > change is something for this CL either, and what's implemented here makes sense. > > Let's take the case of a mobile redirect, i.e. http://foo.com -> http://m.foo.com. Let's > assume that this redirect happens on mobile only, and that the user has used > "request desktop site" at least a few times, or that the target website doesn't > redirect the user to the mobile site for some pages. > > Then we have data for both the mobile and the non-mobile version, even though > the mobile site is much more relevant. So, do you suggest that we should fetch resources based on a redirect endpoint first, and then to make fallback to fetching resources based on the incoming URL? Probably, it makes more sense. I added this update to this CL, because I don't want to commit new code only to change it later. And this is not a big change. I think that we also could use redirect confidence parameter here. For example, at first try to fetch resources based on redirect with confidence over 0.9 then if such wasn't found to fetch resources based on incoming url, then try some less confident redirects.
lgtm, thank you. https://codereview.chromium.org/2397943004/diff/100001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2397943004/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:536: // Attempt to fetch URLs based on the incoming URL/host. On 2016/10/11 13:14:41, alexilin wrote: > On 2016/10/10 15:56:04, Benoit L wrote: > > I'm not sure that's the policy we want, though I'm not sure that the policy > > change is something for this CL either, and what's implemented here makes > sense. > > > > Let's take the case of a mobile redirect, i.e. http://foo.com -> > http://m.foo.com. Let's > > assume that this redirect happens on mobile only, and that the user has used > > "request desktop site" at least a few times, or that the target website > doesn't > > redirect the user to the mobile site for some pages. > > > > Then we have data for both the mobile and the non-mobile version, even though > > the mobile site is much more relevant. > > So, do you suggest that we should fetch resources based on a redirect endpoint > first, and then to make fallback to fetching resources based on the incoming > URL? > Probably, it makes more sense. > I added this update to this CL, because I don't want to commit new code only to > change it later. And this is not a big change. > > I think that we also could use redirect confidence parameter here. For example, > at first try to fetch resources based on redirect with confidence over 0.9 then > if such wasn't found to fetch resources based on incoming url, then try some > less confident redirects. I think that what you suggest is better than what's implemented here, but they should not differ very much. Thank you for addressing that. https://codereview.chromium.org/2397943004/diff/120001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_unittest.cc (right): https://codereview.chromium.org/2397943004/diff/120001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:1348: // The data to be sure that other PrefetchData won't affect. nit: won't be affected.
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...
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...
CL is ready for commit. pasko@ this is a last chance to say a word. https://codereview.chromium.org/2397943004/diff/100001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2397943004/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:536: // Attempt to fetch URLs based on the incoming URL/host. On 2016/10/11 13:37:11, Benoit L wrote: > On 2016/10/11 13:14:41, alexilin wrote: > > On 2016/10/10 15:56:04, Benoit L wrote: > > > I'm not sure that's the policy we want, though I'm not sure that the policy > > > change is something for this CL either, and what's implemented here makes > > sense. > > > > > > Let's take the case of a mobile redirect, i.e. http://foo.com -> > > http://m.foo.com. Let's > > > assume that this redirect happens on mobile only, and that the user has used > > > "request desktop site" at least a few times, or that the target website > > doesn't > > > redirect the user to the mobile site for some pages. > > > > > > Then we have data for both the mobile and the non-mobile version, even > though > > > the mobile site is much more relevant. > > > > So, do you suggest that we should fetch resources based on a redirect endpoint > > first, and then to make fallback to fetching resources based on the incoming > > URL? > > Probably, it makes more sense. > > I added this update to this CL, because I don't want to commit new code only > to > > change it later. And this is not a big change. > > > > I think that we also could use redirect confidence parameter here. For > example, > > at first try to fetch resources based on redirect with confidence over 0.9 > then > > if such wasn't found to fetch resources based on incoming url, then try some > > less confident redirects. > > I think that what you suggest is better than what's implemented here, but they > should not differ very much. > > Thank you for addressing that. Done.
lgtm with one relatively minor question addressed https://codereview.chromium.org/2397943004/diff/80001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2397943004/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:555: *url_table_cache_, urls)) On 2016/10/11 13:14:41, alexilin wrote: > On 2016/10/07 17:18:54, pasko wrote: > > On 2016/10/07 16:33:44, alexilin wrote: > > > On 2016/10/07 15:42:44, pasko wrote: > > > > if the condition takes multiple lines, it requires braces (used to be > > chromium > > > > style, but I cannot find a reference easily - good for consistency) > > > > > > Oh, I've thought that this rule is applied only for single multiline > statement > > > under condition. Good to know, thanks! > > > > > > Quote from > > > https://google.github.io/styleguide/cppguide.html#Conditionals > > > "In general, curly braces are not required for single-line statements, but > > they > > > are allowed if you like them; conditional or loop statements with complex > > > conditions or statements may be more readable with curly braces. Some > projects > > > require that an if must always always have an accompanying brace." > > > > > > So actually this rule isn't obligatory. > > > > I was curious ... because I actually prefer always-braces. > > > > The chromium style guide used to have this line: > > "Omit {} for one-line conditional or loop bodies", but it was removed 4 years > > ago without notice .. (I just traced it, rev#126 of [1] - pkasting@), somewhat > > later there is a conversation on chromium-dev@ [2] where he expresses his > > opinion: > > > > """ > > there are files and submodules within Chrome that require braces on all > > conditionals; if you're modifying code in those, follow the prevailing > practice. > > Most code does not use braces on one-line conditionals, and I am not a huge > fan > > of either changing it all or just instating a new rule, because the amount of > > code affected is so large -- probably larger than with any other rule change > > we've ever discussed. > > """ > > > > so indeed putting or not-putting braces is not obligatory, hence, as usual we > > should aim at consistency with the current file/module. > > > > In any case, this is a bit orthogonal, multi-line statements require braces :) > > > > [1] https://sites.google.com/a/chromium.org/dev/developers/coding-style > > > > [2] > > > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/bf385RfI_JQ/bON68... > > Benoit has already forced me to remove brackets in one-line if statement. So I'm > between two fires with you guys. > Done. Sorry for the fires :) I am with Benoit on one-line conditionals (for consistency), but this statement did not fit one line. https://codereview.chromium.org/2397943004/diff/140001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2397943004/diff/140001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:240: if (ComputeRedirectConfidence(*best_redirect) < are redirect_endpoints guaranteed to be non-empty? DCHECK would be nice, otherwise just return false if best_redirect == redirect_data.redirect_endpoints().end()
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Committing now. https://codereview.chromium.org/2397943004/diff/80001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2397943004/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:555: *url_table_cache_, urls)) On 2016/10/11 14:16:19, pasko wrote: > On 2016/10/11 13:14:41, alexilin wrote: > > On 2016/10/07 17:18:54, pasko wrote: > > > On 2016/10/07 16:33:44, alexilin wrote: > > > > On 2016/10/07 15:42:44, pasko wrote: > > > > > if the condition takes multiple lines, it requires braces (used to be > > > chromium > > > > > style, but I cannot find a reference easily - good for consistency) > > > > > > > > Oh, I've thought that this rule is applied only for single multiline > > statement > > > > under condition. Good to know, thanks! > > > > > > > > Quote from > > > > https://google.github.io/styleguide/cppguide.html#Conditionals > > > > "In general, curly braces are not required for single-line statements, but > > > they > > > > are allowed if you like them; conditional or loop statements with complex > > > > conditions or statements may be more readable with curly braces. Some > > projects > > > > require that an if must always always have an accompanying brace." > > > > > > > > So actually this rule isn't obligatory. > > > > > > I was curious ... because I actually prefer always-braces. > > > > > > The chromium style guide used to have this line: > > > "Omit {} for one-line conditional or loop bodies", but it was removed 4 > years > > > ago without notice .. (I just traced it, rev#126 of [1] - pkasting@), > somewhat > > > later there is a conversation on chromium-dev@ [2] where he expresses his > > > opinion: > > > > > > """ > > > there are files and submodules within Chrome that require braces on all > > > conditionals; if you're modifying code in those, follow the prevailing > > practice. > > > Most code does not use braces on one-line conditionals, and I am not a huge > > fan > > > of either changing it all or just instating a new rule, because the amount > of > > > code affected is so large -- probably larger than with any other rule change > > > we've ever discussed. > > > """ > > > > > > so indeed putting or not-putting braces is not obligatory, hence, as usual > we > > > should aim at consistency with the current file/module. > > > > > > In any case, this is a bit orthogonal, multi-line statements require braces > :) > > > > > > [1] https://sites.google.com/a/chromium.org/dev/developers/coding-style > > > > > > [2] > > > > > > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/bf385RfI_JQ/bON68... > > > > Benoit has already forced me to remove brackets in one-line if statement. So > I'm > > between two fires with you guys. > > Done. > > Sorry for the fires :) > > I am with Benoit on one-line conditionals (for consistency), but this statement > did not fit one line. Yeah, I see that this is another case. To summarize, no braces if the condition AND the statement fits in one line. Otherwise, put braces. It also depends on surrounding code, of course. https://codereview.chromium.org/2397943004/diff/140001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2397943004/diff/140001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:240: if (ComputeRedirectConfidence(*best_redirect) < On 2016/10/11 14:16:19, pasko wrote: > are redirect_endpoints guaranteed to be non-empty? DCHECK would be nice, > otherwise just return false if best_redirect == > redirect_data.redirect_endpoints().end() It's a good point, thanks. I've actually forgot to add this check. In fact, this should never happen because I'm deleting RedirectData without redirects from cache and db. But things could be changed later.
The CQ bit was checked by alexilin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lizeb@chromium.org, pasko@chromium.org Link to the patchset: https://codereview.chromium.org/2397943004/#ps160001 (title: "Non-empty check.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2397943004/diff/80001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2397943004/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:555: *url_table_cache_, urls)) On 2016/10/11 15:24:29, alexilin wrote: > On 2016/10/11 14:16:19, pasko wrote: > > On 2016/10/11 13:14:41, alexilin wrote: > > > On 2016/10/07 17:18:54, pasko wrote: > > > > On 2016/10/07 16:33:44, alexilin wrote: > > > > > On 2016/10/07 15:42:44, pasko wrote: > > > > > > if the condition takes multiple lines, it requires braces (used to be > > > > chromium > > > > > > style, but I cannot find a reference easily - good for consistency) > > > > > > > > > > Oh, I've thought that this rule is applied only for single multiline > > > statement > > > > > under condition. Good to know, thanks! > > > > > > > > > > Quote from > > > > > https://google.github.io/styleguide/cppguide.html#Conditionals > > > > > "In general, curly braces are not required for single-line statements, > but > > > > they > > > > > are allowed if you like them; conditional or loop statements with > complex > > > > > conditions or statements may be more readable with curly braces. Some > > > projects > > > > > require that an if must always always have an accompanying brace." > > > > > > > > > > So actually this rule isn't obligatory. > > > > > > > > I was curious ... because I actually prefer always-braces. > > > > > > > > The chromium style guide used to have this line: > > > > "Omit {} for one-line conditional or loop bodies", but it was removed 4 > > years > > > > ago without notice .. (I just traced it, rev#126 of [1] - pkasting@), > > somewhat > > > > later there is a conversation on chromium-dev@ [2] where he expresses his > > > > opinion: > > > > > > > > """ > > > > there are files and submodules within Chrome that require braces on all > > > > conditionals; if you're modifying code in those, follow the prevailing > > > practice. > > > > Most code does not use braces on one-line conditionals, and I am not a > huge > > > fan > > > > of either changing it all or just instating a new rule, because the amount > > of > > > > code affected is so large -- probably larger than with any other rule > change > > > > we've ever discussed. > > > > """ > > > > > > > > so indeed putting or not-putting braces is not obligatory, hence, as usual > > we > > > > should aim at consistency with the current file/module. > > > > > > > > In any case, this is a bit orthogonal, multi-line statements require > braces > > :) > > > > > > > > [1] https://sites.google.com/a/chromium.org/dev/developers/coding-style > > > > > > > > [2] > > > > > > > > > > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/bf385RfI_JQ/bON68... > > > > > > Benoit has already forced me to remove brackets in one-line if statement. So > > I'm > > > between two fires with you guys. > > > Done. > > > > Sorry for the fires :) > > > > I am with Benoit on one-line conditionals (for consistency), but this > statement > > did not fit one line. > > Yeah, I see that this is another case. > To summarize, no braces if the condition AND the statement fits in one line. > Otherwise, put braces. > It also depends on surrounding code, of course. not only that, there are more stupid rules regarding the 'else' :) in short: if each of the parts of the conditional statement take one line - no braces, otherwise braces for everything. Examples of good: if (foo) DoFoo() else if (bar) DoBar() if (foo) { DoFoo() } else if (Condition(multi, line)) { DoBar() }
Message was sent while issue was closed.
Committed patchset #7 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== predictors: Use redirect data in prefetch. The data about redirects now is used for populate subresource URLs in ResourcePrefetchPredictor::GetPrefetchData when main page doesn't have its own subresources. BUG=631966 ========== to ========== predictors: Use redirect data in prefetch. The data about redirects now is used for populate subresource URLs in ResourcePrefetchPredictor::GetPrefetchData when main page doesn't have its own subresources. BUG=631966 Committed: https://crrev.com/a5d0b1f24b9e4a3ae59081a33e83a9e9daf0d005 Cr-Commit-Position: refs/heads/master@{#424447} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/a5d0b1f24b9e4a3ae59081a33e83a9e9daf0d005 Cr-Commit-Position: refs/heads/master@{#424447} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
