InstantExtended: Transient naventry for preview.

InstantExtended: Transient naventry for preview.

Adds a transient navigation entry to the active tab when an uncommited
preview is made full height. This makes the back button go back to the
page the user searched from, rather than the page before it; and causes
the tab title to show "Google" when it is previewing search results,
rather than the title of the page the user searched from.

BUG=170479

[Jered, 2013-01-17 19:21:05]

Please review
sreeram -> instant
sky -> content
samarth fyi

[sky, 2013-01-17 20:28:39]

sky->brettw . Brett, only look at the content changes.

[sreeram, 2013-01-17 22:00:08]

This change scares me a bit, because the transient entry is what is returned by
GetActiveEntry() and GetVisibleEntry(), which are used in many places. So, I'd
like to take some time to fully understand what the implications are.

For example, as mentioned offline, if you hit Reload, we end up reloading the
transient entry, thus navigating to the Google homepage. Regardless of what we
think should happen if the user hits Reload (i.e., whether the search preview
should reload or whether the previous committed page should reload), reloading
the Google homepage is clearly wrong.

So anyway, long story short, give me some time to look through the implications
carefully. More eyes are better, so please also comb through the callers of
GetActiveEntry() and such carefully as well.

[brettw, 2013-01-18 21:44:11]

I'm scared about adding a transient entry. I'd also like more info on the
"blowing away the old transient entry" case. If the current page is an SSL error
page, you do instant and then cancel that, is the error page now gone?

[Jered, 2013-01-19 00:09:29]

On 2013/01/18 21:44:11, brettw wrote:
> I'm scared about adding a transient entry. I'd also like more info on the
> "blowing away the old transient entry" case. If the current page is an SSL
error
> page, you do instant and then cancel that, is the error page now gone?

PTAL.

In PS3, I save any existing transient history entry before Instant replaces it,
and restore it when Instant is discarded. I also update the transient URL with
the query in progress so that it gets reloaded and committed if the user presses
reload.

If the user activates Instant on an interstitial page, then presses escape, the
interstitial page will be restored as expected. If they press back, they'll go
back before the interstitial page.

[sreeram, 2013-01-22 05:28:34]

I'm still worried :) Convince me that this change is safe.

In particular, now I wonder whether it's possible for an existing transient
entry to be cleared while the Instant preview is showing. I.e., through one of
the myriad ways that
NavigationControllerImpl::DiscardNonCommittedEntriesInternal() can be called. If
so, then when the Instant preview is dismissed (say because the user now hits
Escape), we'll end up restoring a transient entry that's no longer valid.

Also, it seems that you'll end up hitting the DCHECK() in
NavigationControllerImpl::CopyStateFromAndPrune() when the Instant preview is
committed. No?

https://chromiumcodereview.appspot.com/12001002/diff/9001/chrome/browser/inst...
File chrome/browser/instant/instant_controller.cc (right):

https://chromiumcodereview.appspot.com/12001002/diff/9001/chrome/browser/inst...
chrome/browser/instant/instant_controller.cc:1186: instant_set_transient_entry_
= true;
Wow. This is really hacky. Why do we need this? Can't we just update the
transient entry URL whenever the InstantLoader observes a navigation? Yes, if
you type quickly and press Reload (before 3 seconds are up), you'll end up
reloading the "wrong" URL, but I'm not really concerned about that. I think it's
an extreme corner case brought about by the user's pathological action that we
don't have to worry about.

[Jered, 2013-01-23 21:52:00]

On 2013/01/22 05:28:34, sreeram wrote:
> I'm still worried :) Convince me that this change is safe.
> 
> In particular, now I wonder whether it's possible for an existing transient
> entry to be cleared while the Instant preview is showing. I.e., through one of
> the myriad ways that
> NavigationControllerImpl::DiscardNonCommittedEntriesInternal() can be called.
If
> so, then when the Instant preview is dismissed (say because the user now hits
> Escape), we'll end up restoring a transient entry that's no longer valid.
> 
> Also, it seems that you'll end up hitting the DCHECK() in
> NavigationControllerImpl::CopyStateFromAndPrune() when the Instant preview is
> committed. No?
> 
>
https://chromiumcodereview.appspot.com/12001002/diff/9001/chrome/browser/inst...
> File chrome/browser/instant/instant_controller.cc (right):
> 
>
https://chromiumcodereview.appspot.com/12001002/diff/9001/chrome/browser/inst...
> chrome/browser/instant/instant_controller.cc:1186:
instant_set_transient_entry_
> = true;
> Wow. This is really hacky. Why do we need this? Can't we just update the
> transient entry URL whenever the InstantLoader observes a navigation? Yes, if
> you type quickly and press Reload (before 3 seconds are up), you'll end up
> reloading the "wrong" URL, but I'm not really concerned about that. I think
it's
> an extreme corner case brought about by the user's pathological action that we
> don't have to worry about.

