MD Settings: Allow search within settings to track multiple requests separately.

MD Settings: Allow search within settings to track multiple requests separately.

Previously it was always assumed that two calls to SearchManager#search(), one
for the "basic" and one for the "advanced" page, would happen in the same turn
of the JS message loop. For this reason it was safe to use the same queue for
tracking all subtasks, as well as use the same promise to signify the completion
of both calls.

Making the "advanced" page lazy loaded (upcoming change), means that the 2nd
call to search() can't be made in the same turn of the message loop, and
therefore tracking the two calls requires different queues and different
promises.

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

Review-Url: https://codereview.chromium.org/2739323005
Cr-Commit-Position: refs/heads/master@{#456626}
Committed: https://chromium.googlesource.com/chromium/src/+/08266b3fca707804065a2cfd60331722ade41969

[dpapad, 2017-03-11 01:29:55]

Description was changed from

==========
MD Settings: Allow search within settings to track multiple requests separately.

Previously it was always assumed that two calls to SearchManager#search(), one
for the "basic" and one for the "advanced" page, would happen in the same turn
of the JS message loop. For this reason it was safe to use the same queue for
tracking all subtasks, as well as use the same promise to signify the completion
of both calls.

Making the "advanced" page lazy loaded (upcoming change), means that the 2nd
call to search() can't be made in the same turn of the message loop, and
therefore tracking the two calls requires different queues and different
promises.

BUG=597347
==========

to

==========
MD Settings: Allow search within settings to track multiple requests separately.

Previously it was always assumed that two calls to SearchManager#search(), one
for the "basic" and one for the "advanced" page, would happen in the same turn
of the JS message loop. For this reason it was safe to use the same queue for
tracking all subtasks, as well as use the same promise to signify the completion
of both calls.

Making the "advanced" page lazy loaded (upcoming change), means that the 2nd
call to search() can't be made in the same turn of the message loop, and
therefore tracking the two calls requires different queues and different
promises.

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

[dpapad, 2017-03-11 02:23:52]

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

[commit-bot: I haz the power, 2017-03-11 02:24:08]

Dry run: 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-03-11 02:34:36]

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

[commit-bot: I haz the power, 2017-03-11 02:34:37]

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

[dpapad, 2017-03-11 02:35:17]

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

[commit-bot: I haz the power, 2017-03-11 02:35:32]

Dry run: 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-03-11 02:44:45]

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

[commit-bot: I haz the power, 2017-03-11 02:44:46]

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

[dpapad, 2017-03-13 17:33:35]

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

[commit-bot: I haz the power, 2017-03-13 17:34:18]

Dry run: 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-03-13 19:10:24]

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

[commit-bot: I haz the power, 2017-03-13 19:10:25]

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

[dpapad, 2017-03-13 20:47:07]

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

[commit-bot: I haz the power, 2017-03-13 20:47:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dpapad, 2017-03-13 21:01:48]

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

[commit-bot: I haz the power, 2017-03-13 22:25:39]

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

[commit-bot: I haz the power, 2017-03-13 22:25:40]

Dry run: This issue passed the CQ dry run.

[Dan Beam, 2017-03-14 00:44:28]

https://codereview.chromium.org/2739323005/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/basic_page/basic_page.js (right):

https://codereview.chromium.org/2739323005/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/basic_page/basic_page.js:113:
settings.getSearchManager().search(
wrong indent, when you add less indent dare you to unwrap as well ;)

https://codereview.chromium.org/2739323005/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/search_settings.js (right):

