Bug 147955 – Web Inspector: load ProtocolTestStub from the WebInspectorUI bundle

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Blaze Burg]

This will probably be a method similar to InspectorFrontendClient::localizedStringsURL. It cannot be done directly in Internals.cpp, because we need to use ObjC to get the bundle path on the Mac port.

[Blaze Burg]

Created attachment 259008 [details]
WIP (Mac)

[Radar WebKit Bug Importer]

<rdar://problem/22290196>

[WebKit Commit Bot]

Attachment 259008 [details] did not pass style-queue:


ERROR: Tools/DumpRenderTree/mac/TestRunnerMac.mm:45:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Tools/WebKitTestRunner/InjectedBundle/mac/TestRunnerMac.mm:29:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 259008 [details]
WIP (Mac)

Attachment 259008 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/58867

New failing tests:
http/tests/inspector/console/access-inspected-object.html
http/tests/inspector/page/loading-iframe-document-node.html

[Build Bot]

Created attachment 259014 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 259008 [details]
WIP (Mac)

Attachment 259008 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/58873

New failing tests:
http/tests/inspector/console/access-inspected-object.html
http/tests/inspector/page/loading-iframe-document-node.html

[Build Bot]

Created attachment 259016 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Blaze Burg]

Created attachment 259035 [details]
Patch

[Blaze Burg]

Created attachment 259038 [details]
Try again (EFL)

[Build Bot]

Comment on attachment 259038 [details]
Try again (EFL)

Attachment 259038 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/59295

New failing tests:
inspector/unit-tests/async-test-suite.html
inspector/layers/layers-anonymous.html
inspector/dom/getAccessibilityPropertiesForNode.html
inspector/layers/layers-reflected-content.html
inspector/dom/dom-search-with-context.html
inspector/console/console-message.html
inspector/dom/request-child-nodes-depth.html
inspector/console/js-source-locations.html
inspector/layers/layers-for-node.html
inspector/unit-tests/sync-test-suite.html
inspector/layers/layers-generated-content.html
inspector/layers/layers-blending-compositing-reasons.html
inspector/page/frameStartedLoading.html
inspector/layers/layers-compositing-reasons.html
inspector/console/css-source-locations.html
inspector/runtime/getProperties.html
inspector/css/getSupportedCSSProperties.html
inspector/dom/getAccessibilityPropertiesForNode_mouseEventNodeId.html
inspector/dom/getAccessibilityPropertiesForNode_liveRegion.html
inspector/dom/dom-search.html
inspector/dom-debugger/node-removed.html
inspector/page/frameScheduledNavigation.html
inspector/dom/remove-multiple-nodes.html
inspector/page/archive.html
inspector/console/x-frame-options-message.html

[Build Bot]

Created attachment 259040 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Blaze Burg]

Created attachment 259043 [details]
Try again (Win, Mac)

[Devin Rousso]

Comment on attachment 259043 [details]
Try again (Win, Mac)

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

> Source/WebInspectorUI/UserInterface/Base/TestStub.js:104
> +        if (!name || typeof name !== "string")

This would also allow the name " ".  Do you want this, or should you add "!name.trim().length" to the list of possible arguments for failure?

> Source/WebInspectorUI/UserInterface/Base/TestStub.js:172
> +                if (i > 0 && priorLogCount + 1 < suite._harness.logCount)

This is a super NIT, but suite._harness looks really weird (line 191 too)  and, until I read it twice and understood that suite was this, it looked like a layering violation.  Not really necessary, but it may be worth it to make a getter for _harness so that this reads better.

> Source/WebInspectorUI/UserInterface/Base/TestStub.js:197
> +InjectedTestHarness.SyncTestSuite = class SyncTestSuite

Since most of the methods of this class are repeated verbatim from InjectedTestHarness.AsyncTestSuite, would it be worth it to create a superclass (called something like InjectedTestHarness.TestSuite) that defines the methods that do not change (passCount, skipCount, addTestCase, etc.)  so that it makes it easier to edit both the sync and async if the need arises?

> Source/WebInspectorUI/UserInterface/Base/TestStub.js:310
> +        var stringifiedMessage = typeof message !== "string" ? JSON.stringify(message) : message;

Since this is repeated code, you could make a private function (something like _createLogForMessage) that takes in a logType (which would be "log" or "debugLog") and performs the rest of the actions in one place.

> Source/WebInspectorUI/UserInterface/Base/TestStub.js:351
> +    var messageObject = {method, params, "id": this._requestId};

NIT: Quotes around "id" are not necessary  (Same on line 362)