A bit distracted lately but one quick question for you below...

[Jered, 2013-01-23 21:52:07]

https://chromiumcodereview.appspot.com/12001002/diff/9001/chrome/browser/inst...
File chrome/browser/instant/instant_controller.cc (right):

https://chromiumcodereview.appspot.com/12001002/diff/9001/chrome/browser/inst...
chrome/browser/instant/instant_controller.cc:1186: instant_set_transient_entry_
= true;
On 2013/01/22 05:28:34, sreeram wrote:
> Wow. This is really hacky. Why do we need this? Can't we just update the
> transient entry URL whenever the InstantLoader observes a navigation? Yes, if
> you type quickly and press Reload (before 3 seconds are up), you'll end up
> reloading the "wrong" URL, but I'm not really concerned about that. I think
it's
> an extreme corner case brought about by the user's pathological action that we
> don't have to worry about.

Navigating after 3 seconds is a very Google-specific behavior, so depending on
that would feel like more of a hack to me. This way seems like a portable way to
make reload search for what the user has typed, no?

[sreeram, 2013-01-23 22:07:09]

On 2013/01/23 21:52:07, Jered wrote:
> Navigating after 3 seconds is a very Google-specific behavior, so depending on
> that would feel like more of a hack to me.

Without Instant, if you type something into the omnibox (without pressing Enter)
and hit Reload, it of course reloads the underlying permanent page's URL. But if
you had modified the underlying page's DOM in ways that are not reflected in the
URL, then hitting Reload wouldn't give you back the same page state. I think we
all agree that there's really no way around this.

Similarly, with a full-page Instant preview obscuring the underlying page, the
case we are making is that the user sees it as a regular tab, so hitting Reload
should reload the transient overlay. Fine. I'd argue that similarly, we should
reload the URL of the overlay, whatever it is. If the page is showing some state
in the overlay that it has not yet updated into the URL, then it's equivalent to
the above case.

So, it's not so much that we "depend" on the Google-specific 3-second behaviour
(or are somehow making exceptions for it). We are saying that the page should
maintain state in the URL, so that Reload works as expected. (Alternatively,
maintain state in other places - cookies, sessionStorage, etc. - that the page
can use to bring back the state to what it was actually showing.) This applies
equally to an InstantTab, for example.

> This way seems like a portable way to
> make reload search for what the user has typed, no?

I don't necessarily think this is portable. For example, there's no guarantee
that the "search_url" of the template will do the same thing as the instant_url
given a query. We are showing the latter as the overlay, but will be reloading
the former.

[Jered, 2013-01-24 19:25:08]

On 2013/01/23 22:07:09, sreeram wrote:
> On 2013/01/23 21:52:07, Jered wrote:
> > Navigating after 3 seconds is a very Google-specific behavior, so depending
on
> > that would feel like more of a hack to me.
> 
> Without Instant, if you type something into the omnibox (without pressing
Enter)
> and hit Reload, it of course reloads the underlying permanent page's URL. But
if
> you had modified the underlying page's DOM in ways that are not reflected in
the
> URL, then hitting Reload wouldn't give you back the same page state. I think
we
> all agree that there's really no way around this.
> 
> Similarly, with a full-page Instant preview obscuring the underlying page, the
> case we are making is that the user sees it as a regular tab, so hitting
Reload
> should reload the transient overlay. Fine. I'd argue that similarly, we should
> reload the URL of the overlay, whatever it is. If the page is showing some
state
> in the overlay that it has not yet updated into the URL, then it's equivalent
to
> the above case.
> 
> So, it's not so much that we "depend" on the Google-specific 3-second
behaviour
> (or are somehow making exceptions for it). We are saying that the page should
> maintain state in the URL, so that Reload works as expected. (Alternatively,
> maintain state in other places - cookies, sessionStorage, etc. - that the page
> can use to bring back the state to what it was actually showing.) This applies
> equally to an InstantTab, for example.

This seems reasonable, provided that we're ok with the behavior that, if you
click reload prior to Google's 3 second pause, the tab will reload the instant
url.

