600874 - Show restart notification for add-on updates

Status: VERIFIED WORKSFORME

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

When add-on updates are found, we add a label to the add-on item, but don't show a restart notification. If the updated add-on requires a restart, we should.

Not sure if we also want to show a system notification when this happens?

Relevant code:
http://mxr.mozilla.org/mobile-browser/source/chrome/content/extensions.js#673

Comment 1

[Matt Brubeck (:mbrubeck)]

Without the restart notification, there is no way to actually activate the new version of an addon that had been disabled for compatibility.

Comment 2

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

We should be showing a restart notification if the extension was marked as "updateable":
http://mxr.mozilla.org/mobile-browser/source/chrome/content/extensions.js#709

That allows us to customize the restart message:
http://mxr.mozilla.org/mobile-browser/source/chrome/content/extensions.js#797

but since we actually check before we set the _updating flag, it's worthless:
http://mxr.mozilla.org/mobile-browser/source/chrome/content/extensions.js#776

Looks like we need to re-arrange the code a bit there (and figure out why it was moved in the first place - I don't want to regress something else)

Comment 3

[Wesley Johnston (:wesj)]

I think this is fallout from Bug 562495. It looks like XPInstallDownloadManager had onInstallEnded and onInstallsCompleted methods that we collapsed into one single onInstallEnded method.

Comment 4

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

(In reply to comment #3)
> I think this is fallout from Bug 562495. It looks like XPInstallDownloadManager
> had onInstallEnded and onInstallsCompleted methods that we collapsed into one
> single onInstallEnded method.

OK, but fallout how?

Comment 5

[Wesley Johnston (:wesj)]

Attachment: Simple fix

Not quite sure what you mean finkle, but this should fix this. Its really hard to test though, so I'm going to try and get some browser chrome tests to make sure.

Comment 6

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

Comment on attachment 483209 [details] [diff] [review]
Simple fix

This will fix the type of message shown in the restart, but you did nothing extra to show the restart itself. Unless the fact we were accessing | this._updating |, which wasn't set, was throwing an error and exiting the method.

The fix is good in any case.

Comment 7

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

pushed:
http://hg.mozilla.org/mobile-browser/rev/641b08fc0e8e

Comment 8

[Wesley Johnston (:wesj)]

Whoops. This regressed (Bug 605102) because I forgot to move up the:
 element = ...
part. I have 90% of some tests written for this anyway, and hopefully I can get a better fix in today.

Comment 9

[Wesley Johnston (:wesj)]

Attachment: Fix + Tests

It seems like things are mostly working fine. If you update an extension or install an extension (non-bootstraped) we show a restart notification. Bootstrapped extensions don't provide much feedback that they've installed correctly, but that needs a different bug. The text in the update case was not updated to say anything about the upgrade, so I fixed that here, and added a few tests.

Comment 10

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

Comment on attachment 484108 [details] [diff] [review]
Fix + Tests

File a bug on the bootstrap add-on installation message?

Comment 11

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

pushed:
http://hg.mozilla.org/mobile-browser/rev/26a434136891

Comment 12

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Restart for addons are still not showing for Android/Maemo:

Mozilla/5.0 (Maemo;Linux armv71; rv:2.0b8pre) Gecko/20101019 Firefox/4.0b8pre Fennec/4.0b2pre
Mozilla/5.0 (Android; Linux armv71; rv2.0b8pre) Gecko/20101019 Firefox/4.0b8pre Fennec/4.0b2pre

Comment 13

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

Wes - Thoughts? Are the tests passing?

Comment 14

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Note: the addon was installed using a install trigger.
( bug 605801 )

Comment 15

[Wesley Johnston (:wesj)]

Naoki, I've been playing with this a lot lately and haven't been able to reproduce with my addons. Can you give me some more specific steps?

Comment 16

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

WFM:

Mozilla/5.0 (Maemo;Linux armv71; rv:2.0b8pre) Gecko/20101104 Firefox/4.0b8pre Fennec/4.0b3pre
Mozilla/5.0 (Android; Linux armv71; rv2.0b8pre) Gecko/20101104 Firefox/4.0b8pre Fennec/4.0b3pre

Comment 17

[Aakash Desai [:aakashd]]

WFM as well on builds:
Mozilla/5.0 (Maemo; Linux armv71; rv:2.0b8pre) Gecko/20101105 Namoroka/4.0b8pre Fennec/4.0b2pre

and

Mozilla/5.0 (Android; Linux armv71; rv:2.0b8pre) Gecko/20101105 Namoroka/4.0b8pre Fennec/4.0b2pre