Enabling autoplay and fullscreen for downloaded media

Enabling autoplay and fullscreen for downloaded media

For downloaded video, the desired behavior is to autoplay with fullscreen
 at the start. This CL enables the video to enter
fullscreen by adding an event listener for onloadedmetadata. A web
preference was added for triggering this feature.

BUG=682848
Review-Url: https://codereview.chromium.org/2640023008
Cr-Commit-Position: refs/heads/master@{#452967}
Committed: https://chromium.googlesource.com/chromium/src/+/f97f7538cc5be6da3145a830ff1a786f6c0bbf78

[shaktisahu, 2017-01-20 00:20:50]

shaktisahu@chromium.org changed reviewers:
+ dtrainor@chromium.org

[shaktisahu, 2017-01-20 00:20:51]

dtrainor@ - PTAL

[DaleCurtis, 2017-01-20 00:34:14]

dalecurtis@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[DaleCurtis, 2017-01-20 00:34:15]

Why launch a MediaDocument versus a custom HTML page that can do all this?

[David Trainor- moved to gerrit, 2017-01-20 17:59:13]

dtrainor@chromium.org changed reviewers:
+ mlamouri@chromium.org

[David Trainor- moved to gerrit, 2017-01-20 17:59:13]

+mlamouri@ as we talked about this a bit last week.

dalecurtis@: Yeah I think that's a good point.  When we talked earlier I
associated building the DOM in MediaDocument with the custom HTML you were
talking about.  Sorry for the confusion.  Shakti you should try this approach
out.  It makes sense if we don't want *all* media playback paths to have these
properties.

Just a note that we might want to revisit this if we can't get the experience we
want through traditional HTML.  In particular it's not ideal that we can't
eventually force the MediaDocument to *immediately* start in full screen.  I'm
not saying this as an argument for staying in MediaDocument now though, as I
don't know if it's going to be a real problem in release or if there is a
reasonable fix for it.

[DaleCurtis, 2017-01-20 18:06:16]

On 2017/01/20 at 17:59:13, dtrainor wrote:
> +mlamouri@ as we talked about this a bit last week.
> 
> dalecurtis@: Yeah I think that's a good point.  When we talked earlier I
associated building the DOM in MediaDocument with the custom HTML you were
talking about.  Sorry for the confusion.  Shakti you should try this approach
out.  It makes sense if we don't want *all* media playback paths to have these
properties.
> 
> Just a note that we might want to revisit this if we can't get the experience
we want through traditional HTML.  In particular it's not ideal that we can't
eventually force the MediaDocument to *immediately* start in full screen.  I'm
not saying this as an argument for staying in MediaDocument now though, as I
don't know if it's going to be a real problem in release or if there is a
reasonable fix for it.

Not sure I follow the last paragraph. If you launch your renderer with gesture
requirements disabled, you can request fullscreen in your custom page at launch.

[mlamouri (slow - plz ping), 2017-01-23 00:37:33]

dtrainor@ when we talked about this change, I suggested that maybe the
auto-fullscreen could apply to all MediaDocument. This should get some product
approval but I think it's generally something we might consider doing given that
it will improve the general MediaDocument experience. In this case, you could
also heavily reduce the boilerplate.

Then, you could make the MediaDocument go eagerly fullscreen on page load which
can be implemented in addition of going fullscreen on play.

[David Trainor- moved to gerrit, 2017-01-24 16:45:20]

On 2017/01/20 18:06:16, DaleCurtis wrote:
> On 2017/01/20 at 17:59:13, dtrainor wrote:
> > +mlamouri@ as we talked about this a bit last week.
> > 
> > dalecurtis@: Yeah I think that's a good point.  When we talked earlier I
> associated building the DOM in MediaDocument with the custom HTML you were
> talking about.  Sorry for the confusion.  Shakti you should try this approach
> out.  It makes sense if we don't want *all* media playback paths to have these
> properties.
> > 
> > Just a note that we might want to revisit this if we can't get the
experience
> we want through traditional HTML.  In particular it's not ideal that we can't
> eventually force the MediaDocument to *immediately* start in full screen.  I'm
> not saying this as an argument for staying in MediaDocument now though, as I
> don't know if it's going to be a real problem in release or if there is a
> reasonable fix for it.
> 
> Not sure I follow the last paragraph. If you launch your renderer with gesture
> requirements disabled, you can request fullscreen in your custom page at
launch.

When we were trying some changes out in MediaDocument, requesting fullscreen on
the media element seemed to require the video to load first or it wouldn't work,
which led to a few frames of non-fullscreen and some visible jank.  But
mlamouri@ recommended that we try out release builds and it looked smoother
there :).

