Replace #ifdefs in browser code by AppConstants

RESOLVED FIXED in Firefox 45

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Felipe, Assigned: Felipe)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(19 attachments)

40 bytes, text/x-review-board-request
Gijs
: review+
Details
40 bytes, text/x-review-board-request
Gijs
: review+
Details
40 bytes, text/x-review-board-request
Gijs
: review+
Details
40 bytes, text/x-review-board-request
Gijs
: review+
Details
40 bytes, text/x-review-board-request
Gijs
: review+
Details
40 bytes, text/x-review-board-request
Gijs
: review+
Details
40 bytes, text/x-review-board-request
mconley
: review+
Details
40 bytes, text/x-review-board-request
Gijs
: review+
Details
40 bytes, text/x-review-board-request
Gijs
: review+
Details
40 bytes, text/x-review-board-request
Gijs
: review+
Details
40 bytes, text/x-review-board-request
Gijs
: review+
Details
40 bytes, text/x-review-board-request
Gijs
: review+
Details
40 bytes, text/x-review-board-request
Gijs
: review+
Details
40 bytes, text/x-review-board-request
Gijs
: review+
Details
40 bytes, text/x-review-board-request
Gijs
: review+
Details
40 bytes, text/x-review-board-request
Gijs
: review+
Details
40 bytes, text/x-review-board-request
Gijs
: review+
Details
40 bytes, text/x-review-board-request
Gijs
: review+
Details
40 bytes, text/x-review-board-request
Gijs
: review+
Details
(Assignee)

Description

3 years ago
The companion bug to bug 1228627, in order to remove pre-processing from browser/
(Assignee)

Updated

3 years ago
Assignee: nobody → felipc
Status: NEW → ASSIGNED

Updated

3 years ago
Blocks: 1150859
(Assignee)

Comment 1

3 years ago
Created attachment 8693677 [details]
MozReview Request: Bug 1228655 - Remove ifdefs for gEditUIVisible. r=Gijs

Bug 1228655 - Remove ifdefs for gEditUIVisible. r=Gijs
Attachment #8693677 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 2

3 years ago
Created attachment 8693678 [details]
MozReview Request: Bug 1228655 - Remove ifdef for Win7Features. r=Gijs

Bug 1228655 - Remove ifdef for Win7Features. r=Gijs
Attachment #8693678 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 3

3 years ago
Created attachment 8693679 [details]
MozReview Request: Bug 1228655 - Remove ifdef for macBrowserOverlay functions. r=Gijs

Bug 1228655 - Remove ifdef for macBrowserOverlay functions. r=Gijs
Attachment #8693679 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 4

3 years ago
Created 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
Attachment #8693680 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 5

3 years ago
Created 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.
Attachment #8693681 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 6

3 years ago
Created 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
Attachment #8693682 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 7

3 years ago
Created 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
Attachment #8693683 - Flags: review?(mconley)
(Assignee)

Comment 8

3 years ago
Created 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
Attachment #8693684 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 9

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

Bug 1228655 - Remove ifdefs for gEditUIVisible. r=Gijs
(Assignee)

Comment 10

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

Bug 1228655 - Remove ifdef for Win7Features. r=Gijs
(Assignee)

Comment 11

3 years ago
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
(Assignee)

Comment 12

3 years ago
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
(Assignee)

Comment 13

3 years ago
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.
(Assignee)

Comment 14

3 years ago
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
(Assignee)

Comment 15

3 years ago
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
(Assignee)

Comment 16

3 years ago
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
(Assignee)

Comment 17

3 years ago
Created attachment 8693686 [details]
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
Attachment #8693686 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 18

3 years ago
Created attachment 8693687 [details]
MozReview Request: Bug 1228655 - Remove ifdefs for generic platform-specific behavior. r=Gijs

Bug 1228655 - Remove ifdefs for generic platform-specific behavior. r=Gijs
Attachment #8693687 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 19

3 years ago
Created attachment 8693689 [details]
MozReview Request: Bug 1228655 - Remove ifdefs for MOZ_WIDGET_GTK. r=Gijs

Bug 1228655 - Remove ifdefs for MOZ_WIDGET_GTK. r=Gijs
Attachment #8693689 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 20

3 years ago
Created attachment 8693690 [details]
MozReview Request: Bug 1228655 - Remove ifdefs in aboutDialog.js. r=Gijs

Bug 1228655 - Remove ifdefs in aboutDialog.js. r=Gijs
Attachment #8693690 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 21

3 years ago
Created attachment 8693691 [details]
MozReview Request: Bug 1228655 - Remove ifdefs in browser-devedition.js. r=Gijs

Bug 1228655 - Remove ifdefs in browser-devedition.js. r=Gijs
Attachment #8693691 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 22

3 years ago
Created attachment 8693692 [details]
MozReview Request: Bug 1228655 - Remove ifdefs in browser-fullscreen.js. r=Gijs

Bug 1228655 - Remove ifdefs in browser-fullscreen.js. r=Gijs
Attachment #8693692 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 23

3 years ago
Created attachment 8693693 [details]
MozReview Request: Bug 1228655 - Remove ifdefs in browser-gesturesupport.js. r=Gijs

