[webkit-reviews] review granted: [Bug 203262] Web Inspector: frontend tests should clear output before resending results : [Attachment 381637] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Yury Semikhatsky
<yurys at chromium.org>'s request for review:
Bug 203262: Web Inspector: frontend tests should clear output before resending
results
https://bugs.webkit.org/show_bug.cgi?id=203262

Attachment 381637: Patch

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




--- Comment #3 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 381637
  --> https://bugs.webkit.org/attachment.cgi?id=381637
Patch

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

r=me, with some style comments

> LayoutTests/http/tests/inspector/resources/inspector-test.js:180
> +	   return;
> +    let output = this._resultElement;

Style: missing newline

> LayoutTests/http/tests/inspector/resources/inspector-test.js:182
> +    while (output.firstChild)
> +	   output.removeChild(output.firstChild);

NIT: we normally just use `output.textContent = "";` to remove all children.

> LayoutTests/inspector/debugger/breakpoint-action-eval.html:41
> +    let reloadPromise =
InspectorTest.awaitEvent(FrontendTestHarness.Event.TestPageDidLoad);

NIT: I'd put this before the `InspectorTest.reloadPage` call, so that we're
already listening for the event before we trigger anything on the backend

> LayoutTests/inspector/debugger/breakpoint-action-eval.html:43
> +    Promise.all([reloadPromise, breakpointPromise.promise])
> +	   .then(() => InspectorTest.evaluateInPage("runBreakpointActions()"));

Style: we normally split this onto separate lines and don't indent the
`.then(...)`:
```
    Promise.all([
	reloadPromise,
	breapointPromise.promise,
    ])
    .then(() => {
	InspectorTest.evaluateInPage("runBreakpointActions()");
    });
```

> LayoutTests/inspector/debugger/breakpoint-action-log.html:53
> +	       let reloadPromise =
InspectorTest.awaitEvent(FrontendTestHarness.Event.TestPageDidLoad);
> +	       Promise.all([reloadPromise, breakpointPromise.promise])
> +		   .then(() =>
InspectorTest.evaluateInPage("runBreakpointActions()"))
> +		   .then(resolve);;

Ditto

> LayoutTests/inspector/debugger/probe-manager-add-remove-actions.html:55
> +    let reloadPromise =
InspectorTest.awaitEvent(FrontendTestHarness.Event.TestPageDidLoad);
> +    Promise.all([reloadPromise, breakpointPromise.promise])
> +	   .then(() => InspectorTest.evaluateInPage("breakpointActions(12,
{x:1,y:2})"));

Ditto

> LayoutTests/inspector/debugger/probe-manager-add-remove-actions.html:56
> +

Style: unnecessary newline