Closed Bug 389766 Opened 16 years ago Closed 15 years ago

Hard-to-understand presentation of protocol-handlers on Linux

Categories

(Firefox :: File Handling, defect, P2)

x86
Linux
defect

Tracking

()

VERIFIED FIXED
Firefox 3 beta2

People

(Reporter: cmtalbert, Assigned: ventnor.bugzilla)

References

(Blocks 1 open bug, )

Details

(Whiteboard: [proto])

Attachments

(1 file, 1 obsolete file)

1.19 KB, patch
Biesinger
: review+
Biesinger
: superreview+
Details | Diff | Splinter Review
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a7pre) Gecko/2007072604 Minefield/3.0a7pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a7pre) Gecko/2007072604 Minefield/3.0a7pre

On Ubuntu 7.04, the webcal links no longer load into evolution by default.  Previously, they would launch the "unknown protocol" window, but would offer to launch Evolution for you to handle the link.  With this build, a "Choose Application" window pops up, and nothing you tell it will cause Evolution to work.

Reproducible: Always

Steps to Reproduce:
1. Go to the webcal link in the above URL on FFx on Ubuntu 7.04
2. In the choose application window that pops up, choose any version of Evolution you can find. On this system, I tried it with evolution, evolution-2.2, and evolution-2.10, all from /usr/bin

Actual Results:  
The webcal link is not imported to Evolution.

Expected Results:  
The webcal link should be handled by evolution. This means that Evolution will open a window asking you which color to assign to the calendar and how often to update it.

This is the regression range for the break:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=mozilla%2F&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-07-25+05%3A00%3A00&maxdate=2007-07-26+05%3A00%3A00&cvsroot=%2Fcvsroot
Keywords: regression
Blocks: 385065
Flags: blocking-firefox3?
Target Milestone: --- → Firefox 3 M7
Ok, thanks to Reed, I narrowed this down a much better regression range.  My money is on the protocol handler checkin.

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1185422820&maxdate=1185430739
confirmed on Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a7pre) Gecko/200707260404 Minefield/3.0a7pre - i see the same like clint

this works on Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.6) Gecko/2007072517 Firefox/2.0.0.6 (Firefox 2.0.0.6 RC2)- so it seems this is a trunk regression only
Version: unspecified → Trunk
duping to Bug 389870 because 389870 has a patch
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → DUPLICATE
Turns out this still happens even after the fix to bug 389870 landed.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
/desktop/gnome/url-handlers/webcal contains the handler that older code finds and newer code doesn't.
(according to gconf-editor, that is)
So, the immediate cause if this is that when the dialog is building its listbox, it checks |if (this._handlerInfo.hasDefaultHandler)|, which is false. That's because nsOSHelperAppService::GetProtocolInfoFromOS doesn't call handlerInfo->SetDefaultApplication().

It gets kind of ugly from here, because it seems like code in nsGNOMERegistry returns the default app as a string (calling it a "description" in the process)... The above code does call handlerInfo->SetDefaultDescription(), which gives us the string "/usr/lib/evolution-webcal/evolution-webcal %s". I think we need to:

1) change nsGNOMERegistry to return a human-readable string as the "description"
2) add an interface there to return the nsIFile for the app, instead of the semiformatted commandline
3) use these interfaces in ::GetProtocolInfoFromOS

#2 makes me think that something in here needs to be able to deal with formatted commandlines (eg, "/usr/bin/foo -bar -with %s -baz"), which I'm not sure is handled right now?
Assignee: nobody → dolske
Status: REOPENED → NEW
Flags: blocking-firefox3? → blocking-firefox3+
Summary: WebCal Links no longer Work with Evolution on Ubuntu 7.04 → WebCal links no longer work with Evolution on Ubuntu 7.04
Moving out after discussion with mconnor.  We might take a fix for this for M7, but we don't want to block on it.  If we don't take a fix, we should probably relnote it.

dolske: Those comments look right to me.  What do you think of it having the interface described in 2) instead return an nsILocalHandlerApp which would then live as a defaultApplicationHandler attribute on nsIHandlerInfo?  This would allow us to get rid of defaultDescription and hasDefaultHandler, further simplifying nsIHandlerInfo. 
Target Milestone: Firefox 3 M7 → Firefox 3 M8
This is actually working now from the checkin for bug 389969.
It is the nasty "mDefaultAppDescription means there is a system handler" fix, which I really threw in to help me test bug 389969.

Could there be consequences from this,
if somehow an mDefaultAppDescription existed but the system couldn't handle it?

