Bug 201480 - When disabling legacy private browsing for testing, change the SessionID back to what it was, not the defaultSessionID
Summary: When disabling legacy private browsing for testing, change the SessionID back...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords: InRadar
Depends on:
Blocks: 200050
  Show dependency tree / graph
 
Reported: 2019-09-04 16:50 PDT by Alex Christensen
Modified: 2019-09-06 09:21 PDT (History)
11 users (show)

See Also:

Attachments
Patch (20.86 KB, patch)
2019-09-04 16:59 PDT, Alex Christensen 		no flags Details | Formatted Diff | Diff
Patch (19.08 KB, patch)
2019-09-05 07:52 PDT, Alex Christensen 		no flags Details | Formatted Diff | Diff
Patch (19.21 KB, patch)
2019-09-05 08:00 PDT, Alex Christensen 		no flags Details | Formatted Diff | Diff
WebKitGLibAPITests build fixes (517 bytes, patch)
2019-09-05 13:33 PDT, Zan Dobersek 		no flags Details | Formatted Diff | Diff
Patch (19.77 KB, patch)
2019-09-05 16:54 PDT, Alex Christensen 		no flags Details | Formatted Diff | Diff
Patch (18.49 KB, patch)
2019-09-06 09:05 PDT, Alex Christensen 		no flags Details | Formatted Diff | Diff
Show Obsolete (5) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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>