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

Status: RESOLVED FIXED

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

Description

[Bobby Holley (:bholley)]

This is the refactoring I proposed in bug 877673 comment 42.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Part 1 - Refactor nsJSUtils::EvaluateString to take an EvaluateOptions structure. v1

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 2 - Give EvaluateString consumers the option to propagate exceptions instead of reporting. v1

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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 4

[Gabor Krizsanits (INACTIVE)]

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?

Comment 5

[Bobby Holley (:bholley)]

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

Comment 6

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/dacad5a40811
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/901902206d27

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dacad5a40811
https://hg.mozilla.org/mozilla-central/rev/901902206d27