Leaving this open so it can be fixed as suggested above.
Depends on: 389969
according to biesi, if the description exists, the system can handle it.
No longer blocks: 385065
Depends on: 385065
Target Milestone: Firefox 3 M8 → Firefox 3 M9
The regression bug here has been papered over, as indicated by karlt, so removing the regression keyword.  What's left is simplification/cleanup work, so marking as blocking bug 393122.
Blocks: 393122
Keywords: regression
Summary: WebCal links no longer work with Evolution on Ubuntu 7.04 → hard-to-understand presentation of protocol-handlers on Linux
I'm not actively working on this, so I'm going to unassign myself.
Assignee: dolske → nobody
Assignee: nobody → dmose
moving out bugs that don't need to block b1
Target Milestone: Firefox 3 M9 → Firefox 3 M10
Priority: -- → P4
Target Milestone: Firefox 3 M10 → ---
Keywords: uiwanted
Target Milestone: --- → Firefox 3 M10
Priority: P4 → P2
Whiteboard: [proto]
We already have a bug for passing command-line parameters to apps (that's not going to make Firefox 3).  

So I think all that really needs to be done to fix this bug is to make nsGNOMERegistry::GetAppDescForScheme be smarter.  Michael, is this something you'd be up for taking a crack at for M10?
Whiteboard: [proto] → [proto][needs patch]
I don't think Linux programs have descriptions that can be easily gotten, however one thing I can do at this stage is limit the output to just the executable name, getting rid of the path and command line options, and use that as the description.
Attached patch Patch (obsolete) — Splinter Review
Descriptions are stored in .desktop files, but not every executable has a .desktop file, especially in the case of some programs like Evolution or Pidgin which have separate executables for incoming protocol handling. I think the simplest and least risky way to improve things here is to only show the name of the executable. Strip off the path and the arguments.
Assignee: dmose → ventnor.bugzilla
Status: NEW → ASSIGNED
Attachment #290148 - Flags: superreview?(cbiesinger)
Attachment #290148 - Flags: review?(cbiesinger)
Sounds good to me.  Would it be reasonable/non-risky to look for a .desktop file first and then fall back to the "just show the executable" approach?
(In reply to comment #17)
> Sounds good to me.  Would it be reasonable/non-risky to look for a .desktop
> file first and then fall back to the "just show the executable" approach?
> 

There isn't really a way to get the desktop information for an executable name with the documented GNOME functions.
Ah well.
Keywords: uiwanted
Whiteboard: [proto][needs patch] → [proto][needs ui-r, r, sr]
Attachment #290148 - Flags: ui-review?(beltzner)
Whiteboard: [proto][needs ui-r, r, sr] → [proto][needs ui-r beltzner, r, sr]
Attachment #290148 - Flags: ui-review?(beltzner) → ui-review+
Whiteboard: [proto][needs ui-r beltzner, r, sr] → [proto][needs r/sr biesi]
Comment on attachment 290148 [details] [diff] [review]
Patch

+        execName = firstSpace + (lastSlash - firstSpace + 1);

make that lastSlash + 1

Also please move the declaration of the variables on the lines where they are used

I'm not really happy with you using strtok here, can you just use strchr + manual setting to '\0'? I think that would be easier to understand.
Attachment #290148 - Flags: superreview?(cbiesinger)
Attachment #290148 - Flags: review?(cbiesinger)
Attachment #290148 - Flags: review-
Whiteboard: [proto][needs r/sr biesi] → [proto][needs new patch]
Attached patch Patch 2Splinter Review
Attachment #290148 - Attachment is obsolete: true
Attachment #291269 - Flags: superreview?(cbiesinger)
Attachment #291269 - Flags: review?(cbiesinger)
Whiteboard: [proto][needs new patch] → [proto] [needs r, sr biesi]
Attachment #291269 - Flags: superreview?(cbiesinger)
Attachment #291269 - Flags: superreview+
Attachment #291269 - Flags: review?(cbiesinger)
Attachment #291269 - Flags: review+
Attachment #291269 - Flags: approval1.9?
Comment on attachment 291269 [details] [diff] [review]
Patch 2

Whoops, this is blocking. No need for approval.
Attachment #291269 - Flags: approval1.9?
Keywords: checkin-needed
Whiteboard: [proto] [needs r, sr biesi] → [proto] [needs checkin]
Checking in uriloader/exthandler/unix/nsGNOMERegistry.cpp;
/cvsroot/mozilla/uriloader/exthandler/unix/nsGNOMERegistry.cpp,v  <--  nsGNOMERegistry.cpp
new revision: 1.17; previous revision: 1.16
done
Status: ASSIGNED → RESOLVED
Closed: 16 years ago15 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Summary: hard-to-understand presentation of protocol-handlers on Linux → Hard-to-understand presentation of protocol-handlers on Linux
Whiteboard: [proto] [needs checkin] → [proto]
Verified in Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9pre) Gecko/2008050904 Minefield/3.0pre.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.