Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2935893004:
Add unittest for ArcNotificationContentView (Closed)
|
Created:
3 years, 6 months ago by yoshiki Modified:
3 years, 6 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, victorhsieh+watch_chromium.org, yusukes+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd unittest for ArcNotificationContentView
Major changes
- Add an unittest for ArcNotificationContentView
- Create a wrapper of NotificationSurface
- Separate the implementation of ArcNotificationSurfaceManager from the interface
BUG=697359
TEST=none
Review-Url: https://codereview.chromium.org/2935893004
Cr-Commit-Position: refs/heads/master@{#481473}
Committed: https://chromium.googlesource.com/chromium/src/+/5817e8d93be3f4224c921c1c1d0499d4e27c2508
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : Rebased #
Total comments: 41
Patch Set 4 : Addressed comment #
Total comments: 12
Patch Set 5 : Addressed comment #
Total comments: 2
Patch Set 6 : . #
Total comments: 3
Patch Set 7 : . #Messages
Total messages: 69 (53 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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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.
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...
Description was changed from ========== wip BUG= ========== to ========== Add unittest for ArcNotificationContentView Major changes - Add an unittest for ArcNotificationContentView - Create a wrapper of NotificationSurface - Separate the implementation of ArcNotificationSurfaceManager from the interface BUG=697359 TEST=none ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Patchset #2 (id:20001) has been deleted
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.
Patchset #1 (id:1) has been deleted
yoshiki@chromium.org changed reviewers: + edcourtney@chromium.org, hidehiko@chromium.org, yhanada@chromium.org
yoshiki@chromium.org changed required reviewers: + hidehiko@chromium.org
Hidehiko-san, PTAL. (Hanada-san, Eliot, FYI) Failures with the patch set #1 shouldn't be related with this CL.
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.
Yay more testing! Mostly cosmetic/style comments. https://codereview.chromium.org/2935893004/diff/80001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main_extra_parts_exo.h (right): https://codereview.chromium.org/2935893004/diff/80001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main_extra_parts_exo.h:35: std::unique_ptr<arc::ArcNotificationSurfaceManagerImpl> nit: unique_ptr<NotificationSurfaceManager> instead? https://codereview.chromium.org/2935893004/diff/80001/ui/arc/notification/arc... File ui/arc/notification/arc_notification_content_view.h (right): https://codereview.chromium.org/2935893004/diff/80001/ui/arc/notification/arc... ui/arc/notification/arc_notification_content_view.h:82: ArcNotificationSurfaceManager* GetSurfaceManager() const; nit: please move this into anonymous namespace in .cc https://codereview.chromium.org/2935893004/diff/80001/ui/arc/notification/arc... File ui/arc/notification/arc_notification_content_view_unittest.cc (right): https://codereview.chromium.org/2935893004/diff/80001/ui/arc/notification/arc... ui/arc/notification/arc_notification_content_view_unittest.cc:31: : notification_key_(notification_key), window_(std::move(window)) {} #include <utility> ? https://codereview.chromium.org/2935893004/diff/80001/ui/arc/notification/arc... ui/arc/notification/arc_notification_content_view_unittest.cc:57: std::string notification_key_; #include <string> ? https://codereview.chromium.org/2935893004/diff/80001/ui/arc/notification/arc... ui/arc/notification/arc_notification_content_view_unittest.cc:94: std::map<std::string, std::unique_ptr<ArcNotificationSurface>> surface_map_; #include <map> ? https://codereview.chromium.org/2935893004/diff/80001/ui/arc/notification/arc... ui/arc/notification/arc_notification_content_view_unittest.cc:95: static int surface_found_count_; Please make this non-static. https://codereview.chromium.org/2935893004/diff/80001/ui/arc/notification/arc... ui/arc/notification/arc_notification_content_view_unittest.cc:206: std::set<std::string> removed_ids_; #include <set> ? https://codereview.chromium.org/2935893004/diff/80001/ui/arc/notification/arc... ui/arc/notification/arc_notification_content_view_unittest.cc:225: surface_manager_.reset(new TestNotificationSurfaceManager()); nit: please use base::MakeUnique<>. Ditto for below. https://codereview.chromium.org/2935893004/diff/80001/ui/arc/notification/arc... ui/arc/notification/arc_notification_content_view_unittest.cc:227: views::ViewsTestBase::SetUp(); nit: in general, parent's SetUp() should be called at the beginning. If there is special reason, please comment it. https://codereview.chromium.org/2935893004/diff/80001/ui/arc/notification/arc... ui/arc/notification/arc_notification_content_view_unittest.cc:261: notification_view_.reset(); nit: notificatrion_item_.reset() and notification_delegate_ = nullptr are missing. https://codereview.chromium.org/2935893004/diff/80001/ui/arc/notification/arc... ui/arc/notification/arc_notification_content_view_unittest.cc:292: scoped_refptr<ArcNotificationDelegate> notification_delegate_; nit: Could you sort in the order of initialization? https://codereview.chromium.org/2935893004/diff/80001/ui/arc/notification/arc... File ui/arc/notification/arc_notification_surface.h (right): https://codereview.chromium.org/2935893004/diff/80001/ui/arc/notification/arc... ui/arc/notification/arc_notification_surface.h:41: // Attach the surface window to the native host. nit: s/Attach/Attaches/ https://codereview.chromium.org/2935893004/diff/80001/ui/arc/notification/arc... ui/arc/notification/arc_notification_surface.h:44: virtual void Detach() = 0; Doc? https://codereview.chromium.org/2935893004/diff/80001/ui/arc/notification/arc... ui/arc/notification/arc_notification_surface.h:46: virtual bool IsAttached() const = 0; Doc? https://codereview.chromium.org/2935893004/diff/80001/ui/arc/notification/arc... File ui/arc/notification/arc_notification_surface_impl.cc (right): https://codereview.chromium.org/2935893004/diff/80001/ui/arc/notification/arc... ui/arc/notification/arc_notification_surface_impl.cc:53: DCHECK(!surface_->window()->children().empty()); If above DCHECK fails (i.e. if surface_->window() is nullptr), this would crash. On the other hand, you handles |native_view_host_| == nullptr case to avoid the cras at L34. What is the policy? https://codereview.chromium.org/2935893004/diff/80001/ui/arc/notification/arc... File ui/arc/notification/arc_notification_surface_impl.h (right): https://codereview.chromium.org/2935893004/diff/80001/ui/arc/notification/arc... ui/arc/notification/arc_notification_surface_impl.h:33: exo::NotificationSurface* surface_ = nullptr; nit: s/ = nullptr// because you always set |surface| in ctor. https://codereview.chromium.org/2935893004/diff/80001/ui/arc/notification/arc... File ui/arc/notification/arc_notification_surface_manager_impl.cc (right): https://codereview.chromium.org/2935893004/diff/80001/ui/arc/notification/arc... ui/arc/notification/arc_notification_surface_manager_impl.cc:16: ArcNotificationSurfaceManagerImpl::ArcNotificationSurfaceManagerImpl() {} nit: s/{}/= default;/ please for consistency. (Please see ARC's style guide). https://codereview.chromium.org/2935893004/diff/80001/ui/arc/notification/arc... ui/arc/notification/arc_notification_surface_manager_impl.cc:36: auto it = notification_surface_map_.find(notification_key); nit/optional: you can avoid dup code? ArcNotificationSurface* arc_surface = GetArcSurface(notification_key); return arc_surface == nullptr ? nullptr : arc_surface->surface(); https://codereview.chromium.org/2935893004/diff/80001/ui/arc/notification/arc... ui/arc/notification/arc_notification_surface_manager_impl.cc:43: if (notification_surface_map_.find(surface->notification_key()) != Can you use auto result = map::insert(...); if (!result.second) { NOTREACHED(); return; } https://codereview.chromium.org/2935893004/diff/80001/ui/arc/notification/arc... ui/arc/notification/arc_notification_surface_manager_impl.cc:54: notification_surface_map_[surface->notification_key()].get()); Let's avoid searching map repeatedly. https://codereview.chromium.org/2935893004/diff/80001/ui/arc/notification/arc... File ui/arc/notification/arc_notification_surface_manager_impl.h (right): https://codereview.chromium.org/2935893004/diff/80001/ui/arc/notification/arc... ui/arc/notification/arc_notification_surface_manager_impl.h:42: std::map<std::string, std::unique_ptr<ArcNotificationSurfaceImpl>>; nit: unordered_map sounds more efficient?
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...
Patchset #4 (id:100001) has been deleted
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.
Patchset #4 (id:120001) has been deleted
Hidehiko-san, PTAL. Thanks. https://codereview.chromium.org/2935893004/diff/80001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main_extra_parts_exo.h (right): https://codereview.chromium.org/2935893004/diff/80001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main_extra_parts_exo.h:35: std::unique_ptr<arc::ArcNotificationSurfaceManagerImpl> On 2017/06/15 15:26:20, hidehiko wrote: > nit: unique_ptr<NotificationSurfaceManager> instead? It's impossible. The dtor of NotificationSurfaceManager is protected. https://codereview.chromium.org/2935893004/diff/80001/ui/arc/notification/arc... File ui/arc/notification/arc_notification_content_view.h (right): https://codereview.chromium.org/2935893004/diff/80001/ui/arc/notification/arc... ui/arc/notification/arc_notification_content_view.h:82: ArcNotificationSurfaceManager* GetSurfaceManager() const; On 2017/06/15 15:26:20, hidehiko wrote: > nit: please move this into anonymous namespace in .cc I completely removed this method. https://codereview.chromium.org/2935893004/diff/80001/ui/arc/notification/arc... File ui/arc/notification/arc_notification_content_view_unittest.cc (right): https://codereview.chromium.org/2935893004/diff/80001/ui/arc/notification/arc... ui/arc/notification/arc_notification_content_view_unittest.cc:31: : notification_key_(notification_key), window_(std::move(window)) {} On 2017/06/15 15:26:20, hidehiko wrote: > #include <utility> ? Done. https://codereview.chromium.org/2935893004/diff/80001/ui/arc/notification/arc... ui/arc/notification/arc_notification_content_view_unittest.cc:57: std::string notification_key_; On 2017/06/15 15:26:20, hidehiko wrote: > #include <string> ? Done. https://codereview.chromium.org/2935893004/diff/80001/ui/arc/notification/arc... ui/arc/notification/arc_notification_content_view_unittest.cc:94: std::map<std::string, std::unique_ptr<ArcNotificationSurface>> surface_map_; On 2017/06/15 15:26:20, hidehiko wrote: > #include <map> ? Done. https://codereview.chromium.org/2935893004/diff/80001/ui/arc/notification/arc... ui/arc/notification/arc_notification_content_view_unittest.cc:95: static int surface_found_count_; On 2017/06/15 15:26:20, hidehiko wrote: > Please make this non-static. This needs to be static, since this property is modified in const method. https://codereview.chromium.org/2935893004/diff/80001/ui/arc/notification/arc... ui/arc/notification/arc_notification_content_view_unittest.cc:206: std::set<std::string> removed_ids_; On 2017/06/15 15:26:20, hidehiko wrote: > #include <set> ? Done. https://codereview.chromium.org/2935893004/diff/80001/ui/arc/notification/arc... ui/arc/notification/arc_notification_content_view_unittest.cc:225: surface_manager_.reset(new TestNotificationSurfaceManager()); On 2017/06/15 15:26:20, hidehiko wrote: > nit: please use base::MakeUnique<>. > > Ditto for below. Thanks. I added some test and updated around here. https://codereview.chromium.org/2935893004/diff/80001/ui/arc/notification/arc... ui/arc/notification/arc_notification_content_view_unittest.cc:227: views::ViewsTestBase::SetUp(); On 2017/06/15 15:26:20, hidehiko wrote: > nit: in general, parent's SetUp() should be called at the beginning. > If there is special reason, please comment it. Done. https://codereview.chromium.org/2935893004/diff/80001/ui/arc/notification/arc... ui/arc/notification/arc_notification_content_view_unittest.cc:292: scoped_refptr<ArcNotificationDelegate> notification_delegate_; On 2017/06/15 15:26:20, hidehiko wrote: > nit: Could you sort in the order of initialization? Done. https://codereview.chromium.org/2935893004/diff/80001/ui/arc/notification/arc... File ui/arc/notification/arc_notification_surface.h (right): https://codereview.chromium.org/2935893004/diff/80001/ui/arc/notification/arc... ui/arc/notification/arc_notification_surface.h:41: // Attach the surface window to the native host. On 2017/06/15 15:26:20, hidehiko wrote: > nit: s/Attach/Attaches/ Done. https://codereview.chromium.org/2935893004/diff/80001/ui/arc/notification/arc... ui/arc/notification/arc_notification_surface.h:44: virtual void Detach() = 0; On 2017/06/15 15:26:20, hidehiko wrote: > Doc? Done. https://codereview.chromium.org/2935893004/diff/80001/ui/arc/notification/arc... ui/arc/notification/arc_notification_surface.h:46: virtual bool IsAttached() const = 0; On 2017/06/15 15:26:20, hidehiko wrote: > Doc? Done. https://codereview.chromium.org/2935893004/diff/80001/ui/arc/notification/arc... File ui/arc/notification/arc_notification_surface_impl.cc (right): https://codereview.chromium.org/2935893004/diff/80001/ui/arc/notification/arc... ui/arc/notification/arc_notification_surface_impl.cc:53: DCHECK(!surface_->window()->children().empty()); On 2017/06/15 15:26:20, hidehiko wrote: > If above DCHECK fails (i.e. if surface_->window() is nullptr), this would crash. > On the other hand, you handles |native_view_host_| == nullptr case to avoid the > cras at L34. What is the policy? Previously double-detach existed so we had null-check and return in Detach() method. But it's now solved so let me remove the code in Detach() (and add IsAttached() check in ArcNotificationContentService). https://codereview.chromium.org/2935893004/diff/80001/ui/arc/notification/arc... File ui/arc/notification/arc_notification_surface_impl.h (right): https://codereview.chromium.org/2935893004/diff/80001/ui/arc/notification/arc... ui/arc/notification/arc_notification_surface_impl.h:33: exo::NotificationSurface* surface_ = nullptr; On 2017/06/15 15:26:21, hidehiko wrote: > nit: s/ = nullptr// because you always set |surface| in ctor. Done. https://codereview.chromium.org/2935893004/diff/80001/ui/arc/notification/arc... File ui/arc/notification/arc_notification_surface_manager_impl.cc (right): https://codereview.chromium.org/2935893004/diff/80001/ui/arc/notification/arc... ui/arc/notification/arc_notification_surface_manager_impl.cc:16: ArcNotificationSurfaceManagerImpl::ArcNotificationSurfaceManagerImpl() {} On 2017/06/15 15:26:21, hidehiko wrote: > nit: s/{}/= default;/ please for consistency. (Please see ARC's style guide). Done. https://codereview.chromium.org/2935893004/diff/80001/ui/arc/notification/arc... ui/arc/notification/arc_notification_surface_manager_impl.cc:36: auto it = notification_surface_map_.find(notification_key); On 2017/06/15 15:26:21, hidehiko wrote: > nit/optional: you can avoid dup code? > ArcNotificationSurface* arc_surface = GetArcSurface(notification_key); > return arc_surface == nullptr ? nullptr : arc_surface->surface(); ArcNotificationSurface class doesn't have surface() method, so we can't use the result of GetArcSurface(). Of course, casting should be safe here, but I don't want to use it here. https://codereview.chromium.org/2935893004/diff/80001/ui/arc/notification/arc... ui/arc/notification/arc_notification_surface_manager_impl.cc:43: if (notification_surface_map_.find(surface->notification_key()) != On 2017/06/15 15:26:21, hidehiko wrote: > Can you use > > auto result = map::insert(...); > if (!result.second) { > NOTREACHED(); > return; > } Done. https://codereview.chromium.org/2935893004/diff/80001/ui/arc/notification/arc... ui/arc/notification/arc_notification_surface_manager_impl.cc:54: notification_surface_map_[surface->notification_key()].get()); On 2017/06/15 15:26:21, hidehiko wrote: > Let's avoid searching map repeatedly. Done. https://codereview.chromium.org/2935893004/diff/80001/ui/arc/notification/arc... File ui/arc/notification/arc_notification_surface_manager_impl.h (right): https://codereview.chromium.org/2935893004/diff/80001/ui/arc/notification/arc... ui/arc/notification/arc_notification_surface_manager_impl.h:42: std::map<std::string, std::unique_ptr<ArcNotificationSurfaceImpl>>; On 2017/06/15 15:26:21, hidehiko wrote: > nit: unordered_map sounds more efficient? Done.
Almost looks good. Minor comments only. https://codereview.chromium.org/2935893004/diff/140001/ui/arc/notification/ar... File ui/arc/notification/arc_notification_content_view_unittest.cc (right): https://codereview.chromium.org/2935893004/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_notification_content_view_unittest.cc:98: static int surface_found_count_; Then what you need is mutable, I think. https://google.github.io/styleguide/cppguide.html#Use_of_const https://codereview.chromium.org/2935893004/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_notification_content_view_unittest.cc:169: TestMessageCenterController() {} nit: = default; please. Ditto for below ctors and dtors. https://codereview.chromium.org/2935893004/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_notification_content_view_unittest.cc:234: void TearDown() override { nit/optional: Maybe calling CloseNotificationView for the case that some expectation failed? (with some change not to crash in case of wrapper_widget_ is nullptr) https://codereview.chromium.org/2935893004/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_notification_content_view_unittest.cc:276: new ArcNotificationDelegate(notification_item->GetWeakPtr()); nit: you can inline for consistency. https://codereview.chromium.org/2935893004/diff/140001/ui/arc/notification/ar... File ui/arc/notification/arc_notification_surface.h (right): https://codereview.chromium.org/2935893004/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_notification_surface.h:45: virtual void Detach() = 0; Please document that this must not be called if IsAttached() returns false. https://codereview.chromium.org/2935893004/diff/140001/ui/arc/notification/ar... File ui/arc/notification/arc_notification_surface_manager_impl.cc (right): https://codereview.chromium.org/2935893004/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_notification_surface_manager_impl.cc:54: for (auto& observer : observers_) { Please elide braces for one line body statement.
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.
Hidehiko-san, PTAL. Thanks. https://codereview.chromium.org/2935893004/diff/140001/ui/arc/notification/ar... File ui/arc/notification/arc_notification_content_view_unittest.cc (right): https://codereview.chromium.org/2935893004/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_notification_content_view_unittest.cc:98: static int surface_found_count_; On 2017/06/18 17:44:05, hidehiko wrote: > Then what you need is mutable, I think. > https://google.github.io/styleguide/cppguide.html#Use_of_const Done. https://codereview.chromium.org/2935893004/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_notification_content_view_unittest.cc:169: TestMessageCenterController() {} On 2017/06/18 17:44:05, hidehiko wrote: > nit: = default; please. > Ditto for below ctors and dtors. Done. https://codereview.chromium.org/2935893004/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_notification_content_view_unittest.cc:234: void TearDown() override { On 2017/06/18 17:44:05, hidehiko wrote: > nit/optional: Maybe calling CloseNotificationView for the case that some > expectation failed? (with some change not to crash in case of wrapper_widget_ is > nullptr) Added checks not to forget call CloseNotificationView. https://codereview.chromium.org/2935893004/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_notification_content_view_unittest.cc:276: new ArcNotificationDelegate(notification_item->GetWeakPtr()); On 2017/06/18 17:44:05, hidehiko wrote: > nit: you can inline for consistency. Done. https://codereview.chromium.org/2935893004/diff/140001/ui/arc/notification/ar... File ui/arc/notification/arc_notification_surface.h (right): https://codereview.chromium.org/2935893004/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_notification_surface.h:45: virtual void Detach() = 0; On 2017/06/18 17:44:05, hidehiko wrote: > Please document that this must not be called if IsAttached() returns false. Done. https://codereview.chromium.org/2935893004/diff/140001/ui/arc/notification/ar... File ui/arc/notification/arc_notification_surface_manager_impl.cc (right): https://codereview.chromium.org/2935893004/diff/140001/ui/arc/notification/ar... ui/arc/notification/arc_notification_surface_manager_impl.cc:54: for (auto& observer : observers_) { On 2017/06/18 17:44:05, hidehiko wrote: > Please elide braces for one line body statement. Done.
https://codereview.chromium.org/2935893004/diff/160001/ui/arc/notification/ar... File ui/arc/notification/arc_notification_content_view_unittest.cc (right): https://codereview.chromium.org/2935893004/diff/160001/ui/arc/notification/ar... ui/arc/notification/arc_notification_content_view_unittest.cc:98: mutable int surface_found_count_; Uninitialized memory. mutable int surface_found_count_ = 0;
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...
https://codereview.chromium.org/2935893004/diff/160001/ui/arc/notification/ar... File ui/arc/notification/arc_notification_content_view_unittest.cc (right): https://codereview.chromium.org/2935893004/diff/160001/ui/arc/notification/ar... ui/arc/notification/arc_notification_content_view_unittest.cc:98: mutable int surface_found_count_; On 2017/06/19 10:04:33, hidehiko wrote: > Uninitialized memory. > > mutable int surface_found_count_ = 0; Done.
LGTM, assuming all bots pass.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
yoshiki@chromium.org changed reviewers: + reveman@chromium.org, sky@chromium.org
PTAL. Thanks! sky@: chrome/browser/chrome_browser_main_extra_parts_exo.* reveman@: components/exo/surface.*
chrome/browser/chrome_browser_main_extra_parts_exo.* LGTM
https://codereview.chromium.org/2935893004/diff/180001/components/exo/surface.cc File components/exo/surface.cc (right): https://codereview.chromium.org/2935893004/diff/180001/components/exo/surface... components/exo/surface.cc:193: window_->SetName(kSurfaceWindowName); We're going to remove |window_| soon to support MUS. Is there some other way we can solve this? Can you add something to the NotificationSurface class?
https://codereview.chromium.org/2935893004/diff/180001/components/exo/surface.cc File components/exo/surface.cc (right): https://codereview.chromium.org/2935893004/diff/180001/components/exo/surface... components/exo/surface.cc:193: window_->SetName(kSurfaceWindowName); On 2017/06/21 15:16:36, reveman wrote: > We're going to remove |window_| soon to support MUS. Is there some other way we > can solve this? Can you add something to the NotificationSurface class? Although I'm not sure how you remove |window_|, the window name (kSurfaceWindowName) is just for DCHECK to ensure the window hierarchy. So, we can safely just remove this when you remove |window_|.
https://codereview.chromium.org/2935893004/diff/180001/components/exo/surface.cc File components/exo/surface.cc (right): https://codereview.chromium.org/2935893004/diff/180001/components/exo/surface... components/exo/surface.cc:193: window_->SetName(kSurfaceWindowName); On 2017/06/21 at 16:13:12, yoshiki wrote: > On 2017/06/21 15:16:36, reveman wrote: > > We're going to remove |window_| soon to support MUS. Is there some other way we > > can solve this? Can you add something to the NotificationSurface class? > > Although I'm not sure how you remove |window_|, the window name (kSurfaceWindowName) is just for DCHECK to ensure the window hierarchy. So, we can safely just remove this when you remove |window_|. Each surface will no longer have a window. Only the top-level delegate (e.g. ShellSurface) will have a window. If the notification code rely on this window then we need to start looking at how to avoid that asap. If it's only for a DCHECK that we need to remove soon then I prefer not making kSurfaceWindowName public.
On 2017/06/21 16:22:24, reveman wrote: > https://codereview.chromium.org/2935893004/diff/180001/components/exo/surface.cc > File components/exo/surface.cc (right): > > https://codereview.chromium.org/2935893004/diff/180001/components/exo/surface... > components/exo/surface.cc:193: window_->SetName(kSurfaceWindowName); > On 2017/06/21 at 16:13:12, yoshiki wrote: > > On 2017/06/21 15:16:36, reveman wrote: > > > We're going to remove |window_| soon to support MUS. Is there some other way > we > > > can solve this? Can you add something to the NotificationSurface class? > > > > Although I'm not sure how you remove |window_|, the window name > (kSurfaceWindowName) is just for DCHECK to ensure the window hierarchy. So, we > can safely just remove this when you remove |window_|. > > Each surface will no longer have a window. Only the top-level delegate (e.g. > ShellSurface) will have a window. If the notification code rely on this window > then we need to start looking at how to avoid that asap. Notification manipulates a transform matrix of window of Surface to scale notification: https://cs.chromium.org/chromium/src/ui/arc/notification/arc_notification_con... At least, we need to figure out another way to scale the surface. And let me confirm. You don't remove a window from NotificationSurface, right? A window of NotificationSurface is also used in the notification. If you have a plan to remove it, we need to consider alternative way for them. > If it's only for a DCHECK that we need to remove soon then I prefer not making > kSurfaceWindowName public. Ok, let me revert this change in surface.cc.
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_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
The patchset sent to the CQ was uploaded after l-g-t-m from hidehiko@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2935893004/#ps200001 (title: ".")
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": 200001, "attempt_start_ts": 1498114363130450,
"parent_rev": "fb12baa9349003273ebf3d9acfdecda482dfc595", "commit_rev":
"a8e21688787a8c2569b3519835e5b3f7adecb5e3"}
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1498114363130450,
"parent_rev": "3f4fde21fc4f7f0b38b9c1b861eb62add89a99f4", "commit_rev":
"5817e8d93be3f4224c921c1c1d0499d4e27c2508"}
Message was sent while issue was closed.
Description was changed from ========== Add unittest for ArcNotificationContentView Major changes - Add an unittest for ArcNotificationContentView - Create a wrapper of NotificationSurface - Separate the implementation of ArcNotificationSurfaceManager from the interface BUG=697359 TEST=none ========== to ========== Add unittest for ArcNotificationContentView Major changes - Add an unittest for ArcNotificationContentView - Create a wrapper of NotificationSurface - Separate the implementation of ArcNotificationSurfaceManager from the interface BUG=697359 TEST=none Review-Url: https://codereview.chromium.org/2935893004 Cr-Commit-Position: refs/heads/master@{#481473} Committed: https://chromium.googlesource.com/chromium/src/+/5817e8d93be3f4224c921c1c1d04... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:200001) as https://chromium.googlesource.com/chromium/src/+/5817e8d93be3f4224c921c1c1d04... |
