[webkit-reviews] review denied: [Bug 35576] WebKit should tell plug-in instances when private browsing state changes : [Attachment 49800] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Mark Rowe (bdash) <mrowe at apple.com>'s
request for review:
Bug 35576: WebKit should tell plug-in instances when private browsing state
changes
https://bugs.webkit.org/show_bug.cgi?id=35576

Attachment 49800: Patch
https://bugs.webkit.org/attachment.cgi?id=49800&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +	   const HashSet<RefPtr<Widget> >* children = view->children();
> +	   ASSERT(children);

If the children function never returns 0, I wish it returned a reference
instead of a pointer.

> +	   HashSet<RefPtr<Widget> >::const_iterator end = children->end();
> +	   for (HashSet<RefPtr<Widget> >::const_iterator it =
children->begin(); it != end; ++it) {
> +	       Widget* widget = (*it).get();
> +	       if (!widget->isPluginView())
> +		   continue;
> +	      
static_cast<PluginView*>(widget)->privateBrowsingStateChanged(privateBrowsingEn
abled);
> +	   }

Is there a guarantee that the code called by the privateBrowsingStateChanged
function won't change the children of the FrameView*?

Is there a guarantee that the code called by the privateBrowsingStateChanged
function won't cause the Frame pointed to by the "frame" local variable to be
deleted?

I think we might need to build a vector of widgets to call before calling any
of them.

> +    m_page->privateBrowsingStateChanged();

Can't a single Settings be used by more than one page? Maybe I'm thinking only
of Objective-C.

review- because I'm concerned this won't work if the plug-in does a lot of work
in the browsing state changed function