Some improvement of validation message bubble UI.

Some improvement of validation message bubble UI.

* Change anchor position arguments of some functions from the screen
  coordinate system to the root view coordinate system.  Blink can't
  provide a rectangle in the screen coordinate on Android, and we'd like
  to make it consistent. Each of validation message UI implementation
  converts the anchor postion to the screen coordinate.

* Support WebValidationMessageClient::moveValidationMessage.
  Add functions to ValidationMessageAgent, ValidationmessageFilter, and
  ValidationMessageBubble classes, and add new IPC message to
  validation_message_messages.h.

Changs for platform-specific files:
* chrome/browser/ui/android/:
  Just rename argument names; anchor_in_screen -> anchor_in_root_view
* chrome/browser/ui/cocoa/:
  Implement SetPositionRelativeToAnchor, add a helper function
* chrome/browser/ui/gtk/:
  Add BubbleGtk::SetPositionRelativeToAnchor, and use it for
  ValidationMessageBubble::SetPositionRelativeToAnchor implementation
* chrome/browser/ui/views/:
  Add SetPositionRelativeToAnchor implementation.
  Also, fixes a bug that ValidationMessageBubbleDelegate is never
  destructed because Hide() won't call DeleteDelegate. We should call
  Close().

BUG=146212
R=estade@chromium.org, sky@chromium.org, thakis@chromium.org, tsepez@chromium.org

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

[tkent, 2013-06-07 05:51:15]

This CL requires a blink change which is not landed yet.
https://codereview.chromium.org/16180009/

thakis, would you review platform-independent part and cocoa part please?
sky, would you review chrome/browser/ui/views/ please?
estade, would you review chrome/browser/ui/gtk/ please?

[Nico, 2013-06-07 16:35:31]

Looks generally good.

https://codereview.chromium.org/16583005/diff/11001/chrome/browser/ui/android...
File chrome/browser/ui/android/validation_message_bubble_android.h (right):

