[webkit-reviews] review granted: [Bug 63545] dom/html/level2/html/HTMLDocument12.html and dom/xhtml/level2/html/HTMLDocument12.xhtml fail on my machine : [Attachment 105418] with Tools changes

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Alexey Proskuryakov <ap at webkit.org>'s
request for review:
Bug 63545: dom/html/level2/html/HTMLDocument12.html and
dom/xhtml/level2/html/HTMLDocument12.xhtml fail on my machine
https://bugs.webkit.org/show_bug.cgi?id=63545

Attachment 105418: with Tools changes
https://bugs.webkit.org/attachment.cgi?id=105418&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=105418&action=review


The patch failed to compile on both Mac and Windows.

> Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:985
> +    if (privateStorageSession())

Since this function uses privateStorageSession() twice we might want to use a
local variable to hold the references.

> Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:989
> +    RetainPtr<CFStringRef> cfIdentifier(AdoptCF,
String::format("%s.PrivateBrowsing", base.utf8().data()).createCFString());

Seems possibly overkill to use String::format just to concatenate two strings.

> Source/WebCore/platform/network/mac/ResourceHandleMac.mm:168
> +    RetainPtr<CFHTTPCookieStorageRef> cfCookieStorage =
currentCFHTTPCookieStorage();
> +    if (cfCookieStorage)

I’d prefer to see the definition inside the if statement.

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePrivate.h:87
> +// For testing only, not to be confused with private browsing.
> +WK_EXPORT void WKBundleUsePrivateSessionForNetworkLoading(WKBundleRef
bundle);

As Eric pointed out, the verb “use” here is not great; its tense is slightly
ambiguous and makes it sound like this could be a poorly named boolean getter
function. Maybe activatePrivateSessionForNetworkLoading? Not really sure the
word “session” here is all that great. The fact that the machinery used is
named “session” seems like an implementation detail. I’m also confused that
there is a way to turn this on but not off. I’m not even sure what “private”
means.

Here’s one try at naming it: SwitchNetworkLoaderToNewNonPersistentDataStorage.
OK, that wasn’t a very good try.

> Source/WebKit/win/WebCookieManagerCFNet.cpp:45
> +    // Need to retain locally to make sure the result is valid in caller.
> +    static RetainPtr<CFHTTPCookieStorageRef> result;
> +    result = currentCFHTTPCookieStorage();

Do you want to call currentCFHTTPCookieStorage one time or multiple times?

If you call it a second time, and it returns a new value, then the return value
from the first time will be released. Is that intentional? I think the comment
ought to state that rule/intent a little more clearly.