Redraw the location bar on reload

Redraw the location bar on reload as well.

o Added following functions:
  - LocationBar::Revert() (pure virtual)
  - LocationBarView::Revert() which calls AutocompleteEditView::RevertAll() so that the bar's permanent is shown.

o Modified Browser::Reload() so that the function calls the Revert() function above.

BUG=2985

[Yusuke (unused), 2009-04-30 06:53:55]

Could you review this change? (please ignore the patch set 1 and 2.)

Thanks,
Yusuke

[Peter Kasting, 2009-04-30 13:08:00]

There are two problems with this patch.

First, turning off INVALIDATE_URL for auto-navigations breaks location bar
refreshes or these navigations, which we _want_ to occur when the user isn't
editing.

Second, I don't think the NavigationController's "is_auto" bit is necessarily
what you want.  I don't know for sure, but I'd imagine that if I type in a URL
and hit enter, then when the load commits it's not marked as an "automatic"
load; yet we don't want to force-override the autocomplete edit here because
this is precisely what the "don't change text if the user is editing" code was
written for: hit enter, then rapidly re-select the address bar and start editing
again.

Instead, I think perhaps you want to plumb this "revert location bar" call
directly from the "open file" and "reload page" command handlers.

[Yusuke (unused), 2009-04-30 22:57:07]

Thanks for your review.

I've reverted navigation_controller.cc and modified Reload() handler in
browser.cc as you suggested. Note that since the "open file" case seems to be
handled properly in AutocompleteEditViewWin::Update(), I left the "open file"
handler as is.

Please take another look.

Thanks,
Yusuke

On 2009/04/30 13:08:00, pkasting wrote:
> There are two problems with this patch.
> 
> First, turning off INVALIDATE_URL for auto-navigations breaks location bar
> refreshes or these navigations, which we _want_ to occur when the user isn't
> editing.
> 
> Second, I don't think the NavigationController's "is_auto" bit is necessarily
> what you want.  I don't know for sure, but I'd imagine that if I type in a URL
> and hit enter, then when the load commits it's not marked as an "automatic"
> load; yet we don't want to force-override the autocomplete edit here because
> this is precisely what the "don't change text if the user is editing" code was
> written for: hit enter, then rapidly re-select the address bar and start
editing
> again.
> 
> Instead, I think perhaps you want to plumb this "revert location bar" call
> directly from the "open file" and "reload page" command handlers.

[Peter Kasting, 2009-04-30 23:34:43]

http://codereview.chromium.org/100198/diff/29/1042
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/100198/diff/29/1042#newcode693
Line 693: // http://code.google.com/p/chromium/issues/detail?id=2985
Don't mention bug numbers; instead just write comments about why we need to do
what we're doing.  ("Forcibly reset the location bar, since otherwise it won't
discard any ongoing user edits, since it doesn't realize this is a
user-initiated action.")

http://codereview.chromium.org/100198/diff/29/1042#newcode694
Line 694: window_->RevertLocationbar();
Instead of calling this on BrowserWindow, get the location bar directly and call
things on it (e.g. see just below in OpenCurrentURL()).  This will reduce the
number of plumbing pieces you need to add (it should remove the toolbar and
browser window pieces).

[Yusuke (unused), 2009-05-01 00:58:14]

Thanks. The number of modified files is significantly reduced. Now checking
Mac/Linux builds using try server...

Updated the change description as well.

http://codereview.chromium.org/100198/diff/29/1042
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/100198/diff/29/1042#newcode693
Line 693: // http://code.google.com/p/chromium/issues/detail?id=2985
On 2009/04/30 23:34:44, pkasting wrote:
> Don't mention bug numbers; instead just write comments about why we need to do
> what we're doing.  ("Forcibly reset the location bar, since otherwise it won't
> discard any ongoing user edits, since it doesn't realize this is a
> user-initiated action.")

Done.

http://codereview.chromium.org/100198/diff/29/1042#newcode694
Line 694: window_->RevertLocationbar();
On 2009/04/30 23:34:44, pkasting wrote:
> Instead of calling this on BrowserWindow, get the location bar directly and
call
> things on it (e.g. see just below in OpenCurrentURL()).  This will reduce the
> number of plumbing pieces you need to add (it should remove the toolbar and
> browser window pieces).

Done.

[Yusuke (unused), 2009-05-01 01:42:22]

Mac/Linux builds and tests succeeded.
Could you take another look?

--Yusuke


On 2009/05/01 00:58:14, Yusuke wrote:
> Thanks. The number of modified files is significantly reduced. Now checking
> Mac/Linux builds using try server...
> 
> Updated the change description as well.
> 
> http://codereview.chromium.org/100198/diff/29/1042
> File chrome/browser/browser.cc (right):
> 
> http://codereview.chromium.org/100198/diff/29/1042#newcode693
> Line 693: // http://code.google.com/p/chromium/issues/detail?id=2985
> On 2009/04/30 23:34:44, pkasting wrote:
> > Don't mention bug numbers; instead just write comments about why we need to
do
> > what we're doing.  ("Forcibly reset the location bar, since otherwise it
won't
> > discard any ongoing user edits, since it doesn't realize this is a
> > user-initiated action.")
> 
> Done.
> 
> http://codereview.chromium.org/100198/diff/29/1042#newcode694
> Line 694: window_->RevertLocationbar();
> On 2009/04/30 23:34:44, pkasting wrote:
> > Instead of calling this on BrowserWindow, get the location bar directly and
> call
> > things on it (e.g. see just below in OpenCurrentURL()).  This will reduce
the
> > number of plumbing pieces you need to add (it should remove the toolbar and
> > browser window pieces).
> 
> Done.

[Peter Kasting, 2009-05-01 01:50:54]

http://codereview.chromium.org/100198/diff/49/53
File chrome/browser/location_bar.h (right):

http://codereview.chromium.org/100198/diff/49/53#newcode57
Line 57: #if defined(OS_WIN)
Don't use #ifdefs to stub things.  Instead, put stub implementations in the Mac
and Linux files.

[Yusuke (unused), 2009-05-01 17:12:26]

http://codereview.chromium.org/100198/diff/49/53
File chrome/browser/location_bar.h (right):

http://codereview.chromium.org/100198/diff/49/53#newcode57
Line 57: #if defined(OS_WIN)
On 2009/05/01 01:50:54, pkasting wrote:
> Don't use #ifdefs to stub things.  Instead, put stub implementations in the
Mac
> and Linux files.

Done.

[Yusuke (unused), 2009-05-01 19:52:55]

Mac/Linux builds and tests passed. Could you take another look?

--Yusuke

On 2009/05/01 17:12:26, Yusuke wrote:
> http://codereview.chromium.org/100198/diff/49/53
> File chrome/browser/location_bar.h (right):
> 
> http://codereview.chromium.org/100198/diff/49/53#newcode57
> Line 57: #if defined(OS_WIN)
> On 2009/05/01 01:50:54, pkasting wrote:
> > Don't use #ifdefs to stub things.  Instead, put stub implementations in the
> Mac
> > and Linux files.
> 
> Done.

[Peter Kasting, 2009-05-01 19:55:32]

LGTM

[Peter Kasting, 2009-05-04 17:11:10]

Landed in r15211.