Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1782793002:
Ash: Implement Toasts (Closed)
DescriptionAsh: Implement Toasts
This patch adds the implementation of toast. Currently this is not used from anywhere. The client code will be added in separated patch.
BUG=b/25797993
Committed: https://crrev.com/32f516395c1d165053be9b93ef9d2cbdfd94685a
Cr-Commit-Position: refs/heads/master@{#383003}
Patch Set 1 #Patch Set 2 : . #
Total comments: 3
Patch Set 3 : Addressed comments #
Total comments: 26
Patch Set 4 : Fix build failure and misc #Patch Set 5 : Addressed comments #Patch Set 6 : Add comment #
Total comments: 12
Patch Set 7 : Addressed Comments and Added Tests #
Total comments: 20
Patch Set 8 : Addressed the comment and skipped failed tests on win #Patch Set 9 : Addressed comments and rebased #
Total comments: 2
Patch Set 10 : addressed the comment #
Messages
Total messages: 39 (19 generated)
Description was changed from ========== wip: implement toast support BUG= ========== to ========== wip: implement toast support BUG= ==========
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Description was changed from ========== wip: implement toast support BUG= ========== to ========== Ash: Implement Toasts BUG=b/25797993 ==========
yoshiki@chromium.org changed reviewers: + oshima@chromium.org
Patchset #2 (id:60001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #2 (id:100001) has been deleted
Description was changed from ========== Ash: Implement Toasts BUG=b/25797993 ========== to ========== Ash: Implement Toasts This patch adds the implementation of toast. Currently, this is not used from anywhere. The client code will be added in separated patch. BUG=b/25797993 ==========
Description was changed from ========== Ash: Implement Toasts This patch adds the implementation of toast. Currently, this is not used from anywhere. The client code will be added in separated patch. BUG=b/25797993 ========== to ========== Ash: Implement Toasts This patch adds the implementation of toast. Currently this is not used from anywhere. The client code will be added in separated patch. BUG=b/25797993 ==========
Oshima-san, PTAL. Thanks.
can you use ui/wm/core/window_animations.h instead of implementing your own? https://codereview.chromium.org/1782793002/diff/120001/ash/system/toast/toast... File ash/system/toast/toast_manager.cc (right): https://codereview.chromium.org/1782793002/diff/120001/ash/system/toast/toast... ash/system/toast/toast_manager.cc:62: base::ThreadTaskRunnerHandle::Get()->PostDelayedTask( Won't the posted task outlive ash?
Patchset #3 (id:140001) has been deleted
Patchset #3 (id:160001) has been deleted
Patchset #3 (id:180001) has been deleted
Oshima-san, PTAL. Thanks. https://codereview.chromium.org/1782793002/diff/120001/ash/system/toast/toast... File ash/system/toast/toast_manager.cc (right): https://codereview.chromium.org/1782793002/diff/120001/ash/system/toast/toast... ash/system/toast/toast_manager.cc:62: base::ThreadTaskRunnerHandle::Get()->PostDelayedTask( On 2016/03/11 02:06:29, oshima wrote: > Won't the posted task outlive ash? Done. Now, Ash::Shell has an instance. https://codereview.chromium.org/1782793002/diff/120001/ash/system/toast/toast... File ash/system/toast/toast_overlay.cc (right): https://codereview.chromium.org/1782793002/diff/120001/ash/system/toast/toast... ash/system/toast/toast_overlay.cc:86: SetTextColor(STATE_NORMAL, SkColorSetARGB(0xFF, 0x64, 0xA5, 0xF5)); FYI: These color values might be changed (I'm asking UX). Please review the rest part. Thanks.
I'll check the ownership stuff later. https://codereview.chromium.org/1782793002/diff/200001/ash/shell.h File ash/shell.h (right): https://codereview.chromium.org/1782793002/diff/200001/ash/shell.h#newcode586 ash/shell.h:586: ToastManager* toast_manager() const { return toast_manager_.get(); } remove const (because it returns non const member) https://codereview.chromium.org/1782793002/diff/200001/ash/system/toast/toast... File ash/system/toast/toast_manager.cc (right): https://codereview.chromium.org/1782793002/diff/200001/ash/system/toast/toast... ash/system/toast/toast_manager.cc:16: uint64_t kMinimumDurationMs = 200; new line after this https://codereview.chromium.org/1782793002/diff/200001/ash/system/toast/toast... ash/system/toast/toast_manager.cc:17: } } // namespace https://codereview.chromium.org/1782793002/diff/200001/ash/system/toast/toast... ash/system/toast/toast_manager.cc:33: DCHECK(thread_checker_.CalledOnValidThread()); Since this isn't multi threaded, you can remove these check (you may keep it if you want) https://codereview.chromium.org/1782793002/diff/200001/ash/system/toast/toast... File ash/system/toast/toast_manager.h (right): https://codereview.chromium.org/1782793002/diff/200001/ash/system/toast/toast... ash/system/toast/toast_manager.h:19: class ToastManagerTest; friend class decl below should be enough. https://codereview.chromium.org/1782793002/diff/200001/ash/system/toast/toast... ash/system/toast/toast_manager.h:21: class ASH_EXPORT ToastManager : public ToastOverlay::Delegate, document the class (explain Show will queue the text) https://codereview.chromium.org/1782793002/diff/200001/ash/system/toast/toast... ash/system/toast/toast_manager.h:28: // ToastOverlay::Delegate https://codereview.chromium.org/1782793002/diff/200001/ash/system/toast/toast... ash/system/toast/toast_manager.h:32: friend struct base::DefaultSingletonTraits<ToastManager>; remove this? https://codereview.chromium.org/1782793002/diff/200001/ash/system/toast/toast... File ash/system/toast/toast_manager_unittest.cc (right): https://codereview.chromium.org/1782793002/diff/200001/ash/system/toast/toast... ash/system/toast/toast_manager_unittest.cc:36: void TearDown() override { test::AshTestBase::TearDown(); } this isn't necessary https://codereview.chromium.org/1782793002/diff/200001/ash/system/toast/toast... ash/system/toast/toast_manager_unittest.cc:41: int GetToastId() const { return manager_->toast_id_for_testing(); } new line https://codereview.chromium.org/1782793002/diff/200001/ash/system/toast/toast... ash/system/toast/toast_manager_unittest.cc:44: } new line https://codereview.chromium.org/1782793002/diff/200001/ash/system/toast/toast... ash/system/toast/toast_manager_unittest.cc:75: } can you also add the cases where messages are queued? https://codereview.chromium.org/1782793002/diff/200001/ash/system/toast/toast... File ash/system/toast/toast_overlay.cc (right): https://codereview.chromium.org/1782793002/diff/200001/ash/system/toast/toast... ash/system/toast/toast_overlay.cc:80: } do you need this?
Oshima-san, PTAL. https://codereview.chromium.org/1782793002/diff/200001/ash/shell.h File ash/shell.h (right): https://codereview.chromium.org/1782793002/diff/200001/ash/shell.h#newcode586 ash/shell.h:586: ToastManager* toast_manager() const { return toast_manager_.get(); } On 2016/03/11 09:40:24, oshima wrote: > remove const (because it returns non const member) Done. https://codereview.chromium.org/1782793002/diff/200001/ash/system/toast/toast... File ash/system/toast/toast_manager.cc (right): https://codereview.chromium.org/1782793002/diff/200001/ash/system/toast/toast... ash/system/toast/toast_manager.cc:16: uint64_t kMinimumDurationMs = 200; On 2016/03/11 09:40:24, oshima wrote: > new line after this Done. https://codereview.chromium.org/1782793002/diff/200001/ash/system/toast/toast... ash/system/toast/toast_manager.cc:17: } On 2016/03/11 09:40:24, oshima wrote: > } // namespace Done. https://codereview.chromium.org/1782793002/diff/200001/ash/system/toast/toast... ash/system/toast/toast_manager.cc:33: DCHECK(thread_checker_.CalledOnValidThread()); On 2016/03/11 09:40:24, oshima wrote: > Since this isn't multi threaded, you can remove these check (you may keep it if > you want) Done. https://codereview.chromium.org/1782793002/diff/200001/ash/system/toast/toast... File ash/system/toast/toast_manager.h (right): https://codereview.chromium.org/1782793002/diff/200001/ash/system/toast/toast... ash/system/toast/toast_manager.h:19: class ToastManagerTest; On 2016/03/11 09:40:24, oshima wrote: > friend class decl below should be enough. Done. https://codereview.chromium.org/1782793002/diff/200001/ash/system/toast/toast... ash/system/toast/toast_manager.h:21: class ASH_EXPORT ToastManager : public ToastOverlay::Delegate, On 2016/03/11 09:40:24, oshima wrote: > document the class (explain Show will queue the text) Done. https://codereview.chromium.org/1782793002/diff/200001/ash/system/toast/toast... ash/system/toast/toast_manager.h:28: On 2016/03/11 09:40:24, oshima wrote: > // ToastOverlay::Delegate Done. https://codereview.chromium.org/1782793002/diff/200001/ash/system/toast/toast... ash/system/toast/toast_manager.h:32: friend struct base::DefaultSingletonTraits<ToastManager>; On 2016/03/11 09:40:24, oshima wrote: > remove this? Done. https://codereview.chromium.org/1782793002/diff/200001/ash/system/toast/toast... File ash/system/toast/toast_manager_unittest.cc (right): https://codereview.chromium.org/1782793002/diff/200001/ash/system/toast/toast... ash/system/toast/toast_manager_unittest.cc:36: void TearDown() override { test::AshTestBase::TearDown(); } On 2016/03/11 09:40:24, oshima wrote: > this isn't necessary Done. https://codereview.chromium.org/1782793002/diff/200001/ash/system/toast/toast... ash/system/toast/toast_manager_unittest.cc:41: int GetToastId() const { return manager_->toast_id_for_testing(); } On 2016/03/11 09:40:24, oshima wrote: > new line Done. https://codereview.chromium.org/1782793002/diff/200001/ash/system/toast/toast... ash/system/toast/toast_manager_unittest.cc:44: } On 2016/03/11 09:40:24, oshima wrote: > new line Done. https://codereview.chromium.org/1782793002/diff/200001/ash/system/toast/toast... ash/system/toast/toast_manager_unittest.cc:75: } On 2016/03/11 09:40:24, oshima wrote: > can you also add the cases where messages are queued? Done. https://codereview.chromium.org/1782793002/diff/200001/ash/system/toast/toast... File ash/system/toast/toast_overlay.cc (right): https://codereview.chromium.org/1782793002/diff/200001/ash/system/toast/toast... ash/system/toast/toast_overlay.cc:80: } On 2016/03/11 09:40:24, oshima wrote: > do you need this? This is because the method in the parent class is protected. I removed this and make the user a friend.
Patchset #6 (id:260001) has been deleted
looks good. Can you update the bounds computation and test it using --ash-host-window-bounds=1000x500,0+600-100x500 --ash-enable-unified-desktop? https://codereview.chromium.org/1782793002/diff/280001/ash/shell.cc File ash/shell.cc (right): https://codereview.chromium.org/1782793002/diff/280001/ash/shell.cc#newcode765 ash/shell.cc:765: toast_manager_.reset(); can you move this to the reverse order of the creation? You may not be able to find the exact reverse order, but just try to be roughly the reverse order. https://codereview.chromium.org/1782793002/diff/280001/ash/system/toast/toast... File ash/system/toast/toast_manager.h (right): https://codereview.chromium.org/1782793002/diff/280001/ash/system/toast/toast... ash/system/toast/toast_manager.h:39: ToastOverlay* GetCurrentOverlayForTesting() const { return overlay_.get(); } nit: remove const https://codereview.chromium.org/1782793002/diff/280001/ash/system/toast/toast... File ash/system/toast/toast_manager_unittest.cc (right): https://codereview.chromium.org/1782793002/diff/280001/ash/system/toast/toast... ash/system/toast/toast_manager_unittest.cc:41: ToastOverlay* GetCurrentOverlay() const { ditto https://codereview.chromium.org/1782793002/diff/280001/ash/system/toast/toast... ash/system/toast/toast_manager_unittest.cc:83: base::RunLoop().RunUntilIdle(); can you check each step is showing the correct toast? https://codereview.chromium.org/1782793002/diff/280001/ash/system/toast/toast... File ash/system/toast/toast_overlay.cc (right): https://codereview.chromium.org/1782793002/diff/280001/ash/system/toast/toast... ash/system/toast/toast_overlay.cc:125: set_owned_by_client(); this was necessary because you store the view in scoped_ptr, and that's not uncommon for content view. just FYI. https://codereview.chromium.org/1782793002/diff/280001/ash/system/toast/toast... ash/system/toast/toast_overlay.cc:231: : root_bounds.height(); Can you use ScreenUtil::GetShelfDisplayBoundsInRoot(primary_rooot) to find out the position instead? It'll work better in unified desktop mode.
Oshima-san, PTAL. Thanks. https://codereview.chromium.org/1782793002/diff/280001/ash/shell.cc File ash/shell.cc (right): https://codereview.chromium.org/1782793002/diff/280001/ash/shell.cc#newcode765 ash/shell.cc:765: toast_manager_.reset(); On 2016/03/15 09:07:32, oshima wrote: > can you move this to the reverse order of the creation? > You may not be able to find the exact reverse order, but just try to be roughly > the reverse order. Done. https://codereview.chromium.org/1782793002/diff/280001/ash/system/toast/toast... File ash/system/toast/toast_manager.h (right): https://codereview.chromium.org/1782793002/diff/280001/ash/system/toast/toast... ash/system/toast/toast_manager.h:39: ToastOverlay* GetCurrentOverlayForTesting() const { return overlay_.get(); } On 2016/03/15 09:07:32, oshima wrote: > nit: remove const Done. https://codereview.chromium.org/1782793002/diff/280001/ash/system/toast/toast... File ash/system/toast/toast_manager_unittest.cc (right): https://codereview.chromium.org/1782793002/diff/280001/ash/system/toast/toast... ash/system/toast/toast_manager_unittest.cc:41: ToastOverlay* GetCurrentOverlay() const { On 2016/03/15 09:07:33, oshima wrote: > ditto Done. https://codereview.chromium.org/1782793002/diff/280001/ash/system/toast/toast... ash/system/toast/toast_manager_unittest.cc:83: base::RunLoop().RunUntilIdle(); On 2016/03/15 09:07:32, oshima wrote: > can you check each step is showing the correct toast? Done. https://codereview.chromium.org/1782793002/diff/280001/ash/system/toast/toast... File ash/system/toast/toast_overlay.cc (right): https://codereview.chromium.org/1782793002/diff/280001/ash/system/toast/toast... ash/system/toast/toast_overlay.cc:125: set_owned_by_client(); On 2016/03/15 09:07:33, oshima wrote: > this was necessary because you store the view in scoped_ptr, > and that's not uncommon for content view. just FYI. Thanks! Done. https://codereview.chromium.org/1782793002/diff/280001/ash/system/toast/toast... ash/system/toast/toast_overlay.cc:231: : root_bounds.height(); On 2016/03/15 09:07:33, oshima wrote: > Can you use ScreenUtil::GetShelfDisplayBoundsInRoot(primary_rooot) to find out > the position instead? It'll work better in unified desktop mode. I uses ShelfLayoutManager::user_work_area_bounds() instead. And Added tests with ScreenUtil::GetShelfDisplayBoundsInRoot(primary_root).
very close thank you for adding test. can you look into win bot failure? https://codereview.chromium.org/1782793002/diff/300001/ash/system/toast/toast... File ash/system/toast/toast_manager.cc (right): https://codereview.chromium.org/1782793002/diff/300001/ash/system/toast/toast... ash/system/toast/toast_manager.cc:53: base::Unretained(this), // |this| is never destroyed. Use weak ptr and update the comment. https://codereview.chromium.org/1782793002/diff/300001/ash/system/toast/toast... File ash/system/toast/toast_manager_unittest.cc (right): https://codereview.chromium.org/1782793002/diff/300001/ash/system/toast/toast... ash/system/toast/toast_manager_unittest.cc:41: ToastManager* manager() const { return manager_; } remove const https://codereview.chromium.org/1782793002/diff/300001/ash/system/toast/toast... ash/system/toast/toast_manager_unittest.cc:56: return overlay ? overlay->text_ : ""; std::string() https://codereview.chromium.org/1782793002/diff/300001/ash/system/toast/toast... File ash/system/toast/toast_overlay.cc (right): https://codereview.chromium.org/1782793002/diff/300001/ash/system/toast/toast... ash/system/toast/toast_overlay.cc:64: SetEnabledColor(SkColorSetARGB(0xFF, 0xFF, 0xFF, 0xFF)); Sk_ColorWHITE https://codereview.chromium.org/1782793002/diff/300001/ash/system/toast/toast... ash/system/toast/toast_overlay.cc:80: friend class ToastOverlay; // To call NotifyClick(). this isn't necessary anymore? https://codereview.chromium.org/1782793002/diff/300001/ash/system/toast/toast... ash/system/toast/toast_overlay.cc:86: views::ButtonListener* listener) nit: keep the same arg order of button. https://codereview.chromium.org/1782793002/diff/300001/ash/system/toast/toast... ash/system/toast/toast_overlay.cc:92: SetTextColor(STATE_PRESSED, SkColorSetARGB(0xFF, 0x64, 0xA5, 0xF5)); define these colors as const. https://codereview.chromium.org/1782793002/diff/300001/ash/system/toast/toast... ash/system/toast/toast_overlay.cc:151: paint.setColor(SkColorSetARGB(0xFF, 0x32, 0x32, 0x32)); ditto https://codereview.chromium.org/1782793002/diff/300001/ash/system/toast/toast... ash/system/toast/toast_overlay.cc:227: gfx::Rect work_area_bounds = shelf_layout_manager->user_work_area_bounds(); it's probably cleaner to use work_area_bounds.ClampToCenteredSize(widget_size_); then adjust the vertical position.
> thank you for adding test. can you look into win bot failure? The cause of failure is that the following 2 methods return different results only in the win bot. - ScreenUtil::GetShelfDisplayBoundsInRoot(Shell::GetPrimaryRootWindow()) - ScreenUtil::GetDisplayBoundsInParent(shelf_layout_view->shelf_widget()->GetNativeView) I think they should return same result. Is it a bug on win ash?
On 2016/03/16 15:56:28, yoshiki wrote: > > thank you for adding test. can you look into win bot failure? > > The cause of failure is that the following 2 methods return different results > only in the win bot. > - ScreenUtil::GetShelfDisplayBoundsInRoot(Shell::GetPrimaryRootWindow()) > - > ScreenUtil::GetDisplayBoundsInParent(shelf_layout_view->shelf_widget()->GetNativeView) > > I think they should return same result. Is it a bug on win ash? Win ash's host window can't be resized by design and that's probably why. You can just skip by checking SupportsHostWindowResize()
Patchset #8 (id:320001) has been deleted
On 2016/03/16 18:00:02, oshima wrote: > On 2016/03/16 15:56:28, yoshiki wrote: > > > thank you for adding test. can you look into win bot failure? > > > > The cause of failure is that the following 2 methods return different results > > only in the win bot. > > - ScreenUtil::GetShelfDisplayBoundsInRoot(Shell::GetPrimaryRootWindow()) > > - > > > ScreenUtil::GetDisplayBoundsInParent(shelf_layout_view->shelf_widget()->GetNativeView) > > > > I think they should return same result. Is it a bug on win ash? > > Win ash's host window can't be resized by design and that's probably why. You > can just skip by checking SupportsHostWindowResize() I skip some checking. PTAL. Thanks.
https://codereview.chromium.org/1782793002/diff/300001/ash/system/toast/toast... File ash/system/toast/toast_manager.cc (right): https://codereview.chromium.org/1782793002/diff/300001/ash/system/toast/toast... ash/system/toast/toast_manager.cc:53: base::Unretained(this), // |this| is never destroyed. On 2016/03/15 19:03:19, oshima wrote: > Use weak ptr and update the comment. Done. https://codereview.chromium.org/1782793002/diff/300001/ash/system/toast/toast... File ash/system/toast/toast_manager_unittest.cc (right): https://codereview.chromium.org/1782793002/diff/300001/ash/system/toast/toast... ash/system/toast/toast_manager_unittest.cc:41: ToastManager* manager() const { return manager_; } On 2016/03/15 19:03:19, oshima wrote: > remove const Done. https://codereview.chromium.org/1782793002/diff/300001/ash/system/toast/toast... ash/system/toast/toast_manager_unittest.cc:56: return overlay ? overlay->text_ : ""; On 2016/03/15 19:03:19, oshima wrote: > std::string() Done. https://codereview.chromium.org/1782793002/diff/300001/ash/system/toast/toast... File ash/system/toast/toast_overlay.cc (right): https://codereview.chromium.org/1782793002/diff/300001/ash/system/toast/toast... ash/system/toast/toast_overlay.cc:64: SetEnabledColor(SkColorSetARGB(0xFF, 0xFF, 0xFF, 0xFF)); On 2016/03/15 19:03:19, oshima wrote: > Sk_ColorWHITE Done. https://codereview.chromium.org/1782793002/diff/300001/ash/system/toast/toast... ash/system/toast/toast_overlay.cc:80: friend class ToastOverlay; // To call NotifyClick(). On 2016/03/15 19:03:19, oshima wrote: > this isn't necessary anymore? No, it uses in test, which checks dismiss button. https://codereview.chromium.org/1782793002/diff/300001/ash/system/toast/toast... ash/system/toast/toast_overlay.cc:86: views::ButtonListener* listener) On 2016/03/15 19:03:19, oshima wrote: > nit: keep the same arg order of button. Done. https://codereview.chromium.org/1782793002/diff/300001/ash/system/toast/toast... ash/system/toast/toast_overlay.cc:92: SetTextColor(STATE_PRESSED, SkColorSetARGB(0xFF, 0x64, 0xA5, 0xF5)); On 2016/03/15 19:03:19, oshima wrote: > define these colors as const. Done. https://codereview.chromium.org/1782793002/diff/300001/ash/system/toast/toast... ash/system/toast/toast_overlay.cc:151: paint.setColor(SkColorSetARGB(0xFF, 0x32, 0x32, 0x32)); On 2016/03/15 19:03:19, oshima wrote: > ditto Done. https://codereview.chromium.org/1782793002/diff/300001/ash/system/toast/toast... ash/system/toast/toast_overlay.cc:227: gfx::Rect work_area_bounds = shelf_layout_manager->user_work_area_bounds(); On 2016/03/15 19:03:19, oshima wrote: > it's probably cleaner to use > > work_area_bounds.ClampToCenteredSize(widget_size_); > > then adjust the vertical position. I made it simpler. I think it's better than clamping and adjusting y-pos and height.
https://codereview.chromium.org/1782793002/diff/300001/ash/system/toast/toast... File ash/system/toast/toast_overlay.cc (right): https://codereview.chromium.org/1782793002/diff/300001/ash/system/toast/toast... ash/system/toast/toast_overlay.cc:227: gfx::Rect work_area_bounds = shelf_layout_manager->user_work_area_bounds(); On 2016/03/17 07:59:11, yoshiki wrote: > On 2016/03/15 19:03:19, oshima wrote: > > it's probably cleaner to use > > > > work_area_bounds.ClampToCenteredSize(widget_size_); > > > > then adjust the vertical position. > > I made it simpler. I think it's better than clamping and adjusting y-pos and > height. It'll look like gfx::Rect bounds = shelf_layout_manager->user_work_area_bounds(); int target_y = bounds.bottom() - widget_size_.height() - kVerticalOffset; bounds.ClampToCenteredSize(widget_size_); bounds.set_y(target_y); return bounds; which looks more readable to me.
Oshima-san, PTAL. Thanks. https://codereview.chromium.org/1782793002/diff/300001/ash/system/toast/toast... File ash/system/toast/toast_overlay.cc (right): https://codereview.chromium.org/1782793002/diff/300001/ash/system/toast/toast... ash/system/toast/toast_overlay.cc:227: gfx::Rect work_area_bounds = shelf_layout_manager->user_work_area_bounds(); On 2016/03/17 17:50:06, oshima wrote: > On 2016/03/17 07:59:11, yoshiki wrote: > > On 2016/03/15 19:03:19, oshima wrote: > > > it's probably cleaner to use > > > > > > work_area_bounds.ClampToCenteredSize(widget_size_); > > > > > > then adjust the vertical position. > > > > I made it simpler. I think it's better than clamping and adjusting y-pos and > > height. > It'll look like > > gfx::Rect bounds = shelf_layout_manager->user_work_area_bounds(); > int target_y = bounds.bottom() - widget_size_.height() - kVerticalOffset; > bounds.ClampToCenteredSize(widget_size_); > bounds.set_y(target_y); > return bounds; > > which looks more readable to me. > Done.
lgmt with one nit. https://codereview.chromium.org/1782793002/diff/360001/ash/system/toast/toast... File ash/system/toast/toast_overlay.cc (right): https://codereview.chromium.org/1782793002/diff/360001/ash/system/toast/toast... ash/system/toast/toast_overlay.cc:240: bounds.set_height(widget_size_.height()); nit: you don't need this.
On 2016/03/17 21:47:22, oshima wrote: > lgmt with one nit. > > https://codereview.chromium.org/1782793002/diff/360001/ash/system/toast/toast... > File ash/system/toast/toast_overlay.cc (right): > > https://codereview.chromium.org/1782793002/diff/360001/ash/system/toast/toast... > ash/system/toast/toast_overlay.cc:240: bounds.set_height(widget_size_.height()); > nit: you don't need this. oops, sorry for typo. lgtm with one nit above.
https://codereview.chromium.org/1782793002/diff/360001/ash/system/toast/toast... File ash/system/toast/toast_overlay.cc (right): https://codereview.chromium.org/1782793002/diff/360001/ash/system/toast/toast... ash/system/toast/toast_overlay.cc:240: bounds.set_height(widget_size_.height()); On 2016/03/17 21:47:22, oshima wrote: > nit: you don't need this. Thanks. You're right.
The CQ bit was checked by yoshiki@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org Link to the patchset: https://codereview.chromium.org/1782793002/#ps380001 (title: "addressed the comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1782793002/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1782793002/380001
Message was sent while issue was closed.
Description was changed from ========== Ash: Implement Toasts This patch adds the implementation of toast. Currently this is not used from anywhere. The client code will be added in separated patch. BUG=b/25797993 ========== to ========== Ash: Implement Toasts This patch adds the implementation of toast. Currently this is not used from anywhere. The client code will be added in separated patch. BUG=b/25797993 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:380001)
Message was sent while issue was closed.
Description was changed from ========== Ash: Implement Toasts This patch adds the implementation of toast. Currently this is not used from anywhere. The client code will be added in separated patch. BUG=b/25797993 ========== to ========== Ash: Implement Toasts This patch adds the implementation of toast. Currently this is not used from anywhere. The client code will be added in separated patch. BUG=b/25797993 Committed: https://crrev.com/32f516395c1d165053be9b93ef9d2cbdfd94685a Cr-Commit-Position: refs/heads/master@{#383003} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/32f516395c1d165053be9b93ef9d2cbdfd94685a Cr-Commit-Position: refs/heads/master@{#383003} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
