Closed Bug 1093358 Opened 5 years ago Closed 5 years ago

Preprocess less JavaScript in mobile/android

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 38

People

(Reporter: nalexander, Assigned: nalexander)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Right now we preprocess a decent amount of JS in mobile/android.  The one that is irritating is browser.js, since line numbers don't line up in stack traces; but there are others.

I propose that we extract a pre-processed AppConstants.jsm, just like we did in Java, and do more of the configuration work at run time.  I think we get nice debugging wins across the board, and we certainly make things easier for IDE users.  (IntelliJ has good JS support but can't handle preprocessor directives.)

A quick skim over what browser.js pre-processes suggests that we're mostly defining lazy-loaded modules.  Having a run-time guard around those loads shouldn't kill our start-up performance or anything like that.

It's possible some other pre-processing will have a larger impact on code size or run time; let's find out!
Did this preprocessed code have the whole # -> //# thing done to it? I have a feeling the patch didn't have any browser.js changes in it, and that change ought to help IDE support very straightforwardly... (Particularly if you can't (or shouldn't) eliminate  it all in here.
Mark's been wanting to shrink browser.js for a while now. I'd definitely support chopping it up to both improve modularity and avoid some preprocessing.
(In reply to Richard Newman [:rnewman] from comment #3)
> Mark's been wanting to shrink browser.js for a while now. I'd definitely
> support chopping it up to both improve modularity and avoid some
> preprocessing.

I am most interested in removing parts and lazy loaded them as needed. WebApps and GeckoView don't use much of what's in browser.js and what's needed for Fennec could still be reduced if we found new ways of lazy loading stuff.
/r/2763 - Bug 1093358 - Add mobile/android AppConstants.jsm. r=mfinkle

Pull down this commit:

hg pull review -r 8fb3601d09010d9efa4fa33b507dc327fe4dca47
Attachment #8552073 - Flags: review?(mark.finkle)
Comment on attachment 8552073 [details]
MozReview Request: bz://1093358/nalexander

rnewman: a second set of eyes on this very-likely-to-rot patch, please.
Attachment #8552073 - Flags: feedback?(rnewman)
Comment on attachment 8552073 [details]
MozReview Request: bz://1093358/nalexander

https://reviewboard.mozilla.org/r/2761/#review1933

This looks fine to me, modulo QI concerns.

::: mobile/android/chrome/content/browser.js
(Diff revision 1)
> +            Services.appinfo instanceof Ci.nsICrashReporter;

I suspect that your direct use of `Services.appinfo` as an `nsICrashReporter` should really involve an explicit QI, here and line 1430.

::: mobile/android/chrome/content/browser.js
(Diff revision 1)
> -#ifdef NIGHTLY_BUILD
> +    if (AppConstants.NIGHTLY_BUILD && this.TCO_DOMAIN == channel.URI.host) {

Isn't it time we removed this nightly conditional? @mfinkle?

::: mobile/android/components/AboutRedirector.js
(Diff revision 1)
> -#ifdef MOZ_DEVICES
> +if (AppConstants.MOZ_DEVICES) {

Incidentally, I find it funny that AppConstants aped nsIAppInfo and friends, and yet here we are, using AppConstants from Gecko.

::: mobile/android/components/DirectoryProvider.js
(Diff revision 1)
> -const SYSTEM_DIST_PATH = "/system/@ANDROID_PACKAGE_NAME@/distribution";
> +const SYSTEM_DIST_PATH = `/system/${AppConstants.ANDROID_PACKAGE_NAME}/distribution`;

Check you out with your fancy backticks.

::: mobile/android/components/SessionStore.js
(Diff revision 1)
> +      Services.appinfo instanceof Ci.nsICrashReporter;

Again, QI.
Attachment #8552073 - Flags: review+
https://reviewboard.mozilla.org/r/2763/#review1941

::: mobile/android/chrome/content/browser.js
(Diff revision 1)
> +          let crashReporterEnabled = "nsICrashReporter" in Ci &&

All on one line please

::: mobile/android/chrome/content/browser.js
(Diff revision 1)
> -        CrashReporter.submitReports = json.value;
> +        let crashReporterEnabled = "nsICrashReporter" in Ci &&

Same

::: mobile/android/components/SessionStore.js
(Diff revision 1)
> -#ifdef MOZ_CRASHREPORTER
> +    let crashReporterEnabled = "nsICrashReporter" in Ci &&

Same

::: mobile/android/components/SessionStore.js
(Diff revision 1)
> +

Remove the blank line

::: mobile/android/components/SessionStore.js
(Diff revision 1)
>        if (ex.result != Components.results.NS_ERROR_NOT_INITIALIZED)

We have Cr for Components.results in this file. Can you change this too?

::: mobile/android/components/SessionStore.js
(Diff revision 1)
>          Components.utils.reportError("SessionStore:" + ex);

We have Cu for Components.utils in this file. Can you change this too?

I am a little hesitant to remove the preprocessing since it removes unused code, but I am hopeful that this might help minification and it should also "fix" our line number issues for error messages. Let's give it a go.
billm, gavin: I decided to do something more expedient for mobile/android only.  I'd be pretty happy to see this grow into a more general thing, but it's sufficient for my purposes right now.
Assignee: nobody → nalexander
Status: NEW → ASSIGNED
(In reply to Chris Kitching [:ckitching] from comment #1)
> Did this preprocessed code have the whole # -> //# thing done to it? I have
> a feeling the patch didn't have any browser.js changes in it, and that
> change ought to help IDE support very straightforwardly... (Particularly if
> you can't (or shouldn't) eliminate  it all in here.

Using EXTRA_PP_{MODULES,COMPONENTS} makes it awkward to do this.  I did look into it.
https://hg.mozilla.org/mozilla-central/rev/a2651cba8673
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
Comment on attachment 8552073 [details]
MozReview Request: bz://1093358/nalexander

Unsure why my review didn't r+ via MozReview. Some PEBKAC no doubt.
Attachment #8552073 - Flags: review?(mark.finkle)
Attachment #8552073 - Flags: review+
Attachment #8552073 - Flags: feedback?(rnewman)
Depends on: 1130812
Depends on: 1134092
Attachment #8552073 - Attachment is obsolete: true
Attachment #8618544 - Flags: review+
You need to log in before you can comment on or make changes to this bug.