Show download progress indicator when installing add-ons

VERIFIED FIXED in mozilla2.0b9

Status

()

Toolkit
Add-ons Manager
VERIFIED FIXED
7 years ago
6 years ago

People

(Reporter: rdoherty, Assigned: mossop)

Tracking

(Depends on: 2 bugs)

Trunk
mozilla2.0b9
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: [AddonsRewrite][strings])

Attachments

(5 attachments, 7 obsolete attachments)

(Reporter)

Description

7 years ago
Currently personas are installed 'silently' with no progress indicators to notify the user the persona images are being downloaded. This is a problem for most users as it takes 3-10 seconds for the images to download. During this time they have no idea what is occurring. We should have some sort of progress indicator showing that Firefox is downloading the persona.
It's not only for Personas but for all types of extensions. This bug is somewhat related to bug 553455 where we request an install notification. Should we merge those two bugs or handle it differently? IMO it would be great when we could popup that doorhanger notification while we download the add-on.
Component: General → Add-ons Manager
Product: Firefox → Toolkit
QA Contact: general → add-ons.manager
Hardware: x86 → All
Whiteboard: [AddonsRewrite]
(Reporter)

Comment 2

7 years ago
I think this bug is different than bug 553455. This is for a progress bar when downloading the persona, not when it's loaded. Bug 553455 seems to talk about notifications for when add-ons have finished installing.
I would leave this decision for Dave and Jennifer. But using the same notification while we download an add-on would be nice. It could look similar to what we wanna have for the download manager progress bars.

For larger add-ons the user really doesn't get any feedback what's happen after he accepted the installation.
blocking2.0: --- → ?
Summary: Show progress indicator when installing a persona → Show progress indicator when installing add-ons
(Assignee)

Comment 4

7 years ago
Blocking for review by UX
blocking2.0: ? → final+
Summary: Show progress indicator when installing add-ons → Show download progress indicator when installing add-ons
Created attachment 458923 [details]
Sketch: add-on being downloaded within new download manager

Ideally we would treat the download of a background or add-on identically to the download of any file, by showing it within the download manager.  It makes sense to treat them similarly - it eliminates the need to create more than one download manager while giving the user a consistent place to always find all of their downloaded content.  The download manager in Firefox 4.0 is the subject of a redesign (see attachment), but until this lands perhaps the download can be slotted into the current download manager.

The exception to showing displaying the download manager with a download is if the download happens entirely within the add-ons manager.  The file should still appear in the download manager in this case, but the manager need not appear.

For more on Firefox 4.0 download manager behavior, please see Limi's post here: http://limi.net/articles/improving-download-behaviors-web-browsers/
(Assignee)

Comment 6

7 years ago
I am fairly skeptical that the proposed download manager redesign is going to make Firefox 4 but the upshot from this (and a short conversation on IRC) is that we should try to re-use the download manager in whatever form it is. I already have half a patch that does this for XPI add-ons so I may as well take this.
Assignee: nobody → dtownsend

Comment 7

