MD Settings: Implement search URLs.

MD Settings: Implement search URLs.

- Modify settings-ui to sync the URL 'search' parameter and the search
  textfield text to each other, and to trigger searching when the current
  route changes to a "search" URL.
- Provide a way to programmatically change cr-search-field's value without
  triggering a 'search-changed' event (necessary for syncing URL with search
  field).

This makes searching and navigation compatible.

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

Committed: https://crrev.com/63797594bc2bb93e645129b0c679081dc925ba5f
Cr-Commit-Position: refs/heads/master@{#429465}

[dpapad, 2016-10-24 23:44:30]

Description was changed from

==========
MD Settings: Implement search URLs, WIP.

This addresses a lot of search + navigation incompatibilities.

BUG=641663
==========

to

==========
MD Settings: Implement search URLs, WIP.

This addresses a lot of search + navigation incompatibilities.

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

[dpapad, 2016-10-25 21:17:11]

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

[commit-bot: I haz the power, 2016-10-25 21:17:40]

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

[dpapad, 2016-10-25 21:20:08]

Description was changed from

==========
MD Settings: Implement search URLs, WIP.

This addresses a lot of search + navigation incompatibilities.

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

to

==========
MD Settings: Implement search URLs.

- Modify settings-main to trigger search as a result of URL changes.
- Modify settings-ui to sync the URL 'search' parameter and the search
  textfield text to each other.

This addresses a lot of search + navigation incompatibilities.

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

[dpapad, 2016-10-25 21:48:05]

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

[commit-bot: I haz the power, 2016-10-25 21:48:25]

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

[dpapad, 2016-10-25 22:28:00]

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

[dpapad, 2016-10-25 22:28:01]

Screencast at https://bugs.chromium.org/p/chromium/issues/detail?id=641663#c10.
Green tests on patch#5.

[dpapad, 2016-10-25 22:33:03]

Description was changed from

==========
MD Settings: Implement search URLs.

- Modify settings-main to trigger search as a result of URL changes.
- Modify settings-ui to sync the URL 'search' parameter and the search
  textfield text to each other.

This addresses a lot of search + navigation incompatibilities.

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

to

==========
MD Settings: Implement search URLs.

- Modify settings-main to trigger search as a result of URL changes.
- Modify settings-ui to sync the URL 'search' parameter and the search
  textfield text to each other.

This makes searching and navigation compatible.

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

[commit-bot: I haz the power, 2016-10-25 23:00:55]

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

[commit-bot: I haz the power, 2016-10-25 23:00:56]

Dry run: This issue passed the CQ dry run.

[Dan Beam, 2016-10-26 01:24:35]

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

[Dan Beam, 2016-10-26 01:24:36]

i think tommycli@ is better cuz routing

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

https://codereview.chromium.org/2449663002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_ui/settings_ui.js:69:
'search-changed', this.onSearchChanged_.bind(this));
any reason why we can't do on-search-changed="onSearchChanged_" in the HTML?

[dpapad, 2016-10-26 01:31:27]

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

https://codereview.chromium.org/2449663002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_ui/settings_ui.js:69:
'search-changed', this.onSearchChanged_.bind(this));
On 2016/10/26 at 01:24:36, Dan Beam wrote:
> any reason why we can't do on-search-changed="onSearchChanged_" in the HTML?

Yes, I'll add a comment to make this more obvious. cr-toolbar-search-field
unfortunately (probably a bug we should fix), fires an accidental search-changed
event during its initialization, which is hard to ignore if we add the listener
in HTML. Adding the listener in 'ready' works around this issue.

[Dan Beam, 2016-10-26 20:50:52]

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

https://codereview.chromium.org/2449663002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_ui/settings_ui.js:69:
'search-changed', this.onSearchChanged_.bind(this));
On 2016/10/26 01:31:27, dpapad wrote:
> On 2016/10/26 at 01:24:36, Dan Beam wrote:
> > any reason why we can't do on-search-changed="onSearchChanged_" in the HTML?
> 
> Yes, I'll add a comment to make this more obvious. cr-toolbar-search-field
> unfortunately (probably a bug we should fix), fires an accidental
search-changed
> event during its initialization, which is hard to ignore if we add the
listener
> in HTML. Adding the listener in 'ready' works around this issue. 

that's a pretty lame work around.  why does triggering with the same value
matter if query == existingQuery no-ops?

[dpapad, 2016-10-26 21:02:57]

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

https://codereview.chromium.org/2449663002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_ui/settings_ui.js:69:
'search-changed', this.onSearchChanged_.bind(this));

