[webkit-reviews] review denied: [Bug 56983] [chromium] Implement preload=none, setPreload hooks to media player : [Attachment 86851] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has denied Victoria Kirst
<vrk at chromium.org>'s request for review:
Bug 56983: [chromium] Implement preload=none, setPreload hooks to media player
https://bugs.webkit.org/show_bug.cgi?id=56983

Attachment 86851: Patch
https://bugs.webkit.org/attachment.cgi?id=86851&action=review

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=86851&action=review

r- for now as I am not sure the details are quite right yet.

> Source/WebKit/chromium/public/WebMediaPlayer.h:103
> +    virtual bool setPreload(Preload) { return false; };

Does this function really  need to return a bool?


> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:190
> +    return static_cast<unsigned>(MediaPlayer::MetaData);

Don't you want to return m_preload here instead of hard coding
MediaPlayer::MetaData?


> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:216
> +    Frame* frame = static_cast<HTMLMediaElement*>(
> +	   m_mediaPlayer->mediaPlayerClient())->document()->frame();

This indentation is odd, I thought at first that a stray tab had snuck in. I am
not sure that the line break helps readability.


> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:259
> +    if (m_delayingLoad)
> +	   startDelayedLoad();

If you don't set m_delayingLoad to false before calling startDelayedLoad(),
won't it ASSERT if this is called when m_preload == MediaPlayer::None?


> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:466
> +    if (m_delayingLoad && m_preload != MediaPlayer::None)
> +	   startDelayedLoad();

Ditto.