[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