https://codereview.chromium.org/2739323005/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/search_settings.js:306:
this.request.queue_.addSearchAndHighlightTask(
should this be this.request_?
                           ^

https://codereview.chromium.org/2739323005/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/search_settings.js:483: this.root = root;
if this is private, root_
                        ^

https://codereview.chromium.org/2739323005/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_main/settings_main.js (right):

https://codereview.chromium.org/2739323005/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_main/settings_main.js:259: if
(requests.some(function(r) { return r.canceled; })) {
can we hide these details in the request class?

https://codereview.chromium.org/2739323005/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_main/settings_main.js:267:
this.inSearchMode_ = !requests[0].isSame('');
should this also be .every()?

https://codereview.chromium.org/2739323005/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_main/settings_main.js:270:
requests.every(function(r) { return !r.didFindMatches(); });
can we hide these details in the request class?

https://codereview.chromium.org/2739323005/diff/100001/chrome/test/data/webui...
File chrome/test/data/webui/settings/cr_settings_browsertest.js (right):

https://codereview.chromium.org/2739323005/diff/100001/chrome/test/data/webui...
chrome/test/data/webui/settings/cr_settings_browsertest.js:1207:
runAccessibilityChecks: false,
i don't think we want this.  i think we want to change a specific test or just
not make a11y tests errors

https://codereview.chromium.org/2739323005/diff/100001/chrome/test/data/webui...
File chrome/test/data/webui/settings/search_settings_test.js (right):

https://codereview.chromium.org/2739323005/diff/100001/chrome/test/data/webui...
chrome/test/data/webui/settings/search_settings_test.js:105:
console.log('ssssstart test');
rrrrremove

[dpapad, 2017-03-14 03:28:11]

https://codereview.chromium.org/2739323005/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/basic_page/basic_page.js (right):

https://codereview.chromium.org/2739323005/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/basic_page/basic_page.js:113:
settings.getSearchManager().search(
On 2017/03/14 at 00:44:27, Dan Beam wrote:
> wrong indent, when you add less indent dare you to unwrap as well ;)

Done.

https://codereview.chromium.org/2739323005/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/search_settings.js (right):

https://codereview.chromium.org/2739323005/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/search_settings.js:306:
this.request.queue_.addSearchAndHighlightTask(
On 2017/03/14 at 00:44:27, Dan Beam wrote:
> should this be this.request_?
>                            ^

This is a protected member variable from the superclass Task, therefore should
not have '_' suffix.

https://codereview.chromium.org/2739323005/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/search_settings.js:483: this.root = root;
On 2017/03/14 at 00:44:27, Dan Beam wrote:
> if this is private, root_
>                         ^

Done.

https://codereview.chromium.org/2739323005/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_main/settings_main.js (right):

https://codereview.chromium.org/2739323005/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_main/settings_main.js:259: if
(requests.some(function(r) { return r.canceled; })) {
On 2017/03/14 at 00:44:28, Dan Beam wrote:
> can we hide these details in the request class?

Done. Wrapped those in a temp object inside basic_page#searchContents.

https://codereview.chromium.org/2739323005/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_main/settings_main.js:267:
this.inSearchMode_ = !requests[0].isSame('');
On 2017/03/14 at 00:44:27, Dan Beam wrote:
> should this also be .every()?

It does not need to, because all the requests correspond to the same text query.
Added a comment to explain that.

https://codereview.chromium.org/2739323005/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_main/settings_main.js:270:
requests.every(function(r) { return !r.didFindMatches(); });
On 2017/03/14 at 00:44:28, Dan Beam wrote:
> can we hide these details in the request class?

Done.

https://codereview.chromium.org/2739323005/diff/100001/chrome/test/data/webui...
File chrome/test/data/webui/settings/cr_settings_browsertest.js (right):

https://codereview.chromium.org/2739323005/diff/100001/chrome/test/data/webui...
chrome/test/data/webui/settings/cr_settings_browsertest.js:1207:
runAccessibilityChecks: false,
On 2017/03/14 at 00:44:28, Dan Beam wrote:
> i don't think we want this.  i think we want to change a specific test or just
not make a11y tests errors

Reverted.
The way we test accessibility is weird. We test whatever is left in the DOM
after our actual test finishes. Most importantly it causes confusion when a test
fails, because when it fails different things are left in the DOM, and then
additional accessibility errors are showing, which are unrelated to the original
failure.

My thinking is that we should disable accessibility checks for all our unit
tests, and add a single test that intentionally force renders everything in the
DOM and just does a11y checks.

https://codereview.chromium.org/2739323005/diff/100001/chrome/test/data/webui...
File chrome/test/data/webui/settings/search_settings_test.js (right):

https://codereview.chromium.org/2739323005/diff/100001/chrome/test/data/webui...
chrome/test/data/webui/settings/search_settings_test.js:105:
console.log('ssssstart test');
On 2017/03/14 at 00:44:28, Dan Beam wrote:
> rrrrremove

Done.

[dpapad, 2017-03-14 03:28:33]

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

[commit-bot: I haz the power, 2017-03-14 03:28:44]

Dry run: 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-03-14 04:34:46]

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

[commit-bot: I haz the power, 2017-03-14 04:34:46]

Dry run: This issue passed the CQ dry run.

[Dan Beam, 2017-03-14 04:56:32]

The CQ bit was checked by dbeam@chromium.org

[Dan Beam, 2017-03-14 04:56:33]

lgtm

[commit-bot: I haz the power, 2017-03-14 04:56:48]

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-03-14 05:01:02]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1489467392507470,
"parent_rev": "b72f9fec8e0106eaa67770ae7972b8c9b3a9ec46", "commit_rev":
"08266b3fca707804065a2cfd60331722ade41969"}

[commit-bot: I haz the power, 2017-03-14 05:13:52]

Description was changed from

==========
MD Settings: Allow search within settings to track multiple requests separately.

Previously it was always assumed that two calls to SearchManager#search(), one
for the "basic" and one for the "advanced" page, would happen in the same turn
of the JS message loop. For this reason it was safe to use the same queue for
tracking all subtasks, as well as use the same promise to signify the completion
of both calls.

Making the "advanced" page lazy loaded (upcoming change), means that the 2nd
call to search() can't be made in the same turn of the message loop, and
therefore tracking the two calls requires different queues and different
promises.

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

to

==========
MD Settings: Allow search within settings to track multiple requests separately.

Previously it was always assumed that two calls to SearchManager#search(), one
for the "basic" and one for the "advanced" page, would happen in the same turn
of the JS message loop. For this reason it was safe to use the same queue for
tracking all subtasks, as well as use the same promise to signify the completion
of both calls.

Making the "advanced" page lazy loaded (upcoming change), means that the 2nd
call to search() can't be made in the same turn of the message loop, and
therefore tracking the two calls requires different queues and different
promises.

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

Review-Url: https://codereview.chromium.org/2739323005
Cr-Commit-Position: refs/heads/master@{#456626}
Committed:
https://chromium.googlesource.com/chromium/src/+/08266b3fca707804065a2cfd6033...
==========

[commit-bot: I haz the power, 2017-03-14 05:13:54]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/08266b3fca707804065a2cfd6033...