[webkit-reviews] review denied: [Bug 184838] Add facility for tracking times and results of page and resource loading : [Attachment 338814] Rebase to fix merge conflicts.

[bugzilla-daemon at webkit.org]

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 338814: Rebase to fix merge conflicts.

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




--- Comment #5 from Brent Fulgham <bfulgham at webkit.org> ---
Comment on attachment 338814
  --> https://bugs.webkit.org/attachment.cgi?id=338814
Rebase to fix merge conflicts.

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

Looks good! r- to fix the iterator stuff and clean up the struct a little. Nice
job!

> Source/WebKit/Configurations/WebKit.xcconfig:115
> +FRAMEWORK_AND_LIBRARY_LDFLAGS = -lobjc -framework CFNetwork -framework
CoreAudio -framework CoreFoundation -framework CoreGraphics -framework CoreText
-framework Foundation -framework ImageIO -framework IOKit -framework
WebKitLegacy -lnetwork $(WK_ACCESSIBILITY_LDFLAGS) $(WK_APPKIT_LDFLAGS)
$(WK_ASSERTION_SERVICES_LDFLAGS) $(WK_CARBON_LDFLAGS) $(WK_CORE_PDF_LDFLAGS)
$(WK_CORE_PREDICTION_LDFLAGS) $(WK_CORE_SERVICES_LDFLAGS)
$(WK_GRAPHICS_SERVICES_LDFLAGS) $(WK_IOSURFACE_LDFLAGS) $(WK_LIBWEBRTC_LDFLAGS)
$(WK_MOBILE_CORE_SERVICES_LDFLAGS) $(WK_MOBILE_GESTALT_LDFLAGS)
$(WK_OPENGL_LDFLAGS) $(WK_PDFKIT_LDFLAGS) $(WK_SAFE_BROWSING_LDFLAGS)
$(WK_UIKIT_LDFLAGS);

Is it safe to add "-lnetwork" unconditionally? Based on EWS I assume it's fine.

> Source/WebKit/NetworkProcess/NetworkActivityTracker.h:73
> +    nw_activity_t get() { return m_networkActivity.get(); }

I wonder if Soup or Curl has any similar network health features? If so, we
could make this a 'PlatformActivity' type that would vary based on platform.
But that's not something we need for this patch.

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:586
> +	       return std::nullopt;

If the network process crashed, shouldn't we create a new activity tracker so
that we can at least capture whatever data comes in for the remaining loads?

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:630
> +	  
m_networkActivityTrackers[itemIndex].m_networkActivity.complete(NetworkActivity
Tracker::CompletionCode::None);

This would be better as:

for (auto& activityTracker : m_networkActivityTrackers)
    activityTracker.m_networkActivity.complete(...)

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:644
> +	       m_networkActivityTrackers.remove(itemIndex);

I really hate this kind of iterating over a container while removing elements.
We have had so many security and stability bugs due to this kind of stuff.

Instead, I propose:

for (auto& activityTracker : m_networkActivityTrackers) {
    if (activityTracker.m_pageID == pageID)
       
acitivityTracker.m_networkActivity.complete(NetworkActivityTracker::CompletionC
ode::None);
}

m_networkActivityTrackers.removeAllMatching([&](const auto& activityTracker) {
return activityTracker.pageID == pageID; });

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:654
> +    }

return m_networkActivityTrackers.findMatching([&](const auto& item) { return
item.m_isRootActivity && item.m_pageID == pageID; });

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:664
> +    }

return m_networkActivityTrackers.findMatching([&](const auto& item) { return
item.m_resourceID == resourceID; });

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h:211
> +	   NetworkActivityTracker m_networkActivity;

Since this is a struct and doesn't encapsulate these members behind accessors,
they should be renamed without the "m_*" prefix.

> Source/WebKit/NetworkProcess/NetworkLoadParameters.h:28
> +#include "NetworkActivityTracker.h"

Could this just be a forward declaration?