Bug 201480

Summary: When disabling legacy private browsing for testing, change the SessionID back to what it was, not the defaultSessionID
Product: WebKit Reporter: Alex Christensen <achristensen>
Component: New BugsAssignee: Alex Christensen <achristensen>
Status: RESOLVED FIXED    
Severity: Normal CC: berto, cdumez, cgarcia, dbates, ews-watchlist, japhet, psaavedra, rwlbuis, webkit-bug-importer, youennf, zan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 200050    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
WebKitGLibAPITests build fixes
none
Patch
none
Patch none

Description Alex Christensen 2019-09-04 16:50:58 PDT
When disabling legacy private browsing for testing, change the SessionID back to what it was, not the defaultSessionID
Comment 1 Alex Christensen 2019-09-04 16:59:28 PDT
Created attachment 378028 [details]
Patch
Comment 2 youenn fablet 2019-09-04 23:51:21 PDT
Comment on attachment 378028 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=378028&action=review

> Source/WebCore/page/Page.h:940
> +    const PAL::SessionID m_initialSessionID;

It is not great to add another sessionID just for testing.
We could replace Page::enableLegacyPrivateBrowsing by something like Page::setSessionIDForTesting.
InjectedBundle would then store the real session ID and switch back and forth.

That will unblock your work.
Ultimately, we will want to do a full fix as started in bug 201475.
Comment 3 Alex Christensen 2019-09-05 07:52:30 PDT
Created attachment 378082 [details]
Patch
Comment 4 Alex Christensen 2019-09-05 08:00:16 PDT
Created attachment 378083 [details]
Patch
Comment 5 Alex Christensen 2019-09-05 08:19:24 PDT
This should not break the linux build, but it does.  Could someone look into it?
Comment 6 Zan Dobersek 2019-09-05 13:33:07 PDT
Created attachment 378112 [details]
WebKitGLibAPITests build fixes

WebKitGLibAPITests targets were throwing errors cause the WTF's config.h was used, which can't bring in the PAL export macros. Removing the WTF include flags avoids this, with WebKit's config.h getting used instead. This should be reworked further if TestWebKitAPI's config.h is ultimately preferred (which I'll ask about other people).
Comment 7 Alex Christensen 2019-09-05 16:54:35 PDT
Created attachment 378138 [details]
Patch
Comment 8 Alex Christensen 2019-09-05 20:28:26 PDT
Ready for review.  This is currently blocking what I'm working on.
Comment 9 youenn fablet 2019-09-06 02:41:25 PDT
Comment on attachment 378138 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=378138&action=review

> Source/WebKit/WebProcess/InjectedBundle/InjectedBundle.cpp:357
> +    if (enabled) {

Might be clearer to have one big if(enabled) instead of several enabled checks.
Comment 10 Alex Christensen 2019-09-06 09:05:21 PDT
Created attachment 378194 [details]
Patch
Comment 11 Alex Christensen 2019-09-06 09:20:05 PDT
http://trac.webkit.org/r249575
Comment 12 Radar WebKit Bug Importer 2019-09-06 09:21:16 PDT
<rdar://problem/55114467>