> that's a pretty lame work around.  why does triggering with the same value
matter if query == existingQuery no-ops?

Well, you are right that it does not matter anymore, so done. Before it would
trigger an unnecessary search on startup.

[dpapad, 2016-10-27 17:39:58]

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

https://codereview.chromium.org/2449663002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_main/settings_main.js:201: if
(!newRoute.isSubpage() &&
As I was explaining this logic to Tommy, he found a small bug case. I'll be
fixing it and re-ping the CL. Thanks!

[tommycli, 2016-10-27 21:07:17]

https://codereview.chromium.org/2449663002/diff/120001/chrome/test/data/webui...
File chrome/test/data/webui/settings/settings_main_test.js (right):

https://codereview.chromium.org/2449663002/diff/120001/chrome/test/data/webui...
chrome/test/data/webui/settings/settings_main_test.js:8: * @extends
{settings.TestBrowserProxy}
If I understand correctly? : The TestSearchManager isn't actually mocking a
browser proxy... it's just leveraging the TestBrowserProxy whenCalled machinery?
If so, it would be worth adding a comment explaining what's going on.

One more thing -- I'm not sure you actually need the whole methodCalled Promise
business, since I think everything is synchronous (correct me if there's any
async bits). Maybe you only need to set a lastQuery variable.

[dpapad, 2016-10-28 01:08:06]

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

[commit-bot: I haz the power, 2016-10-28 01:08:32]

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

[dpapad, 2016-10-28 01:09:12]

Patchset #8 (id:140001) has been deleted

[commit-bot: I haz the power, 2016-10-28 01:11:26]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[commit-bot: I haz the power, 2016-10-28 01:14:36]

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

[commit-bot: I haz the power, 2016-10-28 01:14:37]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dpapad, 2016-10-28 01:27:31]

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

[commit-bot: I haz the power, 2016-10-28 01:27:51]

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, 2016-10-28 01:30:40]

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

[commit-bot: I haz the power, 2016-10-28 01:30:40]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dpapad, 2016-10-28 02:18:37]

Patchset #8 (id:160001) has been deleted

[dpapad, 2016-10-28 02:19:12]

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

[commit-bot: I haz the power, 2016-10-28 02:19: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, 2016-10-28 03:22:11]

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

[commit-bot: I haz the power, 2016-10-28 03:22:12]

Dry run: This issue passed the CQ dry run.

[dpapad, 2016-10-28 17:45:02]

Patchset #9 (id:200001) has been deleted

[dpapad, 2016-10-28 18:15:31]

Patchset #9 (id:220001) has been deleted

[dpapad, 2016-10-28 18:17:51]

Description was changed from

==========
MD Settings: Implement search URLs.

- Modify settings-main to trigger search as a result of URL changes.
- Modify settings-ui to sync the URL 'search' parameter and the search
  textfield text to each other.

This makes searching and navigation compatible.

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

to

==========
MD Settings: Implement search URLs.

- Modify settings-ui to sync the URL 'search' parameter and the search
  textfield text to each other.
- Modify settings-main to used data-binding for triggering searches.
- Provide a way to programmatically change cr-search-field's value without
  triggering a 'search-changed' event (necessary for syncing URL with search
  field.

This makes searching and navigation compatible.

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

[dpapad, 2016-10-28 18:18:01]

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

[commit-bot: I haz the power, 2016-10-28 18:18:21]

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, 2016-10-28 19:20:40]

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

[commit-bot: I haz the power, 2016-10-28 19:20:41]

Dry run: This issue passed the CQ dry run.

[dpapad, 2016-10-28 20:23:39]

I have reworked this CL since previous comments significantly. I think the
latest version is much cleaner (as well as addresses a case not addressed
previously). Specifically

 - settings-main has trivial logic, simply triggering search based on a
data-binding.
 - settings-ui holds all search URL related logic.

Summarizing our in-person conversation (with @tommycli and @dbeam), there is
still a small case where it is not handled correctly, which I plan to address
later (lower priority though). For the record (I will also file a bug with
this), that case is the following
1) Navigate to a subpage, like chrome://md-settings/searchEngines
2) Trigger a search via the textbox (for example "cookies").
3) Click the "back" browser button (nothing happens).

The bug is happening because in the code there is currently no way to
differentiate between the following two distinct cases.
a) Navigating from a highlighted top-level page to a highlighted subpageb
(highlights should be preserved).
b) Navigating from a highlighted top-level page back to a previously shown
(highlighted or not) subpage.

