Split JS::CompartmentOptions into two parts: one part that's immutable after compartment creation, one part that remains mutable

RESOLVED FIXED in Firefox 46

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

unspecified
mozilla46
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(1 attachment)

No description provided.
Posted patch PatchSplinter Review
The names of the two sub-parts are entirely up for debate, as long as debate doesn't go on too long.  I don't have particular love for either of them, but at least strawman names moved the ball forward.
Attachment #8702646 - Flags: review?(terrence)
Comment on attachment 8702646 [details] [diff] [review]
Patch

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

This is a nice improvement!

::: dom/workers/WorkerScope.cpp
@@ +431,5 @@
>                               xpc::ShouldDiscardSystemSource();
>    const bool extraWarnings = usesSystemPrincipal &&
>                               xpc::ExtraWarningsForSystemJS();
>  
> +  auto& behaviors = options.behaviors();

Please spell out the full type in this particular instance: it is not inside SpiderMonkey (where we expect people to be able to infer the type) and there are are no other references to it locally, so it would be extremely difficult for someone stumbling over this line to figure out what to grep for to find the relevant code.

::: js/src/jsapi.h
@@ +2157,5 @@
>  };
>  
> +/**
> + * CompartmentCreationOptions specifies options relevant to creating a new
> + * compartment, that are either immutable characteristics of that compartment

No comma before "that": it is only required before "which".

@@ +2158,5 @@
>  
> +/**
> + * CompartmentCreationOptions specifies options relevant to creating a new
> + * compartment, that are either immutable characteristics of that compartment
> + * or that are discarded after the compartment's been created.

It's a bit odd in modern English to see an apostrophe stand in for "has" rather than "is". I'd just spell both words out fully to avoid ambiguity, especially for non-native English readers that may not even know an apostrophe can be used that way.

@@ +2160,5 @@
> + * CompartmentCreationOptions specifies options relevant to creating a new
> + * compartment, that are either immutable characteristics of that compartment
> + * or that are discarded after the compartment's been created.
> + *
> + * Access to these options on an existing compartment is read-only.  If you

I think one space between sentences is still the rule.

@@ +2166,5 @@
> + */
> +class JS_PUBLIC_API(CompartmentCreationOptions)
> +{
> +  public:
> +    explicit CompartmentCreationOptions()

Explicit is only needed on constructors taking one argument. Please remove it.

@@ +2173,5 @@
> +        invisibleToDebugger_(false),
> +        mergeable_(false),
> +        preserveJitCode_(false),
> +        cloneSingletons_(false)
> +

Remove the extra \n here.

@@ +2192,5 @@
> +        return *this;
> +    }
> +    JSTraceOp getTrace() const {
> +        return traceGlobal_;
> +    }

It's a bit weird that this accessor pair is using subtly different style from all the other accessor pairs in the class. Please make this look like the one above:

JSTraceOp getTrace() const { return traceGlobal_; }
CompartmentCreationOptions& setTrace(JSTraceOp op) {
    traceGlobal_ = op;
    return *this;
}

@@ +2214,5 @@
> +    }
> +
> +    // Compartments used for off-thread compilation have their contents merged
> +    // into a target compartment when the compilation is finished. This is only
> +    // allowed if this flag is set.  The invisibleToDebugger flag must also be

One space between sentences. Especially when the other sentence break already has one space.

@@ +2284,5 @@
>  
>          Mode mode_;
>      };
>  
> +    explicit CompartmentBehaviors()

Remove the unnecessary explicit.

@@ +2321,1 @@
>      void setSingletonsAsValues() {

Why does this single setter not return CompartmentBehaviors&? Even if it's not used (yet), we should fix it for consistency.

@@ +2322,5 @@
>          singletonsAsTemplates_ = false;
>      }
>      bool getSingletonsAsTemplates() const {
>          return singletonsAsTemplates_;
>      }

And this should be on one line above the setter, as with all the others.

