Modified the browser controller so that it can revert...

Modified the browser controller so that it can revert the location bar when a user pressed RELOAD, FORWARD, or BACK buttons.

Note that this change does not revert the bar when Command+r is pressed. That's intentional. Please see the comment #7 by pkasting in http://crbug.com/15464 .

Windows change: http://codereview.chromium.org/155908
Linux change: http://codereview.chromium.org/164137

BUG=18710
TEST=The same as http://codereview.chromium.org/155908 (except the F5 stuff since Chromium Mac seems not to support F5 reload?)


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

[Yusuke Sato, 2009-08-08 03:59:58]

Mike, could you review this change?

[John Grabowski, 2009-08-08 04:38:20]

http://codereview.chromium.org/164145/diff/1001/4
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/164145/diff/1001/4#newcode470
Line 470: if ([sender isKindOfClass:[NSButton class]]) {
This behavior doesn't match the Windows version at
http://codereview.chromium.org/155908.  Can you explain why?  Specifically, what
is the problem with a revert from a Cmd-R (which presumably is the same as a
Windows revert from a Ctrl-R)?
Also, is there a Linux CL to reference?

[Yusuke Sato, 2009-08-08 04:46:46]

Thanks for the review!

http://codereview.chromium.org/164145/diff/1001/4
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/164145/diff/1001/4#newcode470
Line 470: if ([sender isKindOfClass:[NSButton class]]) {
On 2009/08/08 04:38:20, John Grabowski wrote:
> This behavior doesn't match the Windows version at
> http://codereview.chromium.org/155908.  Can you explain why?  

I believe the behavior _does_ match. On Windows, ctrl+r is not handled by the
ToolbarView::ButtonPressed() function (so the bar is not reverted when ctrl+r is
pressed).

> Also, is there a Linux CL to reference?

Linux change is this one: http://codereview.chromium.org/164137

[John Grabowski, 2009-08-08 04:50:16]

LGTM if comment added.
Thanks for going all-out here and covering all 3 platforms.

http://codereview.chromium.org/164145/diff/1001/4
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/164145/diff/1001/4#newcode470
Line 470: if ([sender isKindOfClass:[NSButton class]]) {
On 2009/08/08 04:46:46, Yusuke Sato wrote:
> On 2009/08/08 04:38:20, John Grabowski wrote:
> > This behavior doesn't match the Windows version at
> > http://codereview.chromium.org/155908.  Can you explain why?  
> 
> I believe the behavior _does_ match. On Windows, ctrl+r is not handled by the
> ToolbarView::ButtonPressed() function (so the bar is not reverted when ctrl+r
is
> pressed).

Excellent.  Please add a comment to that effect in this code so if a comparison
is done this question will be answered.

> > Also, is there a Linux CL to reference?
> 
> Linux change is this one: http://codereview.chromium.org/164137

Excellent.  Perhaps reference in main CL comments to make easier to find.

[Yusuke Sato, 2009-08-08 05:13:55]

http://codereview.chromium.org/164145/diff/1001/4
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/164145/diff/1001/4#newcode470
Line 470: if ([sender isKindOfClass:[NSButton class]]) {
> Excellent.  Please add a comment to that effect in this code so if a
comparison
> is done this question will be answered.

Done.

> Excellent.  Perhaps reference in main CL comments to make easier to find.

Modified the CL description.

Thanks!