Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7646003:
Support auto-hide taskbar for panels on Windows. (Closed)
|
Created:
9 years, 4 months ago by jianli Modified:
9 years, 3 months ago CC:
chromium-reviews, prasadt, jianli, dcheng, Paweł Hajdan Jr., dhollowa Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionSupport auto-hide taskbar for panels on Windows.
BUG=none
TEST=browser test
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=98898
Patch Set 1 : '' #
Total comments: 4
Patch Set 2 : '' #
Total comments: 28
Patch Set 3 : '' #
Total comments: 10
Patch Set 4 : '' #
Total comments: 12
Patch Set 5 : '' #Patch Set 6 : '' #
Total comments: 50
Patch Set 7 : '' #
Total comments: 30
Patch Set 8 : '' #
Total comments: 7
Patch Set 9 : '' #Messages
Total messages: 27 (0 generated)
Initial feedback: General question: what about taskbar/Dock being on the left or on the right? It seems it'd be better to think about all these cases before implementing only bottom solution, since this can affect the shape of new classes... http://codereview.chromium.org/7646003/diff/5005/chrome/browser/ui/panels/bot... File chrome/browser/ui/panels/bottom_bar.h (right): http://codereview.chromium.org/7646003/diff/5005/chrome/browser/ui/panels/bot... chrome/browser/ui/panels/bottom_bar.h:14: int GetBottomBarHeight(); Is it instant height, max height or some other height? Not clear from the name. At least a comment should be there. http://codereview.chromium.org/7646003/diff/5005/chrome/browser/ui/panels/bot... chrome/browser/ui/panels/bottom_bar.h:15: bool IsBottomBarFullyVisible(); What is expected return value of this and the next method while the bar is animated?
On Mon, Aug 15, 2011 at 10:55 AM, <dimich@chromium.org> wrote: > Initial feedback: > > General question: what about taskbar/Dock being on the left or on the > right? It > seems it'd be better to think about all these cases before implementing > only > bottom solution, since this can affect the shape of new classes... > Per the discussion, we feel that this is not an issue if taskbar/Dock is set to auto-hide on the left or right of the screen. When the panel is minimized, the user can move the mouse to the bottom of the screen to bring it up. As long as the mouse is not moved to the right-most position, the auto-hide taskbar/dock will not pop up when we bring up the titlebar. > > > http://codereview.chromium.**org/7646003/diff/5005/chrome/** > browser/ui/panels/bottom_bar.h<http://codereview.chromium.org/7646003/diff/5005/chrome/browser/ui/panels/bottom_bar.h> > File chrome/browser/ui/panels/**bottom_bar.h (right): > > http://codereview.chromium.**org/7646003/diff/5005/chrome/** > browser/ui/panels/bottom_bar.**h#newcode14<http://codereview.chromium.org/7646003/diff/5005/chrome/browser/ui/panels/bottom_bar.h#newcode14> > chrome/browser/ui/panels/**bottom_bar.h:14: int GetBottomBarHeight(); > Is it instant height, max height or some other height? Not clear from > the name. At least a comment should be there. > > http://codereview.chromium.**org/7646003/diff/5005/chrome/** > browser/ui/panels/bottom_bar.**h#newcode15<http://codereview.chromium.org/7646003/diff/5005/chrome/browser/ui/panels/bottom_bar.h#newcode15> > chrome/browser/ui/panels/**bottom_bar.h:15: bool > IsBottomBarFullyVisible(); > What is expected return value of this and the next method while the bar > is animated? > > > http://codereview.chromium.**org/7646003/<http://codereview.chromium.org/7646... >
http://codereview.chromium.org/7646003/diff/5005/chrome/browser/ui/panels/bot... File chrome/browser/ui/panels/bottom_bar.h (right): http://codereview.chromium.org/7646003/diff/5005/chrome/browser/ui/panels/bot... chrome/browser/ui/panels/bottom_bar.h:14: int GetBottomBarHeight(); On 2011/08/15 17:55:18, Dmitry Titov wrote: > Is it instant height, max height or some other height? Not clear from the name. > At least a comment should be there. Added more comment. http://codereview.chromium.org/7646003/diff/5005/chrome/browser/ui/panels/bot... chrome/browser/ui/panels/bottom_bar.h:15: bool IsBottomBarFullyVisible(); On 2011/08/15 17:55:18, Dmitry Titov wrote: > What is expected return value of this and the next method while the bar is > animated? Added more comment.
The comments are below. Also, it seems linux_views trybot fails panel-related browser test. http://codereview.chromium.org/7646003/diff/39/chrome/browser/ui/panels/botto... File chrome/browser/ui/panels/bottom_bar.h (right): http://codereview.chromium.org/7646003/diff/39/chrome/browser/ui/panels/botto... chrome/browser/ui/panels/bottom_bar.h:16: bool IsBottomBarInAutoHideMode(); We should have a class here, with static members. It's better then to add functions to the global namespace. This also will let to avoid adding BottomBar to all the names, like: BottomBar::IsFoo() instead of IsBottomBarFoo(). http://codereview.chromium.org/7646003/diff/39/chrome/browser/ui/panels/botto... chrome/browser/ui/panels/bottom_bar.h:25: bool IsBottomBarFullyVisible(); It seems there can be a single method returning enum value, since not all combinations are valid - for example, if IsBottomBarInAutoHideMode == false, then IsBottomBarHidden can only return false. Something like { ALWAYS_VISIBLE, AUTOHIDE_HIDDEN, AUTOHIDE_SHOWN } http://codereview.chromium.org/7646003/diff/39/chrome/browser/ui/panels/botto... File chrome/browser/ui/panels/bottom_bar_cocoa.mm (right): http://codereview.chromium.org/7646003/diff/39/chrome/browser/ui/panels/botto... chrome/browser/ui/panels/bottom_bar_cocoa.mm:7: bool IsBottomBarInAutoHideMode() { Lets add NOTIMPLEMENTED to those? http://codereview.chromium.org/7646003/diff/39/chrome/browser/ui/panels/botto... File chrome/browser/ui/panels/bottom_bar_linux.cc (right): http://codereview.chromium.org/7646003/diff/39/chrome/browser/ui/panels/botto... chrome/browser/ui/panels/bottom_bar_linux.cc:7: bool IsBottomBarInAutoHideMode() { NOTIMPLEMENTED http://codereview.chromium.org/7646003/diff/39/chrome/browser/ui/panels/botto... File chrome/browser/ui/panels/bottom_bar_win.cc (right): http://codereview.chromium.org/7646003/diff/39/chrome/browser/ui/panels/botto... chrome/browser/ui/panels/bottom_bar_win.cc:17: // The thickness of an auto-hide taskbar in pixels. What is exactly a thickness here? Needs better explanation. http://codereview.chromium.org/7646003/diff/39/chrome/browser/ui/panels/panel... File chrome/browser/ui/panels/panel_browser_view_browsertest.cc (right): http://codereview.chromium.org/7646003/diff/39/chrome/browser/ui/panels/panel... chrome/browser/ui/panels/panel_browser_view_browsertest.cc:510: TestMinimizeAndRestore(0); It's incorrect to call a helper function and then use ASSERT_TRUE in it. ASSERT_TRUE actually returns a value on which the gtest depends. JennB knows how to do the right helpers for tests. http://codereview.chromium.org/7646003/diff/39/chrome/browser/ui/panels/panel... File chrome/browser/ui/panels/panel_manager.cc (right): http://codereview.chromium.org/7646003/diff/39/chrome/browser/ui/panels/panel... chrome/browser/ui/panels/panel_manager.cc:309: bring_up_or_down_titlebar_timer_.Stop(); It seems that this code actually wants to receive an event or a notification about bottom bar changing state. The timer is an implementation detail. Perhaps we can make BottomBar class to source these events/notificaitons. Perhaps they can be sourced differently on different platforms. http://codereview.chromium.org/7646003/diff/39/chrome/browser/ui/panels/panel... File chrome/browser/ui/panels/panel_manager.h (right): http://codereview.chromium.org/7646003/diff/39/chrome/browser/ui/panels/panel... chrome/browser/ui/panels/panel_manager.h:52: // bottom of the screen, not the bottom of the work area. GetBottomPositionForExpansionState would probably be better and more consistent. http://codereview.chromium.org/7646003/diff/39/chrome/browser/ui/panels/panel... chrome/browser/ui/panels/panel_manager.h:68: void SetWorkArea(const gfx::Rect& work_area, int bottom_bar_height); Comment needs to reflect if this method should be called when auto-hide behavior changes or the size of the bottom_bar changes. Size of the Dock on OSX can change for example, following user settings. http://codereview.chromium.org/7646003/diff/39/chrome/browser/ui/panels/panel... chrome/browser/ui/panels/panel_manager.h:104: // If the top-most bottom bar, like Windows taskbar or MacOSX dock, is always s/top-most/always-on-top/ http://codereview.chromium.org/7646003/diff/39/chrome/browser/ui/panels/panel... chrome/browser/ui/panels/panel_manager.h:110: // The height of the top-most bottom bar if it is auto-hidden. Otherwise, this always-on-top http://codereview.chromium.org/7646003/diff/39/chrome/browser/ui/panels/panel... chrome/browser/ui/panels/panel_manager.h:111: // value is 0. This is a dual-use of a variable. It makes reading the code more difficult. It is easier to read the code if there is a boolean and a height values. Also, the 'height' variable should have something in the name indicating which height it is - when hidden or when fully visible ("unhiddenHeight"?) http://codereview.chromium.org/7646003/diff/39/chrome/browser/ui/panels/panel... chrome/browser/ui/panels/panel_manager.h:132: // bring up or down the titlebar. s/the titlebar/the panel's titlebars/? http://codereview.chromium.org/7646003/diff/39/chrome/browser/ui/panels/panel... chrome/browser/ui/panels/panel_manager.h:136: bool should_check_auto_hide_bottom_bar_on_titlebar_change_; instead of accessing this member directly, we can have a #if UNIT_TEST set accessor. This is used in multiple tests so we can be consistent.
Also refactored browser tests for Panel and PanelBrowserView to put common code into base class BasePanelBrowserTest. http://codereview.chromium.org/7646003/diff/39/chrome/browser/ui/panels/botto... File chrome/browser/ui/panels/bottom_bar.h (right): http://codereview.chromium.org/7646003/diff/39/chrome/browser/ui/panels/botto... chrome/browser/ui/panels/bottom_bar.h:16: bool IsBottomBarInAutoHideMode(); On 2011/08/17 22:56:38, Dmitry Titov wrote: > We should have a class here, with static members. It's better then to add > functions to the global namespace. This also will let to avoid adding BottomBar > to all the names, like: BottomBar::IsFoo() instead of IsBottomBarFoo(). Done. http://codereview.chromium.org/7646003/diff/39/chrome/browser/ui/panels/botto... chrome/browser/ui/panels/bottom_bar.h:25: bool IsBottomBarFullyVisible(); On 2011/08/17 22:56:38, Dmitry Titov wrote: > It seems there can be a single method returning enum value, since not all > combinations are valid - for example, if IsBottomBarInAutoHideMode == false, > then IsBottomBarHidden can only return false. > > Something like { ALWAYS_VISIBLE, AUTOHIDE_HIDDEN, AUTOHIDE_SHOWN } Done. http://codereview.chromium.org/7646003/diff/39/chrome/browser/ui/panels/botto... File chrome/browser/ui/panels/bottom_bar_cocoa.mm (right): http://codereview.chromium.org/7646003/diff/39/chrome/browser/ui/panels/botto... chrome/browser/ui/panels/bottom_bar_cocoa.mm:7: bool IsBottomBarInAutoHideMode() { On 2011/08/17 22:56:38, Dmitry Titov wrote: > Lets add NOTIMPLEMENTED to those? Done. http://codereview.chromium.org/7646003/diff/39/chrome/browser/ui/panels/botto... File chrome/browser/ui/panels/bottom_bar_linux.cc (right): http://codereview.chromium.org/7646003/diff/39/chrome/browser/ui/panels/botto... chrome/browser/ui/panels/bottom_bar_linux.cc:7: bool IsBottomBarInAutoHideMode() { On 2011/08/17 22:56:38, Dmitry Titov wrote: > NOTIMPLEMENTED Done. http://codereview.chromium.org/7646003/diff/39/chrome/browser/ui/panels/botto... File chrome/browser/ui/panels/bottom_bar_win.cc (right): http://codereview.chromium.org/7646003/diff/39/chrome/browser/ui/panels/botto... chrome/browser/ui/panels/bottom_bar_win.cc:17: // The thickness of an auto-hide taskbar in pixels. On 2011/08/17 22:56:38, Dmitry Titov wrote: > What is exactly a thickness here? Needs better explanation. Done. http://codereview.chromium.org/7646003/diff/39/chrome/browser/ui/panels/panel... File chrome/browser/ui/panels/panel_browser_view_browsertest.cc (right): http://codereview.chromium.org/7646003/diff/39/chrome/browser/ui/panels/panel... chrome/browser/ui/panels/panel_browser_view_browsertest.cc:510: TestMinimizeAndRestore(0); On 2011/08/17 22:56:38, Dmitry Titov wrote: > It's incorrect to call a helper function and then use ASSERT_TRUE in it. > ASSERT_TRUE actually returns a value on which the gtest depends. JennB knows how > to do the right helpers for tests. The helper function does not use ASSERT_* so it is OK for the function to return no value. http://codereview.chromium.org/7646003/diff/39/chrome/browser/ui/panels/panel... File chrome/browser/ui/panels/panel_manager.cc (right): http://codereview.chromium.org/7646003/diff/39/chrome/browser/ui/panels/panel... chrome/browser/ui/panels/panel_manager.cc:309: bring_up_or_down_titlebar_timer_.Stop(); On 2011/08/17 22:56:38, Dmitry Titov wrote: > It seems that this code actually wants to receive an event or a notification > about bottom bar changing state. The timer is an implementation detail. Perhaps > we can make BottomBar class to source these events/notificaitons. Perhaps they > can be sourced differently on different platforms. Done. http://codereview.chromium.org/7646003/diff/39/chrome/browser/ui/panels/panel... File chrome/browser/ui/panels/panel_manager.h (right): http://codereview.chromium.org/7646003/diff/39/chrome/browser/ui/panels/panel... chrome/browser/ui/panels/panel_manager.h:52: // bottom of the screen, not the bottom of the work area. On 2011/08/17 22:56:38, Dmitry Titov wrote: > GetBottomPositionForExpansionState would probably be better and more consistent. Done. http://codereview.chromium.org/7646003/diff/39/chrome/browser/ui/panels/panel... chrome/browser/ui/panels/panel_manager.h:68: void SetWorkArea(const gfx::Rect& work_area, int bottom_bar_height); On 2011/08/17 22:56:38, Dmitry Titov wrote: > Comment needs to reflect if this method should be called when auto-hide behavior > changes or the size of the bottom_bar changes. Size of the Dock on OSX can > change for example, following user settings. bottom_bar_height argument is removed. http://codereview.chromium.org/7646003/diff/39/chrome/browser/ui/panels/panel... chrome/browser/ui/panels/panel_manager.h:104: // If the top-most bottom bar, like Windows taskbar or MacOSX dock, is always On 2011/08/17 22:56:38, Dmitry Titov wrote: > s/top-most/always-on-top/ Done. http://codereview.chromium.org/7646003/diff/39/chrome/browser/ui/panels/panel... chrome/browser/ui/panels/panel_manager.h:110: // The height of the top-most bottom bar if it is auto-hidden. Otherwise, this On 2011/08/17 22:56:38, Dmitry Titov wrote: > always-on-top Done. http://codereview.chromium.org/7646003/diff/39/chrome/browser/ui/panels/panel... chrome/browser/ui/panels/panel_manager.h:111: // value is 0. On 2011/08/17 22:56:38, Dmitry Titov wrote: > This is a dual-use of a variable. It makes reading the code more difficult. It > is easier to read the code if there is a boolean and a height values. > Also, the 'height' variable should have something in the name indicating which > height it is - when hidden or when fully visible ("unhiddenHeight"?) Removed since it is not needed as I introduced AutoHideBottomBar class instance. http://codereview.chromium.org/7646003/diff/39/chrome/browser/ui/panels/panel... chrome/browser/ui/panels/panel_manager.h:132: // bring up or down the titlebar. On 2011/08/17 22:56:38, Dmitry Titov wrote: > s/the titlebar/the panel's titlebars/? Removed since this variable is not needed.. http://codereview.chromium.org/7646003/diff/39/chrome/browser/ui/panels/panel... chrome/browser/ui/panels/panel_manager.h:136: bool should_check_auto_hide_bottom_bar_on_titlebar_change_; On 2011/08/17 22:56:38, Dmitry Titov wrote: > instead of accessing this member directly, we can have a #if UNIT_TEST set > accessor. This is used in multiple tests so we can be consistent. Done.
A few drive-by comments... http://codereview.chromium.org/7646003/diff/10001/chrome/browser/ui/panels/ba... File chrome/browser/ui/panels/base_panel_browser_test.h (right): http://codereview.chromium.org/7646003/diff/10001/chrome/browser/ui/panels/ba... chrome/browser/ui/panels/base_panel_browser_test.h:60: mock_auto_hide_bottom_bar_ = new MockAutoHideBottomBar(); Hmm...now even tests that don't care about auto hide task bar will end up creating one. Could you refactor so that only tests that use task bar will create one? http://codereview.chromium.org/7646003/diff/10001/chrome/browser/ui/panels/ba... chrome/browser/ui/panels/base_panel_browser_test.h:71: Panel* CreatePanel(const std::string& panel_name, Usually, panels will be active when created in tests. Rather than have every test pass show_flag explicitly, what about having a CreatePanel that calls this one with show_flag set to active? http://codereview.chromium.org/7646003/diff/10001/chrome/browser/ui/panels/pa... File chrome/browser/ui/panels/panel_manager.cc (right): http://codereview.chromium.org/7646003/diff/10001/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_manager.cc:39: scoped_refptr<PanelManager> panel_instance; Rename this to panel_manager_instance? Took me a while to figure out why CreateForTesting was only updating a single panel until I realized panel_instance meant the panel_manager instance. http://codereview.chromium.org/7646003/diff/10001/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_manager.cc:52: void PanelManager::CreateForTesting(const gfx::Rect& work_area, Alternative would be to have a method to set the AutoHideBottomBar, just for testing. That allows overriding the default auto hide bottom bar in tests. Also avoids having to add params to this method as we increase the number of things we need to override in this class when testing. http://codereview.chromium.org/7646003/diff/10001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): http://codereview.chromium.org/7646003/diff/10001/chrome/chrome_browser.gypi#... chrome/chrome_browser.gypi:3000: 'browser/ui/panels/base_panel_browser_test.h', Should this go in chrome_tests.gypi?
Awesome, thanks for major iteration. In addition to Jenn's comments, here are some new ones: http://codereview.chromium.org/7646003/diff/12003/chrome/browser/ui/panels/au... File chrome/browser/ui/panels/auto_hide_bottom_bar.h (right): http://codereview.chromium.org/7646003/diff/12003/chrome/browser/ui/panels/au... chrome/browser/ui/panels/auto_hide_bottom_bar.h:18: class AutoHideBottomBar : public base::RefCountedThreadSafe<AutoHideBottomBar> { why this class is refcounted? It seems the singleton PanelManager keeps the only pointer to it and can completely control its lifetime via scoped_ptr? http://codereview.chromium.org/7646003/diff/12003/chrome/browser/ui/panels/au... chrome/browser/ui/panels/auto_hide_bottom_bar.h:20: enum Visibility { What do you think about this: Do we also need AUTOHIDE_DISABLED here? Then we can fire OnAutoHideBottomBarVisibilityChanged with this as well - for example, when user moves the bar to the side or clears 'autohide' checkbox in the settings. The panels might need to rearrange then - for example, VISIBLE panel may slide to the bottom if taskbar was moved to the left. Otherwise, how would panels know when to move? http://codereview.chromium.org/7646003/diff/12003/chrome/browser/ui/panels/au... chrome/browser/ui/panels/auto_hide_bottom_bar.h:46: virtual bool Exists() = 0; See comment above about AUTOHIDE_DISABLED. If we had that, we would not need this method. http://codereview.chromium.org/7646003/diff/12003/chrome/browser/ui/panels/au... chrome/browser/ui/panels/auto_hide_bottom_bar.h:48: // Returns the fixed height of the bottom bar. Note that the height remains Suggestion on comment (not sure it's clearer though): Returns the height of the bottom bar that corresponds to Visibility VISIBLE. This value does not depend on current bar's Visibility. The height of the bar when it's HIDDEN is usually between 0 and few pixels. http://codereview.chromium.org/7646003/diff/12003/chrome/browser/ui/panels/pa... File chrome/browser/ui/panels/panel_manager.cc (right): http://codereview.chromium.org/7646003/diff/12003/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_manager.cc:35: // The delayed interval to do the check to ensure we bring up or down titlebars. It seems some explanation has to be here. I think the need for the delay is because the mouse hover is usually a reason to change visibility of the titlebar. The auto-hiding bottom bar aslo is activated by mouse move. To visually separate the action of panels from the action of autohiding bar, you add this delay (if I understand it correctly). If I don't understand it correctly, please add a good comment :-) Also, the comment can help with a better name for the constant. http://codereview.chromium.org/7646003/diff/12003/chrome/browser/ui/panels/pa... File chrome/browser/ui/panels/panel_manager.h (right): http://codereview.chromium.org/7646003/diff/12003/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_manager.h:23: public base::RefCountedThreadSafe<PanelManager> { Why the PanelManager is refcounted? it is a singleton that lives forever...
http://codereview.chromium.org/7646003/diff/10001/chrome/browser/ui/panels/ba... File chrome/browser/ui/panels/base_panel_browser_test.h (right): http://codereview.chromium.org/7646003/diff/10001/chrome/browser/ui/panels/ba... chrome/browser/ui/panels/base_panel_browser_test.h:60: mock_auto_hide_bottom_bar_ = new MockAutoHideBottomBar(); On 2011/08/22 21:00:16, jennb wrote: > Hmm...now even tests that don't care about auto hide task bar will end up > creating one. Could you refactor so that only tests that use task bar will > create one? As discussed, this is needed to setup a consistent testing environment to reduce the flakiness of tests. Added comment for it. http://codereview.chromium.org/7646003/diff/10001/chrome/browser/ui/panels/ba... chrome/browser/ui/panels/base_panel_browser_test.h:71: Panel* CreatePanel(const std::string& panel_name, On 2011/08/22 21:00:16, jennb wrote: > Usually, panels will be active when created in tests. Rather than have every > test pass show_flag explicitly, what about having a CreatePanel that calls this > one with show_flag set to active? Done. http://codereview.chromium.org/7646003/diff/10001/chrome/browser/ui/panels/pa... File chrome/browser/ui/panels/panel_manager.cc (right): http://codereview.chromium.org/7646003/diff/10001/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_manager.cc:39: scoped_refptr<PanelManager> panel_instance; On 2011/08/22 21:00:16, jennb wrote: > Rename this to panel_manager_instance? Took me a while to figure out why > CreateForTesting was only updating a single panel until I realized > panel_instance meant the panel_manager instance. Done. http://codereview.chromium.org/7646003/diff/10001/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_manager.cc:52: void PanelManager::CreateForTesting(const gfx::Rect& work_area, On 2011/08/22 21:00:16, jennb wrote: > Alternative would be to have a method to set the AutoHideBottomBar, just for > testing. That allows overriding the default auto hide bottom bar in tests. Also > avoids having to add params to this method as we increase the number of things > we need to override in this class when testing. I've tried this but it caused crash if we call PanelManager::GetInstance from the test setup routine. When PanelManager is created via PanelManager::GetInstance, we set up a default bottom bar handler that might need to access the MessageLoop which is not yet initialized. Added comment for it. http://codereview.chromium.org/7646003/diff/10001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): http://codereview.chromium.org/7646003/diff/10001/chrome/chrome_browser.gypi#... chrome/chrome_browser.gypi:3000: 'browser/ui/panels/base_panel_browser_test.h', On 2011/08/22 21:00:16, jennb wrote: > Should this go in chrome_tests.gypi? Done. http://codereview.chromium.org/7646003/diff/12003/chrome/browser/ui/panels/au... File chrome/browser/ui/panels/auto_hide_bottom_bar.h (right): http://codereview.chromium.org/7646003/diff/12003/chrome/browser/ui/panels/au... chrome/browser/ui/panels/auto_hide_bottom_bar.h:18: class AutoHideBottomBar : public base::RefCountedThreadSafe<AutoHideBottomBar> { On 2011/08/22 22:35:53, Dmitry Titov wrote: > why this class is refcounted? It seems the singleton PanelManager keeps the only > pointer to it and can completely control its lifetime via scoped_ptr? The test also keep a ref count to the mock because ref count is needed for PostTask. Added comment for it. http://codereview.chromium.org/7646003/diff/12003/chrome/browser/ui/panels/au... chrome/browser/ui/panels/auto_hide_bottom_bar.h:20: enum Visibility { On 2011/08/22 22:35:53, Dmitry Titov wrote: > What do you think about this: Do we also need AUTOHIDE_DISABLED here? > Then we can fire OnAutoHideBottomBarVisibilityChanged with this as well - for > example, when user moves the bar to the side or clears 'autohide' checkbox in > the settings. The panels might need to rearrange then - for example, VISIBLE > panel may slide to the bottom if taskbar was moved to the left. Otherwise, how > would panels know when to move? I think this state is not needed because this class in only intended to deal with auto-hide bottom bar. If the bottom bar is not in auto-hide mode, we do not need to know about its visibility and height. When the auto-hide mode is changed, the work area is also changed. Thus PanelManager::OnDisplayChanged is called, which also calls AutoHideBottomBar::UpdateWorkArea. http://codereview.chromium.org/7646003/diff/12003/chrome/browser/ui/panels/au... chrome/browser/ui/panels/auto_hide_bottom_bar.h:46: virtual bool Exists() = 0; On 2011/08/22 22:35:53, Dmitry Titov wrote: > See comment above about AUTOHIDE_DISABLED. If we had that, we would not need > this method. See my reply above. http://codereview.chromium.org/7646003/diff/12003/chrome/browser/ui/panels/au... chrome/browser/ui/panels/auto_hide_bottom_bar.h:48: // Returns the fixed height of the bottom bar. Note that the height remains On 2011/08/22 22:35:53, Dmitry Titov wrote: > Suggestion on comment (not sure it's clearer though): > Returns the height of the bottom bar that corresponds to Visibility VISIBLE. > This value does not depend on current bar's Visibility. The height of the bar > when it's HIDDEN is usually between 0 and few pixels. Done. http://codereview.chromium.org/7646003/diff/12003/chrome/browser/ui/panels/pa... File chrome/browser/ui/panels/panel_manager.cc (right): http://codereview.chromium.org/7646003/diff/12003/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_manager.cc:35: // The delayed interval to do the check to ensure we bring up or down titlebars. On 2011/08/22 22:35:53, Dmitry Titov wrote: > It seems some explanation has to be here. I think the need for the delay is > because the mouse hover is usually a reason to change visibility of the > titlebar. The auto-hiding bottom bar aslo is activated by mouse move. To > visually separate the action of panels from the action of autohiding bar, you > add this delay (if I understand it correctly). If I don't understand it > correctly, please add a good comment :-) > Also, the comment can help with a better name for the constant. The reason for adding this additional logic is that occasionally the system, i.e. Windows, might not bring up or down the taskbar when the mouse enters or leaves the bottom area. So if we do not receive the taskbar visibility change notification within this time period, we do not want to wait any more. Added more detail here.. http://codereview.chromium.org/7646003/diff/12003/chrome/browser/ui/panels/pa... File chrome/browser/ui/panels/panel_manager.h (right): http://codereview.chromium.org/7646003/diff/12003/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_manager.h:23: public base::RefCountedThreadSafe<PanelManager> { On 2011/08/22 22:35:53, Dmitry Titov wrote: > Why the PanelManager is refcounted? it is a singleton that lives forever... Needed by PostTask. Added comment for it.
Thanks for explanations! LGTM from me, please get the Jenn's one as well.
http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/au... File chrome/browser/ui/panels/auto_hide_bottom_bar.h (right): http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/au... chrome/browser/ui/panels/auto_hide_bottom_bar.h:16: // located at the bottom of the screen and set to auto-hide, like Windows Just thinking ahead: this one handles bottom of screen only. Would we need a separate class for task/app bars on the left/right of screen, or would we extend this class to handle left/right cases as well? http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/au... chrome/browser/ui/panels/auto_hide_bottom_bar.h:46: virtual bool IsEnabled() = 0; After reading more, I realize this value does double duty as both a boolean for is-auto-hide-enabled and a boolean for is-system-bar-aligned-to-bottom. I wish there was some way to make this clearer than "IsEnabled", but I can live with it if not. http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/au... chrome/browser/ui/panels/auto_hide_bottom_bar.h:50: Extra blank line? http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/au... File chrome/browser/ui/panels/auto_hide_bottom_bar_linux.cc (right): http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/au... chrome/browser/ui/panels/auto_hide_bottom_bar_linux.cc:12: class AutoHideBottomBarLinux : public AutoHideBottomBar { Use GTK suffix. http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/au... File chrome/browser/ui/panels/auto_hide_bottom_bar_win.cc (right): http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/au... chrome/browser/ui/panels/auto_hide_bottom_bar_win.cc:145: if (!window_ || !aligned_to_bottom_) if (!IsEnabled()) http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/ba... File chrome/browser/ui/panels/base_panel_browser_test.h (right): http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/ba... chrome/browser/ui/panels/base_panel_browser_test.h:13: #include "chrome/common/chrome_switches.h" alpha ordering wrong http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/ba... chrome/browser/ui/panels/base_panel_browser_test.h:57: virtual void SetUp() OVERRIDE { Comments in InProcessBrowserTest header say to override SetUpInProcessBrowserTextFixture instead of SetUp. Once you do that, the message loop is already created, so you could get by without CreateForTesting. Instead, do GetInstance on PanelManager, then call SetWorkArea and SetAutoHideBottomBar to customize for testing. http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/ba... chrome/browser/ui/panels/base_panel_browser_test.h:90: bounds.set_size(gfx::Size(kDefaultPanelWidth, kDefaultPanelHeight)); Is this necessary? PanelManager will pick defaults for the panel if bounds are not provided, right? http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/ba... chrome/browser/ui/panels/base_panel_browser_test.h:154: class MockAutoHideBottomBar : public AutoHideBottomBar { Would code be cleaner if this class was NOT nested inside BasePanelBrowserTest? If you want to keep it private, you can move it to the .cc file. http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/pa... File chrome/browser/ui/panels/panel_browser_view_browsertest.cc (right): http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_browser_view_browsertest.cc:343: void TestChangeAutoHideTaskBarHeight() { Does this test need updating as we found logic errors in get bottom code? http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_browser_view_browsertest.cc:505: // Test that the taskbar is always visible. Don't need this comment. Test name is pretty clear. Ditto for auto hide test. http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/pa... File chrome/browser/ui/panels/panel_manager.cc (right): http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_manager.cc:39: const int kMaxTimeWaitForBottomBarVisibilityChange = 1000; s/Time/<name of time unit> http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_manager.cc:341: void PanelManager::DelayedBringUpOrDownTitlebarsCheck() { Seems like this could be coded more compactly: if (delayed_titlebar_action_ == NO_ACTION) return; DoBringUpOrDownTitlebars(delayed_titlebar_action_ == BRING_UP); delayed_titlebar_action_ = NO_ACTION_; return; http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_manager.cc:378: int PanelManager::GetBottomPositionForExpansionState( As discussed, some logic error computing bottom here... http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_manager.cc:388: switch (delayed_titlebar_action_) { Feels like this method can be coded more compactly as well. http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_manager.cc:410: current_x_ = work_area_.right(); Why does current_x_ get updated when height changed? http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/pa... File chrome/browser/ui/panels/panel_manager.h (right): http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_manager.h:46: // Should we bring up the titlebar, given the current mouse point? Nit: Change comment to "Returns true if we should bring up the ... " http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_manager.h:63: // bar handler, for the testing purpose. We cannot instantiate the default nit: s/for the testing purpose/for testing purposes http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_manager.h:64: // PanelManager instance by calling PanelManager::GetInstance() in the test Must the PanelManager instance be instantiated in SetUp? Why not do this at the beginning of a test, after message loop has been initialized? http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_manager.h:74: enum DelayedTitlebarAction { Drop 'Delay' prefix. How you use it later brings about the delay part so there's no need to have delay in the name here. http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_manager.h:121: void DelayedBringUpOrDownTitlebarsCheck(); Comment? What is it checking? http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_manager.h:125: static void CreateForTestingHelper(const gfx::Rect& work_area, Why is a helper needed? http://codereview.chromium.org/7646003/diff/18001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): http://codereview.chromium.org/7646003/diff/18001/chrome/chrome_browser.gypi#... chrome/chrome_browser.gypi:2974: 'browser/ui/panels/auto_hide_bottom_bar_linux.cc', Usually, _gtk.cc is the suffix, not _linux.cc. Using a different ending might miss some platform excludes in .gyp files.
Drive-by comments. I think if panels have any special behavior for auto-hide taskbars on screen bottom, e.g. shifting upwards, then they should have similar behaviors for taskbars on the right (shifting leftwards when the taskbar opens). http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/au... File chrome/browser/ui/panels/auto_hide_bottom_bar_win.cc (right): http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/au... chrome/browser/ui/panels/auto_hide_bottom_bar_win.cc:120: // might move the taskbar from non-bottom edge to bottom edge. Note that it is technically possible to have multiple auto-hide taskbars simultaneously, on various different screen edges. Your code should not assume that there is exactly or at most one such bar. http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/au... chrome/browser/ui/panels/auto_hide_bottom_bar_win.cc:131: HWND AutoHideBottomBarWin::GetAutoHideWindowOnScreenEdge(UINT edge) const { It looks like this function is almost identical to code in native_widget_win.cc. Can you just make that version return an HWND, or find some other way to avoid having this code two places?
On Tue, Aug 23, 2011 at 1:39 PM, <pkasting@chromium.org> wrote: > Drive-by comments. > > I think if panels have any special behavior for auto-hide taskbars on > screen > bottom, e.g. shifting upwards, then they should have similar behaviors for > taskbars on the right (shifting leftwards when the taskbar opens). We do not shift panels upward when the bottom auto-hide taskbar becomes visible. Instead, we always keep panels above the potential taskbar area. The reason for doing this If the auto-hide taskbar is moved to the right, we do not want to do the same thing since most of the time, the right-most panel, no matter whether it is expanded or minimized, will not be obstructed by the right taskbar. Only when the mouse is moved to the right-most screen point, the taskbar will show up. We think it is fine to live with this since we do not want to lose that piece of space. > > > > http://codereview.chromium.**org/7646003/diff/18001/chrome/** > browser/ui/panels/auto_hide_**bottom_bar_win.cc<http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/auto_hide_bottom_bar_win.cc> > File chrome/browser/ui/panels/auto_**hide_bottom_bar_win.cc (right): > > http://codereview.chromium.**org/7646003/diff/18001/chrome/** > browser/ui/panels/auto_hide_**bottom_bar_win.cc#newcode120<http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/auto_hide_bottom_bar_win.cc#newcode120> > chrome/browser/ui/panels/auto_**hide_bottom_bar_win.cc:120: // might move > the taskbar from non-bottom edge to bottom edge. > Note that it is technically possible to have multiple auto-hide taskbars > simultaneously, on various different screen edges. Your code should not > assume that there is exactly or at most one such bar. > > http://codereview.chromium.**org/7646003/diff/18001/chrome/** > browser/ui/panels/auto_hide_**bottom_bar_win.cc#newcode131<http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/auto_hide_bottom_bar_win.cc#newcode131> > chrome/browser/ui/panels/auto_**hide_bottom_bar_win.cc:131: HWND > AutoHideBottomBarWin::**GetAutoHideWindowOnScreenEdge(**UINT edge) const { > It looks like this function is almost identical to code in > native_widget_win.cc. Can you just make that version return an HWND, or > find some other way to avoid having this code two places? > > > http://codereview.chromium.**org/7646003/<http://codereview.chromium.org/7646... >
On Tue, Aug 23, 2011 at 4:24 PM, Jian Li <jianli@chromium.org> wrote: > > > On Tue, Aug 23, 2011 at 1:39 PM, <pkasting@chromium.org> wrote: > >> Drive-by comments. >> >> I think if panels have any special behavior for auto-hide taskbars on >> screen >> bottom, e.g. shifting upwards, then they should have similar behaviors for >> taskbars on the right (shifting leftwards when the taskbar opens). > > > We do not shift panels upward when the bottom auto-hide taskbar becomes > visible. Instead, we always keep panels above the potential taskbar area. > The reason for doing this > (Sorry, hit Send too fast). I mean: the reason for doing this is to make the panel in titlebar-only state not being blocked by the taskbar when the mouse is over the bottom. > > If the auto-hide taskbar is moved to the right, we do not want to do the > same thing since most of the time, the right-most panel, no matter whether > it is expanded or minimized, will not be obstructed by the right taskbar. > Only when the mouse is moved to the right-most screen point, the taskbar > will show up. We think it is fine to live with this since we do not want to > lose that piece of space. > >> >> >> >> http://codereview.chromium.**org/7646003/diff/18001/chrome/** >> browser/ui/panels/auto_hide_**bottom_bar_win.cc<http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/auto_hide_bottom_bar_win.cc> >> File chrome/browser/ui/panels/auto_**hide_bottom_bar_win.cc (right): >> >> http://codereview.chromium.**org/7646003/diff/18001/chrome/** >> browser/ui/panels/auto_hide_**bottom_bar_win.cc#newcode120<http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/auto_hide_bottom_bar_win.cc#newcode120> >> chrome/browser/ui/panels/auto_**hide_bottom_bar_win.cc:120: // might move >> the taskbar from non-bottom edge to bottom edge. >> Note that it is technically possible to have multiple auto-hide taskbars >> simultaneously, on various different screen edges. Your code should not >> assume that there is exactly or at most one such bar. >> >> http://codereview.chromium.**org/7646003/diff/18001/chrome/** >> browser/ui/panels/auto_hide_**bottom_bar_win.cc#newcode131<http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/auto_hide_bottom_bar_win.cc#newcode131> >> chrome/browser/ui/panels/auto_**hide_bottom_bar_win.cc:131: HWND >> AutoHideBottomBarWin::**GetAutoHideWindowOnScreenEdge(**UINT edge) const >> { >> It looks like this function is almost identical to code in >> native_widget_win.cc. Can you just make that version return an HWND, or >> find some other way to avoid having this code two places? >> >> >> http://codereview.chromium.**org/7646003/<http://codereview.chromium.org/7646... >> > >
On 2011/08/23 23:24:53, jianli wrote: > We do not shift panels upward when the bottom auto-hide taskbar becomes > visible. Instead, we always keep panels above the potential taskbar area. > The reason for doing this is to make the panel in titlebar-only state not > being blocked by the taskbar when the mouse is over the bottom. This seems potentially bad if the auto-hide taskbar is large, e.g. if the user has resized it to 3x or more of its "typical" height (which some people do in order to change the taskbar to a kind of uber-launcher app). Then you'll just have panels seemingly floating in space. Even with a normal-height taskbar this seems odd-looking. Why not just shift things up when the taskbar unhides? Then you leave no unusual gaps and you can handle any size taskbar. For taskbars on the right, OTOH, I think it actually makes sense to just move the panel away from the taskbar area like you're currently doing for the bottom. Then you don't have overlap issues (especially if panels have embedded links, buttons, or other controls that extend toward the right side), and it doesn't look particularly weird.
On Tue, Aug 23, 2011 at 4:30 PM, <pkasting@chromium.org> wrote: > On 2011/08/23 23:24:53, jianli wrote: > >> We do not shift panels upward when the bottom auto-hide taskbar becomes >> visible. Instead, we always keep panels above the potential taskbar area. >> The reason for doing this is to make the panel in titlebar-only state not >> being blocked by the taskbar when the mouse is over the bottom. >> > > This seems potentially bad if the auto-hide taskbar is large, e.g. if the > user > has resized it to 3x or more of its "typical" height (which some people do > in > order to change the taskbar to a kind of uber-launcher app). Then you'll > just > have panels seemingly floating in space. Even with a normal-height taskbar > this > seems odd-looking. > > Why not just shift things up when the taskbar unhides? Then you leave no > unusual gaps and you can handle any size taskbar. > We thought about this, but this approach also has other problems: 1) If we have many panels, it will be kind of slow when we shift all of them up or down when auto-hide taskbar appear or disappear. 2) If the user is working on something at the bottom of the panel, like a link, the user may move the mouse close enough to the bottom of the screen that causes the taskbar to appear and the panel to shift up. Then the user moves the mouse up to catch that which may causes the taskbar to disappear and the panel to go down. Basically we would like to keep panels at stable positions so that the user will not get distracted by the auto-hide taskbar behavior. In addition, the panels are more like pop-ups. So if they're expanded, we expect the user to interact with them a lot. Otherwise, the user could simply minimize the panels to 3-pixel lines that sits at the bottom of the screen. So if the users like to keep a very wide auto-hide taskbar, they can reduce the unwanted gap by rearranging and minimizing panels. This is the model we currently choose to hope it works for most users per the discussion with UI guy. We can revisit this if needed. > > For taskbars on the right, OTOH, I think it actually makes sense to just > move > the panel away from the taskbar area like you're currently doing for the > bottom. > Then you don't have overlap issues (especially if panels have embedded > links, > buttons, or other controls that extend toward the right side), and it > doesn't > look particularly weird. Normally we do not expect the users to move to the far right side of the element to click on it. It is more likely for the bottom element since these elements tend to have far bigger width than height. > > > http://codereview.chromium.**org/7646003/<http://codereview.chromium.org/7646... >
On 2011/08/24 00:21:14, jianli wrote: > 1) If we have many panels, it will be kind of slow when we shift all of them > up or down when auto-hide taskbar appear or disappear. ? If we can't move all the panels instantaneously, something seems wrong. > 2) If the user is working on something at the bottom of the panel, like a > link, the user may move the mouse close enough to the bottom of the screen > that causes the taskbar to appear and the panel to shift up. Then the user > moves the mouse up to catch that which may causes the taskbar to disappear > and the panel to go down. Ah, very good point. That's a real killer for this proposal; I withdraw it. > Normally we do not expect the users to move to the far right side of the > element to click on it. It is more likely for the bottom element since these > elements tend to have far bigger width than height. Even if true, that just sounds like the bad cases would occur less often; it doesn't really produce a strong argument for not doing this. At least some early mockups and implementations I saw had controls in the top right corner of the panel. Even if those are gone, one could easily imaging instances where controls on the right are common. For example, Glen has argued for reimplementing downloaded files using panels. In this case, if you imagine something like http://pixelsdaily.com/wp-content/uploads/2011/08/Safari-v5.1OSX-Lion-Downloa... in a panel, you can see why we might wind up putting controls on the right-hand side. It seems like just insetting from the right the way you're doing for the bottom would address these sorts of issues without any real downside I can see.
On 2011/08/24 00:42:49, Peter Kasting wrote: > At least some early mockups and implementations I saw had controls in the top > right corner of the panel. Even if those are gone, one could easily imaging > instances where controls on the right are common. For example, Glen has argued > for reimplementing downloaded files using panels. In this case, if you imagine > something like > http://pixelsdaily.com/wp-content/uploads/2011/08/Safari-v5.1OSX-Lion-Downloa... > in a panel, you can see why we might wind up putting controls on the right-hand > side. > > It seems like just insetting from the right the way you're doing for the bottom > would address these sorts of issues without any real downside I can see. I think I agree with Peter here. Avoiding overlap with auto-hiding taskbar on the left/right side by indenting all the panels seems like a good idea. At least until we dogfood the thing, I am sure there will be a lot of fine-tuning at that stage. Adding that indent could be a separate patch though.
I talked with Ken Moore and Glen at lunch today and they were both in favor of avoiding the side taskbar area the way we already avoid the bottom taskbar area.
Yes, I am making changes to do the same thing for left/right auto-hide taskbar. For the top taskbar, we do not have such problem since the panel cannot reach that high due to max height constraint. On Wed, Aug 24, 2011 at 3:04 PM, <pkasting@chromium.org> wrote: > I talked with Ken Moore and Glen at lunch today and they were both in favor > of > avoiding the side taskbar area the way we already avoid the bottom taskbar > area. > > > http://codereview.chromium.**org/7646003/<http://codereview.chromium.org/7646... >
http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/au... File chrome/browser/ui/panels/auto_hide_bottom_bar.h (right): http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/au... chrome/browser/ui/panels/auto_hide_bottom_bar.h:16: // located at the bottom of the screen and set to auto-hide, like Windows On 2011/08/23 20:28:34, jennb wrote: > Just thinking ahead: this one handles bottom of screen only. Would we need a > separate class for task/app bars on the left/right of screen, or would we extend > this class to handle left/right cases as well? Refactored to handle all other edges. http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/au... chrome/browser/ui/panels/auto_hide_bottom_bar.h:46: virtual bool IsEnabled() = 0; On 2011/08/23 20:28:34, jennb wrote: > After reading more, I realize this value does double duty as both a boolean for > is-auto-hide-enabled and a boolean for is-system-bar-aligned-to-bottom. I wish > there was some way to make this clearer than "IsEnabled", but I can live with it > if not. Now it takes an alignment parameter. http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/au... chrome/browser/ui/panels/auto_hide_bottom_bar.h:50: On 2011/08/23 20:28:34, jennb wrote: > Extra blank line? Not needed. Removed. http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/au... File chrome/browser/ui/panels/auto_hide_bottom_bar_linux.cc (right): http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/au... chrome/browser/ui/panels/auto_hide_bottom_bar_linux.cc:12: class AutoHideBottomBarLinux : public AutoHideBottomBar { On 2011/08/23 20:28:34, jennb wrote: > Use GTK suffix. Done. Also added new file for chromeos. http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/au... File chrome/browser/ui/panels/auto_hide_bottom_bar_win.cc (right): http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/au... chrome/browser/ui/panels/auto_hide_bottom_bar_win.cc:120: // might move the taskbar from non-bottom edge to bottom edge. On 2011/08/23 20:39:18, Peter Kasting wrote: > Note that it is technically possible to have multiple auto-hide taskbars > simultaneously, on various different screen edges. Your code should not assume > that there is exactly or at most one such bar. Fixed. http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/au... chrome/browser/ui/panels/auto_hide_bottom_bar_win.cc:131: HWND AutoHideBottomBarWin::GetAutoHideWindowOnScreenEdge(UINT edge) const { On 2011/08/23 20:39:18, Peter Kasting wrote: > It looks like this function is almost identical to code in native_widget_win.cc. > Can you just make that version return an HWND, or find some other way to avoid > having this code two places? Done. http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/au... chrome/browser/ui/panels/auto_hide_bottom_bar_win.cc:145: if (!window_ || !aligned_to_bottom_) On 2011/08/23 20:28:34, jennb wrote: > if (!IsEnabled()) Changed. http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/ba... File chrome/browser/ui/panels/base_panel_browser_test.h (right): http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/ba... chrome/browser/ui/panels/base_panel_browser_test.h:13: #include "chrome/common/chrome_switches.h" On 2011/08/23 20:28:34, jennb wrote: > alpha ordering wrong Done. http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/ba... chrome/browser/ui/panels/base_panel_browser_test.h:57: virtual void SetUp() OVERRIDE { On 2011/08/23 20:28:34, jennb wrote: > Comments in InProcessBrowserTest header say to override > SetUpInProcessBrowserTextFixture instead of SetUp. > > Once you do that, the message loop is already created, so you could get by > without CreateForTesting. Instead, do GetInstance on PanelManager, then call > SetWorkArea and SetAutoHideBottomBar to customize for testing. Changed to override SetUpOnMainThread since the message loop is not up yet on SetUpInProcessBrowserTextFixture. http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/ba... chrome/browser/ui/panels/base_panel_browser_test.h:90: bounds.set_size(gfx::Size(kDefaultPanelWidth, kDefaultPanelHeight)); On 2011/08/23 20:28:34, jennb wrote: > Is this necessary? PanelManager will pick defaults for the panel if bounds are > not provided, right? Done. http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/ba... chrome/browser/ui/panels/base_panel_browser_test.h:154: class MockAutoHideBottomBar : public AutoHideBottomBar { On 2011/08/23 20:28:34, jennb wrote: > Would code be cleaner if this class was NOT nested inside BasePanelBrowserTest? > If you want to keep it private, you can move it to the .cc file. Done. http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/pa... File chrome/browser/ui/panels/panel_browser_view_browsertest.cc (right): http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_browser_view_browsertest.cc:343: void TestChangeAutoHideTaskBarHeight() { On 2011/08/23 20:28:34, jennb wrote: > Does this test need updating as we found logic errors in get bottom code? Updated. http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_browser_view_browsertest.cc:505: // Test that the taskbar is always visible. On 2011/08/23 20:28:34, jennb wrote: > Don't need this comment. Test name is pretty clear. Ditto for auto hide test. Done. http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/pa... File chrome/browser/ui/panels/panel_manager.cc (right): http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_manager.cc:39: const int kMaxTimeWaitForBottomBarVisibilityChange = 1000; On 2011/08/23 20:28:34, jennb wrote: > s/Time/<name of time unit> Done. http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_manager.cc:341: void PanelManager::DelayedBringUpOrDownTitlebarsCheck() { On 2011/08/23 20:28:34, jennb wrote: > Seems like this could be coded more compactly: > > if (delayed_titlebar_action_ == NO_ACTION) > return; > > DoBringUpOrDownTitlebars(delayed_titlebar_action_ == BRING_UP); > delayed_titlebar_action_ = NO_ACTION_; > return; Done. http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_manager.cc:378: int PanelManager::GetBottomPositionForExpansionState( On 2011/08/23 20:28:34, jennb wrote: > As discussed, some logic error computing bottom here... Done. http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_manager.cc:388: switch (delayed_titlebar_action_) { On 2011/08/23 20:28:34, jennb wrote: > Feels like this method can be coded more compactly as well. Done. http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_manager.cc:410: current_x_ = work_area_.right(); On 2011/08/23 20:28:34, jennb wrote: > Why does current_x_ get updated when height changed? Changed the logic such that current_x_ is removed. http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/pa... File chrome/browser/ui/panels/panel_manager.h (right): http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_manager.h:46: // Should we bring up the titlebar, given the current mouse point? On 2011/08/23 20:28:34, jennb wrote: > Nit: Change comment to "Returns true if we should bring up the ... " Done. http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_manager.h:63: // bar handler, for the testing purpose. We cannot instantiate the default On 2011/08/23 20:28:34, jennb wrote: > nit: s/for the testing purpose/for testing purposes Done. http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_manager.h:64: // PanelManager instance by calling PanelManager::GetInstance() in the test On 2011/08/23 20:28:34, jennb wrote: > Must the PanelManager instance be instantiated in SetUp? Why not do this at the > beginning of a test, after message loop has been initialized? Done. http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_manager.h:74: enum DelayedTitlebarAction { On 2011/08/23 20:28:34, jennb wrote: > Drop 'Delay' prefix. How you use it later brings about the delay part so there's > no need to have delay in the name here. Done. http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_manager.h:121: void DelayedBringUpOrDownTitlebarsCheck(); On 2011/08/23 20:28:34, jennb wrote: > Comment? What is it checking? Done. http://codereview.chromium.org/7646003/diff/18001/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_manager.h:125: static void CreateForTestingHelper(const gfx::Rect& work_area, On 2011/08/23 20:28:34, jennb wrote: > Why is a helper needed? Removed. Not needed any more. http://codereview.chromium.org/7646003/diff/18001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): http://codereview.chromium.org/7646003/diff/18001/chrome/chrome_browser.gypi#... chrome/chrome_browser.gypi:2974: 'browser/ui/panels/auto_hide_bottom_bar_linux.cc', On 2011/08/23 20:28:34, jennb wrote: > Usually, _gtk.cc is the suffix, not _linux.cc. Using a different ending might > miss some platform excludes in .gyp files. Done.
Looking much better. Mostly minor nits. http://codereview.chromium.org/7646003/diff/30001/chrome/browser/ui/panels/au... File chrome/browser/ui/panels/auto_hiding_desktop_bar.h (right): http://codereview.chromium.org/7646003/diff/30001/chrome/browser/ui/panels/au... chrome/browser/ui/panels/auto_hiding_desktop_bar.h:36: class Observer { Which one of these is called when a non-auto-hiding taskbar becomes auto-hiding or vice versa? http://codereview.chromium.org/7646003/diff/30001/chrome/browser/ui/panels/au... File chrome/browser/ui/panels/auto_hiding_desktop_bar_win.cc (right): http://codereview.chromium.org/7646003/diff/30001/chrome/browser/ui/panels/au... chrome/browser/ui/panels/auto_hiding_desktop_bar_win.cc:23: const int kAutoHideTaskbarThickness = 2; kHiddenAutoHideTaskbarThickness? kAutoHideTaskbarThicknessWhenHidden? http://codereview.chromium.org/7646003/diff/30001/chrome/browser/ui/panels/au... chrome/browser/ui/panels/auto_hiding_desktop_bar_win.cc:85: bool exists_taskbar = CheckTaskbars(false); s/exists_taskbar/taskbar_exists ? http://codereview.chromium.org/7646003/diff/30001/chrome/browser/ui/panels/au... chrome/browser/ui/panels/auto_hiding_desktop_bar_win.cc:87: // If not any auto-hiding taskbar exists, we do not need to start the polling s/not any/no http://codereview.chromium.org/7646003/diff/30001/chrome/browser/ui/panels/au... chrome/browser/ui/panels/auto_hiding_desktop_bar_win.cc:146: AutoHidingDesktopBarWin::GetVisibilityFromBounds( Can you write a unittest that just tests this one single method with different bounds to see if the expected results are returned? http://codereview.chromium.org/7646003/diff/30001/chrome/browser/ui/panels/au... chrome/browser/ui/panels/auto_hiding_desktop_bar_win.cc:147: AutoHidingDesktopBar::Alignment alignment, const gfx::Rect& bounds) const { Might help to rename bounds to taskbar_bounds. I kept reading the code as if we're comparing the panel bounds against the work area. http://codereview.chromium.org/7646003/diff/30001/chrome/browser/ui/panels/au... chrome/browser/ui/panels/auto_hiding_desktop_bar_win.cc:152: else if (bounds.y() > work_area_.bottom() - kAutoHideTaskbarThickness) Should be >= ? http://codereview.chromium.org/7646003/diff/30001/chrome/browser/ui/panels/au... chrome/browser/ui/panels/auto_hiding_desktop_bar_win.cc:160: else if (bounds.x() > work_area_.x() + kAutoHideTaskbarThickness) Should this be bounds.right() <= work_area_.x() + thickness ? http://codereview.chromium.org/7646003/diff/30001/chrome/browser/ui/panels/au... chrome/browser/ui/panels/auto_hiding_desktop_bar_win.cc:168: else if (bounds.x() > work_area_.right() - kAutoHideTaskbarThickness) Should be >= ? http://codereview.chromium.org/7646003/diff/30001/chrome/browser/ui/panels/au... chrome/browser/ui/panels/auto_hiding_desktop_bar_win.cc:184: bool exists_taskbar = false; s/exists_taskbar/taskbar_exists http://codereview.chromium.org/7646003/diff/30001/chrome/browser/ui/panels/au... chrome/browser/ui/panels/auto_hiding_desktop_bar_win.cc:192: if (!exists_taskbar) { Maybe we should skip this if-clause and let the code continue. That way we get notifications when we go from having taskbars to not having any. http://codereview.chromium.org/7646003/diff/30001/chrome/browser/ui/panels/pa... File chrome/browser/ui/panels/panel_browser_view_browsertest.cc (right): http://codereview.chromium.org/7646003/diff/30001/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_browser_view_browsertest.cc:5: #include "chrome/browser/ui/panels/base_panel_browser_test.h" Should this be placed in alpha ordering? http://codereview.chromium.org/7646003/diff/30001/chrome/browser/ui/panels/pa... File chrome/browser/ui/panels/panel_manager.h (right): http://codereview.chromium.org/7646003/diff/30001/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_manager.h:49: // point? s/?/. http://codereview.chromium.org/7646003/diff/30001/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_manager.h:124: // for the notifications to trigger it any more, and start to bring them up or s/any more/anymore http://codereview.chromium.org/7646003/diff/30001/views/widget/monitor_win.cc File views/widget/monitor_win.cc (right): http://codereview.chromium.org/7646003/diff/30001/views/widget/monitor_win.cc... views/widget/monitor_win.cc:33: if (::IsWindow(taskbar) && (monitor != NULL) && If we always return null unless monitor is set, should we check monitor != NULL first thing in this method and bail early?
http://codereview.chromium.org/7646003/diff/30001/chrome/browser/ui/panels/au... File chrome/browser/ui/panels/auto_hiding_desktop_bar.h (right): http://codereview.chromium.org/7646003/diff/30001/chrome/browser/ui/panels/au... chrome/browser/ui/panels/auto_hiding_desktop_bar.h:36: class Observer { On 2011/08/26 20:41:57, jennb wrote: > Which one of these is called when a non-auto-hiding taskbar becomes auto-hiding > or vice versa? In such case, the work area will be changed and thus PanelManager::OnDisplayChanged will get called. http://codereview.chromium.org/7646003/diff/30001/chrome/browser/ui/panels/au... File chrome/browser/ui/panels/auto_hiding_desktop_bar_win.cc (right): http://codereview.chromium.org/7646003/diff/30001/chrome/browser/ui/panels/au... chrome/browser/ui/panels/auto_hiding_desktop_bar_win.cc:23: const int kAutoHideTaskbarThickness = 2; On 2011/08/26 20:41:57, jennb wrote: > kHiddenAutoHideTaskbarThickness? > kAutoHideTaskbarThicknessWhenHidden? Done. http://codereview.chromium.org/7646003/diff/30001/chrome/browser/ui/panels/au... chrome/browser/ui/panels/auto_hiding_desktop_bar_win.cc:85: bool exists_taskbar = CheckTaskbars(false); On 2011/08/26 20:41:57, jennb wrote: > s/exists_taskbar/taskbar_exists ? Done. http://codereview.chromium.org/7646003/diff/30001/chrome/browser/ui/panels/au... chrome/browser/ui/panels/auto_hiding_desktop_bar_win.cc:87: // If not any auto-hiding taskbar exists, we do not need to start the polling On 2011/08/26 20:41:57, jennb wrote: > s/not any/no Done. http://codereview.chromium.org/7646003/diff/30001/chrome/browser/ui/panels/au... chrome/browser/ui/panels/auto_hiding_desktop_bar_win.cc:146: AutoHidingDesktopBarWin::GetVisibilityFromBounds( On 2011/08/26 20:41:57, jennb wrote: > Can you write a unittest that just tests this one single method with different > bounds to see if the expected results are returned? Done. http://codereview.chromium.org/7646003/diff/30001/chrome/browser/ui/panels/au... chrome/browser/ui/panels/auto_hiding_desktop_bar_win.cc:147: AutoHidingDesktopBar::Alignment alignment, const gfx::Rect& bounds) const { On 2011/08/26 20:41:57, jennb wrote: > Might help to rename bounds to taskbar_bounds. I kept reading the code as if > we're comparing the panel bounds against the work area. Done. http://codereview.chromium.org/7646003/diff/30001/chrome/browser/ui/panels/au... chrome/browser/ui/panels/auto_hiding_desktop_bar_win.cc:152: else if (bounds.y() > work_area_.bottom() - kAutoHideTaskbarThickness) On 2011/08/26 20:41:57, jennb wrote: > Should be >= ? Done. http://codereview.chromium.org/7646003/diff/30001/chrome/browser/ui/panels/au... chrome/browser/ui/panels/auto_hiding_desktop_bar_win.cc:160: else if (bounds.x() > work_area_.x() + kAutoHideTaskbarThickness) On 2011/08/26 20:41:57, jennb wrote: > Should this be bounds.right() <= work_area_.x() + thickness ? Done. http://codereview.chromium.org/7646003/diff/30001/chrome/browser/ui/panels/au... chrome/browser/ui/panels/auto_hiding_desktop_bar_win.cc:168: else if (bounds.x() > work_area_.right() - kAutoHideTaskbarThickness) On 2011/08/26 20:41:57, jennb wrote: > Should be >= ? Done. http://codereview.chromium.org/7646003/diff/30001/chrome/browser/ui/panels/au... chrome/browser/ui/panels/auto_hiding_desktop_bar_win.cc:184: bool exists_taskbar = false; On 2011/08/26 20:41:57, jennb wrote: > s/exists_taskbar/taskbar_exists Done. http://codereview.chromium.org/7646003/diff/30001/chrome/browser/ui/panels/au... chrome/browser/ui/panels/auto_hiding_desktop_bar_win.cc:192: if (!exists_taskbar) { On 2011/08/26 20:41:57, jennb wrote: > Maybe we should skip this if-clause and let the code continue. That way we get > notifications when we go from having taskbars to not having any. As mentioned above, this is taken care of by work area change notification. http://codereview.chromium.org/7646003/diff/30001/chrome/browser/ui/panels/pa... File chrome/browser/ui/panels/panel_browser_view_browsertest.cc (right): http://codereview.chromium.org/7646003/diff/30001/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_browser_view_browsertest.cc:5: #include "chrome/browser/ui/panels/base_panel_browser_test.h" On 2011/08/26 20:41:57, jennb wrote: > Should this be placed in alpha ordering? Done. http://codereview.chromium.org/7646003/diff/30001/chrome/browser/ui/panels/pa... File chrome/browser/ui/panels/panel_manager.h (right): http://codereview.chromium.org/7646003/diff/30001/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_manager.h:49: // point? On 2011/08/26 20:41:57, jennb wrote: > s/?/. Done. http://codereview.chromium.org/7646003/diff/30001/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_manager.h:124: // for the notifications to trigger it any more, and start to bring them up or On 2011/08/26 20:41:57, jennb wrote: > s/any more/anymore Seems any more is better. http://codereview.chromium.org/7646003/diff/30001/views/widget/monitor_win.cc File views/widget/monitor_win.cc (right): http://codereview.chromium.org/7646003/diff/30001/views/widget/monitor_win.cc... views/widget/monitor_win.cc:33: if (::IsWindow(taskbar) && (monitor != NULL) && On 2011/08/26 20:41:57, jennb wrote: > If we always return null unless monitor is set, should we check monitor != NULL > first thing in this method and bail early? Done.
LGTM pending try jobs and dimich re-review. http://codereview.chromium.org/7646003/diff/38001/chrome/browser/ui/panels/au... File chrome/browser/ui/panels/auto_hiding_desktop_bar_win_unittest.cc (right): http://codereview.chromium.org/7646003/diff/38001/chrome/browser/ui/panels/au... chrome/browser/ui/panels/auto_hiding_desktop_bar_win_unittest.cc:28: AutoHidingDesktopBar::Visibility visivility; typo: visivility
LGTM
native_widget_win.cc/monitor_win.* LGTM http://codereview.chromium.org/7646003/diff/38001/views/widget/monitor_win.cc File views/widget/monitor_win.cc (right): http://codereview.chromium.org/7646003/diff/38001/views/widget/monitor_win.cc... views/widget/monitor_win.cc:28: if (monitor == NULL) Nit: I'd roll this into the conditional below like the old code did. http://codereview.chromium.org/7646003/diff/38001/views/widget/monitor_win.cc... views/widget/monitor_win.cc:30: APPBARDATA taskbar_data = { 0 }; Nit: Shorter: APPBARDATA taskbar_data = { sizeof APPBARDATA, NULL, 0, edge }; http://codereview.chromium.org/7646003/diff/38001/views/widget/monitor_win.cc... views/widget/monitor_win.cc:35: if (::IsWindow(taskbar) && Nit: Shorter: return (...) ? taskbar : NULL;
http://codereview.chromium.org/7646003/diff/38001/views/widget/monitor_win.cc File views/widget/monitor_win.cc (right): http://codereview.chromium.org/7646003/diff/38001/views/widget/monitor_win.cc... views/widget/monitor_win.cc:28: if (monitor == NULL) On 2011/08/30 18:31:32, Peter Kasting wrote: > Nit: I'd roll this into the conditional below like the old code did. Done. http://codereview.chromium.org/7646003/diff/38001/views/widget/monitor_win.cc... views/widget/monitor_win.cc:30: APPBARDATA taskbar_data = { 0 }; On 2011/08/30 18:31:32, Peter Kasting wrote: > Nit: Shorter: > > APPBARDATA taskbar_data = { sizeof APPBARDATA, NULL, 0, edge }; Done. http://codereview.chromium.org/7646003/diff/38001/views/widget/monitor_win.cc... views/widget/monitor_win.cc:35: if (::IsWindow(taskbar) && On 2011/08/30 18:31:32, Peter Kasting wrote: > Nit: Shorter: > > return (...) ? > taskbar : NULL; Done. |
