InspectorValue class doesn't handle undefined type witch may cause issues with other classes that handle undefined type.
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.