Closed Bug 581974 Opened 10 years ago Closed 8 years ago

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

Categories

(SeaMonkey :: UI Design, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED DUPLICATE of bug 595810

People

(Reporter: kairo, Unassigned)

References

Details

(Whiteboard: [doorhanger])

Attachments

(1 file, 4 obsolete files)

Bug 55296 improved web installation failure messages in Firefox and converted them to use doorhangers, we should port that to SeaMonkey.
Attached patch port the Firefox work (obsolete) — Splinter Review
This ports the Firefox frontend work to SeaMonkey. The test passes, but I haven't done manual testing of this yet myself.
Attachment #460274 - Flags: review?(neil)
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?
Attachment #460274 - Flags: review?(neil)
(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 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.)
(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.
Depends on: 581229, 577048
(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!
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.
Attachment #460274 - Attachment is obsolete: true
Attachment #461560 - Flags: review?(neil)
(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 :-(
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. ;-)
Attachment #461560 - Attachment is obsolete: true
Attachment #462474 - Flags: review?(neil)
Attachment #461560 - Flags: review?(neil)
Bug 579779 also applied a slight change to this work in Firefox, this patch incorporates that and the test extension.
Attachment #462474 - Attachment is obsolete: true
Attachment #466292 - Flags: review?(neil)
Attachment #462474 - Flags: review?(neil)
(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...
(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.
Bug 589954 slightly adapted this, and the test was fixed for an ID change as well, those are incorporated in this patch.
Attachment #466292 - Attachment is obsolete: true
Attachment #472194 - Flags: review?(neil)
Attachment #466292 - Flags: review?(neil)
Blocks: 594776
Assignee: kairo → nobody
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.
Attachment #472194 - Flags: review?(neil)
Whiteboard: [doorhanger]
This is WFM Probably fixed as part of Bug 595810.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 595810
You need to log in before you can comment on or make changes to this bug.