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

Status: RESOLVED FIXED

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

Description

[Olli Pettay [:smaug]]

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.

Comment 1

[Olli Pettay [:smaug]]

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.

Comment 2

[Olli Pettay [:smaug]]

Attachment: untested patch (but at least it compiles)

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?

Comment 3

[Guillermo López :willyaranda (probably SLOW response)]

Yes, I will try this, and ping you in case that I don't know how to do it.

Comment 4

[Olli Pettay [:smaug]]

Attachment: patch

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());

Comment 5

[Guillermo López :willyaranda (probably SLOW response)]

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 6

[Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)]

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;
}

Comment 7

[Olli Pettay [:smaug]]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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

Comment 8

[Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)]

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.

Comment 9

[Olli Pettay [:smaug]]

Attachment: v2

Comment 10

[Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)]

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.

Comment 11

[Olli Pettay [:smaug]]

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.

Comment 12

[Olli Pettay [:smaug]]

Or perhaps I need to manually enter to a compartment.
JSAPI isn't very friendly.

Comment 13

[Olli Pettay [:smaug]]

No, can't find any better way to enter to the default compartment of a JSContext.

Comment 14

[Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)]

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 ...

Comment 15

[Olli Pettay [:smaug]]

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 16

[Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)]

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

Comment 17

[Olli Pettay [:smaug]]

Attachment: handle empty value better

Comment 18

[Olli Pettay [:smaug]]

https://hg.mozilla.org/mozilla-central/rev/b3d5c1bcf847
https://hg.mozilla.org/mozilla-central/rev/5a707ebc0329

Comment 19

[Guillermo López :willyaranda (probably SLOW response)]

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

Comment 20

[Guillermo López :willyaranda (probably SLOW response)]

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

Comment 21

[Guillermo López :willyaranda (probably SLOW response)]

Better: "Risk to taking this patch (and alternatives if risky):" --> <smaug> no risk

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5e57959751ed
https://hg.mozilla.org/releases/mozilla-aurora/rev/85293aa3815e