[Webkit-unassigned] [Bug 153516] Web Inspector: High Level Memory Overview Instrument

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=153516

--- Comment #4 from Brian Burg <bburg at apple.com> ---
Comment on attachment 269943
  --> https://bugs.webkit.org/attachment.cgi?id=269943
[PATCH] Proposed Fix

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

Backend parts look really clean.

> LayoutTests/ChangeLog:11
> +        Basic test for the new domain tracking commands ane events.

typo: 'and'

> LayoutTests/inspector/memory/tracking.html:26
> +                let types = categories.reduce((list, x) => { list.push(x.type); return list; }, []).sort();

This should be Array.prototype.map.

> Source/JavaScriptCore/ChangeLog:8
> +

Message: 'Add ENABLE(RESOURCE_USAGE). This is already used in WebCore.'

> Source/JavaScriptCore/inspector/protocol/Memory.json:8
> +            "id": "Event",

I can't figure out this name. Why? I would have said 'ResourceUsageData' or something.

> Source/JavaScriptCore/inspector/protocol/Memory.json:16
> +            "id": "Category",

I was expecting 'Category' to be the name of the enum. Please rename to CategoryData.

> Source/JavaScriptCore/inspector/protocol/Memory.json:39
> +                { "name": "timestamp", "type": "number" }

One parameter per line, please. That is how most inspector protocol specifications are formatted.

> Source/JavaScriptCore/inspector/protocol/Memory.json:44
> +            "description": "While tracking is enabled the backend will periodically update the frontend with event data.",

Nit: 'after tracking has started' (there's no 'enabled command)

> Source/JavaScriptCore/inspector/protocol/Memory.json:46
> +                { "name": "event", "$ref": "Event" }

Ditto.

> Source/JavaScriptCore/inspector/protocol/Memory.json:51
> +            "description": "When tracking is complete."

Please unify the tense of the tracking event descriptions.

"Memory usage tracking [has] started/stopped."

> Source/JavaScriptCore/inspector/scripts/codegen/generator.py:41
> +_ALWAYS_UPPERCASED_ENUM_VALUE_SUBSTRINGS = set(['API', 'CSS', 'DOM', 'HTML', 'JIT', 'XHR', 'XML'])

Did you rebaseline the generator tests?

> Source/WebCore/ChangeLog:8
> +

'Add a new agent that gathers data from the ResourceUsageThread...'

> Source/WebCore/inspector/InspectorMemoryAgent.cpp:82
> +void InspectorMemoryAgent::sample(const ResourceUsageData& data)

I would rename it to 'collectSample'. It's unclear otherwise that sample is the verb or noun.

> Source/WebCore/inspector/InspectorMemoryAgent.h:66
> +#endif // !defined(InspectorMemoryAgent_h)

Newline at EOF?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20160128/ffcf5135/attachment.html>