> 
> > This way seems like a portable way to
> > make reload search for what the user has typed, no?
> 
> I don't necessarily think this is portable. For example, there's no guarantee
> that the "search_url" of the template will do the same thing as the
instant_url
> given a query. We are showing the latter as the overlay, but will be reloading
> the former.

Fair point.

[sreeram, 2013-01-24 19:39:03]

On 2013/01/24 19:25:08, Jered wrote:
> This seems reasonable, provided that we're ok with the behavior that, if you
> click reload prior to Google's 3 second pause, the tab will reload the instant
> url.

Heh :) Which directly contradicts what I said earlier about "reloading the
Google homepage is clearly wrong". Gah. I'm sick of munging the URL and playing
games with virtual URLs and such, so I think we should ignore my comment about
"clearly wrong". So yeah, reload the instant_url if we have to. Fine. I doubt
this is going to be a significant issue (same behaviour if you go to google.com,
type and quickly hit Ctrl-R). Even if it turns out to be, Google can take steps
to avoid it.

[Jered, 2013-01-25 00:17:52]

PTAL sreeram. PS5 has the new behavior we talked about.

I spent a long time trying to trace through the code by inspection, but it's
just too twisted. Instead I printed out stack traces wherever relevant methods
were called for a set of actions. It's a bit confusing since there're a lot of
random notifications flying around.

I think we're really only concerned with actions that cause navigation on top of
a transient page. Here are the cases I tried, and how they worked out:

- On an interstitial page, type 'q', wait for results, click back.
1. Add an entry for Instant when results are shown.
   ShowLoader() -> SetTransientHistoryEntry()
2. When back is pressed, reset search mode based on transient entry.
   ToolbarView::ButtonPressed() -> OmniboxEditModel::Revert() ->
chrome::search::SearchTabHelper::UpdateModelBasedOnURL() -> SearchModeChanged()
2:3
3. Search mode immediately causes HideLoader() -> ResetTransientHistoryEntry()
4. This resets the original transient history entry.
   chrome::search::SearchTabHelper::NavigationEntryUpdated() ->
SearchModeChanged() 0:0
5. Finally we get the back button event, discarding the original entry.
   chrome::GoBack() -> NavigationControllerImpl::DiscardNonCommittedEntries()

- On an interstitial page, type 'q', wait for results, press escape.
1. Add an entry for Instant when results are shown.
   ShowLoader() -> SetTransientHistoryEntry()
2. When escape is pressed, discard Instant's entry.
   OmniboxEditModel::OnEscapeKeyPressed() -> Nav::DiscardTransientEntry()
3. Restore the original entry.
   HideLoader() -> ResetTransientHistoryEntry()

- On an interstitial page, type 'q', wait for results, press enter.
1. Add an entry for Instant when results are shown.
   ShowLoader() -> SetTransientHistoryEntry()
2. OnSubmit, HideLoader() calls ResetTransientHistoryEntry().
3. LoadURL causes NavigationControllerImpl::DiscardTransientEntry().

- On an interstitial page, click a link to navigate.
1. Add an entry for Instant when results are shown.
   ShowLoader() -> SetTransientHistoryEntry()
2. Updates transient entry on navigation. ** note this doesn't commit the page.
   InstantClient::DidCommitProvisionalLoadForFrame() ->
UpdateTransientHistoryEntry()
3. Commit on omnibox lost focus, reset the original transient history entry.
   OmniboxLostFocus() -> CommitIfPossible() -> SearchModeChanged() ->
HideLoader() -> ResetTransientHistoryEntry()
4. About a bazillion calls on the active tab discard the transient entry, e.g.
   ~WebContentsImpl() -> InterstitialPageImpl::DontProceed(),
   NavigationControllerImpl::RendererDidNavigate() ->
NavigationControllerImpl::DiscardTransientEntry().

[sreeram, 2013-01-25 18:17:13]

On 2013/01/25 00:17:52, Jered wrote:
> I think we're really only concerned with actions that cause navigation on top
of
> a transient page. Here are the cases I tried, and how they worked out:
>
> [snip]

Thanks for the investigation! I appreciate it.

Meanwhile, I came up with one case that fails: Closing a tab: Say you are on tab
A and you type into the omnibox (no Enter). We create the transient entry for
Instant. Now you close the tab. Hide() is called, but only after the active tab
has already changed to say tab B. So we end up removing the transient entry from
a _different_ tab (if such an entry exists).

I would think the fix is simple: Add logic in
InstantController::TabDeactivated() to call ResetTransientEntry() say if the tab
is closing. Note that we can't just clear the state variables in
InstantController alone. We need to restore any previous transient entry (such
as an SSL interstitial) of the closing tab, to make sure tab-restore does the
right thing. The only thing I'm not sure of is whether such munging of the
NavigationController is a safe thing to do when the tab is already considered to
be closing.

[sreeram, 2013-01-25 18:17:28]

https://codereview.chromium.org/12001002/diff/12002/chrome/browser/instant/in...
File chrome/browser/instant/instant_controller.cc (right):

https://codereview.chromium.org/12001002/diff/12002/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:8: #include
"base/debug/stack_trace.h"
Remove this before submitting.

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

https://codereview.chromium.org/12001002/diff/12002/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:229: // Copies the loader's
NavigationEntry into a transient history entry for the
loader's NavigationEntry -> loader's active NavigationEntry

[Jered, 2013-01-25 19:27:18]

Thanks for catching the tab close issue, fixed.

https://chromiumcodereview.appspot.com/12001002/diff/12002/chrome/browser/ins...
File chrome/browser/instant/instant_controller.cc (right):

https://chromiumcodereview.appspot.com/12001002/diff/12002/chrome/browser/ins...
chrome/browser/instant/instant_controller.cc:8: #include
"base/debug/stack_trace.h"
On 2013/01/25 18:17:29, sreeram wrote:
> Remove this before submitting.

Done.

https://chromiumcodereview.appspot.com/12001002/diff/12002/chrome/browser/ins...
File chrome/browser/instant/instant_controller.h (right):

https://chromiumcodereview.appspot.com/12001002/diff/12002/chrome/browser/ins...
chrome/browser/instant/instant_controller.h:229: // Copies the loader's
NavigationEntry into a transient history entry for the
On 2013/01/25 18:17:29, sreeram wrote:
> loader's NavigationEntry -> loader's active NavigationEntry

Done.

[sreeram, 2013-01-25 21:28:02]

https://codereview.chromium.org/12001002/diff/11002/chrome/browser/instant/in...
File chrome/browser/instant/instant_controller.cc (right):

https://codereview.chromium.org/12001002/diff/11002/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:699:
saved_transient_entry_.reset();
Doesn't this break tab-restore? Quoting myself from earlier:
"""
Note that we can't just clear the state variables in InstantController alone. We
need to restore any previous transient entry (such as an SSL interstitial) of
the closing tab, to make sure tab-restore does the right thing.
"""

Test this scenario:
1. Have at least two tabs.
2. On tab A, navigate to ianfette.org. This should cause a malware interstitial.
3. Focus the omnibox and type a query (no Enter). Wait for the preview to
appear.
4. Close the tab.
5. Ctrl-Shift-T to restore the tab.

I'd expect that with the CL as it is, it would try to restore the Instant
overlay. This may seem okay, but would actually lead to trouble further down.
Because now, the restored tab has a transient Instant overlay, but not one that
InstantController knows about. So if you focus the omnibox and start typing
again, we'll end up treating the existing transient entry (Instant-created) as
something like the SSL interstitial, and replace it with yet another
Instant-created entry. Maybe it can all work, but it makes my head hurt.

So, I'd settle for having tab-restore the malware interstitial seen at step 2.

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

https://codereview.chromium.org/12001002/diff/11002/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:235: // Updates the url in the
transient history entry for the overlay.
Nit: url -> URL

[Jered, 2013-02-04 16:35:42]

Hey Sreeram,

I am just coming back to this today. New PS leaves the transient entry alone
when one is already set (e.g. on interstitial pages). I think this is our safest
bet. As we've seen, interstitial pages already have some weird/questionable
behavior. I'd rather have better user behavior for the 99.9999% case and circle
back to deal with interstitial pages in Instant later.

