Closed Bug 553868 Opened 14 years ago Closed 14 years ago

onUpdateFinished doesn't specify if there was no update found

Categories

(Toolkit :: Add-ons Manager, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9.3a5

People

(Reporter: Unfocused, Assigned: mossop)

References

Details

(Whiteboard: [rewrite])

Attachments

(1 file, 1 obsolete file)

onUpdateFinished is fired regardless of whether there was an update found or not. It should specify if there was no update found, either by an additional parameter, or by using the error parameter.

An alternative approach could be to fire a different event, such as onNoUpdateFound.
Blocks: 553870
Isn't this something you can handle yourself in the listener? You could flag in  onUpdateAvailable or onCompatibilityUpdated so you can see in onUpdateFinished that whatever appropriate was found. I guess I could add two boolean parameters to do that but that doesn't seem to add much, and adding the actual update seems to make the onUpdateAvailable event pointless. Is the API just wrong here?
Flags: in-testsuite?
Flags: in-litmus-
(In reply to comment #1)
> Isn't this something you can handle yourself in the listener? You could flag in
>  onUpdateAvailable or onCompatibilityUpdated so you can see in onUpdateFinished
> that whatever appropriate was found.

I could, yes - but it feels like something the API should be providing. 

I do think the API is wrong here. Trying to detect something based on the lack of an event isn't right.

> I guess I could add two boolean parameters
> to do that but that doesn't seem to add much, and adding the actual update
> seems to make the onUpdateAvailable event pointless.

Agreed - I think the events need rethought a bit. How about:

onCheckingUpdate (new)
onCompatibilityUpdated
onUpdateAvailable 
onNoUpdateFound (new)


onCheckingUpdate would be nice for some UI feedback. It could be fired only when its a user-requested update check, or pass the update type as a parameter (perfered).


Additionally, it seems that if the listener has onCompatibilityUpdated defined, the update reason is changed. I don't think the UI will use that, but consumers in general should be able to listen for compat updates without changing the reason that's sent. Can file a new bug for that, if needed.
We now have the following events:

onCompatibilityUpdated
onUpdateAvailable
onNoUpdateAvailable
onUpdateFinished

http://hg.mozilla.org/projects/addonsmgr/rev/04e9066af247
Flags: in-testsuite? → in-testsuite+
(In reply to comment #2)
> Additionally, it seems that if the listener has onCompatibilityUpdated defined,
> the update reason is changed. I don't think the UI will use that, but consumers
> in general should be able to listen for compat updates without changing the
> reason that's sent. Can file a new bug for that, if needed.

This is kind of intentional, but lets look at it in bug 553123
Status: NEW → ASSIGNED
Whiteboard: [rewrite] → [rewrite][fixed-in-addonsmgr][needs-review]
Attached patch patch rev 1 (obsolete) (deleted) — Splinter Review
Changes the update events fired to those specified by comment 3
Attachment #435794 - Flags: review?(robert.bugzilla)
Comment on attachment 435794 [details] [diff] [review]
patch rev 1

r=me as long as the new events are tested here or via tests in one of the other bugs.
Attachment #435794 - Flags: review?(robert.bugzilla) → review+
Whiteboard: [rewrite][fixed-in-addonsmgr][needs-review] → [rewrite][fixed-in-addonsmgr]
Attached patch patch rev 2 (deleted) — Splinter Review
Found an error in this patch where I had mistyped onNoUpdateAvailable as onNoUpdateFound. onNoUpdateAvailable is the only new event added here and it is tested in this patch and the patch in bug 552732.
Assignee: nobody → dtownsend
Attachment #435794 - Attachment is obsolete: true
Attachment #437344 - Flags: review?(robert.bugzilla)
Whiteboard: [rewrite][fixed-in-addonsmgr] → [rewrite][fixed-in-addonsmgr][needs-review]
Attachment #437344 - Flags: review?(robert.bugzilla) → review+
Whiteboard: [rewrite][fixed-in-addonsmgr][needs-review] → [rewrite][fixed-in-addonsmgr]
Whiteboard: [rewrite][fixed-in-addonsmgr] → [rewrite][fixed-in-addonsmgr][needs-landing]
http://hg.mozilla.org/mozilla-central/rev/1ce03040d7a4
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [rewrite][fixed-in-addonsmgr][needs-landing] → [rewrite]
Target Milestone: --- → mozilla1.9.3a5
Marking as verified fixed based on check-in and passing automated tests.
Status: RESOLVED → VERIFIED
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: