Issue 6913026: Compact Navigation prototype

Issue 6913026: Compact Navigation prototype (Closed)

Created:
9 years, 7 months ago by SteveT

Modified:
9 years, 7 months ago

Reviewers:
Peter Kasting, sky

CC:
chromium-reviews, pam+watch_chromium.org, Paweł Hajdan Jr., oshima

Base URL:
http://git.chromium.org/git/chromium.git@trunk

Visibility:
Public.

Description

The initial prototype code for the compact navigation (cnav) prototype, currently only active in windows. This is well hidden behind a flag and a context menu option. Loosely based off oshima's original prototype patch: http://codereview.chromium.org/165272 BUG=None TEST=Activate the cnav prototype in about:flags. Right click a tab and select "Hide Toolbar" to go into cnav mode. Ensure that the compact location bar provides the same basic functionality as the toolbar (except browser actions). Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=84971

Patch Set 1 #

Total comments: 218

Patch Set 2 : Addressed initial review comments. Merged with tip of tree. Fixed lint issues and mac/linux tests. #

Patch Set 3 : Fixed last few issues from sky@. #

Patch Set 4 : Added missing DropdownBarHostDelegate header file. #

Total comments: 23

Patch Set 5 : Fixed additional nits. Fixed cnav/options bar layout and comment. Merged with ToT. #

Patch Set 6 : Fixed a few lint issues. Fixed a linux compiler error due to missing timer header. #

Patch Set 7 : Clang build fix. After commit/revert. #

Created: 9 years, 7 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+2455 lines, -197 lines)			Patch
M	chrome/app/chrome_command_ids.h	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/app/generated_resources.grd	View	1 2 3 4 5 6	3 chunks	+15 lines, -0 lines	0 comments	Download
M	chrome/app/theme/theme_resources.grd	View	1 2 3 4 5 6	1 chunk	+6 lines, -0 lines	0 comments	Download
M	chrome/browser/about_flags.cc	View	1 2 3 4 5 6	1 chunk	+7 lines, -0 lines	0 comments	Download
M	chrome/browser/tabs/default_tab_handler.h	View	1 2 3 4 5 6	1 chunk	+2 lines, -0 lines	0 comments	Download
M	chrome/browser/tabs/default_tab_handler.cc	View	1 2 3 4 5 6	2 chunks	+8 lines, -0 lines	0 comments	Download
M	chrome/browser/tabs/tab_strip_model.h	View	1 2 3 4 5 6	2 chunks	+5 lines, -0 lines	0 comments	Download
M	chrome/browser/tabs/tab_strip_model.cc	View	1 2 3 4 5 6	5 chunks	+18 lines, -0 lines	0 comments	Download
M	chrome/browser/tabs/tab_strip_model_delegate.h	View	1 2 3 4 5 6	1 chunk	+6 lines, -0 lines	0 comments	Download
M	chrome/browser/tabs/tab_strip_model_observer.h	View	1 2 3 4 5 6	1 chunk	+4 lines, -0 lines	0 comments	Download
M	chrome/browser/tabs/tab_strip_model_observer.cc	View	1 2 3 4 5 6	2 chunks	+3 lines, -1 line	0 comments	Download
M	chrome/browser/tabs/tab_strip_model_unittest.cc	View	1 2 3 4 5 6	1 chunk	+2 lines, -0 lines	0 comments	Download
M	chrome/browser/ui/browser.h	View	1 2 3 4 5 6	5 chunks	+9 lines, -1 line	0 comments	Download
M	chrome/browser/ui/browser.cc	View	1 2 3 4 5 6	10 chunks	+36 lines, -0 lines	0 comments	Download
M	chrome/browser/ui/browser_window.h	View	1 2 3 4 5 6	1 chunk	+3 lines, -0 lines	0 comments	Download
M	chrome/browser/ui/cocoa/browser_window_cocoa.h	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/browser/ui/cocoa/tabs/tab_strip_controller_unittest.mm	View	1 2 3 4 5 6	1 chunk	+4 lines, -0 lines	0 comments	Download
M	chrome/browser/ui/cocoa/view_id_util_browsertest.mm	View	1 2 3 4 5 6	2 chunks	+5 lines, -2 lines	0 comments	Download
M	chrome/browser/ui/gtk/browser_window_gtk.h	View	1 2 3 4 5 6	1 chunk	+2 lines, -1 line	0 comments	Download
M	chrome/browser/ui/gtk/browser_window_gtk.cc	View	1 2 3 4 5 6	1 chunk	+6 lines, -0 lines	0 comments	Download
M	chrome/browser/ui/gtk/view_id_util_browsertest.cc	View	1 2 3 4 5 6	1 chunk	+4 lines, -1 line	0 comments	Download
M	chrome/browser/ui/panels/panel.h	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/browser/ui/panels/panel.cc	View	1 2 3 4 5 6	1 chunk	+4 lines, -0 lines	0 comments	Download
M	chrome/browser/ui/panels/panel_browser_window_cocoa.h	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/browser/ui/panels/panel_browser_window_cocoa.mm	View	1 2 3 4 5 6	1 chunk	+4 lines, -0 lines	0 comments	Download
M	chrome/browser/ui/tabs/tab_menu_model.h	View	1 2 3 4 5 6	1 chunk	+3 lines, -0 lines	0 comments	Download
M	chrome/browser/ui/tabs/tab_menu_model.cc	View	1 2 3 4 5 6	3 chunks	+28 lines, -6 lines	0 comments	Download
M	chrome/browser/ui/view_ids.h	View	1 2 3 4 5 6	1 chunk	+9 lines, -0 lines	0 comments	Download
A	chrome/browser/ui/views/compact_nav/compact_location_bar_view.h	View	1 2 3 4	1 chunk	+104 lines, -0 lines	0 comments	Download
A	chrome/browser/ui/views/compact_nav/compact_location_bar_view.cc	View	1 2 3 4	1 chunk	+332 lines, -0 lines	0 comments	Download
A	chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.h	View	1 2 3 4 5	1 chunk	+138 lines, -0 lines	0 comments	Download
A	chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.cc	View	1 2 3 4 5 6	1 chunk	+518 lines, -0 lines	0 comments	Download
A	chrome/browser/ui/views/compact_nav/compact_navigation_bar.h	View	1 2 3 4	1 chunk	+64 lines, -0 lines	0 comments	Download
A	chrome/browser/ui/views/compact_nav/compact_navigation_bar.cc	View	1 2 3 4 5	1 chunk	+186 lines, -0 lines	0 comments	Download
A	chrome/browser/ui/views/compact_nav/compact_options_bar.h	View	1	1 chunk	+81 lines, -0 lines	0 comments	Download
A	chrome/browser/ui/views/compact_nav/compact_options_bar.cc	View	1	1 chunk	+151 lines, -0 lines	0 comments	Download
M	chrome/browser/ui/views/dropdown_bar_host.h	View	1 2 3 4 5 6	4 chunks	+7 lines, -6 lines	0 comments	Download
M	chrome/browser/ui/views/dropdown_bar_host.cc	View	1 2 3 4 5 6	5 chunks	+25 lines, -8 lines	0 comments	Download
A	chrome/browser/ui/views/dropdown_bar_host_delegate.h	View	1 2 3 4	1 chunk	+21 lines, -0 lines	0 comments	Download
M	chrome/browser/ui/views/dropdown_bar_view.h	View	1 2 3 4 5 6	5 chunks	+33 lines, -13 lines	0 comments	Download
A	chrome/browser/ui/views/dropdown_bar_view.cc	View	1	1 chunk	+107 lines, -0 lines	0 comments	Download
M	chrome/browser/ui/views/find_bar_host.cc	View	1 2 3 4 5 6	1 chunk	+2 lines, -1 line	0 comments	Download
M	chrome/browser/ui/views/find_bar_view.cc	View	1 2 3 4 5 6	5 chunks	+4 lines, -43 lines	0 comments	Download
M	chrome/browser/ui/views/frame/browser_view.h	View	1 2 3 4 5 6	10 chunks	+42 lines, -2 lines	0 comments	Download
M	chrome/browser/ui/views/frame/browser_view.cc	View	1 2 3 4 5 6	20 chunks	+120 lines, -13 lines	0 comments	Download
M	chrome/browser/ui/views/frame/browser_view_layout.h	View	1 2 3 4 5 6	2 chunks	+8 lines, -3 lines	0 comments	Download
M	chrome/browser/ui/views/frame/browser_view_layout.cc	View	1 2 3 4 5 6	11 chunks	+122 lines, -15 lines	0 comments	Download
M	chrome/browser/ui/views/frame/opaque_browser_frame_view.cc	View	1 2 3 4 5 6	4 chunks	+34 lines, -3 lines	0 comments	Download
M	chrome/browser/ui/views/location_bar/location_bar_view.h	View	1 2 3 4 5 6	4 chunks	+17 lines, -1 line	0 comments	Download
M	chrome/browser/ui/views/location_bar/location_bar_view.cc	View	1 2 3 4 5 6	2 chunks	+11 lines, -1 line	0 comments	Download
M	chrome/browser/ui/views/tabs/base_tab.cc	View	1 2 3 4 5 6	1 chunk	+2 lines, -0 lines	0 comments	Download
M	chrome/browser/ui/views/tabs/base_tab_strip.h	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/browser/ui/views/tabs/base_tab_strip.cc	View	1 2 3 4 5 6	1 chunk	+7 lines, -0 lines	0 comments	Download
M	chrome/browser/ui/views/tabs/browser_tab_strip_controller.h	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc	View	1 2 3 4 5 6	1 chunk	+5 lines, -0 lines	0 comments	Download
M	chrome/browser/ui/views/tabs/tab_controller.h	View	1 2 3 4 5 6	1 chunk	+4 lines, -0 lines	0 comments	Download
M	chrome/browser/ui/views/tabs/tab_strip_controller.h	View	1 2 3 4 5 6	1 chunk	+4 lines, -0 lines	0 comments	Download
M	chrome/browser/ui/views/toolbar_view.h	View	1 2 3 4 5 6	2 chunks	+4 lines, -4 lines	0 comments	Download
M	chrome/browser/ui/views/toolbar_view.cc	View	1 2 3 4 5 6	2 chunks	+55 lines, -55 lines	0 comments	Download
M	chrome/chrome_browser.gypi	View	1 2 3 4 5 6	4 chunks	+19 lines, -0 lines	0 comments	Download
M	chrome/common/chrome_switches.h	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/common/chrome_switches.cc	View	1 2 3 4 5 6	1 chunk	+4 lines, -0 lines	0 comments	Download
M	chrome/common/pref_names.h	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/common/pref_names.cc	View	1 2 3 4 5 6	1 chunk	+3 lines, -0 lines	0 comments	Download
M	chrome/test/test_browser_window.h	View	1 2 3 4 5 6	1 chunk	+1 line, -3 lines	0 comments	Download
M	views/events/event.h	View	1 2 3 4 5 6	1 chunk	+5 lines, -0 lines	0 comments	Download
M	views/events/event_win.cc	View	1 2 3 4 5 6	2 chunks	+14 lines, -14 lines	0 comments	Download

Messages

Total messages: 9 (0 generated)

Expand Messages | Collapse Messages

SteveT

Over to you, Scott. Finally! - We had a ton of issues wrangling with presubmit, ...

9 years, 7 months ago (2011-05-03 15:20:50 UTC) #1

Peter Kasting

A few drive-by nits http://codereview.chromium.org/6913026/diff/1/chrome/browser/autocomplete/autocomplete_edit_view.h File chrome/browser/autocomplete/autocomplete_edit_view.h (right): http://codereview.chromium.org/6913026/diff/1/chrome/browser/autocomplete/autocomplete_edit_view.h#newcode36 chrome/browser/autocomplete/autocomplete_edit_view.h:36: Nit: Don't add this. This ...

9 years, 7 months ago (2011-05-03 18:04:24 UTC) #2

sky

Mostly just style nits. -Scott http://codereview.chromium.org/6913026/diff/1/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): http://codereview.chromium.org/6913026/diff/1/chrome/browser/about_flags.cc#newcode251 chrome/browser/about_flags.cc:251: kOsWin, // TODO(stevet): Add ...

9 years, 7 months ago (2011-05-03 18:38:32 UTC) #3

SteveT

Hi Peter/Scott, Thanks for looking over this long CL, guys. I've addressed all your issues, ...

9 years, 7 months ago (2011-05-06 18:48:43 UTC) #4

Hi Peter/Scott,

Thanks for looking over this long CL, guys. I've addressed all your issues,
Peter, and almost all your issues, Sky. There were a few questions we had left
over.

Thanks!


Steve (+MAD)

http://codereview.chromium.org/6913026/diff/1/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

http://codereview.chromium.org/6913026/diff/1/chrome/browser/about_flags.cc#n...
chrome/browser/about_flags.cc:251: kOsWin,  // TODO(stevet): Add other platforms
when ready.
On 2011/05/03 18:38:32, sky wrote:
> Should this include chromeos?

Not yet. We've only implemented and tested the prototype for Win. We'll add
other platforms once we get the OK on the feature and polish up the current one.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/autocomplete/aut...
File chrome/browser/autocomplete/autocomplete_edit_view.h (right):

http://codereview.chromium.org/6913026/diff/1/chrome/browser/autocomplete/aut...
chrome/browser/autocomplete/autocomplete_edit_view.h:36: 
On 2011/05/03 18:38:32, sky wrote:
> No newline here.

Done - reverting as per Peter's suggestion.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/autocomplete/aut...
chrome/browser/autocomplete/autocomplete_edit_view.h:36: 
On 2011/05/03 18:04:24, Peter Kasting wrote:
> Nit: Don't add this.  This file should just be reverted.

Done.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/tabs/tab_strip_m...
File chrome/browser/tabs/tab_strip_model_observer.h (right):

http://codereview.chromium.org/6913026/diff/1/chrome/browser/tabs/tab_strip_m...
chrome/browser/tabs/tab_strip_model_observer.h:131: virtual void
TabReselected(int index);
On 2011/05/03 18:38:32, sky wrote:
> The implementation that triggers calling this only triggers the call if the
tab
> is already active and clicked. Also, reselected makes it sound like the
> selection is changing, which isn't the case. How about ActiveTabClicked.

"ActiveTabClicked" was actually one of the names I considered when first writing
this. :) And I guess 'Reselected' might be a bit confusing now that the tab
selection feature has landed. Will rename as per your suggestion.

I've renamed the corresponding methods from "ReselectTab" to "ClickActiveTab"
(ex - in base_tab.cc). Not sure if I'm a big fan of the wording, so let me know
if you have a suggestion.

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

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/browser.cc#ne...
chrome/browser/ui/browser.cc:1204:
command_updater_.UpdateCommandEnabled(IDC_COMPACT_NAVBAR, true);
On 2011/05/03 18:38:32, sky wrote:
> If IDC_COMPACT_NAVBAR is always enabled, then it should be in
InitCommandState.
> But do you really want it enabled for apps and popups?

Sorry - used to be true for everyone but changed to be more specific in a merge
we didn't see. Should follow others.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/gtk/browser_w...
File chrome/browser/ui/gtk/browser_window_gtk.h (right):

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/gtk/browser_w...
chrome/browser/ui/gtk/browser_window_gtk.h:129: virtual void
ToggleUseCompactNavigationBar() {}
On 2011/05/03 18:38:32, sky wrote:
> Don't inline virtual methods.

Fixed, and for ToggleTabStripMode, too.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/view_ids.h
File chrome/browser/ui/view_ids.h (right):

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/view_ids.h#ne...
chrome/browser/ui/view_ids.h:88: VIEW_ID_SPACER,
On 2011/05/03 18:38:32, sky wrote:
> Name is too generic, use VIEW_ID_COMPACT_NAV_BAR_SPACER, or something.

Done here and everywhere.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/bookmar...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right):

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/bookmar...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:737: // looks weird. If
we're in compact nav mode, also overlap by zero.
On 2011/05/03 18:04:24, Peter Kasting wrote:
> Nit: This additional comment just restates the code.  Say why it is correct to
> overlap by zero (or perhaps why it is wrong to do otherwise).

Turns out this check isn't necessary anymore. It was originally to avoid
overlapping the bookmark bar too bar over the spacer, but the spacer was
eventually made large enough that it looks fine.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/bookmar...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:738: if ((IsDetached() &&
infobar_visible_) || browser()->UseCompactNavigationBar())
On 2011/05/03 18:04:24, Peter Kasting wrote:
> Nit: 80 columns

Done.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/bookmar...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:738: if ((IsDetached() &&
infobar_visible_) || browser()->UseCompactNavigationBar())
On 2011/05/03 18:38:32, sky wrote:
> > 80. Also, NULL check browser_.

Done.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
File chrome/browser/ui/views/compact_nav/compact_location_bar_view.cc (right):

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view.cc:9: #if
defined(OS_LINUX)
On 2011/05/03 18:38:32, sky wrote:
> TOOLKIT_USES_GTK

Done.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view.cc:43: const int
kDefaultLocationEntryWidth = 375;
On 2011/05/03 18:38:32, sky wrote:
> newline between 42 & 43 and 57 and 58

Done.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view.cc:55: const
SkBitmap* kDialogLeft = NULL;
On 2011/05/03 18:38:32, sky wrote:
> Is there really any reason to define these here? They are only used once. Look
> them up each time.

