Bug 96134

Summary: Mac: <audio> and <video> should send Do Not Track when appropriate
Product: WebKit Reporter: Jer Noble <jer.noble>
Component: New BugsAssignee: Jer Noble <jer.noble>
Status: RESOLVED WONTFIX    
Severity: Normal CC: abecsi, cmarcelo, dglazkov, eric.carlson, feature-media-reviews, japhet, menard, webkit.review.bot, zoltan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch eric.carlson: review+, webkit.review.bot: commit-queue-

Description Jer Noble 2012-09-07 11:17:14 PDT 
<audio> and <video> should send Do Not Track when appropriate
Comment 1 Jer Noble 2012-09-07 11:40:26 PDT 
Created attachment 162834 [details]
Patch
Comment 2 Eric Carlson 2012-09-07 12:17:45 PDT 
Comment on attachment 162834 [details]
Patch

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

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:304
>      if ([headerFields.get() count])
>          [options.get() setObject:headerFields.get() forKey:@"AVURLAssetHTTPHeaderFieldsKey"];
> +
> +    if (player()->mediaPlayerClient()->mediaPlayerShouldSendDoNotTrackHTTPHeader())
> +        [headerFields.get() setObject:@"1" forKey:@"DNT"];

You should set the DNT header field first or AVURLAssetHTTPHeaderFieldsKey won't be set in the dictionary if there isn't a referrer or a UA header.

> LayoutTests/http/tests/media/resources/video-donottrack-check-donottrack.php:8
> +
> +

Nit: two blank lines?

> LayoutTests/http/tests/media/resources/video-donottrack-check-donottrack.php:17
> +
> +    

Ditto.

> LayoutTests/http/tests/media/resources/video-donottrack-check-donottrack.php:18
> +    error_log(var_export(headers_list(), true));

What does this do?

> LayoutTests/http/tests/media/resources/video-donottrack-check-donottrack.php:20
> +    
> +

Nit: two blank lines?

> LayoutTests/http/tests/media/video-donottrack.html:11
> +<script src=../../media-resources/video-test.js></script>
> +<script src=../../media-resources/media-file.js></script>
> +<script>
> +    function runTest () {

Nit: is there any reason this isn't in the <head>?
Comment 3 Jer Noble 2012-09-07 12:55:14 PDT 
Comment on attachment 162834 [details]
Patch

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

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:304
>> +        [headerFields.get() setObject:@"1" forKey:@"DNT"];
> 
> You should set the DNT header field first or AVURLAssetHTTPHeaderFieldsKey won't be set in the dictionary if there isn't a referrer or a UA header.

Sure thing.

>> LayoutTests/http/tests/media/resources/video-donottrack-check-donottrack.php:8
>> +
> 
> Nit: two blank lines?

I'll condense these.

>> LayoutTests/http/tests/media/resources/video-donottrack-check-donottrack.php:18
>> +    error_log(var_export(headers_list(), true));
> 
> What does this do?

This was just for debugging purposes, so that run-webkit-httpd would spit out the entire outgoing header array before returning.  I'll remove it.

Thanks!
Comment 4 Jer Noble 2012-09-07 13:18:35 PDT 
Created attachment 162852 [details]
Patch

Patch for the EWS bots.
Comment 5 Early Warning System Bot 2012-09-07 14:51:51 PDT 
Comment on attachment 162852 [details]
Patch

Attachment 162852 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13772887
Comment 6 Jer Noble 2012-09-07 15:49:15 PDT 
Created attachment 162889 [details]
Patch

Fix the Qt build.
Comment 7 Eric Carlson 2012-09-07 17:18:56 PDT 
Comment on attachment 162889 [details]
Patch

This looks fine to me, but don't you have to skip the new test on some platforms?
Comment 8 WebKit Review Bot 2012-09-08 05:34:20 PDT 
Comment on attachment 162889 [details]
Patch

Attachment 162889 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13794258

New failing tests:
http/tests/media/video-donottrack.html
Comment 9 Jer Noble 2012-09-10 11:10:39 PDT 
Committed r128081: <http://trac.webkit.org/changeset/128081>
Comment 11 Jer Noble 2012-09-10 16:39:59 PDT 
Rolled out in <http://trac.webkit.org/changeset/128129>