[Webkit-unassigned] [Bug 56983] [chromium] Implement preload=none, setPreload hooks to media player

[bugzilla-daemon at webkit.org]

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





--- Comment #6 from Victoria Kirst <vrk at chromium.org>  2011-03-28 15:48:50 PST ---
(From update of attachment 86851)
View in context: https://bugs.webkit.org/attachment.cgi?id=86851&action=review

Thanks for looking! I uploaded a new patch with the fixes - let me know whatever questions/concerns you still have!

>> Source/WebKit/chromium/public/WebMediaPlayer.h:103
>> +    virtual bool setPreload(Preload) { return false; };
> 
> Does this function really  need to return a bool?

No it doesn't. Fixed!

>> 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?

Yes, thanks! :) Fixed!

>> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:216
>> +        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.

OK! Changed to all on one line.

>> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:259
>> +        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?

It shouldn't - the assert is for m_delayingLoad, not !m_delayingLoad (i.e. m_delayingLoad must be true whenever you first call startDelayedLoad).

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