Initial implementation of new-style notification

Initial implementation of new-style notification

This works behind flag. Currently only normal notification is supported.

BUG=723144
Review-Url: https://codereview.chromium.org/2892893002
Cr-Commit-Position: refs/heads/master@{#475798}
Committed: https://chromium.googlesource.com/chromium/src/+/f14ac82cdb0d6e6953e59558e0f2884ca8bf89c8

[yoshiki, 2017-05-19 07:49:11]

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

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

Dry run: CQ is trying da patch.

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

[yoshiki, 2017-05-19 07:49:56]

Description was changed from

==========
Initial implementation of new-style notification

This works behind flag. Currently only normal notification is supported.

BUG=723144
==========

to

==========
Initial implementation of new-style notification

This works behind flag. Currently only normal notification is supported.

BUG=723144
==========

[yoshiki, 2017-05-19 07:49:56]

yoshiki@chromium.org changed reviewers:
+ fukino@chromium.org

[yoshiki, 2017-05-19 07:50:10]

Fukino-san, PTAL

[commit-bot: I haz the power, 2017-05-19 07:53:20]

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

[commit-bot: I haz the power, 2017-05-19 07:53:21]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[yoshiki, 2017-05-19 08:04:06]

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

[commit-bot: I haz the power, 2017-05-19 08:04:24]

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-19 08:24:40]

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

[commit-bot: I haz the power, 2017-05-19 08:24:41]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Peter Beverloo, 2017-05-19 12:11:10]

Could you please share a design doc, mocks (if there are any) and plans to unify
this with the existing notification types?

[yoshiki, 2017-05-23 03:43:04]

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

[commit-bot: I haz the power, 2017-05-23 03:43:25]

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-23 04:51:27]

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

[commit-bot: I haz the power, 2017-05-23 04:51:28]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yoshiki, 2017-05-24 00:32:19]

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

[commit-bot: I haz the power, 2017-05-24 00:33:38]

Dry run: CQ is trying da patch.

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

[yoshiki, 2017-05-24 00:39:55]

On 2017/05/19 12:11:10, Peter Beverloo wrote:
> Could you please share a design doc, mocks (if there are any) and plans to
unify
> this with the existing notification types?

Doc:
https://docs.google.com/document/d/1VXM-Zp_aGDKj_v4l1gTfYjCL8E3x2i_V4XyfkByAn...
Mock: https://gallery.googleplex.com/projects/MCHbtQVoQ2HCZYhMpwexg3v6

And we don't have a plan to unify the types. Thanks.

[commit-bot: I haz the power, 2017-05-24 02:17:33]

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

[commit-bot: I haz the power, 2017-05-24 02:17:33]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yoshiki, 2017-05-25 05:27:56]

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

[commit-bot: I haz the power, 2017-05-25 05:28:20]

Dry run: CQ is trying da patch.

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

[yoshiki, 2017-05-25 06:02:34]

Patchset #3 (id:30001) has been deleted

[commit-bot: I haz the power, 2017-05-25 06:47:07]

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

[commit-bot: I haz the power, 2017-05-25 06:47:09]

Dry run: This issue passed the CQ dry run.

[fukino, 2017-05-25 07:35:11]

I'm sorry for a late response!

https://codereview.chromium.org/2892893002/diff/50001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/2892893002/diff/50001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:1863: kOsDesktop,
MULTI_VALUE_TYPE(kMessageCenterNewStyleNotificationChoices)},
Can we use ENABLE_DISABLE_VALUE_TYPE to make related code simpler?

https://codereview.chromium.org/2892893002/diff/50001/ui/message_center/views...
File ui/message_center/views/message_view_factory.cc (right):

https://codereview.chromium.org/2892893002/diff/50001/ui/message_center/views...
ui/message_center/views/message_view_factory.cc:19: static base::Optional<bool>
new_style_notification_enabled;
This seems to be forbidden by coding style.
https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables

I think we don't need to worry about the cost of
base::CommandLine::ForCurrentProcess()->GetSwitchValuesASCII() or HasSwitch()
and we can check the flag each time the MessageView is constructed.
If we measure the performance overhead about it, let's optimize it later.

https://codereview.chromium.org/2892893002/diff/50001/ui/message_center/views...
File ui/message_center/views/notification_view_md.cc (right):

