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.
Created attachment 259008 [details] WIP (Mac)
<rdar://problem/22290196>
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.
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
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
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
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
Created attachment 259035 [details] Patch
Created attachment 259038 [details] Try again (EFL)
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
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
Created attachment 259043 [details] Try again (Win, Mac)
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.
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.
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?
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).
Committed r188539: <http://trac.webkit.org/changeset/188539>
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
This also broke some build styles; I sent an e-mail. Rolling out.
Re-opened since this is blocked by bug 148122
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.
Committed r188598: <http://trac.webkit.org/changeset/188598>
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?