Bug 128410

Summary: Add API for accessing session ephemerality from a page bundle
Product: WebKit Reporter: Martin Hock <mhock>
Component: WebKit2Assignee: Martin Hock <mhock>
Severity: Normal CC: ap, commit-queue
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Description Flags
ap: review-
expose just the bool from bundle
ap: review+, ap: commit-queue-
rename method none

Description Martin Hock 2014-02-07 16:11:23 PST
Add API for accessing session from a page or bundle.
Comment 1 Martin Hock 2014-02-07 16:17:07 PST
Created attachment 223510 [details]
Comment 2 Alexey Proskuryakov 2014-02-07 16:57:42 PST
Comment on attachment 223510 [details]

View in context: https://bugs.webkit.org/attachment.cgi?id=223510&action=review

I think that it is tricky to expose a session API when we don't really track sessions internally yet.

I would suggest not doing anything in UIProcess API, and exposing WKBundlePageIsUsingEphemeralSession in bundle API.

> Source/WebKit2/UIProcess/API/C/WKPage.cpp:1496
> +    return toAPI(session.release().leakRef());

An API function that returns something that should be released should have "create" or "copy" in the name, not "get".

This one would be "WKPageCopySession" (but note the above, let's not add it yet).

> Source/WebKit2/UIProcess/APISession.h:39
> +    static PassRefPtr<Session> fromID(uint64_t sessionID);

This looks like something SessionTracker would be responsible for, not Session itself.

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp:548
> +    RefPtr<API::Session> session = API::Session::fromID(toImpl(pageRef)->sessionID());

UI process API and bundle API are different. APISession.h file is in UIProcess directory, so it cannot be used here.

You have this header indirectly included via WKSharedAPICast.h. But it shouldn't be there, as WKSession.h itself is in UIProcess. The include should be in WKAPICast.h instead.
Comment 3 Martin Hock 2014-02-07 17:23:22 PST
Created attachment 223523 [details]
expose just the bool from bundle
Comment 4 Alexey Proskuryakov 2014-02-08 02:24:02 PST
Comment on attachment 223523 [details]
expose just the bool from bundle

View in context: https://bugs.webkit.org/attachment.cgi?id=223523&action=review

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2110
> +bool WebPage::isEphemeral() const

Why not use the same name, isUsingEphemeralSession()? "Page is ephemeral" is not a concept we have.
Comment 5 Martin Hock 2014-02-10 10:10:22 PST
Created attachment 223722 [details]
rename method
Comment 6 WebKit Commit Bot 2014-02-10 10:54:43 PST
Comment on attachment 223722 [details]
rename method

Clearing flags on attachment: 223722

Committed r163800: <http://trac.webkit.org/changeset/163800>
Comment 7 WebKit Commit Bot 2014-02-10 10:54:45 PST
All reviewed patches have been landed.  Closing bug.