553868 - onUpdateFinished doesn't specify if there was no update found

Status: VERIFIED FIXED

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

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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.

Comment 1

[Dave Townsend [:mossop]]

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?

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(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.

Comment 3

[Dave Townsend [:mossop]]

We now have the following events:

onCompatibilityUpdated
onUpdateAvailable
onNoUpdateAvailable
onUpdateFinished

http://hg.mozilla.org/projects/addonsmgr/rev/04e9066af247

Comment 4

[Dave Townsend [:mossop]]

(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

Comment 5

[Dave Townsend [:mossop]]

Attachment: patch rev 1

Changes the update events fired to those specified by comment 3

Comment 6

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

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.

Comment 7

[Dave Townsend [:mossop]]

Attachment: patch rev 2

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.

Comment 8

[Dave Townsend [:mossop]]

http://hg.mozilla.org/mozilla-central/rev/1ce03040d7a4

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Marking as verified fixed based on check-in and passing automated tests.