Bug 173793

Summary: Move JSONValues to WTF and convert uses of InspectorValues.h to JSONValues.h
Product: WebKit Reporter: Blaze Burg <bburg>
Component: Web InspectorAssignee: Blaze Burg <bburg>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, ap, bburg, buildbot, cgarcia, ews-watchlist, inspector-bugzilla-changes, jbedard, joepeck, rniwa, ryanhaddad, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on: 173619, 173662    
Bug Blocks:    
Attachments:
Description Flags
Patch
joepeck: review-
Updated patch
bburg: review+, buildbot: commit-queue-
Archive of layout-test-results from ews121 for ios-simulator-wk2
none
Try to fix win EWS
none
Archive of layout-test-results from ews121 for ios-simulator-wk2
none
For EWS
none
Archive of layout-test-results from ews125 for ios-simulator-wk2
none
Patch with hacks in Assertions.h to work around EWS
none
Try a different EWS hack
none
Archive of layout-test-results from ews123 for ios-simulator-wk2
none
Archive of layout-test-results from ews106 for mac-elcapitan-wk2
none
Another attemp
none
Proposed Fix v2
none
Proposed Fix v2
none
Patch
none
Fix GTK
none
Fix GTK more
none
Patch none

Blaze Burg
Reported 2017-06-23 14:57:10 PDT
.
Attachments
Patch (476.54 KB, patch)
2017-06-23 15:05 PDT, Blaze Burg 		joepeck: review-
DetailsFormatted DiffDiff
Updated patch (489.99 KB, patch)
2017-11-13 07:38 PST, Carlos Garcia Campos 		bburg: review+
buildbot: commit-queue-
DetailsFormatted DiffDiff
Archive of layout-test-results from ews121 for ios-simulator-wk2 (1.33 MB, application/zip)
2017-11-13 08:50 PST, Build Bot 		no flags
Details
Try to fix win EWS (490.86 KB, patch)
2017-11-14 00:23 PST, Carlos Garcia Campos 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from ews121 for ios-simulator-wk2 (1.31 MB, application/zip)
2017-11-14 01:35 PST, Build Bot 		no flags
Details
For EWS (619.33 KB, patch)
2017-11-16 00:05 PST, Blaze Burg 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from ews125 for ios-simulator-wk2 (1.33 MB, application/zip)
2017-11-16 01:16 PST, Build Bot 		no flags
Details
Patch with hacks in Assertions.h to work around EWS (621.98 KB, patch)
2017-11-16 12:05 PST, Blaze Burg 		no flags
DetailsFormatted DiffDiff
Try a different EWS hack (622.18 KB, patch)
2017-11-17 07:02 PST, Blaze Burg 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from ews123 for ios-simulator-wk2 (4.59 MB, application/zip)
2017-11-17 08:31 PST, EWS Watchlist 		no flags
Details
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (3.08 MB, application/zip)
2017-11-17 11:23 PST, EWS Watchlist 		no flags
Details
Another attemp (633.20 KB, patch)
2017-11-18 11:48 PST, Blaze Burg 		no flags
DetailsFormatted DiffDiff
Proposed Fix v2 (633.82 KB, patch)
2017-11-19 09:58 PST, Blaze Burg 		no flags
DetailsFormatted DiffDiff
Proposed Fix v2 (637.75 KB, patch)
2017-11-19 15:48 PST, Blaze Burg 		no flags
DetailsFormatted DiffDiff
Patch (635.67 KB, patch)
2017-11-27 13:51 PST, Blaze Burg 		no flags
DetailsFormatted DiffDiff
Fix GTK (635.73 KB, patch)
2017-11-27 14:55 PST, Blaze Burg 		no flags
DetailsFormatted DiffDiff
Fix GTK more (636.10 KB, patch)
2017-11-27 15:27 PST, Blaze Burg 		no flags
DetailsFormatted DiffDiff
Patch (637.46 KB, patch)
2017-11-28 09:13 PST, Blaze Burg 		no flags
DetailsFormatted DiffDiff
Show Obsolete (17) View All Add attachment proposed patch, testcase, etc.
Blaze Burg
Comment 1 2017-06-23 15:05:14 PDT
Created attachment 313752 [details] Patch Has dependencies, will also upload a combined patch.
Joseph Pecoraro
Comment 2 2017-06-28 16:25:35 PDT
Comment on attachment 313752 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313752&action=review > Source/JavaScriptCore/ChangeLog:9 > + These changes are mostly mechanical. Later patches will streamline > + variable names and other surrounding conventions which are now clunky. And it looks like this one is intended to be the JSON::ArrayOf<> patch?
Carlos Garcia Campos
Comment 3 2017-11-13 07:38:45 PST
Created attachment 326760 [details] Updated patch
Build Bot
Comment 4 2017-11-13 07:42:26 PST Comment hidden (obsolete)
Build Bot
Comment 5 2017-11-13 08:50:19 PST
Comment on attachment 326760 [details] Updated patch Attachment 326760 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/5211309 Number of test failures exceeded the failure limit.
Build Bot
Comment 6 2017-11-13 08:50:20 PST
Created attachment 326762 [details] Archive of layout-test-results from ews121 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
Blaze Burg
Comment 7 2017-11-13 08:51:02 PST
Comment on attachment 326760 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=326760&action=review r=me This probably requires changes in Safari / WebInspector.framework code that has a dependency on this class. I'll land this patch and an internal change at the same time to avoid problems. Thanks Carlos for picking this back up! > Source/WTF/wtf/JSONValues.cppSource/JavaScriptCore/inspector/InspectorValues.cpp:47 > + ObjectBegin, Nice cleanup. This code is really old, I think we may switch over to use a shared event based JSON parser in the future.
Radar WebKit Bug Importer
Comment 8 2017-11-13 09:09:28 PST
<rdar://problem/35505216>
Blaze Burg
Comment 9 2017-11-13 09:09:40 PST
iOS failures are pre-existing, but Windows looks like a problem with the API test for JSON::Value.
Carlos Garcia Campos
Comment 10 2017-11-14 00:23:10 PST
Created attachment 326865 [details] Try to fix win EWS
Build Bot
Comment 11 2017-11-14 00:25:50 PST Comment hidden (obsolete)
Build Bot
Comment 12 2017-11-14 01:35:43 PST
Comment on attachment 326865 [details] Try to fix win EWS Attachment 326865 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/5225915 Number of test failures exceeded the failure limit.
Build Bot
Comment 13 2017-11-14 01:35:44 PST
Created attachment 326869 [details] Archive of layout-test-results from ews121 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
Blaze Burg
Comment 14 2017-11-14 16:43:41 PST
Committed r224863: <https://trac.webkit.org/changeset/224863>
Ryan Haddad
Comment 15 2017-11-15 09:06:18 PST
This change caused crashes on iOS Simulator: ASSERTION FAILED: dlopen(/System/Library/Frameworks/QuickLook.framework/QuickLook, 2): Symbol not found: __ZN9Inspector14InspectorArray6createEv Referenced from: /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/Library/CoreSimulator/Profiles/Runtimes/iOS.simruntime/Contents/Resources/RuntimeRoot/System/Library/PrivateFrameworks/WebInspector.framework/WebInspector Expected in: /Volumes/Data/slave/ios-simulator-11-debug-tests-wk2/build/WebKitBuild/Debug-iphonesimulator/JavaScriptCore.framework/JavaScriptCore in /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/Library/CoreSimulator/Profiles/Runtimes/iOS.simruntime/Contents/Resources/RuntimeRoot/System/Library/PrivateFrameworks/WebInspector.framework/WebInspector frameworkLibrary /Volumes/Data/slave/ios-simulator-11-debug/build/Source/WebCore/platform/ios/QuickLookSoftLink.mm(32) : void *WebCore::QuickLookLibrary(bool)_block_invoke 1 0x1106372fd WTFCrash 2 0x1131d5198 invocation function for block in WebCore::QuickLookLibrary(bool) 3 0x108a802b5 _dispatch_client_callout 4 0x108a81749 dispatch_once_f 5 0x1131d50e5 WebCore::QuickLookLibrary(bool) 6 0x1131e0af9 invocation function for block in WebCore::initQuickLookQLPreviewGetSupportedMIMETypes() 7 0x108a802b5 _dispatch_client_callout 8 0x108a81749 dispatch_once_f 9 0x1131d51e9 WebCore::initQuickLookQLPreviewGetSupportedMIMETypes() 10 0x1131acfad WebCore::softLink_QuickLook_QLPreviewGetSupportedMIMETypes() 11 0x1131acf76 WebCore::QLPreviewGetSupportedMIMETypesSet() 12 0x112cd9cfd WebCore::PreviewLoader::shouldCreateForMIMEType(WTF::String const&)::$_1::operator()() const 13 0x112cd9cab void std::__1::__call_once_proxy<std::__1::tuple<WebCore::PreviewLoader::shouldCreateForMIMEType(WTF::String const&)::$_1&&> >(void*) 14 0x10724ea4e std::__1::__call_once(unsigned long volatile&, void*, void (*)(void*)) 15 0x112cd4a68 WebCore::PreviewLoader::shouldCreateForMIMEType(WTF::String const&) 16 0x114a8f63f WebCore::SubresourceLoader::shouldCreatePreviewLoaderForResponse(WebCore::ResourceResponse const&) const 17 0x114a8f72f WebCore::SubresourceLoader::didReceiveResponse(WebCore::ResourceResponse const&) 18 0x103da1529 WebKit::WebResourceLoader::didReceiveResponse(WebCore::ResourceResponse const&, bool) 19 0x103da4af8 void IPC::callMemberFunctionImpl<WebKit::WebResourceLoader, void (WebKit::WebResourceLoader::*)(WebCore::ResourceResponse const&, bool), std::__1::tuple<WebCore::ResourceResponse, bool>, 0ul, 1ul>(WebKit::WebResourceLoader*, void (WebKit::WebResourceLoader::*)(WebCore::ResourceResponse const&, bool), std::__1::tuple<WebCore::ResourceResponse, bool>&&, std::__1::integer_sequence<unsigned long, 0ul, 1ul>) 20 0x103da48c0 void IPC::callMemberFunction<WebKit::WebResourceLoader, void (WebKit::WebResourceLoader::*)(WebCore::ResourceResponse const&, bool), std::__1::tuple<WebCore::ResourceResponse, bool>, std::__1::integer_sequence<unsigned long, 0ul, 1ul> >(std::__1::tuple<WebCore::ResourceResponse, bool>&&, WebKit::WebResourceLoader*, void (WebKit::WebResourceLoader::*)(WebCore::ResourceResponse const&, bool)) 21 0x103da3ed1 void IPC::handleMessage<Messages::WebResourceLoader::DidReceiveResponse, WebKit::WebResourceLoader, void (WebKit::WebResourceLoader::*)(WebCore::ResourceResponse const&, bool)>(IPC::Decoder&, WebKit::WebResourceLoader*, void (WebKit::WebResourceLoader::*)(WebCore::ResourceResponse const&, bool)) 22 0x103da3790 WebKit::WebResourceLoader::didReceiveWebResourceLoaderMessage(IPC::Connection&, IPC::Decoder&) 23 0x1035917d9 WebKit::NetworkProcessConnection::didReceiveMessage(IPC::Connection&, IPC::Decoder&) 24 0x103351803 IPC::Connection::dispatchMessage(IPC::Decoder&) 25 0x103347458 IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >) 26 0x103351e0a IPC::Connection::dispatchOneMessage() 27 0x103369a4d IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_14::operator()() 28 0x1033699a9 WTF::Function<void ()>::CallableWrapper<IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_14>::call() 29 0x1106704bb WTF::Function<void ()>::operator()() const 30 0x110692323 WTF::RunLoop::performWork() 31 0x110692ba4 WTF::RunLoop::performWork(void*) LEAK: 1 WebPageProxy https://build.webkit.org/results/Apple%20iOS%2011%20Simulator%20Debug%20WK2%20(Tests)/r224865%20(1050)/results.html
Ryan Haddad
Comment 16 2017-11-15 09:13:34 PST
Reverted r224863 for reason: Introduced LayoutTest crashes on iOS Simulator. Committed r224879: <https://trac.webkit.org/changeset/224879>
Blaze Burg
Comment 17 2017-11-15 11:38:09 PST
Likely fix is to just keep around the old InspectorValues header and implementation but name it "InspectorValuesForBinaryCompatibility.h" or something so nobody accidentally includes it. I don't think there is any valid case where we would build an old WebInspector.framework against a new WebKit and thus expect the old header to be present. So we'll just maintain symbols in JavaScriptCore.framework until iOS-sim builders update to a new iOS SDK that includes the new symbols. I'm not 100% sure when it would be safe to remove the symbols entirely. I suppose iOS-sim EWS would explode again if it wasn't safe yet. Do we expect to build open source iOS Simulator against older iOS SDKs?
Blaze Burg
Comment 18 2017-11-16 00:05:12 PST
Created attachment 327050 [details] For EWS
Build Bot
Comment 19 2017-11-16 00:09:09 PST Comment hidden (obsolete)
Build Bot
Comment 20 2017-11-16 01:16:35 PST
Comment on attachment 327050 [details] For EWS Attachment 327050 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/5255947 Number of test failures exceeded the failure limit.
Build Bot
Comment 21 2017-11-16 01:16:37 PST
Created attachment 327051 [details] Archive of layout-test-results from ews125 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
Blaze Burg
Comment 22 2017-11-16 08:15:58 PST
New patch includes the required symbols in DeprecatedInspectorValues.h/cpp and I verified this with `nm`. However the bot is still failing for unknown reasons. I would need to spin up my own non-internal checkout to try and spin my own debug ios-sim build. 🙄
Jonathan Bedard
Comment 23 2017-11-16 08:28:07 PST
(In reply to Brian Burg from comment #22) > New patch includes the required symbols in DeprecatedInspectorValues.h/cpp > and I verified this with `nm`. However the bot is still failing for unknown > reasons. I would need to spin up my own non-internal checkout to try and > spin my own debug ios-sim build. 🙄 That archive of results should have crash logs in it, they may be helpful.
Blaze Burg
Comment 24 2017-11-16 11:18:54 PST
(In reply to Jonathan Bedard from comment #23) > (In reply to Brian Burg from comment #22) > > New patch includes the required symbols in DeprecatedInspectorValues.h/cpp > > and I verified this with `nm`. However the bot is still failing for unknown > > reasons. I would need to spin up my own non-internal checkout to try and > > spin my own debug ios-sim build. 🙄 > > That archive of results should have crash logs in it, they may be helpful. Yes, the crash logs are there, and no, they are not useful, or I would have never landed this in the first place. The soft linking code does not report the error message from dlopen() in release builds. That error message is the only way to know–sans debugger–why the dlopen call failed.
Blaze Burg
Comment 25 2017-11-16 11:38:40 PST
(In reply to Brian Burg from comment #24) > (In reply to Jonathan Bedard from comment #23) > > (In reply to Brian Burg from comment #22) > > > New patch includes the required symbols in DeprecatedInspectorValues.h/cpp > > > and I verified this with `nm`. However the bot is still failing for unknown > > > reasons. I would need to spin up my own non-internal checkout to try and > > > spin my own debug ios-sim build. 🙄 > > > > That archive of results should have crash logs in it, they may be helpful. > > Yes, the crash logs are there, and no, they are not useful, or I would have > never landed this in the first place. The soft linking code does not report > the error message from dlopen() in release builds. That error message is the > only way to know–sans debugger–why the dlopen call failed. I misspoke slightly. The crash logs have never contained the assertion message, at least for WKTR. The backtrace with missing symbol is in the stderr of the link Ryan posted. So perhaps if EWS saved the stderr like the CI builders do, then we'd be able to see what's going on.
Blaze Burg
Comment 26 2017-11-16 12:05:06 PST
Created attachment 327092 [details] Patch with hacks in Assertions.h to work around EWS
Build Bot
Comment 27 2017-11-16 12:07:56 PST Comment hidden (obsolete)
Blaze Burg
Comment 28 2017-11-17 07:02:57 PST
Created attachment 327168 [details] Try a different EWS hack
EWS Watchlist
Comment 29 2017-11-17 07:04:55 PST Comment hidden (obsolete)
EWS Watchlist
Comment 30 2017-11-17 08:31:03 PST
Comment on attachment 327168 [details] Try a different EWS hack Attachment 327168 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/5274640 Number of test failures exceeded the failure limit.
EWS Watchlist
Comment 31 2017-11-17 08:31:05 PST
Created attachment 327174 [details] Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
EWS Watchlist
Comment 32 2017-11-17 11:23:14 PST
Comment on attachment 327168 [details] Try a different EWS hack Attachment 327168 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/5277142 New failing tests: http/tests/appcache/resource-redirect-2.html
EWS Watchlist
Comment 33 2017-11-17 11:23:16 PST
Created attachment 327200 [details] Archive of layout-test-results from ews106 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Blaze Burg
Comment 34 2017-11-18 11:48:42 PST
Created attachment 327316 [details] Another attemp
EWS Watchlist
Comment 35 2017-11-18 11:52:34 PST Comment hidden (obsolete)
Blaze Burg
Comment 36 2017-11-19 09:58:33 PST
Created attachment 327345 [details] Proposed Fix v2 This approach seems workable, though the migration path towards reasonable symbols is quite extended. I think it may make more sense in the refactoring sense to pull the JSON RPC server parts into a new class, and only keep around the old InspectorBackendDispatcher class for legacy linking purposes. Please read the JSC changelog for full details of the workarounds needed to make this work for iOS public SDK builds.
EWS Watchlist
Comment 37 2017-11-19 10:02:26 PST Comment hidden (obsolete)
Blaze Burg
Comment 38 2017-11-19 15:48:35 PST
Created attachment 327352 [details] Proposed Fix v2
Alex Christensen
Comment 39 2017-11-27 09:27:39 PST
Comment on attachment 327352 [details] Proposed Fix v2 View in context: https://bugs.webkit.org/attachment.cgi?id=327352&action=review > Source/WTF/wtf/JSONValues.h:39 > +namespace JSON { Are we starting to put things in WTF that aren't in namespace WTF? Could this be "namespace WTF { namespace JSON {" ? > Source/WTF/wtf/JSONValues.h:46 > +class WTF_EXPORT_PRIVATE Value : public RefCounted<Value> { This is all duplicated logic. Please add // FIXME: Merge this with JSONParse
Blaze Burg
Comment 40 2017-11-27 10:45:36 PST
Comment on attachment 327352 [details] Proposed Fix v2 View in context: https://bugs.webkit.org/attachment.cgi?id=327352&action=review >> Source/WTF/wtf/JSONValues.h:39 >> +namespace JSON { > > Are we starting to put things in WTF that aren't in namespace WTF? Could this be "namespace WTF { namespace JSON {" ? Hi Alex. I don't want to have to use 'using WTF;' or 'WTF::JSON::Value' throughout the entire codebase. If we can export the necessary symbols in the header, as we do in WTFString.h, then I am fine with the suggestion. >> Source/WTF/wtf/JSONValues.h:46 >> +class WTF_EXPORT_PRIVATE Value : public RefCounted<Value> { > > This is all duplicated logic. Please add // FIXME: Merge this with JSONParse Yes. I'm not sure if a bug already existed for that. It would need to be event-based, but JSONParse currently assumes a JSC object model. JSON::Value objects are not associated with a particular JSContext and are used in places such as WebDriver server where JS does not execute.
Joseph Pecoraro
Comment 41 2017-11-27 10:51:29 PST
(In reply to Brian Burg from comment #40) > Comment on attachment 327352 [details] > Proposed Fix v2 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=327352&action=review > > >> Source/WTF/wtf/JSONValues.h:39 > >> +namespace JSON { > > > > Are we starting to put things in WTF that aren't in namespace WTF? Could this be "namespace WTF { namespace JSON {" ? > > Hi Alex. I don't want to have to use 'using WTF;' or 'WTF::JSON::Value' > throughout the entire codebase. If we can export the necessary symbols in > the header, as we do in WTFString.h, then I am fine with the suggestion. I think a `using` statement is just normally inserted at the bottom of the WTF header to avoid this. For example, with a mundane class like WTF::Dequeue: > namespace WTF { > > class Deque { > ... > }; > > } // namespace WTF > > using WTF::Deque;
Blaze Burg
Comment 42 2017-11-27 11:52:59 PST
(In reply to Joseph Pecoraro from comment #41) > (In reply to Brian Burg from comment #40) > > Comment on attachment 327352 [details] > > Proposed Fix v2 > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=327352&action=review > > > > >> Source/WTF/wtf/JSONValues.h:39 > > >> +namespace JSON { > > > > > > Are we starting to put things in WTF that aren't in namespace WTF? Could this be "namespace WTF { namespace JSON {" ? > > > > Hi Alex. I don't want to have to use 'using WTF;' or 'WTF::JSON::Value' > > throughout the entire codebase. If we can export the necessary symbols in > > the header, as we do in WTFString.h, then I am fine with the suggestion. > > I think a `using` statement is just normally inserted at the bottom of the > WTF header to avoid this. > > For example, with a mundane class like WTF::Dequeue: > > > namespace WTF { > > > > class Deque { > > ... > > }; > > > > } // namespace WTF > > > > using WTF::Deque; That only works for classes and some other definitions. We'd not want `using namespace WTF` in any header. I'm trying out a different approach where the definitions are inside namespace WTF::JSONObjects (ensuring reasonable symbol names), but the namespace is re-exported via top-level namespace JSON introduced at the bottom of the file.
Blaze Burg
Comment 43 2017-11-27 13:51:30 PST
Created attachment 327672 [details] Patch
EWS Watchlist
Comment 44 2017-11-27 13:55:09 PST Comment hidden (obsolete)
Blaze Burg
Comment 45 2017-11-27 14:55:58 PST
Created attachment 327688 [details] Fix GTK
Blaze Burg
Comment 46 2017-11-27 15:27:39 PST
Created attachment 327697 [details] Fix GTK more
EWS Watchlist
Comment 47 2017-11-27 15:30:20 PST Comment hidden (obsolete)
Joseph Pecoraro
Comment 48 2017-11-27 17:07:53 PST
Comment on attachment 327697 [details] Fix GTK more View in context: https://bugs.webkit.org/attachment.cgi?id=327697&action=review r=me, Looks good to me. For such a generic class and namespace we should probably have TestWTF tests for this. I think the only tests we really have are for memoryCost. But there are all kinds of things these classes do / need to do that should be tested (property order, valid parsing, invalid parsing, valid stringification, invalid stringification). I think I've commented at length about that elsewhere though. > Source/JavaScriptCore/Sources.txt:530 > inspector/InspectorAgentRegistry.cpp I think DeprecatedInspectorValues should be in here somewhere and not in the Sources section of the Xcode project. (Uncheck it from the JavaScriptCore target, and just include it here in Sources). Unless we really want it to be Mac only?! > Source/WTF/wtf/JSONValues.cpp:102 > + // According to RFC4627, a valid number is: [minus] int [frac] [exp] Style: Fix spacing. > Source/WTF/wtf/JSONValues.cpp:118 > + // Optional fraction part Style: Fix style so this ends in a period. > Source/WTF/wtf/JSONValues.cpp:131 > + // Optional exponent part Style: Fix style so this ends in a period. > Source/WTF/wtf/JSONValues.cpp:515 > +bool Value::asValue(RefPtr<Value> & value) Style: "RefPtr<Value> &" => "RefPtr<Value>&" > Source/WTF/wtf/JSONValues.h:122 > + Value() > + : m_type(Type::Null) { } > + > + explicit Value(Type type) > + : m_type(type) { } Style: We could probably convert all of these to use the newer brace instead of parenthesis syntax: `m_type { Type::Null }`
Blaze Burg
Comment 49 2017-11-27 17:31:24 PST
(In reply to Joseph Pecoraro from comment #48) > Comment on attachment 327697 [details] > Fix GTK more > > View in context: > https://bugs.webkit.org/attachment.cgi?id=327697&action=review > > r=me, Looks good to me. > > For such a generic class and namespace we should probably have TestWTF tests > for this. I think the only tests we really have are for memoryCost. But > there are all kinds of things these classes do / need to do that should be > tested (property order, valid parsing, invalid parsing, valid > stringification, invalid stringification). I think I've commented at length > about that elsewhere though. Carlos added API tests for these things you mentioned in a prior review. It covers the entire API surface. > > > Source/JavaScriptCore/Sources.txt:530 > > inspector/InspectorAgentRegistry.cpp > > I think DeprecatedInspectorValues should be in here somewhere and not in the > Sources section of the Xcode project. (Uncheck it from the JavaScriptCore > target, and just include it here in Sources). Unless we really want it to be > Mac only?! > > > Source/WTF/wtf/JSONValues.cpp:102 > > + // According to RFC4627, a valid number is: [minus] int [frac] [exp] > > Style: Fix spacing. > > > Source/WTF/wtf/JSONValues.cpp:118 > > + // Optional fraction part > > Style: Fix style so this ends in a period. > > > Source/WTF/wtf/JSONValues.cpp:131 > > + // Optional exponent part > > Style: Fix style so this ends in a period. > > > Source/WTF/wtf/JSONValues.cpp:515 > > +bool Value::asValue(RefPtr<Value> & value) > > Style: "RefPtr<Value> &" => "RefPtr<Value>&" > > > Source/WTF/wtf/JSONValues.h:122 > > + Value() > > + : m_type(Type::Null) { } > > + > > + explicit Value(Type type) > > + : m_type(type) { } > > Style: We could probably convert all of these to use the newer brace instead > of parenthesis syntax: `m_type { Type::Null }`
Carlos Garcia Campos
Comment 50 2017-11-27 23:47:30 PST
(In reply to Brian Burg from comment #42) > (In reply to Joseph Pecoraro from comment #41) > > (In reply to Brian Burg from comment #40) > > > Comment on attachment 327352 [details] > > > Proposed Fix v2 > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=327352&action=review > > > > > > >> Source/WTF/wtf/JSONValues.h:39 > > > >> +namespace JSON { > > > > > > > > Are we starting to put things in WTF that aren't in namespace WTF? Could this be "namespace WTF { namespace JSON {" ? > > > > > > Hi Alex. I don't want to have to use 'using WTF;' or 'WTF::JSON::Value' > > > throughout the entire codebase. If we can export the necessary symbols in > > > the header, as we do in WTFString.h, then I am fine with the suggestion. > > > > I think a `using` statement is just normally inserted at the bottom of the > > WTF header to avoid this. > > > > For example, with a mundane class like WTF::Dequeue: > > > > > namespace WTF { > > > > > > class Deque { > > > ... > > > }; > > > > > > } // namespace WTF > > > > > > using WTF::Deque; > > That only works for classes and some other definitions. We'd not want `using > namespace WTF` in any header. I'm trying out a different approach where the > definitions are inside namespace WTF::JSONObjects (ensuring reasonable > symbol names), but the namespace is re-exported via top-level namespace JSON > introduced at the bottom of the file. Right, I tried different ways and none of them worked for me, that's why I decided to not use WTF namespace in the end. Note that we have other cases in WTF like Gigacage that is not using WTF namespace.
Blaze Burg
Comment 51 2017-11-28 08:47:57 PST
(In reply to Joseph Pecoraro from comment #48) > Comment on attachment 327697 [details] > Fix GTK more > > View in context: > https://bugs.webkit.org/attachment.cgi?id=327697&action=review > > I think DeprecatedInspectorValues should be in here somewhere and not in the > Sources section of the Xcode project. (Uncheck it from the JavaScriptCore > target, and just include it here in Sources). Unless we really want it to be > Mac only?! It really is for Cocoa only, but it should go into SourcesCocoa.txt.
Blaze Burg
Comment 52 2017-11-28 09:13:52 PST
Created attachment 327755 [details] Patch
EWS Watchlist
Comment 53 2017-11-28 09:16:12 PST
Attachment 327755 [details] did not pass style-queue: ERROR: Source/WebDriver/SessionHost.cpp:42: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:82: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:99: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:153: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:63: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:64: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:65: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:66: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:67: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:68: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:69: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:70: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:71: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:72: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:73: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:74: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:75: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:76: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:77: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:78: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:79: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:80: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:81: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:82: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:83: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:84: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:85: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:86: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:87: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:88: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:89: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:90: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:91: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:92: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:93: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:94: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:95: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:96: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:97: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:98: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:99: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:100: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:101: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:102: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:103: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:104: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:105: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:106: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:107: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:108: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:111: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:119: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WTF/wtf/JSONValues.h:431: Do not use 'using namespace WTF::JSONImpl;'. [build/using_namespace] [4] ERROR: Source/WebDriver/SessionHost.h:64: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:540: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:816: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:1663: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/inspector/cocoa/DeprecatedInspectorValues.cpp:47: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/JavaScriptCore/inspector/cocoa/DeprecatedInspectorValues.cpp:48: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/JavaScriptCore/inspector/cocoa/DeprecatedInspectorValues.cpp:49: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/JavaScriptCore/inspector/cocoa/DeprecatedInspectorValues.cpp:50: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/JavaScriptCore/inspector/cocoa/DeprecatedInspectorValues.cpp:51: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/JavaScriptCore/inspector/cocoa/DeprecatedInspectorValues.cpp:52: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/JavaScriptCore/inspector/cocoa/DeprecatedInspectorValues.cpp:53: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/JavaScriptCore/inspector/cocoa/DeprecatedInspectorValues.cpp:54: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/JavaScriptCore/inspector/cocoa/DeprecatedInspectorValues.cpp:55: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/JavaScriptCore/inspector/cocoa/DeprecatedInspectorValues.cpp:56: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/JavaScriptCore/inspector/cocoa/DeprecatedInspectorValues.cpp:57: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/JavaScriptCore/inspector/cocoa/DeprecatedInspectorValues.cpp:58: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/JavaScriptCore/inspector/cocoa/DeprecatedInspectorValues.cpp:100: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/JavaScriptCore/inspector/cocoa/DeprecatedInspectorValues.cpp:389: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/JavaScriptCore/inspector/cocoa/DeprecatedInspectorValues.cpp:427: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/inspector/cocoa/DeprecatedInspectorValues.cpp:430: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/JavaScriptCore/inspector/cocoa/DeprecatedInspectorValues.cpp:473: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/JavaScriptCore/inspector/cocoa/DeprecatedInspectorValues.cpp:827: Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(). [security/assertion] [5] ERROR: Source/WebDriver/WebDriverService.cpp:349: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:492: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:578: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:663: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:686: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:703: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:726: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:743: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:760: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:777: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:794: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:811: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:819: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:842: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:891: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:915: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:932: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:940: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:963: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:980: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:990: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:1003: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:1024: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:1045: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:1064: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:1083: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:1100: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:1115: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:1136: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:1151: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:1166: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:1181: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:1196: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:1211: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:1226: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:1241: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:1281: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:1294: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:1307: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:1329: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:1351: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:1368: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:1435: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:1464: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:1487: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:1504: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:1521: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:1538: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:1555: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:1578: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:1595: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 126 in 106 files If any of these errors are false positives, please file a bug against check-webkit-style.
Blaze Burg
Comment 54 2017-11-28 11:58:31 PST
Committed r225231: <https://trac.webkit.org/changeset/225231>
Note You need to log in before you can comment on or make changes to this bug.