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

Issue 2845003002: Merge ArcNotificationItem and ArcCustomNotificationItem (Closed)

Created:
3 years, 8 months ago by yoshiki
Modified:
3 years, 7 months ago
Reviewers:
hidehiko
CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, yusukes+watch_chromium.org, Peter Beverloo, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, victorhsieh+watch_chromium.org, mlamouri+watch-notifications_chromium.org, awdf+watch_chromium.org, yhanada, Eliot Courtney
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Merge ArcNotificationItem and ArcCustomNotificationItem We have introduced ArcNotificationItem for ARC notifications but it's no longer used because currently all ARC notifications use ArcCustomNotificationItem. This patch merges ArcCustomNotificationItem into ArcNotificationItem. This patch also removes the unused code in ArcNotificationItem, which was used for non-custom ARC notifications. In addition, this CL splits the class into an interface and an implementation. BUG=697359 TEST=none Review-Url: https://codereview.chromium.org/2845003002 Cr-Commit-Position: refs/heads/master@{#469982} Committed: https://chromium.googlesource.com/chromium/src/+/0a0ea6a0c083e27e574b2a872ce6b810d2c69360

Patch Set 1 #

Total comments: 53

Patch Set 2 : Addressed comments #

Patch Set 3 : . #

Total comments: 8

Patch Set 4 : Addressed comments (#56) #

Total comments: 2

Patch Set 5 : Addressed comments (#67) #

Patch Set 6 : Rebased #

Unified diffs Side-by-side diffs Delta from patch set Stats (+476 lines, -704 lines) Patch
M ui/arc/BUILD.gn View 1 2 1 chunk +4 lines, -3 lines 0 comments Download
D ui/arc/notification/arc_custom_notification_item.h View 1 chunk +0 lines, -76 lines 0 comments Download
D ui/arc/notification/arc_custom_notification_item.cc View 1 chunk +0 lines, -141 lines 0 comments Download
M ui/arc/notification/arc_custom_notification_view.h View 1 2 4 chunks +5 lines, -5 lines 0 comments Download
M ui/arc/notification/arc_custom_notification_view.cc View 1 2 3 4 5 10 chunks +32 lines, -26 lines 0 comments Download
A ui/arc/notification/arc_notification_delegate.h View 1 2 3 1 chunk +42 lines, -0 lines 0 comments Download
A ui/arc/notification/arc_notification_delegate.cc View 1 2 3 4 1 chunk +39 lines, -0 lines 0 comments Download
M ui/arc/notification/arc_notification_item.h View 1 2 3 1 chunk +63 lines, -91 lines 0 comments Download
D ui/arc/notification/arc_notification_item.cc View 1 2 1 chunk +0 lines, -349 lines 0 comments Download
A ui/arc/notification/arc_notification_item_impl.h View 1 2 1 chunk +92 lines, -0 lines 0 comments Download
A ui/arc/notification/arc_notification_item_impl.cc View 1 2 1 chunk +194 lines, -0 lines 0 comments Download
M ui/arc/notification/arc_notification_manager.cc View 1 2 chunks +5 lines, -13 lines 0 comments Download

Messages

Total messages: 83 (73 generated)
yoshiki
Hidehiko-san, PTAL. This is a part of https://codereview.chromium.org/2723143002/.
3 years, 7 months ago (2017-05-01 11:39:05 UTC) #39
hidehiko
Nice refactoring! I'm looking forward to seeing unittest :-). First round. https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/arc_custom_notification_view.cc File ui/arc/notification/arc_custom_notification_view.cc (right): ...
3 years, 7 months ago (2017-05-01 12:29:19 UTC) #40
yoshiki
Hidehiko-san, PTAL. https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/arc_custom_notification_view.cc File ui/arc/notification/arc_custom_notification_view.cc (right): https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/arc_custom_notification_view.cc#newcode240 ui/arc/notification/arc_custom_notification_view.cc:240: if (ArcNotificationSurfaceManager::Get()) { On 2017/05/01 12:29:17, hidehiko ...
3 years, 7 months ago (2017-05-08 06:24:54 UTC) #55
hidehiko
My main point is; ui/arc/notification/arc_notification_item_impl.cc:85 Others are just minor comments, and LG. https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/arc_notification_delegate.h File ui/arc/notification/arc_notification_delegate.h ...
3 years, 7 months ago (2017-05-08 09:34:41 UTC) #56
yoshiki
https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/arc_notification_item_impl.cc File ui/arc/notification/arc_notification_item_impl.cc (right): https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/arc_notification_item_impl.cc#newcode85 ui/arc/notification/arc_notification_item_impl.cc:85: if (HasPendingNotification()) { On 2017/05/08 09:34:41, hidehiko wrote: > ...
3 years, 7 months ago (2017-05-08 09:41:53 UTC) #57
yoshiki
And I addressed comment. Thank you for reviewing! https://codereview.chromium.org/2845003002/diff/220001/ui/arc/notification/arc_notification_delegate.h File ui/arc/notification/arc_notification_delegate.h (right): https://codereview.chromium.org/2845003002/diff/220001/ui/arc/notification/arc_notification_delegate.h#newcode9 ui/arc/notification/arc_notification_delegate.h:9: #include ...
3 years, 7 months ago (2017-05-08 09:51:52 UTC) #64
hidehiko
LGTM with minor comments. (but you'd need to rebase). https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/arc_notification_item_impl.cc File ui/arc/notification/arc_notification_item_impl.cc (right): https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/arc_notification_item_impl.cc#newcode85 ui/arc/notification/arc_notification_item_impl.cc:85: ...
3 years, 7 months ago (2017-05-08 10:12:39 UTC) #67
yoshiki
Thank you! https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/arc_notification_item_impl.cc File ui/arc/notification/arc_notification_item_impl.cc (right): https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/arc_notification_item_impl.cc#newcode85 ui/arc/notification/arc_notification_item_impl.cc:85: if (HasPendingNotification()) { On 2017/05/08 10:12:39, hidehiko ...
3 years, 7 months ago (2017-05-08 14:31:19 UTC) #77
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/2845003002/300001
3 years, 7 months ago (2017-05-08 14:32:23 UTC) #80
commit-bot: I haz the power
3 years, 7 months ago (2017-05-08 14:36:49 UTC) #83
Message was sent while issue was closed.
Committed patchset #6 (id:300001) as
https://chromium.googlesource.com/chromium/src/+/0a0ea6a0c083e27e574b2a872ce6...

Powered by Google App Engine
This is Rietveld 408576698