Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
338633003:
Prerender Instant search base url on omnibox focus event (Closed)
|
Created:
6 years, 6 months ago by sidharthms Modified:
6 years, 6 months ago CC:
chromium-reviews, skanuj+watch_chromium.org, melevin+watch_chromium.org, dhollowa+watch_chromium.org, dougw+watch_chromium.org, donnd+watch_chromium.org, dominich, jfweitz+watch_chromium.org, David Black, samarth+watch_chromium.org, kmadhusu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionPrerender Instant search base url on omnibox focus event.
Prerenders the Instant search base page URL on omnibox focus event provided the
underlying page is not a search results page and not in incognito profile.
Prerender request for the current tab is cancelled when a navigation entry is
committed or when the current tab is deactivated.
BUG=384106
TEST=Run canary chrome with the following flags:
--prefetch-search-results --prerender=enabled --force-fieldtrials="EmbeddedSearch/Group1 prerender_instant_url_on_omnibox_focus:1"
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278731
Patch Set 1 #Patch Set 2 : Rebase on master #
Total comments: 2
Patch Set 3 : Comments addressed #
Total comments: 4
Patch Set 4 : Update to handle corner case #
Total comments: 4
Patch Set 5 : Comments addressed #Patch Set 6 : Omnibox check added #
Total comments: 6
Patch Set 7 : Comments addressed #
Total comments: 2
Patch Set 8 : Active Navgiation check added #Messages
Total messages: 19 (0 generated)
On 2014/06/14 00:22:10, sidharthms wrote: +jered@ to the reviewers list. I tested your changes. As of now, we init InstantSearchPrerenderer only on the receipt of a focus change event and hence we are not prerendering the InstantSearchPrerenderer when the omnibox receives its first focus or when a new tab is created, etc. Please fix it and also add a test to verify this behavior. Thanks.
https://codereview.chromium.org/338633003/diff/20001/chrome/browser/ui/omnibo... File chrome/browser/ui/omnibox/omnibox_current_page_delegate_impl.cc (right): https://codereview.chromium.org/338633003/diff/20001/chrome/browser/ui/omnibo... chrome/browser/ui/omnibox/omnibox_current_page_delegate_impl.cc:110: return; Omit this statement since it will happen anyway.
Updated. Please take a look https://codereview.chromium.org/338633003/diff/20001/chrome/browser/ui/omnibo... File chrome/browser/ui/omnibox/omnibox_current_page_delegate_impl.cc (right): https://codereview.chromium.org/338633003/diff/20001/chrome/browser/ui/omnibo... chrome/browser/ui/omnibox/omnibox_current_page_delegate_impl.cc:110: return; On 2014/06/16 22:43:13, Jered wrote: > Omit this statement since it will happen anyway. Done.
https://codereview.chromium.org/338633003/diff/40001/chrome/browser/ui/browse... File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/338633003/diff/40001/chrome/browser/ui/browse... chrome/browser/ui/browser.cc:921: instant_controller_->TabInsertedAt(contents, index, foreground); As per our discussion, you are doing this to submit a prerender request when an user adds a NTP. This only fixes the "Add new tab" workflow. This solution will not work for the following use case: 1. Open a new tab (Tab_1) (This will submit a prerender request for Tab_1) 2. Open another new tab (Tab_2). (This will cancel the prerender request for Tab_1 and submit a prerender request for Tab_2). 3. Switch to Tab_1 (This will cancel the prerender request for Tab_2 but will not submit a prerender request for Tab_1 which we should have done because the omnibox has focus). https://codereview.chromium.org/338633003/diff/40001/chrome/browser/ui/omnibo... File chrome/browser/ui/omnibox/omnibox_current_page_delegate_impl.cc (right): https://codereview.chromium.org/338633003/diff/40001/chrome/browser/ui/omnibo... chrome/browser/ui/omnibox/omnibox_current_page_delegate_impl.cc:109: container_bounds.size()); Let's start with a simple approach. - When the omnibox gains focus, prerender the Instant search base page. - When the omnibox loses focus, cancel the prerender request.
https://codereview.chromium.org/338633003/diff/40001/chrome/browser/ui/browse... File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/338633003/diff/40001/chrome/browser/ui/browse... chrome/browser/ui/browser.cc:921: instant_controller_->TabInsertedAt(contents, index, foreground); On 2014/06/17 18:29:35, kmadhusu wrote: > As per our discussion, you are doing this to submit a prerender request when an > user adds a NTP. This only fixes the "Add new tab" workflow. This solution will > not work for the following use case: > > 1. Open a new tab (Tab_1) (This will submit a prerender request for Tab_1) > 2. Open another new tab (Tab_2). (This will cancel the prerender request for > Tab_1 and submit a prerender request for Tab_2). > 3. Switch to Tab_1 (This will cancel the prerender request for Tab_2 but will > not submit a prerender request for Tab_1 which we should have done because the > omnibox has focus). Done. https://codereview.chromium.org/338633003/diff/40001/chrome/browser/ui/omnibo... File chrome/browser/ui/omnibox/omnibox_current_page_delegate_impl.cc (right): https://codereview.chromium.org/338633003/diff/40001/chrome/browser/ui/omnibo... chrome/browser/ui/omnibox/omnibox_current_page_delegate_impl.cc:109: container_bounds.size()); On 2014/06/17 18:29:35, kmadhusu wrote: > Let's start with a simple approach. > > - When the omnibox gains focus, prerender the Instant search base page. > - When the omnibox loses focus, cancel the prerender request. > Done.
This is looking good. Can you add a test? Thanks. https://codereview.chromium.org/338633003/diff/60001/chrome/browser/ui/omnibo... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): https://codereview.chromium.org/338633003/diff/60001/chrome/browser/ui/omnibo... chrome/browser/ui/omnibox/omnibox_edit_model.cc:371: if (prerenderer && !chrome::ShouldPrerenderInstantUrlOnOmniboxFocus() && We should not add this check here. Consider the following case: 1. Open 2 browser windows. 2. Activate the new tab in browser_window_2. Since the omnibox has focus, we should have prerendered an Instant search base page. 3. Don't type or switch to other tabs. Wait until the prerender manager times out this prerendered page. 4. Now type a search query in the omnibox. Expected behavior: We should have prerendered the Instant search base page and used it to prefetch search results for the high-confidence query. Actual behavior: Because of this check, we will never prerender the Instant search base page after it is timed out. https://codereview.chromium.org/338633003/diff/60001/chrome/browser/ui/search... File chrome/browser/ui/search/search_tab_helper.cc (right): https://codereview.chromium.org/338633003/diff/60001/chrome/browser/ui/search... chrome/browser/ui/search/search_tab_helper.cc:181: if (state != OMNIBOX_FOCUS_NONE) { nit: For better readability, can we split this if-blocks as follows: if (!prerenderer) return; if (state == OMNIBOX_FOCUS_NONE) { prerenderer->Cancel(); return; } if (!IsSearchResultsPage()) Init();
Tests are coming soon... https://codereview.chromium.org/338633003/diff/60001/chrome/browser/ui/omnibo... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): https://codereview.chromium.org/338633003/diff/60001/chrome/browser/ui/omnibo... chrome/browser/ui/omnibox/omnibox_edit_model.cc:371: if (prerenderer && !chrome::ShouldPrerenderInstantUrlOnOmniboxFocus() && On 2014/06/18 16:23:50, kmadhusu wrote: > We should not add this check here. Consider the following case: > > 1. Open 2 browser windows. > 2. Activate the new tab in browser_window_2. Since the omnibox has focus, we > should have prerendered an Instant search base page. > 3. Don't type or switch to other tabs. Wait until the prerender manager times > out this prerendered page. > 4. Now type a search query in the omnibox. > > Expected behavior: We should have prerendered the Instant search base page and > used it to prefetch search results for the high-confidence query. > Actual behavior: Because of this check, we will never prerender the Instant > search base page after it is timed out. Done. https://codereview.chromium.org/338633003/diff/60001/chrome/browser/ui/search... File chrome/browser/ui/search/search_tab_helper.cc (right): https://codereview.chromium.org/338633003/diff/60001/chrome/browser/ui/search... chrome/browser/ui/search/search_tab_helper.cc:181: if (state != OMNIBOX_FOCUS_NONE) { On 2014/06/18 16:23:50, kmadhusu wrote: > nit: For better readability, can we split this if-blocks as follows: > > if (!prerenderer) > return; > > if (state == OMNIBOX_FOCUS_NONE) { > prerenderer->Cancel(); > return; > } > > if (!IsSearchResultsPage()) > Init(); Done.
lgtm. As we discussed, you can add the tests in a follow up CL. Thanks. https://codereview.chromium.org/338633003/diff/100001/chrome/browser/ui/searc... File chrome/browser/ui/search/search_tab_helper.cc (right): https://codereview.chromium.org/338633003/diff/100001/chrome/browser/ui/searc... chrome/browser/ui/search/search_tab_helper.cc:181: return; style nit: Add a new line after this. https://codereview.chromium.org/338633003/diff/100001/chrome/browser/ui/searc... chrome/browser/ui/search/search_tab_helper.cc:185: } style nit: Add a new line after this. https://codereview.chromium.org/338633003/diff/100001/chrome/browser/ui/searc... chrome/browser/ui/search/search_tab_helper.cc:186: if (!IsSearchResultsPage()) style nit: Add curly braces here.
https://codereview.chromium.org/338633003/diff/100001/chrome/browser/ui/searc... File chrome/browser/ui/search/search_tab_helper.cc (right): https://codereview.chromium.org/338633003/diff/100001/chrome/browser/ui/searc... chrome/browser/ui/search/search_tab_helper.cc:181: return; On 2014/06/19 20:44:54, kmadhusu wrote: > style nit: Add a new line after this. Done. https://codereview.chromium.org/338633003/diff/100001/chrome/browser/ui/searc... chrome/browser/ui/search/search_tab_helper.cc:185: } On 2014/06/19 20:44:55, kmadhusu wrote: > style nit: Add a new line after this. Done. https://codereview.chromium.org/338633003/diff/100001/chrome/browser/ui/searc... chrome/browser/ui/search/search_tab_helper.cc:186: if (!IsSearchResultsPage()) On 2014/06/19 20:44:54, kmadhusu wrote: > style nit: Add curly braces here. Done.
https://codereview.chromium.org/338633003/diff/120001/chrome/browser/ui/searc... File chrome/browser/ui/search/search_tab_helper.cc (right): https://codereview.chromium.org/338633003/diff/120001/chrome/browser/ui/searc... chrome/browser/ui/search/search_tab_helper.cc:173: if (web_contents_->GetController().GetPendingEntry() == NULL) { This check makes me worry that there is a case where we might start spinning up a prerender after the user has selected a match in the omnibox dropdown, since it seems there are spurious focus change events in that case. It'd be bad to delay navigation when we know that the user is not going to search. Are you certain this doesn't happen?
https://codereview.chromium.org/338633003/diff/120001/chrome/browser/ui/searc... File chrome/browser/ui/search/search_tab_helper.cc (right): https://codereview.chromium.org/338633003/diff/120001/chrome/browser/ui/searc... chrome/browser/ui/search/search_tab_helper.cc:173: if (web_contents_->GetController().GetPendingEntry() == NULL) { On 2014/06/19 21:09:46, Jered wrote: > This check makes me worry that there is a case where we might start spinning up > a prerender after the user has selected a match in the omnibox dropdown, since > it seems there are spurious focus change events in that case. It'd be bad to > delay navigation when we know that the user is not going to search. Are you > certain this doesn't happen? Done.
On 2014/06/20 00:31:44, sidharthms wrote: > https://codereview.chromium.org/338633003/diff/120001/chrome/browser/ui/searc... > File chrome/browser/ui/search/search_tab_helper.cc (right): > > https://codereview.chromium.org/338633003/diff/120001/chrome/browser/ui/searc... > chrome/browser/ui/search/search_tab_helper.cc:173: if > (web_contents_->GetController().GetPendingEntry() == NULL) { > On 2014/06/19 21:09:46, Jered wrote: > > This check makes me worry that there is a case where we might start spinning > up > > a prerender after the user has selected a match in the omnibox dropdown, since > > it seems there are spurious focus change events in that case. It'd be bad to > > delay navigation when we know that the user is not going to search. Are you > > certain this doesn't happen? > > Done. lgtm
The CQ bit was checked by sidharthms@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sidharthms@chromium.org/338633003/140001
The CQ bit was unchecked by sidharthms@chromium.org
The CQ bit was checked by sidharthms@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sidharthms@chromium.org/338633003/140001
Message was sent while issue was closed.
Change committed as 278731 |
