Add error handling for supervised user import flow.

Add error handling for supervised user import flow.

Add a check if the user tries to create a profile with the
name of an already existing supervised user. In that case,
the user is not allowed to create a new such supervised user
profile, instead he can directly import the existing supervised
user.
A screenshot can be found in the bug description.

BUG=282464
TEST=browser_tests

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243855

[Adrian Kuegel, 2014-01-07 16:30:10]

Pam, can you please review this CL?

[Pam (message me for reviews), 2014-01-08 11:21:35]

https://codereview.chromium.org/125993002/diff/130001/chrome/browser/resource...
File chrome/browser/resources/options/manage_profile_overlay.css (right):

https://codereview.chromium.org/125993002/diff/130001/chrome/browser/resource...
chrome/browser/resources/options/manage_profile_overlay.css:59: }
From the screenshot, it looks like you also want to remove the padding around
the supervised-user-import link-button (see extra spaces before and after the
text).

https://codereview.chromium.org/125993002/diff/130001/chrome/browser/resource...
File chrome/browser/resources/options/manage_profile_overlay.js (right):

https://codereview.chromium.org/125993002/diff/130001/chrome/browser/resource...
chrome/browser/resources/options/manage_profile_overlay.js:284: * @param {Event}
event The event object.
|event| is no longer a parameter.

https://codereview.chromium.org/125993002/diff/130001/chrome/browser/resource...
chrome/browser/resources/options/manage_profile_overlay.js:339:
'manageProfilesExistingSupervisedUser', newName);
You'll need to HTML-escape and elide the new name, so user-entered names like
  "<i>Italics?<i>"
and
  "This is a very long name that will otherwise make the overlay look funny!"
look good too.
Since managed_user_create_confirm.js also uses those two methods (HTMLEscape()
and elide()), it's worth moving them to some shared options util file.

https://codereview.chromium.org/125993002/diff/130001/chrome/browser/resource...
chrome/browser/resources/options/manage_profile_overlay.js:342: // Check if
there exists also other supervised users with that name.
nit: if another supervised user also exists

https://codereview.chromium.org/125993002/diff/130001/chrome/browser/resource...
File chrome/browser/resources/options/managed_user_import.js (right):

https://codereview.chromium.org/125993002/diff/130001/chrome/browser/resource...
chrome/browser/resources/options/managed_user_import.js:10: * ManagedUserList
class.
I'd suggest moving this into its own file.

https://codereview.chromium.org/125993002/diff/130001/chrome/browser/resource...
chrome/browser/resources/options/managed_user_import.js:13: * possible to serve
future requests immediately. The first request will be
Are we concerned about the cache becoming stale, e.g. if the user creates a new
SU in another instance of Chrome after this has been loaded?

https://codereview.chromium.org/125993002/diff/130001/chrome/browser/resource...
chrome/browser/resources/options/managed_user_import.js:38: * Receives a list of
managed users.
and passes the list to each of the callbacks.

https://codereview.chromium.org/125993002/diff/130001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/manage_profile_browsertest.js (right):

https://codereview.chromium.org/125993002/diff/130001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/manage_profile_browsertest.js:181: // Initially,
the ok button should not be disabled and the import link should
This would be clearer as "should be enabled"

https://codereview.chromium.org/125993002/diff/130001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/manage_profile_browsertest.js:204: });
Also test creating a name that doesn't exist.

[Adrian Kuegel, 2014-01-08 12:51:46]

https://codereview.chromium.org/125993002/diff/130001/chrome/browser/resource...
File chrome/browser/resources/options/manage_profile_overlay.css (right):

https://codereview.chromium.org/125993002/diff/130001/chrome/browser/resource...
chrome/browser/resources/options/manage_profile_overlay.css:59: }
On 2014/01/08 11:21:35, Pam (also PM for reviews) wrote:
> From the screenshot, it looks like you also want to remove the padding around
> the supervised-user-import link-button (see extra spaces before and after the
> text).

