[webkit-reviews] review denied: [Bug 195248] Web Inspector: DOMDebugger: protocol error on first open : [Attachment 363433] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has denied Devin Rousso
<drousso at apple.com>'s request for review:
Bug 195248: Web Inspector: DOMDebugger: protocol error on first open
https://bugs.webkit.org/show_bug.cgi?id=195248

Attachment 363433: Patch

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




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

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

r- for some multi-target logic. I didn't go in depth after finding a few
issues.

The Multimap code looks good, you may want to separate that out into its own
patch.

> Source/WebInspectorUI/UserInterface/Base/Multimap.js:65
> +	   let deleted = valueSet.delete(value);
> +
> +	   // Allow an entire key to be removed by not passing a value.
> +	   if (arguments.length === 1 || !valueSet.size)
> +	       deleted = this._map.delete(key);

I'd go with:

    // Allow an entire key to be removed by not passing a value.
    if (arguments.length === 1)
	return this._map.delete(key);

    let deleted = valueSet.delete(value);
    if (deleted && !valueSet.size)
	this._map.delete(key);
    return deleted;

Which:

    1. Avoids a `set.delete(undefined)` when arguments.length is 1
    2. Avoids a `map.delete(key)` when valueSet shouldn't be empty
    3. Avoids a duplicate unnecessary assignment to `deleted` when valueSet is
empty

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:180
> +	       for (let target of WI.targets)
> +		   this._updateDOMBreakpoint(breakpoint, target);

This is another where maybe we use WI.assumingMainTarget().

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:211
> +	       for (let target of WI.targets)
> +		   target.DOMDebuggerAgent.removeDOMBreakpoint(nodeIdentifier,
breakpoint.type);

It is not safe to assume the target has a DOMDebuggerAgent (a ServiceWorker
target will not). Typically the first thing we *should* do inside of WI.target
loops is check for the relevant agent the loop will make use of. Currently many
call sites don't because they deal with agents that are guaranteed to exist in
all targets (RuntimeAgent, DebuggerAgent, and ConsoleAgent). But like the loop
you just added for CanvasAgent and another for HeapAgent checking is the right
thing to do.

The pattern I'd suggest is:

    for (let target of WI.targets) {
	if (target.DOMDebuggerAgent) {
	    ...
	}
    }

Note that some of your other code gets around this by burying the agent check
inside functions, which is fine but tricky to review.

We could add a utility such as:

    for (let target of WI.targetsWith("DOMDebugger")) {
	...
    }

Which might reduce complexity at sites like this. Then this entire file could
follow that pattern.

---

In any case, this specific code (DOM Breakpoints / Node breakpoint) will likely
be incorrect for the coming future anyways. A DOMNode / nodeIdentifier will
only be valid in a single Target (eventually a Frame target). We probably don't
want to tell all targets about a nodeIdentifier that doesn't correspond to
them. Ideally in this case the breakpoint would know the target the
nodeIdentifier is associated with, so you'd end up with:

    breakpoint.target.removeDOMBreakpoint(nodeIdentifier, breakpoint.type);

So for now maybe convert this one to just:

    // We should get the target associated with the nodeIdentifier in this
breakpoint.
    let target = WI.assumingMainTarget();
    target.DOMDebuggerAgent.removeDOMBreakpoint(nodeIdentifier,
breakpoint.type);

Put simply:
• Some DOMDebugger breakpoint types (Event, XHR, URL) are universal, and make
sense to broadcast messages to all targets with a DOMDebuggerAgent.
• Some DOMDebugger breakpoint types (Node, EventListener) are context specific,
and don't make sense to broadcast, but instead should be associated with a
single target.

Let me know if that all made sense or if you view this differently.

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:285
> +	   for (let target of WI.targets) {

Check target.DOMDebuggerAgent.

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:355
> +	   for (let target of WI.targets) {

Check target.DOMDebuggerAgent.

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:449
> -	   this._updateDOMBreakpoint(breakpoint);
> +	   this._updateDOMBreakpoint(breakpoint, WI.assumingMainTarget());

Lets add a similar comment that ideally we would find the target associated
with this target specific DOM breakpoint.

I like the idea of adding comments next to assumingMainTarget so when we come
across it later we know why!

> LayoutTests/inspector/unit-tests/multimap.html:86
> +    suite.addTestCase({
> +	   name: "Multimap.prototype.delete",
> +	   test() {
> +	       let multimap = new Multimap;
> +
> +	       multimap.add(0, 1);
> +	       multimap.add(2, 3);
> +	       multimap.add(2, 3);
> +
> +	       InspectorTest.log(multimap);
> +
> +	       InspectorTest.expectThat(multimap.delete(0, 1), "The key and the
value were successfully deleted.");
> +
> +	       InspectorTest.log(multimap);
> +
> +	       InspectorTest.expectThat(multimap.delete(2, 3), "The key and the
value were successfully deleted.");
> +
> +	       InspectorTest.log(multimap);
> +	   },
> +    });

I'd like to see this test deleting a value from a set that has multiple values.

    multimap.add(0, 1);
    multimap.add(2, 3);
    multimap.add(2, 4);

    log();
    delete(0, 1); log(); // should see `0` keys go away entirely
    delete(2, 3); log(); // should see `[2,4]` still
    delete(2, 4); log(); // should see empty

> LayoutTests/inspector/unit-tests/multimap.html:155
> +	       multimap.add("badger", "raccoon");

Would be nice to give this more values.