Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2171513004:
MD History: Hide sign in guide in guest mode (Closed)
|
Created:
4 years, 5 months ago by lshang Modified:
4 years, 5 months ago CC:
chromium-reviews, Patrick Dubroy, michaelpg+watch-md-ui_chromium.org, dbeam+watch-history_chromium.org, pam+watch_chromium.org, arv+watch_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: Hide sign in guide in guest mode
Hide sign in guide in guest mode and show 'no synced tabs'.
BUG=628102
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/18fefd6452c2fae6251a0fb667c6c0e2c92ddbaa
Cr-Commit-Position: refs/heads/master@{#407009}
Patch Set 1 #Patch Set 2 : add some annotation #Patch Set 3 : add test #
Total comments: 10
Patch Set 4 : address review comments #
Total comments: 4
Patch Set 5 : add guestSession to arguments #
Total comments: 4
Patch Set 6 : format #
Messages
Total messages: 31 (13 generated)
Description was changed from ========== MD History: Hide sign in guide in guest mode BUG= ========== to ========== MD History: Hide sign in guide in guest mode BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== MD History: Hide sign in guide in guest mode BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD History: Hide sign in guide in guest mode Hide sign in guide in guest mode and show 'no synced tabs'. BUG=628102 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
lshang@chromium.org changed reviewers: + calamity@chromium.org, tsergeant@chromium.org
PTAL thanks! I also would like to add a test for this, but haven't found ways to set up a guest session in test environment, any ideas?
On 2016/07/21 00:45:25, Liu Shang wrote: > PTAL thanks! > > I also would like to add a test for this, but haven't found ways to set up a > guest session in test environment, any ideas? As discussed, probably just add a test for the JS side.
I added a test for this, PTAL thanks!
On 2016/07/21 04:32:07, tsergeant wrote: > On 2016/07/21 00:45:25, Liu Shang wrote: > > PTAL thanks! > > > > I also would like to add a test for this, but haven't found ways to set up a > > guest session in test environment, any ideas? > > As discussed, probably just add a test for the JS side. I don't have an opinion on how to test, but FYI it's easy to create a test in guest mode: just add the right switches. Thanks to a certain *brilliant* someone, you can do this in your JS browser test by setting the commandLineSwitches property: https://cs.chromium.org/chromium/src/chrome/test/base/js2gtest.js?q=js2gtest+... These are the switches you need (just find the string values of those constants): https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/options/chromeos...
https://codereview.chromium.org/2171513004/diff/40001/chrome/browser/resource... File chrome/browser/resources/md_history/synced_device_manager.js (right): https://codereview.chromium.org/2171513004/diff/40001/chrome/browser/resource... chrome/browser/resources/md_history/synced_device_manager.js:121: return true; nit: Newline after early return. https://codereview.chromium.org/2171513004/diff/40001/chrome/browser/resource... chrome/browser/resources/md_history/synced_device_manager.js:126: * Decide whether or not should hide sign in guide. * Hides the signin guide when the user is already signed in or in a guest session. https://codereview.chromium.org/2171513004/diff/40001/chrome/browser/resource... chrome/browser/resources/md_history/synced_device_manager.js:130: hideSignInGuide: function(signInState) { Maybe make this 'showSignInGuide' and invert everything. That would be consistent with showNoSyncedMessage above. https://codereview.chromium.org/2171513004/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/md_history/history_synced_tabs_test.js (right): https://codereview.chromium.org/2171513004/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/md_history/history_synced_tabs_test.js:267: element.$['no-synced-tabs'].textContent.indexOf(noSyncedResults)); I think it's enough to just check this is hidden. https://codereview.chromium.org/2171513004/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/md_history/history_synced_tabs_test.js:268: element.guestSession_ = false; Put this in the teardown?
On 2016/07/21 04:39:10, michaelpg wrote: > On 2016/07/21 04:32:07, tsergeant wrote: > > On 2016/07/21 00:45:25, Liu Shang wrote: > > > PTAL thanks! > > > > > > I also would like to add a test for this, but haven't found ways to set up a > > > guest session in test environment, any ideas? > > > > As discussed, probably just add a test for the JS side. > > I don't have an opinion on how to test, but FYI it's easy to create a test in > guest mode: just add the right switches. > > Thanks to a certain *brilliant* someone, you can do this in your JS browser test > by setting the commandLineSwitches property: > https://cs.chromium.org/chromium/src/chrome/test/base/js2gtest.js?q=js2gtest+... > > These are the switches you need (just find the string values of those > constants): > https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/options/chromeos... I totally forgot this was possible. Let's do this.
Patchset #4 (id:60001) has been deleted
Patchset #4 (id:80001) has been deleted
https://codereview.chromium.org/2171513004/diff/40001/chrome/browser/resource... File chrome/browser/resources/md_history/synced_device_manager.js (right): https://codereview.chromium.org/2171513004/diff/40001/chrome/browser/resource... chrome/browser/resources/md_history/synced_device_manager.js:121: return true; On 2016/07/21 04:56:49, calamity wrote: > nit: Newline after early return. Done. https://codereview.chromium.org/2171513004/diff/40001/chrome/browser/resource... chrome/browser/resources/md_history/synced_device_manager.js:126: * Decide whether or not should hide sign in guide. On 2016/07/21 04:56:49, calamity wrote: > * Hides the signin guide when the user is already signed in or in a guest > session. Done. https://codereview.chromium.org/2171513004/diff/40001/chrome/browser/resource... chrome/browser/resources/md_history/synced_device_manager.js:130: hideSignInGuide: function(signInState) { On 2016/07/21 04:56:49, calamity wrote: > Maybe make this 'showSignInGuide' and invert everything. That would be > consistent with showNoSyncedMessage above. Done. Yeah I was also thinking of this, this is probably better:-) https://codereview.chromium.org/2171513004/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/md_history/history_synced_tabs_test.js (right): https://codereview.chromium.org/2171513004/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/md_history/history_synced_tabs_test.js:267: element.$['no-synced-tabs'].textContent.indexOf(noSyncedResults)); On 2016/07/21 04:56:49, calamity wrote: > I think it's enough to just check this is hidden. Done. https://codereview.chromium.org/2171513004/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/md_history/history_synced_tabs_test.js:268: element.guestSession_ = false; On 2016/07/21 04:56:49, calamity wrote: > Put this in the teardown? Done.
On 2016/07/21 05:08:59, tsergeant wrote: > On 2016/07/21 04:39:10, michaelpg wrote: > > On 2016/07/21 04:32:07, tsergeant wrote: > > > On 2016/07/21 00:45:25, Liu Shang wrote: > > > > PTAL thanks! > > > > > > > > I also would like to add a test for this, but haven't found ways to set up > a > > > > guest session in test environment, any ideas? > > > > > > As discussed, probably just add a test for the JS side. > > > > I don't have an opinion on how to test, but FYI it's easy to create a test in > > guest mode: just add the right switches. > > > > Thanks to a certain *brilliant* someone, you can do this in your JS browser > test > > by setting the commandLineSwitches property: > > > https://cs.chromium.org/chromium/src/chrome/test/base/js2gtest.js?q=js2gtest+... > > > > These are the switches you need (just find the string values of those > > constants): > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/options/chromeos... > > I totally forgot this was possible. Let's do this. As discussed, since the switches for opening guest window only work on chromeos, we might just add a test for JS side. But this is a good reminder anyway:)
https://codereview.chromium.org/2171513004/diff/100001/chrome/browser/resourc... File chrome/browser/resources/md_history/synced_device_manager.html (right): https://codereview.chromium.org/2171513004/diff/100001/chrome/browser/resourc... chrome/browser/resources/md_history/synced_device_manager.html:76: hidden="[[!showNoSyncedMessage(signInState_, syncedDevices_.length)]]"> You should probably add guestSession here so that the value will update when guestSession changes: [[!showNoSyncedMessage(signInState_, syncedDevices_.length, guestSession)]] https://codereview.chromium.org/2171513004/diff/100001/chrome/browser/resourc... chrome/browser/resources/md_history/synced_device_manager.html:79: <div id="sign-in-guide" hidden$="[[!showSignInGuide(signInState_)]]"> And here: [[showSignInGuide(guestSession_, signInState_)]]
On 2016/07/21 08:02:43, tsergeant wrote: > https://codereview.chromium.org/2171513004/diff/100001/chrome/browser/resourc... > File chrome/browser/resources/md_history/synced_device_manager.html (right): > > https://codereview.chromium.org/2171513004/diff/100001/chrome/browser/resourc... > chrome/browser/resources/md_history/synced_device_manager.html:76: > hidden="[[!showNoSyncedMessage(signInState_, syncedDevices_.length)]]"> > You should probably add guestSession here so that the value will update when > guestSession changes: > > [[!showNoSyncedMessage(signInState_, syncedDevices_.length, guestSession)]] > > https://codereview.chromium.org/2171513004/diff/100001/chrome/browser/resourc... > chrome/browser/resources/md_history/synced_device_manager.html:79: <div > id="sign-in-guide" hidden$="[[!showSignInGuide(signInState_)]]"> > And here: > > [[showSignInGuide(guestSession_, signInState_)]] My observation is guest session will always be opened in a new window (and will close the window when 'Exit Guest'), so guestSession is determined when history page gets loaded and won't change. Also there is nowhere else could change its value, so I didn't add it to the arguments, but I'm ok to add it if you feel strongly about this:-)
On 2016/07/21 08:11:55, Liu Shang wrote: > On 2016/07/21 08:02:43, tsergeant wrote: > > > https://codereview.chromium.org/2171513004/diff/100001/chrome/browser/resourc... > > File chrome/browser/resources/md_history/synced_device_manager.html (right): > > > > > https://codereview.chromium.org/2171513004/diff/100001/chrome/browser/resourc... > > chrome/browser/resources/md_history/synced_device_manager.html:76: > > hidden="[[!showNoSyncedMessage(signInState_, syncedDevices_.length)]]"> > > You should probably add guestSession here so that the value will update when > > guestSession changes: > > > > [[!showNoSyncedMessage(signInState_, syncedDevices_.length, guestSession)]] > > > > > https://codereview.chromium.org/2171513004/diff/100001/chrome/browser/resourc... > > chrome/browser/resources/md_history/synced_device_manager.html:79: <div > > id="sign-in-guide" hidden$="[[!showSignInGuide(signInState_)]]"> > > And here: > > > > [[showSignInGuide(guestSession_, signInState_)]] > > My observation is guest session will always be opened in a new window (and will > close the window when 'Exit Guest'), so guestSession is determined when history > page gets loaded and won't change. Also there is nowhere else could change its > value, so I didn't add it to the arguments, but I'm ok to add it if you feel > strongly about this:-) The problem is that the test does explicitly change the value, and so it only works because you call updateSignInState after changing the guestSession value. I think I'd prefer if you did change it, just because this pattern feels like it would be a common source of bugs.
https://codereview.chromium.org/2171513004/diff/100001/chrome/browser/resourc... File chrome/browser/resources/md_history/synced_device_manager.html (right): https://codereview.chromium.org/2171513004/diff/100001/chrome/browser/resourc... chrome/browser/resources/md_history/synced_device_manager.html:76: hidden="[[!showNoSyncedMessage(signInState_, syncedDevices_.length)]]"> On 2016/07/21 08:02:43, tsergeant wrote: > You should probably add guestSession here so that the value will update when > guestSession changes: > > [[!showNoSyncedMessage(signInState_, syncedDevices_.length, guestSession)]] Done. https://codereview.chromium.org/2171513004/diff/100001/chrome/browser/resourc... chrome/browser/resources/md_history/synced_device_manager.html:79: <div id="sign-in-guide" hidden$="[[!showSignInGuide(signInState_)]]"> On 2016/07/21 08:02:43, tsergeant wrote: > And here: > > [[showSignInGuide(guestSession_, signInState_)]] Done.
On 2016/07/21 08:18:51, tsergeant wrote: > On 2016/07/21 08:11:55, Liu Shang wrote: > > On 2016/07/21 08:02:43, tsergeant wrote: > > > > > > https://codereview.chromium.org/2171513004/diff/100001/chrome/browser/resourc... > > > File chrome/browser/resources/md_history/synced_device_manager.html (right): > > > > > > > > > https://codereview.chromium.org/2171513004/diff/100001/chrome/browser/resourc... > > > chrome/browser/resources/md_history/synced_device_manager.html:76: > > > hidden="[[!showNoSyncedMessage(signInState_, syncedDevices_.length)]]"> > > > You should probably add guestSession here so that the value will update when > > > guestSession changes: > > > > > > [[!showNoSyncedMessage(signInState_, syncedDevices_.length, guestSession)]] > > > > > > > > > https://codereview.chromium.org/2171513004/diff/100001/chrome/browser/resourc... > > > chrome/browser/resources/md_history/synced_device_manager.html:79: <div > > > id="sign-in-guide" hidden$="[[!showSignInGuide(signInState_)]]"> > > > And here: > > > > > > [[showSignInGuide(guestSession_, signInState_)]] > > > > My observation is guest session will always be opened in a new window (and > will > > close the window when 'Exit Guest'), so guestSession is determined when > history > > page gets loaded and won't change. Also there is nowhere else could change its > > value, so I didn't add it to the arguments, but I'm ok to add it if you feel > > strongly about this:-) > > The problem is that the test does explicitly change the value, and so it only > works because you call updateSignInState after changing the guestSession value. > > I think I'd prefer if you did change it, just because this pattern feels like it > would be a common source of bugs. Yeah it makes sense:-)
lgtm with two nits https://codereview.chromium.org/2171513004/diff/120001/chrome/browser/resourc... File chrome/browser/resources/md_history/synced_device_manager.html (right): https://codereview.chromium.org/2171513004/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_history/synced_device_manager.html:76: hidden="[[!showNoSyncedMessage(signInState_, syncedDevices_.length, guestSession_)]]"> Nit: You can wrap the attribute to avoid having long lines: hidden="[[!showNoSyncedMessage(signInState_, syncedDevices_.length, guestSession_)]]"> (also make sure it's indented by 4 spaces) https://codereview.chromium.org/2171513004/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_history/synced_device_manager.html:80: hidden="[[!showSignInGuide(signInState_, guestSession_)]]"> Nit: Also indent this by 4 spaces
The CQ bit was checked by lshang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2171513004/diff/120001/chrome/browser/resourc... File chrome/browser/resources/md_history/synced_device_manager.html (right): https://codereview.chromium.org/2171513004/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_history/synced_device_manager.html:76: hidden="[[!showNoSyncedMessage(signInState_, syncedDevices_.length, guestSession_)]]"> On 2016/07/22 00:14:02, tsergeant wrote: > Nit: You can wrap the attribute to avoid having long lines: > > hidden="[[!showNoSyncedMessage(signInState_, syncedDevices_.length, > guestSession_)]]"> > > (also make sure it's indented by 4 spaces) Done. https://codereview.chromium.org/2171513004/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_history/synced_device_manager.html:80: hidden="[[!showSignInGuide(signInState_, guestSession_)]]"> On 2016/07/22 00:14:02, tsergeant wrote: > Nit: Also indent this by 4 spaces Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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 Link to the patchset: https://codereview.chromium.org/2171513004/#ps140001 (title: "format")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== MD History: Hide sign in guide in guest mode Hide sign in guide in guest mode and show 'no synced tabs'. BUG=628102 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD History: Hide sign in guide in guest mode Hide sign in guide in guest mode and show 'no synced tabs'. BUG=628102 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #6 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== MD History: Hide sign in guide in guest mode Hide sign in guide in guest mode and show 'no synced tabs'. BUG=628102 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD History: Hide sign in guide in guest mode Hide sign in guide in guest mode and show 'no synced tabs'. BUG=628102 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/18fefd6452c2fae6251a0fb667c6c0e2c92ddbaa Cr-Commit-Position: refs/heads/master@{#407009} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/18fefd6452c2fae6251a0fb667c6c0e2c92ddbaa Cr-Commit-Position: refs/heads/master@{#407009} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
