Bug 129141 – Create SessionID class

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Martin Hock]

Create SessionID class.

[Martin Hock]

Created attachment 224827 [details]
patch

[Martin Hock]

Created attachment 224829 [details]
build

[Martin Hock]

Created attachment 224830 [details]
changelog

[Martin Hock]

Created attachment 224832 [details]
typo

[Martin Hock]

Created attachment 224835 [details]
symbol

[Martin Hock]

Created attachment 224836 [details]
rebase

[Anders Carlsson]

Comment on attachment 224836 [details]
rebase

Why is this an identifier in WebCore instead of a simple POD like class?

[Martin Hock]

(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.

[Brady Eidson]

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.

[Martin Hock]

Created attachment 224950 [details]
make SessionID a value class per comment

[Martin Hock]

Created attachment 224952 [details]
build fix

[Martin Hock]

Created attachment 224973 [details]
whitespace

[Martin Hock]

(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.

[Brady Eidson]

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?

[Martin Hock]

Created attachment 225086 [details]
remove isSessionIDSet()

[Martin Hock]

(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.

[Brady Eidson]

Comment on attachment 225086 [details]
remove isSessionIDSet()

That efl-wk2 failure clearly isn't due to this patch.

[WebKit Commit Bot]

Comment on attachment 225086 [details]
remove isSessionIDSet()

Clearing flags on attachment: 225086

Committed r164611: <http://trac.webkit.org/changeset/164611>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

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)

[Alexey Proskuryakov]

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.

[Martin Hock]

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.

[Martin Hock]

Comment on attachment 225184 [details]
restore isSessionIDSet()

Need to fix pbxproj!

[Martin Hock]

Created attachment 225188 [details]
fix pbxproj

[WebKit Commit Bot]

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

[Martin Hock]

Created attachment 225263 [details]
rebase

[WebKit Commit Bot]

Comment on attachment 225263 [details]
rebase

Clearing flags on attachment: 225263

Committed r164726: <http://trac.webkit.org/changeset/164726>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.