Multi-profile WebUI settings

Multi-profile WebUI settings

BUG=89643, 89736
TEST=go to chrome://settings/personal when running with multi-profiles enabled.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=93461

[binji, 2011-07-19 19:09:29]

jhawkins: please review WebUI stuff
mirandac: please review profile_info_cache, strings
sail: FYI

[sail, 2011-07-19 19:39:50]

LGTM
Awesome!

http://codereview.chromium.org/7400032/diff/9001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_info_cache.cc (right):

http://codereview.chromium.org/7400032/diff/9001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_info_cache.cc:86:
NotificationService::current()->Notify(
Good catch. Can you add this to AddProfileToCache too?

http://codereview.chromium.org/7400032/diff/9001/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/profiles_manage_handler.cc (right):

http://codereview.chromium.org/7400032/diff/9001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/profiles_manage_handler.cc:97: if (profile_index
== std::string::npos) {
no need for braces for single line statements, more below

[James Hawkins, 2011-07-19 20:22:09]

http://codereview.chromium.org/7400032/diff/9001/chrome/browser/resources/opt...
File chrome/browser/resources/options/options.html (right):

http://codereview.chromium.org/7400032/diff/9001/chrome/browser/resources/opt...
chrome/browser/resources/options/options.html:43: <link rel="stylesheet"
href="profiles_manage_overlay.css">
profiles_manage is somewhat strange. Should this be profiles_manager?

http://codereview.chromium.org/7400032/diff/9001/chrome/browser/resources/opt...
File chrome/browser/resources/options/personal_options.css (right):

http://codereview.chromium.org/7400032/diff/9001/chrome/browser/resources/opt...
chrome/browser/resources/options/personal_options.css:21: /* override the
default min-height for a list */
Should remove this comment.

http://codereview.chromium.org/7400032/diff/9001/chrome/browser/resources/opt...
chrome/browser/resources/options/personal_options.css:26: .profile-item {
Remove empty class.

http://codereview.chromium.org/7400032/diff/9001/chrome/browser/resources/opt...
File chrome/browser/resources/options/personal_options.html (right):

http://codereview.chromium.org/7400032/diff/9001/chrome/browser/resources/opt...
chrome/browser/resources/options/personal_options.html:40: <h3
i18n-content="profiles"></h3>
Remove blank line.

http://codereview.chromium.org/7400032/diff/9001/chrome/browser/resources/opt...
File chrome/browser/resources/options/personal_options.js (right):

http://codereview.chromium.org/7400032/diff/9001/chrome/browser/resources/opt...
chrome/browser/resources/options/personal_options.js:37: // sync
Sync.

Capitalize comment sentences (or word) and add period, here and elsewhere.

http://codereview.chromium.org/7400032/diff/9001/chrome/browser/resources/opt...
chrome/browser/resources/options/personal_options.js:56:
profilesList.addEventListener('change', function(event) {
onchange = function(...

http://codereview.chromium.org/7400032/diff/9001/chrome/browser/resources/opt...
chrome/browser/resources/options/personal_options.js:247:
chrome.send('profilesDelete', [profileInfo.filePath]);
rename: deleteProfile

http://codereview.chromium.org/7400032/diff/9001/chrome/browser/resources/opt...
chrome/browser/resources/options/personal_options.js:253: * @param {number} The
number of profiles to display.
@param {number} numProfiles The...

http://codereview.chromium.org/7400032/diff/9001/chrome/browser/resources/opt...
chrome/browser/resources/options/personal_options.js:256:
$('profiles-list').hidden = numProfiles <= 1;
You can do the same for profiles-manage and profiles-delete as well.

http://codereview.chromium.org/7400032/diff/9001/chrome/browser/resources/opt...
chrome/browser/resources/options/personal_options.js:268: * Called by the C++
handler. Adds all |profiles| to the list.
No need to specify the first sentence.

http://codereview.chromium.org/7400032/diff/9001/chrome/browser/resources/opt...
chrome/browser/resources/options/personal_options.js:270: *     each object is
like:
s/like/of the form/

http://codereview.chromium.org/7400032/diff/9001/chrome/browser/resources/opt...
File chrome/browser/resources/options/profiles_manage_overlay.css (right):

http://codereview.chromium.org/7400032/diff/9001/chrome/browser/resources/opt...
chrome/browser/resources/options/profiles_manage_overlay.css:15: padding: 0px;
s/px//

0px -> 0 here and elsewhere.

http://codereview.chromium.org/7400032/diff/9001/chrome/browser/resources/opt...
chrome/browser/resources/options/profiles_manage_overlay.css:39:
-webkit-box-orient: horizontal;
-webkit properties should be at the top.

http://codereview.chromium.org/7400032/diff/9001/chrome/browser/resources/opt...
File chrome/browser/resources/options/profiles_manage_overlay.html (right):

http://codereview.chromium.org/7400032/diff/9001/chrome/browser/resources/opt...
chrome/browser/resources/options/profiles_manage_overlay.html:2: <!-- dialog for
managing profiles -->
Dialog...profiles.

http://codereview.chromium.org/7400032/diff/9001/chrome/browser/resources/opt...
chrome/browser/resources/options/profiles_manage_overlay.html:9: <input
id="profiles-manage-name" type="text"></input>
input element does not have closing tag.

http://codereview.chromium.org/7400032/diff/9001/chrome/browser/resources/opt...
chrome/browser/resources/options/profiles_manage_overlay.html:12: class="hiding"
i18n-content="profilesManageDuplicateNameError">
What is the 'hiding' class? I can't find it anywhere.

http://codereview.chromium.org/7400032/diff/9001/chrome/browser/resources/opt...
chrome/browser/resources/options/profiles_manage_overlay.html:21:
i18n-content="profilesManageOK"></button>
Use the options-global OK and cancel strings.

http://codereview.chromium.org/7400032/diff/9001/chrome/browser/resources/opt...
File chrome/browser/resources/options/profiles_manage_overlay.js (right):

http://codereview.chromium.org/7400032/diff/9001/chrome/browser/resources/opt...
chrome/browser/resources/options/profiles_manage_overlay.js:13: * Encapsulated
handling of the 'Manage profile...' overlay page.
@constructor

http://codereview.chromium.org/7400032/diff/9001/chrome/browser/resources/opt...
chrome/browser/resources/options/profiles_manage_overlay.js:121: *     was
valid.
s/was/were/

http://codereview.chromium.org/7400032/diff/9001/chrome/browser/resources/opt...
chrome/browser/resources/options/profiles_manage_overlay.js:171:
ProfilesManageOverlay.show = function(profileInfo) {
Move to a private implementation and forward.

http://codereview.chromium.org/7400032/diff/9001/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/personal_options_handler.h (right):

http://codereview.chromium.org/7400032/diff/9001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/personal_options_handler.h:56: void
SendProfilesInfo();
Document these methods.

http://codereview.chromium.org/7400032/diff/9001/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/profiles_delete_handler.h (right):

http://codereview.chromium.org/7400032/diff/9001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/profiles_delete_handler.h:23: void
ShowDeleteWarnings(const base::ListValue* args);
Document methods.

http://codereview.chromium.org/7400032/diff/9001/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/profiles_manage_handler.h (right):

http://codereview.chromium.org/7400032/diff/9001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/profiles_manage_handler.h:30: void
SendProfileIcons();
Document methods.

[Miranda Callahan, 2011-07-19 20:40:55]

This is great!  A few nitty comments, and agree w/James about the need for more
documentation of methods. Otherwise -- LGTM.

http://codereview.chromium.org/7400032/diff/9001/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/7400032/diff/9001/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:13016: This name is already used!
nit: could you change to "Sorry, this name is already in use!" Both for the
semi-lightening of a small apology to start, and "is in use" is more correct
than "is used".

http://codereview.chromium.org/7400032/diff/9001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_info_cache.cc (right):

http://codereview.chromium.org/7400032/diff/9001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_info_cache.cc:215: bool
ProfileInfoCache::IsDefaultAvatarIconUrl(const std::string& url,
nit: could you put "// static" above this method signature? actually, all of
these static "GetDefault..." methods need this comment.

http://codereview.chromium.org/7400032/diff/9001/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/profiles_manage_handler.cc (right):

http://codereview.chromium.org/7400032/diff/9001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/profiles_manage_handler.cc:63: void
ProfilesManageHandler::SendProfileIcons() {
I feel like this should be changed to "InitializeDefaultProfileIcons", so it's
clear that it's not performing a function analogous to sending the profile names
with every profile info change...

[James Hawkins, 2011-07-19 20:41:51]

http://codereview.chromium.org/7400032/diff/9001/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/7400032/diff/9001/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:13016: This name is already used!
On 2011/07/19 20:40:55, Miranda Callahan wrote:
> nit: could you change to "Sorry, this name is already in use!" Both for the
> semi-lightening of a small apology to start, and "is in use" is more correct
> than "is used".

Technically we're not supposed to apologize or use the word 'sorry' in the UI.

[Miranda Callahan, 2011-07-19 20:47:32]

http://codereview.chromium.org/7400032/diff/9001/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/7400032/diff/9001/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:13016: This name is already used!
On 2011/07/19 20:41:52, James Hawkins wrote:
> On 2011/07/19 20:40:55, Miranda Callahan wrote:
> > nit: could you change to "Sorry, this name is already in use!" Both for the
> > semi-lightening of a small apology to start, and "is in use" is more correct
> > than "is used".
> 
> Technically we're not supposed to apologize or use the word 'sorry' in the UI.

Oh, that's news to me!  Then how about "Whoops, this name is already in use!"

[binji, 2011-07-19 23:12:43]

On 2011/07/19 20:22:09, James Hawkins wrote:
>
http://codereview.chromium.org/7400032/diff/9001/chrome/browser/resources/opt...
> File chrome/browser/resources/options/options.html (right):
> 
>
http://codereview.chromium.org/7400032/diff/9001/chrome/browser/resources/opt...
> chrome/browser/resources/options/options.html:43: <link rel="stylesheet"
> href="profiles_manage_overlay.css">
> profiles_manage is somewhat strange. Should this be profiles_manager?
Well, it's not a manager. It's the handler for the "Manage Profile" overlay.
Maybe profile_manage* is better?


http://codereview.chromium.org/7400032/diff/9001/chrome/browser/resources/opt...
> chrome/browser/resources/options/personal_options.css:21: /* override the
> default min-height for a list */
> Should remove this comment.
done.


http://codereview.chromium.org/7400032/diff/9001/chrome/browser/resources/opt...
> chrome/browser/resources/options/personal_options.css:26: .profile-item {
> Remove empty class.
done.


http://codereview.chromium.org/7400032/diff/9001/chrome/browser/resources/opt...
> chrome/browser/resources/options/personal_options.html:40: <h3
> i18n-content="profiles"></h3>
> Remove blank line.
There isn't actually a blank line. The codereviewer is adding in fake spacing.


>
http://codereview.chromium.org/7400032/diff/9001/chrome/browser/resources/opt...
> chrome/browser/resources/options/personal_options.js:37: // sync
> Sync.
> Capitalize comment sentences (or word) and add period, here and elsewhere.
done.


>
http://codereview.chromium.org/7400032/diff/9001/chrome/browser/resources/opt...
> chrome/browser/resources/options/personal_options.js:56:
> profilesList.addEventListener('change', function(event) {
> onchange = function(...
done.


http://codereview.chromium.org/7400032/diff/9001/chrome/browser/resources/opt...
> chrome/browser/resources/options/personal_options.js:247:
> chrome.send('profilesDelete', [profileInfo.filePath]);
> rename: deleteProfile
done.
Also profilesCreate -> createProfile.


http://codereview.chromium.org/7400032/diff/9001/chrome/browser/resources/opt...
> chrome/browser/resources/options/personal_options.js:253: * @param {number}
The
> number of profiles to display.
> @param {number} numProfiles The...
done.


http://codereview.chromium.org/7400032/diff/9001/chrome/browser/resources/opt...
> chrome/browser/resources/options/personal_options.js:256:
> $('profiles-list').hidden = numProfiles <= 1;
> You can do the same for profiles-manage and profiles-delete as well.
done.


http://codereview.chromium.org/7400032/diff/9001/chrome/browser/resources/opt...
> chrome/browser/resources/options/personal_options.js:268: * Called by the C++
> handler. Adds all |profiles| to the list.
> No need to specify the first sentence.
done.


http://codereview.chromium.org/7400032/diff/9001/chrome/browser/resources/opt...
> chrome/browser/resources/options/personal_options.js:270: *     each object is
> like:
> s/like/of the form/
done.


http://codereview.chromium.org/7400032/diff/9001/chrome/browser/resources/opt...
> chrome/browser/resources/options/profiles_manage_overlay.css:15: padding: 0px;
> s/px//
> 
> 0px -> 0 here and elsewhere.
done.


http://codereview.chromium.org/7400032/diff/9001/chrome/browser/resources/opt...
> chrome/browser/resources/options/profiles_manage_overlay.css:39:
> -webkit-box-orient: horizontal;
> -webkit properties should be at the top.
done.


http://codereview.chromium.org/7400032/diff/9001/chrome/browser/resources/opt...
> chrome/browser/resources/options/profiles_manage_overlay.html:2: <!-- dialog
for
> managing profiles -->
> Dialog...profiles.
done.


http://codereview.chromium.org/7400032/diff/9001/chrome/browser/resources/opt...
> chrome/browser/resources/options/profiles_manage_overlay.html:9: <input
> id="profiles-manage-name" type="text"></input>
> input element does not have closing tag.
done.


http://codereview.chromium.org/7400032/diff/9001/chrome/browser/resources/opt...
> chrome/browser/resources/options/profiles_manage_overlay.html:12:
class="hiding"
> i18n-content="profilesManageDuplicateNameError">
> What is the 'hiding' class? I can't find it anywhere.
it's used in profiles_manage_overlay.css:66 and also
profiles_manage_overlay.js:129,131.
I use it to animate the error message showing/hiding.


http://codereview.chromium.org/7400032/diff/9001/chrome/browser/resources/opt...
> chrome/browser/resources/options/profiles_manage_overlay.html:21:
> i18n-content="profilesManageOK"></button>
> Use the options-global OK and cancel strings.
done.


http://codereview.chromium.org/7400032/diff/9001/chrome/browser/resources/opt...
> chrome/browser/resources/options/profiles_manage_overlay.js:13: * Encapsulated
> handling of the 'Manage profile...' overlay page.
> @constructor
done.


http://codereview.chromium.org/7400032/diff/9001/chrome/browser/resources/opt...
> chrome/browser/resources/options/profiles_manage_overlay.js:121: *     was
> valid.
> s/was/were/
done.


http://codereview.chromium.org/7400032/diff/9001/chrome/browser/resources/opt...
> chrome/browser/resources/options/profiles_manage_overlay.js:171:
> ProfilesManageOverlay.show = function(profileInfo) {
> Move to a private implementation and forward.
done.


http://codereview.chromium.org/7400032/diff/9001/chrome/browser/ui/webui/opti...
> chrome/browser/ui/webui/options/personal_options_handler.h:56: void
> SendProfilesInfo();
> Document these methods.
done.


http://codereview.chromium.org/7400032/diff/9001/chrome/browser/ui/webui/opti...
> chrome/browser/ui/webui/options/profiles_delete_handler.h:23: void
> ShowDeleteWarnings(const base::ListValue* args);
> Document methods.
done. Removed actually, because it wasn't used.


http://codereview.chromium.org/7400032/diff/9001/chrome/browser/ui/webui/opti...
> chrome/browser/ui/webui/options/profiles_manage_handler.h:30: void
> SendProfileIcons();
> Document methods.
done.

[binji, 2011-07-19 23:13:58]

>
http://codereview.chromium.org/7400032/diff/9001/chrome/app/generated_resourc...
> chrome/app/generated_resources.grd:13016: This name is already used!
> nit: could you change to "Sorry, this name is already in use!" Both for the
> semi-lightening of a small apology to start, and "is in use" is more correct
> than "is used".
done.


>
http://codereview.chromium.org/7400032/diff/9001/chrome/browser/profiles/prof...
> chrome/browser/profiles/profile_info_cache.cc:215: bool
> ProfileInfoCache::IsDefaultAvatarIconUrl(const std::string& url,
> nit: could you put "// static" above this method signature? actually, all of
> these static "GetDefault..." methods need this comment.
done.


http://codereview.chromium.org/7400032/diff/9001/chrome/browser/ui/webui/opti...
> chrome/browser/ui/webui/options/profiles_manage_handler.cc:63: void
> ProfilesManageHandler::SendProfileIcons() {
> I feel like this should be changed to "InitializeDefaultProfileIcons", so it's
> clear that it's not performing a function analogous to sending the profile
names
> with every profile info change...
done.

[binji, 2011-07-20 01:18:33]

Removed profiles_delete_handler.*
Merged profiles_delete_handler logic into profiles_manage_handler.
Renamed profiles_manage_* -> manage_profile_*

[James Hawkins, 2011-07-20 01:24:22]

http://codereview.chromium.org/7400032/diff/16002/chrome/browser/resources/op...
File chrome/browser/resources/options/manage_profile_overlay.html (right):

http://codereview.chromium.org/7400032/diff/16002/chrome/browser/resources/op...
chrome/browser/resources/options/manage_profile_overlay.html:1: <div
id="profiles-manage-overlay" class="page" hidden>
Change the ID and similarly-named IDs.

http://codereview.chromium.org/7400032/diff/16002/chrome/browser/resources/op...
File chrome/browser/resources/options/personal_options.html (right):

http://codereview.chromium.org/7400032/diff/16002/chrome/browser/resources/op...
chrome/browser/resources/options/personal_options.html:46: <button
id="profiles-delete" i18n-content="profilesDelete" disabled>
delete-profiles and deleteProfiles?

http://codereview.chromium.org/7400032/diff/16002/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/manage_profile_handler.cc (right):

http://codereview.chromium.org/7400032/diff/16002/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/manage_profile_handler.cc:104: }
Optional: It's optional according to the style guide, but I implore you to
remove braces from single-line logic blocks.

[binji, 2011-07-20 01:56:17]

http://codereview.chromium.org/7400032/diff/16002/chrome/browser/resources/op...
File chrome/browser/resources/options/manage_profile_overlay.html (right):

http://codereview.chromium.org/7400032/diff/16002/chrome/browser/resources/op...
chrome/browser/resources/options/manage_profile_overlay.html:1: <div
id="profiles-manage-overlay" class="page" hidden>
On 2011/07/20 01:24:22, James Hawkins wrote:
> Change the ID and similarly-named IDs.

Done.

http://codereview.chromium.org/7400032/diff/16002/chrome/browser/resources/op...
File chrome/browser/resources/options/personal_options.html (right):

http://codereview.chromium.org/7400032/diff/16002/chrome/browser/resources/op...
chrome/browser/resources/options/personal_options.html:46: <button
id="profiles-delete" i18n-content="profilesDelete" disabled>
On 2011/07/20 01:24:22, James Hawkins wrote:
> delete-profiles and deleteProfiles?

Done.

http://codereview.chromium.org/7400032/diff/16002/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/manage_profile_handler.cc (right):

http://codereview.chromium.org/7400032/diff/16002/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/manage_profile_handler.cc:104: }
On 2011/07/20 01:24:22, James Hawkins wrote:
> Optional: It's optional according to the style guide, but I implore you to
> remove braces from single-line logic blocks.

Done.

[James Hawkins, 2011-07-20 02:00:35]

LGTM. You can probably use this for JS readability.

[binji, 2011-07-20 20:20:40]

James, can you take another look? I changed list->grid.

[James Hawkins, 2011-07-20 20:22:32]

SLGTM

[commit-bot: I haz the power, 2011-07-21 20:17:19]

Change committed as 93461