Notifications: Retain button hover state during content updates.

Notifications: Retain button hover state during content updates.

Rapidly updating notifications (like progressbar indicators) can
be hard to interact with if the state resets with each update.

BUG=368025
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272609

[dewittj, 2014-05-12 21:16:16]

mukai: ptal

[Jun Mukai, 2014-05-13 07:23:23]

https://codereview.chromium.org/273173003/diff/1/ui/message_center/views/noti...
File ui/message_center/views/notification_view.cc (right):

https://codereview.chromium.org/273173003/diff/1/ui/message_center/views/noti...
ui/message_center/views/notification_view.cc:454: std::vector<View*>
buttons(action_buttons_.begin(), action_buttons_.end());
I think omitting views:: namespace leads to a compile error?

https://codereview.chromium.org/273173003/diff/1/ui/message_center/views/noti...
ui/message_center/views/notification_view.cc:705: std::vector<ButtonInfo>
buttons = notification.buttons();
const &?

https://codereview.chromium.org/273173003/diff/1/ui/message_center/views/noti...
ui/message_center/views/notification_view.cc:708: if (new_buttons ||
buttons.size() == 0) {
Does this means that if the hover state will be lost if the number of buttons
has changed?
Consider the case like:
- notification created, with button A and B
- mouse hover on A
- notification updated to button A (button B removed)


There is a similar but a bit trickier scenario like:
- notification updated to button B (button A removed)
in this case -- I personally think the button B should get the hover background,
because the mouse cursor still exists there.


Considering this, it would be better to invoke
GetWidget()->SynthesizeMouseMoveEvent() which rerun a dummy mouse move event for
this purpose.
MessageCenterView already has this for refocusing the close button: see
message_center_view.cc for the details.

https://codereview.chromium.org/273173003/diff/1/ui/message_center/views/noti...
ui/message_center/views/notification_view.cc:711: separators_.clear();
I think STLDeleteElements(&separators_); (in base/stl_util.h) would be more
concise.

https://codereview.chromium.org/273173003/diff/1/ui/message_center/views/noti...
ui/message_center/views/notification_view.cc:715: action_buttons_.clear();
ditto

[Oren Blasberg, 2014-05-19 17:42:10]

Ping for status update? :)
We would really like this in M36 if possible so just want to make sure it is
still being looked at -- thanks!

[dewittj, 2014-05-20 18:32:13]

looking at this..

https://codereview.chromium.org/273173003/diff/1/ui/message_center/views/noti...
File ui/message_center/views/notification_view.cc (right):

https://codereview.chromium.org/273173003/diff/1/ui/message_center/views/noti...
ui/message_center/views/notification_view.cc:454: std::vector<View*>
buttons(action_buttons_.begin(), action_buttons_.end());
On 2014/05/13 07:23:23, Jun Mukai wrote:
> I think omitting views:: namespace leads to a compile error?

Pretty sure I compiled this :) but I'll check

https://codereview.chromium.org/273173003/diff/1/ui/message_center/views/noti...
ui/message_center/views/notification_view.cc:705: std::vector<ButtonInfo>
buttons = notification.buttons();
On 2014/05/13 07:23:23, Jun Mukai wrote:
> const &?

I don't like marking the result of a method call as const &.  It's not too big
of a copy anyway.  If you feel strongly I can update.

https://codereview.chromium.org/273173003/diff/1/ui/message_center/views/noti...
ui/message_center/views/notification_view.cc:708: if (new_buttons ||
buttons.size() == 0) {
On 2014/05/13 07:23:23, Jun Mukai wrote:
> Does this means that if the hover state will be lost if the number of buttons
> has changed?
> Consider the case like:
> - notification created, with button A and B
> - mouse hover on A
> - notification updated to button A (button B removed)
> 
> 
> There is a similar but a bit trickier scenario like:
> - notification updated to button B (button A removed)
> in this case -- I personally think the button B should get the hover
background,
> because the mouse cursor still exists there.
> 
> 
> Considering this, it would be better to invoke
> GetWidget()->SynthesizeMouseMoveEvent() which rerun a dummy mouse move event
for
> this purpose.
> MessageCenterView already has this for refocusing the close button: see
> message_center_view.cc for the details.

It does mean this.  However for frequent updates, SynthesizeMouseEvent is not
sufficient to prevent flicker.  I'll try adding a SynthesizeMouseEvent if the
number of buttons changes.

https://codereview.chromium.org/273173003/diff/1/ui/message_center/views/noti...
ui/message_center/views/notification_view.cc:711: separators_.clear();
On 2014/05/13 07:23:23, Jun Mukai wrote:
> I think STLDeleteElements(&separators_); (in base/stl_util.h) would be more
> concise.

Will do!

[dewittj, 2014-05-21 21:13:30]

mukai: ptal

https://codereview.chromium.org/273173003/diff/1/ui/message_center/views/noti...
File ui/message_center/views/notification_view.cc (right):

https://codereview.chromium.org/273173003/diff/1/ui/message_center/views/noti...
ui/message_center/views/notification_view.cc:454: std::vector<View*>
buttons(action_buttons_.begin(), action_buttons_.end());
On 2014/05/20 18:32:13, dewittj wrote:
> On 2014/05/13 07:23:23, Jun Mukai wrote:
> > I think omitting views:: namespace leads to a compile error?
> 
> Pretty sure I compiled this :) but I'll check

Done.

https://codereview.chromium.org/273173003/diff/1/ui/message_center/views/noti...
ui/message_center/views/notification_view.cc:715: action_buttons_.clear();
On 2014/05/13 07:23:23, Jun Mukai wrote:
> ditto

Done.

[dewittj, 2014-05-23 00:07:22]

mukai:ping

[Jun Mukai, 2014-05-23 06:24:08]

lgtm

https://codereview.chromium.org/273173003/diff/40001/ui/message_center/views/...
File ui/message_center/views/notification_view.cc (right):

https://codereview.chromium.org/273173003/diff/40001/ui/message_center/views/...
ui/message_center/views/notification_view.cc:712: separators_.clear();
STLDeleteElements() invokes DeleteContainerPointers and then clear() :)

[dewittj, 2014-05-23 17:32:28]

The CQ bit was checked by dewittj@chromium.org

[dewittj, 2014-05-23 17:32:41]

https://codereview.chromium.org/273173003/diff/40001/ui/message_center/views/...
File ui/message_center/views/notification_view.cc (right):

https://codereview.chromium.org/273173003/diff/40001/ui/message_center/views/...
ui/message_center/views/notification_view.cc:712: separators_.clear();
On 2014/05/23 06:24:08, Jun Mukai wrote:
> STLDeleteElements() invokes DeleteContainerPointers and then clear() :)

Done.

[commit-bot: I haz the power, 2014-05-23 17:33:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dewittj@chromium.org/273173003/60001

[commit-bot: I haz the power, 2014-05-23 22:08:42]

Change committed as 272609