[webkit-reviews] review granted: [Bug 233760] Move content filtering to Networking process : [Attachment 452576] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has granted Per Arne Vollan
<pvollan at apple.com>'s request for review:
Bug 233760: Move content filtering to Networking process
https://bugs.webkit.org/show_bug.cgi?id=233760

Attachment 452576: Patch

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




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

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

r=me

> Source/WebCore/loader/ContentFilter.cpp:122
> +    LOG(ContentFiltering, "ContentFilter will start filtering main resource
at <%s>.\n", url.string().ascii().data());

This is privacy-impacting. I think you need to use the %{sensitive}s logging
declaration here.

> Source/WebCore/loader/ContentFilter.cpp:176
> +	   LOG(ContentFiltering, "ContentFilter received %zu bytes of data from
<%s>.\n", data.size(), url().string().ascii().data());

Ditto (even though the old code added logging with the secret information).

> Source/WebCore/loader/ContentFilter.cpp:216
> +	   LOG(ContentFiltering, "ContentFilter will finish filtering main
resource at <%s>.\n", url().string().ascii().data());

Ditto.

> Source/WebCore/loader/ContentFilter.cpp:271
> +    LOG(ContentFiltering, "ContentFilter decided load should be %s for main
resource at <%s>.\n", state == State::Allowed ? "allowed" : "blocked",
url().string().ascii().data());

Ditto.

> Source/WebCore/loader/ContentFilter.cpp:355
> +    for (unsigned i = 0; i < m_buffers.size(); i++)

It seems like this could be a modern C++ loop:

for (auto& buffer in m_buffers)
    deliverResourceData(*buffer.buffer, buffer.encodedDataLength);

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:1296
> +    shouldFilter = m_contentFilter &&
!m_contentFilter->continueAfterDataReceived(sharedBuffer,
m_bufferedDataEncodedDataLength);

Why not:
bool shouldFilter = m_contentFilter .... <<ETC>>

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:614
> +    if (!page)

I'd just do:

if (auto* page = ...
    page->reload....