1228655 - Replace #ifdefs in browser code by AppConstants

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[:Felipe Gomes (needinfo for replies!)]

The companion bug to bug 1228627, in order to remove pre-processing from browser/

Comment 1

[:Felipe Gomes (needinfo for replies!)]

Attachment: MozReview Request: Bug 1228655 - Remove ifdefs for gEditUIVisible. r=Gijs

Bug 1228655 - Remove ifdefs for gEditUIVisible. r=Gijs

Comment 2

[:Felipe Gomes (needinfo for replies!)]

Attachment: MozReview Request: Bug 1228655 - Remove ifdef for Win7Features. r=Gijs

Bug 1228655 - Remove ifdef for Win7Features. r=Gijs

Comment 3

[:Felipe Gomes (needinfo for replies!)]

Attachment: MozReview Request: Bug 1228655 - Remove ifdef for macBrowserOverlay functions. r=Gijs

Bug 1228655 - Remove ifdef for macBrowserOverlay functions. r=Gijs

Comment 4

[:Felipe Gomes (needinfo for replies!)]

Attachment: MozReview Request: Bug 1228655 - Remove ifdefs for e10s testing UI. r=Gijs

Bug 1228655 - Remove ifdefs for e10s testing UI. r=Gijs

Comment 5

[:Felipe Gomes (needinfo for replies!)]

Attachment: MozReview Request: Bug 1228655 - Move TabsInTitlebar to its own file to remove ifdefs. r=Gijs

Bug 1228655 - Move TabsInTitlebar to its own file to remove ifdefs. r=Gijs

TabsInTitlebar has various ifdefs to avoid shipping code that's not gonna be used on Linux. Instead of complicating the logic with AppConstants checks and shipping this extra code unecessarily on Linux, i've separated the code to a different file that can be replaced by a stub implementation.

Comment 6

[:Felipe Gomes (needinfo for replies!)]

Attachment: MozReview Request: Bug 1228655 - Remove ifdefs for MOZ_SAFE_BROWSING. r=Gijs

Bug 1228655 - Remove ifdefs for MOZ_SAFE_BROWSING. r=Gijs

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Attachment: MozReview Request: Bug 1228655 - Remove ifdefs for MOZ_CRASHREPORTER. r=mconley

Bug 1228655 - Remove ifdefs for MOZ_CRASHREPORTER. r=mconley

gPluginHandler.CrashSubmit appears unused

Comment 8

[:Felipe Gomes (needinfo for replies!)]

Attachment: MozReview Request: Bug 1228655 - Remove ifdefs for data/health reporting. r=Gijs

Bug 1228655 - Remove ifdefs for data/health reporting. r=Gijs

Comment 9

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8693677 [details]
MozReview Request: Bug 1228655 - Remove ifdefs for gEditUIVisible. r=Gijs

Bug 1228655 - Remove ifdefs for gEditUIVisible. r=Gijs

Comment 10

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8693678 [details]
MozReview Request: Bug 1228655 - Remove ifdef for Win7Features. r=Gijs

Bug 1228655 - Remove ifdef for Win7Features. r=Gijs

Comment 11

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8693679 [details]
MozReview Request: Bug 1228655 - Remove ifdef for macBrowserOverlay functions. r=Gijs

Bug 1228655 - Remove ifdef for macBrowserOverlay functions. r=Gijs

Comment 12

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8693680 [details]
MozReview Request: Bug 1228655 - Remove ifdefs for e10s testing UI. r=Gijs

Bug 1228655 - Remove ifdefs for e10s testing UI. r=Gijs

Comment 13

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8693681 [details]
MozReview Request: Bug 1228655 - Move TabsInTitlebar to its own file to remove ifdefs. r=Gijs

Bug 1228655 - Move TabsInTitlebar to its own file to remove ifdefs. r=Gijs

TabsInTitlebar has various ifdefs to avoid shipping code that's not gonna be used on Linux. Instead of complicating the logic with AppConstants checks and shipping this extra code unecessarily on Linux, i've separated the code to a different file that can be replaced by a stub implementation.

Comment 14

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8693682 [details]
MozReview Request: Bug 1228655 - Remove ifdefs for MOZ_SAFE_BROWSING. r=Gijs

Bug 1228655 - Remove ifdefs for MOZ_SAFE_BROWSING. r=Gijs

Comment 15

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8693683 [details]
MozReview Request: Bug 1228655 - Remove ifdefs for MOZ_CRASHREPORTER. r=mconley

Bug 1228655 - Remove ifdefs for MOZ_CRASHREPORTER. r=mconley

gPluginHandler.CrashSubmit appears unused

Comment 16

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8693684 [details]
MozReview Request: Bug 1228655 - Remove ifdefs for data/health reporting. r=Gijs

