Update the background color of ARC notifications when notification settings is opened.

Update the background color of ARC notifications when notification settings is opened.

The notification settings window has a grey background. it does
not match to the default background color of the settings button
and the close button.
This CL changes the background color of these buttons when opening
the notification settings.

BUG=712114
Review-Url: https://codereview.chromium.org/2820073002
Cr-Commit-Position: refs/heads/master@{#465141}
Committed: https://chromium.googlesource.com/chromium/src/+/4c5ebef72b1198a911d46813140d558333131803

[yhanada, 2017-04-17 07:26:19]

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

[yhanada, 2017-04-17 07:26:35]

yhanada@chromium.org changed reviewers:
+ edcourtney@chromium.org, yoshiki@chromium.org

[yhanada, 2017-04-17 07:26:35]

PTAL.

[commit-bot: I haz the power, 2017-04-17 07:26:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Eliot Courtney, 2017-04-17 07:32:33]

lgtm with one nit - please wait for yoshiki@ as well.

https://codereview.chromium.org/2820073002/diff/1/ui/arc/notification/arc_cus...
File ui/arc/notification/arc_custom_notification_view.cc (right):

https://codereview.chromium.org/2820073002/diff/1/ui/arc/notification/arc_cus...
ui/arc/notification/arc_custom_notification_view.cc:211:
set_background(views::Background::CreateSolidBackground(
nit: If this color isn't a constant anywhere else, it might be good to pull this
out as a constant as well.

[commit-bot: I haz the power, 2017-04-17 07:54:54]

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

[commit-bot: I haz the power, 2017-04-17 07:54:54]

Dry run: This issue passed the CQ dry run.

[yoshiki, 2017-04-17 08:20:57]

https://codereview.chromium.org/2820073002/diff/1/ui/arc/notification/arc_cus...
File ui/arc/notification/arc_custom_notification_view.cc (right):

https://codereview.chromium.org/2820073002/diff/1/ui/arc/notification/arc_cus...
ui/arc/notification/arc_custom_notification_view.cc:32: constexpr SkColor
kSettingsViewBackgroundColor =
nit: I assume this value is set in Android code originally. Please add a comment
about the source location?

https://codereview.chromium.org/2820073002/diff/1/ui/arc/notification/arc_cus...
ui/arc/notification/arc_custom_notification_view.cc:210: if (shown_contents ==
mojom::ArcNotificationShownContents::CONTENTS_SHOWN)
nit: Please add braces ("{" and "}) for multi-line statement.

https://codereview.chromium.org/2820073002/diff/1/ui/arc/notification/arc_cus...
ui/arc/notification/arc_custom_notification_view.cc:281: if (item_)
It looks CreateSettingsButton is called only when item != null as for now. Is it
better to remove this "if", and add DCHECK(item) as one of preconditions of this
method?

https://codereview.chromium.org/2820073002/diff/1/ui/arc/notification/arc_cus...
ui/arc/notification/arc_custom_notification_view.cc:301: CreateCloseButton();
This is out of this CL, but should we check item_->pinned() for
CreateCloseButton() as well?

https://codereview.chromium.org/2820073002/diff/1/ui/arc/notification/arc_cus...
ui/arc/notification/arc_custom_notification_view.cc:411: if (!surface_)
I think it's possible to return here even for "GetWidget() == null" case,
because settings_button_ and close_button_ never be non-null if GetWidget() is
null.

[yhanada, 2017-04-17 11:58:09]

https://codereview.chromium.org/2820073002/diff/1/ui/arc/notification/arc_cus...
File ui/arc/notification/arc_custom_notification_view.cc (right):

https://codereview.chromium.org/2820073002/diff/1/ui/arc/notification/arc_cus...
ui/arc/notification/arc_custom_notification_view.cc:32: constexpr SkColor
kSettingsViewBackgroundColor =
On 2017/04/17 08:20:57, yoshiki wrote:
> nit: I assume this value is set in Android code originally. Please add a
comment
> about the source location?

Changed to SK_ColorTRANSPARENT, so removed this.

https://codereview.chromium.org/2820073002/diff/1/ui/arc/notification/arc_cus...
ui/arc/notification/arc_custom_notification_view.cc:210: if (shown_contents ==
mojom::ArcNotificationShownContents::CONTENTS_SHOWN)
On 2017/04/17 08:20:57, yoshiki wrote:
> nit: Please add braces ("{" and "}) for multi-line statement.

Done.

https://codereview.chromium.org/2820073002/diff/1/ui/arc/notification/arc_cus...
ui/arc/notification/arc_custom_notification_view.cc:211:
set_background(views::Background::CreateSolidBackground(
On 2017/04/17 07:32:33, Eliot Courtney wrote:
> nit: If this color isn't a constant anywhere else, it might be good to pull
this
> out as a constant as well.

Done.

https://codereview.chromium.org/2820073002/diff/1/ui/arc/notification/arc_cus...
ui/arc/notification/arc_custom_notification_view.cc:281: if (item_)
On 2017/04/17 08:20:57, yoshiki wrote:
> It looks CreateSettingsButton is called only when item != null as for now. Is
it
> better to remove this "if", and add DCHECK(item) as one of preconditions of
this
> method? 

Done.

https://codereview.chromium.org/2820073002/diff/1/ui/arc/notification/arc_cus...
ui/arc/notification/arc_custom_notification_view.cc:301: CreateCloseButton();
On 2017/04/17 08:20:57, yoshiki wrote:
> This is out of this CL, but should we check item_->pinned() for
> CreateCloseButton() as well?

Done.

https://codereview.chromium.org/2820073002/diff/1/ui/arc/notification/arc_cus...
ui/arc/notification/arc_custom_notification_view.cc:411: if (!surface_)
On 2017/04/17 08:20:57, yoshiki wrote:
> I think it's possible to return here even for "GetWidget() == null" case,
> because settings_button_ and close_button_ never be non-null if GetWidget() is
> null.

Done.

[yhanada, 2017-04-17 11:58:20]

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

[commit-bot: I haz the power, 2017-04-17 11:58:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yoshiki, 2017-04-17 12:43:45]

lgtm. thanks!

[commit-bot: I haz the power, 2017-04-17 12:53:03]

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

[commit-bot: I haz the power, 2017-04-17 12:53:04]

Dry run: This issue passed the CQ dry run.

[yhanada, 2017-04-17 14:03:55]

The CQ bit was checked by yhanada@chromium.org

[yhanada, 2017-04-17 14:03:56]

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

[commit-bot: I haz the power, 2017-04-17 14:04:10]

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-04-17 14:09:40]

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

[commit-bot: I haz the power, 2017-04-17 14:09:41]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[yhanada, 2017-04-17 14:13:57]

yhanada@chromium.org changed reviewers:
+ dcheng@chromium.org

[yhanada, 2017-04-17 14:13:57]

dcheng@: Could you review the mojo change?

[dcheng, 2017-04-17 19:06:42]

mojo lgtm with comments addressed

https://codereview.chromium.org/2820073002/diff/20001/components/arc/common/n...
File components/arc/common/notifications.mojom (right):

https://codereview.chromium.org/2820073002/diff/20001/components/arc/common/n...
components/arc/common/notifications.mojom:60: CONTENTS_SHOWN = 0,
Nit: add some comments to this enum, I don't think it's clear what these two
enum values mean (or what this enum field means overall in the struct below)

[yhanada, 2017-04-18 04:03:01]

Thank you all for reviewing!

https://codereview.chromium.org/2820073002/diff/20001/components/arc/common/n...
File components/arc/common/notifications.mojom (right):

https://codereview.chromium.org/2820073002/diff/20001/components/arc/common/n...
components/arc/common/notifications.mojom:60: CONTENTS_SHOWN = 0,
On 2017/04/17 19:06:42, dcheng (OOO through May 2) wrote:
> Nit: add some comments to this enum, I don't think it's clear what these two
> enum values mean (or what this enum field means overall in the struct below)

Done.

[yhanada, 2017-04-18 04:03:07]

The CQ bit was checked by yhanada@chromium.org

[yhanada, 2017-04-18 04:03:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
yoshiki@chromium.org, edcourtney@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2820073002/#ps40001
(title: "add comments")

[commit-bot: I haz the power, 2017-04-18 04:03:31]

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-04-18 04:49:17]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1492488187635780,
"parent_rev": "5f9e10d18a42268051f35d98c4ed5eb19597fa0c", "commit_rev":
"4c5ebef72b1198a911d46813140d558333131803"}

[commit-bot: I haz the power, 2017-04-18 04:50:03]

Description was changed from

==========
Update the background color of ARC notifications when notification settings is
opened.

The notification settings window has a grey background. it does
not match to the default background color of the settings button
and the close button.
This CL changes the background color of these buttons when opening
the notification settings.

BUG=712114
==========

to

==========
Update the background color of ARC notifications when notification settings is
opened.

The notification settings window has a grey background. it does
not match to the default background color of the settings button
and the close button.
This CL changes the background color of these buttons when opening
the notification settings.

BUG=712114

Review-Url: https://codereview.chromium.org/2820073002
Cr-Commit-Position: refs/heads/master@{#465141}
Committed:
https://chromium.googlesource.com/chromium/src/+/4c5ebef72b1198a911d46813140d...
==========

[commit-bot: I haz the power, 2017-04-18 04:50:04]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/4c5ebef72b1198a911d46813140d...