Settings People Revamp: Split Profile Info out into its own handler.

Settings People Revamp: Split Profile Info out into its own handler.

In preparation for overhauling the Advanced Sync Settings, this refactor
splits off the Profile Info functionality into its own handler both
with C++ tests and TestBrowserProxy-based JavaScript tests.

Although this adds more SLOC, this reduces the complexity of the
1000-line PeopleHandler.

BUG=563721
Committed: https://crrev.com/e391ce5e330c9affc85afe5be6abc9dfd0a7e05f
Cr-Commit-Position: refs/heads/master@{#388108}

[tommycli, 2016-04-18 21:59:42]

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

[commit-bot: I haz the power, 2016-04-18 22:00:19]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1900913002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1900913002/40001

[tommycli, 2016-04-18 22:46:54]

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

[tommycli, 2016-04-18 22:46:55]

dpapad: PTAL. I apologize for the MEGA CL. it's a refactor + adding a C++
handler test + adding a browsertest.

Again I apologize for the largeness.

[dpapad, 2016-04-18 23:30:50]

LGTM++

https://codereview.chromium.org/1900913002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/people_page/profile_info_browser_proxy.js
(right):

https://codereview.chromium.org/1900913002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/profile_info_browser_proxy.js:38:
// The singleton instance_ is replaced with a test version of this wrapper
Nit(optional): I think it is OK to not copy this comment into every
browser_proxy. It is already copied in quite a few such files.

https://codereview.chromium.org/1900913002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_resources.grd (right):

https://codereview.chromium.org/1900913002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_resources.grd:480: flattenhtml="true"
Nit: Is flattenhtml necessary here?

https://codereview.chromium.org/1900913002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/profile_info_handler_unittest.cc (right):

https://codereview.chromium.org/1900913002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/profile_info_handler_unittest.cc:132:
EXPECT_EQ("profile-info-changed", event_id);
Nit: Can we put this event name into a const and refer to it from here and from
within PushProfileInfo? Something like
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...,
but for the event name.

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

https://codereview.chromium.org/1900913002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/people_page_test.js:24: name: 'fakeName',
Nit (optional): Make this object a public member var of the
TestProfileInfoBrowserProxy, and refer to it from within the tests, see
following related comment.

https://codereview.chromium.org/1900913002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/people_page_test.js:63:
assertEquals('http://fake-icon-url.com/',
assertEquals(browserProxy.profileInfo.iconUrl,
peoplePage.$$('#profile-icon').src);

[tommycli, 2016-04-19 00:03:27]

dpapad: thanks for the super fast review

https://codereview.chromium.org/1900913002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/people_page/profile_info_browser_proxy.js
(right):

https://codereview.chromium.org/1900913002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/profile_info_browser_proxy.js:38:
// The singleton instance_ is replaced with a test version of this wrapper
On 2016/04/18 23:30:49, dpapad wrote:
> Nit(optional): I think it is OK to not copy this comment into every
> browser_proxy. It is already copied in quite a few such files.

Done.

https://codereview.chromium.org/1900913002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_resources.grd (right):

https://codereview.chromium.org/1900913002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_resources.grd:480: flattenhtml="true"
On 2016/04/18 23:30:49, dpapad wrote:
> Nit: Is flattenhtml necessary here?

Done.

https://codereview.chromium.org/1900913002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/profile_info_handler_unittest.cc (right):

https://codereview.chromium.org/1900913002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/profile_info_handler_unittest.cc:132:
EXPECT_EQ("profile-info-changed", event_id);
On 2016/04/18 23:30:49, dpapad wrote:
> Nit: Can we put this event name into a const and refer to it from here and
from
> within PushProfileInfo? Something like
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...,
> but for the event name.

Done.

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

https://codereview.chromium.org/1900913002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/people_page_test.js:24: name: 'fakeName',
On 2016/04/18 23:30:49, dpapad wrote:
> Nit (optional): Make this object a public member var of the
> TestProfileInfoBrowserProxy, and refer to it from within the tests, see
> following related comment.

Done.

https://codereview.chromium.org/1900913002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/people_page_test.js:63:
assertEquals('http://fake-icon-url.com/',
On 2016/04/18 23:30:49, dpapad wrote:
> assertEquals(browserProxy.profileInfo.iconUrl,
> peoplePage.$$('#profile-icon').src);

Done.

[tommycli, 2016-04-19 00:03:39]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2016-04-19 00:03:39]

The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org
Link to the patchset: https://codereview.chromium.org/1900913002/#ps80001
(title: " ")

[commit-bot: I haz the power, 2016-04-19 00:03:58]

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

[commit-bot: I haz the power, 2016-04-19 01:16:35]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-04-19 01:17:18]

Description was changed from

==========
Settings People Revamp: Split Profile Info out into its own handler.

In preparation for overhauling the Advanced Sync Settings, this refactor
splits off the Profile Info functionality into its own handler both
with C++ tests and TestBrowserProxy-based JavaScript tests.

Although this adds more SLOC, this reduces the complexity of the
1000-line PeopleHandler.

BUG=563721
==========

to

==========
Settings People Revamp: Split Profile Info out into its own handler.

In preparation for overhauling the Advanced Sync Settings, this refactor
splits off the Profile Info functionality into its own handler both
with C++ tests and TestBrowserProxy-based JavaScript tests.

Although this adds more SLOC, this reduces the complexity of the
1000-line PeopleHandler.

BUG=563721

Committed: https://crrev.com/e391ce5e330c9affc85afe5be6abc9dfd0a7e05f
Cr-Commit-Position: refs/heads/master@{#388108}
==========

[commit-bot: I haz the power, 2016-04-19 01:17:20]

Patchset 5 (id:??) landed as
https://crrev.com/e391ce5e330c9affc85afe5be6abc9dfd0a7e05f
Cr-Commit-Position: refs/heads/master@{#388108}