Currently prefs allow to set the same alias to a different engine if the casing differs. The urlbar will handle all the same (as lowercase). Based on the discussion in bug 1336083, we would like to have a direct correlation between aliases and engines, and we feel like allowing casing this way is just confusing for the user. The check happens here, it should likely use a .toLocaleLowercase(): http://searchfox.org/mozilla-central/rev/d20e4431d0cf40311afa797868bc5c58c54790a2/browser/components/preferences/in-content/search.js#269 I don't know if we have tests, off-hand I couldn't find anything in browser/components/preferences/in-content/tests so it may be a good time to add one!

Jared, would you be interested in mentoring this bug? The fix is trivial, but testing in-content prefs may not be. Or maybe you know of someone who worked on in-content prefs that is still in mozilla and would like to mentor this? It's surprising we don't have any test coverage here, I'd expect at least: * check all the engines are in the list * check it's possible to disable/enable an engine * check it's possible to add/remove an alias (and test trying to insert dupes)

(In reply to Marco Bonardo [::mak] from comment #1) > It's surprising we don't have any test coverage here I also recently stumbled on the lack of test for the search pane, in bug 1327953, and filed bug 1335467 to cleanup that code.

Jalen, would you like to work on this?

Hello, I'm newcomer and for my first contribution I'm interested to fix this bug. I have a local copy of code. Is this issue unassigned?

Can i work on this issue?

ping?

Comment on attachment 8868513 [details] Bug 1336395 - Disallow same alias in search engines https://reviewboard.mozilla.org/r/140136/#review145240 Looks very good! After you make the following changes I think this will be ready to check in, but I'd like to have one more look at it before granting r+. Thanks for the patch! ::: browser/components/preferences/in-content-old/search.js:272 (Diff revision 1) > // Check for duplicates in changes we haven't committed yet > let engines = gEngineView._engineStore.engines; > + let lc_keyword = keyword.toLowerCase(); > for (let engine of engines) { > - if (engine.alias == keyword && > + if (engine.alias && > + engine.alias.toLowerCase() == lc_keyword && toLocaleLowerCase() please. ::: browser/components/preferences/in-content/main.js:1048 (Diff revision 1) > // Check for duplicates in Places keywords. > let bduplicate = !!(await PlacesUtils.keywords.fetch(keyword)); > > // Check for duplicates in changes we haven't committed yet > let engines = gEngineView._engineStore.engines; > + let lc_keyword = keyword.toLowerCase(); Please use keyword.toLocaleLowerCase() instead of .toLowerCase() since .toLocaleLowerCase() will handle various language rules better than .toLowerCase() will. See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/toLocaleLowerCase for more information. ::: browser/components/preferences/in-content/tests/browser_engines.js:1 (Diff revision 1) > +/* Any copyright is dedicated to the Public Domain. > + * http://creativecommons.org/publicdomain/zero/1.0/ */ The license header isn't necessary in test files. Test files are public domain by default. ::: browser/components/preferences/in-content/tests/browser_engines.js:15 (Diff revision 1) > + let tree = doc.querySelector("#engineList"); > + ok(!tree.hidden, "The search engine list should be visible when Search is requested"); > + > + // Check for default search engines to be displayed in the engineList > + let defaultEngines = Services.search.getDefaultEngines(); > + for(let i=0; i<defaultEngines.length; i++) { nit, please use for-of since we don't need to use the index separately. for (let engine of defaultEngines) { ... } ::: browser/components/preferences/in-content/tests/browser_engines.js:17 (Diff revision 1) > + > + // Check for default search engines to be displayed in the engineList > + let defaultEngines = Services.search.getDefaultEngines(); > + for(let i=0; i<defaultEngines.length; i++) { > + let engine = defaultEngines[i]; > + let cellName = tree.view.getCellText(i,tree.columns.getNamedColumn('engineName')); nit, space after comma ::: browser/components/preferences/in-content/tests/browser_engines.js:18 (Diff revision 1) > + // Check for default search engines to be displayed in the engineList > + let defaultEngines = Services.search.getDefaultEngines(); > + for(let i=0; i<defaultEngines.length; i++) { > + let engine = defaultEngines[i]; > + let cellName = tree.view.getCellText(i,tree.columns.getNamedColumn('engineName')); > + is(cellName, engine.name,'Default search engine '+engine.name+' displayed correctly'); nit, space after comma and a space on each side of the + operator. Also, please use double-quote strings here and elsewhere. ::: browser/components/preferences/in-content/tests/browser_engines.js:28 (Diff revision 1) > + tree.view.setCellValue(0,tree.columns.getNamedColumn('engineShown'), false); > + is(tree.view.getCellValue(0,tree.columns.getNamedColumn('engineShown')) , 'false', 'Disable engine'); > + tree.view.setCellValue(0,tree.columns.getNamedColumn('engineShown'), true); > + is(tree.view.getCellValue(0,tree.columns.getNamedColumn('engineShown')) , 'true', 'Enable engine'); These four lines aren't really testing anything specific about the engineList, they are simply testing the functionality of tree's setCellValue/getCellValue. If you can set a row as selected, then you can call synthesizeKey with "space" to toggle if the engine is enabled or disabled. > is(tree.view.getCellValue(0, tree.columns.getNamedColumn("engineShown")), "true", "The first row should be enabled by default"); > tree.view.selection.select(0); // select the first row > EventUtils.synthesizeKey(" ", {}); // Space toggles if the engine is enabled/disabled > is(tree.view.getCellValue(0, tree.columns.getNamedColumn("engineShown")), "false", "The first row should be disabled after pressing Space"); > EventUtils.synthesizeKey(" ", {}); > is(tree.view.getCellValue(0, tree.columns.getNamedColumn("engineShown")), "true", "The first row should be enabled after pressing Space"); ::: browser/components/preferences/in-content/tests/browser_engines.js:37 (Diff revision 1) > +function openPreferencesViaHash(aPane) { > + return new Promise(resolve => { > + gBrowser.selectedTab = gBrowser.addTab("about:preferences" + (aPane ? "#" + aPane : "")); Instead of duplicating the openPreferencesViaHash function, which already exists in browser_bug1020245_openPreferences_to_paneContent.js, we should move the function to head.js so it can be shared.

@tfe, sorry this didn't get reviewed sooner. I may have noticed it in my emails but I've got a huge email backlog at the moment. For your next patch, please add 'r?jaws' to the commit message and I will be notified quicker that there is a patch for me to review. Cheers :)

