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

RESOLVED FIXED in mozilla15

Status

()

Core
Document Navigation
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: jimm, Assigned: ekw)

Tracking

Trunk
mozilla15
x86_64
Windows 7
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 6 obsolete attachments)

(Reporter)

Description

5 years ago
STR:

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" - 

"http://www.google.com/search?q=ttdeleteembeddedfont&ie=utf-8&oe=utf-8&aq=t&rls=org.mozilla:en-US:official&client=firefox-a"

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
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 1

5 years ago
Created attachment 602816 [details] [diff] [review]
Retain case when sending location bar text to search engine service.

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.

Updated

5 years ago
Attachment #602816 - Flags: review?(bzbarsky)
(Assignee)

Comment 2

5 years ago
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 :)
(Assignee)

Comment 5

5 years ago
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-
(Assignee)

Comment 7

5 years ago
Created attachment 604230 [details] [diff] [review]
Keep changes within nsDocShell

Is this too simple?  Am I missing anything?
Attachment #602816 - Attachment is obsolete: true
Attachment #604230 - Flags: feedback?(gavin.sharp)
(Assignee)

Comment 8

5 years ago
Created attachment 604232 [details]
mochitest results for docshell/test


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?(gavin.sharp) → feedback+
Component: Location Bar → Document Navigation
Product: Firefox → Core
QA Contact: location.bar → docshell
(Assignee)

Updated

5 years ago
Attachment #604230 - Flags: review?(bzbarsky)

Updated

5 years ago
Whiteboard: [autoland-try:-p linux,linux64,macosx64,win32,macosx -u mochitest -t none]

Updated

5 years ago
Whiteboard: [autoland-try:-p linux,linux64,macosx64,win32,macosx -u mochitest -t none] → [autoland-in-queue]

Comment 10

5 years ago
Autoland Patchset:
	Patches: 604230
	Branch: mozilla-central => try
Insufficient permissions to push to try.

Updated

5 years ago
Whiteboard: [autoland-in-queue]

Comment 11

5 years ago
Try run for d2f40a675b89 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=d2f40a675b89
Results (out of 93 total builds):
    success: 84
    warnings: 9
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/josh@joshmatthews.net-d2f40a675b89
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-
(Assignee)

Comment 13

5 years ago
Created attachment 607008 [details]
Made changes based on Comment 12

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);

  mOriginalUriString.Truncate();

Looks fine to me with that changed.
Attachment #607008 - Flags: review+
Attachment #607008 - Flags: feedback?(bzbarsky)
Attachment #607008 - Flags: feedback+
(Assignee)

Comment 15

5 years ago
Created attachment 614349 [details] [diff] [review]
Made changes based on Comment 14
Attachment #607008 - Attachment is obsolete: true
(Assignee)

Comment 16

5 years ago
Created attachment 614350 [details]
mochitest-plain test results for docshell

544 INFO Passed: 430
545 INFO Failed: 0
546 INFO Todo:   0

Comment 17

5 years ago
Try server run: https://tbpl.mozilla.org/?tree=Try&rev=c6ef72f5f196
(Assignee)

Comment 18

5 years ago
Created attachment 619351 [details] [diff] [review]
Same as last patch, but redone to apply cleanly to current mozilla-central tip
Attachment #614349 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ec73828b931
http://hg.mozilla.org/mozilla-central/rev/7ec73828b931

Possible to write a test for this?
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
(Assignee)

Comment 21

5 years ago
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"

Updated

5 years ago
Attachment #619351 - Attachment is patch: true
You might be able to just modify browser/base/content/test/browser_keywordSearch.js to test this.
(Assignee)

Comment 23

5 years ago
Created attachment 621267 [details]
Browser chrome testcase for this bug

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: "http://example.com/?q=TestSearch" 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?(gavin.sharp)
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...
(Assignee)

Comment 25

5 years ago
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 "http://example.com/?q=TestSearch" (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?(gavin.sharp) → feedback+
(Assignee)

Comment 27

5 years ago
Created attachment 632567 [details] [diff] [review]
Test for bugfix.

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?(gavin.sharp)
Attachment #632567 - Flags: feedback?(gavin.sharp)
You need to log in before you can comment on or make changes to this bug.