[David Trainor- moved to gerrit, 2017-01-24 16:50:37]

On 2017/01/23 00:37:33, mlamouri wrote:
> dtrainor@ when we talked about this change, I suggested that maybe the
> auto-fullscreen could apply to all MediaDocument. This should get some product
> approval but I think it's generally something we might consider doing given
that
> it will improve the general MediaDocument experience. In this case, you could
> also heavily reduce the boilerplate.
> 
> Then, you could make the MediaDocument go eagerly fullscreen on page load
which
> can be implemented in addition of going fullscreen on play.

Yeah I brought it up with Jon but we didn't come to a decision.  Like you said
we'd need some approval due to hiding the Chrome UX automatically.  Will follow
up.

[mlamouri (slow - plz ping), 2017-02-15 10:54:30]

The general approach seems fine. However, instead of adding a
"media_playback_fullscreen_by_default" can we have a more generic
"immersive_media_document_experience" or something like that? So, if you want to
add more features related to this special media document, it does not require
that much boilerplate.

Also, I would prefer if the tab setting was also related of this media document
feature instead of setting media playback options.

[DaleCurtis, 2017-02-15 17:36:14]

Did the custom html page not work?

[shaktisahu, 2017-02-15 17:47:39]

On 2017/02/15 17:36:14, DaleCurtis wrote:
> Did the custom html page not work?

Hi Dale - The custom html page works but it is not clean, requires more code and
still needs the same boilerplate for setting prefs.  That's why I am leaning
towards MediaDocument approach.
https://docs.google.com/document/d/1_n0tYbGO1GyxhNEQA5UUBskhhYssO03WEbvOkNtUOwg
Feel free to comment on the doc. Thanks!

[shaktisahu, 2017-02-15 17:47:41]

On 2017/02/15 17:36:14, DaleCurtis wrote:
> Did the custom html page not work?

Hi Dale - The custom html page works but it is not clean, requires more code and
still needs the same boilerplate for setting prefs.  That's why I am leaning
towards MediaDocument approach.
https://docs.google.com/document/d/1_n0tYbGO1GyxhNEQA5UUBskhhYssO03WEbvOkNtUOwg
Feel free to comment on the doc. Thanks!

[shaktisahu, 2017-02-16 07:08:54]

Description was changed from

==========
Enabling fullscreen for downloaded media

For downloaded video, the desired behavior is to enter fullscreen
automatically at the start. This CL enables the video to enter
fullscreen by adding an event listener for onloadedmetadata. A web
preference was added for triggering this feature.

BUG=682848
==========

to

==========
Enabling autoplay and fullscreen for downloaded media

For downloaded video, the desired behavior is to autoplay with fullscreen
 at the start. This CL enables the video to enter
fullscreen by adding an event listener for onloadedmetadata. A web
preference was added for triggering this feature.

BUG=682848
==========

[shaktisahu, 2017-02-16 23:37:24]

PTAL

[mlamouri (slow - plz ping), 2017-02-17 10:01:43]

https://codereview.chromium.org/2640023008/diff/60001/chrome/browser/android/...
File chrome/browser/android/tab_android.cc (right):

https://codereview.chromium.org/2640023008/diff/60001/chrome/browser/android/...
chrome/browser/android/tab_android.cc:728:
prefs.user_gesture_required_for_media_playback = false;
Is it needed? Can't you simply autoplay from C++ to avoid worrying about this?

https://codereview.chromium.org/2640023008/diff/60001/chrome/browser/android/...
chrome/browser/android/tab_android.cc:730: host->UpdateWebkitPreferences(prefs);
That does not sound correct because if any one calls UpdateWebkitPreferences
later, your settings will be disabled. Instead, you should make sure that when
the settings are refreshed, it checks for the Tab's information.

[shaktisahu, 2017-02-17 19:18:06]

