| Summary: | WebKit should tell plug-in instances when private browsing state changes | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Mark Rowe (bdash) <mrowe> | ||||||
| Component: | Plug-ins | Assignee: | Mark Rowe (bdash) <mrowe> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | darin, jhoneycutt, robert | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | PC | ||||||||
| OS: | OS X 10.5 | ||||||||
| Attachments: |
|
||||||||
|
Description
Mark Rowe (bdash)
2010-03-02 04:20:10 PST
Created attachment 49800 [details]
Patch
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 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 (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. Created attachment 49832 [details]
Patch v2
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.
|