Bug 1228655 - Remove ifdefs for data/health reporting. r=Gijs

Comment 17

[:Felipe Gomes (needinfo for replies!)]

Attachment: MozReview Request: Bug 1228655 - Remove ifdefs for OSX non-browser-window behavior. r=Gijs

Bug 1228655 - Remove ifdefs for OSX non-browser-window behavior. r=Gijs

Comment 18

[:Felipe Gomes (needinfo for replies!)]

Attachment: MozReview Request: Bug 1228655 - Remove ifdefs for generic platform-specific behavior. r=Gijs

Bug 1228655 - Remove ifdefs for generic platform-specific behavior. r=Gijs

Comment 19

[:Felipe Gomes (needinfo for replies!)]

Attachment: MozReview Request: Bug 1228655 - Remove ifdefs for MOZ_WIDGET_GTK. r=Gijs

Bug 1228655 - Remove ifdefs for MOZ_WIDGET_GTK. r=Gijs

Comment 20

[:Felipe Gomes (needinfo for replies!)]

Attachment: MozReview Request: Bug 1228655 - Remove ifdefs in aboutDialog.js. r=Gijs

Bug 1228655 - Remove ifdefs in aboutDialog.js. r=Gijs

Comment 21

[:Felipe Gomes (needinfo for replies!)]

Attachment: MozReview Request: Bug 1228655 - Remove ifdefs in browser-devedition.js. r=Gijs

Bug 1228655 - Remove ifdefs in browser-devedition.js. r=Gijs

Comment 22

[:Felipe Gomes (needinfo for replies!)]

Attachment: MozReview Request: Bug 1228655 - Remove ifdefs in browser-fullscreen.js. r=Gijs

Bug 1228655 - Remove ifdefs in browser-fullscreen.js. r=Gijs

Comment 23

[:Felipe Gomes (needinfo for replies!)]

Attachment: MozReview Request: Bug 1228655 - Remove ifdefs in browser-gesturesupport.js. r=Gijs

Bug 1228655 - Remove ifdefs in browser-gesturesupport.js. r=Gijs

Comment 24

[:Felipe Gomes (needinfo for replies!)]

Attachment: MozReview Request: Bug 1228655 - Remove ifdefs in browser-places.js. r=Gijs

Bug 1228655 - Remove ifdefs in browser-places.js. r=Gijs

Comment 25

[:Felipe Gomes (needinfo for replies!)]

Attachment: MozReview Request: Bug 1228655 - Remove ifdefs of MOZ_SERVICES_CLOUDSYNC. r=Gijs

Bug 1228655 - Remove ifdefs of MOZ_SERVICES_CLOUDSYNC. r=Gijs

Comment 26

[:Felipe Gomes (needinfo for replies!)]

Attachment: MozReview Request: Bug 1228655 - Remove ifdefs from utilityOverlay. r=Gijs

Bug 1228655 - Remove ifdefs from utilityOverlay. r=Gijs

Comment 27

[:Felipe Gomes (needinfo for replies!)]

Attachment: MozReview Request: Bug 1228655 - Don't preprocess browser.js r=Gijs

Bug 1228655 - Don't preprocess browser.js r=Gijs

Comment 28

[:Gijs (he/him)]

Comment on attachment 8693677 [details]
MozReview Request: Bug 1228655 - Remove ifdefs for gEditUIVisible. r=Gijs

https://reviewboard.mozilla.org/r/26533/#review23953

::: browser/base/content/browser.js:3975
(Diff revision 1)
> -#ifndef XP_MACOSX
> +  if (AppConstants.platform = "macosx")

== please :-)

Comment 29

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8693683 [details]
MozReview Request: Bug 1228655 - Remove ifdefs for MOZ_CRASHREPORTER. r=mconley

https://reviewboard.mozilla.org/r/26545/#review23959

::: browser/base/content/browser.js:231
(Diff revision 1)
> -#ifdef MOZ_CRASHREPORTER
> +
>  XPCOMUtils.defineLazyModuleGetter(this, "PluginCrashReporter",
>    "resource:///modules/ContentCrashHandlers.jsm");
> -#endif

Out of curiosity, is there a reason you're wholesale removing these ifdef's, as opposed to replacing them with

```JavaScript

if (AppConstants.MOZ_CRASHREPORTER) {
  // ...
}
```
?

Comment 30

[:Gijs (he/him)]

Comment on attachment 8693678 [details]
MozReview Request: Bug 1228655 - Remove ifdef for Win7Features. r=Gijs

https://reviewboard.mozilla.org/r/26535/#review23955

::: browser/base/content/browser.js:268
(Diff revision 1)
> +

Nit: remove added newline.

Comment 31

