Closed
Bug 548061
Opened 15 years ago
Closed 15 years ago
Billboard should handle 404 (other errors?) for billboard better
Categories
(Toolkit :: Application Update, defect)
Toolkit
Application Update
Tracking
()
RESOLVED
FIXED
People
(Reporter: robert.strong.bugs, Assigned: robert.strong.bugs)
References
Details
Attachments
(2 files, 5 obsolete files)
28.68 KB,
image/png
|
Details | |
27.94 KB,
patch
|
robert.strong.bugs
:
review+
|
Details | Diff | Splinter Review |
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?
Assignee | ||
Comment 1•15 years ago
|
||
Assignee: nobody → robert.bugzilla
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•15 years ago
|
||
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.
Assignee | ||
Comment 3•15 years ago
|
||
Attachment #428515 -
Attachment is obsolete: true
Assignee | ||
Comment 4•15 years ago
|
||
Attachment #431146 -
Attachment is obsolete: true
Attachment #431255 -
Flags: review?(dtownsend)
Assignee | ||
Comment 5•15 years ago
|
||
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)
Assignee | ||
Updated•15 years ago
|
Attachment #431290 -
Flags: review?(dtownsend) → review?(dolske)
Comment 6•15 years ago
|
||
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.
Assignee | ||
Comment 7•15 years ago
|
||
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.
Assignee | ||
Comment 8•15 years ago
|
||
Attachment #431290 -
Attachment is obsolete: true
Attachment #432404 -
Flags: review?(dolske)
Attachment #431290 -
Flags: review?(dolske)
Comment 9•15 years ago
|
||
(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 10•15 years ago
|
||
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+
Assignee | ||
Comment 11•15 years ago
|
||
(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.
Assignee | ||
Comment 12•15 years ago
|
||
Carrying forward r+
Attachment #432404 -
Attachment is obsolete: true
Attachment #433188 -
Flags: review+
Assignee | ||
Comment 13•15 years ago
|
||
Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/46ac7271006e
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•