Added the reply/loop icon to the video player.

Added the reply/loop icon to the video player.

Once the video is finished, the play button's icon is reply instead of play. The same will be done, once assets for the audio player are provided. Along the way, converted the class names from ambiguous state0/1/2 to default, playing, ended.

TEST=Launch a video in Files.app, wait until end or scroll to the end.
BUG=177591
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=187255

[mtomasz, 2013-03-08 04:29:00]

@yoshiki: PTAL.

[yoshiki, 2013-03-08 08:20:07]

https://chromiumcodereview.appspot.com/12676002/diff/9001/chrome/browser/reso...
File chrome/browser/resources/file_manager/js/media/media_controls.js (right):

https://chromiumcodereview.appspot.com/12676002/diff/9001/chrome/browser/reso...
chrome/browser/resources/file_manager/js/media/media_controls.js:73: * @param
{number=} opt_numStates Number of states, default: 1.
Expressing the status as a raw number is difficult to understand. Could you
create an enum to express the status and use it? (as enum's in file_manager.js)

https://chromiumcodereview.appspot.com/12676002/diff/9001/chrome/browser/reso...
chrome/browser/resources/file_manager/js/media/media_controls.js:78:
opt_numStates = opt_numStates || 1;
nit: |opt_numStates| is no longer optional. Please rename.

var numStates = opt_numStates || 1;

[mtomasz, 2013-03-08 09:10:00]

https://codereview.chromium.org/12676002/diff/9001/chrome/browser/resources/f...
File chrome/browser/resources/file_manager/js/media/media_controls.js (right):

https://codereview.chromium.org/12676002/diff/9001/chrome/browser/resources/f...
chrome/browser/resources/file_manager/js/media/media_controls.js:78:
opt_numStates = opt_numStates || 1;
On 2013/03/08 08:20:07, yoshiki wrote:
> nit: |opt_numStates| is no longer optional. Please rename.
> 
> var numStates = opt_numStates || 1;

We use this pattern very often. opt_sth = opt_sth || default_value. Do we really
want an additional variable for every optional argument? If so, then we have to
change it in tons of places.

[yoshiki, 2013-03-11 01:45:32]

lgtm

https://codereview.chromium.org/12676002/diff/9001/chrome/browser/resources/f...
File chrome/browser/resources/file_manager/js/media/media_controls.js (right):

https://codereview.chromium.org/12676002/diff/9001/chrome/browser/resources/f...
chrome/browser/resources/file_manager/js/media/media_controls.js:78:
opt_numStates = opt_numStates || 1;
I prefer adding additional value, but it's not strong feeling. I will leave the
choice to you.

On 2013/03/08 09:10:00, mtomasz wrote:
> On 2013/03/08 08:20:07, yoshiki wrote:
> > nit: |opt_numStates| is no longer optional. Please rename.
> > 
> > var numStates = opt_numStates || 1;
> 
> We use this pattern very often. opt_sth = opt_sth || default_value. Do we
really
> want an additional variable for every optional argument? If so, then we have
to
> change it in tons of places.

[mtomasz, 2013-03-11 03:47:15]

I've just addressed your comments. PTAL one more time.

https://codereview.chromium.org/12676002/diff/9001/chrome/browser/resources/f...
File chrome/browser/resources/file_manager/js/media/media_controls.js (right):

https://codereview.chromium.org/12676002/diff/9001/chrome/browser/resources/f...
chrome/browser/resources/file_manager/js/media/media_controls.js:73: * @param
{number=} opt_numStates Number of states, default: 1.
On 2013/03/08 08:20:07, yoshiki wrote:
> Expressing the status as a raw number is difficult to understand. Could you
> create an enum to express the status and use it? (as enum's in
file_manager.js)

Done.

https://codereview.chromium.org/12676002/diff/9001/chrome/browser/resources/f...
chrome/browser/resources/file_manager/js/media/media_controls.js:78:
opt_numStates = opt_numStates || 1;
On 2013/03/11 01:45:32, yoshiki wrote:
> I prefer adding additional value, but it's not strong feeling. I will leave
the
> choice to you.
> 
> On 2013/03/08 09:10:00, mtomasz wrote:
> > On 2013/03/08 08:20:07, yoshiki wrote:
> > > nit: |opt_numStates| is no longer optional. Please rename.
> > > 
> > > var numStates = opt_numStates || 1;
> > 
> > We use this pattern very often. opt_sth = opt_sth || default_value. Do we
> really
> > want an additional variable for every optional argument? If so, then we have
> to
> > change it in tons of places. 
> 

The reason I'd prefer this is that I don't think there is something wrong in
setting a default value to an optional argument. This trick opt_x = opt_x ||
default_value is an emulation of function(opt_argument = default_value), which
is not supported in Javascript, but it is in eg. C++. We used this trick
extensively in my previous project. Moreover, creating a second variable x =
opt_x || default_value causes having two variables x and opt_x, which can be bug
prone. Anyway, I'll leave it as it is for now, but I am open to discuss in the
future about it. Done.

[yoshiki, 2013-03-11 04:42:45]

https://codereview.chromium.org/12676002/diff/9001/chrome/browser/resources/f...
File chrome/browser/resources/file_manager/js/media/media_controls.js (right):

https://codereview.chromium.org/12676002/diff/9001/chrome/browser/resources/f...
chrome/browser/resources/file_manager/js/media/media_controls.js:78:
opt_numStates = opt_numStates || 1;
Ok, let's leave it as it is. 

But the code may be warned by closure compiler, because it does implicit
type-cast from |Type=| (undefined-able) to |Type| (non-undefined-able). Perhaps,
I'll do bulk-replace when applying closure compiler in the feature.

On 2013/03/11 03:47:16, mtomasz wrote:
> On 2013/03/11 01:45:32, yoshiki wrote:
> > I prefer adding additional value, but it's not strong feeling. I will leave
> the
> > choice to you.
> > 
> > On 2013/03/08 09:10:00, mtomasz wrote:
> > > On 2013/03/08 08:20:07, yoshiki wrote:
> > > > nit: |opt_numStates| is no longer optional. Please rename.
> > > > 
> > > > var numStates = opt_numStates || 1;
> > > 
> > > We use this pattern very often. opt_sth = opt_sth || default_value. Do we
> > really
> > > want an additional variable for every optional argument? If so, then we
have
> > to
> > > change it in tons of places. 
> > 
> 
> The reason I'd prefer this is that I don't think there is something wrong in
> setting a default value to an optional argument. This trick opt_x = opt_x ||
> default_value is an emulation of function(opt_argument = default_value), which
> is not supported in Javascript, but it is in eg. C++. We used this trick
> extensively in my previous project. Moreover, creating a second variable x =
> opt_x || default_value causes having two variables x and opt_x, which can be
bug
> prone. Anyway, I'll leave it as it is for now, but I am open to discuss in the
> future about it. Done.

https://codereview.chromium.org/12676002/diff/17001/chrome/browser/resources/...
File chrome/browser/resources/file_manager/js/media/media_controls.js (right):

https://codereview.chromium.org/12676002/diff/17001/chrome/browser/resources/...
chrome/browser/resources/file_manager/js/media/media_controls.js:83: * @param
{number=} opt_numStates Number of states, default: 1.
Could you change the type of the last argument to MediaControls.ButtonStateType=
from number=?

https://codereview.chromium.org/12676002/diff/17001/chrome/browser/resources/...
chrome/browser/resources/file_manager/js/media/media_controls.js:166:
this.togglePlayState.bind(this), opt_parent, 3 /* States. */);
Could you change "3" to MediaControls.ButtonStateType.ENDED?