[:Gijs (he/him)]

Comment on attachment 8693679 [details]
MozReview Request: Bug 1228655 - Remove ifdef for macBrowserOverlay functions. r=Gijs

https://reviewboard.mozilla.org/r/26537/#review23961

::: browser/base/content/browser.js:1636
(Diff revision 1)
>    },

Nit: should also be ; instead of ,

::: browser/base/content/browser.js:1656
(Diff revision 1)
> -var nonBrowserWindowStartup        = gBrowserInit.nonBrowserWindowStartup.bind(gBrowserInit);
> +  var nonBrowserWindowStartup        = gBrowserInit.nonBrowserWindowStartup.bind(gBrowserInit);

According to DXR nobody uses these aliases. Can you file a bug to remove them?

Comment 32

[:Felipe Gomes (needinfo for replies!)]

https://reviewboard.mozilla.org/r/26545/#review23959

> Out of curiosity, is there a reason you're wholesale removing these ifdef's, as opposed to replacing them with
> 
> ```JavaScript
> 
> if (AppConstants.MOZ_CRASHREPORTER) {
>   // ...
> }
> ```
> ?

As they are defineLazyModuleGetter, it's a conditional declaration anyways that is only gonna run the Cu.import if PluginCrashReporter is accessed, which supposedly shouldn't happen if MOZ_CRASHREPORTER isn't defined. The #ifdef only helps with not shipping the code, but if it's gonna ship anyway it seemed this was just a double check for the same thing.  I could be wrong though, so that's why I tagged you here for a second set of eyes.

Comment 33

[:Gijs (he/him)]

Comment on attachment 8693680 [details]
MozReview Request: Bug 1228655 - Remove ifdefs for e10s testing UI. r=Gijs

https://reviewboard.mozilla.org/r/26539/#review23967

Comment 34

[:Felipe Gomes (needinfo for replies!)]

https://reviewboard.mozilla.org/r/26537/#review23969

::: browser/base/content/browser.js:1656
(Diff revision 1)
> -var nonBrowserWindowStartup        = gBrowserInit.nonBrowserWindowStartup.bind(gBrowserInit);
> +  var nonBrowserWindowStartup        = gBrowserInit.nonBrowserWindowStartup.bind(gBrowserInit);

Yeah, I've got a couple of things that I saw while doing this work that can be cleaned up or improved. I'll file bugs for them. I'll leave this issue open until I do it to not forget.

There's a comment here though that says "Legacy global init functions", so it appears something intentionally left for add-ons to use?

Comment 35

[:Gijs (he/him)]

Comment on attachment 8693684 [details]
MozReview Request: Bug 1228655 - Remove ifdefs for data/health reporting. r=Gijs

https://reviewboard.mozilla.org/r/26547/#review23973

::: toolkit/modules/AppConstants.jsm:62
(Diff revision 1)
> +  MOZ_DATA_REPORTING:
> +#ifdef MOZ_DATA_REPORTING

I'm assuming you checked this is defined?

Comment 36

[:Felipe Gomes (needinfo for replies!)]

https://reviewboard.mozilla.org/r/26547/#review23975

::: toolkit/modules/AppConstants.jsm:62
(Diff revision 1)
> +  MOZ_DATA_REPORTING:
> +#ifdef MOZ_DATA_REPORTING

Yep, it's defined globally in configure.in

Comment 37

[:Gijs (he/him)]

Comment on attachment 8693682 [details]
MozReview Request: Bug 1228655 - Remove ifdefs for MOZ_SAFE_BROWSING. r=Gijs

https://reviewboard.mozilla.org/r/26543/#review23971

::: browser/base/content/browser-safebrowsing.js
(Diff revision 1)
> -#ifdef MOZ_SAFE_BROWSING

Can we still ensure we don't define gSafeBrowsing when this is turned off, by shipping an empty file instead of browser-safebrowsing.js ?

::: browser/base/content/browser.js
(Diff revision 1)
> -#ifdef MOZ_SAFE_BROWSING
>  XPCOMUtils.defineLazyModuleGetter(this, "SafeBrowsing",
>    "resource://gre/modules/SafeBrowsing.jsm");
> -#endif

I'm a little worried what the side-effects of this are. Can we just stick this behind AppConstants as well?

::: browser/base/content/utilityOverlay.js:639
(Diff revision 1)
> -  if (typeof gSafeBrowsing != "undefined")
> +  if (AppConstants.MOZ_SAFE_BROWSING)

I think this should probably still check typeof gSafeBrowsing, because it'll be included in windows that don't have a gSafeBrowsing.

Comment 38

[:Gijs (he/him)]

Comment on attachment 8693686 [details]
MozReview Request: Bug 1228655 - Remove ifdefs for OSX non-browser-window behavior. r=Gijs

