Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2077473002:
MD History: add sign in promo in synced tabs when user isn't logged in (Closed)
|
Created:
4 years, 6 months ago by lshang Modified:
4 years, 6 months ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, michaelpg+watch-options_chromium.org, michaelpg+watch-md-settings_chromium.org, Patrick Dubroy, michaelpg+watch-md-ui_chromium.org, dbeam+watch-history_chromium.org, pam+watch_chromium.org, arv+watch_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD History: add sign in promo in synced tabs when user isn't logged in
Show sign in promo when user is signed out. When user clicks to sign in,
show messages when it's loading synced tabs from backend or there is no
synced device found.
BUG=614595
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/8e52a260683db459e9adb524f124c3d8af9d937c
Cr-Commit-Position: refs/heads/master@{#401220}
Patch Set 1 #Patch Set 2 : show messages #Patch Set 3 : first acceptable version #
Total comments: 34
Patch Set 4 : rebase #
Total comments: 18
Patch Set 5 : address review comments #
Total comments: 8
Patch Set 6 : minor change #Patch Set 7 : rebase #Messages
Total messages: 28 (14 generated)
Description was changed from ========== MD History: add sign in promo in synced tabs when user isn't logged in Show sign in promo when user is signed out and there is no synced device history. The sign in promo will guide user to sign in to Chrome. After user has signed in, hide the sign in promo. BUG=614595 ========== to ========== MD History: add sign in promo in synced tabs when user isn't logged in Show sign in promo when user is signed out and there is no synced device history. The sign in promo will guide user to sign in to Chrome. After user has signed in, hide the sign in promo. BUG=614595 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Description was changed from ========== MD History: add sign in promo in synced tabs when user isn't logged in Show sign in promo when user is signed out and there is no synced device history. The sign in promo will guide user to sign in to Chrome. After user has signed in, hide the sign in promo. BUG=614595 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== MD History: add sign in promo in synced tabs when user isn't logged in Show sign in promo when user is signed out. When user clicks to sign in, show messages when it's loading synced tabs from backend or there is no synced device found. BUG=614595 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
lshang@chromium.org changed reviewers: + calamity@chromium.org, tsergeant@chromium.org
PTAL thanks!
https://codereview.chromium.org/2077473002/diff/80001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2077473002/diff/80001/chrome/app/generated_re... chrome/app/generated_resources.grd:460: + No synced devices found. String given to us by bettes@ was: "No synced tabs" (with no period at the end) https://codereview.chromium.org/2077473002/diff/80001/chrome/app/generated_re... chrome/app/generated_resources.grd:466: + SIGN IN TO CHROME Write this in normal titlecase ("Sign in to Chrome") and use CSS to display it in uppercase (paper-button does it by default, I think). https://codereview.chromium.org/2077473002/diff/80001/chrome/browser/browser_... File chrome/browser/browser_resources.grd (right): https://codereview.chromium.org/2077473002/diff/80001/chrome/browser/browser_... chrome/browser/browser_resources.grd:235: <include name="IDR_MD_HISTORY_100_NO_SYNCED_TAB_PNG" file="resources\md_history\images\100\no_synced_tab.png" type="BINDATA" /> Nit: Include 'images' in the names: IDR_MD_HISTORY_IMAGES_100_NO_SYNCED_TAB_PNG And sort this section alphabetically. https://codereview.chromium.org/2077473002/diff/80001/chrome/browser/resource... File chrome/browser/resources/md_history/app.js (right): https://codereview.chromium.org/2077473002/diff/80001/chrome/browser/resource... chrome/browser/resources/md_history/app.js:232: waitForUpgrade(syncedDeviceManagerElem).then(function() { There's no need to use waitForUpgrade here. That's only necessary in history.js, which runs before Polymer has started. In app.js (a polymer file), all the child elements of <history-app> will always be initialized (with one or two exceptions). https://codereview.chromium.org/2077473002/diff/80001/chrome/browser/resource... File chrome/browser/resources/md_history/synced_device_manager.html (right): https://codereview.chromium.org/2077473002/diff/80001/chrome/browser/resource... chrome/browser/resources/md_history/synced_device_manager.html:16: padding-top: var(--first-card-padding-top); It would be a good idea to move this padding out of :host and onto something which only affects the synced cards (perhaps add a new wrapper div around the dom-repeat). This way, the layout of the rest of the elements doesn't need to take this into account. I have a CL out at the moment which is going to change the amount of padding here, so it's good to keep it independent of that. https://codereview.chromium.org/2077473002/diff/80001/chrome/browser/resource... chrome/browser/resources/md_history/synced_device_manager.html:31: bottom: 0; It shouldn't be necessary to use absolute positioning here. Instead, try setting `height: 100%` on #no-synced-tabs and `class="centered-message"` should handle the rest. https://codereview.chromium.org/2077473002/diff/80001/chrome/browser/resource... chrome/browser/resources/md_history/synced_device_manager.html:77: <div> This div doesn't seem to be doing much. You can get rid of it if you set a couple of extra CSS properties on #sign-in-guide: flex-direction: column; align-items: center; https://codereview.chromium.org/2077473002/diff/80001/chrome/browser/resource... chrome/browser/resources/md_history/synced_device_manager.html:81: <a> Similarly, this <a> isn't doing anything and can be removed. https://codereview.chromium.org/2077473002/diff/80001/chrome/browser/resource... File chrome/browser/resources/md_history/synced_device_manager.js (right): https://codereview.chromium.org/2077473002/diff/80001/chrome/browser/resource... chrome/browser/resources/md_history/synced_device_manager.js:1: // Copyright 2016 The Chromium Authors. All rights reserved. I won't review this file until after calamity@ is done. https://codereview.chromium.org/2077473002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/md_history_ui.cc (right): https://codereview.chromium.org/2077473002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/md_history_ui.cc:91: source->AddResourcePath("images/100/no_synced_tab.png", Nit: Grouped these with the rest of the AddResourcePath lines below, sorted alphabetically https://codereview.chromium.org/2077473002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/md_history_ui.cc:144: web_ui->AddMessageHandler(new settings::PeopleHandler(profile)); Perhaps add a comment here explaining what this is used for.
https://codereview.chromium.org/2077473002/diff/80001/chrome/browser/browser_... File chrome/browser/browser_resources.grd (right): https://codereview.chromium.org/2077473002/diff/80001/chrome/browser/browser_... chrome/browser/browser_resources.grd:236: <include name="IDR_MD_HISTORY_200_NO_SYNCED_TAB_PNG" file="resources\md_history\images\200\no_synced_tab.png" type="BINDATA" /> Change the file name to sign_in_promo.png. https://codereview.chromium.org/2077473002/diff/80001/chrome/browser/resource... File chrome/browser/resources/md_history/synced_device_manager.js (right): https://codereview.chromium.org/2077473002/diff/80001/chrome/browser/resource... chrome/browser/resources/md_history/synced_device_manager.js:110: (this.syncedDevices_.length == 0); It would be 'more Polymer' to use a computed binding to change the text. The two things you want to compute are 'whether the message should be displayed at all' and 'what the message should be'. So it should be: noSyncedTabsMessage: function(signInState) { return loadTimeData.getString(signInState ? 'noSyncedResults' : 'loading'); } showNoSyncedTabsMessage: function(signInState, syncedDevicesLength) { return signInState && syncedDevicesLength == 0; } then in the html, ... hidden="[[!showNoSyncedTabsMessage(signInState_, syncedDevices_.length]]"> [[noSyncedTabsMessage(signInState_)]] </div> https://codereview.chromium.org/2077473002/diff/80001/chrome/browser/resource... chrome/browser/resources/md_history/synced_device_manager.js:150: return; You can remove this if you do the above since notifications won't propagate through when the value is the same. https://codereview.chromium.org/2077473002/diff/80001/chrome/browser/resource... chrome/browser/resources/md_history/synced_device_manager.js:156: this.syncedDevices_ = []; You should make a method called clearDisplayedSyncedDevices that just runs this.syncedDevices = []. It's getting confusing to remember why we need to clear both the session list _and_ the syncedDevices array. https://codereview.chromium.org/2077473002/diff/80001/chrome/browser/resource... chrome/browser/resources/md_history/synced_device_manager.js:157: this.updateSyncedDevices(this.sessionList); I don't think you'll need this line either. https://codereview.chromium.org/2077473002/diff/80001/chrome/browser/resource... chrome/browser/resources/md_history/synced_device_manager.js:162: } All of this is fairly nuanced, we should have a test in history_synced_tabs_test.js that makes sure the right things are being hidden and shown for different configurations of signin and sessions.
Patchset #4 (id:100001) has been deleted
Patchset #4 (id:120001) has been deleted
Tim, Chris, PTALA thanks! https://codereview.chromium.org/2077473002/diff/80001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2077473002/diff/80001/chrome/app/generated_re... chrome/app/generated_resources.grd:460: + No synced devices found. On 2016/06/17 06:22:02, tsergeant wrote: > String given to us by bettes@ was: > > "No synced tabs" > > (with no period at the end) Done. https://codereview.chromium.org/2077473002/diff/80001/chrome/app/generated_re... chrome/app/generated_resources.grd:466: + SIGN IN TO CHROME On 2016/06/17 06:22:02, tsergeant wrote: > Write this in normal titlecase ("Sign in to Chrome") and use CSS to display it > in uppercase (paper-button does it by default, I think). Done. Yes paper-button will change it to uppercase. https://codereview.chromium.org/2077473002/diff/80001/chrome/browser/browser_... File chrome/browser/browser_resources.grd (right): https://codereview.chromium.org/2077473002/diff/80001/chrome/browser/browser_... chrome/browser/browser_resources.grd:235: <include name="IDR_MD_HISTORY_100_NO_SYNCED_TAB_PNG" file="resources\md_history\images\100\no_synced_tab.png" type="BINDATA" /> On 2016/06/17 06:22:02, tsergeant wrote: > Nit: Include 'images' in the names: > > IDR_MD_HISTORY_IMAGES_100_NO_SYNCED_TAB_PNG > > And sort this section alphabetically. Done. https://codereview.chromium.org/2077473002/diff/80001/chrome/browser/browser_... chrome/browser/browser_resources.grd:236: <include name="IDR_MD_HISTORY_200_NO_SYNCED_TAB_PNG" file="resources\md_history\images\200\no_synced_tab.png" type="BINDATA" /> On 2016/06/17 06:25:45, calamity wrote: > Change the file name to sign_in_promo.png. Done. https://codereview.chromium.org/2077473002/diff/80001/chrome/browser/resource... File chrome/browser/resources/md_history/app.js (right): https://codereview.chromium.org/2077473002/diff/80001/chrome/browser/resource... chrome/browser/resources/md_history/app.js:232: waitForUpgrade(syncedDeviceManagerElem).then(function() { On 2016/06/17 06:22:02, tsergeant wrote: > There's no need to use waitForUpgrade here. > > That's only necessary in history.js, which runs before Polymer has started. > > In app.js (a polymer file), all the child elements of <history-app> will always > be initialized (with one or two exceptions). Done. https://codereview.chromium.org/2077473002/diff/80001/chrome/browser/resource... File chrome/browser/resources/md_history/synced_device_manager.html (right): https://codereview.chromium.org/2077473002/diff/80001/chrome/browser/resource... chrome/browser/resources/md_history/synced_device_manager.html:16: padding-top: var(--first-card-padding-top); On 2016/06/17 06:22:03, tsergeant wrote: > It would be a good idea to move this padding out of :host and onto something > which only affects the synced cards (perhaps add a new wrapper div around the > dom-repeat). > > This way, the layout of the rest of the elements doesn't need to take this into > account. I have a CL out at the moment which is going to change the amount of > padding here, so it's good to keep it independent of that. Done. I moved padding out of :host and onto the wrapper div synced-device-list. The padding will still be there even though there is no content in the list, so I added a hidden property binding to syncedDevices_.length to fully hide the div, if you are ok with this. https://codereview.chromium.org/2077473002/diff/80001/chrome/browser/resource... chrome/browser/resources/md_history/synced_device_manager.html:31: bottom: 0; On 2016/06/17 06:22:03, tsergeant wrote: > It shouldn't be necessary to use absolute positioning here. > > Instead, try setting `height: 100%` on #no-synced-tabs and > `class="centered-message"` should handle the rest. Done. It works! With a single line, instead of whole bunch of lines...(⊙＿⊙) https://codereview.chromium.org/2077473002/diff/80001/chrome/browser/resource... chrome/browser/resources/md_history/synced_device_manager.html:77: <div> On 2016/06/17 06:22:03, tsergeant wrote: > This div doesn't seem to be doing much. > > You can get rid of it if you set a couple of extra CSS properties on > #sign-in-guide: > > flex-direction: column; > align-items: center; Done. https://codereview.chromium.org/2077473002/diff/80001/chrome/browser/resource... chrome/browser/resources/md_history/synced_device_manager.html:81: <a> On 2016/06/17 06:22:03, tsergeant wrote: > Similarly, this <a> isn't doing anything and can be removed. Done. https://codereview.chromium.org/2077473002/diff/80001/chrome/browser/resource... File chrome/browser/resources/md_history/synced_device_manager.js (right): https://codereview.chromium.org/2077473002/diff/80001/chrome/browser/resource... chrome/browser/resources/md_history/synced_device_manager.js:110: (this.syncedDevices_.length == 0); On 2016/06/17 06:25:45, calamity wrote: > It would be 'more Polymer' to use a computed binding to change the text. > > The two things you want to compute are 'whether the message should be displayed > at all' and 'what the message should be'. > > So it should be: > > noSyncedTabsMessage: function(signInState) { > return loadTimeData.getString(signInState ? 'noSyncedResults' : 'loading'); > } > > showNoSyncedTabsMessage: function(signInState, syncedDevicesLength) { > return signInState && syncedDevicesLength == 0; > } > > then in the html, > > ... > hidden="[[!showNoSyncedTabsMessage(signInState_, syncedDevices_.length]]"> > [[noSyncedTabsMessage(signInState_)]] > </div> > Done. It's really cool that computed binding will be recalculated whenever any of the params changes. https://codereview.chromium.org/2077473002/diff/80001/chrome/browser/resource... chrome/browser/resources/md_history/synced_device_manager.js:150: return; On 2016/06/17 06:25:45, calamity wrote: > You can remove this if you do the above since notifications won't propagate > through when the value is the same. When user changes state from signed out to signed in, then there is a short period of time when it's fetching synced tabs and message 'Loading' is displayed, then after the data returns, message will be 'no synced tabs'. We want to make sure that 'Loading' only be displayed when user's state is changed from signed out to signed in. I did this because there are occasions when user has two Chrome windows with different accounts A and B, A is logged in and B is not logged in. when user also signs in account B, both A and B will receive updateSignInState notifications from backend, but we would only want B to show the 'Loading' message. https://codereview.chromium.org/2077473002/diff/80001/chrome/browser/resource... chrome/browser/resources/md_history/synced_device_manager.js:156: this.syncedDevices_ = []; On 2016/06/17 06:25:45, calamity wrote: > You should make a method called clearDisplayedSyncedDevices that just runs > this.syncedDevices = []. > > It's getting confusing to remember why we need to clear both the session list > _and_ the syncedDevices array. Done. Yeah I think we just want to clear the UI here. https://codereview.chromium.org/2077473002/diff/80001/chrome/browser/resource... chrome/browser/resources/md_history/synced_device_manager.js:157: this.updateSyncedDevices(this.sessionList); On 2016/06/17 06:25:45, calamity wrote: > I don't think you'll need this line either. Done. https://codereview.chromium.org/2077473002/diff/80001/chrome/browser/resource... chrome/browser/resources/md_history/synced_device_manager.js:162: } On 2016/06/17 06:25:45, calamity wrote: > All of this is fairly nuanced, we should have a test in > history_synced_tabs_test.js that makes sure the right things are being hidden > and shown for different configurations of signin and sessions. Done. https://codereview.chromium.org/2077473002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/md_history_ui.cc (right): https://codereview.chromium.org/2077473002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/md_history_ui.cc:91: source->AddResourcePath("images/100/no_synced_tab.png", On 2016/06/17 06:22:03, tsergeant wrote: > Nit: Grouped these with the rest of the AddResourcePath lines below, sorted > alphabetically Done. https://codereview.chromium.org/2077473002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/md_history_ui.cc:144: web_ui->AddMessageHandler(new settings::PeopleHandler(profile)); On 2016/06/17 06:22:03, tsergeant wrote: > Perhaps add a comment here explaining what this is used for. Done.
https://codereview.chromium.org/2077473002/diff/80001/chrome/browser/resource... File chrome/browser/resources/md_history/synced_device_manager.html (right): https://codereview.chromium.org/2077473002/diff/80001/chrome/browser/resource... chrome/browser/resources/md_history/synced_device_manager.html:16: padding-top: var(--first-card-padding-top); On 2016/06/21 03:00:26, Liu Shang wrote: > On 2016/06/17 06:22:03, tsergeant wrote: > > It would be a good idea to move this padding out of :host and onto something > > which only affects the synced cards (perhaps add a new wrapper div around the > > dom-repeat). > > > > This way, the layout of the rest of the elements doesn't need to take this > into > > account. I have a CL out at the moment which is going to change the amount of > > padding here, so it's good to keep it independent of that. > > Done. > I moved padding out of :host and onto the wrapper div synced-device-list. The > padding will still be there even though there is no content in the list, so I > added a hidden property binding to syncedDevices_.length to fully hide the div, > if you are ok with this. Yup, sounds good to me https://codereview.chromium.org/2077473002/diff/140001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2077473002/diff/140001/chrome/app/generated_r... chrome/app/generated_resources.grd:459: + <message name="IDS_MD_HISTORY_NO_SYNCED_RESULTS" desc="Text indicating that there is no synced tab from other devices."> Nit: "there are no synced tabs" in the description https://codereview.chromium.org/2077473002/diff/140001/chrome/browser/browser... File chrome/browser/browser_resources.grd (right): https://codereview.chromium.org/2077473002/diff/140001/chrome/browser/browser... chrome/browser/browser_resources.grd:231: <include name="IDR_MD_HISTORY_IMAGES_100_NO_SYNCED_TAB_PNG" file="resources\md_history\images\100\sign_in_promo.png" type="BINDATA" /> You should change the resource name to match the new filename (SIGN_IN_PROMO_PNG) https://codereview.chromium.org/2077473002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/md_history/synced_device_manager.html (right): https://codereview.chromium.org/2077473002/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_history/synced_device_manager.html:83: $i18n{signInButton} Nit: Deindent this so it's 2 spaces in from the parent tag. https://codereview.chromium.org/2077473002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/md_history/synced_device_manager.js (right): https://codereview.chromium.org/2077473002/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_history/synced_device_manager.js:107: * @param {number} syncedDevicesLength Add an @return https://codereview.chromium.org/2077473002/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_history/synced_device_manager.js:114: * Decide what message should be diaplayed when user is logged in and there is Minor nit: typo in "displayed" also, "there are no synced tabs" https://codereview.chromium.org/2077473002/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_history/synced_device_manager.js:116: * @param {boolean} fetchingSyncedTabs Add an @return as well https://codereview.chromium.org/2077473002/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_history/synced_device_manager.js:133: this.fetchingSyncedTabs_ = false; Just put this once at the very start of the method. Then you'll be able to remove the duplicate line below. https://codereview.chromium.org/2077473002/diff/140001/chrome/test/data/webui... File chrome/test/data/webui/md_history/history_synced_tabs_test.js (right): https://codereview.chromium.org/2077473002/diff/140001/chrome/test/data/webui... chrome/test/data/webui/md_history/history_synced_tabs_test.js:179: .then(function() { Put flush and then on the same line, then indent by two spaces. return flush().then(function() { blah() blah() } https://codereview.chromium.org/2077473002/diff/140001/chrome/test/data/webui... chrome/test/data/webui/md_history/history_synced_tabs_test.js:207: '\n Loading...\n ', Checking the string like this is quite means that this test won't work in non-English, and it will break if the spacing changes or the string changes. A better approach would be to use indexOf and loadTimeData: var loading = loadTimeData.getString('loading'); assertNotEquals(-1, element.$['no-synced-tabs'].textContent.indexOf(loading))
https://codereview.chromium.org/2077473002/diff/140001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2077473002/diff/140001/chrome/app/generated_r... chrome/app/generated_resources.grd:459: + <message name="IDS_MD_HISTORY_NO_SYNCED_RESULTS" desc="Text indicating that there is no synced tab from other devices."> On 2016/06/21 04:33:50, tsergeant wrote: > Nit: "there are no synced tabs" in the description Done. https://codereview.chromium.org/2077473002/diff/140001/chrome/browser/browser... File chrome/browser/browser_resources.grd (right): https://codereview.chromium.org/2077473002/diff/140001/chrome/browser/browser... chrome/browser/browser_resources.grd:231: <include name="IDR_MD_HISTORY_IMAGES_100_NO_SYNCED_TAB_PNG" file="resources\md_history\images\100\sign_in_promo.png" type="BINDATA" /> On 2016/06/21 04:33:50, tsergeant wrote: > You should change the resource name to match the new filename > (SIGN_IN_PROMO_PNG) Done. Also changed md_history_ui.cc correspondingly. https://codereview.chromium.org/2077473002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/md_history/synced_device_manager.html (right): https://codereview.chromium.org/2077473002/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_history/synced_device_manager.html:83: $i18n{signInButton} On 2016/06/21 04:33:50, tsergeant wrote: > Nit: Deindent this so it's 2 spaces in from the parent tag. Done. https://codereview.chromium.org/2077473002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/md_history/synced_device_manager.js (right): https://codereview.chromium.org/2077473002/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_history/synced_device_manager.js:107: * @param {number} syncedDevicesLength On 2016/06/21 04:33:51, tsergeant wrote: > Add an @return Done. https://codereview.chromium.org/2077473002/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_history/synced_device_manager.js:114: * Decide what message should be diaplayed when user is logged in and there is On 2016/06/21 04:33:51, tsergeant wrote: > Minor nit: typo in "displayed" > > also, > > "there are no synced tabs" Done. https://codereview.chromium.org/2077473002/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_history/synced_device_manager.js:116: * @param {boolean} fetchingSyncedTabs On 2016/06/21 04:33:51, tsergeant wrote: > Add an @return as well Done. https://codereview.chromium.org/2077473002/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_history/synced_device_manager.js:133: this.fetchingSyncedTabs_ = false; On 2016/06/21 04:33:51, tsergeant wrote: > Just put this once at the very start of the method. Then you'll be able to > remove the duplicate line below. Done. https://codereview.chromium.org/2077473002/diff/140001/chrome/test/data/webui... File chrome/test/data/webui/md_history/history_synced_tabs_test.js (right): https://codereview.chromium.org/2077473002/diff/140001/chrome/test/data/webui... chrome/test/data/webui/md_history/history_synced_tabs_test.js:179: .then(function() { On 2016/06/21 04:33:51, tsergeant wrote: > Put flush and then on the same line, then indent by two spaces. > > return flush().then(function() { > blah() > blah() > } Done. https://codereview.chromium.org/2077473002/diff/140001/chrome/test/data/webui... chrome/test/data/webui/md_history/history_synced_tabs_test.js:207: '\n Loading...\n ', On 2016/06/21 04:33:51, tsergeant wrote: > Checking the string like this is quite means that this test won't work in > non-English, and it will break if the spacing changes or the string changes. > > A better approach would be to use indexOf and loadTimeData: > > var loading = loadTimeData.getString('loading'); > assertNotEquals(-1, element.$['no-synced-tabs'].textContent.indexOf(loading)) Done.
lgtm, let's see what calamity@ thinks
lgtm Sweet! https://codereview.chromium.org/2077473002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/md_history/synced_device_manager.js (right): https://codereview.chromium.org/2077473002/diff/160001/chrome/browser/resourc... chrome/browser/resources/md_history/synced_device_manager.js:102: clearDisplayedSyncedDevices: function() { This can be private. https://codereview.chromium.org/2077473002/diff/160001/chrome/browser/resourc... chrome/browser/resources/md_history/synced_device_manager.js:168: return; nit: newline after return. https://codereview.chromium.org/2077473002/diff/160001/chrome/browser/resourc... chrome/browser/resources/md_history/synced_device_manager.js:173: this.clearDisplayedSyncedDevices(); nit: Use an early return here instead of if-else. https://codereview.chromium.org/2077473002/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/md_history_ui.cc (right): https://codereview.chromium.org/2077473002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_history_ui.cc:147: // Add handler for showing Chrome log in overlay. nit: s/log in/sign in/
Thanks all! https://codereview.chromium.org/2077473002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/md_history/synced_device_manager.js (right): https://codereview.chromium.org/2077473002/diff/160001/chrome/browser/resourc... chrome/browser/resources/md_history/synced_device_manager.js:102: clearDisplayedSyncedDevices: function() { On 2016/06/22 02:20:48, calamity wrote: > This can be private. Done. https://codereview.chromium.org/2077473002/diff/160001/chrome/browser/resourc... chrome/browser/resources/md_history/synced_device_manager.js:168: return; On 2016/06/22 02:20:48, calamity wrote: > nit: newline after return. Done. https://codereview.chromium.org/2077473002/diff/160001/chrome/browser/resourc... chrome/browser/resources/md_history/synced_device_manager.js:173: this.clearDisplayedSyncedDevices(); On 2016/06/22 02:20:48, calamity wrote: > nit: Use an early return here instead of if-else. Done. https://codereview.chromium.org/2077473002/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/md_history_ui.cc (right): https://codereview.chromium.org/2077473002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_history_ui.cc:147: // Add handler for showing Chrome log in overlay. On 2016/06/22 02:20:48, calamity wrote: > nit: s/log in/sign in/ Done.
The CQ bit was checked by lshang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tsergeant@chromium.org, calamity@chromium.org Link to the patchset: https://codereview.chromium.org/2077473002/#ps180001 (title: "minor change")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2077473002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by lshang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tsergeant@chromium.org, calamity@chromium.org Link to the patchset: https://codereview.chromium.org/2077473002/#ps200001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2077473002/200001
Message was sent while issue was closed.
Description was changed from ========== MD History: add sign in promo in synced tabs when user isn't logged in Show sign in promo when user is signed out. When user clicks to sign in, show messages when it's loading synced tabs from backend or there is no synced device found. BUG=614595 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== MD History: add sign in promo in synced tabs when user isn't logged in Show sign in promo when user is signed out. When user clicks to sign in, show messages when it's loading synced tabs from backend or there is no synced device found. BUG=614595 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #7 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== MD History: add sign in promo in synced tabs when user isn't logged in Show sign in promo when user is signed out. When user clicks to sign in, show messages when it's loading synced tabs from backend or there is no synced device found. BUG=614595 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== MD History: add sign in promo in synced tabs when user isn't logged in Show sign in promo when user is signed out. When user clicks to sign in, show messages when it's loading synced tabs from backend or there is no synced device found. BUG=614595 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/8e52a260683db459e9adb524f124c3d8af9d937c Cr-Commit-Position: refs/heads/master@{#401220} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/8e52a260683db459e9adb524f124c3d8af9d937c Cr-Commit-Position: refs/heads/master@{#401220} |
