[webkit-reviews] review granted: [Bug 50799] Add Memory Sampler to WebKit : [Attachment 77976] patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Stephanie Lewis
<slewis at apple.com>'s request for review:
Bug 50799: Add Memory Sampler to WebKit
https://bugs.webkit.org/show_bug.cgi?id=50799

Attachment 77976: patch
https://bugs.webkit.org/attachment.cgi?id=77976&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=77976&action=review

Please make sure to address these comments before landing.

> WebKit2/WebKit2Prefix.h:71
> +#if PLATFORM(MAC)
> +#define ENABLE_MEMORY_SAMPLER 1
> +#endif

I don't see this #define used anywhere. What's it for?

> WebKit2/Shared/WebMemorySampler.cpp:34
> +static WebMemorySampler *sharedMemorySampler = 0;

Should be "WebMemorySampler* ".

You can put this static inside WebMemorySampler::shared().

Since statics are implicitly initialized to 0, there's no need to explicitly
assign 0 here.

> WebKit2/Shared/WebMemorySampler.cpp:137
> +    m_separator = String("\t");    

You should initialize m_separator in the WebMemorySampler constructor. It's
difficult to reason about data members that are initialized at unpredictable
times.

> WebKit2/Shared/WebMemorySampler.cpp:138
> +    String header = String(); 

A simpler way to say this is "String header;"

> WebKit2/Shared/WebMemorySampler.cpp:166
> +    String statString = String(); 

"String statString;"

> WebKit2/UIProcess/WebContext.cpp:208
> +	   // way to get name of process from UI side?

What does this comment mean?

> WebKit2/UIProcess/WebContext.cpp:557
> +    

Our style is to exclude braces from one-line if statements.

> WebKit2/UIProcess/WebContext.cpp:580
> +    

Our style is to exclude braces from one-line if statements.