https://codereview.chromium.org/2640023008/diff/60001/chrome/browser/android/...
File chrome/browser/android/tab_android.cc (right):

https://codereview.chromium.org/2640023008/diff/60001/chrome/browser/android/...
chrome/browser/android/tab_android.cc:728:
prefs.user_gesture_required_for_media_playback = false;
On 2017/02/17 10:01:43, mlamouri wrote:
> Is it needed? Can't you simply autoplay from C++ to avoid worrying about this?

Done. Thanks!

https://codereview.chromium.org/2640023008/diff/60001/chrome/browser/android/...
chrome/browser/android/tab_android.cc:730: host->UpdateWebkitPreferences(prefs);
On 2017/02/17 10:01:43, mlamouri wrote:
> That does not sound correct because if any one calls UpdateWebkitPreferences
> later, your settings will be disabled. Instead, you should make sure that when
> the settings are refreshed, it checks for the Tab's information.

I looked around and didn't find any function that does it differently.
Whoever wants to set prefs, they first call GetWebkitPreferences to get the
existing prefs and add their preferences to it. So, unless someone wants to
specifically update embedded_media_experience_enabled, it should't be an issue.

[mlamouri (slow - plz ping), 2017-02-20 11:31:50]

https://codereview.chromium.org/2640023008/diff/60001/chrome/browser/android/...
File chrome/browser/android/tab_android.cc (right):

https://codereview.chromium.org/2640023008/diff/60001/chrome/browser/android/...
chrome/browser/android/tab_android.cc:730: host->UpdateWebkitPreferences(prefs);
On 2017/02/17 at 19:18:06, shaktisahu wrote:
> On 2017/02/17 10:01:43, mlamouri wrote:
> > That does not sound correct because if any one calls UpdateWebkitPreferences
> > later, your settings will be disabled. Instead, you should make sure that
when
> > the settings are refreshed, it checks for the Tab's information.
> 
> I looked around and didn't find any function that does it differently.
> Whoever wants to set prefs, they first call GetWebkitPreferences to get the
existing prefs and add their preferences to it. So, unless someone wants to
specifically update embedded_media_experience_enabled, it should't be an issue.

The issue is that you are calling UpdateWebkitPreferences with the value set to
true but any one calling this later will revert your setting. The expectation is
that UpdateWebkitPreferences will have the logic that checks for the value. You
can look at |media_playback_gesture_whitelist_scope| for an example.

[shaktisahu, 2017-02-21 07:02:40]

On 2017/02/20 11:31:50, mlamouri wrote:
>
https://codereview.chromium.org/2640023008/diff/60001/chrome/browser/android/...
> File chrome/browser/android/tab_android.cc (right):
> 
>
https://codereview.chromium.org/2640023008/diff/60001/chrome/browser/android/...
> chrome/browser/android/tab_android.cc:730:
host->UpdateWebkitPreferences(prefs);
> On 2017/02/17 at 19:18:06, shaktisahu wrote:
> > On 2017/02/17 10:01:43, mlamouri wrote:
> > > That does not sound correct because if any one calls
UpdateWebkitPreferences
> > > later, your settings will be disabled. Instead, you should make sure that
> when
> > > the settings are refreshed, it checks for the Tab's information.
> > 
> > I looked around and didn't find any function that does it differently.
> > Whoever wants to set prefs, they first call GetWebkitPreferences to get the
> existing prefs and add their preferences to it. So, unless someone wants to
> specifically update embedded_media_experience_enabled, it should't be an
issue.
> 
> The issue is that you are calling UpdateWebkitPreferences with the value set
to
> true but any one calling this later will revert your setting. The expectation
is
> that UpdateWebkitPreferences will have the logic that checks for the value.
You
> can look at |media_playback_gesture_whitelist_scope| for an example.

mlamouri@ - PTAL

[shaktisahu, 2017-02-21 07:02:56]

https://codereview.chromium.org/2640023008/diff/60001/chrome/browser/android/...
File chrome/browser/android/tab_android.cc (right):

