Closed Bug 475210 Opened 15 years ago Closed 15 years ago

URLs in tooltips need to be unescaped

Categories

(Camino Graveyard :: General, defect)

All
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: alqahira, Assigned: alqahira)

References

()

Details

Attachments

(1 file, 2 obsolete files)

We currently show URLs in tooltips for Back, Forward, and the throbber in the toolbar (I think that's it), for any bookmarks in the Bookmark Bar, and for completed downloads.  There may be other places I'm forgetting.

None of these are currently escaped, and they all probably should be.
Attached patch Unescaped tooltips (obsolete) — Splinter Review
Ignore what I said about the throbber; it can only be http://caminobrowser.org :P

This patches the three places that can actually have UTF-8 URIs.  Is there a better place (helper/shared method) to do the unescaping in PVC rather than the two places?
Assignee: nobody → alqahira
Status: NEW → ASSIGNED
Attachment #358792 - Flags: review?(trendyhendy2000)
Attached patch Unescaped tooltips, v1.1 (obsolete) — Splinter Review
This consolidates the PVC code; hendy explained to me that the crashes/hangs from when I tried this approach before were due to the fun world of Cocoa memory management, of which I know nothing ("unescapedURI returns an autoreleased object, and we need a retained one").
Attachment #358792 - Attachment is obsolete: true
Attachment #358797 - Flags: review?(trendyhendy2000)
Attachment #358792 - Flags: review?(trendyhendy2000)
Comment on attachment 358797 [details] [diff] [review]
Unescaped tooltips, v1.1

Works as intended. There are a few other tooltips that need to be escaped (eg: BM Manager), but I'll do them as part of Bug 464501.
Attachment #358797 - Flags: superreview?(stuart.morgan+bugzilla)
Attachment #358797 - Flags: review?(trendyhendy2000)
Attachment #358797 - Flags: review+
Attachment #358797 - Flags: superreview?(stuart.morgan+bugzilla) → superreview-
Comment on attachment 358797 [details] [diff] [review]
Unescaped tooltips, v1.1

> -(void)setSourceURL:(NSString*)aSourceURL
> {
>   [mSourceURL autorelease];
>-  mSourceURL = [aSourceURL copy];
>+  mSourceURL = [[[aSourceURL copy] unescapedURI] retain];
>   [mProgressView setToolTip:mSourceURL];
>   [mCompletedView setToolTip:mSourceURL];
>   //[self tryToSetFinderComments];
> }

This is so un-crashy it leaks ;) It should be [[aSourceURL unescapedURI] copy]--except that I don't know that we want exactly this. This means things like "Copy Source URL" will get an unescaped version, and anything in the code that asks the PVC for its URL will get an unescaped version. I'd feel better about just changing the tooltips for now.
(In reply to comment #4)
> This is so un-crashy it leaks ;) It should be [[aSourceURL unescapedURI]
> copy]--except that I don't know that we want exactly this. This means things
> like "Copy Source URL" will get an unescaped version, and anything in the code
> that asks the PVC for its URL will get an unescaped version. 

The prevailing thought in the other bug is we want to copy an unescaped version, so long as we also include an escaped flavor, too (which we wouldn't be doing here, so yeah; this bug was only about tooltips, anyway ;) ).

> I'd feel better about just changing the tooltips for now.

So attachment 358792 [details] [diff] [review], then?
Yeah, only store [mSourceURL unescapedURI] locally and use that variable, so the work isn't being done twice.
Like so?
Attachment #358797 - Attachment is obsolete: true
Attachment #359218 - Flags: superreview?(stuart.morgan+bugzilla)
Attachment #359218 - Flags: superreview?(stuart.morgan+bugzilla) → superreview+
Comment on attachment 359218 [details] [diff] [review]
Unescaped tooltips, v1.3

Yep :) sr=smorgan
Checked in.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: