[webkit-reviews] review granted: [Bug 189213] Added dumping function for testing resource load statistics : [Attachment 348690] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted Woodrow Wang
<wwang153 at stanford.edu>'s request for review:
Bug 189213: Added dumping function for testing resource load statistics
https://bugs.webkit.org/show_bug.cgi?id=189213

Attachment 348690: Patch

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




--- Comment #4 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 348690
  --> https://bugs.webkit.org/attachment.cgi?id=348690
Patch

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

> Source/WebCore/ChangeLog:3
> +	   Added dumping function for testing resource load statistics

Maybe a better title would be: Add infrastructure to dump resource load
statistics

> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:155
> +    store->dumpResourceLoadStatistics([context, callback] (String
dumpResourceLoadStatistics) {

This is ref'ing the passed string and this function does not need to take
ownership of the string. I suggest the callback take a "const String&" as the
parameter type. Can we come up with a better name for the parameter? Maybe
resourceLoadStatistics?

> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:156
> +	  
callback(WKStringCreateWithUTF8CString(dumpResourceLoadStatistics.utf8().data()
), context);

Is there a better way to convert from WTF::String to WKStringRef. I am
surprised that this problem has not already been solved. Does it make sense to
to use toCopiedAPI()?

> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:678
> +    

Please remove the leading whitespace from this line.

> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:679
> +    StringBuilder dumpResourceLoadStatisticsMapString;

Can we come up with a better name for this variable? Maybe result?

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:500
> +    

Please remove the leading whitespace from this line.

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:502
> +	   String dumpResourceLoadStatistics = m_memoryStore ?
m_memoryStore->dumpResourceLoadStatistics() : emptyString();

Can we come up with a better name for name of the local variable? Maybe result?
or resourceLoadStatistics?

> Tools/WebKitTestRunner/InjectedBundle/Bindings/TestRunner.idl:271
> +    void setDumpResourceLoadStatistics(boolean shouldDump);

I suggest that we take a similar approach as the other dumping function is this
file and call this function dumpResourceLoadStatistics() and it should not take
any arguments. We should also group this declaration with the declarations of
the other dumping functions at the top of this file.

> Tools/WebKitTestRunner/TestController.cpp:2808
> +    WKStringRef dump;

Can we come up with a more descriptive name for this?