Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2220303002:
MD History: Show 'no synced tabs' when tab sync is disabled (Closed)
|
Created:
4 years, 4 months ago by lshang Modified:
4 years, 4 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: Show 'no synced tabs' when tab sync is disabled
When user signs in and disables tab sync, setForeignSessions will return
directly, leaving "Loading" message shown in synced tabs page.
This CL notifies synced device manager to change message from "Loading"
to "No synced devices" when tab sync is disabled.
BUG=633558
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/aaca255abd350f4d3b85ccff3d3dec1358255313
Cr-Commit-Position: refs/heads/master@{#410943}
Patch Set 1 : #
Total comments: 6
Patch Set 2 : address review comments #
Total comments: 2
Patch Set 3 : revise test #
Total comments: 2
Patch Set 4 : minor change #
Messages
Total messages: 34 (22 generated)
Description was changed from ========== MD History: Show 'no synced tabs' when tab sync is disabled sync disabled BUG= ========== to ========== MD History: Show 'no synced tabs' when tab sync is disabled sync disabled BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
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...
Patchset #1 (id:1) has been deleted
Description was changed from ========== MD History: Show 'no synced tabs' when tab sync is disabled sync disabled BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD History: Show 'no synced tabs' when tab sync is disabled When user signs in and disables tab sync, setForeignSessions will return directly, leaving "Loading" message shown in synced tabs page. This CL notifies synced device manager to change message from "Loading" to "No synced devices" when tab sync is disabled. BUG=633558 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
lshang@chromium.org changed reviewers: + calamity@chromium.org, tsergeant@chromium.org
PTAL thanks!
https://codereview.chromium.org/2220303002/diff/20001/chrome/browser/resource... File chrome/browser/resources/md_history/synced_device_manager.js (right): https://codereview.chromium.org/2220303002/diff/20001/chrome/browser/resource... chrome/browser/resources/md_history/synced_device_manager.js:195: this.fetchingSyncedTabs_ = false; This doesn't quite work as intended in all cases: If I am already signed in with the history page open, then open settings and disable tab sync, then I would want the devices to disappear from the history page. The history page will receive a setForeignSessions message with isTabSyncEnabled=false, but it won't change anything in the UI. You could fix this by clearing the syncedDevices_ list here ( this.set('syncedDevices_', []) ), or by adding a new variable which controls whether the list is shown. https://codereview.chromium.org/2220303002/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/md_history/history_synced_tabs_test.js (right): https://codereview.chromium.org/2220303002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/md_history/history_synced_tabs_test.js:275: updateSignInState(true); Do you mind pulling this out into a separate test()? You're testing something mostly separate here, and when a single test gets this long, it's hard to know if anything above is affecting this section of the test. https://codereview.chromium.org/2220303002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/md_history/history_synced_tabs_test.js:284: assertFalse(element.$['no-synced-tabs'].hidden); Nit: You can pull this code out into a method like assertNoSyncedTabsMessageShown() and use it above (line 248) and here.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
richardsonfamily79dimmy@gmail.com changed reviewers: + richardsonfamily79dimmy@gmail.com
lgtm
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2220303002/diff/20001/chrome/browser/resource... File chrome/browser/resources/md_history/synced_device_manager.js (right): https://codereview.chromium.org/2220303002/diff/20001/chrome/browser/resource... chrome/browser/resources/md_history/synced_device_manager.js:195: this.fetchingSyncedTabs_ = false; On 2016/08/08 04:49:15, tsergeant wrote: > This doesn't quite work as intended in all cases: > > If I am already signed in with the history page open, then open settings and > disable tab sync, then I would want the devices to disappear from the history > page. > > The history page will receive a setForeignSessions message with > isTabSyncEnabled=false, but it won't change anything in the UI. > > You could fix this by clearing the syncedDevices_ list here ( > this.set('syncedDevices_', []) ), or by adding a new variable which controls > whether the list is shown. Done. Thanks for reminding:P https://codereview.chromium.org/2220303002/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/md_history/history_synced_tabs_test.js (right): https://codereview.chromium.org/2220303002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/md_history/history_synced_tabs_test.js:275: updateSignInState(true); On 2016/08/08 04:49:15, tsergeant wrote: > Do you mind pulling this out into a separate test()? > > You're testing something mostly separate here, and when a single test gets this > long, it's hard to know if anything above is affecting this section of the test. Yep, got rid of the looooooooong test. https://codereview.chromium.org/2220303002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/md_history/history_synced_tabs_test.js:284: assertFalse(element.$['no-synced-tabs'].hidden); On 2016/08/08 04:49:15, tsergeant wrote: > Nit: You can pull this code out into a method like > > assertNoSyncedTabsMessageShown() > > and use it above (line 248) and here. Done.
https://codereview.chromium.org/2220303002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/md_history/history_synced_tabs_test.js (right): https://codereview.chromium.org/2220303002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/md_history/history_synced_tabs_test.js:46: assertFalse(element.$['no-synced-tabs'].hidden); I had intended that you could pull out the code that checks the test as well (you can even put the string ID to be checked in a method argument): var noSyncedResults = loadTimeData.getString('noSyncedResults'); assertNotEquals( -1, element.$['no-synced-tabs'].textContent.indexOf(noSyncedResults));
Patchset #3 (id:60001) has been deleted
https://codereview.chromium.org/2220303002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/md_history/history_synced_tabs_test.js (right): https://codereview.chromium.org/2220303002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/md_history/history_synced_tabs_test.js:46: assertFalse(element.$['no-synced-tabs'].hidden); On 2016/08/09 04:06:59, tsergeant wrote: > I had intended that you could pull out the code that checks the test as well > (you can even put the string ID to be checked in a method argument): > > var noSyncedResults = loadTimeData.getString('noSyncedResults'); > assertNotEquals( > -1, > element.$['no-synced-tabs'].textContent.indexOf(noSyncedResults)); Done.
lgtm, but let calamity@ review as well. https://codereview.chromium.org/2220303002/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/md_history/history_synced_tabs_test.js (right): https://codereview.chromium.org/2220303002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/md_history/history_synced_tabs_test.js:51: } Nit: This should have a semicolon. (Same with the getCards method above)
lgtm
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks all! https://codereview.chromium.org/2220303002/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/md_history/history_synced_tabs_test.js (right): https://codereview.chromium.org/2220303002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/md_history/history_synced_tabs_test.js:51: } On 2016/08/09 05:17:23, tsergeant wrote: > Nit: This should have a semicolon. (Same with the getCards method above) Done.
The CQ bit was checked by lshang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from richardsonfamily79dimmy@gmail.com, tsergeant@chromium.org, calamity@chromium.org Link to the patchset: https://codereview.chromium.org/2220303002/#ps100001 (title: "minor change")
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: Show 'no synced tabs' when tab sync is disabled When user signs in and disables tab sync, setForeignSessions will return directly, leaving "Loading" message shown in synced tabs page. This CL notifies synced device manager to change message from "Loading" to "No synced devices" when tab sync is disabled. BUG=633558 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD History: Show 'no synced tabs' when tab sync is disabled When user signs in and disables tab sync, setForeignSessions will return directly, leaving "Loading" message shown in synced tabs page. This CL notifies synced device manager to change message from "Loading" to "No synced devices" when tab sync is disabled. BUG=633558 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #4 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== MD History: Show 'no synced tabs' when tab sync is disabled When user signs in and disables tab sync, setForeignSessions will return directly, leaving "Loading" message shown in synced tabs page. This CL notifies synced device manager to change message from "Loading" to "No synced devices" when tab sync is disabled. BUG=633558 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD History: Show 'no synced tabs' when tab sync is disabled When user signs in and disables tab sync, setForeignSessions will return directly, leaving "Loading" message shown in synced tabs page. This CL notifies synced device manager to change message from "Loading" to "No synced devices" when tab sync is disabled. BUG=633558 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/aaca255abd350f4d3b85ccff3d3dec1358255313 Cr-Commit-Position: refs/heads/master@{#410943} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/aaca255abd350f4d3b85ccff3d3dec1358255313 Cr-Commit-Position: refs/heads/master@{#410943} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
