Bug 33889

Summary:

autobuffer value not forwarded media element to MediaPlayer

Product:

WebKit

Reporter:

Philippe Normand <pnormand>

Component:

Media

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

Priority:

Version:

528+ (Nightly build)

Hardware:

OS:

OS X 10.5

Bug Depends on:

Bug Blocks:

30004

Attachments:

Description	Flags
Forward autobuffer value to MediaPlayer instance.	darin: review+
updated patch	eric: review+

Philippe Normand

Reported 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()).

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
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Philippe Normand

Comment 1 2010-01-20 07:42:40 PST

Created attachment 47032 [details] Forward autobuffer value to MediaPlayer instance.

Darin Adler

Comment 2 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.

Philippe Normand

Comment 3 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 ;)

Philippe Normand

Comment 4 2010-01-25 05:11:33 PST

Created attachment 47341 [details] updated patch

Eric Seidel (no email)

Comment 5 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.

Philippe Normand

Comment 6 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.

Note You need to log in before you can comment on or make changes to this bug.