MD Settings: Search page, remove last usage of chrome.searchPrivate API.

MD Settings: Search page, remove last usage of chrome.searchPrivate API.

BUG=479359, 588189
Committed: https://crrev.com/4ad7fd803305d2303f852258d989f2c003033464
Cr-Commit-Position: refs/heads/master@{#377086}

[dpapad, 2016-02-20 01:36:24]

Description was changed from

==========
MD Settings: Search page, remove references to chrome.searchPrivate API.

BUG=479359,588189
==========

to

==========
MD Settings: Search page, remove last usage of chrome.searchPrivate API.

BUG=479359,588189
==========

[dpapad, 2016-02-20 01:43:40]

Patchset #2 (id:20001) has been deleted

[dpapad, 2016-02-20 02:09:05]

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

[michaelpg, 2016-02-20 04:31:16]

drive-by question

https://codereview.chromium.org/1714843004/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/search_page/search_page.html (right):

https://codereview.chromium.org/1714843004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/search_page/search_page.html:5: <link
rel="import"
href="chrome://md-settings/search_engines_page/search_engines_browser_proxy.html">
nit: alphabetize

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

https://codereview.chromium.org/1714843004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/search_page/search_page.js:46:
cr.addWebUIListener(
Since the BrowserProxy handles push (setDefaultSearchEngine) and pull
(getSearchEnginesList), would it make sense to also have it handle events
(onSearchEnginesChanged) instead of going down to the cr layer here in the UI?

[dpapad, 2016-02-22 18:57:53]

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

https://codereview.chromium.org/1714843004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/search_page/search_page.js:46:
cr.addWebUIListener(
On 2016/02/20 at 04:31:16, michaelpg wrote:
> Since the BrowserProxy handles push (setDefaultSearchEngine) and pull
(getSearchEnginesList), would it make sense to also have it handle events
(onSearchEnginesChanged) instead of going down to the cr layer here in the UI?

Are you suggesting to wrap the cr.addWebUIListener() call with a new method on
the proxy that delegates to cr.addWebUIListener, or to not use
cr.addWebUIListener at all?


If you are suggesting the latter, I think it would make things more complicated.
Briefly giving a description of the pattern we have ended up, which seems to
work well enough (both for prod and testing).

1) The browser proxy intentionally has no state, it simply wraps every single
chrome.send() call with a type annotated version. It has no reference back to
any Polymer element that is using the proxy.
2) The proxy does not handle any events, and does not expose and global method
that is called from C++.
3) Individual Polymer elements are responsible for calling cr.addWebUIListener
(and removeWebUIListener if needed).

The above 3 characteristics allow the following usage during tests.
1) A test version of the proxy is used to intercept JS -> C++ calls, and to
provide mock responses.
2) C++ event callback codepaths can be triggered during the test by directly
calling cr.webUIListenerCallback.

[Dan Beam, 2016-02-23 02:41:22]

lgtm

https://codereview.chromium.org/1714843004/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/search_page/search_page.js (left):

https://codereview.chromium.org/1714843004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/search_page/search_page.js:32: * List of
default search engines available.
this removes "default" as a bit of documentation.  is that important?

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

https://codereview.chromium.org/1714843004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/search_page/search_page.js:43: };
bind() only once here

[dpapad, 2016-02-23 18:35:39]

https://codereview.chromium.org/1714843004/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/search_page/search_page.js (left):

https://codereview.chromium.org/1714843004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/search_page/search_page.js:32: * List of
default search engines available.
On 2016/02/23 at 02:41:22, Dan Beam wrote:
> this removes "default" as a bit of documentation.  is that important?

Restored.

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

https://codereview.chromium.org/1714843004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/search_page/search_page.js:43: };
On 2016/02/23 at 02:41:22, Dan Beam wrote:
> bind() only once here

Done.

[dpapad, 2016-02-23 19:38:10]

The CQ bit was checked by dpapad@chromium.org

[dpapad, 2016-02-23 19:38:11]

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/1714843004/#ps60001
(title: "Addressing comments")

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

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1714843004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1714843004/60001

[commit-bot: I haz the power, 2016-02-23 21:19:41]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-02-23 21:20:46]

Description was changed from

==========
MD Settings: Search page, remove last usage of chrome.searchPrivate API.

BUG=479359,588189
==========

to

==========
MD Settings: Search page, remove last usage of chrome.searchPrivate API.

BUG=479359,588189

Committed: https://crrev.com/4ad7fd803305d2303f852258d989f2c003033464
Cr-Commit-Position: refs/heads/master@{#377086}
==========

[commit-bot: I haz the power, 2016-02-23 21:20:47]

Patchset 3 (id:??) landed as
https://crrev.com/4ad7fd803305d2303f852258d989f2c003033464
Cr-Commit-Position: refs/heads/master@{#377086}