Done.

https://codereview.chromium.org/125993002/diff/130001/chrome/browser/resource...
File chrome/browser/resources/options/manage_profile_overlay.js (right):

https://codereview.chromium.org/125993002/diff/130001/chrome/browser/resource...
chrome/browser/resources/options/manage_profile_overlay.js:284: * @param {Event}
event The event object.
On 2014/01/08 11:21:35, Pam (also PM for reviews) wrote:
> |event| is no longer a parameter.

Right. Removed the comment now.

https://codereview.chromium.org/125993002/diff/130001/chrome/browser/resource...
chrome/browser/resources/options/manage_profile_overlay.js:339:
'manageProfilesExistingSupervisedUser', newName);
On 2014/01/08 11:21:35, Pam (also PM for reviews) wrote:
> You'll need to HTML-escape and elide the new name, so user-entered names like
>   "<i>Italics?<i>"
> and
>   "This is a very long name that will otherwise make the overlay look funny!"
> look good too.
> Since managed_user_create_confirm.js also uses those two methods (HTMLEscape()
> and elide()), it's worth moving them to some shared options util file.

Done.

https://codereview.chromium.org/125993002/diff/130001/chrome/browser/resource...
chrome/browser/resources/options/manage_profile_overlay.js:342: // Check if
there exists also other supervised users with that name.
On 2014/01/08 11:21:35, Pam (also PM for reviews) wrote:
> nit: if another supervised user also exists

Done.

https://codereview.chromium.org/125993002/diff/130001/chrome/browser/resource...
File chrome/browser/resources/options/managed_user_import.js (right):

https://codereview.chromium.org/125993002/diff/130001/chrome/browser/resource...
chrome/browser/resources/options/managed_user_import.js:10: * ManagedUserList
class.
On 2014/01/08 11:21:35, Pam (also PM for reviews) wrote:
> I'd suggest moving this into its own file.

Yes, I thought about that, but I guess I should then rename this class, because
a managed_user_list.js file already exists which handles the ui part of the list
of existing supervised users. Maybe ManagedUserCache would be ok?

https://codereview.chromium.org/125993002/diff/130001/chrome/browser/resource...
chrome/browser/resources/options/managed_user_import.js:13: * possible to serve
future requests immediately. The first request will be
On 2014/01/08 11:21:35, Pam (also PM for reviews) wrote:
> Are we concerned about the cache becoming stale, e.g. if the user creates a
new
> SU in another instance of Chrome after this has been loaded?

Yes, I think we should handle that. I think we could add a notification for this
event. Then we would not have to load the list each time, but the list is
updated when there are changes.
Would it be ok for you if I delay this to a future CL?

https://codereview.chromium.org/125993002/diff/130001/chrome/browser/resource...
chrome/browser/resources/options/managed_user_import.js:38: * Receives a list of
managed users.
On 2014/01/08 11:21:35, Pam (also PM for reviews) wrote:
> and passes the list to each of the callbacks.

Done.

https://codereview.chromium.org/125993002/diff/130001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/manage_profile_browsertest.js (right):

https://codereview.chromium.org/125993002/diff/130001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/manage_profile_browsertest.js:181: // Initially,
the ok button should not be disabled and the import link should
On 2014/01/08 11:21:35, Pam (also PM for reviews) wrote:
> This would be clearer as "should be enabled"

Done.

https://codereview.chromium.org/125993002/diff/130001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/manage_profile_browsertest.js:204: });
On 2014/01/08 11:21:35, Pam (also PM for reviews) wrote:
> Also test creating a name that doesn't exist.

Done.

[Pam (message me for reviews), 2014-01-08 14:36:45]

https://codereview.chromium.org/125993002/diff/130001/chrome/browser/resource...
File chrome/browser/resources/options/managed_user_import.js (right):

https://codereview.chromium.org/125993002/diff/130001/chrome/browser/resource...
chrome/browser/resources/options/managed_user_import.js:10: * ManagedUserList
class.
On 2014/01/08 12:51:47, Adrian Kuegel wrote:
> On 2014/01/08 11:21:35, Pam (also PM for reviews) wrote:
> > I'd suggest moving this into its own file.
> 
> Yes, I thought about that, but I guess I should then rename this class,
because
> a managed_user_list.js file already exists which handles the ui part of the
list
> of existing supervised users. Maybe ManagedUserCache would be ok?

Oh boy, what a batch of confusing class names. Oh well -- ManagedUserCache
sounds good. Or maybe ManagedUserDataModel, ManagedUserListData,
ManagedUserListModel?

https://codereview.chromium.org/125993002/diff/130001/chrome/browser/resource...
chrome/browser/resources/options/managed_user_import.js:13: * possible to serve
future requests immediately. The first request will be
On 2014/01/08 12:51:47, Adrian Kuegel wrote:
> On 2014/01/08 11:21:35, Pam (also PM for reviews) wrote:
> > Are we concerned about the cache becoming stale, e.g. if the user creates a
> new
> > SU in another instance of Chrome after this has been loaded?
> 
> Yes, I think we should handle that. I think we could add a notification for
this
> event. Then we would not have to load the list each time, but the list is
> updated when there are changes.
> Would it be ok for you if I delay this to a future CL?

Yes, since the failure case (managing to create a new SU with the same name as
an existing one) isn't horrible for the short term; it's exactly what we have
now.

An easy improvement would be to update the cached list when a new SU is created
locally. That could happen entirely within JS.

https://codereview.chromium.org/125993002/diff/250001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/manage_profile_browsertest.js (right):

https://codereview.chromium.org/125993002/diff/250001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/manage_profile_browsertest.js:193:
assertTrue($('supervised-user-import') != null);
How about
  assertFalse($('supervised-user-import') == null);
here and below, to make the difference from the other cases more apparent.

https://codereview.chromium.org/125993002/diff/250001/ui/webui/resources/js/u...
File ui/webui/resources/js/util.js (right):

