chromeos: Remove TrayBubbleView::Delegate::OnBeforeBubbleWidgetInit

chromeos: Remove TrayBubbleView::Delegate::OnBeforeBubbleWidgetInit

This method was added to let mash set the widget container back when
mash could not use aura::Window. Now we can set the parent aura::Window
explicitly.

Eliminate the TrayBubbleView::Create() factory method because its
signature is identical to the constructor and it doesn't do any
validation.

Eliminate some GetInitParams / GetDefaultInitParams methods that were
only used by WebNotificationTray.

This is mostly a manual revert of:
https://codereview.chromium.org/2092473002/

BUG=671246
TEST=ash_unittests, views_unittests, manually opening all system tray
bubbles on primary and secondary monitors

Review-Url: https://codereview.chromium.org/2901273003
Cr-Commit-Position: refs/heads/master@{#475682}
Committed: https://chromium.googlesource.com/chromium/src/+/c3301607c94a89ec896e7c33f0ec1612e995e84e

[James Cook, 2017-05-24 16:42:18]

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

[James Cook, 2017-05-24 16:42:29]

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

[James Cook, 2017-05-24 16:42:30]

msw, please take a look.

[commit-bot: I haz the power, 2017-05-24 16:42:43]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-24 17:44:15]

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

[commit-bot: I haz the power, 2017-05-24 17:44:16]

Dry run: This issue passed the CQ dry run.

[msw, 2017-05-24 18:04:39]

A couple questions, the parent one is more pertinent.

https://codereview.chromium.org/2901273003/diff/20001/ui/views/bubble/bubble_...
File ui/views/bubble/bubble_dialog_delegate.h (left):

