Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7256002:
Multi-Profiles: New Profile Setup UI (Closed)
|
Created:
9 years, 6 months ago by sail Modified:
9 years, 5 months ago CC:
chromium-reviews, rginda+watch_chromium.org, arv (Not doing code reviews), achuith+watch_chromium.org, Paweł Hajdan Jr. Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionMulti-Profiles: New Profile Setup UI
This change adds DOM UI to setup a new profile. When the user clicks Wrench Menu -> New Profile the following happens:
1 we create a new profile and give it a good default name and icon
2 we open a browser window and navigate it to chrome://newprofile
3 user customizes the name and icon if they wish and then click Create
4 we navigate to the new tab page where we optionally show the Sign In To Sync promo page
This change implements 3 and 4. Screenshots:
- http://www.dropmocks.com/mW4un
BUG=None
TEST=Navigated to chrome://newprofile and verified that things looked ok
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=91573
Patch Set 1 #Patch Set 2 : Diff against parent branch #Patch Set 3 : Remove debug code #Patch Set 4 : show product logo at bottom #
Total comments: 28
Patch Set 5 : address review feedback #Patch Set 6 : reverse button order on windows #Patch Set 7 : address review comments #
Total comments: 63
Patch Set 8 : Address review comments #
Total comments: 17
Patch Set 9 : Address review comments #
Total comments: 9
Patch Set 10 : address review comments #
Total comments: 21
Patch Set 11 : Add todo #Patch Set 12 : Address review comments #
Total comments: 2
Patch Set 13 : Address review comments #Messages
Total messages: 37 (0 generated)
This is great -- mostly nits, but a few issues/questions: 1. The boxes for profile name and profile icon don't line up on the left, and they probably should. 2. The blue background used for showing which icon is selected is really BLUE -- it overwhelms everything. Could this be made at least into the mellower blue color used when selecting text on the page (if not a nice rounded-corner outline)? 3. The buttons at the bottom should line up with the right edge of the icons box. Or, if they line up with the text, the text itself should be justified so it makes a harder right edge. 4. When I open the chrome://newprofile page, the text seems very small on the page. In fact, the whole page seems kind of empty. What if we moved the whole box up to where the incognito text sits? I think that if we had less empty space above the text and boxes it would look less lonely in a giant field of white. http://codereview.chromium.org/7256002/diff/3001/chrome/app/generated_resourc... File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/7256002/diff/3001/chrome/app/generated_resourc... chrome/app/generated_resources.grd:12724: Create New <ph name="SHORT_PRODUCT_NAME">$1<ex>Chrome</ex></ph> Profile Change title to "Create a New...", which sounds less abrupt. http://codereview.chromium.org/7256002/diff/3001/chrome/app/generated_resourc... chrome/app/generated_resources.grd:12731: be used by another person. I would say "For example, you can use one profile, and your friend can use another." This avoids passive voice, and sounds friendlier. http://codereview.chromium.org/7256002/diff/3001/chrome/app/generated_resourc... chrome/app/generated_resources.grd:12734: Choose a name and an avatar then click Create to create a new profile. Add a comma between "avatar" and "then". I would also end the sentence with a friendly "!" instead of a drab "." http://codereview.chromium.org/7256002/diff/3001/chrome/browser/profiles/prof... File chrome/browser/profiles/profile_manager.h (right): http://codereview.chromium.org/7256002/diff/3001/chrome/browser/profiles/prof... chrome/browser/profiles/profile_manager.h:151: ProfileInfoCache& GetProfileInfoCache(); add brief descriptive comment. http://codereview.chromium.org/7256002/diff/3001/chrome/browser/resources/new... File chrome/browser/resources/new_profile.js (right): http://codereview.chromium.org/7256002/diff/3001/chrome/browser/resources/new... chrome/browser/resources/new_profile.js:19: return false; Not sure why we're returning "false" here, and in onCancel? http://codereview.chromium.org/7256002/diff/3001/chrome/browser/resources/new... chrome/browser/resources/new_profile.js:24: return false; see comment above. http://codereview.chromium.org/7256002/diff/3001/chrome/browser/resources/new... chrome/browser/resources/new_profile.js:68: function themeChanged() { Is this for future use? http://codereview.chromium.org/7256002/diff/3001/chrome/browser/resources/new... chrome/browser/resources/new_profile.js:78: return false; again, should this be returning false? http://codereview.chromium.org/7256002/diff/3001/chrome/browser/ui/webui/new_... File chrome/browser/ui/webui/new_profile_dom_handler.cc (right): http://codereview.chromium.org/7256002/diff/3001/chrome/browser/ui/webui/new_... chrome/browser/ui/webui/new_profile_dom_handler.cc:43: g_browser_process->profile_manager()->GetProfileInfoCache(); indent 2 more sp http://codereview.chromium.org/7256002/diff/3001/chrome/browser/ui/webui/new_... chrome/browser/ui/webui/new_profile_dom_handler.cc:76: g_browser_process->profile_manager()->GetProfileInfoCache(); indent 2 more sp http://codereview.chromium.org/7256002/diff/3001/chrome/browser/ui/webui/new_... File chrome/browser/ui/webui/new_profile_dom_handler.h (right): http://codereview.chromium.org/7256002/diff/3001/chrome/browser/ui/webui/new_... chrome/browser/ui/webui/new_profile_dom_handler.h:21: // Callback for the "create" message - finishs creating a profile. s/finishs/finishes http://codereview.chromium.org/7256002/diff/3001/chrome/browser/ui/webui/new_... File chrome/browser/ui/webui/new_profile_ui.cc (right): http://codereview.chromium.org/7256002/diff/3001/chrome/browser/ui/webui/new_... chrome/browser/ui/webui/new_profile_ui.cc:72: l10n_util::GetStringUTF16(IDS_SHORT_PRODUCT_NAME); indent http://codereview.chromium.org/7256002/diff/3001/chrome/browser/ui/webui/new_... chrome/browser/ui/webui/new_profile_ui.cc:79: // and the time now is between these two times, show the custom logo. Is this promo really going to be governed by a time slice? What will the logo look like -- do we have ideas about this?
Similar to Miranda's comments: are there mocks for this UI? I have a lot of concerns about the current look-n-feel.
Another issue -- when I launch an incognito window with your patch installed, I get the regular NTP, but with no thumbnails showing.
New Screenshots: http://www.dropmocks.com/mWxdW > 1. The boxes for profile name and profile icon don't line up on the left, and they probably should. Done. > 2. The blue background used for showing which icon is selected is really BLUE -- it overwhelms everything. Could this be made at least into the mellower blue color used when selecting text on the page (if not a nice rounded-corner outline)? Done. Change to #bbcee9 to match settings-list. I think there are more improvements we could make here, hover effect, rounded corners. If no one minds I'd like to get this and work on improvements once the sync promo work is done. > 3. The buttons at the bottom should line up with the right edge of the icons box. Or, if they line up with the text, the text itself should be justified so it makes a harder right edge. I think it should align with the text at the top. This better matches the dialogs in our DOM settings UI. I don't think justifying the text is a good idea. I don't think we do that anywhere else. I agree though that this looks bad right now. We can definitely keep tweaking this later. > 4. When I open the chrome://newprofile page, the text seems very small on the page. In fact, the whole page seems kind of empty. What if we moved the whole box up to where the incognito text sits? I think that if we had less empty space above the text and boxes it would look less lonely in a giant field of white. Copying the incognito layout seems like a really good idea. Mind if I do that after this is checked in? http://codereview.chromium.org/7256002/diff/3001/chrome/app/generated_resourc... File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/7256002/diff/3001/chrome/app/generated_resourc... chrome/app/generated_resources.grd:12724: Create New <ph name="SHORT_PRODUCT_NAME">$1<ex>Chrome</ex></ph> Profile On 2011/06/27 16:42:08, Miranda Callahan wrote: > Change title to "Create a New...", which sounds less abrupt. Done. http://codereview.chromium.org/7256002/diff/3001/chrome/app/generated_resourc... chrome/app/generated_resources.grd:12731: be used by another person. On 2011/06/27 16:42:08, Miranda Callahan wrote: > I would say "For example, you can use one profile, and your friend can use > another." This avoids passive voice, and sounds friendlier. Done. Ahh, thats much better than my wording. http://codereview.chromium.org/7256002/diff/3001/chrome/app/generated_resourc... chrome/app/generated_resources.grd:12734: Choose a name and an avatar then click Create to create a new profile. On 2011/06/27 16:42:08, Miranda Callahan wrote: > Add a comma between "avatar" and "then". I would also end the sentence with a > friendly "!" instead of a drab "." Done. Added a comma, holding out on the ! though :-P http://codereview.chromium.org/7256002/diff/3001/chrome/browser/profiles/prof... File chrome/browser/profiles/profile_manager.h (right): http://codereview.chromium.org/7256002/diff/3001/chrome/browser/profiles/prof... chrome/browser/profiles/profile_manager.h:151: ProfileInfoCache& GetProfileInfoCache(); On 2011/06/27 16:42:08, Miranda Callahan wrote: > add brief descriptive comment. Done. http://codereview.chromium.org/7256002/diff/3001/chrome/browser/resources/new... File chrome/browser/resources/new_profile.js (right): http://codereview.chromium.org/7256002/diff/3001/chrome/browser/resources/new... chrome/browser/resources/new_profile.js:19: return false; On 2011/06/27 16:42:08, Miranda Callahan wrote: > Not sure why we're returning "false" here, and in onCancel? Done. Oops, left over code from when I was using this directly for the onsubmit handler. http://codereview.chromium.org/7256002/diff/3001/chrome/browser/resources/new... chrome/browser/resources/new_profile.js:24: return false; On 2011/06/27 16:42:08, Miranda Callahan wrote: > see comment above. Done. http://codereview.chromium.org/7256002/diff/3001/chrome/browser/resources/new... chrome/browser/resources/new_profile.js:68: function themeChanged() { On 2011/06/27 16:42:08, Miranda Callahan wrote: > Is this for future use? Yea, I removed this and add a TODO in the .cc code. http://codereview.chromium.org/7256002/diff/3001/chrome/browser/resources/new... chrome/browser/resources/new_profile.js:78: return false; On 2011/06/27 16:42:08, Miranda Callahan wrote: > again, should this be returning false? Yea, this has to return false so that the submit handler doesn't end up doing a post. Using a submit handler is necessary since we want the user to be able to press return to create the profile. This is used in the sync setting DOM UI code as well. http://codereview.chromium.org/7256002/diff/3001/chrome/browser/ui/webui/new_... File chrome/browser/ui/webui/new_profile_dom_handler.cc (right): http://codereview.chromium.org/7256002/diff/3001/chrome/browser/ui/webui/new_... chrome/browser/ui/webui/new_profile_dom_handler.cc:43: g_browser_process->profile_manager()->GetProfileInfoCache(); On 2011/06/27 16:42:08, Miranda Callahan wrote: > indent 2 more sp Done. http://codereview.chromium.org/7256002/diff/3001/chrome/browser/ui/webui/new_... chrome/browser/ui/webui/new_profile_dom_handler.cc:76: g_browser_process->profile_manager()->GetProfileInfoCache(); On 2011/06/27 16:42:08, Miranda Callahan wrote: > indent 2 more sp Done. http://codereview.chromium.org/7256002/diff/3001/chrome/browser/ui/webui/new_... File chrome/browser/ui/webui/new_profile_dom_handler.h (right): http://codereview.chromium.org/7256002/diff/3001/chrome/browser/ui/webui/new_... chrome/browser/ui/webui/new_profile_dom_handler.h:21: // Callback for the "create" message - finishs creating a profile. On 2011/06/27 16:42:08, Miranda Callahan wrote: > s/finishs/finishes Done. http://codereview.chromium.org/7256002/diff/3001/chrome/browser/ui/webui/new_... File chrome/browser/ui/webui/new_profile_ui.cc (right): http://codereview.chromium.org/7256002/diff/3001/chrome/browser/ui/webui/new_... chrome/browser/ui/webui/new_profile_ui.cc:72: l10n_util::GetStringUTF16(IDS_SHORT_PRODUCT_NAME); On 2011/06/27 16:42:08, Miranda Callahan wrote: > indent Done. http://codereview.chromium.org/7256002/diff/3001/chrome/browser/ui/webui/new_... chrome/browser/ui/webui/new_profile_ui.cc:79: // and the time now is between these two times, show the custom logo. On 2011/06/27 16:42:08, Miranda Callahan wrote: > Is this promo really going to be governed by a time slice? What will the logo > look like -- do we have ideas about this? So when designing this UI I thought it would be good to make it match the NTP UI as much as possible. This will make transitioning from this UI to the NTP UI more seamless. I'm not sure what the custom logo business is about. I just copy pasted this code. I definitely want to come back and refactor this with the NTP code. I added a TODO here.
On 2011/06/27 16:44:18, Miranda Callahan wrote: > Another issue -- when I launch an incognito window with your patch installed, I > get the regular NTP, but with no thumbnails showing. Do you mean the avatar icon list is empty? I don't see this on my machine.
On 2011/06/27 17:37:24, sail wrote: > New Screenshots: http://www.dropmocks.com/mWxdW > > > 1. The boxes for profile name and profile icon don't line up on the left, and > they probably should. > > Done. > > > 2. The blue background used for showing which icon is selected is really BLUE > -- it overwhelms everything. Could this be made at least into the mellower blue > color used when selecting text on the page (if not a nice rounded-corner > outline)? > > Done. Change to #bbcee9 to match settings-list. I think there are more > improvements we could make here, hover effect, rounded corners. If no one minds > I'd like to get this and work on improvements once the sync promo work is done. That's fine, and makes sense. > > > 3. The buttons at the bottom should line up with the right edge of the icons > box. Or, if they line up with the text, the text itself should be justified so > it makes a harder right edge. > > I think it should align with the text at the top. This better matches the > dialogs in our DOM settings UI. I don't think justifying the text is a good > idea. I don't think we do that anywhere else. > I agree though that this looks bad right now. We can definitely keep tweaking > this later. Yeah, I hate justified text -- but right now, it's just not reading as aligned, so it looks a bit random. Tweaking later is fine. > > > 4. When I open the chrome://newprofile page, the text seems very small on the > page. In fact, the whole page seems kind of empty. What if we moved the whole > box up to where the incognito text sits? I think that if we had less empty space > above the text and boxes it would look less lonely in a giant field of white. > > Copying the incognito layout seems like a really good idea. Mind if I do that > after this is checked in? Sure, no problem. > > http://codereview.chromium.org/7256002/diff/3001/chrome/app/generated_resourc... > File chrome/app/generated_resources.grd (right): > > http://codereview.chromium.org/7256002/diff/3001/chrome/app/generated_resourc... > chrome/app/generated_resources.grd:12724: Create New <ph > name="SHORT_PRODUCT_NAME">$1<ex>Chrome</ex></ph> Profile > On 2011/06/27 16:42:08, Miranda Callahan wrote: > > Change title to "Create a New...", which sounds less abrupt. > > Done. > > http://codereview.chromium.org/7256002/diff/3001/chrome/app/generated_resourc... > chrome/app/generated_resources.grd:12731: be used by another person. > On 2011/06/27 16:42:08, Miranda Callahan wrote: > > I would say "For example, you can use one profile, and your friend can use > > another." This avoids passive voice, and sounds friendlier. > > Done. Ahh, thats much better than my wording. > > http://codereview.chromium.org/7256002/diff/3001/chrome/app/generated_resourc... > chrome/app/generated_resources.grd:12734: Choose a name and an avatar then click > Create to create a new profile. > On 2011/06/27 16:42:08, Miranda Callahan wrote: > > Add a comma between "avatar" and "then". I would also end the sentence with a > > friendly "!" instead of a drab "." > > Done. > Added a comma, holding out on the ! though :-P > > http://codereview.chromium.org/7256002/diff/3001/chrome/browser/profiles/prof... > File chrome/browser/profiles/profile_manager.h (right): > > http://codereview.chromium.org/7256002/diff/3001/chrome/browser/profiles/prof... > chrome/browser/profiles/profile_manager.h:151: ProfileInfoCache& > GetProfileInfoCache(); > On 2011/06/27 16:42:08, Miranda Callahan wrote: > > add brief descriptive comment. > > Done. > > http://codereview.chromium.org/7256002/diff/3001/chrome/browser/resources/new... > File chrome/browser/resources/new_profile.js (right): > > http://codereview.chromium.org/7256002/diff/3001/chrome/browser/resources/new... > chrome/browser/resources/new_profile.js:19: return false; > On 2011/06/27 16:42:08, Miranda Callahan wrote: > > Not sure why we're returning "false" here, and in onCancel? > > Done. > Oops, left over code from when I was using this directly for the onsubmit > handler. > > http://codereview.chromium.org/7256002/diff/3001/chrome/browser/resources/new... > chrome/browser/resources/new_profile.js:24: return false; > On 2011/06/27 16:42:08, Miranda Callahan wrote: > > see comment above. > > Done. > > http://codereview.chromium.org/7256002/diff/3001/chrome/browser/resources/new... > chrome/browser/resources/new_profile.js:68: function themeChanged() { > On 2011/06/27 16:42:08, Miranda Callahan wrote: > > Is this for future use? > > Yea, I removed this and add a TODO in the .cc code. > > http://codereview.chromium.org/7256002/diff/3001/chrome/browser/resources/new... > chrome/browser/resources/new_profile.js:78: return false; > On 2011/06/27 16:42:08, Miranda Callahan wrote: > > again, should this be returning false? > > Yea, this has to return false so that the submit handler doesn't end up doing a > post. Using a submit handler is necessary since we want the user to be able to > press return to create the profile. > > This is used in the sync setting DOM UI code as well. > > http://codereview.chromium.org/7256002/diff/3001/chrome/browser/ui/webui/new_... > File chrome/browser/ui/webui/new_profile_dom_handler.cc (right): > > http://codereview.chromium.org/7256002/diff/3001/chrome/browser/ui/webui/new_... > chrome/browser/ui/webui/new_profile_dom_handler.cc:43: > g_browser_process->profile_manager()->GetProfileInfoCache(); > On 2011/06/27 16:42:08, Miranda Callahan wrote: > > indent 2 more sp > > Done. > > http://codereview.chromium.org/7256002/diff/3001/chrome/browser/ui/webui/new_... > chrome/browser/ui/webui/new_profile_dom_handler.cc:76: > g_browser_process->profile_manager()->GetProfileInfoCache(); > On 2011/06/27 16:42:08, Miranda Callahan wrote: > > indent 2 more sp > > Done. > > http://codereview.chromium.org/7256002/diff/3001/chrome/browser/ui/webui/new_... > File chrome/browser/ui/webui/new_profile_dom_handler.h (right): > > http://codereview.chromium.org/7256002/diff/3001/chrome/browser/ui/webui/new_... > chrome/browser/ui/webui/new_profile_dom_handler.h:21: // Callback for the > "create" message - finishs creating a profile. > On 2011/06/27 16:42:08, Miranda Callahan wrote: > > s/finishs/finishes > > Done. > > http://codereview.chromium.org/7256002/diff/3001/chrome/browser/ui/webui/new_... > File chrome/browser/ui/webui/new_profile_ui.cc (right): > > http://codereview.chromium.org/7256002/diff/3001/chrome/browser/ui/webui/new_... > chrome/browser/ui/webui/new_profile_ui.cc:72: > l10n_util::GetStringUTF16(IDS_SHORT_PRODUCT_NAME); > On 2011/06/27 16:42:08, Miranda Callahan wrote: > > indent > > Done. > > http://codereview.chromium.org/7256002/diff/3001/chrome/browser/ui/webui/new_... > chrome/browser/ui/webui/new_profile_ui.cc:79: // and the time now is between > these two times, show the custom logo. > On 2011/06/27 16:42:08, Miranda Callahan wrote: > > Is this promo really going to be governed by a time slice? What will the logo > > look like -- do we have ideas about this? > > So when designing this UI I thought it would be good to make it match the NTP UI > as much as possible. This will make transitioning from this UI to the NTP UI > more seamless. > > I'm not sure what the custom logo business is about. I just copy pasted this > code. I definitely want to come back and refactor this with the NTP code. I > added a TODO here. This stuff is specifically for a custom logo that's controlled by a server -- it was developed to show special images on the NTP for special promotions. That's fine if we discuss this in your TODO patch.
On 2011/06/27 17:44:11, sail wrote: > On 2011/06/27 16:44:18, Miranda Callahan wrote: > > Another issue -- when I launch an incognito window with your patch installed, > I > > get the regular NTP, but with no thumbnails showing. > > Do you mean the avatar icon list is empty? I don't see this on my machine. No -- I mean I don't see the incognito message; I see the regular NTP field of eight thumbnails, but with no data.
On 2011/06/27 17:59:23, Miranda Callahan wrote: > On 2011/06/27 17:44:11, sail wrote: > > On 2011/06/27 16:44:18, Miranda Callahan wrote: > > > Another issue -- when I launch an incognito window with your patch > installed, > > I > > > get the regular NTP, but with no thumbnails showing. > > > > Do you mean the avatar icon list is empty? I don't see this on my machine. > > No -- I mean I don't see the incognito message; I see the regular NTP field of > eight thumbnails, but with no data. Hm... are you manually navigating to chrome://newprofile from an incognito window? Ideally this UI should only be accessible from the Wrench Menu -> New Profile command. Is there a way to enforce that. I was thinking that most users won't know about this URL so that would be enough to hide it :-P
On Mon, Jun 27, 2011 at 14:23, <sail@chromium.org> wrote: > On 2011/06/27 17:59:23, Miranda Callahan wrote: >> >> On 2011/06/27 17:44:11, sail wrote: >> > On 2011/06/27 16:44:18, Miranda Callahan wrote: >> > > Another issue -- when I launch an incognito window with your patch >> installed, >> > I >> > > get the regular NTP, but with no thumbnails showing. >> > >> > Do you mean the avatar icon list is empty? I don't see this on my >> > machine. > >> No -- I mean I don't see the incognito message; I see the regular NTP >> field of >> eight thumbnails, but with no data. > > Hm... are you manually navigating to chrome://newprofile from an incognito > window? Nope -- what's happening is: 1. I go to chrome://newprofile 2. I hit ctrl-shift-n to launch a new incognito window 3. I see strange things > > Ideally this UI should only be accessible from the Wrench Menu -> New > Profile > command. Is there a way to enforce that. > > I was thinking that most users won't know about this URL so that would be > enough > to hide it :-P I think you're totally right about that -- I don't think there's any reason to hide it. > > http://codereview.chromium.org/7256002/ >
> 1. I go to chrome://newprofile > 2. I hit ctrl-shift-n to launch a new incognito window > 3. I see strange things Hm... I'm not seeing this at all. This change shouldn't affect the incognito UI at all. I'm building on windows. I'll see if I can reproduce it there.
On 2011/06/27 18:44:07, sail wrote: > > 1. I go to chrome://newprofile > > 2. I hit ctrl-shift-n to launch a new incognito window > > 3. I see strange things > > Hm... I'm not seeing this at all. This change shouldn't affect the incognito UI > at all. > I'm building on windows. I'll see if I can reproduce it there. I've only tried it on windows, which is where I'm seeing it. I have another patch on the same build, but it's related to safe browsing and shouldn't affect incognito at all...
Um, did you accidentally commit this?
On 2011/06/28 19:52:06, James Hawkins wrote: > Um, did you accidentally commit this? Oops, sorry about that, reverted. Miranda figured out that the incognito problem was unrelated to this change. Let me know if you have any more feedback. Thanks!
On 2011/06/28 20:09:59, sail wrote: > On 2011/06/28 19:52:06, James Hawkins wrote: > > Um, did you accidentally commit this? > > Oops, sorry about that, reverted. > > Miranda figured out that the incognito problem was unrelated to this change. Let > me know if you have any more feedback. > > Thanks! I'm really concerned about the UI. I know your intentions are to launch and iterate, but I'd like at least a minimal once-over by a UI-lead. I have my criticisms of the UI in the pic you uploaded, but a UX dev would be able to give you better feedback.
On 2011/06/28 20:11:31, James Hawkins wrote: > On 2011/06/28 20:09:59, sail wrote: > > On 2011/06/28 19:52:06, James Hawkins wrote: > > > Um, did you accidentally commit this? > > > > Oops, sorry about that, reverted. > > > > Miranda figured out that the incognito problem was unrelated to this change. > Let > > me know if you have any more feedback. > > > > Thanks! > > I'm really concerned about the UI. I know your intentions are to launch and > iterate, but I'd like at least a minimal once-over by a UI-lead. I have my > criticisms of the UI in the pic you uploaded, but a UX dev would be able to give > you better feedback. Definitely sounds good. Sending out a UI review request now.
I will withhold comments until the ui review comes back, but I would like to point out ahead of time that the order of the 'continue'/'cancel' buttons should be inverted on linux and mac. See .button-strip in options_page.css.
On 2011/06/28 22:03:02, Evan Stade wrote: > I will withhold comments until the ui review comes back, but I would like to > point out ahead of time that the order of the 'continue'/'cancel' buttons should > be inverted on linux and mac. See .button-strip in options_page.css. I'm just about to fix the ordering. The suggestions from the UI review were: #1 move the labels above the controls #2 remove the vertical centering of the page #3 use default selection on the Create button I'm doing all of the above except #3. Instead of setting focus on the Create button I'm setting focus on the Profile Name text field. Pressing return still causes the form to be submitted so I think this is ok. One thing I haven't been able to do is to get the Escape key to trigger the cancel button. Do you know how I can do that? Thanks!
On 2011/06/28 22:07:21, sail wrote: > On 2011/06/28 22:03:02, Evan Stade wrote: > > I will withhold comments until the ui review comes back, but I would like to > > point out ahead of time that the order of the 'continue'/'cancel' buttons > should > > be inverted on linux and mac. See .button-strip in options_page.css. > > I'm just about to fix the ordering. The suggestions from the UI review were: > #1 move the labels above the controls > #2 remove the vertical centering of the page > #3 use default selection on the Create button > > I'm doing all of the above except #3. Instead of setting focus on the Create > button I'm setting focus on the Profile Name text field. Pressing return still > causes the form to be submitted so I think this is ok. > > One thing I haven't been able to do is to get the Escape key to trigger the > cancel button. Do you know how I can do that? > See options_page.js:750.
Address all comments - layout is like incognito now, horizontally centered but pinned to the top - create and cancel button are now reversed on windows - escape key now cancels the operation - labels are now above controls Screenshots: http://www.dropmocks.com/mW4un
Thanks -- LGTM with one tiny nit. http://codereview.chromium.org/7256002/diff/3001/chrome/browser/resources/new... File chrome/browser/resources/new_profile.js (right): http://codereview.chromium.org/7256002/diff/3001/chrome/browser/resources/new... chrome/browser/resources/new_profile.js:78: return false; On 2011/06/27 17:37:24, sail wrote: > On 2011/06/27 16:42:08, Miranda Callahan wrote: > > again, should this be returning false? > > Yea, this has to return false so that the submit handler doesn't end up doing a > post. Using a submit handler is necessary since we want the user to be able to > press return to create the profile. > > This is used in the sync setting DOM UI code as well. Can you add a comment to just clarify that for future readers of this code, so nobody thinks they're being clever and deletes it? Thanks!
Evan and James could you take another look. I'd like to get this in soon because it's blocking some other people. Thanks. http://codereview.chromium.org/7256002/diff/3001/chrome/browser/resources/new... File chrome/browser/resources/new_profile.js (right): http://codereview.chromium.org/7256002/diff/3001/chrome/browser/resources/new... chrome/browser/resources/new_profile.js:78: return false; On 2011/06/30 22:31:46, Miranda Callahan wrote: > On 2011/06/27 17:37:24, sail wrote: > > On 2011/06/27 16:42:08, Miranda Callahan wrote: > > > again, should this be returning false? > > > > Yea, this has to return false so that the submit handler doesn't end up doing > a > > post. Using a submit handler is necessary since we want the user to be able to > > press return to create the profile. > > > > This is used in the sync setting DOM UI code as well. > > Can you add a comment to just clarify that for future readers of this code, so > nobody thinks they're being clever and deletes it? Thanks! Done.
http://codereview.chromium.org/7256002/diff/21004/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/7256002/diff/21004/chrome/app/generated_resour... chrome/app/generated_resources.grd:12769: <message name="IDS_NEW_PROFILE_SETUP_TITLE" desc="Title of the new profile setup page."> You'll need to take non-title case into account. Search for "pp_ifdef('use_titlecase')". http://codereview.chromium.org/7256002/diff/21004/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_info_cache.cc (right): http://codereview.chromium.org/7256002/diff/21004/chrome/browser/profiles/pro... chrome/browser/profiles/profile_info_cache.cc:178: int icon_index = index; Why are you using a temp int? http://codereview.chromium.org/7256002/diff/21004/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_manager.h (right): http://codereview.chromium.org/7256002/diff/21004/chrome/browser/profiles/pro... chrome/browser/profiles/profile_manager.h:212: scoped_ptr<ProfileInfoCache> profile_info_cache_; Document this member variable. http://codereview.chromium.org/7256002/diff/21004/chrome/browser/resources/ne... File chrome/browser/resources/new_profile.css (right): http://codereview.chromium.org/7256002/diff/21004/chrome/browser/resources/ne... chrome/browser/resources/new_profile.css:2: font-family: segoe ui, arial, helvetica, sans-serif; Put the font family names in single quotes. http://codereview.chromium.org/7256002/diff/21004/chrome/browser/resources/ne... chrome/browser/resources/new_profile.css:7: width: 337px; Alphabetize properties. http://codereview.chromium.org/7256002/diff/21004/chrome/browser/resources/ne... chrome/browser/resources/new_profile.css:27: color: rgb(75, 75, 75); Use #aabbcc instead. http://codereview.chromium.org/7256002/diff/21004/chrome/browser/resources/ne... chrome/browser/resources/new_profile.css:87: -webkit-box-align: center; Move webkit property to the top of the block. http://codereview.chromium.org/7256002/diff/21004/chrome/browser/resources/ne... File chrome/browser/resources/new_profile.html (right): http://codereview.chromium.org/7256002/diff/21004/chrome/browser/resources/ne... chrome/browser/resources/new_profile.html:13: Remove blank lines. http://codereview.chromium.org/7256002/diff/21004/chrome/browser/resources/ne... chrome/browser/resources/new_profile.html:14: <div id="overlay" class="overlay"> ID and class name the same? Technically it's OK, but I'd rather you make the ID more descriptive. http://codereview.chromium.org/7256002/diff/21004/chrome/browser/resources/ne... chrome/browser/resources/new_profile.html:21: <table> This is not tabular data; don't use a table. http://codereview.chromium.org/7256002/diff/21004/chrome/browser/resources/ne... chrome/browser/resources/new_profile.html:32: <br/> Don't use <br>; use proper styling adjustments. http://codereview.chromium.org/7256002/diff/21004/chrome/browser/resources/ne... chrome/browser/resources/new_profile.html:37: i18n-values="value:cancelProfile" /> <input> does not have an end tag, thus remove the last forward slash. http://codereview.chromium.org/7256002/diff/21004/chrome/browser/resources/ne... chrome/browser/resources/new_profile.html:42: </div> <!-- page --> Remove these end comments. http://codereview.chromium.org/7256002/diff/21004/chrome/browser/resources/ne... chrome/browser/resources/new_profile.html:51: <script src="chrome://newprofile/new_profile.js"></script> Move these scripts to the top. http://codereview.chromium.org/7256002/diff/21004/chrome/browser/resources/ne... File chrome/browser/resources/new_profile.js (right): http://codereview.chromium.org/7256002/diff/21004/chrome/browser/resources/ne... chrome/browser/resources/new_profile.js:5: var gSelectedAvatarIconIndex = 0; Document this variable. http://codereview.chromium.org/7256002/diff/21004/chrome/browser/resources/ne... chrome/browser/resources/new_profile.js:8: // Helper functions Use a more descriptive name or remove the comment. http://codereview.chromium.org/7256002/diff/21004/chrome/browser/resources/ne... chrome/browser/resources/new_profile.js:9: function $(o) {return document.getElementById(o);} Haven't you already included cr.js? This is already provided. http://codereview.chromium.org/7256002/diff/21004/chrome/browser/resources/ne... chrome/browser/resources/new_profile.js:18: chrome.send('requestProfileInfo', []); No need for the empty last param, here and elsewhere. http://codereview.chromium.org/7256002/diff/21004/chrome/browser/resources/ne... chrome/browser/resources/new_profile.js:33: var menu = document.getElementById("avatar-menu"); $('') http://codereview.chromium.org/7256002/diff/21004/chrome/browser/resources/ne... chrome/browser/resources/new_profile.js:37: button.setAttribute("style", "background-color: #bbcee9"); You should use a class that specifies this instead. http://codereview.chromium.org/7256002/diff/21004/chrome/browser/resources/ne... chrome/browser/resources/new_profile.js:47: if (document.documentElement.getAttribute('customlogo') == 'true') $('') http://codereview.chromium.org/7256002/diff/21004/chrome/browser/resources/ne... chrome/browser/resources/new_profile.js:54: // Close the top overlay or sub-page on esc. You copied this comment from options_page.js and forgot to remove it. Also, have you verified with UI leads that pressing escape should close this page? That seems unintuitive. http://codereview.chromium.org/7256002/diff/21004/chrome/browser/resources/ne... chrome/browser/resources/new_profile.js:69: var menu = document.getElementById("avatar-menu"); $('') http://codereview.chromium.org/7256002/diff/21004/chrome/browser/resources/ne... chrome/browser/resources/new_profile.js:82: $('create-button').onclick = function () { onCreate(''); }; Do all of this work in load(). http://codereview.chromium.org/7256002/diff/21004/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chrome_web_ui_data_source.cc (right): http://codereview.chromium.org/7256002/diff/21004/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chrome_web_ui_data_source.cc:31: void ChromeWebUIDataSource::AddLocalizedString( What's wrong with AddString()? http://codereview.chromium.org/7256002/diff/21004/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chrome_web_ui_factory.cc (right): http://codereview.chromium.org/7256002/diff/21004/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chrome_web_ui_factory.cc:29: #include "chrome/browser/ui/webui/new_profile_ui.h" If this is only used for OS_CHROMEOS, move this include to an ifdef block. http://codereview.chromium.org/7256002/diff/21004/chrome/browser/ui/webui/new... File chrome/browser/ui/webui/new_profile_dom_handler.h (right): http://codereview.chromium.org/7256002/diff/21004/chrome/browser/ui/webui/new... chrome/browser/ui/webui/new_profile_dom_handler.h:13: // The handler for Javascript messages related to the "new profile" view. s/view/page/ http://codereview.chromium.org/7256002/diff/21004/chrome/browser/ui/webui/new... chrome/browser/ui/webui/new_profile_dom_handler.h:31: void SendDefaultAvatarImages(); Document these methods. http://codereview.chromium.org/7256002/diff/21004/chrome/browser/ui/webui/new... File chrome/browser/ui/webui/new_profile_ui.cc (right): http://codereview.chromium.org/7256002/diff/21004/chrome/browser/ui/webui/new... chrome/browser/ui/webui/new_profile_ui.cc:25: static const char kNewProfileJsFile[] = "new_profile.js"; Move to anonymous namespace and remove static. http://codereview.chromium.org/7256002/diff/21004/chrome/browser/ui/webui/new... chrome/browser/ui/webui/new_profile_ui.cc:29: // Is the current time within a given date range? Rephrase this question as a declarative about what the function does. I thought this was a TODO at first. http://codereview.chromium.org/7256002/diff/21004/chrome/browser/ui/webui/new... chrome/browser/ui/webui/new_profile_ui.cc:63: AddLocalizedString("title", IDS_NEW_PROFILE_SETUP_TITLE); These should be added in the MessageHandler.
http://codereview.chromium.org/7256002/diff/21004/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/7256002/diff/21004/chrome/app/generated_resour... chrome/app/generated_resources.grd:12769: <message name="IDS_NEW_PROFILE_SETUP_TITLE" desc="Title of the new profile setup page."> On 2011/06/30 23:02:58, James Hawkins wrote: > You'll need to take non-title case into account. > > Search for "pp_ifdef('use_titlecase')". Done. http://codereview.chromium.org/7256002/diff/21004/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_info_cache.cc (right): http://codereview.chromium.org/7256002/diff/21004/chrome/browser/profiles/pro... chrome/browser/profiles/profile_info_cache.cc:178: int icon_index = index; On 2011/06/30 23:02:58, James Hawkins wrote: > Why are you using a temp int? Removed. http://codereview.chromium.org/7256002/diff/21004/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_manager.h (right): http://codereview.chromium.org/7256002/diff/21004/chrome/browser/profiles/pro... chrome/browser/profiles/profile_manager.h:212: scoped_ptr<ProfileInfoCache> profile_info_cache_; On 2011/06/30 23:02:58, James Hawkins wrote: > Document this member variable. Done. http://codereview.chromium.org/7256002/diff/21004/chrome/browser/resources/ne... File chrome/browser/resources/new_profile.css (right): http://codereview.chromium.org/7256002/diff/21004/chrome/browser/resources/ne... chrome/browser/resources/new_profile.css:2: font-family: segoe ui, arial, helvetica, sans-serif; On 2011/06/30 23:02:58, James Hawkins wrote: > Put the font family names in single quotes. Done. http://codereview.chromium.org/7256002/diff/21004/chrome/browser/resources/ne... chrome/browser/resources/new_profile.css:7: width: 337px; On 2011/06/30 23:02:58, James Hawkins wrote: > Alphabetize properties. Done. http://codereview.chromium.org/7256002/diff/21004/chrome/browser/resources/ne... chrome/browser/resources/new_profile.css:27: color: rgb(75, 75, 75); On 2011/06/30 23:02:58, James Hawkins wrote: > Use #aabbcc instead. Done. http://codereview.chromium.org/7256002/diff/21004/chrome/browser/resources/ne... chrome/browser/resources/new_profile.css:87: -webkit-box-align: center; On 2011/06/30 23:02:58, James Hawkins wrote: > Move webkit property to the top of the block. Done. http://codereview.chromium.org/7256002/diff/21004/chrome/browser/resources/ne... File chrome/browser/resources/new_profile.html (right): http://codereview.chromium.org/7256002/diff/21004/chrome/browser/resources/ne... chrome/browser/resources/new_profile.html:13: On 2011/06/30 23:02:58, James Hawkins wrote: > Remove blank lines. Done. http://codereview.chromium.org/7256002/diff/21004/chrome/browser/resources/ne... chrome/browser/resources/new_profile.html:14: <div id="overlay" class="overlay"> On 2011/06/30 23:02:58, James Hawkins wrote: > ID and class name the same? Technically it's OK, but I'd rather you make the ID > more descriptive. Removed id since it's not being referenced. http://codereview.chromium.org/7256002/diff/21004/chrome/browser/resources/ne... chrome/browser/resources/new_profile.html:21: <table> On 2011/06/30 23:02:58, James Hawkins wrote: > This is not tabular data; don't use a table. Done. http://codereview.chromium.org/7256002/diff/21004/chrome/browser/resources/ne... chrome/browser/resources/new_profile.html:32: <br/> On 2011/06/30 23:02:58, James Hawkins wrote: > Don't use <br>; use proper styling adjustments. Done. http://codereview.chromium.org/7256002/diff/21004/chrome/browser/resources/ne... chrome/browser/resources/new_profile.html:37: i18n-values="value:cancelProfile" /> On 2011/06/30 23:02:58, James Hawkins wrote: > <input> does not have an end tag, thus remove the last forward slash. Done. http://codereview.chromium.org/7256002/diff/21004/chrome/browser/resources/ne... chrome/browser/resources/new_profile.html:42: </div> <!-- page --> On 2011/06/30 23:02:58, James Hawkins wrote: > Remove these end comments. Done. http://codereview.chromium.org/7256002/diff/21004/chrome/browser/resources/ne... chrome/browser/resources/new_profile.html:51: <script src="chrome://newprofile/new_profile.js"></script> On 2011/06/30 23:02:58, James Hawkins wrote: > Move these scripts to the top. Do you mean move this to the head section? This breaks the string loading code. It also means that I can't set the onload function in new_profile.js. http://codereview.chromium.org/7256002/diff/21004/chrome/browser/resources/ne... File chrome/browser/resources/new_profile.js (right): http://codereview.chromium.org/7256002/diff/21004/chrome/browser/resources/ne... chrome/browser/resources/new_profile.js:5: var gSelectedAvatarIconIndex = 0; On 2011/06/30 23:02:58, James Hawkins wrote: > Document this variable. Done. http://codereview.chromium.org/7256002/diff/21004/chrome/browser/resources/ne... chrome/browser/resources/new_profile.js:8: // Helper functions On 2011/06/30 23:02:58, James Hawkins wrote: > Use a more descriptive name or remove the comment. Done. http://codereview.chromium.org/7256002/diff/21004/chrome/browser/resources/ne... chrome/browser/resources/new_profile.js:9: function $(o) {return document.getElementById(o);} On 2011/06/30 23:02:58, James Hawkins wrote: > Haven't you already included cr.js? This is already provided. Done. http://codereview.chromium.org/7256002/diff/21004/chrome/browser/resources/ne... chrome/browser/resources/new_profile.js:18: chrome.send('requestProfileInfo', []); On 2011/06/30 23:02:58, James Hawkins wrote: > No need for the empty last param, here and elsewhere. Done. http://codereview.chromium.org/7256002/diff/21004/chrome/browser/resources/ne... chrome/browser/resources/new_profile.js:33: var menu = document.getElementById("avatar-menu"); On 2011/06/30 23:02:58, James Hawkins wrote: > $('') Done. http://codereview.chromium.org/7256002/diff/21004/chrome/browser/resources/ne... chrome/browser/resources/new_profile.js:37: button.setAttribute("style", "background-color: #bbcee9"); On 2011/06/30 23:02:58, James Hawkins wrote: > You should use a class that specifies this instead. Done. http://codereview.chromium.org/7256002/diff/21004/chrome/browser/resources/ne... chrome/browser/resources/new_profile.js:47: if (document.documentElement.getAttribute('customlogo') == 'true') On 2011/06/30 23:02:58, James Hawkins wrote: > $('') Done. http://codereview.chromium.org/7256002/diff/21004/chrome/browser/resources/ne... chrome/browser/resources/new_profile.js:54: // Close the top overlay or sub-page on esc. On 2011/06/30 23:02:58, James Hawkins wrote: > You copied this comment from options_page.js and forgot to remove it. Also, have > you verified with UI leads that pressing escape should close this page? That > seems unintuitive. Asking. Now that I think about it, I agree that escape probably doesn't make sense here. http://codereview.chromium.org/7256002/diff/21004/chrome/browser/resources/ne... chrome/browser/resources/new_profile.js:82: $('create-button').onclick = function () { onCreate(''); }; On 2011/06/30 23:02:58, James Hawkins wrote: > Do all of this work in load(). Done. http://codereview.chromium.org/7256002/diff/21004/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chrome_web_ui_data_source.cc (right): http://codereview.chromium.org/7256002/diff/21004/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chrome_web_ui_data_source.cc:31: void ChromeWebUIDataSource::AddLocalizedString( On 2011/06/30 23:02:58, James Hawkins wrote: > What's wrong with AddString()? Oh cool, that wasn't there when I wrote this code. Using that instead. http://codereview.chromium.org/7256002/diff/21004/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chrome_web_ui_factory.cc (right): http://codereview.chromium.org/7256002/diff/21004/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chrome_web_ui_factory.cc:29: #include "chrome/browser/ui/webui/new_profile_ui.h" On 2011/06/30 23:02:58, James Hawkins wrote: > If this is only used for OS_CHROMEOS, move this include to an ifdef block. Done. http://codereview.chromium.org/7256002/diff/21004/chrome/browser/ui/webui/new... File chrome/browser/ui/webui/new_profile_dom_handler.h (right): http://codereview.chromium.org/7256002/diff/21004/chrome/browser/ui/webui/new... chrome/browser/ui/webui/new_profile_dom_handler.h:13: // The handler for Javascript messages related to the "new profile" view. On 2011/06/30 23:02:58, James Hawkins wrote: > s/view/page/ Done. http://codereview.chromium.org/7256002/diff/21004/chrome/browser/ui/webui/new... chrome/browser/ui/webui/new_profile_dom_handler.h:31: void SendDefaultAvatarImages(); On 2011/06/30 23:02:58, James Hawkins wrote: > Document these methods. Done. http://codereview.chromium.org/7256002/diff/21004/chrome/browser/ui/webui/new... File chrome/browser/ui/webui/new_profile_ui.cc (right): http://codereview.chromium.org/7256002/diff/21004/chrome/browser/ui/webui/new... chrome/browser/ui/webui/new_profile_ui.cc:25: static const char kNewProfileJsFile[] = "new_profile.js"; On 2011/06/30 23:02:58, James Hawkins wrote: > Move to anonymous namespace and remove static. Done. http://codereview.chromium.org/7256002/diff/21004/chrome/browser/ui/webui/new... chrome/browser/ui/webui/new_profile_ui.cc:29: // Is the current time within a given date range? On 2011/06/30 23:02:58, James Hawkins wrote: > Rephrase this question as a declarative about what the function does. I thought > this was a TODO at first. Done. http://codereview.chromium.org/7256002/diff/21004/chrome/browser/ui/webui/new... chrome/browser/ui/webui/new_profile_ui.cc:29: // Is the current time within a given date range? On 2011/06/30 23:02:58, James Hawkins wrote: > Rephrase this question as a declarative about what the function does. I thought > this was a TODO at first. Done. http://codereview.chromium.org/7256002/diff/21004/chrome/browser/ui/webui/new... chrome/browser/ui/webui/new_profile_ui.cc:63: AddLocalizedString("title", IDS_NEW_PROFILE_SETUP_TITLE); On 2011/06/30 23:02:58, James Hawkins wrote: > These should be added in the MessageHandler. Do you mean move this into NewProfileDOMHandler::Attach()? I would then need a reference to ChromeWebUIDataSource. Should I pass it to the construtor of NewProfileDOMHandler? Are you should this should be moved? All the other code has it in the data source.
http://codereview.chromium.org/7256002/diff/21004/chrome/browser/ui/webui/new... File chrome/browser/ui/webui/new_profile_ui.cc (right): http://codereview.chromium.org/7256002/diff/21004/chrome/browser/ui/webui/new... chrome/browser/ui/webui/new_profile_ui.cc:63: AddLocalizedString("title", IDS_NEW_PROFILE_SETUP_TITLE); On 2011/07/01 01:16:18, sail wrote: > On 2011/06/30 23:02:58, James Hawkins wrote: > > These should be added in the MessageHandler. > > Do you mean move this into NewProfileDOMHandler::Attach()? I would then need a > reference to ChromeWebUIDataSource. Should I pass it to the construtor of > NewProfileDOMHandler? > > Are you should this should be moved? All the other code has it in the data > source. Ignore my earlier comment; I'm used to OptionsUI handlers which add localized strings in the MessageHandlers. http://codereview.chromium.org/7256002/diff/19008/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/7256002/diff/19008/chrome/app/generated_resour... chrome/app/generated_resources.grd:12774: Create Only the multi-word messages need to go in these blocks. http://codereview.chromium.org/7256002/diff/19008/chrome/browser/resources/ne... File chrome/browser/resources/new_profile.css (right): http://codereview.chromium.org/7256002/diff/19008/chrome/browser/resources/ne... chrome/browser/resources/new_profile.css:100: width:100%; Space after colon. http://codereview.chromium.org/7256002/diff/19008/chrome/browser/resources/ne... File chrome/browser/resources/new_profile.js (right): http://codereview.chromium.org/7256002/diff/19008/chrome/browser/resources/ne... chrome/browser/resources/new_profile.js:16: $('create-button').onclick = function () { onCreate(''); }; Remove the parameters to onCreate() (which doesn't take any parameters). http://codereview.chromium.org/7256002/diff/19008/chrome/browser/resources/ne... chrome/browser/resources/new_profile.js:17: $('cancel-button').onclick = function () { onCancel(''); }; Same with onCancel. http://codereview.chromium.org/7256002/diff/19008/chrome/browser/resources/ne... chrome/browser/resources/new_profile.js:38: function onAvatarClicked(index) { Document all of these functions. http://codereview.chromium.org/7256002/diff/19008/chrome/browser/resources/ne... chrome/browser/resources/new_profile.js:39: var menu = $("avatar-menu"); Use single quotes, not double quotes. http://codereview.chromium.org/7256002/diff/19008/chrome/browser/resources/ne... chrome/browser/resources/new_profile.js:59: // Close the top overlay or sub-page on esc. As per previous comment, fix this comment. http://codereview.chromium.org/7256002/diff/19008/chrome/browser/resources/ne... chrome/browser/resources/new_profile.js:62: } nit: Remove braces for one-line blocks. http://codereview.chromium.org/7256002/diff/19008/chrome/browser/ui/webui/new... File chrome/browser/ui/webui/new_profile_ui.h (right): http://codereview.chromium.org/7256002/diff/19008/chrome/browser/ui/webui/new... chrome/browser/ui/webui/new_profile_ui.h:11: class NewProfileUI : public ChromeWebUI { Document this class.
http://codereview.chromium.org/7256002/diff/19008/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/7256002/diff/19008/chrome/app/generated_resour... chrome/app/generated_resources.grd:12774: Create On 2011/07/01 01:52:48, James Hawkins wrote: > Only the multi-word messages need to go in these blocks. I think some single word messages get translated into multi-word messages. In that case we still need two blocks here. http://codereview.chromium.org/7256002/diff/19008/chrome/browser/resources/ne... File chrome/browser/resources/new_profile.css (right): http://codereview.chromium.org/7256002/diff/19008/chrome/browser/resources/ne... chrome/browser/resources/new_profile.css:100: width:100%; On 2011/07/01 01:52:48, James Hawkins wrote: > Space after colon. Done. http://codereview.chromium.org/7256002/diff/19008/chrome/browser/resources/ne... File chrome/browser/resources/new_profile.js (right): http://codereview.chromium.org/7256002/diff/19008/chrome/browser/resources/ne... chrome/browser/resources/new_profile.js:16: $('create-button').onclick = function () { onCreate(''); }; On 2011/07/01 01:52:48, James Hawkins wrote: > Remove the parameters to onCreate() (which doesn't take any parameters). Done. http://codereview.chromium.org/7256002/diff/19008/chrome/browser/resources/ne... chrome/browser/resources/new_profile.js:17: $('cancel-button').onclick = function () { onCancel(''); }; On 2011/07/01 01:52:48, James Hawkins wrote: > Same with onCancel. Done. http://codereview.chromium.org/7256002/diff/19008/chrome/browser/resources/ne... chrome/browser/resources/new_profile.js:38: function onAvatarClicked(index) { On 2011/07/01 01:52:48, James Hawkins wrote: > Document all of these functions. Done. http://codereview.chromium.org/7256002/diff/19008/chrome/browser/resources/ne... chrome/browser/resources/new_profile.js:39: var menu = $("avatar-menu"); On 2011/07/01 01:52:48, James Hawkins wrote: > Use single quotes, not double quotes. Done. http://codereview.chromium.org/7256002/diff/19008/chrome/browser/resources/ne... chrome/browser/resources/new_profile.js:59: // Close the top overlay or sub-page on esc. On 2011/07/01 01:52:48, James Hawkins wrote: > As per previous comment, fix this comment. Removed. http://codereview.chromium.org/7256002/diff/19008/chrome/browser/resources/ne... chrome/browser/resources/new_profile.js:62: } On 2011/07/01 01:52:48, James Hawkins wrote: > nit: Remove braces for one-line blocks. Removed.
LGTM with nits. Note that for the CSS colors, if it ends up actually being #xxyyzz for some values of x,y,z, shorten it to #xyz. http://codereview.chromium.org/7256002/diff/22001/chrome/browser/resources/ne... File chrome/browser/resources/new_profile.css (right): http://codereview.chromium.org/7256002/diff/22001/chrome/browser/resources/ne... chrome/browser/resources/new_profile.css:35: color: rgb(75, 75, 75); #aabbcc http://codereview.chromium.org/7256002/diff/22001/chrome/browser/resources/ne... chrome/browser/resources/new_profile.css:45: background: -webkit-linear-gradient(white, rgb(235, 235, 235)); #aabbcc http://codereview.chromium.org/7256002/diff/22001/chrome/browser/resources/ne... File chrome/browser/resources/new_profile.html (right): http://codereview.chromium.org/7256002/diff/22001/chrome/browser/resources/ne... chrome/browser/resources/new_profile.html:35: <input id="create-button" type="button" 4-space indent from previous line. http://codereview.chromium.org/7256002/diff/22001/chrome/browser/resources/ne... chrome/browser/resources/new_profile.html:46: <script src="chrome://newprofile/new_profile.js"></script> Move the scripts to the top.
http://codereview.chromium.org/7256002/diff/22001/chrome/browser/resources/ne... File chrome/browser/resources/new_profile.html (right): http://codereview.chromium.org/7256002/diff/22001/chrome/browser/resources/ne... chrome/browser/resources/new_profile.html:46: <script src="chrome://newprofile/new_profile.js"></script> On 2011/07/01 18:15:32, James Hawkins wrote: > Move the scripts to the top. As per the previous discussion, moving this to <HEAD> would break some scripts that hook onto the DOM.
http://codereview.chromium.org/7256002/diff/22001/chrome/browser/resources/ne... File chrome/browser/resources/new_profile.html (right): http://codereview.chromium.org/7256002/diff/22001/chrome/browser/resources/ne... chrome/browser/resources/new_profile.html:46: <script src="chrome://newprofile/new_profile.js"></script> On 2011/07/01 18:20:22, sail wrote: > On 2011/07/01 18:15:32, James Hawkins wrote: > > Move the scripts to the top. > As per the previous discussion, moving this to <HEAD> would break some scripts > that hook onto the DOM. How? See print_preview.html and print_preview.js for examples of what I'm asking.
> How? See print_preview.html and print_preview.js for examples of what I'm > asking. I moved new_profile.js to HEAD. If I move strings.js, i18n_template.js, and i18n_process.js to HEAD the page no longer has any text in it. http://codereview.chromium.org/7256002/diff/22001/chrome/browser/resources/ne... File chrome/browser/resources/new_profile.css (right): http://codereview.chromium.org/7256002/diff/22001/chrome/browser/resources/ne... chrome/browser/resources/new_profile.css:35: color: rgb(75, 75, 75); On 2011/07/01 18:15:32, James Hawkins wrote: > #aabbcc Done. http://codereview.chromium.org/7256002/diff/22001/chrome/browser/resources/ne... chrome/browser/resources/new_profile.css:45: background: -webkit-linear-gradient(white, rgb(235, 235, 235)); On 2011/07/01 18:15:32, James Hawkins wrote: > #aabbcc Done. http://codereview.chromium.org/7256002/diff/22001/chrome/browser/resources/ne... File chrome/browser/resources/new_profile.html (right): http://codereview.chromium.org/7256002/diff/22001/chrome/browser/resources/ne... chrome/browser/resources/new_profile.html:35: <input id="create-button" type="button" On 2011/07/01 18:15:32, James Hawkins wrote: > 4-space indent from previous line. Done.
LGTM but add a TODO about the scripts at the bottom. There is a bug somewhere, as you shouldn't need to place them at the bottom.
On 2011/07/01 20:13:48, James Hawkins wrote: > LGTM but add a TODO about the scripts at the bottom. There is a bug somewhere, > as you shouldn't need to place them at the bottom. Will do. Thanks for the review!
http://codereview.chromium.org/7256002/diff/24001/chrome/browser/resources/ne... File chrome/browser/resources/new_profile.css (right): http://codereview.chromium.org/7256002/diff/24001/chrome/browser/resources/ne... chrome/browser/resources/new_profile.css:1: html { license header http://codereview.chromium.org/7256002/diff/24001/chrome/browser/resources/ne... chrome/browser/resources/new_profile.css:38: .page * input[type="button"], why do you have this twice http://codereview.chromium.org/7256002/diff/24001/chrome/browser/resources/ne... File chrome/browser/resources/new_profile.html (right): http://codereview.chromium.org/7256002/diff/24001/chrome/browser/resources/ne... chrome/browser/resources/new_profile.html:1: <!DOCTYPE HTML> license header http://codereview.chromium.org/7256002/diff/24001/chrome/browser/resources/ne... chrome/browser/resources/new_profile.html:10: <script src="chrome://resources/js/util.js"></script> can you throw in some line returns for readability http://codereview.chromium.org/7256002/diff/24001/chrome/browser/resources/ne... chrome/browser/resources/new_profile.html:28: <div i18n-content="profileIconLabel"> </div> remove blank space here and above http://codereview.chromium.org/7256002/diff/24001/chrome/browser/resources/ne... chrome/browser/resources/new_profile.html:37: i18n-values="value:createProfile"> i18n-content=foo http://codereview.chromium.org/7256002/diff/24001/chrome/browser/resources/ne... File chrome/browser/resources/new_profile.js (right): http://codereview.chromium.org/7256002/diff/24001/chrome/browser/resources/ne... chrome/browser/resources/new_profile.js:4: namespace everything in this file http://codereview.chromium.org/7256002/diff/24001/chrome/browser/resources/ne... chrome/browser/resources/new_profile.js:6: var gSelectedAvatarIconIndex = 0; I don't think this is the right variable naming style http://codereview.chromium.org/7256002/diff/24001/chrome/browser/resources/ne... chrome/browser/resources/new_profile.js:14: $('create-button').onclick = function () { onCreate(); }; simply onclick = onCreate http://codereview.chromium.org/7256002/diff/24001/chrome/browser/ui/webui/new... File chrome/browser/ui/webui/new_profile_dom_handler.h (right): http://codereview.chromium.org/7256002/diff/24001/chrome/browser/ui/webui/new... chrome/browser/ui/webui/new_profile_dom_handler.h:14: class NewProfileDOMHandler : public WebUIMessageHandler { why DOM? if NewProfileHandler is too generic it should be NewProfileWebUIHandler http://codereview.chromium.org/7256002/diff/24001/chrome/browser/ui/webui/new... File chrome/browser/ui/webui/new_profile_ui.cc (right): http://codereview.chromium.org/7256002/diff/24001/chrome/browser/ui/webui/new... chrome/browser/ui/webui/new_profile_ui.cc:92: void NewProfileUIHTMLSource::StartDataRequest(const std::string& path, tsepez recently refactored this. See http://codereview.chromium.org/7278008 for the new way of doing things
http://codereview.chromium.org/7256002/diff/24001/chrome/browser/resources/ne... File chrome/browser/resources/new_profile.css (right): http://codereview.chromium.org/7256002/diff/24001/chrome/browser/resources/ne... chrome/browser/resources/new_profile.css:1: html { On 2011/07/01 20:33:18, Evan Stade wrote: > license header Done. http://codereview.chromium.org/7256002/diff/24001/chrome/browser/resources/ne... chrome/browser/resources/new_profile.css:38: .page * input[type="button"], On 2011/07/01 20:33:18, Evan Stade wrote: > why do you have this twice oops, removed http://codereview.chromium.org/7256002/diff/24001/chrome/browser/resources/ne... File chrome/browser/resources/new_profile.html (right): http://codereview.chromium.org/7256002/diff/24001/chrome/browser/resources/ne... chrome/browser/resources/new_profile.html:1: <!DOCTYPE HTML> On 2011/07/01 20:33:18, Evan Stade wrote: > license header Done. http://codereview.chromium.org/7256002/diff/24001/chrome/browser/resources/ne... chrome/browser/resources/new_profile.html:10: <script src="chrome://resources/js/util.js"></script> On 2011/07/01 20:33:18, Evan Stade wrote: > can you throw in some line returns for readability Done. http://codereview.chromium.org/7256002/diff/24001/chrome/browser/resources/ne... chrome/browser/resources/new_profile.html:28: <div i18n-content="profileIconLabel"> </div> On 2011/07/01 20:33:18, Evan Stade wrote: > remove blank space here and above Done. http://codereview.chromium.org/7256002/diff/24001/chrome/browser/resources/ne... chrome/browser/resources/new_profile.html:37: i18n-values="value:createProfile"> On 2011/07/01 20:33:18, Evan Stade wrote: > i18n-content=foo I don't think this works for <input>. I tried changing this to i18n-content="cancelProfile" and it didn't work. http://codereview.chromium.org/7256002/diff/24001/chrome/browser/resources/ne... File chrome/browser/resources/new_profile.js (right): http://codereview.chromium.org/7256002/diff/24001/chrome/browser/resources/ne... chrome/browser/resources/new_profile.js:14: $('create-button').onclick = function () { onCreate(); }; On 2011/07/01 20:33:18, Evan Stade wrote: > simply onclick = onCreate Done. http://codereview.chromium.org/7256002/diff/24001/chrome/browser/ui/webui/new... File chrome/browser/ui/webui/new_profile_dom_handler.h (right): http://codereview.chromium.org/7256002/diff/24001/chrome/browser/ui/webui/new... chrome/browser/ui/webui/new_profile_dom_handler.h:14: class NewProfileDOMHandler : public WebUIMessageHandler { On 2011/07/01 20:33:18, Evan Stade wrote: > why DOM? if NewProfileHandler is too generic it should be NewProfileWebUIHandler Done. Renamed to NewProfileHandler. http://codereview.chromium.org/7256002/diff/24001/chrome/browser/ui/webui/new... File chrome/browser/ui/webui/new_profile_ui.cc (right): http://codereview.chromium.org/7256002/diff/24001/chrome/browser/ui/webui/new... chrome/browser/ui/webui/new_profile_ui.cc:92: void NewProfileUIHTMLSource::StartDataRequest(const std::string& path, On 2011/07/01 20:33:18, Evan Stade wrote: > tsepez recently refactored this. See http://codereview.chromium.org/7278008 for > the new way of doing things Done.
thanks. LGTM with nits http://codereview.chromium.org/7256002/diff/24001/chrome/browser/resources/ne... File chrome/browser/resources/new_profile.html (right): http://codereview.chromium.org/7256002/diff/24001/chrome/browser/resources/ne... chrome/browser/resources/new_profile.html:37: i18n-values="value:createProfile"> On 2011/07/01 23:47:04, sail wrote: > On 2011/07/01 20:33:18, Evan Stade wrote: > > i18n-content=foo > > I don't think this works for <input>. I tried changing this to > i18n-content="cancelProfile" and it didn't work. use <button>. It's more flexible. I don't know of any reason to ever prefer input type="button". But I am not an expert. http://codereview.chromium.org/7256002/diff/10002/chrome/browser/resources/ne... chrome/browser/resources/new_profile.html:14: <body i18n-values=".style.fontFamily:fontfamily;"> sorry for not being specific. I meant in general, throughout the file.
http://codereview.chromium.org/7256002/diff/10002/chrome/browser/resources/ne... File chrome/browser/resources/new_profile.html (right): http://codereview.chromium.org/7256002/diff/10002/chrome/browser/resources/ne... chrome/browser/resources/new_profile.html:14: On 2011/07/01 23:59:36, Evan Stade wrote: > sorry for not being specific. I meant in general, throughout the file. jhawkins asked me to remove blank lines earlier in the review. |
