Re-implement form validation message UI with native widgets. (Common and Mac)

Re-implement form validation message UI with native widgets. (Common and Mac)

The current form validation message UI with Shadow DOM has a lot of
problems and some of them are unresolvable if we continue to use
Shadow DOM. We should stop using Shadow DOM.

This patch implements a platform-indepedent part of
WebKit::WebValidationMessageClient, and the Mac part.

 - Introduce ValidationMessageAgent, which implements
   WebKit::WebValidationMessageClient and lives in a renderer process of the
   chrome module. It dispatches IPC messages to the browser process.
   ValidationMessageAgent is used only on Mac for now.

 - ValidationMessageFilter receives the IPC messages in the browser process, and
   instructs to show/hide validation message UI.

 - Add common interface to control validation message UI
   validation_message_bubble.h
 - Add skelton implementations for non-Mac and a real implementation for Mac
   validation_message_bubble_*.*

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

[jam, 2013-03-29 16:48:40]

the content plumbing seems unnecessary. i.e. you can have a separate WebKit
client interface, like WebSpellCheckClient or WebAutofillClient, which is
implemented by Chrome. It can even be part of WebAutofillClient.

[tkent, 2013-04-01 03:17:24]

On 2013/03/29 16:48:40, jam wrote:
> the content plumbing seems unnecessary. i.e. you can have a separate WebKit
> client interface, like WebSpellCheckClient or WebAutofillClient, which is
> implemented by Chrome. It can even be part of WebAutofillClient.

Thank you for the comment.
Unlike autofill and spell checker, a web browser must implement this feature to
support HTML5. Should we make a separated client interface in such case?

[tkent, 2013-04-08 21:18:08]

On 2013/04/01 03:17:24, tkent wrote:
> On 2013/03/29 16:48:40, jam wrote:
> > the content plumbing seems unnecessary. i.e. you can have a separate WebKit
> > client interface, like WebSpellCheckClient or WebAutofillClient, which is
> > implemented by Chrome. It can even be part of WebAutofillClient.
> 
> Thank you for the comment.
> Unlike autofill and spell checker, a web browser must implement this feature
to
> support HTML5. Should we make a separated client interface in such case?

What's the policy of introducing separated client interface? Should we keep
WebViewClient small as possible? Should we split existing WebViewClient
functions to very small clients? Should we introduce single client for
form-related functions?

[darin (slow to review), 2013-04-10 18:03:06]

I've generally just added sections to WebViewClient to keep the interface
count to a minimum.  However, it seems like "tear-offs" (i.e., optional
client interfaces) are useful when it enables us to avoid adding any code
to src/content/ for the related functionality.  For example, a tear-off
might enable us to put all of the consumer code in src/chrome/ so that
src/content/ can blissfully ignore the feature.  That seems like a good
thing.  Otherwise, if src/content/ needs to provide the implementation,
then often adding another section to WebViewClient is fine.


On Mon, Apr 8, 2013 at 2:18 PM, <tkent@chromium.org> wrote:

> On 2013/04/01 03:17:24, tkent wrote:
>
>> On 2013/03/29 16:48:40, jam wrote:
>> > the content plumbing seems unnecessary. i.e. you can have a separate
>> WebKit
>> > client interface, like WebSpellCheckClient or WebAutofillClient, which
>> is
>> > implemented by Chrome. It can even be part of WebAutofillClient.
>>
>
>  Thank you for the comment.
>> Unlike autofill and spell checker, a web browser must implement this
>> feature
>>
> to
>
>> support HTML5. Should we make a separated client interface in such case?
>>
>
> What's the policy of introducing separated client interface? Should we keep
> WebViewClient small as possible? Should we split existing WebViewClient
> functions to very small clients? Should we introduce single client for
> form-related functions?
>
>
>
https://codereview.chromium.**org/13160004/<https://codereview.chromium.org/1...
>

[jam, 2013-04-11 17:17:35]

On 2013/04/01 03:17:24, tkent wrote:
> On 2013/03/29 16:48:40, jam wrote:
> > the content plumbing seems unnecessary. i.e. you can have a separate WebKit
> > client interface, like WebSpellCheckClient or WebAutofillClient, which is
> > implemented by Chrome. It can even be part of WebAutofillClient.
> 
> Thank you for the comment.
> Unlike autofill and spell checker, a web browser must implement this feature
to
> support HTML5. Should we make a separated client interface in such case?

sorry for the delay, I just saw this. in the future please ping me if I don't
respond within a reasonable time.

the issue is where you want the implementation to be. if it'll be in chrome,
then it's best to avoid a content interface that just purely wraps a WebKit
interface (or a set of related methods on WebViewClient).

if this is needed for an html5 feature, I'm curious if it's possible to
implement this inside content? i.e. why did you implement this in the chrome
layer?

[tkent, 2013-04-12 04:22:27]

On 2013/04/11 17:17:35, jam wrote:
> the issue is where you want the implementation to be. if it'll be in chrome,
> then it's best to avoid a content interface that just purely wraps a WebKit
> interface (or a set of related methods on WebViewClient).

ok, I understand.

> if this is needed for an html5 feature, I'm curious if it's possible to
> implement this inside content? i.e. why did you implement this in the chrome
> layer?

It's a good question.  I wanted to implement this feature in the content layer
actually. But I couldn't do it because bubble-style windows for Cocoa and GTK
are not available in the content layer.
(chrome/browser/ui/cocoa/base_bubble_controller.*,
chrome/browser/ui/gtk/bubble/bubble_gtk.*)

My current opinion is that we should implement this in the chrome layer because
other browsers using the content layer might want to use different UI for
validation messages.

I'll change the Blink interface and update this patch later.

[tkent, 2013-04-22 21:34:09]

Hi everyone,

I uploaded new patch, which does not modify the content module at all.  Please
take a look at this.

[tkent, 2013-04-24 23:28:11]

thakis, ben, would you review changes in chrome/ please?
cevans, would you check new IPC messages please?

[Nico, 2013-04-25 22:57:49]

Sorry for the delay. This looks really good.

Can you add some test coverage for the new ui code?

https://codereview.chromium.org/13160004/diff/25001/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/base_bubble_controller.h (right):

https://codereview.chromium.org/13160004/diff/25001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/base_bubble_controller.h:40: bool shouldOpenKeyWindow_;
shouldOpenAsKeyWindow_

https://codereview.chromium.org/13160004/diff/25001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/base_bubble_controller.h:48: @property(nonatomic,
assign) bool shouldOpenKeyWindow;
likewise

https://codereview.chromium.org/13160004/diff/25001/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/base_bubble_controller.mm (right):

https://codereview.chromium.org/13160004/diff/25001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/base_bubble_controller.mm:49: shouldOpenKeyWindow_ =
true;
s/true/YES/, also move it one line up into the if

https://codereview.chromium.org/13160004/diff/25001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/base_bubble_controller.mm:92: shouldOpenKeyWindow_ =
true;
likewise

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

https://codereview.chromium.org/13160004/diff/25001/chrome/browser/ui/cocoa/v...
chrome/browser/ui/cocoa/validation_message_bubble_cocoa.mm:23: const CGFloat
kWindowPadding2x = 16;
Not used, remove

