1093358 - Preprocess less JavaScript in mobile/android

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Nick Alexander :nalexander [he/him]]

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!

Comment 1

[Chris Kitching [:ckitching]]

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.

Comment 2

[Chris Kitching [:ckitching]]

)

Comment 3

[Richard Newman [:rnewman]]

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.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Comment 5

[Nick Alexander :nalexander [he/him]]

https://tbpl.mozilla.org/?tree=Try&rev=8fb3601d0901

Comment 6

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: bz://1093358/nalexander

/r/2763 - Bug 1093358 - Add mobile/android AppConstants.jsm. r=mfinkle

Pull down this commit:

hg pull review -r 8fb3601d09010d9efa4fa33b507dc327fe4dca47

Comment 7

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8552073 [details]
MozReview Request: bz://1093358/nalexander

rnewman: a second set of eyes on this very-likely-to-rot patch, please.

Comment 8

[Richard Newman [: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.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 10

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/fx-team/rev/a2651cba8673

Comment 11

[Nick Alexander :nalexander [he/him]]

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.

Comment 12

[Nick Alexander :nalexander [he/him]]

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

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a2651cba8673

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8552073 [details]
MozReview Request: bz://1093358/nalexander

Unsure why my review didn't r+ via MozReview. Some PEBKAC no doubt.

Comment 16

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1093358 - Add mobile/android AppConstants.jsm. r=mfinkle,f=rnewman