480696 - Add ifdef MOZ_UPDATER for the app update preferences UI and Help menu

Status: RESOLVED FIXED

(SeaMonkey :: Preferences, defect)

Description

[Robert Strong (they/them - no direct email)]

Attachment: patch rev1

Spinoff of bug 464791 for SeaMonkey... Bug 477192 is the Thunderbird bug.

If SeaMonkey is compiled with --disable-updater the app update preferences UI should not be displayed. This simplifies the work Linux distros have to do since they don't use app update and don't want to display this ui.

Comment 1

[Frank Wein [:mcsmurf]]

Not sure if SeaMonkey wants the patch like that as it breaks building with flat chrome (symlinks) for the files which are preprocessed. Do you know if there is a reason why nsUpdateService.js is still built/included when the updater is disabled? Otherwise one could check if the "@mozilla.org/updates/update-service;1" CID is present and based on that display/hide the UI (I would update the patch for that then).

Comment 2

[Frank Wein [:mcsmurf]]

Actually, see http://mxr.mozilla.org/comm-central/search?string=@mozilla.org/updates/updat. We would probably need to check that all app parts have #ifdef where needed.

Comment 3

[Robert Strong (they/them - no direct email)]

(In reply to comment #2)
> Actually, see
> http://mxr.mozilla.org/comm-central/search?string=@mozilla.org/updates/updat.
> We would probably need to check that all app parts have #ifdef where needed.
Right but that can come later and there really isn't that much more to do.

Reassigning to default since I doubt I will have time to finish this up any time soon,

Comment 5

[Robert Strong (they/them - no direct email)]

Just a heads up. Bug 471219 which added the ifdef's to the remaining code that needed this except for the SeaMonkey pref UI and Help menu which this bug is about landed today on trunk and will likely land for 1.9.2 as well.

Comment 6

[Robert Strong (they/them - no direct email)]

Does not depend on bug 471219... for that matter it doesn't really depend on bug 464791. We've always had MOZ_UPDATER and apps chose to always show the ui even though it would never work 100% since the updater binary wasn't present.

Comment 7

[Robert Strong (they/them - no direct email)]

Serge, there is an obsoleted patch in this bug that doesn't conform to the SeaMonkey requirement for flat chrome... this is what all of the other apps did to accomplish this and SeaMonkey will need to do a one-off to support flat chrome.

Comment 8

[Robert Kaiser]

Rob, is there a way to check from JS if updater is present? We usually like to use such checks as we can circumvent ifdefs with them, and preprocessed chrome breaks symlinked chrome and rapid development/testing cycles for a number of people.

Comment 9

[Robert Strong (they/them - no direct email)]

For 1.9.2 you can check for nsIApplicationUpdateService

The only way I can think of for 1.9.1 is to check for the existence of the update.locale file in a manner similar to the following.
http://mxr.mozilla.org/mozilla1.9.1/source/toolkit/mozapps/update/src/nsUpdateService.js.in#529

Comment 11

[Justin Wood (:Callek)]

Given the reasons in c#0 and the solution to do this in JS in c#8/9 I'd love to have this in 2.1

I don't see the benefit to do it in 2.0 [Gecko 1.9.1] at this stage.

I also will NOT want to block on this.

Comment 12

[Edmund Wong (:ewong)]

Attachment: Added ifdef MOZ_UPDATER for the app update preferences UI.

Comment 13

[Edmund Wong (:ewong)]

Attachment: Add ifdenf MOZ_UPDATER for the app update preferences UI and Help Menu

Comment 14

[Serge Gautherie (:sgautherie)]

Comment on attachment 526905 [details] [diff] [review]
Add ifdenf MOZ_UPDATER for the app update preferences UI and Help Menu

>diff --git a/mozconfig-extra b/mozconfig-extra

Probably not part of this bug.

>diff --git a/suite/Makefile.in b/suite/Makefile.in

(Is this part actually needed? Just wondering.)

Comment 15

[Philip Chee]

> +#ifdef MOZ_UPDATER
>    UpdateAppItems();
> +#endif
Please Read comment 8 and comment 9. Hint nsIApplicationUpdateService.

Comment 16

[Serge Gautherie (:sgautherie)]

Comment on attachment 364656 [details] [diff] [review]
patch rev1

>+#else
>+  // Some extensions may rely on the menuseparator being present so only hide it
>+  // when there are no elements besides the check for updates menuitem between
>+  // the two separators.
>+  var updatesSeparator = document.getElementById("menu_HelpUpdatesSeparator");
>+  var checkForUpdates = document.getElementById("checkForUpdates");
>+  if (updatesSeparator.nextSibling === checkForUpdates &&
>+      checkForUpdates.nextSibling.nodeName == "menuseparator")
>+    updatesSeparator.hidden = true;
>+#endif

Just to be explicit, don't we want this part too?
http://mxr.mozilla.org/comm-central/search?string=updatesSeparator&case=on

Comment 17

[Edmund Wong (:ewong)]

Attachment: Add ifdef MOZ_UPDATER for the app update preferences.

Comment 18

[Justin Wood (:Callek)]

Comment on attachment 527924 [details] [diff] [review]
Add ifdef MOZ_UPDATER for the app update preferences.

Review of attachment 527924 [details] [diff] [review]:

Looks good overall, few nits. Have not looked too deeply if there are other parts of this we need to patch (stayed out of MXR) so leaving neil's review request intact.

::: suite/common/pref/pref-smartupdate.js
@@ +41,5 @@
 var gCanCheckForUpdates;
 
 function Startup()
 {
+  var updater = ("nsIApplicationUpdateService" in Components.interfaces);

nit: var hasUpdater

::: suite/common/utilityOverlay.js
@@ +686,5 @@
 }
 
 function updateCheckUpdatesItem()
 {
+  var updater = ("nsIApplicationUpdateService" in Components.interfaces);

Nit: (again) hasUpdater

Comment 19

[Serge Gautherie (:sgautherie)]

Comment on attachment 527924 [details] [diff] [review]
Add ifdef MOZ_UPDATER for the app update preferences.

Review of attachment 527924 [details] [diff] [review]:

Is this patch actually working? (Maybe I'm too tired to have a quick look at it, but see below...)
Ftr, why didn't you include the help part anymore?

::: suite/common/Makefile.in
@@ +63,5 @@
            $(NULL)
 
+ifdef MOZ_UPDATER
+DEFINES += -DMOZ_UPDATER=1
+endif

This isn't needed anymore.

::: suite/common/utilityOverlay.js
@@ +688,5 @@
 function updateCheckUpdatesItem()
 {
+  var updater = ("nsIApplicationUpdateService" in Components.interfaces);
+  var checkForUpdates = document.getElementById("checkForUpdates");
+  var updateSeparator = document.getElementById("updateSeparator");

'updateSeparator' should be inside next 'if()'.

@@ +692,5 @@
+  var updateSeparator = document.getElementById("updateSeparator");
+
+  if (!updater)
+  {
+    checkForUpdates.setAttribute("hidden", !updates);

This case should be an additional one, but not replace existing one.

@@ +693,5 @@
+
+  if (!updater)
+  {
+    checkForUpdates.setAttribute("hidden", !updates);
+    updateSeparator.setAttribute("hidden", !updates);

'updates' isn't defined yet.

Comment 20

[[e-mail]]

Comment on attachment 527924 [details] [diff] [review]
Add ifdef MOZ_UPDATER for the app update preferences.

Review of attachment 527924 [details] [diff] [review]:

::: suite/common/pref/pref-smartupdate.js
@@ +41,5 @@
 var gCanCheckForUpdates;
 
 function Startup()
 {
+  var updater = ("nsIApplicationUpdateService" in Components.interfaces);

Nit: don't need the ()s.

@@ +48,5 @@
+  {
+    var aus = Components.classes["@mozilla.org/updates/update-service;1"]
+                        .getService(Components.interfaces.nsIApplicationUpdateService);
+    gCanCheckForUpdates = aus.canCheckForUpdates;
+  

Nit: trailing whitespace.

::: suite/common/utilityOverlay.js
@@ +686,5 @@
 }
 
 function updateCheckUpdatesItem()
 {
+  var updater = ("nsIApplicationUpdateService" in Components.interfaces);

Nit: (again) don't need the ()s.

@@ +693,5 @@
+
+  if (!updater)
+  {
+    checkForUpdates.setAttribute("hidden", !updates);
+    updateSeparator.setAttribute("hidden", !updates);

This probably should just say checkForUpdates.hidden = true; etc.

@@ -698,2 @@
   var canCheckForUpdates = updates.canCheckForUpdates;
-  checkForUpdates.setAttribute("disabled", !canCheckForUpdates);

This change makes no sense.

Comment 21

[Edmund Wong (:ewong)]

(In reply to comment #19)
> Comment on attachment 527924 [details] [diff] [review]
> Add ifdef MOZ_UPDATER for the app update preferences.
> 
> Review of attachment 527924 [details] [diff] [review]:
> 
> Is this patch actually working? (Maybe I'm too tired to have a quick look at
> it, but see below...)
> Ftr, why didn't you include the help part anymore?


Thanks for the feedback, Serge.  

The reason why the help part isn't added was because that I was under 
the impression that disabling the help part was unnecessary.  I had a 
chat with Neil over on IRC, and understood that since this bug is 
about making a 'custom' build, disabling the help would not be
necessary.  Now perhaps I understood it wrongly.  If so, someone
can correct my misunderstanding.

Comment 22

[Edmund Wong (:ewong)]

Attachment: if --disable-updater is specified, then app update preferences UI are hidden. [Checkin: comment 25]

Comment 23

[Serge Gautherie (:sgautherie)]

Comment on attachment 528548 [details] [diff] [review]
if --disable-updater is specified, then app update preferences UI are hidden. [Checkin: comment 25]

Review of attachment 528548 [details] [diff] [review]:

(In reply to comment #21)

> chat with Neil over on IRC, and understood that since this bug is 
> about making a 'custom' build, disabling the help would not be
> necessary.

If noone is building on top of SeaMonkey (i.e. Linux distros), then I agree developers shouldn't care about the extra help.

> Now perhaps I understood it wrongly.  If so, someone
> can correct my misunderstanding.

I believe you understood correctly, I was just curious.

::: suite/common/pref/pref-smartupdate.js
@@ +43,3 @@
 function Startup()
 {
+  var hasUpdater = "nsIApplicationUpdateService" in Components.interfaces;

Nit: here and elsewhere, I wouldn't add a var line when not required, but if reviewers are happy then fine...

::: suite/common/utilityOverlay.js
@@ +694,5 @@
+  {
+    var updateSeparator = document.getElementById("updateSeparator");
+
+    checkForUpdates.hidden = true;
+    updateSeparator.hidden = true;

Nit: I still wonder about doing it more like TB (comment 16)... but at least this should work (usually/ftb).

@@ +709,3 @@
   var canCheckForUpdates = updates.canCheckForUpdates;
   checkForUpdates.setAttribute("disabled", !canCheckForUpdates);
+

Nit: I'm not sure this blank line helps, but...

Comment 24

[Justin Wood (:Callek)]

Comment on attachment 528548 [details] [diff] [review]
if --disable-updater is specified, then app update preferences UI are hidden. [Checkin: comment 25]

Neil's review is enough here.

Comment 25

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 528548 [details] [diff] [review]
if --disable-updater is specified, then app update preferences UI are hidden. [Checkin: comment 25]

http://hg.mozilla.org/comm-central/rev/6d6c201242f3
http://hg.mozilla.org/releases/comm-2.0/rev/aec836bb20d0