Closed Bug 548061 Opened 14 years ago Closed 14 years ago

Billboard should handle 404 (other errors?) for billboard better

Categories

(Toolkit :: Application Update, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: robert.strong.bugs, Assigned: robert.strong.bugs)

References

Details

Attachments

(2 files, 5 obsolete files)

Attached image screenshot
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?
Attached patch patch in progress (obsolete) — Splinter Review
Assignee: nobody → robert.bugzilla
Status: NEW → ASSIGNED
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.
Attached patch patch in progress (obsolete) — Splinter Review
Attachment #428515 - Attachment is obsolete: true
Attached patch patch rev1 (obsolete) — Splinter Review
Attachment #431146 - Attachment is obsolete: true
Attachment #431255 - Flags: review?(dtownsend)
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.
Attachment #431255 - Attachment is obsolete: true
Attachment #431290 - Flags: review?(dtownsend)
Attachment #431255 - Flags: review?(dtownsend)
Attachment #431290 - Flags: review?(dtownsend) → review?(dolske)
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.
Attached patch patch rev3 (obsolete) — Splinter Review
Attachment #431290 - Attachment is obsolete: true
Attachment #432404 - Flags: review?(dolske)
Attachment #431290 - Flags: review?(dolske)
(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...
Attachment #432404 - Flags: review?(dolske) → review+
(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+
Attachment #432404 - Attachment is obsolete: true
Attachment #433188 - Flags: review+
Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/46ac7271006e
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.