https://codereview.chromium.org/125993002/diff/250001/ui/webui/resources/js/u...
ui/webui/resources/js/util.js:386: * Replace '&', '<', '>', '"', and '''
characters with their HTML encoding.
nit: Replaces

[Adrian Kuegel, 2014-01-08 16:49:07]

https://codereview.chromium.org/125993002/diff/130001/chrome/browser/resource...
File chrome/browser/resources/options/managed_user_import.js (right):

https://codereview.chromium.org/125993002/diff/130001/chrome/browser/resource...
chrome/browser/resources/options/managed_user_import.js:10: * ManagedUserList
class.
On 2014/01/08 14:36:45, Pam (also PM for reviews) wrote:
> On 2014/01/08 12:51:47, Adrian Kuegel wrote:
> > On 2014/01/08 11:21:35, Pam (also PM for reviews) wrote:
> > > I'd suggest moving this into its own file.
> > 
> > Yes, I thought about that, but I guess I should then rename this class,
> because
> > a managed_user_list.js file already exists which handles the ui part of the
> list
> > of existing supervised users. Maybe ManagedUserCache would be ok?
> 
> Oh boy, what a batch of confusing class names. Oh well -- ManagedUserCache
> sounds good. Or maybe ManagedUserDataModel, ManagedUserListData,
> ManagedUserListModel?

I decided to use ManagedUserListData now.

https://codereview.chromium.org/125993002/diff/130001/chrome/browser/resource...
chrome/browser/resources/options/managed_user_import.js:13: * possible to serve
future requests immediately. The first request will be
On 2014/01/08 14:36:45, Pam (also PM for reviews) wrote:
> On 2014/01/08 12:51:47, Adrian Kuegel wrote:
> > On 2014/01/08 11:21:35, Pam (also PM for reviews) wrote:
> > > Are we concerned about the cache becoming stale, e.g. if the user creates
a
> > new
> > > SU in another instance of Chrome after this has been loaded?
> > 
> > Yes, I think we should handle that. I think we could add a notification for
> this
> > event. Then we would not have to load the list each time, but the list is
> > updated when there are changes.
> > Would it be ok for you if I delay this to a future CL?
> 
> Yes, since the failure case (managing to create a new SU with the same name as
> an existing one) isn't horrible for the short term; it's exactly what we have
> now.
> 
> An easy improvement would be to update the cached list when a new SU is
created
> locally. That could happen entirely within JS.

Ok, makes sense. Otherwise I think the import dialog would still allow to import
the supervised user that just got imported.
I added a function which will just reload the list. This seemed to be the
easiest way.
This will not handle the case that you have two settings pages open at the same
time, but at least now the list in the same settings page will be updated after
a supervised user profile is deleted or created.

https://codereview.chromium.org/125993002/diff/250001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/manage_profile_browsertest.js (right):

https://codereview.chromium.org/125993002/diff/250001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/manage_profile_browsertest.js:193:
assertTrue($('supervised-user-import') != null);
On 2014/01/08 14:36:46, Pam (also PM for reviews) wrote:
> How about
>   assertFalse($('supervised-user-import') == null);
> here and below, to make the difference from the other cases more apparent.

Done.

https://codereview.chromium.org/125993002/diff/250001/ui/webui/resources/js/u...
File ui/webui/resources/js/util.js (right):

https://codereview.chromium.org/125993002/diff/250001/ui/webui/resources/js/u...
ui/webui/resources/js/util.js:386: * Replace '&', '<', '>', '"', and '''
characters with their HTML encoding.
On 2014/01/08 14:36:46, Pam (also PM for reviews) wrote:
> nit: Replaces

Done.

[Pam (message me for reviews), 2014-01-08 17:21:16]

LGTM, but see nit below.

https://codereview.chromium.org/125993002/diff/390001/chrome/browser/resource...
File chrome/browser/resources/options/manage_profile_overlay.js (right):

https://codereview.chromium.org/125993002/diff/390001/chrome/browser/resource...
chrome/browser/resources/options/manage_profile_overlay.js:70:
options.ManagedUserListData.reloadExistingManagedUsers();
Since deleting a profile locally doesn't remove it from the manager's list, this
shouldn't be needed.

[Adrian Kuegel, 2014-01-09 08:56:34]

https://codereview.chromium.org/125993002/diff/390001/chrome/browser/resource...
File chrome/browser/resources/options/manage_profile_overlay.js (right):

https://codereview.chromium.org/125993002/diff/390001/chrome/browser/resource...
chrome/browser/resources/options/manage_profile_overlay.js:70:
options.ManagedUserListData.reloadExistingManagedUsers();
On 2014/01/08 17:21:16, Pam (also PM for reviews) wrote:
> Since deleting a profile locally doesn't remove it from the manager's list,
this
> shouldn't be needed.

There is one small change: it will have the onCurrentDevice property updated, so
it will be possible again to import this supervised user. So I think this might
make sense here.

[Pam (message me for reviews), 2014-01-09 09:00:19]

https://codereview.chromium.org/125993002/diff/390001/chrome/browser/resource...
File chrome/browser/resources/options/manage_profile_overlay.js (right):

https://codereview.chromium.org/125993002/diff/390001/chrome/browser/resource...
chrome/browser/resources/options/manage_profile_overlay.js:70:
options.ManagedUserListData.reloadExistingManagedUsers();
On 2014/01/09 08:56:34, Adrian Kuegel wrote:
> On 2014/01/08 17:21:16, Pam (also PM for reviews) wrote:
> > Since deleting a profile locally doesn't remove it from the manager's list,
> this
> > shouldn't be needed.
> 
> There is one small change: it will have the onCurrentDevice property updated,
so
> it will be possible again to import this supervised user. So I think this
might
> make sense here.

