Use AppConstants in browser/modules

RESOLVED FIXED in Firefox 39

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: billm, Assigned: billm)

Tracking

34 Branch
Firefox 39
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(1 attachment)

No description provided.
Summary: Use AppConstants in toolkit/modules → 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 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.)
Attachment #8576706 - Flags: review?(gavin.sharp)
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.
Attachment #8576706 - Flags: review?(gavin.sharp) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/56da5d401fe2
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
You need to log in before you can comment on or make changes to this bug.