Closed Bug 442629 Opened 11 years ago Closed 11 years ago

Ignore mailcap entries with needsterminal

Categories

(Core Graveyard :: File Handling, defect)

All
Linux
defect
Not set

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)

Attached patch patchSplinter Review
Forked off bug #120380 per request.

Note this patch's context conflicts with patch from bug #440840
Attachment #327365 - Flags: review?(bzbarsky)
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 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+
Do the testingframework have a nice way to setup environment ? (i.e. a MAILCAP environment variable could possibly do the trick)
On the other hand, adding a test for this also means that the test will fail whenever bug 120380 is actually dealt with.
Attached patch unit testSplinter Review
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 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+
Pushed as 15838:7f8d627ac32e.
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1a1
Attachment #327365 - Flags: approval1.9.0.2?
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.
Is it possible to run the test with NSPR_LOG_MODULES=HelperAppService:5 on the tinderbox ?
(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.
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?
Attached patch unit test v2 (obsolete) — Splinter Review
This should be okay, provided there is no PERSONAL_MAILCAP environment variable defined on the not linux tinderboxes ;)
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)
Damn, I thought I filtered these. Drop this hunk.
Attached patch unit test v2.1Splinter Review
With the bad hunk filtered out
Attachment #329220 - Attachment is obsolete: true
Attachment #329269 - Flags: review?(bzbarsky)
Attachment #329220 - Flags: review?(bzbarsky)
Comment on attachment 329269 [details] [diff] [review]
unit test v2.1

r=bzbarsky
Attachment #329269 - Flags: review?(bzbarsky) → review+
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?
(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.
Flags: in-testsuite? → in-testsuite+
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+
Shawn do you have time to land this patch on 1.9 as well?
Flags: wanted1.9.0.x? → wanted1.9.0.x+
Whiteboard: [needs checkin on 1.9]
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
Whiteboard: [needs checkin on 1.9]
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
Fix landed before branching.
Flags: wanted1.9.1?
Flags: blocking1.9.1?
Flags: blocking1.9.1+
Keywords: fixed1.9.1
Blocks: 1047225
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.