Add download button to MediaDocument on Android.

Add download button to MediaDocument on Android.

For a MediaDocument on android, it is not easy
to long press the element and download audio.
This change adds a download button to the MediaDocument on android.
This facilitates user to save the media file locally.

UI
Review: https://groups.google.com/a/google.com/forum/#!msg/chrome-ui-review/XzyQ2QsVyRE/0x5jU-KECwAJ
Mocks: https://folio.googleplex.com/chrome-ux/mocks/065-downloads/06-media/022216_Buttons

BUG=590043
Committed: https://crrev.com/20651adff7805d35635fcf6a2be9fd6a1e01b40b
Cr-Commit-Position: refs/heads/master@{#385607}

[qinmin, 2016-03-09 23:04:01]

qinmin@chromium.org changed reviewers:
+ esprehn@chromium.org, jochen@chromium.org, philipj@opera.com

[qinmin, 2016-03-09 23:04:02]

philipj@ for media review
esprehn@ for WebKit/Source/Platform OWNER
jochen@ for content/ OWNER

philipj@, you might not be able to open the UI review and mocks page, but they
are essentially just for displaying this button

[philipj_slow, 2016-03-10 06:37:36]

In Opera for Android we've added a download button as part of the media controls
themselves. I'd be happy to upstream this if you'd like it. We could arrange
things such that by default that button only shows up in MediaDocument. Would
that be an acceptable UX, or do you think it's important that the button be very
big?

[philipj_slow, 2016-03-10 06:45:19]

Here's a screenshot: http://imgur.com/CFHEBSc

[esprehn, 2016-03-10 07:09:30]

I like the opera UI, could we just do that?

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[esprehn, 2016-03-10 07:10:23]

I like the opera UI, could we just do that?

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[philipj_slow, 2016-03-10 08:48:16]

In addition to MediaDocument, it would be a straightforward spec change to allow
sites to opt in to a download button in any context:
<video controls download src="stuff.webm">

If you want it in all contexts like Opera for Android I'd obviously have no
complaint, either.

[qinmin, 2016-03-10 21:21:18]

On 2016/03/10 08:48:16, philipj_UTC7 wrote:
> In addition to MediaDocument, it would be a straightforward spec change to
allow
> sites to opt in to a download button in any context:
> <video controls download src="stuff.webm">
> 
> If you want it in all contexts like Opera for Android I'd obviously have no
> complaint, either.

philipj@, do you have any usage stats on the download button?
The media controller already has a lot of things on it, play/pause button, seek
slider, current time, cast button, fullscreen button, mute/unmute button, cc
button, so our UI team is looking at standalone download button, rather than
making the controller overcrowded

[qinmin, 2016-03-11 00:19:24]

On 2016/03/10 21:21:18, qinmin wrote:
> On 2016/03/10 08:48:16, philipj_UTC7 wrote:
> > In addition to MediaDocument, it would be a straightforward spec change to
> allow
> > sites to opt in to a download button in any context:
> > <video controls download src="stuff.webm">
> > 
> > If you want it in all contexts like Opera for Android I'd obviously have no
> > complaint, either.
> 
> philipj@, do you have any usage stats on the download button?
> The media controller already has a lot of things on it, play/pause button,
seek
> slider, current time, cast button, fullscreen button, mute/unmute button, cc
> button, so our UI team is looking at standalone download button, rather than
> making the controller overcrowded

BTW, opera is welcome to upload its implemntation of the download button on the
media controller(possibly also behind a flag), but chrome UI folks prefer a
standalone download button for now, we might move it to the media controller in
future when there are better UI designs to solve the crowded issue

[esprehn, 2016-03-11 00:35:33]

This doesn't seem like the right way to handle this, resize events are async,
which means your download button will lag behind the correct location. I think
you need to use flex box with wrapper divs and alignment or something. Trying to
react to the video sizing like this isn't right.

https://codereview.chromium.org/1780043002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/MediaDocument.cpp (right):

https://codereview.chromium.org/1780043002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/MediaDocument.cpp:74: class
MediaEventListener : public EventListener {
final

https://codereview.chromium.org/1780043002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/MediaDocument.cpp:81: static const
MediaEventListener* cast(const EventListener* listener)
this cast method doesn't seem used outside ==, just inline the type() == check
there.

https://codereview.chromium.org/1780043002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/MediaDocument.cpp:96:
MediaEventListener(MediaDocument* document)
explicit

https://codereview.chromium.org/1780043002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/MediaDocument.cpp:98: , m_doc(document)
m_document, don't abbreviate

https://codereview.chromium.org/1780043002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/MediaDocument.cpp:104:
m_doc->updateLayout();
you don't need the m_document thing, just do toDocument on the ExecutionContext
argument, then check isMediaDocument(), then
toMediaDocument(document)->doThing()

https://codereview.chromium.org/1780043002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/MediaDocument.cpp:142:
RefPtrWillBeRawPtr<EventListener> listener =
MediaEventListener::create(static_cast<MediaDocument*>(document()));
toMediaDocument, don't static cast. If toMediaDocument doesn't exist lets add it
with the casting macros

https://codereview.chromium.org/1780043002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/MediaDocument.cpp:143:
media->addEventListener("resize", listener, false);
forcing a layout for ever resize event doesn't make sense, don't do that

https://codereview.chromium.org/1780043002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/MediaDocument.cpp:148:
anchor->setInnerText(document()->getCachedLocale(document()->contentLanguage()).queryString(WebLocalizedString::DownloadButtonLabel),
ASSERT_NO_EXCEPTION);
setTextContent, don't use setInnerText

https://codereview.chromium.org/1780043002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/MediaDocument.cpp:162:
"text-decoration:none;"
missing space, also this is so much stuff. Is there really no .css file for
media documents? We should really add one instead

https://codereview.chromium.org/1780043002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/MediaDocument.cpp:233: void
MediaDocument::updateLayout()
updateLayout is a real method you're shadowing, you need a different name. This
should just be handleResize or something.

https://codereview.chromium.org/1780043002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/MediaDocument.cpp:238: int top =
mediaElement->offsetTop() + mediaElement->offsetHeight() + 36;
this doesn't seem right, what is 36? This seems really hacky setting the top and
marginTop like this.

https://codereview.chromium.org/1780043002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/MediaDocument.cpp:240:
anchorElement->setInlineStyleProperty(CSSPropertyMarginTop, 0,
CSSPrimitiveValue::UnitType::Pixels);
huh? why always set to 0?

[qinmin, 2016-03-11 23:46:42]

On 2016/03/11 00:35:33, esprehn wrote:
> This doesn't seem like the right way to handle this, resize events are async,
> which means your download button will lag behind the correct location. I think
> you need to use flex box with wrapper divs and alignment or something. Trying
to
> react to the video sizing like this isn't right.

Using flex box doesn't really help here. According to the UI design, the video
element still needs to be centered with max width and height at 100%. 
And the download button needs to be 32px below the video according to android
material design.
Flexbox cannot achieve such layout requirement.
I have changed the CL to use a CSS transform by placing the media element and
the button inside an absolutely positioned div element.
In order to center the media element, I also added a padding on top of the media
element to compensate for the height of the button.
PTAL again.

> 
>
https://codereview.chromium.org/1780043002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/html/MediaDocument.cpp (right):
> 
>
https://codereview.chromium.org/1780043002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/html/MediaDocument.cpp:74: class
> MediaEventListener : public EventListener {
> final
> 
>
https://codereview.chromium.org/1780043002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/html/MediaDocument.cpp:81: static const
> MediaEventListener* cast(const EventListener* listener)
> this cast method doesn't seem used outside ==, just inline the type() == check
> there.
> 
>
https://codereview.chromium.org/1780043002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/html/MediaDocument.cpp:96:
> MediaEventListener(MediaDocument* document)
> explicit
> 
>
https://codereview.chromium.org/1780043002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/html/MediaDocument.cpp:98: , m_doc(document)
> m_document, don't abbreviate
> 
>
https://codereview.chromium.org/1780043002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/html/MediaDocument.cpp:104:
> m_doc->updateLayout();
> you don't need the m_document thing, just do toDocument on the
ExecutionContext
> argument, then check isMediaDocument(), then
> toMediaDocument(document)->doThing()
> 
>
https://codereview.chromium.org/1780043002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/html/MediaDocument.cpp:142:
> RefPtrWillBeRawPtr<EventListener> listener =
> MediaEventListener::create(static_cast<MediaDocument*>(document()));
> toMediaDocument, don't static cast. If toMediaDocument doesn't exist lets add
it
> with the casting macros
> 
>
https://codereview.chromium.org/1780043002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/html/MediaDocument.cpp:143:
> media->addEventListener("resize", listener, false);
> forcing a layout for ever resize event doesn't make sense, don't do that
> 
>
https://codereview.chromium.org/1780043002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/html/MediaDocument.cpp:148:
>
anchor->setInnerText(document()->getCachedLocale(document()->contentLanguage()).queryString(WebLocalizedString::DownloadButtonLabel),
> ASSERT_NO_EXCEPTION);
> setTextContent, don't use setInnerText
> 
>
https://codereview.chromium.org/1780043002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/html/MediaDocument.cpp:162:
> "text-decoration:none;"
> missing space, also this is so much stuff. Is there really no .css file for
> media documents? We should really add one instead
> 
>
https://codereview.chromium.org/1780043002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/html/MediaDocument.cpp:233: void
> MediaDocument::updateLayout()
> updateLayout is a real method you're shadowing, you need a different name.
This
> should just be handleResize or something.
> 
>
https://codereview.chromium.org/1780043002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/html/MediaDocument.cpp:238: int top =
> mediaElement->offsetTop() + mediaElement->offsetHeight() + 36;
> this doesn't seem right, what is 36? This seems really hacky setting the top
and
> marginTop like this.
> 
>
https://codereview.chromium.org/1780043002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/html/MediaDocument.cpp:240:
> anchorElement->setInlineStyleProperty(CSSPropertyMarginTop, 0,
> CSSPrimitiveValue::UnitType::Pixels);
> huh? why always set to 0?

[qinmin, 2016-03-11 23:50:55]

https://codereview.chromium.org/1780043002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/MediaDocument.cpp (right):

https://codereview.chromium.org/1780043002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/MediaDocument.cpp:74: class
MediaEventListener : public EventListener {
On 2016/03/11 00:35:32, esprehn wrote:
> final

Done. This class is no longer needed

https://codereview.chromium.org/1780043002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/MediaDocument.cpp:81: static const
MediaEventListener* cast(const EventListener* listener)
On 2016/03/11 00:35:32, esprehn wrote:
> this cast method doesn't seem used outside ==, just inline the type() == check
> there.

Done. Class removed

https://codereview.chromium.org/1780043002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/MediaDocument.cpp:96:
MediaEventListener(MediaDocument* document)
On 2016/03/11 00:35:33, esprehn wrote:
> explicit

Class removed.

https://codereview.chromium.org/1780043002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/MediaDocument.cpp:98: , m_doc(document)
On 2016/03/11 00:35:33, esprehn wrote:
> m_document, don't abbreviate

Class removed

https://codereview.chromium.org/1780043002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/MediaDocument.cpp:104:
m_doc->updateLayout();
On 2016/03/11 00:35:33, esprehn wrote:
> you don't need the m_document thing, just do toDocument on the
ExecutionContext
> argument, then check isMediaDocument(), then
> toMediaDocument(document)->doThing()

Class removed

https://codereview.chromium.org/1780043002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/MediaDocument.cpp:142:
RefPtrWillBeRawPtr<EventListener> listener =
MediaEventListener::create(static_cast<MediaDocument*>(document()));
On 2016/03/11 00:35:33, esprehn wrote:
> toMediaDocument, don't static cast. If toMediaDocument doesn't exist lets add
it
> with the casting macros

Line removed

https://codereview.chromium.org/1780043002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/MediaDocument.cpp:143:
media->addEventListener("resize", listener, false);
On 2016/03/11 00:35:33, esprehn wrote:
> forcing a layout for ever resize event doesn't make sense, don't do that

removed

https://codereview.chromium.org/1780043002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/MediaDocument.cpp:148:
anchor->setInnerText(document()->getCachedLocale(document()->contentLanguage()).queryString(WebLocalizedString::DownloadButtonLabel),
ASSERT_NO_EXCEPTION);
On 2016/03/11 00:35:32, esprehn wrote:
> setTextContent, don't use setInnerText

Done.

https://codereview.chromium.org/1780043002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/MediaDocument.cpp:162:
"text-decoration:none;"
On 2016/03/11 00:35:33, esprehn wrote:
> missing space, also this is so much stuff. Is there really no .css file for
> media documents? We should really add one instead

Done. As I mentioned in the TODO above, these css stule can be moved to
MediaCOntrolsAndroidNew.css.
However, there is currently a layout bug(http://crbug.com/592080) preventing us
from doing this.
If these css style are moved to the .css file, then when the MediaDocument get
loaded, the layout is incorrect as if the css is not applied.
But when I reload the page, the css will be applied. So somehow we are not
applying the css correctly for MediaDocument, and I suspect the same happens for
ImageDocument, etc. I have talked to xianzhu and he thinks that we are probably
missing one layout if the css is external.
So I will leave this inside this file until that bug is addressed.

https://codereview.chromium.org/1780043002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/MediaDocument.cpp:233: void
MediaDocument::updateLayout()
On 2016/03/11 00:35:33, esprehn wrote:
> updateLayout is a real method you're shadowing, you need a different name.
This
> should just be handleResize or something.

removed

https://codereview.chromium.org/1780043002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/MediaDocument.cpp:238: int top =
mediaElement->offsetTop() + mediaElement->offsetHeight() + 36;
On 2016/03/11 00:35:33, esprehn wrote:
> this doesn't seem right, what is 36? This seems really hacky setting the top
and
> marginTop like this.

This should actually be 32. It is to move the button 32 px below the
mediaElement to conform to the android material design.
See
https://docs.google.com/document/d/1SqmvJjxhmDvdgzqQEIls129ZYq2QFnvcSYbwbTfZ4...
for the UI design.
Anyway, moved this to the css above

https://codereview.chromium.org/1780043002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/MediaDocument.cpp:240:
anchorElement->setInlineStyleProperty(CSSPropertyMarginTop, 0,
CSSPrimitiveValue::UnitType::Pixels);
On 2016/03/11 00:35:32, esprehn wrote:
> huh? why always set to 0?

This is margin top. Since both the video and anchor position are absolute, so as
long as the button is 36px below the video, we don't need margin top.

[rune, 2016-03-12 01:20:39]

On 2016/03/11 23:50:55, qinmin wrote:

> On 2016/03/11 00:35:33, esprehn wrote:
> > missing space, also this is so much stuff. Is there really no .css file for
> > media documents? We should really add one instead
> 
> Done. As I mentioned in the TODO above, these css stule can be moved to
> MediaCOntrolsAndroidNew.css.
> However, there is currently a layout bug(http://crbug.com/592080) preventing
us
> from doing this.

I've added a comment in the issue about why I think this happens.

[qinmin, 2016-03-13 01:06:08]

On 2016/03/12 01:20:39, rune wrote:
> On 2016/03/11 23:50:55, qinmin wrote:
> 
> > On 2016/03/11 00:35:33, esprehn wrote:
> > > missing space, also this is so much stuff. Is there really no .css file
for
> > > media documents? We should really add one instead
> > 
> > Done. As I mentioned in the TODO above, these css stule can be moved to
> > MediaCOntrolsAndroidNew.css.
> > However, there is currently a layout bug(http://crbug.com/592080) preventing
> us
> > from doing this.
> 
> I've added a comment in the issue about why I think this happens.

Moved all styles to html.css works, thanks for the info. Updated the CL.

[qinmin, 2016-03-15 17:04:06]

ping. esprehn@, would you please take a look again? thanks

[esprehn, 2016-03-15 17:33:07]

This makes div and <a> slower for the whole web. We can't put stuff like that
into html.css lets put it into a style element in the media document instead?

https://codereview.chromium.org/1780043002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/html.css (right):

https://codereview.chromium.org/1780043002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/html.css:66: body:-webkit-full-page-media >
div > a {
We can't do this. It adds rules for every <a> in every page. And marks bits on
the ancestors, it means children of all divs that contain anchors also stop
style sharing. You can add a class name or id instead. I think I'd rather you
just put this in a style element in the media document not here.

https://codereview.chromium.org/1780043002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/html.css:83: body:-webkit-full-page-media >
div {
Ditto, you can't put generic rules or use combinators like this in the UA sheet.

[qinmin, 2016-03-15 17:52:16]

https://codereview.chromium.org/1780043002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/html.css (right):

https://codereview.chromium.org/1780043002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/html.css:66: body:-webkit-full-page-media >
div > a {
On 2016/03/15 17:33:07, esprehn wrote:
> We can't do this. It adds rules for every <a> in every page. And marks bits on
> the ancestors, it means children of all divs that contain anchors also stop
> style sharing. You can add a class name or id instead. I think I'd rather you
> just put this in a style element in the media document not here.

Done. Moved the style back into MediaDocument

https://codereview.chromium.org/1780043002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/html.css:83: body:-webkit-full-page-media >
div {
On 2016/03/15 17:33:07, esprehn wrote:
> Ditto, you can't put generic rules or use combinators like this in the UA
sheet.

Done.

[philipj_slow, 2016-03-16 08:39:21]

On 2016/03/10 21:21:18, qinmin wrote:
> On 2016/03/10 08:48:16, philipj_UTC7 wrote:
> > In addition to MediaDocument, it would be a straightforward spec change to
> allow
> > sites to opt in to a download button in any context:
> > <video controls download src="stuff.webm">
> > 
> > If you want it in all contexts like Opera for Android I'd obviously have no
> > complaint, either.
> 
> philipj@, do you have any usage stats on the download button?
> The media controller already has a lot of things on it, play/pause button,
seek
> slider, current time, cast button, fullscreen button, mute/unmute button, cc
> button, so our UI team is looking at standalone download button, rather than
> making the controller overcrowded

We don't have any usage stats, I'm afraid.

[philipj_slow, 2016-03-16 08:40:08]

On 2016/03/11 00:19:24, qinmin wrote:
> On 2016/03/10 21:21:18, qinmin wrote:
> > On 2016/03/10 08:48:16, philipj_UTC7 wrote:
> > > In addition to MediaDocument, it would be a straightforward spec change to
> > allow
> > > sites to opt in to a download button in any context:
> > > <video controls download src="stuff.webm">
> > > 
> > > If you want it in all contexts like Opera for Android I'd obviously have
no
> > > complaint, either.
> > 
> > philipj@, do you have any usage stats on the download button?
> > The media controller already has a lot of things on it, play/pause button,
> seek
> > slider, current time, cast button, fullscreen button, mute/unmute button, cc
> > button, so our UI team is looking at standalone download button, rather than
> > making the controller overcrowded
> 
> BTW, opera is welcome to upload its implemntation of the download button on
the
> media controller(possibly also behind a flag), but chrome UI folks prefer a
> standalone download button for now, we might move it to the media controller
in
> future when there are better UI designs to solve the crowded issue

I'm a bit behind on everything now, so I think the best way to proceed would be
for this to land first, then I can see about upstreaming our download button and
sharing whatever can be shared.

[qinmin, 2016-03-16 16:31:06]

On 2016/03/16 08:40:08, philipj_UTC7 wrote:
> On 2016/03/11 00:19:24, qinmin wrote:
> > On 2016/03/10 21:21:18, qinmin wrote:
> > > On 2016/03/10 08:48:16, philipj_UTC7 wrote:
> > > > In addition to MediaDocument, it would be a straightforward spec change
to
> > > allow
> > > > sites to opt in to a download button in any context:
> > > > <video controls download src="stuff.webm">
> > > > 
> > > > If you want it in all contexts like Opera for Android I'd obviously have
> no
> > > > complaint, either.
> > > 
> > > philipj@, do you have any usage stats on the download button?
> > > The media controller already has a lot of things on it, play/pause button,
> > seek
> > > slider, current time, cast button, fullscreen button, mute/unmute button,
cc
> > > button, so our UI team is looking at standalone download button, rather
than
> > > making the controller overcrowded
> > 
> > BTW, opera is welcome to upload its implemntation of the download button on
> the
> > media controller(possibly also behind a flag), but chrome UI folks prefer a
> > standalone download button for now, we might move it to the media controller
> in
> > future when there are better UI designs to solve the crowded issue
> 
> I'm a bit behind on everything now, so I think the best way to proceed would
be
> for this to land first, then I can see about upstreaming our download button
and
> sharing whatever can be shared.

Thanks, phillipj@. The chromium team wants to experiment this feature first and
gather some usage stats to see if this is something user needed.
At the mean time, the UI team is working on the new media controller UI to
include the download button, while not making the controller over crowded.

[qinmin, 2016-03-16 16:41:34]

On 2016/03/16 16:31:06, qinmin wrote:
> On 2016/03/16 08:40:08, philipj_UTC7 wrote:
> > On 2016/03/11 00:19:24, qinmin wrote:
> > > On 2016/03/10 21:21:18, qinmin wrote:
> > > > On 2016/03/10 08:48:16, philipj_UTC7 wrote:
> > > > > In addition to MediaDocument, it would be a straightforward spec
change
> to
> > > > allow
> > > > > sites to opt in to a download button in any context:
> > > > > <video controls download src="stuff.webm">
> > > > > 
> > > > > If you want it in all contexts like Opera for Android I'd obviously
have
> > no
> > > > > complaint, either.
> > > > 
> > > > philipj@, do you have any usage stats on the download button?
> > > > The media controller already has a lot of things on it, play/pause
button,
> > > seek
> > > > slider, current time, cast button, fullscreen button, mute/unmute
button,
> cc
> > > > button, so our UI team is looking at standalone download button, rather
> than
> > > > making the controller overcrowded
> > > 
> > > BTW, opera is welcome to upload its implemntation of the download button
on
> > the
> > > media controller(possibly also behind a flag), but chrome UI folks prefer
a
> > > standalone download button for now, we might move it to the media
controller
> > in
> > > future when there are better UI designs to solve the crowded issue
> > 
> > I'm a bit behind on everything now, so I think the best way to proceed would
> be
> > for this to land first, then I can see about upstreaming our download button
> and
> > sharing whatever can be shared.
> 
> Thanks, phillipj@. The chromium team wants to experiment this feature first
and
> gather some usage stats to see if this is something user needed.
> At the mean time, the UI team is working on the new media controller UI to
> include the download button, while not making the controller over crowded.

esprehn@, is this looking good now? thanks

[qinmin, 2016-03-17 21:04:47]

On 2016/03/16 16:41:34, qinmin wrote:
> On 2016/03/16 16:31:06, qinmin wrote:
> > On 2016/03/16 08:40:08, philipj_UTC7 wrote:
> > > On 2016/03/11 00:19:24, qinmin wrote:
> > > > On 2016/03/10 21:21:18, qinmin wrote:
> > > > > On 2016/03/10 08:48:16, philipj_UTC7 wrote:
> > > > > > In addition to MediaDocument, it would be a straightforward spec
> change
> > to
> > > > > allow
> > > > > > sites to opt in to a download button in any context:
> > > > > > <video controls download src="stuff.webm">
> > > > > > 
> > > > > > If you want it in all contexts like Opera for Android I'd obviously
> have
> > > no
> > > > > > complaint, either.
> > > > > 
> > > > > philipj@, do you have any usage stats on the download button?
> > > > > The media controller already has a lot of things on it, play/pause
> button,
> > > > seek
> > > > > slider, current time, cast button, fullscreen button, mute/unmute
> button,
> > cc
> > > > > button, so our UI team is looking at standalone download button,
rather
> > than
> > > > > making the controller overcrowded
> > > > 
> > > > BTW, opera is welcome to upload its implemntation of the download button
> on
> > > the
> > > > media controller(possibly also behind a flag), but chrome UI folks
prefer
> a
> > > > standalone download button for now, we might move it to the media
> controller
> > > in
> > > > future when there are better UI designs to solve the crowded issue
> > > 
> > > I'm a bit behind on everything now, so I think the best way to proceed
would
> > be
> > > for this to land first, then I can see about upstreaming our download
button
> > and
> > > sharing whatever can be shared.
> > 
> > Thanks, phillipj@. The chromium team wants to experiment this feature first
> and
> > gather some usage stats to see if this is something user needed.
> > At the mean time, the UI team is working on the new media controller UI to
> > include the download button, while not making the controller over crowded.
> 
> esprehn@, is this looking good now? thanks


ping, esprehn@, PTAL again

[qinmin, 2016-03-17 21:10:25]

qinmin@chromium.org changed reviewers:
+ wangxianzhu@chromium.org

[qinmin, 2016-03-17 21:10:25]

+wangxianzhu for core/ OWNER

[Xianzhu, 2016-03-17 21:49:22]

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

https://codereview.chromium.org/1780043002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/MediaDocument.cpp:106: div->setAttribute(
Add a comment about why you inline these styles here.

https://codereview.chromium.org/1780043002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/MediaDocument.cpp:120:
div->appendChild(padding.release());
Can you just use padding-top on the first div?

https://codereview.chromium.org/1780043002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/MediaDocument.cpp:130: // Using css style
according to android material design.
Nit: s/css/CSS/, s/android/Android/.

[qinmin, 2016-03-17 21:50:06]

qinmin@chromium.org changed reviewers:
+ aelias@chromium.org
- esprehn@chromium.org

[qinmin, 2016-03-17 21:50:07]

+aelias for public/ OWNER

[aelias_OOO_until_Jul13, 2016-03-17 22:04:26]

third_party/WebKit/public/ lgtm

[qinmin, 2016-03-17 22:07:59]

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

https://codereview.chromium.org/1780043002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/MediaDocument.cpp:106: div->setAttribute(
On 2016/03/17 21:49:21, Xianzhu wrote:
> Add a comment about why you inline these styles here.

Done.

https://codereview.chromium.org/1780043002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/MediaDocument.cpp:120:
div->appendChild(padding.release());
On 2016/03/17 21:49:21, Xianzhu wrote:
> Can you just use padding-top on the first div?

the first div is also used in MediaDocuent without the download button, so
adding the padding-top will shift the MediaElement downwards, and it is no
longer centered.

https://codereview.chromium.org/1780043002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/MediaDocument.cpp:130: // Using css style
according to android material design.
On 2016/03/17 21:49:21, Xianzhu wrote:
> Nit: s/css/CSS/, s/android/Android/.

Done.

[Xianzhu, 2016-03-17 22:09:31]

third_party/WebKit/Source/core lgtm.

[qinmin, 2016-03-17 22:19:28]

On 2016/03/17 22:09:31, Xianzhu wrote:
> third_party/WebKit/Source/core lgtm.

jochen@, would you please take a look at content/? thanks

[esprehn, 2016-03-18 05:19:38]

esprehn@chromium.org changed reviewers:
+ esprehn@chromium.org, ojan@chromium.org

[esprehn, 2016-03-18 05:19:39]

ojan@ Can you help them with flexbox here or something? This negative margin and
transform hack doesn't feel right at all.

https://codereview.chromium.org/1780043002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/MediaDocument.cpp (right):

https://codereview.chromium.org/1780043002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/MediaDocument.cpp:118: "transform:
translate(-50%, -50%);");
Can you explain what you're doing here? magin-right: -50% and this translate
doesn't seem like the right way to get the effect you want.

https://codereview.chromium.org/1780043002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/MediaDocument.cpp:150: "margin: 32px auto 0
auto;");
can we insert a <style> element and give these two things ids instead?

[qinmin, 2016-03-18 06:52:40]

I tried flex, and it didn't really work in this case for 2 reasons:
1. the video size is unknown
2. it is really hard to position the download button correctly without using the
onresize() call.

https://codereview.chromium.org/1780043002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/MediaDocument.cpp (right):

https://codereview.chromium.org/1780043002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/MediaDocument.cpp:118: "transform:
translate(-50%, -50%);");
On 2016/03/18 05:19:39, esprehn wrote:
> Can you explain what you're doing here? magin-right: -50% and this translate
> doesn't seem like the right way to get the effect you want.

The technique for centering the element with the margin-right:-50% and (-50%,
-50%) translate can be found here:
https://www.w3.org/Style/Examples/007/center.en.html

Without the transform, the element's left-top corner is at the center of the
page.
So the translate will move the element back to the center of the page (-50%,
-50% will drag the element 1/2 way towards the up-left corder, thus making it
centered)

Since the element's left side is at 50% of the viewport width, so its width is
shrink to 50% of the original size. And the "margin-right: -50%" style will give
back the other 50% width to the element, so that it is at its full length.

[qinmin, 2016-03-18 06:52:40]

I tried flex, and it didn't really work in this case for 2 reasons:
1. the video size is unknown
2. it is really hard to position the download button correctly without using the
onresize() call.

https://codereview.chromium.org/1780043002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/MediaDocument.cpp (right):

https://codereview.chromium.org/1780043002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/MediaDocument.cpp:118: "transform:
translate(-50%, -50%);");
On 2016/03/18 05:19:39, esprehn wrote:
> Can you explain what you're doing here? magin-right: -50% and this translate
> doesn't seem like the right way to get the effect you want.

The technique for centering the element with the margin-right:-50% and (-50%,
-50%) translate can be found here:
https://www.w3.org/Style/Examples/007/center.en.html

Without the transform, the element's left-top corner is at the center of the
page.
So the translate will move the element back to the center of the page (-50%,
-50% will drag the element 1/2 way towards the up-left corder, thus making it
centered)

Since the element's left side is at 50% of the viewport width, so its width is
shrink to 50% of the original size. And the "margin-right: -50%" style will give
back the other 50% width to the element, so that it is at its full length.

[qinmin, 2016-03-18 07:24:22]

To use flex, i think we can either:
1. Center the video element with flex box, and use onresize() call to position
the download button.  But since onresize() is asynchronous, we could see the
download button in the wrong place for a moment when video just  got resized.

2. put the video element and the download button inside a div element, and use
flex to center that div. However, in this case, the video element is no longer
centered due to the download button, so we need to introduce some paddings on
top of the video element to compensate for the height/paddings of the download
button. 
But then there is a problem on how to remove the top padding when the viewport
height shrinks.  The (-50%, 50%) translate doesn't suffer from this problem.

[qinmin, 2016-03-18 19:42:03]

PTAL again. Thanks.

esprehn@, i have included the w3c example in the comments.
jochen@, please take a look at content/

https://codereview.chromium.org/1780043002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/MediaDocument.cpp (right):

https://codereview.chromium.org/1780043002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/MediaDocument.cpp:118: "transform:
translate(-50%, -50%);");
On 2016/03/18 06:52:40, qinmin wrote:
> On 2016/03/18 05:19:39, esprehn wrote:
> > Can you explain what you're doing here? magin-right: -50% and this translate
> > doesn't seem like the right way to get the effect you want.
> 
Added a comment here to include the w3c example page.

https://codereview.chromium.org/1780043002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/MediaDocument.cpp:150: "margin: 32px auto 0
auto;");
On 2016/03/18 05:19:39, esprehn wrote:
> can we insert a <style> element and give these two things ids instead?

Done.

[esprehn, 2016-03-22 18:11:21]

You should be able to use align-items and align-content to center with flexbox.

https://codereview.chromium.org/1780043002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/MediaDocument.cpp (right):

https://codereview.chromium.org/1780043002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/MediaDocument.cpp:130:
padding->setAttribute(heightAttr, "68px;");
please don't add spacer divs, I think you want to add margins or padding to some
element?

[ojan, 2016-03-22 20:13:39]

I said this over IM as well, but I think you can use flexboxes here and get
something more robust.

http://jsbin.com/cemudacera/1/edit?html,output

https://codereview.chromium.org/1780043002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/virtual/android/media/mediadocument/media-document-with-download-button.html
(right):

https://codereview.chromium.org/1780043002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/virtual/android/media/mediadocument/media-document-with-download-button.html:1:
<script src="../../../../resources/run-after-layout-and-paint.js"></script>
Please use a reftest. I think you should be able to make this a reftest and that
way we can avoid having platform-specific baselines.

https://codereview.chromium.org/1780043002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/MediaDocument.cpp (right):

https://codereview.chromium.org/1780043002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/MediaDocument.cpp:145: "display: block;"
Use an inline-block instead. Then you won't need to set width. It will just size
to the content.

https://codereview.chromium.org/1780043002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/MediaDocument.cpp:146: "width: 120px;"
I don't think you want to set an explicit width and height. It won't work well
for when the "Download" string is translated to different languages. Instead,
use CSS padding.

https://codereview.chromium.org/1780043002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/MediaDocument.cpp:155: "text-transform:
uppercase;"
Don't use text-transform to uppercase it. It's much slower. If you want an
uppercased string, just make it uppercased before you call setTextContent.

https://codereview.chromium.org/1780043002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/MediaDocument.cpp:157: "line-height: 36px;"
You shouldn't need to set line-height.

[ojan, 2016-03-23 05:11:10]

Whoops. As discussed on IM, I messed up my test case when cleaning it up. Here's
the actually correct version. :) http://jsbin.com/wecahowusa/1/edit?html,output

[qinmin, 2016-03-24 18:51:19]

Thanks ojan@ for the awesome "min-height: min-content" trick.

esprehn@, PTAL again.

https://codereview.chromium.org/1780043002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/virtual/android/media/mediadocument/media-document-with-download-button.html
(right):

https://codereview.chromium.org/1780043002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/virtual/android/media/mediadocument/media-document-with-download-button.html:1:
<script src="../../../../resources/run-after-layout-and-paint.js"></script>
On 2016/03/22 20:13:39, ojan wrote:
> Please use a reftest. I think you should be able to make this a reftest and
that
> way we can avoid having platform-specific baselines.

Done.

https://codereview.chromium.org/1780043002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/MediaDocument.cpp (right):

https://codereview.chromium.org/1780043002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/MediaDocument.cpp:130:
padding->setAttribute(heightAttr, "68px;");
On 2016/03/22 18:11:21, esprehn wrote:
> please don't add spacer divs, I think you want to add margins or padding to
some
> element?

Switched to use ojan's flex trick.

https://codereview.chromium.org/1780043002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/MediaDocument.cpp:145: "display: block;"
On 2016/03/22 20:13:39, ojan wrote:
> Use an inline-block instead. Then you won't need to set width. It will just
size
> to the content.

inline-block will append the anchor element to the right side of the video
element, instead of beneath it.
put the anchor element inside a div element and append it to the outer
container.

https://codereview.chromium.org/1780043002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/MediaDocument.cpp:146: "width: 120px;"
On 2016/03/22 20:13:39, ojan wrote:
> I don't think you want to set an explicit width and height. It won't work well
> for when the "Download" string is translated to different languages. Instead,
> use CSS padding.

Done.

https://codereview.chromium.org/1780043002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/MediaDocument.cpp:155: "text-transform:
uppercase;"
On 2016/03/22 20:13:39, ojan wrote:
> Don't use text-transform to uppercase it. It's much slower. If you want an
> uppercased string, just make it uppercased before you call setTextContent.

Done.

https://codereview.chromium.org/1780043002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/MediaDocument.cpp:157: "line-height: 36px;"
On 2016/03/22 20:13:39, ojan wrote:
> You shouldn't need to set line-height. 

Android material design uses 36px for button height. Apply padding will make it
hard to control the actual button height.So use line-height here to vertically
center the button label.

[esprehn, 2016-03-29 05:10:31]

lgtm w/ nits fixed.

https://codereview.chromium.org/1780043002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/virtual/android/media/mediadocument/media-document-with-download-button-expected.html
(right):

https://codereview.chromium.org/1780043002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/virtual/android/media/mediadocument/media-document-with-download-button-expected.html:1:
<script src="../../../../resources/run-after-layout-and-paint.js"></script>
missing doctype

https://codereview.chromium.org/1780043002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/virtual/android/media/mediadocument/media-document-with-download-button.html
(right):

https://codereview.chromium.org/1780043002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/virtual/android/media/mediadocument/media-document-with-download-button.html:1:
<script src="../../../../resources/run-after-layout-and-paint.js"></script>
doctype

https://codereview.chromium.org/1780043002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/virtual/android/media/mediadocument/resources/standalone-audio.html
(right):

https://codereview.chromium.org/1780043002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/virtual/android/media/mediadocument/resources/standalone-audio.html:1:
<head>
missing <!DOCTYPE html>

https://codereview.chromium.org/1780043002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/MediaDocument.cpp (right):

https://codereview.chromium.org/1780043002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/MediaDocument.cpp:116: StringBuilder
styleContent;
On second thought lets do inline styles. I'd still do one property per line, but
I'd call

->setAttribute(styleAttr,
   "display: flex;"
   "flex-direction: column;"
   "...");

https://codereview.chromium.org/1780043002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/MediaDocument.cpp:141: "#download {"
no need to guard this, just put all your CSS in a single variable at the top.

https://codereview.chromium.org/1780043002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/MediaDocument.cpp:157:
styleContent.append("#button-container { position: absolute; text-align: center;
left: 0; right: 0; }\n");
I'd just make the styles inline styles if you're going to set them right after
the ids anyway.

[qinmin, 2016-03-29 19:23:37]

https://codereview.chromium.org/1780043002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/virtual/android/media/mediadocument/media-document-with-download-button-expected.html
(right):

https://codereview.chromium.org/1780043002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/virtual/android/media/mediadocument/media-document-with-download-button-expected.html:1:
<script src="../../../../resources/run-after-layout-and-paint.js"></script>
On 2016/03/29 05:10:31, esprehn wrote:
> missing doctype

Done.

https://codereview.chromium.org/1780043002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/virtual/android/media/mediadocument/media-document-with-download-button.html
(right):

https://codereview.chromium.org/1780043002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/virtual/android/media/mediadocument/media-document-with-download-button.html:1:
<script src="../../../../resources/run-after-layout-and-paint.js"></script>
On 2016/03/29 05:10:31, esprehn wrote:
> doctype

Done.

https://codereview.chromium.org/1780043002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/virtual/android/media/mediadocument/resources/standalone-audio.html
(right):

https://codereview.chromium.org/1780043002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/virtual/android/media/mediadocument/resources/standalone-audio.html:1:
<head>
On 2016/03/29 05:10:31, esprehn wrote:
> missing <!DOCTYPE html>

Done.

https://codereview.chromium.org/1780043002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/MediaDocument.cpp (right):

https://codereview.chromium.org/1780043002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/MediaDocument.cpp:116: StringBuilder
styleContent;
On 2016/03/29 05:10:31, esprehn wrote:
> On second thought lets do inline styles. I'd still do one property per line,
but
> I'd call
> 
> ->setAttribute(styleAttr,
>    "display: flex;"
>    "flex-direction: column;"
>    "...");

Done.

https://codereview.chromium.org/1780043002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/MediaDocument.cpp:141: "#download {"
On 2016/03/29 05:10:31, esprehn wrote:
> no need to guard this, just put all your CSS in a single variable at the top.

Done. use inline style

https://codereview.chromium.org/1780043002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/MediaDocument.cpp:157:
styleContent.append("#button-container { position: absolute; text-align: center;
left: 0; right: 0; }\n");
On 2016/03/29 05:10:31, esprehn wrote:
> I'd just make the styles inline styles if you're going to set them right after
> the ids anyway.

Done.

[qinmin, 2016-03-29 19:43:20]

Patchset #8 (id:140001) has been deleted

[jochen (gone - plz use gerrit), 2016-03-30 16:32:51]

do you expect to use this switch on anything but android? If not, why not just
unconditionally enable the feature when we run on android?

[qinmin, 2016-03-31 00:06:43]

On 2016/03/30 16:32:51, jochen wrote:
> do you expect to use this switch on anything but android? If not, why not just
> unconditionally enable the feature when we run on android?

@jochen, this feature will be enabled through finch trial on some android
devices. I have switched the code to use base::Feature instead of command-line
switches.

[qinmin, 2016-03-31 00:07:11]

qinmin@chromium.org changed reviewers:
+ isherman@chromium.org

[Ilya Sherman, 2016-03-31 00:34:59]

Metrics LGTM % comment:

https://codereview.chromium.org/1780043002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/MediaDocument.cpp (right):

https://codereview.chromium.org/1780043002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/MediaDocument.cpp:60: };
Please document that this enum is used to back a histogram, and should therefore
be treated as append-only.

[jochen (gone - plz use gerrit), 2016-03-31 15:16:13]

lgtm with better strings

https://codereview.chromium.org/1780043002/diff/180001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1780043002/diff/180001/chrome/app/generated_r...
chrome/app/generated_resources.grd:5686: +          Download button on
MediaDocument.
translators (and users) will have a hard time understanding what this is. The
desc should give enough background for a translator to be able to translate the
string, but "MediaDocument" is not self-explanatory.

[qinmin, 2016-04-06 17:27:13]

per discussions on blink-dev, slightly changed the MediaDocument.cpp to put the
div and anchor elements in UA shadowroot.

wangxianzhu@ or esprehn@, would any one of you take another look at
MediaDocument.cpp, thanks.

https://codereview.chromium.org/1780043002/diff/180001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1780043002/diff/180001/chrome/app/generated_r...
chrome/app/generated_resources.grd:5686: +          Download button on
MediaDocument.
On 2016/03/31 15:16:13, jochen wrote:
> translators (and users) will have a hard time understanding what this is. The
> desc should give enough background for a translator to be able to translate
the
> string, but "MediaDocument" is not self-explanatory.

Done.

https://codereview.chromium.org/1780043002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/MediaDocument.cpp (right):

https://codereview.chromium.org/1780043002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/MediaDocument.cpp:60: };
On 2016/03/31 00:34:59, Ilya Sherman wrote:
> Please document that this enum is used to back a histogram, and should
therefore
> be treated as append-only.

Done.

[esprehn, 2016-04-06 17:32:08]

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

https://codereview.chromium.org/1780043002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/MediaDocument.cpp:115: if (event->type() ==
EventTypeNames::click && !m_clicked) {
Why do you need to check for == click? This listener is only added for click :)

https://codereview.chromium.org/1780043002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/MediaDocument.cpp:160: ShadowRoot&
shadowRoot = body->ensureUserAgentShadowRoot();
put this down with the usage below the large if block

[Xianzhu, 2016-04-06 17:32:46]

On 2016/04/06 17:27:13, qinmin wrote:
> per discussions on blink-dev, slightly changed the MediaDocument.cpp to put
the
> div and anchor elements in UA shadowroot.
> 
> wangxianzhu@ or esprehn@, would any one of you take another look at
> MediaDocument.cpp, thanks.
> 

lgtm

[qinmin, 2016-04-06 19:40:29]

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

https://codereview.chromium.org/1780043002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/MediaDocument.cpp:115: if (event->type() ==
EventTypeNames::click && !m_clicked) {
On 2016/04/06 17:32:07, esprehn wrote:
> Why do you need to check for == click? This listener is only added for click
:)

Done. removed

https://codereview.chromium.org/1780043002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/MediaDocument.cpp:160: ShadowRoot&
shadowRoot = body->ensureUserAgentShadowRoot();
On 2016/04/06 17:32:07, esprehn wrote:
> put this down with the usage below the large if block

Done.

[qinmin, 2016-04-06 19:40:38]

The CQ bit was checked by qinmin@chromium.org

[qinmin, 2016-04-06 19:40:39]

The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org,
esprehn@chromium.org, jochen@chromium.org, isherman@chromium.org,
wangxianzhu@chromium.org
Link to the patchset: https://codereview.chromium.org/1780043002/#ps220001
(title: "nits")

[commit-bot: I haz the power, 2016-04-06 19:41:09]

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

[commit-bot: I haz the power, 2016-04-06 22:46:01]

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

[commit-bot: I haz the power, 2016-04-06 22:46:03]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[qinmin, 2016-04-06 23:27:18]

The CQ bit was checked by qinmin@chromium.org

[commit-bot: I haz the power, 2016-04-06 23:28:13]

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

[commit-bot: I haz the power, 2016-04-07 01:05:38]

Committed patchset #11 (id:220001)

[commit-bot: I haz the power, 2016-04-07 01:07:09]

Description was changed from

==========
Add download button to MediaDocument on Android.

For a MediaDocument on android, it is not easy
to long press the element and download audio.
This change adds a download button to the MediaDocument on android.
This facilitates user to save the media file locally.

UI
Review:
https://groups.google.com/a/google.com/forum/#!msg/chrome-ui-review/XzyQ2QsVy...
Mocks:
https://folio.googleplex.com/chrome-ux/mocks/065-downloads/06-media/022216_Bu...

BUG=590043
==========

to

==========
Add download button to MediaDocument on Android.

For a MediaDocument on android, it is not easy
to long press the element and download audio.
This change adds a download button to the MediaDocument on android.
This facilitates user to save the media file locally.

UI
Review:
https://groups.google.com/a/google.com/forum/#!msg/chrome-ui-review/XzyQ2QsVy...
Mocks:
https://folio.googleplex.com/chrome-ux/mocks/065-downloads/06-media/022216_Bu...

BUG=590043

Committed: https://crrev.com/20651adff7805d35635fcf6a2be9fd6a1e01b40b
Cr-Commit-Position: refs/heads/master@{#385607}
==========

[commit-bot: I haz the power, 2016-04-07 01:07:11]

Patchset 11 (id:??) landed as
https://crrev.com/20651adff7805d35635fcf6a2be9fd6a1e01b40b
Cr-Commit-Position: refs/heads/master@{#385607}