Implementation of media controls download button.

Implementation of media controls download button.

BUG=601247
Committed: https://crrev.com/dadcd47646c31ffc367fce0187369dfb84818773
Cr-Commit-Position: refs/heads/master@{#418540}

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

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

[commit-bot: I haz the power, 2016-09-12 17:28:14]

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 19:32:55]

Description was changed from

==========
Implementation of media controls download button.

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

to

==========
Implementation of media controls download button.

BUG=601247
==========

[kdsilva, 2016-09-12 19:33:12]

kdsilva@google.com changed reviewers:
+ avayvod@chromium.org, mlamouri@chromium.org

[commit-bot: I haz the power, 2016-09-12 21:25:13]

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

[commit-bot: I haz the power, 2016-09-12 21:25:14]

Dry run: This issue passed the CQ dry run.

[kdsilva, 2016-09-13 13:14:56]

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

[commit-bot: I haz the power, 2016-09-13 13:15:09]

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-13 14:04:31]

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

[commit-bot: I haz the power, 2016-09-13 14:04:32]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[kdsilva, 2016-09-13 15:33:57]

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

[commit-bot: I haz the power, 2016-09-13 15:34:08]

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

[whywhat, 2016-09-13 15:38:59]

On 2016/09/12 at 19:33:12, kdsilva wrote:
> kdsilva@google.com changed reviewers:
> + avayvod@chromium.org, mlamouri@chromium.org

I think you should also "Publish and mail comments" when mailing the reviewers
so they get an email (I don't think code review sends a notification if you add
a reviewer but don't send an email). I still remember looking at my reviews
queue yesterday and missing your CR anyway though. Sorry :)

[whywhat, 2016-09-13 15:52:16]

lgtm with a couple of nits

Re the image change, I assume that it updates how media document looks as well
and everyone is onboard with that.

https://codereview.chromium.org/2334803002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/video-controls-download-button-saves-media-expected.txt
(right):

https://codereview.chromium.org/2334803002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/video-controls-download-button-saves-media-expected.txt:1:
Downloading URL with suggested filename ""
I trust that this actually verifies the download :) (I assume this is the same
way MediaDocument download is tested). It's just not very explicit :) Maybe add
a comment above waitUntilExternalURLLoad that this is the test's exit condition?

https://codereview.chromium.org/2334803002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp
(right):

https://codereview.chromium.org/2334803002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:663: if
(event->type() == EventTypeNames::click && url != KURL()) {
nit: perhaps replace url != KURL() with !(url.IsNull() || url.IsEmpty()) ?
== operator for urls seems to invoke lots of methods for just comparing with a
new empty URL.
maybe it optimizes itself out though. I suggest checking manually at least that
null and empty urls will work correctly (I remember getting NPE with KURL just
for not checking IsNull in some places).

https://codereview.chromium.org/2334803002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (left):

https://codereview.chromium.org/2334803002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:261:
MediaControlsDownloadButton settable_from_internals=True
So we're removing the feature flag? You should mention that in the change
description (they are usually used to describe the changes you make :) )

[commit-bot: I haz the power, 2016-09-13 16:27:40]

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

[commit-bot: I haz the power, 2016-09-13 16:27:41]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[kdsilva, 2016-09-13 17:31:27]

Description was changed from

==========
Implementation of media controls download button.

BUG=601247
==========

to

==========
Implementation of media controls download button.

This implementation removes the MediaControlsDownloadButton Feature flag and
adds functionality to make the download button actually download media.

BUG=601247
==========

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

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

[commit-bot: I haz the power, 2016-09-13 17:32:49]

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-13 17:33:01]

@mlamouri, PTAL?

https://codereview.chromium.org/2334803002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/video-controls-download-button-saves-media-expected.txt
(right):

https://codereview.chromium.org/2334803002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/video-controls-download-button-saves-media-expected.txt:1:
Downloading URL with suggested filename ""
On 2016/09/13 15:52:16, whywhat wrote:
> I trust that this actually verifies the download :) (I assume this is the same
> way MediaDocument download is tested). It's just not very explicit :) Maybe
add
> a comment above waitUntilExternalURLLoad that this is the test's exit
condition?

Done. MediaDocument isn't testing this functionality (so far as I could tell),
but I found other examples of people using this way of testing download
functionality.

https://codereview.chromium.org/2334803002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp
(right):

https://codereview.chromium.org/2334803002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:663: if
(event->type() == EventTypeNames::click && url != KURL()) {
On 2016/09/13 15:52:16, whywhat wrote:
> nit: perhaps replace url != KURL() with !(url.IsNull() || url.IsEmpty()) ?
> == operator for urls seems to invoke lots of methods for just comparing with a
> new empty URL.
> maybe it optimizes itself out though. I suggest checking manually at least
that
> null and empty urls will work correctly (I remember getting NPE with KURL just
> for not checking IsNull in some places).

Done.

https://codereview.chromium.org/2334803002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (left):

https://codereview.chromium.org/2334803002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:261:
MediaControlsDownloadButton settable_from_internals=True
On 2016/09/13 15:52:16, whywhat wrote:
> So we're removing the feature flag? You should mention that in the change
> description (they are usually used to describe the changes you make :) )

