[webkit-reviews] review denied: [Bug 195301] Resource Load Statistics: Log first-party navigations with link decoration : [Attachment 363772] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has denied John Wilander
<wilander at apple.com>'s request for review:
Bug 195301: Resource Load Statistics: Log first-party navigations with link
decoration
https://bugs.webkit.org/show_bug.cgi?id=195301

Attachment 363772: Patch

https://bugs.webkit.org/attachment.cgi?id=363772&action=review




--- Comment #15 from Brent Fulgham <bfulgham at webkit.org> ---
Comment on attachment 363772
  --> https://bugs.webkit.org/attachment.cgi?id=363772
Patch

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

I think this looks right, but I'd suggest getting rid of the new column for
'gotLinkDecorationFromPrevalentResource' in ObservedDomains. It's already
modeled in the database separately.

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:83
> +constexpr auto gotLinkDecorationFromPrevalentResourceQuery = "SELECT
gotLinkDecorationFromPrevalentResource FROM ObservedDomains WHERE
registrableDomain = ?"_s;

This is trying to retrieve a column from ObservedDomains that doesn't exist (it
would needed to be added to the CREATE for ObservedDomains).

However: I think you are properly modeling it as a relationship between the two
origins, so an explicit flag is probably not needed in the table. And the fact
that we were redirected to by a prevalent domain is already represented as the
JOIN of your new table and the ObservedOrigin's existing 'isPrevalent' flag.

So I don't think you need these two queries.

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:178
> +    , m_gotLinkDecorationFromPrevalentResource(m_database,
gotLinkDecorationFromPrevalentResourceQuery)

I don't think we need these two statements (and they aren't used!)

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:1007
> +	       RELEASE_LOG_ERROR(Network, "%p -
ResourceLoadStatisticsDatabaseStore::m_updateGotLinkDecorationFromPrevalentReso
urceStatement failed, error message: %{public}s", this,
m_database.lastErrorMsg());

I don't think we want to add this column to ObservedDomains. Instead, I think
we want to teach "recursivelyFindNonPrevalentDomainsThatRedirectedToThisDomain"
to check for this, or to have our query about whether a resource has been
cross-site loaded with link decoration just join the tables, since we know the
from ID, and we can look up if one of the fromID's was prevalent.

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.h:1
81
> +    mutable WebCore::SQLiteStatement
m_gotLinkDecorationFromPrevalentResource;

I don't think we need these two statements.

>
LayoutTests/http/tests/resourceLoadStatistics/log-cross-site-load-with-link-dec
oration-expected.txt:12
> +    gotLinkDecorationFromPrevalentResource: Yes    isPrevalentResource: No

Where is this information coming from? dumpResourceLoadStatistics? I haven't
made sure that works yet :-)