[webkit-reviews] review denied: [Bug 217196] Web Inspector: Blackboxing URL patters should apply to source mapped locations : [Attachment 414364] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has denied Nikita Vasilyev
<nvasilyev at apple.com>'s request for review:
Bug 217196: Web Inspector: Blackboxing URL patters should apply to source
mapped locations
https://bugs.webkit.org/show_bug.cgi?id=217196

Attachment 414364: Patch

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




--- Comment #3 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 414364
  --> https://bugs.webkit.org/attachment.cgi?id=414364
Patch

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

r- as this appears to be a purely visual change that doesn't actually make
blackboxing work with "virtual" source map resources.  If you set a breakpoint
inside the "virtual" `react-dom.development.js` source map resource (e.g.
inside `registerTwoPhaseEvent`) it will still pause even if the
`react-dom\.development\.js$` pattern is blackboxed (the call frames in the
"virtual" `react-dom.development.js ` source map resource also do not appear as
though they are blackboxed when doing this).  Also, it's worth mentioning that
"virtual" source map resources are not allowed to be individually blackboxed
(see `get supportsScriptBlackboxing`).

> Source/WebInspectorUI/ChangeLog:10
> +	   Select first non-blackboxed call frame on pause.

Why is this necessary?	The definition of script blackboxing means that the
first call frame is not blackboxed, as otherwise we wouldn't pause in it.

> Source/WebInspectorUI/UserInterface/Models/CallFrame.js:70
> +	   return
!!WI.debuggerManager.blackboxDataForSourceCode(this._sourceCodeLocation.display
SourceCode);

please see <https://webkit.org/b/216897#c21>

> Source/WebInspectorUI/UserInterface/Views/CallFrameTreeElement.js:104
> +	      
this._callFrame.sourceCodeLocation.addEventListener(WI.SourceCodeLocation.Event
.DisplayLocationChanged, this._updateStatus, this);

This doesn't really make any sense.  Something is either blackboxed or it
isn't.	That shouldn't change depending on what source code location Web
Inspector decides to show for it.