Bug 33889 - autobuffer value not forwarded media element to MediaPlayer
Summary: autobuffer value not forwarded media element to MediaPlayer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 30004
  Show dependency treegraph
 
Reported: 2010-01-20 02:44 PST by Philippe Normand
Modified: 2010-01-27 00:40 PST (History)
0 users

See Also:


Attachments
Forward autobuffer value to MediaPlayer instance. (1.43 KB, patch)
2010-01-20 07:42 PST, Philippe Normand
darin: review+
Details | Formatted Diff | Diff
updated patch (2.20 KB, patch)
2010-01-25 05:11 PST, Philippe Normand
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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 4 Philippe Normand 2010-01-25 05:11:33 PST
Created attachment 47341 [details]
updated patch
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.