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)
Firefox
Search
Tracking
()
VERIFIED
FIXED
Firefox 48
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
Reporter | ||
Updated•8 years ago
|
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
Comment 1•8 years ago
|
||
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]
Comment 2•8 years ago
|
||
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
Updated•8 years ago
|
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)
Comment 4•8 years ago
|
||
(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)
Comment 5•8 years ago
|
||
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.
Assignee | ||
Comment 7•8 years ago
|
||
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 | ||
Updated•8 years ago
|
Assignee: nobody → gasolin
Assignee | ||
Comment 9•8 years ago
|
||
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)
Assignee | ||
Comment 10•8 years ago
|
||
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 11•8 years ago
|
||
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+
Comment 12•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ee9c77b2515
Flags: needinfo?(mak77)
Updated•8 years ago
|
Flags: needinfo?(mak77)
Keywords: checkin-needed
Comment 13•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/642e91672f8a
Keywords: checkin-needed
Assignee | ||
Comment 14•8 years ago
|
||
thanks!
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Comment 15•8 years ago
|
||
Bugs are marked as FIXED automatically once they are merged to mozilla-central
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 16•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/642e91672f8a
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Reporter | ||
Updated•8 years ago
|
QA Whiteboard: [good first verify]
Assignee | ||
Comment 17•8 years ago
|
||
nice to done everything automatically, Thanks!
Comment 18•8 years ago
|
||
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]
Comment 19•8 years ago
|
||
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.
Description
•