Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(14)

Issue 2820073002: Update the background color of ARC notifications when notification settings is opened. (Closed)

Created:
3 years, 8 months ago by yhanada
Modified:
3 years, 8 months ago
CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, yusukes+watch_chromium.org, viettrungluu+watch_chromium.org, hidehiko+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, lhchavez+watch_chromium.org, victorhsieh+watch_chromium.org, darin (slow to review), qsr+mojo_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 12

Patch Set 2 : . #

Total comments: 2

Patch Set 3 : add comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+123 lines, -5 lines) Patch
M components/arc/common/notifications.mojom View 1 2 2 chunks +12 lines, -0 lines 0 comments Download
M ui/arc/notification/arc_custom_notification_item.h View 1 chunk +5 lines, -0 lines 0 comments Download
M ui/arc/notification/arc_custom_notification_item.cc View 1 chunk +1 line, -0 lines 0 comments Download
M ui/arc/notification/arc_custom_notification_view.h View 1 6 chunks +15 lines, -1 line 0 comments Download
M ui/arc/notification/arc_custom_notification_view.cc View 1 2 9 chunks +85 lines, -2 lines 0 comments Download
M ui/message_center/message_center_style.h View 1 1 chunk +3 lines, -0 lines 0 comments Download
M ui/message_center/views/padded_button.cc View 1 1 chunk +2 lines, -2 lines 0 comments Download

Messages

Total messages: 29 (17 generated)
yhanada
PTAL.
3 years, 8 months ago (2017-04-17 07:26:35 UTC) #3
Eliot Courtney
lgtm with one nit - please wait for yoshiki@ as well. https://codereview.chromium.org/2820073002/diff/1/ui/arc/notification/arc_custom_notification_view.cc File ui/arc/notification/arc_custom_notification_view.cc (right): ...
3 years, 8 months ago (2017-04-17 07:32:33 UTC) #5
yoshiki
https://codereview.chromium.org/2820073002/diff/1/ui/arc/notification/arc_custom_notification_view.cc File ui/arc/notification/arc_custom_notification_view.cc (right): https://codereview.chromium.org/2820073002/diff/1/ui/arc/notification/arc_custom_notification_view.cc#newcode32 ui/arc/notification/arc_custom_notification_view.cc:32: constexpr SkColor kSettingsViewBackgroundColor = nit: I assume this value ...
3 years, 8 months ago (2017-04-17 08:20:57 UTC) #8
yhanada
https://codereview.chromium.org/2820073002/diff/1/ui/arc/notification/arc_custom_notification_view.cc File ui/arc/notification/arc_custom_notification_view.cc (right): https://codereview.chromium.org/2820073002/diff/1/ui/arc/notification/arc_custom_notification_view.cc#newcode32 ui/arc/notification/arc_custom_notification_view.cc:32: constexpr SkColor kSettingsViewBackgroundColor = On 2017/04/17 08:20:57, yoshiki wrote: ...
3 years, 8 months ago (2017-04-17 11:58:09 UTC) #9
yoshiki
lgtm. thanks!
3 years, 8 months ago (2017-04-17 12:43:45 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2820073002/20001
3 years, 8 months ago (2017-04-17 14:04:10 UTC) #17
commit-bot: I haz the power
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_presubmit/builds/412705)
3 years, 8 months ago (2017-04-17 14:09:41 UTC) #19
yhanada
dcheng@: Could you review the mojo change?
3 years, 8 months ago (2017-04-17 14:13:57 UTC) #21
dcheng
mojo lgtm with comments addressed https://codereview.chromium.org/2820073002/diff/20001/components/arc/common/notifications.mojom File components/arc/common/notifications.mojom (right): https://codereview.chromium.org/2820073002/diff/20001/components/arc/common/notifications.mojom#newcode60 components/arc/common/notifications.mojom:60: CONTENTS_SHOWN = 0, Nit: ...
3 years, 8 months ago (2017-04-17 19:06:42 UTC) #22
yhanada
Thank you all for reviewing! https://codereview.chromium.org/2820073002/diff/20001/components/arc/common/notifications.mojom File components/arc/common/notifications.mojom (right): https://codereview.chromium.org/2820073002/diff/20001/components/arc/common/notifications.mojom#newcode60 components/arc/common/notifications.mojom:60: CONTENTS_SHOWN = 0, On ...
3 years, 8 months ago (2017-04-18 04:03:01 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2820073002/40001
3 years, 8 months ago (2017-04-18 04:03:31 UTC) #26
commit-bot: I haz the power
3 years, 8 months ago (2017-04-18 04:50:04 UTC) #29
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/4c5ebef72b1198a911d46813140d...

Powered by Google App Engine
This is Rietveld 408576698