30.42 KB, patch
Darin Fisher: superreview+
|Details | Diff | Splinter Review|
915 bytes, patch
|Details | Diff | Splinter Review|
32.06 KB, patch
|Details | Diff | Splinter Review|
When using keyword searches from the url bar (for failed urls) the page displayed appears broken when relative urls are used. To reproduce: 1. Set keyword.URL to http://users.blueprintit.co.uk/~dave/filestore/keyword/testsearch.html?whatever= 2. Type "testing testing" into the url bar. 3. View the page and see that it has a broken image. Regression range: http://tinderbox.mozilla.org/bonsai/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1146743774&maxdate=1146855420 Bug 335457 is about the only likely looking candidate to my eyes.
So the problem is that the HTTP channel the keyword protocol handler creates has the keyword: URI as the originalURI and does NOT have LOAD_REPLACE set. The only way I see offhand to deal with this is to have a separate keyword channel class that would wrap the final channel and add the LOAD_REPLACE flag, then just pass on AsyncOpen. Do we care to QI to nsIHttpChannel and such too, sorta the way view-source does? It's only an issue for the docshell code that creates the channel, and I'd _think_ we don't want referrers and such going with a keyword search, right? Or POST data? Or anything else like that? So I'm tempted to say "no"...
Right, it should not need to implement those interfaces. View source is special because we want to make sure we can view what would be loaded w/ various configurations of the HTTP channel. That said, I'd really like to avoid adding another wrapping nsIChannel implementation if it can be helped. Why is LOAD_REPLACE needed? Why isn't the solution merely to set the OriginalURI property on the channel created by nsKeywordProtocolHandler::NewChannel?
The nsResProtocolHandler is very similar to nsKeywordProtocolHandler, and all it does is set OriginalURI. Is it also broken? If not, then let's just change the keyword protocol handler to do the same thing.
I also don't understand why you can't just set the load flag using SetLoadFlags?
The call tree here is: nsDocShell stuff NS_NewChannel ioService->NewChannel(); channel->SetLoadFlags() channel->SetOriginalURI(); So the docshell will always clobber whatever original URI got set in newChannel. NS_NewChannel will always clobber whatever flags got set in newChannel. Hence setting either one in nsKeywordProtocolHandler::NewChannel is effectively a no-op. I need to set them later. > Why is LOAD_REPLACE needed? Because unlike the resource: case (where we want the document to see the original resource: uri as opposed to the final jar: or file: URI), here we want the document to see the final http: URI, not the original keyword: URI.
OK, I see. I guess I don't understand fully how the system treats LOAD_REPLACE. I still think it is a good idea for nsKeywordProtocolHandler::NewChannel to set the OriginalURI property of the resulting channel for consistency. That way it is clear from the channel that another URI (the OriginalURI) was passed to NewChannel.
The way LOAD_REPLACE is treated is that if it's set then the channel's URI is the "real" URI. If it's not set, then the originalURI is the "real" URI....
See also bug 264830, "If keyword.URL doesn't lead to a redirect, resulting page has bogus 'keyword:' URL".
Yeah, that's basically the same issue.
The keyword: url remaining in the url bar happened even in Firefox 1.0 from my tests, but the pages it displayed worked correctly.
Right, but that was a bug. ;) Which I fixed in bug 335457. So now we consistently treat that page as having a keyword: URL.... Anyway, I'll try to do comment 1 in the next couple of days.
11 years ago
Created attachment 221872 [details] [diff] [review] Fix
Created attachment 221880 [details] [diff] [review] Darin voted that we nuke this altogether Is there a way I can make the keyword dir not pull too?
(In reply to comment #13) > Is there a way I can make the keyword dir not pull too? You just have to remove all files (including .cvsignore files) Does anyone use the keyword: protocol directly, who would be broken by this?
Heh, I frequently type "keyword:" followed by some search terms into my Firefox. That's only because I had to hack off the automatic attempts at keyword searching because they suck lots, though. :) It also has the advantage that they are remembered and kept in the auto-complete list...
> Does anyone use the keyword: protocol directly, who would be broken by this? There are no consumers in mozilla.org CVS. It's DenyProtocol in CheckLoadURI, so can't be linked to. That leaves people typing it in by hand (like James) and extensions. Frankly, I would say the former is a tiny number and the latter can use the URI fixup API I'm exposing.
Comment on attachment 221880 [details] [diff] [review] Darin voted that we nuke this altogether OK, users who want this can probably just create a bookmark w/ keyword. nsDefaultURIFixup.cpp MangleKeywordIntoURI is just copied from elsewhere, right? + // if we can't find a keyword.URL keywords won't work. + if (url.IsEmpty()) + return NS_ERROR_FAILURE; NS_ERROR_NOT_AVAILABLE?
someone should remove shopKeyword and such from http://lxr.mozilla.org/seamonkey/source/xpfe/browser/resources/locale/en-US/region.properties#3
Created attachment 221989 [details] [diff] [review] Updated to biesi's comments > MangleKeywordIntoURI is just copied from elsewhere, right? Yes, from the current keyword: protocol handler.
Fixed. Rest in peace, little protocol handler. Let your dreams be untroubled.
Created attachment 222720 [details] [diff] [review] remove netwerk/protocol/keyword/ from allmakefiles.sh netwerk/protocol/keyword/still remains in allmakefiles.sh.
Comment on attachment 222720 [details] [diff] [review] remove netwerk/protocol/keyword/ from allmakefiles.sh I just checked this in. In general, though, it's worth it to request review to keep patches from getting lost.
Anyone have cycles to get a 1.8 branch suitable version of this? We need bug 335457 for that branch and don't want to take that without a fix for this.
Darin, do you think it's OK to remove keyword: on branch? If so, we just need to deal with the nsIURIFixup changes... Otherwise we need a totally different approach.
Yeah, just remove it! :)
OK. Merging this to branch correctly will be much more painful and time-consuming than I can deal with in the near future because bug 245597 landed on branch, but not the same way it landed on trunk, etc. Easiest might be to just back out that checkin, land this, then land the trunk version of that bug on branch; failing that, the patch here needs surgery. I won't be able to do anything like that until I get back in mid-July at this point, esp. since I'd need to write tests to make sure I'm not regressing bug 245597. Peter, if you have such tests they'd be much appreciated (and should be checked into the tree!). If no one is able to do this before then, I guess I'll look at this when I get back...
I'm not sure how to write tests for this, but I do have a set of testcases that I test to make sure my patch is working, in the form of typing things into the urlbar and seeing what they do. If anyone can give me a pointer on how to turn those into automated tests I'd be happy to write them. As I mentioned on bug 245597, I'd actually _prefer_ that you back out my branch patch, merge this, and then merge my trunk patch. I don't know how cleanly your patch merges otherwise. If it's clean, someone else with CVS access can get the work done. Presumably the patches that need a+ are the last two listed above?
Created attachment 227330 [details] [diff] [review] Merged to 1.8 branch This is the 1.8 branch version of the patch; I backed out the branch version of bug 337339, merged, and reapplied the trunk version of bug 337339.
Fixed on 1.8.1 branch.