Implement functions to use ResouceLoadStatistics on curl port. In this ticket we aim to pass all ResourceLoadStatistics tests on curl port.
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.
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.
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.
Created attachment 400716 [details] Patch
Created attachment 400719 [details] Patch
(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.
Created attachment 400727 [details] Patch
(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
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.
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.
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?
Created attachment 401123 [details] Patch
(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.
(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.
Created attachment 401426 [details] Patch
We found we can improve some tests by implementing getHostnamesWithCookies. So we updated the patch.
Committed r262791: <https://trac.webkit.org/changeset/262791> All reviewed patches have been landed. Closing bug and clearing flags on attachment 401426 [details].
<rdar://problem/64163880>
<rdar://problem/64163879>
Committed r262834: <https://trac.webkit.org/changeset/262834>
Reopen to fix WK1 crash bug.
Created attachment 401571 [details] Patch
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.
Created attachment 401753 [details] Patch
Committed r262971: <https://trac.webkit.org/changeset/262971> All reviewed patches have been landed. Closing bug and clearing flags on attachment 401753 [details].