Instant Extended: Fallback to local preview if the remote instant page is not ready on user input.

Instant Extended: Fallback to local preview if the remote instant page is not ready on user input.

Reverts back to the remote URL when the preview is not showing and the omnibox looses focus.

BUG=159289
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=180520

[Shishir, 2013-01-10 22:26:28]

This CL is built on top of 11829062.

PTAL.

[sreeram, 2013-01-22 03:58:03]

https://chromiumcodereview.appspot.com/11833043/diff/2001/chrome/browser/inst...
File chrome/browser/instant/instant_loader.h (right):

https://chromiumcodereview.appspot.com/11833043/diff/2001/chrome/browser/inst...
chrome/browser/instant/instant_loader.h:87: bool is_in_fallback_mode() const {
return is_in_fallback_mode_; }
Do we really need this? Can't we just use IsUsingLocalPreview()? If you are
thinking about incognito mode, we can surely adjust for it by also checking
use_local_preview_only_ appropriately.

https://chromiumcodereview.appspot.com/11833043/diff/2001/chrome/browser/inst...
chrome/browser/instant/instant_loader.h:106: void InitializeFonts();
I realize that the reason why this was being done in InstantSupportDetermined
previously was so that the page was guaranteed to get an event with the font
information. I think that's wrong. The page should just check the font name and
size when it loads. If they are non-blank, it should use that information right
away. Otherwise, when they become available, the page will get an event, as
expected. In other words, the font information should be treated exactly the
same way searchbox.value is treated.

We should convey this to the google.com folks internally, so they know what to
do if stuff breaks due to this CL.

https://chromiumcodereview.appspot.com/11833043/diff/2001/chrome/browser/reso...
File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js
(right):

https://chromiumcodereview.appspot.com/11833043/diff/2001/chrome/browser/reso...
chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:231:
appendSuggestionStyles();
This isn't quite right. It's possible that the page loads so fast that we end up
here even before the font info IPC has been received. So, you should do the same
as suggested for google.com: Set up the font information event handler. Then,
check font name/size. If it's non-blank, go ahead and call
appendSuggestionStyles(). Otherwise, it will be called from the event handler.
Also, inside handleNativeSuggestions(), if we still haven't received any font
information, don't try to show suggestions.

[Shishir, 2013-01-25 22:11:25]

https://codereview.chromium.org/11833043/diff/2001/chrome/browser/instant/ins...
File chrome/browser/instant/instant_loader.h (right):

https://codereview.chromium.org/11833043/diff/2001/chrome/browser/instant/ins...
chrome/browser/instant/instant_loader.h:87: bool is_in_fallback_mode() const {
return is_in_fallback_mode_; }
On 2013/01/22 03:58:03, sreeram wrote:
> Do we really need this? Can't we just use IsUsingLocalPreview()? If you are
> thinking about incognito mode, we can surely adjust for it by also checking
> use_local_preview_only_ appropriately.

There are a couple of reasons why I think its better to have this. 

1. Using use_local_preview_only_and IsUsingLocalPreview() will not help us
distinguish the case where we are using the local preview because the instant
page is temporarily slow vs the case where we are using it becuase all other
instant urls are blacklisted (unless we want to call GetInstantURL).

2. We need EnsureLoaderIsCurrent to not delete the loader if it is in fallback
mode and delete it once MaybeSwitchLocalLoader is called.

I will send out an alternate patch that works around 1 and 2 but I am not sure
it is the better approach.

https://codereview.chromium.org/11833043/diff/2001/chrome/browser/instant/ins...
chrome/browser/instant/instant_loader.h:106: void InitializeFonts();
On 2013/01/22 03:58:03, sreeram wrote:
> I realize that the reason why this was being done in InstantSupportDetermined
> previously was so that the page was guaranteed to get an event with the font
> information. I think that's wrong. The page should just check the font name
and
> size when it loads. If they are non-blank, it should use that information
right
> away. Otherwise, when they become available, the page will get an event, as
> expected. In other words, the font information should be treated exactly the
> same way searchbox.value is treated.
> 
> We should convey this to the http://google.com folks internally, so they know
what to
> do if stuff breaks due to this CL.