The code currently treats both a and b as a (which is a much more frequent
case).

https://codereview.chromium.org/2449663002/diff/120001/chrome/test/data/webui...
File chrome/test/data/webui/settings/settings_main_test.js (right):

https://codereview.chromium.org/2449663002/diff/120001/chrome/test/data/webui...
chrome/test/data/webui/settings/settings_main_test.js:8: * @extends
{settings.TestBrowserProxy}
On 2016/10/27 at 21:07:17, tommycli wrote:
> If I understand correctly? : The TestSearchManager isn't actually mocking a
browser proxy... it's just leveraging the TestBrowserProxy whenCalled machinery?
If so, it would be worth adding a comment explaining what's going on.

Added comment. Even though TestBrowserProxy has been traditionally used with
BrowserProxy classes, it is actually able to act as a proxy for any external
dependency. I am using it here to ensure that the code being tested is making
the right calls to SearchManager, which is not part of this unit test.

> 
> One more thing -- I'm not sure you actually need the whole methodCalled
Promise business, since I think everything is synchronous (correct me if there's
any async bits). Maybe you only need to set a lastQuery variable.

The call to SearchManager#search is happening within a setTimeout() see
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/settin....

[dpapad, 2016-10-28 21:20:37]

Description was changed from

==========
MD Settings: Implement search URLs.

- Modify settings-ui to sync the URL 'search' parameter and the search
  textfield text to each other.
- Modify settings-main to used data-binding for triggering searches.
- Provide a way to programmatically change cr-search-field's value without
  triggering a 'search-changed' event (necessary for syncing URL with search
  field.

This makes searching and navigation compatible.

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

to

==========
MD Settings: Implement search URLs.

- Modify settings-ui to sync the URL 'search' parameter and the search
  textfield text to each other.
- Modify settings-main to used data-binding for triggering searches.
- Provide a way to programmatically change cr-search-field's value without
  triggering a 'search-changed' event (necessary for syncing URL with search
  field).

This makes searching and navigation compatible.

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

[dpapad, 2016-10-31 22:42:39]

On 2016/10/28 at 20:23:39, dpapad wrote:
> I have reworked this CL since previous comments significantly. I think the
latest version is much cleaner (as well as addresses a case not addressed
previously). Specifically
> 
>  - settings-main has trivial logic, simply triggering search based on a
data-binding.
>  - settings-ui holds all search URL related logic.
> 
> Summarizing our in-person conversation (with @tommycli and @dbeam), there is
still a small case where it is not handled correctly, which I plan to address
later (lower priority though). For the record (I will also file a bug with
this), that case is the following
> 1) Navigate to a subpage, like chrome://md-settings/searchEngines
> 2) Trigger a search via the textbox (for example "cookies").
> 3) Click the "back" browser button (nothing happens).
> 
> The bug is happening because in the code there is currently no way to
differentiate between the following two distinct cases.
> a) Navigating from a highlighted top-level page to a highlighted subpageb
(highlights should be preserved).
> b) Navigating from a highlighted top-level page back to a previously shown
(highlighted or not) subpage.
> 
> The code currently treats both a and b as a (which is a much more frequent
case).
> 
>
https://codereview.chromium.org/2449663002/diff/120001/chrome/test/data/webui...
> File chrome/test/data/webui/settings/settings_main_test.js (right):
> 
>
https://codereview.chromium.org/2449663002/diff/120001/chrome/test/data/webui...
> chrome/test/data/webui/settings/settings_main_test.js:8: * @extends
{settings.TestBrowserProxy}
> On 2016/10/27 at 21:07:17, tommycli wrote:
> > If I understand correctly? : The TestSearchManager isn't actually mocking a
browser proxy... it's just leveraging the TestBrowserProxy whenCalled machinery?
If so, it would be worth adding a comment explaining what's going on.
> 
> Added comment. Even though TestBrowserProxy has been traditionally used with
BrowserProxy classes, it is actually able to act as a proxy for any external
dependency. I am using it here to ensure that the code being tested is making
the right calls to SearchManager, which is not part of this unit test.
> 
> > 
> > One more thing -- I'm not sure you actually need the whole methodCalled
Promise business, since I think everything is synchronous (correct me if there's
any async bits). Maybe you only need to set a lastQuery variable.
> 
> The call to SearchManager#search is happening within a setTimeout() see
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/settin....

Friendly ping.

[Dan Beam, 2016-10-31 23:18:46]

Description was changed from

==========
MD Settings: Implement search URLs.

