Refactor nsJSUtils::EvaluateString to take an option object and optionally propagate exceptions

RESOLVED FIXED in mozilla26

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Bobby Holley (On Leave Until June 11th), Assigned: Bobby Holley (On Leave Until June 11th))

Tracking

unspecified
mozilla26
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

This is the refactoring I proposed in bug 877673 comment 42.
Created attachment 785561 [details] [diff] [review]
Part 1 - Refactor nsJSUtils::EvaluateString to take an EvaluateOptions structure. v1
Attachment #785561 - Flags: review?(gkrizsanits)
Created attachment 785562 [details] [diff] [review]
Part 2 - Give EvaluateString consumers the option to propagate exceptions instead of reporting. v1
Attachment #785562 - Flags: review?(gkrizsanits)
Comment on attachment 785561 [details] [diff] [review]
Part 1 - Refactor nsJSUtils::EvaluateString to take an EvaluateOptions structure. v1

Review of attachment 785561 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsJSEnvironment.cpp
@@ +1254,2 @@
>      rv = nsJSUtils::EvaluateString(mContext, aScript, aScopeObject,
> +                                            aCompileOptions, evalOptions, aRetValue);

While you're here, please fix the indentation?

::: dom/base/nsJSUtils.h
@@ +71,5 @@
> +    {}
> +
> +    EvaluateOptions& setCoerceToString(bool aCoerce) {
> +      coerceToString = aCoerce;
> +      return *this;

(I really don't like this chaining pattern.)
Comment on attachment 785561 [details] [diff] [review]
Part 1 - Refactor nsJSUtils::EvaluateString to take an EvaluateOptions structure. v1

Review of attachment 785561 [details] [diff] [review]:
-----------------------------------------------------------------

Setters on EvaluateOptions do not make much sense to me, since the members are public... Ofc for chaining them, they could be handy I just don't see that happening... the idea is that this struct will have many members like the CompileOptions in the future?
Attachment #785561 - Flags: review?(gkrizsanits) → review+
Attachment #785562 - Flags: review?(gkrizsanits) → review+
(In reply to :Ms2ger from comment #3)
> (I really don't like this chaining pattern.)


(In reply to Gabor Krizsanits [:krizsa :gabor] (moving to Berlin: in and out for a few weeks) from comment #4)
> Setters on EvaluateOptions do not make much sense to me, since the members
> are public... Ofc for chaining them, they could be handy I just don't see
> that happening... the idea is that this struct will have many members like
> the CompileOptions in the future?

I'm mostly just interested in following the established JSAPI pattern on this (see CompartmentOptions, CompileOptions, etc).
https://hg.mozilla.org/mozilla-central/rev/dacad5a40811
https://hg.mozilla.org/mozilla-central/rev/901902206d27
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.