As we discussed, the font information will now always be available as we
initialize it in RenderViewCreated.

https://codereview.chromium.org/11833043/diff/2001/chrome/browser/resources/l...
File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js
(right):

https://codereview.chromium.org/11833043/diff/2001/chrome/browser/resources/l...
chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:231:
appendSuggestionStyles();
On 2013/01/22 03:58:03, sreeram wrote:
> This isn't quite right. It's possible that the page loads so fast that we end
up
> here even before the font info IPC has been received. So, you should do the
same
> as suggested for google.com: Set up the font information event handler. Then,
> check font name/size. If it's non-blank, go ahead and call
> appendSuggestionStyles(). Otherwise, it will be called from the event handler.
> Also, inside handleNativeSuggestions(), if we still haven't received any font
> information, don't try to show suggestions.

With the change to push font info in RenderViewCreated, this should now work.

[sreeram, 2013-01-29 17:02:21]

https://codereview.chromium.org/11833043/diff/2001/chrome/browser/instant/ins...
File chrome/browser/instant/instant_loader.h (right):

https://codereview.chromium.org/11833043/diff/2001/chrome/browser/instant/ins...
chrome/browser/instant/instant_loader.h:87: bool is_in_fallback_mode() const {
return is_in_fallback_mode_; }
On 2013/01/25 22:11:26, Shishir wrote:
> On 2013/01/22 03:58:03, sreeram wrote:
> > Do we really need this? Can't we just use IsUsingLocalPreview()? If you are
> > thinking about incognito mode, we can surely adjust for it by also checking
> > use_local_preview_only_ appropriately.
> 
> There are a couple of reasons why I think its better to have this. 
> 
> 1. Using use_local_preview_only_and IsUsingLocalPreview() will not help us
> distinguish the case where we are using the local preview because the instant
> page is temporarily slow vs the case where we are using it becuase all other
> instant urls are blacklisted (unless we want to call GetInstantURL).
> 
> 2. We need EnsureLoaderIsCurrent to not delete the loader if it is in fallback
> mode and delete it once MaybeSwitchLocalLoader is called.
> 
> I will send out an alternate patch that works around 1 and 2 but I am not sure
> it is the better approach.

Re: 1, I don't see the need to distinguish the cases. So yes, we should just use
GetInstantURL() and use it if it's not blacklisted.

Re: 2, Why not? In all the cases I see, EnsureLoaderIsCurrent() is only being
called when the preview (whether local or remote) is not being used. So, it
should go ahead and delete the preview and try the remote URL if possible.

[Shishir, 2013-01-29 20:06:31]

https://codereview.chromium.org/11833043/diff/2001/chrome/browser/instant/ins...
File chrome/browser/instant/instant_loader.h (right):

https://codereview.chromium.org/11833043/diff/2001/chrome/browser/instant/ins...
chrome/browser/instant/instant_loader.h:87: bool is_in_fallback_mode() const {
return is_in_fallback_mode_; }
On 2013/01/29 17:02:21, sreeram wrote:
> On 2013/01/25 22:11:26, Shishir wrote:
> > On 2013/01/22 03:58:03, sreeram wrote:
> > > Do we really need this? Can't we just use IsUsingLocalPreview()? If you
are
> > > thinking about incognito mode, we can surely adjust for it by also
checking
> > > use_local_preview_only_ appropriately.
> > 
> > There are a couple of reasons why I think its better to have this. 
> > 
> > 1. Using use_local_preview_only_and IsUsingLocalPreview() will not help us
> > distinguish the case where we are using the local preview because the
instant
> > page is temporarily slow vs the case where we are using it becuase all other
> > instant urls are blacklisted (unless we want to call GetInstantURL).
> > 
> > 2. We need EnsureLoaderIsCurrent to not delete the loader if it is in
fallback
> > mode and delete it once MaybeSwitchLocalLoader is called.
> > 
> > I will send out an alternate patch that works around 1 and 2 but I am not
sure
> > it is the better approach.
> 
> Re: 1, I don't see the need to distinguish the cases. So yes, we should just
use
> GetInstantURL() and use it if it's not blacklisted.
> 
> Re: 2, Why not? In all the cases I see, EnsureLoaderIsCurrent() is only being
> called when the preview (whether local or remote) is not being used. So, it
> should go ahead and delete the preview and try the remote URL if possible.

