Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2153963002:
Autoclick on md-settings is synced with autoclick on settings. (Closed)
|
Created:
4 years, 5 months ago by sammiequon Modified:
4 years, 5 months ago 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 Base URL:
https://chromium.googlesource.com/chromium/src.git@lkgr Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAutoclick on md-settings is synced with autoclick on settings.
BUG=623944
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/6302068a9d5156a5a17278ff14d868ffc3cf6546
Cr-Commit-Position: refs/heads/master@{#406717}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Fixed indentation. #
Total comments: 15
Patch Set 3 : Fixed patch set 2 errors. #
Messages
Total messages: 24 (11 generated)
Description was changed from ========== Autoclick on md-settings is synced with autoclick on settings. BUG=623944 ========== to ========== Autoclick on md-settings is synced with autoclick on settings. BUG=623944 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Autoclick on md-settings is synced with autoclick on settings. BUG=623944 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Autoclick on md-settings is synced with autoclick on settings. BUG=623944 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
sammiequon@chromium.org changed reviewers: + dbeam@chromium.org
dbeam@ - Please take a look.
lgtm https://codereview.chromium.org/2153963002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/a11y_page/a11y_page.html (right): https://codereview.chromium.org/2153963002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/a11y_page/a11y_page.html:4: <link rel="import" href="/controls/settings_checkbox.html"> <link rel="import" href="/controls/settings_dropdown_menu.html"> https://codereview.chromium.org/2153963002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/a11y_page/a11y_page.js (right): https://codereview.chromium.org/2153963002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/a11y_page/a11y_page.js:44: }, nit: return [ { value: 600, name: loadTimeData.getString('delayBeforeClickExtremelyShort'), }, ... ]; is probably more common indentation
The CQ bit was checked by sammiequon@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2153963002/#ps20001 (title: "Fixed indentation.")
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_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
michaelpg@chromium.org changed reviewers: + michaelpg@chromium.org
https://codereview.chromium.org/2153963002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/a11y_page/a11y_page.js (right): https://codereview.chromium.org/2153963002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/a11y_page/a11y_page.js:44: }, On 2016/07/16 00:35:27, Dan Beam wrote: > nit: > > return [ > { > value: 600, > name: loadTimeData.getString('delayBeforeClickExtremelyShort'), > }, > ... > ]; > > is probably more common indentation +1 https://codereview.chromium.org/2153963002/diff/20001/chrome/app/settings_str... File chrome/app/settings_strings.grdp (right): https://codereview.chromium.org/2153963002/diff/20001/chrome/app/settings_str... chrome/app/settings_strings.grdp:139: extremely short (600ms) Did you check with tbuckley@ or bettes@ that we should include these numbers in MD Settings? https://codereview.chromium.org/2153963002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/a11y_page/a11y_page.html (right): https://codereview.chromium.org/2153963002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/a11y_page/a11y_page.html:6: <link rel="import" href="/settings_shared_css.html"> import settings/i18n_setup.html for loadTimeData usage https://codereview.chromium.org/2153963002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/a11y_page/a11y_page.html:50: <settings-dropdown-menu id="defaultAutoClickDelay" Why does this need an ID? https://codereview.chromium.org/2153963002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/a11y_page/a11y_page.html:51: pref="{{prefs.settings.a11y.autoclick_delay_ms}}" 4-space indent, not 6 https://codereview.chromium.org/2153963002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/a11y_page/a11y_page.js (right): https://codereview.chromium.org/2153963002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/a11y_page/a11y_page.js:28: autoClickDelayOptions_: { this should be inside the <if expr="chromeos"> below, because: 1. stripping out unused code on other platforms reduces startup time and resource size 2. you'll get JS errors on non-chromeos otherwise https://codereview.chromium.org/2153963002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/a11y_page/a11y_page.js:29: readOnly: true, 2 space indent, not 1
On 2016/07/16 06:56:31, sammiequon wrote: > The CQ bit was checked by mailto:sammiequon@chromium.org Sammie, please address our other comments before submitting.
https://codereview.chromium.org/2153963002/diff/20001/chrome/app/settings_str... File chrome/app/settings_strings.grdp (right): https://codereview.chromium.org/2153963002/diff/20001/chrome/app/settings_str... chrome/app/settings_strings.grdp:139: extremely short (600ms) On 2016/07/16 08:36:27, michaelpg wrote: > Did you check with tbuckley@ or bettes@ that we should include these numbers in > MD Settings? i think they're just updating based on changes to other code: https://codereview.chromium.org/2016073004 though we should possibly be doing a <ph> in both cases (ph = placeholder)
https://codereview.chromium.org/2153963002/diff/20001/chrome/app/settings_str... File chrome/app/settings_strings.grdp (right): https://codereview.chromium.org/2153963002/diff/20001/chrome/app/settings_str... chrome/app/settings_strings.grdp:139: extremely short (600ms) On 2016/07/18 19:44:36, Dan Beam wrote: > On 2016/07/16 08:36:27, michaelpg wrote: > > Did you check with tbuckley@ or bettes@ that we should include these numbers > in > > MD Settings? > > i think they're just updating based on changes to other code: > https://codereview.chromium.org/2016073004 > > though we should possibly be doing a <ph> in both cases (ph = placeholder) Got it. Thanks!
https://codereview.chromium.org/2153963002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/a11y_page/a11y_page.js (right): https://codereview.chromium.org/2153963002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/a11y_page/a11y_page.js:44: }, On 2016/07/16 00:35:27, Dan Beam wrote: > nit: > > return [ > { > value: 600, > name: loadTimeData.getString('delayBeforeClickExtremelyShort'), > }, > ... > ]; > > is probably more common indentation Done. https://codereview.chromium.org/2153963002/diff/20001/chrome/app/settings_str... File chrome/app/settings_strings.grdp (right): https://codereview.chromium.org/2153963002/diff/20001/chrome/app/settings_str... chrome/app/settings_strings.grdp:139: extremely short (600ms) On 2016/07/18 20:45:02, michaelpg wrote: > On 2016/07/18 19:44:36, Dan Beam wrote: > > On 2016/07/16 08:36:27, michaelpg wrote: > > > Did you check with tbuckley@ or bettes@ that we should include these numbers > > in > > > MD Settings? > > > > i think they're just updating based on changes to other code: > > https://codereview.chromium.org/2016073004 > > > > though we should possibly be doing a <ph> in both cases (ph = placeholder) > > Got it. Thanks! Just waiting for a reply now. https://codereview.chromium.org/2153963002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/a11y_page/a11y_page.html (right): https://codereview.chromium.org/2153963002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/a11y_page/a11y_page.html:6: <link rel="import" href="/settings_shared_css.html"> On 2016/07/16 08:36:27, michaelpg (slow) wrote: > import settings/i18n_setup.html for loadTimeData usage Done. https://codereview.chromium.org/2153963002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/a11y_page/a11y_page.html:50: <settings-dropdown-menu id="defaultAutoClickDelay" On 2016/07/16 08:36:27, michaelpg (slow) wrote: > Why does this need an ID? Done. https://codereview.chromium.org/2153963002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/a11y_page/a11y_page.html:51: pref="{{prefs.settings.a11y.autoclick_delay_ms}}" On 2016/07/16 08:36:27, michaelpg (slow) wrote: > 4-space indent, not 6 Done. https://codereview.chromium.org/2153963002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/a11y_page/a11y_page.js (right): https://codereview.chromium.org/2153963002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/a11y_page/a11y_page.js:28: autoClickDelayOptions_: { On 2016/07/16 08:36:27, michaelpg wrote: > this should be inside the <if expr="chromeos"> below, because: > > 1. stripping out unused code on other platforms reduces startup time and > resource size > 2. you'll get JS errors on non-chromeos otherwise Done. https://codereview.chromium.org/2153963002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/a11y_page/a11y_page.js:29: readOnly: true, On 2016/07/16 08:36:27, michaelpg wrote: > 2 space indent, not 1 Done.
lgtm https://codereview.chromium.org/2153963002/diff/20001/chrome/app/settings_str... File chrome/app/settings_strings.grdp (right): https://codereview.chromium.org/2153963002/diff/20001/chrome/app/settings_str... chrome/app/settings_strings.grdp:139: extremely short (600ms) On 2016/07/20 17:19:31, sammiequon wrote: > On 2016/07/18 20:45:02, michaelpg wrote: > > On 2016/07/18 19:44:36, Dan Beam wrote: > > > On 2016/07/16 08:36:27, michaelpg wrote: > > > > Did you check with tbuckley@ or bettes@ that we should include these > numbers > > > in > > > > MD Settings? > > > > > > i think they're just updating based on changes to other code: > > > https://codereview.chromium.org/2016073004 > > > > > > though we should possibly be doing a <ph> in both cases (ph = placeholder) > > > > Got it. Thanks! > > Just waiting for a reply now. well, feel free to land as is. Is there an open bug where my comment would be appropriate? https://bugs.chromium.org/p/chromium/issues/detail?id=610476#c10
The CQ bit was checked by sammiequon@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2153963002/#ps40001 (title: "Fixed patch set 2 errors.")
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 ========== Autoclick on md-settings is synced with autoclick on settings. BUG=623944 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Autoclick on md-settings is synced with autoclick on settings. BUG=623944 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Autoclick on md-settings is synced with autoclick on settings. BUG=623944 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Autoclick on md-settings is synced with autoclick on settings. BUG=623944 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/6302068a9d5156a5a17278ff14d868ffc3cf6546 Cr-Commit-Position: refs/heads/master@{#406717} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/6302068a9d5156a5a17278ff14d868ffc3cf6546 Cr-Commit-Position: refs/heads/master@{#406717} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
