500324 - "Open" button in download manager doesn't correctly launch the file in its viewer

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Al Billings [:abillings - ex-MoCo]]

On Maemo, if a user downloads a PDF (one of the few file types that we can save), and attempts to open it via the download manager, the PDF Viewer for Maemo will open but without loading the file. It should load the file. 

This could be a PDF viewer bug but it may be our interaction with it.

This was found in the Beta 2 candidate build from 6/24.

Steps to Reproduce
1. Go to http://www.irs.gov/pub/irs-pdf/fw4.pdf and save the PDF when prompted.
2. Click on the options button in the right pane
3. Click on the download manager button in options
4. On the line for fw4.pdf in the download manager, click on "Open"

Result: PDF Viewer opens but without loading the file.

Comment 1

[Joel Maher ( :jmaher ) (UTC -8)]

I have verified this on my n810.  I believe we have had this problem in the past, but at a beta2 quality, we should be getting rid of a lot of these issues.

Comment 2

[Al Billings [:abillings - ex-MoCo]]

In Microb, the pdf file is opened within the PDF Viewer.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch

This patch adds the dbus and hildon code required to launch a file

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch 2

Removed the failed attempt to fix ::Reveal()

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 388624 [details] [diff] [review]
patch 2

This code is a minor bit of duplication from some code in nsMIMEInfoUnix.cpp which also launches files, but with the aid of a mime-type. We have no mime-type here, so we only use the fallback hildon method.

Tested and works in Fennec/Maemo

Comment 6

[Benjamin Smedberg]

Comment on attachment 388624 [details] [diff] [review]
patch 2

>diff --git a/xpcom/io/Makefile.in b/xpcom/io/Makefile.in

>+ifdef MOZ_PLATFORM_HILDON
>+LOCAL_INCLUDES   += $(TK_CFLAGS) $(MOZ_DBUS_GLIB_CFLAGS)
>+EXTRA_DSO_LDOPTS += $(MOZ_DBUS_GLIB_LIBS)
>+ifdef MOZ_ENABLE_GNOMEVFS
>+LOCAL_INCLUDES   += $(MOZ_GNOMEVFS_CFLAGS)
>+EXTRA_DSO_LDOPTS += $(MOZ_GNOMEVFS_LIBS)
>+endif
>+endif

EXTRA_DSO_LDOPTS has no effect in xpcom/io, please remove it.

>diff --git a/xpcom/io/nsLocalFileUnix.cpp b/xpcom/io/nsLocalFileUnix.cpp

> NS_IMETHODIMP
> nsLocalFile::Launch()

The actual logic-guts here need review by someone who knows dbus/hildon.

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 388624 [details] [diff] [review]
patch 2

>diff --git a/xpcom/io/Makefile.in b/xpcom/io/Makefile.in
>+
>+ifdef MOZ_PLATFORM_HILDON
>+LOCAL_INCLUDES   += $(TK_CFLAGS) $(MOZ_DBUS_GLIB_CFLAGS)
>+EXTRA_DSO_LDOPTS += $(MOZ_DBUS_GLIB_LIBS)
>+ifdef MOZ_ENABLE_GNOMEVFS
>+LOCAL_INCLUDES   += $(MOZ_GNOMEVFS_CFLAGS)
>+EXTRA_DSO_LDOPTS += $(MOZ_GNOMEVFS_LIBS)
>+endif
>+endif
>+

a couple of nits here.  You shouldn't need the MOZ_GNOMEVFS_* stuff.  Those paths will be provided by LIBHILDONMIME_CFLAGS, which you should use.  You also shouldn't need TK_CFLAGS.

also, I think these should be added to CFLAGS or CXXFLAGS and not LOCAL_INCLUDES.

unrelated to this bug, it seems we're not using LIBHILDONMIME_CFLAGS in uriloader/exthandler/Makefile.in, which I think we should.

> 
>-LOCAL_INCLUDES	= -I..
>+LOCAL_INCLUDES	+= -I..

Shouldn't need this if you switch to using CFLAGS above

r=me with nits addressed

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed with all review nits fixed:
http://hg.mozilla.org/mozilla-central/rev/137530fe08c1