https://codereview.chromium.org/2892893002/diff/50001/ui/message_center/views...
ui/message_center/views/notification_view_md.cc:54: const SkColor
kSmallImageBackgroundColor = SK_ColorWHITE;
nit: constexpr for SkColor.

https://codereview.chromium.org/2892893002/diff/50001/ui/message_center/views...
ui/message_center/views/notification_view_md.cc:56: const SkColor
kSmallImageColor = SkColorSetRGB(0x43, 0x43, 0x43);
ditto

https://codereview.chromium.org/2892893002/diff/50001/ui/message_center/views...
ui/message_center/views/notification_view_md.cc:157:
main_view_->SetLayoutManager(main_box_layout);
nit: Instead of having |main_box_layout|, calling SetLayoutManager(new
views::BoxLayout...) is more consistent with how bottom_view_ is created.

https://codereview.chromium.org/2892893002/diff/50001/ui/message_center/views...
ui/message_center/views/notification_view_md.cc:173:
CreateOrUpdateCloseButtonView(notification);
Shouldn't we do CreateOrUpdateCloseButtonView() and
CreateOrUpdateSettingsButtonView() inside CreateOrUpdateViews()?

https://codereview.chromium.org/2892893002/diff/50001/ui/message_center/views...
ui/message_center/views/notification_view_md.cc:183: // View::Layout() is not
called in MessageView::Layout() for some reason.
I'm a bit concerned about calling both views::View::Layout() and
MessageView::Layout(). Can we call Views::Layout() inside MessageView::Layout()?

https://codereview.chromium.org/2892893002/diff/50001/ui/message_center/views...
ui/message_center/views/notification_view_md.cc:316:
context_title_view_->SetLineLimit(kMaxTitleLines);
This line should be removed? (as kMaxContextTitleLines seems the right value)

[yoshiki, 2017-05-25 16:49:53]

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

[commit-bot: I haz the power, 2017-05-25 16:50:21]

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-25 18:03:41]

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

[commit-bot: I haz the power, 2017-05-25 18:03:42]

Dry run: This issue passed the CQ dry run.

[yoshiki, 2017-05-29 03:52:39]

Fukino-san, PTAL. Thanks.

https://codereview.chromium.org/2892893002/diff/50001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/2892893002/diff/50001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:1863: kOsDesktop,
MULTI_VALUE_TYPE(kMessageCenterNewStyleNotificationChoices)},
On 2017/05/25 07:35:10, fukino wrote:
> Can we use ENABLE_DISABLE_VALUE_TYPE to make related code simpler?

I want to make it tri-state since we may re-disable the feature after enabling
it. If it is binary, the user who has set to enabled or disabled once can't
change back to the default.

https://codereview.chromium.org/2892893002/diff/50001/ui/message_center/views...
File ui/message_center/views/message_view_factory.cc (right):

https://codereview.chromium.org/2892893002/diff/50001/ui/message_center/views...
ui/message_center/views/message_view_factory.cc:19: static base::Optional<bool>
new_style_notification_enabled;
On 2017/05/25 07:35:10, fukino wrote:
> This seems to be forbidden by coding style.
> https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables
> 
> I think we don't need to worry about the cost of
> base::CommandLine::ForCurrentProcess()->GetSwitchValuesASCII() or HasSwitch()
> and we can check the flag each time the MessageView is constructed.
> If we measure the performance overhead about it, let's optimize it later.

Ok, Done.

https://codereview.chromium.org/2892893002/diff/50001/ui/message_center/views...
File ui/message_center/views/notification_view_md.cc (right):

https://codereview.chromium.org/2892893002/diff/50001/ui/message_center/views...
ui/message_center/views/notification_view_md.cc:54: const SkColor
kSmallImageBackgroundColor = SK_ColorWHITE;
On 2017/05/25 07:35:10, fukino wrote:
> nit: constexpr for SkColor.

Done.

https://codereview.chromium.org/2892893002/diff/50001/ui/message_center/views...
ui/message_center/views/notification_view_md.cc:56: const SkColor
kSmallImageColor = SkColorSetRGB(0x43, 0x43, 0x43);
On 2017/05/25 07:35:10, fukino wrote:
> ditto

We can't make it constexpr since SkColorSetRGB is not a constexpr function.

