Bug 178927 - WebDriver process should not link to JavaScriptCore
Summary: WebDriver process should not link to JavaScriptCore
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebDriver (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-10-27 01:25 PDT by Carlos Garcia Campos
Modified: 2017-12-01 10:04 PST (History)
1 user (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2017-10-27 01:25:55 PDT
We only do that to use InspectorValues because we don't have any other json API available in WTF. I think we should move InspectorValues implementation to WTF, or add a new implementation instead (or even use an existing one like https://github.com/nlohmann/json that looks pretty good). In the meantime, maybe we can just build the current InspectorValues as part of the WebDriver process since InspectorValues is self-contained and only depends on WTF.
Comment 1 BJ Burg 2017-10-27 09:03:10 PDT
I had some patch series to move InspectorValue to WTF::JSONValue. Unfortunately I didn't have time to write tests so it never got reviewed. If you want to work on this, I can point you to the patches.
Comment 2 Carlos Garcia Campos 2017-10-27 23:21:19 PDT
Sure, I can just write the tests.
Comment 3 BJ Burg 2017-10-29 10:18:06 PDT
Hey Carlos, here's some more information to help you out. The previous patch series moved stuff into the top-level JSON namespace, so classes were JSON::{Array, Object, Value}. If this goes into WTF then we can keep that, or make these WTF::JSON{Array, Object, Value}. Let me know what you think; if it is a general-purpose JSON class with tests than I think either name is acceptable.

The first patch:

https://bugs.webkit.org/show_bug.cgi?id=173619

Joe wrote out some reasonable test cases that you can cover for now:

https://bugs.webkit.org/show_bug.cgi?id=173619#c12

Once that patch lands (to change all callsites to use the non-Inspector namespace) the rest is moving the implementation and changing header includes:

https://bugs.webkit.org/show_bug.cgi?id=173793

When all the code is moved into the new place then there are some great cleanups I'd like to pursue. The first is already posted here, but don't worry about fixing it up:

https://bugs.webkit.org/show_bug.cgi?id=173662
Comment 4 Carlos Garcia Campos 2017-12-01 01:09:14 PST
This was finally fixed in r225231
Comment 5 BJ Burg 2017-12-01 10:04:16 PST
Hooray!