Ah, yes, you're quite right!

[commit-bot: I haz the power, 2014-01-09 09:01:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akuegel@chromium.org/125993002/390001

[commit-bot: I haz the power, 2014-01-09 10:00:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akuegel@chromium.org/125993002/570002

[commit-bot: I haz the power, 2014-01-09 12:01:57]

Change committed as 243855

[Bernhard Bauer, 2014-01-09 13:39:07]

Post-commit drive-by review! Don't you just love those? ;-)

https://codereview.chromium.org/125993002/diff/570002/chrome/browser/resource...
File chrome/browser/resources/options/managed_user_list_data.js (right):

https://codereview.chromium.org/125993002/diff/570002/chrome/browser/resource...
chrome/browser/resources/options/managed_user_list_data.js:49: var i;
Nit: You can inline the var into the for loop.

https://codereview.chromium.org/125993002/diff/570002/chrome/browser/resource...
chrome/browser/resources/options/managed_user_list_data.js:79: * @param {Object}
errback the callback function which is called on error.
It might be nice to do the whole thing with DOM futures... :)

That would actually make some of the behavior really nice, like caching the
|managed_users_| list (we just create the promise lazily and send out the
request to the browser when creating it, then keep it around, so future requests
will be immediately served by the fulfilled promise).

https://codereview.chromium.org/125993002/diff/570002/chrome/browser/resource...
chrome/browser/resources/options/managed_user_list_data.js:83: var instance =
ManagedUserListData.getInstance();
The usual way to do this is to make this a private instance method, and forward
to it from the (public) static method.

https://codereview.chromium.org/125993002/diff/570002/chrome/browser/resource...
chrome/browser/resources/options/managed_user_list_data.js:88:
ManagedUserListData.receiveExistingManagedUsers(instance.managedUsers_);
Note that this will not only call |callback|, but also any already existing
callbacks on |instance|. Couldn't you just directly call |callback|?

Hm... Can we even have more than one callback/errback at any point? If not, why
don't we just store them directly on the instance?

https://codereview.chromium.org/125993002/diff/570002/ui/webui/resources/js/u...
File ui/webui/resources/js/util.js (right):

https://codereview.chromium.org/125993002/diff/570002/ui/webui/resources/js/u...
ui/webui/resources/js/util.js:410: return original.substring(0, maxLength - 3) +
'...';
Nit: You should use an ellipsis (…) for slightly nicer typography.

[Adrian Kuegel, 2014-01-09 14:12:41]

Bernhard, I see you like to review code :) I will send you my next CL, ok?

https://codereview.chromium.org/125993002/diff/570002/chrome/browser/resource...
File chrome/browser/resources/options/managed_user_list_data.js (right):

https://codereview.chromium.org/125993002/diff/570002/chrome/browser/resource...
chrome/browser/resources/options/managed_user_list_data.js:49: var i;
On 2014/01/09 13:39:08, Bernhard Bauer wrote:
> Nit: You can inline the var into the for loop.

Right. I am just not used to inline this into the for loop because jslint.com
complains about that, and I used that tool when I started with javascript. I
currently work on another CL which also touches this file, and I can change it
there.

https://codereview.chromium.org/125993002/diff/570002/chrome/browser/resource...
chrome/browser/resources/options/managed_user_list_data.js:79: * @param {Object}
errback the callback function which is called on error.
On 2014/01/09 13:39:08, Bernhard Bauer wrote:
> It might be nice to do the whole thing with DOM futures... :)
> 
> That would actually make some of the behavior really nice, like caching the
> |managed_users_| list (we just create the promise lazily and send out the
> request to the browser when creating it, then keep it around, so future
requests
> will be immediately served by the fulfilled promise). 