https://codereview.chromium.org/2892893002/diff/50001/ui/message_center/views...
ui/message_center/views/notification_view_md.cc:157:
main_view_->SetLayoutManager(main_box_layout);
On 2017/05/25 07:35:10, fukino wrote:
> nit: Instead of having |main_box_layout|, calling SetLayoutManager(new
> views::BoxLayout...) is more consistent with how bottom_view_ is created.

Done.

https://codereview.chromium.org/2892893002/diff/50001/ui/message_center/views...
ui/message_center/views/notification_view_md.cc:173:
CreateOrUpdateCloseButtonView(notification);
On 2017/05/25 07:35:10, fukino wrote:
> Shouldn't we do CreateOrUpdateCloseButtonView() and
> CreateOrUpdateSettingsButtonView() inside CreateOrUpdateViews()?

Done.

https://codereview.chromium.org/2892893002/diff/50001/ui/message_center/views...
ui/message_center/views/notification_view_md.cc:183: // View::Layout() is not
called in MessageView::Layout() for some reason.
On 2017/05/25 07:35:11, fukino wrote:
> I'm a bit concerned about calling both views::View::Layout() and
> MessageView::Layout(). Can we call Views::Layout() inside
MessageView::Layout()?

That's ideal but breaks NotificationView actually. We need to refactor to
respect views hierarchy and remove this hack in near feature.

https://codereview.chromium.org/2892893002/diff/50001/ui/message_center/views...
ui/message_center/views/notification_view_md.cc:316:
context_title_view_->SetLineLimit(kMaxTitleLines);
On 2017/05/25 07:35:11, fukino wrote:
> This line should be removed? (as kMaxContextTitleLines seems the right value)

Done.

[fukino, 2017-05-30 07:32:33]

I'm sorry for a late response!
lgtm with nits.

https://codereview.chromium.org/2892893002/diff/50001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/2892893002/diff/50001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:1863: kOsDesktop,
MULTI_VALUE_TYPE(kMessageCenterNewStyleNotificationChoices)},
On 2017/05/29 03:52:38, yoshiki wrote:
> On 2017/05/25 07:35:10, fukino wrote:
> > Can we use ENABLE_DISABLE_VALUE_TYPE to make related code simpler?
> 
> I want to make it tri-state since we may re-disable the feature after enabling
> it. If it is binary, the user who has set to enabled or disabled once can't
> change back to the default.

We have Enable/Disable/Default in chrome://flags for ENABLE_DISABLE_VALUE_TYPE
We can change the default behavior when we don't have either kEnableXxx or
kDisableXxx.
Doesn't it work for our user case?
If it doesn't work, having MULTI_VALUE_TYPE lgtm.

https://codereview.chromium.org/2892893002/diff/50001/ui/message_center/views...
File ui/message_center/views/notification_view_md.cc (right):

https://codereview.chromium.org/2892893002/diff/50001/ui/message_center/views...
ui/message_center/views/notification_view_md.cc:56: const SkColor
kSmallImageColor = SkColorSetRGB(0x43, 0x43, 0x43);
On 2017/05/29 03:52:39, yoshiki wrote:
> On 2017/05/25 07:35:10, fukino wrote:
> > ditto
> 
> We can't make it constexpr since SkColorSetRGB is not a constexpr function.

I see. Acknowledged.

https://codereview.chromium.org/2892893002/diff/50001/ui/message_center/views...
ui/message_center/views/notification_view_md.cc:183: // View::Layout() is not
called in MessageView::Layout() for some reason.
On 2017/05/29 03:52:38, yoshiki wrote:
> On 2017/05/25 07:35:11, fukino wrote:
> > I'm a bit concerned about calling both views::View::Layout() and
> > MessageView::Layout(). Can we call Views::Layout() inside
> MessageView::Layout()?
> 
> That's ideal but breaks NotificationView actually. We need to refactor to
> respect views hierarchy and remove this hack in near feature.

Oh, it sounds scary that calling View::Layout() inside MessageView::Layout()
breaks NotificationView...
Could you add a TODO comment or file a bug for it?
With a TODO comment or a bug, I think calling both View::Layout() and
MessageView::Layout() is OK for now.

[yoshiki, 2017-05-30 09:33:11]

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

[commit-bot: I haz the power, 2017-05-30 09:33:32]

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-30 10:09:05]

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

[commit-bot: I haz the power, 2017-05-30 10:09:06]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yoshiki, 2017-05-31 01:09:49]

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

