Implement learn more bubble on views.

Implement learn more bubble on views.

Screenshots: http://imgur.com/a/3w2eO

Behavior I'd like to add in a separate CL:
- make the bubble show upward if not enough bottom space

BUG=258553
R=estade@chromium.org,thestig@chromium.org
NOTRY=true

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=233255

[Dan Beam, 2013-11-02 01:43:11]

+thestig@: chrome/app
+estade@: for everything else

[Lei Zhang, 2013-11-02 05:14:46]

chrome/app lgtm, though I can never remember if CQ can commit binaries for you
or not.

[Dan Beam, 2013-11-05 01:44:50]

estade@: ping

[Evan Stade, 2013-11-05 02:01:39]

https://codereview.chromium.org/55243005/diff/140001/chrome/browser/ui/views/...
File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right):

https://codereview.chromium.org/55243005/diff/140001/chrome/browser/ui/views/...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:542: if
(anchor_->GetClassName() == LearnMoreIcon::kViewClassName) {
boooo

https://codereview.chromium.org/55243005/diff/140001/chrome/browser/ui/views/...
File chrome/browser/ui/views/autofill/decorated_textfield.cc (right):

https://codereview.chromium.org/55243005/diff/140001/chrome/browser/ui/views/...
chrome/browser/ui/views/autofill/decorated_textfield.cc:129: return
views::Textfield::GetEventHandlerForPoint(point);
I'd write this as:

views::View* handler = views::Textfield::GetEventHandlerForPoint(point);
if (handler classname is LearnMoreIcon)
  return handler;

return native_wrapper_->GetView();

https://codereview.chromium.org/55243005/diff/140001/chrome/browser/ui/views/...
chrome/browser/ui/views/autofill/decorated_textfield.cc:178:
icon_view_->SetSize(gfx::Size());
why necessary?

https://codereview.chromium.org/55243005/diff/140001/chrome/browser/ui/views/...
File chrome/browser/ui/views/autofill/learn_more_icon.h (right):

https://codereview.chromium.org/55243005/diff/140001/chrome/browser/ui/views/...
chrome/browser/ui/views/autofill/learn_more_icon.h:18: // A delegate to notify
when events happen.
I'd much prefer if the bubble were managed by the icon. Removes the need for the
delegate interface, improves encapsulation, reduces amount of special code in
AutofillDialogViews. I think you should still be able to figure out how to make
the bubble float where you want it to. (You'll need to pull
ErrorBubble/InfoBubble into its own file)

https://codereview.chromium.org/55243005/diff/140001/chrome/browser/ui/views/...
chrome/browser/ui/views/autofill/learn_more_icon.h:30: class LearnMoreIcon :
public views::ImageView {
leave the class name as TooltipIcon, or if you hate the name tooltip then maybe
FurtherInfoIcon or some similar mouthful.

[Evan Stade, 2013-11-05 02:01:40]

https://codereview.chromium.org/55243005/diff/140001/chrome/browser/ui/views/...
File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right):

https://codereview.chromium.org/55243005/diff/140001/chrome/browser/ui/views/...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:542: if
(anchor_->GetClassName() == LearnMoreIcon::kViewClassName) {
boooo

https://codereview.chromium.org/55243005/diff/140001/chrome/browser/ui/views/...
File chrome/browser/ui/views/autofill/decorated_textfield.cc (right):

https://codereview.chromium.org/55243005/diff/140001/chrome/browser/ui/views/...
chrome/browser/ui/views/autofill/decorated_textfield.cc:129: return
views::Textfield::GetEventHandlerForPoint(point);
I'd write this as:

views::View* handler = views::Textfield::GetEventHandlerForPoint(point);
if (handler classname is LearnMoreIcon)
  return handler;

return native_wrapper_->GetView();

https://codereview.chromium.org/55243005/diff/140001/chrome/browser/ui/views/...
chrome/browser/ui/views/autofill/decorated_textfield.cc:178:
icon_view_->SetSize(gfx::Size());
why necessary?

https://codereview.chromium.org/55243005/diff/140001/chrome/browser/ui/views/...
File chrome/browser/ui/views/autofill/learn_more_icon.h (right):

https://codereview.chromium.org/55243005/diff/140001/chrome/browser/ui/views/...
chrome/browser/ui/views/autofill/learn_more_icon.h:18: // A delegate to notify
when events happen.
I'd much prefer if the bubble were managed by the icon. Removes the need for the
delegate interface, improves encapsulation, reduces amount of special code in
AutofillDialogViews. I think you should still be able to figure out how to make
the bubble float where you want it to. (You'll need to pull
ErrorBubble/InfoBubble into its own file)

https://codereview.chromium.org/55243005/diff/140001/chrome/browser/ui/views/...
chrome/browser/ui/views/autofill/learn_more_icon.h:30: class LearnMoreIcon :
public views::ImageView {
leave the class name as TooltipIcon, or if you hate the name tooltip then maybe
FurtherInfoIcon or some similar mouthful.

[Dan Beam, 2013-11-05 07:00:54]

https://codereview.chromium.org/55243005/diff/140001/chrome/browser/ui/views/...
File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right):

https://codereview.chromium.org/55243005/diff/140001/chrome/browser/ui/views/...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:542: if
(anchor_->GetClassName() == LearnMoreIcon::kViewClassName) {
On 2013/11/05 02:01:40, Evan Stade wrote:
> boooo

removed

https://codereview.chromium.org/55243005/diff/140001/chrome/browser/ui/views/...
File chrome/browser/ui/views/autofill/decorated_textfield.cc (right):

https://codereview.chromium.org/55243005/diff/140001/chrome/browser/ui/views/...
chrome/browser/ui/views/autofill/decorated_textfield.cc:129: return
views::Textfield::GetEventHandlerForPoint(point);
On 2013/11/05 02:01:40, Evan Stade wrote:
> I'd write this as:
> 
> views::View* handler = views::Textfield::GetEventHandlerForPoint(point);
> if (handler classname is LearnMoreIcon)
>   return handler;
> 
> return native_wrapper_->GetView();

Done.

https://codereview.chromium.org/55243005/diff/140001/chrome/browser/ui/views/...
chrome/browser/ui/views/autofill/decorated_textfield.cc:178:
icon_view_->SetSize(gfx::Size());
On 2013/11/05 02:01:40, Evan Stade wrote:
> why necessary?

not

https://codereview.chromium.org/55243005/diff/140001/chrome/browser/ui/views/...
File chrome/browser/ui/views/autofill/learn_more_icon.h (right):

https://codereview.chromium.org/55243005/diff/140001/chrome/browser/ui/views/...
chrome/browser/ui/views/autofill/learn_more_icon.h:18: // A delegate to notify
when events happen.
On 2013/11/05 02:01:40, Evan Stade wrote:
> I'd much prefer if the bubble were managed by the icon. Removes the need for
the
> delegate interface, improves encapsulation, reduces amount of special code in
> AutofillDialogViews. I think you should still be able to figure out how to
make
> the bubble float where you want it to. (You'll need to pull
> ErrorBubble/InfoBubble into its own file)

Done.

https://codereview.chromium.org/55243005/diff/140001/chrome/browser/ui/views/...
chrome/browser/ui/views/autofill/learn_more_icon.h:30: class LearnMoreIcon :
public views::ImageView {
On 2013/11/05 02:01:40, Evan Stade wrote:
> leave the class name as TooltipIcon, or if you hate the name tooltip then
maybe
> FurtherInfoIcon or some similar mouthful.

left as is

https://codereview.chromium.org/55243005/diff/420001/chrome/browser/ui/views/...
File chrome/browser/ui/views/autofill/info_bubble.h (right):

https://codereview.chromium.org/55243005/diff/420001/chrome/browser/ui/views/...
chrome/browser/ui/views/autofill/info_bubble.h:49: }
^ these can also be protected if it's worth making an
AutofillDialogViews::ErrorBubble (which inherits from InfoBubble)

[Evan Stade, 2013-11-05 19:57:22]

lgtm

https://codereview.chromium.org/55243005/diff/420001/chrome/browser/ui/views/...
File chrome/browser/ui/views/autofill/info_bubble.cc (right):

https://codereview.chromium.org/55243005/diff/420001/chrome/browser/ui/views/...
chrome/browser/ui/views/autofill/info_bubble.cc:35: show_above_anchor_(
I think it would be slightly nicer if there were a setter for this.

InfoBubble bubble(anchor, text);
bubble.set_show_above_anchor(true);

The only reason we want to show above comboboxes is because we show /when the
combobox is clicked/. You could imagine someone creating a bubble that points at
a combobox at some other point, meaning there would be no need for this
adjustment.

https://codereview.chromium.org/55243005/diff/420001/chrome/browser/ui/views/...
File chrome/browser/ui/views/autofill/info_bubble.h (right):

https://codereview.chromium.org/55243005/diff/420001/chrome/browser/ui/views/...
chrome/browser/ui/views/autofill/info_bubble.h:49: }
On 2013/11/05 07:00:55, Dan Beam wrote:
> ^ these can also be protected if it's worth making an
> AutofillDialogViews::ErrorBubble (which inherits from InfoBubble)

seems fine

[commit-bot: I haz the power, 2013-11-05 21:10:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dbeam@chromium.org/55243005/490001

[commit-bot: I haz the power, 2013-11-05 21:10:50]

Failed to apply patch for
chrome/browser/ui/views/autofill/decorated_textfield.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/ui/views/autofill/decorated_textfield.cc
  Hunk #1 FAILED at 122.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/ui/views/autofill/decorated_textfield.cc.rej

Patch:       chrome/browser/ui/views/autofill/decorated_textfield.cc
Index: chrome/browser/ui/views/autofill/decorated_textfield.cc
diff --git a/chrome/browser/ui/views/autofill/decorated_textfield.cc
b/chrome/browser/ui/views/autofill/decorated_textfield.cc
index
47d3b30da56ed413492b8a7e3796dd4a9362d02e..de9a8b1351cf8605f575a0f2657356b857993b3a
100644
--- a/chrome/browser/ui/views/autofill/decorated_textfield.cc
+++ b/chrome/browser/ui/views/autofill/decorated_textfield.cc
@@ -122,6 +122,9 @@ const char* DecoratedTextfield::GetClassName() const {
 
 views::View* DecoratedTextfield::GetEventHandlerForPoint(
     const gfx::Point& point) {
+  views::View* handler = views::Textfield::GetEventHandlerForPoint(point);
+  if (handler->GetClassName() == TooltipIcon::kViewClassName)
+    return handler;
   return native_wrapper_->GetView();
 }

[commit-bot: I haz the power, 2013-11-06 00:26:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dbeam@chromium.org/55243005/600001

[commit-bot: I haz the power, 2013-11-06 04:44:23]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-11-06 04:59:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dbeam@chromium.org/55243005/600001

[commit-bot: I haz the power, 2013-11-06 09:12:04]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-11-06 10:33:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dbeam@chromium.org/55243005/600001

[commit-bot: I haz the power, 2013-11-06 10:35:40]

Change committed as 233255