Unify private browsing with sessions. Remove private browsing setting from WebCore::Settings and replace use with sessions. Reference-count sessions in SessionTracker to properly dispose of unused sessions and to allow implementation of WebContext::somePageIsPrivateBrowsing (needed for WebIconDatabase's private browsing setting). Update corresponding WebKit2 session code.
Created attachment 226453 [details] patch
Created attachment 226459 [details] style + gtk build fix
Created attachment 226554 [details] efl + gtk build fixes
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Created attachment 226601 [details] efl fix + restore test
Created attachment 226948 [details] new window inherits session
Created attachment 226960 [details] gtk build fix
Comment on attachment 226960 [details] gtk build fix The refactoring looks like a good idea. This patch also includes a bug fix, which is not the right way to fix it. As discussed in person, please fix it in a separate bug. When session ID is set in createNewPage, it should be captured in WebPageCreationParameters, and passed back to WebProcess as a response to createNewPage.
Created attachment 227334 [details] remove bug fix
Comment on attachment 227334 [details] remove bug fix View in context: https://bugs.webkit.org/attachment.cgi?id=227334&action=review > Source/WebCore/ChangeLog:8 > + Unless otherwise noted, the following consists solely of mechanical changes to replace querying WebCore::Settings::privateBrowsingEnabled() with Page::sessionID().isEphemeral(). In addition to this, the patch fixes session leaks by introducing refcounting. I'm very concerned about this, as I think that it's not possible to implement session refcounting for as long as we support old private browsing SPI. Having a large patch with many mechanical changes makes it very difficult to verify whether this works. > Source/WebCore/html/HTMLMediaElement.cpp:1237 > + bool privateMode = document().page() && document().page()->sessionID().isEphemeral(); SessionID still annoys me to no end. An ID can't be ephemeral, it's a concrete immutable unsigned number. I think that for this patch, a reasonable approach would be changing page()->sessionID().isEphemeral() to page()->usesEphemeralSession(), and then we should fix the SessionID class. > Source/WebCore/page/Page.cpp:1497 > + ASSERT(m_sessionID.isValid()); I thought this change made it do that every page had a session. How can one be invalid? > Source/WebCore/page/Page.cpp:1517 > + if (sessionID != m_sessionID) { > + m_sessionID = sessionID; > + > + for (Frame* frame = &mainFrame(); frame; frame = frame->tree().traverseNext()) > + frame->document()->privateBrowsingStateDidChange(); > + > + // Collect the PluginViews in to a vector to ensure that action the plug-in takes > + // from below privateBrowsingStateChanged does not affect their lifetime. > + auto views = pluginViews(); > + > + for (unsigned i = 0; i < views.size(); ++i) > + views[i]->privateBrowsingStateChanged(sessionID.isEphemeral()); > + } It might not matter in current use, but this code is not correct in general case - we probably don't want to call privateBrowsingStateDidChange when session changes, but private browsing state doesn't change. Also, - we prefer early return to nesting code; - for loops should be C++ style. > Tools/TestWebKitAPI/Tests/WebKit2/EphemeralSessionPushStateNoHistoryCallback.cpp:47 > +TEST(WebKit2, EphemeralSessionPushStateNoHistoryCallback) Does this test for yet another bug that is fixed in this patch? Which bug is that?
Created attachment 227800 [details] remove changes to reference counting, change isUsingEphemeralSession to usesEphemeralSession
Created attachment 227810 [details] fix gtk build + address Page comments
(In reply to comment #10) > (From update of attachment 227334 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=227334&action=review > > > Source/WebCore/ChangeLog:8 > > + Unless otherwise noted, the following consists solely of mechanical changes to replace querying WebCore::Settings::privateBrowsingEnabled() with Page::sessionID().isEphemeral(). > > In addition to this, the patch fixes session leaks by introducing refcounting. > > I'm very concerned about this, as I think that it's not possible to implement session refcounting for as long as we support old private browsing SPI. Having a large patch with many mechanical changes makes it very difficult to verify whether this works. OK, I have gotten rid of the refcounting changes for this patch. > > Source/WebCore/html/HTMLMediaElement.cpp:1237 > > + bool privateMode = document().page() && document().page()->sessionID().isEphemeral(); > > SessionID still annoys me to no end. An ID can't be ephemeral, it's a concrete immutable unsigned number. > > I think that for this patch, a reasonable approach would be changing page()->sessionID().isEphemeral() to page()->usesEphemeralSession(), and then we should fix the SessionID class. Renamed (as well as renaming the parallel WebPage::isUsingEphemeralSession()). > > Source/WebCore/page/Page.cpp:1497 > > + ASSERT(m_sessionID.isValid()); > > I thought this change made it do that every page had a session. How can one be invalid? You're right, this is unnecessary, so I've removed it. > > Source/WebCore/page/Page.cpp:1517 > > + if (sessionID != m_sessionID) { > > + m_sessionID = sessionID; > > + > > + for (Frame* frame = &mainFrame(); frame; frame = frame->tree().traverseNext()) > > + frame->document()->privateBrowsingStateDidChange(); > > + > > + // Collect the PluginViews in to a vector to ensure that action the plug-in takes > > + // from below privateBrowsingStateChanged does not affect their lifetime. > > + auto views = pluginViews(); > > + > > + for (unsigned i = 0; i < views.size(); ++i) > > + views[i]->privateBrowsingStateChanged(sessionID.isEphemeral()); > > + } > > It might not matter in current use, but this code is not correct in general case - we probably don't want to call privateBrowsingStateDidChange when session changes, but private browsing state doesn't change. > > Also, > - we prefer early return to nesting code; > - for loops should be C++ style. I've updated the logic to try to implement this. I don't know how to improve the first "for" loop since it's doing a nonstandard iteration. > > Tools/TestWebKitAPI/Tests/WebKit2/EphemeralSessionPushStateNoHistoryCallback.cpp:47 > > +TEST(WebKit2, EphemeralSessionPushStateNoHistoryCallback) > > Does this test for yet another bug that is fixed in this patch? Which bug is that? This test simply reimplements PrivateBrowsingPushStateNoHistoryCallback for sessions. It didn't pass before these changes because we needed to unify private browsing with sessions, which is what this bug is fixing.
Comment on attachment 227810 [details] fix gtk build + address Page comments View in context: https://bugs.webkit.org/attachment.cgi?id=227810&action=review The changes in WebPage::updatePreferences look like they break disabling private browsing. Was that intentional? > Source/WebCore/page/Page.cpp:1513 > + bool stateChanged = (sessionID.isEphemeral() != m_sessionID.isEphemeral()); I'd name this variable "privateBrowsingStateChanged". There is no implicit "state" that we are dealing with here. > Source/WebCore/page/Page.cpp:1526 > + for (auto &view : pluginViews()) Space position: should be "auto&", not "auto &". > Source/WebCore/page/Settings.cpp:429 > void Settings::setPrivateBrowsingEnabled(bool privateBrowsingEnabled) Does this need to remain a function on Settings object? It's misleading at call sites, because this is no longer a setting. > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2489 > - if (m_page->isSessionIDSet()) > - settings.setPrivateBrowsingEnabled(m_page->sessionID().isEphemeral()); > - else > - settings.setPrivateBrowsingEnabled(store.getBoolValueForKey(WebPreferencesKey::privateBrowsingEnabledKey())); > + if (store.getBoolValueForKey(WebPreferencesKey::privateBrowsingEnabledKey()) && !m_page->usesEphemeralSession()) > + m_page->setSessionID(SessionID::legacyPrivateSessionID()); This code used to disable private browsing when the preference was unset, but no longer does that. This looks like a bug.
Created attachment 227965 [details] address comments
Comment on attachment 227965 [details] address comments Looks good to me. Please fix Gtk build.
Created attachment 227976 [details] fix gtk build
Created attachment 227978 [details] fix typo
Comment on attachment 227978 [details] fix typo Clearing flags on attachment: 227978 Committed r166661: <http://trac.webkit.org/changeset/166661>
All reviewed patches have been landed. Closing bug.