[webkit-reviews] review denied: [Bug 74603] Web Inspector: Implement CSS selector profiler backend : [Attachment 119589] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Alexander Pavlov (apavlov)
<apavlov at chromium.org>'s request for review:
Bug 74603: Web Inspector: Implement CSS selector profiler backend
https://bugs.webkit.org/show_bug.cgi?id=74603

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

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=119589&action=review


> Source/WebCore/inspector/Inspector.json:1618
> +		   "id": "ProfileEntry",

SelectorProfileEntry

> Source/WebCore/inspector/Inspector.json:1629
> +		   "id": "Profile",

SelectorProfile

> Source/WebCore/inspector/Inspector.json:1633
> +		       { "name": "uid", "type": "integer", "description":
"Profile identifier (unique among CSS selector profiles.)" },

Should be of SelectorProfileId type inherited from string.

> Source/WebCore/inspector/Inspector.json:1806
> +		   "name": "setRecordingProfile",

Why would you need this?

> Source/WebCore/inspector/InspectorCSSAgent.cpp:220
> +	   .setData(data);

When will we have types collections?

> Source/WebCore/inspector/InspectorCSSAgent.cpp:245
> +    , m_recordingUserInitiatedSelectorProfile(false)

Nit: managing profile ids is way too complex. Why don't we persist the profiles
in localStorage for now? It would make your change significantly smaller.

> Source/WebCore/inspector/InspectorCSSAgent.cpp:546
> +    toggleRecordButton(true);

You should never control front-end state from the backend. Let is update itself
from the callback to startSelectorProfiler command.

> Source/WebCore/inspector/InspectorCSSAgent.h:95
> +    void clearProfiles(ErrorString*) { resetProfilerState(); }

I think it is more appropriate not to introduce getProfiles, removeProfile,
clearProfiles in this change and do front-end side persistence. I know you were
copying the CPU / Heap profile schema, but it itself is broken in some cases /
needs to persist heap snapshot on the page side in other cases.