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

Status: VERIFIED FIXED

(Firefox :: Search, defect, P3)

Description

[Bogdan Maris, Desktop Test Engineering [OOO until 14th April]]]

Attachment: Screencast showing the issue

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

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

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.

Comment 2

[Marco Bonardo [:mak]]

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.

Comment 3

[akurnya]

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

Comment 4

[Marco Bonardo [:mak]]

(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.

Comment 5

[Marco Bonardo [:mak]]

To clarify, the problem here are engines having CASED aliases.

Comment 6

[akurnya]

(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.

Comment 7

[[e-mail]]

Hi akurnya,

Do you still interest in solving this one? Or I'd like to take it a try

Comment 8

[akurnya]

@Fred 
Yes you may take this up. I'm not able to give time to this at the moment.

Comment 9

[[e-mail]]

Attachment: MozReview Request: Bug 1240727 - match search engine alias with lower case; r=mak

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/

Comment 10

[[e-mail]]

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

Comment 11

[Marco Bonardo [:mak]]

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.

Comment 12

[Marco Bonardo [:mak]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ee9c77b2515

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/642e91672f8a

Comment 14

[[e-mail]]

thanks!

Comment 15

[Marco Bonardo [:mak]]

Bugs are marked as FIXED automatically once they are merged to mozilla-central

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/642e91672f8a

Comment 17

[[e-mail]]

nice to done everything automatically, Thanks!

Comment 18

[Nazir Ahmed Sabbir [:NaSb]]

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

Comment 19

[Rezaul Huque Nayeem]

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!