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

Issue 2969603003: Implement new-style image notification resizing. (Closed)

Created:
3 years, 5 months ago by tetsui
Modified:
3 years, 4 months ago
CC:
chromium-reviews, Peter Beverloo, mlamouri+watch-notifications_chromium.org, awdf+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Implement new-style image notification resizing. In new-style notification, the resizing behavior of image notification is different form the non-MD notification. This CL adds LargeImageView, which implements the specific resizing behavior. BUG=737850 TEST=manual Review-Url: https://codereview.chromium.org/2969603003 Cr-Commit-Position: refs/heads/master@{#491992} Committed: https://chromium.googlesource.com/chromium/src/+/996de39b52e4392d01859e4343a916be940d456b

Patch Set 1 #

Patch Set 2 : Adjust height to follow the mock. #

Total comments: 7

Patch Set 3 : Implement Android like resizing. #

Patch Set 4 : Revert and rebase. #

Patch Set 5 : Fix memory leak. #

Total comments: 4

Patch Set 6 : Resolve review comments and fix test failure. #

Patch Set 7 : Rebase. #

Patch Set 8 : Rebase. #

Total comments: 2

Patch Set 9 : Add LargeImageContainerView. #

Patch Set 10 : Fix unit test. #

Total comments: 4

Patch Set 11 : Fix nits. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+136 lines, -38 lines) Patch
M ui/message_center/views/notification_view_md.h View 1 2 3 4 5 6 7 8 2 chunks +2 lines, -2 lines 0 comments Download
M ui/message_center/views/notification_view_md.cc View 1 2 3 4 5 6 7 8 9 10 8 chunks +132 lines, -34 lines 0 comments Download
M ui/message_center/views/notification_view_md_unittest.cc View 1 2 3 4 5 6 7 8 9 2 chunks +2 lines, -2 lines 0 comments Download

Messages

Total messages: 64 (50 generated)
tetsui
PTAL. Thanks! https://codereview.chromium.org/2969603003/diff/20001/ui/message_center/views/notification_view_md.cc File ui/message_center/views/notification_view_md.cc (right): https://codereview.chromium.org/2969603003/diff/20001/ui/message_center/views/notification_view_md.cc#newcode56 ui/message_center/views/notification_view_md.cc:56: constexpr gfx::Size kLargeImageMaxSize(328, 218); In new-style notification ...
3 years, 5 months ago (2017-07-03 03:51:25 UTC) #6
johnme
Drive-by: this resizing is very different from how Android does it, in particular for vertical ...
3 years, 5 months ago (2017-07-03 17:30:32 UTC) #11
tetsui
On 2017/07/03 17:30:32, johnme wrote: > Drive-by: this resizing is very different from how Android ...
3 years, 5 months ago (2017-07-04 01:03:32 UTC) #13
tetsui
yoshiki@: I implemented Android-like resizing. How about landing this CL first and then modifying later ...
3 years, 5 months ago (2017-07-19 08:25:55 UTC) #18
tetsui
Sebastien clarified that we are going to use the resizing behavior in the mock here, ...
3 years, 4 months ago (2017-07-28 04:55:39 UTC) #28
yoshiki
https://codereview.chromium.org/2969603003/diff/20001/ui/message_center/views/notification_view_md.cc File ui/message_center/views/notification_view_md.cc (right): https://codereview.chromium.org/2969603003/diff/20001/ui/message_center/views/notification_view_md.cc#newcode336 ui/message_center/views/notification_view_md.cc:336: views::View* container_ = nullptr; nit: please add a comment ...
3 years, 4 months ago (2017-07-28 06:13:04 UTC) #35
tetsui
https://codereview.chromium.org/2969603003/diff/20001/ui/message_center/views/notification_view_md.cc File ui/message_center/views/notification_view_md.cc (right): https://codereview.chromium.org/2969603003/diff/20001/ui/message_center/views/notification_view_md.cc#newcode336 ui/message_center/views/notification_view_md.cc:336: views::View* container_ = nullptr; On 2017/07/28 06:13:03, yoshiki wrote: ...
3 years, 4 months ago (2017-07-28 06:38:45 UTC) #38
tetsui
yoshiki@: friendly ping
3 years, 4 months ago (2017-08-04 06:42:12 UTC) #45
Yoshiki IGUCHI
https://codereview.chromium.org/2969603003/diff/160001/ui/message_center/views/notification_view_md.cc File ui/message_center/views/notification_view_md.cc (right): https://codereview.chromium.org/2969603003/diff/160001/ui/message_center/views/notification_view_md.cc#newcode285 ui/message_center/views/notification_view_md.cc:285: views::View* const container_; It's not good that a child ...
3 years, 4 months ago (2017-08-04 07:00:33 UTC) #47
tetsui
https://codereview.chromium.org/2969603003/diff/160001/ui/message_center/views/notification_view_md.cc File ui/message_center/views/notification_view_md.cc (right): https://codereview.chromium.org/2969603003/diff/160001/ui/message_center/views/notification_view_md.cc#newcode285 ui/message_center/views/notification_view_md.cc:285: views::View* const container_; On 2017/08/04 07:00:33, Yoshiki IGUCHI wrote: ...
3 years, 4 months ago (2017-08-04 07:39:31 UTC) #50
yoshiki
thans~ lgtm https://codereview.chromium.org/2969603003/diff/200001/ui/message_center/views/notification_view_md.cc File ui/message_center/views/notification_view_md.cc (right): https://codereview.chromium.org/2969603003/diff/200001/ui/message_center/views/notification_view_md.cc#newcode345 ui/message_center/views/notification_view_md.cc:345: explicit LargeImageContainerView(); nit: explicit is unnecessary. https://codereview.chromium.org/2969603003/diff/200001/ui/message_center/views/notification_view_md.cc#newcode352 ...
3 years, 4 months ago (2017-08-04 08:46:23 UTC) #55
tetsui
yoshiki@: Thank you for reviewing! https://codereview.chromium.org/2969603003/diff/200001/ui/message_center/views/notification_view_md.cc File ui/message_center/views/notification_view_md.cc (right): https://codereview.chromium.org/2969603003/diff/200001/ui/message_center/views/notification_view_md.cc#newcode345 ui/message_center/views/notification_view_md.cc:345: explicit LargeImageContainerView(); On 2017/08/04 ...
3 years, 4 months ago (2017-08-04 09:32:59 UTC) #58
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2969603003/220001
3 years, 4 months ago (2017-08-04 09:33:19 UTC) #61
commit-bot: I haz the power
3 years, 4 months ago (2017-08-04 11:14:59 UTC) #64
Message was sent while issue was closed.
Committed patchset #11 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/996de39b52e4392d01859e4343a9...

Powered by Google App Engine
This is Rietveld 408576698