297903 - Extension updates should link to further information

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, enhancement, P2)

Description

[Mark Clements]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.8) Gecko/20050511 Firefox/1.0.4 (ax)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.8) Gecko/20050511 Firefox/1.0.4 (ax)

When the update manager alerts you that a new version of an extension is
available, there is no indication about what has changed in the new version.
There is no way for users to evaluate whether the update is appropriate or
necessary.



Reproducible: Always

Steps to Reproduce:
Run the update manager.  (Only applicable if updates are available)

Actual Results:  
You see a list of extensions that need updating, with the URL the extension
comes from.

Expected Results:  
This list should include information about what has changed in this version (or
a direct link to that information).

Making the 'from' URL clickable is not a solution - I typed in the URL listed
for the TabBrowser Preferences extension (216.55.161.203) and simply got a page
containing the text 'hello'.

Comment 1

[Jorge Villalobos [:jorgev] (he/him)]

Might be a duplicate of Bug 295350, although the first comment clearly states
that the bug should be separated and there's no indication that it was actually
done.

Comment 2

[Mark Clements]

This is not the same as any of the points mentioned in Bug 295350.  This refers
to the Firefox Update dialog, whilst the other refers to a bunch of other issues
in the install extension dialog.

Comment 3

[Mark Clements]

So is anyone going to take this on?  Currently I am unable to trust the update
procedure for anything other than Firefox itself - I need to manually trace the
appropriate web page to check whether the update is desirable and trustworthy.

Comment 4

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

We could add an optional update rdf property that could contain an url to more information (e.g. release note or whatever your fav term is). This could then be used to display a More Information link next to the Version x.x is available message in a similar manner as we display the More Information link next to the blocklisted message in the UI. We should probably verify that the url is safe before adding it to the extensions datasource but this seems quite doable now that bug 329045 has landed on the trunk.

Comment 5

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

Essentially the same as bug 263438

Comment 6

[Jesse Ruderman]

[sg:want P4] - letting users know which extension updates are security updates (rather than risky new features or language pack additions) will make it more likely that they'll apply security updates.

Comment 7

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

You may want to also add bug 304397 to the sg:want bugs

Comment 9

[Mike Connor [:mconnor]]

Not a blocker, but it'd be nice to provide more info about the update.

Comment 11

[Dave Townsend [:mossop]]

Attachment: UI mockup

This is a working prototype I have for this at the moment. Essentially provides a browser at the bottom of the updates pane that displays a url provided in the update.rdf.

Shaver said that there would be issues with launching a full browser window to display the information and I tend to agree (think session store in particular) so I would suggest that this, or potentially a second openable window with nothing but a webpage in it.

Space is limited but then if authors are aware of that they can accomodate, the page in there now is a working example from my site.

Should we choose this as the right way forward then I think locking down the browser should be thought about, maybe even make it untouchable except for scrolling?

Comment 12

[Chris Bloom]

I hadn't considered using an embedded browser. Certainly that would allow for longer change lists. However, I don't know that you need complete HTML rendering for this. Perhaps you could simply update the spec for XPI files such that a new RDF or XML document could be included using a specific format. The browser could extract it (or perhaps the spec would require that the file sit alongside the XPI) and do some basic interpretation of the contents. You really would only need to accommodate for generic URLs (in case there is need to link out to a bug report or other external resource), but other markup probably wouldn't be required. Actually, you might even consider basing it off of TeXtile or some other plain-text markup format.  After all, a change list should really only be just that - a list of changes :) Just a thought.

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Proposal: When the available updates are shown on startup and the clicking on one shows the list with changes, there should be a checkbox on the top "load this page after installing the updates", so e.g. the user could use the link for getting more information about the new support for XYZ.

Mossop: Ick, now you're assuming that we are a browser ;) 
Mossop: I'm not a fan of that but feel free to stick it into the bug then whoever does ui-r can take it into account

Comment 14

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

Dave, if you haven't already locked down the browser take a look at what Seth Spitzer did similarly for Software Update.

archaeopteryx, update notification will be moved to a new widget that will provide notification of updates during normal browsing and no longer on startup.

Comment 15

[Dave Townsend [:mossop]]

If we're no longer doing the update notification on startup then I'm not sure the embedded browser in the update pane is necessary any more, we could just open a browser window from a button. Which way do we want to go, a question more for beltzner I guess.

