[Webkit-unassigned] [Bug 90498] add a setting to allow user to play a media by clicking on it

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=90498





--- Comment #5 from Min Qin <qinmin at chromium.org>  2012-07-03 16:49:25 PST ---
(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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.