[webkit-reviews] review granted: [Bug 200651] Web Inspector: DOMDebugger: support event breakpoints in Worker contexts : [Attachment 376735] 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 200651: Web Inspector: DOMDebugger: support event breakpoints in Worker
contexts
https://bugs.webkit.org/show_bug.cgi?id=200651

Attachment 376735: Patch

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




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

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

r=me. Nice tests! Probably needs a rebaseline.

> Source/WebCore/inspector/agents/page/PageDOMDebuggerAgent.cpp:55
> +    if (typeString == "subtree-modified")
> +	   return SubtreeModified;
> +    if (typeString == "attribute-modified")
> +	   return AttributeModified;
> +    if (typeString == "node-removed")
> +	   return NodeRemoved;

Weird that we can do this comparison and the right hand doesn't need `_s`
suffix. I wonder which is more performant?

> Source/WebCore/inspector/agents/page/PageDOMDebuggerAgent.cpp:65
> +    case SubtreeModified: return "subtree-modified"_s;
> +    case AttributeModified: return "attribute-modified"_s;
> +    case NodeRemoved: return "node-removed"_s;

Lets fix this style while moving the code.

> LayoutTests/inspector/worker/dom-debugger-dom-breakpoints.html:60
> +    let listener =
WI.targetManager.addEventListener(WI.TargetManager.Event.TargetAdded, (event)
=> {
> +	   let {target} = event.data;
> +	   if (target.type !== WI.Target.Type.Worker)
> +	       return;
> +
> +	   workerTarget = target;
> +	  
WI.targetManager.removeEventListener(WI.TargetManager.Event.TargetAdded,
listener);
> +
> +	   suite.runTestCasesAndFinish();
> +    });
> +
> +    InspectorTest.evaluateInPage(`createWorker()`);

I feel like this is a pattern we may end up doing more and more often with
worker tests. (I just noticed you do it in each of these tests!)

We could turn this into a helper that could even send all the code to the
frontend:

    createWorkerTargetForTests("resources/worker-dom-debugger.js", () => {
	suite.runTestCasesAndFinish();
    });

Or a Promise if desired:

    createWorkerTargetForTests("resources/worker-dom-debugger.js").then(() => {
	suite.runTestCasesAndFinish();
    });

Implementation could be (untested):

    TestPage.registerInitializer(() => {
	window.createWorkerTargetForTests = function(workerScript, callback) {
	    InspectorTest.evaluateInPage(`window[Symbol()] = new
Worker(${JSON.stringify(workerScript)});`);
	    let listener =
WI.targetManager.addEventListener(WI.TargetManager.Event.TargetAdded, (event)
=> {
		let {target} = event.data;
		if (target.type !== WI.Target.Type.Worker)
		    return;

		window.workerTarget = target;
	       
WI.targetManager.removeEventListener(WI.TargetManager.Event.TargetAdded,
listener);
		callback();
	    });
	}
    });

Which eliminates the `createWorker` code, `workerTarget` declaration and pretty
much all boilerplate in the tests.