Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
209393009:
In the "create user" overlay, associate avatars with their default names (Closed)
|
Created:
6 years, 9 months ago by Marc Treib Modified:
6 years, 8 months ago Reviewers:
Pam (message me for reviews) CC:
chromium-reviews, dbeam+watch-options_chromium.org, arv+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionIn the "create user" overlay, properly associate avatars with their
default names, so that choosing another avatar also updates the default
name. However, do not change the new name after the user has edited it.
BUG=177043
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=261367
Patch Set 1 #Patch Set 2 : Fix test & clang build #Patch Set 3 : Added tests. #Patch Set 4 : Cleanup. #
Total comments: 20
Patch Set 5 : Review comments. #
Total comments: 6
Patch Set 6 : More review comments. #
Total comments: 2
Messages
Total messages: 18 (0 generated)
Lots of test failures, but as far as I can tell they're all unrelated. Possible open question: What to do about "generic" profile items, which don't really have a default name. Currently, the suggested name (provided by the ProfileInfoCache) is something like "User1" which is not very pretty.
https://codereview.chromium.org/209393009/diff/110001/chrome/browser/resource... File chrome/browser/resources/options/manage_profile_overlay.js (right): https://codereview.chromium.org/209393009/diff/110001/chrome/browser/resource... chrome/browser/resources/options/manage_profile_overlay.js:30: // Whether the current profile name is a default name for a new profile; For clarity, something like "currently chosen name for a new profile", rather than "current profile name". In most of the settings UI code, the "current profile" is the one in use right now. Also, the comment isn't strictly accurate. I could be randomly assigned the hamburger and get "Pickles", then change it to "Sweetness" (or to "Me" and then back to "Pickles"). Then it is still a default name, maybe even the default name corresponding to this avatar, but this flag will have been cleared. Actually, I tend to think that if the name gets edited back to the default for that avatar, we should treat it as unedited. Let's check with Rachel about that. If so, it removes the need for the flag at all (but adds the need for a method to check the name vs. its avatar). https://codereview.chromium.org/209393009/diff/110001/chrome/browser/resource... chrome/browser/resources/options/manage_profile_overlay.js:37: // An object containing all known profile names. "Known" is now ambiguous. For clarity: something like "all names of existing profiles". https://codereview.chromium.org/209393009/diff/110001/chrome/browser/resource... chrome/browser/resources/options/manage_profile_overlay.js:192: * Sets the name of the currently edited profile. Not just edited, anymore. https://codereview.chromium.org/209393009/diff/110001/chrome/browser/resource... chrome/browser/resources/options/manage_profile_overlay.js:207: * @param {Array.<string>} iconURLs An array of icon URLs. Also document 'mode' https://codereview.chromium.org/209393009/diff/110001/chrome/browser/resource... chrome/browser/resources/options/manage_profile_overlay.js:319: if (newName != oldName) Is this test needed (in the current design)? If the name hasn't been changed, the method shouldn't be called. If it has ever changed been, the flag will be cleared. And since nothing ever re-sets the flag, even if the current edit is changing newName back to oldName, it shouldn't do any harm to clear the flag again. Or am I missing something? https://codereview.chromium.org/209393009/diff/110001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/manage_profile_browsertest.js (right): https://codereview.chromium.org/209393009/diff/110001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/manage_profile_browsertest.js:186: // Also add the names 'Test' and 'Test2' to |existingProfileNames_| to Outdated comment (I guess) -- it's adding 'SameName', not 'Test2'. https://codereview.chromium.org/209393009/diff/110001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/manage_profile_browsertest.js:523: var pageName = mode + 'Profile'; Might as well just use OptionsPage.showPageByName('createProfile'); since the page name isn't used anywhere else. https://codereview.chromium.org/209393009/diff/110001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/manage_profile_browsertest.js:529: } Missing ; https://codereview.chromium.org/209393009/diff/110001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/manage_profile_browsertest.js:551: // If the user manually entered a name, it should not be changed anymore. Also good to test changing the name to one of the other choices in the list, or changing it to something else and then changing it back to the default, before choosing another icon. https://codereview.chromium.org/209393009/diff/110001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/manage_profile_handler.cc (right): https://codereview.chromium.org/209393009/diff/110001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/manage_profile_handler.cc:263: // Next add the default avatar icons. ...and names.
https://codereview.chromium.org/209393009/diff/110001/chrome/browser/resource... File chrome/browser/resources/options/manage_profile_overlay.js (right): https://codereview.chromium.org/209393009/diff/110001/chrome/browser/resource... chrome/browser/resources/options/manage_profile_overlay.js:30: // Whether the current profile name is a default name for a new profile; On 2014/03/26 15:58:28, Pam (also PM for reviews) wrote: > For clarity, something like "currently chosen name for a new profile", rather > than "current profile name". In most of the settings UI code, the "current > profile" is the one in use right now. Done. > Also, the comment isn't strictly accurate. I could be randomly assigned the > hamburger and get "Pickles", then change it to "Sweetness" (or to "Me" and then > back to "Pickles"). Then it is still a default name, maybe even the default name > corresponding to this avatar, but this flag will have been cleared. Updated the comment to be more accurate. > Actually, I tend to think that if the name gets edited back to the default for > that avatar, we should treat it as unedited. Let's check with Rachel about that. > If so, it removes the need for the flag at all (but adds the need for a method > to check the name vs. its avatar). I checked with Patrick, who prefers the current behavior, i.e. don't update if it has ever been edited. https://codereview.chromium.org/209393009/diff/110001/chrome/browser/resource... chrome/browser/resources/options/manage_profile_overlay.js:37: // An object containing all known profile names. On 2014/03/26 15:58:28, Pam (also PM for reviews) wrote: > "Known" is now ambiguous. For clarity: something like "all names of existing > profiles". Done. https://codereview.chromium.org/209393009/diff/110001/chrome/browser/resource... chrome/browser/resources/options/manage_profile_overlay.js:192: * Sets the name of the currently edited profile. On 2014/03/26 15:58:28, Pam (also PM for reviews) wrote: > Not just edited, anymore. Updated comment. https://codereview.chromium.org/209393009/diff/110001/chrome/browser/resource... chrome/browser/resources/options/manage_profile_overlay.js:207: * @param {Array.<string>} iconURLs An array of icon URLs. On 2014/03/26 15:58:28, Pam (also PM for reviews) wrote: > Also document 'mode' Done. https://codereview.chromium.org/209393009/diff/110001/chrome/browser/resource... chrome/browser/resources/options/manage_profile_overlay.js:319: if (newName != oldName) On 2014/03/26 15:58:28, Pam (also PM for reviews) wrote: > Is this test needed (in the current design)? If the name hasn't been changed, > the method shouldn't be called. If it has ever changed been, the flag will be > cleared. And since nothing ever re-sets the flag, even if the current edit is > changing newName back to oldName, it shouldn't do any harm to clear the flag > again. Or am I missing something? Correct, the test isn't necessary. Removed. https://codereview.chromium.org/209393009/diff/110001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/manage_profile_browsertest.js (right): https://codereview.chromium.org/209393009/diff/110001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/manage_profile_browsertest.js:186: // Also add the names 'Test' and 'Test2' to |existingProfileNames_| to On 2014/03/26 15:58:28, Pam (also PM for reviews) wrote: > Outdated comment (I guess) -- it's adding 'SameName', not 'Test2'. Done. https://codereview.chromium.org/209393009/diff/110001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/manage_profile_browsertest.js:523: var pageName = mode + 'Profile'; On 2014/03/26 15:58:28, Pam (also PM for reviews) wrote: > Might as well just use > OptionsPage.showPageByName('createProfile'); > since the page name isn't used anywhere else. Done. https://codereview.chromium.org/209393009/diff/110001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/manage_profile_browsertest.js:529: } On 2014/03/26 15:58:28, Pam (also PM for reviews) wrote: > Missing ; Done. https://codereview.chromium.org/209393009/diff/110001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/manage_profile_browsertest.js:551: // If the user manually entered a name, it should not be changed anymore. On 2014/03/26 15:58:28, Pam (also PM for reviews) wrote: > Also good to test changing the name to one of the other choices in the list, or > changing it to something else and then changing it back to the default, before > choosing another icon. Split the test into three, which collectively cover the mentioned cases. https://codereview.chromium.org/209393009/diff/110001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/manage_profile_handler.cc (right): https://codereview.chromium.org/209393009/diff/110001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/manage_profile_handler.cc:263: // Next add the default avatar icons. On 2014/03/26 15:58:28, Pam (also PM for reviews) wrote: > ...and names. Done.
https://codereview.chromium.org/209393009/diff/180001/chrome/browser/resource... File chrome/browser/resources/options/manage_profile_overlay.js (right): https://codereview.chromium.org/209393009/diff/180001/chrome/browser/resource... chrome/browser/resources/options/manage_profile_overlay.js:163: self.onNameChanged_(mode); You didn't add this, but at a quick glance it's not clear to me why we have onNameChanged_ called oninput here, and also onchange in the CreateProfileOverlay. Which event is the right one? And can the latter call be removed? https://codereview.chromium.org/209393009/diff/180001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/manage_profile_browsertest.js (right): https://codereview.chromium.org/209393009/diff/180001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/manage_profile_browsertest.js:545: // Select icon without an associated name; the profile name shouldn't change. For thoroughness, test selecting a second icon and check that the name is updated again. (This makes sure that nothing in the programmatic-name-change process marks it as user-modified.) https://codereview.chromium.org/209393009/diff/180001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/manage_profile_browsertest.js:608: expectEquals(defaultProfile.iconURL, gridEl.selectedItem); From here up is identical among the three tests (or could be, with both 'Another Name' and '') and should be extracted into a common method in ManageProfileUITest.
https://codereview.chromium.org/209393009/diff/180001/chrome/browser/resource... File chrome/browser/resources/options/manage_profile_overlay.js (right): https://codereview.chromium.org/209393009/diff/180001/chrome/browser/resource... chrome/browser/resources/options/manage_profile_overlay.js:163: self.onNameChanged_(mode); On 2014/03/31 14:01:45, Pam (also PM for reviews) wrote: > You didn't add this, but at a quick glance it's not clear to me why we have > onNameChanged_ called oninput here, and also onchange in the > CreateProfileOverlay. Which event is the right one? And can the latter call be > removed? The call from $('create-profile-managed').onchange is to update (show/hide) the "import" link for existing supervised users. However, in that case we do not want to clear the profileNameIsDefault_ flag. I have fixed this, and (hopefully) made the whole thing a bit clearer. https://codereview.chromium.org/209393009/diff/180001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/manage_profile_browsertest.js (right): https://codereview.chromium.org/209393009/diff/180001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/manage_profile_browsertest.js:545: // Select icon without an associated name; the profile name shouldn't change. On 2014/03/31 14:01:45, Pam (also PM for reviews) wrote: > For thoroughness, test selecting a second icon and check that the name is > updated again. (This makes sure that nothing in the programmatic-name-change > process marks it as user-modified.) Done. https://codereview.chromium.org/209393009/diff/180001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/manage_profile_browsertest.js:608: expectEquals(defaultProfile.iconURL, gridEl.selectedItem); On 2014/03/31 14:01:45, Pam (also PM for reviews) wrote: > From here up is identical among the three tests (or could be, with both 'Another > Name' and '') and should be extracted into a common method in > ManageProfileUITest. Done.
LGTM! I did point out two nits in the tests, but changing them is optional. - Pam https://codereview.chromium.org/209393009/diff/200001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/manage_profile_browsertest.js (right): https://codereview.chromium.org/209393009/diff/200001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/manage_profile_browsertest.js:567: assertEquals('', this.defaultNames[2]); This isn't really necessary. If the test data is changed, the test will fail even without this line. https://codereview.chromium.org/209393009/diff/200001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/manage_profile_browsertest.js:572: assertNotEquals('', this.defaultNames[1]); Same here.
On 2014/04/02 10:43:23, Pam (also PM for reviews) wrote: > LGTM! I did point out two nits in the tests, but changing them is optional. > > - Pam > > https://codereview.chromium.org/209393009/diff/200001/chrome/browser/ui/webui... > File chrome/browser/ui/webui/options/manage_profile_browsertest.js (right): > > https://codereview.chromium.org/209393009/diff/200001/chrome/browser/ui/webui... > chrome/browser/ui/webui/options/manage_profile_browsertest.js:567: > assertEquals('', this.defaultNames[2]); > This isn't really necessary. If the test data is changed, the test will fail > even without this line. > > https://codereview.chromium.org/209393009/diff/200001/chrome/browser/ui/webui... > chrome/browser/ui/webui/options/manage_profile_browsertest.js:572: > assertNotEquals('', this.defaultNames[1]); > Same here. True. My intention was to make the code easier to read, since the definition of the default data is now "far away". (You could argue that the comments already make it clear, but still...)
The CQ bit was checked by treib@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/treib@chromium.org/209393009/200001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on android_aosp for step(s) compile http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_ao...
The CQ bit was checked by treib@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/treib@chromium.org/209393009/200001
The CQ bit was unchecked by commit-bot@chromium.org
Commit queue rejected this change because the description was changed between the time the change entered the commit queue and the time it was ready to commit. You can safely check the commit box again.
The CQ bit was checked by treib@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/treib@chromium.org/209393009/200001
Message was sent while issue was closed.
Change committed as 261367 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