https://codereview.chromium.org/2901273003/diff/20001/ui/views/bubble/bubble_...
ui/views/bubble/bubble_dialog_delegate.h:103: virtual void
OnBeforeBubbleWidgetInit(Widget::InitParams* params,
q: We should be able to eliminate this soon, if not now, right?

https://codereview.chromium.org/2901273003/diff/20001/ui/views/bubble/tray_bu...
File ui/views/bubble/tray_bubble_view.cc (right):

https://codereview.chromium.org/2901273003/diff/20001/ui/views/bubble/tray_bu...
ui/views/bubble/tray_bubble_view.cc:186:
TrayBubbleView::TrayBubbleView(gfx::NativeWindow parent_window,
q: Should this be passed in as init_params.parent?

[James Cook, 2017-05-24 18:27:04]

https://codereview.chromium.org/2901273003/diff/20001/ui/views/bubble/bubble_...
File ui/views/bubble/bubble_dialog_delegate.h (left):

https://codereview.chromium.org/2901273003/diff/20001/ui/views/bubble/bubble_...
ui/views/bubble/bubble_dialog_delegate.h:103: virtual void
OnBeforeBubbleWidgetInit(Widget::InitParams* params,
On 2017/05/24 18:04:38, msw wrote:
> q: We should be able to eliminate this soon, if not now, right?

Unfortunately this one is still used to do things like set custom shadows,
allocate a new widget for the app list, etc. We might be able to remove it, but
it's not trivial.

https://codereview.chromium.org/2901273003/diff/20001/ui/views/bubble/tray_bu...
File ui/views/bubble/tray_bubble_view.cc (right):

https://codereview.chromium.org/2901273003/diff/20001/ui/views/bubble/tray_bu...
ui/views/bubble/tray_bubble_view.cc:186:
TrayBubbleView::TrayBubbleView(gfx::NativeWindow parent_window,
On 2017/05/24 18:04:38, msw wrote:
> q: Should this be passed in as init_params.parent?

I considered that, but decided to keep InitParams as plain-old data. That means
InitParams doesn't need any validation when it is constructed (like validating
that parent isn't null). But it would work either way - do you want me to change
it?

[msw, 2017-05-24 18:48:36]

https://codereview.chromium.org/2901273003/diff/20001/ui/views/bubble/bubble_...
File ui/views/bubble/bubble_dialog_delegate.h (left):

https://codereview.chromium.org/2901273003/diff/20001/ui/views/bubble/bubble_...
ui/views/bubble/bubble_dialog_delegate.h:103: virtual void
OnBeforeBubbleWidgetInit(Widget::InitParams* params,
On 2017/05/24 18:27:04, James Cook wrote:
> On 2017/05/24 18:04:38, msw wrote:
> > q: We should be able to eliminate this soon, if not now, right?
> 
> Unfortunately this one is still used to do things like set custom shadows,
> allocate a new widget for the app list, etc. We might be able to remove it,
but
> it's not trivial.

Acknowledged. Sounds worth pursuing later on.

https://codereview.chromium.org/2901273003/diff/20001/ui/views/bubble/tray_bu...
File ui/views/bubble/tray_bubble_view.cc (right):

https://codereview.chromium.org/2901273003/diff/20001/ui/views/bubble/tray_bu...
ui/views/bubble/tray_bubble_view.cc:186:
TrayBubbleView::TrayBubbleView(gfx::NativeWindow parent_window,
On 2017/05/24 18:27:04, James Cook wrote:
> On 2017/05/24 18:04:38, msw wrote:
> > q: Should this be passed in as init_params.parent?
> 
> I considered that, but decided to keep InitParams as plain-old data. That
means
> InitParams doesn't need any validation when it is constructed (like validating
> that parent isn't null). But it would work either way - do you want me to
change
> it?

I think it's a bit odd to have two parent params; I would prefer one if it
doesn't add much complexity.

[James Cook, 2017-05-30 16:03:35]

Description was changed from

==========
chromeos: Remove TrayBubbleView::Delegate::OnBeforeBubbleWidgetInit

This method was added to let mash set the widget container back when
mash could not use aura::Window. Now we can set the parent aura::Window
explicitly.

Eliminate the TrayBubbleView::Create() factory method because its
signature is identical to the constructor and it doesn't do any
validation.

This is mostly a manual revert of:
https://codereview.chromium.org/2092473002/

BUG=671246
TEST=ash_unittests, views_unittests, manually opening all system tray
bubbles on primary and secondary monitors
==========

to

==========
chromeos: Remove TrayBubbleView::Delegate::OnBeforeBubbleWidgetInit

This method was added to let mash set the widget container back when
mash could not use aura::Window. Now we can set the parent aura::Window
explicitly.

Eliminate the TrayBubbleView::Create() factory method because its
signature is identical to the constructor and it doesn't do any
validation.

Eliminate some GetInitParams / GetDefaultInitParams methods that were
only used by WebNotificationTray.

This is mostly a manual revert of:
https://codereview.chromium.org/2092473002/

BUG=671246
TEST=ash_unittests, views_unittests, manually opening all system tray
bubbles on primary and secondary monitors
==========

[James Cook, 2017-05-30 16:04:30]

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

[commit-bot: I haz the power, 2017-05-30 16:04:51]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[James Cook, 2017-05-30 16:07:25]

msw, please take another look. I converted TrayBubbleView to take all its params
via InitParams, rather than a mix of args + InitParams. This revealed that
methods like GetInitParams / GetDefaultInitParams were only used by
WebNotificationTray, so I inlined the code there.

[commit-bot: I haz the power, 2017-05-30 17:17:45]

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

[commit-bot: I haz the power, 2017-05-30 17:17:46]

Dry run: This issue passed the CQ dry run.

[msw, 2017-05-30 18:36:04]

lgtm with an optional nit; thanks for the extra cleanup!

https://codereview.chromium.org/2901273003/diff/40001/ui/views/bubble/tray_bu...
File ui/views/bubble/tray_bubble_view.cc (right):

https://codereview.chromium.org/2901273003/diff/40001/ui/views/bubble/tray_bu...
ui/views/bubble/tray_bubble_view.cc:175: : delegate(nullptr),
optional nit: move member init to the decl?

[James Cook, 2017-05-30 19:48:13]

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

[commit-bot: I haz the power, 2017-05-30 19:49:05]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[James Cook, 2017-05-30 20:03:27]

jamescook@chromium.org changed reviewers:
+ stevenjb@chromium.org

[James Cook, 2017-05-30 20:03:27]

Steven, can I get owners approval for ui/message_center/* ?

(Also, do you happen to know if there was a Windows/Mac version of message
center that was removed? There seems to be a moderate amount of "cross-platform"
code that is Chrome OS only.)

[stevenjb, 2017-05-30 21:13:39]

On 2017/05/30 20:03:27, James Cook wrote:
> Steven, can I get owners approval for ui/message_center/* ?
> 
> (Also, do you happen to know if there was a Windows/Mac version of message
> center that was removed? There seems to be a moderate amount of
"cross-platform"
> code that is Chrome OS only.)

There *was* a Windows+Ash version of message center when there was a Windows
version of Ash.
There *may* also be a non-Ash Windows/Mac version as well. I don't *think* that
we support that any more, but I'm not sure and would have to investigate
further.
If there is "cross-platform" Chrome OS only code, that is most likely left over
from Windows+Ash and can be deprecated.

[stevenjb, 2017-05-30 21:14:31]

ui/message_center lgtm

[commit-bot: I haz the power, 2017-05-30 21:26:02]

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

[commit-bot: I haz the power, 2017-05-30 21:26:03]

Dry run: This issue passed the CQ dry run.

[James Cook, 2017-05-30 21:43:44]

The CQ bit was checked by jamescook@chromium.org

[James Cook, 2017-05-30 21:43:44]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org
Link to the patchset: https://codereview.chromium.org/2901273003/#ps60001
(title: "nit")

[commit-bot: I haz the power, 2017-05-30 21:44:25]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-30 21:49:30]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1496180624287420,
"parent_rev": "c0831478a26c1a0cc351f787780a8f191601d316", "commit_rev":
"c3301607c94a89ec896e7c33f0ec1612e995e84e"}

[commit-bot: I haz the power, 2017-05-30 21:49:40]

Description was changed from

==========
chromeos: Remove TrayBubbleView::Delegate::OnBeforeBubbleWidgetInit

This method was added to let mash set the widget container back when
mash could not use aura::Window. Now we can set the parent aura::Window
explicitly.

Eliminate the TrayBubbleView::Create() factory method because its
signature is identical to the constructor and it doesn't do any
validation.

Eliminate some GetInitParams / GetDefaultInitParams methods that were
only used by WebNotificationTray.

This is mostly a manual revert of:
https://codereview.chromium.org/2092473002/

BUG=671246
TEST=ash_unittests, views_unittests, manually opening all system tray
bubbles on primary and secondary monitors
==========

to

==========
chromeos: Remove TrayBubbleView::Delegate::OnBeforeBubbleWidgetInit

This method was added to let mash set the widget container back when
mash could not use aura::Window. Now we can set the parent aura::Window
explicitly.

Eliminate the TrayBubbleView::Create() factory method because its
signature is identical to the constructor and it doesn't do any
validation.

Eliminate some GetInitParams / GetDefaultInitParams methods that were
only used by WebNotificationTray.

This is mostly a manual revert of:
https://codereview.chromium.org/2092473002/

BUG=671246
TEST=ash_unittests, views_unittests, manually opening all system tray
bubbles on primary and secondary monitors

Review-Url: https://codereview.chromium.org/2901273003
Cr-Commit-Position: refs/heads/master@{#475682}
Committed:
https://chromium.googlesource.com/chromium/src/+/c3301607c94a89ec896e7c33f0ec...
==========

[commit-bot: I haz the power, 2017-05-30 21:49:41]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/c3301607c94a89ec896e7c33f0ec...