MD Settings: URL encode text queries before adding them to the URL.

MD Settings: URL encode text queries before adding them to the URL.

Searching is triggered via currentRouteChanged, which was getting a wrong value
from QuerySearchParams(), because the original query was not URL encoded first.

BUG=726684
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2905393002
Cr-Commit-Position: refs/heads/master@{#475221}
Committed: https://chromium.googlesource.com/chromium/src/+/7b3779b51c2acb9f65812723406c28d72fb6e26f

[dpapad, 2017-05-26 22:16:57]

Description was changed from

==========
MD Settings: URL encode text queries before adding them to the URL.

Searching is triggered via currentRouteChanged, which was getting a wrong value
from QuerySearchParams(), because the original query was not URL encoded first.

BUG=726684
==========

to

==========
MD Settings: URL encode text queries before adding them to the URL.

Searching is triggered via currentRouteChanged, which was getting a wrong value
from QuerySearchParams(), because the original query was not URL encoded first.

BUG=726684
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2017-05-27 00:07:22]

The CQ bit was checked by dpapad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-27 00:07:36]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dpapad, 2017-05-27 00:12:09]

The CQ bit was checked by dpapad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-27 00:12:19]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dpapad, 2017-05-27 00:18:09]

dpapad@chromium.org changed reviewers:
+ dschuyler@chromium.org

[dpapad, 2017-05-27 00:18:10]

Before/after screenshots: http://imgur.com/a/Xae4t

https://codereview.chromium.org/2905393002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_main/settings_main.js (right):

https://codereview.chromium.org/2905393002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_main/settings_main.js:273:
this.inSearchMode_ && !result.didFindMatches;
The value of |result.didFindMatches| was wrong, but when it comes to booleans, 2
wrongs make 1 right :>

basic_page.js supplied the wrong value and settings_main.js used "!" where it
should not, cancelling each other's errors.

I noticed this problem while fixing this bug, so I decided to keep it in the
same CL.

https://codereview.chromium.org/2905393002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_ui/settings_ui.js (right):

https://codereview.chromium.org/2905393002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_ui/settings_ui.js:252: new
URLSearchParams('search=' + encodeURIComponent(query)) :
This is the only line necessary for the fix.

https://codereview.chromium.org/2905393002/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/settings_ui_browsertest.js (right):

https://codereview.chromium.org/2905393002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/settings_ui_browsertest.js:140:
assertEquals(value, settings.getQueryParameters().get('search'));
I verified that this test would have caught the problem. Unfortunately though,
tests in this file are disabled above (see DISABLED_All). I'll look into
potentially re-enabling them in follow up CL.

[commit-bot: I haz the power, 2017-05-27 00:20:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-27 00:20:59]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dschuyler, 2017-05-27 01:32:13]

lgtm

https://codereview.chromium.org/2905393002/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/settings_ui_browsertest.js (right):

https://codereview.chromium.org/2905393002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/settings_ui_browsertest.js:139:
searchField.setValue(value);
wdyt of asserting that the internal value of searchField does  not  match the
local |value| var?

[dpapad, 2017-05-27 01:39:34]

https://codereview.chromium.org/2905393002/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/settings_ui_browsertest.js (right):

https://codereview.chromium.org/2905393002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/settings_ui_browsertest.js:139:
searchField.setValue(value);
On 2017/05/27 at 01:32:13, dschuyler wrote:
> wdyt of asserting that the internal value of searchField does  not  match the
local |value| var?

That would not be correct (the internal value DOES match |value|). Its' the URL
that does not match (chrome://md-settings/?search=%2B%2B%2B). This is clearer if
you try the following in the dev console.

new URLSearchParams('search=' + '+++').get('search') // returns '   ' which is
wrong
new URLSearchParams('search=' + encodeURIComponent('+++')).get('search') //
returns '+++' which is right

Let me know if that did not answer your question (I am not sure).

[dschuyler, 2017-05-27 01:44:18]

https://codereview.chromium.org/2905393002/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/settings_ui_browsertest.js (right):

https://codereview.chromium.org/2905393002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/settings_ui_browsertest.js:139:
searchField.setValue(value);
On 2017/05/27 01:39:34, dpapad wrote:
> On 2017/05/27 at 01:32:13, dschuyler wrote:
> > wdyt of asserting that the internal value of searchField does  not  match
the
> local |value| var?
> 
> That would not be correct (the internal value DOES match |value|). Its' the
URL
> that does not match (chrome://md-settings/?search=%2B%2B%2B). This is clearer
if
> you try the following in the dev console.
> 
> new URLSearchParams('search=' + '+++').get('search') // returns '   ' which is
> wrong
> new URLSearchParams('search=' + encodeURIComponent('+++')).get('search') //
> returns '+++' which is right
> 
> Let me know if that did not answer your question (I am not sure).

Acknowledged.

[dpapad, 2017-05-27 02:16:11]

The CQ bit was checked by dpapad@chromium.org

[commit-bot: I haz the power, 2017-05-27 02:16:31]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-27 02:24:05]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1495851371566190,
"parent_rev": "c173ef62237c56354fd42882ea0999cc5564ef1c", "commit_rev":
"7b3779b51c2acb9f65812723406c28d72fb6e26f"}

[commit-bot: I haz the power, 2017-05-27 02:24:16]

Description was changed from

==========
MD Settings: URL encode text queries before adding them to the URL.

Searching is triggered via currentRouteChanged, which was getting a wrong value
from QuerySearchParams(), because the original query was not URL encoded first.

BUG=726684
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: URL encode text queries before adding them to the URL.

Searching is triggered via currentRouteChanged, which was getting a wrong value
from QuerySearchParams(), because the original query was not URL encoded first.

BUG=726684
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2905393002
Cr-Commit-Position: refs/heads/master@{#475221}
Committed:
https://chromium.googlesource.com/chromium/src/+/7b3779b51c2acb9f65812723406c...
==========

[commit-bot: I haz the power, 2017-05-27 02:24:17]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/7b3779b51c2acb9f65812723406c...