Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
132013002:
Replace own callback handling with Promises. (Closed)
|
Created:
6 years, 11 months ago by Adrian Kuegel Modified:
6 years, 11 months ago Reviewers:
Bernhard Bauer CC:
chromium-reviews, dbeam+watch-options_chromium.org, tfarina, pam+watch_chromium.org, arv+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionReplace own callback handling with Promises.
Simplify the code by using DOM Promises.
BUG=282464
TEST=browser_tests
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=245264
Patch Set 1 #
Total comments: 4
Patch Set 2 : Use Promise. #Patch Set 3 : Fix tests. #
Total comments: 16
Patch Set 4 : Address review comments. #
Total comments: 4
Patch Set 5 : Change tests to async. #
Total comments: 4
Patch Set 6 : Remove name conflict checks. #
Total comments: 4
Patch Set 7 : Address nits. #
Messages
Total messages: 25 (0 generated)
Hi Bernhard, can you please review this CL? I did some of the changes you suggested for the CL I committed today. I didn't change the elide function, because that is not possible (at least not with the way it is used). This function is used to shorten strings, not HTML. Ok, I could add the character in the code instead of the HTML, but I am not sure this would work properly.
On 2014/01/09 16:18:28, Adrian Kuegel wrote: > Hi Bernhard, > > can you please review this CL? I did some of the changes you suggested for the > CL I committed today. I didn't change the elide function, because that is not > possible (at least not with the way it is used). This function is used to > shorten strings, not HTML. Ok, I could add the character in the code instead of > the HTML, but I am not sure this would work properly. Oh, you escape the elided name to HTML afterwards. Just use \u2026 in that case.
https://codereview.chromium.org/132013002/diff/1/chrome/app/generated_resourc... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/132013002/diff/1/chrome/app/generated_resourc... chrome/app/generated_resources.grd:14708: + (name conflict with existing local profile) Has this string been reviewed? Also, does this work with i18n/RTL? https://codereview.chromium.org/132013002/diff/1/chrome/browser/resources/opt... File chrome/browser/resources/options/managed_user_list_data.js (right): https://codereview.chromium.org/132013002/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/managed_user_list_data.js:83: if (!this.requestInProgress_) { Early-return if a request is in progress?
https://codereview.chromium.org/132013002/diff/1/chrome/app/generated_resourc... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/132013002/diff/1/chrome/app/generated_resourc... chrome/app/generated_resources.grd:14708: + (name conflict with existing local profile) On 2014/01/09 16:33:18, Bernhard Bauer wrote: > Has this string been reviewed? > > Also, does this work with i18n/RTL? No, it had not been reviewed. I just talked with Rachel, and she suggested a better string. But generally we need some better way to handle this case. Pam suggested to discuss this in the next team meeting. Rachel had an interesting idea: we could let the user rename the supervised user to be imported. I think that should work by just changing the local data, and it will be synced automatically. https://codereview.chromium.org/132013002/diff/1/chrome/browser/resources/opt... File chrome/browser/resources/options/managed_user_list_data.js (right): https://codereview.chromium.org/132013002/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/managed_user_list_data.js:83: if (!this.requestInProgress_) { On 2014/01/09 16:33:18, Bernhard Bauer wrote: > Early-return if a request is in progress? This is obsolete now. I already replaced my code with Promises.
> Rachel had an > interesting idea: we could let the user rename the supervised user to be > imported. I think that should work by just changing the local data, and it > will be synced automatically. That's problematic, because we can't guarantee that the new name won't conflict with some other local profile name on another device. Renaming the local profile should be safe though.
On 2014/01/10 10:01:55, Pam (also PM for reviews) wrote: > > Rachel had an > > interesting idea: we could let the user rename the supervised user to be > > imported. I think that should work by just changing the local data, and it > > will be synced automatically. > > That's problematic, because we can't guarantee that the new name won't conflict > with some other local profile name on another device. Renaming the local profile > should be safe though. We would not rename the other local profiles on other devices, just name that will be displayed on the dashboard. And we could not allow to use a name that already exists on the dashboard.
On 2014/01/10 10:11:01, Adrian Kuegel wrote: > On 2014/01/10 10:01:55, Pam (also PM for reviews) wrote: > > > Rachel had an > > > interesting idea: we could let the user rename the supervised user to be > > > imported. I think that should work by just changing the local data, and it > > > will be synced automatically. > > > > That's problematic, because we can't guarantee that the new name won't > conflict > > with some other local profile name on another device. Renaming the local > profile > > should be safe though. > > We would not rename the other local profiles on other devices, just name that > will be displayed on the dashboard. And we could not allow to use a name that > already exists on the dashboard. Suppose I have one desktop machine, with users Alice Bob Charlotte Dora -- supervised user, call it #1 And another machine, with users Alice Bob Dora -- local user, call it #2 Now I try to import Dora #1 onto the second machine, get a name conflict, and decide to rename the supervised user (Dora #1) to "Charlotte". That gets synced back to the first machine, which now has two users named Charlotte. ...or are you proposing that we have the name that's displayed on the dashboard be different from the name that's on the local device for that user, and possibly different from one machine to another? That sounds very confusing.
On 2014/01/10 10:18:35, Pam (also PM for reviews) wrote: > On 2014/01/10 10:11:01, Adrian Kuegel wrote: > > On 2014/01/10 10:01:55, Pam (also PM for reviews) wrote: > > > > Rachel had an > > > > interesting idea: we could let the user rename the supervised user to be > > > > imported. I think that should work by just changing the local data, and it > > > > will be synced automatically. > > > > > > That's problematic, because we can't guarantee that the new name won't > > conflict > > > with some other local profile name on another device. Renaming the local > > profile > > > should be safe though. > > > > We would not rename the other local profiles on other devices, just name that > > will be displayed on the dashboard. And we could not allow to use a name that > > already exists on the dashboard. > > Suppose I have one desktop machine, with users > Alice > Bob > Charlotte > Dora -- supervised user, call it #1 > > And another machine, with users > Alice > Bob > Dora -- local user, call it #2 > > Now I try to import Dora #1 onto the second machine, get a name conflict, and > decide to rename the supervised user (Dora #1) to "Charlotte". That gets synced > back to the first machine, which now has two users named Charlotte. > > ...or are you proposing that we have the name that's displayed on the dashboard > be different from the name that's on the local device for that user, and > possibly different from one machine to another? That sounds very confusing. Yes, that was what I was proposing. Currently we would not have a way to sync the name changes to local profiles, anyway. We can just affect the names in the list of managed users that is also used for the dashboard. But I agree, it can be confusing if that differs.
On 2014/01/10 10:23:54, Adrian Kuegel wrote: > On 2014/01/10 10:18:35, Pam (also PM for reviews) wrote: > > On 2014/01/10 10:11:01, Adrian Kuegel wrote: > > > On 2014/01/10 10:01:55, Pam (also PM for reviews) wrote: > > > > > Rachel had an > > > > > interesting idea: we could let the user rename the supervised user to be > > > > > imported. I think that should work by just changing the local data, and > it > > > > > will be synced automatically. > > > > > > > > That's problematic, because we can't guarantee that the new name won't > > > conflict > > > > with some other local profile name on another device. Renaming the local > > > profile > > > > should be safe though. > > > > > > We would not rename the other local profiles on other devices, just name > that > > > will be displayed on the dashboard. And we could not allow to use a name > that > > > already exists on the dashboard. > > > > Suppose I have one desktop machine, with users > > Alice > > Bob > > Charlotte > > Dora -- supervised user, call it #1 > > > > And another machine, with users > > Alice > > Bob > > Dora -- local user, call it #2 > > > > Now I try to import Dora #1 onto the second machine, get a name conflict, and > > decide to rename the supervised user (Dora #1) to "Charlotte". That gets > synced > > back to the first machine, which now has two users named Charlotte. > > > > ...or are you proposing that we have the name that's displayed on the > dashboard > > be different from the name that's on the local device for that user, and > > possibly different from one machine to another? That sounds very confusing. > > Yes, that was what I was proposing. Currently we would not have a way to sync > the name changes to local profiles, anyway. We can just affect the names in the > list of managed users that is also used for the dashboard. But I agree, it can > be confusing if that differs. Seems like it would be easier to just rename the local profile that conflicts, moving it out of the way for the imported one so the latter can stay consistent everywhere.
On 2014/01/10 10:25:54, Pam (also PM for reviews) wrote: > On 2014/01/10 10:23:54, Adrian Kuegel wrote: > > On 2014/01/10 10:18:35, Pam (also PM for reviews) wrote: > > > On 2014/01/10 10:11:01, Adrian Kuegel wrote: > > > > On 2014/01/10 10:01:55, Pam (also PM for reviews) wrote: > > > > > > Rachel had an > > > > > > interesting idea: we could let the user rename the supervised user to > be > > > > > > imported. I think that should work by just changing the local data, > and > > it > > > > > > will be synced automatically. > > > > > > > > > > That's problematic, because we can't guarantee that the new name won't > > > > conflict > > > > > with some other local profile name on another device. Renaming the local > > > > profile > > > > > should be safe though. > > > > > > > > We would not rename the other local profiles on other devices, just name > > that > > > > will be displayed on the dashboard. And we could not allow to use a name > > that > > > > already exists on the dashboard. > > > > > > Suppose I have one desktop machine, with users > > > Alice > > > Bob > > > Charlotte > > > Dora -- supervised user, call it #1 > > > > > > And another machine, with users > > > Alice > > > Bob > > > Dora -- local user, call it #2 > > > > > > Now I try to import Dora #1 onto the second machine, get a name conflict, > and > > > decide to rename the supervised user (Dora #1) to "Charlotte". That gets > > synced > > > back to the first machine, which now has two users named Charlotte. > > > > > > ...or are you proposing that we have the name that's displayed on the > > dashboard > > > be different from the name that's on the local device for that user, and > > > possibly different from one machine to another? That sounds very confusing. > > > > Yes, that was what I was proposing. Currently we would not have a way to sync > > the name changes to local profiles, anyway. We can just affect the names in > the > > list of managed users that is also used for the dashboard. But I agree, it can > > be confusing if that differs. > > Seems like it would be easier to just rename the local profile that conflicts, > moving it out of the way for the imported one so the latter can stay consistent > everywhere. Yes, but we don't want to allow one user to change the name of another user, right? So technically it would be possible of course, the question is if it is possible from a privacy/product perspective.
On Fri, Jan 10, 2014 at 10:23 AM, <akuegel@chromium.org> wrote: > On 2014/01/10 10:18:35, Pam (also PM for reviews) wrote: > >> On 2014/01/10 10:11:01, Adrian Kuegel wrote: >> > On 2014/01/10 10:01:55, Pam (also PM for reviews) wrote: >> > > > Rachel had an >> > > > interesting idea: we could let the user rename the supervised user >> to be >> > > > imported. I think that should work by just changing the local data, >> and >> > it > >> > > > will be synced automatically. >> > > >> > > That's problematic, because we can't guarantee that the new name won't >> > conflict >> > > with some other local profile name on another device. Renaming the >> local >> > profile >> > > should be safe though. >> > >> > We would not rename the other local profiles on other devices, just name >> > that > >> > will be displayed on the dashboard. And we could not allow to use a name >> > that > >> > already exists on the dashboard. >> > > Suppose I have one desktop machine, with users >> Alice >> Bob >> Charlotte >> Dora -- supervised user, call it #1 >> > > And another machine, with users >> Alice >> Bob >> Dora -- local user, call it #2 >> > > Now I try to import Dora #1 onto the second machine, get a name conflict, >> and >> decide to rename the supervised user (Dora #1) to "Charlotte". That gets >> > synced > >> back to the first machine, which now has two users named Charlotte. >> > > ...or are you proposing that we have the name that's displayed on the >> > dashboard > >> be different from the name that's on the local device for that user, and >> possibly different from one machine to another? That sounds very >> confusing. >> > > Yes, that was what I was proposing. Currently we would not have a way to > sync > the name changes to local profiles, anyway. We can do that (we have a ManagedUserSetting that controls the name), but we can only trigger it from the dashboard at the moment. > We can just affect the names in the > list of managed users that is also used for the dashboard. But I agree, it > can > be confusing if that differs. > > https://codereview.chromium.org/132013002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I need to think about this more, but I'm coming to the conclusion that there's no way we can 100% prevent name conflicts between supervised and non-supervised users. So perhaps we should think about a way to mark which are which instead, so there's still a way to tell them apart. We can talk about it more Tuesday, too.
On 2014/01/10 10:42:34, Pam (also PM for reviews) wrote: > I need to think about this more, but I'm coming to the conclusion that there's > no way we can 100% prevent name conflicts between supervised and non-supervised > users. So perhaps we should think about a way to mark which are which instead, > so there's still a way to tell them apart. +1. It seems we are putting a lot of effort in to solve a self-imposed constraint.
Bernhard, can you please take another look? I had to fix the tests so that they work with promises. Also, it looks like maybe there is still a bug in the promise implementation, or I am using it in an unsupported way: I expect if I create a promise and resolve it immediately, and later reuse the promise and call .then(), that it will also resolve immediately. This is not happening the first time, but for further times it seems to happen.
https://codereview.chromium.org/132013002/diff/200001/chrome/browser/resource... File chrome/browser/resources/options/managed_user_list_data.js (right): https://codereview.chromium.org/132013002/diff/200001/chrome/browser/resource... chrome/browser/resources/options/managed_user_list_data.js:16: this.promise_ = null; You could get by without this initialization if you don't compare to null (just use the values directly in the if-statements). https://codereview.chromium.org/132013002/diff/200001/chrome/browser/resource... chrome/browser/resources/options/managed_user_list_data.js:19: this.managedUsers_ = null; This isn't used anymore. https://codereview.chromium.org/132013002/diff/200001/chrome/browser/resource... chrome/browser/resources/options/managed_user_list_data.js:40: if (this.promise_ == null) I'd rather you didn't change the production code to make the test work, but change the test code. To answer your question, promises *always* resolve asynchronously (on the next turn of the event loop, to be exact), not only the first time. https://codereview.chromium.org/132013002/diff/200001/chrome/browser/resource... chrome/browser/resources/options/managed_user_list_data.js:55: this.promise_ = null; You could call resetPromise_() if you already have it. https://codereview.chromium.org/132013002/diff/200001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/manage_profile_browsertest.js (left): https://codereview.chromium.org/132013002/diff/200001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/manage_profile_browsertest.js:155: managedUserListData.managedUsers_ = [ Would this work if you inject a fulfilled promise here? https://codereview.chromium.org/132013002/diff/200001/ui/webui/resources/js/u... File ui/webui/resources/js/util.js (right): https://codereview.chromium.org/132013002/diff/200001/ui/webui/resources/js/u... ui/webui/resources/js/util.js:410: return original.substring(0, maxLength - 3) + '\u2026'; Now that you use a single character for the ellipsis, you can make the string longer. It also might be nice to add a test that verifies that the length of the elided string increases monotonically with the length of the original string (which doesn't happen right now).
https://codereview.chromium.org/132013002/diff/200001/chrome/browser/resource... File chrome/browser/resources/options/managed_user_list_data.js (right): https://codereview.chromium.org/132013002/diff/200001/chrome/browser/resource... chrome/browser/resources/options/managed_user_list_data.js:16: this.promise_ = null; On 2014/01/10 13:52:08, Bernhard Bauer wrote: > You could get by without this initialization if you don't compare to null (just > use the values directly in the if-statements). If I don't initialize those values, would I still list them here? From a readability perspective I think that would make sense, so it is clear which properties the class has. But at the same time this seems weird, because this is probably a no-op. https://codereview.chromium.org/132013002/diff/200001/chrome/browser/resource... chrome/browser/resources/options/managed_user_list_data.js:19: this.managedUsers_ = null; On 2014/01/10 13:52:08, Bernhard Bauer wrote: > This isn't used anymore. Right. I removed it now. https://codereview.chromium.org/132013002/diff/200001/chrome/browser/resource... chrome/browser/resources/options/managed_user_list_data.js:40: if (this.promise_ == null) On 2014/01/10 13:52:08, Bernhard Bauer wrote: > I'd rather you didn't change the production code to make the test work, but > change the test code. Ok, I changed it now. But I think it then makes sense to assert that |promise_| is not null. > To answer your question, promises *always* resolve asynchronously (on the next > turn of the event loop, to be exact), not only the first time. Ok, thanks for the explanation. I guess I then also need to add a few more then() to the test code to make sure it always works. https://codereview.chromium.org/132013002/diff/200001/chrome/browser/resource... chrome/browser/resources/options/managed_user_list_data.js:55: this.promise_ = null; On 2014/01/10 13:52:08, Bernhard Bauer wrote: > You could call resetPromise_() if you already have it. Done. https://codereview.chromium.org/132013002/diff/200001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/manage_profile_browsertest.js (left): https://codereview.chromium.org/132013002/diff/200001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/manage_profile_browsertest.js:155: managedUserListData.managedUsers_ = [ On 2014/01/10 13:52:08, Bernhard Bauer wrote: > Would this work if you inject a fulfilled promise here? Yes, it does. Thanks for the suggestion. https://codereview.chromium.org/132013002/diff/200001/ui/webui/resources/js/u... File ui/webui/resources/js/util.js (right): https://codereview.chromium.org/132013002/diff/200001/ui/webui/resources/js/u... ui/webui/resources/js/util.js:410: return original.substring(0, maxLength - 3) + '\u2026'; On 2014/01/10 13:52:08, Bernhard Bauer wrote: > Now that you use a single character for the ellipsis, you can make the string > longer. Done. > It also might be nice to add a test that verifies that the length of the elided > string increases monotonically with the length of the original string (which > doesn't happen right now). I added a few more cases to the test which checks elision for the managed_user_create_confirm dialog.
https://codereview.chromium.org/132013002/diff/200001/chrome/browser/resource... File chrome/browser/resources/options/managed_user_list_data.js (right): https://codereview.chromium.org/132013002/diff/200001/chrome/browser/resource... chrome/browser/resources/options/managed_user_list_data.js:16: this.promise_ = null; On 2014/01/10 16:52:01, Adrian Kuegel wrote: > On 2014/01/10 13:52:08, Bernhard Bauer wrote: > > You could get by without this initialization if you don't compare to null > (just > > use the values directly in the if-statements). > > If I don't initialize those values, would I still list them here? No, you'd completely remove them here. > From a > readability perspective I think that would make sense, so it is clear which > properties the class has. But at the same time this seems weird, because this is > probably a no-op. Another thing I have seen is to list the default values (null or undef) on the prototype. As soon as you assign a value, it will override the value from the protoype, and it allows you to add JSDoc annotations to the fields. https://codereview.chromium.org/132013002/diff/200001/chrome/browser/resource... chrome/browser/resources/options/managed_user_list_data.js:40: if (this.promise_ == null) On 2014/01/10 16:52:01, Adrian Kuegel wrote: > On 2014/01/10 13:52:08, Bernhard Bauer wrote: > > I'd rather you didn't change the production code to make the test work, but > > change the test code. > > Ok, I changed it now. But I think it then makes sense to assert that |promise_| > is not null. Well, you'll get an exception if it is null, right? > > To answer your question, promises *always* resolve asynchronously (on the next > > turn of the event loop, to be exact), not only the first time. > > Ok, thanks for the explanation. I guess I then also need to add a few more > then() to the test code to make sure it always works. https://codereview.chromium.org/132013002/diff/330001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/manage_profile_browsertest.js (right): https://codereview.chromium.org/132013002/diff/330001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/manage_profile_browsertest.js:179: var promise = new Promise(function(resolve, reject) { This is available as Promise.resolve(...). https://codereview.chromium.org/132013002/diff/330001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/manage_profile_browsertest.js:202: }).then(function() { This will be called immediately afterwards, so you might as well remove it. The reason for that is that `then(f)` itself returns a promise that will be fulfilled with the return value of |f|, or if that return value itself is a promise, when that promise is fulfilled. Savvy? :-) Basically, what it means is that you can chain multiple asynchronous calls by returning a promise in your callback function. If you don't return a promise (as in this case, where you implicitly return undef), the returned promise will be immediately fulfilled, so the next callback will run immediately (where "immediately" means "on the next turn of the event loop"). I suspect what you want to do here is repeatedly change the name, wait for the user list to get updated, then verify properties? In that case, you can simply do the same thing as above: Set the name, get the promise out of the ManagedProfileOverlay instance, and wait on that promise. Also, it's very likely that all of this code doesn't run at all, because the test ends before that (remember that the callbacks only run on the next turn of the event loop!). What you should do here is mark the test as asynchronous (by setting isAsync to true on the test fixture prototype), then calling testDone() at the end when you're finished. Sorry this is getting more complicated, but this way you can actually test the whole thing including the communication with the browser.
https://codereview.chromium.org/132013002/diff/200001/chrome/browser/resource... File chrome/browser/resources/options/managed_user_list_data.js (right): https://codereview.chromium.org/132013002/diff/200001/chrome/browser/resource... chrome/browser/resources/options/managed_user_list_data.js:16: this.promise_ = null; On 2014/01/13 14:33:50, Bernhard Bauer wrote: > On 2014/01/10 16:52:01, Adrian Kuegel wrote: > > On 2014/01/10 13:52:08, Bernhard Bauer wrote: > > > You could get by without this initialization if you don't compare to null > > (just > > > use the values directly in the if-statements). > > > > If I don't initialize those values, would I still list them here? > > No, you'd completely remove them here. Ok, I removed them now. > > From a > > readability perspective I think that would make sense, so it is clear which > > properties the class has. But at the same time this seems weird, because this > is > > probably a no-op. > > Another thing I have seen is to list the default values (null or undef) on the > prototype. As soon as you assign a value, it will override the value from the > protoype, and it allows you to add JSDoc annotations to the fields. Yes, I think I have seen that as well, especially in closure code. However, in Chrome code it seems to be usually not the case that the variables are set to their default value in the constructor. So I decided now not to do this. https://codereview.chromium.org/132013002/diff/200001/chrome/browser/resource... chrome/browser/resources/options/managed_user_list_data.js:40: if (this.promise_ == null) On 2014/01/13 14:33:50, Bernhard Bauer wrote: > On 2014/01/10 16:52:01, Adrian Kuegel wrote: > > On 2014/01/10 13:52:08, Bernhard Bauer wrote: > > > I'd rather you didn't change the production code to make the test work, but > > > change the test code. > > > > Ok, I changed it now. But I think it then makes sense to assert that > |promise_| > > is not null. > > Well, you'll get an exception if it is null, right? > > > > To answer your question, promises *always* resolve asynchronously (on the > next > > > turn of the event loop, to be exact), not only the first time. > > > > Ok, thanks for the explanation. I guess I then also need to add a few more > > then() to the test code to make sure it always works. I am not sure if I would get an exception if |promise_| is null, but |resolve_| is not null (for example after calling resetPromise()). Of course I could also reset |reject_| and |resolve_| in resetPromise(), maybe that is cleaner? What do you think? https://codereview.chromium.org/132013002/diff/330001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/manage_profile_browsertest.js (right): https://codereview.chromium.org/132013002/diff/330001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/manage_profile_browsertest.js:179: var promise = new Promise(function(resolve, reject) { On 2014/01/13 14:33:50, Bernhard Bauer wrote: > This is available as Promise.resolve(...). Cool. Thanks for pointing that out :) https://codereview.chromium.org/132013002/diff/330001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/manage_profile_browsertest.js:202: }).then(function() { On 2014/01/13 14:33:50, Bernhard Bauer wrote: > This will be called immediately afterwards, so you might as well remove it. The > reason for that is that `then(f)` itself returns a promise that will be > fulfilled with the return value of |f|, or if that return value itself is a > promise, when that promise is fulfilled. Savvy? :-) Basically, what it means is > that you can chain multiple asynchronous calls by returning a promise in your > callback function. If you don't return a promise (as in this case, where you > implicitly return undef), the returned promise will be immediately fulfilled, so > the next callback will run immediately (where "immediately" means "on the next > turn of the event loop"). > > I suspect what you want to do here is repeatedly change the name, wait for the > user list to get updated, then verify properties? In that case, you can simply > do the same thing as above: Set the name, get the promise out of the > ManagedProfileOverlay instance, and wait on that promise. > > Also, it's very likely that all of this code doesn't run at all, because the > test ends before that (remember that the callbacks only run on the next turn of > the event loop!). What you should do here is mark the test as asynchronous (by > setting isAsync to true on the test fixture prototype), then calling testDone() > at the end when you're finished. Sorry this is getting more complicated, but > this way you can actually test the whole thing including the communication with > the browser. Thank you very much for the detailed explanation. Makes perfect sense now to me. I changed the test to async, and use a few nested then calls which wait for the promise to resolve after each name change.
https://codereview.chromium.org/132013002/diff/160002/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/manage_profile_browsertest.js (right): https://codereview.chromium.org/132013002/diff/160002/chrome/browser/ui/webui... chrome/browser/ui/webui/options/manage_profile_browsertest.js:101: testDone(); Alternatively, you could make a subclass for the async test. Up to you, really. https://codereview.chromium.org/132013002/diff/160002/chrome/browser/ui/webui... chrome/browser/ui/webui/options/manage_profile_browsertest.js:209: promise.then(function() { You're waiting on the same promise, which is already fulfilled. If the test currently succeeds, that tells me that you're relying on the fact that the promise hasn't been reset in the mean time. I'm not sure if that's a good assumption... In any case, you can get rid of the nested calls (regardless of which promise you end up waiting on) by returning the promise from the callback here.
https://codereview.chromium.org/132013002/diff/160002/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/manage_profile_browsertest.js (right): https://codereview.chromium.org/132013002/diff/160002/chrome/browser/ui/webui... chrome/browser/ui/webui/options/manage_profile_browsertest.js:101: testDone(); On 2014/01/13 17:18:47, Bernhard Bauer wrote: > Alternatively, you could make a subclass for the async test. Up to you, really. Ok, this seems to be the better solution, since we only need the async behavior in one test. So I changed it. https://codereview.chromium.org/132013002/diff/160002/chrome/browser/ui/webui... chrome/browser/ui/webui/options/manage_profile_browsertest.js:209: promise.then(function() { On 2014/01/13 17:18:47, Bernhard Bauer wrote: > You're waiting on the same promise, which is already fulfilled. If the test > currently succeeds, that tells me that you're relying on the fact that the > promise hasn't been reset in the mean time. I'm not sure if that's a good > assumption... You are right, in a normal execution it could have been reset in the mean time. > In any case, you can get rid of the nested calls (regardless of which promise > you end up waiting on) by returning the promise from the callback here. Good idea. Done.
https://codereview.chromium.org/132013002/diff/510001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/manage_profile_browsertest.js (right): https://codereview.chromium.org/132013002/diff/510001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/manage_profile_browsertest.js:143: function ManageProfileAsyncUITest() {} Nit: Async test fixtures are usually named <thing>TestAsync. https://codereview.chromium.org/132013002/diff/510001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/manage_profile_browsertest.js:196: // profiles with that names exist on the device. Nit: "with those names"
https://codereview.chromium.org/132013002/diff/510001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/manage_profile_browsertest.js (right): https://codereview.chromium.org/132013002/diff/510001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/manage_profile_browsertest.js:143: function ManageProfileAsyncUITest() {} On 2014/01/16 15:26:34, Bernhard Bauer wrote: > Nit: Async test fixtures are usually named <thing>TestAsync. Done. https://codereview.chromium.org/132013002/diff/510001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/manage_profile_browsertest.js:196: // profiles with that names exist on the device. On 2014/01/16 15:26:34, Bernhard Bauer wrote: > Nit: "with those names" Done.
LGTM!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/akuegel@chromium.org/132013002/570001
Message was sent while issue was closed.
Change committed as 245264 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
