Closed
Bug 797806
Opened 9 years ago
Closed 9 years ago
Add a helper service or functions to deal with stringified JSON in C++
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla19
People
(Reporter: smaug, Assigned: smaug)
References
Details
Attachments
(2 files, 2 obsolete files)
4.28 KB,
patch
|
khuey
:
review+
bajaj
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
1.79 KB,
patch
|
khuey
:
review+
bajaj
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
Right now C++ needs to first parse the string and then use JS API to get values out from it. Usually expected JSON objects are simple dictionaries, so I hope we could re-use the existing dictionary handling code here. Investigating.
Assignee | ||
Comment 1•9 years ago
|
||
But before doing this would be good to know how often json is used in C++. I've seen it few times in b2g related code, I think.
Assignee | ||
Comment 2•9 years ago
|
||
Like this perhaps. So one would need to add an idl dictionary to some .idl file (see for example EventInit in nsIDOMEvent.idl) and then add the dictionary name and file name to dictionary_helper_gen.conf (js/xpconnect/src). Then in C++ one could just say. #include "DictionaryHelpers.h" ... mozilla::dom::YourDictionary d; nsresult rv = d.Init(yourStringContainingTheSerializedJSON); NS_ENSURE_SUCCESS(rv, rv); now d has all the properties set. For example with EventInit. mozilla::dom::EventInit d; nsresult rv = d.Init(NS_LITERAL_STRING("{\"bubbles\":true,\"cancelable\":true}")); NS_ENSURE_SUCCESS(rv, rv); Now d.bubbles == true; d.cancelable == true; willyaranda, want to try this?
Yes, I will try this, and ping you in case that I don't know how to do it.
Assignee | ||
Comment 4•9 years ago
|
||
Ok, using webidl then. This is for mainthread only for now. JSAutoRequest usage is a bit ugly, but I wanted to keep the Init inline so that hopefully the code isn't usually generated, but the ParseJSON in the .cpp so that ContentUtils can be used. Create .webidl file, put it to dom/webidl and add the filename to WebIDL.mk (#ifdef if needed). Then add dictionary to the file (ExampleDict.webidl), for example dictionary ExampleDict { boolean val1 = false; // default value is false DOMString val2 = ""; // default value is "" }; Then in C++ #include "mozilla/dom/ExampleDict.h" mozilla::dom::ExampleDict d; d.Init(NS_LITERAL_STRING("{ \"val1\": true, \"val2\": \"foobar\"}")); printf("val1=%i, val2=%s\n", d.val1, NS_ConvertUTF16toUTF8(d.val2).get());
Attachment #668141 -
Flags: review?(khuey)
Olli, I'm trying this and I cannot get it working either. I will ping you on IRC later today if you are connected.
Comment on attachment 668141 [details] [diff] [review] patch Review of attachment 668141 [details] [diff] [review]: ----------------------------------------------------------------- I would like to see some code that uses this before r+ing it. I don't really like this API (particularly the returning the JSContext* part). ::: dom/bindings/BindingUtils.cpp @@ +634,5 @@ > +MainThreadDictionaryBase::ParseJSON(const nsAString& aJSON, JS::Value& aVal) > +{ > + JSContext* cx = nsContentUtils::ThreadJSContextStack()->GetSafeJSContext(); > + NS_ENSURE_TRUE(cx, nullptr); > + JSAutoRequest ar(cx); Do we really not need to enter a compartment here? @@ +637,5 @@ > + NS_ENSURE_TRUE(cx, nullptr); > + JSAutoRequest ar(cx); > + NS_ENSURE_TRUE(JS_ParseJSON(cx, > + static_cast<const jschar*>(PromiseFlatString(aJSON).get()), > + aJSON.Length(), &aVal), nullptr); Yuck. if (!JS_ParseJSON ...) { return nullptr; }
Assignee | ||
Comment 7•9 years ago
|
||
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #6) > > Do we really not need to enter a compartment here? Hmm, I guess entering wouldn't cause any harm. > if (!JS_ParseJSON ...) { > return nullptr; > } Why. The warning NS_ENSURE_TRUE would give would be useful
Then what compartment does the object get created in? then do bool b = JS_ParseJSON .. NS_ENSURE_TRUE(b, nullptr); putting the function call inside the NS_ENSURE_TRUE is really ugly.
Assignee | ||
Comment 9•9 years ago
|
||
Attachment #668141 -
Attachment is obsolete: true
Attachment #668141 -
Flags: review?(khuey)
Attachment #669332 -
Flags: review?(khuey)
Comment on attachment 669332 [details] [diff] [review] v2 Review of attachment 669332 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bindings/BindingUtils.cpp @@ +658,5 @@ > + JS::Value& aVal) > +{ > + JSContext* cx = nsContentUtils::ThreadJSContextStack()->GetSafeJSContext(); > + NS_ENSURE_TRUE(cx, nullptr); > + JSObject* global = JS_GetGlobalObject(cx); This is not actually going to do anything. JS_GetGlobalObject(cx) is going to return the global of the compartment that the context is already in, so entering the compartment is effectively a no-op. So you still haven't fixed the problem that the last patch had.
Attachment #669332 -
Flags: review?(khuey) → review-
Assignee | ||
Comment 11•9 years ago
|
||
Comment on attachment 669332 [details] [diff] [review] v2 I'm not aware of other ways to get a sane JSObject here so that entering to a compartment works. If that is not what you mean, then it is not clear to me what you want.
Attachment #669332 -
Flags: review- → review?(khuey)
Assignee | ||
Comment 12•9 years ago
|
||
Or perhaps I need to manually enter to a compartment. JSAPI isn't very friendly.
Assignee | ||
Comment 13•9 years ago
|
||
No, can't find any better way to enter to the default compartment of a JSContext.
You need the global object of the compartment that is going to use the objects you're creating. This is why it would really help to see some code that actually uses this ...
Assignee | ||
Comment 15•9 years ago
|
||
Eh, what? The whole point of the patch is that C++ caller must not need to deal with JSAPI at all. JSObject won't be used after JSON parsing + initializing the dictionary. All you do in C++ is mozilla::dom::myDictionary; if (myDictionary.Init(JSONString)) { // do something with myDictionary.property1 // do something with myDictionary.property2 }
Comment on attachment 669332 [details] [diff] [review] v2 Review of attachment 669332 [details] [diff] [review]: ----------------------------------------------------------------- Aha. Now that I finally understand how this is supposed to work, it's fine. r=me
Attachment #669332 -
Flags: review?(khuey) → review+
Assignee | ||
Comment 17•9 years ago
|
||
Attachment #671875 -
Flags: review?(khuey)
Attachment #671875 -
Flags: review?(khuey) → review+
Assignee | ||
Comment 18•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b3d5c1bcf847 https://hg.mozilla.org/mozilla-central/rev/5a707ebc0329
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•9 years ago
|
Attachment #667938 -
Attachment is obsolete: true
Comment on attachment 671875 [details] [diff] [review] handle empty value better [Approval Request Comment] Bug caused by (feature/regressing bug #): needed by 794011 User impact if declined: "The reason is that the addition of this flag within the 'ussdreceived' event would be useful to close an existing USSD dialog on the UI when the USSD session is closed. But the user can still close it manually when she decides to." (bug 794011, comment 20 and 16) Testing completed (on m-c, etc.): on mozilla-central Risk to taking this patch (and alternatives if risky): backout of bug 794011 String or UUID changes made by this patch: None
Attachment #671875 -
Flags: approval-mozilla-aurora?
Comment on attachment 669332 [details] [diff] [review] v2 [Approval Request Comment] Bug caused by (feature/regressing bug #): needed by 794011 User impact if declined: "The reason is that the addition of this flag within the 'ussdreceived' event would be useful to close an existing USSD dialog on the UI when the USSD session is closed. But the user can still close it manually when she decides to." (bug 794011, comment 20 and 16) Testing completed (on m-c, etc.): on mozilla-central Risk to taking this patch (and alternatives if risky): backout of bug 794011 String or UUID changes made by this patch: None
Attachment #669332 -
Flags: approval-mozilla-aurora?
Better: "Risk to taking this patch (and alternatives if risky):" --> <smaug> no risk
Updated•9 years ago
|
Attachment #669332 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•9 years ago
|
Attachment #671875 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•9 years ago
|
Target Milestone: --- → mozilla19
Comment 22•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/5e57959751ed https://hg.mozilla.org/releases/mozilla-aurora/rev/85293aa3815e
Updated•2 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•