Summary: | Inspector::InspectorValue class doesn't handle undefined type | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Karim <karim> | ||||||||||
Component: | Web Inspector | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | bburg, buildbot, commit-queue, joepeck, karim, keith_miller, mark.lam, mattbaker, msaboff, nvasilyev, rniwa, saam, timothy | ||||||||||
Priority: | P2 | Keywords: | DoNotImportToRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Attachments: |
|
Description
Karim
2016-12-06 18:39:48 PST
Created attachment 296360 [details]
Patch
Comment on attachment 296360 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=296360&action=review > Source/JavaScriptCore/ChangeLog:4 > + Added undefined type and fixed related issues > + https://bugs.webkit.org/show_bug.cgi?id=165506 "undefined" is not a valid JSON value type: http://www.json.org http://www.ecma-international.org/publications/files/ECMA-ST/ECMA-404.pdf >>> JSON.stringify({a:undefined}) {} >>> JSON.stringify({a:null}) {"a":null} Is there a specific reason we want to be adding it to InspectorValues? > Source/JavaScriptCore/ChangeLog:11 > + * runtime/VM.h: This looks stale, the diff doesn't show any modifications to VM.h. There is a FIXME tag at:JavaScriptCore/bindings/ScriptValue.cpp:56
// FIXME: Why is it OK to turn undefined into null?
> Comment on attachment 296360 [details]
> Patch
>
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=296360&action=review
>
> > Source/JavaScriptCore/ChangeLog:4
> > + Added undefined type and fixed related issues
> > + https://bugs.webkit.org/show_bug.cgi?id=165506
>
> "undefined" is not a valid JSON value type:
> http://www.json.org
> http://www.ecma-international.org/publications/files/ECMA-ST/ECMA-404.pdf
>
> >>> JSON.stringify({a:undefined})
> {}
>
> >>> JSON.stringify({a:null})
> {"a":null}
>
> Is there a specific reason we want to be adding it to InspectorValues?
>
> > Source/JavaScriptCore/ChangeLog:11
> > + * runtime/VM.h:
>
> This looks stale, the diff doesn't show any modifications to VM.h.
Comment on attachment 296360 [details] Patch >Index: Source/JavaScriptCore/ChangeLog >=================================================================== >--- Source/JavaScriptCore/ChangeLog (revision 209439) >+++ Source/JavaScriptCore/ChangeLog (working copy) >@@ -1,3 +1,15 @@ >+2016-12-06 Karim H <karim@karhm.com> >+ >+ Added undefined type and fixed related issues >+ https://bugs.webkit.org/show_bug.cgi?id=165506 >+ >+ Reviewed by NOBODY (OOPS!). >+ >+ * inspector/InspectorValues.cpp: >+ (Inspector::InspectorValue::undefined): >+ * inspector/InspectorValues.h: >+ > 2016-12-06 Alexey Proskuryakov <ap@apple.com> > > Correct SDKROOT values in xcconfig files >Index: Source/JavaScriptCore/inspector/InspectorValues.cpp >=================================================================== >--- Source/JavaScriptCore/inspector/InspectorValues.cpp (revision 209405) >+++ Source/JavaScriptCore/inspector/InspectorValues.cpp (working copy) >@@ -485,6 +485,11 @@ Ref<InspectorValue> InspectorValue::null > return adoptRef(*new InspectorValue); > } > >+Ref<InspectorValue> InspectorValue::undefined() >+{ >+ return adoptRef(*new InspectorValue(Type::Undefined)); >+} >+ > Ref<InspectorValue> InspectorValue::create(bool value) > { > return adoptRef(*new InspectorValue(value)); >Index: Source/JavaScriptCore/inspector/InspectorValues.h >=================================================================== >--- Source/JavaScriptCore/inspector/InspectorValues.h (revision 209405) >+++ Source/JavaScriptCore/inspector/InspectorValues.h (working copy) >@@ -64,11 +64,13 @@ public: > break; > case Type::Object: > case Type::Array: >+ case Type::Undefined: > break; > } > } > > static Ref<InspectorValue> null(); >+ static Ref<InspectorValue> undefined(); > static Ref<InspectorValue> create(bool); > static Ref<InspectorValue> create(int); > static Ref<InspectorValue> create(double); >@@ -83,10 +85,12 @@ public: > String, > Object, > Array, >+ Undefined, > }; > > Type type() const { return m_type; } > bool isNull() const { return m_type == Type::Null; } >+ bool isUndefined() const { return m_type == Type::Undefined; } > > bool asBoolean(bool&) const; > bool asInteger(int&) const; Sounds like we have a reason, we should remove the FIXME comment. Please just remove the FIXME. There is nothing to fix. Created attachment 296567 [details]
Patch
Comment on attachment 296567 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=296567&action=review > Source/JavaScriptCore/ChangeLog:9 > + * bindings/ScriptValue.cpp: > + (Inspector::jsToInspectorValue): We should include a description here about why. Something like: It is okay to turn undefined into null because we are producing values for a JSON representation (InspectorValue) and JSON has a `null` value and no `undefined` value. Created attachment 296574 [details]
Patch
Comment on attachment 296574 [details] Patch Attachment 296574 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2656533 New failing tests: http/tests/xmlhttprequest/access-control-repeated-failed-preflight-crash.html Created attachment 296583 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 296574 [details] Patch Clearing flags on attachment: 296574 Committed r209650: <http://trac.webkit.org/changeset/209650> All reviewed patches have been landed. Closing bug. |