Fix omnibox update bug.

Fix omnibox update bug.

When tab-switching happens, the state of the tab is stored so that it can be restored later. However, when a background tab is being closed, it doesn't cause a tab-switch, and trying to save the state causes problems in some cases (e.g. 61588). So do not save state of a background tab.

BUG=61588
TEST=see bug

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=66445

[sky, 2010-11-16 16:44:40]

On 2010/11/16 05:17:53, sadrul wrote:

Could you elaborate on what problems this is causing? I believe this is a some
what recent regression, but the code you're changing hasn't changed in a while
(my patch just moved some of the code around).

That said, I do agree we shouldn't bother saving state if the tab is not
selected.

  -Scott

[sadrul, 2010-11-16 17:30:49]

On 2010/11/16 16:44:40, sky wrote:
> On 2010/11/16 05:17:53, sadrul wrote:
> 
> Could you elaborate on what problems this is causing? 

When a tab gets closed, it triggers the following sequence:
 * TAB_CONTENTS_DESTROYED notification triggers
TabStripModel::DetachTabContentsAt
 * This triggers Browser::TabDetachedAt, which calls TabDetachedAtImpl with
DETACH_TAB_DETACH
 * This calls SaveStateToContents, which in turn calls SaveStateToTab. This
calls AutocompleteEditModel::GetStateForTabSwitch (I checked and this happens
from SaveStateToTab for all of gtk, win, mac).
 * If the user edits the location entry, (i.e. |user_input_in_progress_| is set)
and empties it, then GetStateForTabSwitch reverts the change.
 * Even if the tab being closed is a background tab, and the user was editing
the location-entry for the foreground tab (which is the case for this bug), this
resets the location-entry.
 * While resetting the entry, it gets the text from the NavigationController.
The state of the NavigationController hasn't yet been updated at this point, so
it ends up giving the incorrect text for the specified index. And that's the
bug.

> I believe this is a some
> what recent regression, but the code you're changing hasn't changed in a while
> (my patch just moved some of the code around).

I am not sure how old the bug is (and which parts actually changed recently),
but it's reproducible in 7.0.517.41

[sky, 2010-11-16 17:44:26]

So, I think the bug is we're always save the state and should only
save the state if the tab is the selected tab.

  -Scott

On Tue, Nov 16, 2010 at 9:30 AM,  <sadrul@chromium.org> wrote:
> On 2010/11/16 16:44:40, sky wrote:
>>
>> On 2010/11/16 05:17:53, sadrul wrote:
>
>> Could you elaborate on what problems this is causing?
>
> When a tab gets closed, it triggers the following sequence:
>  * TAB_CONTENTS_DESTROYED notification triggers
> TabStripModel::DetachTabContentsAt
>  * This triggers Browser::TabDetachedAt, which calls TabDetachedAtImpl with
> DETACH_TAB_DETACH
>  * This calls SaveStateToContents, which in turn calls SaveStateToTab. This
> calls AutocompleteEditModel::GetStateForTabSwitch (I checked and this
> happens
> from SaveStateToTab for all of gtk, win, mac).
>  * If the user edits the location entry, (i.e. |user_input_in_progress_| is
> set)
> and empties it, then GetStateForTabSwitch reverts the change.
>  * Even if the tab being closed is a background tab, and the user was
> editing
> the location-entry for the foreground tab (which is the case for this bug),
> this
> resets the location-entry.
>  * While resetting the entry, it gets the text from the
> NavigationController.
> The state of the NavigationController hasn't yet been updated at this point,
> so
> it ends up giving the incorrect text for the specified index. And that's the
> bug.
>
>> I believe this is a some
>> what recent regression, but the code you're changing hasn't changed in a
>> while
>> (my patch just moved some of the code around).
>
> I am not sure how old the bug is (and which parts actually changed
> recently),
> but it's reproducible in 7.0.517.41
>
>
> http://codereview.chromium.org/5075001/
>

[Peter Kasting, 2010-11-16 19:29:36]

On 2010/11/16 17:44:26, sky wrote:
> So, I think the bug is we're always save the state and should only
> save the state if the tab is the selected tab.

I agree with this if we're explicit about making "save state" mean "save the
state of the visible omnibox", not "save the state of the tab being switched
away from".  Because if "save state" meant the latter, it would imply that it
ought to be harmless to call all the time.

I am curious what changed that caused the bug to be visible here.  It wasn't
problematic for all Chrome's history, I don't think.

[sadrul, 2010-11-17 01:15:22]

On 2010/11/16 17:44:26, sky wrote:
> So, I think the bug is we're always save the state and should only
> save the state if the tab is the selected tab.

I have made the change so the state is saved only for the selected tab, and only
if it's not closing. What do you think?

[Peter Kasting, 2010-11-17 01:17:54]

On 2010/11/17 01:15:22, sadrul wrote:
> On 2010/11/16 17:44:26, sky wrote:
> > So, I think the bug is we're always save the state and should only
> > save the state if the tab is the selected tab.
> 
> I have made the change so the state is saved only for the selected tab, and
only
> if it's not closing.

Why should we care if the tab is closing?  It doesn't seem like it hurts
anything to save in that case.

Also, you need to comment in the code on why it matters that the tab is the
selected tab.

[sadrul, 2010-11-17 01:32:44]

On 2010/11/17 01:17:54, Peter Kasting wrote:
> On 2010/11/17 01:15:22, sadrul wrote:
> > On 2010/11/16 17:44:26, sky wrote:
> > > So, I think the bug is we're always save the state and should only
> > > save the state if the tab is the selected tab.
> > 
> > I have made the change so the state is saved only for the selected tab, and
> only
> > if it's not closing.
> 
> Why should we care if the tab is closing?  It doesn't seem like it hurts
> anything to save in that case.

It doesn't hurt. I do not think it is useful either, though. But I am not sure.
So I have made the change so it doesn't take into consideration whether the tab
is closing or not.

> Also, you need to comment in the code on why it matters that the tab is the
> selected tab.

Done. PTAL!

[Peter Kasting, 2010-11-17 01:41:24]

LGTM

http://codereview.chromium.org/5075001/diff/11001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/5075001/diff/11001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:4013: // Save what the user has typed in the
location bar. But do this only if the
Nit: Might be clearer as:

// Save the current location bar state, but only if the tab being detached is
the selected tab.  Because saving state can conditionally revert the location
bar, saving the current tab's location bar state to a non-selected tab can
corrupt both tabs.

[sadrul, 2010-11-17 02:09:24]

http://codereview.chromium.org/5075001/diff/11001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/5075001/diff/11001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:4013: // Save what the user has typed in the
location bar. But do this only if the
On 2010/11/17 01:41:24, Peter Kasting wrote:
> Nit: Might be clearer as:
> 
> // Save the current location bar state, but only if the tab being detached is
> the selected tab.  Because saving state can conditionally revert the location
> bar, saving the current tab's location bar state to a non-selected tab can
> corrupt both tabs.

Done.

[sky, 2010-11-17 16:02:24]

LGTM