619432 - AUS notification fails and is not offered again if Fennec closes during update

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Daniel Einspanjer [:dre] [:deinspanjer]]

Mozilla/5.0 (Android; Linux armv7l; rv:2.0b8pre) Gecko/20101214 Firefox/4.0b8pre Fennec/4.0b3pre

STR:

Terminate any running instance of Fennec
Start Fennec
Go to "About Fennec" page
Click Check for Updates
See notification of new update available
Click notification
Begin downloading update
Terminate Fennec
Start Fennec
Go to "About Fennec" page
Click Check for Updates

Expected results:
Notification of new update is displayed again

Actual results:
The button says an update is available, but the notification icon does not appear.
At some point in the future it fixes itself and the update will be offered, but I don't know what the criteria is for fixing it.

Comment 1

[Tony Chung [:tchung]]

i've seen this myself.  Triggering the check for updates should bring the notification back.

Comment 2

[Alex Pakhotin (:alexp)]

Attachment: Fix

Check for update in progress on browser startup.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 500153 [details] [diff] [review]
Fix

The only part of this patch I don't like is the need for gIsNotificationDisplayed. Do we really need it?

Why are we using the raw alerts service in checkForUpdates instead of using _showNotification?

Comment 4

[Alex Pakhotin (:alexp)]

(In reply to comment #3)

> The only part of this patch I don't like is the need for
> gIsNotificationDisplayed. Do we really need it?

Well, it's not really required, nothing should break without it, that's just an attempt to avoid extra calls to UpdateManager and AlertService in case if the notification was already displayed. But currently we'll probably have the only call to this checkForUpdates, so not having that global flag should not hurt.

> Why are we using the raw alerts service in checkForUpdates instead of using
> _showNotification?

The update being downloaded is not provided to this function, it is obtained from nsIUpdateManager.activeUpdate, which theoretically could be null, so passing it as-is to _showNotification and from there to _handleUpdate would be dangerous.
But I guess I could check aUpdate for null in _handleUpdate and just return from there if it's not valid.

Comment 5

[Alex Pakhotin (:alexp)]

(In reply to comment #4)
> But I guess I could check aUpdate for null in _handleUpdate and just return
> from there if it's not valid.

Looks like even this is not required, as _handleUpdate uses the aUpdate argument only for other modes. OK, I'll change to _showNotification (as a matter of fact, this was my original intention).

Comment 6

[Alex Pakhotin (:alexp)]

Attachment: Fix v2

Fixed review comments.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 502188 [details] [diff] [review]
Fix v2

Thanks for the extra tweaks. Looks good.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed:
http://hg.mozilla.org/mobile-browser/rev/670e6b6c2140

Comment 9

[Aakash Desai [:aakashd]]

verified FIXED on build:

Mozilla/5.0 (Android; Linux armv71; rv:2.0b9pre) Gecko/20100109 Namoroka/4.0b9pre Fennec/4.0b4pre

Comment 10

[Ioana Chiorean]

https://litmus.mozilla.org/show_test.cgi?id=15211