442629 - Ignore mailcap entries with needsterminal

Status: RESOLVED FIXED

Description

[Mike Hommey [:glandium]]

Created attachment 327365 [details] [diff] [review]
patch

Forked off bug #120380 per request.

Note this patch's context conflicts with patch from bug #440840

Comment 1

[Reed Loden [:reed] (use needinfo?)]

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.

Comment 2

[Boris Zbarsky [:bz] (no decent commit message means r-)]

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

Comment 3

[Mike Hommey [:glandium]]

Do the testingframework have a nice way to setup environment ? (i.e. a MAILCAP environment variable could possibly do the trick)

Comment 4

[Mike Hommey [:glandium]]

On the other hand, adding a test for this also means that the test will fail whenever bug 120380 is actually dealt with.

Comment 5

[Mike Hommey [:glandium]]

Created attachment 328832 [details] [diff] [review]
unit test

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

Comment 6

[Boris Zbarsky [:bz] (no decent commit message means r-)]

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.

Comment 7

[Reed Loden [:reed] (use needinfo?)]

Pushed as 15838:7f8d627ac32e.

Comment 8

[Reed Loden [:reed] (use needinfo?)]

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.

Comment 9

[Mike Hommey [:glandium]]

Is it possible to run the test with NSPR_LOG_MODULES=HelperAppService:5 on the tinderbox ?

Comment 10

[Reed Loden [:reed] (use needinfo?)]

(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

[Reed Loden [:reed] (use needinfo?)]

I had to comment out the test, as it's unix-only, and mac and win32 were failing. Need to make it unix only.

Comment 12

[Mike Hommey [:glandium]]

Created attachment 329220 [details] [diff] [review]
unit test v2

This should be okay, provided there is no PERSONAL_MAILCAP environment variable defined on the not linux tinderboxes ;)

Comment 13

[Reed Loden [:reed] (use needinfo?)]

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?

Comment 14

[Mike Hommey [:glandium]]

Damn, I thought I filtered these. Drop this hunk.

Comment 15

[Mike Hommey [:glandium]]

Created attachment 329269 [details] [diff] [review]
unit test v2.1

With the bad hunk filtered out

Comment 16

[Boris Zbarsky [:bz] (no decent commit message means r-)]

Comment on attachment 329269 [details] [diff] [review]
unit test v2.1

r=bzbarsky

Comment 17

[cmtalbert]

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

[Reed Loden [:reed] (use needinfo?)]

(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

[Shawn Wilsher :sdwilsh]

Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/8c55641fe8ff

Comment 20

[Samuel Sidler (old account; do not CC)]

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.

Comment 21

[Samuel Sidler (old account; do not CC)]

Shawn do you have time to land this patch on 1.9 as well?

Comment 22

[:Gavin Sharp [email: [e-mail]]]

Created attachment 334735 [details] [diff] [review]
rollup CVS patch

Comment 23

[Reed Loden [:reed] (use needinfo?)]

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

Comment 24

[Reed Loden [:reed] (use needinfo?)]

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

[Benjamin Smedberg]

Fix landed before branching.