Bug 55287 - <audio> and <video> should respect private browsing mode
Summary: <audio> and <video> should respect private browsing mode
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-25 19:06 PST by Eric Carlson
Modified: 2011-03-01 13:49 PST (History)
3 users (show)

See Also:


Attachments
Proposed patch (11.45 KB, patch)
2011-02-25 21:22 PST, Eric Carlson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2011-02-25 19:06:09 PST
Some media engines download files for <audio> and <video> directly, so they need enough information to manage media data appropriately.
Comment 1 Eric Carlson 2011-02-25 19:18:04 PST
<rdar://problem/9057699>
Comment 2 Eric Carlson 2011-02-25 21:22:48 PST
Created attachment 83924 [details]
Proposed patch
Comment 3 Chris Marrin 2011-02-28 13:59:18 PST
Comment on attachment 83924 [details]
Proposed patch

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

r+ after dealing with naming issues and debug glop for scary weak pointer traversal.

> Source/WebCore/dom/Document.cpp:6
> + * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2011 Apple Inc. All rights reserved.

Isn't it allowed to say 2004-2011 or something to make this a little less crazy?

> Source/WebCore/dom/Document.cpp:3973
> +        (*i)->privateBrowsingStateChanged(privateBrowsingEnabled);

Most places use 'it' rather than 'i' to make the deref of what is usually an index less weird.

Also, you're managing this list of weak pointers with the ctor/dtor. It might be good to put some debugging flags here to make sure you're not mutating this list while walking it (e.g., one of the callbacks destroys another element that is also in the list).

> Source/WebCore/dom/Document.h:989
> +    void privateBrowsingStateChanged(bool);

A boolean here seems odd. It makes it seem like 'true' means the state changed and 'false' mean it hasn't. A better name might be setPrivateBrowsingEnabled. Or better yet, the Darin approved 'use an enum' approach might be the most clear.

> Source/WebCore/dom/Element.h:6
> + * Copyright (C) 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011 Apple Inc. All rights reserved.

Ditto

> Source/WebCore/html/HTMLMediaElement.cpp:2
> + * Copyright (C) 2007, 2008, 2009, 2010, 2011 Apple Inc. All rights reserved.

Ditto

> Source/WebCore/page/Page.cpp:3
>   * Copyright (C) 2008 Torch Mobile Inc. All rights reserved. (http://www.torchmobile.com/)

Ditto

> Source/WebCore/platform/graphics/MediaPlayer.cpp:2
> + * Copyright (C) 2007, 2008, 2009, 2010, 2011 Apple Inc. All rights reserved.

Ditto

> Source/WebCore/platform/graphics/MediaPlayer.h:2
> + * Copyright (C) 2007, 2008, 2009, 2010, 2011 Apple Inc. All rights reserved.

Ditto
Comment 4 Eric Carlson 2011-03-01 12:55:19 PST
http://trac.webkit.org/changeset/80030
Comment 5 WebKit Review Bot 2011-03-01 13:23:31 PST
http://trac.webkit.org/changeset/80030 might have broken Qt Linux Release
The following tests are not passing:
editing/deleting/4916235-1.html
editing/deleting/5390681.html
editing/deleting/5546763.html
editing/deleting/delete-3775172-fix.html
editing/deleting/delete-at-paragraph-boundaries-007.html
editing/deleting/delete-before-block-image-1.html
Comment 6 Darin Adler 2011-03-01 13:49:21 PST
Comment on attachment 83924 [details]
Proposed patch

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

>> Source/WebCore/dom/Document.cpp:6
>> + * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2011 Apple Inc. All rights reserved.
> 
> Isn't it allowed to say 2004-2011 or something to make this a little less crazy?

No. Years of publication need to all be listed. A range does not make it clear which years this was published in.