"Open with 'TextEditor.app'" Should be "Open with 'TextEditor'"

VERIFIED FIXED in mozilla1.9.1b1

Status

Core Graveyard
File Handling
VERIFIED FIXED
10 years ago
2 years ago

People

(Reporter: Håkan Waara, Assigned: sdwilsh)

Tracking

({polish})

Trunk
mozilla1.9.1b1
x86
Mac OS X
polish
Bug Flags:
wanted1.9.1 ?
wanted1.9.0.x -
in-testsuite ?
in-litmus +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

10 years ago
In the download manager's "Open / Save file" dialog, we should not use the application's name with ".app". It looks really unprofessional.

It's trivial, but needed polish, IMHO.
Flags: blocking-firefox3?
(Assignee)

Comment 1

10 years ago
This is actually what exthandler gives us.  I think this was a recent change to use LaunchServices or something too.
Component: Download Manager → File Handling
Flags: blocking-firefox3?
Product: Firefox → Core
QA Contact: download.manager → file-handling
(Reporter)

Comment 2

10 years ago
I think you need to make nsExternalHelperAppService returns the right human readable name.  See nsILocalFileMac::bundleDisplayName which should do the work for us.
(Reporter)

Comment 3

10 years ago
Created attachment 312549 [details] [diff] [review]
Patch

This patch makes us use the correct way to get at an executable's name on mac; using the bundleDisplayName (which excludes the .app suffix)