7 years ago
(In reply to comment #5)
> Created attachment 458923 [details]
> Sketch: add-on being downloaded within new download manager

Great painting. I love it!
(Assignee)

Updated

7 years ago
Keywords: uiwanted
(Reporter)

Comment 8

7 years ago
(In reply to comment #5)
> Created attachment 458923 [details]
> Sketch: add-on being downloaded within new download manager

I'm wondering if personas might need some additional or different UI as Firefox currently shows a notification bar after applying a persona.
Still not sure how likely it is for 4.0, but fwiw a patch has been posted on bug 564934 for the new download manager UI.
Shall we add it to the dependency list? Or is it now a hard-blocker for us and we have to find an interim solution?
(Assignee)

Comment 11

7 years ago
(In reply to comment #10)
> Shall we add it to the dependency list? Or is it now a hard-blocker for us and
> we have to find an interim solution?

Unless they are completely rewriting the download manager it isn't a dependency I think.
I imagine the current situation of no notifications is confusing for users installing large add-ons.

e.g. Kidzui, 5MB+
https://addons.mozilla.org/en-US/firefox/addon/9689/
From the time I clicked the final install button to when I saw the install dialog on my connection I counted 11 seconds.

Updated

7 years ago
Duplicate of this bug: 591943
(Assignee)

Comment 14

7 years ago
The plan for implementing this is now useless since we no longer have notifications for downloads in the status bar, which means we either have to drop this (if we are content with not showing the user that files are downloading then I'm not sure why we wouldn't be for add-ons) or we have to come up with some new UI, except we're right up against feature freeze so we need to consider this some more.
blocking2.0: final+ → ?
(Assignee)

Comment 15

7 years ago
From IRC beltzner agrees that this sucks but we aren't going to block on it.
Assignee: dtownsend → nobody
blocking2.0: ? → -
(Assignee)

Comment 16

7 years ago
And about face. Have a plan in progress, will probably need to pre-land some strings for some simple UI that we're cooking up.
Assignee: nobody → dtownsend
blocking2.0: - → beta8+
Whiteboard: [AddonsRewrite] → [AddonsRewrite][strings]
Clear to pre-land the strings on this one; Axel, be aware that I'm calling an audible now on the possibility that these might come in after b7 :(
(Assignee)

Comment 18

7 years ago
Created attachment 479450 [details]
Download progress UI

This is the planned UI showing download progress, should be shown near immediately after the user clicks install and after it completes the install confirmation dialog will appear.
Attachment #458923 - Attachment is obsolete: true
(Assignee)

Comment 19

7 years ago
Created attachment 479451 [details]
Cancelled download UI

If the user cancels the download then this should get shown
(Assignee)

Comment 20

7 years ago
Created attachment 479455 [details] [diff] [review]
strings patch rev 1

Spoke with Axel over IRC and he is happy for us to just re-use the download manager strings for the ETA messaging under the progress bar so these are the only strings we need to add here.
Attachment #479455 - Flags: review?(gavin.sharp)
(Assignee)

Updated

7 years ago
Attachment #479455 - Attachment is obsolete: true
Attachment #479455 - Flags: review?(gavin.sharp)
(Assignee)

Comment 21

7 years ago
Created attachment 479468 [details] [diff] [review]
strings patch rev 2
Attachment #479468 - Flags: review?
(Assignee)

Updated

7 years ago
Attachment #479468 - Flags: review? → review?(gavin.sharp)
Dave, does it also apply for add-on installations via the search pane? Will we drop the download progress meter in favor of the notification bar?
(Assignee)

Comment 23

7 years ago
(In reply to comment #22)
> Dave, does it also apply for add-on installations via the search pane? Will we
> drop the download progress meter in favor of the notification bar?

No
Attachment #479468 - Flags: review?(gavin.sharp) → review+
(Assignee)

Updated

7 years ago
Depends on: 593535
(Assignee)

Comment 24

7 years ago
Created attachment 479631 [details] [diff] [review]
strings patch rev 3

Sorry, came across two more strings, a tooltip for the cancel button and an access key for the restart button.
Attachment #479468 - Attachment is obsolete: true
Attachment #479631 - Flags: review?(gavin.sharp)
(Assignee)

Comment 25

7 years ago
Created attachment 479632 [details] [diff] [review]
WIP

This is the WIP that uses the strings, it is working aside from needing bug 593535 to be fixed for restarting to work and some odd issue with error cases, and of course it lacks tests right now.
(Assignee)

Updated

7 years ago
Attachment #479631 - Attachment is obsolete: true
Attachment #479631 - Flags: review?(gavin.sharp)
(Assignee)

Comment 26

7 years ago
Created attachment 479633 [details] [diff] [review]
strings patch rev 3 [checked in]

And this corrects the localization note.
Attachment #479633 - Flags: review?(gavin.sharp)
Comment on attachment 479633 [details] [diff] [review]
strings patch rev 3 [checked in]

Hmm, is it safe to use only one accesskey to cover multiple plural forms? I suppose it likely is.
Attachment #479633 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 28

7 years ago
Landed the strings: http://hg.mozilla.org/mozilla-central/rev/0b62fa6416d9
Whiteboard: [AddonsRewrite][strings] → [AddonsRewrite][strings][strings landed]
(Assignee)

Updated

7 years ago
Attachment #479633 - Attachment description: strings patch rev 3 → strings patch rev 3 [checked in]

Comment 29

7 years ago
No idea how the translate toolkit and narro are going to deal with http://hg.mozilla.org/mozilla-central/file/0b62fa6416d9/browser/locales/en-US/chrome/browser/browser.properties#l42 and their attempts to inline accesskeys. CCing :l10n for input.
Does the "restart" button restart the download from the first byte or does it resume the download from where it was previously stopped? I guess the former, but it might not be completely clear to the localizers.
(Assignee)

Comment 31

7 years ago
(In reply to comment #30)
> Does the "restart" button restart the download from the first byte or does it
> resume the download from where it was previously stopped? I guess the former,
> but it might not be completely clear to the localizers.

It is the former, although the network cache may have the first part of the file so it might almost resume automatically.
(Assignee)

Updated

7 years ago
Whiteboard: [AddonsRewrite][strings][strings landed] → [AddonsRewrite][strings][strings landed][needs 593535]
(Assignee)

Updated

7 years ago
blocking2.0: beta8+ → betaN+
(Assignee)

Updated

7 years ago
Whiteboard: [AddonsRewrite][strings][strings landed][needs 593535] → [AddonsRewrite][strings][strings landed]
(Assignee)

Comment 32

7 years ago
This is going to need the patches from bug 570012 to land
Depends on: 611076
(In reply to comment #32)
> This is going to need the patches from bug 570012 to land

I'm guessing you meant bug 611076?
(Assignee)

Comment 34

7 years ago
(In reply to comment #33)
> (In reply to comment #32)
> > This is going to need the patches from bug 570012 to land
> 
> I'm guessing you meant bug 611076?

Yep. I basically have this bug fixed now just need to verify that it looks ok on Windows and Linux.
(Assignee)

Comment 35

7 years ago
Created attachment 490909 [details] [diff] [review]
patch rev 1

This implements the doorhanger progress meter for add-on installs as well as supporting cancelling and then restarting the install. Hopefully fairly straightforward. The test changes look a lot worse than they are, I'll also attach a diff with whitespace ignored for that file where it is more obvious that it is mostly additions.
Attachment #490909 - Flags: review?(gavin.sharp)
(Assignee)

Comment 36

7 years ago
Created attachment 490910 [details] [diff] [review]
diff -w of the test file
(Assignee)

Updated

7 years ago
Whiteboard: [AddonsRewrite][strings][strings landed] → [AddonsRewrite][strings][strings landed][has patch][needs review gavin]

Comment 37

7 years ago
I'm not a fan of en-US strings hard-coded in tests, but this doesn't make this test file much worse, I guess.

Gavin, I'd like to ramp down on our pre-landed strings, can you get to the review of this one?
button.click() ftw :)
Duplicate of this bug: 613501
Comment on attachment 490909 [details] [diff] [review]
patch rev 1

In testing, I was able to get into a broken state by canceling the download progress notification immediately before the prompt appeared. I'm not sure exactly what state the download was in, but I ended up hitting http://hg.mozilla.org/mozilla-central/annotate/2b44a6a3bfd8/toolkit/mozapps/extensions/XPIProvider.jsm#l4956 and so I couldn't close the installation confirmation prompt.

A couple of minor issues I also noticed, perhaps not worth fixing:
- The case of multiple installs where one fails and the other succeeds is somewhat confusing, because you see a flash of the "download failed" notification for the failed one, then the install prompt for the successful one, and then the "failed" notification is dismissed and doesn't really make it clear that it's for one of two that failed. Rather edge-casey I guess, and I'm not sure I have better suggestions for how that should be handled.
- The "restart download" button for canceled downloads re-triggers the "Minefield prevented this site from asking you..." dialog rather than immediately resuming the download, which is somewhat suboptimal.

>diff --git a/browser/base/content/urlbarBindings.xml b/browser/base/content/urlbarBindings.xml

>+  <binding id="addon-progress-notification" extends="chrome://global/content/bindings/notification.xml#popup-notification">

>+        <xul:description class="popup-notification-description"
>+                         xbl:inherits="value=label"/>

This needs to be updated per http://hg.mozilla.org/mozilla-central/rev/2df08dfa1b22 .

>+      <destructor><![CDATA[
>+        this.notification.options.installs.forEach(function(aInstall) {
>+          aInstall.removeListener(this);
>+        }, this);
>+      ]]></destructor>

This won't necessarily be called immediately on notification removal, given the way PopupNotifications.jsm works (it only clears old notifications if on show, rather than hide). Perhaps it would be better to have this code in a destroy() that's called from updateProgress() (next to the remove() call).

>+      <method name="setProgress">

>+          let speed = (aProgress - this.notification.lastProgress) / delta;
>+          if (this.notification.speed)
>+            speed = speed * 0.9 + this.notification.speed * 0.1;

Would be good to add a comment mentioning that this logic is the same as in nsDownloadManager.

>+      <method name="cancel">

>+          let action = {
>+            label: buttonText,
>+            accessKey: gNavigatorBundle.getString("addonDownloadRestart.accessKey"),
>+            callback: function() {
>+              AddonManager.installAddonsFromWebpage("application/x-xpinstall",
>+                                                    browser.contentWindow,
>+                                                    browser.currentURI, installs);

browser.contentWindow and browser.currentURI could be from entirely different sites when this is triggered which seems like it would be problematic...

>diff --git a/browser/themes/pinstripe/browser/browser.css b/browser/themes/pinstripe/browser/browser.css

>+.popup-progress-cancel:hover {
>+  -moz-image-region: rect(0px, 32px, 16px, 16px);
>+}

This styling is nearly impossible to notice given that the image's background is the dark notification panel (the change in opacity/darkness isn't really visiblee on hover), but I suppose it doesn't hurt to leave it. Might want to revisit this styling later I guess. I assume you tested the styling on the other platforms (I only tested Mac).
Attachment #490909 - Flags: review?(gavin.sharp) → review-
(Assignee)

Updated

7 years ago
Whiteboard: [AddonsRewrite][strings][strings landed][has patch][needs review gavin] → [AddonsRewrite][strings][strings landed][has patch]
(Assignee)

Comment 41

7 years ago
(In reply to comment #40)
> Comment on attachment 490909 [details] [diff] [review]
> patch rev 1
> 
> In testing, I was able to get into a broken state by canceling the download
> progress notification immediately before the prompt appeared. I'm not sure
> exactly what state the download was in, but I ended up hitting
> http://hg.mozilla.org/mozilla-central/annotate/2b44a6a3bfd8/toolkit/mozapps/extensions/XPIProvider.jsm#l4956
> and so I couldn't close the installation confirmation prompt.

I wasn't able to reproduce this, was it with a specific add-on or collection of add-ons? The attached patch at least adds a little extra logging to say what state it was in when it failed that.

> A couple of minor issues I also noticed, perhaps not worth fixing:
> - The case of multiple installs where one fails and the other succeeds is
> somewhat confusing, because you see a flash of the "download failed"
> notification for the failed one, then the install prompt for the successful
> one, and then the "failed" notification is dismissed and doesn't really make it
> clear that it's for one of two that failed. Rather edge-casey I guess, and I'm
> not sure I have better suggestions for how that should be handled.

Yeah not sure what the solution is there so deferring it to a follow-up

> - The "restart download" button for canceled downloads re-triggers the
> "Minefield prevented this site from asking you..." dialog rather than
> immediately resuming the download, which is somewhat suboptimal.

Fixed that by bypassing part of the add-ons manager code.

> >diff --git a/browser/base/content/urlbarBindings.xml b/browser/base/content/urlbarBindings.xml
> 
> >+  <binding id="addon-progress-notification" extends="chrome://global/content/bindings/notification.xml#popup-notification">
> 
> >+        <xul:description class="popup-notification-description"
> >+                         xbl:inherits="value=label"/>
> 
> This needs to be updated per
> http://hg.mozilla.org/mozilla-central/rev/2df08dfa1b22 .

Fixed, also had to make the size slightly larger than that allowed as the progress messages were a but too long.

> >+      <destructor><![CDATA[
> >+        this.notification.options.installs.forEach(function(aInstall) {
> >+          aInstall.removeListener(this);
> >+        }, this);
> >+      ]]></destructor>
> 
> This won't necessarily be called immediately on notification removal, given the
> way PopupNotifications.jsm works (it only clears old notifications if on show,
> rather than hide). Perhaps it would be better to have this code in a destroy()
> that's called from updateProgress() (next to the remove() call).

Done

> >+      <method name="setProgress">
> 
> >+          let speed = (aProgress - this.notification.lastProgress) / delta;
> >+          if (this.notification.speed)
> >+            speed = speed * 0.9 + this.notification.speed * 0.1;
> 
> Would be good to add a comment mentioning that this logic is the same as in
> nsDownloadManager.

Done

> >+      <method name="cancel">
> 
> >+          let action = {
> >+            label: buttonText,
> >+            accessKey: gNavigatorBundle.getString("addonDownloadRestart.accessKey"),
> >+            callback: function() {
> >+              AddonManager.installAddonsFromWebpage("application/x-xpinstall",
> >+                                                    browser.contentWindow,
> >+                                                    browser.currentURI, installs);
> 
> browser.contentWindow and browser.currentURI could be from entirely different
> sites when this is triggered which seems like it would be problematic...

Cached these in notification.options.

> >diff --git a/browser/themes/pinstripe/browser/browser.css b/browser/themes/pinstripe/browser/browser.css
> 
> >+.popup-progress-cancel:hover {
> >+  -moz-image-region: rect(0px, 32px, 16px, 16px);
> >+}
> 
> This styling is nearly impossible to notice given that the image's background
> is the dark notification panel (the change in opacity/darkness isn't really
> visiblee on hover), but I suppose it doesn't hurt to leave it. Might want to
> revisit this styling later I guess. I assume you tested the styling on the
> other platforms (I only tested Mac).

I basically copied the styles from the download manager so I guess it's true they may not be ideal, but I'd rather just land with these and then if we have time to polish up after the fact then we do it then.
Whiteboard: [AddonsRewrite][strings][strings landed][has patch] → [AddonsRewrite][strings][strings landed][has patch][waiting on try]
(Assignee)

Comment 42

7 years ago
Created attachment 497948 [details] [diff] [review]
patch rev 2

Updated from comments
Attachment #490909 - Attachment is obsolete: true
Attachment #490910 - Attachment is obsolete: true
Attachment #497948 - Flags: review?(gavin.sharp)
Attachment #497948 - Attachment is patch: true
Attachment #497948 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 497948 [details] [diff] [review]
patch rev 2

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>+    case "addon-install-started":
>+      function needsDownload(aInstall) {
>+        return aInstall.state != AddonManager.STATE_DOWNLOADED;
>+      }

Should this be state < STATE_DOWNLOADED?

>+      options.contentWindow = browser.contentWindow;
>+      options.sourceURI = browser.currentURI;

This is kind of scary from a leak-potential POV... Is it crazy to suggest that we just not have the retry functionality? Accidentally cancelling doesn't seem that likely, and in the common case (e.g. AMO, pages with "install now" buttons) retriggering an install manually isn't difficult.

>diff --git a/browser/base/content/test/browser_bug553455.js b/browser/base/content/test/browser_bug553455.js

>+  //Services.obs.addObserver(function (aSubject, aTopic, aData) {
>+  //  Services.obs.removeObserver(arguments.callee, "PopupNotifications-updateShown");
>+  //  ok(!PopupNotifications.panel.hidden, "Popup panel should not be hidden");
>+  //}, "PopupNotifications-updateShown", false);

I guess this should be removed?

>diff --git a/browser/base/content/urlbarBindings.xml b/browser/base/content/urlbarBindings.xml

>+  <binding id="addon-progress-notification" extends="chrome://global/content/bindings/notification.xml#popup-notification">

Now this needs to be updated for http://hg.mozilla.org/mozilla-central/rev/2df08dfa1b22 :/

>+      <constructor><![CDATA[

Is it possible that by the time this constructor is called (i.e. between the check before show() in browser.js and this constructor running), the installs all complete, such that no more install progress events will be fired? Seems like we would leave the notification around indefinitely in that case.

>diff --git a/browser/themes/gnomestripe/browser/browser.css b/browser/themes/gnomestripe/browser/browser.css

>+#addon-progress-notification .popup-notification-description {

You could give your binding's popup-notification-description an additional specific class (addon-progress-description?) to avoid the need to use the descendent selector here (and in other themes).
Comment on attachment 497948 [details] [diff] [review]
patch rev 2

cancelling review pending response to comment 43.
Attachment #497948 - Flags: review?(gavin.sharp)
(Assignee)

Comment 45

7 years ago
(In reply to comment #43)
> Comment on attachment 497948 [details] [diff] [review]
> patch rev 2
> 
> >diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
> 
> >+    case "addon-install-started":
> >+      function needsDownload(aInstall) {
> >+        return aInstall.state != AddonManager.STATE_DOWNLOADED;
> >+      }
> 
> Should this be state < STATE_DOWNLOADED?

No, because this can be called to restart a failed download then it can be in the cancelled/failed states.

> >+      options.contentWindow = browser.contentWindow;
> >+      options.sourceURI = browser.currentURI;
> 
> This is kind of scary from a leak-potential POV... Is it crazy to suggest that
> we just not have the retry functionality? Accidentally cancelling doesn't seem
> that likely, and in the common case (e.g. AMO, pages with "install now"
> buttons) retriggering an install manually isn't difficult.

I'll chat with Boriss about it, at this point I'm inclined to say we should just take it without the restart stuff.

> >diff --git a/browser/base/content/test/browser_bug553455.js b/browser/base/content/test/browser_bug553455.js
> 
> >+  //Services.obs.addObserver(function (aSubject, aTopic, aData) {
> >+  //  Services.obs.removeObserver(arguments.callee, "PopupNotifications-updateShown");
> >+  //  ok(!PopupNotifications.panel.hidden, "Popup panel should not be hidden");
> >+  //}, "PopupNotifications-updateShown", false);
> 
> I guess this should be removed?

Oops sorry, left-over experimentation code

> >diff --git a/browser/base/content/urlbarBindings.xml b/browser/base/content/urlbarBindings.xml
> 
> >+  <binding id="addon-progress-notification" extends="chrome://global/content/bindings/notification.xml#popup-notification">
> 
> Now this needs to be updated for
> http://hg.mozilla.org/mozilla-central/rev/2df08dfa1b22 :/

Didn't I already do that? That is why I needed the styles to fix the width that that patch forced

> >+      <constructor><![CDATA[
> 
> Is it possible that by the time this constructor is called (i.e. between the
> check before show() in browser.js and this constructor running), the installs
> all complete, such that no more install progress events will be fired? Seems
> like we would leave the notification around indefinitely in that case.

In all my tests the constructor was getting called immediately on creation from PopupNotifications.show, in fact that was causing problems because if a notification constructor removes the notification then we recurse into PopupNotifications._update in a way that causes problems. Assuming this remains the case then this should be ok, if not then I don't know how to deal with both cases without fixing PopupNotifications to handle the re-entrency.
(In reply to comment #45)
> > Now this needs to be updated for
> > http://hg.mozilla.org/mozilla-central/rev/2df08dfa1b22 :/
> 
> Didn't I already do that? That is why I needed the styles to fix the width that
> that patch forced

Er sorry I meant http://hg.mozilla.org/mozilla-central/rev/545fcdd02090 . (MXR is out of date and linked to the wrong thing)
(Assignee)

Comment 47

7 years ago
(In reply to comment #46)
> (In reply to comment #45)
> > > Now this needs to be updated for
> > > http://hg.mozilla.org/mozilla-central/rev/2df08dfa1b22 :/
> > 
> > Didn't I already do that? That is why I needed the styles to fix the width that
> > that patch forced
> 
> Er sorry I meant http://hg.mozilla.org/mozilla-central/rev/545fcdd02090 . (MXR
> is out of date and linked to the wrong thing)

MARGARET!!!
(Assignee)

Updated

7 years ago
Whiteboard: [AddonsRewrite][strings][strings landed][has patch][waiting on try] → [AddonsRewrite][strings][strings landed][has patch]
(Assignee)

Comment 48

7 years ago
Created attachment 498949 [details] [diff] [review]
patch rev 3

Updated from the comments. I've kept the cancel/restart bits since Boriss really really wanted them. I think we should be relatively safe on the leak front, we aren't creating a cycle that I can see, the content window that we hold onto doesn't have any reference to the notification but to be safer I've made the destroy method null out any references and made sure it is called whenever the notification finishes downloading, is cancelled or is just removed due to tab closure.
Attachment #497948 - Attachment is obsolete: true
Attachment #498949 - Flags: review?(gavin.sharp)
(Assignee)

Updated

7 years ago
Whiteboard: [AddonsRewrite][strings][strings landed][has patch] → [AddonsRewrite][strings][strings landed][has patch][needs review gavin]
I was able to reproduce that error again with your latest patch:
Error: Cannot cancel install of http://gavinsharp.com/tmp/badaddon.xpi from this state (4)
Source File: resource://gre/modules/XPIProvider.jsm
Line: 4939

4 is DOWNLOAD_FAILED. I guess the cancel()s should be wrapped in a try/catch, since this seems valid for addons to be in this state when the cancel button is clicked.
Comment on attachment 498949 [details] [diff] [review]
patch rev 3

>diff --git a/browser/base/content/urlbarBindings.xml b/browser/base/content/urlbarBindings.xml

>+  <binding id="addon-progress-notification" extends="chrome://global/content/bindings/notification.xml#popup-notification">

>+          <xul:button anonid="button"
>+                      type="menu-button"
>+                      class="popup-notification-menubutton"

nit: putting type after class matches the base binding and makes them easier to diff

>+      <constructor><![CDATA[

>+        this.notification.options.installs.forEach(function(aInstall) {
>+          aInstall.addListener(this);
>+        }, this);

Call me paranoid, but perhaps you should executeSoon() calls to destroy()/PopupNotifications.remove(this.notification) and return early here if !installs.some(function (i) i.state < AddonManager.STATE_DOWNLOADED) to protect against the case from the last part of comment 45 (if we somehow stop calling this constructor so eagerly in the future)? Not adamant about it so if you think it's not worth it just forget it.

>+      <method name="setProgress">

>+          let utils = {};
>+          Components.utils.import("resource://gre/modules/DownloadUtils.jsm", utils);

I think subsequent import()s of the same module are generally cheap, but it still seems like this could benefit from being in a <field>.

>+      <method name="cancel">

>+          installs.forEach(function(aInstall) {
>+            aInstall.cancel();
>+          }, this);

Think this wants a try/catch per comment 49.

r=me with those addressed.
Attachment #498949 - Flags: review?(gavin.sharp) → review+
(Assignee)

Updated

7 years ago
Whiteboard: [AddonsRewrite][strings][strings landed][has patch][needs review gavin] → [AddonsRewrite][strings][strings landed][has patch]
(Assignee)

Comment 51

7 years ago
(In reply to comment #50)
> >+      <constructor><![CDATA[
> 
> >+        this.notification.options.installs.forEach(function(aInstall) {
> >+          aInstall.addListener(this);
> >+        }, this);
> 
> Call me paranoid, but perhaps you should executeSoon() calls to
> destroy()/PopupNotifications.remove(this.notification) and return early here if
> !installs.some(function (i) i.state < AddonManager.STATE_DOWNLOADED) to protect
> against the case from the last part of comment 45 (if we somehow stop calling
> this constructor so eagerly in the future)? Not adamant about it so if you
> think it's not worth it just forget it.

I didn't like the idea of pushing events to the thread queue from release code (do we do that anywhere else?) so I left that for the time being, we can fix it in a standalone bug if necessary.

Landed: http://hg.mozilla.org/mozilla-central/rev/4294a3a4edf3
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Flags: in-litmus-
Resolution: --- → FIXED
Whiteboard: [AddonsRewrite][strings][strings landed][has patch] → [AddonsRewrite][strings]
Target Milestone: --- → mozilla2.0b9
Blocks: 467530
Depends on: 623192
Depends on: 623195
This looks great! It's definitely one of the most visual updates for the AOM in the last couple weeks. Thanks Dave! Verified across platforms with builds like Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b9pre) Gecko/20110104 Firefox/4.0b9pre ID:20110104030354

I filed two follow-up bugs, which I have noticed during testing this feature. Otherwise lets mark this bug as verified.
Status: RESOLVED → VERIFIED
Blocks: 595810
No longer blocks: 467530
Depends on: 625050

Updated

6 years ago
Depends on: 632337

Updated

6 years ago
Depends on: 644234
You need to log in before you can comment on or make changes to this bug.