I already have the caching in the code. I didn't know about DOM futures, I will
read about them now. Maybe they can make some of the code I have written
obsolete. However one functionality we need is that we can invalidate the cache,
because the underlying data can change.

https://codereview.chromium.org/125993002/diff/570002/chrome/browser/resource...
chrome/browser/resources/options/managed_user_list_data.js:83: var instance =
ManagedUserListData.getInstance();
On 2014/01/09 13:39:08, Bernhard Bauer wrote:
> The usual way to do this is to make this a private instance method, and
forward
> to it from the (public) static method.

Ok, I didn't know this is a required pattern. I can change it.

https://codereview.chromium.org/125993002/diff/570002/chrome/browser/resource...
chrome/browser/resources/options/managed_user_list_data.js:88:
ManagedUserListData.receiveExistingManagedUsers(instance.managedUsers_);
On 2014/01/09 13:39:08, Bernhard Bauer wrote:
> Note that this will not only call |callback|, but also any already existing
> callbacks on |instance|. Couldn't you just directly call |callback|?

I think that if |requestInProgress_| is false, the list of callbacks is empty.
So this will essentially just call |callback|. But you are right, I could just
directly call |callback|, and this would be easier to read.

> Hm... Can we even have more than one callback/errback at any point? If not,
why
> don't we just store them directly on the instance?

Currently I think there can be more than one callback/errback, but I think it
could be avoided. I need to make sure not to add another request call in the
onNameChanged_() method if the previous request was not handled. Also I possibly
need a way to remove the callback if the corresponding overlay was closed,
because then I don't care about the result anymore (but I could then have opened
the other overlay that adds a new callback).
Anyway, before I go forward changing this I want to read up on DOM futures,
maybe this will solve the problem anyway.

https://codereview.chromium.org/125993002/diff/570002/ui/webui/resources/js/u...
File ui/webui/resources/js/util.js (right):

https://codereview.chromium.org/125993002/diff/570002/ui/webui/resources/js/u...
ui/webui/resources/js/util.js:410: return original.substring(0, maxLength - 3) +
'...';
On 2014/01/09 13:39:08, Bernhard Bauer wrote:
> Nit: You should use an ellipsis (…) for slightly nicer typography.

Ok, I will change it.

[Bernhard Bauer, 2014-01-09 14:19:26]

https://codereview.chromium.org/125993002/diff/570002/chrome/browser/resource...
File chrome/browser/resources/options/managed_user_list_data.js (right):

https://codereview.chromium.org/125993002/diff/570002/chrome/browser/resource...
chrome/browser/resources/options/managed_user_list_data.js:79: * @param {Object}
errback the callback function which is called on error.
On 2014/01/09 14:12:42, Adrian Kuegel wrote:
> On 2014/01/09 13:39:08, Bernhard Bauer wrote:
> > It might be nice to do the whole thing with DOM futures... :)
> > 
> > That would actually make some of the behavior really nice, like caching the
> > |managed_users_| list (we just create the promise lazily and send out the
> > request to the browser when creating it, then keep it around, so future
> requests
> > will be immediately served by the fulfilled promise). 
> 
> I already have the caching in the code. I didn't know about DOM futures, I
will
> read about them now. Maybe they can make some of the code I have written
> obsolete. However one functionality we need is that we can invalidate the
cache,
> because the underlying data can change.

We can still do that; just throw away the future.

https://codereview.chromium.org/125993002/diff/570002/chrome/browser/resource...
chrome/browser/resources/options/managed_user_list_data.js:83: var instance =
ManagedUserListData.getInstance();
On 2014/01/09 14:12:42, Adrian Kuegel wrote:
> On 2014/01/09 13:39:08, Bernhard Bauer wrote:
> > The usual way to do this is to make this a private instance method, and
> forward
> > to it from the (public) static method.
> 
> Ok, I didn't know this is a required pattern. I can change it.

