Re-implement form validation message UI with native widgets (Views)

Re-implement form validation message UI with native widgets (Views)

This CL adds Views implementation of ValidationMessageBubble, and
enable ValidationMessageAgent for platforms with TOOLKIT_VIEWS.

Also, we can remove a #if in chrome_content_browser_client.cc.
Messages handled by ValidationMessageFilter are sent only in platforms
which matches to #if in ValidationMessageAgent.

BUG=90252, 90958, 92816, 104829, 106621, 113352, 115451, 125330, 143356, 155448, 162440, 166981, 231170, 235719
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=197786

[tkent, 2013-04-30 09:24:14]

thakis@, sky@, would you review this please?

[tkent, 2013-04-30 09:25:31]

This is a Views implementation for
https://chromiumcodereview.appspot.com/13160004/

[sky, 2013-04-30 13:53:20]

https://codereview.chromium.org/14627004/diff/7001/chrome/browser/chrome_cont...
File chrome/browser/chrome_content_browser_client.cc (left):

https://codereview.chromium.org/14627004/diff/7001/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.cc:730: #if defined(OS_MACOSX)
I don't see a gtk implementation. How is the linux side working?

https://codereview.chromium.org/14627004/diff/7001/chrome/browser/ui/views/va...
File chrome/browser/ui/views/validation_message_bubble_delegate.h (right):

https://codereview.chromium.org/14627004/diff/7001/chrome/browser/ui/views/va...
chrome/browser/ui/views/validation_message_bubble_delegate.h:17: virtual
gfx::Size GetPreferredSize() OVERRIDE;
nit: newline between 16/17 and prefix with:
// BubbleDelegateView overrides:

https://codereview.chromium.org/14627004/diff/7001/chrome/browser/ui/views/va...
chrome/browser/ui/views/validation_message_bubble_delegate.h:19: virtual
~ValidationMessageBubbleDelegate();
This should be right after constructor.

https://codereview.chromium.org/14627004/diff/7001/chrome/browser/ui/views/va...
File chrome/browser/ui/views/validation_message_bubble_delegate_unittest.cc
(right):

https://codereview.chromium.org/14627004/diff/7001/chrome/browser/ui/views/va...
chrome/browser/ui/views/validation_message_bubble_delegate_unittest.cc:5:
#include "chrome/browser/ui/views/validation_message_bubble_delegate.h"
nit: newline between 5/6.

