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

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect, P1)

Description

[raymond [:retornam] (inactive)]

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

Comment 1

[raymond [:retornam] (inactive)]

Attachment: Restartless add-on

Comment 2

[Dave Townsend [:mossop]]

For me the name appears after closing and re-opening the add-ons manager, is the same true for you?

Comment 3

[raymond [:retornam] (inactive)]

(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

Comment 4

[Drew Willcoxon :adw]

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."

Comment 5

[Dave Townsend [:mossop]]

(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.

Comment 6

[Drew Willcoxon :adw]

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?

Comment 7

[Drew Willcoxon :adw]

Er, in addition to live-updating the placeholder to the real name once the download is complete, as you say.

Comment 8

[Dave Townsend [:mossop]]

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

Comment 9

[Drew Willcoxon :adw]

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?

Comment 10

[Drew Willcoxon :adw]

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.

Comment 11

[Drew Willcoxon :adw]

Attachment: 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.

Comment 12

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

Comment 13

[Drew Willcoxon :adw]

Attachment: patch with test

Blair, could you make sure the test looks OK?  It passes.

Comment 14

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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.

Comment 15

[Robert Strong (they/them - no direct email)]

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;
}

Comment 16

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #15)
> I would think you could just create a simple helper in head.js

Yea, I like that better.

Comment 17

[Drew Willcoxon :adw]

Attachment: 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.

Comment 18

[Dave Townsend [:mossop]]

Is this ready to land?

Comment 19

[Drew Willcoxon :adw]

Yes, I'll land it once the tree reopens.

Comment 20

[Drew Willcoxon :adw]

http://hg.mozilla.org/mozilla-central/rev/6b22b96b2d6f

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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?

Comment 22

[Drew Willcoxon :adw]

Yes, the automated test in the patch should be sufficient.

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Drew, as the jetpack name already shows, I can say that's wunderbar! :)