add a setting to allow user to play a media by clicking on it

[Min Qin]

add a setting to allow user to play a media by clicking on it

[Min Qin]

Created attachment 150677 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Adam Barth]

Comment on attachment 150677 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=150677&action=review

A few details below.

> Source/WebCore/html/HTMLMediaElement.cpp:3853
> +    if (document()->settings() && document()->settings()->mediaClickToPlayEnabled() && event->type() == eventNames().clickEvent && !hasEventListeners(eventNames().clickEvent) && canPlay()) {

Is !hasEventListeners(eventNames().clickEvent)  needed here?  I guess folks who handle the click event won't be preventing the default if there isn't one currently...

> Source/WebCore/testing/InternalSettings.cpp:371
> +fprintf(stderr, "InternalSettings::setMediaClickToPlayEnabled");

We should probably remove this line before landing.

> Source/WebCore/testing/InternalSettings.cpp:373
> +    settings()->setMediaClickToPlayEnabled(enabled);

Please reset this setting to its default value in InternalSettings::restoreTo

> LayoutTests/media/video-click-to-play-enabled.html:14
> +            function cleanClickToPlayRequirement() {
> +                if (window.internals) 
> +                    window.internals.settings.setMediaClickToPlayEnabled(false);
> +            }

Rather than doing this in each test, we have the InternalsSettings object do this work for us.

> LayoutTests/media/video-click-to-play-enabled.html:21
> +                video.dispatchEvent(mouseClick);
> +                testExpected("video.paused", false);

Can you add a test case for when the video element has a click event listener?

[Min Qin]

Created attachment 150691 [details]
Patch

[Min Qin]

(In reply to comment #3)
> (From update of attachment 150677 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=150677&action=review
> 
> A few details below.
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:3853
> > +    if (document()->settings() && document()->settings()->mediaClickToPlayEnabled() && event->type() == eventNames().clickEvent && !hasEventListeners(eventNames().clickEvent) && canPlay()) {
> 
> Is !hasEventListeners(eventNames().clickEvent)  needed here?  I guess folks who handle the click event won't be preventing the default if there isn't one currently...
> 

Good point. This should no longer be needed here. In downstream, because clicking the media could pause the playback. So we introduced it to prevent the case that a single click will cause both play() and pause(). Since the behavior is changed when upstreaming this, we no longer need this. 

> > Source/WebCore/testing/InternalSettings.cpp:371
> > +fprintf(stderr, "InternalSettings::setMediaClickToPlayEnabled");
> 
> We should probably remove this line before landing.

Done, I forgot to remove this running the webkit-patch tool.

> 
> > Source/WebCore/testing/InternalSettings.cpp:373
> > +    settings()->setMediaClickToPlayEnabled(enabled);
> 
> Please reset this setting to its default value in InternalSettings::restoreTo

Done.

> 
> > LayoutTests/media/video-click-to-play-enabled.html:14
> > +            function cleanClickToPlayRequirement() {
> > +                if (window.internals) 
> > +                    window.internals.settings.setMediaClickToPlayEnabled(false);
> > +            }
> 
> Rather than doing this in each test, we have the InternalsSettings object do this work for us.

Done.

> 
> > LayoutTests/media/video-click-to-play-enabled.html:21
> > +                video.dispatchEvent(mouseClick);
> > +                testExpected("video.paused", false);
> 
> Can you add a test case for when the video element has a click event listener?

Done. added a listener to the media element for the 2nd click.

[Adam Barth]

Comment on attachment 150691 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=150691&action=review

> Source/WebCore/testing/InternalSettings.cpp:110
> +    , m_mediaClickToPlayEnabled(settings()->mediaClickToPlayEnabled())

nit: I've been using the prefix "original" for these variables to indicate that it's not the current value.

[Adam Barth]

This looks reasonable to me, but let's give Eric Carlson a chance to comment.

[Alexey Proskuryakov]

> For video on mobile devices, expose a setting so that user can click on a media element to start playing it.

This sounds confusing - mobile devices generally don't have controllers that support clicking.

[Min Qin]

Both chrome on android and android browser has media controllers that support clicking. But the buttons are quite small, especially for smartphone users. This change is to make it easier for user to play a media by just clicking the element.

for chrome (In reply to comment #8)
> > For video on mobile devices, expose a setting so that user can click on a media element to start playing it.
> 
> This sounds confusing - mobile devices generally don't have controllers that support clicking.

[Eric Carlson]

Comment on attachment 150691 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=150691&action=review

> Source/WebCore/html/HTMLMediaElement.cpp:3855
> +    if (document()->settings() && document()->settings()->mediaClickToPlayEnabled() && event->type() == eventNames().clickEvent && canPlay()) {
> +        play();
> +        event->setDefaultHandled();

This strikes me as the wrong way to enable this feature. Other (most?) players that enable "click to play" have a UI affordance to help the user understand that clicking will initiate playback, often a large "play" button. If someone wanted to implement that in a WebKit port, they would not be able to use this setting because the play button would intercept the click, and they would presumably want to restrict the behavior to clicks in the play button.

It seems to me that you are adding a new control function, so the right place for this is in the media control code. It should be easy to add an element to the controls shadow DOM and do the hit testing there. Whether you actually show a button or not is tangential.

[Min Qin]

ok, I will try put the functionality into  the media control code then.

(In reply to comment #10)
> (From update of attachment 150691 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=150691&action=review
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:3855
> > +    if (document()->settings() && document()->settings()->mediaClickToPlayEnabled() && event->type() == eventNames().clickEvent && canPlay()) {
> > +        play();
> > +        event->setDefaultHandled();
> 
> This strikes me as the wrong way to enable this feature. Other (most?) players that enable "click to play" have a UI affordance to help the user understand that clicking will initiate playback, often a large "play" button. If someone wanted to implement that in a WebKit port, they would not be able to use this setting because the play button would intercept the click, and they would presumably want to restrict the behavior to clicks in the play button.
> 
> It seems to me that you are adding a new control function, so the right place for this is in the media control code. It should be easy to add an element to the controls shadow DOM and do the hit testing there. Whether you actually show a button or not is tangential.

[Min Qin]

(In reply to comment #10)
> (From update of attachment 150691 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=150691&action=review
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:3855
> > +    if (document()->settings() && document()->settings()->mediaClickToPlayEnabled() && event->type() == eventNames().clickEvent && canPlay()) {
> > +        play();
> > +        event->setDefaultHandled();
> 
> This strikes me as the wrong way to enable this feature. Other (most?) players that enable "click to play" have a UI affordance to help the user understand that clicking will initiate playback, often a large "play" button. If someone wanted to implement that in a WebKit port, they would not be able to use this setting because the play button would intercept the click, and they would presumably want to restrict the behavior to clicks in the play button.
> 
> It seems to me that you are adding a new control function, so the right place for this is in the media control code. It should be easy to add an element to the controls shadow DOM and do the hit testing there. Whether you actually show a button or not is tangential.

One problem with shadow DOM is that if the media element doesn't have the controls attribute, then clicking the media element will not work.

[Eric Carlson]

(In reply to comment #12)
> One problem with shadow DOM is that if the media element doesn't have the controls attribute, then clicking the media element will not work.

Unless something like your new setting also enables the shadow DOM, eg.

  if (controls() || document()->settings()->mediaClickToPlayEnabled())
    ...

[Min Qin]

deprecating this patch as I created a new one to add a shadow DOM change for android: 
https://bugs.webkit.org/show_bug.cgi?id=92132