608198 - Prefixing text in the location bar with "?" includes that in the search

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect)

Description

[Dave Townsend [:mossop]]

It used to be the case that putting a ? before what you typed into the location bar forced a search of the following test using keyword.URL however the ? was not included in the search. Now it is getting included.

Comment 1

[Dave Townsend [:mossop]]

This is irritating when you need to use this to force a search for a string that normally would resolve to a website.

Comment 2

[Dietrich Ayala (:dietrich)]

Looks broken. Blocking+. Anyone know what could've caused this (and the tests still passing)?

Comment 3

[Shawn Wilsher :sdwilsh]

(In reply to comment #2)
> Looks broken. Blocking+. Anyone know what could've caused this (and the tests
> still passing)?
Not offhand.  I don't recall anything changing with the keyword code recently...

Comment 4

[Ed Lee :Mardak]

keyword.URL was changed when disabling browse by name.

Just checking though, "?" wasn't a bookmark keyword or a search engine keyword? It was some special hardcoded token?

Comment 5

[Dave Townsend [:mossop]]

Yes it was hardcoded in the docshell fixup stuff somewhere.

Comment 6

[Ed Lee :Mardak]

Oh I see. Seems like the code is still there:

http://hg.mozilla.org/mozilla-central/file/29ea30892eae/docshell/base/nsDefaultURIFixup.cpp#l357

Comment 7

[Frank Yan (:fryn)]

cc'ing gavin, since he made the docshell changes for disabling browse-by-name.

Comment 8

[Ed Lee :Mardak]

One thing that changed is that it doesn't seem to be hitting that code I linked in comment 6. That code appends the keyword whereas trunk seems to be inserting it in the middle:

var {classes:Cc,interfaces:Ci}=Components; Components.classes["@mozilla.org/docshell/urifixup;1"].getService(Ci.nsIURIFixup).createFixupURI("? foo", 1).spec

http://www.google.com/search?q=%3F+foo&ie=utf-8&...

Comment 9

[Ed Lee :Mardak]

Oh hehe. keyword.URL is empty ;)

Mossop: If you set this, do you get the original behavior?

keyword.URL;http://www.google.com/search?ie=UTF-8&oe=UTF-8&sourceid=navclient&q=

Comment 10

[Dave Townsend [:mossop]]

Yeah that'd be the problem

Comment 11

[Frank Yan (:fryn)]

(In reply to comment #9)
> Oh hehe. keyword.URL is empty ;)

keyword.URL was intentionally changed to empty, since we want the dynamically set affiliate bits in the URL.

Comment 12

[Dave Townsend [:mossop]]

(In reply to comment #11)
> (In reply to comment #9)
> > Oh hehe. keyword.URL is empty ;)
> 
> keyword.URL was intentionally changed to empty, since we want the dynamically
> set affiliate bits in the URL.

Right, but as it stands since bug 586821 if there is no keyword.URL then MangleKeywordIntoURI is never called and so the special handling of the ? prefix doesn't happen.

Comment 13

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

The main change is to just always do the "?"-removal and space-trimming, regardless of which path is taken (search service or pref value).

Also updated browser_keywordSearch to mostly cover nsDefaultURIFixup::KeywordToURI (it's under browser because it depends on Firefox-specific behavior details - don't really want to do the work to remove that dependency).

Aside: the test was kind of busted (used nsIWebProgressListener.NOTIFY_STATE_DOCUMENT which doesn't exist, which didn't matter because of bug 608628), but that was only exposed when I made it do more than one load, so I had to fix that too.

Comment 14

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 15

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/mozilla-central/rev/4c55c33b49d3