WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
173619
Web Inspector: use JSON::{Array,Object,Value} instead of Inspector{Array,Object,Value}
https://bugs.webkit.org/show_bug.cgi?id=173619
Summary
Web Inspector: use JSON::{Array,Object,Value} instead of Inspector{Array,Obje...
Blaze Burg
Reported
2017-06-20 13:43:41 PDT
This object system is used outside of Inspector code so let's start moving it out.
Attachments
Patch
(61.87 KB, patch)
2017-06-20 13:49 PDT
,
Blaze Burg
no flags
Details
Formatted Diff
Diff
Patch v1.1
(108.82 KB, patch)
2017-06-21 10:34 PDT
,
Blaze Burg
achristensen
: review+
Details
Formatted Diff
Diff
Rebased patch + unit tests
(133.15 KB, patch)
2017-11-03 06:56 PDT
,
Carlos Garcia Campos
no flags
Details
Formatted Diff
Diff
Try to fix mac builds
(133.13 KB, patch)
2017-11-05 23:28 PST
,
Carlos Garcia Campos
bburg
: review+
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Blaze Burg
Comment 1
2017-06-20 13:49:54 PDT
Created
attachment 313434
[details]
Patch
Build Bot
Comment 2
2017-06-20 13:50:54 PDT
This patch modifies the inspector protocol generator. Please ensure that you have rebaselined any generator test results (i.e., by running `Tools/Scripts/run-inspector-generator-tests --reset-results`)
Build Bot
Comment 3
2017-06-20 13:51:01 PDT
Attachment 313434
[details]
did not pass style-queue: ERROR: Source/JavaScriptCore/inspector/scripts/codegen/cpp_generator.py:43: whitespace before '}' [pep8/E202] [5] Total errors found: 1 in 18 files If any of these errors are false positives, please file a bug against check-webkit-style.
Blaze Burg
Comment 4
2017-06-21 10:34:38 PDT
Created
attachment 313529
[details]
Patch v1.1
Build Bot
Comment 5
2017-06-21 10:36:48 PDT
Attachment 313529
[details]
did not pass style-queue: ERROR: Source/JavaScriptCore/inspector/scripts/codegen/cpp_generator.py:43: whitespace before '}' [pep8/E202] [5] Total errors found: 1 in 23 files If any of these errors are false positives, please file a bug against check-webkit-style.
Alex Christensen
Comment 6
2017-06-22 08:23:08 PDT
Comment on
attachment 313529
[details]
Patch v1.1 View in context:
https://bugs.webkit.org/attachment.cgi?id=313529&action=review
> Source/JavaScriptCore/inspector/InspectorValues.h:52 > +namespace JSON {
Is this namespace used anywhere else? How are you making JSON objects? Probably completely unrelated to this patch, but we've been meaning to re-organize the JSON parser to use callbacks like the YARR parser so we can use it for other things and remove json.hpp. Would that help this project, too?
Blaze Burg
Comment 7
2017-06-23 10:42:07 PDT
(In reply to Alex Christensen from
comment #6
)
> Comment on
attachment 313529
[details]
> Patch v1.1 > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=313529&action=review
> > > Source/JavaScriptCore/inspector/InspectorValues.h:52 > > +namespace JSON { > > Is this namespace used anywhere else?
Unknown. I have never seen it, though.
> How are you making JSON objects?
This code is ancient, and has its own JSON parser and stringifier. I believe ScriptValue is able to bridge between our C++ object representation of a JSON object and JS object representation. Usually, though, this code parses strings from the remote debugger and send back out strings with payloads / command results.
> Probably completely unrelated to this patch, but we've been meaning to > re-organize the JSON parser to use callbacks like the YARR parser so we can > use it for other things and remove json.hpp. Would that help this project, > too?
Unless there's some reason I am unaware of, this code's parser (and maybe stringifier) could be replaced if JSC's JSON parser used callbacks (so we could create C++ objects for values instead of JS objects).
Alex Christensen
Comment 8
2017-06-28 10:57:25 PDT
Comment on
attachment 313529
[details]
Patch v1.1 View in context:
https://bugs.webkit.org/attachment.cgi?id=313529&action=review
r=me
>>> Source/JavaScriptCore/inspector/InspectorValues.h:52 >>> +namespace JSON { >> >> Is this namespace used anywhere else? How are you making JSON objects? Probably completely unrelated to this patch, but we've been meaning to re-organize the JSON parser to use callbacks like the YARR parser so we can use it for other things and remove json.hpp. Would that help this project, too? > > Unknown. I have never seen it, though.
I think I'd prefer this namespace be called something like "InspectorProtocol" or "InspectorJSON" unless we want to make it purely for JSON and not have any inspector anything inside it.
Joseph Pecoraro
Comment 9
2017-06-28 16:18:31 PDT
Comment on
attachment 313529
[details]
Patch v1.1 View in context:
https://bugs.webkit.org/attachment.cgi?id=313529&action=review
>>>> Source/JavaScriptCore/inspector/InspectorValues.h:52 >>>> +namespace JSON { >>> >>> Is this namespace used anywhere else? How are you making JSON objects? Probably completely unrelated to this patch, but we've been meaning to re-organize the JSON parser to use callbacks like the YARR parser so we can use it for other things and remove json.hpp. Would that help this project, too? >> >> Unknown. I have never seen it, though. > > I think I'd prefer this namespace be called something like "InspectorProtocol" or "InspectorJSON" unless we want to make it purely for JSON and not have any inspector anything inside it.
I tend to agree. I was under the impression that InspectorValue was not totally compatible with JSON. But I did a quick scan and didn't find any issues. In any case, I wouldn't want it named JSON unless it really did handle all of JSON. Moving it to such a generic namespace I'd also expect to see unit tests. To a lesser degree this is somewhat well tested via Inspector tests right now.
Blaze Burg
Comment 10
2017-08-03 20:35:49 PDT
(In reply to Joseph Pecoraro from
comment #9
)
> Comment on
attachment 313529
[details]
> Patch v1.1 > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=313529&action=review
> > >>>> Source/JavaScriptCore/inspector/InspectorValues.h:52 > >>>> +namespace JSON { > >>> > >>> Is this namespace used anywhere else? How are you making JSON objects? Probably completely unrelated to this patch, but we've been meaning to re-organize the JSON parser to use callbacks like the YARR parser so we can use it for other things and remove json.hpp. Would that help this project, too? > >> > >> Unknown. I have never seen it, though. > > > > I think I'd prefer this namespace be called something like "InspectorProtocol" or "InspectorJSON" unless we want to make it purely for JSON and not have any inspector anything inside it. > > I tend to agree. I was under the impression that InspectorValue was not > totally compatible with JSON. But I did a quick scan and didn't find any > issues. In any case, I wouldn't want it named JSON unless it really did > handle all of JSON. Moving it to such a generic namespace I'd also expect to > see unit tests. To a lesser degree this is somewhat well tested via > Inspector tests right now.
I'm willing to pick this patch back up and write tests. However, we need to first do the renaming so that further attempts on this bug stop bit-rotting. Just since this patch, there are dozens of new uses especially in Canvas and Source/WebDriver.
Blaze Burg
Comment 11
2017-08-03 20:39:42 PDT
(In reply to Brian Burg from
comment #10
)
> (In reply to Joseph Pecoraro from
comment #9
) > > Comment on
attachment 313529
[details]
> > Patch v1.1 > > > > View in context: > >
https://bugs.webkit.org/attachment.cgi?id=313529&action=review
> > > > >>>> Source/JavaScriptCore/inspector/InspectorValues.h:52 > > >>>> +namespace JSON { > > >>> > > >>> Is this namespace used anywhere else? How are you making JSON objects? Probably completely unrelated to this patch, but we've been meaning to re-organize the JSON parser to use callbacks like the YARR parser so we can use it for other things and remove json.hpp. Would that help this project, too? > > >> > > >> Unknown. I have never seen it, though. > > > > > > I think I'd prefer this namespace be called something like "InspectorProtocol" or "InspectorJSON" unless we want to make it purely for JSON and not have any inspector anything inside it. > > > > I tend to agree. I was under the impression that InspectorValue was not > > totally compatible with JSON. But I did a quick scan and didn't find any > > issues. In any case, I wouldn't want it named JSON unless it really did > > handle all of JSON. Moving it to such a generic namespace I'd also expect to > > see unit tests. To a lesser degree this is somewhat well tested via > > Inspector tests right now. > > I'm willing to pick this patch back up and write tests. However, we need to > first do the renaming so that further attempts on this bug stop bit-rotting. > Just since this patch, there are dozens of new uses especially in Canvas and > Source/WebDriver.
In other words, let's land the renamings and then I'll work on the tests. Unit tests are a prerequisite to adopting the shared JSON parser via callbacks, to ensure we don't regress anything. Joe, did you have any particular ideas for JSON unit tests? It seems like a big area and a priori deciding what tests to write seems kind of weird. Alex, how did you go about writing tests for the new URL parser? It seems like a similarly large space to test from scratch. Maybe we can start by writing tests that deserialize various JSON strings, both valid and invalid. We can write the equivalent tests that create valid and invalid JSON::Value trees in memory and see what happens when we try to serialize.
Joseph Pecoraro
Comment 12
2017-08-04 12:17:16 PDT
> Joe, did you have any particular ideas for JSON unit tests? It seems like a > big area and a priori deciding what tests to write seems kind of weird.
You'd should update the InspectorValue TestWebKitAPI test that was recently added: Tools/TestWebKitAPI/Tests/JavaScriptCore/InspectorValue.cpp I'd expect at least basic tests for: Basic Construction of the different JSON Value types: Null, Boolean, Double, Integer, String, Object, Array, InspectorValue::toJSONString - validate the output of the above types, non-finite numbers, nested objects / arrays InspectorValue::parseString - all JSON types, empty string, numbers >2^53, non-finite numbers, NaN, nested objects / arrays, some invalid strings InspectorObject - size() - get/set/remove InspectorArray: - length() - get/push These would be trivial to write but validate our expectations in edge cases to see if this really is JSON or not. --- I see already there is one place this can produce invalid JSON, since NaN is not valid (maybe it should be "null"):
> // Not enough room for decimal. Use exponential format. > if (decimal.bufferLengthForStringExponential() > WTF::NumberToStringBufferLength) { > // Fallback for an abnormal case if it's too little even for exponential. > output.appendLiteral("NaN"); > return; > }
Carlos Garcia Campos
Comment 13
2017-11-03 06:56:14 PDT
Created
attachment 325891
[details]
Rebased patch + unit tests Rebased patch and included basic unit tests.
Build Bot
Comment 14
2017-11-03 06:58:41 PDT
Attachment 325891
[details]
did not pass style-queue: ERROR: Source/JavaScriptCore/inspector/scripts/codegen/cpp_generator.py:43: whitespace before '}' [pep8/E202] [5] Total errors found: 1 in 24 files If any of these errors are false positives, please file a bug against check-webkit-style.
Carlos Garcia Campos
Comment 15
2017-11-05 23:28:07 PST
Created
attachment 326105
[details]
Try to fix mac builds
Build Bot
Comment 16
2017-11-05 23:31:19 PST
Attachment 326105
[details]
did not pass style-queue: ERROR: Source/JavaScriptCore/inspector/scripts/codegen/cpp_generator.py:43: whitespace before '}' [pep8/E202] [5] Total errors found: 1 in 24 files If any of these errors are false positives, please file a bug against check-webkit-style.
Carlos Garcia Campos
Comment 17
2017-11-07 23:51:06 PST
Brian, could you review the unit test changes?
Blaze Burg
Comment 18
2017-11-08 00:11:35 PST
Comment on
attachment 326105
[details]
Try to fix mac builds View in context:
https://bugs.webkit.org/attachment.cgi?id=326105&action=review
r=me, very thorough tests!
> Source/JavaScriptCore/ChangeLog:6 > + Reviewed by Alex Christensen.
Please fix this so that you are the top line author, the original patch is by me, and the patch is reviewed by me and Alex.
Carlos Garcia Campos
Comment 19
2017-11-08 05:02:22 PST
Committed
r224576
: <
https://trac.webkit.org/changeset/224576
>
Radar WebKit Bug Importer
Comment 20
2017-11-15 13:13:11 PST
<
rdar://problem/35569025
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug