Bug 35576 - WebKit should tell plug-in instances when private browsing state changes
Summary: WebKit should tell plug-in instances when private browsing state changes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Plug-ins (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Mark Rowe (bdash)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-03-02 04:20 PST by Mark Rowe (bdash)
Modified: 2010-03-02 15:14 PST (History)
3 users (show)

See Also:


Attachments
Patch (12.70 KB, patch)
2010-03-02 04:22 PST, Mark Rowe (bdash)
darin: review-
Details | Formatted Diff | Diff
Patch v2 (12.81 KB, patch)
2010-03-02 12:51 PST, Mark Rowe (bdash)
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Rowe (bdash) 2010-03-02 04:20:10 PST
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.
Comment 1 Mark Rowe (bdash) 2010-03-02 04:22:04 PST
Created attachment 49800 [details]
Patch
Comment 2 Mark Rowe (bdash) 2010-03-02 04:33:21 PST
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).
Comment 3 Darin Adler 2010-03-02 12:14:05 PST
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
Comment 4 Mark Rowe (bdash) 2010-03-02 12:25:48 PST
(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.
Comment 5 Mark Rowe (bdash) 2010-03-02 12:51:08 PST
Created attachment 49832 [details]
Patch v2
Comment 6 Darin Adler 2010-03-02 13:01:15 PST
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.
Comment 7 Mark Rowe (bdash) 2010-03-02 15:14:34 PST
Landed in r55433.