@@ +2338,5 @@
>  };
>  
> +/**
> + * CompartmentOptions specifies compartment characteristics: both those that
> + * can't be changed on a compartment, and those that can.

I'd spell out the exact types you are talking about here:

CompartmentOptions specifies compartment characteristics: both those that cannot be changed on a compartment after it is created (CompartmentCreationOptions), and those that can be changed at runtime (CompartmentBehaviors).

@@ +2343,5 @@
> + */
> +class JS_PUBLIC_API(CompartmentOptions)
> +{
> +  public:
> +    explicit CompartmentOptions()

Remove the unnecessary explicit.

::: js/src/jscompartment.h
@@ +222,5 @@
>  } // namespace js
>  
>  struct JSCompartment
>  {
> +    const JS::CompartmentCreationOptions creationOptions_;

\o/ Full of win.

::: js/src/shell/js.cpp
@@ +3956,4 @@
>      JS::CompartmentOptions options;
> +
> +    JS::CompartmentCreationOptions& creationOptions = options.creationOptions();
> +    JS::CompartmentBehaviors& behaviors = options.behaviors();

I think it would be fine to use auto& for creationOptions and behaviors here, since there is a reference to CompartmentOptions right above it.

::: js/src/vm/Debugger.h
@@ +118,5 @@
>      bool relookupOrAdd(AddPtr& p, const KeyInput& k, const ValueInput& v) {
>          MOZ_ASSERT(v->compartment() == this->compartment);
> +        MOZ_ASSERT(!k->compartment()->creationOptions_.mergeable());
> +        MOZ_ASSERT_IF(!InvisibleKeysOk,
> +                      !k->compartment()->creationOptions_.invisibleToDebugger());

Do we have to make a direct reference to creationOptions_ here? Please use the accessor if possible.
Attachment #8702646 - Flags: review?(terrence) → review+
(In reply to Terrence Cole [:terrence] from comment #2)
> there
> are are no other references to it locally, so it would be extremely
> difficult for someone stumbling over this line to figure out what to grep
> for to find the relevant code.

While this is true, it'd be no less true if I'd instead changed it to

  options.behaviors()
         .setDiscardSource(discardSource)
         .extraWarningsOverride().set(extraWarnings);

The reason I didn't do the above, is that the above introduces a subtle change of return-value-type to method chaining, on a line-by-line basis.  (The eWO().set() change I don't much like, either, but it *is* limited to a single line, and the change is ultimately ignored as the ignored value of the overall expression.)  IMO that's much worse than having a variable of not-locally-specified type as I did.

Still, changed to add the type.

> > + * CompartmentCreationOptions specifies options relevant to creating a new
> > + * compartment, that are either immutable characteristics of that compartment
> 
> No comma before "that": it is only required before "which".

It's not *required*, but it's not fulfilling that purpose.

The purpose of the comma is to separate one complete clause, from another complete clause that could confusingly blend with it for someone reading quickly.  The comma is a carriage return indicating the reader should reset his mental state before continuing.  (Just as my comma after "complete clause" two sentences ago did.  :-) )

I left this comma in place.

(And, to digress, the argument about that/which usage pedantry and commas I find entirely stupid.  :-) )

> > + * CompartmentCreationOptions specifies options relevant to creating a new
> > + * compartment, that are either immutable characteristics of that compartment
> > + * or that are discarded after the compartment's been created.
> 
> It's a bit odd in modern English to see an apostrophe stand in for "has"
> rather than "is". I'd just spell both words out fully to avoid ambiguity,
> especially for non-native English readers that may not even know an
> apostrophe can be used that way.

I changed this, but I don't think it's at all odd, in modern English or otherwise, for an apostrophe to stand in for "has".

> Explicit is only needed on constructors taking one argument. Please remove
> it.
> 
> It's a bit weird that this accessor pair is using subtly different style
> from all the other accessor pairs in the class.

Note that most of this code was copied from a prior location, so I was just preserving what was done before, for most of it.  Still, made the changes.

> I'd spell out the exact types you are talking about here:

Fair, done.
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #4)
> (In reply to Terrence Cole [:terrence] from comment #2)
> > > + * CompartmentCreationOptions specifies options relevant to creating a new
> > > + * compartment, that are either immutable characteristics of that compartment
> > 
> > No comma before "that": it is only required before "which".
> 
> It's not *required*, but it's not fulfilling that purpose.
> 
> The purpose of the comma is to separate one complete clause, from another
> complete clause that could confusingly blend with it for someone reading
> quickly.  The comma is a carriage return indicating the reader should reset
> his mental state before continuing.  (Just as my comma after "complete
> clause" two sentences ago did.  :-) )

I see your pedantry and I'll raise you an "it's not *required* for that purpose either." ;-)

> I left this comma in place.
> 
> (And, to digress, the argument about that/which usage pedantry and commas I
> find entirely stupid.  :-) )

In general I do to, but you raised the stakes by putting in a full-on /**doc comment*/.
https://hg.mozilla.org/mozilla-central/rev/58942104c315
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.