581974 - Improve web installation failure messages, use doorhanger (port bug 552965 frontend)

Status: RESOLVED DUPLICATE of bug 595810

(SeaMonkey :: UI Design, defect)

Description

[Robert Kaiser]

Bug 55296 improved web installation failure messages in Firefox and converted them to use doorhangers, we should port that to SeaMonkey.

Comment 1

[Robert Kaiser]

Attachment: port the Firefox work

This ports the Firefox frontend work to SeaMonkey. The test passes, but I haven't done manual testing of this yet myself.

Comment 2

[[e-mail]]

Comment on attachment 460274 [details] [diff] [review]
port the Firefox work

So unfortunately you asked me for review of a patch that depends on a patch that has bitrotted...

>+__defineGetter__("AddonManager", function() {
>+  Components.utils.import("resource://gre/modules/AddonManager.jsm");
>+  return this.AddonManager;
>+});
>+__defineSetter__("AddonManager", function (val) {
>+  delete this.AddonManager;
>+  return this.AddonManager = val;
>+});
>+
>+
> function ReloadThemes()
> {
>   AddonManager.getAddonsByTypes(["theme"], function(themes) {
As you can see, AddonManager is already defined.

>+  _findChildShell: function (aDocShell, aSoughtShell)
>+  {
>+    if (aDocShell == aSoughtShell)
>+      return aDocShell;
>+
>+    var node = aDocShell.QueryInterface(Components.interfaces.nsIDocShellTreeNode);
>+    for (var i = 0; i < node.childCount; ++i) {
>+      var docShell = node.getChildAt(i);
>+      docShell = this._findChildShell(docShell, aSoughtShell);
>+      if (docShell == aSoughtShell)
>+        return docShell;
>+    }
>+    return null;
>+  },
>+
>+  _getBrowser: function (aDocShell)
>+  {
>+    for (var i = 0; i < gBrowser.browsers.length; ++i) {
>+      var browser = gBrowser.getBrowserAtIndex(i);
>+      if (this._findChildShell(browser.docShell, aDocShell))
>+        return browser;
>+    }
>+    return null;
>+  },
This is a lot easier in the notification bar :-( It also fails for the sidebar.

>+        enabled = gPrefService.getBoolPref("xpinstall.enabled");
Services.prefs surely?

Comment 3

[Robert Kaiser]

(In reply to comment #2)
> So unfortunately you asked me for review of a patch that depends on a patch
> that has bitrotted...

Well, I can't update the other patch until someone tells me exactly what code to use for that one thing where you requested me to use code I don't grasp.

> As you can see, AddonManager is already defined.

Interesting, didn't notice that, wonder where it comes from.

> This is a lot easier in the notification bar :-( It also fails for the sidebar.

Hmm, not sure how I can solve that (or if I should at all).

> >+        enabled = gPrefService.getBoolPref("xpinstall.enabled");
> Services.prefs surely?

Oh, sure, no idea why FF guys didn't directly use it (their gPrefService actually only refers to Servcies.prefs but that sounds like compat shim to me).

Comment 4

[[e-mail]]

Comment on attachment 460274 [details] [diff] [review]
port the Firefox work

>+addonInstallRestartButton=Restart Now
>+addonInstallRestartButton.accesskey=R
>+addonInstallManage=Open Add-ons Manager
>+addonInstallManage.accesskey=O
Shouldn't these be addonInstallManagerButton[.accesskey]?

>+addonError-4=#1 could not be installed because Firefox cannot modify the needed file.
This can't be right! (Fixed in bug 559265 comment 26.)

Comment 5

[Tony Mechelynck [:tonymec]]

(In reply to comment #4)
[...]
> >+addonError-4=#1 could not be installed because Firefox cannot modify the needed file.
> This can't be right! (Fixed in bug 559265 comment 26.)

This can't be right! The last comment to date in bug 559265 is #4.
From de dep-list, I guess you meant bug 552965 comment #26.

Comment 6

[Robert Kaiser]

(In reply to comment #2)
> As you can see, AddonManager is already defined.

Just realized we do a direct import of it. Well, if we ever start caring about navigator startup perf, this might be something to look into, but for now I'll leave it.

> This is a lot easier in the notification bar :-( It also fails for the sidebar.

Any easy way to address this? If not, worth a followup at least for the sidebar case?

(In reply to comment #4)
> Shouldn't these be addonInstallManagerButton[.accesskey]?

Well, creates another strange and unneeded difference to Firefox, but maybe we can coerce them to fix it as well. You're once again catching stuff their reviewers didn't. :-/

> >+addonError-4=#1 could not be installed because Firefox cannot modify the needed file.
> This can't be right! (Fixed in bug 559265 comment 26.)

Ah, good catch!

Comment 7

[Robert Kaiser]

Attachment: v1.1: address comments, integrate bug 577048 and bug 581229 work

This patch addresses the comments, and integrates bug 577048 and bug 581229 work. Of course, it needs the bug 570004 to be applied to work.

Comment 8

[[e-mail]]

(In reply to comment #5)
> (In reply to comment #4)
> > >+addonError-4=#1 could not be installed because Firefox cannot modify the needed file.
> > This can't be right! (Fixed in bug 559265 comment 26.)
> This can't be right! The last comment to date in bug 559265 is #4.
> From de dep-list, I guess you meant bug 552965 comment #26.
Yeah, I keep typoing bug numbers :-(

Comment 9

[Robert Kaiser]

Attachment: v1.2: also integrate a tiny bit of bug 572967 work

This patch also integrates a tiny bit of bug 572967 work to match my updated patch in the main sm-doorhanger bug.

Note to Neil: This review should be lower priority than the places stuff. ;-)

Comment 10

[Robert Kaiser]

Attachment: v1.3: incorporate slight change from bug 579779 as well

Bug 579779 also applied a slight change to this work in Firefox, this patch incorporates that and the test extension.

Comment 11

[[e-mail]]

(In reply to comment #6)
> (In reply to comment #2)
> > As you can see, AddonManager is already defined.
> Just realized we do a direct import of it. Well, if we ever start caring about
> navigator startup perf, this might be something to look into, but for now I'll
> leave it.
We can't not define it, since we need to start async watching for themes...

Comment 12

[Robert Kaiser]

(In reply to comment #11)
> We can't not define it, since we need to start async watching for themes...

Sure, I was just thinking about possibly doing it async so that it's not in the direct startup path - but then, we are not even doing delayedStartup() right now, so it probably doesn't matter right at the moment.

Comment 13

[Robert Kaiser]

Attachment: v1.4: incorporate the slight change from bug 589954 as well

Bug 589954 slightly adapted this, and the test was fixed for an ID change as well, those are incorporated in this patch.

Comment 14

[Robert Kaiser]

Comment on attachment 472194 [details] [diff] [review]
v1.4: incorporate the slight change from bug 589954 as well

Doesn't apply any more after core doorhanger work in bug 570004 has been dropped, so also dropping review request.

Feel free to pick up this bug and patch, I can't as long as the API for doorhangers in SeaMonkey isn't clear and I probably will have no time to do much work on anything before November anyhow.

Comment 15

[Philip Chee]

This is WFM Probably fixed as part of Bug 595810.