489686 - Fix breakage and Ts regression from bug 474492

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

Attachment: patch

Moving the DownloadsView.init() and ExtensionsView.init() into BrowserUI.init() caused a ~100ms Ts regression and caused some errors when initiating a download or add-on installation because the views were not 100% inited.

This patch fixes the errors and attempts to fix the Ts

Comment 1

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

Attachment: alternative patch

I really could bring myself to land the previous patch. I wasn't happy with it. The Ts fixes were fine (moving extra init code into _delayedInit), but I didn't like the hack for forcing a _delayedInit when the managers were acted on by an outside trigger.

For downloads, we only needed to worry about showing the alert notifications if the view was not visible (and completely initialized). So I added a separate download progress listener whose purpose is to handle alerts. It does not need the view to be initialized to handle them. The old code mixed alerts and UI management together in a progress listener. This patch just splits them out.

For extensions, we don't need to use _delayedInit, since the outside trigger is already protected from partial view initialization (we check for "element" in the install listener). The bigger issue here is how and where we will add outside installs (add-ons installed from content) into the view's UI. I'll file a bug on that.

Overall, I like this patch better. It feels less hacky.

Comment 3

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

Attachment: alt patch 2

Same as the previous with some commented code removed and the code to jump to the add-on manager removed too (was bug 493799, but easier here)

Comment 5

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

This bug should be blocking b2 (it fixes b2 and b3 blockers)

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 378879 [details] [diff] [review]
alt patch 2

>diff --git a/chrome/content/downloads.js b/chrome/content/downloads.js

>+DownloadAlertsListener.prototype = {

>+  QueryInterface: function (aIID) {
>+    if (!aIID.equals(Ci.nsIDownloadProgressListener) &&

Implementation seems to be missing an getter for the "document" attribute.

Comment 7

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

(In reply to comment #6)
> (From update of attachment 378879 [details] [diff] [review])
> >diff --git a/chrome/content/downloads.js b/chrome/content/downloads.js
> 
> >+DownloadAlertsListener.prototype = {
> 
> >+  QueryInterface: function (aIID) {
> >+    if (!aIID.equals(Ci.nsIDownloadProgressListener) &&
> 
> Implementation seems to be missing an getter for the "document" attribute.

That seems to be unsed. No other implementers in Firefox have it either.

Comment 8

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

http://hg.mozilla.org/mobile-browser/rev/b058f033a4b6