1142542 - Use AppConstants in browser/modules

Status: RESOLVED FIXED

(Firefox :: General, defect)

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: Use AppConstants in browser/modules

This patch is mostly straightforward. There are two places where I had to make some decisions:

1. In the Android code we use an instanceof test to decide if the crash reporter is available:
  "nsICrashReporter" in Ci && Services.appinfo instanceof Ci.nsICrashReporter
This seems a lot more awkward to me than the code it's replacing, so I just made MOZ_CRASHREPORTER available.

2. There are a lot of platform #ifdefs. In some case they're pretty confusing: MOZ_WIDGET_GTK versus MOZ_WIDGET_GTK2? You might know XP_UNIX is defined on Macs, but what about Android or b2g? I tried to condense things down to a simpler thing, AppConstants.widgets, which is just a string. I looked through our remaining #ifdef use and it seems like these will mostly work fine.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8576706 [details] [diff] [review]
Use AppConstants in browser/modules

(Sorry for all the spam. I'm trying out a new tool to attach patches and having trouble.)

Comment 3

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

Comment on attachment 8576706 [details] [diff] [review]
Use AppConstants in browser/modules

I think most front-end code using XP_WIN/XP_MACOSX/XP_UNIX/MOZ_WIDGET_GTK actually cares to answer the higher-level question "is this Windows, Linux or Mac?", and does not care about the distinction between "Linux" and other Unixes/Qt/Android/B2G etc.

Given that, I wonder if we can we use this as an opportunity to introduce a "platform" attribute that is one of "linux", "mac", "windows", and start using that? We probably won't be able to use it everywhere, but it seems like it'd be useful.

Looking at the users in your patch, I guess you might still want AppConstants.widgets for the broken_wm_z_order case in RecentWindow.jsm, since the behavior is actually dependent on the widget implementation. But we're already not being super-careful about which we include and which we don't in the edge cases, so it will probably never matter in practice. webrtcUI would be fine using "platform".

It's possible this just overall introduces too much complication, though...

>diff --git a/browser/modules/webrtcUI.jsm b/browser/modules/webrtcUI.jsm

> function getGlobalIndicator() {
>-#ifndef XP_MACOSX

>-#else

Looks like you forgot to remove an #endif here? Did this cause tests to fail? Possible the preprocessor just strips those out silently...

This is certainly a step in the right direction, so r=me, but curious what you think of my suggestion above.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/56da5d401fe2

I decided to go with the name "platform". If we need more specificity than that, we can add support for it later.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/56da5d401fe2