[chromium] Add setPreload hooks to Chromium media player
Created attachment 86730 [details] Patch
Comment on attachment 86730 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=86730&action=review > Source/WebKit/chromium/ChangeLog:9 > + setAutobuffer methods. This has been replace with Typos: methods -> method, and replace -> replaced
Created attachment 86851 [details] Patch
Comment on attachment 86851 [details] Patch 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.
Created attachment 87226 [details] Patch
Comment on attachment 86851 [details] Patch 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).
Comment on attachment 87226 [details] Patch Rejecting attachment 87226 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'apply-..." exit_code: 2 Last 500 characters of output: s). patching file Source/WebKit/chromium/src/WebMediaPlayerClientImpl.h Hunk #1 FAILED at 76. Hunk #2 succeeded at 88 (offset 1 line). Hunk #3 succeeded at 108 (offset 1 line). Hunk #4 succeeded at 131 (offset 1 line). Hunk #5 succeeded at 144 (offset 1 line). 1 out of 5 hunks FAILED -- saving rejects to file Source/WebKit/chromium/src/WebMediaPlayerClientImpl.h.rej Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Eric Carlson', u'--for..." exit_code: 1 Full output: http://queues.webkit.org/results/8273869
Since there were merge conflicts, I'll ask scherkus or someone to land by hand!
Attachment 87226 [details] was posted by a committer and has review+, assigning to Victoria Kirst for commit.
blah I landed this a while back: http://trac.webkit.org/changeset/82641 apologies (again!) for not updating this bug
Comment on attachment 87226 [details] Patch Obsoleting patch since it was landed a while ago.