Bug 143731

Summary: Make sure media element loads hit content filter extensions
Product: WebKit Reporter: Brady Eidson <beidson>
Component: Page LoadingAssignee: Brady Eidson <beidson>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, jer.noble
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch v1 none

Description Brady Eidson 2015-04-14 15:14:53 PDT
Make sure media element loads hit content filter extensions

rdar://problem/20014012
Comment 1 Brady Eidson 2015-04-14 15:19:50 PDT
Created attachment 250746 [details]
Patch v1
Comment 2 Jer Noble 2015-04-14 15:27:09 PDT
Comment on attachment 250746 [details]
Patch v1

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

> LayoutTests/http/tests/contentextensions/media-filtered.html:3
> +if (window.testRunner)
> +    testRunner.waitUntilDone();

This should probably also call 'testRunner.dumpAsText()' so that the test doesn't start failing when there's an unrelated font or layout change.

> LayoutTests/http/tests/contentextensions/media-filtered.html:17
> +function videoError()
> +{
> +    console.log("Video error");
> +    if (window.testRunner)
> +        testRunner.notifyDone();
> +}

video-test.js has these nice functions called `waitForEventAndEnd()` and `waitForEventAndFail()` which would sure be handy here.

> LayoutTests/http/tests/contentextensions/text-track-blocked.html:3
> +    testRunner.waitUntilDone();

Ditto.

> LayoutTests/http/tests/contentextensions/text-track-blocked.html:17
> +function vttLoaded()
> +{
> +    console.log("vttLoaded");
> +    if (window.testRunner)
> +        testRunner.notifyDone();
> +}
> +
> +function vttError()
> +{
> +    console.log("vttError");
> +    if (window.testRunner)
> +        testRunner.notifyDone();
> +}

Ditto about `waitForEventAndEnd()` and `waitForEventAndFail()`.
Comment 3 Brady Eidson 2015-04-14 15:32:44 PDT
(In reply to comment #2)
> Comment on attachment 250746 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=250746&action=review
> 
> > LayoutTests/http/tests/contentextensions/media-filtered.html:3
> > +if (window.testRunner)
> > +    testRunner.waitUntilDone();
> 
> This should probably also call 'testRunner.dumpAsText()' so that the test
> doesn't start failing when there's an unrelated font or layout change.

I wanted to also capture the default size of RenderVideo as opposed to the actual video size as further evidence of load/not load. We've had bugs where a resource loads but sends onerror instead of onload, and this helps.
> 
> > LayoutTests/http/tests/contentextensions/media-filtered.html:17
> > +function videoError()
> > +{
> > +    console.log("Video error");
> > +    if (window.testRunner)
> > +        testRunner.notifyDone();
> > +}
> 
> video-test.js has these nice functions called `waitForEventAndEnd()` and
> `waitForEventAndFail()` which would sure be handy here.

I'm totally fine with (and prefer) keeping all the code for a test self contained for small tests like these :)

Thanks for the review!
Comment 4 WebKit Commit Bot 2015-04-14 16:48:03 PDT
Comment on attachment 250746 [details]
Patch v1

Clearing flags on attachment: 250746

Committed r182820: <http://trac.webkit.org/changeset/182820>
Comment 5 WebKit Commit Bot 2015-04-14 16:48:06 PDT
All reviewed patches have been landed.  Closing bug.