WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
35576
WebKit should tell plug-in instances when private browsing state changes
https://bugs.webkit.org/show_bug.cgi?id=35576
Summary
WebKit should tell plug-in instances when private browsing state changes
Mark Rowe (bdash)
Reported
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.
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
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Mark Rowe (bdash)
Comment 1
2010-03-02 04:22:04 PST
Created
attachment 49800
[details]
Patch
Mark Rowe (bdash)
Comment 2
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).
Darin Adler
Comment 3
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
Mark Rowe (bdash)
Comment 4
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.
Mark Rowe (bdash)
Comment 5
2010-03-02 12:51:08 PST
Created
attachment 49832
[details]
Patch v2
Darin Adler
Comment 6
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.
Mark Rowe (bdash)
Comment 7
2010-03-02 15:14:34 PST
Landed in
r55433
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug