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

Alex Christensen
Reported 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
Attachments
Patch (20.86 KB, patch)
2019-09-04 16:59 PDT, Alex Christensen 		no flags
DetailsFormatted DiffDiff
Patch (19.08 KB, patch)
2019-09-05 07:52 PDT, Alex Christensen 		no flags
DetailsFormatted DiffDiff
Patch (19.21 KB, patch)
2019-09-05 08:00 PDT, Alex Christensen 		no flags
DetailsFormatted DiffDiff
WebKitGLibAPITests build fixes (517 bytes, patch)
2019-09-05 13:33 PDT, Zan Dobersek 		no flags
DetailsFormatted DiffDiff
Patch (19.77 KB, patch)
2019-09-05 16:54 PDT, Alex Christensen 		no flags
DetailsFormatted DiffDiff
Patch (18.49 KB, patch)
2019-09-06 09:05 PDT, Alex Christensen 		no flags
DetailsFormatted DiffDiff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.
Alex Christensen
Comment 1 2019-09-04 16:59:28 PDT
Created attachment 378028 [details] Patch
youenn fablet
Comment 2 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.
Alex Christensen
Comment 3 2019-09-05 07:52:30 PDT
Created attachment 378082 [details] Patch
Alex Christensen
Comment 4 2019-09-05 08:00:16 PDT
Created attachment 378083 [details] Patch
Alex Christensen
Comment 5 2019-09-05 08:19:24 PDT
This should not break the linux build, but it does. Could someone look into it?
Zan Dobersek
Comment 6 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).
Alex Christensen
Comment 7 2019-09-05 16:54:35 PDT
Created attachment 378138 [details] Patch
Alex Christensen
Comment 8 2019-09-05 20:28:26 PDT
Ready for review. This is currently blocking what I'm working on.
youenn fablet
Comment 9 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.
Alex Christensen
Comment 10 2019-09-06 09:05:21 PDT
Created attachment 378194 [details] Patch
Alex Christensen
Comment 11 2019-09-06 09:20:05 PDT
http://trac.webkit.org/r249575
Radar WebKit Bug Importer
Comment 12 2019-09-06 09:21:16 PDT
<rdar://problem/55114467>
Note You need to log in before you can comment on or make changes to this bug.