Done.

[commit-bot: I haz the power, 2016-09-13 18:33:27]

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

[commit-bot: I haz the power, 2016-09-13 18:33:29]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[whywhat, 2016-09-13 19:39:22]

On 2016/09/13 at 17:33:01, kdsilva wrote:
> @mlamouri, PTAL?
> 
>
https://codereview.chromium.org/2334803002/diff/60001/third_party/WebKit/Layo...
> File
third_party/WebKit/LayoutTests/media/video-controls-download-button-saves-media-expected.txt
(right):
> 
>
https://codereview.chromium.org/2334803002/diff/60001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/media/video-controls-download-button-saves-media-expected.txt:1:
Downloading URL with suggested filename ""
> On 2016/09/13 15:52:16, whywhat wrote:
> > I trust that this actually verifies the download :) (I assume this is the
same
> > way MediaDocument download is tested). It's just not very explicit :) Maybe
add
> > a comment above waitUntilExternalURLLoad that this is the test's exit
condition?
> 
> Done. MediaDocument isn't testing this functionality (so far as I could tell),
but I found other examples of people using this way of testing download
functionality.
> 
>
https://codereview.chromium.org/2334803002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp
(right):
> 
>
https://codereview.chromium.org/2334803002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:663: if
(event->type() == EventTypeNames::click && url != KURL()) {
> On 2016/09/13 15:52:16, whywhat wrote:
> > nit: perhaps replace url != KURL() with !(url.IsNull() || url.IsEmpty()) ?
> > == operator for urls seems to invoke lots of methods for just comparing with
a
> > new empty URL.
> > maybe it optimizes itself out though. I suggest checking manually at least
that
> > null and empty urls will work correctly (I remember getting NPE with KURL
just
> > for not checking IsNull in some places).
> 
> Done.
> 
>
https://codereview.chromium.org/2334803002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (left):
> 
>
https://codereview.chromium.org/2334803002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:261:
MediaControlsDownloadButton settable_from_internals=True
> On 2016/09/13 15:52:16, whywhat wrote:
> > So we're removing the feature flag? You should mention that in the change
> > description (they are usually used to describe the changes you make :) )
> 
> Done.

Whops. Seems like the Cast tests are broken since they rely on the media
controls layout?

[kdsilva, 2016-09-14 08:50:38]

Description was changed from

==========
Implementation of media controls download button.

This implementation removes the MediaControlsDownloadButton Feature flag and
adds functionality to make the download button actually download media.

BUG=601247
==========

to

==========
Implementation of media controls download button.


BUG=601247
==========

[kdsilva, 2016-09-14 08:59:55]

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

[commit-bot: I haz the power, 2016-09-14 09:00:09]

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-14 10:19:44]

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

[commit-bot: I haz the power, 2016-09-14 10:20:06]

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

[mlamouri (slow - plz ping), 2016-09-14 11:22:32]

lgtm

[commit-bot: I haz the power, 2016-09-14 12:24:33]

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

[commit-bot: I haz the power, 2016-09-14 12:24:34]

Dry run: This issue passed the CQ dry run.

[kdsilva, 2016-09-14 12:25:46]

The CQ bit was checked by kdsilva@google.com

[kdsilva, 2016-09-14 12:25:47]

The patchset sent to the CQ was uploaded after l-g-t-m from avayvod@chromium.org
Link to the patchset: https://codereview.chromium.org/2334803002/#ps120001
(title: "fixed tests")

[commit-bot: I haz the power, 2016-09-14 12:26:06]

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-14 12:33:12]

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

[commit-bot: I haz the power, 2016-09-14 12:33:13]

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...)

[kdsilva, 2016-09-14 12:44:31]

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

[kdsilva, 2016-09-14 12:44:31]

@foolip: Could you rubber stamp the Webkit code?

[foolip, 2016-09-14 12:47:41]

third_party/WebKit/public/default_100_percent/blink/mediaplayer_download.png
lgtm

[kdsilva, 2016-09-14 12:51:23]

The CQ bit was checked by kdsilva@google.com

[commit-bot: I haz the power, 2016-09-14 12:51:38]

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-14 12:56:32]

Description was changed from

==========
Implementation of media controls download button.


BUG=601247
==========

to

==========
Implementation of media controls download button.


BUG=601247
==========

[commit-bot: I haz the power, 2016-09-14 12:56:34]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-09-14 12:57:48]

Description was changed from

==========
Implementation of media controls download button.


BUG=601247
==========

to

==========
Implementation of media controls download button.

BUG=601247

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

[commit-bot: I haz the power, 2016-09-14 12:57:49]

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