Closed Bug 1336395 Opened 7 years ago Closed 7 years ago

Disallow the same alias with different casing on different search engines

Categories

(Firefox :: Settings UI, defect)

Product:
Firefox
Component:
Settings UI
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: mak, Assigned: tfeserver, Mentored)

References

Details

(Keywords: good-first-bug, Whiteboard: [lang=js])

Attachments

(1 file, 3 obsolete files)

Bug 1336395 - Disallow same alias in search engines
59 bytes, text/x-review-board-request
jaws
: review+
Details 
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)
Flags: needinfo?(jaws)
(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?
Flags: needinfo?(jaws) → needinfo?(leftysolara)
Mentor: jaws
Flags: needinfo?(leftysolara)
Keywords: good-first-bug
Whiteboard: [lang=js]
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?
Attachment #8868513 - Flags: review?(jaws)
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.
Attachment #8868513 - Flags: review?(jaws) → review-
Assignee: nobody → tfeserver
Status: NEW → ASSIGNED
@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?
Flags: needinfo?(jaws)
Damn, mercurial is SOOOOO confusing :( , sorry about the multiple patches
Attachment #8870448 - Attachment is obsolete: true
Attachment #8870448 - Flags: review?(jaws)
Attachment #8870449 - Attachment is obsolete: true
Attachment #8870449 - Flags: review?(jaws)
Attachment #8870450 - Attachment is obsolete: true
Attachment #8870450 - Flags: review?(jaws)
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
Attachment #8868513 - Flags: review?(jaws) → review-
Flags: needinfo?(jaws)
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
Attachment #8868513 - Flags: review?(jaws) → review+
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c2257d5c371e
Disallow same alias in search engines r=jaws
https://hg.mozilla.org/mozilla-central/rev/c2257d5c371e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
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
QA Whiteboard: [bugday-20170802]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: