Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9838064:
Add a sign-in promo message to the Other Devices menu. (Closed)
|
Created:
8 years, 9 months ago by Patrick Dubroy Modified:
8 years, 9 months ago CC:
chromium-reviews, dbeam+watch-ntp_chromium.org, estade+watch_chromium.org, arv (Not doing code reviews) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd a message to the Other Devices menu when user has no foreign sessions.
When the user is signed in, has tab sync enabled, but has no foreign sessions,
we would previously not show the menu. Now we show it with an informational
message.
BUG=119699
TEST=Manual.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=129397
Patch Set 1 #
Total comments: 37
Patch Set 2 : Address dbeam's comments. #
Total comments: 22
Patch Set 3 : Only show promo when user is signed in but has no tab sync data. #
Total comments: 19
Patch Set 4 : Address comments, and only enable menu when tab sync is enabled. #
Total comments: 16
Patch Set 5 : Address non-nits. #Patch Set 6 : Address previously-unaddressed nits. #
Total comments: 23
Patch Set 7 : Address dbeam's comments. #
Total comments: 9
Patch Set 8 : Address dbeam's final comments & estade's comments. #Patch Set 9 : Fix copyright header. #Messages
Total messages: 21 (0 generated)
Hey Dan, can you please review? cc: estade, since he's probably overloading with reviews right now.
http://codereview.chromium.org/9838064/diff/1/chrome/app/generated_resources.grd File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/9838064/diff/1/chrome/app/generated_resources.... chrome/app/generated_resources.grd:9207: + <message name="IDS_NEW_TAB_OTHER_SESSIONS_SIGN_IN_TEXT" desc="In title case. The text of the link to open the sync setup page. Shown in the 'Other Sessions' menu when the user is not signed in."> any reason you're not re-using one of: IDS_SYNC_PROMO_TITLE_EXISTING_USER, IDS_SYNC_PROMO_NOT_SIGNED_IN_STATUS_HEADER, IDS_SYNC_START_SYNC_BUTTON_LABEL, IDS_SYNC_PROMO_TITLE_SHORT? http://codereview.chromium.org/9838064/diff/1/chrome/browser/resources/ntp4/n... File chrome/browser/resources/ntp4/new_tab.js (right): http://codereview.chromium.org/9838064/diff/1/chrome/browser/resources/ntp4/n... chrome/browser/resources/ntp4/new_tab.js:101: localStrings.getString('login_status_message')); Casting to Boolean() is a little different than just saying if (localString.getString('login_status_message')) as well as this just shows a message, which could be present whether logged in or not... you should check if their GAIA users name is empty or get this information from the sync service. http://codereview.chromium.org/9838064/diff/1/chrome/browser/resources/ntp4/n... chrome/browser/resources/ntp4/new_tab.js:164: if (isUserSignedIn) { I'd undo this. http://codereview.chromium.org/9838064/diff/1/chrome/browser/resources/ntp4/n... chrome/browser/resources/ntp4/new_tab.js:223: showSyncLoginUI(this); can you change this to loginContainer.addEventListener('click', showSyncLoginUI); http://codereview.chromium.org/9838064/diff/1/chrome/browser/resources/ntp4/n... chrome/browser/resources/ntp4/new_tab.js:533: * Show the sync login UI. |element| is the element that triggered the login. doc param http://codereview.chromium.org/9838064/diff/1/chrome/browser/resources/ntp4/n... chrome/browser/resources/ntp4/new_tab.js:535: function showSyncLoginUI(element) { s/element/e/ var element = e.currentTarget; http://codereview.chromium.org/9838064/diff/1/chrome/browser/resources/ntp4/o... File chrome/browser/resources/ntp4/other_sessions.js (right): http://codereview.chromium.org/9838064/diff/1/chrome/browser/resources/ntp4/o... chrome/browser/resources/ntp4/other_sessions.js:30: var showSignInUI; s/UI/Ui/ (and if it's like this anywhere else, fix, :P) http://codereview.chromium.org/9838064/diff/1/chrome/browser/resources/ntp4/o... chrome/browser/resources/ntp4/other_sessions.js:54: * @param {Boolean} signedIn Is the current user signed in? s/Boolean/boolean/ http://codereview.chromium.org/9838064/diff/1/chrome/browser/resources/ntp4/o... chrome/browser/resources/ntp4/other_sessions.js:155: this.menu.appendChild(doc.createTextNode(emptyMessage)); eh, just put this in the markup and hide/unhide, will make code simpler. http://codereview.chromium.org/9838064/diff/1/chrome/browser/resources/ntp4/o... chrome/browser/resources/ntp4/other_sessions.js:161: console.log(showSignInUI); remove http://codereview.chromium.org/9838064/diff/1/chrome/browser/resources/ntp4/o... chrome/browser/resources/ntp4/other_sessions.js:162: showSignInUI(this); use e.currentTarget, don't use |this| in JavaScript as much as possible, it's fragile a source of subtle bugs, i.e. if (this.booleanFlag) // if scope changes this will go unnoticed |this| is // guaranteed to be an object and undefined co-erces http://codereview.chromium.org/9838064/diff/1/chrome/browser/resources/ntp4/o... chrome/browser/resources/ntp4/other_sessions.js:179: this.menu.innerHTML = ''; can you move this to central function that's called both places? http://codereview.chromium.org/9838064/diff/1/chrome/browser/resources/ntp4/o... chrome/browser/resources/ntp4/other_sessions.js:189: * Called from the new tab page when the user's signed in state changes. doc param http://codereview.chromium.org/9838064/diff/1/chrome/browser/ui/webui/ntp/for... File chrome/browser/ui/webui/ntp/foreign_session_handler.cc (right): http://codereview.chromium.org/9838064/diff/1/chrome/browser/ui/webui/ntp/for... chrome/browser/ui/webui/ntp/foreign_session_handler.cc:98: // Called before associator created, exit. possibly move comment above if, remove curlies and "== NULL" if you feel like it... http://codereview.chromium.org/9838064/diff/1/chrome/browser/ui/webui/ntp/for... chrome/browser/ui/webui/ntp/foreign_session_handler.cc:102: if (associator->GetAllForeignSessions(&sessions)) { no curlies, also should there be a ! in front of this? http://codereview.chromium.org/9838064/diff/1/chrome/browser/ui/webui/ntp/ntp... File chrome/browser/ui/webui/ntp/ntp_login_handler.cc (right): http://codereview.chromium.org/9838064/diff/1/chrome/browser/ui/webui/ntp/ntp... chrome/browser/ui/webui/ntp/ntp_login_handler.cc:238: is_user_signed_in, header_value, sub_header_value, icon_url_value); why'd you add a new param to this method at beginning, btw?
http://codereview.chromium.org/9838064/diff/1/chrome/app/generated_resources.grd File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/9838064/diff/1/chrome/app/generated_resources.... chrome/app/generated_resources.grd:9207: + <message name="IDS_NEW_TAB_OTHER_SESSIONS_SIGN_IN_TEXT" desc="In title case. The text of the link to open the sync setup page. Shown in the 'Other Sessions' menu when the user is not signed in."> On 2012/03/23 21:12:47, Dan Beam wrote: > any reason you're not re-using one of: IDS_SYNC_PROMO_TITLE_EXISTING_USER, > IDS_SYNC_PROMO_NOT_SIGNED_IN_STATUS_HEADER, > IDS_SYNC_START_SYNC_BUTTON_LABEL, IDS_SYNC_PROMO_TITLE_SHORT? I thought there might have been a reason why those were all defined separately, rather than shared. But I'm fine to re-use one of the existing ones. http://codereview.chromium.org/9838064/diff/1/chrome/browser/resources/ntp4/n... File chrome/browser/resources/ntp4/new_tab.js (right): http://codereview.chromium.org/9838064/diff/1/chrome/browser/resources/ntp4/n... chrome/browser/resources/ntp4/new_tab.js:101: localStrings.getString('login_status_message')); On 2012/03/23 21:12:47, Dan Beam wrote: > Casting to Boolean() is a little different than just saying > > if (localString.getString('login_status_message')) What's the difference? According to the spec it should be the same. > as well as this just shows a message, which could be present whether logged in > or not... you should check if their GAIA users name is empty or get this > information from the sync service. Ok, good point. Done. http://codereview.chromium.org/9838064/diff/1/chrome/browser/resources/ntp4/n... chrome/browser/resources/ntp4/new_tab.js:164: if (isUserSignedIn) { On 2012/03/23 21:12:47, Dan Beam wrote: > I'd undo this. Done. http://codereview.chromium.org/9838064/diff/1/chrome/browser/resources/ntp4/n... chrome/browser/resources/ntp4/new_tab.js:533: * Show the sync login UI. |element| is the element that triggered the login. On 2012/03/23 21:12:47, Dan Beam wrote: > doc param Done. http://codereview.chromium.org/9838064/diff/1/chrome/browser/resources/ntp4/n... chrome/browser/resources/ntp4/new_tab.js:535: function showSyncLoginUI(element) { On 2012/03/23 21:12:47, Dan Beam wrote: > s/element/e/ > > var element = e.currentTarget; Done. http://codereview.chromium.org/9838064/diff/1/chrome/browser/resources/ntp4/o... File chrome/browser/resources/ntp4/other_sessions.js (right): http://codereview.chromium.org/9838064/diff/1/chrome/browser/resources/ntp4/o... chrome/browser/resources/ntp4/other_sessions.js:30: var showSignInUI; On 2012/03/23 21:12:47, Dan Beam wrote: > s/UI/Ui/ (and if it's like this anywhere else, fix, :P) Hmmm. The style guide says, "Prefer to capitalize well-known abbreviations as if they were words" but it seems to be much more common to use UI rather than Ui. If I change it here, I'm not sure where to stop. There are 81 instances of "LoginUI" in the codebase. http://codereview.chromium.org/9838064/diff/1/chrome/browser/resources/ntp4/o... chrome/browser/resources/ntp4/other_sessions.js:54: * @param {Boolean} signedIn Is the current user signed in? On 2012/03/23 21:12:47, Dan Beam wrote: > s/Boolean/boolean/ Done. http://codereview.chromium.org/9838064/diff/1/chrome/browser/resources/ntp4/o... chrome/browser/resources/ntp4/other_sessions.js:155: this.menu.appendChild(doc.createTextNode(emptyMessage)); On 2012/03/23 21:12:47, Dan Beam wrote: > eh, just put this in the markup and hide/unhide, will make code simpler. I put it into the markup in the template section. We still need to insert it dynamically, because the menu itself is not defined in markup. http://codereview.chromium.org/9838064/diff/1/chrome/browser/resources/ntp4/o... chrome/browser/resources/ntp4/other_sessions.js:161: console.log(showSignInUI); On 2012/03/23 21:12:47, Dan Beam wrote: > remove Done. http://codereview.chromium.org/9838064/diff/1/chrome/browser/resources/ntp4/o... chrome/browser/resources/ntp4/other_sessions.js:162: showSignInUI(this); On 2012/03/23 21:12:47, Dan Beam wrote: > use e.currentTarget, don't use |this| in JavaScript as much as possible, it's > fragile a source of subtle bugs, i.e. > > if (this.booleanFlag) // if scope changes this will go unnoticed |this| is > // guaranteed to be an object and undefined co-erces Done. Thanks, that's a good thing to remember for all JS code that I write. http://codereview.chromium.org/9838064/diff/1/chrome/browser/resources/ntp4/o... chrome/browser/resources/ntp4/other_sessions.js:179: this.menu.innerHTML = ''; On 2012/03/23 21:12:47, Dan Beam wrote: > can you move this to central function that's called both places? Done. http://codereview.chromium.org/9838064/diff/1/chrome/browser/resources/ntp4/o... chrome/browser/resources/ntp4/other_sessions.js:189: * Called from the new tab page when the user's signed in state changes. On 2012/03/23 21:12:47, Dan Beam wrote: > doc param Done. http://codereview.chromium.org/9838064/diff/1/chrome/browser/ui/webui/ntp/for... File chrome/browser/ui/webui/ntp/foreign_session_handler.cc (right): http://codereview.chromium.org/9838064/diff/1/chrome/browser/ui/webui/ntp/for... chrome/browser/ui/webui/ntp/foreign_session_handler.cc:98: // Called before associator created, exit. On 2012/03/23 21:12:47, Dan Beam wrote: > possibly move comment above if, remove curlies and "== NULL" if you feel like > it... Done. http://codereview.chromium.org/9838064/diff/1/chrome/browser/ui/webui/ntp/for... chrome/browser/ui/webui/ntp/foreign_session_handler.cc:102: if (associator->GetAllForeignSessions(&sessions)) { On 2012/03/23 21:12:47, Dan Beam wrote: > no curlies, also should there be a ! in front of this? Ummm...yeah. Nice catch. :-) Done. http://codereview.chromium.org/9838064/diff/1/chrome/browser/ui/webui/ntp/ntp... File chrome/browser/ui/webui/ntp/ntp_login_handler.cc (right): http://codereview.chromium.org/9838064/diff/1/chrome/browser/ui/webui/ntp/ntp... chrome/browser/ui/webui/ntp/ntp_login_handler.cc:238: is_user_signed_in, header_value, sub_header_value, icon_url_value); On 2012/03/23 21:12:47, Dan Beam wrote: > why'd you add a new param to this method at beginning, btw? I don't know, I guess I usually try to put more general parameters first, and more specific things later. I've never consciously thought about it. I've moved it to the end.
http://codereview.chromium.org/9838064/diff/1/chrome/app/generated_resources.grd File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/9838064/diff/1/chrome/app/generated_resources.... chrome/app/generated_resources.grd:9207: + <message name="IDS_NEW_TAB_OTHER_SESSIONS_SIGN_IN_TEXT" desc="In title case. The text of the link to open the sync setup page. Shown in the 'Other Sessions' menu when the user is not signed in."> On 2012/03/26 15:16:46, dubroy wrote: > On 2012/03/23 21:12:47, Dan Beam wrote: > > any reason you're not re-using one of: IDS_SYNC_PROMO_TITLE_EXISTING_USER, > > IDS_SYNC_PROMO_NOT_SIGNED_IN_STATUS_HEADER, > > IDS_SYNC_START_SYNC_BUTTON_LABEL, IDS_SYNC_PROMO_TITLE_SHORT? > > I thought there might have been a reason why those were all defined separately, > rather than shared. But I'm fine to re-use one of the existing ones. I don't really know the answer here, but the process _seems_ to be: if message is context invariant: make this a generic variable and don't duplicate otherwise it's OK to duplicate So don't do it for now, but I'll ask sail@ (current OWNER of sync promo stuff) and estade@ (for his i18n skillz) how they feel about combining these (and if it's appropriate). http://codereview.chromium.org/9838064/diff/1/chrome/browser/resources/ntp4/n... File chrome/browser/resources/ntp4/new_tab.js (right): http://codereview.chromium.org/9838064/diff/1/chrome/browser/resources/ntp4/n... chrome/browser/resources/ntp4/new_tab.js:101: localStrings.getString('login_status_message')); On 2012/03/26 15:16:46, dubroy wrote: > On 2012/03/23 21:12:47, Dan Beam wrote: > > Casting to Boolean() is a little different than just saying > > > > if (localString.getString('login_status_message')) > > What's the difference? According to the spec it should be the same. tl;dr casting or comparing to different types in JS causes some unexpected results: if ([]) alert('yep'); if ([] == true) alert('nope'); > > as well as this just shows a message, which could be present whether logged in > > or not... you should check if their GAIA users name is empty or get this > > information from the sync service. > > Ok, good point. Done. > http://codereview.chromium.org/9838064/diff/1/chrome/browser/resources/ntp4/o... File chrome/browser/resources/ntp4/other_sessions.js (right): http://codereview.chromium.org/9838064/diff/1/chrome/browser/resources/ntp4/o... chrome/browser/resources/ntp4/other_sessions.js:30: var showSignInUI; On 2012/03/26 15:16:46, dubroy wrote: > On 2012/03/23 21:12:47, Dan Beam wrote: > > s/UI/Ui/ (and if it's like this anywhere else, fix, :P) > > Hmmm. The style guide says, "Prefer to capitalize well-known abbreviations as if > they were words" but it seems to be much more common to use UI rather than Ui. > If I change it here, I'm not sure where to stop. There are 81 instances of > "LoginUI" in the codebase. Then we need to eventually update those to *Ui* if being used as identifiers. It's in the style guide: Prefer to capitalize well-known abbreviations as if they were words: HttpUrl instead of HTTPURL. http://dev.chromium.org/developers/coding-style#Naming http://codereview.chromium.org/9838064/diff/6001/chrome/browser/resources/ntp... File chrome/browser/resources/ntp4/footer_menu.css (right): http://codereview.chromium.org/9838064/diff/6001/chrome/browser/resources/ntp... chrome/browser/resources/ntp4/footer_menu.css:140: .footer-menu .link-span { I'm confused, shouldn't anything with .link-* have cursor: pointer; already? Looking for this class, it doesn't seem it's used anywhere else (AFAICT: http://code.google.com/p/chromium/source/search?q=%5Cblink-span%5Cb+chrome%2F...), want to just update in new_tab.css? http://codereview.chromium.org/9838064/diff/6001/chrome/browser/resources/ntp... File chrome/browser/resources/ntp4/new_tab.html (right): http://codereview.chromium.org/9838064/diff/6001/chrome/browser/resources/ntp... chrome/browser/resources/ntp4/new_tab.html:202: <div id="other-sessions-promo-message" class="promo-message" hidden> you don't need to hide this as the whole section is [hidden] http://codereview.chromium.org/9838064/diff/6001/chrome/browser/resources/ntp... File chrome/browser/resources/ntp4/new_tab.js (right): http://codereview.chromium.org/9838064/diff/6001/chrome/browser/resources/ntp... chrome/browser/resources/ntp4/new_tab.js:497: * @param {Boolean} isUserSignedIn Indicates if the user is signed in or not. nit: s/Boolean/boolean/ (as it's not a wrapper class) http://codereview.chromium.org/9838064/diff/6001/chrome/browser/resources/ntp... chrome/browser/resources/ntp4/new_tab.js:530: * @param {Element} element The element that triggered the login. isn't this an {Event}, not element... you're calling e.currentTarget http://codereview.chromium.org/9838064/diff/6001/chrome/browser/resources/ntp... File chrome/browser/resources/ntp4/other_sessions.js (right): http://codereview.chromium.org/9838064/diff/6001/chrome/browser/resources/ntp... chrome/browser/resources/ntp4/other_sessions.js:44: this.menu.appendChild(messageContent); I think appendChild() will implicitly remove the node from the DOM anyways... so you probably don't need to removeChild() it first. http://codereview.chromium.org/9838064/diff/6001/chrome/browser/resources/ntp... chrome/browser/resources/ntp4/other_sessions.js:55: * Initialize the menu. may want to be more specific of which menu or just remove this comment http://codereview.chromium.org/9838064/diff/6001/chrome/browser/resources/ntp... chrome/browser/resources/ntp4/other_sessions.js:60: var promo = $('other-sessions-promo-message'); nit: why not document.querySelector('#other-sessions-promo-message .link-span') and is this always there? http://codereview.chromium.org/9838064/diff/6001/chrome/browser/resources/ntp... chrome/browser/resources/ntp4/other_sessions.js:67: chrome.send('getForeignSessions'); replace this code with: this.updateSignInState_(signedIn); http://codereview.chromium.org/9838064/diff/6001/chrome/browser/resources/ntp... chrome/browser/resources/ntp4/other_sessions.js:163: $('other-sessions-promo-message').hidden = false; nit: you seem to do this a couple times (toggle this node's hidden state), personally I'd change to: /** * Show or hide the other sessions promo message based on |visible|. * @param {boolean} visible Whether the message should be shown. */ setOtherSessionsPromoHidden_: function(visible) { $('other-sessions-promo-message').hidden = !visible; }, or something to that effect, but that's up to you (I like to update code in as little places as possible) http://codereview.chromium.org/9838064/diff/6001/chrome/browser/resources/ntp... chrome/browser/resources/ntp4/other_sessions.js:169: * from other devices. nit: +4\s before from http://codereview.chromium.org/9838064/diff/6001/chrome/browser/resources/ntp... chrome/browser/resources/ntp4/other_sessions.js:182: $('other-sessions-promo-message').hidden = true; this .hidden = true; seems redundant now that it's done in resetMenu_(), eh?
This is changed so that the promo message only appears when the user is signed in, but has no foreign session data (e.g. they have tab sync disabled). http://codereview.chromium.org/9838064/diff/1/chrome/app/generated_resources.grd File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/9838064/diff/1/chrome/app/generated_resources.... chrome/app/generated_resources.grd:9207: + <message name="IDS_NEW_TAB_OTHER_SESSIONS_SIGN_IN_TEXT" desc="In title case. The text of the link to open the sync setup page. Shown in the 'Other Sessions' menu when the user is not signed in."> On 2012/03/26 17:08:33, Dan Beam wrote: > On 2012/03/26 15:16:46, dubroy wrote: > > On 2012/03/23 21:12:47, Dan Beam wrote: > > > any reason you're not re-using one of: IDS_SYNC_PROMO_TITLE_EXISTING_USER, > > > IDS_SYNC_PROMO_NOT_SIGNED_IN_STATUS_HEADER, > > > IDS_SYNC_START_SYNC_BUTTON_LABEL, IDS_SYNC_PROMO_TITLE_SHORT? > > > > I thought there might have been a reason why those were all defined > separately, > > rather than shared. But I'm fine to re-use one of the existing ones. > > I don't really know the answer here, but the process _seems_ to be: > > if message is context invariant: > make this a generic variable and don't duplicate > otherwise > it's OK to duplicate > > So don't do it for now, but I'll ask sail@ (current OWNER of sync promo stuff) > and estade@ (for his i18n skillz) how they feel about combining these (and if > it's appropriate). Actually, it's now moot because I've been asked to implement this differently -- it's says "Learn more" now. http://codereview.chromium.org/9838064/diff/1/chrome/browser/resources/ntp4/n... File chrome/browser/resources/ntp4/new_tab.js (right): http://codereview.chromium.org/9838064/diff/1/chrome/browser/resources/ntp4/n... chrome/browser/resources/ntp4/new_tab.js:223: showSyncLoginUI(this); On 2012/03/23 21:12:47, Dan Beam wrote: > can you change this to > > loginContainer.addEventListener('click', showSyncLoginUI); Done. http://codereview.chromium.org/9838064/diff/1/chrome/browser/resources/ntp4/o... File chrome/browser/resources/ntp4/other_sessions.js (right): http://codereview.chromium.org/9838064/diff/1/chrome/browser/resources/ntp4/o... chrome/browser/resources/ntp4/other_sessions.js:30: var showSignInUI; On 2012/03/26 17:08:33, Dan Beam wrote: > On 2012/03/26 15:16:46, dubroy wrote: > > On 2012/03/23 21:12:47, Dan Beam wrote: > > > s/UI/Ui/ (and if it's like this anywhere else, fix, :P) > > > > Hmmm. The style guide says, "Prefer to capitalize well-known abbreviations as > if > > they were words" but it seems to be much more common to use UI rather than Ui. > > If I change it here, I'm not sure where to stop. There are 81 instances of > > "LoginUI" in the codebase. > > Then we need to eventually update those to *Ui* if being used as identifiers. > It's in the style guide: > > Prefer to capitalize well-known abbreviations as if they were words: HttpUrl > instead of HTTPURL. > > http://dev.chromium.org/developers/coding-style#Naming I'm pretty sure "prefer" != "must". There is also an overriding rule that it's better to be consistent, which I think applies here. http://codereview.chromium.org/9838064/diff/6001/chrome/browser/resources/ntp... File chrome/browser/resources/ntp4/footer_menu.css (right): http://codereview.chromium.org/9838064/diff/6001/chrome/browser/resources/ntp... chrome/browser/resources/ntp4/footer_menu.css:140: .footer-menu .link-span { On 2012/03/26 17:08:34, Dan Beam wrote: > I'm confused, shouldn't anything with .link-* have cursor: pointer; already? > Looking for this class, it doesn't seem it's used anywhere else (AFAICT: > http://code.google.com/p/chromium/source/search?q=%255Cblink-span%255Cb+chrom...), > want to just update in new_tab.css? Moot, because I'm now using an <A>. http://codereview.chromium.org/9838064/diff/6001/chrome/browser/resources/ntp... File chrome/browser/resources/ntp4/new_tab.html (right): http://codereview.chromium.org/9838064/diff/6001/chrome/browser/resources/ntp... chrome/browser/resources/ntp4/new_tab.html:202: <div id="other-sessions-promo-message" class="promo-message" hidden> On 2012/03/26 17:08:34, Dan Beam wrote: > you don't need to hide this as the whole section is [hidden] Done. http://codereview.chromium.org/9838064/diff/6001/chrome/browser/resources/ntp... File chrome/browser/resources/ntp4/new_tab.js (right): http://codereview.chromium.org/9838064/diff/6001/chrome/browser/resources/ntp... chrome/browser/resources/ntp4/new_tab.js:497: * @param {Boolean} isUserSignedIn Indicates if the user is signed in or not. On 2012/03/26 17:08:34, Dan Beam wrote: > nit: s/Boolean/boolean/ (as it's not a wrapper class) Done. http://codereview.chromium.org/9838064/diff/6001/chrome/browser/resources/ntp... chrome/browser/resources/ntp4/new_tab.js:530: * @param {Element} element The element that triggered the login. On 2012/03/26 17:08:34, Dan Beam wrote: > isn't this an {Event}, not element... you're calling e.currentTarget Done. http://codereview.chromium.org/9838064/diff/6001/chrome/browser/resources/ntp... File chrome/browser/resources/ntp4/other_sessions.js (right): http://codereview.chromium.org/9838064/diff/6001/chrome/browser/resources/ntp... chrome/browser/resources/ntp4/other_sessions.js:44: this.menu.appendChild(messageContent); On 2012/03/26 17:08:34, Dan Beam wrote: > I think appendChild() will implicitly remove the node from the DOM anyways... so > you probably don't need to removeChild() it first. Done. http://codereview.chromium.org/9838064/diff/6001/chrome/browser/resources/ntp... chrome/browser/resources/ntp4/other_sessions.js:55: * Initialize the menu. On 2012/03/26 17:08:34, Dan Beam wrote: > may want to be more specific of which menu or just remove this comment Done. http://codereview.chromium.org/9838064/diff/6001/chrome/browser/resources/ntp... chrome/browser/resources/ntp4/other_sessions.js:60: var promo = $('other-sessions-promo-message'); On 2012/03/26 17:08:34, Dan Beam wrote: > nit: why not document.querySelector('#other-sessions-promo-message .link-span') > and is this always there? This code is gone. http://codereview.chromium.org/9838064/diff/6001/chrome/browser/resources/ntp... chrome/browser/resources/ntp4/other_sessions.js:67: chrome.send('getForeignSessions'); On 2012/03/26 17:08:34, Dan Beam wrote: > replace this code with: > > this.updateSignInState_(signedIn); Done. http://codereview.chromium.org/9838064/diff/6001/chrome/browser/resources/ntp... chrome/browser/resources/ntp4/other_sessions.js:163: $('other-sessions-promo-message').hidden = false; On 2012/03/26 17:08:34, Dan Beam wrote: > nit: you seem to do this a couple times (toggle this node's hidden state), > personally I'd change to: > > /** > * Show or hide the other sessions promo message based on |visible|. > * @param {boolean} visible Whether the message should be shown. > */ > setOtherSessionsPromoHidden_: function(visible) { > $('other-sessions-promo-message').hidden = !visible; > }, > > or something to that effect, but that's up to you (I like to update code in as > little places as possible) Done. http://codereview.chromium.org/9838064/diff/6001/chrome/browser/resources/ntp... chrome/browser/resources/ntp4/other_sessions.js:169: * from other devices. On 2012/03/26 17:08:34, Dan Beam wrote: > nit: +4\s before from Done. http://codereview.chromium.org/9838064/diff/6001/chrome/browser/resources/ntp... chrome/browser/resources/ntp4/other_sessions.js:182: $('other-sessions-promo-message').hidden = true; On 2012/03/26 17:08:34, Dan Beam wrote: > this .hidden = true; seems redundant now that it's done in resetMenu_(), eh? Done.
http://codereview.chromium.org/9838064/diff/11001/chrome/browser/resources/nt... File chrome/browser/resources/ntp4/new_tab.html (right): http://codereview.chromium.org/9838064/diff/11001/chrome/browser/resources/nt... chrome/browser/resources/ntp4/new_tab.html:205: i18n-content="learnMore"></a></p> new line for each </close:tag> if it's not 1 line http://codereview.chromium.org/9838064/diff/11001/chrome/browser/resources/nt... File chrome/browser/resources/ntp4/new_tab.js (left): http://codereview.chromium.org/9838064/diff/11001/chrome/browser/resources/nt... chrome/browser/resources/ntp4/new_tab.js:160: nit: don't remove this \n http://codereview.chromium.org/9838064/diff/11001/chrome/browser/resources/nt... File chrome/browser/resources/ntp4/other_sessions.js (right): http://codereview.chromium.org/9838064/diff/11001/chrome/browser/resources/nt... chrome/browser/resources/ntp4/other_sessions.js:26: can you document when/where this will be changed http://codereview.chromium.org/9838064/diff/11001/chrome/browser/resources/nt... chrome/browser/resources/ntp4/other_sessions.js:43: this.menu.appendChild(messageContent); nit: inline this this.menu.appendChild($('other-sessions-promo-message')); or simply call .resetMenu_() ? http://codereview.chromium.org/9838064/diff/11001/chrome/browser/resources/nt... chrome/browser/resources/ntp4/other_sessions.js:92: * Reset the menu to the default state. @private http://codereview.chromium.org/9838064/diff/11001/chrome/browser/resources/nt... chrome/browser/resources/ntp4/other_sessions.js:151: * @param {boolean} visible Whether the message should be shown. @private http://codereview.chromium.org/9838064/diff/11001/chrome/browser/resources/nt... chrome/browser/resources/ntp4/other_sessions.js:180: updateSignInState: function(signedIn) { if this is called nowhere else outside of this function, make private_ / @private http://codereview.chromium.org/9838064/diff/11001/chrome/browser/ui/webui/ntp... File chrome/browser/ui/webui/ntp/foreign_session_handler.cc (right): http://codereview.chromium.org/9838064/diff/11001/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/foreign_session_handler.cc:102: for (std::vector<const SyncedSession*>::const_iterator i = change from iterator to index http://codereview.chromium.org/9838064/diff/11001/chrome/browser/ui/webui/ntp... File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right): http://codereview.chromium.org/9838064/diff/11001/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:292: PrefService* prefs = profile_->GetPrefs(); I wonder why there is no const form of this... was going to ask you to use it but I guess it doesn't exist.
http://codereview.chromium.org/9838064/diff/11001/chrome/browser/ui/webui/ntp... File chrome/browser/ui/webui/ntp/foreign_session_handler.cc (right): http://codereview.chromium.org/9838064/diff/11001/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/foreign_session_handler.cc:114: if (SessionWindowToValue(*window, window_data.get())) { nit: no curlies
One more slight change, we should only show the Other Devices button when tab sync is actually enabled. http://codereview.chromium.org/9838064/diff/11001/chrome/browser/resources/nt... File chrome/browser/resources/ntp4/new_tab.html (right): http://codereview.chromium.org/9838064/diff/11001/chrome/browser/resources/nt... chrome/browser/resources/ntp4/new_tab.html:205: i18n-content="learnMore"></a></p> On 2012/03/26 20:42:39, Dan Beam wrote: > new line for each </close:tag> if it's not 1 line Done. http://codereview.chromium.org/9838064/diff/11001/chrome/browser/resources/nt... File chrome/browser/resources/ntp4/new_tab.js (left): http://codereview.chromium.org/9838064/diff/11001/chrome/browser/resources/nt... chrome/browser/resources/ntp4/new_tab.js:160: On 2012/03/26 20:42:39, Dan Beam wrote: > nit: don't remove this \n Done. http://codereview.chromium.org/9838064/diff/11001/chrome/browser/resources/nt... File chrome/browser/resources/ntp4/other_sessions.js (right): http://codereview.chromium.org/9838064/diff/11001/chrome/browser/resources/nt... chrome/browser/resources/ntp4/other_sessions.js:26: On 2012/03/26 20:42:39, Dan Beam wrote: > can you document when/where this will be changed Done. http://codereview.chromium.org/9838064/diff/11001/chrome/browser/resources/nt... chrome/browser/resources/ntp4/other_sessions.js:43: this.menu.appendChild(messageContent); On 2012/03/26 20:42:39, Dan Beam wrote: > nit: inline this > > this.menu.appendChild($('other-sessions-promo-message')); > > or simply call .resetMenu_() ? Done. http://codereview.chromium.org/9838064/diff/11001/chrome/browser/resources/nt... chrome/browser/resources/ntp4/other_sessions.js:92: * Reset the menu to the default state. On 2012/03/26 20:42:39, Dan Beam wrote: > @private Done. http://codereview.chromium.org/9838064/diff/11001/chrome/browser/resources/nt... chrome/browser/resources/ntp4/other_sessions.js:151: * @param {boolean} visible Whether the message should be shown. On 2012/03/26 20:42:39, Dan Beam wrote: > @private Done. http://codereview.chromium.org/9838064/diff/11001/chrome/browser/resources/nt... chrome/browser/resources/ntp4/other_sessions.js:180: updateSignInState: function(signedIn) { On 2012/03/26 20:42:39, Dan Beam wrote: > if this is called nowhere else outside of this function, make private_ / > @private It's called from the ntp.js when the sign-in state changes. http://codereview.chromium.org/9838064/diff/11001/chrome/browser/ui/webui/ntp... File chrome/browser/ui/webui/ntp/foreign_session_handler.cc (right): http://codereview.chromium.org/9838064/diff/11001/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/foreign_session_handler.cc:102: for (std::vector<const SyncedSession*>::const_iterator i = On 2012/03/26 20:42:39, Dan Beam wrote: > change from iterator to index Done. http://codereview.chromium.org/9838064/diff/11001/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/foreign_session_handler.cc:114: if (SessionWindowToValue(*window, window_data.get())) { On 2012/03/26 20:57:38, Dan Beam wrote: > nit: no curlies Done.
https://chromiumcodereview.appspot.com/9838064/diff/14002/chrome/browser/reso... File chrome/browser/resources/ntp4/new_tab.js (right): https://chromiumcodereview.appspot.com/9838064/diff/14002/chrome/browser/reso... chrome/browser/resources/ntp4/new_tab.js:120: $('other-sessions-menu-button').initialize(templateData.isUserSignedIn); any reason you're not assigning this getElementById() to variable and using getRequiredElement() like above for |notificationContainer|? https://chromiumcodereview.appspot.com/9838064/diff/14002/chrome/browser/reso... chrome/browser/resources/ntp4/new_tab.js:565: function foreignSessions(sessionList, isTabSyncEnabled) { nit: this should probably be named getForeignSessions and possibly be passing through the return value, should that ever matter https://chromiumcodereview.appspot.com/9838064/diff/14002/chrome/browser/reso... File chrome/browser/resources/ntp4/other_sessions.js (right): https://chromiumcodereview.appspot.com/9838064/diff/14002/chrome/browser/reso... chrome/browser/resources/ntp4/other_sessions.js:30: var isUserSignedIn = false; nit: this is probably meant to go on the class prototype so it's instance specific, but this doesn't seem pressing... https://chromiumcodereview.appspot.com/9838064/diff/14002/chrome/browser/reso... chrome/browser/resources/ntp4/other_sessions.js:97: var promoContent = $('other-sessions-promo-message'); why do you need to do var promoContent = $('other-sessions-promo-message'); isn't |this| already the same thing? https://chromiumcodereview.appspot.com/9838064/diff/14002/chrome/browser/ui/w... File chrome/browser/ui/webui/ntp/foreign_session_handler.cc (right): https://chromiumcodereview.appspot.com/9838064/diff/14002/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/foreign_session_handler.cc:93: return service && service->GetSessionModelAssociator() != NULL; nit: remove "!= NULL" if possible, IMO https://chromiumcodereview.appspot.com/9838064/diff/14002/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/foreign_session_handler.cc:110: session->windows.begin(); it != session->windows.end(); ++it) { nit: +1 \s https://chromiumcodereview.appspot.com/9838064/diff/14002/chrome/browser/ui/w... File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right): https://chromiumcodereview.appspot.com/9838064/diff/14002/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:292: PrefService* prefs = profile_->GetPrefs(); thanks for this, btw
Addressed non-nits. https://chromiumcodereview.appspot.com/9838064/diff/14002/chrome/browser/reso... File chrome/browser/resources/ntp4/new_tab.js (right): https://chromiumcodereview.appspot.com/9838064/diff/14002/chrome/browser/reso... chrome/browser/resources/ntp4/new_tab.js:120: $('other-sessions-menu-button').initialize(templateData.isUserSignedIn); On 2012/03/26 23:18:54, Dan Beam wrote: > any reason you're not assigning this getElementById() to variable and using > getRequiredElement() like above for |notificationContainer|? Done. https://chromiumcodereview.appspot.com/9838064/diff/14002/chrome/browser/reso... File chrome/browser/resources/ntp4/other_sessions.js (right): https://chromiumcodereview.appspot.com/9838064/diff/14002/chrome/browser/reso... chrome/browser/resources/ntp4/other_sessions.js:97: var promoContent = $('other-sessions-promo-message'); On 2012/03/26 23:18:54, Dan Beam wrote: > why do you need to do > > var promoContent = $('other-sessions-promo-message'); > > isn't |this| already the same thing? No, 'this' is the button that you click to show the menu. The promo message is the content that gets put in the menu under certain conditions.
https://chromiumcodereview.appspot.com/9838064/diff/14002/chrome/browser/reso... File chrome/browser/resources/ntp4/other_sessions.js (right): https://chromiumcodereview.appspot.com/9838064/diff/14002/chrome/browser/reso... chrome/browser/resources/ntp4/other_sessions.js:97: var promoContent = $('other-sessions-promo-message'); On 2012/03/26 23:46:07, dubroy wrote: > On 2012/03/26 23:18:54, Dan Beam wrote: > > why do you need to do > > > > var promoContent = $('other-sessions-promo-message'); > > > > isn't |this| already the same thing? > > No, 'this' is the button that you click to show the menu. The promo message is > the content that gets put in the menu under certain conditions. Then it seems like you should be assigning this to a private or protected member on initialization (and possibly passing it to initialize()).
Ok, I've addressed all your previous issues now. http://codereview.chromium.org/9838064/diff/14002/chrome/browser/resources/nt... File chrome/browser/resources/ntp4/new_tab.js (right): http://codereview.chromium.org/9838064/diff/14002/chrome/browser/resources/nt... chrome/browser/resources/ntp4/new_tab.js:565: function foreignSessions(sessionList, isTabSyncEnabled) { On 2012/03/26 23:18:54, Dan Beam wrote: > nit: this should probably be named getForeignSessions and possibly be passing > through the return value, should that ever matter Done. http://codereview.chromium.org/9838064/diff/14002/chrome/browser/resources/nt... File chrome/browser/resources/ntp4/other_sessions.js (right): http://codereview.chromium.org/9838064/diff/14002/chrome/browser/resources/nt... chrome/browser/resources/ntp4/other_sessions.js:30: var isUserSignedIn = false; On 2012/03/26 23:18:54, Dan Beam wrote: > nit: this is probably meant to go on the class prototype so it's instance > specific, but this doesn't seem pressing... Actually, it's not even used anymore. http://codereview.chromium.org/9838064/diff/14002/chrome/browser/resources/nt... chrome/browser/resources/ntp4/other_sessions.js:97: var promoContent = $('other-sessions-promo-message'); On 2012/03/27 00:03:42, Dan Beam wrote: > On 2012/03/26 23:46:07, dubroy wrote: > > On 2012/03/26 23:18:54, Dan Beam wrote: > > > why do you need to do > > > > > > var promoContent = $('other-sessions-promo-message'); > > > > > > isn't |this| already the same thing? > > > > No, 'this' is the button that you click to show the menu. The promo message is > > the content that gets put in the menu under certain conditions. > > Then it seems like you should be assigning this to a private or protected member > on initialization (and possibly passing it to initialize()). Done. http://codereview.chromium.org/9838064/diff/14002/chrome/browser/ui/webui/ntp... File chrome/browser/ui/webui/ntp/foreign_session_handler.cc (right): http://codereview.chromium.org/9838064/diff/14002/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/foreign_session_handler.cc:93: return service && service->GetSessionModelAssociator() != NULL; On 2012/03/26 23:18:54, Dan Beam wrote: > nit: remove "!= NULL" if possible, IMO Done. http://codereview.chromium.org/9838064/diff/14002/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/foreign_session_handler.cc:110: session->windows.begin(); it != session->windows.end(); ++it) { On 2012/03/26 23:18:54, Dan Beam wrote: > nit: +1 \s Done. http://codereview.chromium.org/9838064/diff/14002/chrome/browser/ui/webui/ntp... File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right): http://codereview.chromium.org/9838064/diff/14002/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:292: PrefService* prefs = profile_->GetPrefs(); On 2012/03/26 23:18:54, Dan Beam wrote: > thanks for this, btw np.
http://codereview.chromium.org/9838064/diff/13006/chrome/browser/resources/nt... File chrome/browser/resources/ntp4/new_tab.html (right): http://codereview.chromium.org/9838064/diff/13006/chrome/browser/resources/nt... chrome/browser/resources/ntp4/new_tab.html:113: <div class="disclosure-triangle"></div> might be better for promo message to live inside but [hidden], see below http://codereview.chromium.org/9838064/diff/13006/chrome/browser/resources/nt... chrome/browser/resources/ntp4/new_tab.html:201: there is no session data (e.g. they have tab sync turned off). --> btw, estade brought up a good point that if you're not copying this multiple times maybe it should live above ^ http://codereview.chromium.org/9838064/diff/13006/chrome/browser/resources/nt... File chrome/browser/resources/ntp4/new_tab.js (right): http://codereview.chromium.org/9838064/diff/13006/chrome/browser/resources/nt... chrome/browser/resources/ntp4/new_tab.js:573: if (otherSessionsButton) nit: curlies http://codereview.chromium.org/9838064/diff/13006/chrome/browser/resources/nt... chrome/browser/resources/ntp4/new_tab.js:617: setForeignSessions: setForeignSessions, I mentioned getForeignSessions, but I guess this is fine (if not better). http://codereview.chromium.org/9838064/diff/13006/chrome/browser/resources/nt... File chrome/browser/resources/ntp4/other_sessions.js (right): http://codereview.chromium.org/9838064/diff/13006/chrome/browser/resources/nt... chrome/browser/resources/ntp4/other_sessions.js:39: this.promoMessage_ = $('other-sessions-promo-message'); this would be much more loosely coupled if you passed: a) the node b) the ID c) a selector to decorate somehow (but I don't think that's how it works) or added a node with a class (not ID) inside of this menu and .querySelector()'d for it. http://codereview.chromium.org/9838064/diff/13006/chrome/browser/resources/nt... chrome/browser/resources/ntp4/other_sessions.js:41: this.resetMenu_(); you're probably better off doing all this in initialize() http://codereview.chromium.org/9838064/diff/13006/chrome/browser/resources/nt... chrome/browser/resources/ntp4/other_sessions.js:95: this.menu.appendChild(this.promoMessage_); also, if you never need more than 1 and/or you move the markup, you can remove this appendChild() line http://codereview.chromium.org/9838064/diff/13006/chrome/browser/resources/nt... chrome/browser/resources/ntp4/other_sessions.js:96: this.promoMessage_.hidden = true; this should probably be using setPromoVisible_() if you really want reset to change visiblity, but I see why it's not (it'd be circular) http://codereview.chromium.org/9838064/diff/13006/chrome/browser/resources/nt... chrome/browser/resources/ntp4/other_sessions.js:153: this.resetMenu_(); should setting it |visible| remove the contents? http://codereview.chromium.org/9838064/diff/13006/chrome/browser/resources/nt... chrome/browser/resources/ntp4/other_sessions.js:168: this.setPromoVisible_(sessionList.length == 0); shouldn't this be done after you rebuild the session list? http://codereview.chromium.org/9838064/diff/13006/chrome/browser/ui/webui/ntp... File chrome/browser/ui/webui/ntp/foreign_session_handler.cc (right): http://codereview.chromium.org/9838064/diff/13006/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/foreign_session_handler.cc:91: ProfileSyncService* service(ProfileSyncServiceFactory:: nit: can you split this into multiple lines?
Addressed most of your comments. Also updated the description and bug # to better reflect what the CL is actually doing. http://codereview.chromium.org/9838064/diff/13006/chrome/browser/resources/nt... File chrome/browser/resources/ntp4/new_tab.html (right): http://codereview.chromium.org/9838064/diff/13006/chrome/browser/resources/nt... chrome/browser/resources/ntp4/new_tab.html:113: <div class="disclosure-triangle"></div> On 2012/03/27 01:21:06, Dan Beam wrote: > might be better for promo message to live inside but [hidden], see below I'm not sure. The thing is, the promo message eventually lives inside the <menu> element, which is created dynamically. If I put it here, it brings the two pieces of related code close together, but I don't think it really makes sense for the promo message to be a child of the button. I'd just end up having to reparent it to make it a child of the menu. Meta-review: when you use words like "might" or "maybe" in your comments, it's hard for me to figure out whether you'd like me to change it, or whether you are leaving it up to me to decide. http://codereview.chromium.org/9838064/diff/13006/chrome/browser/resources/nt... chrome/browser/resources/ntp4/new_tab.html:201: there is no session data (e.g. they have tab sync turned off). --> On 2012/03/27 01:21:06, Dan Beam wrote: > btw, estade brought up a good point that if you're not copying this multiple > times maybe it should live above ^ See my comment above. I don't think it makes any more sense to put it there. http://codereview.chromium.org/9838064/diff/13006/chrome/browser/resources/nt... File chrome/browser/resources/ntp4/new_tab.js (right): http://codereview.chromium.org/9838064/diff/13006/chrome/browser/resources/nt... chrome/browser/resources/ntp4/new_tab.js:573: if (otherSessionsButton) On 2012/03/27 01:21:06, Dan Beam wrote: > nit: curlies Done. http://codereview.chromium.org/9838064/diff/13006/chrome/browser/resources/nt... chrome/browser/resources/ntp4/new_tab.js:617: setForeignSessions: setForeignSessions, On 2012/03/27 01:21:06, Dan Beam wrote: > I mentioned getForeignSessions, but I guess this is fine (if not better). Yeah, I saw what you wrote but getForeignSessions doesn't really make sense IMO. Perhaps "getForeignSessionsCallback", but not "getForeignSessions". http://codereview.chromium.org/9838064/diff/13006/chrome/browser/resources/nt... File chrome/browser/resources/ntp4/other_sessions.js (right): http://codereview.chromium.org/9838064/diff/13006/chrome/browser/resources/nt... chrome/browser/resources/ntp4/other_sessions.js:39: this.promoMessage_ = $('other-sessions-promo-message'); On 2012/03/27 01:21:06, Dan Beam wrote: > this would be much more loosely coupled if you passed: > > a) the node > b) the ID > c) a selector What is the advantage of that? Currently new_tab.js has no knowledge of the promo message, because it's an implementation detail of this element. I don't see why it would be better for new_tab.js to have more knowledge about the inner workings of this element. > to decorate somehow (but I don't think that's how it works) or added a node with > a class (not ID) inside of this menu and .querySelector()'d for it. That sounds like _tighter_ coupling of the promo and the menu. That sounds like a better idea, but as mentioned in my other comment, I don't think it makes sense to make the promo message a child of the button. http://codereview.chromium.org/9838064/diff/13006/chrome/browser/resources/nt... chrome/browser/resources/ntp4/other_sessions.js:41: this.resetMenu_(); On 2012/03/27 01:21:06, Dan Beam wrote: > you're probably better off doing all this in initialize() It's not clear what you're talking about here. What do you mean by "all this"? Why would it be better? http://codereview.chromium.org/9838064/diff/13006/chrome/browser/resources/nt... chrome/browser/resources/ntp4/other_sessions.js:95: this.menu.appendChild(this.promoMessage_); On 2012/03/27 01:21:06, Dan Beam wrote: > also, if you never need more than 1 and/or you move the markup, you can remove > this appendChild() line What is the purpose of "also" in this sentence? Is it referring to a previous comment? The reason for appendChild() here is that all the children of the menu, including the promo message, get removed in the previous line (this.menu.innerHTML = ''). The reason for this is that the menu might contain all the elements that were added by addSession_(). Rather than try to remove those elements individually, it's simpler to clear all the children and then re-insert the promo. http://codereview.chromium.org/9838064/diff/13006/chrome/browser/resources/nt... chrome/browser/resources/ntp4/other_sessions.js:96: this.promoMessage_.hidden = true; On 2012/03/27 01:21:06, Dan Beam wrote: > this should probably be using setPromoVisible_() if you really want reset to > change visiblity, but I see why it's not (it'd be circular) I've removed this line altogether. It's not really necessary. http://codereview.chromium.org/9838064/diff/13006/chrome/browser/resources/nt... chrome/browser/resources/ntp4/other_sessions.js:153: this.resetMenu_(); On 2012/03/27 01:21:06, Dan Beam wrote: > should setting it |visible| remove the contents? No, probably not. Fixed. http://codereview.chromium.org/9838064/diff/13006/chrome/browser/resources/nt... chrome/browser/resources/ntp4/other_sessions.js:168: this.setPromoVisible_(sessionList.length == 0); On 2012/03/27 01:21:06, Dan Beam wrote: > shouldn't this be done after you rebuild the session list? In this version of the code, it needed to be done *before* building the session list, because setPromoVisible_() also called resetMenu_(). Now that I've changed setPromoVisible_() to not call resetMenu_(), it's not _necessary_ for setPromoVisible_() to happen before building the session list, but there's no reason that it can't. There is no ordering dependency between the two operations. http://codereview.chromium.org/9838064/diff/13006/chrome/browser/ui/webui/ntp... File chrome/browser/ui/webui/ntp/foreign_session_handler.cc (right): http://codereview.chromium.org/9838064/diff/13006/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/foreign_session_handler.cc:91: ProfileSyncService* service(ProfileSyncServiceFactory:: On 2012/03/27 01:21:06, Dan Beam wrote: > nit: can you split this into multiple lines? I'm quite sure what you mean, since it's already a two-line statement. I can split it up some more though.
http://codereview.chromium.org/9838064/diff/13006/chrome/browser/resources/nt... File chrome/browser/resources/ntp4/new_tab.html (right): http://codereview.chromium.org/9838064/diff/13006/chrome/browser/resources/nt... chrome/browser/resources/ntp4/new_tab.html:113: <div class="disclosure-triangle"></div> On 2012/03/27 13:44:21, dubroy wrote: > On 2012/03/27 01:21:06, Dan Beam wrote: > > might be better for promo message to live inside but [hidden], see below > > I'm not sure. The thing is, the promo message eventually lives inside the <menu> > element, which is created dynamically. If I put it here, it brings the two > pieces of related code close together, but I don't think it really makes sense > for the promo message to be a child of the button. I'd just end up having to > reparent it to make it a child of the menu. Cloning a node from the template section like we talked about seems fine as long as the menu owns that node. > Meta-review: when you use words like "might" or "maybe" in your comments, it's > hard for me to figure out whether you'd like me to change it, or whether you are > leaving it up to me to decide. We talked about this and maybe/might means I'm asking for your input as well. http://codereview.chromium.org/9838064/diff/16005/chrome/browser/resources/nt... File chrome/browser/resources/ntp4/other_sessions.js (right): http://codereview.chromium.org/9838064/diff/16005/chrome/browser/resources/nt... chrome/browser/resources/ntp4/other_sessions.js:91: resetMenuContents_: function() { thank you for renaming this, beat me to it
lgtm w/nits mentioned in last comment, estade still needs to stamp for ui/webui/ntp/* until I add to OWNERS (which I'm doing right now)
http://codereview.chromium.org/9838064/diff/16005/chrome/browser/resources/nt... File chrome/browser/resources/ntp4/new_tab.js (right): http://codereview.chromium.org/9838064/diff/16005/chrome/browser/resources/nt... chrome/browser/resources/ntp4/new_tab.js:574: return otherSessionsButton.setForeignSessions(sessionList, looks weird to have a return statement in one case, and no return in the other case http://codereview.chromium.org/9838064/diff/16005/chrome/browser/resources/nt... File chrome/browser/resources/ntp4/other_sessions.js (right): http://codereview.chromium.org/9838064/diff/16005/chrome/browser/resources/nt... chrome/browser/resources/ntp4/other_sessions.js:149: setPromoVisible_: function(visible) { can you do this with css instead? something like: * ~ promo-message-id { display: none; } http://codereview.chromium.org/9838064/diff/16005/chrome/browser/resources/nt... chrome/browser/resources/ntp4/other_sessions.js:171: } \n http://codereview.chromium.org/9838064/diff/16005/chrome/browser/ui/webui/ntp... File chrome/browser/ui/webui/ntp/foreign_session_handler.cc (right): http://codereview.chromium.org/9838064/diff/16005/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/foreign_session_handler.cc:116: } nit: prefer newlines after end curlies
http://codereview.chromium.org/9838064/diff/16005/chrome/browser/resources/nt... File chrome/browser/resources/ntp4/new_tab.js (right): http://codereview.chromium.org/9838064/diff/16005/chrome/browser/resources/nt... chrome/browser/resources/ntp4/new_tab.js:574: return otherSessionsButton.setForeignSessions(sessionList, On 2012/03/27 18:20:20, Evan Stade wrote: > looks weird to have a return statement in one case, and no return in the other > case Ok, I've removed it here. There is no meaningful return value, so I don't think it makes sense to put it in. http://codereview.chromium.org/9838064/diff/16005/chrome/browser/resources/nt... File chrome/browser/resources/ntp4/other_sessions.js (right): http://codereview.chromium.org/9838064/diff/16005/chrome/browser/resources/nt... chrome/browser/resources/ntp4/other_sessions.js:149: setPromoVisible_: function(visible) { On 2012/03/27 18:20:20, Evan Stade wrote: > can you do this with css instead? something like: > > * ~ promo-message-id { > display: none; > } Nice idea. Doesn't quite work but your second suggestion, using :only-child, does. http://codereview.chromium.org/9838064/diff/16005/chrome/browser/resources/nt... chrome/browser/resources/ntp4/other_sessions.js:171: } On 2012/03/27 18:20:20, Evan Stade wrote: > \n Done. http://codereview.chromium.org/9838064/diff/16005/chrome/browser/ui/webui/ntp... File chrome/browser/ui/webui/ntp/foreign_session_handler.cc (right): http://codereview.chromium.org/9838064/diff/16005/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/foreign_session_handler.cc:116: } On 2012/03/27 18:20:20, Evan Stade wrote: > nit: prefer newlines after end curlies Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dubroy@chromium.org/9838064/18011
Try job failure for 9838064-18011 (retry) on linux_rel for step "update". It's a second try, previously, step "update" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&... Step "update" is always a major failure. Look at the try server FAQ for more details. |
