Closed Bug 797806 Opened 7 years ago Closed 7 years ago

Add a helper service or functions to deal with stringified JSON in C++

Categories

(Core :: DOM: Core & HTML, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: smaug, Assigned: smaug)

References

Details

Attachments

(2 files, 2 obsolete files)

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.
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.
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.
Attached patch patch (obsolete) — Splinter Review
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;
}
(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.
Attached patch v2Splinter Review
Attachment #668141 - Attachment is obsolete: true
Attachment #668141 - Flags: review?(khuey)
Attachment #669332 - Flags: review?(khuey)
Blocks: 794011
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-
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)
Or perhaps I need to manually enter to a compartment.
JSAPI isn't very friendly.
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 ...
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+
Attachment #671875 - Flags: review?(khuey)
https://hg.mozilla.org/mozilla-central/rev/b3d5c1bcf847
https://hg.mozilla.org/mozilla-central/rev/5a707ebc0329
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
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
Attachment #669332 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #671875 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Target Milestone: --- → mozilla19
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.