Bug 207692 – [Curl] Implement functions to use ResourceLoadStatistics.

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Takashi Komori]

Implement functions to use ResouceLoadStatistics on curl port.
In this ticket we aim to pass all ResourceLoadStatistics tests on curl port.

[Takashi Komori]

Created attachment 390635 [details]
Implement some functions for ResouceLoadStatistics.

We implement some functions for curl port while refferring cocoa port.
Now 150 ResourceLoadStatistics tests pass and 11 tests fail.

[Ross Kirsling]

Comment on attachment 390635 [details]
Implement some functions for ResouceLoadStatistics.

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

A couple of nitpicks from me.

> Source/WebKit/NetworkProcess/curl/NetworkDataTaskCurl.cpp:60
> +    bool blockCookies = false;
> +#if ENABLE(RESOURCE_LOAD_STATISTICS)
> +    blockCookies = storedCredentialsPolicy == WebCore::StoredCredentialsPolicy::EphemeralStateless || shouldBlockCookies(request);
> +#endif

nit: Seems like an #else would be slightly better here.

> Tools/WebKitTestRunner/TestController.cpp:3115
> +    explicit GetAllStorageAccessEntriesCallbackContext(TestController& controller, CompletionHandler<void(Vector<String>&&)>&& handler)

nit: You shouldn't need `explicit` since this has two parameters.

[Stephan Szabo]

Comment on attachment 390635 [details]
Implement some functions for ResouceLoadStatistics.

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

> Source/WebKit/NetworkProcess/curl/NetworkDataTaskCurl.cpp:59
> +    blockCookies = storedCredentialsPolicy == WebCore::StoredCredentialsPolicy::EphemeralStateless || shouldBlockCookies(request);

Looking at the usage, is there a case where we want to separate the comparison with WebCore::StoredCredentialsPolicy::EphemeralStateless and the value of shouldBlockCookies(request)?
I wonder if it'd make sense to check the credentials policy inside shouldBlockCookies to prevent the possibility of misuse if the m_storedCredentialsPolicy == WebCore::StoredCredentialsPolicy::EphemeralStateless type comparsion is always required.

[Takashi Komori]

Created attachment 400716 [details]
Patch

[Takashi Komori]

Created attachment 400719 [details]
Patch

[Takashi Komori]

