Closed
Bug 1038399
Opened 10 years ago
Closed 10 years ago
Enable WebIDL dictionaries to serialize themselves to JSON
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla34
People
(Reporter: rbarnes, Assigned: bzbarsky)
References
Details
Attachments
(1 file, 1 obsolete file)
8.26 KB,
patch
|
lmandel
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
WebIDL dictionary objects can be initialized from JSON via DictionaryBase::ParseJSON() and the Init(const nsAString& aJSON) methods generated for the individual classes. What is needed here is: * A generated method on derived dictionary classes that calls ToJSValue * A method on DictionaryBase that calls JS_Stringify
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8465196 -
Flags: review?(bobbyholley)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Assignee | ||
Updated•10 years ago
|
Whiteboard: [need review]
Comment 3•10 years ago
|
||
Comment on attachment 8465196 [details] [diff] [review] Add a ToJSON method to Web IDL dictionaries Review of attachment 8465196 [details] [diff] [review]: ----------------------------------------------------------------- r=me with those things fixed. ::: dom/bindings/BindingDeclarations.h @@ +36,5 @@ > bool ParseJSON(JSContext* aCx, const nsAString& aJSON, > JS::MutableHandle<JS::Value> aVal); > + > + // For convienience, callers can pass false for aConvertedOK, in > + // which case we won't examine aValue. I would say "For Codegen convenience" and "in which case this function is a no-op". But really, it's not clear to me why this is useful enough to warrant the confusing API. Can't we just explicitly check + propagate ToObjectInternal before doing the StringifyToJSON call? @@ +41,5 @@ > + bool StringifyToJSON(JSContext* aCx, bool aConvertedOK, > + JS::MutableHandle<JS::Value> aValue, > + nsAString& aJSON); > +private: > + // aString is expected to actually be an nsAString*. Should only be called from Nit - Incomplete comment and trailing whitespace. ::: dom/bindings/BindingUtils.cpp @@ +1614,5 @@ > +{ > + if (!aConvertedOK || > + !JS_Stringify(aCx, aValue, JS::NullPtr(), JS::NullHandleValue, > + AppendJSONToString, &aJSON)) { > + JS_ClearPendingException(aCx); Hm, why squelch the exception here? I think it should bubble up to the nearest AutoJSAPI. ::: dom/bindings/Codegen.py @@ +10835,5 @@ > + "ToJSON", "bool", > + [Argument('nsAString&', 'aJSON')], > + body=dedent(""" > + MOZ_ASSERT(NS_IsMainThread()); > + AutoSafeJSContext cx; Please don't add more AutoSafeJSContexts. Instead, do: AutoJSAPI jsapi; jsapi.Init(); JSContext *cx = jsapi.cx();
Attachment #8465196 -
Flags: review?(bobbyholley) → review+
Assignee | ||
Comment 4•10 years ago
|
||
> Can't we just explicitly check + propagate ToObjectInternal before doing the > StringifyToJSON call? We can, but it seems like more code (e.g. have to to the JS_ClearPendingException twice), and we have a lot of dictionary types. I can do it the other way if you really think the codesize hit is not a big deal. > Hm, why squelch the exception here? I think it should bubble up to the nearest AutoJSAPI. What makes you think there is a nearest AutoJSAPI at all? > Please don't add more AutoSafeJSContexts. Instead, do: And enter which compartment? The important part here is that I have to be in some compartment. It doesn't even matter all that much which one, as long as I'm in one...
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(bobbyholley)
Comment 5•10 years ago
|
||
(In reply to Boris Zbarsky [:bz] (On vacation Aug 5-18) from comment #4) > We can, but it seems like more code (e.g. have to to the > JS_ClearPendingException twice) I'm suggesting getting rid of both JS_ClearPendingException calls. >, and we have a lot of dictionary types. I > can do it the other way if you really think the codesize hit is not a big > deal. My suggestion for Codegen.py is this: MOZ_ASSERT(NS_IsMainThread()); AutoJSAPI jsapi; jsapi.Init(); JSContext *cx = jsapi.cx(); JSAutoCompartment ac(cx, xpc::GetSafeJSContextGlobal()); // Usage approved by bholley JS::Rooted<JS::Value> obj(cx); if (!ToObjectInternal(cx, &obj)) { return false; } return StringifyToJSON(cx, &obj, aJSON); I don't think that there's much codesize difference between that and the version where we pass the bool to StringifyToJSON. > > Hm, why squelch the exception here? I think it should bubble up to the nearest AutoJSAPI. > > What makes you think there is a nearest AutoJSAPI at all? Because we're adding one (for the Codegen). And for everything else, AutoJSAPI is _almost_ the only place where JSContexts come from these days. Bob is on track to get rid of that "almost" within the next week or two. > > Please don't add more AutoSafeJSContexts. Instead, do: > > And enter which compartment? The important part here is that I have to be > in some compartment. It doesn't even matter all that much which one, as > long as I'm in one... Yeah, just use xpc::GetSafeJSContextGlobal (per above). I'll rename all of these to something like |JSAutoEnterDummyCompartment ac(cx)| at some point. We want to eliminate AutoSafeJSContext because 90% of the cases don't actually need to be in the dummy global. Let me know if any of that doesn't make sense.
Flags: needinfo?(bobbyholley)
Assignee | ||
Comment 6•10 years ago
|
||
> Because we're adding one (for the Codegen)
Ah, I see. And it'll just report the exception and move on?
Comment 7•10 years ago
|
||
(In reply to Boris Zbarsky [:bz] (On vacation Aug 5-18) from comment #6) > > Because we're adding one (for the Codegen) > > Ah, I see. And it'll just report the exception and move on? Correct, unless the caller steals and propagates it (which, in this case, it won't).
Assignee | ||
Comment 8•10 years ago
|
||
Great. Then comment 5 sounds reasonable to me, except with: return ToObjectInternal(cx, &obj) && StringifyToJSON(cx, &obj, aJSON);
Assignee | ||
Comment 9•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8465196 -
Attachment is obsolete: true
Assignee | ||
Comment 10•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c67ef1f9515
Flags: in-testsuite+
Whiteboard: [need review]
Target Milestone: --- → mozilla34
Comment 11•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9c67ef1f9515
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 13•10 years ago
|
||
Comment on attachment 8465680 [details] [diff] [review] Updated to comments Approval Request Comment [Feature/regressing bug #]: Not a regression [User impact if declined]: Can't uplift bug 1033423 [Describe test coverage new/current, TBPL]: [Risks and why]: Low risk. Only changes shipping code in webcrypto, which is not enabled by default. [String/UUID change made/needed]: None.
Attachment #8465680 -
Flags: approval-mozilla-aurora?
Comment 14•10 years ago
|
||
Comment on attachment 8465680 [details] [diff] [review] Updated to comments This is needed for dependent bug 1033423. Aurora+
Attachment #8465680 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 15•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/2479de9ab541
status-firefox33:
--- → fixed
status-firefox34:
--- → fixed
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•