If the billboard url is unavailable an error is displayed in place of the billboard (see attached screenshot for an example). Instead of showing this error we should instead have a fallback... perhaps to the non billboard page?

Not sure how to handle / if this should handle the 'hotel' case where the html ends up being the internet access login page. I'm not sure about whether this should be handled since if that is the case it wouldn't be possible to download the mar anyways and the vast majority of time there wouldn't be a update available notification.

Slightly cleaner in that the load listeners of remotecontent in updates.js should only handle the event when it is for remotecontent and not for its children.

I noticed that in your screenshot, the message displayed doesn't help the usability for the user. It would be more helpful if you said something like "A new version of Firefox has been found, but..." at the start. If you don't mention that there is a new update, then the user might be confused by the randomness of the message.

Actually you are looking at screenshots for how it works currently which was implemented before Firefox 3.0 and not how it will work after this bug is fixed. With this bug fixed we will be displaying the non-billboard application update page that allows the user to update.

(In reply to comment #7) > Actually you are looking at screenshots for how it works currently which was > implemented before Firefox 3.0 and not how it will work after this bug is > fixed. With this bug fixed we will be displaying the non-billboard application > update page that allows the user to update. I see. I'm sorry that I misunderstood.

Comment on attachment 432404 [details] [diff] [review] patch rev3 > if (!aus.canApplyUpdates) { >+ // Prevent multiple notifications for the same update when the user is >+ // unable to apply updates. >+ gUpdates.never(); Is it possible (and likely) that the user might later log in with additional access? (In which case we might want to prompt again?) >+ remoteContent.addEventListener("load", >+ gUpdatesFoundBillboardPage.onBillboardLoad, >+ false); Do we show a throbber while waiting for the billboard to load? > this.setAttribute("state", "error"); >+ var e = document.createEvent("Events"); >+ e.initEvent("load", false, true); >+ this.dispatchEvent(e); It's a little odd to have onError generating a "load" event instead of an "error" event, but I guess that does simplify your listener code... Well, at least, it seems the onLoad handlers are already being called (maybe accidentally by the error page load?) when in the error state...

(In reply to comment #10) > (From update of attachment 432404 [details] [diff] [review]) > > if (!aus.canApplyUpdates) { > >+ // Prevent multiple notifications for the same update when the user is > >+ // unable to apply updates. > >+ gUpdates.never(); > > Is it possible (and likely) that the user might later log in with additional > access? (In which case we might want to prompt again?) It is possible but they would have to change their account on their OS to have additional perms or change the perms on the install dir so their account has access. At that point they could either manually check for updates to get the version that was previously set to never or the next update will either auto download or notify the user of its availability. > > >+ remoteContent.addEventListener("load", > >+ gUpdatesFoundBillboardPage.onBillboardLoad, > >+ false); > > Do we show a throbber while waiting for the billboard to load? Yes, along with text. > > this.setAttribute("state", "error"); > >+ var e = document.createEvent("Events"); > >+ e.initEvent("load", false, true); > >+ this.dispatchEvent(e); > > It's a little odd to have onError generating a "load" event instead of an > "error" event, but I guess that does simplify your listener code... Well, at > least, it seems the onLoad handlers are already being called (maybe > accidentally by the error page load?) when in the error state... Agreed... previously there was no check for where the event was coming from and the error does send the load event on the browser element that propagated up. Now it explicitly calls it on the remoteelement widget instead. The pre-existing code that handled it before still handles it but now it checks that the event is coming from the remoteelement widget.

Carrying forward r+

Pushed to mozilla-central http://hg.mozilla.org/mozilla-central/rev/46ac7271006e