Closed Bug 724841 Opened 12 years ago Closed 12 years ago

Searching from the address bar lower cases search phrase sent to search engines


(Core :: DOM: Navigation, defect)

Windows 7
Not set





(Reporter: jimm, Assigned: ekw)



(3 files, 6 obsolete files)


1) search from the address bar with text that includes capital letters.

Example "TTDeleteEmbeddedFont"

result: the phrase will be sent to your default search engine as lower case, e.g. "ttdeleteembeddedfont" - 


I wasn't able to find another bug on this, although I'm surprised it hasn't been filed.

Note, both IE and Chrome send the original, properly cased phrase.
Assignee: nobody → ekw1
Bug only occurs with one word searches from location bar due to attempt to load as URL first:

"TTDeleteEmbeddedFont" --> Try to load http://ttdeleteembeddedfont --> Search for "ttdeleteembeddedfont"

Two or more word searches from location bar are fine (there's no attempt to load as URL first):

"TTDeleteEmbeddedFont Other Stuff" --> search for "TTDeleteEmbeddedFont Other Stuff"

This patch saves the original text when nsStandardURL object is created (via nsIURI interface) and uses it later when load fails and text is sent to search engine service.
Attachment #602816 - Flags: review?(bzbarsky)
I haven't run any tests yet.  This being my first attempt at fixing a mozilla bug, I'm trying to figure out which tests I should run.  Any help here appreciated.
I think it's probably a little too invasive to be making an API addition to nsIURI just to cover this use case (nsIURI is a "core" interface, and this doesn't seem like something relevant to most implementations of it). Can nsDocShell keep a reference to the source string through some other mechanism?
Oh, and great work looking into the issue, Eric - impressive effort for your first patch :)
Thanks, I will look into keeping things within nsDocShell.
Comment on attachment 602816 [details] [diff] [review]
Retain case when sending location bar text to search engine service.

Yeah, I don't think we want to put this on nsIURI....
Attachment #602816 - Flags: review?(bzbarsky) → review-
Attached patch Keep changes within nsDocShell (obsolete) — Splinter Review
Is this too simple?  Am I missing anything?
Attachment #602816 - Attachment is obsolete: true
Attachment #604230 - Flags: feedback?(
Attached file mochitest results for docshell/test (obsolete) —
543 INFO Passed: 430
544 INFO Failed: 0
545 INFO Todo:   0
Comment on attachment 604230 [details] [diff] [review]
Keep changes within nsDocShell

