Make info button appear only when mouse is over the panel or the panel has focus.

Make info button appear only when mouse is over the panel or the panel has focus.

BUG=none
TEST=panel_browser_view_browsertest
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=86499

[jennb, 2011-05-20 23:13:12]

http://codereview.chromium.org/7055001/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_browser_frame_view.h (right):

http://codereview.chromium.org/7055001/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_frame_view.h:77: class MouseWatcher :
public MessageLoopForUI::Observer {
Did you look at using views/mouse_watcher.h?  MouseWatcher only notifies
listener when mouse moves outside of view. Do we really need to watch both
mouse-enter and mouse-leave?  TabStrip uses MouseWatcherListener and yet tab is
able to change appearance when mouse moves in and out of the tab so it looks
like it should be sufficient.

http://codereview.chromium.org/7055001/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_browser_view_browsertest.cc (right):

http://codereview.chromium.org/7055001/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_view_browsertest.cc:193: // The info
button should be visible if the panel is activated or the cursor
Suggest making this a separate test so we can check visibility of info button
even if there are layout bugs.  Also, it's really too bad this test is specific
to Windows panels implementation when checking that info button is visible if
panel is activated or cursor is over the window is something that all platforms
will want to verify.  Let's keep our TODO list of tests that need to be made
platform-independent from getting too long...

[jianli, 2011-05-23 21:21:02]

http://codereview.chromium.org/7055001/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_browser_frame_view.h (right):

http://codereview.chromium.org/7055001/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_frame_view.h:77: class MouseWatcher :
public MessageLoopForUI::Observer {
On 2011/05/20 23:13:12, jennb wrote:
> Did you look at using views/mouse_watcher.h?  MouseWatcher only notifies
> listener when mouse moves outside of view. Do we really need to watch both
> mouse-enter and mouse-leave?  TabStrip uses MouseWatcherListener and yet tab
is
> able to change appearance when mouse moves in and out of the tab so it looks
> like it should be sufficient.

Yes, I've looked at views/mouse_watcher.* to see if I can reuse it.
Unfortunately it is only for mouse leaving and we would add up much more
complexity to it if we want also to support mouse entering. So I rather put our
mouse watching logic in panel code since the panel is the only user. If we have
other scenario needing this in the future, I am happy to refactor it.

The code to change appearance when mouse moves in and out of the tab is in
base_tab.cc (OnMouseEntered and OnMouseExited). This approach did not work in
our panel case for the following reasons:
1) For non-client view, we need to hook up with OnMouseEntered and OnMouseExited
for PanelBrowserFrameView, its view for close button, and its view for info
button. This is because when we move the mouse  from non-close-button area to
close-button area in the title bar, PanelBrowserFrameView gets triggered with
OnMouseExited and the view for close button gets triggered with OnMouseEntered.
This is a bit complicated but it is doable.,
2) For client view,  OnMouseEntered and OnMouseExited are not called at all. I
think this is by design. So we have no way to listen to mouse entering and mouse
exiting event for client view. Even we get notified, we might hard time to
coordinate events from non-client view and client view.

So what I do here seems to be a only reliable way to detect if the mouse moves
in to or out of a panel window, including both non-client view and client view.

http://codereview.chromium.org/7055001/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_browser_view_browsertest.cc (right):

http://codereview.chromium.org/7055001/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_view_browsertest.cc:193: // The info
button should be visible if the panel is activated or the cursor
On 2011/05/20 23:13:12, jennb wrote:
> Suggest making this a separate test so we can check visibility of info button
> even if there are layout bugs.  Also, it's really too bad this test is
specific
> to Windows panels implementation when checking that info button is visible if
> panel is activated or cursor is over the window is something that all
platforms
> will want to verify.  Let's keep our TODO list of tests that need to be made
> platform-independent from getting too long...

Made it a separate test.

[jennb, 2011-05-23 22:33:23]

LGTM

Thanks for researching the other MouseWatcher option.

[Dmitry Titov, 2011-05-24 20:21:56]

LGTM too with couple of suggestions:

http://codereview.chromium.org/7055001/diff/4001/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_browser_frame_view.h (right):

http://codereview.chromium.org/7055001/diff/4001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browser_frame_view.h:97: bool in_view_;
I saw often in the past the suggestion to keep names of this kind of variables
with the "Is" in them, like: is_mouse_within_

http://codereview.chromium.org/7055001/diff/4001/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_browser_view_browsertest.cc (right):

http://codereview.chromium.org/7055001/diff/4001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browser_view_browsertest.cc:31:
cursor_in_view_(false) {
Same: "is_cursor_in_view_"?

http://codereview.chromium.org/7055001/diff/4001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browser_view_browsertest.cc:204:
IN_PROC_BROWSER_TEST_F(PanelBrowserViewTest, ShowOrHideInfoButton) {
I wonder if it can be a unit test. The benefit of the unit test is that they are
very fast because they don't need to launch a browser. I'm not sure how much
faster though. Do you know?

[jianli, 2011-05-24 21:58:55]

http://codereview.chromium.org/7055001/diff/4001/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_browser_frame_view.h (right):

http://codereview.chromium.org/7055001/diff/4001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browser_frame_view.h:97: bool in_view_;
On 2011/05/24 20:21:56, Dmitry Titov wrote:
> I saw often in the past the suggestion to keep names of this kind of variables
> with the "Is" in them, like: is_mouse_within_

Done.

http://codereview.chromium.org/7055001/diff/4001/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_browser_view_browsertest.cc (right):

http://codereview.chromium.org/7055001/diff/4001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browser_view_browsertest.cc:31:
cursor_in_view_(false) {
On 2011/05/24 20:21:56, Dmitry Titov wrote:
> Same: "is_cursor_in_view_"?

Done.

http://codereview.chromium.org/7055001/diff/4001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browser_view_browsertest.cc:204:
IN_PROC_BROWSER_TEST_F(PanelBrowserViewTest, ShowOrHideInfoButton) {
On 2011/05/24 20:21:56, Dmitry Titov wrote:
> I wonder if it can be a unit test. The benefit of the unit test is that they
are
> very fast because they don't need to launch a browser. I'm not sure how much
> faster though. Do you know?

For this case, probably we need to use browser test since we  need to test it
when the panel is activated that needs to have the browser present.