(In reply to Ross Kirsling from comment #2)
> Comment on attachment 390635 [details]
> Implement some functions for ResouceLoadStatistics.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=390635&action=review
> 
> A couple of nitpicks from me.
> 
> > Source/WebKit/NetworkProcess/curl/NetworkDataTaskCurl.cpp:60
> > +    bool blockCookies = false;
> > +#if ENABLE(RESOURCE_LOAD_STATISTICS)
> > +    blockCookies = storedCredentialsPolicy == WebCore::StoredCredentialsPolicy::EphemeralStateless || shouldBlockCookies(request);
> > +#endif
> 
> nit: Seems like an #else would be slightly better here.

I didn't add else part because now all lines are inside if clause.
Is it okay?

> > Tools/WebKitTestRunner/TestController.cpp:3115
> > +    explicit GetAllStorageAccessEntriesCallbackContext(TestController& controller, CompletionHandler<void(Vector<String>&&)>&& handler)
> 
> nit: You shouldn't need `explicit` since this has two parameters.

Fixed.

[Takashi Komori]

Created attachment 400727 [details]
Patch

[Takashi Komori]

(In reply to Stephan Szabo from comment #3)
> Comment on attachment 390635 [details]
> Implement some functions for ResouceLoadStatistics.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=390635&action=review
> 
> > Source/WebKit/NetworkProcess/curl/NetworkDataTaskCurl.cpp:59
> > +    blockCookies = storedCredentialsPolicy == WebCore::StoredCredentialsPolicy::EphemeralStateless || shouldBlockCookies(request);
> 
> Looking at the usage, is there a case where we want to separate the
> comparison with WebCore::StoredCredentialsPolicy::EphemeralStateless and the
> value of shouldBlockCookies(request)?
> I wonder if it'd make sense to check the credentials policy inside
> shouldBlockCookies to prevent the possibility of misuse if the
> m_storedCredentialsPolicy ==
> WebCore::StoredCredentialsPolicy::EphemeralStateless type comparsion is
> always required.

We put together comparing StoredCredentialsPolicy and calling NetworkStorageSession::shouldBlockCookies

[Basuke Suzuki]

Comment on attachment 400727 [details]
Patch

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

This is informal review: LGTM excepts for tiny documentation improvement.

> Source/WebCore/ChangeLog:8
> +        Implement functions which are required to implement ResourceLoadStatistics for wincairo.

This patch seems neutral to all ports using Curl. WinCairo shouldn't be specified as the target of this patch.

[Basuke Suzuki]

It is not the comment for this patch, but adding more comment makes easier to understand the intent of the patch, especially the patch contains both WebKit and WebCore, WTF. Basically what you write is the objective of the patch and that is true for WebKit for many cases. For WebCore/WTF, to achieve that goal please describe what you did on that layer more specifically.

[John Wilander]

Comment on attachment 400727 [details]
Patch

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

LGTM. See my one comment though. I'll let someone more familiar with the Curl integration to make the final call.

> Source/WebCore/platform/network/curl/CookieJarDB.cpp:622
> +bool CookieJarDB::deleteCookiesForHostname(const String& hostname, bool includeHttpOnlyCookies)

Why switch to a bool here when we have the IncludeHttpOnlyCookies enum that helps with self documentation?

[Takashi Komori]

Created attachment 401123 [details]
Patch

[Takashi Komori]

(In reply to Basuke Suzuki from comment #9)
> Comment on attachment 400727 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=400727&action=review
> 
> This is informal review: LGTM excepts for tiny documentation improvement.
> 
> > Source/WebCore/ChangeLog:8
> > +        Implement functions which are required to implement ResourceLoadStatistics for wincairo.
> 
> This patch seems neutral to all ports using Curl. WinCairo shouldn't be
> specified as the target of this patch.

Thank you for your review.
Changed to curl.

[Takashi Komori]

(In reply to John Wilander from comment #11)
> Comment on attachment 400727 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=400727&action=review
> 
> LGTM. See my one comment though. I'll let someone more familiar with the
> Curl integration to make the final call.
> 
> > Source/WebCore/platform/network/curl/CookieJarDB.cpp:622
> > +bool CookieJarDB::deleteCookiesForHostname(const String& hostname, bool includeHttpOnlyCookies)
> 
> Why switch to a bool here when we have the IncludeHttpOnlyCookies enum that
> helps with self documentation?

Thank you very much for the review.
There is no reason using bool value so we changed to IncludeHttpOnlyCookies.

[Takashi Komori]

Created attachment 401426 [details]
Patch

[Takashi Komori]

We found we can improve some tests by implementing getHostnamesWithCookies.
So we updated the patch.

[EWS]

Committed r262791: <https://trac.webkit.org/changeset/262791>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 401426 [details].

[Radar WebKit Bug Importer]

<rdar://problem/64163880>

[Radar WebKit Bug Importer]

<rdar://problem/64163879>

[Fujii Hironori]

Committed r262834: <https://trac.webkit.org/changeset/262834>

[Takashi Komori]

Reopen to fix WK1 crash bug.

[Takashi Komori]

Created attachment 401571 [details]
Patch

[Takashi Komori]

In WK1 some tests crashes because the value passed to NetworkStorageSession::clientSideCookieCap was uninitialized value.
We should not have dereferenced the variable.

This patch fixes the crash and fixes another compile issue that If we disable ENABLE_RESOURCE_LOAD_STATISTICS option, the build fails.

[Takashi Komori]

Created attachment 401753 [details]
Patch

[EWS]

Committed r262971: <https://trac.webkit.org/changeset/262971>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 401753 [details].