Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2577003002:
MD Settings: actually fix sync Learn More click issues (Closed)
|
Created:
4 years ago by Dan Beam Modified:
4 years ago Reviewers:
tommycli 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: actually fix sync Learn More click issues
So, I didn't actually try the last CL because Chrome is harder to build
that Chromium. BUT, I did actually try it today and it totally didn't
work. So here's a working CL that
1) works
2) actually works
3) combines with another handler that does basically the same thing
4) I've actually tried and written a test for
R=tommycli@chromium.org
BUG=621702
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
NOTRY=true # telemetry_unit_tests are unrelated-ly try failing
Committed: https://crrev.com/2449e5a74380149f97858f18208a442d624a4de0
Cr-Commit-Position: refs/heads/master@{#439067}
Patch Set 1 #Patch Set 2 : . #
Total comments: 3
Patch Set 3 : closure #
Total comments: 6
Patch Set 4 : tommycli@ review #
Messages
Total messages: 29 (19 generated)
Description was changed from ========== MD Settings: actually fix sync Learn More click issues So, I didn't actually try the last CL because Chrome is harder to build that Chromium. BUT, I did actually try it today and it totally didn't work. So here's a working CL that 1) works 2) actually works 3) combines with another handler that does basically the same thing 4) I've actually tried I can write a test if you want. The original defect just isn't the end of the world. R=tommycli@chromium.org BUG=621702 ========== to ========== MD Settings: actually fix sync Learn More click issues So, I didn't actually try the last CL because Chrome is harder to build that Chromium. BUT, I did actually try it today and it totally didn't work. So here's a working CL that 1) works 2) actually works 3) combines with another handler that does basically the same thing 4) I've actually tried I can write a test if you want. The original defect just isn't the end of the world. R=tommycli@chromium.org BUG=621702 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
https://codereview.chromium.org/2577003002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/sync_page.html (right): https://codereview.chromium.org/2577003002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/sync_page.html:191: $i18nRaw{encryptWithSyncPassphraseLabel} so, the link was hiding in here https://codereview.chromium.org/2577003002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/sync_page.js (left): https://codereview.chromium.org/2577003002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/sync_page.js:50: I18nBehavior, not used, afaict https://codereview.chromium.org/2577003002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/sync_page.js:298: onSyncLearnMoreTap_: function(e) { annnnd there's another one of these JUST like this handler ... in the same file shame cube
Description was changed from ========== MD Settings: actually fix sync Learn More click issues So, I didn't actually try the last CL because Chrome is harder to build that Chromium. BUT, I did actually try it today and it totally didn't work. So here's a working CL that 1) works 2) actually works 3) combines with another handler that does basically the same thing 4) I've actually tried I can write a test if you want. The original defect just isn't the end of the world. R=tommycli@chromium.org BUG=621702 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: actually fix sync Learn More click issues So, I didn't actually try the last CL because Chrome is harder to build that Chromium. BUT, I did actually try it today and it totally didn't work. So here's a working CL that 1) works 2) actually works 3) combines with another handler that does basically the same thing 4) I've actually tried and written a test R=tommycli@chromium.org BUG=621702 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== MD Settings: actually fix sync Learn More click issues So, I didn't actually try the last CL because Chrome is harder to build that Chromium. BUT, I did actually try it today and it totally didn't work. So here's a working CL that 1) works 2) actually works 3) combines with another handler that does basically the same thing 4) I've actually tried and written a test R=tommycli@chromium.org BUG=621702 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: actually fix sync Learn More click issues So, I didn't actually try the last CL because Chrome is harder to build that Chromium. BUT, I did actually try it today and it totally didn't work. So here's a working CL that 1) works 2) actually works 3) combines with another handler that does basically the same thing 4) I've actually tried and written a test for R=tommycli@chromium.org BUG=621702 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by dbeam@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.
lgtm except: https://codereview.chromium.org/2577003002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/sync_page.html (right): https://codereview.chromium.org/2577003002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/people_page/sync_page.html:190: <span on-tap="onLearnMoreTap_"> probably rename this: onControlLabelTap_ or onLabelTap_ https://codereview.chromium.org/2577003002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/sync_page.js (right): https://codereview.chromium.org/2577003002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/people_page/sync_page.js:430: // checkbox labels won't change the checkbox value. nit: update comment since it applies to radio control labels now too https://codereview.chromium.org/2577003002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/people_page_sync_page_test.js (right): https://codereview.chromium.org/2577003002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/people_page_sync_page_test.js:294: // Make sure the checkbox is enabled and checked. nit: it's actually a radio box
https://codereview.chromium.org/2577003002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/sync_page.html (right): https://codereview.chromium.org/2577003002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/people_page/sync_page.html:190: <span on-tap="onLearnMoreTap_"> On 2016/12/15 16:08:33, tommycli wrote: > probably rename this: onControlLabelTap_ or onLabelTap_ ehhhh https://codereview.chromium.org/2577003002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/sync_page.js (right): https://codereview.chromium.org/2577003002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/people_page/sync_page.js:430: // checkbox labels won't change the checkbox value. On 2016/12/15 16:08:33, tommycli wrote: > nit: update comment since it applies to radio control labels now too Done. https://codereview.chromium.org/2577003002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/people_page_sync_page_test.js (right): https://codereview.chromium.org/2577003002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/people_page_sync_page_test.js:294: // Make sure the checkbox is enabled and checked. On 2016/12/15 16:08:33, tommycli wrote: > nit: it's actually a radio box Done.
Description was changed from ========== MD Settings: actually fix sync Learn More click issues So, I didn't actually try the last CL because Chrome is harder to build that Chromium. BUT, I did actually try it today and it totally didn't work. So here's a working CL that 1) works 2) actually works 3) combines with another handler that does basically the same thing 4) I've actually tried and written a test for R=tommycli@chromium.org BUG=621702 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: actually fix sync Learn More click issues So, I didn't actually try the last CL because Chrome is harder to build that Chromium. BUT, I did actually try it today and it totally didn't work. So here's a working CL that 1) works 2) actually works 3) combines with another handler that does basically the same thing 4) I've actually tried and written a test for R=tommycli@chromium.org BUG=621702 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation NOTRY=true # see patchset 3, just comment changes in 4 ==========
Description was changed from ========== MD Settings: actually fix sync Learn More click issues So, I didn't actually try the last CL because Chrome is harder to build that Chromium. BUT, I did actually try it today and it totally didn't work. So here's a working CL that 1) works 2) actually works 3) combines with another handler that does basically the same thing 4) I've actually tried and written a test for R=tommycli@chromium.org BUG=621702 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation NOTRY=true # see patchset 3, just comment changes in 4 ========== to ========== MD Settings: actually fix sync Learn More click issues So, I didn't actually try the last CL because Chrome is harder to build that Chromium. BUT, I did actually try it today and it totally didn't work. So here's a working CL that 1) works 2) actually works 3) combines with another handler that does basically the same thing 4) I've actually tried and written a test for R=tommycli@chromium.org BUG=621702 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by dbeam@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommycli@chromium.org Link to the patchset: https://codereview.chromium.org/2577003002/#ps60001 (title: "tommycli@ review")
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
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dbeam@chromium.org
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
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== MD Settings: actually fix sync Learn More click issues So, I didn't actually try the last CL because Chrome is harder to build that Chromium. BUT, I did actually try it today and it totally didn't work. So here's a working CL that 1) works 2) actually works 3) combines with another handler that does basically the same thing 4) I've actually tried and written a test for R=tommycli@chromium.org BUG=621702 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: actually fix sync Learn More click issues So, I didn't actually try the last CL because Chrome is harder to build that Chromium. BUT, I did actually try it today and it totally didn't work. So here's a working CL that 1) works 2) actually works 3) combines with another handler that does basically the same thing 4) I've actually tried and written a test for R=tommycli@chromium.org BUG=621702 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation NOTRY=true # telemetry_unit_tests are unrelated-ly try failing ==========
The CQ bit was checked by dbeam@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481875965415620,
"parent_rev": "b16eab4d1ee93bcdf961000158d8f9372718ff8a", "commit_rev":
"2a908844697c37c89751de216bc8ab1c84aca84a"}
Message was sent while issue was closed.
Description was changed from ========== MD Settings: actually fix sync Learn More click issues So, I didn't actually try the last CL because Chrome is harder to build that Chromium. BUT, I did actually try it today and it totally didn't work. So here's a working CL that 1) works 2) actually works 3) combines with another handler that does basically the same thing 4) I've actually tried and written a test for R=tommycli@chromium.org BUG=621702 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation NOTRY=true # telemetry_unit_tests are unrelated-ly try failing ========== to ========== MD Settings: actually fix sync Learn More click issues So, I didn't actually try the last CL because Chrome is harder to build that Chromium. BUT, I did actually try it today and it totally didn't work. So here's a working CL that 1) works 2) actually works 3) combines with another handler that does basically the same thing 4) I've actually tried and written a test for R=tommycli@chromium.org BUG=621702 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation NOTRY=true # telemetry_unit_tests are unrelated-ly try failing Review-Url: https://codereview.chromium.org/2577003002 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== MD Settings: actually fix sync Learn More click issues So, I didn't actually try the last CL because Chrome is harder to build that Chromium. BUT, I did actually try it today and it totally didn't work. So here's a working CL that 1) works 2) actually works 3) combines with another handler that does basically the same thing 4) I've actually tried and written a test for R=tommycli@chromium.org BUG=621702 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation NOTRY=true # telemetry_unit_tests are unrelated-ly try failing Review-Url: https://codereview.chromium.org/2577003002 ========== to ========== MD Settings: actually fix sync Learn More click issues So, I didn't actually try the last CL because Chrome is harder to build that Chromium. BUT, I did actually try it today and it totally didn't work. So here's a working CL that 1) works 2) actually works 3) combines with another handler that does basically the same thing 4) I've actually tried and written a test for R=tommycli@chromium.org BUG=621702 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation NOTRY=true # telemetry_unit_tests are unrelated-ly try failing Committed: https://crrev.com/2449e5a74380149f97858f18208a442d624a4de0 Cr-Commit-Position: refs/heads/master@{#439067} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/2449e5a74380149f97858f18208a442d624a4de0 Cr-Commit-Position: refs/heads/master@{#439067} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