https://codereview.chromium.org/14627004/diff/7001/chrome/browser/ui/views/va...
chrome/browser/ui/views/validation_message_bubble_delegate_unittest.cc:11:
gfx::Size GetSizeForMessages(const char* main_text, const char* sub_text) {
Make these take const std::string&.

https://codereview.chromium.org/14627004/diff/7001/chrome/browser/ui/views/va...
chrome/browser/ui/views/validation_message_bubble_delegate_unittest.cc:12:
scoped_ptr<ValidationMessageBubbleDelegate> delegate(
Why the scoped_ptr here? Why not create on the stack?

https://codereview.chromium.org/14627004/diff/7001/chrome/browser/ui/views/va...
chrome/browser/ui/views/validation_message_bubble_delegate_unittest.cc:19:
gfx::Size shortMainEmptySubSize = GetSizeForMessages("foo", "");
short_main_empty_sub_size, in fact you have the wrong style nearly every where
here.

https://codereview.chromium.org/14627004/diff/7001/chrome/browser/ui/views/va...
chrome/browser/ui/views/validation_message_bubble_delegate_unittest.cc:20:
EXPECT_GT(shortMainEmptySubSize.width(), 40);
Where do these numbers come from?

https://codereview.chromium.org/14627004/diff/7001/chrome/browser/ui/views/va...
chrome/browser/ui/views/validation_message_bubble_delegate_unittest.cc:40: 
No new empty line here.

https://codereview.chromium.org/14627004/diff/7001/chrome/browser/ui/views/va...
File chrome/browser/ui/views/validation_message_bubble_view.cc (right):

https://codereview.chromium.org/14627004/diff/7001/chrome/browser/ui/views/va...
chrome/browser/ui/views/validation_message_bubble_view.cc:5: #include
"chrome/browser/ui/validation_message_bubble.h"
newline between 5/6.

https://codereview.chromium.org/14627004/diff/7001/chrome/browser/ui/views/va...
chrome/browser/ui/views/validation_message_bubble_view.cc:13: const int
kWindowMinWidth = 64;
These belong in a namespace or static.

https://codereview.chromium.org/14627004/diff/7001/chrome/browser/ui/views/va...
chrome/browser/ui/views/validation_message_bubble_view.cc:19:
ValidationMessageBubbleDelegate::ValidationMessageBubbleDelegate(
Why is the ValidateMessageBubbleDelegate implementation in this file instead of
its own?

[tkent, 2013-05-01 03:15:02]

https://codereview.chromium.org/14627004/diff/7001/chrome/browser/chrome_cont...
File chrome/browser/chrome_content_browser_client.cc (left):

https://codereview.chromium.org/14627004/diff/7001/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.cc:730: #if defined(OS_MACOSX)
On 2013/04/30 13:53:21, sky wrote:
> I don't see a gtk implementation. How is the linux side working?

GTK and Android implementations are not checked in and not in this CL.  I'm
going to post separated CLs.
Enabling ValidationMessageMessagesFilter here for all platforms has no problem
because we have a guard in validation_message_agent.cc and IPC messages are
never sent on GTK and Android.

https://codereview.chromium.org/14627004/diff/7001/chrome/browser/ui/views/va...
File chrome/browser/ui/views/validation_message_bubble_delegate.h (right):

https://codereview.chromium.org/14627004/diff/7001/chrome/browser/ui/views/va...
chrome/browser/ui/views/validation_message_bubble_delegate.h:17: virtual
gfx::Size GetPreferredSize() OVERRIDE;
On 2013/04/30 13:53:21, sky wrote:
> nit: newline between 16/17 and prefix with:
> // BubbleDelegateView overrides:

Done.

https://codereview.chromium.org/14627004/diff/7001/chrome/browser/ui/views/va...
chrome/browser/ui/views/validation_message_bubble_delegate.h:19: virtual
~ValidationMessageBubbleDelegate();
On 2013/04/30 13:53:21, sky wrote:
> This should be right after constructor.

Done.

https://codereview.chromium.org/14627004/diff/7001/chrome/browser/ui/views/va...
File chrome/browser/ui/views/validation_message_bubble_delegate_unittest.cc
(right):

https://codereview.chromium.org/14627004/diff/7001/chrome/browser/ui/views/va...
chrome/browser/ui/views/validation_message_bubble_delegate_unittest.cc:5:
#include "chrome/browser/ui/views/validation_message_bubble_delegate.h"
On 2013/04/30 13:53:21, sky wrote:
> nit: newline between 5/6.

Done.

https://codereview.chromium.org/14627004/diff/7001/chrome/browser/ui/views/va...
chrome/browser/ui/views/validation_message_bubble_delegate_unittest.cc:11:
gfx::Size GetSizeForMessages(const char* main_text, const char* sub_text) {
On 2013/04/30 13:53:21, sky wrote:
> Make these take const std::string&.

Done.

https://codereview.chromium.org/14627004/diff/7001/chrome/browser/ui/views/va...
chrome/browser/ui/views/validation_message_bubble_delegate_unittest.cc:12:
scoped_ptr<ValidationMessageBubbleDelegate> delegate(
On 2013/04/30 13:53:21, sky wrote:
> Why the scoped_ptr here? Why not create on the stack?

No reason to use scoped_ptr. Changed to an object on the stack.

https://codereview.chromium.org/14627004/diff/7001/chrome/browser/ui/views/va...
chrome/browser/ui/views/validation_message_bubble_delegate_unittest.cc:19:
gfx::Size shortMainEmptySubSize = GetSizeForMessages("foo", "");
On 2013/04/30 13:53:21, sky wrote:
> short_main_empty_sub_size, in fact you have the wrong style nearly every where
> here.

Updated all local variables.

https://codereview.chromium.org/14627004/diff/7001/chrome/browser/ui/views/va...
chrome/browser/ui/views/validation_message_bubble_delegate_unittest.cc:20:
EXPECT_GT(shortMainEmptySubSize.width(), 40);
On 2013/04/30 13:53:21, sky wrote:
> Where do these numbers come from?

It is a value smaller than kWindowMinWidth.
Using a smaller value is not important. So I changed it to 64, which is same as
kWindowMinWidth.

https://codereview.chromium.org/14627004/diff/7001/chrome/browser/ui/views/va...
chrome/browser/ui/views/validation_message_bubble_delegate_unittest.cc:40: 
On 2013/04/30 13:53:21, sky wrote:
> No new empty line here.

Done.

https://codereview.chromium.org/14627004/diff/7001/chrome/browser/ui/views/va...
File chrome/browser/ui/views/validation_message_bubble_view.cc (right):

https://codereview.chromium.org/14627004/diff/7001/chrome/browser/ui/views/va...
chrome/browser/ui/views/validation_message_bubble_view.cc:5: #include
"chrome/browser/ui/validation_message_bubble.h"
On 2013/04/30 13:53:21, sky wrote:
> newline between 5/6.

Done.

https://codereview.chromium.org/14627004/diff/7001/chrome/browser/ui/views/va...
chrome/browser/ui/views/validation_message_bubble_view.cc:13: const int
kWindowMinWidth = 64;
On 2013/04/30 13:53:21, sky wrote:
> These belong in a namespace or static.

Made them static.
BTW, why is it necessary? "const" implies file scope in C++.

https://codereview.chromium.org/14627004/diff/7001/chrome/browser/ui/views/va...
chrome/browser/ui/views/validation_message_bubble_view.cc:19:
ValidationMessageBubbleDelegate::ValidationMessageBubbleDelegate(
On 2013/04/30 13:53:21, sky wrote:
> Why is the ValidateMessageBubbleDelegate implementation in this file instead
of
> its own?

Moved ValidationMessageBubbleDelegate functions to
validation_message_bubble_delegate.cc.

Background:
I put it in this file because only ValidationMessageBubbleImpl used
ValidationMesageBubbleDelegate. Then, I moved the class definition to its own
header in order to add unit test.

[tkent, 2013-05-01 03:18:14]

I have updated the patch. Would you take another look please?

[sky, 2013-05-01 04:35:53]

https://codereview.chromium.org/14627004/diff/7001/chrome/browser/chrome_cont...
File chrome/browser/chrome_content_browser_client.cc (left):

https://codereview.chromium.org/14627004/diff/7001/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.cc:730: #if defined(OS_MACOSX)
On 2013/05/01 03:15:02, tkent wrote:
> On 2013/04/30 13:53:21, sky wrote:
> > I don't see a gtk implementation. How is the linux side working?
> 
> GTK and Android implementations are not checked in and not in this CL.  I'm
> going to post separated CLs.
> Enabling ValidationMessageMessagesFilter here for all platforms has no problem
> because we have a guard in validation_message_agent.cc and IPC messages are
> never sent on GTK and Android.
> 

Don't you still need an ifdef then?

[tkent, 2013-05-01 04:50:12]

https://codereview.chromium.org/14627004/diff/7001/chrome/browser/chrome_cont...
File chrome/browser/chrome_content_browser_client.cc (left):

https://codereview.chromium.org/14627004/diff/7001/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.cc:730: #if defined(OS_MACOSX)
It's not needed.
We may keep the ifdef to avoid to register unnecessary ValidationMessageFilter
in GTK and Android.
Or, we may remove the ifdef to reduce patch sizes for incoming GTK and Android
patches. We can remove the ifdef because the messages are never sent in GTK and
Android.

[sky, 2013-05-01 14:05:05]

https://codereview.chromium.org/14627004/diff/7001/chrome/browser/ui/views/va...
File chrome/browser/ui/views/validation_message_bubble_delegate_unittest.cc
(right):

https://codereview.chromium.org/14627004/diff/7001/chrome/browser/ui/views/va...
chrome/browser/ui/views/validation_message_bubble_delegate_unittest.cc:20:
EXPECT_GT(shortMainEmptySubSize.width(), 40);
On 2013/05/01 03:15:02, tkent wrote:
> On 2013/04/30 13:53:21, sky wrote:
> > Where do these numbers come from?
> 
> It is a value smaller than kWindowMinWidth.
> Using a smaller value is not important. So I changed it to 64, which is same
as
> kWindowMinWidth.

Duplicating the values of constants defeats the value of the constant. Use the
real constant here.

https://codereview.chromium.org/14627004/diff/17001/chrome/browser/ui/views/v...
File chrome/browser/ui/views/validation_message_bubble_delegate.cc (right):

https://codereview.chromium.org/14627004/diff/17001/chrome/browser/ui/views/v...
chrome/browser/ui/views/validation_message_bubble_delegate.cc:83:
ValidationMessageBubbleDelegate::~ValidationMessageBubbleDelegate() {}
Make order match header.

https://codereview.chromium.org/14627004/diff/17001/chrome/browser/ui/views/v...
File chrome/browser/ui/views/validation_message_bubble_delegate_unittest.cc
(right):

https://codereview.chromium.org/14627004/diff/17001/chrome/browser/ui/views/v...
chrome/browser/ui/views/validation_message_bubble_delegate_unittest.cc:21:
EXPECT_GE(short_main_empty_sub_size.width(), 64);
expect/assert have the format expected, actual.

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

https://codereview.chromium.org/14627004/diff/17001/chrome/browser/ui/views/v...
chrome/browser/ui/views/validation_message_bubble_view.cc:20:
delegate_->GetWidget()->Hide();
How do you know delegate_ is valid here?

https://codereview.chromium.org/14627004/diff/17001/chrome/browser/ui/views/v...
chrome/browser/ui/views/validation_message_bubble_view.cc:33: // The delegate_
will kill itself in ValidationMessageBubbleDelegate::
nit: wrap ValidationMessageBubbleDelegate::DeleteDelegate() to a newline just
like you would in code.

[tkent, 2013-05-01 21:42:30]

https://codereview.chromium.org/14627004/diff/7001/chrome/browser/ui/views/va...
File chrome/browser/ui/views/validation_message_bubble_delegate_unittest.cc
(right):

https://codereview.chromium.org/14627004/diff/7001/chrome/browser/ui/views/va...
chrome/browser/ui/views/validation_message_bubble_delegate_unittest.cc:20:
EXPECT_GT(shortMainEmptySubSize.width(), 40);
On 2013/05/01 14:05:05, sky wrote:
> On 2013/05/01 03:15:02, tkent wrote:
> > On 2013/04/30 13:53:21, sky wrote:
> > > Where do these numbers come from?
> > 
> > It is a value smaller than kWindowMinWidth.
> > Using a smaller value is not important. So I changed it to 64, which is same
> as
> > kWindowMinWidth.
> 
> Duplicating the values of constants defeats the value of the constant. Use the
> real constant here.

Done.

https://codereview.chromium.org/14627004/diff/17001/chrome/browser/ui/views/v...
File chrome/browser/ui/views/validation_message_bubble_delegate.cc (right):

https://codereview.chromium.org/14627004/diff/17001/chrome/browser/ui/views/v...
chrome/browser/ui/views/validation_message_bubble_delegate.cc:83:
ValidationMessageBubbleDelegate::~ValidationMessageBubbleDelegate() {}
On 2013/05/01 14:05:05, sky wrote:
> Make order match header.

Done.

https://codereview.chromium.org/14627004/diff/17001/chrome/browser/ui/views/v...
File chrome/browser/ui/views/validation_message_bubble_delegate_unittest.cc
(right):

https://codereview.chromium.org/14627004/diff/17001/chrome/browser/ui/views/v...
chrome/browser/ui/views/validation_message_bubble_delegate_unittest.cc:21:
EXPECT_GE(short_main_empty_sub_size.width(), 64);
On 2013/05/01 14:05:05, sky wrote:
> expect/assert have the format expected, actual.

Done.

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

https://codereview.chromium.org/14627004/diff/17001/chrome/browser/ui/views/v...
chrome/browser/ui/views/validation_message_bubble_view.cc:20:
delegate_->GetWidget()->Hide();
On 2013/05/01 14:05:05, sky wrote:
> How do you know delegate_ is valid here?

I thought it's always valid.  It might be a wrong assumption.
I changed the code so that we clear delegate_ whenever the widget is closing.

https://codereview.chromium.org/14627004/diff/17001/chrome/browser/ui/views/v...
chrome/browser/ui/views/validation_message_bubble_view.cc:33: // The delegate_
will kill itself in ValidationMessageBubbleDelegate::
On 2013/05/01 14:05:05, sky wrote:
> nit: wrap ValidationMessageBubbleDelegate::DeleteDelegate() to a newline just
> like you would in code.

Done.

[tkent, 2013-05-01 21:44:00]

I have updated the patch.  Please take another look.

[sky, 2013-05-01 23:14:48]

LGTM with the following fixes.

https://codereview.chromium.org/14627004/diff/26001/chrome/browser/ui/views/v...
File chrome/browser/ui/views/validation_message_bubble_delegate.cc (right):

https://codereview.chromium.org/14627004/diff/26001/chrome/browser/ui/views/v...
chrome/browser/ui/views/validation_message_bubble_delegate.cc:13: const int
ValidationMessageBubbleDelegate::kWindowMinWidth = 64;
Prefix these with // static on the previous line.

https://codereview.chromium.org/14627004/diff/26001/chrome/browser/ui/views/v...
chrome/browser/ui/views/validation_message_bubble_delegate.cc:24: :
observer_(observer) {
indent 2 more, and initialize width_/height_.

https://codereview.chromium.org/14627004/diff/26001/chrome/browser/ui/views/v...
File chrome/browser/ui/views/validation_message_bubble_delegate.h (right):

https://codereview.chromium.org/14627004/diff/26001/chrome/browser/ui/views/v...
chrome/browser/ui/views/validation_message_bubble_delegate.h:18: };
Add virtual protected destructor.

https://codereview.chromium.org/14627004/diff/26001/chrome/browser/ui/views/v...
chrome/browser/ui/views/validation_message_bubble_delegate.h:33: static const
int kWindowMinWidth;
constants before constructor.

[tkent, 2013-05-01 23:42:16]

Thank you!

https://codereview.chromium.org/14627004/diff/26001/chrome/browser/ui/views/v...
File chrome/browser/ui/views/validation_message_bubble_delegate.cc (right):

https://codereview.chromium.org/14627004/diff/26001/chrome/browser/ui/views/v...
chrome/browser/ui/views/validation_message_bubble_delegate.cc:13: const int
ValidationMessageBubbleDelegate::kWindowMinWidth = 64;
On 2013/05/01 23:14:49, sky wrote:
> Prefix these with // static on the previous line.

Done.

https://codereview.chromium.org/14627004/diff/26001/chrome/browser/ui/views/v...
chrome/browser/ui/views/validation_message_bubble_delegate.cc:24: :
observer_(observer) {
On 2013/05/01 23:14:49, sky wrote:
> indent 2 more, and initialize width_/height_.

Done.

https://codereview.chromium.org/14627004/diff/26001/chrome/browser/ui/views/v...
File chrome/browser/ui/views/validation_message_bubble_delegate.h (right):

https://codereview.chromium.org/14627004/diff/26001/chrome/browser/ui/views/v...
chrome/browser/ui/views/validation_message_bubble_delegate.h:18: };
On 2013/05/01 23:14:49, sky wrote:
> Add virtual protected destructor.

Done.

https://codereview.chromium.org/14627004/diff/26001/chrome/browser/ui/views/v...
chrome/browser/ui/views/validation_message_bubble_delegate.h:33: static const
int kWindowMinWidth;
On 2013/05/01 23:14:49, sky wrote:
> constants before constructor.

Done.

[commit-bot: I haz the power, 2013-05-01 23:43:01]

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

[commit-bot: I haz the power, 2013-05-02 02:17:44]

Change committed as 197786

[Nico, 2013-05-02 16:52:29]

No need to change anything for the comment below, it's just fyi.

https://chromiumcodereview.appspot.com/14627004/diff/30001/chrome/browser/ui/...
File chrome/browser/ui/views/validation_message_bubble_delegate.cc (right):

https://chromiumcodereview.appspot.com/14627004/diff/30001/chrome/browser/ui/...
chrome/browser/ui/views/validation_message_bubble_delegate.cc:17: static const
int kPadding = 0;
fyi: const has implicit internal linkage, so there's no need to explicitly write
"static" before it (or put it in an unnamed namespace).