It's not required, but this is how it is usually done, and there is something to
be said for consistency. It also avoids having to get the |instance| every time
(and reaching into it to call private methods; calling them on |this| instead
reads nicer to me).

[Adrian Kuegel, 2014-01-09 15:34:16]

https://codereview.chromium.org/125993002/diff/570002/chrome/browser/resource...
File chrome/browser/resources/options/managed_user_list_data.js (right):

https://codereview.chromium.org/125993002/diff/570002/chrome/browser/resource...
chrome/browser/resources/options/managed_user_list_data.js:79: * @param {Object}
errback the callback function which is called on error.
On 2014/01/09 14:19:27, Bernhard Bauer wrote:
> On 2014/01/09 14:12:42, Adrian Kuegel wrote:
> > On 2014/01/09 13:39:08, Bernhard Bauer wrote:
> > > It might be nice to do the whole thing with DOM futures... :)
> > > 
> > > That would actually make some of the behavior really nice, like caching
the
> > > |managed_users_| list (we just create the promise lazily and send out the
> > > request to the browser when creating it, then keep it around, so future
> > requests
> > > will be immediately served by the fulfilled promise). 
> > 
> > I already have the caching in the code. I didn't know about DOM futures, I
> will
> > read about them now. Maybe they can make some of the code I have written
> > obsolete. However one functionality we need is that we can invalidate the
> cache,
> > because the underlying data can change.
> 
> We can still do that; just throw away the future.

From what I have read so far, it looks like it got renamed to Promise, and the
spec is still in development. So maybe we should wait a bit before it is
finalized, and then use it. I saw we have cr.Promise available, but that seems
to be a bit different from what the spec currently says. Anyway, it will
definitely be useful and simplifies the code.

[Bernhard Bauer, 2014-01-09 16:25:11]

https://codereview.chromium.org/125993002/diff/570002/chrome/browser/resource...
File chrome/browser/resources/options/managed_user_list_data.js (right):

https://codereview.chromium.org/125993002/diff/570002/chrome/browser/resource...
chrome/browser/resources/options/managed_user_list_data.js:79: * @param {Object}
errback the callback function which is called on error.
On 2014/01/09 15:34:17, Adrian Kuegel wrote:
> On 2014/01/09 14:19:27, Bernhard Bauer wrote:
> > On 2014/01/09 14:12:42, Adrian Kuegel wrote:
> > > On 2014/01/09 13:39:08, Bernhard Bauer wrote:
> > > > It might be nice to do the whole thing with DOM futures... :)
> > > > 
> > > > That would actually make some of the behavior really nice, like caching
> the
> > > > |managed_users_| list (we just create the promise lazily and send out
the
> > > > request to the browser when creating it, then keep it around, so future
> > > requests
> > > > will be immediately served by the fulfilled promise). 
> > > 
> > > I already have the caching in the code. I didn't know about DOM futures, I
> > will
> > > read about them now. Maybe they can make some of the code I have written
> > > obsolete. However one functionality we need is that we can invalidate the
> > cache,
> > > because the underlying data can change.
> > 
> > We can still do that; just throw away the future.
> 
> From what I have read so far, it looks like it got renamed to Promise, and the
> spec is still in development. So maybe we should wait a bit before it is
> finalized, and then use it. I saw we have cr.Promise available, but that seems
> to be a bit different from what the spec currently says. Anyway, it will
> definitely be useful and simplifies the code.

Promises are available from Chrome 32 on, according to
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje....

[Adrian Kuegel, 2014-01-09 18:05:43]

