Closed Bug 1240727 Opened 8 years ago Closed 8 years ago

Capital letters keywords are not recognized when showing which of the search engine will be used in the next search

Categories

(Firefox :: Search, defect, P3)

defect

Tracking

()

VERIFIED FIXED
Firefox 48
Tracking Status
firefox44 --- affected
firefox45 --- affected
firefox46 --- affected
firefox48 --- verified

People

(Reporter: bmaris, Assigned: gasolin, Mentored)

Details

(Whiteboard: [fxsearch][good first bug][lang=js])

Attachments

(2 files)

Affected builds:
- Firefox 44.0 RC
- latest Developer Edition 45.0a2
- latest Nightly 46.0a1

Affected OS's:
- Windows 7 64-bit
- Ubuntu 14.04 32-bit
- Mac OS X 10.11

STR:
1. Start Firefox
2. Visit about:preferences#search
3. Add a keyword for a search engine with capital letter. eg: YA for Yahoo
4. Type in Navigation bar 'YA test'

Expected results: Navigation bar shows that the next search will be made on Yahoo

Actual results: Navigation bar shows that the search will be made on Google

Notes:
- Don't know if I'm CC'ing the right people here bit, there it goes.
- This is not a regression, I can reproduce on the build where unified autocomplete was enabled.

m-c
Last good revision: 8c4f30ce39918652b285b5dc8e6c99598b71159b
First bad revision: ccbf26a27a35ae5da89df47f18541a8a037d81ec

Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8c4f30ce39918652b285b5dc8e6c99598b71159b&tochange=ccbf26a27a35ae5da89df47f18541a8a037d81ec

m-i
Last good revision: 0a9484a091f5c7891be36a7d6e32e1a6f819cf2e
First bad revision: 6968dac17cdfe0a5055378c1a1929bf6c2f2fb0c

Pushlog:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=0a9484a091f5c7891be36a7d6e32e1a6f819cf2e&tochange=6968dac17cdfe0a5055378c1a1929bf6c2f2fb0c

6968dac17cdf	Marco Bonardo — Bug 1168811 - Enable Unified AutoComplete. r=Mossop
Summary: Capital letters keywords are not recognized when showing which of the search engine will be used in the next searach → Capital letters keywords are not recognized when showing which of the search engine will be used in the next search
Marco can you do a quick evaluation of this bug? I'm inclined to track it as a P3, but if this is a bug we've had forever, we could also treat it as a P4.
Flags: needinfo?(mak77)
Whiteboard: [fxsearch]
It's very likely a unified complete regression, the bug is in the comparison in findMatchByAlias

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/PlacesSearchAutocompleteProvider.jsm#244

it should be possible to add a test to toolkit/components/places/tests/unit/test_PlacesSearchAutocompleteProvider.js

