Closed Bug 1449833 Opened 2 years ago Closed 2 years ago

Use StaticPrefs in nsJSEnvironment.cpp

Categories

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

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: njn, Assigned: njn)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

See bug 1448219 for details.
Comment on attachment 8963452 [details]
Bug 1449833 - Use StaticPrefs in nsJSEnvironment.cpp.

https://reviewboard.mozilla.org/r/232366/#review237788

::: dom/base/nsJSEnvironment.cpp:2321
(Diff revision 1)
>          }
>        }
>  
>        if (!sShuttingDown) {
> -        if (sPostGCEventsToObserver || Telemetry::CanRecordExtended()) {
> +        if (StaticPrefs::javascript_options_mem_notify() ||
> +            Telemetry::CanRecordExtended()) {

I don't know Gecko style that well, but in SpiderMonkey we would want the { on a separate line for a multi-line conditional.

::: modules/libpref/init/StaticPrefList.h:119
(Diff revision 1)
> +// appropriate pref is set.
> +#ifdef ANDROID
> +  // Disable the JS engine's GC on memory pressure, since we do one in the
> +  // mobile browser (bug 669346).
> +  // XXX: that GC appears to no longer be present. Should this value be
> +  //      changed, or the pref removed entirely?

I don't know what's going on here, but it seems important to make this sensible. I'll needinfo people for it.
jonco, smaug - this patch is r=me other than the question about what's going on with memory pressure-triggered GCs (see the XXX comment above). I'm checking if either of you are familiar with what we want to have happen there.
Flags: needinfo?(jcoppeard)
Flags: needinfo?(bugs)
(In reply to Steve Fink [:sfink] [:s:] from comment #2)
> I don't know Gecko style that well, but in SpiderMonkey we would want the {
> on a separate line for a multi-line conditional.

I think that is a SpiderMonkey thing, but it's something I've really come to appreciate.

> > +  // Disable the JS engine's GC on memory pressure, since we do one in the
> > +  // mobile browser (bug 669346).

Oh wow, I wasn't aware of this.  If possible we should make this work the same as the desktop browser, but someone who works on the mobile browser should check this is OK.

(Actually we should get rid of this GC and do compacting on demand, as suggested in bug 1421966).
Flags: needinfo?(jcoppeard)
{ doesn't go to its own line with 'if'
Flags: needinfo?(bugs)
So at least the XXX comment should be tweaked a bit. Best would be to get rid of the explicit gc call in Android call and not have the special case in the prefs.
I have filed bug 1450787 as a follow-up to decide what to do with javascript.options.gc_on_memory_pressure, and updated the comment to point to that bug.

FWIW, the '{' on it's own line thing is quite useful when you have four space indents, but less useful when you have two space indents, which is the standard for Gecko.

With four space indents, the condition and body look very similar:

> if (condition1() &&
>     condition2()) {
>     body1()
>     body2()
> }

With two space indents, it's less of a problem:

> if (condition1() &&
>     condition2()) {
>   body1()
>   body2()
> }
Comment on attachment 8963452 [details]
Bug 1449833 - Use StaticPrefs in nsJSEnvironment.cpp.

https://reviewboard.mozilla.org/r/232366/#review240400

Sorry for the delay, I hadn't noticed the patch had been updated.
Attachment #8963452 - Flags: review?(sphink) → review+
Pushed by nnethercote@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/69f03ba7f761
Use StaticPrefs in nsJSEnvironment.cpp. r=sfink
https://hg.mozilla.org/mozilla-central/rev/69f03ba7f761
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.