And 3 is out of the array :-)

[mtomasz, 2013-03-11 04:48:15]

https://codereview.chromium.org/12676002/diff/17001/chrome/browser/resources/...
File chrome/browser/resources/file_manager/js/media/media_controls.js (right):

https://codereview.chromium.org/12676002/diff/17001/chrome/browser/resources/...
chrome/browser/resources/file_manager/js/media/media_controls.js:83: * @param
{number=} opt_numStates Number of states, default: 1.
On 2013/03/11 04:42:45, yoshiki wrote:
> Could you change the type of the last argument to
MediaControls.ButtonStateType=
> from number=?

I think it would make the code worse. The enum contains classes. Passing
MediaControls.ButtonStateType.PLAYING doesn't make much sense in my opinion. How
about passing an array of allowed states? Eg.

createButton(..., [MediaControls.ButtonStateType.DEFAULT,
MediaControls.ButtonStateType.PLAYING]);

This is quite long, but logical. WDYT?

https://codereview.chromium.org/12676002/diff/17001/chrome/browser/resources/...
chrome/browser/resources/file_manager/js/media/media_controls.js:166:
this.togglePlayState.bind(this), opt_parent, 3 /* States. */);
On 2013/03/11 04:42:45, yoshiki wrote:
> Could you change "3" to MediaControls.ButtonStateType.ENDED?
> 
> And 3 is out of the array :-)

It is not out of array. This is not an index, but size. So for 3, states 0, 1, 2
will be created.

[yoshiki, 2013-03-11 05:17:03]

lgtm

https://codereview.chromium.org/12676002/diff/17001/chrome/browser/resources/...
File chrome/browser/resources/file_manager/js/media/media_controls.js (right):

https://codereview.chromium.org/12676002/diff/17001/chrome/browser/resources/...
chrome/browser/resources/file_manager/js/media/media_controls.js:83: * @param
{number=} opt_numStates Number of states, default: 1.
On 2013/03/11 04:48:15, mtomasz wrote:
> On 2013/03/11 04:42:45, yoshiki wrote:
> > Could you change the type of the last argument to
> MediaControls.ButtonStateType=
> > from number=?
> 
> I think it would make the code worse. The enum contains classes. Passing
> MediaControls.ButtonStateType.PLAYING doesn't make much sense in my opinion.
How
> about passing an array of allowed states? Eg.
> 
> createButton(..., [MediaControls.ButtonStateType.DEFAULT,
> MediaControls.ButtonStateType.PLAYING]);
> 
> This is quite long, but logical. WDYT?

Sorry, I misunderstood. |number=| is ok

https://codereview.chromium.org/12676002/diff/17001/chrome/browser/resources/...
chrome/browser/resources/file_manager/js/media/media_controls.js:166:
this.togglePlayState.bind(this), opt_parent, 3 /* States. */);
On 2013/03/11 04:48:15, mtomasz wrote:
> On 2013/03/11 04:42:45, yoshiki wrote:
> > Could you change "3" to MediaControls.ButtonStateType.ENDED?
> > 
> > And 3 is out of the array :-)
> 
> It is not out of array. This is not an index, but size. So for 3, states 0, 1,
2
> will be created.

ditto, "3" is ok

[commit-bot: I haz the power, 2013-03-11 05:46:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtomasz@chromium.org/12676002/17001

[commit-bot: I haz the power, 2013-03-11 08:51:38]

Change committed as 187255