1228258 - One click search aliases should be trimmed

Status: RESOLVED FIXED

(Firefox :: Search, defect)

Description

[Adrian Florinescu [:aflorinescu]]

Steps:
1. Click on the magnifying glass from the Search toolbox and choose "Change Search Settings".
2. Duble click under the Keyword column and set an alias adding several spaces before: eg. "    test_alias".

Actual Result:
The alias is saved as "    test_alias".

Expected Result:
The alias is expected to be left trimmed and saved as "test_alias".

Comment 1

[[e-mail]]

Would like to match and modify keyword with aNewKeyword.trim() in
https://dxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/search.js#244

Any suggestion to add the test case in right place?

ex: 
toolkit/components/places/tests/unit/test_async_transactions.js 
or 
toolkit/components/places/tests/unit/test_PlacesSearchAutocompleteProvider.js

Comment 2

[[e-mail]]

Attachment: MozReview Request: Bug 1228258 - search aliases should be trimmed; r?mak

Review commit: https://reviewboard.mozilla.org/r/48683/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48683/

Comment 3

[Marco Bonardo [:mak]]

(In reply to Fred Lin [:gasolin] from comment #1)
> Any suggestion to add the test case in right place?
> 
> ex: 
> toolkit/components/places/tests/unit/test_async_transactions.js 
> or 
> toolkit/components/places/tests/unit/test_PlacesSearchAutocompleteProvider.js

I don't think the test should live in Places, it should rather be in http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/tests/ if we want a test for this preference panel.

I think we should do this in prefs code, but I wonder if we should also do this in the alias setter of toolkit/components/search/nsSearchService.js, since I don't see a benefit in allowing an untrimmed alias. And then we may just want to modify a test in http://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/tests/xpcshell/
Florian, what do you think?

Comment 4

[Florian Quèze [:florian]]

(In reply to Marco Bonardo [::mak] from comment #3)

> I think we should do this in prefs code, but I wonder if we should also do
> this in the alias setter of toolkit/components/search/nsSearchService.js,
> since I don't see a benefit in allowing an untrimmed alias.

I don't care too strongly, but I think trimming in the alias setter in the search service is slightly better. And if the setter does it already, I'm not sure doing it the preferences code too is still useful.

> And then we may
> just want to modify a test in
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/
> tests/xpcshell/
> Florian, what do you think?

I don't see any existing test covering the alias attribute, so we may need to create a new one.

Comment 5

[Marco Bonardo [:mak]]

(In reply to Florian Quèze [:florian] [:flo] from comment #4)
> I don't care too strongly, but I think trimming in the alias setter in the
> search service is slightly better. And if the setter does it already, I'm
> not sure doing it the preferences code too is still useful.

it actually is needed to trim in prefs cause before setting the alias we compare it with bookmark keywords (to avoid confusion), it's better to compare apples with apples :)
When we will merge bookmark and search keywords, then we could stop doing that.

Comment 6

[[e-mail]]

Comment on attachment 8744806 [details]
MozReview Request: Bug 1228258 - search aliases should be trimmed; r?mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48683/diff/1-2/

Comment 7

[[e-mail]]

Comment on attachment 8744806 [details]
MozReview Request: Bug 1228258 - search aliases should be trimmed; r?mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48683/diff/2-3/

Comment 8

[Florian Quèze [:florian]]

https://reviewboard.mozilla.org/r/48683/#review46039

::: toolkit/components/search/nsSearchService.js:2139
(Diff revision 3)
>    // nsISearchEngine
>    get alias() {
>      return this.getAttr("alias");
>    },
>    set alias(val) {
> -    this.setAttr("alias", val);
> +    this.setAttr("alias", val.trim()); // search aliases should be trimmed

This comment doesn't add any information that isn't immediately visible when looking at the code. (This also applies to a lesser extent to the same comment in search.js.)

::: toolkit/components/search/tests/xpcshell/test_save_sorted_engines.js:69
(Diff revision 3)
>    metadata = yield promiseEngineMetadata();
>    do_check_eq(metadata["foo"].alias, "foo");
>    do_check_true(metadata["foo"].order > 0);
> +
> +  // Test a new engine alias with right space
> +  search.addEngineWithDetails("foo2", "", "   e", "", "GET",

addEngineWithDetails calling the alias setter is an implementation detail.

The test I would really like to see is something calling directly the alias setter (ie. similar to what happens when a user sets an alias from the search preferences), and then checking the saved value using the getter.

Comment 9

[Florian Quèze [:florian]]

(In reply to Marco Bonardo [::mak] from comment #5)

> it actually is needed to trim in prefs cause before setting the alias we
> compare it with bookmark keywords (to avoid confusion), it's better to
> compare apples with apples :)

Good point! Do we also somehow trim bookmark keywords in places?

Comment 10

[Marco Bonardo [:mak]]

(In reply to Florian Quèze [:florian] [:flo] from comment #9)
> Good point! Do we also somehow trim bookmark keywords in places?

yes, we do.

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 8744806 [details]
MozReview Request: Bug 1228258 - search aliases should be trimmed; r?mak

https://reviewboard.mozilla.org/r/48683/#review46067

clearing request per comment 8

Comment 12

[[e-mail]]

Comment on attachment 8744806 [details]
MozReview Request: Bug 1228258 - search aliases should be trimmed; r?mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48683/diff/3-4/

Comment 13

[[e-mail]]

add test cases to directly test the alias setter

Comment 14

[Marco Bonardo [:mak]]

Comment on attachment 8744806 [details]
MozReview Request: Bug 1228258 - search aliases should be trimmed; r?mak

https://reviewboard.mozilla.org/r/48683/#review46433

::: toolkit/components/search/nsSearchService.js:2139
(Diff revision 4)
>    // nsISearchEngine
>    get alias() {
>      return this.getAttr("alias");
>    },
>    set alias(val) {
> -    this.setAttr("alias", val);
> +    this.setAttr("alias", val.trim());

you have xpcshell test failures due to this, cause val may be null and val.trim() is not valid.

::: toolkit/components/search/tests/xpcshell/test_engine_set_alias.js:2
(Diff revision 4)
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */

This is not needed, tests default to PD license, thus it can be omitted

::: toolkit/components/search/tests/xpcshell/test_engine_set_alias.js:42
(Diff revision 4)
> +    let engine = new Services.search.Engine("test4", false);
> +    engine.alias = "   g  ";
> +    Assert.equal(engine.alias, "g");
> +    removeEngine(engine);
> +  });
> +});

what about a test passing only whitespaces?
It should likely unset an alias... Likely the same behavior as invoking the alias setter with null.

Comment 15

[Florian Quèze [:florian]]

https://reviewboard.mozilla.org/r/48683/#review46441

::: toolkit/components/search/tests/xpcshell/test_engine_set_alias.js:5
(Diff revision 4)
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +add_task(function* test_engine_set_alias() {
> +  asyncInit().then(function() {

Given that you are in a task, you can just do:
  yield asyncInit();

::: toolkit/components/search/tests/xpcshell/test_engine_set_alias.js:7
(Diff revision 4)
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +add_task(function* test_engine_set_alias() {
> +  asyncInit().then(function() {
> +    do_print("Set engine alias");
> +    let engine = new Services.search.Engine("test", false);

Services.search.Engine doesn't seem intentionally exported. I'm surprised this works.

::: toolkit/components/search/tests/xpcshell/test_engine_set_alias.js:15
(Diff revision 4)
> +    removeEngine(engine);
> +  });
> +});
> +
> +add_task(function* test_engine_set_alias_with_left_space() {
> +  asyncInit().then(function() {

You only need to initialize the search service once in your whole test file.

Comment 16

[[e-mail]]

https://reviewboard.mozilla.org/r/48683/#review46441

> Services.search.Engine doesn't seem intentionally exported. I'm surprised this works.

Will use addTestEngines test helper instead

Comment 17

[[e-mail]]

Comment on attachment 8744806 [details]
MozReview Request: Bug 1228258 - search aliases should be trimmed; r?mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48683/diff/4-5/

Comment 18

[[e-mail]]

Comment on attachment 8744806 [details]
MozReview Request: Bug 1228258 - search aliases should be trimmed; r?mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48683/diff/5-6/

Comment 19

[Florian Quèze [:florian]]

I would prefer if at least some of the test used the alias setter explicitly (engine.alias = ...) without relying on the abstraction provided by addTestEngines.

Comment 20

[Marco Bonardo [:mak]]

Comment on attachment 8744806 [details]
MozReview Request: Bug 1228258 - search aliases should be trimmed; r?mak

https://reviewboard.mozilla.org/r/48683/#review47239

I don't have further comments, please fix Florian's comment about having some alias setter calls, and we should be fine.

Comment 21

[[e-mail]]

(In reply to Florian Quèze [:florian] [:flo] from comment #19)
> I would prefer if at least some of the test used the alias setter explicitly
> (engine.alias = ...) without relying on the abstraction provided by
> addTestEngines.

Thanks for point it out. After change asyncInit to yield statement, I found `engine` does not exposed via `Services.search`, therefore I took a step back to test with addTestEngines.

If still want to test engine directly, either we can expose `engine` to `Services.search` or test via nsISearchEngine?

Comment 22

[Florian Quèze [:florian]]

addTestEngines returns an array of engines implementing the nsISearchEngine interface, so you can use the alias setter on them.

Comment 23

[[e-mail]]

Comment on attachment 8744806 [details]
MozReview Request: Bug 1228258 - search aliases should be trimmed; r?mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48683/diff/6-7/

Comment 24

[[e-mail]]

Comment on attachment 8744806 [details]
MozReview Request: Bug 1228258 - search aliases should be trimmed; r?mak

Florian, thanks for noting that! 
I've append the direct engine.alias test along with existing addEngines tests. Please help confirm if it works as you expected.

Comment 25

[Florian Quèze [:florian]]

Comment on attachment 8744806 [details]
MozReview Request: Bug 1228258 - search aliases should be trimmed; r?mak

Looks good to me, thanks!

Comment 26

[[e-mail]]

Thanks!

Comment 27

[Pulsebot]

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

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ce2501a5f268

Comment 29

[Tanvir Rahman]

I have reproduced this bug with nightly 45.0a1 (2015-11-26) on Windows 10, 64 bit!

The Bug's fix is verified on Latest Aurora 49.0a2.

Build ID 	20160708004052
User Agent 	Mozilla/5.0 (Windows NT 10.0; WOW64; rv:49.0) Gecko/20100101 Firefox/49.0

[testday-20160708]

Comment 30

[Rezaul Huque Nayeem]

Reproduced this bug in firefox nightly 45.0a1 (2015-11-26) with ubuntu 16.04 (64 bit)

Verified as this bug fixed with latest firefox aurora 49.0a2 (Build ID: 20160726004006)
Mozilla/5.0 (X11; Linux x86_64; rv:49.0) Gecko/20100101 Firefox/49.0