[webkit-reviews] review granted: [Bug 188717] Web Inspector: provide autocompletion for event breakpoints : [Attachment 347430] Patch

[bugzilla-daemon at webkit.org]

Brian Burg <bburg at apple.com> has granted Devin Rousso <drousso at apple.com>'s
request for review:
Bug 188717: Web Inspector: provide autocompletion for event breakpoints
https://bugs.webkit.org/show_bug.cgi?id=188717

Attachment 347430: Patch

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




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

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

r=me with some bikeshedding

> Source/WebInspectorUI/UserInterface/Controllers/DOMTreeManager.js:543
> +    getSupportedEventNames(callback)

The current approach of bridging a promise to callback doesn't seem to buy us
anything vs. returning this._getSupportedEventNamesPromise.

I would prefer to return the promise, which may be cached from a prior call.
The popover / UI class can cache the values it has fetched from the model by
unwrapping the promise.

Did you notice any stutter when the completions were initially fetched during a
keydown event? I think that's the only performance gotcha here when using both
sync and async callback.

> Source/WebInspectorUI/UserInterface/Views/EventBreakpointPopover.js:91
> +		   if (this._currentCompletions.length) {

Nit: reverse the conditions to make the zero completions case use an early
return.