[webkit-reviews] review granted: [Bug 23870] Allow ports to define restrictions on media loading : [Attachment 27530] revised patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 23870: Allow ports to define restrictions on media loading
https://bugs.webkit.org/show_bug.cgi?id=23870

Attachment 27530: revised patch
https://bugs.webkit.org/attachment.cgi?id=27530&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
> Index: WebCore/ChangeLog
> ===================================================================

> +	   https://bugs.webkit.org/post_bug.cgi

Please fix the link.

> +	   make it possible for a port to require a user gesture for an <audio>
or <video> element
> +	   to load a file

To load a url? Sentence case.

> Index: WebCore/html/HTMLMediaElement.cpp
> ===================================================================


> -    if (m_currentLoop < playCount() - 1 && currentTime() >=
effectiveLoopEnd()) {
> +    float now = currentTime();

I'm worried about name conflicts with wtf/CurrentTime at some point, but that's
not
your problem.

> +bool HTMLMediaElement::processingUserGesture()
> +{
> +    Frame* frame = document()->frame();
> +    FrameLoader *loader = frame ? frame->loader() : NULL;

For C++, use FrameLoader* loader 
Use 0 not NULL.

> +    return loader ? loader->userGestureHint() : false;

Should this return 'true' in the failure case for safety?

> Index: WebCore/html/HTMLMediaElement.h
> ===================================================================
> --- WebCore/html/HTMLMediaElement.h	(revision 40823)
> +++ WebCore/html/HTMLMediaElement.h	(working copy)
> @@ -159,6 +159,8 @@ private:
>      void seek(float time, ExceptionCode& ec);
>      void checkIfSeekNeeded();
>      
> +    bool processingUserGesture();

Make this |const|?

> +    enum LoadingRestrictions

Rename to LoadRestrictions
 
> +    { 
> +	   NO_RESTRICTIONS = 0,
> +	   REQUIRE_USER_GESTURE = 1 << 0, 
> +    };

Maybe rename to NoLoadRestriction, RequireUserGestureLoadRestriction


r=me with those fixes