Bug 55287 – <audio> and <video> should respect private browsing mode

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Eric Carlson]

Some media engines download files for <audio> and <video> directly, so they need enough information to manage media data appropriately.

[Eric Carlson]

<rdar://problem/9057699>

[Eric Carlson]

Created attachment 83924 [details]
Proposed patch

[Chris Marrin]

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

[Eric Carlson]

http://trac.webkit.org/changeset/80030

[WebKit Review Bot]

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

[Darin Adler]

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.