Looks reasonable to me! bz should do the actual review (I haven't verified that all of the possible code paths where mOriginalUriString is set correspond to paths where it is used, and vice-versa).
Attachment #604230 - Flags: feedback?( → feedback+
Component: Location Bar → Document Navigation
Product: Firefox → Core
QA Contact: → docshell
Attachment #604230 - Flags: review?(bzbarsky)
Whiteboard: [autoland-try:-p linux,linux64,macosx64,win32,macosx -u mochitest -t none]
Whiteboard: [autoland-try:-p linux,linux64,macosx64,win32,macosx -u mochitest -t none] → [autoland-in-queue]
Autoland Patchset:
	Patches: 604230
	Branch: mozilla-central => try
Insufficient permissions to push to try.
Whiteboard: [autoland-in-queue]
Try run for d2f40a675b89 is complete.
Detailed breakdown of the results available here:
Results (out of 93 total builds):
    success: 84
    warnings: 9
Builds (or logs if builds failed) available at:
Comment on attachment 604230 [details] [diff] [review]
Keep changes within nsDocShell

No, this is definitely wrong.  There are loading entrypoints that start with an nsIURI, not a string, but can end up in EndPageLoad.  For example <a href="something"> loads.  With this patch if you do a load from the url bar and then click on an anchor and the anchor fails to resolve we'll do a search for the thing that originally came from the url bar.  Worth adding a testcase.

What should probably happen is that starting a load (InternalLoad) always clears mOriginalUriString.  And you'd want to set mOriginalUriString in the string version of LoadURI only after calling the URI version of LoadURI.
Attachment #604230 - Flags: review?(bzbarsky) → review-
Attached file Made changes based on Comment 12 (obsolete) —
Made changes based on Boris Zbarsky's comment 12:
"What should probably happen is that starting a load (InternalLoad) always clears mOriginalUriString.  And you'd want to set mOriginalUriString in the string version of LoadURI only after calling the URI version of LoadURI."

This latest patch also fixes the bug.  If feedback is positive, I will look into adding a testcase for this (and will need pointers on where/how to add a testcase as this is my first bug).
Attachment #604230 - Attachment is obsolete: true
Attachment #604232 - Attachment is obsolete: true
Attachment #607008 - Flags: feedback?(bzbarsky)
Comment on attachment 607008 [details]
Made changes based on Comment 12

> +    mOriginalUriString.Assign(NULL, 0);


Looks fine to me with that changed.
Attachment #607008 - Flags: review+
Attachment #607008 - Flags: feedback?(bzbarsky)
Attachment #607008 - Flags: feedback+
Attached patch Made changes based on Comment 14 (obsolete) — Splinter Review
Attachment #607008 - Attachment is obsolete: true
544 INFO Passed: 430
545 INFO Failed: 0
546 INFO Todo:   0

Possible to write a test for this?
Closed: 12 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
I can write a testcase, but will need guidance on how to do so.  Will it be a mochitest?  How can I simulate entering a search in the url bar?  I tried location.href = "TTDeleteEmbeddedFont", but that gives "/tests/docshell/test/TTDeleteEmbeddedFont was not found"
Attachment #619351 - Attachment is patch: true
You might be able to just modify browser/base/content/test/browser_keywordSearch.js to test this.
Attached file Browser chrome testcase for this bug (obsolete) —
Used browser/base/content/test/browser_keywordSearch.js as template for this testcase.  TC1 fails without the patch, passes with the patch.  TC2-TC4 passes with or without patch, but added for completeness.  

This info line appears in the test output that's not in the output for browser_keywordSearch.js:
TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/browser_bug724841.js | Console message: [JavaScript Error: "The character encoding of the HTML document was not declared. The document will render with garbled text in some browser configurations if the document contains characters from outside the US-ASCII range. The character encoding of the page must to be declared in the document or in the transfer protocol." {file: "" line: 0}]

Not sure why.  Maybe because this test goes all the way through NETWORK/WINDOW STOP state instead of stopping at DOCUMENT START state like browser_keywordSearch.js does.
Attachment #621267 - Flags: feedback?(
You can mostly ignore that message, it just means that your test triggered a load of a page that triggers that warning (which is a little overly aggressive, but I digress).

Was it necessary to observe STOP instead of START for the test to be effective? I was thinking you could have just added an additional test to browser_keywordSearch.js itself...
I did initially try to just add an additional test to browser_keywordSearch.js, but when "TestSearch" is entered in the URL bar, "http://testsearch" is returned first before returning the expected "" (See Comment 1).  

If START is observed, the test will test against "http://testsearch" and fail.  I had to observe STOP to skip the part where it first tries to use "TestSearch" as a host before testing against the desired part where it uses "TestSearch" as a search term.
Comment on attachment 621267 [details]
Browser chrome testcase for this bug

Sorry for the horrible delay here Eric - this is awesome work. A couple of comments about the code:

>diff --git a/browser/base/content/test/browser_bug724841.js b/browser/base/content/test/browser_bug724841.js

>+function test() {

>+  let windowObserver = {
>+    observe: function(aSubject, aTopic, aData) {
>+      if (aTopic == "domwindowopened") {
>+        ok(false, "Alert window opened");
>+        let win = aSubject.QueryInterface(Ci.nsIDOMEventTarget);
>+        win.addEventListener("load", function() {
>+          win.removeEventListener("load", arguments.callee, false);
>+          win.close();
>+        }, false);
>+        executeSoon(finish);
>+      }
>+    }
>+  };
>+  Services.ww.registerNotification(windowObserver);

You should be able to remove this stuff (and the corresponding unregisterNotification) from the test, since the failure cases for this variant of the test aren't alert dialogs.

>+      // Only care when network and window stop
>+      let netWinStop = Ci.nsIWebProgressListener.STATE_IS_NETWORK |
>+                     Ci.nsIWebProgressListener.STATE_IS_WINDOW |
>+                     Ci.nsIWebProgressListener.STATE_STOP;

Did NETWORK | STOP not work? I wonder now whether we can just change browser_keywordSearch to observe STOP instead of START, so that we can add these tests to it. IIRC I chose to use START for browser_keywordSearch instead of STOP only because it occurs first and was sufficient. If we can change that test to use STOP and maintain its test coverage, it would be simpler to just do that.

I realize you may no longer be available to drive this through - if so I can try to do that at some point. Apologies again for the delayed response.
Attachment #621267 - Flags: feedback?( → feedback+
Attached patch Test for bugfix.Splinter Review
I couldn't change browser_keywordSearch to use STOP instead of START.  The existing tests break if I do that.  What I did instead is to make the flags that trigger the test dynamic, so each test can use different flags to trigger the test.  Problem with this though is the ordering of the tests matter.  I had to put all the tests using STOP first, otherwise if they're at the end, the first test that uses STOP will actually be triggered by the previous test's STOP.  I couldn't figure out how to tell a test to wait until the previous test's progress cycle has completed so, for now, I just put all the tests that uses STOP first.

I also changed the way the progress flags are evaluated to determine if the test should start or not.  Let me know if I'm wrong, but I think it was incorrect the way it was before.  Before, the test would start if any of the specified flags were present.  Now, the test will start only if all of the specified flags are present.  With the original tests, it just happened to work anyway.  With these new tests, I had to make sure both NETWORK and STOP are set (the previous code would start the test if either NETWORK or STOP were set).
Attachment #621267 - Attachment is obsolete: true
Attachment #632567 - Flags: feedback?(
Attachment #632567 - Flags: feedback?(
You need to log in before you can comment on or make changes to this bug.