Add a download button to the media player

Adding a download button to the media player controls.

This CL contains the logic behind a runtime enabled flag to display a download button for downloadable files. It does not contain the full implementation to download the media.

BUG=601247
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

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

[kdsilva, 2016-08-31 16:36:54]

Description was changed from

==========
Adding tests.

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

to

==========
Adding a download button to the media play controls.

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

[kdsilva, 2016-08-31 16:37:50]

Description was changed from

==========
Adding a download button to the media play controls.

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

to

==========
Adding a download button to the media player controls.

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

[kdsilva, 2016-09-07 15:10:28]

Description was changed from

==========
Adding a download button to the media player controls.

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

to

==========
Adding a download button to the media player controls.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[kdsilva, 2016-09-07 18:20:53]

Patchset #1 (id:1) has been deleted

[kdsilva, 2016-09-07 18:21:08]

Patchset #1 (id:20001) has been deleted

[kdsilva, 2016-09-07 18:35:54]

Description was changed from

==========
Adding a download button to the media player controls.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Adding a download button to the media player controls.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[kdsilva, 2016-09-07 18:38:43]

Description was changed from

==========
Adding a download button to the media player controls.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Adding a download button to the media player controls. 

This CL contains the logic to display the button when appropriate but does not
contain the full implementation to download the media.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[kdsilva, 2016-09-07 19:18:34]

Description was changed from

==========
Adding a download button to the media player controls. 

This CL contains the logic to display the button when appropriate but does not
contain the full implementation to download the media.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Adding a download button to the media player controls. 

This CL contains the logic behind a runtime enabled flag to display a download
button for downloadable files. It does not contain the full implementation to
download the media.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[kdsilva, 2016-09-07 19:22:20]

kdsilva@google.com changed reviewers:
+ avayvod@chromium.org, mounir.lamouri@gmail.com

[kdsilva, 2016-09-07 19:22:21]

This CL doesn't contain code to actually download media but instead the logic
for when to show the download button. Would appreciate any feedback! :)

[mlamouri (slow - plz ping), 2016-09-07 19:58:03]

mlamouri@chromium.org changed reviewers:
+ mlamouri@chromium.org
- mounir.lamouri@gmail.com

[whywhat, 2016-09-08 14:22:57]

lgtm

what are the plans for implementing the download? could you point to a bug in
your TODO or provide more details as to when and maybe how this will be
implemented?

[whywhat, 2016-09-08 14:23:28]

Also, add a bug number to the CL description.

[kdsilva, 2016-09-11 17:14:59]

The CQ bit was checked by kdsilva@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-11 17:15:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-11 17:17:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-11 17:17:41]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[kdsilva, 2016-09-12 09:51:58]

Patchset #7 (id:160001) has been deleted

[kdsilva, 2016-09-12 09:52:47]

The CQ bit was checked by kdsilva@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-12 09:53:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kdsilva, 2016-09-12 10:13:17]

Description was changed from

==========
Adding a download button to the media player controls. 

This CL contains the logic behind a runtime enabled flag to display a download
button for downloadable files. It does not contain the full implementation to
download the media.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Adding a download button to the media player controls. 

This CL contains the logic behind a runtime enabled flag to display a download
button for downloadable files. It does not contain the full implementation to
download the media.

BUG=601247
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[kdsilva, 2016-09-12 10:29:58]

The CQ bit was checked by kdsilva@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-12 10:30:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kdsilva, 2016-09-12 10:36:07]

Done. To answer your question, the code is in progress with the implementation
of the download button. I hope to have it ready for you to review by EOD today.

[mlamouri (slow - plz ping), 2016-09-12 11:06:25]

lgtm but can you add tests for MSE/MediaStream?

https://codereview.chromium.org/2293273002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp
(right):

https://codereview.chromium.org/2293273002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:653: if
(!HTMLMediaElement::isMediaStreamURL(url.getString()) && !url.protocolIs("blob")
&& !HTMLMediaSource::lookup(url)) {
Will trust avayvod@ on this :)

https://codereview.chromium.org/2293273002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:655: }
style: no { }

