[webkit-reviews] review denied: [Bug 128410] Add API for accessing session from a page or bundle : [Attachment 223510] patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Martin Hock <mhock at apple.com>'s
request for review:
Bug 128410: Add API for accessing session from a page or bundle
https://bugs.webkit.org/show_bug.cgi?id=128410

Attachment 223510: patch
https://bugs.webkit.org/attachment.cgi?id=223510&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
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.