[commit-bot: I haz the power, 2017-05-31 01:10:36]

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-31 01:51:55]

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

[commit-bot: I haz the power, 2017-05-31 01:51:56]

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

[yoshiki, 2017-05-31 02:15:20]

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

[commit-bot: I haz the power, 2017-05-31 02:15:56]

Dry run: CQ is trying da patch.

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

[yoshiki, 2017-05-31 03:52:39]

Patchset #5 (id:90001) has been deleted

[yoshiki, 2017-05-31 03:52:47]

Patchset #5 (id:110001) has been deleted

[yoshiki, 2017-05-31 03:53:01]

https://codereview.chromium.org/2892893002/diff/50001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/2892893002/diff/50001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:1863: kOsDesktop,
MULTI_VALUE_TYPE(kMessageCenterNewStyleNotificationChoices)},
On 2017/05/30 07:32:32, fukino wrote:
> On 2017/05/29 03:52:38, yoshiki wrote:
> > On 2017/05/25 07:35:10, fukino wrote:
> > > Can we use ENABLE_DISABLE_VALUE_TYPE to make related code simpler?
> > 
> > I want to make it tri-state since we may re-disable the feature after
enabling
> > it. If it is binary, the user who has set to enabled or disabled once can't
> > change back to the default.
> 
> We have Enable/Disable/Default in chrome://flags for ENABLE_DISABLE_VALUE_TYPE
> We can change the default behavior when we don't have either kEnableXxx or
> kDisableXxx.
> Doesn't it work for our user case?
> If it doesn't work, having MULTI_VALUE_TYPE lgtm.

Ah, I confused with SINGLE_VALUE_TYPE. Ok, let me change.

https://codereview.chromium.org/2892893002/diff/50001/ui/message_center/views...
File ui/message_center/views/notification_view_md.cc (right):

https://codereview.chromium.org/2892893002/diff/50001/ui/message_center/views...
ui/message_center/views/notification_view_md.cc:183: // View::Layout() is not
called in MessageView::Layout() for some reason.
On 2017/05/30 07:32:32, fukino wrote:
> On 2017/05/29 03:52:38, yoshiki wrote:
> > On 2017/05/25 07:35:11, fukino wrote:
> > > I'm a bit concerned about calling both views::View::Layout() and
> > > MessageView::Layout(). Can we call Views::Layout() inside
> > MessageView::Layout()?
> > 
> > That's ideal but breaks NotificationView actually. We need to refactor to
> > respect views hierarchy and remove this hack in near feature.
> 
> Oh, it sounds scary that calling View::Layout() inside MessageView::Layout()
> breaks NotificationView...
> Could you add a TODO comment or file a bug for it?
> With a TODO comment or a bug, I think calling both View::Layout() and
> MessageView::Layout() is OK for now.

As I checked, all wrong values which were set by View::Layout() are overridden
by NotificationView::Layout(). It is bad design but works without issues.

[commit-bot: I haz the power, 2017-05-31 03:59:27]

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

[commit-bot: I haz the power, 2017-05-31 03:59:28]

Dry run: This issue passed the CQ dry run.

[yoshiki, 2017-05-31 04:21:53]

The CQ bit was checked by yoshiki@chromium.org

[yoshiki, 2017-05-31 04:21:53]

The patchset sent to the CQ was uploaded after l-g-t-m from fukino@chromium.org
Link to the patchset: https://codereview.chromium.org/2892893002/#ps130001
(title: "addressed comments")

[commit-bot: I haz the power, 2017-05-31 04:22:11]

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-31 04:26:17]

CQ is committing da patch.

Bot data: {"patchset_id": 130001, "attempt_start_ts": 1496204513166460,
"parent_rev": "0e5657e03379575203b9617d955e1c410d3db7f2", "commit_rev":
"f14ac82cdb0d6e6953e59558e0f2884ca8bf89c8"}

[commit-bot: I haz the power, 2017-05-31 04:26:30]

Description was changed from

==========
Initial implementation of new-style notification

This works behind flag. Currently only normal notification is supported.

BUG=723144
==========

to

==========
Initial implementation of new-style notification

This works behind flag. Currently only normal notification is supported.

BUG=723144

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

[commit-bot: I haz the power, 2017-05-31 04:26:32]

Committed patchset #5 (id:130001) as
https://chromium.googlesource.com/chromium/src/+/f14ac82cdb0d6e6953e59558e0f2...