|Summary:||autobuffer value not forwarded media element to MediaPlayer|
|Product:||WebKit||Reporter:||Philippe Normand <pnormand>|
|Version:||528+ (Nightly build)|
|OS:||OS X 10.5|
|Bug Depends on:|
Description Philippe Normand 2010-01-20 02:44:29 PST
In HTMLMediaElement parseMappedAttribute() forwards autobuffer only if m_player has been created. m_player.setAutobuffer() should also be called just after the m_player has been created too (in loadResource()).
Comment 1 Philippe Normand 2010-01-20 07:42:40 PST
Created attachment 47032 [details] Forward autobuffer value to MediaPlayer instance.
Comment 2 Darin Adler 2010-01-20 08:22:12 PST
Comment on attachment 47032 [details] Forward autobuffer value to MediaPlayer instance. Code change looks great. We normally require test cases or an explanation of why a test case is lacking. Since nobody is doing anything with the autobuffer flag right now, at least as far as I can tell with grep, I guess that's not possible here.
Comment 3 Philippe Normand 2010-01-20 08:31:21 PST
Well there is a test for autobuffer but it checks only the DOM attributes. After this patch I thought it would also be good for the MediaPlayer to call setAutobuffer() on its media engine after loading an url. Otherwise each media engine implementing setAutoBuffer() would need to manually call it. What do think? I am fine with sending an updated patch. BTW I need this for Bug 30004 ;)
Comment 5 Eric Seidel (no email) 2010-01-26 15:04:18 PST
Comment on attachment 47341 [details] updated patch How do we test this? Looks sane, but all changes should have tests or explanation as to why testing is impossible.
Comment 6 Philippe Normand 2010-01-27 00:40:20 PST
(In reply to comment #5) > (From update of attachment 47341 [details]) > How do we test this? Looks sane, but all changes should have tests or > explanation as to why testing is impossible. Well I am still new to this but I didn't see unittests for the MediaPlayer, maybe it would make sense to have some? The MediaPlayerPrivate implementations are not unittested either, afaics. Anyway I landed this patch as r53910. I will think about unittesting the MediaPlayer at least.