Comment 16

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #14)
> archaeopteryx, update notification will be moved to a new widget that will
> provide notification of updates during normal browsing and no longer on
> startup.

Oh, I didn't think that would be exclusive of notification on startup; I quite like the notification on startup, actually, since it's a lot easier to deal with than having to click a notification widget. The additional widget would be there for people who're running multi-day sessions, mostly.

(In reply to comment #11)
> Space is limited but then if authors are aware of that they can accomodate, the
> page in there now is a working example from my site.

Some thoughts:

 - do we need to provide full HTML control, or can we just ask for a bunch of <li> elements and constrain the design a little more? 
 - I'm somewhat nervous about spoofing/security implications, but would have expected Jesse to weigh in if there was something really serious :)

> Should we choose this as the right way forward then I think locking down the
> browser should be thought about, maybe even make it untouchable except for
> scrolling?

 - yeah, that too

UI-review coming with some ideas/further thoughts.

Comment 17

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

(In reply to comment #16)
> (In reply to comment #14)
> > archaeopteryx, update notification will be moved to a new widget that will
> > provide notification of updates during normal browsing and no longer on
> > startup.
> 
> Oh, I didn't think that would be exclusive of notification on startup; I quite
> like the notification on startup, actually, since it's a lot easier to deal
> with than having to click a notification widget. The additional widget would be
> there for people who're running multi-day sessions, mostly.
There are a ton of problems with notification on startup. See meta bug 366777.

Comment 18

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

not to mention that app startup is a fragile place to be doing this as has been seen previously with the add-ons compatibility wizard.

Comment 19

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #17 & comment #18)

This debate belongs over in that bug, so I added a comment there. See bug 366777 comment 2 :)

Comment 20

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 268354 [details]
UI mockup

I think we want to closely associate the information about the update with the update itself, and to make it more of something the user chooses to see instead of what they see by default.

I'd prefer to see something like:

----------------------------------------------------------------
### Nightly Tester Tools version 1.3b1 is available
### 
 
    (+) Show Details                     [x] Include this update
----------------------------------------------------------------


clicking on the "Show Details" expando-widget would show:

----------------------------------------------------------------
### Nightly Tester Tools version 1.3b1 is available
### 
   .----------------------------------------------------------.
   | Blah blah blah blah blah blah blah blah blah blah blah |^|
   | blah blah blah blah blah blah blah blah blah blah blah |=|
   | blah blah blah blah blah blah blah blah blah blah blah |_|
   | blah blah blah blah blah blah blah blah blah blah blah |V|
   '----------------------------------------------------------'

    (-) Hide Details                     [x] Include this update
----------------------------------------------------------------

where "blah blah blah" can be a HTML snippet with only a certain set of tags rendered, like the set usually allowed by most weblogs: <b>, <i>, <ul>, <ol>, <tt>, <h1>, etc.

Per my comments in bug 366777 comment 2, we can and should also think of ways of making it easier to perform actions like deferring the choice and open Firefox, allowing user to revisit the choice though the notification widget.

(cc'ing madhava, as he's working with rob strong on a bunch of XPI install/update related stuff)

Comment 21

[Dave Townsend [:mossop]]

Attachment: Newer mock-up

This is further work towards beltzner's thoughts. There are a couple of bugs with the implementation as it stands (one of which appears to be an issue with the mac scrollbar) but before I press on with those I would appreciate some thoughts.

Details are by default hidden, clicking on show details expands the entry and displays a throbber until the page is loaded when that displays (or an error message if the page could not be loaded).

The only sanitisation on the page at the moment is that it is loaded in a content browser with js, plugins, authentication and frames disabled. This matches that done in the app update notification dialog.

Comment 22

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

Dave, could you provide a screenshot with enough extensions so there is a scrollbar in the list and a scrollbar in the update info? 

Comment 23

[Dave Townsend [:mossop]]

Attachment: Another shot

As requested

Comment 24

[Dave Townsend [:mossop]]

From IRC:

Make the expand button a twisty (expander)
Restrict html content so we can style it ourselves.

Comment 25

[Dave Townsend [:mossop]]

I have some stuff working on restricting the html content, but it assumes I can parse the content into a DOM. Currently I'm working with a good valid xhtml document as a test but really this needs 102699 so we can handle html content.

Comment 26

[Dave Townsend [:mossop]]

Attachment: Mock up with new expander and sanitised html

This is a new mock up with the now working expander element and sanitised html. The inner scrollbar for the details looks a bit weird I think (particularly when the add-on is selected) however I'm not sure that making the display larger (or even allowing it to grow to fit the content) is a great idea. Thoughts?

Comment 27

[Dave Townsend [:mossop]]

Attachment: Shot highlighting the main issue

Comment 28

[Mike Beltzner [:beltzner, not reading bugmail]]

Hm. Yes, I agree, that does look odd. I also agree that resizing the window isn't what we want to do here.

If we were to continue this design direction, I'd suggest:

 - adding a keyline border around the details area to prevent the scrollbar from just floating there
 - considering dropping the highlight treatment on selection, since selection doesn't actually do anything here
 - (nit) aligning the twisty with the add-on title

Other totally off-the-wall, free-thinking ideas:

 - make the "Details" button an actual button that slides (or DHTML overlays) a layer on top of the rich list box, and put the sanitised HTML and a "Done" or "Close" button in that overlay; this makes the details interaction modal, but I can live with that

 - use a tray-like UI as QuickSilver on OSX does (though this would be pretty strange on Windows, I think)

 - have a "details" pane in the UI which shows the details for the selected add-on (I regret even mentioning that, and don't like the idea very much, but in the interest of completeness, I thought I'd mention it)

Madhava, any quick thoughts?

Comment 29

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

(In reply to comment #28)
> Hm. Yes, I agree, that does look odd. I also agree that resizing the window
> isn't what we want to do here.
> 
> If we were to continue this design direction, I'd suggest:
> 
>  - adding a keyline border around the details area to prevent the scrollbar
> from just floating there
>  - considering dropping the highlight treatment on selection, since selection
> doesn't actually do anything here
The selection informs the user which item that the commands available in the context menu and the keybinding for toggling the "include this update" will be performed upon.


>  - (nit) aligning the twisty with the add-on title
> 
> Other totally off-the-wall, free-thinking ideas:
> 
>  - make the "Details" button an actual button that slides (or DHTML overlays) a
> layer on top of the rich list box, and put the sanitised HTML and a "Done" or
> "Close" button in that overlay; this makes the details interaction modal, but I
> can live with that
I personally prefer something along this line of thinking.

Comment 30

[Chris Bloom]

(In reply to comment #29)
> The selection informs the user which item that the commands available in the
> context menu and the keybinding for toggling the "include this update" will be
> performed upon.
Highlighting the whole item, details and all, makes reading a bit awkward. White text on a blue background isn't a great contrast. 

(In reply to comment #28)
>  - adding a keyline border around the details area to prevent the scrollbar
> from just floating there - adding a keyline border around the details area to
> prevent the scrollbar
> from just floating there
I think this would help, although ...

>  - make the "Details" button an actual button that slides (or DHTML overlays) a
> layer on top of the rich list box, and put the sanitised HTML and a "Done" or
> "Close" button in that overlay; this makes the details interaction modal, but I
> can live with that
... I feel that this is a better solution.

Comment 31

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

(In reply to comment #30)
> (In reply to comment #29)
> > The selection informs the user which item that the commands available in the
> > context menu and the keybinding for toggling the "include this update" will be
> > performed upon.
> Highlighting the whole item, details and all, makes reading a bit awkward.
> White text on a blue background isn't a great contrast. 
We respect the OS colors for lists (see Add / Remove Programs if running Windows) as we should and yes, they are a bit awkward to read just as other lists.

Comment 32

[Daniel Veditz [:dveditz]]

One concern I have with this approach is that since the information is appearing in a browser-generated dialog it's not at all clear that this information is provided by a web site. If we no longer have the restrictions imposed by doing this during start-up I would be much happier with a simple "detailed information" link that opens in a true browser window where the user can see
 - what site the information comes from
 - whether it's a secure link (if there's any worry about spoofing)
 - the ability to browse around the author's site for even deeper
   information if wanted and available (support fora?)

I also have a secondary worry that if the snippets are shown by default some developers might start trying to stuff ads in there, even just text ads if we shut off images. They might even have some incentive for unnecessary revs of the product just to show those ads. Having ads on their own site would be legit though, for users who do open the page -- at least it's clear that the _developer_ is the one with the ads, it's not "OMG Firefox showing ads!".

Comment 33

[Daniel Veditz [:dveditz]]

I do like the _look_ of the in-line information, very classy looking if we could rely on developers to provide text-only succinct info as shown. But given the whole conduit mess I expect any wiggle-room will be fully exploited in any way possible.

Comment 34

[Dave Townsend [:mossop]]

I can understand the concern about advertising, however I think given the extreme sanitisation that is going on that it shouldn't be much of an issue. What we display is raw text, no links, no images, no objects. I can't foresee a situation where putting advertising in it would actually gain an author anything. Also what I have right now shows nothing by default.

Comment 35

[Daniel Veditz [:dveditz]]

Advertising aside, my main concern is that there's no context or clue where this content comes from and users will assume that it comes from Firefox. Or at the very least the site from which the extension was installed (which for 99+% of extensions used in the wild still means "Mozilla"). If the URL points at some other site then the content could change after it has been reviewed on AMO and be perhaps misleading or offensive.

I am uneasy presenting this information in a way that could be mistaken as information provided by and/or endorsed by Firefox.

Or were you planning to enforce that the details URL host equals the updateURL host (the default update URL host if empty)? That's probably overly restrictive and my concern could probably be fixed with more subtlety by simple wording or presentation changes on the dialog. Alas, my own ideas for "fixing" it are rather clunky, but maybe the UE professionals will have a brainstorm.

Comment 36

[Daniel Veditz [:dveditz]]

I also fear developers will tend to link to a generic "change log" page on their (or AMO's) site as your own screenshot shows in attachment 271036 [details]. It'd be a fine place to link to if people can see it's just part of a web page, but as part of the update dialog I think it will confuse a lot of "regular folks" to get a long long list of irrelevant changes in previous versions.

Comment 37

[Chris Bloom]

(In reply to comment #35)
> ... If the URL points at
> some other site then the content could change after it has been reviewed on AMO
> and be perhaps misleading or offensive.
I suggested earlier that the change log be embedded in the update. Once it's included in the package it can't change unless the package is resubmitted. You would just need to allow for a new multi-line field in the package RDF, perhaps with limited ReST formatting support for displaying lists. 

Comment 38

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

(In reply to comment #37)
> (In reply to comment #35)
> > ... If the URL points at
> > some other site then the content could change after it has been reviewed on AMO
> > and be perhaps misleading or offensive.
> I suggested earlier that the change log be embedded in the update. Once it's
> included in the package it can't change unless the package is resubmitted. You
> would just need to allow for a new multi-line field in the package RDF, perhaps
> with limited ReST formatting support for displaying lists. 
Embedded implies within the xpi and the data should be displayed prior to downloading the xpi.

Comment 39

[Dave Townsend [:mossop]]

(In reply to comment #36)
> I also fear developers will tend to link to a generic "change log" page on
> their (or AMO's) site as your own screenshot shows in attachment 271036 [details]. It'd
> be a fine place to link to if people can see it's just part of a web page, but
> as part of the update dialog I think it will confuse a lot of "regular folks"
> to get a long long list of irrelevant changes in previous versions.

I confess to being confused then. Just what "further information" do you think we should be providing here?

Comment 40

[Mark Clements]

I don't know much about how Firefox gets its update information, but I would imagine that the best solution is for the place that tells Firefox that 'a new version is available' and 'this is the new version number' is the correct place to hold this info, so it also provides a 'this is the list of changes' (or 'these are the release notes, or whatever'.  It would also allow the browser to show all release notes before version N installed on the client, and version X available on the server (but not version M, which is older than the installed version).

(In reply to comment #35)
> I am uneasy presenting this information in a way that could be mistaken as
> information provided by and/or endorsed by Firefox.

If the info is provided by a third-party website, then surely just a line at the top of the 'details' box that says "The following information is provided by X, and has not been checked by the makers of Firefox:" (or words to that effect) would be sufficient.

Comment 41

[Daniel Veditz [:dveditz]]

(In reply to comment #39)
> (In reply to comment #36)
>
> I confess to being confused then. Just what "further information" do you think
> we should be providing here?

I was concerned a long changelog containing versions the user already had would confuse and overwhelm some people. But, you're right that the user may not be on the most recent previous version so that could in fact be extremely important.

  version 1.3b2 -- fixed typo                  (yawn)
  version 1.3b1 -- !!critical security patch!! (oh, better get it)

A dynamically generated site like AMO could easily spit out just the relevant change information based on what the user actually had.(In reply to comment #38)


> (In reply to comment #37)
> Embedded implies within the xpi and the data should be displayed prior to
> downloading the xpi.

Yeah, putting it in the xpi wouldn't work, but I'm pretty sure he meant embedded in the update.rdf sent by AMO to tell the client about the update. That would work, and would save a load even, but those update.rdf files weren't really designed with embedded HTML in mind.

Comment 42

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

(In reply to comment #41)
>...
> > (In reply to comment #37)
> > Embedded implies within the xpi and the data should be displayed prior to
> > downloading the xpi.
> 
> Yeah, putting it in the xpi wouldn't work, but I'm pretty sure he meant
> embedded in the update.rdf sent by AMO to tell the client about the update.
> That would work, and would save a load even, but those update.rdf files weren't
> really designed with embedded HTML in mind.
Also, we check for updates approximately once per day and download the update rdf at that time. Adding this data to the update rdf would mean a very significant increase in the size of this download when checking for updates... much better to keep this data separate.

Comment 43

[Dave Townsend [:mossop]]

Attachment: New mockup

This is a mockup of the updated work after consultation with UX

Comment 44

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

Comment on attachment 277934 [details]
New mockup

Hide Information for the button label is very vague and with the button over on the left it isn't directly associated with the release notes on the right.

Comment 45

[Dave Townsend [:mossop]]

(In reply to comment #44)
> (From update of attachment 277934 [details])
> Hide Information for the button label is very vague and with the button over on
> the left it isn't directly associated with the release notes on the right.

Agreed, but in discussion last week we went with the button there as essentially placeholder UI so I could get this pretty much wrapped up with more thought to go into the final UI with the redesign.

Comment 46

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

Not a big deal then but it may be better to always display the release note for now instead.

Comment 47

[Dave Townsend [:mossop]]

Ok that's a fairly simple change, but given that I need to hear what's happening with bug 102699 before I can submit a final patch here it's possible the UI rework may happen before we get a final patch committed ;)

Comment 48

[Dave Townsend [:mossop]]

Attachment: patch rev 1

Progress on bug 102699 seems a little slow and given that this is wanted I propose a new plan. This is the patch that implements the feature, the only restriction is that it requires the update information page provided by the extension author to be valid XHTML (passing all XML validity requirements). This is I think not a major burden, and if the html parser is completed in time it is a simple few lines to make this use it to relax that restriction.

The basic architecture of this feature is the addition of a possible em:updateInfoURL arc to the update manifest. If present it should be a url pointing to a page containing information about the update. The backed EM caches this in the same way as the updateURL. The frontend extends the theme preview panel to display the update information. When visible if the update selected has information we request it and display a sanitised version of it.

The sanitisation is performed with an xsl stylesheet. All it lets through is h(1-3), p, ul and ol tags and where appropriate li, b, i, em, strong tags to allow some minimal formatting.

In extensions.js UpdateInfoLoader performs the work of getting and sanitising the html. To do this it must load both the document from the provided info url and the xsl from a fixed chrome url. The latter it caches such that viewing information about multiple add-ons only requires loading the stylesheet once.

There are two parts to this that I think warrant some consideration. The first is the update info URL. I was thinking it might be good to perform the same kind of escaping on it that we do for the update rdf url, (app id, version etc.). Could just factor out the code that does this and call it. This would allow extension authors to just provide a single update info url in their manifest that automatically displayed only the relevant information.

The second is the modifications to nsIUpdateItem. I spoke with you about this before Rob. While I think this addition is actually relevant (any caller requesting an update ought to get informed about the update info) I know of at least 2 other patches in progress which also add properties to it. Individually they may seem fine, but I'm concerned we are overloading this interface and there may be a better way. Possibly it is too late in the game to be too worried with that and we wait till the next version where we could have gone for sqlite and so all this will need serious work anyway ;)

Comment 49

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

Comment on attachment 278225 [details] [diff] [review]
patch rev 1

It looks like the diff isn't complete (see the end of the patch)... could you resubmit?

A couple of quick comments.
>Index: toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd,v
>retrieving revision 1.22
>diff -u8 -p -r1.22 extensions.dtd
>--- toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd	8 Aug 2007 18:18:24 -0000	1.22
>+++ toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd	25 Aug 2007 17:51:37 -0000	
>@@ -65,16 +65,22 @@

For the strings I defer to UI / UX people but they seem a bit off

> <!ENTITY cmd.uninstall2.accesskey         "U">
> <!ENTITY cmd.uninstall2.tooltip           "Uninstall this Add-on when &brandShortName; is restarted">
> <!ENTITY cmd.cancelUninstall.label        "Cancel Uninstall">
> <!ENTITY cmd.cancelUninstall.accesskey    "C">
> <!ENTITY cmd.cancelUninstall.tooltip      "Cancel the uninstall of this Add-on">
> <!ENTITY cmd.installUpdate.label          "Install Update">
> <!ENTITY cmd.installUpdate.accesskey      "I">
> <!ENTITY cmd.installUpdate.tooltip        "Install an update for this Add-on">
>+<!ENTITY cmd.showUpdateInfo.label         "Show Information">
>+<!ENTITY cmd.showUpdateInfo.accesskey     "S">
>+<!ENTITY cmd.showUpdateInfo.tooltip       "Show more information about these updates">
"more" seems superfluous here... I think this should just be
"Show information about these updates"

>@@ -99,16 +105,19 @@
> 
> <!ENTITY getExtensions.label              "Get Extensions">
> <!ENTITY getExtensions.tooltip            "Get Extensions from addons.mozilla.org">
> <!ENTITY getThemes.label                  "Get Themes">
> <!ENTITY getThemes.tooltip                "Get Themes from addons.mozilla.org">
> <!ENTITY previewNoThemeSelected.label     "No Theme Selected">
> <!ENTITY previewNoPreviewImage.label      "This Theme does not have a Preview Image">
> <!ENTITY moreInfo.label                   "More Information">
>+<!ENTITY infoNoAddonSelected.label        "No Update Selected">
>+<!ENTITY infoNoUpdateInfo.label           "This update does not have any additional information">
>+<!ENTITY infoUpdateInfoError.label        "There was an error loading the information about this update">
Seems like this should be something along the lines of
"There was an error loading the update's information"

>Index: toolkit/mozapps/extensions/content/extensions.js
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.js,v
>retrieving revision 1.137
>diff -u8 -p -r1.137 extensions.js
>--- toolkit/mozapps/extensions/content/extensions.js	25 Aug 2007 14:26:44 -0000	1.137
>+++ toolkit/mozapps/extensions/content/extensions.js	25 Aug 2007 18:56:12 -0000	
>@@ -17,16 +17,17 @@
> # The Initial Developer of the Original Code is Ben Goodger.
> # Portions created by the Initial Developer are Copyright (C) 2004
> # the Initial Developer. All Rights Reserved.
> # 
> # Contributor(s):
> #   Ben Goodger <[e-mail]>
> #   Robert Strong <[e-mail]>
> #   Dão Gottwald <[e-mail]>
>+#   Dave Townsend <[e-mail]>
It is about time! :P

>Index: toolkit/mozapps/extensions/src/nsExtensionManager.js.in
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/src/nsExtensionManager.js.in,v
>retrieving revision 1.238
>diff -u8 -p -r1.238 nsExtensionManager.js.in
>--- toolkit/mozapps/extensions/src/nsExtensionManager.js.in	20 Aug 2007 08:02:22 -0000	1.238
>+++ toolkit/mozapps/extensions/src/nsExtensionManager.js.in	25 Aug 2007 18:31:19 -0000	
>...
>@@ -4481,17 +4484,18 @@ ExtensionManager.prototype = {
>     // They will be reset as appropriate by the upgrade/install process.
>     var ds = this.datasource;
>     ds.updateVisibleList(id, installLocation.name, false);
>     var props = { installLocation : EM_L(installLocation.name),
>                   type            : EM_I(type),
>                   newVersion      : EM_L(getManifestProperty(installManifest, "version")),
>                   availableUpdateURL      : null,
>                   availableUpdateHash     : null,
>-                  availableUpdateVersion  : null };
>+               
Eeep!

Comment 50

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

Comment on attachment 278225 [details] [diff] [review]
patch rev 1

Nevermind... it just didn't load completely. :(

Comment 51

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

Comment on attachment 278225 [details] [diff] [review]
patch rev 1

Dave, can I get an un-bitrotted version of this patch for review? Thanks

Comment 52

[Dave Townsend [:mossop]]

Attachment: updated to trunk

Updated to current trunk. Note that the extensions off my site should be serving the update info for testing purposes. Because of the need to valid xhtml one or two might not work right but Nightly Tester Tools definately does at the moment.

Comment 53

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

Dave, I need to work on the installer tonight but I should be able to get to this by no later than Tuesday evening.

Comment 54

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

Dave, I've used custom EM datasource properties in the past for things like this instead of extending nsIUpdateItem... seems like that would be appropriate for this use case.

Comment 55

[Dave Townsend [:mossop]]

So I had actually considered this to be a very valid addition to nsIUpdateItem given that anyone using the APIs for an extension update (third part app etc) might well want to be informed of the update info url in the onAddonUpdateEnded.

However I can change it to just use the datasource property if you wish.

Comment 56

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

I think that argument could be made for every single piece of metadata and for the cases where they want it after an update check the data is readily available via the datasource.

Comment 57

[Dave Townsend [:mossop]]

Attachment: patch rev 2

This removes the changes to nsIUpdateItem. The place where availableUpdateInfo is set is a little sub-optimal I think but it should work. We enter that block every time we find an updated version that is better than our previous newest version, and the last time we enter that is the new version that eventually gets pushed out to the listeners.

Comment 58

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

Comment on attachment 279809 [details] [diff] [review]
patch rev 2

r=me on the condition that you fix winstripe... those buttons require an image.

http://lxr.mozilla.org/seamonkey/source/toolkit/themes/winstripe/mozapps/extensions/extensions.css#31

Comment 59

[Dave Townsend [:mossop]]

Attachment: ready for checkin

This fixes the icon, just points it to an existing image for now, will need new icons as part of the UI update. Filed bug 395117 to get icon updates.

Comment 60

[Dave Townsend [:mossop]]

Checking in toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd;
/cvsroot/mozilla/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd,v  <--  extensions.dtd
new revision: 1.26; previous revision: 1.25
done
Checking in toolkit/mozapps/extensions/jar.mn;
/cvsroot/mozilla/toolkit/mozapps/extensions/jar.mn,v  <--  jar.mn
new revision: 1.5; previous revision: 1.4
done
Checking in toolkit/mozapps/extensions/content/extensions.js;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.js,v  <--  extensions.js
new revision: 1.149; previous revision: 1.148
done
Checking in toolkit/mozapps/extensions/content/extensions.xml;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.xml,v  <--  extensions.xml
new revision: 1.48; previous revision: 1.47
done
Checking in toolkit/mozapps/extensions/content/extensions.xul;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.xul,v  <--  extensions.xul
new revision: 1.64; previous revision: 1.63
done
RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/content/updateinfo.xsl,v
done
Checking in toolkit/mozapps/extensions/content/updateinfo.xsl;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/updateinfo.xsl,v  <--  updateinfo.xsl
initial revision: 1.1
done
Checking in toolkit/mozapps/extensions/src/nsExtensionManager.js.in;
/cvsroot/mozilla/toolkit/mozapps/extensions/src/nsExtensionManager.js.in,v  <--  nsExtensionManager.js.in
new revision: 1.252; previous revision: 1.251
done
Checking in toolkit/themes/pinstripe/mozapps/extensions/extensions.css;
/cvsroot/mozilla/toolkit/themes/pinstripe/mozapps/extensions/extensions.css,v  <--  extensions.css
new revision: 1.31; previous revision: 1.30
done
Checking in toolkit/themes/winstripe/mozapps/extensions/extensions.css;
/cvsroot/mozilla/toolkit/themes/winstripe/mozapps/extensions/extensions.css,v  <--  extensions.css
new revision: 1.35; previous revision: 1.34
done

Comment 61

[Tony Chung [:tchung]]

Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X; es-ES; rv:1.9a9pre) Gecko/2007092804 Minefield/3.0a9pre.  Info updates now in right hand pane.

Comment 63

[Dave Townsend [:mossop]]

This has a test in browser_manualupdates.js now