Aura: Tooltips

Aura: Tooltip refactor. This Cl does the following:

* Add tooltip functionality to aura_shell that can be access through aura client API
* Provide an implementation of views::TooltipManager for aura (TooltipManagerAura) that views can use to display/update tooltips in aura
* Remove the no longer needed TooltipManagerViews.

BUG=97249
TEST=none


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

[Ben Goodger (Google), 2011-11-30 21:54:19]

http://codereview.chromium.org/8747022/diff/2001/ui/aura_shell/shell_tooltip_...
File ui/aura_shell/shell_tooltip_manager.cc (right):

http://codereview.chromium.org/8747022/diff/2001/ui/aura_shell/shell_tooltip_...
ui/aura_shell/shell_tooltip_manager.cc:53: return monitor_bounds.width() == 0 ?
800 : (monitor_bounds.width() + 1) / 2;
does this happen? (return 0)?

http://codereview.chromium.org/8747022/diff/2001/ui/aura_shell/shell_tooltip_...
ui/aura_shell/shell_tooltip_manager.cc:109: widget->SetOpacity(0xFF);
I believe this will be the default opacity, so you won't need this line.

http://codereview.chromium.org/8747022/diff/2001/ui/aura_shell/shell_tooltip_...
ui/aura_shell/shell_tooltip_manager.cc:129:
tooltip_widget_->SetAlwaysOnTop(true);
you shouldn't need this. the stacking controller should be smart enough to stack
the tooltip widget into its own container.

http://codereview.chromium.org/8747022/diff/2001/ui/aura_shell/shell_tooltip_...
ui/aura_shell/shell_tooltip_manager.cc:213: int max_width, line_count;
by doing the Tooltip class thing I mentioned in the previous file, you can
encapsulate all this adjustment into the Tooltip class, and simply do:

tooltip_->SetText();
tooltip_->Show();

or something like that.

Everyone else in this class that configures tooltip_widget_ directly can now go
through Tooltip.

http://codereview.chromium.org/8747022/diff/2001/ui/aura_shell/shell_tooltip_...
File ui/aura_shell/shell_tooltip_manager.h (right):

http://codereview.chromium.org/8747022/diff/2001/ui/aura_shell/shell_tooltip_...
ui/aura_shell/shell_tooltip_manager.h:72: scoped_ptr<views::Widget>
tooltip_widget_;
Instead of tooltip_widget_, I'd create some class Tooltip and forward declare it
here and have a member pointer:

class Tooltip;
scoped_ptr<Tooltip> tooltip_;

I would define Tooltip in an anonymous namespace at the top of the .cc file. The
Tooltip class would have a Label. You would not subclass Label. Instead, fix
border rendering in label.cc... it's just broken.

The Tooltip class would also own this widget.

Tooltip would present a simple API of:

SetText()
Show();
Hide();

[varunjain, 2011-11-30 22:42:07]

http://codereview.chromium.org/8747022/diff/2001/ui/aura_shell/shell_tooltip_...
File ui/aura_shell/shell_tooltip_manager.cc (right):

http://codereview.chromium.org/8747022/diff/2001/ui/aura_shell/shell_tooltip_...
ui/aura_shell/shell_tooltip_manager.cc:53: return monitor_bounds.width() == 0 ?
800 : (monitor_bounds.width() + 1) / 2;
On 2011/11/30 21:54:19, Ben Goodger (Google) wrote:
> does this happen? (return 0)?

I am not sure when this could happen. This code is copied from the GTK version.
I'll remove the check and reinsert later if required.

http://codereview.chromium.org/8747022/diff/2001/ui/aura_shell/shell_tooltip_...
ui/aura_shell/shell_tooltip_manager.cc:109: widget->SetOpacity(0xFF);
On 2011/11/30 21:54:19, Ben Goodger (Google) wrote:
> I believe this will be the default opacity, so you won't need this line.

Done.

http://codereview.chromium.org/8747022/diff/2001/ui/aura_shell/shell_tooltip_...
ui/aura_shell/shell_tooltip_manager.cc:129:
tooltip_widget_->SetAlwaysOnTop(true);
On 2011/11/30 21:54:19, Ben Goodger (Google) wrote:
> you shouldn't need this. the stacking controller should be smart enough to
stack
> the tooltip widget into its own container.

Done.