> LayoutTests/http/tests/inspector/dom/resources/InspectorDOMListener.js:30
> +ProtocolTestProxy.registerInitializer(function(){

NIT: wouldn't this mean that everything within this function is now indented one more time, or is there a convention I am unaware of?  Also, you should have a space between function() and {

> LayoutTests/http/tests/inspector/resources/console-test.js:1
> +ProtocolTestProxy.registerInitializer(function() {

See comment on line 30 of LayoutTests/http/tests/inspector/dom/resources/InspectorDOMListener.js

> LayoutTests/http/tests/inspector/resources/probe-test.js:1
> +ProtocolTestProxy.registerInitializer(function() {

See comment on line 30 of LayoutTests/http/tests/inspector/dom/resources/InspectorDOMListener.js

> LayoutTests/http/tests/inspector/resources/protocol-test.js:95
> +    if (!(typeof testFunction === "function")) {

Wouldn't  typeof testFunction !== "function"  work?

> LayoutTests/inspector/dom/resources/dom-search-queries.js:1
> +ProtocolTestProxy.registerInitializer(function() {

See comment on line 30 of LayoutTests/http/tests/inspector/dom/resources/InspectorDOMListener.js


One other thing I noticed is that for the tests which you modified, you replaced some instances of var with let.  There were some cases in the base or resource files that made sense to keep as var, but would you be interested in replacing most of them with let (which I think is what we are going to do going forward) since you pretty much rewrote the file?  Just a thought.

[Blaze Burg]

Comment on attachment 259043 [details]
Try again (Win, Mac)

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

>> Source/WebInspectorUI/UserInterface/Base/TestStub.js:172
>> +                if (i > 0 && priorLogCount + 1 < suite._harness.logCount)
> 
> This is a super NIT, but suite._harness looks really weird (line 191 too)  and, until I read it twice and understood that suite was this, it looked like a layering violation.  Not really necessary, but it may be worth it to make a getter for _harness so that this reads better.

I'll hold out for lexical `this`; in the current state, it's just too awkward to get it bound correctly.

>> Source/WebInspectorUI/UserInterface/Base/TestStub.js:310
>> +        var stringifiedMessage = typeof message !== "string" ? JSON.stringify(message) : message;
> 
> Since this is repeated code, you could make a private function (something like _createLogForMessage) that takes in a logType (which would be "log" or "debugLog") and performs the rest of the actions in one place.

It's only repeated twice, and it's a single line. I think a helper would reduce readability. This is a moved file that is shown as add+remove, so it would be a new patch anyway.

>> Source/WebInspectorUI/UserInterface/Base/TestStub.js:351
>> +    var messageObject = {method, params, "id": this._requestId};
> 
> NIT: Quotes around "id" are not necessary  (Same on line 362)

It actually used to be unquoted throughout this code, but we made it quoted some time ago to emphasize we are building JSONs. I'm ambivalent.

>> LayoutTests/http/tests/inspector/dom/resources/InspectorDOMListener.js:30
>> +ProtocolTestProxy.registerInitializer(function(){
> 
> NIT: wouldn't this mean that everything within this function is now indented one more time, or is there a convention I am unaware of?  Also, you should have a space between function() and {

Yes, but in this case the indentation doesn't tell you much, so I broke the rule.

There should be a space.

>> LayoutTests/http/tests/inspector/resources/protocol-test.js:95
>> +    if (!(typeof testFunction === "function")) {
> 
> Wouldn't  typeof testFunction !== "function"  work?

Ooops!

>> LayoutTests/inspector/dom/resources/dom-search-queries.js:1
>> +ProtocolTestProxy.registerInitializer(function() {
> 
> See comment on line 30 of LayoutTests/http/tests/inspector/dom/resources/InspectorDOMListener.js
> 
> 
> One other thing I noticed is that for the tests which you modified, you replaced some instances of var with let.  There were some cases in the base or resource files that made sense to keep as var, but would you be interested in replacing most of them with let (which I think is what we are going to do going forward) since you pretty much rewrote the file?  Just a thought.

In the interest of making progress, I am mostly deferring style cleanup until I make a pass over the individual tests.

[Timothy Hatcher]

Comment on attachment 259043 [details]
Try again (Win, Mac)

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

>>> Source/WebInspectorUI/UserInterface/Base/TestStub.js:172
>>> +                if (i > 0 && priorLogCount + 1 < suite._harness.logCount)
>> 
>> This is a super NIT, but suite._harness looks really weird (line 191 too)  and, until I read it twice and understood that suite was this, it looked like a layering violation.  Not really necessary, but it may be worth it to make a getter for _harness so that this reads better.
> 
> I'll hold out for lexical `this`; in the current state, it's just too awkward to get it bound correctly.

One option is to use `self` for the var name. That would be more obvious it is analogous to `this`.

>>> Source/WebInspectorUI/UserInterface/Base/TestStub.js:351
>>> +    var messageObject = {method, params, "id": this._requestId};
>> 
>> NIT: Quotes around "id" are not necessary  (Same on line 362)
> 
> It actually used to be unquoted throughout this code, but we made it quoted some time ago to emphasize we are building JSONs. I'm ambivalent.

We can drop the quotes. Especially now that we are using the short object syntax.

> Tools/DumpRenderTree/mac/TestRunnerMac.mm:800
> +    CFBundleRef inspectorBundle = CFBundleGetBundleWithIdentifier(CFSTR("com.apple.WebInspectorUI"));
> +    if (!inspectorBundle)
> +        return nullptr;
> +
> +    RetainPtr<CFURLRef> url = adoptCF(CFBundleCopyResourceURL(inspectorBundle, CFSTR("TestStub"), CFSTR("html"), NULL));
> +    if (!url)
> +        return nullptr;
> +
> +    CFStringRef urlString = CFURLGetString(url.get());
> +    return JSStringCreateWithCFString(urlString);

Why not do the ObjC version of this?

[Blaze Burg]

Comment on attachment 259043 [details]
Try again (Win, Mac)

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

>>>> Source/WebInspectorUI/UserInterface/Base/TestStub.js:172
>>>> +                if (i > 0 && priorLogCount + 1 < suite._harness.logCount)
>>> 
>>> This is a super NIT, but suite._harness looks really weird (line 191 too)  and, until I read it twice and understood that suite was this, it looked like a layering violation.  Not really necessary, but it may be worth it to make a getter for _harness so that this reads better.
>> 
>> I'll hold out for lexical `this`; in the current state, it's just too awkward to get it bound correctly.
> 
> One option is to use `self` for the var name. That would be more obvious it is analogous to `this`.

Ok.

>> Source/WebInspectorUI/UserInterface/Base/TestStub.js:197
>> +InjectedTestHarness.SyncTestSuite = class SyncTestSuite
> 
> Since most of the methods of this class are repeated verbatim from InjectedTestHarness.AsyncTestSuite, would it be worth it to create a superclass (called something like InjectedTestHarness.TestSuite) that defines the methods that do not change (passCount, skipCount, addTestCase, etc.)  so that it makes it easier to edit both the sync and async if the need arises?

Started in www.webkit.org/b/148077.

>>>> Source/WebInspectorUI/UserInterface/Base/TestStub.js:351
>>>> +    var messageObject = {method, params, "id": this._requestId};
>>> 
>>> NIT: Quotes around "id" are not necessary  (Same on line 362)
>> 
>> It actually used to be unquoted throughout this code, but we made it quoted some time ago to emphasize we are building JSONs. I'm ambivalent.
> 
> We can drop the quotes. Especially now that we are using the short object syntax.

Ok.

>> Tools/DumpRenderTree/mac/TestRunnerMac.mm:800
>> +    return JSStringCreateWithCFString(urlString);
> 
> Why not do the ObjC version of this?

The CF version can be copy-pasted for the windows implementation (www.webkit.org/b/148037, www.webit.org/b/148025) with slight adjustments to the bundle name. And it's easier to get a JSStringRef from a CFString (though I suppose that can be easily done from NSString).

[Blaze Burg]

Committed r188539: <http://trac.webkit.org/changeset/188539>

[Alexey Proskuryakov]

This broke a number of tests on Windows:

inspector/debugger/searchInContent-linebreaks.html
inspector/debugger/setBreakpoint-actions.html
inspector/dom/focus.html
inspector/dom/remove-multiple-nodes.html
inspector/layers/layers-reflected-content.html

[Alexey Proskuryakov]

This also broke some build styles; I sent an e-mail.

Rolling out.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 148122

[Blaze Burg]

Created attachment 259300 [details]
Proposed Fix (re-land)

This patch includes a change to FRAMEWORK_SEARCH_PATH for WebKitTestRunner, which will hopefully fix build failures for some configurations.

[Blaze Burg]

Committed r188598: <http://trac.webkit.org/changeset/188598>

[Joseph Pecoraro]

Comment on attachment 259300 [details]
Proposed Fix (re-land)

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

> Source/WebInspectorUI/UserInterface/Base/TestStub.js:100
> +    constructor(harness, name) {

Style: Brace on its own line.

> Source/WebInspectorUI/UserInterface/Base/TestStub.js:168
> +        var priorLogCount = this._harness.logCount;

Should we switch to `let` everywhere in new code?

> Source/WebInspectorUI/UserInterface/TestStub.html:27
> +    <script type="text/javascript" src="Base/TestStub.js"></script>

Nit: Unnecessary "type".

> Tools/WebKitTestRunner/Configurations/Base.xcconfig:59
> +FRAMEWORK_SEARCH_PATHS = $(inherited) $(SYSTEM_LIBRARY_DIR)/PrivateFrameworks $(SYSTEM_LIBRARY_DIR)/Frameworks/WebKit.framework/Versions/A/Frameworks;

This is actually defined above as WEBKIT_UMBRELLA_FRAMEWORKS_DIR in a slightly different form (SDKROOT)(NEXT_ROOT). Maybe there is a relevant difference?