- Modify settings-ui to sync the URL 'search' parameter and the search
  textfield text to each other.
- Modify settings-main to used data-binding for triggering searches.
- Provide a way to programmatically change cr-search-field's value without
  triggering a 'search-changed' event (necessary for syncing URL with search
  field).

This makes searching and navigation compatible.

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

to

==========
MD Settings: Implement search URLs.

- Modify settings-ui to sync the URL 'search' parameter and the search
  textfield text to each other.
- Modify settings-main to used data-binding for triggering searches.
- Provide a way to programmatically change cr-search-field's value without
  triggering a 'search-changed' event (necessary for syncing URL with search
  field).

This makes searching and navigation compatible.

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

[Dan Beam, 2016-10-31 23:18:46]

dbeam@chromium.org changed reviewers:
- dbeam@chromium.org

[tommycli, 2016-11-01 21:05:26]

lgtm except these complaints: sorry for the delay!

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

https://codereview.chromium.org/2449663002/diff/240001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_main/settings_main.js:91:
searchQuery: {
Looks like settings-ui is messing with this. In that case, it's not private
right?

Might be worth adding a comment that it's populated from settings-ui also
instead of internally.

Finally - if the only usage of this property is to trigger calling
searchContents, maybe just have settings_ui directly call searchContents? Maybe
this hurts encapsulation a little bit - but parent elements are allow to call
methods on contained elements right?

https://codereview.chromium.org/2449663002/diff/240001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_ui/settings_ui.html (right):

https://codereview.chromium.org/2449663002/diff/240001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_ui/settings_ui.html:95:
on-search-changed="onSearchChanged_"
nit: it seems like this event should be called on-search-query-changed? Can be
separate CL, just nitpicking.

https://codereview.chromium.org/2449663002/diff/240001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_ui/settings_ui.js (right):

https://codereview.chromium.org/2449663002/diff/240001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_ui/settings_ui.js:130: // applied (if
any), no further action should be taken.
optional: may also be worth pointing out that all new searches take place on the
BASIC route anyways, so any navigations into subpages are either non-searches,
or continuations of an existing search.

https://codereview.chromium.org/2449663002/diff/240001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_ui/settings_ui.js:148:
this.searchQuery_ = urlSearchQuery;
Yeah the more I look at it, the more I think we can eliminate two properties and
just use this.$.main.searchContents. Will also let you eliminate that clause for
the observer initialization too...

[dpapad, 2016-11-01 22:02:02]

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

https://codereview.chromium.org/2449663002/diff/240001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_main/settings_main.js:91:
searchQuery: {
Removed searchQuery property per suggestion.

https://codereview.chromium.org/2449663002/diff/240001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_ui/settings_ui.html (right):

https://codereview.chromium.org/2449663002/diff/240001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_ui/settings_ui.html:95:
on-search-changed="onSearchChanged_"
On 2016/11/01 at 21:05:26, tommycli wrote:
> nit: it seems like this event should be called on-search-query-changed? Can be
separate CL, just nitpicking.

The native HTML <input type="search"> element is firing a simple 'search' event.
We chose 'search-changed' for the shared cr-search-field element, since unlike
the native element, it only fires if the value actually changed. I think is fine
since it is shorter than 'search-query-changed' without being too cryptic
hopefully.

https://codereview.chromium.org/2449663002/diff/240001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_ui/settings_ui.js (right):

https://codereview.chromium.org/2449663002/diff/240001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_ui/settings_ui.js:130: // applied (if
any), no further action should be taken.
On 2016/11/01 at 21:05:26, tommycli wrote:
> optional: may also be worth pointing out that all new searches take place on
the BASIC route anyways, so any navigations into subpages are either
non-searches, or continuations of an existing search.

Rephrased the comment per your suggestion. Also added a TODO describing the
corner case that is not being addressed, the one we've talked about before.

https://codereview.chromium.org/2449663002/diff/240001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_ui/settings_ui.js:148:
this.searchQuery_ = urlSearchQuery;
On 2016/11/01 at 21:05:26, tommycli wrote:
> Yeah the more I look at it, the more I think we can eliminate two properties
and just use this.$.main.searchContents. Will also let you eliminate that clause
for the observer initialization too...

Done.

[tommycli, 2016-11-01 22:05:47]

still lgtm - looking much cleaner!

https://codereview.chromium.org/2449663002/diff/240001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_ui/settings_ui.html (right):

