Bug 173793 - Move JSONValues to WTF and convert uses of InspectorValues.h to JSONValues.h
Summary: Move JSONValues to WTF and convert uses of InspectorValues.h to JSONValues.h
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Blaze Burg
URL:
Keywords: InRadar
Depends on: 173619 173662
Blocks:
  Show dependency tree / graph
 
Reported: 2017-06-23 14:57 PDT by Blaze Burg
Modified: 2017-11-28 11:58 PST (History)
12 users (show)

See Also:

Attachments
Patch (476.54 KB, patch)
2017-06-23 15:05 PDT, Blaze Burg 		joepeck: review-
Details | Formatted Diff | Diff
Updated patch (489.99 KB, patch)
2017-11-13 07:38 PST, Carlos Garcia Campos 		bburg: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
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 Details | Formatted Diff | Diff
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 Details | Formatted Diff | Diff
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 Details | Formatted Diff | Diff
Try a different EWS hack (622.18 KB, patch)
2017-11-17 07:02 PST, Blaze Burg 		no flags Details | Formatted Diff | Diff
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 Details | Formatted Diff | Diff
Proposed Fix v2 (633.82 KB, patch)
2017-11-19 09:58 PST, Blaze Burg 		no flags Details | Formatted Diff | Diff
Proposed Fix v2 (637.75 KB, patch)
2017-11-19 15:48 PST, Blaze Burg 		no flags Details | Formatted Diff | Diff
Patch (635.67 KB, patch)
2017-11-27 13:51 PST, Blaze Burg 		no flags Details | Formatted Diff | Diff
Fix GTK (635.73 KB, patch)
2017-11-27 14:55 PST, Blaze Burg 		no flags Details | Formatted Diff | Diff
Fix GTK more (636.10 KB, patch)
2017-11-27 15:27 PST, Blaze Burg 		no flags Details | Formatted Diff | Diff
Patch (637.46 KB, patch)
2017-11-28 09:13 PST, Blaze Burg 		no flags Details | Formatted Diff | Diff
Show Obsolete (17) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Blaze Burg 2017-06-23 14:57:10 PDT 
.
Comment 1 Blaze Burg 2017-06-23 15:05:14 PDT 
Created attachment 313752 [details]
Patch

Has dependencies, will also upload a combined patch.
Comment 2 Joseph Pecoraro 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?
Comment 3 Carlos Garcia Campos 2017-11-13 07:38:45 PST 
Created attachment 326760 [details]
Updated patch
Comment 4 Build Bot 2017-11-13 07:42:26 PST Comment hidden (obsolete)
Comment 5 Build Bot 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.
Comment 6 Build Bot 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
Comment 7 Blaze Burg 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.
Comment 8 Radar WebKit Bug Importer 2017-11-13 09:09:28 PST 
<rdar://problem/35505216>
Comment 9 Blaze Burg 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.
Comment 10 Carlos Garcia Campos 2017-11-14 00:23:10 PST 
Created attachment 326865 [details]
Try to fix win EWS
Comment 11 Build Bot 2017-11-14 00:25:50 PST Comment hidden (obsolete)
Comment 12 Build Bot 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.
Comment 13 Build Bot 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
Comment 14 Blaze Burg 2017-11-14 16:43:41 PST 
Committed r224863: <https://trac.webkit.org/changeset/224863>
Comment 15 Ryan Haddad 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
Comment 16 Ryan Haddad 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>
Comment 17 Blaze Burg 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?
Comment 18 Blaze Burg 2017-11-16 00:05:12 PST 
Created attachment 327050 [details]
For EWS
Comment 19 Build Bot 2017-11-16 00:09:09 PST Comment hidden (obsolete)
Comment 20 Build Bot 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.
Comment 21 Build Bot 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
Comment 22 Blaze Burg 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. 🙄
Comment 23 Jonathan Bedard 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.
Comment 24 Blaze Burg 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.
Comment 25 Blaze Burg 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.
Comment 26 Blaze Burg 2017-11-16 12:05:06 PST 
Created attachment 327092 [details]
Patch with hacks in Assertions.h to work around EWS
Comment 27 Build Bot 2017-11-16 12:07:56 PST Comment hidden (obsolete)
Comment 28 Blaze Burg 2017-11-17 07:02:57 PST 
Created attachment 327168 [details]
Try a different EWS hack
Comment 29 EWS Watchlist 2017-11-17 07:04:55 PST Comment hidden (obsolete)
Comment 30 EWS Watchlist 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.
Comment 31 EWS Watchlist 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
Comment 32 EWS Watchlist 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
Comment 33 EWS Watchlist 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
Comment 34 Blaze Burg 2017-11-18 11:48:42 PST 
Created attachment 327316 [details]
Another attemp
Comment 35 EWS Watchlist 2017-11-18 11:52:34 PST Comment hidden (obsolete)
Comment 36 Blaze Burg 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.
Comment 37 EWS Watchlist 2017-11-19 10:02:26 PST Comment hidden (obsolete)
Comment 38 Blaze Burg 2017-11-19 15:48:35 PST 
Created attachment 327352 [details]
Proposed Fix v2
Comment 39 Alex Christensen 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
Comment 40 Blaze Burg 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.
Comment 41 Joseph Pecoraro 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;
Comment 42 Blaze Burg 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.
Comment 43 Blaze Burg 2017-11-27 13:51:30 PST 
Created attachment 327672 [details]
Patch
Comment 44 EWS Watchlist 2017-11-27 13:55:09 PST Comment hidden (obsolete)
Comment 45 Blaze Burg 2017-11-27 14:55:58 PST 
Created attachment 327688 [details]
Fix GTK
Comment 46 Blaze Burg 2017-11-27 15:27:39 PST 
Created attachment 327697 [details]
Fix GTK more
Comment 47 EWS Watchlist 2017-11-27 15:30:20 PST Comment hidden (obsolete)
Comment 48 Joseph Pecoraro 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 }`
Comment 49 Blaze Burg 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 }`
Comment 50 Carlos Garcia Campos 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.
Comment 51 Blaze Burg 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.
Comment 52 Blaze Burg 2017-11-28 09:13:52 PST 
Created attachment 327755 [details]
Patch
Comment 53 EWS Watchlist 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.
Comment 54 Blaze Burg 2017-11-28 11:58:31 PST 
Committed r225231: <https://trac.webkit.org/changeset/225231>