As discussed, we want to keep the EnsureLoaderIsCurrent call in Update. Made the
change to check fallback mode only in update.

[Shishir, 2013-01-29 21:52:25]

Fixed tests. PTAL.

[sreeram, 2013-01-30 19:28:01]

On 2013/01/29 20:06:31, Shishir wrote:
> As discussed, we want to keep the EnsureLoaderIsCurrent call in Update. Made
the
> change to check fallback mode only in update.

It seems to me that we _still_ don't need the "is fallback mode" bit. Why can't
we just do this in Update():

if (!instant_tab_ && !loader_->IsUsingLocalPreview() &&
!EnsureLoaderIsCurrent()) {
  HideLoader();
  return false;
}

if (extended_enabled_ && !loader_->supports_instant() &&
!loader_->IsUsingLocalPreview())
  CreateLoader(kLocalOmniboxPopupURL, ...);

[sreeram, 2013-01-30 19:28:07]

https://codereview.chromium.org/11833043/diff/16003/chrome/browser/instant/in...
File chrome/browser/instant/instant_controller.h (right):

https://codereview.chromium.org/11833043/diff/16003/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:196: int end_margin() const { return
end_margin_; }
Won't it be simpler to just have the InstantLoader call into the
InstantController (say controller_->RenderViewCreated()) and have the
InstantController perform all the initializations? Especially since some of
these are already APIs that the IC calls on the IL (such as SetMarginSize(),
SetDisplayInstantResults, etc). Then you don't need all these accessors.

[sreeram, 2013-01-30 19:29:25]

https://codereview.chromium.org/11833043/diff/16003/chrome/browser/instant/in...
File chrome/browser/instant/instant_test_utils.cc (right):

https://codereview.chromium.org/11833043/diff/16003/chrome/browser/instant/in...
chrome/browser/instant/instant_test_utils.cc:61:
instant()->SetInstantEnabled(true);
Yuck. Okay, I'll clean this up soon. Please add:
// TODO(sreeram): Fix this ugly hack.

[Shishir, 2013-01-31 23:46:47]

PTAL.

https://codereview.chromium.org/11833043/diff/16003/chrome/browser/instant/in...
File chrome/browser/instant/instant_controller.h (right):

https://codereview.chromium.org/11833043/diff/16003/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:196: int end_margin() const { return
end_margin_; }
On 2013/01/30 19:28:08, sreeram wrote:
> Won't it be simpler to just have the InstantLoader call into the
> InstantController (say controller_->RenderViewCreated()) and have the
> InstantController perform all the initializations? Especially since some of
> these are already APIs that the IC calls on the IL (such as SetMarginSize(),
> SetDisplayInstantResults, etc). Then you don't need all these accessors.

Done.

https://codereview.chromium.org/11833043/diff/16003/chrome/browser/instant/in...
File chrome/browser/instant/instant_test_utils.cc (right):

https://codereview.chromium.org/11833043/diff/16003/chrome/browser/instant/in...
chrome/browser/instant/instant_test_utils.cc:61:
instant()->SetInstantEnabled(true);
On 2013/01/30 19:29:25, sreeram wrote:
> Yuck. Okay, I'll clean this up soon. Please add:
> // TODO(sreeram): Fix this ugly hack.

Done.

[sreeram, 2013-02-01 21:21:42]

lgtm

[commit-bot: I haz the power, 2013-02-01 21:31:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shishir@chromium.org/11833043/7014

[commit-bot: I haz the power, 2013-02-01 21:32:10]

Presubmit check for 11833043-7014 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/resources

Presubmit checks took 5.3s to calculate.

[Shishir, 2013-02-01 21:33:44]

Adding estade for chrome/browser/resources.

PTAL.

[Evan Stade, 2013-02-04 19:14:44]

lgtm

[commit-bot: I haz the power, 2013-02-04 19:18:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shishir@chromium.org/11833043/7014

[commit-bot: I haz the power, 2013-02-04 21:27:58]

Change committed as 180520