Convert ValidationMessageBubbleView to BubbleDialogDelegate

Convert ValidationMessageBubbleView to BubbleDialogDelegate

- merge with ValidationMessageBubbleDelegate (which is the actual view)
- make bubble self-owned, like almost all bubbles (makes shutdown destruction less tricky)

BUG=585312
Committed: https://crrev.com/53f17391c1e7884791dc02b14cbba2e6dd65ff91
Cr-Commit-Position: refs/heads/master@{#387845}

[Evan Stade, 2016-04-12 19:04:30]

Description was changed from

==========
WIP - ValidationMessageBubbleView

- merge with ValidationMessageBubbleDelegate (which is the actual view)
- improve ownership model
- convert to BubbleDialogDelegate

BUG=585312
==========

to

==========
Convert ValidationMessageBubbleView to BubbleDialogDelegate

- merge with ValidationMessageBubbleDelegate (which is the actual view)
- make bubble self-owned, like almost all bubbles (makes shutdown destruction
less tricky)

BUG=585312
==========

[Evan Stade, 2016-04-12 19:04:30]

estade@chromium.org changed reviewers:
+ groby@chromium.org

[Evan Stade, 2016-04-12 19:04:42]

+groby can you have a look at the cocoa side of this

[Evan Stade, 2016-04-12 19:04:48]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-12 19:05:13]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1880853002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1880853002/60001

[commit-bot: I haz the power, 2016-04-12 19:13:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-12 19:13:26]

Dry run: Try jobs failed on following builders:
  cast_shell_android on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[groby-ooo-7-16, 2016-04-13 01:42:32]

https://codereview.chromium.org/1880853002/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/validation_message_bubble_cocoa.h (right):

https://codereview.chromium.org/1880853002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/validation_message_bubble_cocoa.h:21: public
base::SupportsWeakPtr<ValidationMessageBubbleCocoa> {
You're aware of the lifetime issues (WeakPtr is only invalid after full dtor
including member vars has run), I assume? Would WeakPtrFactory make more sense
here?

https://codereview.chromium.org/1880853002/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/validation_message_bubble_cocoa.mm (right):

https://codereview.chromium.org/1880853002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/validation_message_bubble_cocoa.mm:180: delete this;
I'm not a fan of self-deletion. Do we have to?

[Evan Stade, 2016-04-13 15:39:19]

https://codereview.chromium.org/1880853002/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/validation_message_bubble_cocoa.h (right):

https://codereview.chromium.org/1880853002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/validation_message_bubble_cocoa.h:21: public
base::SupportsWeakPtr<ValidationMessageBubbleCocoa> {
On 2016/04/13 01:42:31, groby wrote:
> You're aware of the lifetime issues (WeakPtr is only invalid after full dtor
> including member vars has run), I assume? Would WeakPtrFactory make more sense
> here?

supports is fine for our purposes, I think.

https://codereview.chromium.org/1880853002/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/validation_message_bubble_cocoa.mm (right):

https://codereview.chromium.org/1880853002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/validation_message_bubble_cocoa.mm:180: delete this;
On 2016/04/13 01:42:31, groby wrote:
> I'm not a fan of self-deletion. Do we have to?

I had to make it a WeakPtr (bubble is self-owning) to make ownership simpler and
more orthodox on Views. In general bubbles usually own themselves.
ConfirmBubbleController on cocoa is an example (if I'm understanding it
correctly). Should this be [self autorelease]; instead of delete this?

[groby-ooo-7-16, 2016-04-13 21:05:56]

c/b/ui/cocoa LGTM

https://codereview.chromium.org/1880853002/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/validation_message_bubble_cocoa.mm (right):

https://codereview.chromium.org/1880853002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/validation_message_bubble_cocoa.mm:180: delete this;
On 2016/04/13 15:39:19, Evan Stade wrote:
> On 2016/04/13 01:42:31, groby wrote:
> > I'm not a fan of self-deletion. Do we have to?
> 
> I had to make it a WeakPtr (bubble is self-owning) to make ownership simpler
and
> more orthodox on Views. In general bubbles usually own themselves.
> ConfirmBubbleController on cocoa is an example (if I'm understanding it
> correctly). Should this be [self autorelease]; instead of delete this?

No, [self autorelease] is an Objective C thing. I just don't like delete this,
but if it's consistent, fine...

[Evan Stade, 2016-04-13 22:25:02]

estade@chromium.org changed reviewers:
+ msw@chromium.org

[Evan Stade, 2016-04-13 22:25:02]

+msw for views review
+groby can you also verify mac still works by going to [1] and typing something
into the "banana or cherry?" textfield?

[1]
https://developer.mozilla.org/en-US/docs/Web/Guide/HTML/Forms/Data_form_valid...

[Evan Stade, 2016-04-13 22:25:12]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-13 22:25:46]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1880853002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1880853002/80001

[commit-bot: I haz the power, 2016-04-13 22:53:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-13 22:53:55]

Dry run: Try jobs failed on following builders:
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[groby-ooo-7-16, 2016-04-14 00:37:54]

Seems Mac is still working - screenshot: http://imgur.com/SGuWHuT

https://codereview.chromium.org/1880853002/diff/80001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/validation_message_bubble_cocoa.mm (right):

https://codereview.chromium.org/1880853002/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/validation_message_bubble_cocoa.mm:178:
ValidationMessageBubbleCocoa::CloseValidationMessage() {
Ooops. Missed this. Please add "void" :)