https://codereview.chromium.org/2293273002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:661: //
TODO(kdsilva): cr.bug/601247 The implementation for this download button
nit: cr.bug => https://crbug.com/

I would change the comment to: "implementation will be finished as part of
https://crbug.com/..."

https://codereview.chromium.org/2293273002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right):

https://codereview.chromium.org/2293273002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:312: }
style: no { }

[commit-bot: I haz the power, 2016-09-12 12:03:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-12 12:03:09]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kdsilva, 2016-09-12 13:20:57]

kdsilva@google.com changed reviewers:
+ clamy@chromium.org, foolip@chromium.org

[kdsilva, 2016-09-12 13:20:58]

@clamy: Could you rubber stamp the content code?
@foolip: Could you rubber stamp the Webkit code?

I'm in the process of applying @mlamouri's changes, but if you could take a look
in the meantime that would be great.

[clamy, 2016-09-12 14:12:29]

content/ lgtm.

[foolip, 2016-09-12 15:46:32]

rs lgtm

[kdsilva, 2016-09-12 15:56:03]

The CQ bit was checked by kdsilva@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-12 15:56:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kdsilva, 2016-09-12 17:25:49]

https://codereview.chromium.org/2293273002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp
(right):

https://codereview.chromium.org/2293273002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:655: }
On 2016/09/12 11:06:25, mlamouri wrote:
> style: no { }

Done.

https://codereview.chromium.org/2293273002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:661: //
TODO(kdsilva): cr.bug/601247 The implementation for this download button
On 2016/09/12 11:06:25, mlamouri wrote:
> nit: cr.bug => https://crbug.com/
> 
> I would change the comment to: "implementation will be finished as part of
> https://crbug.com/...%22

Done.

https://codereview.chromium.org/2293273002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right):

https://codereview.chromium.org/2293273002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:312: }
On 2016/09/12 11:06:25, mlamouri wrote:
> style: no { }

Done.

[mlamouri (slow - plz ping), 2016-09-12 17:33:07]

tests lgtm

https://codereview.chromium.org/2293273002/diff/220001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/media/video-controls-download-button-not-displayed-mse.html
(right):

https://codereview.chromium.org/2293273002/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/video-controls-download-button-not-displayed-mse.html:17:
v.srcObject = null;
Remove `v.srcObject=null`

[kdsilva, 2016-09-12 17:36:36]

The CQ bit was checked by kdsilva@google.com

[kdsilva, 2016-09-12 17:36:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
avayvod@chromium.org, clamy@chromium.org, foolip@chromium.org,
mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/2293273002/#ps240001
(title: "updated test, slightly")

[commit-bot: I haz the power, 2016-09-12 17:37:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kdsilva, 2016-09-12 17:37:54]

https://codereview.chromium.org/2293273002/diff/220001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/media/video-controls-download-button-not-displayed-mse.html
(right):

https://codereview.chromium.org/2293273002/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/video-controls-download-button-not-displayed-mse.html:17:
v.srcObject = null;
On 2016/09/12 17:33:07, mlamouri wrote:
> Remove `v.srcObject=null`

Done.

[commit-bot: I haz the power, 2016-09-12 19:20:54]

Description was changed from

==========
Adding a download button to the media player controls. 

This CL contains the logic behind a runtime enabled flag to display a download
button for downloadable files. It does not contain the full implementation to
download the media.

BUG=601247
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Adding a download button to the media player controls. 

This CL contains the logic behind a runtime enabled flag to display a download
button for downloadable files. It does not contain the full implementation to
download the media.

BUG=601247
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[commit-bot: I haz the power, 2016-09-12 19:20:55]

Committed patchset #10 (id:240001)

[commit-bot: I haz the power, 2016-09-12 19:22:37]

Description was changed from

==========
Adding a download button to the media player controls. 

This CL contains the logic behind a runtime enabled flag to display a download
button for downloadable files. It does not contain the full implementation to
download the media.

BUG=601247
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Adding a download button to the media player controls.

This CL contains the logic behind a runtime enabled flag to display a download
button for downloadable files. It does not contain the full implementation to
download the media.

BUG=601247
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

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

[commit-bot: I haz the power, 2016-09-12 19:22:39]

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