Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2969603003:
Implement new-style image notification resizing. (Closed)
DescriptionImplement 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. #
Messages
Total messages: 64 (50 generated)
The CQ bit was checked by tetsui@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.
tetsui@chromium.org changed reviewers: + fukino@chromium.org, yoshiki@chromium.org
PTAL. Thanks! https://codereview.chromium.org/2969603003/diff/20001/ui/message_center/views... File ui/message_center/views/notification_view_md.cc (right): https://codereview.chromium.org/2969603003/diff/20001/ui/message_center/views... ui/message_center/views/notification_view_md.cc:56: constexpr gfx::Size kLargeImageMaxSize(328, 218); In new-style notification mock, the notification width is 352dp while it is 360dp in old-style. So the layout is slightly broken. This can be easily fixed by setting kNotificationWidth = 352 in message_center_style.h.
The CQ bit was checked by tetsui@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.
Drive-by: this resizing is very different from how Android does it, in particular for vertical images since it crops differently (Android center-crops), and it scales small images differently instead. I left a more detailed comment on the design spec: https://goto.google.com/fuwow. Could you check with the UX designer that diverging from Android is deliberate here? For Web Notifications in particular it'd be preferable to match Android exactly. Thanks!
Description was changed from ========== Implement new-style image notification resizing. In new-style notification, the resizing behavior of image notification is different form the non-MD notification as shown in in the mock. This CL adds LargeImageView, which implements the specific resizing behavior and its container. BUG=737850 TEST=manual ========== to ========== DO NOT MERGE: Implement new-style image notification resizing. In new-style notification, the resizing behavior of image notification is different form the non-MD notification as shown in in the mock. This CL adds LargeImageView, which implements the specific resizing behavior and its container. BUG=737850 TEST=manual ==========
On 2017/07/03 17:30:32, johnme wrote: > Drive-by: this resizing is very different from how Android does it, in > particular for vertical images since it crops differently (Android > center-crops), and it scales small images differently instead. > > I left a more detailed comment on the design spec: > https://goto.google.com/fuwow. > > Could you check with the UX designer that diverging from Android is deliberate > here? For Web Notifications in particular it'd be preferable to match Android > exactly. Thanks! Thank you! I marked this CL as DO NOT MERGE, and I would check with the designer (He's OOO until July 24th).
The CQ bit was checked by tetsui@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 ========== DO NOT MERGE: Implement new-style image notification resizing. In new-style notification, the resizing behavior of image notification is different form the non-MD notification as shown in in the mock. This CL adds LargeImageView, which implements the specific resizing behavior and its container. BUG=737850 TEST=manual ========== to ========== Implement new-style image notification resizing. In new-style notification, the resizing behavior of image notification is different form the non-MD notification and similar to Android's one. This CL adds LargeImageView, which implements the specific resizing behavior that is similar to Android's one. BUG=737850 TEST=manual ==========
Description was changed from ========== Implement new-style image notification resizing. In new-style notification, the resizing behavior of image notification is different form the non-MD notification and similar to Android's one. This CL adds LargeImageView, which implements the specific resizing behavior that is similar to Android's one. BUG=737850 TEST=manual ========== to ========== Implement new-style image notification resizing. In new-style notification, the resizing behavior of image notification is different form the non-MD notification and similar to Android's one. This CL adds LargeImageView, which implements the specific resizing behavior that is similar to Android's one. BUG=737850 TEST=manual ==========
yoshiki@: I implemented Android-like resizing. How about landing this CL first and then modifying later if the final mock is different? Thanks!
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 tetsui@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 tetsui@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 ========== Implement new-style image notification resizing. In new-style notification, the resizing behavior of image notification is different form the non-MD notification and similar to Android's one. This CL adds LargeImageView, which implements the specific resizing behavior that is similar to Android's one. BUG=737850 TEST=manual ========== to ========== 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 ==========
Sebastien clarified that we are going to use the resizing behavior in the mock here, which is different from Android's one. (Thanks johnme@ for pointing out the difference.) yoshiki@ fukino@ Could you finally review the CL as now we can land it? Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by tetsui@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 tetsui@chromium.org
Patchset #6 (id:100001) has been deleted
https://codereview.chromium.org/2969603003/diff/20001/ui/message_center/views... File ui/message_center/views/notification_view_md.cc (right): https://codereview.chromium.org/2969603003/diff/20001/ui/message_center/views... ui/message_center/views/notification_view_md.cc:336: views::View* container_ = nullptr; nit: please add a comment about how the container works. It looks not obvious. If you think necessary, please add a reference to the mock. https://codereview.chromium.org/2969603003/diff/20001/ui/message_center/views... ui/message_center/views/notification_view_md.cc:338: }; Please add DISALLOW_COPY_AND_ASSIGN. This class should be non-copyable. https://codereview.chromium.org/2969603003/diff/20001/ui/message_center/views... ui/message_center/views/notification_view_md.cc:393: } else { optional: I prefer the following style since this if-block is a kind of short-circuit. But it's up to you. if (...) { return ...; } xxx; https://codereview.chromium.org/2969603003/diff/80001/ui/message_center/views... File ui/message_center/views/notification_view_md.cc (right): https://codereview.chromium.org/2969603003/diff/80001/ui/message_center/views... ui/message_center/views/notification_view_md.cc:314: views::View* container_ = nullptr; Could you add const? https://codereview.chromium.org/2969603003/diff/80001/ui/message_center/views... ui/message_center/views/notification_view_md.cc:316: }; Please add DISALLOW_COPY_AND_ASSIGN
The CQ bit was checked by tetsui@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/2969603003/diff/20001/ui/message_center/views... File ui/message_center/views/notification_view_md.cc (right): https://codereview.chromium.org/2969603003/diff/20001/ui/message_center/views... ui/message_center/views/notification_view_md.cc:336: views::View* container_ = nullptr; On 2017/07/28 06:13:03, yoshiki wrote: > nit: please add a comment about how the container works. It looks not obvious. > If you think necessary, please add a reference to the mock. Done. https://codereview.chromium.org/2969603003/diff/20001/ui/message_center/views... ui/message_center/views/notification_view_md.cc:338: }; On 2017/07/28 06:13:03, yoshiki wrote: > Please add DISALLOW_COPY_AND_ASSIGN. This class should be non-copyable. Done. https://codereview.chromium.org/2969603003/diff/20001/ui/message_center/views... ui/message_center/views/notification_view_md.cc:393: } else { On 2017/07/28 06:13:03, yoshiki wrote: > optional: I prefer the following style since this if-block is a kind of > short-circuit. But it's up to you. > > if (...) { > return ...; > } > > xxx; Done. https://codereview.chromium.org/2969603003/diff/80001/ui/message_center/views... File ui/message_center/views/notification_view_md.cc (right): https://codereview.chromium.org/2969603003/diff/80001/ui/message_center/views... ui/message_center/views/notification_view_md.cc:314: views::View* container_ = nullptr; On 2017/07/28 06:13:03, yoshiki wrote: > Could you add const? Done. https://codereview.chromium.org/2969603003/diff/80001/ui/message_center/views... ui/message_center/views/notification_view_md.cc:316: }; On 2017/07/28 06:13:03, yoshiki wrote: > Please add DISALLOW_COPY_AND_ASSIGN Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by tetsui@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@: friendly ping
yoshiki@google.com changed reviewers: + yoshiki@google.com
https://codereview.chromium.org/2969603003/diff/160001/ui/message_center/view... File ui/message_center/views/notification_view_md.cc (right): https://codereview.chromium.org/2969603003/diff/160001/ui/message_center/view... ui/message_center/views/notification_view_md.cc:285: views::View* const container_; It's not good that a child has a parent. In this implementation, we can't use LargeImageView directly, instead, we must use container(): it's not standard way of views. I think not this class but NotificationViewMD should have a container view. Or how about creating LargeImageContainerView class which owns LargeImageView?
The CQ bit was checked by tetsui@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/2969603003/diff/160001/ui/message_center/view... File ui/message_center/views/notification_view_md.cc (right): https://codereview.chromium.org/2969603003/diff/160001/ui/message_center/view... ui/message_center/views/notification_view_md.cc:285: views::View* const container_; On 2017/08/04 07:00:33, Yoshiki IGUCHI wrote: > It's not good that a child has a parent. In this implementation, we can't use > LargeImageView directly, instead, we must use container(): it's not standard way > of views. > > I think not this class but NotificationViewMD should have a container view. Or > how about creating LargeImageContainerView class which owns LargeImageView? Done.
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 tetsui@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...
thans~ lgtm https://codereview.chromium.org/2969603003/diff/200001/ui/message_center/view... File ui/message_center/views/notification_view_md.cc (right): https://codereview.chromium.org/2969603003/diff/200001/ui/message_center/view... 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/view... ui/message_center/views/notification_view_md.cc:352: LargeImageView* image_view_; nit: const
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
yoshiki@: Thank you for reviewing! https://codereview.chromium.org/2969603003/diff/200001/ui/message_center/view... File ui/message_center/views/notification_view_md.cc (right): https://codereview.chromium.org/2969603003/diff/200001/ui/message_center/view... ui/message_center/views/notification_view_md.cc:345: explicit LargeImageContainerView(); On 2017/08/04 08:46:23, yoshiki wrote: > nit: explicit is unnecessary. Done. https://codereview.chromium.org/2969603003/diff/200001/ui/message_center/view... ui/message_center/views/notification_view_md.cc:352: LargeImageView* image_view_; On 2017/08/04 08:46:23, yoshiki wrote: > nit: const Done.
The CQ bit was checked by tetsui@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yoshiki@chromium.org Link to the patchset: https://codereview.chromium.org/2969603003/#ps220001 (title: "Fix nits.")
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": 220001, "attempt_start_ts": 1501839187156730,
"parent_rev": "53d98e32f71834a92dfbd38c2ddce40942e93278", "commit_rev":
"996de39b52e4392d01859e4343a916be940d456b"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/996de39b52e4392d01859e4343a9... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:220001) as https://chromium.googlesource.com/chromium/src/+/996de39b52e4392d01859e4343a9... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