[msw, 2016-04-14 01:51:38]

lgtm with nits and qs

https://codereview.chromium.org/1880853002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/validation_message_bubble_view.cc (right):

https://codereview.chromium.org/1880853002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/validation_message_bubble_view.cc:20: static const int
kPadding = 0;
nit: Remove this and inline 0 as needed?

https://codereview.chromium.org/1880853002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/validation_message_bubble_view.cc:43:
icon->SetBounds(kPadding, kPadding, size.width(), size.height());
Can we use a LayoutManager here?

https://codereview.chromium.org/1880853002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/validation_message_bubble_view.cc:56: views::Label*
sub_label = NULL;
nit: nullptr

https://codereview.chromium.org/1880853002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/validation_message_bubble_view.h (right):

https://codereview.chromium.org/1880853002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/validation_message_bubble_view.h:23: static const int
kWindowMinWidth;
Can these be moved to the cc file if you're removing the test file?

https://codereview.chromium.org/1880853002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/validation_message_bubble_view.h:43: int width_;
nit: gfx::Size?

https://codereview.chromium.org/1880853002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/validation_message_bubble_view.h:45:
DISALLOW_COPY_AND_ASSIGN(ValidationMessageBubbleView);
nit: blank line above?

[Evan Stade, 2016-04-18 01:16:44]

https://codereview.chromium.org/1880853002/diff/80001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/validation_message_bubble_cocoa.mm (right):

https://codereview.chromium.org/1880853002/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/validation_message_bubble_cocoa.mm:178:
ValidationMessageBubbleCocoa::CloseValidationMessage() {
On 2016/04/14 00:37:53, groby wrote:
> Ooops. Missed this. Please add "void" :)

Done. Thanks.

https://codereview.chromium.org/1880853002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/validation_message_bubble_view.cc (right):

https://codereview.chromium.org/1880853002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/validation_message_bubble_view.cc:20: static const int
kPadding = 0;
On 2016/04/14 01:51:38, msw wrote:
> nit: Remove this and inline 0 as needed?

Done.

https://codereview.chromium.org/1880853002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/validation_message_bubble_view.cc:43:
icon->SetBounds(kPadding, kPadding, size.width(), size.height());
On 2016/04/14 01:51:38, msw wrote:
> Can we use a LayoutManager here?

I'd rather just not touch it (this is just a refactor)

https://codereview.chromium.org/1880853002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/validation_message_bubble_view.cc:56: views::Label*
sub_label = NULL;
On 2016/04/14 01:51:38, msw wrote:
> nit: nullptr

Done.

https://codereview.chromium.org/1880853002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/validation_message_bubble_view.h (right):

https://codereview.chromium.org/1880853002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/validation_message_bubble_view.h:23: static const int
kWindowMinWidth;
On 2016/04/14 01:51:38, msw wrote:
> Can these be moved to the cc file if you're removing the test file?

Done.

https://codereview.chromium.org/1880853002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/validation_message_bubble_view.h:43: int width_;
On 2016/04/14 01:51:38, msw wrote:
> nit: gfx::Size?

Done.

https://codereview.chromium.org/1880853002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/validation_message_bubble_view.h:45:
DISALLOW_COPY_AND_ASSIGN(ValidationMessageBubbleView);
On 2016/04/14 01:51:38, msw wrote:
> nit: blank line above?

Done.

[Evan Stade, 2016-04-18 01:28:15]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2016-04-18 01:28:16]

The patchset sent to the CQ was uploaded after l-g-t-m from groby@chromium.org,
msw@chromium.org
Link to the patchset: https://codereview.chromium.org/1880853002/#ps120001
(title: "msw review")

[commit-bot: I haz the power, 2016-04-18 01:28:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1880853002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1880853002/120001

[commit-bot: I haz the power, 2016-04-18 02:22:16]

Description was changed from

==========
Convert ValidationMessageBubbleView to BubbleDialogDelegate

- merge with ValidationMessageBubbleDelegate (which is the actual view)
- make bubble self-owned, like almost all bubbles (makes shutdown destruction
less tricky)

BUG=585312
==========

to

==========
Convert ValidationMessageBubbleView to BubbleDialogDelegate

- merge with ValidationMessageBubbleDelegate (which is the actual view)
- make bubble self-owned, like almost all bubbles (makes shutdown destruction
less tricky)

BUG=585312
==========

[commit-bot: I haz the power, 2016-04-18 02:22:17]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-04-18 02:23:17]

Description was changed from

==========
Convert ValidationMessageBubbleView to BubbleDialogDelegate

- merge with ValidationMessageBubbleDelegate (which is the actual view)
- make bubble self-owned, like almost all bubbles (makes shutdown destruction
less tricky)

BUG=585312
==========

to

==========
Convert ValidationMessageBubbleView to BubbleDialogDelegate

- merge with ValidationMessageBubbleDelegate (which is the actual view)
- make bubble self-owned, like almost all bubbles (makes shutdown destruction
less tricky)

BUG=585312

Committed: https://crrev.com/53f17391c1e7884791dc02b14cbba2e6dd65ff91
Cr-Commit-Position: refs/heads/master@{#387845}
==========

[commit-bot: I haz the power, 2016-04-18 02:23:17]

Patchset 7 (id:??) landed as
https://crrev.com/53f17391c1e7884791dc02b14cbba2e6dd65ff91
Cr-Commit-Position: refs/heads/master@{#387845}