Refactor SessionID to support multiple persistent sessions
Created attachment 308352 [details] Patch
gtk == Forgot to add the new cpp file to cmake, of course. Still reviewable, would fix before landing.
Comment on attachment 308352 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=308352&action=review > Source/WebCore/page/SessionID.cpp:37 > +static uint64_t currentPersistentID = 1; I don't think this will work. Other processes will also need to be able to tell whether this session is ephemeral or not, and their static variables won't be synchronized. We should probably make a SessionID a wrapper around a bool and a uint64_t.
(In reply to Alex Christensen from comment #3) > Comment on attachment 308352 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=308352&action=review > > > Source/WebCore/page/SessionID.cpp:37 > > +static uint64_t currentPersistentID = 1; > > I don't think this will work. Other processes will also need to be able to > tell whether this session is ephemeral or not, and their static variables > won't be synchronized. We should probably make a SessionID a wrapper around > a bool and a uint64_t. Yup, of course it won't, duh.
(In reply to Alex Christensen from comment #3) > We should probably make a SessionID a wrapper around a bool and a uint64_t. Going to leave it a uint64_t, reserving the high bit for the bool.
Created attachment 308406 [details] Patch
Comment on attachment 308406 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=308406&action=review > Source/WebCore/page/SessionID.cpp:35 > +static bool generationProtectionEnabled = false; No need for the = false. > Source/WebCore/page/SessionID.h:90 > + return sessionID.isValid(); Is an invalid sessionID really an decoding failure? Does something stop us from encoding invalid values?
Comment on attachment 308406 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=308406&action=review > Source/WebCore/page/SessionID.cpp:34 > +static uint64_t currentEphemeralID = SessionID::LegacyPrivateSessionID; Couldn't we just start at std::numeric_limits<uint64_t>::max() and count backwards? > Source/WebCore/page/SessionID.h:40 > + enum { Don't we like enum classes these days? Also, does this need to be public?
(In reply to Alex Christensen from comment #8) > Comment on attachment 308406 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=308406&action=review > > > Source/WebCore/page/SessionID.cpp:34 > > +static uint64_t currentEphemeralID = SessionID::LegacyPrivateSessionID; > > Couldn't we just start at std::numeric_limits<uint64_t>::max() and count > backwards? Could we? Yes. But that would be a magic constant with no explanation. Makes sense to me to have the persistent and ephemeral IDs work the same way - Start at the shared default for each and then count up. > > > Source/WebCore/page/SessionID.h:40 > > + enum { > > Don't we like enum classes these days? Also, does this need to be public? We do like enum classes with standalone types but are not adverse to enums inside of classes. Also, the purposes of these enums are not for type safety, but rather to capture some magic constants. If it were an enum class there'd be a lot of unnecessary casting-to-uint64_t literally everywhere they were used.
(In reply to Alex Christensen from comment #8) > Comment on attachment 308406 [details] > > > Source/WebCore/page/SessionID.h:40 > > + enum { > Also, does this need to be public? It does not need to be, but it does need to be declared before the 4 static c'tors that use them. I'll add a second private section for the enums.
(In reply to Andy Estes from comment #7) > Comment on attachment 308406 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=308406&action=review > > > Source/WebCore/page/SessionID.cpp:35 > > +static bool generationProtectionEnabled = false; > > No need for the = false. Indeed. > > > Source/WebCore/page/SessionID.h:90 > > + return sessionID.isValid(); > > Is an invalid sessionID really an decoding failure? Does something stop us > from encoding invalid values? I think so. The invalid ones should only be used for Hashing, and even if you were to encode a HashSet/HashMap with these in it you'd never actually encode/decode invalid sessionIDs.
Created attachment 308411 [details] Patch for landing Will CQ+ after EWS and local API tests run.
(In reply to Brady Eidson from comment #10) > (In reply to Alex Christensen from comment #8) > > Comment on attachment 308406 [details] > > > > > Source/WebCore/page/SessionID.h:40 > > > + enum { > > Also, does this need to be public? > > It does not need to be, but it does need to be declared before the 4 static > c'tors that use them. > > I'll add a second private section for the enums. Actually yes, they need to be public.
Created attachment 308414 [details] Patch for landing
Comment on attachment 308414 [details] Patch for landing Attachment 308414 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3621386 Number of test failures exceeded the failure limit.
Created attachment 308429 [details] Archive of layout-test-results from ews100 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-elcapitan Platform: Mac OS X 10.11.6
That "&" should be a "|" :) (Rebuilding the world now...)
Comment on attachment 308414 [details] Patch for landing Attachment 308414 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3621314 Number of test failures exceeded the failure limit.
Created attachment 308430 [details] Archive of layout-test-results from ews115 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 308414 [details] Patch for landing Attachment 308414 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3621439 Number of test failures exceeded the failure limit.
Created attachment 308431 [details] Archive of layout-test-results from ews107 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Created attachment 308435 [details] Patch for EWS
(In reply to Andy Estes from comment #7) > > > Source/WebCore/page/SessionID.h:90 > > + return sessionID.isValid(); > > Is an invalid sessionID really an decoding failure? Does something stop us > from encoding invalid values? Unfortunately (or perhaps fortunately) this came up in a single API test. I'm replacing the ASSERTs with FIXMEs for now, as I still think it's the right way to go.
Created attachment 308445 [details] Patch for landing
Comment on attachment 308445 [details] Patch for landing Clearing flags on attachment: 308445 Committed r215907: <http://trac.webkit.org/changeset/215907>
All reviewed patches have been landed. Closing bug.