| Summary: | <audio> and <video> should respect private browsing mode | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Eric Carlson <eric.carlson> | ||||
| Component: | Media | Assignee: | Eric Carlson <eric.carlson> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | abarth, eric, webkit.review.bot | ||||
| Priority: | P2 | ||||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | All | ||||||
| OS: | All | ||||||
| Attachments: |
|
||||||
|
Description
Eric Carlson
2011-02-25 19:06:09 PST
Created attachment 83924 [details]
Proposed patch
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 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 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. |