https://codereview.chromium.org/2449663002/diff/240001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_ui/settings_ui.html:95:
on-search-changed="onSearchChanged_"
On 2016/11/01 22:02:02, dpapad wrote:
> On 2016/11/01 at 21:05:26, tommycli wrote:
> > nit: it seems like this event should be called on-search-query-changed? Can
be
> separate CL, just nitpicking.
> 
> The native HTML <input type="search"> element is firing a simple 'search'
event.
> We chose 'search-changed' for the shared cr-search-field element, since unlike
> the native element, it only fires if the value actually changed. I think is
fine
> since it is shorter than 'search-query-changed' without being too cryptic
> hopefully.

Acknowledged.

[dpapad, 2016-11-01 22:37:20]

On 2016/11/01 at 22:05:47, tommycli wrote:
> still lgtm - looking much cleaner!
> 
>
https://codereview.chromium.org/2449663002/diff/240001/chrome/browser/resourc...
> File chrome/browser/resources/settings/settings_ui/settings_ui.html (right):
> 
>
https://codereview.chromium.org/2449663002/diff/240001/chrome/browser/resourc...
> chrome/browser/resources/settings/settings_ui/settings_ui.html:95:
on-search-changed="onSearchChanged_"
> On 2016/11/01 22:02:02, dpapad wrote:
> > On 2016/11/01 at 21:05:26, tommycli wrote:
> > > nit: it seems like this event should be called on-search-query-changed?
Can be
> > separate CL, just nitpicking.
> > 
> > The native HTML <input type="search"> element is firing a simple 'search'
event.
> > We chose 'search-changed' for the shared cr-search-field element, since
unlike
> > the native element, it only fires if the value actually changed. I think is
fine
> > since it is shorter than 'search-query-changed' without being too cryptic
> > hopefully.
> 
> Acknowledged.

Thanks for the suggestion, agreed that dropping the Polymer property made things
cleaner.

[dpapad, 2016-11-01 22:37:27]

The CQ bit was checked by dpapad@chromium.org

[commit-bot: I haz the power, 2016-11-01 22:38:34]

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, 2016-11-01 22:59:09]

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

[commit-bot: I haz the power, 2016-11-01 22:59:11]

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

[dpapad, 2016-11-02 00:28:51]

The CQ bit was checked by dpapad@chromium.org

[commit-bot: I haz the power, 2016-11-02 00:29:20]

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

[Dan Beam, 2016-11-02 00:58:41]

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

[Dan Beam, 2016-11-02 00:58:42]

lgtm

https://codereview.chromium.org/2449663002/diff/260001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/cr_search_field/cr_search_field_behavior.js
(right):

https://codereview.chromium.org/2449663002/diff/260001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/cr_search_field/cr_search_field_behavior.js:61:
this.onValueChanged_(value, opt_noEvent || false);
nit: !!opt_noEvent

[dpapad, 2016-11-02 00:59:57]

The CQ bit was unchecked by dpapad@chromium.org

[dpapad, 2016-11-02 01:02:23]

https://codereview.chromium.org/2449663002/diff/260001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/cr_search_field/cr_search_field_behavior.js
(right):

https://codereview.chromium.org/2449663002/diff/260001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/cr_search_field/cr_search_field_behavior.js:61:
this.onValueChanged_(value, opt_noEvent || false);
On 2016/11/02 at 00:58:41, Dan Beam wrote:
> nit: !!opt_noEvent

Done.

[dpapad, 2016-11-02 01:02:29]

The CQ bit was checked by dpapad@chromium.org

[dpapad, 2016-11-02 01:02:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
tommycli@chromium.org, dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/2449663002/#ps280001
(title: "Nit")

[dpapad, 2016-11-02 01:02:41]

The CQ bit was unchecked by dpapad@chromium.org

[commit-bot: I haz the power, 2016-11-02 01:02:54]

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

[dpapad, 2016-11-02 01:03:30]

Description was changed from

==========
MD Settings: Implement search URLs.

- Modify settings-ui to sync the URL 'search' parameter and the search
  textfield text to each other.
- Modify settings-main to used data-binding for triggering searches.
- Provide a way to programmatically change cr-search-field's value without
  triggering a 'search-changed' event (necessary for syncing URL with search
  field).

This makes searching and navigation compatible.

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

to

==========
MD Settings: Implement search URLs.

- Modify settings-ui to sync the URL 'search' parameter and the search
  textfield text to each other, and to trigger searching when the current
  route changes to a "search" URL exists.
- Provide a way to programmatically change cr-search-field's value without
  triggering a 'search-changed' event (necessary for syncing URL with search
  field).

This makes searching and navigation compatible.

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

[dpapad, 2016-11-02 01:04:00]

Description was changed from

==========
MD Settings: Implement search URLs.

- Modify settings-ui to sync the URL 'search' parameter and the search
  textfield text to each other, and to trigger searching when the current
  route changes to a "search" URL exists.
- Provide a way to programmatically change cr-search-field's value without
  triggering a 'search-changed' event (necessary for syncing URL with search
  field).

This makes searching and navigation compatible.

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

to

==========
MD Settings: Implement search URLs.

- Modify settings-ui to sync the URL 'search' parameter and the search
  textfield text to each other, and to trigger searching when the current
  route changes to a "search" URL.
- Provide a way to programmatically change cr-search-field's value without
  triggering a 'search-changed' event (necessary for syncing URL with search
  field).

This makes searching and navigation compatible.

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

[dpapad, 2016-11-02 01:04:03]

The CQ bit was checked by dpapad@chromium.org

[commit-bot: I haz the power, 2016-11-02 01:04:29]

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, 2016-11-02 02:23:42]

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

