[webkit-reviews] review denied: [Bug 188778] Web Inspector: support breakpoints for timers and animation-frame events : [Attachment 347824] Patch

[bugzilla-daemon at webkit.org]

Brian Burg <bburg at apple.com> has denied Devin Rousso <drousso at apple.com>'s
request for review:
Bug 188778: Web Inspector: support breakpoints for timers and animation-frame
events
https://bugs.webkit.org/show_bug.cgi?id=188778

Attachment 347824: Patch

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




--- Comment #16 from Brian Burg <bburg at apple.com> ---
Comment on attachment 347824
  --> https://bugs.webkit.org/attachment.cgi?id=347824
Patch

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

Overall this is very solid. I have feedback on various parts. Please post a new
patch.

> Source/JavaScriptCore/inspector/protocol/DOMDebugger.json:15
> +	       "enum": ["animation-frame", "listener", "timer"],

Good idea.

> Source/JavaScriptCore/inspector/protocol/DOMDebugger.json:37
> +	       "name": "setEventBreakpoint",

Is this going to work with older backends? Were both of these just added in
trunk or have they been shipped yet?

> Source/WebCore/inspector/InspectorInstrumentation.cpp:-84
> -static const char* const timerFiredEventName = "timerFired";

LOL, sad.

> Source/WebCore/inspector/InspectorInstrumentation.cpp:1058
> +    if (InspectorDOMDebuggerAgent* domDebuggerAgent =
instrumentingAgents.inspectorDOMDebuggerAgent())

I think it's fine to put the agent existence checks at each callsite. However,
the current approach drops the difference between sync and async pausing. Is
this expected? Does the new implementation work with things that used to be
sync (rAF events)?

> Source/WebCore/inspector/agents/InspectorDOMDebuggerAgent.cpp:142
> +    if (breakpointType != animationFrameCategoryType && breakpointType !=
listenerCategoryType && breakpointType != timerCategoryType) {

Please use the generated enum values and parsing helper. Here's an example:

	std::optional<Protocol::Timeline::Instrument> instrumentType =
Protocol::InspectorHelpers::parseEnumValueFromString<Protocol::Timeline::Instru
ment>(enumValueString);

> Source/WebCore/inspector/agents/InspectorDOMDebuggerAgent.cpp:152
> +    m_eventBreakpoints.add(formatEventBreakpointString(breakpointType,
eventName));

Please store std::pair<BreakpointType, eventName> instead of requiring string
allocations to do a hash lookup.

> Source/WebCore/inspector/agents/InspectorDOMDebuggerAgent.cpp:162
> +    if (breakpointType != animationFrameCategoryType && breakpointType !=
listenerCategoryType && breakpointType != timerCategoryType) {

Ditto.

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:434
> +	   if (breakpoint.disabled) {

Please put legacy paths together at top level here; it's okay to check
breakpoint.disabled again. It's hard to follow either code path (legacy or
modern) right now.

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:956
> +	       if (pauseData) {

Make this an early return if !pauseData?

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:1133
> +	       if (pauseData) {

Ditto

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:1319
> +	   if (popover instanceof WI.XHRBreakpointPopover && popover.result ===
WI.InputPopover.Result.Committed) {

Why?

> Source/WebInspectorUI/UserInterface/Views/EventBreakpointTreeElement.js:32
> +	   let classNames = ["breakpoint", "event", breakpoint.type];

Please adjust the last one to be "breakpoint-for-${breakpoint.type}". This
makes the type-dependent CSS class searchable to this call site.

> LayoutTests/inspector/dom-debugger/event-animation-frame-breakpoints.html:15
> +}

These two function names are confusing. How about trigger_requestAnimationFrame
and calledBy_requestAnimationFrame ?

> LayoutTests/inspector/dom-debugger/resources/event-breakpoint-utilities.js:2
> +    window.teardown = function(resolve, reject) {

It's not safe to put these into the global namespace inside the Inspector as
other utility files could pick the same names. The pattern used in other files
is to namespace these under

InspectorTest.EventBreakpoint.<the thing>

Which is more wordy but guaranteed to not have this problem.

> LayoutTests/inspector/dom-debugger/resources/event-breakpoint-utilities.js:51
> +		   InspectorTest.assert(!breakpoint.disabled, "Breakpoint
should not be disabled initially.");

I think this should reference event.data.breakpoint.