https://codereview.chromium.org/13160004/diff/25001/chrome/browser/ui/cocoa/v...
chrome/browser/ui/cocoa/validation_message_bubble_cocoa.mm:69:
scoped_nsobject<NSImageView> image_view([[NSImageView alloc] initWithFrame:
ObjC functions use camelCaseVariableNames

(throughout this function)

https://codereview.chromium.org/13160004/diff/25001/chrome/browser/ui/cocoa/v...
chrome/browser/ui/cocoa/validation_message_bubble_cocoa.mm:76: const CGFloat
text_x = [image_view frame].size.width + kIconTextMargin;
NSWidth([image_view frame])

https://codereview.chromium.org/13160004/diff/25001/chrome/browser/ui/gtk/val...
File chrome/browser/ui/gtk/validation_message_bubble_gtk.cc (right):

https://codereview.chromium.org/13160004/diff/25001/chrome/browser/ui/gtk/val...
chrome/browser/ui/gtk/validation_message_bubble_gtk.cc:14: // TODO(tkent):
Implement this and enable it.
Add a link to some tracking bug for this (also in the android and views file)

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

https://codereview.chromium.org/13160004/diff/25001/chrome/browser/ui/validat...
chrome/browser/ui/validation_message_bubble.h:23: // Open a tooltip-like window
to show the specified messages.  The windows
s/windows/window

https://codereview.chromium.org/13160004/diff/25001/chrome/browser/validation...
File chrome/browser/validation_message_message_filter.h (right):

https://codereview.chromium.org/13160004/diff/25001/chrome/browser/validation...
chrome/browser/validation_message_message_filter.h:18: class
ValidationMessageMessageFilter : public content::BrowserMessageFilter {
Class comment

[tkent, 2013-04-29 22:24:19]

https://codereview.chromium.org/13160004/diff/25001/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/base_bubble_controller.h (right):

https://codereview.chromium.org/13160004/diff/25001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/base_bubble_controller.h:40: bool shouldOpenKeyWindow_;
On 2013/04/25 22:57:49, Nico wrote:
> shouldOpenAsKeyWindow_

Done.

https://codereview.chromium.org/13160004/diff/25001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/base_bubble_controller.h:48: @property(nonatomic,
assign) bool shouldOpenKeyWindow;
On 2013/04/25 22:57:49, Nico wrote:
> likewise

Done.

https://codereview.chromium.org/13160004/diff/25001/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/base_bubble_controller.mm (right):

https://codereview.chromium.org/13160004/diff/25001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/base_bubble_controller.mm:49: shouldOpenKeyWindow_ =
true;
On 2013/04/25 22:57:49, Nico wrote:
> s/true/YES/, also move it one line up into the if

Done.

https://codereview.chromium.org/13160004/diff/25001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/base_bubble_controller.mm:92: shouldOpenKeyWindow_ =
true;
On 2013/04/25 22:57:49, Nico wrote:
> likewise

Done.

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

https://codereview.chromium.org/13160004/diff/25001/chrome/browser/ui/cocoa/v...
chrome/browser/ui/cocoa/validation_message_bubble_cocoa.mm:23: const CGFloat
kWindowPadding2x = 16;
On 2013/04/25 22:57:49, Nico wrote:
> Not used, remove

Done.

https://codereview.chromium.org/13160004/diff/25001/chrome/browser/ui/cocoa/v...
chrome/browser/ui/cocoa/validation_message_bubble_cocoa.mm:69:
scoped_nsobject<NSImageView> image_view([[NSImageView alloc] initWithFrame:
On 2013/04/25 22:57:49, Nico wrote:
> ObjC functions use camelCaseVariableNames
> 
> (throughout this function)

Renamed all of variables.

https://codereview.chromium.org/13160004/diff/25001/chrome/browser/ui/cocoa/v...
chrome/browser/ui/cocoa/validation_message_bubble_cocoa.mm:76: const CGFloat
text_x = [image_view frame].size.width + kIconTextMargin;
On 2013/04/25 22:57:49, Nico wrote:
> NSWidth([image_view frame])

Replaced all of .size.width and .size.height with NSWidth and NSHeight.

https://codereview.chromium.org/13160004/diff/25001/chrome/browser/ui/gtk/val...
File chrome/browser/ui/gtk/validation_message_bubble_gtk.cc (right):

https://codereview.chromium.org/13160004/diff/25001/chrome/browser/ui/gtk/val...
chrome/browser/ui/gtk/validation_message_bubble_gtk.cc:14: // TODO(tkent):
Implement this and enable it.
On 2013/04/25 22:57:49, Nico wrote:
> Add a link to some tracking bug for this (also in the android and views file)

Done.

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

https://codereview.chromium.org/13160004/diff/25001/chrome/browser/ui/validat...
chrome/browser/ui/validation_message_bubble.h:23: // Open a tooltip-like window
to show the specified messages.  The windows
On 2013/04/25 22:57:49, Nico wrote:
> s/windows/window

Done.

https://codereview.chromium.org/13160004/diff/25001/chrome/browser/validation...
File chrome/browser/validation_message_message_filter.h (right):

https://codereview.chromium.org/13160004/diff/25001/chrome/browser/validation...
chrome/browser/validation_message_message_filter.h:18: class
ValidationMessageMessageFilter : public content::BrowserMessageFilter {
On 2013/04/25 22:57:49, Nico wrote:
> Class comment

Done.

[Nico, 2013-04-29 22:35:48]

lgtm!

https://codereview.chromium.org/13160004/diff/44001/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/base_bubble_controller.mm (right):

https://codereview.chromium.org/13160004/diff/44001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/base_bubble_controller.mm:40: anchor_ = anchoredAt;
Nit: All other members are set up here, maybe move shouldOpenAsKeyWindow_ to
here

https://codereview.chromium.org/13160004/diff/44001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/base_bubble_controller.mm:91: shouldOpenAsKeyWindow_ =
YES;
(likewise; probably at least want to do this before -awakeFromNib)

https://codereview.chromium.org/13160004/diff/44001/chrome/browser/ui/cocoa/v...
File chrome/browser/ui/cocoa/validation_message_bubble_controller.h (right):

https://codereview.chromium.org/13160004/diff/44001/chrome/browser/ui/cocoa/v...
chrome/browser/ui/cocoa/validation_message_bubble_controller.h:11: @interface
ValidationMessageBubbleController : BaseBubbleController {
No need for the {}. Add a class comemnt.

https://codereview.chromium.org/13160004/diff/44001/chrome/browser/ui/cocoa/v...
chrome/browser/ui/cocoa/validation_message_bubble_controller.h:23: @end  //
interface ValidationMessageBubbleController
nit: We don't usually have comments on "@end" lines I think (?)

[tkent, 2013-04-29 22:37:29]

thakis, would you take another look please?
cevans, would you check new IPC messages please?

> Can you add some test coverage for the new ui code?

Yeah, I have added validation_message_bubble_controller_unittest.mm.

[tkent, 2013-04-29 22:40:02]

On 2013/04/29 22:37:29, tkent wrote:
> thakis, would you take another look please?

Oh, Nico already reviewed.  Too quick :)

[tkent, 2013-04-29 23:08:01]

https://codereview.chromium.org/13160004/diff/44001/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/base_bubble_controller.mm (right):

https://codereview.chromium.org/13160004/diff/44001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/base_bubble_controller.mm:40: anchor_ = anchoredAt;
On 2013/04/29 22:35:48, Nico wrote:
> Nit: All other members are set up here, maybe move shouldOpenAsKeyWindow_ to
> here

Done.

https://codereview.chromium.org/13160004/diff/44001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/base_bubble_controller.mm:91: shouldOpenAsKeyWindow_ =
YES;
On 2013/04/29 22:35:48, Nico wrote:
> (likewise; probably at least want to do this before -awakeFromNib)

Done.

https://codereview.chromium.org/13160004/diff/44001/chrome/browser/ui/cocoa/v...
File chrome/browser/ui/cocoa/validation_message_bubble_controller.h (right):

https://codereview.chromium.org/13160004/diff/44001/chrome/browser/ui/cocoa/v...
chrome/browser/ui/cocoa/validation_message_bubble_controller.h:11: @interface
ValidationMessageBubbleController : BaseBubbleController {
On 2013/04/29 22:35:48, Nico wrote:
> No need for the {}. Add a class comemnt.

Done.

https://codereview.chromium.org/13160004/diff/44001/chrome/browser/ui/cocoa/v...
chrome/browser/ui/cocoa/validation_message_bubble_controller.h:23: @end  //
interface ValidationMessageBubbleController
Removed

[tkent, 2013-04-29 23:10:02]

Adding more reviewers for ipc_message_start.h.

[Tom Sepez, 2013-04-29 23:14:36]

Messages LGTM.

[commit-bot: I haz the power, 2013-04-29 23:28:55]

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

[commit-bot: I haz the power, 2013-04-30 02:30:28]

Change committed as 197226