On 2013/01/25 21:28:02, sreeram wrote:
>
https://codereview.chromium.org/12001002/diff/11002/chrome/browser/instant/in...
> File chrome/browser/instant/instant_controller.cc (right):
> 
>
https://codereview.chromium.org/12001002/diff/11002/chrome/browser/instant/in...
> chrome/browser/instant/instant_controller.cc:699:
> saved_transient_entry_.reset();
> Doesn't this break tab-restore? Quoting myself from earlier:
> """
> Note that we can't just clear the state variables in InstantController alone.
We
> need to restore any previous transient entry (such as an SSL interstitial) of
> the closing tab, to make sure tab-restore does the right thing.
> """
> 
> Test this scenario:
> 1. Have at least two tabs.
> 2. On tab A, navigate to http://ianfette.org. This should cause a malware
interstitial.
> 3. Focus the omnibox and type a query (no Enter). Wait for the preview to
> appear.
> 4. Close the tab.
> 5. Ctrl-Shift-T to restore the tab.
> 
> I'd expect that with the CL as it is, it would try to restore the Instant
> overlay. This may seem okay, but would actually lead to trouble further down.
> Because now, the restored tab has a transient Instant overlay, but not one
that
> InstantController knows about. So if you focus the omnibox and start typing
> again, we'll end up treating the existing transient entry (Instant-created) as
> something like the SSL interstitial, and replace it with yet another
> Instant-created entry. Maybe it can all work, but it makes my head hurt.
> 
> So, I'd settle for having tab-restore the malware interstitial seen at step 2.
> 
>
https://codereview.chromium.org/12001002/diff/11002/chrome/browser/instant/in...
> File chrome/browser/instant/instant_controller.h (right):
> 
>
https://codereview.chromium.org/12001002/diff/11002/chrome/browser/instant/in...
> chrome/browser/instant/instant_controller.h:235: // Updates the url in the
> transient history entry for the overlay.
> Nit: url -> URL

[Jered, 2013-02-08 20:09:57]

PS9 rebases on top of the committed NTP change.

On 2013/02/04 16:35:42, Jered wrote:
> Hey Sreeram,
> 
> I am just coming back to this today. New PS leaves the transient entry alone
> when one is already set (e.g. on interstitial pages). I think this is our
safest
> bet. As we've seen, interstitial pages already have some weird/questionable
> behavior. I'd rather have better user behavior for the 99.9999% case and
circle
> back to deal with interstitial pages in Instant later.
> 
> On 2013/01/25 21:28:02, sreeram wrote:
> >
>
https://codereview.chromium.org/12001002/diff/11002/chrome/browser/instant/in...
> > File chrome/browser/instant/instant_controller.cc (right):
> > 
> >
>
https://codereview.chromium.org/12001002/diff/11002/chrome/browser/instant/in...
> > chrome/browser/instant/instant_controller.cc:699:
> > saved_transient_entry_.reset();
> > Doesn't this break tab-restore? Quoting myself from earlier:
> > """
> > Note that we can't just clear the state variables in InstantController
alone.
> We
> > need to restore any previous transient entry (such as an SSL interstitial)
of
> > the closing tab, to make sure tab-restore does the right thing.
> > """
> > 
> > Test this scenario:
> > 1. Have at least two tabs.
> > 2. On tab A, navigate to http://ianfette.org. This should cause a malware
> interstitial.
> > 3. Focus the omnibox and type a query (no Enter). Wait for the preview to
> > appear.
> > 4. Close the tab.
> > 5. Ctrl-Shift-T to restore the tab.
> > 
> > I'd expect that with the CL as it is, it would try to restore the Instant
> > overlay. This may seem okay, but would actually lead to trouble further
down.
> > Because now, the restored tab has a transient Instant overlay, but not one
> that
> > InstantController knows about. So if you focus the omnibox and start typing
> > again, we'll end up treating the existing transient entry (Instant-created)
as
> > something like the SSL interstitial, and replace it with yet another
> > Instant-created entry. Maybe it can all work, but it makes my head hurt.
> > 
> > So, I'd settle for having tab-restore the malware interstitial seen at step
2.
> > 
> >
>
https://codereview.chromium.org/12001002/diff/11002/chrome/browser/instant/in...
> > File chrome/browser/instant/instant_controller.h (right):
> > 
> >
>
https://codereview.chromium.org/12001002/diff/11002/chrome/browser/instant/in...
> > chrome/browser/instant/instant_controller.h:235: // Updates the url in the
> > transient history entry for the overlay.
> > Nit: url -> URL

[sreeram, 2013-03-15 19:44:29]

Just a quick update: There's an offline discussion going on about the right
thing to do with respect to nav entries, Escape, Back button, etc. Let's revisit
this CL after the dust has settled on that thread.

[Jered, 2013-04-19 22:41:08]

On 2013/03/15 19:44:29, sreeram wrote:
> Just a quick update: There's an offline discussion going on about the right
> thing to do with respect to nav entries, Escape, Back button, etc. Let's
revisit
> this CL after the dust has settled on that thread.

I still think we should do this, but am abandoning this patch as it's hopelessly
old. I'll rebuild this when we have time.