124014 - URL handled by helper app never marked visited

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect, P1)

Description

[Robin Green]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:0.9.8) Gecko/20020205
BuildID:    2002020511

Say I have xpdf set up to handle PDF files. Clicking on a PDF link launches the
helper app but the link is never marked visited.

Reproducible: Always
Steps to Reproduce:
1. Go to URL above
2. Click on "Pennsylvania Charter Schools"
3. Reload page

Actual Results:  Link clicked on is not marked visited (unlike if you click on
ordinary HTML links).

Expected Results:  Link should appear in visited color.

Comment 1

[Boris Zbarsky [:bzbarsky]]

To file handling.  We should add these to history, I guess....

Comment 2

[Bill Law]

Future-ing; if somebody thinks its really important, please nominate by setting 
the nsbeta1 keyword.

The fix here probably isn't too hard.

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

taking. this bug has always annoyed me :)

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch

this adds a dependency (REQUIRES) on appcomps... I don't know if that's such a
good thing...

I put the code to mark the url visited in OnStartRequest. I think I like that
best, but I'm open to other suggestions (e.g. OnStopRequest or something)

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 126747 [details] [diff] [review]
patch

> 		  uconv \
> 		  windowwatcher \
>+			appcomps \
> 		  $(NULL)

Fix the weird indent, and looks good.

Comment 6

[Darin Fisher]

Comment on attachment 126747 [details] [diff] [review]
patch

>Index: nsExternalHelperAppService.cpp

>     else
>     {
>         rv = LaunchWithApplication(nsnull, PR_FALSE);
>     }
>   }
> 
>+  // Now let's mark the downloaded URL as visited
>+  nsCOMPtr<nsIGlobalHistory> history(do_GetService(NS_GLOBALHISTORY_CONTRACTID));
>+  nsCAutoString spec;
>+  mSourceUrl->GetSpec(spec);
>+  if (history && !spec.IsEmpty()) {
>+    history->AddPage(spec.get());
>+  }
>+

nit: inconsistent brace styles.  but, looks like that's just a problem
with this file in general :-/

more importantly, there seems to be a i18n problem here.  namely, GetSpec
may return UTF-8, but AddPage accepts only ASCII.  AddPage should probably
take a |AUTF8String| param instead of a |string|, but that's probably a
whole other bug.  you could use GetAsciiSpec to get a fully escaped ASCII
URL string, but then that will not necessarily be very user friendly (think
about internationalized domain names, which when converted to ASCII have a
very very non-human-readable form).

Comment 7

[Boris Zbarsky [:bzbarsky]]

> AddPage should probably take a |AUTF8String| param

That interface is frozen.  Hurray.

> namely, GetSpec may return UTF-8, but AddPage accepts only ASCII

In practice, that's only an issue when calling AddPage from JS (because
XPConnect will strip off the high bits).  When calling from C++, things happen
to work because we just pass through the raw pointer...

So yes, the history APIs totally suck, but there is nothing we can do about that
at this point.

Comment 8

[Darin Fisher]

Comment on attachment 126747 [details] [diff] [review]
patch

ok, since everyone is already abusing nsIGlobalHistory::AddPage, i guess we
should continue to do the same.  we should try to fix this issue at some point
though.

sr=darin with style nits.

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

bug 211120 filed about AddPage sucking

patch checked in with nits addressed:
Checking in Makefile.in;
/cvsroot/mozilla/uriloader/exthandler/Makefile.in,v  <--  Makefile.in
new revision: 1.43; previous revision: 1.42
done
Checking in nsExternalHelperAppService.cpp;
/cvsroot/mozilla/uriloader/exthandler/nsExternalHelperAppService.cpp,v  <-- 
nsExternalHelperAppService.cpp
new revision: 1.192; previous revision: 1.191
done