https://reviewboard.mozilla.org/r/26549/#review23977

::: browser/base/content/browser.js:6306
(Diff revision 1)
> -  return isPBWindow || gBrowser.warnAboutClosingTabs(gBrowser.closingTabsEnum.ALL);
> -#else
> -  return true;
> +  return AppConstants.platform == "macosx"
> +         ? (isPBWindow || gBrowser.warnAboutClosingTabs(gBrowser.closingTabsEnum.ALL))
> +         : true;

Nit:

return AppConstants.platform \!= "macosx"
     || ( ...) ;

Comment 39

[:Gijs (he/him)]

Comment on attachment 8693687 [details]
MozReview Request: Bug 1228655 - Remove ifdefs for generic platform-specific behavior. r=Gijs

https://reviewboard.mozilla.org/r/26551/#review23981

::: browser/base/content/browser.js:520
(Diff revision 1)
> -        var popupButtonText = gNavigatorBundle.getString("popupWarningButton");
> +          var popupButtonText = gNavigatorBundle.getString("popupWarningButton");
> -        var popupButtonAccesskey = gNavigatorBundle.getString("popupWarningButton.accesskey");
> +          var popupButtonAccesskey = gNavigatorBundle.getString("popupWarningButton.accesskey");
> -#else
> +        } else {
> -        var popupButtonText = gNavigatorBundle.getString("popupWarningButtonUnix");
> +          var popupButtonText = gNavigatorBundle.getString("popupWarningButtonUnix");
> -        var popupButtonAccesskey = gNavigatorBundle.getString("popupWarningButtonUnix.accesskey");
> +          var popupButtonAccesskey = gNavigatorBundle.getString("popupWarningButtonUnix.accesskey");

This redeclares variables. Could just do:

var stringKey = AppConstants.platform == "win" ? "popupWarningButton" : "popupWarningButtonunix";

and then reuse that key. Looks like that'll be shorter and less repetitive anyway.

::: browser/base/content/browser.js:1799
(Diff revision 1)
>    var backgroundTabModifier = aEvent.button == 1 ||
> -#ifdef XP_MACOSX
> +                              metaKeyPressed;

Nit: seems like this can now just be 1 line.

::: browser/base/content/browser.js
(Diff revision 1)
> -#ifdef XP_MACOSX
>    "print-button": "printButton.tooltip",
> -#endif

This button does something else on OS X compared to everywhere else (print vs. print preview). So we shouldn't show that tooltip everywhere else.

Can you convert the arrays to |let| instead of |const| and add this item dynamically?

::: browser/base/content/browser.js:7683
(Diff revision 1)
> -  return parseFloat(Services.sysinfo.getProperty("version")) < 6;
> -#else
> +  return AppConstants.platform == "win" &&
> +         parseFloat(Services.sysinfo.getProperty("version")) < 6;

Nit: return AppConstants.isPlatformAndVersionAtMost("win", "5.9");

Comment 40

[:Gijs (he/him)]

Comment on attachment 8693689 [details]
MozReview Request: Bug 1228655 - Remove ifdefs for MOZ_WIDGET_GTK. r=Gijs

https://reviewboard.mozilla.org/r/26553/#review23985

::: browser/base/content/browser.js:1022
(Diff revision 1)
> -#if MOZ_WIDGET_GTK == 2
> +        if (AppConstants.platform == "linux") {
> -        // On X, we're not currently able to account for the size of the window
> +          // On X, we're not currently able to account for the size of the window
> -        // border.  Use 28px as a guess (titlebar + bottom window border)
> +          // border.  Use 28px as a guess (titlebar + bottom window border)
> -        defaultHeight -= 28;
> +          defaultHeight -= 28;
> -#endif

This checks for GTK2 vs. GTK3 as well, so checking for Linux isn't quite the same. We should probably add MOZ_WIDGET_GTK to AppConstants instead.

Comment 41

[:Gijs (he/him)]

Comment on attachment 8693690 [details]
MozReview Request: Bug 1228655 - Remove ifdefs in aboutDialog.js. r=Gijs

https://reviewboard.mozilla.org/r/26555/#review23989

::: browser/base/content/aboutDialog.js:71
(Diff revision 1)
> +if (!AppConstants.MOZ_UPDATER) {
> +  return;
> +}

Sadly this will throw errors - you can't return in global scope.

This patch also doesn't remove the final #endif which will throw a syntax error.

Comment 42

[:Gijs (he/him)]

Comment on attachment 8693691 [details]
MozReview Request: Bug 1228655 - Remove ifdefs in browser-devedition.js. r=Gijs

https://reviewboard.mozilla.org/r/26557/#review23991

Comment 43

[:Gijs (he/him)]

Comment on attachment 8693692 [details]
MozReview Request: Bug 1228655 - Remove ifdefs in browser-fullscreen.js. r=Gijs

https://reviewboard.mozilla.org/r/26559/#review23995

Comment 44

[:Gijs (he/him)]

Comment on attachment 8693693 [details]
MozReview Request: Bug 1228655 - Remove ifdefs in browser-gesturesupport.js. r=Gijs

https://reviewboard.mozilla.org/r/26561/#review23997

Comment 45

[:Gijs (he/him)]

Comment on attachment 8693694 [details]
MozReview Request: Bug 1228655 - Remove ifdefs in browser-places.js. r=Gijs

https://reviewboard.mozilla.org/r/26563/#review24001

::: browser/base/content/browser-places.js:788
(Diff revision 1)
> -    var modifKey = aEvent.metaKey || aEvent.shiftKey;
> +      var modifKey = aEvent.metaKey || aEvent.shiftKey;
> -#else
> +    } else {
> -    var modifKey = aEvent.ctrlKey || aEvent.shiftKey;
> +      var modifKey = aEvent.ctrlKey || aEvent.shiftKey;

This redeclares variables again. Either define the variable above or use a ternary.

Comment 46

[:Gijs (he/him)]

Comment on attachment 8693695 [details]
MozReview Request: Bug 1228655 - Remove ifdefs of MOZ_SERVICES_CLOUDSYNC. r=Gijs

https://reviewboard.mozilla.org/r/26565/#review24003

Comment 47

[:Gijs (he/him)]

Comment on attachment 8693696 [details]
MozReview Request: Bug 1228655 - Remove ifdefs from utilityOverlay. r=Gijs

https://reviewboard.mozilla.org/r/26567/#review24005

::: browser/base/content/utilityOverlay.js:514
(Diff revision 1)
> -#ifdef XP_WIN
> +  if (AppConstants.platform == "win") {
> -  var features = "chrome,centerscreen,dependent";
> +    var features = "chrome,centerscreen,dependent";
> -#elifdef XP_MACOSX
> +  } else if (AppConstants.platform == "macosx") {
> -  var features = "chrome,resizable=no,minimizable=no";
> +    var features = "chrome,resizable=no,minimizable=no";
> -#else
> +  } else {
> -  var features = "chrome,centerscreen,dependent,dialog=no";
> +    var features = "chrome,centerscreen,dependent,dialog=no";
> -#endif
> +  }

Some more redeclaration. Probably easiest to:

var features = "chrome,";
if (AppConstants.platform == "win") {
  features += "centerscreen,dependent";
} else if (AppConstants.platform == "macosx") {
  features += "resizable=no,minimizable=no";
} else {
  features += "centerscreen,dependent,dialog=no";
}

Comment 48

[:Gijs (he/him)]

Comment on attachment 8693697 [details]
MozReview Request: Bug 1228655 - Don't preprocess browser.js r=Gijs

https://reviewboard.mozilla.org/r/26569/#review24007

Heh, I was expecting a bigger patch for this one...

Comment 49

[:Gijs (he/him)]

Comment on attachment 8693696 [details]
MozReview Request: Bug 1228655 - Remove ifdefs from utilityOverlay. r=Gijs

https://reviewboard.mozilla.org/r/26567/#review24011

Err, I meant to tick the 'ship it' box here, though the redeclaration nit is still valid.

Comment 50

[:Gijs (he/him)]

(In reply to :Felipe Gomes from comment #32)
> https://reviewboard.mozilla.org/r/26545/#review23959
> 
> > Out of curiosity, is there a reason you're wholesale removing these ifdef's, as opposed to replacing them with
> > 
> > ```JavaScript
> > 
> > if (AppConstants.MOZ_CRASHREPORTER) {
> >   // ...
> > }
> > ```
> > ?
> 
> As they are defineLazyModuleGetter, it's a conditional declaration anyways
> that is only gonna run the Cu.import if PluginCrashReporter is accessed,
> which supposedly shouldn't happen if MOZ_CRASHREPORTER isn't defined. The
> #ifdef only helps with not shipping the code, but if it's gonna ship anyway
> it seemed this was just a double check for the same thing.  I could be wrong
> though, so that's why I tagged you here for a second set of eyes.

Commenting because this happened with the safebrowsing as well: I believe that any access of this variable, including by e.g. devtools enumerating window properties, or by people doing:

if (window.CrashReporter/SafeBrowsing/whatever)

will try to load the module lazily, and that doing so will throw an exception if the module doesn't exist. So we should be careful about this.

Comment 51

[:Gijs (he/him)]

Comment on attachment 8693681 [details]
MozReview Request: Bug 1228655 - Move TabsInTitlebar to its own file to remove ifdefs. r=Gijs

https://reviewboard.mozilla.org/r/26541/#review24023

Fabulous. Thanks a lot!

Comment 52

[:Felipe Gomes (needinfo for replies!)]

https://reviewboard.mozilla.org/r/26543/#review23971

> Can we still ensure we don't define gSafeBrowsing when this is turned off, by shipping an empty file instead of browser-safebrowsing.js ?

Yeah. It was not necessary to ship an empty file, I just added the #ifdef around the <script> tag that would include it in global-scripts.inc, and avoid shipping it at all in jar.mn

> I think this should probably still check typeof gSafeBrowsing, because it'll be included in windows that don't have a gSafeBrowsing.

Sure. With the changes here I think either check would work, but I'm doing both just in case. A bit overkill maybe..

Comment 53

[:Felipe Gomes (needinfo for replies!)]

https://reviewboard.mozilla.org/r/26551/#review24063

::: browser/base/content/browser.js
(Diff revision 1)
> -#ifdef XP_MACOSX
>    "print-button": "printButton.tooltip",
> -#endif

this appears to be just a mapping of nodes to tooltip names, but UpdateDynamicShortcutTooltipText still needs to be called with the corresponding triggerNode for it to be used. So the ifdef was just to avoid shipping this line? (and yet shipping a longer `//@line ...` comment).

I've changed that anyways in case there's something I've overlooked.

It is not necessary to change from const to let because we can alter the contents of the object, it's just the reference that is a const.

::: browser/base/content/browser.js:7683
(Diff revision 1)
> -  return parseFloat(Services.sysinfo.getProperty("version")) < 6;
> -#else
> +  return AppConstants.platform == "win" &&
> +         parseFloat(Services.sysinfo.getProperty("version")) < 6;

Funny, I had done that but wasn't sure about tricking it with the 5.9 comparison. I like it better too so I've changed it.

Comment 54

[:Felipe Gomes (needinfo for replies!)]

https://reviewboard.mozilla.org/r/26555/#review24069

::: browser/base/content/aboutDialog.js:71
(Diff revision 1)
> +if (!AppConstants.MOZ_UPDATER) {
> +  return;
> +}

bah I guess there's no good way to avoid changing the blame here on half of this file by indenting it.. oh well

Comment 55

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8693683 [details]
MozReview Request: Bug 1228655 - Remove ifdefs for MOZ_CRASHREPORTER. r=mconley

https://reviewboard.mozilla.org/r/26545/#review24071

So I did some poking around, and I think this is safe. Everybody attempting to access gCrashHandler or PluginCrashReporter is already checking MOZ_CRASHREPORTER first.

I guess we'll find out soon enough. :)

Comment 56

[:Felipe Gomes (needinfo for replies!)]

https://reviewboard.mozilla.org/r/26553/#review24073

::: browser/base/content/browser.js:1022
(Diff revision 1)
> -#if MOZ_WIDGET_GTK == 2
> +        if (AppConstants.platform == "linux") {
> -        // On X, we're not currently able to account for the size of the window
> +          // On X, we're not currently able to account for the size of the window
> -        // border.  Use 28px as a guess (titlebar + bottom window border)
> +          // border.  Use 28px as a guess (titlebar + bottom window border)
> -        defaultHeight -= 28;
> +          defaultHeight -= 28;
> -#endif

I'm just leave this like this here because this is going away in bug 384336 anyway

Comment 57

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8693677 [details]
MozReview Request: Bug 1228655 - Remove ifdefs for gEditUIVisible. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26533/diff/1-2/

Comment 58

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8693678 [details]
MozReview Request: Bug 1228655 - Remove ifdef for Win7Features. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26535/diff/1-2/

Comment 59

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8693679 [details]
MozReview Request: Bug 1228655 - Remove ifdef for macBrowserOverlay functions. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26537/diff/1-2/

Comment 60

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8693680 [details]
MozReview Request: Bug 1228655 - Remove ifdefs for e10s testing UI. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26539/diff/1-2/

Comment 61

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8693681 [details]
MozReview Request: Bug 1228655 - Move TabsInTitlebar to its own file to remove ifdefs. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26541/diff/1-2/

Comment 62

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8693682 [details]
MozReview Request: Bug 1228655 - Remove ifdefs for MOZ_SAFE_BROWSING. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26543/diff/1-2/

Comment 63

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8693683 [details]
MozReview Request: Bug 1228655 - Remove ifdefs for MOZ_CRASHREPORTER. r=mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26545/diff/1-2/

Comment 64

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8693684 [details]
MozReview Request: Bug 1228655 - Remove ifdefs for data/health reporting. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26547/diff/1-2/

Comment 65

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8693686 [details]
MozReview Request: Bug 1228655 - Remove ifdefs for OSX non-browser-window behavior. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26549/diff/1-2/

Comment 66

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8693687 [details]
MozReview Request: Bug 1228655 - Remove ifdefs for generic platform-specific behavior. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26551/diff/1-2/

Comment 67

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8693689 [details]
MozReview Request: Bug 1228655 - Remove ifdefs for MOZ_WIDGET_GTK. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26553/diff/1-2/

Comment 68

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8693690 [details]
MozReview Request: Bug 1228655 - Remove ifdefs in aboutDialog.js. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26555/diff/1-2/

Comment 69

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8693691 [details]
MozReview Request: Bug 1228655 - Remove ifdefs in browser-devedition.js. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26557/diff/1-2/

Comment 70

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8693692 [details]
MozReview Request: Bug 1228655 - Remove ifdefs in browser-fullscreen.js. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26559/diff/1-2/

Comment 71

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8693693 [details]
MozReview Request: Bug 1228655 - Remove ifdefs in browser-gesturesupport.js. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26561/diff/1-2/

Comment 72

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8693694 [details]
MozReview Request: Bug 1228655 - Remove ifdefs in browser-places.js. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26563/diff/1-2/

Comment 73

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8693695 [details]
MozReview Request: Bug 1228655 - Remove ifdefs of MOZ_SERVICES_CLOUDSYNC. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26565/diff/1-2/

Comment 74

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8693696 [details]
MozReview Request: Bug 1228655 - Remove ifdefs from utilityOverlay. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26567/diff/1-2/

Comment 75

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8693697 [details]
MozReview Request: Bug 1228655 - Don't preprocess browser.js r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26569/diff/1-2/

Comment 76

[:Felipe Gomes (needinfo for replies!)]

https://hg.mozilla.org/try/pushloghtml?changeset=09e35e410240

Comment 77

[:Gijs (he/him)]

Comment on attachment 8693682 [details]
MozReview Request: Bug 1228655 - Remove ifdefs for MOZ_SAFE_BROWSING. r=Gijs

https://reviewboard.mozilla.org/r/26543/#review24151

Comment 78

[:Gijs (he/him)]

Comment on attachment 8693690 [details]
MozReview Request: Bug 1228655 - Remove ifdefs in aboutDialog.js. r=Gijs

https://reviewboard.mozilla.org/r/26555/#review24153

::: browser/base/content/aboutDialog.js:71
(Diff revision 2)
> -#ifdef MOZ_UPDATER
> +if (AppConstants.MOZ_UPDATER) {

600-line if block... :-(

We could factor this out into a new file with hg copy, which would preserve blame and be less awkward?

r=me either way.

Comment 79

[:Gijs (he/him)]

(In reply to :Felipe Gomes from comment #53)
> https://reviewboard.mozilla.org/r/26551/#review24063
> 
> ::: browser/base/content/browser.js
> (Diff revision 1)
> > -#ifdef XP_MACOSX
> >    "print-button": "printButton.tooltip",
> > -#endif
> 
> this appears to be just a mapping of nodes to tooltip names, but
> UpdateDynamicShortcutTooltipText still needs to be called with the
> corresponding triggerNode for it to be used. So the ifdef was just to avoid
> shipping this line? (and yet shipping a longer `//@line ...` comment).
> 
> I've changed that anyways in case there's something I've overlooked.


https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.xul#1031

it's the same button but it does different things and only has a corresponding shortcut on OS X.

Comment 80

[:Gijs (he/him)]

Comment on attachment 8693689 [details]
MozReview Request: Bug 1228655 - Remove ifdefs for MOZ_WIDGET_GTK. r=Gijs

https://reviewboard.mozilla.org/r/26553/#review24155

::: browser/base/content/browser.js:1023
(Diff revision 2)
> -#if MOZ_WIDGET_GTK == 2
> +        if (AppConstants.platform == "linux") {

Indeed, this is going away in bug 384336 - but that's stalled on a valgrind leak, and the current default is gtk3, so I think that to not regress the appearance here we should just remove the block entirely. That will trivially bitrot that bug, but that doesn't seem like a major issue.

Comment 81

[:Felipe Gomes (needinfo for replies!)]

https://reviewboard.mozilla.org/r/26555/#review24153

> 600-line if block... :-(
> 
> We could factor this out into a new file with hg copy, which would preserve blame and be less awkward?
> 
> r=me either way.

I hg copied the file to a new aboutDialog-appUpdater.js file that contains the #ifdef'ed part, and then that file is included in aboutDialog.xul if MOZ_UPDATER is defined.

Comment 82

[:Felipe Gomes (needinfo for replies!)]

(In reply to :Gijs Kruitbosch from comment #50)
> (In reply to :Felipe Gomes from comment #32)
> > https://reviewboard.mozilla.org/r/26545/#review23959
> > 
> > > Out of curiosity, is there a reason you're wholesale removing these ifdef's, as opposed to replacing them with
> > > 
> > > ```JavaScript
> > > 
> > > if (AppConstants.MOZ_CRASHREPORTER) {
> > >   // ...
> > > }
> > > ```
> > > ?
> > 
> > As they are defineLazyModuleGetter, it's a conditional declaration anyways
> > that is only gonna run the Cu.import if PluginCrashReporter is accessed,
> > which supposedly shouldn't happen if MOZ_CRASHREPORTER isn't defined. The
> > #ifdef only helps with not shipping the code, but if it's gonna ship anyway
> > it seemed this was just a double check for the same thing.  I could be wrong
> > though, so that's why I tagged you here for a second set of eyes.
> 
> Commenting because this happened with the safebrowsing as well: I believe
> that any access of this variable, including by e.g. devtools enumerating
> window properties, or by people doing:
> 
> if (window.CrashReporter/SafeBrowsing/whatever)
> 
> will try to load the module lazily, and that doing so will throw an
> exception if the module doesn't exist. So we should be careful about this.


I decided to go with the safe approach and added an AppConstants check for the CrashReporter blocks

Comment 83

[:Felipe Gomes (needinfo for replies!)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=eaea8337bef9

Comment 84

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/5a116951e80b
https://hg.mozilla.org/integration/fx-team/rev/f6c5de47ca51
https://hg.mozilla.org/integration/fx-team/rev/e7edd3c43b7f
https://hg.mozilla.org/integration/fx-team/rev/5d616c187a52
https://hg.mozilla.org/integration/fx-team/rev/71d9d0052975
https://hg.mozilla.org/integration/fx-team/rev/4cb70c595fb9
https://hg.mozilla.org/integration/fx-team/rev/9d7d0981904c
https://hg.mozilla.org/integration/fx-team/rev/6d9977931931
https://hg.mozilla.org/integration/fx-team/rev/41e1b8182742
https://hg.mozilla.org/integration/fx-team/rev/dc3ea89ac257
https://hg.mozilla.org/integration/fx-team/rev/5230f2e66808
https://hg.mozilla.org/integration/fx-team/rev/775db0544540
https://hg.mozilla.org/integration/fx-team/rev/c10f8438cbe5
https://hg.mozilla.org/integration/fx-team/rev/9b7b05751801
https://hg.mozilla.org/integration/fx-team/rev/01cb755b65e4
https://hg.mozilla.org/integration/fx-team/rev/663ccc0ea863
https://hg.mozilla.org/integration/fx-team/rev/93f91765c188
https://hg.mozilla.org/integration/fx-team/rev/e7f7e3e691d1
https://hg.mozilla.org/integration/fx-team/rev/6784553acb42

Comment 85

[Nick Alexander :nalexander [he/him]]

Felipe, Gijs -- this is awesome.  Can I suggest a post to firefox-dev?

Comment 86

[:Felipe Gomes (needinfo for replies!)]

(In reply to Nick Alexander :nalexander from comment #85)
> Felipe, Gijs -- this is awesome.  Can I suggest a post to firefox-dev?

Posted!

Comment 87

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5a116951e80b
https://hg.mozilla.org/mozilla-central/rev/f6c5de47ca51
https://hg.mozilla.org/mozilla-central/rev/e7edd3c43b7f
https://hg.mozilla.org/mozilla-central/rev/5d616c187a52
https://hg.mozilla.org/mozilla-central/rev/71d9d0052975
https://hg.mozilla.org/mozilla-central/rev/4cb70c595fb9
https://hg.mozilla.org/mozilla-central/rev/9d7d0981904c
https://hg.mozilla.org/mozilla-central/rev/6d9977931931
https://hg.mozilla.org/mozilla-central/rev/41e1b8182742
https://hg.mozilla.org/mozilla-central/rev/dc3ea89ac257
https://hg.mozilla.org/mozilla-central/rev/5230f2e66808
https://hg.mozilla.org/mozilla-central/rev/775db0544540
https://hg.mozilla.org/mozilla-central/rev/c10f8438cbe5
https://hg.mozilla.org/mozilla-central/rev/9b7b05751801
https://hg.mozilla.org/mozilla-central/rev/01cb755b65e4
https://hg.mozilla.org/mozilla-central/rev/663ccc0ea863
https://hg.mozilla.org/mozilla-central/rev/93f91765c188
https://hg.mozilla.org/mozilla-central/rev/e7f7e3e691d1
https://hg.mozilla.org/mozilla-central/rev/6784553acb42