Fix breakage and Ts regression from bug 474492

RESOLVED FIXED

Status

Fennec Graveyard
General
RESOLVED FIXED
9 years ago
4 years ago

People

(Reporter: mfinkle, Assigned: mfinkle)

Tracking

Trunk
x86
Windows XP
Bug Flags:
wanted-fennec1.0 +

Details

Attachments

(1 attachment, 2 obsolete attachments)

Created attachment 374169 [details] [diff] [review]
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
Attachment #374169 - Flags: review?(gavin.sharp)
Attachment #374169 - Flags: review?(gavin.sharp) → review+
Created attachment 374224 [details] [diff] [review]
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.
Assignee: nobody → mark.finkle
Attachment #374169 - Attachment is obsolete: true
Attachment #374224 - Flags: review?(gavin.sharp)
Flags: wanted-fennec1.0+
(Assignee)

Updated

9 years ago
tracking-fennec: --- → ?
(Assignee)

Updated

9 years ago
Duplicate of this bug: 493800
Created attachment 378879 [details] [diff] [review]
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)
Attachment #374224 - Attachment is obsolete: true
Attachment #378879 - Flags: review?(gavin.sharp)
Attachment #374224 - Flags: review?(gavin.sharp)
(Assignee)

Updated

9 years ago
Duplicate of this bug: 493799
This bug should be blocking b2 (it fixes b2 and b3 blockers)
Attachment #378879 - Flags: review?(gavin.sharp) → review+
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.
(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.
http://hg.mozilla.org/mobile-browser/rev/b058f033a4b6
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.