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

RESOLVED FIXED in seamonkey2.1final

Status

SeaMonkey
Preferences
RESOLVED FIXED
10 years ago
7 years ago

People

(Reporter: rstrong, Assigned: ewong)

Tracking

Trunk
seamonkey2.1final

SeaMonkey Tracking Flags

(seamonkey2.1 wanted)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Created attachment 364656 [details] [diff] [review]
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.
Attachment #364656 - Flags: review?(bugzilla)

Comment 1

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

10 years ago
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.
Attachment #364656 - Attachment is obsolete: true
Attachment #364656 - Flags: review?(bugzilla)
(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,
Assignee: robert.bugzilla → nobody
Duplicate of this bug: 477193
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.
Depends on: 464791
Depends on: 471219
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.
No longer depends on: 471219
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

9 years ago
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.
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

Updated

8 years ago
Duplicate of this bug: 503264
status-seamonkey2.1: --- → ?
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.
status-seamonkey2.1: ? → wanted
Whiteboard: [good first bug]
(Assignee)

Updated

7 years ago
Assignee: nobody → ewong
Status: NEW → ASSIGNED
(Assignee)

Comment 12

7 years ago
Created attachment 526589 [details] [diff] [review]
Added ifdef MOZ_UPDATER for the app update preferences UI.
Attachment #526589 - Flags: review?(bugspam.Callek)
(Assignee)

Comment 13

7 years ago
Created attachment 526905 [details] [diff] [review]
Add ifdenf MOZ_UPDATER for the app update preferences UI and Help Menu
Attachment #526589 - Attachment is obsolete: true
Attachment #526905 - Flags: review?(bugspam.Callek)
Attachment #526589 - Flags: review?(bugspam.Callek)
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

7 years ago
> +#ifdef MOZ_UPDATER
>    UpdateAppItems();
> +#endif
Please Read comment 8 and comment 9. Hint nsIApplicationUpdateService.
Attachment #526905 - Flags: review?(bugspam.Callek) → feedback-
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
(Assignee)

Comment 17

7 years ago
Created attachment 527924 [details] [diff] [review]
Add ifdef MOZ_UPDATER for the app update preferences.
Attachment #526905 - Attachment is obsolete: true
Attachment #527924 - Flags: review?(bugspam.Callek)
(Assignee)

Updated

7 years ago
Attachment #527924 - Flags: review?(neil)
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
Attachment #527924 - Flags: review?(bugspam.Callek) → review+
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.
Attachment #527924 - Flags: feedback-
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.
Attachment #527924 - Flags: review?(neil) → review-
(Assignee)

Comment 21

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

Comment 22

7 years ago
Created attachment 528548 [details] [diff] [review]
if --disable-updater is specified, then app update preferences UI are hidden. [Checkin: comment 25]
Attachment #527924 - Attachment is obsolete: true
Attachment #528548 - Flags: review?(bugspam.Callek)
(Assignee)

Updated

7 years ago
Attachment #528548 - Flags: review?(neil)

Updated

7 years ago
Attachment #528548 - Flags: review?(neil) → review+
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...
Attachment #528548 - Flags: feedback+
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.
Attachment #528548 - Flags: review?(bugspam.Callek)
(Assignee)

Updated

7 years ago
Keywords: checkin-needed
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
Attachment #528548 - Attachment description: if --disable-updater is specified, then app update preferences UI are hidden. → if --disable-updater is specified, then app update preferences UI are hidden. [Checkin: comment 25]
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1final
Whiteboard: [good first bug]
You need to log in before you can comment on or make changes to this bug.