Bug 78060 - [chromium] Add a layout test for losing the compositor context with a video playing
Summary: [chromium] Add a layout test for losing the compositor context with a video p...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: James Robinson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-07 17:25 PST by James Robinson
Modified: 2012-02-08 13:48 PST (History)
5 users (show)

See Also:


Attachments
Patch (51.63 KB, patch)
2012-02-07 17:27 PST, James Robinson
eric.carlson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Robinson 2012-02-07 17:25:23 PST
[chromium] Add a layout test for losing the compositor context with a video playing
Comment 1 James Robinson 2012-02-07 17:27:36 PST
Created attachment 125960 [details]
Patch
Comment 2 Ami Fischman 2012-02-07 17:44:23 PST
Comment on attachment 125960 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=125960&action=review

lgtm

> LayoutTests/platform/chromium/compositing/lost-compositor-context-with-video.html:23
> +function canplaythrough() {

Drop?

> LayoutTests/platform/chromium/compositing/lost-compositor-context-with-video.html:27
> +<video oncanplaythrough="test();"></video>

Heads-up that chromium has a bug where it fires canplaythrough immediately; since you only seek to 0 that should be fine.
Comment 3 James Robinson 2012-02-07 18:07:04 PST
does "immediately" mean that I can definitely show the first frame?
Comment 4 Ami Fischman 2012-02-07 18:15:37 PST
FTR the fire-canplaythrough-immediately crbug is http://code.google.com/p/chromium/issues/detail?id=73609

Since you're loading from file:// (as opposed to http://) I think you should be fine.
Comment 5 Eric Carlson 2012-02-08 10:11:05 PST
Comment on attachment 125960 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=125960&action=review

> LayoutTests/platform/chromium/compositing/lost-compositor-context-with-video.html:7
> +    layoutTestController.dumpAsText(true);  // This is only useful as a pixel test.

Why call dumpAsText() at all - there is no text output? I would either remove this call completely so it doesn't generate an "expected" text file, or include some text in the test so the text file say something about why it is empty.

>> LayoutTests/platform/chromium/compositing/lost-compositor-context-with-video.html:27
>> +<video oncanplaythrough="test();"></video>
> 
> Heads-up that chromium has a bug where it fires canplaythrough immediately; since you only seek to 0 that should be fine.

Can 'canplaythrough' fire when readyState is HAVE_NOTHING? If so, this will fail because setting currentTime will throw an exception.
Comment 6 Ami Fischman 2012-02-08 10:31:47 PST
(In reply to comment #5)
> >> LayoutTests/platform/chromium/compositing/lost-compositor-context-with-video.html:27
> >> +<video oncanplaythrough="test();"></video>
> > 
> > Heads-up that chromium has a bug where it fires canplaythrough immediately; since you only seek to 0 that should be fine.
> 
> Can 'canplaythrough' fire when readyState is HAVE_NOTHING? If so, this will fail because setting currentTime will throw an exception.

My understanding is that 'canplaythrough' is fired by webkit when chromium's webmediaplayer moves readyState to HAVE_ENOUGH_DATA, so this isn't an issue:
http://code.google.com/codesearch#OAMlx_jo-ck/src/webkit/media/webmediaplayer_impl.cc&exact_package=chromium&q=havenothing%20-file:third_party%20file:%5C.cc&type=cs&l=700
Comment 7 James Robinson 2012-02-08 10:45:04 PST
(In reply to comment #5)
> (From update of attachment 125960 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=125960&action=review
> 
> > LayoutTests/platform/chromium/compositing/lost-compositor-context-with-video.html:7
> > +    layoutTestController.dumpAsText(true);  // This is only useful as a pixel test.
> 
> Why call dumpAsText() at all - there is no text output? I would either remove this call completely so it doesn't generate an "expected" text file, or include some text in the test so the text file say something about why it is empty.
> 

Without the dumpAsText() DRT will produce a render tree dump, which isn't helpful for this test.
Comment 8 James Robinson 2012-02-08 13:48:11 PST
Committed r107132: <http://trac.webkit.org/changeset/107132>