https://codereview.chromium.org/2640023008/diff/60001/chrome/browser/android/...
chrome/browser/android/tab_android.cc:730: host->UpdateWebkitPreferences(prefs);
On 2017/02/20 11:31:50, mlamouri wrote:
> On 2017/02/17 at 19:18:06, shaktisahu wrote:
> > On 2017/02/17 10:01:43, mlamouri wrote:
> > > That does not sound correct because if any one calls
UpdateWebkitPreferences
> > > later, your settings will be disabled. Instead, you should make sure that
> when
> > > the settings are refreshed, it checks for the Tab's information.
> > 
> > I looked around and didn't find any function that does it differently.
> > Whoever wants to set prefs, they first call GetWebkitPreferences to get the
> existing prefs and add their preferences to it. So, unless someone wants to
> specifically update embedded_media_experience_enabled, it should't be an
issue.
> 
> The issue is that you are calling UpdateWebkitPreferences with the value set
to
> true but any one calling this later will revert your setting. The expectation
is
> that UpdateWebkitPreferences will have the logic that checks for the value.
You
> can look at |media_playback_gesture_whitelist_scope| for an example.

Done.
Thanks! I think this is especially useful for the situation where the current
RenderViewHost or even WebContents is swapped for a new one, and we still get
our preference set correctly.

[mlamouri (slow - plz ping), 2017-02-21 11:33:03]

design sgtm, I have left comments on the implementation but we should be close
now :)

https://codereview.chromium.org/2640023008/diff/100001/chrome/browser/android...
File chrome/browser/android/tab_android.h (right):

https://codereview.chromium.org/2640023008/diff/100001/chrome/browser/android...
chrome/browser/android/tab_android.h:248: bool
ShouldEnableEmbeddedMediaExperience();
nit: const

https://codereview.chromium.org/2640023008/diff/100001/chrome/browser/android...
chrome/browser/android/tab_android.h:276: bool
embedded_media_experience_enabled_;
You are not initialising this member. Feel free to add ` = false;` here or in
the ctor.

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

https://codereview.chromium.org/2640023008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/MediaDocument.cpp:121: public:
`WTF_MAKE_NONCOPYABLE(MediaLoadedEventListener);` ?

https://codereview.chromium.org/2640023008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/MediaDocument.cpp:124: }
What's the benefit of using create over the ctor?

https://codereview.chromium.org/2640023008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/MediaDocument.cpp:131:
MediaLoadedEventListener() : EventListener(CPPEventListenerType) {}
`= default;` instead of `{}`

https://codereview.chromium.org/2640023008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/MediaDocument.cpp:135:
*event->target()->toNode()->getRootNode(GetRootNodeOptions()));
Why are you using Traversal<> here? Shouldn't `event->target()` be the video
element?

https://codereview.chromium.org/2640023008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/MediaDocument.cpp:139: media->play();
Maybe it would be better to start playback when we have transitionned to
fullscreen but that might be good enough :)

https://codereview.chromium.org/2640023008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/MediaDocument.cpp:196:
media->addEventListener(EventTypeNames::loadedmetadata, listener, false);
Would it make sense to do:
```
AddEventListenerOptions options;
options.setOnce(true);
AddEventListenerOptionsOrBoolean optionsOrBoolean;
options.setAddEventListenerOptions(options);
media->addEventListener(EventTypeNames::loadedMetadata, optionsOrBoolean);
```

It might be cleaner because the listener would be free'd up after being fired.
It would be nicer if we had an internal method that takes
AddEventListenerOptions but that's maybe for later :)

[shaktisahu, 2017-02-21 16:51:57]

https://codereview.chromium.org/2640023008/diff/100001/chrome/browser/android...
File chrome/browser/android/tab_android.h (right):

https://codereview.chromium.org/2640023008/diff/100001/chrome/browser/android...
chrome/browser/android/tab_android.h:248: bool
ShouldEnableEmbeddedMediaExperience();
On 2017/02/21 11:33:03, mlamouri wrote:
> nit: const

Done.

https://codereview.chromium.org/2640023008/diff/100001/chrome/browser/android...
chrome/browser/android/tab_android.h:276: bool
embedded_media_experience_enabled_;
On 2017/02/21 11:33:03, mlamouri wrote:
> You are not initialising this member. Feel free to add ` = false;` here or in
> the ctor.

Done.

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

https://codereview.chromium.org/2640023008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/MediaDocument.cpp:121: public:
On 2017/02/21 11:33:03, mlamouri wrote:
> `WTF_MAKE_NONCOPYABLE(MediaLoadedEventListener);` ?

