<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - Web Inspector: High Level Memory Overview Instrument"
href="https://bugs.webkit.org/show_bug.cgi?id=153516#c4">Comment # 4</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - Web Inspector: High Level Memory Overview Instrument"
href="https://bugs.webkit.org/show_bug.cgi?id=153516">bug 153516</a>
from <span class="vcard"><a class="email" href="mailto:bburg@apple.com" title="Brian Burg <bburg@apple.com>"> <span class="fn">Brian Burg</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=269943&action=diff" name="attach_269943" title="[PATCH] Proposed Fix">attachment 269943</a> <a href="attachment.cgi?id=269943&action=edit" title="[PATCH] Proposed Fix">[details]</a></span>
[PATCH] Proposed Fix
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=269943&action=review">https://bugs.webkit.org/attachment.cgi?id=269943&action=review</a>
Backend parts look really clean.
<span class="quote">> LayoutTests/ChangeLog:11
> + Basic test for the new domain tracking commands ane events.</span >
typo: 'and'
<span class="quote">> LayoutTests/inspector/memory/tracking.html:26
> + let types = categories.reduce((list, x) => { list.push(x.type); return list; }, []).sort();</span >
This should be Array.prototype.map.
<span class="quote">> Source/JavaScriptCore/ChangeLog:8
> +</span >
Message: 'Add ENABLE(RESOURCE_USAGE). This is already used in WebCore.'
<span class="quote">> Source/JavaScriptCore/inspector/protocol/Memory.json:8
> + "id": "Event",</span >
I can't figure out this name. Why? I would have said 'ResourceUsageData' or something.
<span class="quote">> Source/JavaScriptCore/inspector/protocol/Memory.json:16
> + "id": "Category",</span >
I was expecting 'Category' to be the name of the enum. Please rename to CategoryData.
<span class="quote">> Source/JavaScriptCore/inspector/protocol/Memory.json:39
> + { "name": "timestamp", "type": "number" }</span >
One parameter per line, please. That is how most inspector protocol specifications are formatted.
<span class="quote">> Source/JavaScriptCore/inspector/protocol/Memory.json:44
> + "description": "While tracking is enabled the backend will periodically update the frontend with event data.",</span >
Nit: 'after tracking has started' (there's no 'enabled command)
<span class="quote">> Source/JavaScriptCore/inspector/protocol/Memory.json:46
> + { "name": "event", "$ref": "Event" }</span >
Ditto.
<span class="quote">> Source/JavaScriptCore/inspector/protocol/Memory.json:51
> + "description": "When tracking is complete."</span >
Please unify the tense of the tracking event descriptions.
"Memory usage tracking [has] started/stopped."
<span class="quote">> Source/JavaScriptCore/inspector/scripts/codegen/generator.py:41
> +_ALWAYS_UPPERCASED_ENUM_VALUE_SUBSTRINGS = set(['API', 'CSS', 'DOM', 'HTML', 'JIT', 'XHR', 'XML'])</span >
Did you rebaseline the generator tests?
<span class="quote">> Source/WebCore/ChangeLog:8
> +</span >
'Add a new agent that gathers data from the ResourceUsageThread...'
<span class="quote">> Source/WebCore/inspector/InspectorMemoryAgent.cpp:82
> +void InspectorMemoryAgent::sample(const ResourceUsageData& data)</span >
I would rename it to 'collectSample'. It's unclear otherwise that sample is the verb or noun.
<span class="quote">> Source/WebCore/inspector/InspectorMemoryAgent.h:66
> +#endif // !defined(InspectorMemoryAgent_h)</span >
Newline at EOF?</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>