Shawn, do you know anything about why nsMIMEInfo and nsLocalHandlerApp are basically two class hierarchies that are duplicated? Who uses which? This code is a real mess. :-(
Assignee: nobody → hwaara
Status: NEW → ASSIGNED
Attachment #312549 - Flags: review?(sdwilsh)
(Reporter)

Updated

10 years ago
Attachment #312549 - Flags: superreview?(cbiesinger)
Comment on attachment 312549 [details] [diff] [review]
Patch

@@ -1049,7 +1055,12 @@ nsUnknownContentTypeDialog.prototype = {
+          this.chosenApp.executable.QueryInterface(Components.interfaces.nsILocalFileMac);
+          otherHandler.label = this.chosenApp.executable.bundleDisplayName;

incorrect indentation

+++ b/uriloader/exthandler/mac/nsLocalHandlerAppMac.h	Sun Mar 30 01:51:49 2008 +0100
+    NS_IMETHOD LaunchWithURI(nsIURI* aURI, nsIInterfaceRequestor* aWindowContext);

is that still <= 80 chars?
Attachment #312549 - Flags: superreview?(cbiesinger) → superreview+
(Reporter)

Comment 5

10 years ago
Thanks Christian, I've fixed those.

There's a unit test failing that I'll have to address. I'll come up with a new patch soon.
(Assignee)

Comment 6

10 years ago
Comment on attachment 312549 [details] [diff] [review]
Patch

r=sdwilsh, with biesi's comments addressed.  Note, I'm not a peer or uriloader (but biesi is, so his sr is probably sufficient).
Attachment #312549 - Flags: review?(sdwilsh) → review+
(Assignee)

Comment 7

10 years ago
(In reply to comment #3)
> Shawn, do you know anything about why nsMIMEInfo and nsLocalHandlerApp are
> basically two class hierarchies that are duplicated? Who uses which? This code
> is a real mess. :-(
It's a mess because we've been incrementally rewriting how this works.  nsLocalHandlerApp is new, and I believe the goal is to eventually have nsMIMEInfo return an nsILocalHandlerApp instead of an nsIFile.

Comment 8

10 years ago
Bug 393122 talks about the various cleanups in progress.
(Assignee)

Updated

10 years ago
Attachment #312549 - Flags: approval1.9?
Can we test this?  Need to have at least addressed why we don't have a test for this before approving.
Comment on attachment 312549 [details] [diff] [review]
Patch

re-request approval once tests are addressed.
Attachment #312549 - Flags: approval1.9?
(Assignee)

Comment 11

10 years ago
Might be able to test it, but I can't see it being easy to test.  Litmus test for sure though.
Flags: in-litmus?
(Reporter)

Comment 12

10 years ago
There's a unit test in uriloader/ failing. It's kind of weird, some RDF GetLiteral of some property. I'm not sure if it's the patch or my tree, actually. Anyone care to test the patch and make check?

I'll try to post to this bug with more detail soon otherwise.
in-litmus+:

https://litmus.mozilla.org/show_test.cgi?id=5311
Flags: in-litmus? → in-litmus+
(Reporter)

Comment 14

10 years ago
(In reply to comment #13)
> in-litmus+:
> 
> https://litmus.mozilla.org/show_test.cgi?id=5311
> 

Stephen, note that this fix hasn't landed (pending an issue with a failing test).
(Assignee)

Comment 15

10 years ago
Any luck with that test (which test?)?
(In reply to comment #14)
> (In reply to comment #13)
> > in-litmus+:
> > 
> > https://litmus.mozilla.org/show_test.cgi?id=5311
> > 
> 
> Stephen, note that this fix hasn't landed (pending an issue with a failing
> test).

Sure; the test has this regression bug noted.

(Reporter)

Comment 17

10 years ago
sdwilsh volunteered to look into the failing unit test.
Assignee: hwaara → sdwilsh
Status: ASSIGNED → NEW
(Assignee)

Comment 18

10 years ago
I'll hopefully look at this week so it makes it for 3.1
Whiteboard: [needs patch]
(Assignee)

Comment 19

10 years ago
Created attachment 336144 [details] [diff] [review]
v1.1

Not much change, but we should only be getting the application bundle name if it's actually a package.

This passes make check, and does the right thing when using the browser (yay!)
Attachment #312549 - Attachment is obsolete: true
Attachment #336144 - Flags: superreview?(cbiesinger)
Attachment #336144 - Flags: review?(hwaara)
(Assignee)

Updated

10 years ago
Status: NEW → ASSIGNED
Whiteboard: [needs patch] → [has patch][needs review hwaara][needs sr biesi]
Target Milestone: --- → mozilla1.9.1b1
(Assignee)

Comment 20

10 years ago
I've seen people complain about this all over the internet, so nominating for wanted on 1.9.0.x, and we should really get this for 1.9.1 too.
Flags: wanted1.9.1?
Flags: wanted1.9.0.x?
(Assignee)

Comment 21

10 years ago
Comment on attachment 336144 [details] [diff] [review]
v1.1

>+++ b/uriloader/exthandler/tests/unit/test_handlerService.js
>+  dump(preferredHandler+"\n");
Ignore this - was just there for debugging.
(Reporter)

Updated

10 years ago
Attachment #336144 - Flags: review?(hwaara) → review+
(Reporter)

Comment 22

10 years ago
Comment on attachment 336144 [details] [diff] [review]
v1.1

Looks good to me!

When I wrote the last patch, I thought it was difficult to test it from all "directions" and all places this code is used, but let's be on the lookout for followup bugs. :-)

So the missing package check was the problem in the previous patch?
(Reporter)

Comment 23

10 years ago
Just FYI, in case it would help for a unit test: there's now an app bundle in the tree - SmallApp.app or something like that - in xpcom/tests/unit/resources
(Assignee)

Comment 24

10 years ago
So now that I've had some time to sleep on this, I figured I could give a better description of what was wrong and what we do now.

The underlying issue here is that we were trying to get a bundle name on something that was not a bundle.  This happened to throw, which was the result of the test failure.

This new code has one implication though that isn't true for the other code paths - if you add the handler with some special name that isn't its actual name, and it happens to be a bundle, we will use the bundle name and not the one you supplied.  Myk would probably be the best person to know if this is OK or not.
Comment on attachment 336144 [details] [diff] [review]
v1.1

@@ -1059,17 +1065,22 @@ nsUnknownContentTypeDialog.prototype = {
+          this.chosenApp.executable.QueryInterface(Components.interfaces.nsILocalFileMac);

incorrect indentation

+++ b/uriloader/exthandler/mac/nsLocalHandlerAppMac.mm
+  if (mExecutable) {

shouldn't you have an additional check && mName.IsEmpty(), like in nsLocalHandlerApp?

+  if (mDefaultApplication) {

similar here, for mDefaultAppDescription
Attachment #336144 - Flags: superreview?(cbiesinger) → superreview+
(Assignee)

Comment 26

10 years ago
(In reply to comment #25)
> +++ b/uriloader/exthandler/mac/nsLocalHandlerAppMac.mm
> +  if (mExecutable) {
> 
> shouldn't you have an additional check && mName.IsEmpty(), like in
> nsLocalHandlerApp?
> 
> +  if (mDefaultApplication) {
> 
> similar here, for mDefaultAppDescription
No to both of these - as far as I can tell, we'll always get a name back here.
(Assignee)

Comment 27

10 years ago
Created attachment 336608 [details] [diff] [review]
v1.2

Addresses review comments
Attachment #336144 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Whiteboard: [has patch][needs review hwaara][needs sr biesi] → [has patch][has review][has sr][can land]
(Assignee)

Comment 28

10 years ago
Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/f96f0b755e14
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Whiteboard: [has patch][has review][has sr][can land]
(Assignee)

Comment 29

10 years ago
stephend - make sure to enable that litmus test now ;)
Verified FIXED using  Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b1pre) Gecko/20080904020533 Minefield/3.1b1pre -and-

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.1b1pre) Gecko/20080904020533 Minefield/3.1b1pre

Testcase is enabled, and I marked it was passing with the 10.5 build.
Status: RESOLVED → VERIFIED
Doesn't really meet the "wanted" criteria (security, stability, regression from maintenance release) for 1.9.0.x.
Flags: wanted1.9.0.x? → wanted1.9.0.x-
(Assignee)

Updated

10 years ago
Attachment #336608 - Flags: approval1.9.0.4?
Comment on attachment 336608 [details] [diff] [review]
v1.2

Not taking polish bugs on the branch.
Attachment #336608 - Flags: approval1.9.0.4? → approval1.9.0.4-
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.