[webkit-reviews] review granted: [Bug 200652] Web Inspector: Debugger: add a global breakpoint for pausing in the next microtask : [Attachment 376667] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Devin Rousso
<drousso at apple.com>'s request for review:
Bug 200652: Web Inspector: Debugger: add a global breakpoint for pausing in the
next microtask
https://bugs.webkit.org/show_bug.cgi?id=200652

Attachment 376667: Patch

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




--- Comment #4 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 376667
  --> https://bugs.webkit.org/attachment.cgi?id=376667
Patch

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

Nice! Good tests. r=me

Seems like perhaps the only remaining script entry points (that I know of) are:

  (1) API Entry points
    - Ex. JSEvaluteScript / -[JSContext evaluteScript:]
  (2) Normal Script entry points for a Program / Module
    - Ex. <script src="..." async>
    - Ex. eval('...')

  (3) Observer Script handlers
    - Ex. PerformanceObserver, IntersectionObserver, handlers
  (4) APIs with callbacks that aren't events or Observers
    - Ex. geolocation callbacks


(1) and (2) seem useful for users to break on. Maybe a Breakpoint on script
entry "All Script Entries" / "All Script Evaluations" would be good.

(3) and (4) cases seem like Events / specific functions akin to setTimeout.
Seems overkill to call them out specifically though.

Anything I missed worth considering?

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:152
> +	   // COMPATIBILITY (iOS 12): DebuggerAgent.setPauseOnMicrotasks did
not exist yet.

Nit: I think we should use (iOS 13) in all of these compatibility comments, as
it did not exist in iOS 13.0.

> Source/WebInspectorUI/UserInterface/Images/Microtask.svg:3
> +<svg xmlns="http://www.w3.org/2000/svg" id="root" version="1.1" viewBox="0 0
16 16">

Sometimes we have `id="root"` and other times we don't. Not sure why... I
normally remove it if I don't see a need for it.