1124503 - move AppConstants.jsm to toolkit

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

Introduced in bug 1093358, this is useful more broadly than just Android.

Comment 1

[Philip Chee]

Attachment: Patch v1 Move to toolkit

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c088b98d7ba
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c736b4e0f7b
Except for some expected intermittent failures everything looks green.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8567897 [details] [diff] [review]
Patch v1 Move to toolkit

Could you please split the "use new shared AppConstants" changes from the "move the module to toolkit" changes?

Comment 3

[Philip Chee]

Attachment: V2 Part 1: move AppConstants.jsm to toolkit

Comment 4

[Philip Chee]

Attachment: V2 Part 2: Use the new shared AppConstants

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8569286 [details] [diff] [review]
V2 Part 2: Use the new shared AppConstants

This patch would ideally go in its own bug.

>diff --git a/toolkit/modules/AppConstants.jsm b/toolkit/modules/AppConstants.jsm

>+  get CRASHREPORTER() {
>+    return "nsICrashReporter" in Components.interfaces &&
>+           Services.appinfo instanceof Components.interfaces.nsICrashReporter;
>+  },

This is an odd one - as defined this is not returning a build-time constant. It's also not used, so I would suggest not adding it here.

Looks good otherwise.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8569285 [details] [diff] [review]
V2 Part 1: move AppConstants.jsm to toolkit

>diff --git a/toolkit/modules/moz.build b/toolkit/modules/moz.build

>+for var in ('ANDROID_PACKAGE_NAME',

>+            'MOZ_BUILD_APP'):

>+for var in ('NIGHTLY_BUILD',
>+            'RELEASE_BUILD',
>+            'ACCESSIBILITY',
>+            'MOZ_DEVICES',
>+            'MOZ_OFFICIAL_BRANDING',
>+            'MOZ_SAFE_BROWSING',
>+            'MOZ_SERVICES_HEALTHREPORT',
>+            'MOZ_TELEMETRY_REPORTING',
>+            'MOZ_WEBRTC'):

You should be able to remove all of these given that they are AC_DEFINE()d in configure. Could you test that?

>+# Keep it this way if at all possible.  If you need preprocessing,
>+# consider adding fields to AppConstants.jsm.

I don't understand this comment at all, I would just remove it.

Comment 7

[Philip Chee]

Pushed to mozilla-inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/be4fec24c85c

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 9

[Nick Alexander :nalexander [he/him]]

> >+# Keep it this way if at all possible.  If you need preprocessing,
> >+# consider adding fields to AppConstants.jsm.
> 
> I don't understand this comment at all, I would just remove it.

The idea was to discourage folks from preprocessing additional modules in mobile/android.

Comment 10

[Philip Chee]

Oops forgot to post my checkin comment:

>>diff --git a/toolkit/modules/moz.build b/toolkit/modules/moz.build
> 
>>+for var in ('ANDROID_PACKAGE_NAME',
> 
>>+            'MOZ_BUILD_APP'):
> 
>>+for var in ('NIGHTLY_BUILD',
>>+            'RELEASE_BUILD',
>>+            'ACCESSIBILITY',
>>+            'MOZ_DEVICES',
>>+            'MOZ_OFFICIAL_BRANDING',
>>+            'MOZ_SAFE_BROWSING',
>>+            'MOZ_SERVICES_HEALTHREPORT',
>>+            'MOZ_TELEMETRY_REPORTING',
>>+            'MOZ_WEBRTC'):
> 
> You should be able to remove all of these given that they are AC_DEFINE()d in configure. Could you test that?

> mozbuild.preprocessor.Error: ('c:\\t1\\hg\\comm-central\\mozilla\\toolkit\\modules\\AppConstants.jsm', 97, 'UNDEFINED_VAR', 'ANDROID_PACKAGE_NAME')
Remvoving the other defines don't seem to cause problems.

>>+# Keep it this way if at all possible.  If you need preprocessing,
>>+# consider adding fields to AppConstants.jsm.
> 
> I don't understand this comment at all, I would just remove it.
Removed.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

(In reply to Nick Alexander :nalexander from comment #9)
> The idea was to discourage folks from preprocessing additional modules in
> mobile/android.

I see - could have been clearer! Not relevant to the toolkit/ version so having removed it is fine I think. Can re-add a mobile moz.build variant if needed.

(In reply to Philip Chee from comment #10)
> Oops forgot to post my checkin comment:
> 
> >>diff --git a/toolkit/modules/moz.build b/toolkit/modules/moz.build
> > 
> >>+for var in ('ANDROID_PACKAGE_NAME',
> > 
> >>+            'MOZ_BUILD_APP'):

> > mozbuild.preprocessor.Error: ('c:\\t1\\hg\\comm-central\\mozilla\\toolkit\\modules\\AppConstants.jsm', 97, 'UNDEFINED_VAR', 'ANDROID_PACKAGE_NAME')
> Remvoving the other defines don't seem to cause problems.

Yes sorry, that was just my context-preserving quoting style causing confusion. Thanks for fixing this.