Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2805143002:
Don't shrink the message center height while open (Closed)
DescriptionDon't shrink the message center height while open
Previously, the height of the message center shrank if necessary. But this patch changed this behavior and made it not shrank while the message center opens.
This patch also changes animation. Previously, the notifications above the target were animated. With this patch, the notifications below the target are animated. This is because now we don't update the height of message center so animating below is natural.
The reason why we don't use AnimateNotificationsBelowTarget() is that it has a bug on removing notification.
BUG=709337
BUG=b/36517819
TEST=manual
TEST=unittests pass
Review-Url: https://codereview.chromium.org/2805143002
Cr-Commit-Position: refs/heads/master@{#465184}
Committed: https://chromium.googlesource.com/chromium/src/+/8057405c22f4ca9727ab53b845ee87ed1d865a73
Patch Set 1 #Patch Set 2 : . #
Total comments: 10
Patch Set 3 : Addressed comments #
Total comments: 8
Patch Set 4 : Addressed comments #Patch Set 5 : Remove debug code #Patch Set 6 : Fixed issue with expanded notification above #Patch Set 7 : Addressed offline comment #
Total comments: 14
Patch Set 8 : Addressed comments #
Total comments: 2
Patch Set 9 : Remove unnecessary include #
Messages
Total messages: 82 (63 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: This issue passed the CQ dry run.
Description was changed from ========== wip . . Merged HEAD..master . . . . . . . . . . . BUG= ========== to ========== Don't shrink the message center height while open Previously, the height of the message center shrank if necessary. But this patch changed this behavior and made it not shrank while the message center opens. This patch also changes animation. Previously, the notifications above the target were animated. With this patch, the notifications below the target are animated. This is because now we don't update the height of message center so animating below is natural. The reason why we don't use AnimateNotificationsBelowTarget() is that it has a bug on removing notification. BUG= TEST=manual TEST=unittests pass ==========
yoshiki@chromium.org changed reviewers: + edcourtney@chromium.org
Description was changed from ========== Don't shrink the message center height while open Previously, the height of the message center shrank if necessary. But this patch changed this behavior and made it not shrank while the message center opens. This patch also changes animation. Previously, the notifications above the target were animated. With this patch, the notifications below the target are animated. This is because now we don't update the height of message center so animating below is natural. The reason why we don't use AnimateNotificationsBelowTarget() is that it has a bug on removing notification. BUG= TEST=manual TEST=unittests pass ========== to ========== Don't shrink the message center height while open Previously, the height of the message center shrank if necessary. But this patch changed this behavior and made it not shrank while the message center opens. This patch also changes animation. Previously, the notifications above the target were animated. With this patch, the notifications below the target are animated. This is because now we don't update the height of message center so animating below is natural. The reason why we don't use AnimateNotificationsBelowTarget() is that it has a bug on removing notification. BUG=709337 BUG=b:36517819 TEST=manual TEST=unittests pass ==========
Description was changed from ========== Don't shrink the message center height while open Previously, the height of the message center shrank if necessary. But this patch changed this behavior and made it not shrank while the message center opens. This patch also changes animation. Previously, the notifications above the target were animated. With this patch, the notifications below the target are animated. This is because now we don't update the height of message center so animating below is natural. The reason why we don't use AnimateNotificationsBelowTarget() is that it has a bug on removing notification. BUG=709337 BUG=b:36517819 TEST=manual TEST=unittests pass ========== to ========== Don't shrink the message center height while open Previously, the height of the message center shrank if necessary. But this patch changed this behavior and made it not shrank while the message center opens. This patch also changes animation. Previously, the notifications above the target were animated. With this patch, the notifications below the target are animated. This is because now we don't update the height of message center so animating below is natural. The reason why we don't use AnimateNotificationsBelowTarget() is that it has a bug on removing notification. BUG=709337 BUG=b/36517819 TEST=manual TEST=unittests pass ==========
Eliot, PTAL. Thanks.
https://codereview.chromium.org/2805143002/diff/20001/ui/message_center/views... File ui/message_center/views/message_center_view.cc (right): https://codereview.chromium.org/2805143002/diff/20001/ui/message_center/views... ui/message_center/views/message_center_view.cc:401: // Skip if the view is a custom view. Custom views can be expanded by discussed offline: We want to keep the on-hover reposition target behaviour for custom notifications as well, so we (probably?) still need to set the reposition target here. https://codereview.chromium.org/2805143002/diff/20001/ui/message_center/views... File ui/message_center/views/message_list_view.cc (right): https://codereview.chromium.org/2805143002/diff/20001/ui/message_center/views... ui/message_center/views/message_list_view.cc:178: void MessageListView::UpdateFixedHeight(int requesting_height, *nit: requested_height https://codereview.chromium.org/2805143002/diff/20001/ui/message_center/views... ui/message_center/views/message_list_view.cc:179: int prevent_scroll) { nit: bool prevent_scroll ? https://codereview.chromium.org/2805143002/diff/20001/ui/message_center/views... ui/message_center/views/message_list_view.cc:325: // TODO(yoshiki): Remove this method. It is no longer mainteined. *nit: maintained Yeah, I agree we should remove this sometime. https://codereview.chromium.org/2805143002/diff/20001/ui/message_center/views... File ui/message_center/views/message_list_view.h (right): https://codereview.chromium.org/2805143002/diff/20001/ui/message_center/views... ui/message_center/views/message_list_view.h:100: // is the minimum height, and actcual fixed height should be more than it. *nit: actual
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:60001) has been deleted
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
Eliot, PTAL. Thanks. https://codereview.chromium.org/2805143002/diff/20001/ui/message_center/views... File ui/message_center/views/message_center_view.cc (right): https://codereview.chromium.org/2805143002/diff/20001/ui/message_center/views... ui/message_center/views/message_center_view.cc:401: // Skip if the view is a custom view. Custom views can be expanded by On 2017/04/11 00:58:26, Eliot Courtney wrote: > discussed offline: We want to keep the on-hover reposition target behaviour for > custom notifications as well, so we (probably?) still need to set the reposition > target here. I just removed this, since the reason I added this code is fixed by a recent patch in Android. https://codereview.chromium.org/2805143002/diff/20001/ui/message_center/views... File ui/message_center/views/message_list_view.cc (right): https://codereview.chromium.org/2805143002/diff/20001/ui/message_center/views... ui/message_center/views/message_list_view.cc:178: void MessageListView::UpdateFixedHeight(int requesting_height, On 2017/04/11 00:58:26, Eliot Courtney wrote: > *nit: requested_height Done. https://codereview.chromium.org/2805143002/diff/20001/ui/message_center/views... ui/message_center/views/message_list_view.cc:179: int prevent_scroll) { On 2017/04/11 00:58:26, Eliot Courtney wrote: > nit: bool prevent_scroll ? Done. https://codereview.chromium.org/2805143002/diff/20001/ui/message_center/views... ui/message_center/views/message_list_view.cc:325: // TODO(yoshiki): Remove this method. It is no longer mainteined. On 2017/04/11 00:58:26, Eliot Courtney wrote: > *nit: maintained > > Yeah, I agree we should remove this sometime. Done. https://codereview.chromium.org/2805143002/diff/20001/ui/message_center/views... File ui/message_center/views/message_list_view.h (right): https://codereview.chromium.org/2805143002/diff/20001/ui/message_center/views... ui/message_center/views/message_list_view.h:100: // is the minimum height, and actcual fixed height should be more than it. On 2017/04/11 00:58:26, Eliot Courtney wrote: > *nit: actual Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I patched this in but it seems like if a notification above the hovered notification grows in size, it doesn't keep the hovered notification under the mouse. Can you please check this? Thanks~! https://codereview.chromium.org/2805143002/diff/40001/ui/message_center/views... File ui/message_center/views/message_list_view.cc (left): https://codereview.chromium.org/2805143002/diff/40001/ui/message_center/views... ui/message_center/views/message_list_view.cc:413: // If no items are below |reposition_top_|, use the last item as the target. Is this no longer necessary? How come? https://codereview.chromium.org/2805143002/diff/40001/ui/message_center/views... File ui/message_center/views/message_list_view.cc (right): https://codereview.chromium.org/2805143002/diff/40001/ui/message_center/views... ui/message_center/views/message_list_view.cc:390: // some of items above the target are updated and their height gets longer. nit: gets longer => increased https://codereview.chromium.org/2805143002/diff/40001/ui/message_center/views... ui/message_center/views/message_list_view.cc:417: // If the target view, or any views below it expand they might exceed This comment feels like it might need updating to reflect its relationship with the new UpdateFixedHeight call. https://codereview.chromium.org/2805143002/diff/40001/ui/message_center/views... File ui/message_center/views/message_list_view.h (right): https://codereview.chromium.org/2805143002/diff/40001/ui/message_center/views... ui/message_center/views/message_list_view.h:20: #include "ui/views/controls/scroll_view.h" Maybe it's possible to forward declare ScrollView here, instead of including it?
Currently we don't change the scroll position so if the height is changed by expansion, the position may be moved wrongly. Let me fix it in separate patch (I added TODO comment). The issue exists without this CL and I want to keep this CL smaller. https://codereview.chromium.org/2805143002/diff/40001/ui/message_center/views... File ui/message_center/views/message_list_view.cc (left): https://codereview.chromium.org/2805143002/diff/40001/ui/message_center/views... ui/message_center/views/message_list_view.cc:413: // If no items are below |reposition_top_|, use the last item as the target. On 2017/04/12 06:16:18, Eliot Courtney wrote: > Is this no longer necessary? How come? Now animating direction changed from downward to upward, so we don't need to set the last item as the target. https://codereview.chromium.org/2805143002/diff/40001/ui/message_center/views... File ui/message_center/views/message_list_view.cc (right): https://codereview.chromium.org/2805143002/diff/40001/ui/message_center/views... ui/message_center/views/message_list_view.cc:390: // some of items above the target are updated and their height gets longer. On 2017/04/12 06:16:18, Eliot Courtney wrote: > nit: gets longer => increased Done. https://codereview.chromium.org/2805143002/diff/40001/ui/message_center/views... ui/message_center/views/message_list_view.cc:417: // If the target view, or any views below it expand they might exceed On 2017/04/12 06:16:18, Eliot Courtney wrote: > This comment feels like it might need updating to reflect its relationship with > the new UpdateFixedHeight call. Done. https://codereview.chromium.org/2805143002/diff/40001/ui/message_center/views... File ui/message_center/views/message_list_view.h (right): https://codereview.chromium.org/2805143002/diff/40001/ui/message_center/views... ui/message_center/views/message_list_view.h:20: #include "ui/views/controls/scroll_view.h" On 2017/04/12 06:16:18, Eliot Courtney wrote: > Maybe it's possible to forward declare ScrollView here, instead of including it? Accoring to the style guide, we should include header files as possible, https://google.github.io/styleguide/cppguide.html#Forward_Declarations
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 #5 (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.
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...
Eliot, I fixed the issue we talked offline. PTAL again?
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...
Patchset #7 (id:160001) 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: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Patchset #7 (id:180001) 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.
Eliot, I fixed again. PTAL. Thanks.
Thanks~! Here are some mostly nits. https://codereview.chromium.org/2805143002/diff/200001/ui/message_center/view... File ui/message_center/views/message_list_view.cc (right): https://codereview.chromium.org/2805143002/diff/200001/ui/message_center/view... ui/message_center/views/message_list_view.cc:181: int min_height; nit: Maybe it's better to set min_height to fixed_height_ initially, then modify it if !prevent_scroll && scroller_. It also makes sure min_height is initialised. Up to you~ https://codereview.chromium.org/2805143002/diff/200001/ui/message_center/view... ui/message_center/views/message_list_view.cc:187: // Otherwize (in else block), we use the height of the visible rect. It's to nit: Otherwise nit: rect to make the height as small as possible. https://codereview.chromium.org/2805143002/diff/200001/ui/message_center/view... ui/message_center/views/message_list_view.cc:195: gfx::Rect visible_rect = scroller_->GetVisibleRect(); Is it better to call scroller->GetVisibleRect() inside the if (scroller_) statement? https://codereview.chromium.org/2805143002/diff/200001/ui/message_center/view... ui/message_center/views/message_list_view.cc:400: // some of items above the target are updated and their height incresed. nit: increased https://codereview.chromium.org/2805143002/diff/200001/ui/message_center/view... ui/message_center/views/message_list_view.cc:407: // TODO(yoshiki): Scroll the parent container to keep the phisical position nit: physical https://codereview.chromium.org/2805143002/diff/200001/ui/message_center/view... ui/message_center/views/message_list_view.cc:408: // of the target notification when the scrolling is occured by size change nit: is caused by a size change https://codereview.chromium.org/2805143002/diff/200001/ui/message_center/view... ui/message_center/views/message_list_view.cc:432: // Update the fixed height. |requested_height| is the minimum height to have I thought |requested_height| could be bigger than the minimum height to contain all the notifications in the list - i.e. that it could contain spaces. Is this right?
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...
Eliot, PTAL. Thanks. https://codereview.chromium.org/2805143002/diff/200001/ui/message_center/view... File ui/message_center/views/message_list_view.cc (right): https://codereview.chromium.org/2805143002/diff/200001/ui/message_center/view... ui/message_center/views/message_list_view.cc:181: int min_height; On 2017/04/17 03:39:01, Eliot Courtney wrote: > nit: Maybe it's better to set min_height to fixed_height_ initially, then modify > it if !prevent_scroll && scroller_. It also makes sure min_height is > initialised. Up to you~ I have a plan to write some code in if (prevent_scroll) for better support of scrolling. So let me keep this as it is. https://codereview.chromium.org/2805143002/diff/200001/ui/message_center/view... ui/message_center/views/message_list_view.cc:187: // Otherwize (in else block), we use the height of the visible rect. It's to On 2017/04/17 03:39:01, Eliot Courtney wrote: > nit: Otherwise > nit: rect to make the height as small as possible. Thanks! done. https://codereview.chromium.org/2805143002/diff/200001/ui/message_center/view... ui/message_center/views/message_list_view.cc:195: gfx::Rect visible_rect = scroller_->GetVisibleRect(); On 2017/04/17 03:39:01, Eliot Courtney wrote: > Is it better to call scroller->GetVisibleRect() inside the if (scroller_) > statement? Done. https://codereview.chromium.org/2805143002/diff/200001/ui/message_center/view... ui/message_center/views/message_list_view.cc:400: // some of items above the target are updated and their height incresed. On 2017/04/17 03:39:01, Eliot Courtney wrote: > nit: increased Done. https://codereview.chromium.org/2805143002/diff/200001/ui/message_center/view... ui/message_center/views/message_list_view.cc:407: // TODO(yoshiki): Scroll the parent container to keep the phisical position On 2017/04/17 03:39:01, Eliot Courtney wrote: > nit: physical Done. https://codereview.chromium.org/2805143002/diff/200001/ui/message_center/view... ui/message_center/views/message_list_view.cc:408: // of the target notification when the scrolling is occured by size change On 2017/04/17 03:39:01, Eliot Courtney wrote: > nit: is caused by a size change Done. https://codereview.chromium.org/2805143002/diff/200001/ui/message_center/view... ui/message_center/views/message_list_view.cc:432: // Update the fixed height. |requested_height| is the minimum height to have On 2017/04/17 03:39:01, Eliot Courtney wrote: > I thought |requested_height| could be bigger than the minimum height to contain > all the notifications in the list - i.e. that it could contain spaces. Is this > right? Thanks. Updated the comment.
Patchset #9 (id:240001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Eliot, I removed the latest patch set as we discussed offline. PTAL? Thanks.
lgtm
The CQ bit was checked by yoshiki@chromium.org
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
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
yoshiki@chromium.org changed reviewers: + yhanada@chromium.org
Hanada-san, could you take a look as a committer? Thanks.
Hanada-san, could you take a look as a committer? Thanks.
lgtm https://codereview.chromium.org/2805143002/diff/220001/ui/message_center/view... File ui/message_center/views/message_center_view.cc (right): https://codereview.chromium.org/2805143002/diff/220001/ui/message_center/view... ui/message_center/views/message_center_view.cc:25: #include "ui/message_center/views/custom_notification_view.h" nit: remove?
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...
Thanks! https://codereview.chromium.org/2805143002/diff/220001/ui/message_center/view... File ui/message_center/views/message_center_view.cc (right): https://codereview.chromium.org/2805143002/diff/220001/ui/message_center/view... ui/message_center/views/message_center_view.cc:25: #include "ui/message_center/views/custom_notification_view.h" On 2017/04/18 05:12:30, yhanada wrote: > nit: remove? Done.
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
The patchset sent to the CQ was uploaded after l-g-t-m from edcourtney@chromium.org, yhanada@chromium.org Link to the patchset: https://codereview.chromium.org/2805143002/#ps260001 (title: "Remove unnecessary include ")
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": 260001, "attempt_start_ts": 1492507018323960,
"parent_rev": "0cfcab89310f8e7ad6db537c686200c0f545b613", "commit_rev":
"8057405c22f4ca9727ab53b845ee87ed1d865a73"}
Message was sent while issue was closed.
Description was changed from ========== Don't shrink the message center height while open Previously, the height of the message center shrank if necessary. But this patch changed this behavior and made it not shrank while the message center opens. This patch also changes animation. Previously, the notifications above the target were animated. With this patch, the notifications below the target are animated. This is because now we don't update the height of message center so animating below is natural. The reason why we don't use AnimateNotificationsBelowTarget() is that it has a bug on removing notification. BUG=709337 BUG=b/36517819 TEST=manual TEST=unittests pass ========== to ========== Don't shrink the message center height while open Previously, the height of the message center shrank if necessary. But this patch changed this behavior and made it not shrank while the message center opens. This patch also changes animation. Previously, the notifications above the target were animated. With this patch, the notifications below the target are animated. This is because now we don't update the height of message center so animating below is natural. The reason why we don't use AnimateNotificationsBelowTarget() is that it has a bug on removing notification. BUG=709337 BUG=b/36517819 TEST=manual TEST=unittests pass Review-Url: https://codereview.chromium.org/2805143002 Cr-Commit-Position: refs/heads/master@{#465184} Committed: https://chromium.googlesource.com/chromium/src/+/8057405c22f4ca9727ab53b845ee... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:260001) as https://chromium.googlesource.com/chromium/src/+/8057405c22f4ca9727ab53b845ee... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
