Bug 35576 – WebKit should tell plug-in instances when private browsing state changes

• Component: Plug-ins
• Status: RESOLVED FIXED

[Mark Rowe (bdash)]

The Mac plug-in implementation uses setvalue to notify a plug-in instance when the NPNVprivateModeBool value changes (<http://trac.webkit.org/browser/trunk/WebKit/mac/Plugins/WebNetscapePluginView.mm#L833>).  The cross-platform plug-in implementation in WebCore should do the same.

[Mark Rowe (bdash)]

Created attachment 49800 [details]
Patch

[Mark Rowe (bdash)]

This is the second half of supporting private browsing mode with plug-ins (related to bug 30348 for Windows, bug 33180 for Qt, and bug 35500 for Gtk).

[Darin Adler]

Comment on attachment 49800 [details]
Patch

> +        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(privateBrowsingEnabled);
> +        }

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

[Mark Rowe (bdash)]

(In reply to comment #3)
> > +        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(privateBrowsingEnabled);
> > +        }
> 
> 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.

That’s a good point.  I’ll make this change.

> > +    m_page->privateBrowsingStateChanged();
> 
> Can't a single Settings be used by more than one page? Maybe I'm thinking only
> of Objective-C.

WebPreferences can be shared by many WebView’s, but each Page creates and owns its Settings instance.

[Mark Rowe (bdash)]

Created attachment 49832 [details]
Patch v2

[Darin Adler]

Comment on attachment 49832 [details]
Patch v2

Looks good.

I would add a comment to the Page::privateBrowsingStateChanged function explaining that the function uses local variables so it can run to completion even if the entire Page is destroyed.

[Mark Rowe (bdash)]

Landed in r55433.