Closed
Bug 526733
Opened 15 years ago
Closed 15 years ago
Can't install application/x-xpinstall types by clicking on links in content tabs
Categories
(Thunderbird :: Toolbars and Tabs, defect)
Thunderbird
Toolbars and Tabs
Tracking
(blocking-thunderbird3.1 beta1+, thunderbird3.1 beta1-fixed)
RESOLVED
FIXED
Thunderbird 3.1b1
Tracking | Status | |
---|---|---|
blocking-thunderbird3.1 | --- | beta1+ |
thunderbird3.1 | --- | beta1-fixed |
People
(Reporter: standard8, Assigned: standard8)
References
()
Details
(Whiteboard: [UXprio])
Attachments
(1 file, 1 obsolete file)
14.81 KB,
patch
|
asuth
:
review+
clarkbw
:
ui-review+
|
Details | Diff | Splinter Review |
If you bring up AMO in a content tab and click on a install button, then you'll get prompted with a dialog to save the xpi - that's because amo is forcing the content disposition to be an attachment which causes us to save it to disk.
Even if we got amo to do user-agent sniffing, it still wouldn't quite work - clicking on a link with content-type application/x-xpinstall actually ends up going no-where.
It turns out we're not handling the xpinstall-install-blocked notification. There's already some code we can copy to do that (see link above), but just before TB 3.0 is not the time to squeeze that in and think about security considerations.
Updated•15 years ago
|
blocking-thunderbird3.1: --- → beta1+
Target Milestone: Thunderbird 3.1a1 → Thunderbird 3.1b1
Assignee | ||
Comment 1•15 years ago
|
||
This patch implements the general code and strings for doing this and an automated test. There's still a few loose ends to tie up, but storing for archival purposes anyway.
Updated•15 years ago
|
Whiteboard: [UXprio]
Assignee | ||
Comment 2•15 years ago
|
||
This is the proposed fix. This basically adapts some of the Firefox code to Thunderbird and hooks up the necessarily listener.
Strings are copied from Firefox as well, although they are basically the same as Firefox requesting ui-review from Bryan to make sure he's happy.
MozMill test also included, though there's a few XXX parts. One was that I couldn't get the keypress firing reliably, and the second is that our test infrastructure doesn't seem to like two modal dialogs running in one test. As we've got beta freeze on Tuesday, I thought it best to push forward to review as the code is working and has some basic tests, and follow up on test fixes later.
Attachment #427116 -
Attachment is obsolete: true
Attachment #427751 -
Flags: ui-review?(clarkbw)
Attachment #427751 -
Flags: review?(bugmail)
Assignee | ||
Updated•15 years ago
|
Whiteboard: [UXprio] → [UXprio][has patch][needs review asuth,clarkbw]
Assignee | ||
Updated•15 years ago
|
Whiteboard: [UXprio][has patch][needs review asuth,clarkbw] → [UXprio][has patch][needs review asuth,clarkbw][affects l10n]
Updated•15 years ago
|
Whiteboard: [UXprio][has patch][needs review asuth,clarkbw][affects l10n] → [UXprio][has patch][needs ui-review clarkbw, review asuth][affects l10n]
Comment 3•15 years ago
|
||
Comment on attachment 427751 [details] [diff] [review]
The fix
Strings look fine, though I feel like the word "this" is awkward and I would recommend replacing with "the".
i.e.
%1$S prevented this site (%2$S) from asking you to install software on your computer.
Could be:
%1$S prevented the site (%2$S) from asking you to install software on your computer.
Does that seem better?
Attachment #427751 -
Flags: ui-review?(clarkbw) → ui-review+
Comment 4•15 years ago
|
||
If you use "the", I think you could remove the parentheses, to get:
%1$S prevented the site %2$S from asking you to install software on your
computer.
Later,
Blake.
Updated•15 years ago
|
Whiteboard: [UXprio][has patch][needs ui-review clarkbw, review asuth][affects l10n] → [UXprio][has patch][review asuth][affects l10n]
Comment 5•15 years ago
|
||
Removing the parens as suggested in comment 4 makes it read awkwardly to me.. The string change proposed in comment 3 seems reasonable to me. For some future version, my personal inclination would be to remove the words "the site" _and_ the parens.
Whiteboard: [UXprio][has patch][review asuth][affects l10n] → [UXprio][has patch, ui-review][needs review asuth][needs updated patch][affects l10n]
Comment 6•15 years ago
|
||
Comment on attachment 427751 [details] [diff] [review]
The fix
http://reviews.visophyte.org/r/427751/
on file: mail/base/content/specialTabs.js line 823
> observe: function (aSubject, aTopic, aData) {
> if (aTopic != "mail-startup-done")
> return;
This probably merits a comment since this looks to the untrained eye like an
observer handler used for many events, but it's coded assuming it will only
ever see one.
on file: mail/base/content/specialTabs.js line None
XPCOMUtils.jsm provides a magic defineLazyServiceGetter function. That's
arguably more self-documenting than the idiom.
Those are nits. r=asuth conditional on taking my changes to the mozmill test. Since I'm going to push this for you, that's a given. Feel free to go back and fix up the nits if you want and push. I will not lose sleep either way.
Attachment #427751 -
Flags: review?(bugmail) → review+
Comment 7•15 years ago
|
||
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 8•15 years ago
|
||
The modified unit tests had very-frequent failures across all platforms and both 1.9.2 and trunk:
TEST-UNEXPECTED-FAIL | test_xpinstall_disabled
EXCEPTION: timeout exceeded for waitForEval('subject.alerted')
at: controller.js line 498
Error("timeout exceeded for waitForEval('subject.alerted')") 0
controller.js 498
test-install-xpi.js 71
install_xpi() test-install-xpi.js 115
test_xpinstall_disabled() test-install-xpi.js 171
frame.js 468
frame.js 520
frame.js 562
frame.js 411
frame.js 568
server.js 164
server.js 168
status-thunderbird3.1:
--- → beta1-fixed
Flags: in-testsuite?
Whiteboard: [UXprio][has patch, ui-review][needs review asuth][needs updated patch][affects l10n] → [UXprio]
Assignee | ||
Comment 9•15 years ago
|
||
(hence I backed the test out, but kept the code parts in):
http://hg.mozilla.org/comm-central/rev/a372aa4bea07
Assignee | ||
Updated•9 years ago
|
Flags: in-testsuite?
You need to log in
before you can comment on or make changes to this bug.
Description
•