Closed
Bug 562992
Opened 15 years ago
Closed 14 years ago
The names of restartless extensions installed from the web while about:addons is open should appear after they're installed
Categories
(Toolkit :: Add-ons Manager, defect, P1)
Toolkit
Add-ons Manager
Tracking
()
VERIFIED
FIXED
mozilla2.0b2
Tracking | Status | |
---|---|---|
blocking2.0 | --- | beta2+ |
People
(Reporter: retornam, Assigned: adw)
References
Details
(Whiteboard: [AddonsRewriteTestday][rewrite])
Attachments
(2 files, 2 obsolete files)
(deleted),
image/png
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review |
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•15 years ago
|
||
Reporter | ||
Updated•15 years ago
|
Whiteboard: [AddonsRewriteTestday][rewrite]
Comment 2•15 years ago
|
||
For me the name appears after closing and re-opening the add-ons manager, is the same true for you?
Reporter | ||
Comment 3•15 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
Updated•15 years ago
|
Blocks: 550048
Summary: Display restartless add-on names → Name of restartless extensions isn't shown directly after installation
Updated•15 years ago
|
Priority: -- → P1
Updated•15 years ago
|
blocking2.0: --- → beta1+
Updated•15 years ago
|
blocking2.0: beta1+ → final+
Updated•15 years ago
|
blocking2.0: final+ → beta2+
Assignee | ||
Updated•14 years ago
|
Assignee: nobody → adw
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•14 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."
Comment 5•14 years ago
|
||
(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•14 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•14 years ago
|
||
Er, in addition to live-updating the placeholder to the real name once the download is complete, as you say.
Comment 8•14 years ago
|
||
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•14 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•14 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•14 years ago
|
||
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)
Comment 12•14 years ago
|
||
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•14 years ago
|
||
Blair, could you make sure the test looks OK? It passes.
Attachment #455210 -
Attachment is obsolete: true
Attachment #455321 -
Flags: review?(bmcbride)
Assignee | ||
Updated•14 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 14•14 years ago
|
||
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+
Comment 15•14 years ago
|
||
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•14 years ago
|
||
(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•14 years ago
|
||
(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
Comment 18•14 years ago
|
||
Is this ready to land?
Assignee | ||
Comment 19•14 years ago
|
||
Yes, I'll land it once the tree reopens.
Assignee | ||
Comment 20•14 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b2
Comment 21•14 years ago
|
||
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•14 years ago
|
||
Yes, the automated test in the patch should be sufficient.
Flags: in-testsuite? → in-testsuite+
Comment 23•14 years ago
|
||
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.
Description
•