Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14043009:
Fall back to local page if online NTP fails to load. (Closed)
|
Created:
7 years, 8 months ago by David Black Modified:
7 years, 7 months ago CC:
chromium-reviews, melevin, dhollowa+watch_chromium.org, dougw+watch_chromium.org, sreeram, gideonwald, dominich, samarth+watch_chromium.org, kmadhusu+watch_chromium.org, Jered Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionIf the online search provider page fails to load (404 or other error code) then we fall back to the local fallback page. Also falls back if the user switches away from a tab before it's done loading, to prevent Ctrl-TTTTTTTTTT from completely bogging down the network.
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=198376
Patch Set 1 #Patch Set 2 : DEBUG- upload a set with lots of misc logging statements. Not intended for review. #Patch Set 3 : More debug stuff #Patch Set 4 : Work in progress... #Patch Set 5 : Cleanup #
Total comments: 16
Patch Set 6 : Address some comments #Patch Set 7 : Remove todo #Patch Set 8 : Remove logic to always try to load a server-provided NTP on frontmost tab. #
Total comments: 20
Patch Set 9 : Init Instant tab differently #Patch Set 10 : Fix memory issues and make sure we preload everything #Patch Set 11 : remove search_tab_helper changes #
Total comments: 6
Patch Set 12 : Address some comments #Patch Set 13 : Rebase & add flag guard #Patch Set 14 : Fix terrible flag guarding logic #
Total comments: 16
Patch Set 15 : Addressing comments, added unittest, interactive_ui_test in progress #Patch Set 16 : Added interactive_uitest #Patch Set 17 : Switch to matchesOriginAndPath #Patch Set 18 : Fix failing tests #
Total comments: 40
Patch Set 19 : Actually address comments #Patch Set 20 : Address sreeram's comments #Patch Set 21 : Add some checks for extended_enabled_ to not break old Chrome Instant #
Total comments: 9
Patch Set 22 : Fix failing unittest & address comments #Messages
Total messages: 32 (0 generated)
First pass, no tests yet. Please patch in and bang on and tell me what you think. (start Chrome with --force-fieldtrials="InstantExtended/Group1 local_ntp:1 replace_local:1/")
On 2013/04/26 01:38:32, David Black wrote:
> First pass, no tests yet. Please patch in and bang on and tell me what you
> think.
>
> (start Chrome with --force-fieldtrials="InstantExtended/Group1 local_ntp:1
> replace_local:1/")
This is a good effort, but needs more work. Some initial thoughts:
1. This will swap the Google NTP in for _any_ local NTP, not just the
cold-launch case (new window with an NTP). I suppose this could be considered a
feature, but it's not what we set out to do.
2. Checking last_user_text_ alone isn't enough of a guard against user
interaction. The user could have clicked on am MV tile, for example. Or, could
be clicking on other parts of the page. I am very reluctant to have the browser
do random things when the user is actively interacting with the page.
3. Taking the above point further, what happens if the user is interacting with
Chrome UI elements relevant to the tab. E.g.: they are dragging the tab's title
(handle) along the tabstrip, or trying to bookmark the page, or right-clicking
on the page and about to select something from the context menu. Many (if not
all) of these are async events.
4. InstantSupportDetermined can be called if the page tries to send any message
on its own (say to focus the fakebox by default). When this method returns, the
InstantPage object will be gone, leading to a crash on the next line. I'm
talking about this sort of a pattern in instant_page.cc:
void InstantPage::OnFocusOmnibox(int page_id, OmniboxFocusState state) {
if (contents()->IsActiveEntry(page_id)) {
OnInstantSupportDetermined(page_id, true);
if (ShouldProcessFocusOmnibox())
delegate_->FocusOmnibox(contents(), state);
1) feature! 2&3) I'm going to bang on these cases today to see what happens. If current behavior is reasonable, then there's no need to do a ton of work to not swap in in these cases. The two issues I'm most worried about are a) anything that causes a crash and b) "loosing" a click on a most visited tile. 4) Will investigate. Thanks! On Thu, Apr 25, 2013 at 9:27 PM, <sreeram@chromium.org> wrote: > On 2013/04/26 01:38:32, David Black wrote: > >> First pass, no tests yet. Please patch in and bang on and tell me what >> you >> think. >> > > (start Chrome with --force-fieldtrials="**InstantExtended/Group1 >> local_ntp:1 >> replace_local:1/") >> > > This is a good effort, but needs more work. Some initial thoughts: > > 1. This will swap the Google NTP in for _any_ local NTP, not just the > cold-launch case (new window with an NTP). I suppose this could be > considered a > feature, but it's not what we set out to do. > > 2. Checking last_user_text_ alone isn't enough of a guard against user > interaction. The user could have clicked on am MV tile, for example. Or, > could > be clicking on other parts of the page. I am very reluctant to have the > browser > do random things when the user is actively interacting with the page. > > 3. Taking the above point further, what happens if the user is interacting > with > Chrome UI elements relevant to the tab. E.g.: they are dragging the tab's > title > (handle) along the tabstrip, or trying to bookmark the page, or > right-clicking > on the page and about to select something from the context menu. Many (if > not > all) of these are async events. > > 4. InstantSupportDetermined can be called if the page tries to send any > message > on its own (say to focus the fakebox by default). When this method > returns, the > InstantPage object will be gone, leading to a crash on the next line. I'm > talking about this sort of a pattern in instant_page.cc: > void InstantPage::OnFocusOmnibox(**int page_id, OmniboxFocusState > state) { > if (contents()->IsActiveEntry(**page_id)) { > OnInstantSupportDetermined(**page_id, true); > if (ShouldProcessFocusOmnibox()) > delegate_->FocusOmnibox(**contents(), state); > > https://codereview.chromium.**org/14043009/<https://codereview.chromium.org/1... >
Update: After much (much!) poking around, I have come to the conclusion that it's impossible to do this transition in a non-janky fashion - there's what looks like an unavoidable white flash that often happens when swapping in the new contents. As such, I'm working on a different approach: falling back to the offline NTP when we get an error response back from the server, or if we know ahead of time if we're offline, or (maybe) are on a crummy connection. Initial prototypes look promising - I can catch the error event and use it to trigger what seems to be a pretty seamless load of the local NTP. (aka, no flash of a 404 page or anything like that.) Further bulletins as events warrant.
On 2013/04/27 02:59:15, David Black wrote: > Update: After much (much!) poking around, I have come to the conclusion that > it's impossible to do this transition in a non-janky fashion - there's what > looks like an unavoidable white flash that often happens when swapping in the > new contents. > > As such, I'm working on a different approach: falling back to the offline NTP > when we get an error response back from the server, or if we know ahead of time > if we're offline, or (maybe) are on a crummy connection. Initial prototypes > look promising - I can catch the error event and use it to trigger what seems to > be a pretty seamless load of the local NTP. (aka, no flash of a 404 page or > anything like that.) This might be tough to pull off in all cases. InstantController's current design is going to get in your way. Specifically, we only monitor the active tab. So say the user opens a new window and quickly hits Ctrl-T to create a new tab before the 404 is received. When the user eventually goes back to that original tab, they are going to see the 404 page.
On 2013/04/27 03:16:20, sreeram wrote: > On 2013/04/27 02:59:15, David Black wrote: > > Update: After much (much!) poking around, I have come to the conclusion that > > it's impossible to do this transition in a non-janky fashion - there's what > > looks like an unavoidable white flash that often happens when swapping in the > > new contents. > > > > As such, I'm working on a different approach: falling back to the offline NTP > > when we get an error response back from the server, or if we know ahead of > time > > if we're offline, or (maybe) are on a crummy connection. Initial prototypes > > look promising - I can catch the error event and use it to trigger what seems > to > > be a pretty seamless load of the local NTP. (aka, no flash of a 404 page or > > anything like that.) > > This might be tough to pull off in all cases. InstantController's current design > is going to get in your way. Specifically, we only monitor the active tab. So > say the user opens a new window and quickly hits Ctrl-T to create a new tab > before the 404 is received. When the user eventually goes back to that original > tab, they are going to see the 404 page. I'm pretty sure I can detect the offline case ahead of time, so only the online-but-google-404s case will have this problem, which doesn't seem so bad.
On 2013/04/27 05:20:58, David Black wrote: > On 2013/04/27 03:16:20, sreeram wrote: > > On 2013/04/27 02:59:15, David Black wrote: > > > Update: After much (much!) poking around, I have come to the conclusion > that > > > it's impossible to do this transition in a non-janky fashion - there's what > > > looks like an unavoidable white flash that often happens when swapping in > the > > > new contents. > > > > > > As such, I'm working on a different approach: falling back to the offline > NTP > > > when we get an error response back from the server, or if we know ahead of > > time > > > if we're offline, or (maybe) are on a crummy connection. Initial prototypes > > > look promising - I can catch the error event and use it to trigger what > seems > > to > > > be a pretty seamless load of the local NTP. (aka, no flash of a 404 page or > > > anything like that.) > > > > This might be tough to pull off in all cases. InstantController's current > design > > is going to get in your way. Specifically, we only monitor the active tab. So > > say the user opens a new window and quickly hits Ctrl-T to create a new tab > > before the 404 is received. When the user eventually goes back to that > original > > tab, they are going to see the 404 page. > > I'm pretty sure I can detect the offline case ahead of time, so only the > online-but-google-404s case will have this problem, which doesn't seem so bad. FYI, Anuj is looking at the fully-offline case (crbug.com/229170). I pointed him to a possible listener but I don't know how well that actually works. You should probably sync up with him. Separately, we should definitely not consider the online-but-404 case as an "edge" case as it could be a steady state case for a long time for users. You may be able to handle the cases Sreeram is talking about by adding the "404 observer" to SearchTabHelper with is attached to runs on all tabs, even if they're not active.
Ok, complete reconceptualization and rewrite of this code. PTAL. Seems to work pretty well now. Only issue that seems to remain is that sometimes on a local instant_tab_ the icons don't show up in suggest but everything else works fine. Not sure what's up with that. Opening the dev tools seems to make the problem go away. Any thoughts would be appreciated. Trying to do the listening in SearchTabHelper didn't work at all, as none of the necessary init code ran to push initial data (most visited, etc) down to the embedded page. (Among other problems.) If you want to test it out yourself, the easiest way is to patch it in and just set your --instant-url to something like https://www.google.com/notarealpath?espv=2. (You might see a brief flash of the error page before the local NTP gets swapped in. I'll make sure the real Big Red Button will just display a clean white page so that won't happen.)
I need to take a more careful look, but an initial set of comments from my first pass. High-level question: does this buy you much over just hooking into the existing instant support determined flow? You may need to make some changes to make sure we try to determine Instant support in all "load" cases (failure or not). And if it doesn't support instant, then redirect to the local page when necessary. As I mention below, I don't think we should do anything on TabDeactivated. For M29, we should aim to move all the state into SearchTabHelper which will fix a bunch of related issues along with this. Until then, I think we'll have to live with the case where you open a new window and hit ctrl+T right away. Also, this logic really should be behind a finch flag. We definitely should have the flexibility to turn it off if it's misbehaving / has unintended side-effects. I'll try to look at this more carefully later today. Thanks, Samarth https://codereview.chromium.org/14043009/diff/18001/chrome/browser/search/sea... File chrome/browser/search/search.cc (right): https://codereview.chromium.org/14043009/diff/18001/chrome/browser/search/sea... chrome/browser/search/search.cc:163: return true; I assume this was for debugging. (Shouldn't be necessary btw. Just --instant-url='http://does/not/exist/?espv=2' should work.) https://codereview.chromium.org/14043009/diff/18001/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_controller.cc (right): https://codereview.chromium.org/14043009/diff/18001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_controller.cc:941: // TODO(dcblack): verify this won't nuke loading SRPs and the like. This definitely will. In general, I don't think we need to worry much about the ctrl+TTTT... case. I really doubt that is a normal use case except for Chrome engineers. https://codereview.chromium.org/14043009/diff/18001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_controller.cc:1097: if (ntp_ && IsContentsFrom(ntp(), contents)) { Shouldn't this get handled normally via InstantSupportDetermined? https://codereview.chromium.org/14043009/diff/18001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_controller.cc:1757: contents->GetController().PruneAllButActive(); This isn't right in the general case. For example, let's say you use the home button or bookmark to navigate to chrome://newtab in an existing tab. This will nuke all your history. https://codereview.chromium.org/14043009/diff/18001/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_controller.h (right): https://codereview.chromium.org/14043009/diff/18001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_controller.h:286: virtual void InstantPageLoadFailed(content::WebContents* contents) OVERRIDE; Put this with the rest of the InstantPage::Delegate overrides (and no need to repeat the comment). https://codereview.chromium.org/14043009/diff/18001/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_page.cc (right): https://codereview.chromium.org/14043009/diff/18001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_page.cc:273: void InstantPage::DidNavigateAnyFrame( Why DidNavigateAnyFrame over DidNavigateMainFrame (see https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro... I think this will fire if an iframe on the homepage 404s, which we don't care about. https://codereview.chromium.org/14043009/diff/18001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_page.cc:276: if (details.http_status_code != 200) There's got to be a helper somewhere in Chrome to distinguish success from failure using a code. I don't think you want to count 3xx redirects as a failure for instance.
Flag guarding coming soon. https://codereview.chromium.org/14043009/diff/18001/chrome/browser/search/sea... File chrome/browser/search/search.cc (right): https://codereview.chromium.org/14043009/diff/18001/chrome/browser/search/sea... chrome/browser/search/search.cc:163: return true; On 2013/04/30 17:10:33, samarth wrote: > I assume this was for debugging. (Shouldn't be necessary btw. Just > --instant-url='http://does/not/exist/?espv=2' should work.) No, this is necessary, as that URL otherwise doesn't work. CoerceCommandLineURLToTemplateURL doesn't change the path, and below we check MatchesOriginAndPath vs the built-in template URL. So, if it's not a /search or /webhp page then no go. It took me a while to debug why on earth using --instant-url="https://www.google.com/notarealpah?espv=2" was generating a bunch of really mysterious behavior. https://codereview.chromium.org/14043009/diff/18001/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_controller.cc (right): https://codereview.chromium.org/14043009/diff/18001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_controller.cc:941: // TODO(dcblack): verify this won't nuke loading SRPs and the like. On 2013/04/30 17:10:33, samarth wrote: > This definitely will. In general, I don't think we need to worry much about the > ctrl+TTTT... case. I really doubt that is a normal use case except for Chrome > engineers. As I mentioned in person, this is also necessary to prevent such Ts from actually showing error pages. I'm not actually convinced it'll affect anything we don't want it to. It'll only trigger on instant_urls that don't have Instant support determined. Instant_urls are always the homepage, and if support hasn't been determined yet then there's no way to get a query or anything like that in there. I think this might be ok, but would certainly appreciate others reasoning through it as well. https://codereview.chromium.org/14043009/diff/18001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_controller.cc:1097: if (ntp_ && IsContentsFrom(ntp(), contents)) { On 2013/04/30 17:10:33, samarth wrote: > Shouldn't this get handled normally via InstantSupportDetermined? Whether it *should* or not is not something I'm sure about, but I am sure it *doesn't* get handled by it. If I comment this out, creating a new NTP when GWS returns an error results in the error page. https://codereview.chromium.org/14043009/diff/18001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_controller.cc:1757: contents->GetController().PruneAllButActive(); On 2013/04/30 17:10:33, samarth wrote: > This isn't right in the general case. For example, let's say you use the home > button or bookmark to navigate to chrome://newtab in an existing tab. This will > nuke all your history. Yes, I know. However, I was unable to find a way of removing or replacing the most recent committed history state with the current active entry. I could do it by adding a new function to the controller, but that'll require LGTMs from outside the team, so I'd prefer to leave that as a followup. Added a TODO. https://codereview.chromium.org/14043009/diff/18001/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_controller.h (right): https://codereview.chromium.org/14043009/diff/18001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_controller.h:286: virtual void InstantPageLoadFailed(content::WebContents* contents) OVERRIDE; On 2013/04/30 17:10:33, samarth wrote: > Put this with the rest of the InstantPage::Delegate overrides (and no need to > repeat the comment). Done. https://codereview.chromium.org/14043009/diff/18001/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_page.cc (right): https://codereview.chromium.org/14043009/diff/18001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_page.cc:273: void InstantPage::DidNavigateAnyFrame( On 2013/04/30 17:10:33, samarth wrote: > Why DidNavigateAnyFrame over DidNavigateMainFrame (see > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro... > I think this will fire if an iframe on the homepage 404s, which we don't care > about. Ah, I wasn't immediately sure what the difference between the two was so chose the one that seemed like it would cover more cases. Switched to MainFrame. https://codereview.chromium.org/14043009/diff/18001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_page.cc:276: if (details.http_status_code != 200) On 2013/04/30 17:10:33, samarth wrote: > There's got to be a helper somewhere in Chrome to distinguish success from > failure using a code. I don't think you want to count 3xx redirects as a failure > for instance. Good point. Adjusted. (I wasn't able to find any helper functions in my cursory search, but >=400 seems like it's just as good.)
Updated to remove the logic to attempt to show a server-provided NTP on subsequent tab loads if one isn't fully preloaded, as discussed in person.
https://codereview.chromium.org/14043009/diff/18001/chrome/browser/search/sea... File chrome/browser/search/search.cc (right): https://codereview.chromium.org/14043009/diff/18001/chrome/browser/search/sea... chrome/browser/search/search.cc:163: return true; On 2013/04/30 23:09:08, David Black wrote: > On 2013/04/30 17:10:33, samarth wrote: > > I assume this was for debugging. (Shouldn't be necessary btw. Just > > --instant-url='http://does/not/exist/?espv=2' should work.) > > No, this is necessary, as that URL otherwise doesn't work. > CoerceCommandLineURLToTemplateURL doesn't change the path, and below we check > MatchesOriginAndPath vs the built-in template URL. So, if it's not a /search or > /webhp page then no go. It took me a while to debug why on earth using > --instant-url="https://www.google.com/notarealpah?espv=2" was generating a bunch > of really mysterious behavior. In that case, you should change Coerce... to also change the path. I think you still want search term extraction, etc. to still work even if you're using a custom path. https://codereview.chromium.org/14043009/diff/30001/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_controller.cc (right): https://codereview.chromium.org/14043009/diff/30001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_controller.cc:1077: void InstantController::InstantPageLoadFailed(content::WebContents* contents) { Please keep the functions in the same order as in the header. https://codereview.chromium.org/14043009/diff/30001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_controller.cc:1083: SearchTabHelper::FromWebContents(contents)->RedirectingToLocal(); It't not clear to me why this is necessary. Do you really just want to call ResetInstantTab after the Redirect? https://codereview.chromium.org/14043009/diff/30001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_controller.cc:1084: RedirectToLocalNTP(contents); I can try this tomorrow but what if you have a search page open, lose internet connection, and then reload? I think you'll just get redirected to the local NTP. It's not really a theoretical concern: this can happen if you have session restore and reopen Chrome when you had a bunch of search tabs open. https://codereview.chromium.org/14043009/diff/30001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_controller.cc:1090: ResetNTP(false, true); No, this will lead to an infinite loop of reloading (the true means use the remote NTP again). You should just delete ntp_. Also, you're deleting ntp_ with ntp_ on the stack. You need to do a DeleteSoon(ntp_.release()). https://codereview.chromium.org/14043009/diff/30001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_controller.cc:1096: CreateOverlay(local_fallback_url.spec(), contents); Update already handles the fallback, so you just need to delete overlay_. https://codereview.chromium.org/14043009/diff/30001/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_page.cc (right): https://codereview.chromium.org/14043009/diff/30001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_page.cc:198: int64 frame_id, nit: comment out the parameter names that you won't use https://codereview.chromium.org/14043009/diff/30001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_page.cc:275: const content::FrameNavigateParams& params) { Likewise, here https://codereview.chromium.org/14043009/diff/30001/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_page.h (right): https://codereview.chromium.org/14043009/diff/30001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_page.h:222: nit: extra empty line https://codereview.chromium.org/14043009/diff/30001/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_tab.cc (right): https://codereview.chromium.org/14043009/diff/30001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_tab.cc:17: if (!contents->IsLoading()) Can you explain why you need this change?
Writing a test for this is proving somewhat difficult. It's in progress. Rest of the code seems to work. https://codereview.chromium.org/14043009/diff/18001/chrome/browser/search/sea... File chrome/browser/search/search.cc (right): https://codereview.chromium.org/14043009/diff/18001/chrome/browser/search/sea... chrome/browser/search/search.cc:163: return true; On 2013/05/01 06:26:47, samarth wrote: > On 2013/04/30 23:09:08, David Black wrote: > > On 2013/04/30 17:10:33, samarth wrote: > > > I assume this was for debugging. (Shouldn't be necessary btw. Just > > > --instant-url='http://does/not/exist/?espv=2' should work.) > > > > No, this is necessary, as that URL otherwise doesn't work. > > CoerceCommandLineURLToTemplateURL doesn't change the path, and below we check > > MatchesOriginAndPath vs the built-in template URL. So, if it's not a /search > or > > /webhp page then no go. It took me a while to debug why on earth using > > --instant-url="https://www.google.com/notarealpah?espv=2" was generating a > bunch > > of really mysterious behavior. > > In that case, you should change Coerce... to also change the path. I think you > still want search term extraction, etc. to still work even if you're using a > custom path. Search term extraction still does work. It's only here in this function where we coerce a URL and then verify its path matches the vanilla template URL. It seems quite clear to me that a function called IsInstantURL should always simply return true if IsCommandLineInstantURL returns true - having any coercing going on at all in this function makes no sense to me. https://codereview.chromium.org/14043009/diff/30001/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_controller.cc (right): https://codereview.chromium.org/14043009/diff/30001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_controller.cc:1077: void InstantController::InstantPageLoadFailed(content::WebContents* contents) { On 2013/05/01 06:26:47, samarth wrote: > Please keep the functions in the same order as in the header. Done. https://codereview.chromium.org/14043009/diff/30001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_controller.cc:1083: SearchTabHelper::FromWebContents(contents)->RedirectingToLocal(); On 2013/05/01 06:26:47, samarth wrote: > It't not clear to me why this is necessary. Do you really just want to call > ResetInstantTab after the Redirect? Fixed as we discussed in person. https://codereview.chromium.org/14043009/diff/30001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_controller.cc:1084: RedirectToLocalNTP(contents); On 2013/05/01 06:26:47, samarth wrote: > I can try this tomorrow but what if you have a search page open, lose internet > connection, and then reload? I think you'll just get redirected to the local > NTP. > > It's not really a theoretical concern: this can happen if you have session > restore and reopen Chrome when you had a bunch of search tabs open. Good point. Changed to fall back only if the URL exactly equals the base online Instant URL. https://codereview.chromium.org/14043009/diff/30001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_controller.cc:1090: ResetNTP(false, true); On 2013/05/01 06:26:47, samarth wrote: > No, t his will lead to an infinite loop of reloading (the true means use the > remote NTP again). You should just delete ntp_. > > Also, you're deleting ntp_ with ntp_ on the stack. You need to do a > DeleteSoon(ntp_.release()). Will not cause an infinite loop - second param is use_local, not use_server. Added DeleteSoon. Kept in the ResetNTP call so the NTP still gets preloaded. The speed difference between preloading the local NTP and loading it once they create a new tab is noticeable, so it's nice to do it. https://codereview.chromium.org/14043009/diff/30001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_controller.cc:1096: CreateOverlay(local_fallback_url.spec(), contents); On 2013/05/01 06:26:47, samarth wrote: > Update already handles the fallback, so you just need to delete overlay_. Update is too late - they've already typed a character at that point, right? They'll then have to wait for the local page to fully load before seeing anything, resulting in a latency hit. Like with the ntp case above, it's much better to preload the local overlay. https://codereview.chromium.org/14043009/diff/30001/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_page.cc (right): https://codereview.chromium.org/14043009/diff/30001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_page.cc:198: int64 frame_id, On 2013/05/01 06:26:47, samarth wrote: > nit: comment out the parameter names that you won't use Done. https://codereview.chromium.org/14043009/diff/30001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_page.cc:275: const content::FrameNavigateParams& params) { On 2013/05/01 06:26:47, samarth wrote: > Likewise, here Done. https://codereview.chromium.org/14043009/diff/30001/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_page.h (right): https://codereview.chromium.org/14043009/diff/30001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_page.h:222: On 2013/05/01 06:26:47, samarth wrote: > nit: extra empty line Done. https://codereview.chromium.org/14043009/diff/30001/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_tab.cc (right): https://codereview.chromium.org/14043009/diff/30001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_tab.cc:17: if (!contents->IsLoading()) On 2013/05/01 06:26:47, samarth wrote: > Can you explain why you need this change? If you hook up an instant tab to a page that is still loading (say because it was just redirected to the offline page) this will fire too early and cause Chrome to mistakenly think the page doesn't support Instant. InstantPage::DidFinishLoad will fire off DetermineIfPageSupportsInstant at the correct time in the case instant_tab is bound while the page is still loading.
This is looking pretty good from my perspective (as long as we have a flag to control the redirect behavior). I'll pound on this some more in the morning to see if I can get it to break in any way, but assuming you get Sreeram to sign off, I think we can get this in by tomorrow. Thanks, Samarth https://codereview.chromium.org/14043009/diff/30001/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_tab.cc (right): https://codereview.chromium.org/14043009/diff/30001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_tab.cc:17: if (!contents->IsLoading()) On 2013/05/02 00:49:45, David Black wrote: > On 2013/05/01 06:26:47, samarth wrote: > > Can you explain why you need this change? > > If you hook up an instant tab to a page that is still loading (say because it > was just redirected to the offline page) this will fire too early and cause > Chrome to mistakenly think the page doesn't support Instant. > InstantPage::DidFinishLoad will fire off DetermineIfPageSupportsInstant at the > correct time in the case instant_tab is bound while the page is still loading. Is this change still necessary now that you're no longer resetting instant tab and just re-initializing the data? https://codereview.chromium.org/14043009/diff/41001/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_controller.cc (right): https://codereview.chromium.org/14043009/diff/41001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_controller.cc:715: if (instant_tab_ && IsContentsFrom(instant_tab(), contents)) { IsContentsFrom already checks that the page is not null so the first clause is not necessary. https://codereview.chromium.org/14043009/diff/41001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_controller.cc:728: MessageLoop::current()->DeleteSoon(FROM_HERE, ntp_.release()); It's best to also delete the contents held by ntp_ so we don't get continue to get IPCs from the renderer until ntp_ is actually deleted. Same for overlay below. You can just use the DeletePageSoon() helper I added in 13873010 (which I'll land as soon as the tree re-opens). https://codereview.chromium.org/14043009/diff/41001/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_extended_interactive_uitest.cc (right): https://codereview.chromium.org/14043009/diff/41001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_extended_interactive_uitest.cc:705: IN_PROC_BROWSER_TEST_F(InstantExtendedTest, RedirectToLocalOnLoadFailure) { Please also add a test to test that instanttabs get data re-sent to them when there's a navigation in there.
https://codereview.chromium.org/14043009/diff/30001/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_tab.cc (right): https://codereview.chromium.org/14043009/diff/30001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_tab.cc:17: if (!contents->IsLoading()) On 2013/05/02 03:12:56, samarth wrote: > On 2013/05/02 00:49:45, David Black wrote: > > On 2013/05/01 06:26:47, samarth wrote: > > > Can you explain why you need this change? > > > > If you hook up an instant tab to a page that is still loading (say because it > > was just redirected to the offline page) this will fire too early and cause > > Chrome to mistakenly think the page doesn't support Instant. > > InstantPage::DidFinishLoad will fire off DetermineIfPageSupportsInstant at the > > correct time in the case instant_tab is bound while the page is still loading. > > Is this change still necessary now that you're no longer resetting instant tab > and just re-initializing the data? Perhaps not, but it seems like a sane check to have in here in any case. I'd prefer to leave it. https://codereview.chromium.org/14043009/diff/41001/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_controller.cc (right): https://codereview.chromium.org/14043009/diff/41001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_controller.cc:715: if (instant_tab_ && IsContentsFrom(instant_tab(), contents)) { On 2013/05/02 03:12:56, samarth wrote: > IsContentsFrom already checks that the page is not null so the first clause is > not necessary. Done. https://codereview.chromium.org/14043009/diff/41001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_controller.cc:728: MessageLoop::current()->DeleteSoon(FROM_HERE, ntp_.release()); On 2013/05/02 03:12:56, samarth wrote: > It's best to also delete the contents held by ntp_ so we don't get continue to > get IPCs from the renderer until ntp_ is actually deleted. Same for overlay > below. > > You can just use the DeletePageSoon() helper I added in 13873010 (which I'll > land as soon as the tree re-opens). Ok. That'll require a bunch of rebasing on my part anyway. https://codereview.chromium.org/14043009/diff/41001/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_extended_interactive_uitest.cc (right): https://codereview.chromium.org/14043009/diff/41001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_extended_interactive_uitest.cc:705: IN_PROC_BROWSER_TEST_F(InstantExtendedTest, RedirectToLocalOnLoadFailure) { On 2013/05/02 03:12:56, samarth wrote: > Please also add a test to test that instanttabs get data re-sent to them when > there's a navigation in there. Ok. Will work on that (and on making this test work correctly) tomorrow.
FYI, this crashes in a debug build right now.
One case where the redirect doesn't work: $ out/Release/chrome --user-data-dir=/tmp/udd-cros-new --force-compositing-mode --remote-debugging-port=9222 --vmodule=*searchbox*=1,*instant_*=1 --force-fieldtrials='InstantExtended/Group1 espv:2 use_remote_ntp_on_startup:1/' --host-resolver-rules="MAP * ~NOTFOUND" Otherwise, this is looking really good! Most likely my last round of substantive comments. Sreeram: PTAL. Thanks, Samarth https://codereview.chromium.org/14043009/diff/49002/chrome/browser/search/sea... File chrome/browser/search/search.cc (right): https://codereview.chromium.org/14043009/diff/49002/chrome/browser/search/sea... chrome/browser/search/search.cc:163: return true; Test for this change too please. https://codereview.chromium.org/14043009/diff/49002/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_controller.cc (right): https://codereview.chromium.org/14043009/diff/49002/chrome/browser/ui/search/... chrome/browser/ui/search/instant_controller.cc:689: GURL local_fallback_url = chrome::GetLocalInstantURL(browser_->profile()); not used anymore https://codereview.chromium.org/14043009/diff/49002/chrome/browser/ui/search/... chrome/browser/ui/search/instant_controller.cc:692: std::string instant_url = GetInstantURL(); const std::string& https://codereview.chromium.org/14043009/diff/49002/chrome/browser/ui/search/... chrome/browser/ui/search/instant_controller.cc:695: RedirectToLocalNTP(contents); Please add a comment explaining why we only redirect in this case. https://codereview.chromium.org/14043009/diff/49002/chrome/browser/ui/search/... chrome/browser/ui/search/instant_controller.cc:699: if (ntp_->IsLocal()) I don't see how a local page would ever fail, but if it does, we should call DeletePageSoon in that case too (so we don't use it). But ResetNTP should only be called in the non-local case as you have it. Same thing for overlay below. https://codereview.chromium.org/14043009/diff/49002/chrome/browser/ui/search/... chrome/browser/ui/search/instant_controller.cc:1137: if (!supports_instant) I think it's a good idea to do the same thing here as you're doing in InstantPageLoadFailed -- which is to preload the local page unless this was already the local page. If you'd rather not add it in this CL, I can send out a change for it. https://codereview.chromium.org/14043009/diff/49002/chrome/browser/ui/search/... chrome/browser/ui/search/instant_controller.cc:1198: // The Instant tab loaded. Send it the data it needs to display properly. nit: The Instant tab navigated. https://codereview.chromium.org/14043009/diff/49002/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_tab.cc (right): https://codereview.chromium.org/14043009/diff/49002/chrome/browser/ui/search/... chrome/browser/ui/search/instant_tab.cc:17: if (!contents->IsLoading()) Reading https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro..., this check isn't quite right. I think IsLoading will return true even if the page has fired onLoad, and then is fetching a new resource. So if you switch tabs to a search page in this state, we'll never register Instant support. You need a better check that corresponds to the main frame still loading. Or if you don't need it for your change, just leave it out.
> --host-resolver-rules="MAP * ~NOTFOUND" Is this a real thing? Are you sure this doesn't also nuke the local pages? I'd like to be reassured this represents a real use case before spending effort looking into it. https://codereview.chromium.org/14043009/diff/49002/chrome/browser/search/sea... File chrome/browser/search/search.cc (right): https://codereview.chromium.org/14043009/diff/49002/chrome/browser/search/sea... chrome/browser/search/search.cc:163: return true; On 2013/05/02 21:57:18, samarth wrote: > Test for this change too please. Done. https://codereview.chromium.org/14043009/diff/49002/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_controller.cc (right): https://codereview.chromium.org/14043009/diff/49002/chrome/browser/ui/search/... chrome/browser/ui/search/instant_controller.cc:689: GURL local_fallback_url = chrome::GetLocalInstantURL(browser_->profile()); On 2013/05/02 21:57:18, samarth wrote: > not used anymore Done. https://codereview.chromium.org/14043009/diff/49002/chrome/browser/ui/search/... chrome/browser/ui/search/instant_controller.cc:692: std::string instant_url = GetInstantURL(); On 2013/05/02 21:57:18, samarth wrote: > const std::string& Bloody c++ and its inability to have a basic string type. Done. https://codereview.chromium.org/14043009/diff/49002/chrome/browser/ui/search/... chrome/browser/ui/search/instant_controller.cc:695: RedirectToLocalNTP(contents); On 2013/05/02 21:57:18, samarth wrote: > Please add a comment explaining why we only redirect in this case. You're talking about the url != url condition above, right? Done. https://codereview.chromium.org/14043009/diff/49002/chrome/browser/ui/search/... chrome/browser/ui/search/instant_controller.cc:699: if (ntp_->IsLocal()) On 2013/05/02 21:57:18, samarth wrote: > I don't see how a local page would ever fail, but if it does, we should call > DeletePageSoon in that case too (so we don't use it). But ResetNTP should only > be called in the non-local case as you have it. > > Same thing for overlay below. Done. https://codereview.chromium.org/14043009/diff/49002/chrome/browser/ui/search/... chrome/browser/ui/search/instant_controller.cc:1137: if (!supports_instant) On 2013/05/02 21:57:18, samarth wrote: > I think it's a good idea to do the same thing here as you're doing in > InstantPageLoadFailed -- which is to preload the local page unless this was > already the local page. > > If you'd rather not add it in this CL, I can send out a change for it. Done. https://codereview.chromium.org/14043009/diff/49002/chrome/browser/ui/search/... chrome/browser/ui/search/instant_controller.cc:1198: // The Instant tab loaded. Send it the data it needs to display properly. On 2013/05/02 21:57:18, samarth wrote: > nit: The Instant tab navigated. AARG. THAT MAKES THE LINE 81 CHARS LONG. Done. https://codereview.chromium.org/14043009/diff/49002/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_tab.cc (right): https://codereview.chromium.org/14043009/diff/49002/chrome/browser/ui/search/... chrome/browser/ui/search/instant_tab.cc:17: if (!contents->IsLoading()) On 2013/05/02 21:57:18, samarth wrote: > Reading > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro..., > this check isn't quite right. > > I think IsLoading will return true even if the page has fired onLoad, and then > is fetching a new resource. So if you switch tabs to a search page in this > state, we'll never register Instant support. > > You need a better check that corresponds to the main frame still loading. Or if > you don't need it for your change, just leave it out. Ah, good point. IsWaitingForResponse seems much more like what we want here. Switched to that.
> --host-resolver-rules="MAP * ~NOTFOUND" This works fine for me now. Test added, other tests fixed, code cleaned up a bit. There are no TODOs on my side left - just waiting on comments (and hopefully LGTMs!).
lgtm Nice work! Just nits below. I'm happy that the scary bits are behind a flag so we can always flip it off if something bad happens. Thanks, Samarth https://codereview.chromium.org/14043009/diff/67001/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_controller.cc (right): https://codereview.chromium.org/14043009/diff/67001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_controller.cc:704: if (IsContentsFrom(ntp(), contents)) { nit: use else if here and below. https://codereview.chromium.org/14043009/diff/67001/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_extended_interactive_uitest.cc (right): https://codereview.chromium.org/14043009/diff/67001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_extended_interactive_uitest.cc:743: ASSERT_TRUE(instant()->ntp_); nit: just write: ASSERT_NE(NULL, instant()->ntp()); EXPECT_TRUE(instant()->ntp()->IsLocal()); https://codereview.chromium.org/14043009/diff/67001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_extended_interactive_uitest.cc:2003: ASSERT_TRUE(instant()->overlay_); Same here and elsewhere https://codereview.chromium.org/14043009/diff/67001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_extended_interactive_uitest.cc:2071: class InstantExtendedOnlineTest : public InProcessBrowserTest, nit: how about InstantExtendedFirstTabTest or something https://codereview.chromium.org/14043009/diff/67001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_extended_interactive_uitest.cc:2106: // NTP contents should be preloaded. So you're relying on the fact that initially we try to load google by default which will fail because these tests can't reach the internet? Seems somewhat brittle but I don't have any better ideas. Perhaps Sreeram will. https://codereview.chromium.org/14043009/diff/67001/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_page.cc (right): https://codereview.chromium.org/14043009/diff/67001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_page.cc:198: void InstantPage::DidFailProvisionalLoad( nit: keep functions in same order as in the header file. https://codereview.chromium.org/14043009/diff/67001/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_page.h (right): https://codereview.chromium.org/14043009/diff/67001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_page.h:96: // Called when the preloaded NTP is ready for display. Fix comment please
https://codereview.chromium.org/14043009/diff/67001/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_controller.cc (right): https://codereview.chromium.org/14043009/diff/67001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_controller.cc:1751: void InstantController::RedirectToLocalNTP(content::WebContents* contents) { Oh also, can you add LOG_INSTANT_DEBUG_EVENT logging for this so we can see it in about://instant?
https://codereview.chromium.org/14043009/diff/67001/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_controller.cc (right): https://codereview.chromium.org/14043009/diff/67001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_controller.cc:704: if (IsContentsFrom(ntp(), contents)) { On 2013/05/03 04:39:17, samarth wrote: > nit: use else if here and below. Done. https://codereview.chromium.org/14043009/diff/67001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_controller.cc:1751: void InstantController::RedirectToLocalNTP(content::WebContents* contents) { On 2013/05/03 05:23:11, samarth wrote: > Oh also, can you add LOG_INSTANT_DEBUG_EVENT logging for this so we can see it > in about://instant? Good idea. Added it up in InstantPageLoadFailed. https://codereview.chromium.org/14043009/diff/67001/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_extended_interactive_uitest.cc (right): https://codereview.chromium.org/14043009/diff/67001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_extended_interactive_uitest.cc:743: ASSERT_TRUE(instant()->ntp_); On 2013/05/03 04:39:17, samarth wrote: > nit: just write: > ASSERT_NE(NULL, instant()->ntp()); > EXPECT_TRUE(instant()->ntp()->IsLocal()); Done. https://codereview.chromium.org/14043009/diff/67001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_extended_interactive_uitest.cc:2003: ASSERT_TRUE(instant()->overlay_); On 2013/05/03 04:39:17, samarth wrote: > Same here and elsewhere Done. https://codereview.chromium.org/14043009/diff/67001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_extended_interactive_uitest.cc:2071: class InstantExtendedOnlineTest : public InProcessBrowserTest, On 2013/05/03 04:39:17, samarth wrote: > nit: how about InstantExtendedFirstTabTest or something Done. https://codereview.chromium.org/14043009/diff/67001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_extended_interactive_uitest.cc:2106: // NTP contents should be preloaded. On 2013/05/03 04:39:17, samarth wrote: > So you're relying on the fact that initially we try to load google by default > which will fail because these tests can't reach the internet? Seems somewhat > brittle but I don't have any better ideas. Perhaps Sreeram will. Yep. I can't use the normal way of overriding the URL used in other tests as the browser object for the new window goes ahead and navigates before I get a chance to set anything on it. I could override by setting --instant-url, but I actually sort of like the test as-is, as it's exactly this loading-google behavior that caught the bug I mentioned to you earlier. https://codereview.chromium.org/14043009/diff/67001/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_page.cc (right): https://codereview.chromium.org/14043009/diff/67001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_page.cc:198: void InstantPage::DidFailProvisionalLoad( On 2013/05/03 04:39:17, samarth wrote: > nit: keep functions in same order as in the header file. Done. https://codereview.chromium.org/14043009/diff/67001/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_page.h (right): https://codereview.chromium.org/14043009/diff/67001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_page.h:96: // Called when the preloaded NTP is ready for display. On 2013/05/03 04:39:17, samarth wrote: > Fix comment please Done.
Still reviewing... some initial comments. https://codereview.chromium.org/14043009/diff/67001/chrome/browser/search/sea... File chrome/browser/search/search.cc (right): https://codereview.chromium.org/14043009/diff/67001/chrome/browser/search/sea... chrome/browser/search/search.cc:163: return true; This bypasses the secure & espv checks. I understand your argument that "if IsCommandLineInstantURL, it must be IsInstantURL!". But that's not what this is exactly meant to be. The --instant-url is meant to be a very light-weight way to change the template's instant URL. In particular, I want it to go through as many of the same checks we apply to the regular template URL. In fact, by leaving the code as is (without your change), you are now forced to test the 404/204 case properly. I.e., bring up a google.com server that actually serves a 204 on the expected /search or /webhp path. Because, that's what users will actually experience. By allowing this shortcut, you can "get away" with testing --instant-url="file://foo/bar", which doesn't really show that this would work for the normal user case. https://codereview.chromium.org/14043009/diff/67001/chrome/browser/search/sea... chrome/browser/search/search.cc:486: return true; This is inverted. kEnableLocalFirstLoadNTP should "return false" and kDisableLocalFirstLoadNTP should "return true". https://codereview.chromium.org/14043009/diff/67001/chrome/browser/search/sea... File chrome/browser/search/search_unittest.cc (right): https://codereview.chromium.org/14043009/diff/67001/chrome/browser/search/sea... chrome/browser/search/search_unittest.cc:335: } Given my comment earlier about not doing "return true" for the --instant-url, I don't see the need for this new test at all. https://codereview.chromium.org/14043009/diff/67001/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_controller.cc (right): https://codereview.chromium.org/14043009/diff/67001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_controller.cc:704: if (IsContentsFrom(ntp(), contents)) { On 2013/05/03 06:14:44, David Black wrote: > On 2013/05/03 04:39:17, samarth wrote: > > nit: use else if here and below. > > Done. Not done? https://codereview.chromium.org/14043009/diff/67001/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_page.cc (right): https://codereview.chromium.org/14043009/diff/67001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_page.cc:198: void InstantPage::DidFailProvisionalLoad( On 2013/05/03 06:14:44, David Black wrote: > On 2013/05/03 04:39:17, samarth wrote: > > nit: keep functions in same order as in the header file. > > Done. Not done? https://codereview.chromium.org/14043009/diff/67001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_page.cc:205: delegate_->InstantPageLoadFailed(contents()); Shouldn't you do this only for "if (is_main_frame)"? https://codereview.chromium.org/14043009/diff/67001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_page.cc:276: const content::FrameNavigateParams& /* params */) { Strange indent. Either 4 from beginning of line, or from beginning of "InstantPage" or such. https://codereview.chromium.org/14043009/diff/67001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_page.cc:281: delegate_->InstantPageLoadFailed(contents()); DidNavigateMainFrame() is called for all main page navigations, including after we have successfully loaded the server NTP and used it for searches, etc. It doesn't seem that you should redirect to local NTP in those cases. Right? There are some checks in InstantController where you return if the URL doesn't match the instant_url of the template, etc., but what if those checks pass? Say for example that there's a Back/Forward navigation to the homepage or a history.pushState (say I clicked on the Google logo from the search results to go back to the homepage) or some combination of the two. Is it possible for any of these to return 204? (Not that the server actually returns a 204, but say because of history.pushState semantics or Back/Forward cache?) Basically, I'd like us to be more conservative when deciding that it's okay to replace an InstantTab contents. For example, if we have not yet determined instant_support for it. https://codereview.chromium.org/14043009/diff/67001/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_page.h (right): https://codereview.chromium.org/14043009/diff/67001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_page.h:23: struct LoadCommittedDetails; The ordering should be by the identifier name (ignoring class/struct), so move WebContents to be the last. https://codereview.chromium.org/14043009/diff/67001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_page.h:96: // Called when the preloaded NTP is ready for display. On 2013/05/03 06:14:44, David Black wrote: > On 2013/05/03 04:39:17, samarth wrote: > > Fix comment please > > Done. Not done? https://codereview.chromium.org/14043009/diff/67001/chrome/common/chrome_swit... File chrome/common/chrome_switches.h (right): https://codereview.chromium.org/14043009/diff/67001/chrome/common/chrome_swit... chrome/common/chrome_switches.h:103: extern const char kDisableLocalFirstLoadNTP[]; Add these new flags to about_flags.cc as well. (On ChromeOS devices, it's hard to change command line flags otherwise.)
Forgot to git cl upload the stuff from Samarth's comments - sorry about that. https://codereview.chromium.org/14043009/diff/67001/chrome/browser/search/sea... File chrome/browser/search/search.cc (right): https://codereview.chromium.org/14043009/diff/67001/chrome/browser/search/sea... chrome/browser/search/search.cc:163: return true; On 2013/05/03 23:17:31, sreeram wrote: > This bypasses the secure & espv checks. I understand your argument that "if > IsCommandLineInstantURL, it must be IsInstantURL!". But that's not what this is > exactly meant to be. The --instant-url is meant to be a very light-weight way to > change the template's instant URL. In particular, I want it to go through as > many of the same checks we apply to the regular template URL. > > In fact, by leaving the code as is (without your change), you are now forced to > test the 404/204 case properly. I.e., bring up a http://google.com server that actually > serves a 204 on the expected /search or /webhp path. Because, that's what users > will actually experience. By allowing this shortcut, you can "get away" with > testing --instant-url="file://foo/bar", which doesn't really show that this > would work for the normal user case. As discussed in person, I maintain the logic here as-is is extremely confusing, but reverted back to what it was since it's not critical for this CL. https://codereview.chromium.org/14043009/diff/67001/chrome/browser/search/sea... chrome/browser/search/search.cc:486: return true; On 2013/05/03 23:17:31, sreeram wrote: > This is inverted. kEnableLocalFirstLoadNTP should "return false" and > kDisableLocalFirstLoadNTP should "return true". Whoops. Fixed. https://codereview.chromium.org/14043009/diff/67001/chrome/browser/search/sea... File chrome/browser/search/search_unittest.cc (right): https://codereview.chromium.org/14043009/diff/67001/chrome/browser/search/sea... chrome/browser/search/search_unittest.cc:335: } On 2013/05/03 23:17:31, sreeram wrote: > Given my comment earlier about not doing "return true" for the --instant-url, I > don't see the need for this new test at all. Done. https://codereview.chromium.org/14043009/diff/67001/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_controller.cc (right): https://codereview.chromium.org/14043009/diff/67001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_controller.cc:704: if (IsContentsFrom(ntp(), contents)) { On 2013/05/03 23:17:31, sreeram wrote: > On 2013/05/03 06:14:44, David Black wrote: > > On 2013/05/03 04:39:17, samarth wrote: > > > nit: use else if here and below. > > > > Done. > > Not done? Done. https://codereview.chromium.org/14043009/diff/67001/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_page.cc (right): https://codereview.chromium.org/14043009/diff/67001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_page.cc:198: void InstantPage::DidFailProvisionalLoad( On 2013/05/03 23:17:31, sreeram wrote: > On 2013/05/03 06:14:44, David Black wrote: > > On 2013/05/03 04:39:17, samarth wrote: > > > nit: keep functions in same order as in the header file. > > > > Done. > > Not done? > Done. https://codereview.chromium.org/14043009/diff/67001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_page.cc:205: delegate_->InstantPageLoadFailed(contents()); On 2013/05/03 23:17:31, sreeram wrote: > Shouldn't you do this only for "if (is_main_frame)"? Sure, I guess that sanity check makes sense. Done. https://codereview.chromium.org/14043009/diff/67001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_page.cc:276: const content::FrameNavigateParams& /* params */) { On 2013/05/03 23:17:31, sreeram wrote: > Strange indent. Either 4 from beginning of line, or from beginning of > "InstantPage" or such. Done. https://codereview.chromium.org/14043009/diff/67001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_page.cc:281: delegate_->InstantPageLoadFailed(contents()); On 2013/05/03 23:17:31, sreeram wrote: > DidNavigateMainFrame() is called for all main page navigations, including after > we have successfully loaded the server NTP and used it for searches, etc. It > doesn't seem that you should redirect to local NTP in those cases. Right? > > There are some checks in InstantController where you return if the URL doesn't > match the instant_url of the template, etc., but what if those checks pass? Say > for example that there's a Back/Forward navigation to the homepage or a > history.pushState (say I clicked on the Google logo from the search results to > go back to the homepage) or some combination of the two. Is it possible for any > of these to return 204? (Not that the server actually returns a 204, but say > because of history.pushState semantics or Back/Forward cache?) > > Basically, I'd like us to be more conservative when deciding that it's okay to > replace an InstantTab contents. For example, if we have not yet determined > instant_support for it. This does not seem like a coherent concern to me. A history.pushState won't have an error http_status_code (if it even triggers this function at all, which I doubt, as it's not a navigation), and everything else is an active navigation to a homepage URL, which should always fall back on error in all cases. If I hit the back button to navigate to the NTP and I'm now offline then it should in fact fall back to the local page. A page that has failed to load and has an error status code will never have instant_support because it hasn't loaded. https://codereview.chromium.org/14043009/diff/67001/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_page.h (right): https://codereview.chromium.org/14043009/diff/67001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_page.h:23: struct LoadCommittedDetails; On 2013/05/03 23:17:31, sreeram wrote: > The ordering should be by the identifier name (ignoring class/struct), so move > WebContents to be the last. Done. https://codereview.chromium.org/14043009/diff/67001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_page.h:96: // Called when the preloaded NTP is ready for display. On 2013/05/03 23:17:31, sreeram wrote: > On 2013/05/03 06:14:44, David Black wrote: > > On 2013/05/03 04:39:17, samarth wrote: > > > Fix comment please > > > > Done. > > Not done? > Done. https://codereview.chromium.org/14043009/diff/67001/chrome/common/chrome_swit... File chrome/common/chrome_switches.h (right): https://codereview.chromium.org/14043009/diff/67001/chrome/common/chrome_swit... chrome/common/chrome_switches.h:103: extern const char kDisableLocalFirstLoadNTP[]; On 2013/05/03 23:17:31, sreeram wrote: > Add these new flags to about_flags.cc as well. (On ChromeOS devices, it's hard > to change command line flags otherwise.) Done.
https://codereview.chromium.org/14043009/diff/67001/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_page.cc (right): https://codereview.chromium.org/14043009/diff/67001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_page.cc:281: delegate_->InstantPageLoadFailed(contents()); On 2013/05/04 00:49:23, David Black wrote: > On 2013/05/03 23:17:31, sreeram wrote: > > DidNavigateMainFrame() is called for all main page navigations, including > after > > we have successfully loaded the server NTP and used it for searches, etc. It > > doesn't seem that you should redirect to local NTP in those cases. Right? > > > > There are some checks in InstantController where you return if the URL doesn't > > match the instant_url of the template, etc., but what if those checks pass? > Say > > for example that there's a Back/Forward navigation to the homepage or a > > history.pushState (say I clicked on the Google logo from the search results to > > go back to the homepage) or some combination of the two. Is it possible for > any > > of these to return 204? (Not that the server actually returns a 204, but say > > because of history.pushState semantics or Back/Forward cache?) > > > > Basically, I'd like us to be more conservative when deciding that it's okay to > > replace an InstantTab contents. For example, if we have not yet determined > > instant_support for it. > > This does not seem like a coherent concern to me. A history.pushState won't > have an error http_status_code (if it even triggers this function at all, which > I doubt, as it's not a navigation), and everything else is an active navigation > to a homepage URL, which should always fall back on error in all cases. If I > hit the back button to navigate to the NTP and I'm now offline then it should in > fact fall back to the local page. > > A page that has failed to load and has an error status code will never have > instant_support because it hasn't loaded. I have verified that history.pushState does not result in this function being called.
LGTM My main concern is the nav stack busting thing mentioned below. However, it's tricky to try and copy or preserve nav entries (without introducing crashes), so I'll accept a TODO for now, if you don't feel like spending your Sunday figuring this out. Some other comments (mostly FYI): IsWaitingForResponse() may not be what we want. I think what we want is really "has this page finished loading". But it's okay, because IsWaitingForResponse() is strictly better than the current situation. I'm reasonably confident that if the check is false, we'll eventually observe the DidFinishLoad, so there should never be a situation where we never determine instant support, but I haven't actually verified this assumption. InstantTab::ShouldProcessAboutToNavigateMainFrame is too aggressive. We'll end up sending the theme/etc info on _every_ navigation in that tab, including to non-instant sites (depending on which observer - InstantPage or SearchTabHelper - sees the provisional load commit first). But luckily: 1. We'll soon protect against spurious state updates (cf: @skanuj's CL: https://codereview.chromium.org/14805004/). 2. Since the SearchBox extension isn't actually registered in a non-instant process, other sites can't get this info. Preloading the local NTP/overlay when the server versions fail is mildly inconsistent with the rest of the code (which tries to never preload them). Of course, it's a speed benefit, so it's a good thing. https://codereview.chromium.org/14043009/diff/67001/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_extended_interactive_uitest.cc (right): https://codereview.chromium.org/14043009/diff/67001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_extended_interactive_uitest.cc:2106: // NTP contents should be preloaded. On 2013/05/03 06:14:44, David Black wrote: > On 2013/05/03 04:39:17, samarth wrote: > > So you're relying on the fact that initially we try to load google by default > > which will fail because these tests can't reach the internet? Seems somewhat > > brittle but I don't have any better ideas. Perhaps Sreeram will. > > Yep. I can't use the normal way of overriding the URL used in other tests as > the browser object for the new window goes ahead and navigates before I get a > chance to set anything on it. I could override by setting --instant-url, but I > actually sort of like the test as-is, as it's exactly this loading-google > behavior that caught the bug I mentioned to you earlier. I think the right way would be to set the template URL data to point instant_url to a non-existent URL. However, it's okay. I won't sweat this test. https://codereview.chromium.org/14043009/diff/87002/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_controller.cc (right): https://codereview.chromium.org/14043009/diff/87002/chrome/browser/ui/search/... chrome/browser/ui/search/instant_controller.cc:731: const GURL current_url = contents->GetURL(); const GURL& https://codereview.chromium.org/14043009/diff/87002/chrome/browser/ui/search/... chrome/browser/ui/search/instant_controller.cc:1763: // entry. Won't this nuke the "Forward" history in the Back/Forward nav stack? For example, if the user hits "Back" to go back to the Google NTP, and it 404s or the user is offline. It's fine to switch to the local NTP, but we shouldn't nuke the nav stack. Also, PAGE_TRANSITION_SERVER_REDIRECT isn't strictly true, but we can let that one slide for now.
FYI https://codereview.chromium.org/14043009/diff/87002/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_controller.cc (right): https://codereview.chromium.org/14043009/diff/87002/chrome/browser/ui/search/... chrome/browser/ui/search/instant_controller.cc:736: LOG_INSTANT_DEBUG_EVENT(this, "InstantPageLoadFailed: instant_tab"); So when do we get in here exactly? I think just on a remote NTP or google.com/webhp if it fails to load (instant_url has /webhp right)? It would be moderately weird if visiting google.com when offline took me to the local NTP (since for instance, I might visit google.com to try to figure out if my internet works). https://codereview.chromium.org/14043009/diff/87002/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_extended_interactive_uitest.cc (right): https://codereview.chromium.org/14043009/diff/87002/chrome/browser/ui/search/... chrome/browser/ui/search/instant_extended_interactive_uitest.cc:2298: public InstantTestBase { indent more
https://codereview.chromium.org/14043009/diff/87002/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_controller.cc (right): https://codereview.chromium.org/14043009/diff/87002/chrome/browser/ui/search/... chrome/browser/ui/search/instant_controller.cc:736: LOG_INSTANT_DEBUG_EVENT(this, "InstantPageLoadFailed: instant_tab"); On 2013/05/05 15:08:00, Jered wrote: > So when do we get in here exactly? I think just on a remote NTP or > google.com/webhp if it fails to load (instant_url has /webhp right)? It would be > moderately weird if visiting http://google.com when offline took me to the local NTP > (since for instance, I might visit http://google.com to try to figure out if my > internet works). Yes, instant_url has /webhp. But, it's possible that when users start typing "google.com", a history autocompletion might lead them to "https://www.google.com/webhp". As it turns out, there's another small layer of protection. In general, we only hook up |instant_tab_| when the |search_mode_| is correct. So, if you start from a non-NTP/SERP (so instant_tab_ is NULL to begin with), when you navigate to "https://www.google.com/webhp", search_mode_ won't be updated to become NTP (because it lacks the "espv" param). So, instant_tab_ will remain NULL, and no swapping shenanigans will take place. If you start from a local NTP, we won't swap, so we are still good. (So, if you are offline, and you open a new tab to check out google.com, there still won't be a swap, because you started from a local NTP). There is a risk that you might start from a Google NTP/SERP and type "https://www.google.com/webhp". But I think this risk is small. Moreover, given that you were looking at a valid Google page before, it shouldn't be a surprise that you continued seeing a valid Google-like page (the local NTP), even if you were actually offline. It is also a risk if you happen to type "https://www.google.com/webhp?espv=2" (perhaps aided by an autocompletion). If you are offline or there's a 404 or such, we will nevertheless hook up instant_tab_ (since search_mode_ will be NTP). But search_mode_ is only updated after the provisional load commits (including failures), so the instant_tab_ hookup happens after the failure, so we won't observe any _new_ failure (and so, no swapping). Also, OnInstantSupportDetermined() will fire, and reset instant_tab_ to NULL again. So overall, I think the risk of this confusion is small. Of course, I haven't actually tested any of my claims / theories above. Somebody should. This points to a different concern with the local NTP though. If you are offline and you open a new tab, you see what looks like the google.com page. Many users might simply conclude therefore that Google is reachable and the problem is with whatever website they were trying to access.
https://codereview.chromium.org/14043009/diff/87002/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_controller.cc (right): https://codereview.chromium.org/14043009/diff/87002/chrome/browser/ui/search/... chrome/browser/ui/search/instant_controller.cc:736: LOG_INSTANT_DEBUG_EVENT(this, "InstantPageLoadFailed: instant_tab"); On 2013/05/05 16:38:13, sreeram wrote: > On 2013/05/05 15:08:00, Jered wrote: > > So when do we get in here exactly? I think just on a remote NTP or > > google.com/webhp if it fails to load (instant_url has /webhp right)? It would > be > > moderately weird if visiting http://google.com when offline took me to the > local NTP > > (since for instance, I might visit http://google.com to try to figure out if > my > > internet works). > > Yes, instant_url has /webhp. But, it's possible that when users start typing > "google.com", a history autocompletion might lead them to > "https://www.google.com/webhp". > > As it turns out, there's another small layer of protection. In general, we only > hook up |instant_tab_| when the |search_mode_| is correct. So, if you start from > a non-NTP/SERP (so instant_tab_ is NULL to begin with), when you navigate to > "https://www.google.com/webhp", search_mode_ won't be updated to become NTP > (because it lacks the "espv" param). So, instant_tab_ will remain NULL, and no > swapping shenanigans will take place. > > If you start from a local NTP, we won't swap, so we are still good. (So, if you > are offline, and you open a new tab to check out http://google.com, there still won't > be a swap, because you started from a local NTP). > > There is a risk that you might start from a Google NTP/SERP and type > "https://www.google.com/webhp". But I think this risk is small. Moreover, given > that you were looking at a valid Google page before, it shouldn't be a surprise > that you continued seeing a valid Google-like page (the local NTP), even if you > were actually offline. > > It is also a risk if you happen to type "https://www.google.com/webhp?espv=2" > (perhaps aided by an autocompletion). If you are offline or there's a 404 or > such, we will nevertheless hook up instant_tab_ (since search_mode_ will be > NTP). But search_mode_ is only updated after the provisional load commits > (including failures), so the instant_tab_ hookup happens after the failure, so > we won't observe any _new_ failure (and so, no swapping). Also, > OnInstantSupportDetermined() will fire, and reset instant_tab_ to NULL again. > > So overall, I think the risk of this confusion is small. Of course, I haven't > actually tested any of my claims / theories above. Somebody should. > > This points to a different concern with the local NTP though. If you are offline > and you open a new tab, you see what looks like the http://google.com page. Many users > might simply conclude therefore that Google is reachable and the problem is with > whatever website they were trying to access. Wow, ok, thanks for the explanation. Wheels within wheels. I will patch this and try it Monday.
https://codereview.chromium.org/14043009/diff/87002/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_controller.cc (right): https://codereview.chromium.org/14043009/diff/87002/chrome/browser/ui/search/... chrome/browser/ui/search/instant_controller.cc:731: const GURL current_url = contents->GetURL(); On 2013/05/05 08:40:39, sreeram wrote: > const GURL& Done. https://codereview.chromium.org/14043009/diff/87002/chrome/browser/ui/search/... chrome/browser/ui/search/instant_controller.cc:1763: // entry. On 2013/05/05 08:40:39, sreeram wrote: > Won't this nuke the "Forward" history in the Back/Forward nav stack? For > example, if the user hits "Back" to go back to the Google NTP, and it 404s or > the user is offline. It's fine to switch to the local NTP, but we shouldn't nuke > the nav stack. > > Also, PAGE_TRANSITION_SERVER_REDIRECT isn't strictly true, but we can let that > one slide for now. Yes, good point. Added a check to not redirect instant_tab_ if there exists forward history. https://codereview.chromium.org/14043009/diff/87002/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_extended_interactive_uitest.cc (right): https://codereview.chromium.org/14043009/diff/87002/chrome/browser/ui/search/... chrome/browser/ui/search/instant_extended_interactive_uitest.cc:2298: public InstantTestBase { On 2013/05/05 15:08:00, Jered wrote: > indent more Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dcblack@chromium.org/14043009/98001
Message was sent while issue was closed.
Change committed as 198376 |
