[webkit-reviews] review granted: [Bug 187835] Resource Load Statistics: Enable basic functionality in experimental debug mode : [Attachment 345489] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has granted John Wilander <wilander at apple.com>'s
request for review:
Bug 187835: Resource Load Statistics: Enable basic functionality in
experimental debug mode
https://bugs.webkit.org/show_bug.cgi?id=187835

Attachment 345489: Patch

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




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

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

r=me

> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:323
> +bool
ResourceLoadStatisticsMemoryStore::isDebugModeOverride(ResourceLoadStatistics&
resourceStatistic)

Still unclear IMO. maybe isPrevalentDueToDebugMode() ?

> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:999
> +    batch.reserveCapacity(maxNumberOfDomainsInOneLogStatement);

reserveInitialCapacity

> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:1007
> +	       batchNumber++;

++batchNumber

> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:-968
> -#if !RELEASE_LOG_DISABLED

Shouldn't we keep this?

> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.h:116
> +    bool resourceLoadStatisticsDebugMode() const { return
m_debugModeEnabled; };

isDebugModeEnabled()

boolean getters without a prefix are very unclear.

> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.h:117
> +    void setPrevalentResourceForDebugMode(const String&);

I would keep the parameter name for clarity here.

> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.h:188
> +    String m_debugManualPrevalentResource { };

Unnecessary { }