Thank you for the review. When adding the test: > is(tree.view.getCellValue(0, tree.columns.getNamedColumn("engineShown")), "true", "The first row should be enabled by default"); > tree.view.selection.select(0); // select the first row > EventUtils.synthesizeKey(" ", {}); // Space toggles if the engine is enabled/disabled > is(tree.view.getCellValue(0, tree.columns.getNamedColumn("engineShown")), "false", "The first row should be disabled after pressing Space"); > EventUtils.synthesizeKey(" ", {}); > is(tree.view.getCellValue(0, tree.columns.getNamedColumn("engineShown")), "true", "The first row should be enabled after pressing Space"); I have got a fail when running the tests: > 29 INFO TEST-UNEXPECTED-FAIL | browser/components/preferences/in-content/tests/browser_engines.js | The first row should be > disabled after pressing Space - Got true, expected false > Stack trace: > chrome://mochikit/content/browser-test.js:test_is:928 > chrome://mochitests/content/browser/browser/components/preferences/in-content/tests/browser_engines.js:null:31 > Buffered messages finished

The test looks good to me. I think of maybe synthetizekey is an async function? so the condition is checked before the key is really pressed?

Damn, mercurial is SOOOOO confusing :( , sorry about the multiple patches

Comment on attachment 8868513 [details] Bug 1336395 - Disallow same alias in search engines https://reviewboard.mozilla.org/r/140136/#review145588 Looks good, I'm okay with dropping the synthesizeKey parts from the test for now. After the following fixes this should be ready to land. ::: browser/components/preferences/in-content/tests/browser_engines.js:12 (Diff revision 3) > + for(let i=0; i<defaultEngines.length; i++) { > + let engine = defaultEngines[i]; Did you try using for-of loop here? ::: browser/components/preferences/in-content/tests/browser_engines.js:19 (Diff revision 3) > + tree.view.setCellText(0, tree.columns.getNamedColumn('engineKeyword'), 'keyword'); > + tree.view.setCellText(1, tree.columns.getNamedColumn('engineKeyword'), 'keyword'); The following command will run our linting tool on this test file and point out what changes need to be made before it can be checked in. > mach eslint browser/components/preferences/in-content/tests/browser_engines.js

I am not using the for(xx of yy) because i need the index of the loop to retrieve the correct row, and check the name.

Also thanks for the eslint tip, i didn't knew that was it avaiable with the mac command.

(In reply to tfe from comment #20) > I am not using the for(xx of yy) because i need the index of the loop to > retrieve the correct row, and check the name. Oh sorry, I didn't see that extra reference to `i`.

Comment on attachment 8868513 [details] Bug 1336395 - Disallow same alias in search engines https://reviewboard.mozilla.org/r/140136/#review145990

Pushed by jwein@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c2257d5c371e Disallow same alias in search engines r=jaws

I can reproduce this issue with Nightly 54.0a1 (2017-02-03) (64-bit) in Deepin OS, 64bit This bug is now verified as fixed in Latest Nightly 55.0a1 Build ID 20170531100318 User Agent Mozilla/5.0 (X11; Linux x86_64; rv:55.0) Gecko/20100101 Firefox/55.0 [bugday-20170531]

I can reproduce this issue with Nightly 54.0a1 (2017-02-03) (64-bit) on Windows 7, 64 Bit! This bug's fix is verified with latest Beta! Build ID 20170803103124 User Agent Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0