Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2630623002:
cros: Allow user to configure note taking app in md-settings. (Closed)
|
Created:
3 years, 11 months ago by jdufault Modified:
3 years, 10 months ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, michaelpg+watch-options_chromium.org, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptioncros: Allow user to configure note taking app in md-settings.
This also updates the stylus page to reflect current md-settings style.
Some of the settings code (primarily device_stylus_handler.cc) has been ported
from the current options implementation.
BUG=676518
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2630623002
Cr-Commit-Position: refs/heads/master@{#448498}
Committed: https://chromium.googlesource.com/chromium/src/+/e074ea04adb95ba87ccb22ffea57cf7399949beb
Patch Set 1 : Initial upload #
Total comments: 30
Patch Set 2 : Rebase #Patch Set 3 : Address comments #
Total comments: 12
Patch Set 4 : Rebase #Patch Set 5 : Address comments #
Total comments: 2
Patch Set 6 : Rebase #Patch Set 7 : NoteAppInfo => settings.NoteAppInfo #Messages
Total messages: 69 (58 generated)
Description was changed from ========== cros: Allow user to configure note taking app in md-settings. This also updates the stylus page to reflect current md-settings style. Some of the settings code (primarily device_stylus_handler.cc) has been ported from the current options implementation. BUG=679518 ========== to ========== cros: Allow user to configure note taking app in md-settings. This also updates the stylus page to reflect current md-settings style. Some of the settings code (primarily device_stylus_handler.cc) has been ported from the current options implementation. BUG=679518 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by jdufault@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: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by jdufault@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: Try jobs failed on following builders: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
The CQ bit was checked by jdufault@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.
The CQ bit was checked by jdufault@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.
The CQ bit was checked by jdufault@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: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by jdufault@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.
The CQ bit was checked by jdufault@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.
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
Description was changed from ========== cros: Allow user to configure note taking app in md-settings. This also updates the stylus page to reflect current md-settings style. Some of the settings code (primarily device_stylus_handler.cc) has been ported from the current options implementation. BUG=679518 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== cros: Allow user to configure note taking app in md-settings. This also updates the stylus page to reflect current md-settings style. Some of the settings code (primarily device_stylus_handler.cc) has been ported from the current options implementation. BUG=679518 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
jdufault@chromium.org changed reviewers: + derat@chromium.org, stevenjb@chromium.org
derat@, stevenjb@, PTAL. Thanks!
lgtm for the non-polymer parts https://codereview.chromium.org/2630623002/diff/120001/chrome/app/settings_st... File chrome/app/settings_strings.grdp (right): https://codereview.chromium.org/2630623002/diff/120001/chrome/app/settings_st... chrome/app/settings_strings.grdp:2621: Choose from Google Play did this come from a mock? something like "Open Google Play" sounds a bit better to me. https://codereview.chromium.org/2630623002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/device_page/stylus.js (right): https://codereview.chromium.org/2630623002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/device_page/stylus.js:31: * True if the arc++ container has not finished starting yet. nit: i think i read/heard somewhere that we just call it "ARC" now https://codereview.chromium.org/2630623002/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/chromeos/device_stylus_handler.cc (right): https://codereview.chromium.org/2630623002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/chromeos/device_stylus_handler.cc:90: NoteTakingHelper::Get()->SetPreferredApp(Profile::FromWebUI(web_ui()), since we discussed it before: it ended up making sense to keep this method?
oh, and forgot to mention it, but the associated bug (http://crbug.com/679518) is unrelated.
Please also add tests to chrome/test/data/webui/settings/device_page_tests.js. https://codereview.chromium.org/2630623002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/device_page/stylus.html (right): https://codereview.chromium.org/2630623002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/device_page/stylus.html:48: <div class="secondary app-dropdown" Is app-dropdown needed here? This is just a string. https://codereview.chromium.org/2630623002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/device_page/stylus.html:50: <div>$i18n{stylusNoteTakingAppNoneAvailable}</div> extra <div> shouldn't be necessary. https://codereview.chromium.org/2630623002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/device_page/stylus.html:54: <div>$i18n{stylusNoteTakingAppWaitingForAndroid}</div> <div> shouldn't be necessary? https://codereview.chromium.org/2630623002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/device_page/stylus.html:55: <paper-spinner active></paper-spinner> Can we just modify the margin for this paper-spinner and eliminate app-dropdown (which is kind of confusing to parse). https://codereview.chromium.org/2630623002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/device_page/stylus.js (right): https://codereview.chromium.org/2630623002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/device_page/stylus.js:11: 'https://play.google.com/store/apps/collection/promotion_30023cb_stylus_apps'; Use '+' and multiple strings to avoid wrapping. https://codereview.chromium.org/2630623002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/device_page/stylus.js:28: appChoices_: [], appChoices_ : { type: Array, value: function() { return []; }, }, Otherwise all instances of this element will share the same array. https://codereview.chromium.org/2630623002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/device_page/stylus.js:32: * @type {boolean} not needed, see below. https://codereview.chromium.org/2630623002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/device_page/stylus.js:35: waitingForAndroid_: false, waitingForAndroid_: { type: Boolean, value: false, } https://codereview.chromium.org/2630623002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/device_page/stylus.js:43: chrome.send('requestNoteTakingApps'); In order to test this we will want to set up a proxy class (or use an existing one). See device_page_browser_proxy.js. https://codereview.chromium.org/2630623002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/device_page/stylus.js:51: /** @private */ Need @type for all args https://codereview.chromium.org/2630623002/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/chromeos/device_stylus_handler.cc (right): https://codereview.chromium.org/2630623002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/chromeos/device_stylus_handler.cc:58: Profile::FromWebUI(web_ui()))) { nit: This might be a little more clear with: auto available_aps = NoteTakingHelper::Get()->GetAvailableApps(Profile::FromWebUI(web_ui())) above. https://codereview.chromium.org/2630623002/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/chromeos/device_stylus_handler.h (right): https://codereview.chromium.org/2630623002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/chromeos/device_stylus_handler.h:30: void OnAvailableNoteTakingAppsUpdated() override; It's weird to have one override private and the other public, they should really both be public.
Patchset #2 (id:140001) has been deleted
The CQ bit was checked by jdufault@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: 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_...)
Patchset #3 (id:180001) has been deleted
Patchset #2 (id:160001) has been deleted
The CQ bit was checked by jdufault@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...
Description was changed from ========== cros: Allow user to configure note taking app in md-settings. This also updates the stylus page to reflect current md-settings style. Some of the settings code (primarily device_stylus_handler.cc) has been ported from the current options implementation. BUG=679518 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== cros: Allow user to configure note taking app in md-settings. This also updates the stylus page to reflect current md-settings style. Some of the settings code (primarily device_stylus_handler.cc) has been ported from the current options implementation. BUG=676518 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2630623002/diff/120001/chrome/app/settings_st... File chrome/app/settings_strings.grdp (right): https://codereview.chromium.org/2630623002/diff/120001/chrome/app/settings_st... chrome/app/settings_strings.grdp:2621: Choose from Google Play On 2017/01/19 00:50:05, Daniel Erat wrote: > did this come from a mock? something like "Open Google Play" sounds a bit better > to me. Done. It did come from a mock, but I ran this string past the designer and he also agreed it was better. https://codereview.chromium.org/2630623002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/device_page/stylus.html (right): https://codereview.chromium.org/2630623002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/device_page/stylus.html:48: <div class="secondary app-dropdown" On 2017/01/19 17:48:53, stevenjb wrote: > Is app-dropdown needed here? This is just a string. Yea, the secondary class adds a margin-top which breaks centering. https://codereview.chromium.org/2630623002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/device_page/stylus.html:50: <div>$i18n{stylusNoteTakingAppNoneAvailable}</div> On 2017/01/19 17:48:53, stevenjb wrote: > extra <div> shouldn't be necessary. Done. https://codereview.chromium.org/2630623002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/device_page/stylus.html:54: <div>$i18n{stylusNoteTakingAppWaitingForAndroid}</div> On 2017/01/19 17:48:53, stevenjb wrote: > <div> shouldn't be necessary? Done. https://codereview.chromium.org/2630623002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/device_page/stylus.html:55: <paper-spinner active></paper-spinner> On 2017/01/19 17:48:53, stevenjb wrote: > Can we just modify the margin for this paper-spinner and eliminate app-dropdown > (which is kind of confusing to parse). I've rewritten the css selectors to remove the app-dropdown class. https://codereview.chromium.org/2630623002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/device_page/stylus.js (right): https://codereview.chromium.org/2630623002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/device_page/stylus.js:11: 'https://play.google.com/store/apps/collection/promotion_30023cb_stylus_apps'; On 2017/01/19 17:48:53, stevenjb wrote: > Use '+' and multiple strings to avoid wrapping. Done. https://codereview.chromium.org/2630623002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/device_page/stylus.js:28: appChoices_: [], On 2017/01/19 17:48:53, stevenjb wrote: > appChoices_ : { > type: Array, > value: function() { return []; }, > }, > > Otherwise all instances of this element will share the same array. Done, since this makes it clearer. However, we never actually modify the given array so it should be okay either way. https://codereview.chromium.org/2630623002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/device_page/stylus.js:31: * True if the arc++ container has not finished starting yet. On 2017/01/19 00:50:05, Daniel Erat wrote: > nit: i think i read/heard somewhere that we just call it "ARC" now Done. https://codereview.chromium.org/2630623002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/device_page/stylus.js:32: * @type {boolean} On 2017/01/19 17:48:53, stevenjb wrote: > not needed, see below. Done. https://codereview.chromium.org/2630623002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/device_page/stylus.js:35: waitingForAndroid_: false, On 2017/01/19 17:48:53, stevenjb wrote: > waitingForAndroid_: { > type: Boolean, > value: false, > } Done. It's strange that if closure can understand this pattern it cannot undertand `waitingForAndroid_: false`. https://codereview.chromium.org/2630623002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/device_page/stylus.js:43: chrome.send('requestNoteTakingApps'); On 2017/01/19 17:48:53, stevenjb wrote: > In order to test this we will want to set up a proxy class (or use an existing > one). See device_page_browser_proxy.js. Done. https://codereview.chromium.org/2630623002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/device_page/stylus.js:51: /** @private */ On 2017/01/19 17:48:53, stevenjb wrote: > Need @type for all args Done. https://codereview.chromium.org/2630623002/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/chromeos/device_stylus_handler.cc (right): https://codereview.chromium.org/2630623002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/chromeos/device_stylus_handler.cc:58: Profile::FromWebUI(web_ui()))) { On 2017/01/19 17:48:53, stevenjb wrote: > nit: This might be a little more clear with: > auto available_aps = > NoteTakingHelper::Get()->GetAvailableApps(Profile::FromWebUI(web_ui())) > above. Done. https://codereview.chromium.org/2630623002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/chromeos/device_stylus_handler.cc:90: NoteTakingHelper::Get()->SetPreferredApp(Profile::FromWebUI(web_ui()), On 2017/01/19 00:50:05, Daniel Erat wrote: > since we discussed it before: it ended up making sense to keep this method? Yea, given the nature of how preferred values are updated (ie, not updated if the user does not actively select a different value) I figured it would be better to make the pref handling explicit. https://codereview.chromium.org/2630623002/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/chromeos/device_stylus_handler.h (right): https://codereview.chromium.org/2630623002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/chromeos/device_stylus_handler.h:30: void OnAvailableNoteTakingAppsUpdated() override; On 2017/01/19 17:48:53, stevenjb wrote: > It's weird to have one override private and the other public, they should really > both be public. Done.
https://codereview.chromium.org/2630623002/diff/220001/chrome/browser/resourc... File chrome/browser/resources/settings/device_page/device_page_browser_proxy.js (right): https://codereview.chromium.org/2630623002/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/device_page/device_page_browser_proxy.js:133: }, Hmm. I haven't seen this done as a proxy before. I would expect the usage to be more like: proxy_.addListener('onNoteTakingAppsUpdated', callback) Or proxy_.setNoteTakingAppsUpdatedCallback(callback) https://codereview.chromium.org/2630623002/diff/220001/chrome/browser/resourc... File chrome/browser/resources/settings/device_page/stylus.js (right): https://codereview.chromium.org/2630623002/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/device_page/stylus.js:30: value: () => {} Unfortunately we can no longer use ES6 in settings (it breaks vulcanization), specifically =>, for..of, and let. https://codereview.chromium.org/2630623002/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/device_page/stylus.js:46: settings.DevicePageBrowserProxyImpl.getInstance().requestNoteTakingApps(); Since we have several proxy calls here, we should probably set a local browserProxy_, e.g.: https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/appear... https://codereview.chromium.org/2630623002/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/device_page/stylus.js:56: * @param {Array<{name:string, value:string, preferred:boolean}>} apps {name:string, value:string, preferred:boolean} should be typed in the proxy JS, e.g.: https://cs.chromium.org/chromium/src/chrome/browser/resources/md_user_manager... https://codereview.chromium.org/2630623002/diff/220001/chrome/test/data/webui... File chrome/test/data/webui/settings/device_page_tests.js (right): https://codereview.chromium.org/2630623002/diff/220001/chrome/test/data/webui... chrome/test/data/webui/settings/device_page_tests.js:69: this.requestNoteTakingApps_ += 1; nit: ++ https://codereview.chromium.org/2630623002/diff/220001/chrome/test/data/webui... chrome/test/data/webui/settings/device_page_tests.js:794: Polymer.dom.flush(); It seems like we should be asserting after each of these updates?
Patchset #4 (id:240001) has been deleted
The CQ bit was checked by jdufault@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/2630623002/diff/220001/chrome/browser/resourc... File chrome/browser/resources/settings/device_page/device_page_browser_proxy.js (right): https://codereview.chromium.org/2630623002/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/device_page/device_page_browser_proxy.js:133: }, On 2017/01/25 01:40:08, stevenjb wrote: > Hmm. I haven't seen this done as a proxy before. I would expect the usage to be > more like: > > proxy_.addListener('onNoteTakingAppsUpdated', callback) > > Or > > proxy_.setNoteTakingAppsUpdatedCallback(callback) Done. https://codereview.chromium.org/2630623002/diff/220001/chrome/browser/resourc... File chrome/browser/resources/settings/device_page/stylus.js (right): https://codereview.chromium.org/2630623002/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/device_page/stylus.js:30: value: () => {} On 2017/01/25 01:40:08, stevenjb wrote: > Unfortunately we can no longer use ES6 in settings (it breaks vulcanization), > specifically =>, for..of, and let. Done. https://codereview.chromium.org/2630623002/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/device_page/stylus.js:46: settings.DevicePageBrowserProxyImpl.getInstance().requestNoteTakingApps(); On 2017/01/25 01:40:08, stevenjb wrote: > Since we have several proxy calls here, we should probably set a local > browserProxy_, e.g.: > https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/appear... Done. https://codereview.chromium.org/2630623002/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/device_page/stylus.js:56: * @param {Array<{name:string, value:string, preferred:boolean}>} apps On 2017/01/25 01:40:08, stevenjb wrote: > {name:string, value:string, preferred:boolean} should be typed in the proxy JS, > e.g.: > https://cs.chromium.org/chromium/src/chrome/browser/resources/md_user_manager... Done. https://codereview.chromium.org/2630623002/diff/220001/chrome/test/data/webui... File chrome/test/data/webui/settings/device_page_tests.js (right): https://codereview.chromium.org/2630623002/diff/220001/chrome/test/data/webui... chrome/test/data/webui/settings/device_page_tests.js:69: this.requestNoteTakingApps_ += 1; On 2017/01/25 01:40:08, stevenjb wrote: > nit: ++ Done. https://codereview.chromium.org/2630623002/diff/220001/chrome/test/data/webui... chrome/test/data/webui/settings/device_page_tests.js:794: Polymer.dom.flush(); On 2017/01/25 01:40:08, stevenjb wrote: > It seems like we should be asserting after each of these updates? I think the test is okay without the extra asserts, but to be on the safe side I've added them.
lgtm https://codereview.chromium.org/2630623002/diff/280001/chrome/browser/resourc... File chrome/browser/resources/settings/device_page/device_page_browser_proxy.js (right): https://codereview.chromium.org/2630623002/diff/280001/chrome/browser/resourc... chrome/browser/resources/settings/device_page/device_page_browser_proxy.js:39: var NoteAppInfo; Prefer putting this into the settings namespace and making it more specific, e.g. settings.NoteTakingAppInfo (it's currently global). (I realize the example I gave doesn't use the settings namespace, but we should at least be consistent within this file).
The CQ bit was checked by jdufault@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from derat@chromium.org, stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/2630623002/#ps320001 (title: "NoteAppInfo => settings.NoteAppInfo")
https://codereview.chromium.org/2630623002/diff/280001/chrome/browser/resourc... File chrome/browser/resources/settings/device_page/device_page_browser_proxy.js (right): https://codereview.chromium.org/2630623002/diff/280001/chrome/browser/resourc... chrome/browser/resources/settings/device_page/device_page_browser_proxy.js:39: var NoteAppInfo; On 2017/02/03 22:56:50, stevenjb wrote: > Prefer putting this into the settings namespace and making it more specific, > e.g. settings.NoteTakingAppInfo (it's currently global). > > (I realize the example I gave doesn't use the settings namespace, but we should > at least be consistent within this file). Done.
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": 320001, "attempt_start_ts": 1486425599373820,
"parent_rev": "ad7fe40bd171138a8c8b31c7f68b5e05d6761ea6", "commit_rev":
"e074ea04adb95ba87ccb22ffea57cf7399949beb"}
Message was sent while issue was closed.
Description was changed from ========== cros: Allow user to configure note taking app in md-settings. This also updates the stylus page to reflect current md-settings style. Some of the settings code (primarily device_stylus_handler.cc) has been ported from the current options implementation. BUG=676518 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== cros: Allow user to configure note taking app in md-settings. This also updates the stylus page to reflect current md-settings style. Some of the settings code (primarily device_stylus_handler.cc) has been ported from the current options implementation. BUG=676518 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2630623002 Cr-Commit-Position: refs/heads/master@{#448498} Committed: https://chromium.googlesource.com/chromium/src/+/e074ea04adb95ba87ccb22ffea57... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:320001) as https://chromium.googlesource.com/chromium/src/+/e074ea04adb95ba87ccb22ffea57... |