Done.

https://codereview.chromium.org/2640023008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/MediaDocument.cpp:124: }
On 2017/02/21 11:33:03, mlamouri wrote:
> What's the benefit of using create over the ctor?

hmm.. I wanted to keep it consistent with the other subclasses of EventListener
(see above).

https://codereview.chromium.org/2640023008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/MediaDocument.cpp:131:
MediaLoadedEventListener() : EventListener(CPPEventListenerType) {}
On 2017/02/21 11:33:03, mlamouri wrote:
> `= default;` instead of `{}`

I think default works with only empty initialization list.

https://codereview.chromium.org/2640023008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/MediaDocument.cpp:135:
*event->target()->toNode()->getRootNode(GetRootNodeOptions()));
On 2017/02/21 11:33:03, mlamouri wrote:
> Why are you using Traversal<> here? Shouldn't `event->target()` be the video
> element?

Done.

https://codereview.chromium.org/2640023008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/MediaDocument.cpp:139: media->play();
On 2017/02/21 11:33:03, mlamouri wrote:
> Maybe it would be better to start playback when we have transitionned to
> fullscreen but that might be good enough :)

I plan to revisit this depending on how the experience looks. Also I wanted to
see if I could get rid of the control panel that shows at the start, but may be
that's for later. For the released build, it looks smooth though, but there is
room for improvement.

https://codereview.chromium.org/2640023008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/MediaDocument.cpp:196:
media->addEventListener(EventTypeNames::loadedmetadata, listener, false);
On 2017/02/21 11:33:03, mlamouri wrote:
> Would it make sense to do:
> ```
> AddEventListenerOptions options;
> options.setOnce(true);
> AddEventListenerOptionsOrBoolean optionsOrBoolean;
> options.setAddEventListenerOptions(options);
> media->addEventListener(EventTypeNames::loadedMetadata, optionsOrBoolean);
> ```
> 
> It might be cleaner because the listener would be free'd up after being fired.
> It would be nicer if we had an internal method that takes
> AddEventListenerOptions but that's maybe for later :)

Done.

[shaktisahu, 2017-02-21 22:14:06]

The CQ bit was checked by shaktisahu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-21 22:15:58]

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, 2017-02-22 00:20:40]

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

[commit-bot: I haz the power, 2017-02-22 00:20:41]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[shaktisahu, 2017-02-22 06:59:31]

dtrainor@ - PTAL at chrome/

[mlamouri (slow - plz ping), 2017-02-22 12:15:27]

lgtm! Thank you :)

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

https://codereview.chromium.org/2640023008/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/MediaDocument.cpp:32: #include
"core/dom/GetRootNodeOptions.h"
I don't think you still need this include.

[David Trainor- moved to gerrit, 2017-02-22 17:53:17]

just a quick comment about having this be a per-tab setting.  I know we don't
navigate those tabs, but if the UI changes that could change as well and we
might forget to clear this setting.

https://codereview.chromium.org/2640023008/diff/120001/chrome/browser/android...
File chrome/browser/android/tab_android.cc (right):

https://codereview.chromium.org/2640023008/diff/120001/chrome/browser/android...
chrome/browser/android/tab_android.cc:734: return
embedded_media_experience_enabled_;
Should we clear this on navigation?

[shaktisahu, 2017-02-23 06:51:05]

dtrainor@ - PTAL

https://codereview.chromium.org/2640023008/diff/120001/chrome/browser/android...
File chrome/browser/android/tab_android.cc (right):

https://codereview.chromium.org/2640023008/diff/120001/chrome/browser/android...
chrome/browser/android/tab_android.cc:734: return
embedded_media_experience_enabled_;
On 2017/02/22 17:53:17, David Trainor wrote:
> Should we clear this on navigation?

I added something to clear this in CustomTabObserver.java. PTAL if it makes
sense.

[David Trainor- moved to gerrit, 2017-02-23 21:15:25]

lgtm % comment.  Thanks!

https://codereview.chromium.org/2640023008/diff/140001/chrome/browser/android...
File chrome/browser/android/tab_android.cc (right):

