|
|
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. |
DescriptionMerge 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 #Messages
Total messages: 83 (73 generated)
The CQ bit was checked by yoshiki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by yoshiki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by yoshiki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by yoshiki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by yoshiki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by yoshiki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== wip BUG= ========== to ========== 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 merge them into ArcNotificationItem. In addition, this CL splits the class into an interface and an implementation. BUG=697359 TEST=none ==========
yoshiki@chromium.org changed reviewers: + yhanada@chromium.org
The CQ bit was checked by yoshiki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by yoshiki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
yoshiki@chromium.org changed reviewers: + hidehiko@chromium.org - yhanada@chromium.org
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:120001) has been deleted
Hidehiko-san, PTAL. This is a part of https://codereview.chromium.org/2723143002/.
Nice refactoring! I'm looking forward to seeing unittest :-). First round. https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... File ui/arc/notification/arc_custom_notification_view.cc (right): https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_custom_notification_view.cc:240: if (ArcNotificationSurfaceManager::Get()) { Optional/nit: auto* surface_manager = ArcNotificationSurfaceManager::Get(); if (surface_manager) { surface_manager->AddObserver(this); ... } may be slightly consistent? https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_custom_notification_view.cc:256: item_->DecrementWindowRefCount(); Please call clean up functions in the reverse order of the ctor. So, ArcNotificationSurfaceManager::Get()->RemoveObserver(this); item_->RemoveObserver(this); item_->DecrementWindowRefCount(); (with null check). https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_custom_notification_view.cc:364: surface_ ? surface_->GetSize() Optional/nit: Maybe better to avoid nested "?:" operator? gfx::Size preferred_size; if (surface_) preferred_size = surface_->GetSize(); else if(item_) item_->GetSnapshot().size(); https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... File ui/arc/notification/arc_custom_notification_view.h (right): https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_custom_notification_view.h:36: class ArcCustomNotificationView (slightly off-topic): Do we want to rename this to ArcNotificationView, too? (not necessary to address in this CL, though). https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... File ui/arc/notification/arc_notification_delegate.h (right): https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_notification_delegate.h:15: class ArcNotificationDelegate : public message_center::NotificationDelegate { Document about this class? https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_notification_delegate.h:19: std::unique_ptr<message_center::CustomContent> CreateCustomContent() override; // message_center::NotificationDelegate overrides: for consistency? https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_notification_delegate.h:27: base::WeakPtr<ArcNotificationItem> item_; You use WeakPtr here, but all methods assumes (DCHECKs) item_ is alive. When is WeakPtr actually needed? https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_notification_delegate.h:29: DISALLOW_COPY_AND_ASSIGN(ArcNotificationDelegate); missing base/macros.h https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... File ui/arc/notification/arc_notification_item.h (right): https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item.h:9: #include "base/observer_list.h" Not necessary to include this? https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item.h:31: // Methods called from ArcNotificationManager: As this became an interface, could you describe documents for each method in this class what is the responsibility, and, if necessary, when it is called? https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item.h:36: // Methods called from ArcNotificationItemDelegate: Could you fix the comments. https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item.h:43: virtual void CloseFromCloseButton() = 0; No caller? https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... File ui/arc/notification/arc_notification_item_impl.cc (right): https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item_impl.cc:7: #include <algorithm> Looks unnecessary? https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item_impl.cc:14: #include "base/task_runner.h" ditto for task_runner.h to SkPaint.h? https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item_impl.cc:20: #include "ui/gfx/codec/png_codec.h" Ditto. https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item_impl.cc:54: // fall-through Not a fall-through here...? https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item_impl.cc:56: NOTREACHED() << "Invalid Priority: " << android_priority; Can this be out from switch, so some compiler captures missing case, specifically if a new value is added to ArcNotificationPriority. switch (android_priority) { case ...: case ...: // no default. } NOTREACHED() << ...; return DEFAULT_PRIORITY; https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item_impl.cc:192: void ArcNotificationItemImpl::OnImageDecoded(const SkBitmap& bitmap) { No caller? https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... File ui/arc/notification/arc_notification_item_impl.h (right): https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item_impl.h:35: void OnClosedFromAndroid() override; // ArcNotificationItem overrides: https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item_impl.h:55: protected: Because ArcNotificationItemImpl is no longer inherited, this can be "private:" section? https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item_impl.h:56: // Checks whether there is on-going |notification_|. (optional): Also, several util functions can be inlined. Not necessary to address in this CL, though. https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item_impl.h:71: const AccountId& profile_id() const { return profile_id_; } Also, do not need these accessors? https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item_impl.h:87: bool pinned_ = false; Comments for each field, please? https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item_impl.h:95: base::ObserverList<Observer> observers_; include base/observer_list.h ?
The CQ bit was checked by yoshiki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by yoshiki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by yoshiki@chromium.org to run a CQ dry run
Patchset #3 (id:180001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by yoshiki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #3 (id:200001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
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_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Hidehiko-san, PTAL. https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... File ui/arc/notification/arc_custom_notification_view.cc (right): https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_custom_notification_view.cc:240: if (ArcNotificationSurfaceManager::Get()) { On 2017/05/01 12:29:17, hidehiko wrote: > Optional/nit: > > auto* surface_manager = ArcNotificationSurfaceManager::Get(); > if (surface_manager) { > surface_manager->AddObserver(this); > ... > } > > may be slightly consistent? Done. https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_custom_notification_view.cc:256: item_->DecrementWindowRefCount(); On 2017/05/01 12:29:17, hidehiko wrote: > Please call clean up functions in the reverse order of the ctor. > So, > > ArcNotificationSurfaceManager::Get()->RemoveObserver(this); > item_->RemoveObserver(this); > item_->DecrementWindowRefCount(); > (with null check). Done. https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_custom_notification_view.cc:364: surface_ ? surface_->GetSize() On 2017/05/01 12:29:17, hidehiko wrote: > Optional/nit: > > Maybe better to avoid nested "?:" operator? > > gfx::Size preferred_size; > if (surface_) > preferred_size = surface_->GetSize(); > else if(item_) > item_->GetSnapshot().size(); Done. https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... File ui/arc/notification/arc_custom_notification_view.h (right): https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_custom_notification_view.h:36: class ArcCustomNotificationView On 2017/05/01 12:29:17, hidehiko wrote: > (slightly off-topic): Do we want to rename this to ArcNotificationView, too? > (not necessary to address in this CL, though). Yes, I have a plan to refactor view things after this work. https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... File ui/arc/notification/arc_notification_delegate.h (right): https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_notification_delegate.h:15: class ArcNotificationDelegate : public message_center::NotificationDelegate { On 2017/05/01 12:29:17, hidehiko wrote: > Document about this class? Done. https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_notification_delegate.h:19: std::unique_ptr<message_center::CustomContent> CreateCustomContent() override; On 2017/05/01 12:29:17, hidehiko wrote: > // message_center::NotificationDelegate overrides: > > for consistency? Done. https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_notification_delegate.h:27: base::WeakPtr<ArcNotificationItem> item_; On 2017/05/01 12:29:17, hidehiko wrote: > You use WeakPtr here, but all methods assumes (DCHECKs) item_ is alive. > When is WeakPtr actually needed? I want to keep the weak ptr to detect a potential use-after-free. https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_notification_delegate.h:29: DISALLOW_COPY_AND_ASSIGN(ArcNotificationDelegate); On 2017/05/01 12:29:17, hidehiko wrote: > missing base/macros.h Done. https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... File ui/arc/notification/arc_notification_item.h (right): https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item.h:9: #include "base/observer_list.h" On 2017/05/01 12:29:17, hidehiko wrote: > Not necessary to include this? Done. https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item.h:31: // Methods called from ArcNotificationManager: On 2017/05/01 12:29:17, hidehiko wrote: > As this became an interface, could you describe documents for each method in > this class what is the responsibility, and, if necessary, when it is called? Done. https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item.h:36: // Methods called from ArcNotificationItemDelegate: On 2017/05/01 12:29:18, hidehiko wrote: > Could you fix the comments. Done. https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item.h:43: virtual void CloseFromCloseButton() = 0; On 2017/05/01 12:29:17, hidehiko wrote: > No caller? Removed. https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... File ui/arc/notification/arc_notification_item_impl.cc (right): https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item_impl.cc:7: #include <algorithm> On 2017/05/01 12:29:18, hidehiko wrote: > Looks unnecessary? Done. https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item_impl.cc:14: #include "base/task_runner.h" On 2017/05/01 12:29:18, hidehiko wrote: > ditto for task_runner.h to SkPaint.h? Done. https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item_impl.cc:20: #include "ui/gfx/codec/png_codec.h" On 2017/05/01 12:29:18, hidehiko wrote: > Ditto. Done. https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item_impl.cc:54: // fall-through On 2017/05/01 12:29:18, hidehiko wrote: > Not a fall-through here...? Removed https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item_impl.cc:56: NOTREACHED() << "Invalid Priority: " << android_priority; On 2017/05/01 12:29:18, hidehiko wrote: > Can this be out from switch, so some compiler captures missing case, > specifically if a new value is added to ArcNotificationPriority. > > switch (android_priority) { > case ...: > case ...: > > // no default. > } > > NOTREACHED() << ...; > return DEFAULT_PRIORITY; Done. https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item_impl.cc:192: void ArcNotificationItemImpl::OnImageDecoded(const SkBitmap& bitmap) { On 2017/05/01 12:29:18, hidehiko wrote: > No caller? Removed. https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... File ui/arc/notification/arc_notification_item_impl.h (right): https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item_impl.h:35: void OnClosedFromAndroid() override; On 2017/05/01 12:29:18, hidehiko wrote: > // ArcNotificationItem overrides: Done. https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item_impl.h:55: protected: On 2017/05/01 12:29:18, hidehiko wrote: > Because ArcNotificationItemImpl is no longer inherited, > this can be "private:" section? Done. https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item_impl.h:56: // Checks whether there is on-going |notification_|. On 2017/05/01 12:29:18, hidehiko wrote: > (optional): Also, several util functions can be inlined. Not necessary to > address in this CL, though. Removed unnecessary util methods https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item_impl.h:71: const AccountId& profile_id() const { return profile_id_; } On 2017/05/01 12:29:18, hidehiko wrote: > Also, do not need these accessors? Removed https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item_impl.h:87: bool pinned_ = false; On 2017/05/01 12:29:18, hidehiko wrote: > Comments for each field, please? Done. https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item_impl.h:95: base::ObserverList<Observer> observers_; On 2017/05/01 12:29:18, hidehiko wrote: > include base/observer_list.h ? Done.
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/ar... File ui/arc/notification/arc_notification_delegate.h (right): https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_notification_delegate.h:27: base::WeakPtr<ArcNotificationItem> item_; On 2017/05/08 06:24:52, yoshiki wrote: > On 2017/05/01 12:29:17, hidehiko wrote: > > You use WeakPtr here, but all methods assumes (DCHECKs) item_ is alive. > > When is WeakPtr actually needed? > > I want to keep the weak ptr to detect a potential use-after-free. I see. Would you mind to comment the goal of WeakPtr usage? https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... File ui/arc/notification/arc_notification_item_impl.cc (right): https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item_impl.cc:85: if (HasPendingNotification()) { Is this no longer needed? The original code still looks to have this? https://codereview.chromium.org/2845003002/diff/220001/ui/arc/notification/ar... File ui/arc/notification/arc_notification_delegate.h (right): https://codereview.chromium.org/2845003002/diff/220001/ui/arc/notification/ar... ui/arc/notification/arc_notification_delegate.h:9: #include "base/memory/ptr_util.h" Sorry, I overlooked in the last cycle. Looks unnecessary. https://codereview.chromium.org/2845003002/diff/220001/ui/arc/notification/ar... ui/arc/notification/arc_notification_delegate.h:11: #include "ui/arc/notification/arc_notification_item.h" Ditto. Can be forward decl, instead? https://codereview.chromium.org/2845003002/diff/220001/ui/arc/notification/ar... File ui/arc/notification/arc_notification_item.h (right): https://codereview.chromium.org/2845003002/diff/220001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item.h:55: // Add an observer. nit: Adds. https://codereview.chromium.org/2845003002/diff/220001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item.h:57: // Remove the observer. nit: Removes.
https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... File ui/arc/notification/arc_notification_item_impl.cc (right): https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item_impl.cc:85: if (HasPendingNotification()) { On 2017/05/08 09:34:41, hidehiko wrote: > Is this no longer needed? The original code still looks to have this? This was used by NON-CUSTOM arc notification, but is no longer needed now. Because we do no longer decode an image with custom arc notifications (decoding was done in a separated thread). Let me update the description to mention about this clean-up.
Description was changed from ========== 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 merge them into ArcNotificationItem. In addition, this CL splits the class into an interface and an implementation. BUG=697359 TEST=none ========== to ========== 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 removes the unused code in ArcNotificationItem and merges ArcCustomNotificationItem into ArcNotificationItem. In addition, this CL splits the class into an interface and an implementation. BUG=697359 TEST=none ==========
The CQ bit was checked by yoshiki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by yoshiki@chromium.org to run a CQ dry run
Patchset #4 (id:240001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
And I addressed comment. Thank you for reviewing! https://codereview.chromium.org/2845003002/diff/220001/ui/arc/notification/ar... File ui/arc/notification/arc_notification_delegate.h (right): https://codereview.chromium.org/2845003002/diff/220001/ui/arc/notification/ar... ui/arc/notification/arc_notification_delegate.h:9: #include "base/memory/ptr_util.h" On 2017/05/08 09:34:41, hidehiko wrote: > Sorry, I overlooked in the last cycle. Looks unnecessary. Done. https://codereview.chromium.org/2845003002/diff/220001/ui/arc/notification/ar... ui/arc/notification/arc_notification_delegate.h:11: #include "ui/arc/notification/arc_notification_item.h" On 2017/05/08 09:34:41, hidehiko wrote: > Ditto. Can be forward decl, instead? Done. https://codereview.chromium.org/2845003002/diff/220001/ui/arc/notification/ar... File ui/arc/notification/arc_notification_item.h (right): https://codereview.chromium.org/2845003002/diff/220001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item.h:55: // Add an observer. On 2017/05/08 09:34:41, hidehiko wrote: > nit: Adds. Done. https://codereview.chromium.org/2845003002/diff/220001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item.h:57: // Remove the observer. On 2017/05/08 09:34:41, hidehiko wrote: > nit: Removes. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
LGTM with minor comments. (but you'd need to rebase). https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... File ui/arc/notification/arc_notification_item_impl.cc (right): https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item_impl.cc:85: if (HasPendingNotification()) { On 2017/05/08 09:41:53, yoshiki wrote: > On 2017/05/08 09:34:41, hidehiko wrote: > > Is this no longer needed? The original code still looks to have this? > > This was used by NON-CUSTOM arc notification, but is no longer needed now. > Because we do no longer decode an image with custom arc notifications (decoding > was done in a separated thread). Let me update the description to mention about > this clean-up. Got it. Thank you for explanation. Description looks not updated yet. Please update it before submitting. https://codereview.chromium.org/2845003002/diff/260001/ui/arc/notification/ar... File ui/arc/notification/arc_notification_delegate.cc (right): https://codereview.chromium.org/2845003002/diff/260001/ui/arc/notification/ar... ui/arc/notification/arc_notification_delegate.cc:8: Missing #include "ui/arc/notification/arc_notification_item.h"
The CQ bit was checked by yoshiki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...)
Description was changed from ========== 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 removes the unused code in ArcNotificationItem and merges ArcCustomNotificationItem into ArcNotificationItem. In addition, this CL splits the class into an interface and an implementation. BUG=697359 TEST=none ========== to ========== 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 ==========
The CQ bit was checked by yoshiki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thank you! https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... File ui/arc/notification/arc_notification_item_impl.cc (right): https://codereview.chromium.org/2845003002/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item_impl.cc:85: if (HasPendingNotification()) { On 2017/05/08 10:12:39, hidehiko wrote: > On 2017/05/08 09:41:53, yoshiki wrote: > > On 2017/05/08 09:34:41, hidehiko wrote: > > > Is this no longer needed? The original code still looks to have this? > > > > This was used by NON-CUSTOM arc notification, but is no longer needed now. > > Because we do no longer decode an image with custom arc notifications > (decoding > > was done in a separated thread). Let me update the description to mention > about > > this clean-up. > > Got it. Thank you for explanation. > Description looks not updated yet. Please update it before submitting. Actually I had updated, but let me make it clearer and more detailed. https://codereview.chromium.org/2845003002/diff/260001/ui/arc/notification/ar... File ui/arc/notification/arc_notification_delegate.cc (right): https://codereview.chromium.org/2845003002/diff/260001/ui/arc/notification/ar... ui/arc/notification/arc_notification_delegate.cc:8: On 2017/05/08 10:12:39, hidehiko wrote: > Missing > #include "ui/arc/notification/arc_notification_item.h" Done.
The CQ bit was checked by yoshiki@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hidehiko@chromium.org Link to the patchset: https://codereview.chromium.org/2845003002/#ps300001 (title: "Rebased")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 300001, "attempt_start_ts": 1494253886459570, "parent_rev": "5e1fc76d3dd8959f41e6b5ebe4a0d0a38e0c2770", "commit_rev": "0a0ea6a0c083e27e574b2a872ce6b810d2c69360"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/0a0ea6a0c083e27e574b2a872ce6... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:300001) as https://chromium.googlesource.com/chromium/src/+/0a0ea6a0c083e27e574b2a872ce6... |