Bug 147954

Summary: Web Inspector: refactor ProtocolTest to be an InjectedTestHarness subclass
Product: WebKit Reporter: BJ Burg <bburg>
Component: Web InspectorAssignee: BJ Burg <bburg>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, graouts, joepeck, mattbaker, nvasilyev, timothy, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 147948    
Bug Blocks: 147934    
Attachments:
Description Flags
Proposed Fix joepeck: review+

Description BJ Burg 2015-08-12 14:26:51 PDT
I'm going to move the actual test to a common directory in a separate patch. That requires some changes to WebCore's Internals.idl.
Comment 1 BJ Burg 2015-08-12 14:41:23 PDT
Created attachment 258847 [details]
Proposed Fix
Comment 2 Joseph Pecoraro 2015-08-13 11:32:14 PDT
Comment on attachment 258847 [details]
Proposed Fix

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

r=me

> LayoutTests/http/tests/inspector/resources/ProtocolTestStub.js:29
>  {

Is the window needed here?

> LayoutTests/http/tests/inspector/resources/ProtocolTestStub.js:74
> +        var script = "try { " + xhr.responseText + "} catch (e) { alert(" + JSON.stringify("Error in: " + scriptName) + "); throw e; }";

I wonder if template strings would make this more readable.

> LayoutTests/http/tests/inspector/resources/ProtocolTestStub.js:108
> +}

Style: semicolon

> LayoutTests/http/tests/inspector/resources/ProtocolTestStub.js:111
> +    constructor(harness, name) {

Style: Brace on new line?

> LayoutTests/http/tests/inspector/resources/ProtocolTestStub.js:209
> +    constructor(harness, name) {

Style: Brace on newline?

> LayoutTests/inspector/unit-tests/async-test-suite-expected.txt:1
> +PASS: instantiating SyncTestSuite requires name argument.

async-test-suite outputs SyncTextSuite?

> LayoutTests/inspector/unit-tests/async-test-suite.html:10
> +        let result = new InjectedTestHarness.AsyncTestSuite(this);
> +        ProtocolTest.log("FAIL: instantiating SyncTestSuite requires name argument.");

I think these "SyncTestSuite" changes to this file were accidental.
Comment 3 BJ Burg 2015-08-13 12:48:14 PDT
Comment on attachment 258847 [details]
Proposed Fix

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

>> LayoutTests/inspector/unit-tests/async-test-suite.html:10
>> +        ProtocolTest.log("FAIL: instantiating SyncTestSuite requires name argument.");
> 
> I think these "SyncTestSuite" changes to this file were accidental.

oh, weird. Definitely not intended.
Comment 4 BJ Burg 2015-08-13 14:53:32 PDT
Committed r188406: <http://trac.webkit.org/changeset/188406>
Comment 5 Radar WebKit Bug Importer 2015-08-13 14:53:44 PDT
<rdar://problem/22276793>