Closed
Bug 442629
Opened 16 years ago
Closed 16 years ago
Ignore mailcap entries with needsterminal
Categories
(Core Graveyard :: File Handling, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla1.9.1a1
People
(Reporter: glandium, Assigned: glandium)
References
Details
(Keywords: fixed1.9.0.2, fixed1.9.1)
Attachments
(4 files, 1 obsolete file)
988 bytes,
patch
|
bzbarsky
:
review+
samuel.sidler+old
:
approval1.9.0.2+
|
Details | Diff | Splinter Review |
1.29 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
1.65 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
3.76 KB,
patch
|
Details | Diff | Splinter Review |
Forked off bug #120380 per request. Note this patch's context conflicts with patch from bug #440840
Attachment #327365 -
Flags: review?(bzbarsky)
Comment 1•16 years ago
|
||
Thanks, Mike. Drivers, this fixes a long-running problem where the "Open With" dialog would display entries like "less" and other things that make no sense.
Assignee: nobody → mh+mozilla
Flags: wanted1.9.1?
Flags: wanted1.9.0.x?
Flags: blocking1.9.1?
![]() |
||
Comment 2•16 years ago
|
||
Comment on attachment 327365 [details] [diff] [review] patch Looks good. Sorry for the lag. I'm not sure we have a good way to test it, but if we do, please add a test? Otherwise, please set "in-testsuite?".
Attachment #327365 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 3•16 years ago
|
||
Do the testingframework have a nice way to setup environment ? (i.e. a MAILCAP environment variable could possibly do the trick)
Assignee | ||
Comment 4•16 years ago
|
||
On the other hand, adding a test for this also means that the test will fail whenever bug 120380 is actually dealt with.
Assignee | ||
Comment 5•16 years ago
|
||
How about this ? I used cat and sed as fake handlers because preferredAction is not set to useSystemDefault and defaultDescription not set if the executable is not found. I first tried with less and gview, and got an error because I didn't have gview in the build chroot. Since cat and sed are at least required for configure to work, we're pretty sure there are around...
Attachment #328832 -
Flags: review?(bzbarsky)
![]() |
||
Comment 6•16 years ago
|
||
Comment on attachment 328832 [details] [diff] [review] unit test Nice! If/when the other bug is fixed, we'll handle changing this test, but for now it's great.
Attachment #328832 -
Flags: review?(bzbarsky) → review+
Updated•16 years ago
|
Keywords: checkin-needed
Comment 7•16 years ago
|
||
Pushed as 15838:7f8d627ac32e.
Status: NEW → RESOLVED
Closed: 16 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1a1
Updated•16 years ago
|
Attachment #327365 -
Flags: approval1.9.0.2?
Comment 8•16 years ago
|
||
FYI, this unit test is failing (http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1215860762.1215863519.3613.gz&fulltext=1#err0). Trying to debug it, but could use some help.
Assignee | ||
Comment 9•16 years ago
|
||
Is it possible to run the test with NSPR_LOG_MODULES=HelperAppService:5 on the tinderbox ?
Comment 10•16 years ago
|
||
(In reply to comment #9) > Is it possible to run the test with NSPR_LOG_MODULES=HelperAppService:5 on the > tinderbox ? Not easily, no, but with debugging, found that $(CURDIR) wasn't equating to what was needed. After some back and forth iteration with Mike, came up with $(shell cd $(srcdir) && pwd) that should do what is wanted.
Comment 11•16 years ago
|
||
I had to comment out the test, as it's unix-only, and mac and win32 were failing. Need to make it unix only.
Flags: in-testsuite+ → in-testsuite?
Assignee | ||
Comment 12•16 years ago
|
||
This should be okay, provided there is no PERSONAL_MAILCAP environment variable defined on the not linux tinderboxes ;)
Comment 13•16 years ago
|
||
Comment on attachment 329220 [details] [diff] [review] unit test v2 > // that exists on all platforms (except possibly the application being > // tested, but there doesn't seem to be a way to get a reference to that > // from the directory service), we use the temporary directory itself. >- var executable = HandlerServiceTest._dirSvc.get("TmpD", Ci.nsIFile); >+ var executable = HandlerServiceTest._dirSvc.get("XCurProcD", Ci.nsIFile); >+ executable.append("run-mozilla.sh"); What are these other changes?
Attachment #329220 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 14•16 years ago
|
||
Damn, I thought I filtered these. Drop this hunk.
Assignee | ||
Comment 15•16 years ago
|
||
With the bad hunk filtered out
Attachment #329220 -
Attachment is obsolete: true
Attachment #329269 -
Flags: review?(bzbarsky)
Attachment #329220 -
Flags: review?(bzbarsky)
![]() |
||
Comment 16•16 years ago
|
||
Comment on attachment 329269 [details] [diff] [review] unit test v2.1 r=bzbarsky
Attachment #329269 -
Flags: review?(bzbarsky) → review+
Updated•16 years ago
|
Keywords: checkin-needed
Comment 17•16 years ago
|
||
Looking over this bug, I still see that the in-testsuite flag is '?'. The unit tests here are pretty minimal, but I must admit I'm not an expert on how this setting works. Do we need more tests in addition to the unit tests here?
Comment 18•16 years ago
|
||
(In reply to comment #17) > Looking over this bug, I still see that the in-testsuite flag is '?'. The unit > tests here are pretty minimal, but I must admit I'm not an expert on how this > setting works. Do we need more tests in addition to the unit tests here? The unit test just needs to be landed. Once that's done, in-testsuite+ should be set.
Comment 19•16 years ago
|
||
Pushed to mozilla-central: http://hg.mozilla.org/mozilla-central/index.cgi/rev/8c55641fe8ff
Updated•16 years ago
|
Flags: in-testsuite? → in-testsuite+
Updated•16 years ago
|
Keywords: checkin-needed
Comment 20•16 years ago
|
||
Comment on attachment 327365 [details] [diff] [review] patch Approved for 1.9.0.2. Please land in CVS. a=ss Also, be sure to land the test as well.
Attachment #327365 -
Flags: approval1.9.0.2? → approval1.9.0.2+
Updated•16 years ago
|
Keywords: checkin-needed
Comment 21•16 years ago
|
||
Shawn do you have time to land this patch on 1.9 as well?
Updated•16 years ago
|
Flags: wanted1.9.0.x? → wanted1.9.0.x+
Whiteboard: [needs checkin on 1.9]
Comment 22•16 years ago
|
||
Comment 23•16 years ago
|
||
CVS HEAD: Checking in uriloader/exthandler/tests/Makefile.in; /cvsroot/mozilla/uriloader/exthandler/tests/Makefile.in,v <-- Makefile.in new revision: 1.7; previous revision: 1.6 done Checking in uriloader/exthandler/tests/unit/test_handlerService.js; /cvsroot/mozilla/uriloader/exthandler/tests/unit/test_handlerService.js,v <-- test_handlerService.js new revision: 1.20; previous revision: 1.19 done Checking in uriloader/exthandler/unix/nsOSHelperAppService.cpp; /cvsroot/mozilla/uriloader/exthandler/unix/nsOSHelperAppService.cpp,v <-- nsOSHelperAppService.cpp new revision: 1.75; previous revision: 1.74 done
Keywords: checkin-needed → fixed1.9.0.2
Whiteboard: [needs checkin on 1.9]
Comment 24•16 years ago
|
||
RCS file: /cvsroot/mozilla/uriloader/exthandler/tests/mailcap,v done Checking in uriloader/exthandler/tests/mailcap; /cvsroot/mozilla/uriloader/exthandler/tests/mailcap,v <-- mailcap initial revision: 1.1 done
Comment 25•16 years ago
|
||
Fix landed before branching.
Updated•8 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•