[webkit-reviews] review denied: [Bug 184838] Add facility for tracking times and results of page and resource loading : [Attachment 339164] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed May 2 12:20:27 PDT 2018


Brent Fulgham <bfulgham at webkit.org> has denied Keith Rollin
<krollin at apple.com>'s request for review:
Bug 184838: Add facility for tracking times and results of page and resource
loading
https://bugs.webkit.org/show_bug.cgi?id=184838

Attachment 339164: Patch

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




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

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

Looks great! I had a few things I'd like to see changed slightly, but overall
this is ready to go. r- to make the m_parameters member const again.

> Source/WebCore/ChangeLog:5
> +	   rdar://problem/36548974

<rdar://problem/36548974>

> Source/WebKit/ChangeLog:5
> +	   rdar://problem/36548974

<rdar://problem/36548974>

> Source/WebKitLegacy/ChangeLog:5
> +	   rdar://problem/36548974

<rdar://problem/36548974>

> Source/WebCore/loader/FrameLoader.cpp:257
> +	      
platformStrategies()->loaderStrategy()->pageLoadCompleted(m_frame.loader().clie
nt().pageID().value());

if (auto pageID = m_frame.loader().client().pageID())
    platformStrategies()->loaderStrategy()->pageLoadCompleted(pageID.value());

> Source/WebKit/NetworkProcess/NetworkActivityTracker.h:79
> +#if HAVE(NW_ACTIVITY)

We could probably just use "#if HAVE(NW_ACTIVITY)" to protect
m_networkActivity, since the other items are likely useful for other ports (if
they wanted to implement some kind of network activity tracker for their
network backends). But this is fine as-is.

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:551
> +#if HAVE(NW_ACTIVITY)

I don't think you need this. All other ports will return 'false' from
NetworkProcess::trackNetworkActivity(), so just make this call on all ports.

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:168
> +    m_parameters.networkActivityTracker =
m_connection->startTrackingResourceLoad(m_parameters.webPageID,
m_parameters.identifier, isMainResource());

I don't think we should change m_parameters from const. Instead, I think you
should track m_hasNetworkActivityTracker (or something) as a separate member
(like we do for m_defersLoading), and set 'networkActivityTracker' in the copy
of m_parameters we send to the NetworkLoad object in
NetworkResourceLoader::startNetworkLoad.

> Source/WebKit/NetworkProcess/NetworkResourceLoader.h:162
> +    NetworkResourceLoadParameters m_parameters;

I don't think we should change this.


More information about the webkit-reviews mailing list