On 2014/01/09 16:25:11, Bernhard Bauer wrote:
>
https://codereview.chromium.org/125993002/diff/570002/chrome/browser/resource...
> File chrome/browser/resources/options/managed_user_list_data.js (right):
> 
>
https://codereview.chromium.org/125993002/diff/570002/chrome/browser/resource...
> chrome/browser/resources/options/managed_user_list_data.js:79: * @param
{Object}
> errback the callback function which is called on error.
> On 2014/01/09 15:34:17, Adrian Kuegel wrote:
> > On 2014/01/09 14:19:27, Bernhard Bauer wrote:
> > > On 2014/01/09 14:12:42, Adrian Kuegel wrote:
> > > > On 2014/01/09 13:39:08, Bernhard Bauer wrote:
> > > > > It might be nice to do the whole thing with DOM futures... :)
> > > > > 
> > > > > That would actually make some of the behavior really nice, like
caching
> > the
> > > > > |managed_users_| list (we just create the promise lazily and send out
> the
> > > > > request to the browser when creating it, then keep it around, so
future
> > > > requests
> > > > > will be immediately served by the fulfilled promise). 
> > > > 
> > > > I already have the caching in the code. I didn't know about DOM futures,
I
> > > will
> > > > read about them now. Maybe they can make some of the code I have written
> > > > obsolete. However one functionality we need is that we can invalidate
the
> > > cache,
> > > > because the underlying data can change.
> > > 
> > > We can still do that; just throw away the future.
> > 
> > From what I have read so far, it looks like it got renamed to Promise, and
the
> > spec is still in development. So maybe we should wait a bit before it is
> > finalized, and then use it. I saw we have cr.Promise available, but that
seems
> > to be a bit different from what the spec currently says. Anyway, it will
> > definitely be useful and simplifies the code.
> 
> Promises are available from Chrome 32 on, according to
>
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje....

You are right, and I just tried it, it works. I still need to update the
documentation in my CL,
but I will do that tomorrow, and then upload a new patch.

[Bernhard Bauer, 2014-01-09 19:56:58]

On 2014/01/09 18:05:43, Adrian Kuegel wrote:
> On 2014/01/09 16:25:11, Bernhard Bauer wrote:
> >
>
https://codereview.chromium.org/125993002/diff/570002/chrome/browser/resource...
> > File chrome/browser/resources/options/managed_user_list_data.js (right):
> > 
> >
>
https://codereview.chromium.org/125993002/diff/570002/chrome/browser/resource...
> > chrome/browser/resources/options/managed_user_list_data.js:79: * @param
> {Object}
> > errback the callback function which is called on error.
> > On 2014/01/09 15:34:17, Adrian Kuegel wrote:
> > > On 2014/01/09 14:19:27, Bernhard Bauer wrote:
> > > > On 2014/01/09 14:12:42, Adrian Kuegel wrote:
> > > > > On 2014/01/09 13:39:08, Bernhard Bauer wrote:
> > > > > > It might be nice to do the whole thing with DOM futures... :)
> > > > > > 
> > > > > > That would actually make some of the behavior really nice, like
> caching
> > > the
> > > > > > |managed_users_| list (we just create the promise lazily and send
out
> > the
> > > > > > request to the browser when creating it, then keep it around, so
> future
> > > > > requests
> > > > > > will be immediately served by the fulfilled promise). 
> > > > > 
> > > > > I already have the caching in the code. I didn't know about DOM
futures,
> I
> > > > will
> > > > > read about them now. Maybe they can make some of the code I have
written
> > > > > obsolete. However one functionality we need is that we can invalidate
> the
> > > > cache,
> > > > > because the underlying data can change.
> > > > 
> > > > We can still do that; just throw away the future.
> > > 
> > > From what I have read so far, it looks like it got renamed to Promise, and
> the
> > > spec is still in development. So maybe we should wait a bit before it is
> > > finalized, and then use it. I saw we have cr.Promise available, but that
> seems
> > > to be a bit different from what the spec currently says. Anyway, it will
> > > definitely be useful and simplifies the code.
> > 
> > Promises are available from Chrome 32 on, according to
> >
>
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje....
> 
> You are right, and I just tried it, it works. I still need to update the
> documentation in my CL,
> but I will do that tomorrow, and then upload a new patch.

Cool, thanks!