Settings People Revamp: Update ManageProfileHandler to use Promises.

Settings People Revamp: Update ManageProfileHandler to use Promises.

Updates the GetAvailableIcons call to ManageProfileHandler to use a
Promise. This makes the interaction of call-and-response more explicit.

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

[tommycli, 2016-04-07 18:29:22]

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

[commit-bot: I haz the power, 2016-04-07 18:29:56]

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

[tommycli, 2016-04-07 19:25:36]

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

[tommycli, 2016-04-07 19:25:36]

dpapad: PTAL, this is the promised patch for promises.

[commit-bot: I haz the power, 2016-04-07 19:28:26]

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

[commit-bot: I haz the power, 2016-04-07 19:28:27]

Dry run: This issue passed the CQ dry run.

[dpapad, 2016-04-07 20:34:24]

LGTM!

https://codereview.chromium.org/1871653002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/people_page/manage_profile.js (right):

https://codereview.chromium.org/1871653002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/manage_profile.js:47: attached:
function() {
Nit (optional): you can reuse the same function twice here.

attached: function() {
  var setIcons = function(iconUrls) {
    this.availableIconUrls = iconUrls;
  }.bind(this);

  this.addWebUIListener('available-icons-changed', setIcons);
  this.browserProxy_.getAvailableIcons().then(setIcons);
},

OR you could also start listening only after receiving the direct response as
follows. This seems a bit safer in terms of race-conditions, but in practice I
don't think it makes any difference.

attached: function() {
  var setIcons = function(iconUrls) {
    this.availableIconUrls = iconUrls;
  }.bind(this);

  this.browserProxy_.getAvailableIcons().then(function(iconUrls) {
    setIcons(iconUrls);
    this.addWebUIListener('available-icons-changed', setIcons);
  }.bind(this);
},

https://codereview.chromium.org/1871653002/diff/1/chrome/browser/ui/webui/set...
File
chrome/browser/ui/webui/settings/settings_manage_profile_handler_unittest.cc
(right):

https://codereview.chromium.org/1871653002/diff/1/chrome/browser/ui/webui/set...
chrome/browser/ui/webui/settings/settings_manage_profile_handler_unittest.cc:86:
TEST_F(ManageProfileHandlerTest, GetAvailableIcons) {
Nit: TEST_F(ManageProfileHandlerTest, HandleGetAvailableIcons) {

[tommycli, 2016-04-07 20:48:38]

dpapad: thanks!

https://codereview.chromium.org/1871653002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/people_page/manage_profile.js (right):

https://codereview.chromium.org/1871653002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/manage_profile.js:47: attached:
function() {
On 2016/04/07 20:34:24, dpapad wrote:
> Nit (optional): you can reuse the same function twice here.
> 
> attached: function() {
>   var setIcons = function(iconUrls) {
>     this.availableIconUrls = iconUrls;
>   }.bind(this);
> 
>   this.addWebUIListener('available-icons-changed', setIcons);
>   this.browserProxy_.getAvailableIcons().then(setIcons);
> },
> 
> OR you could also start listening only after receiving the direct response as
> follows. This seems a bit safer in terms of race-conditions, but in practice I
> don't think it makes any difference.
> 
> attached: function() {
>   var setIcons = function(iconUrls) {
>     this.availableIconUrls = iconUrls;
>   }.bind(this);
> 
>   this.browserProxy_.getAvailableIcons().then(function(iconUrls) {
>     setIcons(iconUrls);
>     this.addWebUIListener('available-icons-changed', setIcons);
>   }.bind(this);
> },
> 
> 

I did the first option. I don't think it's racy unless the C++
CallJavaScriptFunction method have their calls reordered.

I think in the second option, technically the profile avatar can be updated
after the getAvailableIcons() is handled, and before the WebUI listener is
attached.

https://codereview.chromium.org/1871653002/diff/1/chrome/browser/ui/webui/set...
File
chrome/browser/ui/webui/settings/settings_manage_profile_handler_unittest.cc
(right):

https://codereview.chromium.org/1871653002/diff/1/chrome/browser/ui/webui/set...
chrome/browser/ui/webui/settings/settings_manage_profile_handler_unittest.cc:86:
TEST_F(ManageProfileHandlerTest, GetAvailableIcons) {
On 2016/04/07 20:34:24, dpapad wrote:
> Nit: TEST_F(ManageProfileHandlerTest, HandleGetAvailableIcons) {

Done.

[tommycli, 2016-04-07 20:48:56]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2016-04-07 20:48:56]

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/1871653002/#ps20001
(title: " ")

[commit-bot: I haz the power, 2016-04-07 20:50:09]

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

[commit-bot: I haz the power, 2016-04-07 21:49:21]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-04-07 21:50:36]

Description was changed from

==========
Settings People Revamp: Update ManageProfileHandler to use Promises.

Updates the GetAvailableIcons call to ManageProfileHandler to use a
Promise. This makes the interaction of call-and-response more explicit.

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

to

==========
Settings People Revamp: Update ManageProfileHandler to use Promises.

Updates the GetAvailableIcons call to ManageProfileHandler to use a
Promise. This makes the interaction of call-and-response more explicit.

BUG=563721

Committed: https://crrev.com/4602816e6f2344de78f17a9471c1d6cf80cdcaec
Cr-Commit-Position: refs/heads/master@{#385885}
==========

[commit-bot: I haz the power, 2016-04-07 21:50:37]

Patchset 2 (id:??) landed as
https://crrev.com/4602816e6f2344de78f17a9471c1d6cf80cdcaec
Cr-Commit-Position: refs/heads/master@{#385885}