Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2235093002:
Fix Loading text for settings-dropdown-menu (Closed)
|
Created:
4 years, 4 months ago by michaelpg Modified:
4 years, 4 months ago Reviewers:
dschuyler CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, dcheng, michaelpg+watch-md-ui_chromium.org, arv+watch_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix Loading text for settings-dropdown-menu
BUG=636540
R=dschuyler@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/2799d0139faab5d8738fdd93ec2a9d20bdd6d00b
Cr-Commit-Position: refs/heads/master@{#413891}
Patch Set 1 #
Total comments: 7
Patch Set 2 : test #
Total comments: 2
Patch Set 3 : rebase #
Dependent Patchsets: Messages
Total messages: 14 (4 generated)
Description was changed from ========== Fix Loading text for settings-dropdown-menu BUG=636540 R=dschuyler@chromium.org ========== to ========== Fix Loading text for settings-dropdown-menu BUG=636540 R=dschuyler@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
PTAL. I verified this manually by delaying the browser proxy response, and verified the test fails without the fix.
https://codereview.chromium.org/2235093002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/controls/settings_dropdown_menu.js (right): https://codereview.chromium.org/2235093002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/controls/settings_dropdown_menu.js:141: return this.menuOptions.length > 0 && selected == this.notFoundValue_; bummer
https://codereview.chromium.org/2235093002/diff/1/chrome/test/data/webui/sett... File chrome/test/data/webui/settings/dropdown_menu_tests.js (right): https://codereview.chromium.org/2235093002/diff/1/chrome/test/data/webui/sett... chrome/test/data/webui/settings/dropdown_menu_tests.js:103: setTimeout(function() { What other choices do we have aside from using a setTimeout here? Can the setTimeout be replaced with something event based? (i.e. some active confirmation that it's ready rather than a passive time delay). https://codereview.chromium.org/2235093002/diff/1/chrome/test/data/webui/sett... chrome/test/data/webui/settings/dropdown_menu_tests.js:117: assertEquals(200, dropdown.pref.value); Consider making a comment in the code about what this assert is implying / why it's part of this test.
https://codereview.chromium.org/2235093002/diff/1/chrome/test/data/webui/sett... File chrome/test/data/webui/settings/dropdown_menu_tests.js (right): https://codereview.chromium.org/2235093002/diff/1/chrome/test/data/webui/sett... chrome/test/data/webui/settings/dropdown_menu_tests.js:103: setTimeout(function() { On 2016/08/11 23:50:58, dschuyler wrote: > What other choices do we have aside from using > a setTimeout here? Can the setTimeout be replaced > with something event based? (i.e. some active > confirmation that it's ready rather than a passive > time delay). In this case the point of the test is to spin for a while and test that setting the dropdown menu options, after a time, still works. Not to run a test immediately when it's ready -- it's "not ready" here because there are no options. providing a time like 100 ensures that any other work that might be scheduled runs first, which is what we want. (theoretically async tasks could take longer than 100 ms, then schedule more async tasks, but I think the test suffices as written.) https://codereview.chromium.org/2235093002/diff/1/chrome/test/data/webui/sett... chrome/test/data/webui/settings/dropdown_menu_tests.js:117: assertEquals(200, dropdown.pref.value); On 2016/08/11 23:50:58, dschuyler wrote: > Consider making a comment in the code about > what this assert is implying / why it's part of this > test. Done, also this probably wasn't the right thing to test!
https://codereview.chromium.org/2235093002/diff/1/chrome/test/data/webui/sett... File chrome/test/data/webui/settings/dropdown_menu_tests.js (right): https://codereview.chromium.org/2235093002/diff/1/chrome/test/data/webui/sett... chrome/test/data/webui/settings/dropdown_menu_tests.js:103: setTimeout(function() { On 2016/08/16 23:38:11, michaelpg wrote: > On 2016/08/11 23:50:58, dschuyler wrote: > > What other choices do we have aside from using > > a setTimeout here? Can the setTimeout be replaced > > with something event based? (i.e. some active > > confirmation that it's ready rather than a passive > > time delay). > > In this case the point of the test is to spin for a while and test that setting > the dropdown menu options, after a time, still works. Not to run a test > immediately when it's ready -- it's "not ready" here because there are no > options. > > providing a time like 100 ensures that any other work that might be scheduled > runs first, which is what we want. (theoretically async tasks could take longer > than 100 ms, then schedule more async tasks, but I think the test suffices as > written.) Ah, I think I was hoping for a different kind of test. One that would test that the user sees the message about "Loading..." while we are waiting for the data. https://codereview.chromium.org/2235093002/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/settings/dropdown_menu_tests.js (right): https://codereview.chromium.org/2235093002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/dropdown_menu_tests.js:102: Would it make sense to have check that the label is shown here and... https://codereview.chromium.org/2235093002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/dropdown_menu_tests.js:105: loadTimeData.getValue('loading'), dropdown.$.dropdownMenu.label); ...that the label is not shown here? I think we want the "Loading..." to be shown while we don't have the options loaded and the "Loading..." to go away when we do have the menuOptions set.
https://codereview.chromium.org/2235093002/diff/1/chrome/test/data/webui/sett... File chrome/test/data/webui/settings/dropdown_menu_tests.js (right): https://codereview.chromium.org/2235093002/diff/1/chrome/test/data/webui/sett... chrome/test/data/webui/settings/dropdown_menu_tests.js:103: setTimeout(function() { On 2016/08/17 23:59:46, dschuyler wrote: > On 2016/08/16 23:38:11, michaelpg wrote: > > On 2016/08/11 23:50:58, dschuyler wrote: > > > What other choices do we have aside from using > > > a setTimeout here? Can the setTimeout be replaced > > > with something event based? (i.e. some active > > > confirmation that it's ready rather than a passive > > > time delay). > > > > In this case the point of the test is to spin for a while and test that > setting > > the dropdown menu options, after a time, still works. Not to run a test > > immediately when it's ready -- it's "not ready" here because there are no > > options. > > > > providing a time like 100 ensures that any other work that might be scheduled > > runs first, which is what we want. (theoretically async tasks could take > longer > > than 100 ms, then schedule more async tasks, but I think the test suffices as > > written.) > > Ah, I think I was hoping for a different kind of test. > One that would test that the user sees the message > about "Loading..." while we are waiting for the data. Yes, that's what this tests: without setting menuOptions, after 100ms the label is 'loading' and nothing is selected (so the label is shown). I was just talking about why the 100ms timeout makes sense to do that.
lgtm
The CQ bit was checked by michaelpg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dschuyler@chromium.org Link to the patchset: https://codereview.chromium.org/2235093002/#ps40001 (title: "rebase")
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.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Fix Loading text for settings-dropdown-menu BUG=636540 R=dschuyler@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Fix Loading text for settings-dropdown-menu BUG=636540 R=dschuyler@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/2799d0139faab5d8738fdd93ec2a9d20bdd6d00b Cr-Commit-Position: refs/heads/master@{#413891} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/2799d0139faab5d8738fdd93ec2a9d20bdd6d00b Cr-Commit-Position: refs/heads/master@{#413891} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
