Closed
Bug 475210
Opened 15 years ago
Closed 15 years ago
URLs in tooltips need to be unescaped
Categories
(Camino Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: alqahira, Assigned: alqahira)
References
()
Details
Attachments
(1 file, 2 obsolete files)
3.78 KB,
patch
|
stuart.morgan+bugzilla
:
superreview+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•15 years ago
|
||
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)
Assignee | ||
Comment 2•15 years ago
|
||
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 3•15 years ago
|
||
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+
Updated•15 years ago
|
Attachment #358797 -
Flags: superreview?(stuart.morgan+bugzilla) → superreview-
Comment 4•15 years ago
|
||
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.
Assignee | ||
Comment 5•15 years ago
|
||
(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?
Comment 6•15 years ago
|
||
Yeah, only store [mSourceURL unescapedURI] locally and use that variable, so the work isn't being done twice.
Assignee | ||
Comment 7•15 years ago
|
||
Like so?
Attachment #358797 -
Attachment is obsolete: true
Attachment #359218 -
Flags: superreview?(stuart.morgan+bugzilla)
Updated•15 years ago
|
Attachment #359218 -
Flags: superreview?(stuart.morgan+bugzilla) → superreview+
Comment 8•15 years ago
|
||
Comment on attachment 359218 [details] [diff] [review] Unescaped tooltips, v1.3 Yep :) sr=smorgan
Assignee | ||
Comment 9•15 years ago
|
||
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.
Description
•