[webkit-reviews] review denied: [Bug 183221] Telemetry for stalled webpage loads : [Attachment 334762] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has denied Keith Rollin <krollin at apple.com>'s
request for review:
Bug 183221: Telemetry for stalled webpage loads
https://bugs.webkit.org/show_bug.cgi?id=183221

Attachment 334762: Patch

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




--- Comment #2 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 334762
  --> https://bugs.webkit.org/attachment.cgi?id=334762
Patch

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

> Source/WebCore/loader/FrameLoader.cpp:237
> +	       m_frame.page()->progress().progressCompleted(m_frame, -999);

I do not think it is a good idea to hardcode and NSError code in
non-platform-specific code. -999 is so obscure here too.

> Source/WebCore/loader/FrameLoader.cpp:1831
> +    m_progressTracker->progressCompleted(-999);

I do not think it is a good idea to hardcode and NSError code in
non-platform-specific code. -999 is so obscure here too.

> Source/WebCore/loader/FrameLoader.cpp:2365
> +	       m_progressTracker->progressCompleted(error.isNull() ? 0 :
error.errorCode());

Why the isNull() check? errorCode() is 0 by default.

> Source/WebCore/loader/ProgressTracker.h:52
> +    void progressCompleted(Frame&, int errorCode);

Could we use a std::optional<ResourceError> or ResourceError::Type? so we don't
have to hardcode some error codes in a few places? Especially considering error
codes are platform-specific.

> Source/WebCore/page/DiagnosticLoggingKeys.cpp:573
> +String DiagnosticLoggingKeys::telemetryPageLoadCanceledKey()

The method name is too specific. Given the string value, using a more generic
name would make it more reusable.

As a matter of fact, I wouldn't be surprised if we already had already getters
for some of those strings.

> Source/WebCore/page/DiagnosticLoggingKeys.cpp:578
> +String DiagnosticLoggingKeys::telemetryPageLoadFailedKey()

The method name is too specific. Given the string value, using a more generic
name would make it more reusable.

> Source/WebCore/page/DiagnosticLoggingKeys.cpp:583
> +String DiagnosticLoggingKeys::telemetryPageLoadHungKey()

The method name is too specific. Given the string value, using a more generic
name would make it more reusable.

> Source/WebCore/page/DiagnosticLoggingKeys.cpp:588
> +String DiagnosticLoggingKeys::telemetryPageLoadOccurredKey()

The method name is too specific. Given the string value, using a more generic
name would make it more reusable.

> Source/WebCore/page/DiagnosticLoggingKeys.cpp:593
> +String DiagnosticLoggingKeys::telemetryPageLoadSuccessfulKey()

The method name is too specific. Given the string value, using a more generic
name would make it more reusable.

> Source/WebCore/page/Page.cpp:193
> +const static Seconds kCancelTimeThreshold { 5_s };

No need for static since it is global and const.

> Source/WebCore/page/Page.cpp:194
> +const static Seconds kHangTimeThreshold { 20_s };

ditto.

> Source/WebCore/page/Page.cpp:195
> +const static int kCanceledErrorCode { -999 };

ditto. Also, I do not think it is a good idea to hardcode and NSError code in
non-platform-specific code.

> Source/WebCore/page/Page.cpp:197
> +class TelemetryProgressTrackerClient : public ProgressTrackerClient {

Can this be moved to its own file? This is a decent amount of new code in
Page.cpp :/

Is this ever subclassed? If not, it should be marked as final?

> Source/WebCore/page/Page.cpp:199
> +    virtual ~TelemetryProgressTrackerClient() = default;

I do not think this brings anything?

> Source/WebCore/page/Page.cpp:208
> +    virtual void progressTrackerDestroyed()

Can some / most of these methods be private?

Also, as per WebKit coding style, we omit the virtual and use either
final/override.

> Source/WebCore/page/Page.cpp:263
> +	   m_reportedResults = false;

Per coding style, boolean variables need a prefix. In this case, I suggest
m_hasResportedResults.

> Source/WebCore/page/Page.cpp:268
> +	   if (m_lastChangedTime - m_previousChangedTime >= kHangTimeThreshold)

We do not usually use k prefix for global variables. Just "hangTimeThreshold"
would do.

> Source/WebCore/page/Page.cpp:283
> +	   if (*m_errorCode == kCanceledErrorCode) {

I suggest using ResourceError and rely on ResourceError::isCancellation(). Or
if you only care about the error type, we could use ResourceError::Type.

> Source/WebCore/page/Page.cpp:299
> +    void logDiagnosticMessage(String message)

const String&