[commit-bot: I haz the power, 2016-11-02 02:23:43]

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

[dpapad, 2016-11-02 02:36:36]

The CQ bit was checked by dpapad@chromium.org

[commit-bot: I haz the power, 2016-11-02 02:37:06]

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, 2016-11-02 03:52:11]

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

[commit-bot: I haz the power, 2016-11-02 03:52:13]

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

[dpapad, 2016-11-02 17:03:44]

On 2016/11/02 at 03:52:13, commit-bot wrote:
> Try jobs failed on following builders:
>   linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

Need to fix ChromeOS legit test failures. Looking..

[dpapad, 2016-11-02 20:56:44]

On 2016/11/02 at 17:03:44, dpapad wrote:
> On 2016/11/02 at 03:52:13, commit-bot wrote:
> > Try jobs failed on following builders:
> >   linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
> 
> Need to fix ChromeOS legit test failures. Looking..

Tommy, see the diff between 11 and 12. Now that we don't use data binding to
trigger the search, we don't get free "don't call if it did not change"
behavior. So I had to introduce a lastSearchQuery_ param.

[tommycli, 2016-11-02 20:59:34]

On 2016/11/02 20:56:44, dpapad wrote:
> On 2016/11/02 at 17:03:44, dpapad wrote:
> > On 2016/11/02 at 03:52:13, commit-bot wrote:
> > > Try jobs failed on following builders:
> > >   linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
> (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
> > 
> > Need to fix ChromeOS legit test failures. Looking..
> 
> Tommy, see the diff between 11 and 12. Now that we don't use data binding to
> trigger the search, we don't get free "don't call if it did not change"
> behavior. So I had to introduce a lastSearchQuery_ param.

Ah, that's a bit annoying. Still a net win over the data binding, IMO, but not
as much of a win. Still lgtm.

[dpapad, 2016-11-02 22:52:44]

The CQ bit was checked by dpapad@chromium.org

[dpapad, 2016-11-02 22:52:44]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/2449663002/#ps300001
(title: "Fix test.")

[commit-bot: I haz the power, 2016-11-02 22:53:40]

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, 2016-11-03 00:05:20]

Committed patchset #12 (id:300001)

[commit-bot: I haz the power, 2016-11-03 00:08:02]

Description was changed from

==========
MD Settings: Implement search URLs.

- Modify settings-ui to sync the URL 'search' parameter and the search
  textfield text to each other, and to trigger searching when the current
  route changes to a "search" URL.
- Provide a way to programmatically change cr-search-field's value without
  triggering a 'search-changed' event (necessary for syncing URL with search
  field).

This makes searching and navigation compatible.

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

to

==========
MD Settings: Implement search URLs.

- Modify settings-ui to sync the URL 'search' parameter and the search
  textfield text to each other, and to trigger searching when the current
  route changes to a "search" URL.
- Provide a way to programmatically change cr-search-field's value without
  triggering a 'search-changed' event (necessary for syncing URL with search
  field).

This makes searching and navigation compatible.

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

Committed: https://crrev.com/63797594bc2bb93e645129b0c679081dc925ba5f
Cr-Commit-Position: refs/heads/master@{#429465}
==========

[commit-bot: I haz the power, 2016-11-03 00:08:04]

Patchset 12 (id:??) landed as
https://crrev.com/63797594bc2bb93e645129b0c679081dc925ba5f
Cr-Commit-Position: refs/heads/master@{#429465}