https://codereview.chromium.org/16583005/diff/11001/chrome/browser/ui/android...
chrome/browser/ui/android/validation_message_bubble_android.h:31: virtual void
MoveOnAnchor(content::RenderWidgetHost* widget_host,
nit: It's not clear to me what "MoveOnAnchor" should do. "MoveToAnchor"?
"ShowAtAnchor"?

https://codereview.chromium.org/16583005/diff/11001/chrome/browser/ui/cocoa/v...
File chrome/browser/ui/cocoa/validation_message_bubble_cocoa.mm (right):

https://codereview.chromium.org/16583005/diff/11001/chrome/browser/ui/cocoa/v...
chrome/browser/ui/cocoa/validation_message_bubble_cocoa.mm:137: NSPoint
GetAnchorPoint(content::RenderWidgetHost* widget_host,
Can you add a comment above this function, explaining what it does? "// Converts
anchor_in_root_view in rwhv coordinates to cocoa screen coordinates" I guess?

https://codereview.chromium.org/16583005/diff/11001/chrome/browser/ui/cocoa/v...
chrome/browser/ui/cocoa/validation_message_bubble_cocoa.mm:145: -
(anchor_in_screen.y() + anchor_in_screen.height()));
If this function does what I think it does, the more usual way to do this is

  NSView* view = widget_host->GetView()->GetNativeView();
  NSRect cocoaRect = [view flipRectToNSRect:anchor_in_root_view];
  NSRect windowRect = [view convertRect:cocoaRect toView:nil];
  NSRect screenRect = [[view window] convertBaseToScreen:windowPoint];
  return screenRect.origin;

https://codereview.chromium.org/16583005/diff/11001/chrome/browser/ui/cocoa/v...
chrome/browser/ui/cocoa/validation_message_bubble_cocoa.mm:165: virtual void
MoveOnAnchor(content::RenderWidgetHost* widget_host,
(same nit as in the android file here too, and in all the other places where
it's overridden)

https://codereview.chromium.org/16583005/diff/11001/chrome/browser/ui/validat...
File chrome/browser/ui/validation_message_bubble.h (right):

https://codereview.chromium.org/16583005/diff/11001/chrome/browser/ui/validat...
chrome/browser/ui/validation_message_bubble.h:34: // Move the window on the
specified anchor.
s/on/to/

https://codereview.chromium.org/16583005/diff/11001/chrome/browser/ui/validat...
chrome/browser/ui/validation_message_bubble.h:34: // Move the window on the
specified anchor.
Say which coordinate system the anchor is in (relative to the containing window,
in dips?)

[sky, 2013-06-07 17:52:52]

What files did you need me to review?

https://codereview.chromium.org/16583005/diff/11001/chrome/browser/ui/validat...
File chrome/browser/ui/validation_message_bubble.h (right):

https://codereview.chromium.org/16583005/diff/11001/chrome/browser/ui/validat...
chrome/browser/ui/validation_message_bubble.h:34: // Move the window on the
specified anchor.
On 2013/06/07 16:35:31, Nico wrote:
> Say which coordinate system the anchor is in (relative to the containing
window,
> in dips?)

This name is also confusing. Maybe something like SetPositionRelativeToAnchor or
something.

[tkent, 2013-06-10 06:15:37]

> What files did you need me to review?

Would you review the following files please?
  chrome/browser/ui/views/validation_message_bubble_delegate.h
  chrome/browser/ui/views/validation_message_bubble_delegate.cc
  chrome/browser/ui/views/validation_message_bubble_view.cc

https://codereview.chromium.org/16583005/diff/11001/chrome/browser/ui/android...
File chrome/browser/ui/android/validation_message_bubble_android.h (right):

https://codereview.chromium.org/16583005/diff/11001/chrome/browser/ui/android...
chrome/browser/ui/android/validation_message_bubble_android.h:31: virtual void
MoveOnAnchor(content::RenderWidgetHost* widget_host,
On 2013/06/07 16:35:31, Nico wrote:
> nit: It's not clear to me what "MoveOnAnchor" should do. "MoveToAnchor"?
> "ShowAtAnchor"?

Done.

https://codereview.chromium.org/16583005/diff/11001/chrome/browser/ui/cocoa/v...
File chrome/browser/ui/cocoa/validation_message_bubble_cocoa.mm (right):

https://codereview.chromium.org/16583005/diff/11001/chrome/browser/ui/cocoa/v...
chrome/browser/ui/cocoa/validation_message_bubble_cocoa.mm:137: NSPoint
GetAnchorPoint(content::RenderWidgetHost* widget_host,
On 2013/06/07 16:35:31, Nico wrote:
> Can you add a comment above this function, explaining what it does? "//
Converts
> anchor_in_root_view in rwhv coordinates to cocoa screen coordinates" I guess?

Done.

https://codereview.chromium.org/16583005/diff/11001/chrome/browser/ui/cocoa/v...
chrome/browser/ui/cocoa/validation_message_bubble_cocoa.mm:145: -
(anchor_in_screen.y() + anchor_in_screen.height()));
On 2013/06/07 16:35:31, Nico wrote:
> If this function does what I think it does, the more usual way to do this is
> 
>   NSView* view = widget_host->GetView()->GetNativeView();
>   NSRect cocoaRect = [view flipRectToNSRect:anchor_in_root_view];
>   NSRect windowRect = [view convertRect:cocoaRect toView:nil];
>   NSRect screenRect = [[view window] convertBaseToScreen:windowPoint];
>   return screenRect.origin;
>   

Done.

https://codereview.chromium.org/16583005/diff/11001/chrome/browser/ui/cocoa/v...
chrome/browser/ui/cocoa/validation_message_bubble_cocoa.mm:165: virtual void
MoveOnAnchor(content::RenderWidgetHost* widget_host,
On 2013/06/07 16:35:31, Nico wrote:
> (same nit as in the android file here too, and in all the other places where
> it's overridden)

Done.

https://codereview.chromium.org/16583005/diff/11001/chrome/browser/ui/validat...
File chrome/browser/ui/validation_message_bubble.h (right):

https://codereview.chromium.org/16583005/diff/11001/chrome/browser/ui/validat...
chrome/browser/ui/validation_message_bubble.h:34: // Move the window on the
specified anchor.
On 2013/06/07 17:52:52, sky wrote:
> On 2013/06/07 16:35:31, Nico wrote:
> > Say which coordinate system the anchor is in (relative to the containing
> window,
> > in dips?)
> 
> This name is also confusing. Maybe something like SetPositionRelativeToAnchor
or
> something.

ok, add comments and renamed it to SetPositionRelativeToAnchor.

[sky, 2013-06-10 14:48:28]

LGTM

https://codereview.chromium.org/16583005/diff/40001/chrome/browser/ui/views/v...
File chrome/browser/ui/views/validation_message_bubble_view.cc (right):

https://codereview.chromium.org/16583005/diff/40001/chrome/browser/ui/views/v...
chrome/browser/ui/views/validation_message_bubble_view.cc:67: const gfx::Rect&
anchor_in_screen = anchor_in_root_view
const gfx::Rect& -> const gfx::Rect

[Nico, 2013-06-10 14:51:40]

lgtm!

https://codereview.chromium.org/16583005/diff/40001/chrome/browser/ui/cocoa/v...
File chrome/browser/ui/cocoa/validation_message_bubble_cocoa.mm (right):

https://codereview.chromium.org/16583005/diff/40001/chrome/browser/ui/cocoa/v...
chrome/browser/ui/cocoa/validation_message_bubble_cocoa.mm:144:
static_cast<BaseView*>(widget_host->GetView()->GetNativeView());
consider base::mac::ObjCCastStrict<> instead of static_cast<>

https://codereview.chromium.org/16583005/diff/40001/chrome/renderer/validatio...
File chrome/renderer/validation_message_agent.h (right):

https://codereview.chromium.org/16583005/diff/40001/chrome/renderer/validatio...
chrome/renderer/validation_message_agent.h:30: // TOOD(tkent): add OVERRIDE
before landing this.
do we add OVERRIDE for overriding blink methods? I once heard that we don't so
that changing these methods requires only 3-sided changes not 4-sided ones.

[Evan Stade, 2013-06-10 19:30:02]

gtk lgtm

https://codereview.chromium.org/16583005/diff/40001/chrome/browser/ui/gtk/bub...
File chrome/browser/ui/gtk/bubble/bubble_gtk.cc (right):

https://codereview.chromium.org/16583005/diff/40001/chrome/browser/ui/gtk/bub...
chrome/browser/ui/gtk/bubble/bubble_gtk.cc:562: if (!UpdateFrameStyle(false)) {
nit: no curlies

https://codereview.chromium.org/16583005/diff/40001/chrome/browser/ui/views/v...
File chrome/browser/ui/views/validation_message_bubble_view.cc (right):

https://codereview.chromium.org/16583005/diff/40001/chrome/browser/ui/views/v...
chrome/browser/ui/views/validation_message_bubble_view.cc:34: +
widget_host->GetView()->GetViewBounds().origin().OffsetFromOrigin());
nit: operators at EOL

[tkent, 2013-06-11 07:02:43]

https://codereview.chromium.org/16583005/diff/40001/chrome/browser/ui/cocoa/v...
File chrome/browser/ui/cocoa/validation_message_bubble_cocoa.mm (right):

https://codereview.chromium.org/16583005/diff/40001/chrome/browser/ui/cocoa/v...
chrome/browser/ui/cocoa/validation_message_bubble_cocoa.mm:144:
static_cast<BaseView*>(widget_host->GetView()->GetNativeView());
On 2013/06/10 14:51:40, Nico wrote:
> consider base::mac::ObjCCastStrict<> instead of static_cast<>

Done.

https://codereview.chromium.org/16583005/diff/40001/chrome/browser/ui/gtk/bub...
File chrome/browser/ui/gtk/bubble/bubble_gtk.cc (right):

https://codereview.chromium.org/16583005/diff/40001/chrome/browser/ui/gtk/bub...
chrome/browser/ui/gtk/bubble/bubble_gtk.cc:562: if (!UpdateFrameStyle(false)) {
On 2013/06/10 19:30:02, Evan Stade wrote:
> nit: no curlies

Done.

https://codereview.chromium.org/16583005/diff/40001/chrome/browser/ui/views/v...
File chrome/browser/ui/views/validation_message_bubble_view.cc (right):

https://codereview.chromium.org/16583005/diff/40001/chrome/browser/ui/views/v...
chrome/browser/ui/views/validation_message_bubble_view.cc:34: +
widget_host->GetView()->GetViewBounds().origin().OffsetFromOrigin());
On 2013/06/10 19:30:02, Evan Stade wrote:
> nit: operators at EOL

Done.

https://codereview.chromium.org/16583005/diff/40001/chrome/browser/ui/views/v...
chrome/browser/ui/views/validation_message_bubble_view.cc:67: const gfx::Rect&
anchor_in_screen = anchor_in_root_view
On 2013/06/10 14:48:28, sky wrote:
> const gfx::Rect& -> const gfx::Rect

Done.

https://codereview.chromium.org/16583005/diff/40001/chrome/renderer/validatio...
File chrome/renderer/validation_message_agent.h (right):

https://codereview.chromium.org/16583005/diff/40001/chrome/renderer/validatio...
chrome/renderer/validation_message_agent.h:30: // TOOD(tkent): add OVERRIDE
before landing this.
On 2013/06/10 14:51:40, Nico wrote:
> do we add OVERRIDE for overriding blink methods? I once heard that we don't so
> that changing these methods requires only 3-sided changes not 4-sided ones.

Yeah, OVERRIDE is optional in a case of implementing Blink API in Chromium
AFAIK.

[tkent, 2013-06-11 07:07:41]

tsepez, would you review chrome/common/validation_message_messages.h please?

[Tom Sepez, 2013-06-11 17:32:48]

lgtm

[commit-bot: I haz the power, 2013-06-11 21:51:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tkent@chromium.org/16583005/64001

[commit-bot: I haz the power, 2013-06-12 00:00:56]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-06-12 00:12:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tkent@chromium.org/16583005/64001

[commit-bot: I haz the power, 2013-06-12 04:21:18]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[tkent, 2013-06-12 04:30:06]

Committed patchset #3 manually as r205723 (presubmit successful).