http://codereview.chromium.org/8747022/diff/2001/ui/aura_shell/shell_tooltip_...
ui/aura_shell/shell_tooltip_manager.cc:213: int max_width, line_count;
On 2011/11/30 21:54:19, Ben Goodger (Google) wrote:
> by doing the Tooltip class thing I mentioned in the previous file, you can
> encapsulate all this adjustment into the Tooltip class, and simply do:
> 
> tooltip_->SetText();
> tooltip_->Show();
> 
> or something like that.
> 
> Everyone else in this class that configures tooltip_widget_ directly can now
go
> through Tooltip.

Done.

http://codereview.chromium.org/8747022/diff/2001/ui/aura_shell/shell_tooltip_...
File ui/aura_shell/shell_tooltip_manager.h (right):

http://codereview.chromium.org/8747022/diff/2001/ui/aura_shell/shell_tooltip_...
ui/aura_shell/shell_tooltip_manager.h:72: scoped_ptr<views::Widget>
tooltip_widget_;
On 2011/11/30 21:54:19, Ben Goodger (Google) wrote:
> Instead of tooltip_widget_, I'd create some class Tooltip and forward declare
it
> here and have a member pointer:
> 
> class Tooltip;
> scoped_ptr<Tooltip> tooltip_;
> 
> I would define Tooltip in an anonymous namespace at the top of the .cc file.
The
> Tooltip class would have a Label. You would not subclass Label. Instead, fix
> border rendering in label.cc... it's just broken.
> 
> The Tooltip class would also own this widget.
> 
> Tooltip would present a simple API of:
> 
> SetText()
> Show();
> Hide();

Done.

[Ben Goodger (Google), 2011-11-30 22:52:20]

One last turn of the crank... simple stuff:

http://codereview.chromium.org/8747022/diff/5002/ui/aura_shell/shell_tooltip_...
File ui/aura_shell/shell_tooltip_manager.h (right):

http://codereview.chromium.org/8747022/diff/5002/ui/aura_shell/shell_tooltip_...
ui/aura_shell/shell_tooltip_manager.h:16: #include "ui/base/events.h"
don't think you need thsi one here either

http://codereview.chromium.org/8747022/diff/5002/ui/aura_shell/shell_tooltip_...
ui/aura_shell/shell_tooltip_manager.h:27: class Canvas;
you can get rid of this now

http://codereview.chromium.org/8747022/diff/5002/ui/views/widget/tooltip_mana...
File ui/views/widget/tooltip_manager_aura.cc (right):

http://codereview.chromium.org/8747022/diff/5002/ui/views/widget/tooltip_mana...
ui/views/widget/tooltip_manager_aura.cc:27: return
ui::ResourceBundle::GetSharedInstance().GetFont(
You should be able to replace this function and GetMaxWidth to a call to
TooltipClient::GetDefaultFont/GetMaxWidth, which would be static methods that
you would implement using your code in ShellTooltipManager.

[varunjain, 2011-11-30 23:25:52]

http://codereview.chromium.org/8747022/diff/5002/ui/aura_shell/shell_tooltip_...
File ui/aura_shell/shell_tooltip_manager.h (right):

http://codereview.chromium.org/8747022/diff/5002/ui/aura_shell/shell_tooltip_...
ui/aura_shell/shell_tooltip_manager.h:16: #include "ui/base/events.h"
On 2011/11/30 22:52:20, Ben Goodger (Google) wrote:
> don't think you need thsi one here either

Done.

http://codereview.chromium.org/8747022/diff/5002/ui/aura_shell/shell_tooltip_...
ui/aura_shell/shell_tooltip_manager.h:27: class Canvas;
On 2011/11/30 22:52:20, Ben Goodger (Google) wrote:
> you can get rid of this now

Done.

http://codereview.chromium.org/8747022/diff/5002/ui/views/widget/tooltip_mana...
File ui/views/widget/tooltip_manager_aura.cc (right):

http://codereview.chromium.org/8747022/diff/5002/ui/views/widget/tooltip_mana...
ui/views/widget/tooltip_manager_aura.cc:27: return
ui::ResourceBundle::GetSharedInstance().GetFont(
On 2011/11/30 22:52:20, Ben Goodger (Google) wrote:
> You should be able to replace this function and GetMaxWidth to a call to
> TooltipClient::GetDefaultFont/GetMaxWidth, which would be static methods that
> you would implement using your code in ShellTooltipManager.

Done.

[Ben Goodger (Google), 2011-11-30 23:27:55]

lgtm

[commit-bot: I haz the power, 2011-12-01 02:07:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varunjain@chromium.org/8747022/11007

[commit-bot: I haz the power, 2011-12-01 04:33:55]

Change committed as 112399