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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Mar 6 16:30:22 PST 2019

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

Attachment 363772: Patch


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

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.

> +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.

> +    , m_gotLinkDecorationFromPrevalentResource(m_database,

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

> +	       RELEASE_LOG_ERROR(Network, "%p -
urceStatement failed, error message: %{public}s", this,

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.

> +    mutable WebCore::SQLiteStatement

I don't think we need these two statements.

> +    gotLinkDecorationFromPrevalentResource: Yes    isPrevalentResource: No

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

More information about the webkit-reviews mailing list