One click search aliases should be trimmed

RESOLVED FIXED in Firefox 49

Status

()

Firefox
Search
--
minor
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: AdrianSV, Assigned: gasolin)

Tracking

Trunk
Firefox 49
Points:
---

Firefox Tracking Flags

(firefox45 affected, firefox49 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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".
(Assignee)

Comment 1

2 years ago
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
Assignee: nobody → gasolin
Flags: needinfo?(mak77)
(Assignee)

Comment 2

2 years ago
Created attachment 8744806 [details]
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/
(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?
Flags: needinfo?(mak77) → needinfo?(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.
Flags: needinfo?(florian)
(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.
(Assignee)

Comment 6

2 years ago
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/
Attachment #8744806 - Attachment description: MozReview Request: Bug 1228258 - search aliases should be trimmed; f?mak → MozReview Request: Bug 1228258 - search aliases should be trimmed; r?mak
Attachment #8744806 - Flags: review?(mak77)
(Assignee)

Comment 7

2 years ago
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/
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.
(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?
(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 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
Attachment #8744806 - Flags: review?(mak77)
(Assignee)

Comment 12

2 years ago
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/
Attachment #8744806 - Flags: review?(mak77)
(Assignee)

Comment 13

2 years ago
add test cases to directly test the alias setter
(Assignee)

Updated

2 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

2 years ago
Summary: One click search aliases should be left trimmed → One click search aliases should be trimmed
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.
Attachment #8744806 - Flags: review?(mak77)
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.
(Assignee)

Comment 16

2 years ago
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
(Assignee)

Comment 17

2 years ago
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/
Attachment #8744806 - Flags: review?(mak77)
(Assignee)

Comment 18

2 years ago
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/
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 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.
Attachment #8744806 - Flags: review?(mak77) → review+
(Assignee)

Comment 21

2 years ago
(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?
Flags: needinfo?(florian)
addTestEngines returns an array of engines implementing the nsISearchEngine interface, so you can use the alias setter on them.
Flags: needinfo?(florian)
(Assignee)

Comment 23

2 years ago
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/
(Assignee)

Comment 24

2 years ago
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.
Attachment #8744806 - Flags: feedback?(florian)
Comment on attachment 8744806 [details]
MozReview Request: Bug 1228258 - search aliases should be trimmed; r?mak

Looks good to me, thanks!
Attachment #8744806 - Flags: feedback?(florian) → feedback+
(Assignee)

Comment 26

2 years ago
Thanks!
Keywords: checkin-needed

Comment 27

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/ce2501a5f268
Keywords: checkin-needed

Comment 28

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ce2501a5f268
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49

Comment 29

a year ago
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]
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
QA Whiteboard: bugday-20160727]
You need to log in before you can comment on or make changes to this bug.