Bug 1228655 - Remove ifdefs in browser-gesturesupport.js. r=Gijs
Attachment #8693693 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 24

3 years ago
Created attachment 8693694 [details]
MozReview Request: Bug 1228655 - Remove ifdefs in browser-places.js. r=Gijs

Bug 1228655 - Remove ifdefs in browser-places.js. r=Gijs
Attachment #8693694 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 25

3 years ago
Created attachment 8693695 [details]
MozReview Request: Bug 1228655 - Remove ifdefs of MOZ_SERVICES_CLOUDSYNC. r=Gijs

Bug 1228655 - Remove ifdefs of MOZ_SERVICES_CLOUDSYNC. r=Gijs
Attachment #8693695 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 26

3 years ago
Created attachment 8693696 [details]
MozReview Request: Bug 1228655 - Remove ifdefs from utilityOverlay. r=Gijs

Bug 1228655 - Remove ifdefs from utilityOverlay. r=Gijs
Attachment #8693696 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 27

3 years ago
Created attachment 8693697 [details]
MozReview Request: Bug 1228655 - Don't preprocess browser.js r=Gijs

Bug 1228655 - Don't preprocess browser.js r=Gijs
Attachment #8693697 - Flags: review?(gijskruitbosch+bugs)

Comment 28

3 years ago
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 :-)
Attachment #8693677 - Flags: review?(gijskruitbosch+bugs) → review+
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) {
  // ...
}
```
?
Attachment #8693683 - Flags: review?(mconley)

Comment 30

3 years ago
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.
Attachment #8693678 - Flags: review?(gijskruitbosch+bugs) → review+

Updated

3 years ago
Attachment #8693679 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 31

3 years ago
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?
(Assignee)

Comment 32

3 years ago
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

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

https://reviewboard.mozilla.org/r/26539/#review23967
Attachment #8693680 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Comment 34

3 years ago
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?

Updated

3 years ago
Attachment #8693684 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 35

3 years ago
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?
(Assignee)

Comment 36

3 years ago
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

3 years ago
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.
Attachment #8693682 - Flags: review?(gijskruitbosch+bugs)

Comment 38

3 years ago
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"
     || ( ...) ;
Attachment #8693686 - Flags: review?(gijskruitbosch+bugs) → review+

Updated

3 years ago
Attachment #8693687 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 39

3 years ago
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

3 years ago
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.
Attachment #8693689 - Flags: review?(gijskruitbosch+bugs)

Comment 41

3 years ago
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.
Attachment #8693690 - Flags: review?(gijskruitbosch+bugs)

Comment 42

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

https://reviewboard.mozilla.org/r/26557/#review23991
Attachment #8693691 - Flags: review?(gijskruitbosch+bugs) → review+

Updated

3 years ago
Attachment #8693692 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 43

3 years ago
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

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

https://reviewboard.mozilla.org/r/26561/#review23997
Attachment #8693693 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 45

3 years ago
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.
Attachment #8693694 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 46

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

https://reviewboard.mozilla.org/r/26565/#review24003
Attachment #8693695 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 47

3 years ago
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";
}
Attachment #8693696 - Flags: review?(gijskruitbosch+bugs)

Comment 48

3 years ago
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...
Attachment #8693697 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 49

3 years ago
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.
Attachment #8693696 - Flags: review+

Comment 50

3 years ago
(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

3 years ago
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!
Attachment #8693681 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Comment 52

3 years ago
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..
(Assignee)

Comment 53

3 years ago
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.
(Assignee)

Comment 54

3 years ago
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 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. :)
Attachment #8693683 - Flags: review+
(Assignee)

Comment 56

3 years ago
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
(Assignee)

Comment 57

3 years ago
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/
(Assignee)

Comment 58

3 years ago
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/
(Assignee)

Comment 59

3 years ago
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/
(Assignee)

Comment 60

3 years ago
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/
(Assignee)

Comment 61

3 years ago
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/
(Assignee)

Comment 62

3 years ago
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/
Attachment #8693682 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 63

3 years ago
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/
(Assignee)

Comment 64

3 years ago
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/
(Assignee)

Comment 65

3 years ago
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/
(Assignee)

Comment 66

3 years ago
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/
(Assignee)

Comment 67

3 years ago
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/
Attachment #8693689 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 68

3 years ago
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/
Attachment #8693690 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 69

3 years ago
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/
(Assignee)

Comment 70

3 years ago
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/
(Assignee)

Comment 71

3 years ago
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/
(Assignee)

Comment 72

3 years ago
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/
(Assignee)

Comment 73

3 years ago
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/
(Assignee)

Comment 74

3 years ago
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/
(Assignee)

Comment 75

3 years ago
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 77

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

https://reviewboard.mozilla.org/r/26543/#review24151
Attachment #8693682 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 78

3 years ago
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.
Attachment #8693690 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 79

3 years ago
(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

3 years ago
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.
Attachment #8693689 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Comment 81

3 years ago
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.
(Assignee)

Comment 82

3 years ago
(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
Felipe, Gijs -- this is awesome.  Can I suggest a post to firefox-dev?
(Assignee)

Comment 86

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

Posted!
You need to log in before you can comment on or make changes to this bug.