Bug 188717

Created attachment 347426 [details]
Patch

Created attachment 347427 [details]
[Image] After Patch is applied

Comment on attachment 347426 [details]
Patch

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

> Source/JavaScriptCore/inspector/protocol/DOM.json:631
> +            "name": "eventNamesParsed",

I don't understand this name. The backend is not parsing anything. This is really something like:

    supportedEventNames

But see my other comments about why a getSupportedEventNames might be better.

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:540
> +    m_frontendDispatcher->eventNamesParsed(WTFMove(eventNames));

How large is this message normally? I'm wondering if we will want to make this a 1 time request, like:

    getSupportedCSSProperties
    getSupportedSystemFontFamilyNames

My idea for this is if we expand to multiple Target debugging we wouldn't want to get this large list from each of the backends we connect to if we know they will be the same.

> Source/WebInspectorUI/UserInterface/Controllers/DOMTreeManager.js:39
> +        this._eventNames = new Set();

Style: Drop the ()s.

> Source/WebInspectorUI/UserInterface/Controllers/DOMTreeManager.js:50
> +        if (DOMAgent.enable)

You will want to also check `if (window.DOMAgent && ...)` since this manager is always created in the frontend, but wouldn't exist for JSContext inspectors.

> Source/WebInspectorUI/UserInterface/Views/EventBreakpointPopover.js:84
> +            let matchRegex = new RegExp("^" + this._inputElement.value, "i");

It might be cheaper to `toLowerCase()` and `startsWith(...)` in the loop then constructing a case insensitive regex.

> Source/WebInspectorUI/UserInterface/Views/EventBreakpointPopover.js:87
> +            for (let eventName of WI.domTreeManager.eventNames) {

Likewise we could make the eventNames stored by the DOMTreeManager be all lowercase. This is a Set right?

Comment on attachment 347426 [details]
Patch

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

>> Source/JavaScriptCore/inspector/protocol/DOM.json:631
>> +            "name": "eventNamesParsed",
> 
> I don't understand this name. The backend is not parsing anything. This is really something like:
> 
>     supportedEventNames
> 
> But see my other comments about why a getSupportedEventNames might be better.

I wanted to add some sort of verb.  Just having "eventNames" seemed like a bad name for an event.

>> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:540
>> +    m_frontendDispatcher->eventNamesParsed(WTFMove(eventNames));
> 
> How large is this message normally? I'm wondering if we will want to make this a 1 time request, like:
> 
>     getSupportedCSSProperties
>     getSupportedSystemFontFamilyNames
> 
> My idea for this is if we expand to multiple Target debugging we wouldn't want to get this large list from each of the backends we connect to if we know they will be the same.

This is a good point.  I'm seeing 283 different event names, and over 4,000 characters for the whole message.   I think I'll switch it to getSupportedEventNames.

Created attachment 347430 [details]
Patch

<rdar://problem/43573235>

Comment on attachment 347430 [details]
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.

Comment on attachment 347430 [details]
Patch

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

>> 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.

Yeah, this was mainly for performance reasons.  I didn't notice any stuttering, so it might not be needed.

Personally, I am a fan of these hybridized approaches (see `WI.DOMTreeManager.requestDocument` for another example) as they only provide the async functionality in the rare case.  The main reason I don't like caching on the UI object is because if we needed this value later in another place, we'd have two copies of the same cached value.  To use the example I already gave, imagine if every caller of `WI.domTreeManager.requestDocument` had to save the document value on their own.  It would be much harder to keep everything in sync.

Created attachment 348174 [details]
Patch

Comment on attachment 348174 [details]
Patch

Clearing flags on attachment: 348174

Committed r235389: <https://trac.webkit.org/changeset/235389>

All reviewed patches have been landed.  Closing bug.