We define them here as a way to make them static, to avoid re-allocation the
image for each time Init is called (for example, once per Chrome Window
created).

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view.cc:73: void
CompactLocationBarView::SetFocusAndSelection(bool select_all) {
On 2011/05/03 18:38:32, sky wrote:
> Make sure order matches that of header.

Done.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view.cc:90: void
CompactLocationBarView::GetAccessibleState(ui::AccessibleViewState* state) {
On 2011/05/03 18:38:32, sky wrote:
> > 80

Done.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view.cc:235: View*
child) {
On 2011/05/03 18:38:32, sky wrote:
> each param on its own line.

Done.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view.cc:236: if (is_add
&& child == this)
On 2011/05/03 18:38:32, sky wrote:
> Make sure you don't end up invoking Init twice.

Done.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
File chrome/browser/ui/views/compact_nav/compact_location_bar_view.h (right):

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view.h:10: #include
"chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.h"
On 2011/05/03 18:38:32, sky wrote:
> Is compact_location_bar_view_host needed in the header?

The clb_host() accessor below complains unless the header is included (it
doesn't know that host() can be static_casted to it's type). I can move the
definition to the cc file if you feel that that's more appropriate.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view.h:12: #include
"chrome/browser/ui/views/location_bar/location_bar_view.h"
On 2011/05/03 18:38:32, sky wrote:
> Is this needed?

Done.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view.h:13: #include
"chrome/browser/ui/views/reload_button.h"
On 2011/05/03 18:38:32, sky wrote:
> forward declare ReloadButton

Done.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view.h:17: #if
defined(OS_WIN)
On 2011/05/03 18:38:32, sky wrote:
> Do you really need these includes here?

No - I guess we originally needed it here but eventually removed it. Removed.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view.h:19: #elif
defined(OS_LINUX)
On 2011/05/03 18:38:32, sky wrote:
> TOOLKIT_USES_GTK

Done.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view.h:34: // a tab for
short period of used when Chrome is in the compact
On 2011/05/03 18:38:32, sky wrote:
> 'short period of used' -> 'short period of time'

Done.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view.h:48: virtual void
SetFocusAndSelection(bool select_all);
On 2011/05/03 18:38:32, sky wrote:
> OVERRIDE, and document this is from DropdownBarView

This is actually not from DropdownBarView and just declared public here.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view.h:50: void
Update(const TabContents* contents);
On 2011/05/03 18:38:32, sky wrote:
> Description?

Done.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
File chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.cc
(right):

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.cc:9: #if
defined(OS_LINUX)
On 2011/05/03 18:38:32, sky wrote:
> TOOLKIT_USES_GTK

Done.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.cc:36: const
int kHideTimeoutInSeconds = 2;
On 2011/05/03 18:38:32, sky wrote:
> either use static for these variables, or put in an anonymous namespace

Done.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.cc:45: #if
!defined(OS_MACOSX)  // MessageLoopForUI::Observer isn't available on Mac.
On 2011/05/03 18:38:32, sky wrote:
> views doesn't run on osx, so I don't see any point in this #ifdef.

Removed.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.cc:48: class
MouseObserver : public MessageLoopForUI::Observer {
On 2011/05/03 18:38:32, sky wrote:
> Don't inline all the methods of this class.

Done - Moved definitions out and below.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.cc:56: 
On 2011/05/03 18:38:32, sky wrote:
> Destructor should invoke StopObservering.

Done.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.cc:59:
virtual void WillProcessMessage(const MSG& native_event) {}
On 2011/05/03 18:38:32, sky wrote:
> OVERRIDE on all these

Done.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.cc:73:
host_->Hide(true);
On 2011/05/03 18:38:32, sky wrote:
> What if the user alt-tabs or the active window changes in some other way?

In these cases, the drop down will remain in the same state (for example, shown
and with focus). Switching back puts you back in the same state, so the user can
continue doing whatever they were doing with the drop down. We believe that this
is fine.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.cc:93: bool
IsMouseEvent(views::NativeEvent native_event) {
On 2011/05/03 18:38:32, sky wrote:
> Can this take a const NativeEvent& ?

Sure can!!

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.cc:97:
native_event.message == WM_MOUSEHOVER ||
On 2011/05/03 18:38:32, sky wrote:
> indentation is off, and refactor so you aren't copying it all.

Fixed. I've fixed the helpers in event_win.cc to take a const NativeEvent& as
well, and made them accessible.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.cc:112: //
TODO(mad): Would be nice to have a NativeEvent -> NativeWindow mapping.
On 2011/05/03 18:38:32, sky wrote:
> newline between 111 and 112. And feel free to add the mapping.

Haven't figured this mapping out yet... we'll take another shot when we do the
Gtk implementation.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.cc:119:
return gdk_window_get_toplevel(((GdkEventAny*)native_event)->window) ==
On 2011/05/03 18:38:32, sky wrote:
> Use C++ casts

Done.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.cc:123: //
Tests if the event occurred on the content area, using
On 2011/05/03 18:38:32, sky wrote:
> newline bewtween 122 and 123.

Done.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.cc:138: if
(HitOnScreen(browser_view_->GetContentsView(),
On 2011/05/03 18:38:32, sky wrote:
> put all on one line (don't wrap).

Done.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.cc:166:
BrowserView* browser_view)
On 2011/05/03 18:38:32, sky wrote:
> indentation is off

Done.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.cc:171:
CompactLocationBarView* clbv = new CompactLocationBarView(this);
On 2011/05/03 18:38:32, sky wrote:
> document who owns clbv

Done.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.cc:177: if
(browser_view() &&  browser_view()->browser() &&
On 2011/05/03 18:38:32, sky wrote:
> '&&  browser_view' -> '&& browser_view'

Done.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.cc:181:
MessageLoopForUI::current()->RemoveObserver(mouse_observer_.get());
On 2011/05/03 18:38:32, sky wrote:
> Remove this and have MouseObservers destructor do it.

Done.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.cc:208: 
On 2011/05/03 18:38:32, sky wrote:
> remove empty line

Done.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.cc:232:
!browser_view()->browser()->tabstrip_model()) {
On 2011/05/03 18:38:32, sky wrote:
> indentation is off

Done.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.cc:269:
Update(index, true);
On 2011/05/03 18:38:32, sky wrote:
> This case (inserting a tab with foregroudn == true) also sends out
> TabSelectedAt. Do you really need to do this here and not in TabSelectedAt? If
> you do really need it here, add a comment as to why.

Thanks for the pointer - no, we can just call it in TabSelectedAt and we're
fine. Removed this and the header entry.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.cc:282:
Hide(false);
On 2011/05/03 18:38:32, sky wrote:
> Are you sure you want to hide when any tab is closed?

The problem with hiding only when it's the current tab (the ideal behaviour) is
noted in Oshima's original TODO, above, which I've reassigned to me. We'll
address this later once we have a better idea of how to handle this.

For now I will avoid hiding it without being able to shift it, since it looks
strange if the active tab shifts to the left.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.cc:289:
current_tab_model_index_ = index;
On 2011/05/03 18:38:32, sky wrote:
> This can be invoked with the same index. Do you need to handle that case?
Maybe
> always show then?

No, I don't think we need to handle that case with always show. One example of
where it is an issue is when you have two tabs open and you close the left tab.
This actually calls TabSelectedAt on the right tab, with
current_tab_model_index_ == index. This then shows the CLB, which isn't
necessarily what we want to do unless the page in the right tab was still
loading.

If you can think of a scenario where it's invoked with the same index and we
want to always show, let us know.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.cc:300:
Update(to_index, false);
On 2011/05/03 18:38:32, sky wrote:
> Don't you only care if from_index == current_tab_model_index_ ?

Hm, is that expression not always true? It seems to always be true since in
order to move the tab from from_index to to_index, you need to select the tab
first, forcing current_tab_model_index_ to be equal to from_index in the
TabMoved call. Correct me if I am wrong?

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.cc:309: bool
was_not_visible = !IsVisible();
On 2011/05/03 18:38:32, sky wrote:
> indentation is off.

Done.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.cc:318:
registrar_.Add(this, NotificationType::LOAD_STOP,
On 2011/05/03 18:38:32, sky wrote:
> I don't see you removing the observer any where. Don't you need to remove it
> when the selection changes or the tab is replaced?

We remove the observe below in CompactLocationBarViewHost::Observe, once we
receive the notification we are listening for.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.cc:327: 
On 2011/05/03 18:38:32, sky wrote:
> You should override TabReplacedAt. You may want a similar implementation to
that
> of TabChangedAt.

TabReplacedAt is called when Instant replaces the current tab contents. Not sure
if we want to change the state of the compact location bar at that point.

If we were to call the same code as TabChangedAt, we wouldn't do anything useful
since it just tries to start a timer (to close the location bar). The timer will
either not start because the user is still typing their query in the omnibox, or
it conflicts with the timer that starts if they choose to navigate to a page.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.cc:357:
BaseTabStrip* tabstrip =
On 2011/05/03 18:38:32, sky wrote:
> What does this code do if the user has enabled side tabs? I suspect we don't
> want to allow both at the same time.

We have some code in browser.cc that prevents side tabs and cnav mode from being
enabled at the same time. I've added a DCHECK to assert this here.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.cc:367:
navbar_bounds.Offset(tabstrip->x(), tabstrip->y());
On 2011/05/03 18:38:32, sky wrote:
> I'm not sure what coordinates system you want this in. Use convertpointtoXXX
to
> avoid dependencies upon the view hierarchy. This may also be why you have the
> ifdef on 373.

Yes, good suggestion. This removes the dependency on browser_view()->y(). We'll
fix this for GTK later if it turns out that their dropdown bars ARE drawn
relative to the browser view.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.cc:420: void
CompactLocationBarViewHost::Update(int model_index, bool animate) {
On 2011/05/03 18:38:32, sky wrote:
> Name these methods differently, especially since this one does nothing if
> model_index is the current tab.

Second method:
Update -> UpdateOnTabChange

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.cc:435: if
(enabled) {
On 2011/05/03 18:38:32, sky wrote:
> What if enabled is already true?

Currently we rely on the caller to behave correctly - which is does since
SetEnabled is called by a toggled switch (so we only get true/false
alternating). We do get a DCHECK in AddObserver if we are added to it twice.

If you insist, we can add an additional is_observed_ flag here as an additional
check before we call AddObserver.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
File chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.h
(right):

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.h:17:
#include "views/controls/textfield/textfield.h"
On 2011/05/03 18:38:32, sky wrote:
> Looks like some of your includes aren't needed (at least animation and
textfield
> for sure). Make sure you only include what you need.

Done.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.h:28: //
compact_location_bar_view_host.cc to draw its content and is
On 2011/05/03 18:38:32, sky wrote:
> Nuke the part about compact_location_bar_view.cc. That comment could exist in
> all our headers.

Done.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.h:39: public
TabStripModelObserver,
On 2011/05/03 18:38:32, sky wrote:
> indentation is off.

Done.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.h:43:
explicit CompactLocationBarViewHost(::BrowserView* browser_view);
On 2011/05/03 18:38:32, sky wrote:
> Is the :: really needed there?

Nope. Sorry, that was a missed artifact from the original prototype code we
started from.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.h:51: //
Returns the bounds to locale the compact location bar under the tab.
On 2011/05/03 18:38:32, sky wrote:
> locale -> locate
> Also, document what coordinate system the bounds is in.

Done.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.h:59: void
Update(int model_index, bool animate);
On 2011/05/03 18:38:32, sky wrote:
> Do these really need to be public? They seem like implementation details. Same
> comment for StartAutoHideTimer and CAncelAutoHideTimer.

Yeah - Update is called by BrowserView when it gets notifications that normally
Update the Toolbar.

The timer is owned and managed by this class, but others need to be able to
trigger and stop it. For example, when the CompactLocationBarView receives
focus, it needs to stop the timer to prevent it from hiding itself.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.h:111: bool
no_redraw);
On 2011/05/03 18:38:32, sky wrote:
> OVERRIDE and document where it comes from.

Not actually overridden from somewhere else, just a public method. Moved this
(and GetCompactLocationBar) up to the public, and generally rearranged the order
of the methods to better group the overridden methods by class (in .cc as well).

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
File chrome/browser/ui/views/compact_nav/compact_navigation_bar.cc (right):

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_navigation_bar.cc:51:
initialized_(false) {
On 2011/05/03 18:38:32, sky wrote:
> initialize back_, bf_separator_ and forward_ too.

Done.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_navigation_bar.cc:62:
browser->command_updater()->AddCommandObserver(IDC_FORWARD, this);
On 2011/05/03 18:38:32, sky wrote:
> I don't see you removing the observers.

Added Remove calls to the dtor. Moved the AddCommandObserver calls to the ctor
for symmetry.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_navigation_bar.cc:135: void
CompactNavigationBar::OnPaint(gfx::Canvas* canvas) {
On 2011/05/03 18:38:32, sky wrote:
> View::OnPaint invokes OnPaintBackground. Do you need this override?

Yeah you're right, this isn't needed. Removed.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
File chrome/browser/ui/views/compact_nav/compact_navigation_bar.h (right):

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_navigation_bar.h:14: class
AutocompleteEditViewGtk;
On 2011/05/03 18:38:32, sky wrote:
> not needed

Done.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_navigation_bar.h:21: class
NativeViewHost;
On 2011/05/03 18:38:32, sky wrote:
> not needed

Done.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_navigation_bar.h:38: virtual
gfx::Size GetPreferredSize();
On 2011/05/03 18:38:32, sky wrote:
> OVERRIDE

Done.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
File chrome/browser/ui/views/compact_nav/compact_options_bar.cc (right):

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_options_bar.cc:20: const int
kPreferredHeight = 25;
On 2011/05/03 18:38:32, sky wrote:
> no indentation in a namespace

Done.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_options_bar.cc:72:
app_menu_->SetBounds(curx, 0, button_size.width(), height());
On 2011/05/03 18:38:32, sky wrote:
> You don't really need curx here.

Oops, yeah. Removing this for now, but we'll bring it back when we add browser
actions next to the wrench button.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_options_bar.cc:77:
OnPaintBackground(canvas);
On 2011/05/03 18:38:32, sky wrote:
> Do you really need to override this?

No - removed.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_options_bar.cc:86: i !=
menu_listeners_.end(); ++i) {
On 2011/05/03 18:38:32, sky wrote:
> indentation is off.

Done.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_options_bar.cc:111: if
(destroyed_flag)
On 2011/05/03 18:38:32, sky wrote:
> Given you've got nothing after destroyed_flag_, do you really need it? Instead
> add a comment that may be destroyed.

That is a good point - do you want me to get ride of destroyed_flag_ in
ToolbarView as well? That was where we were originally mimicing the code from.
They also seem to have nothing after RunMenu returns.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
File chrome/browser/ui/views/compact_nav/compact_options_bar.h (right):

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_options_bar.h:12: #include
"chrome/browser/ui/views/wrench_menu.h"
On 2011/05/03 18:38:32, sky wrote:
> forward declare WrenchMenu

Done.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_options_bar.h:15: #include
"views/controls/button/menu_button.h"
On 2011/05/03 18:38:32, sky wrote:
> forward declare

Done.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_options_bar.h:16: #include
"views/controls/menu/menu_wrapper.h"
On 2011/05/03 18:38:32, sky wrote:
> do you need this?

No - just had to forward declare views::MenuListener.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_options_bar.h:37: virtual gfx::Size
GetPreferredSize();
On 2011/05/03 18:38:32, sky wrote:
> OVERRIDE (and other places)

Done.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_options_bar.h:44: // Add a listener
to receive a callback when the menu opens.
On 2011/05/03 18:38:32, sky wrote:
> Generally all overriden methods are last in a section.

Moved these and implementation up to publics.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_options_bar.h:52: int command_id,
ui::Accelerator* accelerator) OVERRIDE;
On 2011/05/03 18:38:32, sky wrote:
> each param on its own line when you wrap.

Done.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/dropdow...
File chrome/browser/ui/views/dropdown_bar_host.cc (right):

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/dropdow...
chrome/browser/ui/views/dropdown_bar_host.cc:107:
delegate_->SetFocusAndSelection(true);
On 2011/05/03 18:38:32, sky wrote:
> If delegate_ must be non-null, add a DCHECK to that effect when set.

Ah, yes. Done in Init() (and for view, too).

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/dropdow...
chrome/browser/ui/views/dropdown_bar_host.cc:118: if (!animation_->IsClosing())
{
On 2011/05/03 18:38:32, sky wrote:
> Doesn't slideanimation take care of this for you? Meaning can't you just
invoke
> Hide? 

Right, that's true. Removed the Reset call.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/dropdow...
chrome/browser/ui/views/dropdown_bar_host.cc:123: if (animation_->IsClosing()) {
On 2011/05/03 18:38:32, sky wrote:
> I don't understand this code. This is the !animate case, so don't you just
want
> to stop the animation?
> Seems like you're saying the bug is AnimationEnded and you're hacking around
it
> here. Fix the bug in AnimationEnded without having to hack this.

Sorry - I think my weak comment confused things a bit.
CompactLocationBarViewHost discovered a bug in this original code where, if
|animation_| is not closing, StopAnimation() does not set the state of
|animation_| to not showing, but sets it's own visibility and widget to a hidden
state. So later calls to try to re-show the drop down bar fail (again, because
the animation thinks it's still showing).

We noticed this and fixed it by explicitly calling animation_'s Reset and Hide
methods in the case where we're not closing. I've added a comment in both cases
to explain this.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/dropdow...
File chrome/browser/ui/views/dropdown_bar_host.h (right):

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/dropdow...
chrome/browser/ui/views/dropdown_bar_host.h:48: class Delegate {
On 2011/05/03 18:38:32, sky wrote:
> Declare this in a separate class so folks don't need to include
DropdownBarHost
> to implement the delegate.

From what we can see in many other places, the Delegate class is always declared
within the class it belongs to. Has this standard changed somewhere?

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/dropdow...
chrome/browser/ui/views/dropdown_bar_host.h:58: // Initializes the dropdown bar
host with the give view.
On 2011/05/03 18:38:32, sky wrote:
> 'give' -> 'given'

Done.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/dropdow...
chrome/browser/ui/views/dropdown_bar_host.h:59: void Init(views::View* view,
Delegate* delegate);
On 2011/05/03 18:38:32, sky wrote:
> Can delegate be passed to the constructor?

It gets a bit hairy here if I move the Delegate into the ctor. |view| then has
to be moved as well, since both uses of this code passes the same class into
|view| and |delegate|. Then we have to move FindBarHost's instantiation of
FindBarView (which is passed into |view| and |delegate|) out of it's
constructor, which is bad since we pass a pointer of FindBarHost into
FindBarView at instantiation time.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/dropdow...
File chrome/browser/ui/views/dropdown_bar_view.cc (right):

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/dropdow...
chrome/browser/ui/views/dropdown_bar_view.cc:18: // When we are animating, we
draw only the top part of the left and right
On 2011/05/03 18:38:32, sky wrote:
> newlines between 17/18 and 21/22.

Done.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/dropdow...
chrome/browser/ui/views/dropdown_bar_view.cc:22: }
On 2011/05/03 18:38:32, sky wrote:
> //  namespace

Done.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/dropdow...
File chrome/browser/ui/views/dropdown_bar_view.h (right):

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/dropdow...
chrome/browser/ui/views/dropdown_bar_view.h:11: 
On 2011/05/03 18:38:32, sky wrote:
> remove newline

Done.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/dropdow...
chrome/browser/ui/views/dropdown_bar_view.h:12: #include "ui/gfx/canvas.h"
On 2011/05/03 18:38:32, sky wrote:
> forward declare canvas

Done.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/dropdow...
chrome/browser/ui/views/dropdown_bar_view.h:31: virtual void
set_animation_offset(int offset) OVERRIDE;
On 2011/05/03 18:38:32, sky wrote:
> This isn't OVERRIDE, is it? Should be CamelCase for virtual.

Renamed everywhere.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/dropdow...
chrome/browser/ui/views/dropdown_bar_view.h:57: const SkBitmap* dialog_left_;
On 2011/05/03 18:38:32, sky wrote:
> no protected members.

Made private.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/frame/b...
File chrome/browser/ui/views/frame/browser_view.cc (right):

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/frame/b...
chrome/browser/ui/views/frame/browser_view.cc:167: SkBitmap* background_;
On 2011/05/03 18:04:24, Peter Kasting wrote:
> Nit: Put this in a section.

Spacer class was removed since we eventually ended up just using an empty view
to block out the space.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/frame/b...
chrome/browser/ui/views/frame/browser_view.cc:178: class ChromeosTab : public
Tab {
On 2011/05/03 18:04:24, Peter Kasting wrote:
> Nit: Wouldn't ChromeOSTab be better?  (2 classes)

Actually, this commented out code that sky/oshima had originally is no longer
necessary since we're handling this MouseEvent through the Tab[Strip] Controller
classes rather than deriving from Tab/TabStrip again. Removed.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/frame/b...
chrome/browser/ui/views/frame/browser_view.cc:187: virtual void
OnMouseEntered(const views::MouseEvent& event) {
On 2011/05/03 18:04:24, Peter Kasting wrote:
> Nit: Use OVERRIDE (here and elsewhere)
> 
> I also suggest just saying "views::View:" instead of "Overridden from xxx".

Deleted, as above.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/frame/b...
File chrome/browser/ui/views/frame/browser_view_layout.cc (left):

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/frame/b...
chrome/browser/ui/views/frame/browser_view_layout.cc:301: &tabstrip_origin);
On 2011/05/03 18:38:32, sky wrote:
> This spacing is preferable; leave it.

Done.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/frame/b...
File chrome/browser/ui/views/frame/browser_view_layout.cc (right):

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/frame/b...
chrome/browser/ui/views/frame/browser_view_layout.cc:41: static const int
kCompactNavbarSpacerVisibleHeight = 4;
On 2011/05/03 18:38:32, sky wrote:
> no need for static since in anonymouse namespace.

Done.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/frame/b...
chrome/browser/ui/views/frame/browser_view_layout.cc:341:
compact_navigation_bar_->SetVisible(false);
On 2011/05/03 18:38:32, sky wrote:
> Are we really always creating the compact nav classes? We should only create
it
> if needed.

I agree with you here, though I am having troubles accomplishing this. Maybe you
can shed some light on this. Our compact navigation classes are instantiated in
BrowserView::Init along side the rest of the views placed in the BrowserView.

It appears that at this point in the code, the browser prefs have not yet been
loaded. So even if I know my Chrome has the cnav flag set to true, it is
initially false when I try to read it in BrowserView::Init. So the classes can't
be instantiated. Later when we Toggle cnav mode on, the browser will crash
because it expects to be able to use these classes, but they're not there.

Any idea how we can get to these prefs early on in BrowserView::Init? Or is
there somewhere else where it's appropriate to instantiate views that are owned
by BrowserView?

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/frame/o...
File chrome/browser/ui/views/frame/opaque_browser_frame_view.cc (right):

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/frame/o...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:443: if
(browser_view_->UseCompactNavigationBar()) {
On 2011/05/03 18:38:32, sky wrote:
> Did you need to do anything in glass_browser_frame_view?

No - it turns out GlassBrowserFrameView's HitTest handles this tab region
differently and our clicks go through to the NavBar or OptBar without additional
hit tests.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/locatio...
File chrome/browser/ui/views/location_bar/location_bar_view.h (right):

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/locatio...
chrome/browser/ui/views/location_bar/location_bar_view.h:428: int
animation_offset_;
On 2011/05/03 18:38:32, sky wrote:
> Initialize this to 0.

Done.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/tabs/ba...
File chrome/browser/ui/views/tabs/base_tab.cc (right):

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/tabs/ba...
chrome/browser/ui/views/tabs/base_tab.cc:34: #include "views/widget/widget.h"
On 2011/05/03 18:38:32, sky wrote:
> Is this needed?

Removed.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/tabs/ba...
chrome/browser/ui/views/tabs/base_tab.cc:306: if (IsActive()) {
On 2011/05/03 18:38:32, sky wrote:
> combine with else on 305

Done.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/tabs/ta...
File chrome/browser/ui/views/tabs/tab_controller.h (right):

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/tabs/ta...
chrome/browser/ui/views/tabs/tab_controller.h:70: virtual void ReselectTab(const
BaseTab* tab) const = 0;
On 2011/05/03 18:38:32, sky wrote:
> newline between 70 and 71

Done.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/toolbar...
File chrome/browser/ui/views/toolbar_view.h (right):

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/toolbar...
chrome/browser/ui/views/toolbar_view.h:140: SkBitmap
GetAppMenuIcon(views::CustomButton::ButtonState state);
On 2011/05/03 18:38:32, sky wrote:
> Move implementation to match new position in header.

Done in .cc. Also moved this declaration up in the headers to join the publics.

sky

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact_nav/compact_location_bar_view.cc File chrome/browser/ui/views/compact_nav/compact_location_bar_view.cc (right): http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact_nav/compact_location_bar_view.cc#newcode55 chrome/browser/ui/views/compact_nav/compact_location_bar_view.cc:55: const SkBitmap* kDialogLeft = NULL; On 2011/05/06 18:48:43, SteveT ...

9 years, 7 months ago (2011-05-06 19:56:06 UTC) #5

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
File chrome/browser/ui/views/compact_nav/compact_location_bar_view.cc (right):

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view.cc:55: const
SkBitmap* kDialogLeft = NULL;
On 2011/05/06 18:48:43, SteveT wrote:
> On 2011/05/03 18:38:32, sky wrote:
> > Is there really any reason to define these here? They are only used once.
Look
> > them up each time.
> 
> We define them here as a way to make them static, to avoid re-allocation the
> image for each time Init is called (for example, once per Chrome Window
> created).

ResourceBundle caches all the image, so if you ask for the same image twice in a
row no extra allocation happens.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
File chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.cc
(right):

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.cc:300:
Update(to_index, false);
On 2011/05/06 18:48:43, SteveT wrote:
> On 2011/05/03 18:38:32, sky wrote:
> > Don't you only care if from_index == current_tab_model_index_ ?
> 
> Hm, is that expression not always true? It seems to always be true since in
> order to move the tab from from_index to to_index, you need to select the tab
> first, forcing current_tab_model_index_ to be equal to from_index in the
> TabMoved call. Correct me if I am wrong?

It's currently true, but I don't think you should assume its always the case.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.cc:435: if
(enabled) {
On 2011/05/06 18:48:43, SteveT wrote:
> On 2011/05/03 18:38:32, sky wrote:
> > What if enabled is already true?
> 
> Currently we rely on the caller to behave correctly - which is does since
> SetEnabled is called by a toggled switch (so we only get true/false
> alternating). We do get a DCHECK in AddObserver if we are added to it twice.
> 
> If you insist, we can add an additional is_observed_ flag here as an
additional
> check before we call AddObserver.

Yes please.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
File chrome/browser/ui/views/compact_nav/compact_options_bar.cc (right):

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_options_bar.cc:111: if
(destroyed_flag)
On 2011/05/06 18:48:43, SteveT wrote:
> On 2011/05/03 18:38:32, sky wrote:
> > Given you've got nothing after destroyed_flag_, do you really need it?
Instead
> > add a comment that may be destroyed.
> 
> That is a good point - do you want me to get ride of destroyed_flag_ in
> ToolbarView as well? That was where we were originally mimicing the code from.
> They also seem to have nothing after RunMenu returns.

Sure, but in a separate patch please.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/dropdow...
File chrome/browser/ui/views/dropdown_bar_host.h (right):

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/dropdow...
chrome/browser/ui/views/dropdown_bar_host.h:48: class Delegate {
On 2011/05/06 18:48:43, SteveT wrote:
> On 2011/05/03 18:38:32, sky wrote:
> > Declare this in a separate class so folks don't need to include
> DropdownBarHost
> > to implement the delegate.
> 
> From what we can see in many other places, the Delegate class is always
declared
> within the class it belongs to. Has this standard changed somewhere?

From https://sites.google.com/a/chromium.org/dev/developers/coding-style:

"Prefer putting delegate classes in their own header files. Implementors of the
delegate interface will often be included elsewhere, which will often cause more
coupling with the header of the main class."

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/frame/b...
File chrome/browser/ui/views/frame/browser_view_layout.cc (right):

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/frame/b...
chrome/browser/ui/views/frame/browser_view_layout.cc:341:
compact_navigation_bar_->SetVisible(false);
On 2011/05/06 18:48:43, SteveT wrote:
> On 2011/05/03 18:38:32, sky wrote:
> > Are we really always creating the compact nav classes? We should only create
> it
> > if needed.
> 
> I agree with you here, though I am having troubles accomplishing this. Maybe
you
> can shed some light on this. Our compact navigation classes are instantiated
in
> BrowserView::Init along side the rest of the views placed in the BrowserView.
> 
> It appears that at this point in the code, the browser prefs have not yet been
> loaded. So even if I know my Chrome has the cnav flag set to true, it is
> initially false when I try to read it in BrowserView::Init. So the classes
can't
> be instantiated. Later when we Toggle cnav mode on, the browser will crash
> because it expects to be able to use these classes, but they're not there.
> 
> Any idea how we can get to these prefs early on in BrowserView::Init? Or is
> there somewhere else where it's appropriate to instantiate views that are
owned
> by BrowserView?

Are you sure about that? The code for choosing side tab should looks nearly
identical, and as far as I know doesn't have problems. Where do you end up if in
Init you try to call UseCompactNavigationBar?

SteveT

Everything addressed. Back to you for another look. Thanks! Steve http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact_nav/compact_location_bar_view.cc File chrome/browser/ui/views/compact_nav/compact_location_bar_view.cc (right): http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact_nav/compact_location_bar_view.cc#newcode55 ...

9 years, 7 months ago (2011-05-06 22:13:40 UTC) #6

Everything addressed. Back to you for another look. Thanks!


Steve

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
File chrome/browser/ui/views/compact_nav/compact_location_bar_view.cc (right):

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view.cc:55: const
SkBitmap* kDialogLeft = NULL;
On 2011/05/06 19:56:06, sky wrote:
> On 2011/05/06 18:48:43, SteveT wrote:
> > On 2011/05/03 18:38:32, sky wrote:
> > > Is there really any reason to define these here? They are only used once.
> Look
> > > them up each time.
> > 
> > We define them here as a way to make them static, to avoid re-allocation the
> > image for each time Init is called (for example, once per Chrome Window
> > created).
> 
> ResourceBundle caches all the image, so if you ask for the same image twice in
a
> row no extra allocation happens.

Cool. Removed the variables. I can do the same for find_bar_view.cc in a
separate patch for you later (that's where we originally got the code).

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
File chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.cc
(right):

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.cc:300:
Update(to_index, false);
On 2011/05/06 19:56:06, sky wrote:
> On 2011/05/06 18:48:43, SteveT wrote:
> > On 2011/05/03 18:38:32, sky wrote:
> > > Don't you only care if from_index == current_tab_model_index_ ?
> > 
> > Hm, is that expression not always true? It seems to always be true since in
> > order to move the tab from from_index to to_index, you need to select the
tab
> > first, forcing current_tab_model_index_ to be equal to from_index in the
> > TabMoved call. Correct me if I am wrong?
> 
> It's currently true, but I don't think you should assume its always the case.

Done.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.cc:435: if
(enabled) {
On 2011/05/06 19:56:06, sky wrote:
> On 2011/05/06 18:48:43, SteveT wrote:
> > On 2011/05/03 18:38:32, sky wrote:
> > > What if enabled is already true?
> > 
> > Currently we rely on the caller to behave correctly - which is does since
> > SetEnabled is called by a toggled switch (so we only get true/false
> > alternating). We do get a DCHECK in AddObserver if we are added to it twice.
> > 
> > If you insist, we can add an additional is_observed_ flag here as an
> additional
> > check before we call AddObserver.
> 
> Yes please.

Done.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
File chrome/browser/ui/views/compact_nav/compact_options_bar.cc (right):

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_options_bar.cc:111: if
(destroyed_flag)
On 2011/05/06 19:56:06, sky wrote:
> On 2011/05/06 18:48:43, SteveT wrote:
> > On 2011/05/03 18:38:32, sky wrote:
> > > Given you've got nothing after destroyed_flag_, do you really need it?
> Instead
> > > add a comment that may be destroyed.
> > 
> > That is a good point - do you want me to get ride of destroyed_flag_ in
> > ToolbarView as well? That was where we were originally mimicing the code
from.
> > They also seem to have nothing after RunMenu returns.
> 
> Sure, but in a separate patch please.

Noted.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/dropdow...
File chrome/browser/ui/views/dropdown_bar_host.h (right):

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/dropdow...
chrome/browser/ui/views/dropdown_bar_host.h:48: class Delegate {
On 2011/05/06 19:56:06, sky wrote:
> On 2011/05/06 18:48:43, SteveT wrote:
> > On 2011/05/03 18:38:32, sky wrote:
> > > Declare this in a separate class so folks don't need to include
> > DropdownBarHost
> > > to implement the delegate.
> > 
> > From what we can see in many other places, the Delegate class is always
> declared
> > within the class it belongs to. Has this standard changed somewhere?
> 
> From https://sites.google.com/a/chromium.org/dev/developers/coding-style:
> 
> "Prefer putting delegate classes in their own header files. Implementors of
the
> delegate interface will often be included elsewhere, which will often cause
more
> coupling with the header of the main class."

Done.

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/frame/b...
File chrome/browser/ui/views/frame/browser_view_layout.cc (right):

http://codereview.chromium.org/6913026/diff/1/chrome/browser/ui/views/frame/b...
chrome/browser/ui/views/frame/browser_view_layout.cc:341:
compact_navigation_bar_->SetVisible(false);
On 2011/05/06 19:56:06, sky wrote:
> On 2011/05/06 18:48:43, SteveT wrote:
> > On 2011/05/03 18:38:32, sky wrote:
> > > Are we really always creating the compact nav classes? We should only
create
> > it
> > > if needed.
> > 
> > I agree with you here, though I am having troubles accomplishing this. Maybe
> you
> > can shed some light on this. Our compact navigation classes are instantiated
> in
> > BrowserView::Init along side the rest of the views placed in the
BrowserView.
> > 
> > It appears that at this point in the code, the browser prefs have not yet
been
> > loaded. So even if I know my Chrome has the cnav flag set to true, it is
> > initially false when I try to read it in BrowserView::Init. So the classes
> can't
> > be instantiated. Later when we Toggle cnav mode on, the browser will crash
> > because it expects to be able to use these classes, but they're not there.
> > 
> > Any idea how we can get to these prefs early on in BrowserView::Init? Or is
> > there somewhere else where it's appropriate to instantiate views that are
> owned
> > by BrowserView?
> 
> Are you sure about that? The code for choosing side tab should looks nearly
> identical, and as far as I know doesn't have problems. Where do you end up if
in
> Init you try to call UseCompactNavigationBar?

Ah, this was my bad - I was checking the pref (context menu switch) rather than
the actual switch (from about:flags). So we had states where the switch was on,
but the pref was off. I fixed this now.

So what we do is if the about:flags value is Enabled, we instantiate the class.
And then we only show them (checking carefully for nullness) if the tab context
menu item is checked.

I've also renamed the TabMenuModel accessor to the switch to
UseCompactNavigationMODE, for extra clarity.

Thanks for the vigilance on this one.

sky

http://codereview.chromium.org/6913026/diff/19030/chrome/browser/ui/views/compact_nav/compact_location_bar_view.cc File chrome/browser/ui/views/compact_nav/compact_location_bar_view.cc (right): http://codereview.chromium.org/6913026/diff/19030/chrome/browser/ui/views/compact_nav/compact_location_bar_view.cc#newcode83 chrome/browser/ui/views/compact_nav/compact_location_bar_view.cc:83: if (!AccessiblePaneView::SetPaneFocus(view_storage_id, initial_focus)) spacing is off http://codereview.chromium.org/6913026/diff/19030/chrome/browser/ui/views/compact_nav/compact_location_bar_view.cc#newcode91 chrome/browser/ui/views/compact_nav/compact_location_bar_view.cc:91: ui::AccessibleViewState* ...

9 years, 7 months ago (2011-05-09 14:27:21 UTC) #7

SteveT

Back to you. Thanks. http://codereview.chromium.org/6913026/diff/19030/chrome/browser/ui/views/compact_nav/compact_location_bar_view.cc File chrome/browser/ui/views/compact_nav/compact_location_bar_view.cc (right): http://codereview.chromium.org/6913026/diff/19030/chrome/browser/ui/views/compact_nav/compact_location_bar_view.cc#newcode83 chrome/browser/ui/views/compact_nav/compact_location_bar_view.cc:83: if (!AccessiblePaneView::SetPaneFocus(view_storage_id, initial_focus)) On 2011/05/09 ...

9 years, 7 months ago (2011-05-09 19:40:04 UTC) #8

sky

9 years, 7 months ago (2011-05-09 21:15:03 UTC) #9

LGTM

http://codereview.chromium.org/6913026/diff/19030/chrome/browser/ui/views/dro...
File chrome/browser/ui/views/dropdown_bar_host_delegate.h (right):

http://codereview.chromium.org/6913026/diff/19030/chrome/browser/ui/views/dro...
chrome/browser/ui/views/dropdown_bar_host_delegate.h:10: virtual
~DropdownBarHostDelegate() {}
On 2011/05/09 19:40:04, SteveT wrote:
> On 2011/05/09 14:27:22, sky wrote:
> > Move this to the protected section.
> 
> Done. For my understanding: My guess as to why you want this is to prevent the
> delegate owners from destroying the delegate. Is that accurate? Should I add a
> comment to that effect or do you consider that well understood? I don't see it
> being done in some similar cases.

It makes it clear that DropdownBarHost doesn't own the delegate. Well, I suppose
it could be away of a friend, but we typically don't do that.

Expand Messages | Collapse Messages