Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2663083002:
MD Settings: Migrate enable-translation toggle from non-MD settings page. (Closed)
|
Created:
3 years, 10 months ago by scottchen Modified:
3 years, 10 months ago Reviewers:
michaelpg CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, arv+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: Migrate enable-translation toggle from non-MD settings page.
Add a toggle on the language page to enable translation of pages, which existed on the old settings page.
BUG=649551
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2663083002
Cr-Commit-Position: refs/heads/master@{#447663}
Committed: https://chromium.googlesource.com/chromium/src/+/4adbb6c12faa2a355d080b12f832d82a7543153e
Patch Set 1 #Patch Set 2 : add test for enable translation toggle #Patch Set 3 : remove top border; indent code #
Total comments: 15
Patch Set 4 : add test for language-specific option hidden condition #Patch Set 5 : fix tests #
Total comments: 3
Messages
Total messages: 24 (10 generated)
Description was changed from ========== MD Settings: Migrate enable-translation toggle from non-MD settings page. Add a toggle on the language page to enable translation of pages, which existed on the old settings page. BUG=649551 ========== to ========== MD Settings: Migrate enable-translation toggle from non-MD settings page. Add a toggle on the language page to enable translation of pages, which existed on the old settings page. BUG=649551 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by scottchen@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 scottchen@chromium.org
scottchen@chromium.org changed reviewers: + dbeam@chromium.org, michaelpg@chromium.org
Please see screenshot as well as toggling test on a French page. http://imgur.com/a/1Zzxc
Description was changed from ========== MD Settings: Migrate enable-translation toggle from non-MD settings page. Add a toggle on the language page to enable translation of pages, which existed on the old settings page. BUG=649551 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Migrate enable-translation toggle from non-MD settings page. Add a toggle on the language page to enable translation of pages, which existed on the old settings page. BUG=649551 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
dbeam@chromium.org changed reviewers: - dbeam@chromium.org
https://codereview.chromium.org/2663083002/diff/40001/chrome/app/settings_str... File chrome/app/settings_strings.grdp (right): https://codereview.chromium.org/2663083002/diff/40001/chrome/app/settings_str... chrome/app/settings_strings.grdp:1455: <message name="IDS_SETTINGS_LANGUAGES_OFFER_TO_ENABLE_TRANSLATE" desc="The label of the check-box that enables page translate"> nit: ending period https://codereview.chromium.org/2663083002/diff/40001/chrome/app/settings_str... chrome/app/settings_strings.grdp:1455: <message name="IDS_SETTINGS_LANGUAGES_OFFER_TO_ENABLE_TRANSLATE" desc="The label of the check-box that enables page translate"> "that enables the prompt to translate a page." https://codereview.chromium.org/2663083002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/languages_page_browsertest.js (right): https://codereview.chromium.org/2663083002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/languages_page_browsertest.js:239: var toggle = languagesPage.$.offerTranslateOtherLangs; this assumes translate is already enabled. we should first set the pref (see the test below). https://codereview.chromium.org/2663083002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/languages_page_browsertest.js:243: assertEquals(!origTranslateEnabled, newTranslateEnabled); can you also test that disabling the toggle button hides the menu option in a language's dropdown? (again see test below for set-up) https://codereview.chromium.org/2663083002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/languages_page_browsertest.js:246: test('toggle translate', function(done) { change title to something like 'toggle translate for a specific language' since it's ambiguous now
https://codereview.chromium.org/2663083002/diff/40001/chrome/app/settings_str... File chrome/app/settings_strings.grdp (right): https://codereview.chromium.org/2663083002/diff/40001/chrome/app/settings_str... chrome/app/settings_strings.grdp:1455: <message name="IDS_SETTINGS_LANGUAGES_OFFER_TO_ENABLE_TRANSLATE" desc="The label of the check-box that enables page translate"> On 2017/01/31 21:54:10, michaelpg wrote: > nit: ending period Done. https://codereview.chromium.org/2663083002/diff/40001/chrome/app/settings_str... chrome/app/settings_strings.grdp:1455: <message name="IDS_SETTINGS_LANGUAGES_OFFER_TO_ENABLE_TRANSLATE" desc="The label of the check-box that enables page translate"> On 2017/01/31 21:54:10, michaelpg wrote: > "that enables the prompt to translate a page." Done. https://codereview.chromium.org/2663083002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/languages_page_browsertest.js (right): https://codereview.chromium.org/2663083002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/languages_page_browsertest.js:239: var toggle = languagesPage.$.offerTranslateOtherLangs; On 2017/01/31 21:54:10, michaelpg wrote: > this assumes translate is already enabled. we should first set the pref (see the > test below). The button that this line is grabbing is the toggle to enable/disable prefs.translate.enabled itself, which should always exist and be visible on the page. In which case, setting the pref is not necessary, since all the test should care about is translate.enabled is different from the initial value after being clicked. I think you might be thinking of a different button that only exists in the overflow menu if translate.enabled = true (i.e. the button that is being tested in the test below). https://codereview.chromium.org/2663083002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/languages_page_browsertest.js:243: assertEquals(!origTranslateEnabled, newTranslateEnabled); On 2017/01/31 21:54:10, michaelpg wrote: > can you also test that disabling the toggle button hides the menu option in a > language's dropdown? (again see test below for set-up) Do you mean disabling the toggle button as in <settings-toggle-button disabled>, or do you mean disabling translation (i.e. translate.enabled = false)? I don't think <settings-toggle-button disabled> should impact whether the menu option show up either way, and currently there's no condition in which the toggle would be disabled. If you mean the latter, I agree we should test for that :) I'll add a new test though, since it seems like a separate concern than what we're testing for here. https://codereview.chromium.org/2663083002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/languages_page_browsertest.js:246: test('toggle translate', function(done) { On 2017/01/31 21:54:10, michaelpg wrote: > change title to something like 'toggle translate for a specific language' since > it's ambiguous now Done.
https://codereview.chromium.org/2663083002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/languages_page_browsertest.js (right): https://codereview.chromium.org/2663083002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/languages_page_browsertest.js:239: var toggle = languagesPage.$.offerTranslateOtherLangs; On 2017/02/01 01:02:39, scottchen wrote: > On 2017/01/31 21:54:10, michaelpg wrote: > > this assumes translate is already enabled. we should first set the pref (see > the > > test below). > > The button that this line is grabbing is the toggle to enable/disable > prefs.translate.enabled itself, which should always exist and be visible on the > page. In which case, setting the pref is not necessary, since all the test > should care about is translate.enabled is different from the initial value after > being clicked. origTranslateEnabled is used to ensure the value gets reset to the default when we leave this test suite. It's only set once, so relying on it within a test is fragile. If we were to change or swap some tests, the value might be different than origTranslateEnabled when we get to this test. That's why I suggest setting the pref first, or at least querying for the current value of the pref, before testing the toggle. > > I think you might be thinking of a different button that only exists in the > overflow menu if translate.enabled = true (i.e. the button that is being tested > in the test below). https://codereview.chromium.org/2663083002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/languages_page_browsertest.js:243: assertEquals(!origTranslateEnabled, newTranslateEnabled); On 2017/02/01 01:02:38, scottchen wrote: > On 2017/01/31 21:54:10, michaelpg wrote: > > can you also test that disabling the toggle button hides the menu option in a > > language's dropdown? (again see test below for set-up) > > Do you mean disabling the toggle button as in <settings-toggle-button disabled>, > or do you mean disabling translation (i.e. translate.enabled = false)? > > I don't think <settings-toggle-button disabled> should impact whether the menu > option show up either way, and currently there's no condition in which the > toggle would be disabled. > > If you mean the latter, I agree we should test for that :) I'll add a new test > though, since it seems like a separate concern than what we're testing for here. > Sorry, yes, if we "uncheck" the toggle button to turn off translate (translate.enabled = false). Thanks.
https://codereview.chromium.org/2663083002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/languages_page_browsertest.js (right): https://codereview.chromium.org/2663083002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/languages_page_browsertest.js:239: var toggle = languagesPage.$.offerTranslateOtherLangs; On 2017/02/01 01:50:02, michaelpg wrote: > On 2017/02/01 01:02:39, scottchen wrote: > > On 2017/01/31 21:54:10, michaelpg wrote: > > > this assumes translate is already enabled. we should first set the pref (see > > the > > > test below). > > > > The button that this line is grabbing is the toggle to enable/disable > > prefs.translate.enabled itself, which should always exist and be visible on > the > > page. In which case, setting the pref is not necessary, since all the test > > should care about is translate.enabled is different from the initial value > after > > being clicked. > > origTranslateEnabled is used to ensure the value gets reset to the default when > we leave this test suite. It's only set once, so relying on it within a test is > fragile. If we were to change or swap some tests, the value might be different > than origTranslateEnabled when we get to this test. That's why I suggest setting > the pref first, or at least querying for the current value of the pref, before > testing the toggle. > > > > > I think you might be thinking of a different button that only exists in the > > overflow menu if translate.enabled = true (i.e. the button that is being > tested > > in the test below). > The test would be valid regardless of origTranslateEnabled's initial value, because all it checks is whether or not the value is flipped. (new value != old value); it doesn't actually check if the value is truthy or falsey, so even if we query the current value, we'd still be testing for "new value != old value" anyway - so I'm not sure how it'll make a difference. (See the assertEquals line below). So if we query for the current value, we'd be doing: var curValue = origTranslateEnabled; ... //flip the toggle assertEquals(!curValue, newTranslateEnabled); which would be exactly the same as the current code, just with one extra proxy-variable.
https://codereview.chromium.org/2663083002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/languages_page_browsertest.js (right): https://codereview.chromium.org/2663083002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/languages_page_browsertest.js:239: var toggle = languagesPage.$.offerTranslateOtherLangs; On 2017/02/01 02:04:31, scottchen wrote: > On 2017/02/01 01:50:02, michaelpg wrote: > > On 2017/02/01 01:02:39, scottchen wrote: > > > On 2017/01/31 21:54:10, michaelpg wrote: > > > > this assumes translate is already enabled. we should first set the pref > (see > > > the > > > > test below). > > > > > > The button that this line is grabbing is the toggle to enable/disable > > > prefs.translate.enabled itself, which should always exist and be visible on > > the > > > page. In which case, setting the pref is not necessary, since all the test > > > should care about is translate.enabled is different from the initial value > > after > > > being clicked. > > > > origTranslateEnabled is used to ensure the value gets reset to the default > when > > we leave this test suite. It's only set once, so relying on it within a test > is > > fragile. If we were to change or swap some tests, the value might be different > > than origTranslateEnabled when we get to this test. That's why I suggest > setting > > the pref first, or at least querying for the current value of the pref, before > > testing the toggle. > > > > > > > > I think you might be thinking of a different button that only exists in the > > > overflow menu if translate.enabled = true (i.e. the button that is being > > tested > > > in the test below). > > > > The test would be valid regardless of origTranslateEnabled's initial value, > because all it checks is whether or not the value is flipped. (new value != old > value); it doesn't actually check if the value is truthy or falsey, so even if > we query the current value, we'd still be testing for "new value != old value" > anyway - so I'm not sure how it'll make a difference. > (See the assertEquals line below). > > So if we query for the current value, we'd be doing: > > var curValue = origTranslateEnabled; > ... //flip the toggle > assertEquals(!curValue, newTranslateEnabled); > > which would be exactly the same as the current code, just with one extra > proxy-variable. I was playing with this test to try to show what I mean, but it's behaving weirdly. MockInteractions.tap(toggle) doesn't appear to be actually changing the state of the toggle. Which would explain why we're not seeing the failure I expect here.
On 2017/02/01 02:57:16, michaelpg wrote: > https://codereview.chromium.org/2663083002/diff/40001/chrome/test/data/webui/... > File chrome/test/data/webui/settings/languages_page_browsertest.js (right): > > https://codereview.chromium.org/2663083002/diff/40001/chrome/test/data/webui/... > chrome/test/data/webui/settings/languages_page_browsertest.js:239: var toggle = > languagesPage.$.offerTranslateOtherLangs; > On 2017/02/01 02:04:31, scottchen wrote: > > On 2017/02/01 01:50:02, michaelpg wrote: > > > On 2017/02/01 01:02:39, scottchen wrote: > > > > On 2017/01/31 21:54:10, michaelpg wrote: > > > > > this assumes translate is already enabled. we should first set the pref > > (see > > > > the > > > > > test below). > > > > > > > > The button that this line is grabbing is the toggle to enable/disable > > > > prefs.translate.enabled itself, which should always exist and be visible > on > > > the > > > > page. In which case, setting the pref is not necessary, since all the test > > > > should care about is translate.enabled is different from the initial value > > > after > > > > being clicked. > > > > > > origTranslateEnabled is used to ensure the value gets reset to the default > > when > > > we leave this test suite. It's only set once, so relying on it within a test > > is > > > fragile. If we were to change or swap some tests, the value might be > different > > > than origTranslateEnabled when we get to this test. That's why I suggest > > setting > > > the pref first, or at least querying for the current value of the pref, > before > > > testing the toggle. > > > > > > > > > > > I think you might be thinking of a different button that only exists in > the > > > > overflow menu if translate.enabled = true (i.e. the button that is being > > > tested > > > > in the test below). > > > > > > > The test would be valid regardless of origTranslateEnabled's initial value, > > because all it checks is whether or not the value is flipped. (new value != > old > > value); it doesn't actually check if the value is truthy or falsey, so even if > > we query the current value, we'd still be testing for "new value != old value" > > anyway - so I'm not sure how it'll make a difference. > > (See the assertEquals line below). > > > > So if we query for the current value, we'd be doing: > > > > var curValue = origTranslateEnabled; > > ... //flip the toggle > > assertEquals(!curValue, newTranslateEnabled); > > > > which would be exactly the same as the current code, just with one extra > > proxy-variable. > > I was playing with this test to try to show what I mean, but it's behaving > weirdly. MockInteractions.tap(toggle) doesn't appear to be actually changing the > state of the toggle. > > Which would explain why we're not seeing the failure I expect here. Oh, I finally see what you're saying now. The origTranslateEnabled is in SUITE-setup, not just setup, so it only runs once ever, not before each test. Is there any reason why we don't want to run the resets before each test?
https://codereview.chromium.org/2663083002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/languages_page_browsertest.js (right): https://codereview.chromium.org/2663083002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/languages_page_browsertest.js:239: var toggle = languagesPage.$.offerTranslateOtherLangs; On 2017/02/01 02:57:15, michaelpg wrote: > On 2017/02/01 02:04:31, scottchen wrote: > > On 2017/02/01 01:50:02, michaelpg wrote: > > > On 2017/02/01 01:02:39, scottchen wrote: > > > > On 2017/01/31 21:54:10, michaelpg wrote: > > > > > this assumes translate is already enabled. we should first set the pref > > (see > > > > the > > > > > test below). > > > > > > > > The button that this line is grabbing is the toggle to enable/disable > > > > prefs.translate.enabled itself, which should always exist and be visible > on > > > the > > > > page. In which case, setting the pref is not necessary, since all the test > > > > should care about is translate.enabled is different from the initial value > > > after > > > > being clicked. > > > > > > origTranslateEnabled is used to ensure the value gets reset to the default > > when > > > we leave this test suite. It's only set once, so relying on it within a test > > is > > > fragile. If we were to change or swap some tests, the value might be > different > > > than origTranslateEnabled when we get to this test. That's why I suggest > > setting > > > the pref first, or at least querying for the current value of the pref, > before > > > testing the toggle. > > > > > > > > > > > I think you might be thinking of a different button that only exists in > the > > > > overflow menu if translate.enabled = true (i.e. the button that is being > > > tested > > > > in the test below). > > > > > > > The test would be valid regardless of origTranslateEnabled's initial value, > > because all it checks is whether or not the value is flipped. (new value != > old > > value); it doesn't actually check if the value is truthy or falsey, so even if > > we query the current value, we'd still be testing for "new value != old value" > > anyway - so I'm not sure how it'll make a difference. > > (See the assertEquals line below). > > > > So if we query for the current value, we'd be doing: > > > > var curValue = origTranslateEnabled; > > ... //flip the toggle > > assertEquals(!curValue, newTranslateEnabled); > > > > which would be exactly the same as the current code, just with one extra > > proxy-variable. > > I was playing with this test to try to show what I mean, but it's behaving > weirdly. MockInteractions.tap(toggle) doesn't appear to be actually changing the > state of the toggle. > > Which would explain why we're not seeing the failure I expect here. Acknowledged. Please see newest patch.
thanks! lgtm https://codereview.chromium.org/2663083002/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/settings/languages_page_browsertest.js (right): https://codereview.chromium.org/2663083002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/languages_page_browsertest.js:241: .querySelectorAll('paper-toggle-button')[0]; shorter: .root.querySelector('paper-toggle-button') even shorter (Polymer shortcut for root.querySelector): .$$('paper-toggle-button')
https://codereview.chromium.org/2663083002/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/settings/languages_page_browsertest.js (right): https://codereview.chromium.org/2663083002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/languages_page_browsertest.js:241: .querySelectorAll('paper-toggle-button')[0]; On 2017/02/01 23:30:48, michaelpg wrote: > shorter: > .root.querySelector('paper-toggle-button') > > even shorter (Polymer shortcut for root.querySelector): > .$$('paper-toggle-button') I think I'll leave it like this, just in case someone has to another paper-toggle-button on the page it wouldn't break the test :)
The CQ bit was checked by scottchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2663083002/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/settings/languages_page_browsertest.js (right): https://codereview.chromium.org/2663083002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/languages_page_browsertest.js:241: .querySelectorAll('paper-toggle-button')[0]; On 2017/02/01 23:33:50, scottchen wrote: > On 2017/02/01 23:30:48, michaelpg wrote: > > shorter: > > .root.querySelector('paper-toggle-button') > > > > even shorter (Polymer shortcut for root.querySelector): > > .$$('paper-toggle-button') > > I think I'll leave it like this, just in case someone has to another > paper-toggle-button on the page it wouldn't break the test :) They are exactly equivalent -- querySelector returns the first entry of what querySelectorAll would return. But it's not a big deal. (I do agree looking for the 0th paper-toggle-button is a bit fragile, but it's fine.)
On 2017/02/01 23:41:13, michaelpg wrote: > https://codereview.chromium.org/2663083002/diff/80001/chrome/test/data/webui/... > File chrome/test/data/webui/settings/languages_page_browsertest.js (right): > > https://codereview.chromium.org/2663083002/diff/80001/chrome/test/data/webui/... > chrome/test/data/webui/settings/languages_page_browsertest.js:241: > .querySelectorAll('paper-toggle-button')[0]; > On 2017/02/01 23:33:50, scottchen wrote: > > On 2017/02/01 23:30:48, michaelpg wrote: > > > shorter: > > > .root.querySelector('paper-toggle-button') > > > > > > even shorter (Polymer shortcut for root.querySelector): > > > .$$('paper-toggle-button') > > > > I think I'll leave it like this, just in case someone has to another > > paper-toggle-button on the page it wouldn't break the test :) > > They are exactly equivalent -- querySelector returns the first entry of what > querySelectorAll would return. But it's not a big deal. > > (I do agree looking for the 0th paper-toggle-button is a bit fragile, but it's > fine.) Oh yeah, since I'm using [0] that would fall under the same case.. haha.
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1485992050646090,
"parent_rev": "5859aafc5603f3457068257bd664e5b8eb3b5ac5", "commit_rev":
"4adbb6c12faa2a355d080b12f832d82a7543153e"}
Message was sent while issue was closed.
Description was changed from ========== MD Settings: Migrate enable-translation toggle from non-MD settings page. Add a toggle on the language page to enable translation of pages, which existed on the old settings page. BUG=649551 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Migrate enable-translation toggle from non-MD settings page. Add a toggle on the language page to enable translation of pages, which existed on the old settings page. BUG=649551 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2663083002 Cr-Commit-Position: refs/heads/master@{#447663} Committed: https://chromium.googlesource.com/chromium/src/+/4adbb6c12faa2a355d080b12f832... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/4adbb6c12faa2a355d080b12f832... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
