Closed Bug 341971 Opened 18 years ago Closed 18 years ago

Enable app update checking for Sunbird (aus)

Categories

(Calendar :: Sunbird Only, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
Sunbird 0.5

People

(Reporter: mattwillis, Assigned: mattwillis)

References

Details

Attachments

(5 files, 2 obsolete files)

Enable app update checking for Sunbird, so users can get security updates, etc.
Status: NEW → ASSIGNED
This isn't making Sunbird 0.3. Moving to later.
Target Milestone: Sunbird 0.3 → Sunbird 0.4
Blocks: 318927
Depends on: 343207
This patch includes all the app-side infrastructure necessary for AUS updates.

We will need to make changes to each tinderbox's mozconfig and tinder-config.pl so that they generate .mar updates, and I'm sure there's more involved from #build to get the XML hosted and updated properly.
Attachment #244028 - Flags: first-review?(cmtalbert)
This mirrors Firefox and Thunderbird's UI
Attachment #244029 - Flags: ui-review?(mvl)
Tinderbox-side changes to enable building .mar archives for Sb-Trunk.
Attachment #244030 - Flags: first-review?(rhelmer)
Comment on attachment 244028 [details] [diff] [review]
Adds app-side infrastructure for dealing with AUS updates

This looks good to me. There seems to be a toolkit issue caused by the code at: http://lxr.mozilla.org/mozilla/source/toolkit/mozapps/update/content/updates.xml#180

It exhibits like this: When you click "Check for Updates" from the help menu, this error message appears in the Error Console: Error: this.docShell has no properties
Source File: chrome://global/content/bindings/browser.xml

There are two possibilities:
1. We are using the updater incorrectly (which doesn't seem to be the case)
2. There is a bug in the toolkit updater component when there is no license specified (note there are no updates filed for Sunbird when I was testing this). So, r+ with a note that lilmatt will decide how to handle this toolkit issue.
Attachment #244028 - Flags: first-review?(cmtalbert) → first-review+
(In reply to comment #5)
> There are two possibilities:
> 2. There is a bug in the toolkit updater component when there is no license
> specified (note there are no updates filed for Sunbird when I was testing
> this). So, r+ with a note that lilmatt will decide how to handle this toolkit
> issue.

Trunk Thunderbird also has this issue, so it indeed is toolkit.  I opened bug 358761 for it.
Attachment #244028 - Flags: second-review?(jminta)
Attachment #244030 - Flags: first-review?(rhelmer) → first-review+
Comment on attachment 244030 [details] [diff] [review]
tinderbox changes to enable building .mar archives for Sb-Trunk (checked in)

Patch checked in on trunk.
Attachment #244030 - Attachment description: tinderbox changes to enable building .mar archives for Sb-Trunk → tinderbox changes to enable building .mar archives for Sb-Trunk (checked in)
Whiteboard: [patch in hand][needs review jminta]
Comment on attachment 244029 [details]
Screenshot of Sunbird's Advanced tab

ui-review=mvl
Attachment #244029 - Flags: ui-review?(mvl) → ui-review+
lilmatt says the Sunbird versions in the near future are 0.4a1 (current) and 0.5, so they'll need to be in the AUS config.

This will hopefully resolve the current situation, where partial updates are generated for Linux but AUS makes no offer (Windows partials should start tomorrow).
Attachment #244453 - Flags: first-review?(morgamic)
Attached patch Fix update config — — Splinter Review
This should fix this error from today's Windows nightly log:

Pushing third-gen update info...
ssh -i /home/cltbld/.ssh/aus cltbld@aus2-staging.mozilla.org mkdir -p /opt/aus2/build/0/Sunbird/trunk/WINNT_x86-msvc /2006110306/en-US
mkdir: cannot create directory `/2006110306': Permission denied
scp -i /home/cltbld/.ssh/aus /cygdrive/d/builds/tinderbox/Sunbird-Trunk/WINNT_5.2_Depend/mozilla/dist/update/update.snippet.1 cltbld@aus2-staging.mozilla.org:/opt/aus2/build/0/Sunbird/trunk/WINNT_x86-msvc /2006110306/en-US/complete.txt
Attachment #244591 - Flags: first-review?(rhelmer)
Attachment #244591 - Flags: first-review?(rhelmer) → first-review+
(In reply to comment #10)
> Created an attachment (id=244591) [edit]
> Fix update config

Thanks, landed:

Checking in sunbird/win32/tinder-config.pl;
/cvsroot/mozilla/tools/tinderbox-configs/sunbird/win32/tinder-config.pl,v  <--  tinder-config.pl
new revision: 1.53; previous revision: 1.52
done
Attachment #244453 - Flags: first-review?(morgamic) → first-review+
Comment on attachment 244453 [details] [diff] [review]
AUS config changes to support Sunbird versions

(it was missing a ' but I fixed that -- in case you were wondering)
Comment on attachment 244028 [details] [diff] [review]
Adds app-side infrastructure for dealing with AUS updates

over to mvl as jminta will be incommunicado for a while due to schoolwork
Attachment #244028 - Flags: second-review?(jminta) → second-review?(mvl)
Depends on: 360025
Comment on attachment 244028 [details] [diff] [review]
Adds app-side infrastructure for dealing with AUS updates

Don't forget to add the new file 'channel-prefs.js' to m/c/installer/windows/packages-static.
Attached patch Adds channel-prefs.js to packages-static (obsolete) — — Splinter Review
(In reply to comment #14)
> (From update of attachment 244028 [details] [diff] [review] [edit])
> Don't forget to add the new file 'channel-prefs.js' to
> m/c/installer/windows/packages-static.
Thanks Stefan.

This patch is the same, except it adds channel-prefs.js to packages-static so that it gets packaged in Windows.

Carrying forward r+ctalbert
Attachment #244028 - Attachment is obsolete: true
Attachment #245050 - Flags: second-review?(jminta)
Attachment #245050 - Flags: first-review+
Attachment #244028 - Flags: second-review?(mvl)
Comment on attachment 245050 [details] [diff] [review]
Adds channel-prefs.js to packages-static

Wow, this patch hurts. a lot.

1.) Judging by my conversations around moco and the looks of this patch, you stole this code from Waldo?  He should probably get mentioned in the contributors section of the license.

+    // UPDATE TAB
+
+    /*
+     * Preferences:
+     *
Two stars start a long comment please.

+     * States:
+     * Element     p   val     locked    Disabled
Let's make it clear that val and locked belong to p, while disabled belongs to Element

+        updateModeLabel.disabled = updateMode.disabled = disable;
Triple assignment is discouraged in calendar code.

+        var enabledPref = document.getElementById("app.update.enabled");
+        var autoPref = document.getElementById("app.update.auto");
+        var modePref = document.getElementById("app.update.mode");
+
+        var warnIncompatible = document.getElementById("warnIncompatible");
+
+        var disable = enabledPref.locked || !enabledPref.value || autoPref.locked ||
+                      !autoPref.value || modePref.locked;
You follow this same pattern several times, and in the words of shaver "We all love our helper functions.  Yes we do.  Each and every one."

I'm thinking function prefImpliesDisabled(aPrefId) {
  var pref = document.getElementById(aPrefId);
  return pref.locked || !pref.disabled;
}

+     * app.update.mode   Checkbox State   Meaning
+     * 0                 Unchecked        Do not warn
...
+        var doNotWarn = preference.value != 0;
The comment and the result of this code line explicitly contradict.  One of them needs to be fixed.

+        gAdvancedPane._modePreference = doNotWarn ? preference.value : 1;
You are gAdvancedPane, so why not just use |this|?

+        return !warnIncompatible.checked ? 0 : gAdvancedPane._modePreference;
Same comment.  Also, the ! inversion seems really odd.  Flip your second and third operands and drop it.

+                        onchange="gAdvancedPane.updateAppUpdateItems();
+                                  gAdvancedPane.updateAutoItems();
+                                  gAdvancedPane.updateModeItems();"/>
You've made this same set of calls twice now.  Everyone together... "We all love our helper functions..."

+                    <vbox id="autoUpdateGroup"
+                          class="indent">
There are starving children in Africa who want that extra line.  No need to separate these.

+                    <separator id="updateSeparator1"/>
+
These separators don't show up in your screenshot.  Are they turned on in winstripe only?

+                        <radio id="ask"
+                               value="false"
There's no reason to set a value attribute when there's already a default pref that's going to be sync'd up onload.

+<!ENTITY pref.calendar.advanced.update.enableAppUpdate.label "&brandShortName;">
You probably want an l10n note here about not translating this.  More to the point, why are we creating another entity that's exactly the same as &brandShortName?

Index: calendar/resources/content/applicationUtil.js
===================================================================
RCS file: /cvsroot/mozilla/calendar/resources/content/applicationUtil.js,v
+/**
+ * Checks for available updates using AUS
+ */
+function checkForUpdates()
+{
We haven't yet broken Lightning's dependency on applicationUtil yet.  That means this could cause nasty problems with scope pollution.

-pref("extensions.update.interval", 86400);
-
+pref("extensions.update.interval", 86400);  // Check for updates to Extensions
+                                            // and Themes every day
 // Non-symmetric (not shared by extensions) extension-specific [update] preferences
-pref("extensions.getMoreExtensionsURL", "chrome://mozapps/locale/extensions/extensions.properties");
-pref("extensions.getMoreThemesURL", "chrome://mozapps/locale/extensions/extensions.properties");
-pref("extensions.dss.enabled", false);          // Dynamic Skin Switching                                               
-pref("extensions.dss.switchPending", false);    // Non-dynamic switch pending after next
-                                                // restart.
It's in everyone's interest to not merge too much stuff into 1 patch.  These changes aren't relevant to the bug here, haven't been discussed, make an already large patch larger, and generally make reviewing harder.
Attachment #245050 - Flags: second-review?(jminta) → second-review-
Attached patch Addresses jminta's comments — — Splinter Review
(In reply to comment #16)
> (From update of attachment 245050 [details] [diff] [review] [edit])
> 1.) Judging by my conversations around moco and the looks of this patch, you
> stole this code from Waldo?  He should probably get mentioned in the
> contributors section of the license.
"stole" is a harsh word.  I prefer "ported" :)  Added him to contributors.

> Two stars start a long comment please.
fixed

> Let's make it clear that val and locked belong to p, while disabled belongs to
> Element
fixed

> Triple assignment is discouraged in calendar code.
fixed

> +        var enabledPref = document.getElementById("app.update.enabled");
> +        var autoPref = document.getElementById("app.update.auto");
> +        var modePref = document.getElementById("app.update.mode");
> +
> +        var warnIncompatible = document.getElementById("warnIncompatible");
> +
> +        var disable = enabledPref.locked || !enabledPref.value ||
> autoPref.locked ||
> +                      !autoPref.value || modePref.locked;
> You follow this same pattern several times, and in the words of shaver "We all
> love our helper functions.  Yes we do.  Each and every one."
> 
> I'm thinking function prefImpliesDisabled(aPrefId) {
>   var pref = document.getElementById(aPrefId);
>   return pref.locked || !pref.disabled;
> }
While I see the repetition, since the number of comparisons is different in each case, I think adding that helper function hurts readablility more than it helps readability or maintainability.  Therefore I didn't do this.

> The comment and the result of this code line explicitly contradict.  One of
> them needs to be fixed.
renamed the variable


> You are gAdvancedPane, so why not just use |this|?
fixed

> Same comment.  Also, the ! inversion seems really odd.  Flip your second and
> third operands and drop it.
fixed

> +                        onchange="gAdvancedPane.updateAppUpdateItems();
> +                                  gAdvancedPane.updateAutoItems();
> +                                  gAdvancedPane.updateModeItems();"/>
> You've made this same set of calls twice now.  Everyone together... "We all
> love our helper functions..."
I also don't think adding a helper function for this particular instance helps readability either.

> There are starving children in Africa who want that extra line.  No need to
> separate these.
fixed

> These separators don't show up in your screenshot.  Are they turned on in
> winstripe only?
In pinstripe they show up as blank space.

> There's no reason to set a value attribute when there's already a default pref
> that's going to be sync'd up onload.
fixed

> Why are we creating another entity that's exactly the same as &brandShortName?
fixed

> We haven't yet broken Lightning's dependency on applicationUtil yet.  That
> means this could cause nasty problems with scope pollution.
renamed function to prevent scoping issues

> These changes aren't relevant to the bug here, haven't been discussed, make
> an already large patch larger, and generally make reviewing harder.
fine. removed.


carrying ctalbert's r+ forward
Attachment #245050 - Attachment is obsolete: true
Attachment #246578 - Flags: second-review?(jminta)
Attachment #246578 - Flags: first-review+
Comment on attachment 246578 [details] [diff] [review]
Addresses jminta's comments

patching file mozilla/calendar/sunbird/app/profile/channel-prefs.js
patching file mozilla/calendar/sunbird/app/profile/sunbird.js
patch: **** malformed patch at line 622: Index: mozilla/calendar/sunbird/base/content/calendar-menubar.inc

There's also something really weird going on with the last set of preferences. Symptoms:
(1) The 'Warn me if this will disable any of my add-ons' checkbox never gets enabled.
(2) app.update.auto never changes value
(3) Neither of the radio buttons are selected onload.

Looks pretty good otherwise.  I'd like to take one last look at the preference logic once you get that bug sorted out.
Comment on attachment 246578 [details] [diff] [review]
Addresses jminta's comments

lilmatt showed me that removing the value= bits on the radio buttons was the cause of the bustage. r2=jminta with those returned.
Attachment #246578 - Flags: second-review?(jminta) → second-review+
Patch landed on MOZILLA_1_8_BRANCH and trunk.

-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Whiteboard: [patch in hand][needs review jminta]
About when will nightly updates be available?
(In reply to comment #21)
> About when will nightly updates be available?

The November 27 nightly will have the initial update code and will be the first build you can use to test nightly update functionality. Any older builds will not have it.

If you run "Help > Check for updates" in the Nov27 build on Nov28 (after the Nov28 nightly has run and the .mar files are available) then the update notification _should_ appear.

If not, it's a bug.
After clicking check for updates I can see an error in console:
Error: this.docShell has no properties
Source File: chrome://global/content/bindings/browser.xml
Line: 0

But it seems to work...

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20061127 Calendar/0.4a1
(In reply to comment #23)
> After clicking check for updates I can see an error in console:
> Error: this.docShell has no properties
> Source File: chrome://global/content/bindings/browser.xml
> Line: 0

That happens also in Trunk Thunderbird and has been reported as bug 358761.
Update checking works- upgrading from Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20061127
Calendar/0.4a1 to Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20061128 Calendar/0.4a1
(In reply to comment #25)
> Update checking works

--> verified
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: