Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
848993002:
Improve apps v2 upgrade UX (Closed)
DescriptionImprove apps v2 upgrade UX
The current v1 web-app allows users to sign in as any user. Some users may
be signed in using a different account than their chrome profile.
When these users upgrade to the v2 app, their host list will be empty and
it is not straightforward why.
This CL shows a migration tip to inform the users to sign-in to their
previous account if necessary.
BUG=369835
Committed: https://crrev.com/ef56d877133d43c368ba50cf3ced6c39ec2f9c6c
Cr-Commit-Position: refs/heads/master@{#312549}
Patch Set 1 : #
Total comments: 24
Patch Set 2 : Address CL feedback #
Total comments: 16
Patch Set 3 : Refactor + UT #
Total comments: 27
Patch Set 4 : Address CL feedback #
Total comments: 10
Patch Set 5 : Address outstanding feedbacks #
Messages
Total messages: 17 (5 generated)
kelvinp@chromium.org changed reviewers: + jamiewalch@chromium.org
Patchset #1 (id:1) has been deleted
PTAL https://codereview.chromium.org/848993002/diff/20001/remoting/webapp/crd/js/r... File remoting/webapp/crd/js/remoting.js (left): https://codereview.chromium.org/848993002/diff/20001/remoting/webapp/crd/js/r... remoting/webapp/crd/js/remoting.js:248: 'remoting-email' We need to migrate 'remoting-email' as well. https://codereview.chromium.org/848993002/diff/20001/remoting/webapp/crd/js/r... File remoting/webapp/crd/js/remoting.js (right): https://codereview.chromium.org/848993002/diff/20001/remoting/webapp/crd/js/r... remoting/webapp/crd/js/remoting.js:90: remoting.onBeforeUnload = function() { The 'remoting-email' is not set when initGlobalObjects() got called on first-run. If we run the migration in initGlobalObjects(), we would need to at least run the app twice to the info properly cached. Doing the migration on beforeUnload mitigates the problem.
I haven't looked at all of this in detail because I suspect that my comment about the structure of the Promise might entail a fairly substantial refactor. https://codereview.chromium.org/848993002/diff/20001/remoting/resources/remot... File remoting/resources/remoting_strings.grd (right): https://codereview.chromium.org/848993002/diff/20001/remoting/resources/remot... remoting/resources/remoting_strings.grd:320: You have previously signed-in as <ph name="USER_NAME">$1<ex>John Doe</ex></ph> (<ph name="USER_EMAIL">$2<ex>john.doe@gmail.com</ex></ph>). To access your computers in that account, <ph name="LINK_BEGIN">$3<ex><a href=https://support.google.com/chrome/answer/2364824?hl=en></ex></ph>sign-in to chrome<ph name="LINK_END">$4<ex></a></ex></ph> with that account and re-install Chrome Remote Desktop. Quotes around the URL, please. It's only an example, but it might as well be valid HTML :) https://codereview.chromium.org/848993002/diff/20001/remoting/resources/remot... remoting/resources/remoting_strings.grd:320: You have previously signed-in as <ph name="USER_NAME">$1<ex>John Doe</ex></ph> (<ph name="USER_EMAIL">$2<ex>john.doe@gmail.com</ex></ph>). To access your computers in that account, <ph name="LINK_BEGIN">$3<ex><a href=https://support.google.com/chrome/answer/2364824?hl=en></ex></ph>sign-in to chrome<ph name="LINK_END">$4<ex></a></ex></ph> with that account and re-install Chrome Remote Desktop. No hyphen for "signed in" or "sign in" in this context, I don't think. https://codereview.chromium.org/848993002/diff/20001/remoting/resources/remot... remoting/resources/remoting_strings.grd:320: You have previously signed-in as <ph name="USER_NAME">$1<ex>John Doe</ex></ph> (<ph name="USER_EMAIL">$2<ex>john.doe@gmail.com</ex></ph>). To access your computers in that account, <ph name="LINK_BEGIN">$3<ex><a href=https://support.google.com/chrome/answer/2364824?hl=en></ex></ph>sign-in to chrome<ph name="LINK_END">$4<ex></a></ex></ph> with that account and re-install Chrome Remote Desktop. Capital C in Chrome. Also, I think we refer to it as Google Chrome elsewhere. https://codereview.chromium.org/848993002/diff/20001/remoting/resources/remot... remoting/resources/remoting_strings.grd:425: You have previously signed-in as <ph name="USER_NAME">$1<ex>John Doe</ex></ph> (<ph name="USER_EMAIL">$2<ex>john.doe@gmail.com</ex></ph>). To access your computers in that account, <ph name="LINK_BEGIN">$3<ex><a href=https://support.google.com/chrome/answer/2364824?hl=en></ex></ph>sign-in to chrome<ph name="LINK_END">$4<ex></a></ex></ph> with that account and re-install Chromoting. s/chrome/Chromium/ for the open source version. https://codereview.chromium.org/848993002/diff/20001/remoting/webapp/base/htm... File remoting/webapp/base/html/main.css (left): https://codereview.chromium.org/848993002/diff/20001/remoting/webapp/base/htm... remoting/webapp/base/html/main.css:359: } Why is this CSS no longer needed? https://codereview.chromium.org/848993002/diff/20001/remoting/webapp/crd/js/h... File remoting/webapp/crd/js/host_list.js (right): https://codereview.chromium.org/848993002/diff/20001/remoting/webapp/crd/js/h... remoting/webapp/crd/js/host_list.js:300: return; This doesn't seem right. migrationTipShown==true suggests to me that migration tips are (already) shown, which seems to be backed up by the early return, but that's not what the docs for showMigrationTips states. It seems the wrong way round. https://codereview.chromium.org/848993002/diff/20001/remoting/webapp/crd/js/h... remoting/webapp/crd/js/host_list.js:303: var localize = l10n.getTranslationOrError; Please don't assign these shortened aliases if you can avoid it. It took me a while to work out where localize was defined, and once you've added in the annotation, having a shorter function name isn't going to save you a line-break anyway, https://codereview.chromium.org/848993002/diff/20001/remoting/webapp/crd/js/h... remoting/webapp/crd/js/host_list.js:304: var buttonLabel = localize('HOME_DAEMON_START_BUTTON'); Please annotate these strings with /**i18n-content*/ in-keeping with other strings in JS (look elsewhere for the exact syntax), otherwise the verify_resources script won't know the strings are in use (and won't protect you against typos). https://codereview.chromium.org/848993002/diff/20001/remoting/webapp/crd/js/h... remoting/webapp/crd/js/host_list.js:323: return Promise.resolve(false); I don't have a strong opinion either way, but since this method is called showHostMigrationTips (ie, the sort of thing one might call a Boolean variable), might it be better to resolve or reject, rather than resolving with true or false? In other words: showHostMigrationTip().then( <yes>, <no> ); https://codereview.chromium.org/848993002/diff/20001/remoting/webapp/crd/js/h... remoting/webapp/crd/js/host_list.js:383: noHosts.innerHTML = buildMigrationTips(cachedEmail, cachedName); I don't think this is the right way to do structure this. The naming of this function suggests that it tells the caller whether or not to display the migration tips, whereas what it actually does is show them itself. I prefer the former, since the code in the then() function also sets this text and it would be cleaner to have it collocated with this code.
Patchset #2 (id:40001) has been deleted
PTAL https://codereview.chromium.org/848993002/diff/20001/remoting/resources/remot... File remoting/resources/remoting_strings.grd (right): https://codereview.chromium.org/848993002/diff/20001/remoting/resources/remot... remoting/resources/remoting_strings.grd:320: You have previously signed-in as <ph name="USER_NAME">$1<ex>John Doe</ex></ph> (<ph name="USER_EMAIL">$2<ex>john.doe@gmail.com</ex></ph>). To access your computers in that account, <ph name="LINK_BEGIN">$3<ex><a href=https://support.google.com/chrome/answer/2364824?hl=en></ex></ph>sign-in to chrome<ph name="LINK_END">$4<ex></a></ex></ph> with that account and re-install Chrome Remote Desktop. On 2015/01/14 03:55:28, Jamie wrote: > Capital C in Chrome. Also, I think we refer to it as Google Chrome elsewhere. Done. https://codereview.chromium.org/848993002/diff/20001/remoting/resources/remot... remoting/resources/remoting_strings.grd:320: You have previously signed-in as <ph name="USER_NAME">$1<ex>John Doe</ex></ph> (<ph name="USER_EMAIL">$2<ex>john.doe@gmail.com</ex></ph>). To access your computers in that account, <ph name="LINK_BEGIN">$3<ex><a href=https://support.google.com/chrome/answer/2364824?hl=en></ex></ph>sign-in to chrome<ph name="LINK_END">$4<ex></a></ex></ph> with that account and re-install Chrome Remote Desktop. On 2015/01/14 03:55:28, Jamie wrote: > No hyphen for "signed in" or "sign in" in this context, I don't think. Done. https://codereview.chromium.org/848993002/diff/20001/remoting/resources/remot... remoting/resources/remoting_strings.grd:320: You have previously signed-in as <ph name="USER_NAME">$1<ex>John Doe</ex></ph> (<ph name="USER_EMAIL">$2<ex>john.doe@gmail.com</ex></ph>). To access your computers in that account, <ph name="LINK_BEGIN">$3<ex><a href=https://support.google.com/chrome/answer/2364824?hl=en></ex></ph>sign-in to chrome<ph name="LINK_END">$4<ex></a></ex></ph> with that account and re-install Chrome Remote Desktop. On 2015/01/14 03:55:29, Jamie wrote: > Quotes around the URL, please. It's only an example, but it might as well be > valid HTML :) Done. https://codereview.chromium.org/848993002/diff/20001/remoting/resources/remot... remoting/resources/remoting_strings.grd:425: You have previously signed-in as <ph name="USER_NAME">$1<ex>John Doe</ex></ph> (<ph name="USER_EMAIL">$2<ex>john.doe@gmail.com</ex></ph>). To access your computers in that account, <ph name="LINK_BEGIN">$3<ex><a href=https://support.google.com/chrome/answer/2364824?hl=en></ex></ph>sign-in to chrome<ph name="LINK_END">$4<ex></a></ex></ph> with that account and re-install Chromoting. On 2015/01/14 03:55:29, Jamie wrote: > s/chrome/Chromium/ for the open source version. Done. https://codereview.chromium.org/848993002/diff/20001/remoting/webapp/crd/js/h... File remoting/webapp/crd/js/host_list.js (right): https://codereview.chromium.org/848993002/diff/20001/remoting/webapp/crd/js/h... remoting/webapp/crd/js/host_list.js:300: return; On 2015/01/14 03:55:29, Jamie wrote: > This doesn't seem right. migrationTipShown==true suggests to me that migration > tips are (already) shown, which seems to be backed up by the early return, but > that's not what the docs for showMigrationTips states. It seems the wrong way > round. I have change the structure of the code now. Let me know if the new structure doesn't address your concerns. https://codereview.chromium.org/848993002/diff/20001/remoting/webapp/crd/js/h... remoting/webapp/crd/js/host_list.js:303: var localize = l10n.getTranslationOrError; On 2015/01/14 03:55:29, Jamie wrote: > Please don't assign these shortened aliases if you can avoid it. It took me a > while to work out where localize was defined, and once you've added in the > annotation, having a shorter function name isn't going to save you a line-break > anyway, Done. https://codereview.chromium.org/848993002/diff/20001/remoting/webapp/crd/js/h... remoting/webapp/crd/js/host_list.js:304: var buttonLabel = localize('HOME_DAEMON_START_BUTTON'); On 2015/01/14 03:55:29, Jamie wrote: > Please annotate these strings with /**i18n-content*/ in-keeping with other > strings in JS (look elsewhere for the exact syntax), otherwise the > verify_resources script won't know the strings are in use (and won't protect you > against typos). Done. https://codereview.chromium.org/848993002/diff/20001/remoting/webapp/crd/js/h... remoting/webapp/crd/js/host_list.js:323: return Promise.resolve(false); On 2015/01/14 03:55:29, Jamie wrote: > I don't have a strong opinion either way, but since this method is called > showHostMigrationTips (ie, the sort of thing one might call a Boolean variable), > might it be better to resolve or reject, rather than resolving with true or > false? In other words: > > showHostMigrationTip().then( > <yes>, > <no> > ); I have renamed the function to WasSignedWithDifferentAcount https://codereview.chromium.org/848993002/diff/20001/remoting/webapp/crd/js/h... remoting/webapp/crd/js/host_list.js:383: noHosts.innerHTML = buildMigrationTips(cachedEmail, cachedName); On 2015/01/14 03:55:29, Jamie wrote: > I don't think this is the right way to do structure this. The naming of this > function suggests that it tells the caller whether or not to display the > migration tips, whereas what it actually does is show them itself. I prefer the > former, since the code in the then() function also sets this text and it would > be cleaner to have it collocated with this code. Done.
https://codereview.chromium.org/848993002/diff/20001/remoting/webapp/crd/js/r... File remoting/webapp/crd/js/remoting.js (left): https://codereview.chromium.org/848993002/diff/20001/remoting/webapp/crd/js/r... remoting/webapp/crd/js/remoting.js:248: 'remoting-email' On 2015/01/13 23:05:47, kelvinp wrote: > We need to migrate 'remoting-email' as well. See above; this is not about migrating settings from v1 to v2, but about migrating between the legacy and new storage APIs. You shouldn't need to change any of it. https://codereview.chromium.org/848993002/diff/20001/remoting/webapp/crd/js/r... remoting/webapp/crd/js/remoting.js:255: window.localStorage.removeItem(setting); I don't think you wanted to remove this line. https://codereview.chromium.org/848993002/diff/20001/remoting/webapp/crd/js/r... File remoting/webapp/crd/js/remoting.js (right): https://codereview.chromium.org/848993002/diff/20001/remoting/webapp/crd/js/r... remoting/webapp/crd/js/remoting.js:90: remoting.onBeforeUnload = function() { On 2015/01/13 23:05:47, kelvinp wrote: > The 'remoting-email' is not set when initGlobalObjects() got called on > first-run. If we run the migration in initGlobalObjects(), we would need to at > least run the app twice to the info properly cached. Doing the migration on > beforeUnload mitigates the problem. This code is all about moving settings from window.localStorage to chrome.storage.local. It needs to happen at start of day because we only read settings from chrome.storage.local. If it simplifies this CL, I think it's probably safe to remove this migration, since we've been writing to chrome.storage.local for over a year now, but I think it should work without needing to change this at all. https://codereview.chromium.org/848993002/diff/60001/remoting/webapp/crd/js/h... File remoting/webapp/crd/js/host_list.js (right): https://codereview.chromium.org/848993002/diff/60001/remoting/webapp/crd/js/h... remoting/webapp/crd/js/host_list.js:305: remoting.HostList.prototype.showEmptyText_ = function(hostingSupported) { "Empty text" suggests that the text is empty, not that it's what we show when the host-list is empty. I suggest calling this showEmptyHostListMessage_. https://codereview.chromium.org/848993002/diff/60001/remoting/webapp/crd/js/h... remoting/webapp/crd/js/host_list.js:322: var emptyTextDiv = this.noHosts_; I'm not sure what the value of this assignment is. If you think that the member variable is mis-named (which I think is a good call), then it should be renamed everywhere, not aliased here. I'd also suggest something more explicit if you're planning on changing it, in keeping with the method name above; something like emptyHostListMessageDiv_, perhaps? https://codereview.chromium.org/848993002/diff/60001/remoting/webapp/crd/js/h... remoting/webapp/crd/js/host_list.js:358: return Promise.resolve(null); I think my original objection still applies. For the function name you've chosen, I would expect a Promise that resolves if the user is signed in with a different identity and rejects if not. It would also make the then() function a bit cleaner, because you wouldn't need to cast the previous information for Boolean for the conditional, and you wouldn't need to translate HOME_DAEMON_START_BUTTON for the "yes" case, where it's not needed. https://codereview.chromium.org/848993002/diff/60001/remoting/webapp/crd/js/h... remoting/webapp/crd/js/host_list.js:361: var getCachedInfo = new Promise( s/Info/UserInfo/ You could also consider s/Cached/AppsV1/ since the fact that it's cached is an implementation detail--what we're really interested in is the fact that it's the previous (apps v1) identity. https://codereview.chromium.org/848993002/diff/60001/remoting/webapp/crd/js/h... remoting/webapp/crd/js/host_list.js:365: ['remoting-email','remoting-fullname',remoting.HostList.HOSTS_KEY], Nit: Spaces between list elements. https://codereview.chromium.org/848993002/diff/60001/remoting/webapp/crd/js/h... remoting/webapp/crd/js/host_list.js:365: ['remoting-email','remoting-fullname',remoting.HostList.HOSTS_KEY], These strings are duplicated. You should define them as constants, and I think it would be better if they were more explicit about their purpose; perhaps 'remoting-appsv2-migration-email'. If you prefer, you could store them under a single key: remoting-appsv2-migration: { email: ..., fullname: ... } https://codereview.chromium.org/848993002/diff/60001/remoting/webapp/crd/js/h... remoting/webapp/crd/js/host_list.js:391: var cached = /** @type {Object} */ (results[0]); s/cached/cachedUserInfo/ https://codereview.chromium.org/848993002/diff/60001/remoting/webapp/crd/js/h... remoting/webapp/crd/js/host_list.js:575: } I don't think this logic is correct. It will prevent the app from overwriting a previous non-empty host-list with an empty one. I assume you're doing it to make the "different account" message persistent, but I think that can be done less invasively, for example by not loading the host-list until that check has completed, or storing a separate Boolean to indicate whether or not there were hosts that only the v1 app sets. https://codereview.chromium.org/848993002/diff/60001/remoting/webapp/crd/js/h... remoting/webapp/crd/js/host_list.js:580: chrome.storage.local.remove('remoting-email'); Clearing this here seems a bit ad-hoc. I think you've added enough code to this class, and with sufficiently self-contained functionality that it would be cleaner to pull it out into a separate class. Off the top of my head (i.e, don't feel bound by this), the API would be something like this: remoting.AppsV2Migration = { userHasHosts: function() { // apps v1 would save the required state to support migration. // apps v2 would delete the state. }, getEmptyHostListMessage: function() { // This would be pretty what you've already written. } }; With a suitable storage mock, this would be very simple to write a unit-test for completely isolated from any tests for host-list. WDYT?
Patchset #3 (id:80001) has been deleted
PTAL https://codereview.chromium.org/848993002/diff/60001/remoting/webapp/crd/js/h... File remoting/webapp/crd/js/host_list.js (right): https://codereview.chromium.org/848993002/diff/60001/remoting/webapp/crd/js/h... remoting/webapp/crd/js/host_list.js:305: remoting.HostList.prototype.showEmptyText_ = function(hostingSupported) { On 2015/01/15 20:51:30, Jamie wrote: > "Empty text" suggests that the text is empty, not that it's what we show when > the host-list is empty. I suggest calling this showEmptyHostListMessage_. Done. https://codereview.chromium.org/848993002/diff/60001/remoting/webapp/crd/js/h... remoting/webapp/crd/js/host_list.js:322: var emptyTextDiv = this.noHosts_; On 2015/01/15 20:51:30, Jamie wrote: > I'm not sure what the value of this assignment is. If you think that the member > variable is mis-named (which I think is a good call), then it should be renamed > everywhere, not aliased here. I'd also suggest something more explicit if you're > planning on changing it, in keeping with the method name above; something like > emptyHostListMessageDiv_, perhaps? This variable is necessary for the annonymous function in the then block to access it on line 334. I can rename it to var that = this to make the intention clearer. https://codereview.chromium.org/848993002/diff/60001/remoting/webapp/crd/js/h... remoting/webapp/crd/js/host_list.js:358: return Promise.resolve(null); On 2015/01/15 20:51:31, Jamie wrote: > I think my original objection still applies. For the function name you've > chosen, I would expect a Promise that resolves if the user is signed in with a > different identity and rejects if not. It would also make the then() function a > bit cleaner, because you wouldn't need to cast the previous information for > Boolean for the conditional, and you wouldn't need to translate > HOME_DAEMON_START_BUTTON for the "yes" case, where it's not needed. Done. https://codereview.chromium.org/848993002/diff/60001/remoting/webapp/crd/js/h... remoting/webapp/crd/js/host_list.js:361: var getCachedInfo = new Promise( On 2015/01/15 20:51:30, Jamie wrote: > s/Info/UserInfo/ > > You could also consider s/Cached/AppsV1/ since the fact that it's cached is an > implementation detail--what we're really interested in is the fact that it's the > previous (apps v1) identity. Done. https://codereview.chromium.org/848993002/diff/60001/remoting/webapp/crd/js/h... remoting/webapp/crd/js/host_list.js:365: ['remoting-email','remoting-fullname',remoting.HostList.HOSTS_KEY], On 2015/01/15 20:51:30, Jamie wrote: > Nit: Spaces between list elements. Done. https://codereview.chromium.org/848993002/diff/60001/remoting/webapp/crd/js/h... remoting/webapp/crd/js/host_list.js:575: } On 2015/01/15 20:51:30, Jamie wrote: > I don't think this logic is correct. It will prevent the app from overwriting a > previous non-empty host-list with an empty one. I assume you're doing it to make > the "different account" message persistent, but I think that can be done less > invasively, for example by not loading the host-list until that check has > completed, or storing a separate Boolean to indicate whether or not there were > hosts that only the v1 app sets. Done. https://codereview.chromium.org/848993002/diff/60001/remoting/webapp/crd/js/h... remoting/webapp/crd/js/host_list.js:580: chrome.storage.local.remove('remoting-email'); On 2015/01/15 20:51:31, Jamie wrote: > Clearing this here seems a bit ad-hoc. I think you've added enough code to this > class, and with sufficiently self-contained functionality that it would be > cleaner to pull it out into a separate class. Off the top of my head (i.e, don't > feel bound by this), the API would be something like this: > > remoting.AppsV2Migration = { > userHasHosts: function() { > // apps v1 would save the required state to support migration. > // apps v2 would delete the state. > }, > getEmptyHostListMessage: function() { > // This would be pretty what you've already written. > } > }; > > With a suitable storage mock, this would be very simple to write a unit-test for > completely isolated from any tests for host-list. > > WDYT? Done.
This is looking pretty good now. Most of these comments are nits, but I'd like to remove the (explicit) dependency on window.localStorage if possible. https://codereview.chromium.org/848993002/diff/100001/remoting/webapp/crd/js/... File remoting/webapp/crd/js/apps_v2_migration.js (right): https://codereview.chromium.org/848993002/diff/100001/remoting/webapp/crd/js/... remoting/webapp/crd/js/apps_v2_migration.js:29: remoting.MigrationSettings.prototype.hasHosts = /** @type {boolean}*/ (false); These should be member variables, not prototype variables; ie, assign them in the ctor. https://codereview.chromium.org/848993002/diff/100001/remoting/webapp/crd/js/... remoting/webapp/crd/js/apps_v2_migration.js:29: remoting.MigrationSettings.prototype.hasHosts = /** @type {boolean}*/ (false); Is it necessary to save hasHosts, or could we just make the existence of the MigrationSettings object an implicit indicator that there were hosts? https://codereview.chromium.org/848993002/diff/100001/remoting/webapp/crd/js/... remoting/webapp/crd/js/apps_v2_migration.js:49: * @return {Promise} A Promise object that would resolve to the email of the s/email/email and full name/ (or change it so that it only returns the email if that's all you need). https://codereview.chromium.org/848993002/diff/100001/remoting/webapp/crd/js/... remoting/webapp/crd/js/apps_v2_migration.js:68: * { email: string, fullName: string} if the user has signed in with a Nit: No space after "{". https://codereview.chromium.org/848993002/diff/100001/remoting/webapp/crd/js/... remoting/webapp/crd/js/apps_v2_migration.js:68: * { email: string, fullName: string} if the user has signed in with a s/has signed in /has previously signed-in to the v1 app/ https://codereview.chromium.org/848993002/diff/100001/remoting/webapp/crd/js/... remoting/webapp/crd/js/apps_v2_migration.js:84: return { Is this equivalent to return Promise.resolve(...)? If so, then I think it would be clearer. https://codereview.chromium.org/848993002/diff/100001/remoting/webapp/crd/js/... remoting/webapp/crd/js/apps_v2_migration.js:109: /** @param {boolean} hasHosts */ The behaviour of this function is quite subtle, so I think it's worth enumerating the possibilities of (hasHost, isAppsV2); what it does, and why. https://codereview.chromium.org/848993002/diff/100001/remoting/webapp/crd/js/... remoting/webapp/crd/js/apps_v2_migration.js:118: var preference = /**@type{remoting.MigrationSettings}*/ ({}); This would be clearer if the MigrationSettings ctor took email, fullName and hasHosts as parameters. https://codereview.chromium.org/848993002/diff/100001/remoting/webapp/crd/js/... remoting/webapp/crd/js/apps_v2_migration.js:120: preference.fullName = window.localStorage['remoting-fullname']; Can these be passed in to the function? Using window.localStorage and chrome.storage.local in the same function looks odd, and it doesn't seem right that this function needs to know where they are stored. It would also make your unit test simpler, I think. If you don't want to pass them as function parameters, I think it would be better to use the getUserInfo API to retrieve them. https://codereview.chromium.org/848993002/diff/100001/remoting/webapp/crd/js/... File remoting/webapp/crd/js/host_list.js (right): https://codereview.chromium.org/848993002/diff/100001/remoting/webapp/crd/js/... remoting/webapp/crd/js/host_list.js:310: * @param {{ email: string, fullName: string}} previousIdentity Nit: No space before "email". Or it might be cleaner to use remoting.MigrationSettings. https://codereview.chromium.org/848993002/diff/100001/remoting/webapp/crd/js/... File remoting/webapp/crd/js/remoting.js (left): https://codereview.chromium.org/848993002/diff/100001/remoting/webapp/crd/js/... remoting/webapp/crd/js/remoting.js:25: migrateLocalToChromeStorage_(); I don't think this code needs to be removed. It's probably not needed any more, but removing it should not be a part of this CL. https://codereview.chromium.org/848993002/diff/100001/remoting/webapp/unittes... File remoting/webapp/unittests/apps_v2_migration_unittest.js (right): https://codereview.chromium.org/848993002/diff/100001/remoting/webapp/unittes... remoting/webapp/unittests/apps_v2_migration_unittest.js:29: function setup_(v1UserName, v1UserEmail, currentEmail, v1HasHosts) { Assuming you need this because the module's setup method can't take parameters, it would be better if as much as possible of the set-up code were moved into that method, with the remainder having a more specific name (setMigrationData, perhaps?) https://codereview.chromium.org/848993002/diff/100001/remoting/webapp/unittes... remoting/webapp/unittests/apps_v2_migration_unittest.js:33: remoting.identity = {}; There is already a MockIdentity class you can use here. https://codereview.chromium.org/848993002/diff/100001/remoting/webapp/unittes... remoting/webapp/unittests/apps_v2_migration_unittest.js:72: 'hasHostsInV1App() should reject always reject the promise in v1', Duplicate "reject". https://codereview.chromium.org/848993002/diff/100001/remoting/webapp/unittes... remoting/webapp/unittests/apps_v2_migration_unittest.js:98: remoting.AppsV2Migration.savePreferences(true); As stated earlier, there is some subtlety to this method, and I think it's worth testing the various (hasHost, isAppsV2) scenarios.
PTAL https://codereview.chromium.org/848993002/diff/100001/remoting/webapp/crd/js/... File remoting/webapp/crd/js/apps_v2_migration.js (right): https://codereview.chromium.org/848993002/diff/100001/remoting/webapp/crd/js/... remoting/webapp/crd/js/apps_v2_migration.js:29: remoting.MigrationSettings.prototype.hasHosts = /** @type {boolean}*/ (false); On 2015/01/20 22:32:01, Jamie wrote: > Is it necessary to save hasHosts, or could we just make the existence of the > MigrationSettings object an implicit indicator that there were hosts? Done. https://codereview.chromium.org/848993002/diff/100001/remoting/webapp/crd/js/... remoting/webapp/crd/js/apps_v2_migration.js:29: remoting.MigrationSettings.prototype.hasHosts = /** @type {boolean}*/ (false); On 2015/01/20 22:32:01, Jamie wrote: > Is it necessary to save hasHosts, or could we just make the existence of the > MigrationSettings object an implicit indicator that there were hosts? Done. https://codereview.chromium.org/848993002/diff/100001/remoting/webapp/crd/js/... remoting/webapp/crd/js/apps_v2_migration.js:49: * @return {Promise} A Promise object that would resolve to the email of the On 2015/01/20 22:32:01, Jamie wrote: > s/email/email and full name/ (or change it so that it only returns the email if > that's all you need). Done. https://codereview.chromium.org/848993002/diff/100001/remoting/webapp/crd/js/... remoting/webapp/crd/js/apps_v2_migration.js:68: * { email: string, fullName: string} if the user has signed in with a On 2015/01/20 22:32:01, Jamie wrote: > s/has signed in /has previously signed-in to the v1 app/ Done. https://codereview.chromium.org/848993002/diff/100001/remoting/webapp/crd/js/... remoting/webapp/crd/js/apps_v2_migration.js:84: return { On 2015/01/20 22:32:01, Jamie wrote: > Is this equivalent to return Promise.resolve(...)? If so, then I think it would > be clearer. Done. https://codereview.chromium.org/848993002/diff/100001/remoting/webapp/crd/js/... remoting/webapp/crd/js/apps_v2_migration.js:109: /** @param {boolean} hasHosts */ On 2015/01/20 22:32:01, Jamie wrote: > The behaviour of this function is quite subtle, so I think it's worth > enumerating the possibilities of (hasHost, isAppsV2); what it does, and why. Done. https://codereview.chromium.org/848993002/diff/100001/remoting/webapp/crd/js/... remoting/webapp/crd/js/apps_v2_migration.js:120: preference.fullName = window.localStorage['remoting-fullname']; On 2015/01/20 22:32:01, Jamie wrote: > Can these be passed in to the function? Using window.localStorage and > chrome.storage.local in the same function looks odd, and it doesn't seem right > that this function needs to know where they are stored. It would also make your > unit test simpler, I think. > > If you don't want to pass them as function parameters, I think it would be > better to use the getUserInfo API to retrieve them. Done. https://codereview.chromium.org/848993002/diff/100001/remoting/webapp/crd/js/... File remoting/webapp/crd/js/host_list.js (right): https://codereview.chromium.org/848993002/diff/100001/remoting/webapp/crd/js/... remoting/webapp/crd/js/host_list.js:310: * @param {{ email: string, fullName: string}} previousIdentity On 2015/01/20 22:32:01, Jamie wrote: > Nit: No space before "email". Or it might be cleaner to use > remoting.MigrationSettings. Done. https://codereview.chromium.org/848993002/diff/100001/remoting/webapp/crd/js/... File remoting/webapp/crd/js/remoting.js (left): https://codereview.chromium.org/848993002/diff/100001/remoting/webapp/crd/js/... remoting/webapp/crd/js/remoting.js:25: migrateLocalToChromeStorage_(); On 2015/01/20 22:32:01, Jamie wrote: > I don't think this code needs to be removed. It's probably not needed any more, > but removing it should not be a part of this CL. I think it is a good practice to clean up dead code and this is a small enough change to include this CL. I feel like if I leave this code in, I probably won't have time to get to it. https://codereview.chromium.org/848993002/diff/100001/remoting/webapp/unittes... File remoting/webapp/unittests/apps_v2_migration_unittest.js (right): https://codereview.chromium.org/848993002/diff/100001/remoting/webapp/unittes... remoting/webapp/unittests/apps_v2_migration_unittest.js:33: remoting.identity = {}; On 2015/01/20 22:32:01, Jamie wrote: > There is already a MockIdentity class you can use here. The mock identify class mocks chrome.identity but not remoting.identity. https://codereview.chromium.org/848993002/diff/100001/remoting/webapp/unittes... remoting/webapp/unittests/apps_v2_migration_unittest.js:72: 'hasHostsInV1App() should reject always reject the promise in v1', On 2015/01/20 22:32:01, Jamie wrote: > Duplicate "reject". Done. https://codereview.chromium.org/848993002/diff/100001/remoting/webapp/unittes... remoting/webapp/unittests/apps_v2_migration_unittest.js:98: remoting.AppsV2Migration.savePreferences(true); On 2015/01/20 22:32:01, Jamie wrote: > As stated earlier, there is some subtlety to this method, and I think it's worth > testing the various (hasHost, isAppsV2) scenarios. I have removed the hostHost parameter from the method, it is probably just worth testing for the v1 and v2 case
LGTM with nits. https://codereview.chromium.org/848993002/diff/120001/remoting/webapp/crd/js/... File remoting/webapp/crd/js/apps_v2_migration.js (right): https://codereview.chromium.org/848993002/diff/120001/remoting/webapp/crd/js/... remoting/webapp/crd/js/apps_v2_migration.js:72: * {email: string, fullName: string} if the user has previously signed-in to The promise should resolve to remoting.MigrationSettings. https://codereview.chromium.org/848993002/diff/120001/remoting/webapp/crd/js/... remoting/webapp/crd/js/apps_v2_migration.js:90: fullName: v1.fullName This should be new remoting.MigrationSettings. https://codereview.chromium.org/848993002/diff/120001/remoting/webapp/crd/js/... remoting/webapp/crd/js/apps_v2_migration.js:118: remoting.AppsV2Migration.savePreferences = function() { Now that it only saves the user info, I think saveUserInfo would be a better name, or perhaps saveMigrationSettings? If you do change this, please change loadSettings_ accordingly, and move it closer to this function for symmetry (in general, I try to keep "private" functions at the end of the file). https://codereview.chromium.org/848993002/diff/120001/remoting/webapp/unittes... File remoting/webapp/unittests/apps_v2_migration_unittest.js (right): https://codereview.chromium.org/848993002/diff/120001/remoting/webapp/unittes... remoting/webapp/unittests/apps_v2_migration_unittest.js:30: remoting.identity.getUserInfo = function(onDone, onError) { You're overriding this without restoring it at the end of the test. https://codereview.chromium.org/848993002/diff/120001/remoting/webapp/unittes... remoting/webapp/unittests/apps_v2_migration_unittest.js:32: return onDone(currentEmail); No need for "return", and you should specify a dummy fullName as well, since the function expects one.
The CQ bit was checked by kelvinp@chromium.org
FYI https://codereview.chromium.org/848993002/diff/120001/remoting/webapp/crd/js/... File remoting/webapp/crd/js/apps_v2_migration.js (right): https://codereview.chromium.org/848993002/diff/120001/remoting/webapp/crd/js/... remoting/webapp/crd/js/apps_v2_migration.js:72: * {email: string, fullName: string} if the user has previously signed-in to On 2015/01/21 22:02:20, Jamie wrote: > The promise should resolve to remoting.MigrationSettings. Done. https://codereview.chromium.org/848993002/diff/120001/remoting/webapp/crd/js/... remoting/webapp/crd/js/apps_v2_migration.js:90: fullName: v1.fullName On 2015/01/21 22:02:20, Jamie wrote: > This should be new remoting.MigrationSettings. Done. https://codereview.chromium.org/848993002/diff/120001/remoting/webapp/crd/js/... remoting/webapp/crd/js/apps_v2_migration.js:118: remoting.AppsV2Migration.savePreferences = function() { On 2015/01/21 22:02:20, Jamie wrote: > Now that it only saves the user info, I think saveUserInfo would be a better > name, or perhaps saveMigrationSettings? If you do change this, please change > loadSettings_ accordingly, and move it closer to this function for symmetry (in > general, I try to keep "private" functions at the end of the file). Done. https://codereview.chromium.org/848993002/diff/120001/remoting/webapp/unittes... File remoting/webapp/unittests/apps_v2_migration_unittest.js (right): https://codereview.chromium.org/848993002/diff/120001/remoting/webapp/unittes... remoting/webapp/unittests/apps_v2_migration_unittest.js:30: remoting.identity.getUserInfo = function(onDone, onError) { On 2015/01/21 22:02:20, Jamie wrote: > You're overriding this without restoring it at the end of the test. remoting.identity is null to begin with, and we are restoring it back to null https://codereview.chromium.org/848993002/diff/120001/remoting/webapp/unittes... remoting/webapp/unittests/apps_v2_migration_unittest.js:32: return onDone(currentEmail); On 2015/01/21 22:02:20, Jamie wrote: > No need for "return", and you should specify a dummy fullName as well, since the > function expects one. Done.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/848993002/140001
Message was sent while issue was closed.
Committed patchset #5 (id:140001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/ef56d877133d43c368ba50cf3ced6c39ec2f9c6c Cr-Commit-Position: refs/heads/master@{#312549} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
