AudioPlayer.app: Migrate to Polymer 1.0.

AudioPlayer.app: Migrate to Polymer 1.0.

BUG=492939
Committed: https://crrev.com/a9000d248dfb581ac681c5f4642f748857f86dba
Cr-Commit-Position: refs/heads/master@{#333701}

[fukino, 2015-06-10 04:36:40]

fukino@chromium.org changed reviewers:
+ yawano@chromium.org

[fukino, 2015-06-10 04:36:41]

Could you take a look? Thanks!

[yawano, 2015-06-10 05:47:48]

https://codereview.chromium.org/1176483002/diff/20001/ui/file_manager/audio_p...
File ui/file_manager/audio_player/elements/control_panel.js (right):

https://codereview.chromium.org/1176483002/diff/20001/ui/file_manager/audio_p...
ui/file_manager/audio_player/elements/control_panel.js:62: notify: true
notify: false? Since this is property is bound as one-way.

https://codereview.chromium.org/1176483002/diff/20001/ui/file_manager/audio_p...
ui/file_manager/audio_player/elements/control_panel.js:102: value: false,
nit: Why some properties have default value and some don't? (consistency).

https://codereview.chromium.org/1176483002/diff/20001/ui/file_manager/audio_p...
File ui/file_manager/audio_player/elements/volume_controller.js (right):

https://codereview.chromium.org/1176483002/diff/20001/ui/file_manager/audio_p...
ui/file_manager/audio_player/elements/volume_controller.js:99: if (oldValue !=
newValue)
very small nit: !==

https://codereview.chromium.org/1176483002/diff/20001/ui/file_manager/audio_p...
File ui/file_manager/audio_player/js/audio_player.js (right):

https://codereview.chromium.org/1176483002/diff/20001/ui/file_manager/audio_p...
ui/file_manager/audio_player/js/audio_player.js:55:
this.onModelExpandedChanged(false, true);
Can we remove this onModelExpanededChanged call if we add change.type === 'add'
to around line 27?

[fukino, 2015-06-10 06:24:03]

Thanks!

https://codereview.chromium.org/1176483002/diff/20001/ui/file_manager/audio_p...
File ui/file_manager/audio_player/elements/control_panel.js (right):

https://codereview.chromium.org/1176483002/diff/20001/ui/file_manager/audio_p...
ui/file_manager/audio_player/elements/control_panel.js:62: notify: true
On 2015/06/10 05:47:48, yawano wrote:
> notify: false? Since this is property is bound as one-way.

Yes, you are right. Done.

https://codereview.chromium.org/1176483002/diff/20001/ui/file_manager/audio_p...
ui/file_manager/audio_player/elements/control_panel.js:102: value: false,
On 2015/06/10 05:47:48, yawano wrote:
> nit: Why some properties have default value and some don't? (consistency).

Basically, it was based on whether we have default value before this CL.

However, I rewrote this CL to update default values following this criteria.
- For properties which are intended to be bound to child elements' properties,
omit default values.
- Otherwise, write default values explicitly.
WDYT?

https://codereview.chromium.org/1176483002/diff/20001/ui/file_manager/audio_p...
File ui/file_manager/audio_player/elements/volume_controller.js (right):

https://codereview.chromium.org/1176483002/diff/20001/ui/file_manager/audio_p...
ui/file_manager/audio_player/elements/volume_controller.js:99: if (oldValue !=
newValue)
On 2015/06/10 05:47:48, yawano wrote:
> very small nit: !==

Done.

https://codereview.chromium.org/1176483002/diff/20001/ui/file_manager/audio_p...
File ui/file_manager/audio_player/js/audio_player.js (right):

https://codereview.chromium.org/1176483002/diff/20001/ui/file_manager/audio_p...
ui/file_manager/audio_player/js/audio_player.js:55:
this.onModelExpandedChanged(false, true);
On 2015/06/10 05:47:48, yawano wrote:
> Can we remove this onModelExpanededChanged call if we add change.type ===
'add'
> to around line 27?

Done. Thank you for pointing this out!

[yawano, 2015-06-10 06:46:18]

lgtm. Thank you!

https://codereview.chromium.org/1176483002/diff/20001/ui/file_manager/audio_p...
File ui/file_manager/audio_player/elements/control_panel.js (right):

https://codereview.chromium.org/1176483002/diff/20001/ui/file_manager/audio_p...
ui/file_manager/audio_player/elements/control_panel.js:102: value: false,
SGTM.

https://codereview.chromium.org/1176483002/diff/40001/ui/file_manager/audio_p...
File ui/file_manager/audio_player/elements/audio_player.js (right):

https://codereview.chromium.org/1176483002/diff/40001/ui/file_manager/audio_p...
ui/file_manager/audio_player/elements/audio_player.js:173: if (!this.trackList)
Can we remove this condition if we access it via this.$.trackList? (It's okay to
do this in a separate CL).

[fukino, 2015-06-10 07:00:22]

I updated the CL to use this.$ consistently.
Could you take another look?

https://codereview.chromium.org/1176483002/diff/40001/ui/file_manager/audio_p...
File ui/file_manager/audio_player/elements/audio_player.js (right):

https://codereview.chromium.org/1176483002/diff/40001/ui/file_manager/audio_p...
ui/file_manager/audio_player/elements/audio_player.js:173: if (!this.trackList)
On 2015/06/10 06:46:18, yawano wrote:
> Can we remove this condition if we access it via this.$.trackList? (It's okay
to
> do this in a separate CL).

Yes, I updated the CL to use this.$ consistently, to be ready for
propertyChanged event on setting default values.

[yawano, 2015-06-10 07:15:38]

Still lgtm!

https://codereview.chromium.org/1176483002/diff/60001/ui/file_manager/audio_p...
File ui/file_manager/audio_player/elements/audio_player.js (right):

https://codereview.chromium.org/1176483002/diff/60001/ui/file_manager/audio_p...
ui/file_manager/audio_player/elements/audio_player.js:279:
(this.lastAudioUpdateTime_ = this.$.audio.currentTime * 1000);
nit: Can we put this in one line?

[fukino, 2015-06-10 07:18:13]

Thank you!

https://codereview.chromium.org/1176483002/diff/60001/ui/file_manager/audio_p...
File ui/file_manager/audio_player/elements/audio_player.js (right):

https://codereview.chromium.org/1176483002/diff/60001/ui/file_manager/audio_p...
ui/file_manager/audio_player/elements/audio_player.js:279:
(this.lastAudioUpdateTime_ = this.$.audio.currentTime * 1000);
On 2015/06/10 07:15:38, yawano wrote:
> nit: Can we put this in one line?

Done.

[fukino, 2015-06-10 07:18:49]

The CQ bit was checked by fukino@chromium.org

[fukino, 2015-06-10 07:18:50]

The patchset sent to the CQ was uploaded after l-g-t-m from yawano@chromium.org
Link to the patchset: https://codereview.chromium.org/1176483002/#ps80001
(title: "Remove unnecessary line break.")

[commit-bot: I haz the power, 2015-06-10 07:19:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1176483002/80001

[commit-bot: I haz the power, 2015-06-10 07:54:29]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-06-10 07:56:14]

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