[webkit-reviews] review granted: [Bug 55287] <audio> and <video> should respect private browsing mode : [Attachment 83924] Proposed patch

[bugzilla-daemon at webkit.org]

Chris Marrin <cmarrin at apple.com> has granted Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 55287: <audio> and <video> should respect private browsing mode
https://bugs.webkit.org/show_bug.cgi?id=55287

Attachment 83924: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=83924&action=review

------- Additional Comments from Chris Marrin <cmarrin at apple.com>
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