The names of restartless extensions installed from the web while about:addons is open should appear after they're installed

VERIFIED FIXED in mozilla2.0b2

Status

()

Toolkit
Add-ons Manager
P1
normal
VERIFIED FIXED
8 years ago
8 years ago

People

(Reporter: retornam, Assigned: adw)

Tracking

Trunk
mozilla2.0b2
Points:
---
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(blocking2.0 beta2+)

Details

(Whiteboard: [AddonsRewriteTestday][rewrite])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

8 years ago
STR:
visit https://secure.toolness.com/xpi/restartless/
Install any of the add-ons found there
Open Add-ons manager 

Expected Result:
Name and Description of the addon   show up

Actual Results:
Only description of the addon is shown
(Reporter)

Comment 1

8 years ago
Created attachment 442760 [details]
Restartless add-on
(Reporter)

Updated

8 years ago
Whiteboard: [AddonsRewriteTestday][rewrite]
For me the name appears after closing and re-opening the add-ons manager, is the same true for you?
(Reporter)

Comment 3

8 years ago
(In reply to comment #2)
> For me the name appears after closing and re-opening the add-ons manager, is
> the same true for you?

Yes. I still think  the name should appear right after installing the addon
Blocks: 550048
Summary: Display restartless add-on names → Name of restartless extensions isn't shown directly after installation
Priority: -- → P1
blocking2.0: --- → beta1+
blocking2.0: beta1+ → final+
blocking2.0: final+ → beta2+
(Assignee)

Updated

8 years ago
Assignee: nobody → adw
Status: NEW → ASSIGNED
(Assignee)

Comment 4

8 years ago
The STR aren't exactly right.  The problem only happens after installing an add-on while the manager is already open.  If you install an add-on and then later open the manager, the title is OK.

I think this is because the add-on name just isn't available at the time that the UI is updated.  The UI is updated immediately after clicking an XPI on the web, but surely the backend needs to download the XPI to actually find out its name.  Need to investigate more, but just grepping for AddonInstall I see that AddonManagerInternal.getInstallForURL() takes a "placeholder name while the add-on is being downloaded."
(In reply to comment #4)
> The STR aren't exactly right.  The problem only happens after installing an
> add-on while the manager is already open.  If you install an add-on and then
> later open the manager, the title is OK.
> 
> I think this is because the add-on name just isn't available at the time that
> the UI is updated.  The UI is updated immediately after clicking an XPI on the
> web, but surely the backend needs to download the XPI to actually find out its
> name.  Need to investigate more, but just grepping for AddonInstall I see that
> AddonManagerInternal.getInstallForURL() takes a "placeholder name while the
> add-on is being downloaded."

Yeah we get a placeholder name during download. Once downloaded (The onDownloadEnded event) the proper name should be available so the UI should be updated at that point.
(Assignee)

Comment 6

8 years ago
Well, we either don't get a placeholder name during download, or we're not displaying it.  The desired behavior here is to display a placeholder name, correct?
(Assignee)

Comment 7

8 years ago
Er, in addition to live-updating the placeholder to the real name once the download is complete, as you say.
Yep, placeholder while downloading and real name once done, though that might be two separate issues. The empty name here might be because the website isn't providing a placeholder and the download code isn't making up a good one
(Assignee)

Comment 9

8 years ago
Boriss, while a remote XPI is downloading but displayed in the UI, what should we show as its name?  Some form of its URL, a string like "Add-on Downloading...", or something else?
Keywords: uiwanted
(Assignee)

Comment 10

8 years ago
Oh, one of the paths in the addon-installing binding is already using the basename of the URL as a placeholder for the name, but it's not the path that's relevant here:

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/content/extensions.xml#1198

I'll do the same unless I hear otherwise.
(Assignee)

Comment 11

8 years ago
Created attachment 455210 [details] [diff] [review]
patch

I was making this more complicated than it is.  Just need a one-line change to update the "name" attribute when switching bindings from addon-installing to addon-generic.  The placeholder while downloading is already correctly handled by the path I mentioned in comment 10.
Attachment #455210 - Flags: review?(dtownsend)
(Assignee)

Updated

8 years ago
Keywords: uiwanted
Comment on attachment 455210 [details] [diff] [review]
patch

Well that was a nice small fix. Should be possible to have a browser-chrome test for this using the mocking provider in toolkit/mozapps/extensions/test/browser/head.js
Attachment #455210 - Flags: review?(dtownsend) → review+
(Assignee)

Comment 13

8 years ago
Created attachment 455321 [details] [diff] [review]
patch with test

Blair, could you make sure the test looks OK?  It passes.
Attachment #455210 - Attachment is obsolete: true
Attachment #455321 - Flags: review?(bmcbride)
(Assignee)

Updated

8 years ago
Summary: Name of restartless extensions isn't shown directly after installation → The names of restartless extensions installed from the web while about:addons is open should appear after they're installed
Comment on attachment 455321 [details] [diff] [review]
patch with test

>+      <field name="_name">
>+        document.getAnonymousElementByAttribute(this, "anonid",
>+                                                "name");
>+      </field>

I usually don't like adding properties just for tests, but I guess this will end up being used a lot. There's a couple of other tests that are using getAnonymousElementByAttribute() directly, and would benefit from this. Bonus points if you update those to use this new field :)


>+++ b/toolkit/mozapps/extensions/test/browser/browser_bug562992.js
>@@ -0,0 +1,105 @@
>+/* -*- Mode: Java; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* vim:set ts=2 sw=2 sts=2 et: */
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1

AFAIK, tests are meant to have the public-domain license these days (http://www.mozilla.org/MPL/license-policy.html).


>-function MockInstall(aName, aType) {
>+function MockInstall(aName, aType, aRemoteAddon) {

Its a pity this doesn't support upgrade installs yet, as I'd expect the 3rd argument to be for the existingAddon property of the install (because that's how the API does it). Anyway, not sure it being named aRemoteAddon makes sense - maybe aAddonToInstall?


Side note: I just noticed that the aRestartless argument for MockAddon is never actually used anywhere yet.
Attachment #455321 - Flags: review?(bmcbride) → review+
I would think you could just create a simple helper in head.js for getting the name attribute from of the richlistitem itself instead of adding a _name field to the client xbl that gets the value from the anonymous element in the richlistitem. Something like
function getAddonNameFromListItem(aListItem) {
  if (aListItem.hasAttribute("name"))
    return aListItem.getAttribute("name");
  return null;
}
(In reply to comment #15)
> I would think you could just create a simple helper in head.js

Yea, I like that better.
(Assignee)

Comment 17

8 years ago
Created attachment 456940 [details] [diff] [review]
patch with test v2

(In reply to comment #14)
> AFAIK, tests are meant to have the public-domain license these days
> (http://www.mozilla.org/MPL/license-policy.html).

Point 1 on that page says that the tri-license is acceptable in all circumstances except possibly for third-party code.  Point 2 says CC Public Domain is acceptable for testcases.  So I've left the tri-license, especially since that license calls out contributors, which will be useful and much more handy than hg blame if the test regresses and fingers need to be pointed.

> >-function MockInstall(aName, aType) {
> >+function MockInstall(aName, aType, aRemoteAddon) {
> 
> Its a pity this doesn't support upgrade installs yet, as I'd expect the 3rd
> argument to be for the existingAddon property of the install (because that's
> how the API does it). Anyway, not sure it being named aRemoteAddon makes sense
> - maybe aAddonToInstall?

Changed to aAddonToInstall.  ("aRemoteAddon" was supposed to imply that we're simulating the case where a remote URL is provided and the add-on must be downloaded.)

> Side note: I just noticed that the aRestartless argument for MockAddon is never
> actually used anywhere yet.

Yes, that doesn't affect this patch, right?

(In reply to comment #15)
> I would think you could just create a simple helper in head.js for getting the
> name attribute from of the richlistitem itself

Oh duh, since the label gets its value from the item's name attr.  Thanks Robert.  I didn't make a helper function though, since a simple call to getAttribute() is enough for this particular patch.

I'll land this patch with one or two others when I get a chance.
Attachment #455321 - Attachment is obsolete: true
Is this ready to land?
(Assignee)

Comment 19

8 years ago
Yes, I'll land it once the tree reopens.
(Assignee)

Comment 20

8 years ago
http://hg.mozilla.org/mozilla-central/rev/6b22b96b2d6f
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b2
Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; en-US; rv:2.0b2pre) Gecko/20100718 Minefield/4.0b2pre

Can we completely test this automatically?
Status: RESOLVED → VERIFIED
Flags: in-testsuite?
Flags: in-litmus?
(Assignee)

Comment 22

8 years ago
Yes, the automated test in the patch should be sufficient.
Flags: in-testsuite? → in-testsuite+
Drew, as the jetpack name already shows, I can say that's wunderbar! :)
Flags: in-litmus? → in-litmus-
You need to log in before you can comment on or make changes to this bug.