https://codereview.chromium.org/2640023008/diff/140001/chrome/browser/android...
chrome/browser/android/tab_android.cc:734: return
embedded_media_experience_enabled_;
Had a talk offline.  Follow up is maybe we can check the URL here to make sure
it's a file scheme.  Either that or I'm fine without the observer. :)

[shaktisahu, 2017-02-23 22:43:27]

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

https://codereview.chromium.org/2640023008/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/MediaDocument.cpp:32: #include
"core/dom/GetRootNodeOptions.h"
On 2017/02/22 12:15:27, mlamouri wrote:
> I don't think you still need this include.

Done.

https://codereview.chromium.org/2640023008/diff/140001/chrome/browser/android...
File chrome/browser/android/tab_android.cc (right):

https://codereview.chromium.org/2640023008/diff/140001/chrome/browser/android...
chrome/browser/android/tab_android.cc:734: return
embedded_media_experience_enabled_;
On 2017/02/23 21:15:25, David Trainor wrote:
> Had a talk offline.  Follow up is maybe we can check the URL here to make sure
> it's a file scheme.  Either that or I'm fine without the observer. :)

Actually I checked this. We can't add that check here since we set the flag
before loading the file URL :). Since this tab is used for a one-time use only,
I think it should be okay to leave this flag here.

[shaktisahu, 2017-02-23 22:50:55]

shaktisahu@chromium.org changed reviewers:
+ jam@chromium.org

[shaktisahu, 2017-02-23 22:50:55]

jam - PTAL at content/ and third_party/WebKit

[jam, 2017-02-24 00:49:30]

On 2017/02/23 22:50:55, shaktisahu wrote:
> jam - PTAL at content/ and third_party/WebKit

lgtm for content
i'm not an owner for webkit

[shaktisahu, 2017-02-24 01:51:31]

The CQ bit was checked by shaktisahu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-24 01:52:24]

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

[shaktisahu, 2017-02-24 01:58:58]

shaktisahu@chromium.org changed reviewers:
+ kenrb@chromium.org

[commit-bot: I haz the power, 2017-02-24 03:17:32]

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

[commit-bot: I haz the power, 2017-02-24 03:17:33]

Dry run: This issue passed the CQ dry run.

[shaktisahu, 2017-02-24 03:21:07]

The CQ bit was checked by shaktisahu@chromium.org

[shaktisahu, 2017-02-24 03:21:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlamouri@chromium.org, dtrainor@chromium.org
Link to the patchset: https://codereview.chromium.org/2640023008/#ps160001
(title: "removed include")

[commit-bot: I haz the power, 2017-02-24 03:21:33]

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, 2017-02-24 03:30:12]

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

[commit-bot: I haz the power, 2017-02-24 03:30: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...)

[kenrb, 2017-02-24 22:39:44]

ipc lgtm

[shaktisahu, 2017-02-24 22:40:23]

The CQ bit was checked by shaktisahu@chromium.org

[commit-bot: I haz the power, 2017-02-24 22:41:11]

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, 2017-02-24 22:46:16]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1487976023413790,
"parent_rev": "a2adc9e7c94a24c344395a8be00ff2c5f1b6f84c", "commit_rev":
"f97f7538cc5be6da3145a830ff1a786f6c0bbf78"}

[commit-bot: I haz the power, 2017-02-24 22:46:53]

Description was changed from

==========
Enabling autoplay and fullscreen for downloaded media

For downloaded video, the desired behavior is to autoplay with fullscreen
 at the start. This CL enables the video to enter
fullscreen by adding an event listener for onloadedmetadata. A web
preference was added for triggering this feature.

BUG=682848
==========

to

==========
Enabling autoplay and fullscreen for downloaded media

For downloaded video, the desired behavior is to autoplay with fullscreen
 at the start. This CL enables the video to enter
fullscreen by adding an event listener for onloadedmetadata. A web
preference was added for triggering this feature.

BUG=682848

Review-Url: https://codereview.chromium.org/2640023008
Cr-Commit-Position: refs/heads/master@{#452967}
Committed:
https://chromium.googlesource.com/chromium/src/+/f97f7538cc5be6da3145a830ff1a...
==========

[commit-bot: I haz the power, 2017-02-24 22:46:54]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/f97f7538cc5be6da3145a830ff1a...