Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2587443002:
predictors: Make speculative_prefetch_predictor work with PlzNavigate (Closed)
Descriptionpredictors: Make speculative_prefetch_predictor work with PlzNavigate
Summary
--------
We are now tracking navigations for the ResourcePrefetchPredictor
using the tab ids instead of render process and host. Added browser
tests to test edge cases that could break the tab_id usage.
Details
--------
- WebContents is now used to extract the session_id with help from
SessionTabHelper::IdForTab() instead of render_frame_host_id +
render_process_id
- Reworked unit_tests to use this session_id instead
- Reworked browser_tests to test for the following cases : Cross Domain
Navigation inducing cross process behavior and Navigating to new
tabs/window/popup.
BUG=650246
Committed: https://crrev.com/62729f8a7e996d8f6b0451e8bcfcd3a124fd2628
Cr-Commit-Position: refs/heads/master@{#440615}
Patch Set 1 #Patch Set 2 : Reworked Tests #
Total comments: 17
Patch Set 3 : Removed Frame Tree Node Id #
Total comments: 2
Patch Set 4 : From SessionID to SessionID::id_type #
Total comments: 28
Patch Set 5 : Updates after lizeb@ first review and rebase #
Total comments: 13
Patch Set 6 : Small header/namespaces fixes #Patch Set 7 : Updates after lizeb@ second review #
Total comments: 9
Patch Set 8 : Updates after alexilin@ review #
Total comments: 14
Patch Set 9 : Modified after alexilin@ and lizeb@ reviews #
Total comments: 9
Patch Set 10 : Modified after alexilin@ and lizeb@ reviews + rebase #Patch Set 11 : Removing very tiny unwanted change #Patch Set 12 : Added extra safety after lizeb@ offline remark #
Total comments: 6
Patch Set 13 : Modified after alexilin@ review #
Total comments: 7
Patch Set 14 : Modified after alexilin@ comment #Patch Set 15 : Modified after clamy@ review #Messages
Total messages: 89 (53 generated)
Description was changed from ========== Using tab_id + frame_tree_node_id BUG=631966 ========== to ========== Using tab_id + frame_tree_node_id Passing browser_tests and unit_tests with browser side navigation flag off and on BUG=631966 ==========
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
ahemery@chromium.org changed reviewers: + alexilin@chromium.org, lizeb@chromium.org
First draft for the final part of resource prefetch predictor and plzNavigate compatibility. https://codereview.chromium.org/2587443002/diff/60001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_common.cc (right): https://codereview.chromium.org/2587443002/diff/60001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_common.cc:106: return session_id.id() != -1 && !main_frame_url.is_empty(); frame_tree_node_id can be -1 and still be valid so not using it to make assumption about the state https://codereview.chromium.org/2587443002/diff/60001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_common.cc:111: // Quick int copy to be able to use tie. Tie uses lvalues https://codereview.chromium.org/2587443002/diff/60001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_common.h (right): https://codereview.chromium.org/2587443002/diff/60001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_common.h:51: bool IsSameLocation(const NavigationID& other) const; Simple renaming to be more accurate with the new ids https://codereview.chromium.org/2587443002/diff/60001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2587443002/diff/60001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:94: struct ResourceComparisonParameters { Just improving modularity in case we need to add other matchers https://codereview.chromium.org/2587443002/diff/60001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:178: for (const auto& resource : summary.subresource_requests) Tracking what NavigationID's have been used. Required to verify that tab_id has expected behavior. https://codereview.chromium.org/2587443002/diff/60001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:213: SetCurrentHostName("127.0.0.1"); Resolving everything to localhost to be able to use various hostnames https://codereview.chromium.org/2587443002/diff/60001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:255: for (auto& kv : resources_) { Copy of Alex modifications in https://codereview.chromium.org/2553083002/ to facilitate merging. Using his way to disable caching. https://codereview.chromium.org/2587443002/diff/60001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:259: for (const auto& nav : *observer.GetNavigationIds()) Aggregating every observer navigationID map into the browsertest object https://codereview.chromium.org/2587443002/diff/60001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:306: void ClearResources() { resources_.clear(); } Used for clearing resources before hostname change https://codereview.chromium.org/2587443002/diff/60001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:308: void ClearCache() { Copy of Alex modifications. See line 255 in this file https://codereview.chromium.org/2587443002/diff/60001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:342: void SetCurrentHostName(const std::string& new_host) { Will be used to resolve every URL using the getURL method https://codereview.chromium.org/2587443002/diff/60001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:346: unsigned int GetHistoryCount() { return navigation_id_history_.size(); } Returns the number of navigations that have been used in this test so far https://codereview.chromium.org/2587443002/diff/60001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:372: NavigationID(web_contents, main_frame_url, base::TimeTicks::Now()); Using constructor directly with web_contents. Forgot to update it in previous patch https://codereview.chromium.org/2587443002/diff/60001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:389: GURL resource_url = request.GetURL(); At this stage, URL has been resolved to 127.0.0.1, making the resource search fail. We are updating the hostname to match the ones we have stored. We avoid storing every resource with 127.0.0.1 in case we need to test cross site fetching further. https://codereview.chromium.org/2587443002/diff/60001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:458: switches::kSpeculativeResourcePrefetchingLearning); This is the critical line. Disables actual prefetching. This way we can have absolutely no caching, as the browser cache clear does not prevent the prefetch cache to be used. https://codereview.chromium.org/2587443002/diff/60001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:617: IN_PROC_BROWSER_TEST_F(ResourcePrefetchPredictorLearningBrowserTest, When we stay in the same tab, tab_id shouldn't be different but should be different if we change tab/window https://codereview.chromium.org/2587443002/diff/60001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor_test_util.h (right): https://codereview.chromium.org/2587443002/diff/60001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_test_util.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. Everything in this file was used during my debugging. Could be useful for other people to use. Let me know if you think this should be deleted.
Description was changed from ========== Using tab_id + frame_tree_node_id Passing browser_tests and unit_tests with browser side navigation flag off and on BUG=631966 ========== to ========== Using tab_id to identify navigations Summary -------- We are now using tracking navigations for the ResourcePrefetchPredictor using the tab ids instead of render process and host. Added browser tests to test edge cases that could break the tab_id usage. Details -------- - WebContents is now used to extract the session_id with help from SessionTabHelper::IdForTab() instead of render_frame_host_id + render_process_id - Reworked unit_tests to use this session_id instead - Reworked browser_tests to test for the following cases : Cross Domain Navigation inducing cross process behavior and Navigating to new tabs/window/popup. To implement the cases above we added these main features to browser testing : Ability to specify hosts and manage resources properly, cache/resources clearing for repeated navigations, tracking of every NavigationID used during a test for finer grain testing. BUG=631966 ==========
ahemery@chromium.org changed reviewers: + mattcary@chromium.org
The CQ bit was checked by ahemery@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...
Description was changed from ========== Using tab_id to identify navigations Summary -------- We are now using tracking navigations for the ResourcePrefetchPredictor using the tab ids instead of render process and host. Added browser tests to test edge cases that could break the tab_id usage. Details -------- - WebContents is now used to extract the session_id with help from SessionTabHelper::IdForTab() instead of render_frame_host_id + render_process_id - Reworked unit_tests to use this session_id instead - Reworked browser_tests to test for the following cases : Cross Domain Navigation inducing cross process behavior and Navigating to new tabs/window/popup. To implement the cases above we added these main features to browser testing : Ability to specify hosts and manage resources properly, cache/resources clearing for repeated navigations, tracking of every NavigationID used during a test for finer grain testing. BUG=631966 ========== to ========== Using tab_id to identify navigations Summary -------- We are now tracking navigations for the ResourcePrefetchPredictor using the tab ids instead of render process and host. Added browser tests to test edge cases that could break the tab_id usage. Details -------- - WebContents is now used to extract the session_id with help from SessionTabHelper::IdForTab() instead of render_frame_host_id + render_process_id - Reworked unit_tests to use this session_id instead - Reworked browser_tests to test for the following cases : Cross Domain Navigation inducing cross process behavior and Navigating to new tabs/window/popup. To implement the cases above we added these main features to browser testing : Ability to specify hosts and manage resources properly, cache/resources clearing for repeated navigations, tracking of every NavigationID used during a test for finer grain testing. BUG=631966 ==========
Thanks. One high-level comment, will require changes in a bunch of places, so I prefer to send it earlier. https://codereview.chromium.org/2587443002/diff/80001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_common.cc (right): https://codereview.chromium.org/2587443002/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_common.cc:91: session_id.set_id(SessionTabHelper::IdForTab(web_contents)); While the intent is clear here, it is still a bit strange to wrap the return value of this function instead of storing it directly. See the comment above. https://codereview.chromium.org/2587443002/diff/80001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_common.h (right): https://codereview.chromium.org/2587443002/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_common.h:56: SessionID session_id; Looking at the code of SessionID, merely constructing a SessionID increases the value of the next one (in the constructor). Can you store a SessionID::id_type instead? It is an int32_t, and I don't think we risk any wraparound issue here, but perhaps some code somewhere doesn't expect it to increase really fast. Keeping a SessionID here would create a new one for each request, at least. Also, it makes the construction (and copying) of NavigationID subtly not-threadsafe. Finally, it matches the return type of static SessionID::id_type IdForTab(const content::WebContents* tab);
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Definitely make more sense to have more of a "monitoring" behavior than interfering with the SessionID mechanisms. Implemented the change.
The CQ bit was checked by ahemery@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
Patchset #4 (id:100001) has been deleted
Thanks, some more comments. https://codereview.chromium.org/2587443002/diff/120001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2587443002/diff/120001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:94: struct ResourceComparisonParameters { Please remove this struct, it adds unnecessary boilerplate to the code. https://codereview.chromium.org/2587443002/diff/120001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:187: std::map<NavigationID, int>* GetNavigationIds() { From the Chromium C++ style guide: https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md "Simple accessors should generally be the only inline functions. These should be named unix_hacker_style()." Meaning that this should be: navigation_ids(). Also, I would expect this to be a set, from the naming. Perhaps resource_count_by_navigation_id? Finally, this can return a non-const reference. Please change the code to do so. https://codereview.chromium.org/2587443002/diff/120001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:341: void SetCurrentHostName(const std::string& new_host) { nit: set_current_host_name() https://codereview.chromium.org/2587443002/diff/120001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:345: unsigned int GetHistoryCount() { return navigation_id_history_.size(); } map::size() returns a size_t, not an unsigned int. https://codereview.chromium.org/2587443002/diff/120001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:448: // TODO(ahemery): Remove when https://codereview.chromium.org/2553083002/ You can rebase the patch now, as the CL mentioned here has landed. Also, please don't mention people by name. It's ambiguous, here alexilin@ would be better, or just drop it, as the CL number is enough. Also, crrev.com/something is quite useful. "Something" can be an issue number or commit hash. Doesn't make a difference of course, just nice to know. https://codereview.chromium.org/2587443002/diff/120001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_test_util.h (right): https://codereview.chromium.org/2587443002/diff/120001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_test_util.h:37: NavigationID CreateNavigationID(int session_id, SessionID is not an int, it's a SessionID::id_type. Please use this type. https://codereview.chromium.org/2587443002/diff/120001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_test_util.h:47: int tab_id, nit: There is session_id and tab_id in the code. It's better to use only one of them consistently, this reduces confusion. https://codereview.chromium.org/2587443002/diff/120001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_unittest.cc (right): https://codereview.chromium.org/2587443002/diff/120001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:192: int tab_id, As above, please fix the type and the variable name.
Also, the proper bug number for this CL is 650246. Since this bug is tracked by PlzNavigate people, it makes sense to use it.
Description was changed from ========== Using tab_id to identify navigations Summary -------- We are now tracking navigations for the ResourcePrefetchPredictor using the tab ids instead of render process and host. Added browser tests to test edge cases that could break the tab_id usage. Details -------- - WebContents is now used to extract the session_id with help from SessionTabHelper::IdForTab() instead of render_frame_host_id + render_process_id - Reworked unit_tests to use this session_id instead - Reworked browser_tests to test for the following cases : Cross Domain Navigation inducing cross process behavior and Navigating to new tabs/window/popup. To implement the cases above we added these main features to browser testing : Ability to specify hosts and manage resources properly, cache/resources clearing for repeated navigations, tracking of every NavigationID used during a test for finer grain testing. BUG=631966 ========== to ========== Using tab_id to identify navigations Summary -------- We are now tracking navigations for the ResourcePrefetchPredictor using the tab ids instead of render process and host. Added browser tests to test edge cases that could break the tab_id usage. Details -------- - WebContents is now used to extract the session_id with help from SessionTabHelper::IdForTab() instead of render_frame_host_id + render_process_id - Reworked unit_tests to use this session_id instead - Reworked browser_tests to test for the following cases : Cross Domain Navigation inducing cross process behavior and Navigating to new tabs/window/popup. To implement the cases above we added these main features to browser testing : Ability to specify hosts and manage resources properly, cache/resources clearing for repeated navigations, tracking of every NavigationID used during a test for finer grain testing. BUG=650246 ==========
https://codereview.chromium.org/2587443002/diff/120001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2587443002/diff/120001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:94: struct ResourceComparisonParameters { On 2016/12/20 09:53:39, Benoit L wrote: > Please remove this struct, it adds unnecessary boilerplate to the code. Removed https://codereview.chromium.org/2587443002/diff/120001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:187: std::map<NavigationID, int>* GetNavigationIds() { On 2016/12/20 09:53:39, Benoit L wrote: > From the Chromium C++ style guide: > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md > > "Simple accessors should generally be the only inline functions. These should be > named unix_hacker_style()." > > Meaning that this should be: > navigation_ids(). > > Also, I would expect this to be a set, from the naming. > Perhaps resource_count_by_navigation_id? > > Finally, this can return a non-const reference. Please change the code to do so. Renamed and updated types to be more precise https://codereview.chromium.org/2587443002/diff/120001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:215: base::FilePath(FILE_PATH_LITERAL("chrome/test/data"))); Removed as this was unnecessary https://codereview.chromium.org/2587443002/diff/120001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:341: void SetCurrentHostName(const std::string& new_host) { On 2016/12/20 09:53:39, Benoit L wrote: > nit: set_current_host_name() Done https://codereview.chromium.org/2587443002/diff/120001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:345: unsigned int GetHistoryCount() { return navigation_id_history_.size(); } On 2016/12/20 09:53:39, Benoit L wrote: > map::size() returns a size_t, not an unsigned int. Done https://codereview.chromium.org/2587443002/diff/120001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:448: // TODO(ahemery): Remove when https://codereview.chromium.org/2553083002/ On 2016/12/20 09:53:39, Benoit L wrote: > You can rebase the patch now, as the CL mentioned here has landed. > > Also, please don't mention people by name. It's ambiguous, here alexilin@ would > be better, or just drop it, as the CL number is enough. > > Also, crrev.com/something is quite useful. "Something" can be an issue number or > commit hash. Doesn't make a difference of course, just nice to know. Noted, will use logins instead from now on. Regarding the code itself, there was a confusion about what alexilin@ code was doing. This is still needed. https://codereview.chromium.org/2587443002/diff/120001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_test_util.h (right): https://codereview.chromium.org/2587443002/diff/120001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_test_util.h:37: NavigationID CreateNavigationID(int session_id, On 2016/12/20 09:53:39, Benoit L wrote: > SessionID is not an int, it's a SessionID::id_type. Please use this type. Updated with proper type https://codereview.chromium.org/2587443002/diff/120001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_test_util.h:47: int tab_id, On 2016/12/20 09:53:39, Benoit L wrote: > nit: There is session_id and tab_id in the code. It's better to use only one of > them consistently, this reduces confusion. Everything unified to session_id https://codereview.chromium.org/2587443002/diff/120001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_unittest.cc (right): https://codereview.chromium.org/2587443002/diff/120001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:192: int tab_id, On 2016/12/20 09:53:39, Benoit L wrote: > As above, please fix the type and the variable name. Done
The CQ bit was checked by ahemery@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/2587443002/diff/140001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_common.h (right): https://codereview.chromium.org/2587443002/diff/140001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_common.h:11: #include "chrome/browser/sessions/session_tab_helper.h" nit: please move the header file inclusion to the .cc file, not the .h. https://codereview.chromium.org/2587443002/diff/140001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2587443002/diff/140001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:184: std::set<NavigationID>& navigation_ids() { return current_navigation_ids_; } nit: This is not enforced by the style guide, but I prefer it when the accessor has the same name as the member variable (minus the trailing _). https://codereview.chromium.org/2587443002/diff/140001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:236: host_resolver()->AddRule("*", "127.0.0.1"); nit: Can you add a comment saying why this is needed? I guess it's for multi-origin loads, right? https://codereview.chromium.org/2587443002/diff/140001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:389: std::size_t get_navigation_ids_history_size() { nit: getters don't have the initial "get_" in the name. https://codereview.chromium.org/2587443002/diff/140001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:494: // This browser test override is specifically used to have ONLY What is the issue here? https://codereview.chromium.org/2587443002/diff/140001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:560: // TODO(alexilin): Test learning and prefetching once crbug.com/650246 is Since this is the bug you are fixing, can you see how to address the TODO at the same time?
https://codereview.chromium.org/2587443002/diff/140001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2587443002/diff/140001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:184: std::set<NavigationID>& navigation_ids() { return current_navigation_ids_; } On 2016/12/20 13:20:49, Benoit L wrote: > nit: This is not enforced by the style guide, but I prefer it when the accessor > has the same name as the member variable (minus the trailing _). Done. https://codereview.chromium.org/2587443002/diff/140001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:236: host_resolver()->AddRule("*", "127.0.0.1"); On 2016/12/20 13:20:49, Benoit L wrote: > nit: Can you add a comment saying why this is needed? > I guess it's for multi-origin loads, right? Done. https://codereview.chromium.org/2587443002/diff/140001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:389: std::size_t get_navigation_ids_history_size() { On 2016/12/20 13:20:49, Benoit L wrote: > nit: getters don't have the initial "get_" in the name. Done. https://codereview.chromium.org/2587443002/diff/140001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:494: // This browser test override is specifically used to have ONLY On 2016/12/20 13:20:49, Benoit L wrote: > What is the issue here? After more than 2 calls to NavigateToURLAndCheckSubresources(...) the predictor actually starts prefetching the resources, tagging them as "was_cached = true" and causing mismatches in the subsequent calls. Alternative would be another boolean match_caching_information. We would still need ClearCache() calls because server caching is trickier (fills in mime type, etc.) https://codereview.chromium.org/2587443002/diff/140001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:560: // TODO(alexilin): Test learning and prefetching once crbug.com/650246 is On 2016/12/20 13:20:49, Benoit L wrote: > Since this is the bug you are fixing, can you see how to address the TODO at the > same time? Seen with alexilin@, removed the comment and fixed the test.
Thanks. I'm a little bit behind so I send you comments based on 4th patch set. https://codereview.chromium.org/2587443002/diff/120001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_common.h (right): https://codereview.chromium.org/2587443002/diff/120001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_common.h:11: #include "chrome/browser/sessions/session_tab_helper.h" Do we really need this here? Would you move it to .cc file? https://codereview.chromium.org/2587443002/diff/120001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2587443002/diff/120001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:214: embedded_test_server()->AddDefaultHandlers( It is not necessary to do. This is already done in InProcessBrowserTest constructor https://cs.chromium.org/chromium/src/chrome/test/base/in_process_browser_test... https://codereview.chromium.org/2587443002/diff/120001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:315: return embedded_test_server()->GetURL(current_host_name_, path); While I find that the ability to set arbitrary host for a request is useful, I also think that we need to change the approach. Because, for example, it won't work with https server. You will have to pass the right host name along all https_server()->GetURL() calls. It works now because in that test we use default host but if you add SetCurrentHostName("x.y") in the beginning of the test it is broken. Moreover it adds unobvious hidden dependencies: GURL a = GetURL("/a"); SetCurrentHostName("x.y"); GURL b = GetURL("/a"); EXPECT_EQ(a, b); // fails I would suggest you to have immutable host for each embedded_test_server instance. That is if you want to have different host for the server you just create a new one and pass desired host string into the constructor. You could modify existing EnableHttpsServer() function to be a factory method for new servers. You also could add host name parameter to HandleRedirectRequest/HandleResourceRequest and bind it to the value passed into the constructor. Then CrossSiteLearning test will look like that (perhaps you can come up with a better idea for getting url): std::unique_ptr<net::EmbeddedTestServer> foo_server = CreateAndRunTestServer("foo.com", net::EmbeddedTestServer::TYPE_HTTP); // add resources NavigateToURLAndCheckSubresources(foo_server->GetURL("foo.com", kHtmlSubresourcesPath)); ClearResources(); std::unique_ptr<net::EmbeddedTestServer> bar_server = CreateAndRunTestServer("bar.com", net::EmbeddedTestServer::TYPE_HTTP); // add resources NavigateToURLAndCheckSubresources(bar_server->GetURL("bar.com", kHtmlSubresourcesPath)); https://codereview.chromium.org/2587443002/diff/120001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:388: GURL resource_url = request.GetURL(); It would be better to extract this to GURL ReplaceHost(const GURL& x) function https://codereview.chromium.org/2587443002/diff/120001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:617: TabIdBehavingAsExpected) { Session id? https://codereview.chromium.org/2587443002/diff/120001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:631: WindowOpenDisposition::NEW_FOREGROUND_TAB); It would be worth to add some comment about and what we are testing here what behavior we are expecting. https://codereview.chromium.org/2587443002/diff/120001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_test_util.h (right): https://codereview.chromium.org/2587443002/diff/120001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_test_util.h:47: int tab_id, The same remark about SessionID::id_type.
The CQ bit was checked by ahemery@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...
Some additional nits (very poor). https://codereview.chromium.org/2587443002/diff/180001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2587443002/diff/180001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:240: // cross domains navigations (matching url_visit_count_, etc) nit: add period to the end of the sentence. https://codereview.chromium.org/2587443002/diff/180001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:498: // the learning part of the prefetcher without the actual prefetching nit: periods as well. https://codereview.chromium.org/2587443002/diff/180001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:562: TestLearningAndPrefetching(GetURL(kRedirectPath)); It's great that this test works now!
Investigating on the host features to see how do it as clean and safe as possible. https://codereview.chromium.org/2587443002/diff/120001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_common.h (right): https://codereview.chromium.org/2587443002/diff/120001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_common.h:11: #include "chrome/browser/sessions/session_tab_helper.h" On 2016/12/20 14:06:47, alexilin wrote: > Do we really need this here? > Would you move it to .cc file? Done in patch 6 https://codereview.chromium.org/2587443002/diff/120001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2587443002/diff/120001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:214: embedded_test_server()->AddDefaultHandlers( On 2016/12/20 14:06:47, alexilin wrote: > It is not necessary to do. > This is already done in InProcessBrowserTest constructor > https://cs.chromium.org/chromium/src/chrome/test/base/in_process_browser_test... Done in patch 5 https://codereview.chromium.org/2587443002/diff/120001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:617: TabIdBehavingAsExpected) { On 2016/12/20 14:06:47, alexilin wrote: > Session id? Done. https://codereview.chromium.org/2587443002/diff/120001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:631: WindowOpenDisposition::NEW_FOREGROUND_TAB); On 2016/12/20 14:06:47, alexilin wrote: > It would be worth to add some comment about and what we are testing here what > behavior we are expecting. Done. https://codereview.chromium.org/2587443002/diff/180001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2587443002/diff/180001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:240: // cross domains navigations (matching url_visit_count_, etc) On 2016/12/20 14:31:44, alexilin wrote: > nit: add period to the end of the sentence. Done. https://codereview.chromium.org/2587443002/diff/180001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:498: // the learning part of the prefetcher without the actual prefetching On 2016/12/20 14:31:44, alexilin wrote: > nit: periods as well. Done. https://codereview.chromium.org/2587443002/diff/180001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:562: TestLearningAndPrefetching(GetURL(kRedirectPath)); On 2016/12/20 14:31:44, alexilin wrote: > It's great that this test works now! Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I have one more thought about the host feature.
We actually can use the value of the Host header in HandleResourceRequest to
determine the actual host.
It could be obtained by request.headers.at("Host") (don't forget to trim port).
So we just will have to use the embedded_test_server()->GetURL(const
std::string& hostname, const std::string& relative_url) overload for defining
resources and main page if we want to specify host.
It would be much simpler than my initial proposal.
The CQ bit was checked by ahemery@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 ahemery@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...
On 2016/12/20 16:19:53, alexilin wrote:
> I have one more thought about the host feature.
>
> We actually can use the value of the Host header in HandleResourceRequest to
> determine the actual host.
> It could be obtained by request.headers.at("Host") (don't forget to trim
port).
> So we just will have to use the embedded_test_server()->GetURL(const
> std::string& hostname, const std::string& relative_url) overload for defining
> resources and main page if we want to specify host.
> It would be much simpler than my initial proposal.
Implemented as suggested here.
https://codereview.chromium.org/2587443002/diff/140001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2587443002/diff/140001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:494: // This browser test override is specifically used to have ONLY On 2016/12/20 14:06:35, ahemery wrote: > On 2016/12/20 13:20:49, Benoit L wrote: > > What is the issue here? > > After more than 2 calls to NavigateToURLAndCheckSubresources(...) the predictor > actually starts prefetching the resources, tagging them as "was_cached = true" > and causing mismatches in the subsequent calls. Alternative would be another > boolean match_caching_information. We would still need ClearCache() calls > because server caching is trickier (fills in mime type, etc.) Not is you set the flags to "external only". This flag can be set everywhere, making the code simpler. https://codereview.chromium.org/2587443002/diff/200001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2587443002/diff/200001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:101: void ReplaceHost(GURL* gurl, const std::string& host) { nit: This is used in one place only, please inline it. https://codereview.chromium.org/2587443002/diff/200001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:401: size_t navigation_ids_history_size() { return navigation_id_history_.size(); } nit: make the function const. https://codereview.chromium.org/2587443002/diff/200001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:447: std::string host = request.headers.at("Host"); GURL has has_port() and port(). Also, why using .at() here?
lizeb@chromium.org changed reviewers: + clamy@chromium.org
+R: clamy. @clamy: Don't review this yet, but I will wait for your OK before approving this CL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2587443002/diff/140001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2587443002/diff/140001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:494: // This browser test override is specifically used to have ONLY On 2016/12/21 12:38:25, Benoit L wrote: > On 2016/12/20 14:06:35, ahemery wrote: > > On 2016/12/20 13:20:49, Benoit L wrote: > > > What is the issue here? > > > > After more than 2 calls to NavigateToURLAndCheckSubresources(...) the > predictor > > actually starts prefetching the resources, tagging them as "was_cached = true" > > and causing mismatches in the subsequent calls. Alternative would be another > > boolean match_caching_information. We would still need ClearCache() calls > > because server caching is trickier (fills in mime type, etc.) > > Not is you set the flags to "external only". This flag can be set everywhere, > making the code simpler. To clarify: are you saying that we can change this switch directly in the test code? I'm not sure that this is true, because we have to set it before the ResourcePrefetchPredictor creation. https://codereview.chromium.org/2587443002/diff/180001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2587443002/diff/180001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:280: bool match_navigation_id = true; bool match_navigation_id = disposition == WindowOpenDisposition::CURRENT_TAB; https://codereview.chromium.org/2587443002/diff/200001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2587443002/diff/200001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:101: void ReplaceHost(GURL* gurl, const std::string& host) { On 2016/12/21 12:38:26, Benoit L wrote: > nit: This is used in one place only, please inline it. Actually, it was my suggestion. This is copypaste of EmbeddedTestServer::GetURL(host, path) function and I think it would be clearer to use our function to replace host. (See comment below). But this is actually nit. Btw, shouldn't we extract self-contained pieces of code even it is used only in one place? https://codereview.chromium.org/2587443002/diff/200001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:374: GURL GetURL(const std::string& path, const std::string& host) const { nit: Would it be better to use ReplaceHost for this purpose? We could do something like that: AddResource(ReplaceHost(GetURL(kImagePath), "bar.com"), content::RESOURCE_TYPE_IMAGE, net::LOWEST); But I'm also ok with the current implementation. The only thing that bothers me is that EmbeddedTestServer::GetURL(host, path) and ReplaceHost do the same thing (no matter if ReplaceHost is inlined or not) https://codereview.chromium.org/2587443002/diff/200001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:447: std::string host = request.headers.at("Host"); Consider to use HostPortPair https://cs.chromium.org/chromium/src/net/base/host_port_pair.h?dr=C&q=HostPor... https://codereview.chromium.org/2587443002/diff/200001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:508: // the learning part of the prefetcher without the actual prefetching period here as well because this is separate sentence https://codereview.chromium.org/2587443002/diff/200001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:696: // Checking that navigation to a new window yields the same result as There is no need to repeat the same things several times, I think. Feel free to just move this description on a top of the function. (Before IN_PROC_BROWSER_TEST_F)
https://codereview.chromium.org/2587443002/diff/200001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2587443002/diff/200001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:101: void ReplaceHost(GURL* gurl, const std::string& host) { On 2016/12/21 13:08:12, alexilin wrote: > On 2016/12/21 12:38:26, Benoit L wrote: > > nit: This is used in one place only, please inline it. > > Actually, it was my suggestion. > This is copypaste of EmbeddedTestServer::GetURL(host, path) function and I think > it would be clearer to use our function to replace host. (See comment below). > But this is actually nit. > > Btw, shouldn't we extract self-contained pieces of code even it is used only in > one place? Un-inlined. Also as discussed with Alex, we still need this to avoid replacing the port on top of the host for HTTPS tests https://codereview.chromium.org/2587443002/diff/200001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:374: GURL GetURL(const std::string& path, const std::string& host) const { On 2016/12/21 13:08:12, alexilin wrote: > nit: Would it be better to use ReplaceHost for this purpose? > We could do something like that: > > AddResource(ReplaceHost(GetURL(kImagePath), "bar.com"), > content::RESOURCE_TYPE_IMAGE, net::LOWEST); > > But I'm also ok with the current implementation. The only thing that bothers me > is that EmbeddedTestServer::GetURL(host, path) and ReplaceHost do the same thing > (no matter if ReplaceHost is inlined or not) Removed the one with more parameters as it was not really necessary and confusing. Calling embedded_test_server() directly when dealing with custom hosts. https://codereview.chromium.org/2587443002/diff/200001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:401: size_t navigation_ids_history_size() { return navigation_id_history_.size(); } On 2016/12/21 12:38:25, Benoit L wrote: > nit: make the function const. Done. https://codereview.chromium.org/2587443002/diff/200001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:447: std::string host = request.headers.at("Host"); On 2016/12/21 12:38:25, Benoit L wrote: > GURL has has_port() and port(). > Also, why using .at() here? Removed the manual work and used the structure suggested by Alex as it seems to be the one with minimal overhead. We are using .at() because the underlying map is const, de facto disabling operator[]. Let me know if error handling here is acceptable. https://codereview.chromium.org/2587443002/diff/200001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:508: // the learning part of the prefetcher without the actual prefetching On 2016/12/21 13:08:12, alexilin wrote: > period here as well because this is separate sentence Done. https://codereview.chromium.org/2587443002/diff/200001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:696: // Checking that navigation to a new window yields the same result as On 2016/12/21 13:08:12, alexilin wrote: > There is no need to repeat the same things several times, I think. Feel free to > just move this description on a top of the function. (Before > IN_PROC_BROWSER_TEST_F) Condensed on top of the function
Almost there, only really minor things remaining. After this round, it should be fine for clamy@ to take a look at it, to make sure that the approach doesn't leave too many dragons behind. https://codereview.chromium.org/2587443002/diff/220001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_common.h (right): https://codereview.chromium.org/2587443002/diff/220001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_common.h:55: SessionID::id_type session_id; Sorry for spotting that so late, but SessionID is used in a bunch of places, and can identify either a window or a tab. Can you rename this tab_id? https://codereview.chromium.org/2587443002/diff/220001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2587443002/diff/220001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:505: // Used to avoid having caching issues for matcher. Is this still necessary? (per the offline discussion about "external only" prefetching).
A few more high-level things: - Please remove the suppression from testing/buildbot/filters/browser-side-navigation.linux.browser_tests.filter. This patch should fix the issues, and removing the suppression as part of the CL allows us to check that on the CQ. - This is really minor, but commit message lines should be no longer than 72 or 80 char, depending on your tastes. Also, please change the first line to: predictors: Make speculative_prefetch_predictor work with PlzNavigate. The idea is that the first line should tell which component this CL refers to, and what it does, not how. That way, when people are looking for regression or for something that changed some behavior, they have an easier time figuring out what's going on.
Description was changed from ========== Using tab_id to identify navigations Summary -------- We are now tracking navigations for the ResourcePrefetchPredictor using the tab ids instead of render process and host. Added browser tests to test edge cases that could break the tab_id usage. Details -------- - WebContents is now used to extract the session_id with help from SessionTabHelper::IdForTab() instead of render_frame_host_id + render_process_id - Reworked unit_tests to use this session_id instead - Reworked browser_tests to test for the following cases : Cross Domain Navigation inducing cross process behavior and Navigating to new tabs/window/popup. To implement the cases above we added these main features to browser testing : Ability to specify hosts and manage resources properly, cache/resources clearing for repeated navigations, tracking of every NavigationID used during a test for finer grain testing. BUG=650246 ========== to ========== ResourcePrefetchPredictor uses tab_id for tracking navigations Summary -------- We are now tracking navigations for the ResourcePrefetchPredictor using the tab ids instead of render process and host. Added browser tests to test edge cases that could break the tab_id usage. Details -------- - WebContents is now used to extract the session_id with help from SessionTabHelper::IdForTab() instead of render_frame_host_id + render_process_id - Reworked unit_tests to use this session_id instead - Reworked browser_tests to test for the following cases : Cross Domain Navigation inducing cross process behavior and Navigating to new tabs/window/popup. BUG=650246 ==========
Yeah, we are almost there! https://codereview.chromium.org/2587443002/diff/180001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2587443002/diff/180001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:280: bool match_navigation_id = true; On 2016/12/21 13:08:12, alexilin wrote: > bool match_navigation_id = disposition == WindowOpenDisposition::CURRENT_TAB; Still unresolved https://codereview.chromium.org/2587443002/diff/220001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2587443002/diff/220001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:438: // Uses HostPortPair to ignore the port part (to avoid HTTP/HTTPS issues). I didn't get this part about avoiding HTTP/HTTPS issues. What did you mean? We must trim the port part just because we can't pass a host with a port in the replace_host.SetHostStr() function. https://codereview.chromium.org/2587443002/diff/220001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:446: ADD_FAILURE() << "Host header was not found in a HttpRequest"; It would be also useful to include request.GetURL().spec() into the failure message. https://codereview.chromium.org/2587443002/diff/220001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:505: // Used to avoid having caching issues for matcher. On 2016/12/22 10:10:12, Benoit L wrote: > Is this still necessary? (per the offline discussion about "external only" > prefetching). It has to be done after I land the CL http://crrev.com/2596893002/ because without it the tests will become really flaky.
Description was changed from ========== ResourcePrefetchPredictor uses tab_id for tracking navigations Summary -------- We are now tracking navigations for the ResourcePrefetchPredictor using the tab ids instead of render process and host. Added browser tests to test edge cases that could break the tab_id usage. Details -------- - WebContents is now used to extract the session_id with help from SessionTabHelper::IdForTab() instead of render_frame_host_id + render_process_id - Reworked unit_tests to use this session_id instead - Reworked browser_tests to test for the following cases : Cross Domain Navigation inducing cross process behavior and Navigating to new tabs/window/popup. BUG=650246 ========== to ========== ResourcePrefetchPredictor uses tab_id for tracking navigations Summary -------- We are now tracking navigations for the ResourcePrefetchPredictor using the tab ids instead of render process and host. Added browser tests to test edge cases that could break the tab_id usage. Details -------- - WebContents is now used to extract the session_id with help from SessionTabHelper::IdForTab() instead of render_frame_host_id + render_process_id - Reworked unit_tests to use this session_id instead - Reworked browser_tests to test for the following cases : Cross Domain Navigation inducing cross process behavior and Navigating to new tabs/window/popup. BUG=650246 ==========
Description was changed from ========== ResourcePrefetchPredictor uses tab_id for tracking navigations Summary -------- We are now tracking navigations for the ResourcePrefetchPredictor using the tab ids instead of render process and host. Added browser tests to test edge cases that could break the tab_id usage. Details -------- - WebContents is now used to extract the session_id with help from SessionTabHelper::IdForTab() instead of render_frame_host_id + render_process_id - Reworked unit_tests to use this session_id instead - Reworked browser_tests to test for the following cases : Cross Domain Navigation inducing cross process behavior and Navigating to new tabs/window/popup. BUG=650246 ========== to ========== predictors: Make speculative_prefetch_predictor work with PlzNavigate Summary -------- We are now tracking navigations for the ResourcePrefetchPredictor using the tab ids instead of render process and host. Added browser tests to test edge cases that could break the tab_id usage. Details -------- - WebContents is now used to extract the session_id with help from SessionTabHelper::IdForTab() instead of render_frame_host_id + render_process_id - Reworked unit_tests to use this session_id instead - Reworked browser_tests to test for the following cases : Cross Domain Navigation inducing cross process behavior and Navigating to new tabs/window/popup. BUG=650246 ==========
The CQ bit was checked by ahemery@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...
Corrected all the remaining flaws pointed out, rebased off alexilin@ synchronous cache clearing modification and removed test filter to enable dry runs.
https://codereview.chromium.org/2587443002/diff/180001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2587443002/diff/180001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:280: bool match_navigation_id = true; On 2016/12/22 10:32:29, alexilin wrote: > On 2016/12/21 13:08:12, alexilin wrote: > > bool match_navigation_id = disposition == WindowOpenDisposition::CURRENT_TAB; > > Still unresolved Done. Sorry missed this one https://codereview.chromium.org/2587443002/diff/220001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_common.h (right): https://codereview.chromium.org/2587443002/diff/220001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_common.h:55: SessionID::id_type session_id; On 2016/12/22 10:10:12, Benoit L wrote: > Sorry for spotting that so late, but SessionID is used in a bunch of places, and > can identify either a window or a tab. Can you rename this tab_id? Done. https://codereview.chromium.org/2587443002/diff/220001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2587443002/diff/220001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:438: // Uses HostPortPair to ignore the port part (to avoid HTTP/HTTPS issues). On 2016/12/22 10:32:29, alexilin wrote: > I didn't get this part about avoiding HTTP/HTTPS issues. What did you mean? > We must trim the port part just because we can't pass a host with a port in the > replace_host.SetHostStr() function. My mistake, will rephrase the comment. The idea was that we cannot use server()->GetURL because it will cause port replacement in case of HTTP/HTPS crossing. Therefore we have to do the replacement by hand. https://codereview.chromium.org/2587443002/diff/220001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:446: ADD_FAILURE() << "Host header was not found in a HttpRequest"; On 2016/12/22 10:32:29, alexilin wrote: > It would be also useful to include request.GetURL().spec() into the failure > message. Done. https://codereview.chromium.org/2587443002/diff/220001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:505: // Used to avoid having caching issues for matcher. On 2016/12/22 10:32:29, alexilin wrote: > On 2016/12/22 10:10:12, Benoit L wrote: > > Is this still necessary? (per the offline discussion about "external only" > > prefetching). > > It has to be done after I land the CL http://crrev.com/2596893002/ because > without it the tests will become really flaky. Rebasing off Alex changes removes the need for this class indeed.
The CQ bit was checked by ahemery@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: This issue passed the CQ dry run.
https://codereview.chromium.org/2587443002/diff/280001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2587443002/diff/280001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:468: // The actual replacement cannot be done using GetURL() from the server Well, the real thing is that we don't have a reference to the server that handles this request. We don't assume that this handler will be used only with the default embedded_test_server() instance. See http://crrev.com/2545483002 for some background. My suggestion is to delete this comment at all. If you wish you could add top comment to this function to clarify that this is callback called from a different thread by some test server (as well as HandleRedirectRequest). https://codereview.chromium.org/2587443002/diff/280001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:662: AddResource(embedded_test_server()->GetURL("foo.com", kImagePath), tiny nit: "foo.com" could be declared as const as well as "bar.com". It would be more consistent with the surrounding code. https://codereview.chromium.org/2587443002/diff/280001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:673: It probably worth to explain here that we are going to navigate to the different host from *the same tab*.
https://codereview.chromium.org/2587443002/diff/280001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2587443002/diff/280001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:468: // The actual replacement cannot be done using GetURL() from the server On 2016/12/22 14:13:56, alexilin wrote: > Well, the real thing is that we don't have a reference to the server that > handles this request. We don't assume that this handler will be used only with > the default embedded_test_server() instance. > See http://crrev.com/2545483002 for some background. > > My suggestion is to delete this comment at all. If you wish you could add top > comment to this function to clarify that this is callback called from a > different thread by some test server (as well as HandleRedirectRequest). Removed altogether. https://codereview.chromium.org/2587443002/diff/280001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:662: AddResource(embedded_test_server()->GetURL("foo.com", kImagePath), On 2016/12/22 14:13:56, alexilin wrote: > tiny nit: > "foo.com" could be declared as const as well as "bar.com". It would be more > consistent with the surrounding code. Done. https://codereview.chromium.org/2587443002/diff/280001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:673: On 2016/12/22 14:13:56, alexilin wrote: > It probably worth to explain here that we are going to navigate to the different > host from *the same tab*. Done.
The CQ bit was checked by ahemery@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, I'm OK with the patch now. @clamy: Can you take a look at the approach? The idea is that to identify subresource fetches (from the main frame only), the code used render_frame_id + render_process_id + first_party_for_cookies. Now it uses a WebContentsGetter to get a WebContents, and then a tab_id from the WebContents. If there is no WebContents or tab_id, then don't record the resource load. Thanks!
https://codereview.chromium.org/2587443002/diff/300001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2587443002/diff/300001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:684: // process because of cross domain behavior of PlzNavigate. I think that the reasoning isn't quite correct. The creation of a new process isn't connected with PlzNavigate, this is rather a part of the site-per-process policy described here: http://www.chromium.org/developers/design-documents/site-isolation So I would suggest to paraphrase this as "Navigating to kBarHost, although done in the same tab, will generate a new process because of the site-per-process policy." Or just "Navigating to kBarHost, although done in the same tab, will generate a new process."
The CQ bit was checked by ahemery@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...
Done
Thanks! A few comments and questions below. https://codereview.chromium.org/2587443002/diff/300001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_common.cc (right): https://codereview.chromium.org/2587443002/diff/300001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_common.cc:87: main_frame_url(web_contents->GetURL()), Sanity check: this URL is unlikely to be the one we're navigating to. Is that expected? https://codereview.chromium.org/2587443002/diff/300001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_common.h (right): https://codereview.chromium.org/2587443002/diff/300001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_common.h:52: // frame_url_ has been set correctly. This comment needs to be updated since render_process_id_ and render_frame_id_ no longer exist. https://codereview.chromium.org/2587443002/diff/300001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_common.h:55: SessionID::id_type tab_id; How do you deal with the two concurrent navigation in the main frame case? They would have been distinguished by the (render_process_id, render_frame_id) before. IIUC the predictor code correctly, you are only listening for events on the IO thread by observing the ResourceDispatcherHostDelegate. With PlzNavigate enabled, it is not possible to know which of two concurrent navigations will commit only by listening to IO thread events. You need to listen to WebContentsObserver events on the UI thread. While we're talking about special cases in navigation, how do you handle client-side redirects? Are they coalesced with the previous navigation or treated as a different navigation? IMO there's a rationale for saying they should be coalesced with the previous navigation.
https://codereview.chromium.org/2587443002/diff/300001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_common.h (right): https://codereview.chromium.org/2587443002/diff/300001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_common.h:55: SessionID::id_type tab_id; On 2016/12/22 15:52:41, clamy wrote: > How do you deal with the two concurrent navigation in the main frame case? They > would have been distinguished by the (render_process_id, render_frame_id) > before. IIUC the predictor code correctly, you are only listening for events on > the IO thread by observing the ResourceDispatcherHostDelegate. With PlzNavigate > enabled, it is not possible to know which of two concurrent navigations will > commit only by listening to IO thread events. You need to listen to > WebContentsObserver events on the UI thread. > > While we're talking about special cases in navigation, how do you handle > client-side redirects? Are they coalesced with the previous navigation or > treated as a different navigation? IMO there's a rationale for saying they > should be coalesced with the previous navigation. Per offline discussion: - The NavigationID matches not only on the IDs, so this should be fine. - Client-side redirects are not handled yet, but that's a nice addition for a future patch, thanks for letting us know.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2587443002/diff/300001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_common.cc (right): https://codereview.chromium.org/2587443002/diff/300001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_common.cc:87: main_frame_url(web_contents->GetURL()), On 2016/12/22 15:52:41, clamy wrote: > Sanity check: this URL is unlikely to be the one we're navigating to. Is that > expected? Modified to GetLastCommittedURL to reduce risk, as discussed offline. https://codereview.chromium.org/2587443002/diff/300001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_common.h (right): https://codereview.chromium.org/2587443002/diff/300001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_common.h:52: // frame_url_ has been set correctly. On 2016/12/22 15:52:41, clamy wrote: > This comment needs to be updated since render_process_id_ and render_frame_id_ > no longer exist. Done.
The CQ bit was checked by ahemery@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, thanks!
lgtm for testing/buildbot/filters/browser-side-navigation.linux.browser_tests.filter and chrome/browser/net Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/12/22 18:04:38, jam wrote: > lgtm for > testing/buildbot/filters/browser-side-navigation.linux.browser_tests.filter and > chrome/browser/net > Thanks! lgtm, thanks.
The CQ bit was checked by ahemery@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 340001, "attempt_start_ts": 1482488951059980,
"parent_rev": "85363f76ff75573dff1dfcc7029b06dc6c244eb3", "commit_rev":
"c8814a1ddb5780917d450280e8cdfecf423eb1a5"}
Message was sent while issue was closed.
Description was changed from ========== predictors: Make speculative_prefetch_predictor work with PlzNavigate Summary -------- We are now tracking navigations for the ResourcePrefetchPredictor using the tab ids instead of render process and host. Added browser tests to test edge cases that could break the tab_id usage. Details -------- - WebContents is now used to extract the session_id with help from SessionTabHelper::IdForTab() instead of render_frame_host_id + render_process_id - Reworked unit_tests to use this session_id instead - Reworked browser_tests to test for the following cases : Cross Domain Navigation inducing cross process behavior and Navigating to new tabs/window/popup. BUG=650246 ========== to ========== predictors: Make speculative_prefetch_predictor work with PlzNavigate Summary -------- We are now tracking navigations for the ResourcePrefetchPredictor using the tab ids instead of render process and host. Added browser tests to test edge cases that could break the tab_id usage. Details -------- - WebContents is now used to extract the session_id with help from SessionTabHelper::IdForTab() instead of render_frame_host_id + render_process_id - Reworked unit_tests to use this session_id instead - Reworked browser_tests to test for the following cases : Cross Domain Navigation inducing cross process behavior and Navigating to new tabs/window/popup. BUG=650246 Review-Url: https://codereview.chromium.org/2587443002 ==========
Message was sent while issue was closed.
Committed patchset #15 (id:340001)
Message was sent while issue was closed.
Description was changed from ========== predictors: Make speculative_prefetch_predictor work with PlzNavigate Summary -------- We are now tracking navigations for the ResourcePrefetchPredictor using the tab ids instead of render process and host. Added browser tests to test edge cases that could break the tab_id usage. Details -------- - WebContents is now used to extract the session_id with help from SessionTabHelper::IdForTab() instead of render_frame_host_id + render_process_id - Reworked unit_tests to use this session_id instead - Reworked browser_tests to test for the following cases : Cross Domain Navigation inducing cross process behavior and Navigating to new tabs/window/popup. BUG=650246 Review-Url: https://codereview.chromium.org/2587443002 ========== to ========== predictors: Make speculative_prefetch_predictor work with PlzNavigate Summary -------- We are now tracking navigations for the ResourcePrefetchPredictor using the tab ids instead of render process and host. Added browser tests to test edge cases that could break the tab_id usage. Details -------- - WebContents is now used to extract the session_id with help from SessionTabHelper::IdForTab() instead of render_frame_host_id + render_process_id - Reworked unit_tests to use this session_id instead - Reworked browser_tests to test for the following cases : Cross Domain Navigation inducing cross process behavior and Navigating to new tabs/window/popup. BUG=650246 Committed: https://crrev.com/62729f8a7e996d8f6b0451e8bcfcd3a124fd2628 Cr-Commit-Position: refs/heads/master@{#440615} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/62729f8a7e996d8f6b0451e8bcfcd3a124fd2628 Cr-Commit-Position: refs/heads/master@{#440615} |
