Create SessionID class.
Created attachment 224827 [details] patch
Created attachment 224829 [details] build
Created attachment 224830 [details] changelog
Created attachment 224832 [details] typo
Created attachment 224835 [details] symbol
Created attachment 224836 [details] rebase
Comment on attachment 224836 [details] rebase Why is this an identifier in WebCore instead of a simple POD like class?
(In reply to comment #7) > (From update of attachment 224836 [details]) > Why is this an identifier in WebCore instead of a simple POD like class? You're talking about SessionID? I'm not sure what you're asking me to do - take it out of the WebCore namespace? Not make it noncopyable? The reason I'm putting it in WebCore is because of this patch to add session IDs to MemoryCache: https://bugs.webkit.org/show_bug.cgi?id=127794 I split this part out because that patch was getting a bit unwieldy.
Comment on attachment 224836 [details] rebase View in context: https://bugs.webkit.org/attachment.cgi?id=224836&action=review I think it's weird that we have a SessionID class, but we don't represent sessionIDs with it. (i.e. Page stores a uint64_t instead of a "SessionID") What is the point of the SessionID class if we don't use it to represent SessionIDs? Yes, a session ID is just a POD uint64_t, but it also has other properties (such is "is this ephemeral?") that are logically an operation on a SessionID object, rather than a question you ask about an integer. > Source/WebCore/page/Page.cpp:1500 > + return m_sessionID ? m_sessionID : settings().privateBrowsingEnabled() ? SessionID::legacyPrivateSessionID : SessionID::defaultSessionID; We may not have a rule about this, but I don't think it's at all common to stack ternary operator usage like this. It definitely makes this line harder to read than if it were written with if conditionals. > Source/WebCore/page/Page.h:402 > + void setSessionID(uint64_t sessionID) { ASSERT(sessionID); m_sessionID = sessionID; } If we don't already, we should have a style guideline on this. Please don't put multiple statements on the same line. If a function definition in a header has more than one line, it should be written fully expanded (opening brace on new line, one line per statement) > Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.cpp:154 > - if (SessionTracker::isEphemeralID(sessionID)) { > + if (SessionID::isEphemeralID(sessionID)) { If we passed around SessionID objects rather than uint64_ts, then this would read: if (sessionID.isEphermeral()) { Same comment for all usage like this.
Created attachment 224950 [details] make SessionID a value class per comment
Created attachment 224952 [details] build fix
Created attachment 224973 [details] whitespace
(In reply to comment #9) > (From update of attachment 224836 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=224836&action=review > > I think it's weird that we have a SessionID class, but we don't represent sessionIDs with it. (i.e. Page stores a uint64_t instead of a "SessionID") > > What is the point of the SessionID class if we don't use it to represent SessionIDs? > > Yes, a session ID is just a POD uint64_t, but it also has other properties (such is "is this ephemeral?") that are logically an operation on a SessionID object, rather than a question you ask about an integer. I've replaced the session ID with a class. As discussed, it's not a POD as we want to define the default constructor. > > Source/WebCore/page/Page.cpp:1500 > > + return m_sessionID ? m_sessionID : settings().privateBrowsingEnabled() ? SessionID::legacyPrivateSessionID : SessionID::defaultSessionID; > > We may not have a rule about this, but I don't think it's at all common to stack ternary operator usage like this. It definitely makes this line harder to read than if it were written with if conditionals. Done. > > Source/WebCore/page/Page.h:402 > > + void setSessionID(uint64_t sessionID) { ASSERT(sessionID); m_sessionID = sessionID; } > > If we don't already, we should have a style guideline on this. Please don't put multiple statements on the same line. If a function definition in a header has more than one line, it should be written fully expanded (opening brace on new line, one line per statement) Done. > > Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.cpp:154 > > - if (SessionTracker::isEphemeralID(sessionID)) { > > + if (SessionID::isEphemeralID(sessionID)) { > > If we passed around SessionID objects rather than uint64_ts, then this would read: > if (sessionID.isEphermeral()) { > > Same comment for all usage like this. Done.
Comment on attachment 224973 [details] whitespace View in context: https://bugs.webkit.org/attachment.cgi?id=224973&action=review > Source/WebCore/page/Page.h:403 > + bool isSessionIDSet() const { return m_sessionID.isValid(); } I wonder if we need this helper at all. My automatic comment would be "shouldn't this be named isSessionIDValid instead of isSessionIDSet?", and from that comment it follows that everybody who calls it could just call sessionID().isValid() instead. > Source/WebKit2/UIProcess/APISession.cpp:59 > Session::Session(bool isEphemeral) > - : m_isEphemeral(isEphemeral) > - , m_sessionID(generateID(isEphemeral)) > + : m_sessionID(generateID(isEphemeral)) Does anybody ever create one with isEphermeral == false?
Created attachment 225086 [details] remove isSessionIDSet()
(In reply to comment #14) > (From update of attachment 224973 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=224973&action=review > > > Source/WebCore/page/Page.h:403 > > + bool isSessionIDSet() const { return m_sessionID.isValid(); } > > I wonder if we need this helper at all. > > My automatic comment would be "shouldn't this be named isSessionIDValid instead of isSessionIDSet?", and from that comment it follows that everybody who calls it could just call sessionID().isValid() instead. Yes, that makes sense. I've made this change. > > Source/WebKit2/UIProcess/APISession.cpp:59 > > Session::Session(bool isEphemeral) > > - : m_isEphemeral(isEphemeral) > > - , m_sessionID(generateID(isEphemeral)) > > + : m_sessionID(generateID(isEphemeral)) > > Does anybody ever create one with isEphermeral == false? We have ASSERT(isEphemeral) in generateID() for now along with FIXME comments like // FIXME: support creation of non-default, non-ephemeral sessions in create(). The constructor is private, so the public create method seems like the appropriate place to put the FIXME.
Comment on attachment 225086 [details] remove isSessionIDSet() That efl-wk2 failure clearly isn't due to this patch.
Comment on attachment 225086 [details] remove isSessionIDSet() Clearing flags on attachment: 225086 Committed r164611: <http://trac.webkit.org/changeset/164611>
All reviewed patches have been landed. Closing bug.
This broke an API test in debug builds: Note: Google Test filter = WebKit2.PrivateBrowsingPushStateNoHistoryCallback [==========] Running 1 test from 1 test case. [----------] Global test environment set-up. [----------] 1 test from WebKit2 [ RUN ] WebKit2.PrivateBrowsingPushStateNoHistoryCallback /Volumes/Data/slave/mavericks-debug/build/Tools/TestWebKitAPI/Tests/WebKit2/PrivateBrowsingPushStateNoHistoryCallback.cpp:39: Failure Failed /Volumes/Data/slave/mavericks-debug/build/Tools/TestWebKitAPI/Tests/WebKit2/PrivateBrowsingPushStateNoHistoryCallback.cpp:39: Failure Failed [ FAILED ] WebKit2.PrivateBrowsingPushStateNoHistoryCallback (144 ms) [----------] 1 test from WebKit2 (144 ms total)
Rolled out in <http://trac.webkit.org/r164669>, because this change broke WebKit2.PrivateBrowsingPushStateNoHistoryCallback API test, and Martin says that it will take time to fix.
Created attachment 225184 [details] restore isSessionIDSet() We need the isSessionIDSet() method in order for WebKit2 to properly use its settings to override an unset SessionID. Normally, when retrieving the SessionID from Page using sessionID(), we want to replace it with a valid session ID. With this method restored, the broken test passes.
Comment on attachment 225184 [details] restore isSessionIDSet() Need to fix pbxproj!
Created attachment 225188 [details] fix pbxproj
Comment on attachment 225188 [details] fix pbxproj Rejecting attachment 225188 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 225188, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: ess/WebPage/WebPage.h Hunk #1 succeeded at 191 (offset 1 line). Hunk #2 succeeded at 918 (offset 2 lines). patching file Source/WebKit2/WebProcess/WebPage/WebPage.messages.in patching file Source/WebKit2/WebProcess/WebProcess.cpp patching file Source/WebKit2/WebProcess/WebProcess.h patching file Source/WebKit2/WebProcess/WebProcess.messages.in Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Sam Weinig']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Full output: http://webkit-queues.appspot.com/results/5129018839924736
Created attachment 225263 [details] rebase
Comment on attachment 225263 [details] rebase Clearing flags on attachment: 225263 Committed r164726: <http://trac.webkit.org/changeset/164726>