Implement an indeterminate progress-bar

Implement an indeterminate progress-bar

This patch implements indeterminate progress-bar in notifications, which will be used by download notification.

Major changes:
- Implement indeterminate progress bar in the NotificationProgressBar class.
- Extract NotificationProgressBar into notification_progress_bar.[cc|h]
- Add NotificationProgressBar::SetIndeterminate(bool) method which set the indetermination.

BUG=554916
TEST=manual test

Committed: https://crrev.com/fc16346c607d1eff870269962062c20170f5475f
Cr-Commit-Position: refs/heads/master@{#361861}

[yoshiki, 2015-11-16 02:38:38]

Description was changed from

==========
wip: add indeterminate progress-bar

BUG=
==========

to

==========
Implement an indeterminate progress-bar

This patch implements indeterminate progress-bar in notifications, which will be
used by download notification.

Major changes:
- Implement indeterminate progress bar in the NotificationProgressBar class.
- Extract NotificationProgressBar into notification_progress_bar.[cc|h]
- Add NotificationProgressBar::SetIndeterminate(bool) method which set the
indetermination.

BUG=554916
TEST=manual test
==========

[yoshiki, 2015-11-16 02:38:38]

yoshiki@chromium.org changed reviewers:
+ mukai@chromium.org

[yoshiki, 2015-11-16 02:44:38]

Mukai-san, could you take a look? This feature will be used by donwload
notification (Issue: http://crbug.com/554918, entire patch:
http://crrev.com/1434253002). Thanks.

[yoshiki, 2015-11-16 02:44:39]

Mukai-san, could you take a look? This feature will be used by donwload
notification (Issue: http://crbug.com/554918, entire patch:
http://crrev.com/1434253002). Thanks.

[Jun Mukai, 2015-11-16 08:29:31]

Could we split the class implementation, such as NotificationProgressBar and
IntermediateProgressBar class?
I think the it would be better to have gigantic ifs for indeterminate flag.
This way, you'll have to replace recreate the progress bar instance when
indeterminate-ness has changed.

https://codereview.chromium.org/1447933002/diff/1/ui/message_center/views/not...
File ui/message_center/views/notification_progress_bar.cc (right):

https://codereview.chromium.org/1447933002/diff/1/ui/message_center/views/not...
ui/message_center/views/notification_progress_bar.cc:90: bar2_width = 0;
Those values look exactly same as time < 0.5. What's the point of time < 0.25?

[Jun Mukai, 2015-11-16 22:58:55]

also -- I'm wondering whether we could adopt some of tween types rather than
those conditionals.

Such as, EASE_OUT (or EASE_OUT_SNAP) for the right edge and EASE_IN (or
EASE_IN_2) for the left edge. If it doesn't hurt the visual, you would get the
code simplicity as for the benefit.

See ui/gfx/animation/tween.h

[yoshiki, 2015-11-24 16:37:26]

On 2015/11/16 22:58:55, Jun Mukai wrote:
> also -- I'm wondering whether we could adopt some of tween types rather than
> those conditionals.
> 
> Such as, EASE_OUT (or EASE_OUT_SNAP) for the right edge and EASE_IN (or
> EASE_IN_2) for the left edge. If it doesn't hurt the visual, you would get the
> code simplicity as for the benefit.
> 
> See ui/gfx/animation/tween.h

This animation comes from the MD indeterminate progress bar. I don't think any
existing tweens match the MD animation.

[yoshiki, 2015-11-24 16:39:24]

Mukai-san, sorry for late. I splitted the class. Could you take a look again?

https://codereview.chromium.org/1447933002/diff/1/ui/message_center/views/not...
File ui/message_center/views/notification_progress_bar.cc (right):

https://codereview.chromium.org/1447933002/diff/1/ui/message_center/views/not...
ui/message_center/views/notification_progress_bar.cc:90: bar2_width = 0;
On 2015/11/16 08:29:31, Jun Mukai wrote:
> Those values look exactly same as time < 0.5. What's the point of time < 0.25?

The bar2 is hidden if time < 0.5. It's intended.

[yoshiki, 2015-11-24 16:49:39]

As I checked the MD official document, it has only one bar. 
http://www.google.com/design/spec/components/progress-activity.html

Hence my animation parameter might be wrong (maybe old version?). Let me recheck
the parameter. Sorry for confusion.

[Jun Mukai, 2015-11-24 17:51:12]

https://codereview.chromium.org/1447933002/diff/1/ui/message_center/views/not...
File ui/message_center/views/notification_progress_bar.cc (right):

https://codereview.chromium.org/1447933002/diff/1/ui/message_center/views/not...
ui/message_center/views/notification_progress_bar.cc:90: bar2_width = 0;
On 2015/11/24 16:39:24, yoshiki wrote:
> On 2015/11/16 08:29:31, Jun Mukai wrote:
> > Those values look exactly same as time < 0.5. What's the point of time <
0.25?
> 
> The bar2 is hidden if time < 0.5. It's intended.

You don't address my question. I said this line 87-90 looks exactly same as line
92-95, therefore you can remove this 'if (time < 0.25) { ... } else'. Am I
missing something?

https://codereview.chromium.org/1447933002/diff/20001/ui/message_center/notif...
File ui/message_center/notification_types.h (right):

https://codereview.chromium.org/1447933002/diff/20001/ui/message_center/notif...
ui/message_center/notification_types.h:36:
NOTIFICATION_TYPE_PROGRESS_INDETERMINATE,
Based on my comment in NotificationView, I think you don't have to add a new
type here.

https://codereview.chromium.org/1447933002/diff/20001/ui/message_center/views...
File ui/message_center/views/notification_progress_bar.h (right):

https://codereview.chromium.org/1447933002/diff/20001/ui/message_center/views...
ui/message_center/views/notification_progress_bar.h:29: void
SetIndeterminate(bool is_indeteminate);
SetIntermediate wouldn't be necessary, it won't change to an intermediate one,
it's the parent responsibility to swap the view implementation.

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

https://codereview.chromium.org/1447933002/diff/20001/ui/message_center/views...
ui/message_center/views/notification_view.cc:17: #include
"ui/gfx/animation/slide_animation.h"
this is not used here?

https://codereview.chromium.org/1447933002/diff/20001/ui/message_center/views...
File ui/message_center/views/notification_view.h (right):

https://codereview.chromium.org/1447933002/diff/20001/ui/message_center/views...
ui/message_center/views/notification_view.h:137:
NotificationIndeterminateProgressBar* indeterminate_progress_bar_view_;
I don't think we should have two different instance variables, they would share
the same interface, and they are placed in the same position. I proposed to
split class because they make different visual effects.

So, NotificationView will recreate the view instance and swap
|progress_bar_view_| upon intermediate flag.

[yoshiki, 2015-11-25 07:18:25]

Patchset #3 (id:40001) has been deleted

[yoshiki, 2015-11-25 07:54:42]

Patchset #3 (id:60001) has been deleted

[yoshiki, 2015-11-25 15:14:28]

I confirmed the animation is correct and same as the MD lite's one, sorry for
confusion.

And I also addressed your comments. Could you take a look? Thanks.

https://codereview.chromium.org/1447933002/diff/1/ui/message_center/views/not...
File ui/message_center/views/notification_progress_bar.cc (right):

https://codereview.chromium.org/1447933002/diff/1/ui/message_center/views/not...
ui/message_center/views/notification_progress_bar.cc:90: bar2_width = 0;
On 2015/11/24 17:51:11, Jun Mukai wrote:
> On 2015/11/24 16:39:24, yoshiki wrote:
> > On 2015/11/16 08:29:31, Jun Mukai wrote:
> > > Those values look exactly same as time < 0.5. What's the point of time <
> 0.25?
> > 
> > The bar2 is hidden if time < 0.5. It's intended.
> 
> You don't address my question. I said this line 87-90 looks exactly same as
line
> 92-95, therefore you can remove this 'if (time < 0.25) { ... } else'. Am I
> missing something?

Ah, I misunderstood. Your're right, and they were same. I combined them.

https://codereview.chromium.org/1447933002/diff/20001/ui/message_center/notif...
File ui/message_center/notification_types.h (right):

https://codereview.chromium.org/1447933002/diff/20001/ui/message_center/notif...
ui/message_center/notification_types.h:36:
NOTIFICATION_TYPE_PROGRESS_INDETERMINATE,
On 2015/11/24 17:51:11, Jun Mukai wrote:
> Based on my comment in NotificationView, I think you don't have to add a new
> type here.

I misunderstood, you did't mean to add a flag controllable from external. I
changed it back to determine the type if the progress value is negative or not.

https://codereview.chromium.org/1447933002/diff/20001/ui/message_center/views...
File ui/message_center/views/notification_progress_bar.h (right):

https://codereview.chromium.org/1447933002/diff/20001/ui/message_center/views...
ui/message_center/views/notification_progress_bar.h:29: void
SetIndeterminate(bool is_indeteminate);
On 2015/11/24 17:51:11, Jun Mukai wrote:
> SetIntermediate wouldn't be necessary, it won't change to an intermediate one,
> it's the parent responsibility to swap the view implementation.

I missed to remove this. Removed.

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

https://codereview.chromium.org/1447933002/diff/20001/ui/message_center/views...
ui/message_center/views/notification_view.cc:17: #include
"ui/gfx/animation/slide_animation.h"
On 2015/11/24 17:51:12, Jun Mukai wrote:
> this is not used here?

Done.

https://codereview.chromium.org/1447933002/diff/20001/ui/message_center/views...
File ui/message_center/views/notification_view.h (right):

https://codereview.chromium.org/1447933002/diff/20001/ui/message_center/views...
ui/message_center/views/notification_view.h:137:
NotificationIndeterminateProgressBar* indeterminate_progress_bar_view_;
On 2015/11/24 17:51:12, Jun Mukai wrote:
> I don't think we should have two different instance variables, they would
share
> the same interface, and they are placed in the same position. I proposed to
> split class because they make different visual effects.
> 
> So, NotificationView will recreate the view instance and swap
> |progress_bar_view_| upon intermediate flag.

Done.

[Jun Mukai, 2015-11-26 08:43:46]

lgtm

[yoshiki, 2015-11-26 10:04:30]

Thanks!

[yoshiki, 2015-11-26 10:04:35]

The CQ bit was checked by yoshiki@chromium.org

[yoshiki, 2015-11-26 10:04:35]

The patchset sent to the CQ was uploaded after l-g-t-m from mukai@chromium.org
Link to the patchset: https://codereview.chromium.org/1447933002/#ps100001
(title: "Fix test failure")

[commit-bot: I haz the power, 2015-11-26 10:04:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1447933002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1447933002/100001

[commit-bot: I haz the power, 2015-11-26 10:09:33]

Description was changed from

==========
Implement an indeterminate progress-bar

This patch implements indeterminate progress-bar in notifications, which will be
used by download notification.

Major changes:
- Implement indeterminate progress bar in the NotificationProgressBar class.
- Extract NotificationProgressBar into notification_progress_bar.[cc|h]
- Add NotificationProgressBar::SetIndeterminate(bool) method which set the
indetermination.

BUG=554916
TEST=manual test
==========

to

==========
Implement an indeterminate progress-bar

This patch implements indeterminate progress-bar in notifications, which will be
used by download notification.

Major changes:
- Implement indeterminate progress bar in the NotificationProgressBar class.
- Extract NotificationProgressBar into notification_progress_bar.[cc|h]
- Add NotificationProgressBar::SetIndeterminate(bool) method which set the
indetermination.

BUG=554916
TEST=manual test
==========

[commit-bot: I haz the power, 2015-11-26 10:09:34]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2015-11-26 10:10:31]

Description was changed from

==========
Implement an indeterminate progress-bar

This patch implements indeterminate progress-bar in notifications, which will be
used by download notification.

Major changes:
- Implement indeterminate progress bar in the NotificationProgressBar class.
- Extract NotificationProgressBar into notification_progress_bar.[cc|h]
- Add NotificationProgressBar::SetIndeterminate(bool) method which set the
indetermination.

BUG=554916
TEST=manual test
==========

to

==========
Implement an indeterminate progress-bar

This patch implements indeterminate progress-bar in notifications, which will be
used by download notification.

Major changes:
- Implement indeterminate progress bar in the NotificationProgressBar class.
- Extract NotificationProgressBar into notification_progress_bar.[cc|h]
- Add NotificationProgressBar::SetIndeterminate(bool) method which set the
indetermination.

BUG=554916
TEST=manual test

Committed: https://crrev.com/fc16346c607d1eff870269962062c20170f5475f
Cr-Commit-Position: refs/heads/master@{#361861}
==========

[commit-bot: I haz the power, 2015-11-26 10:10:32]

Patchset 4 (id:??) landed as
https://crrev.com/fc16346c607d1eff870269962062c20170f5475f
Cr-Commit-Position: refs/heads/master@{#361861}