Closed
Bug 402589
Opened 18 years ago
Closed 18 years ago
Camino passes https feed URIs to helper apps as generic/non-https feed URIs
Categories
(Camino Graveyard :: OS Integration, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: alqahira, Assigned: philor)
References
()
Details
(Keywords: fixed1.8.1.12)
Attachments
(2 files)
|
1.63 KB,
patch
|
nick.kreeger
:
review+
mikepinkerton
:
superreview+
|
Details | Diff | Splinter Review |
|
1.63 KB,
patch
|
Details | Diff | Splinter Review |
I ran into this one night while testing my handling code for bug 355748 but forgot to file it then.
If you have a feed URI that should be https (e.g., one from a bugzilla bug list, which is specified as relative URL), we pass the URI as feed://bugzilla.mozilla.org/foo instead of feed:https://bugzilla.mozilla.org/foo
In the Bugzilla case, there's an automatic redirect of http://bmo/* to https://bmo/* but presumably not all https sites are set up that way.
|
Assignee | |
Comment 1•18 years ago
|
||
If it wasn't ugly, it wouldn't be RSS.
Assignee: nobody → philringnalda
Status: NEW → ASSIGNED
Attachment #287511 -
Flags: review?(nick.kreeger)
|
|
Assignee | |
Comment 2•18 years ago
|
||
Lest I forget along the way, "is one only one call" has one "one" over the limit.
|
||
Comment 3•18 years ago
|
||
Comment on attachment 287511 [details] [diff] [review]
Fix v.1
Looks good, one nit:
+ feedFullURI.Insert(NS_LITERAL_CSTRING("feed:"),0);
please put a space between arguments in the function call.
Attachment #287511 -
Flags: review?(nick.kreeger) → review+
|
|
Assignee | |
Comment 4•18 years ago
|
||
Comment on attachment 287511 [details] [diff] [review]
Fix v.1
Okay, three nits locally fixed, the comment, the space in the args, and trailing whitespace at the end, my nemesis. I'm still a little bothered by the way I didn't actually check the rv from SchemeIs, but if it fails the world's going to end in OOM in milliseconds, and looking at the utterly random behavior in the tree around SchemeIs, this isn't any worse than the average.
Attachment #287511 -
Flags: superreview?(mikepinkerton)
|
|
||
Comment 5•18 years ago
|
||
Comment on attachment 287511 [details] [diff] [review]
Fix v.1
sr=pink
Attachment #287511 -
Flags: superreview?(mikepinkerton) → superreview+
|
|
Assignee | |
Comment 6•18 years ago
|
||
If the world keeps conspiring against me, I'm going to wind up begging for checkin.
|
|
Assignee | |
Comment 7•18 years ago
|
||
Trunk: camino/src/embedding/CHBrowserListener.mm 1.46
1.8: camino/src/embedding/CHBrowserListener.mm 1.22.2.21
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
|
Reporter | |
Updated•18 years ago
|
Keywords: fixed1.8.1.11
|
|
||
Comment 8•18 years ago
|
||
+ rv = feedURI->SchemeIs("http", &isHttp);
+ if (isHttp)
+ {
+ // for http:, we want feed://example.com
+ feedURI->SetScheme(NS_LITERAL_CSTRING("feed"));
+ feedURI->GetAsciiSpec(feedFullURI);
+ }
+ else
+ {
+ // for https:, we want feed:https://example.com
+ feedURI->GetAsciiSpec(feedFullURI);
+ feedFullURI.Insert(NS_LITERAL_CSTRING("feed:"), 0);
+ }
Not to come in here in the 11th hour.... but this comment doesn't just apply to 'https:' if someone configured a different scheme (say ftp) we wouldn't be appending https:...
Perhaps we shouldn't worry about freak-cases and change the else to an else if to check for feedURI->SchemeIs("https", &isHttp). Maybe changing the name of the local bool to something other than just 'isHttp'.
|
|
Assignee | |
Comment 9•18 years ago
|
||
13th hour, and yes, no, no.
The feed "spec" is essentially "stick 'feed:' in front of your URI, but if it's http (not https) you can s/http/feed/ instead."
So the "for https:, we want" comment is true, it just doesn't explain that "and for ftp:, we want feed:ftp://, and for quux, we want feed:quux://"
The code works (data:text/html,<head><link%20rel="feed"%20type="application/atom+xml"%20href="ftp://ftp.mozilla.org"> will correctly send feed:ftp://ftp.mozilla.org to your reader), the else is correct, the bool name is correct (there's a single special case, http:, and everything else, !isHttp), I just didn't think of a good way to say "for everything else, we want feed:everythingelse://example.com" in under 80 chars before I got tired of thinking and went with the common case.
You need to log in
before you can comment on or make changes to this bug.
Description
•