I agree it's a P3 cause it's less likely for a user to set an uppercase keyword.
That said, it looks easy enough to be a mentored bug.
Mentor: mak77
Flags: needinfo?(mak77)
Priority: -- → P3
Whiteboard: [fxsearch] → [fxsearch][good first bug][lang=js]
(In reply to Marco Bonardo [::mak] from comment #2)
> It's very likely a unified complete regression, the bug is in the comparison
> in findMatchByAlias
> 
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/
> PlacesSearchAutocompleteProvider.jsm#244
> 
> it should be possible to add a test to
> toolkit/components/places/tests/unit/test_PlacesSearchAutocompleteProvider.js
> 
> I agree it's a P3 cause it's less likely for a user to set an uppercase
> keyword.
> That said, it looks easy enough to be a mentored bug.

Just trying out simple bugs here.
Saw this bug. Reproduced on my system.

Would something like this fix it?

>> return SearchAutocompleteProviderInternal.aliasMatches.find(m => m.alias == searchToken.toLocaleLowerCase());

I also see in toolkit/components/places/tests/unit/test_PlacesSearchAutocompleteProvider.js

51 add_task(function* test_aliased_search_engine_match() {
52   do_check_eq(null, yield PlacesSearchAutocompleteProvider.findMatchByAlias("sober"));
53 
54   let match = yield PlacesSearchAutocompleteProvider.findMatchByAlias("pork");
55   do_check_eq(match.engineName, "bacon");
56   do_check_eq(match.alias, "pork");
57   do_check_eq(match.iconUrl, null);
58 });

Could this be added?

54   let match = yield PlacesSearchAutocompleteProvider.findMatchByAlias("AMZN");
or
54   let match = yield PlacesSearchAutocompleteProvider.findMatchByAlias("Amzn");
Flags: needinfo?(mak77)
(In reply to akurnya from comment #3)
> Would something like this fix it?
> 
> >> return SearchAutocompleteProviderInternal.aliasMatches.find(m => m.alias == searchToken.toLocaleLowerCase());

Actually the searchToken is already lowercase (done by UnifiedComplete.js), it's m.alias that is not and should be lower cased.

> I also see in
> toolkit/components/places/tests/unit/test_PlacesSearchAutocompleteProvider.js

> Could this be added?
> 
> 54   let match = yield
> PlacesSearchAutocompleteProvider.findMatchByAlias("AMZN");
> or
> 54   let match = yield
> PlacesSearchAutocompleteProvider.findMatchByAlias("Amzn");

No, that would not be enough.
Instead you should add another subtest with add_task that adds an engine with an uppercase alias, then findMatchByAlias with it lowercased.
Flags: needinfo?(mak77)
To clarify, the problem here are engines having CASED aliases.
(In reply to Marco Bonardo [::mak] from comment #5)
> To clarify, the problem here are engines having CASED aliases.

So it should be

>> return SearchAutocompleteProviderInternal.aliasMatches.find(m => m.alias.toLocaleLowerCase() == searchToken);

and 
need to add another test case that checks for MixedCases starting with Uppercase.

I'll work on this one.
Hi akurnya,

Do you still interest in solving this one? Or I'd like to take it a try
Flags: needinfo?(akurnya)
@Fred 
Yes you may take this up. I'm not able to give time to this at the moment.
Flags: needinfo?(akurnya)
Assignee: nobody → gasolin
matching both alias and searchToken with lower case, add tests to validate lower, upper, cap cases

Review commit: https://reviewboard.mozilla.org/r/42925/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42925/
Attachment #8735712 - Flags: review?(mak77)
Hi Marco, its my first patch for firefox.

The patch works fine via manual test

```
mach run
```

and unit test

```
mach test toolkit/components/places/tests/unit/test_PlacesSearchAutocompleteProvider.js
```

Please help review it
Status: NEW → ASSIGNED
Comment on attachment 8735712 [details]
MozReview Request: Bug 1240727 - match search engine alias with lower case; r=mak

https://reviewboard.mozilla.org/r/42925/#review40045

Thank you, the patch looks good. I will trigger a Try run, and then we can ask for checkin.
Attachment #8735712 - Flags: review?(mak77) → review+
Flags: needinfo?(mak77)
Keywords: checkin-needed
thanks!
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Bugs are marked as FIXED automatically once they are merged to mozilla-central
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/642e91672f8a
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
QA Whiteboard: [good first verify]
nice to done everything automatically, Thanks!
Successfully reproduce this bug on Nightly 46.0a1 (2016-01-19) (Build ID: 20160119030232) on Linux, 64 Bit by following the comment 0's instruction !

This Bug is now verified as fixed on Latest Firefox Nightly 48.0a1 (2016-04-13)

Build ID: 20160413030239
OS: Linux 3.19.0-58-generic x86-64
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:48.0) Gecko/20100101 Firefox/48.0
QA Whiteboard: [good first verify] → [good first verify][bugday-20160413]
Reproduced the bug in 46.0a1 (2016-01-19) on windows 10 x64

Verified as fixed with latest Firefox Nightly 48.0a1 (Build ID: 20160419030312)

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:48.0) Gecko/20100101 Firefox/48.0

As it is also verified on Linux (Comment 18), Marking it as verified!
Status: RESOLVED → VERIFIED
QA Whiteboard: [good first verify][bugday-20160413] → [good first verify][bugday-20160413][bugday-20160419]
You need to log in before you can comment on or make changes to this bug.