Closed Bug 480696 Opened 11 years ago Closed 9 years ago

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

Categories

(SeaMonkey :: Preferences, defect)

defect
Not set

Tracking

(seamonkey2.1 wanted)

RESOLVED FIXED
seamonkey2.1final
Tracking Status
seamonkey2.1 --- wanted

People

(Reporter: rstrong, Assigned: ewong)

References

Details

Attachments

(1 file, 4 obsolete files)

Attached patch patch rev1 (obsolete) — Splinter Review
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)
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).
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
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.
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.
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
Duplicate of this bug: 503264
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.
Whiteboard: [good first bug]
Assignee: nobody → ewong
Status: NEW → ASSIGNED
Attachment #526589 - Flags: review?(bugspam.Callek)
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.)
> +#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
Attachment #526905 - Attachment is obsolete: true
Attachment #527924 - Flags: review?(bugspam.Callek)
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-
(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.
Attachment #528548 - Flags: review?(neil)
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)
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
Closed: 9 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.