[webkit-reviews] review denied: [Bug 181580] Web Inspector: ASSERT_NOT_REACHED in PageDebuggerAgent::didAddEventListener when page adds attribute event listener : [Attachment 339266] Patch

[bugzilla-daemon at webkit.org]

Brian Burg <bburg at apple.com> has denied Matt Baker <mattbaker at apple.com>'s
request for review:
Bug 181580: Web Inspector: ASSERT_NOT_REACHED in
PageDebuggerAgent::didAddEventListener when page adds attribute event listener
https://bugs.webkit.org/show_bug.cgi?id=181580

Attachment 339266: Patch

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




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

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

Please remove unrelated test changes from this patch. It's distracting and
should be separate. I don't want a mistake in a test to get this other
consequential fix rolled out.

> Source/WebCore/dom/EventTarget.cpp:84
> +	   InspectorInstrumentation::didAddEventListener(*this, eventType,
*listenerPointer, options.capture);

Please just pass the entirety of options since we may decide to care about
other options later when hash consing to look up an existing listener.

>>> Source/WebCore/inspector/agents/page/PageDebuggerAgent.cpp:173
>>> +	 auto position = eventListeners.findMatching([&](auto&
registeredListener) {
>> 
>> Style: I think we prefer to add a " " between the capture and parameters.
>> 
>>     [&] (auto& registeredListener) {
> 
> It looks like we use a mix of both, although including a space may be more
common. I'll see if our style guidelines have anything to say about this.

We don't seem to have a style guide rule for this, and style checker doesn't
parse capture lists well if at all. JSC tends to add a space and WebKit
doesn't.