Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2200463002:
Reorganize accessibility settings. (Closed)
|
Created:
4 years, 4 months ago by dmazzoni Modified:
4 years, 4 months ago CC:
arv+watch_chromium.org, chromium-reviews, dbeam+watch-settings_chromium.org, dbeam+watch-closure_chromium.org, jlklein+watch-closure_chromium.org, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, stevenjb+watch-md-settings_chromium.org, tommycli, vitalyp+closure_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionReorganize accessibility settings.
Based on the mock in the bug, put the accessibility settings into a subpage,
organize them by category, and add additional helpful links.
BUG=616802
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/2fa2772be4152c0291ff745f19bd3ace41bf82cd
Cr-Commit-Position: refs/heads/master@{#409924}
Patch Set 1 #
Total comments: 48
Patch Set 2 : Address feedback #Patch Set 3 : Finish i18n #
Total comments: 10
Patch Set 4 : Address feedback #
Total comments: 13
Patch Set 5 : ExtensionRegistry #
Total comments: 5
Patch Set 6 : Fix final nits #
Total comments: 11
Patch Set 7 : Address some feedback #
Total comments: 10
Patch Set 8 : Address last feedback from dbeam #Messages
Total messages: 33 (9 generated)
Description was changed from ========== Reorganize accessibility settings. Based on the mock in the bug, put the accessibility settings into a subpage, organize them by category, and add additional helpful links. BUG=616802 ========== to ========== Reorganize accessibility settings. Based on the mock in the bug, put the accessibility settings into a subpage, organize them by category, and add additional helpful links. BUG=616802 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
dmazzoni@chromium.org changed reviewers: + dbeam@chromium.org, michaelpg@chromium.org
Could I get an initial review? I think the layout is done, it matches the mock pretty well. Please give me feedback on the markup / css, if you'd prefer it to be done in a different way. I'm stuck on one thing: settings.navigateTo is undefined and I can't figure out what to include in order to get that to work. Any ideas? I haven't done i18n yet, that can be next.
On 2016/07/29 20:55:00, dmazzoni wrote: > Could I get an initial review? > > I think the layout is done, it matches the mock pretty well. > Please give me feedback on the markup / css, if you'd prefer > it to be done in a different way. > > I'm stuck on one thing: settings.navigateTo is undefined and > I can't figure out what to include in order to get that to work. > Any ideas? settings.navigateTo landed very recently (8a9687b5) so you may just have to rebase. You should only have to include /route.html. > > I haven't done i18n yet, that can be next.
On 2016/07/29 21:35:56, michaelpg wrote: > On 2016/07/29 20:55:00, dmazzoni wrote: > > Could I get an initial review? > > > > I think the layout is done, it matches the mock pretty well. > > Please give me feedback on the markup / css, if you'd prefer > > it to be done in a different way. > > > > I'm stuck on one thing: settings.navigateTo is undefined and > > I can't figure out what to include in order to get that to work. > > Any ideas? > > > settings.navigateTo landed very recently (8a9687b5) so you may just have to > rebase. You should only have to include /route.html. Yeah, the blob in the diff is old -- just rebase and check `git branch --contains 8a9687b5`. > > > > > I haven't done i18n yet, that can be next.
Aha, perfect! Please take a look for any other potential issues, otherwise I'll fix that and do i18n and then it should be ready. On Fri, Jul 29, 2016 at 2:46 PM <michaelpg@chromium.org> wrote: > On 2016/07/29 21:35:56, michaelpg wrote: > > On 2016/07/29 20:55:00, dmazzoni wrote: > > > Could I get an initial review? > > > > > > I think the layout is done, it matches the mock pretty well. > > > Please give me feedback on the markup / css, if you'd prefer > > > it to be done in a different way. > > > > > > I'm stuck on one thing: settings.navigateTo is undefined and > > > I can't figure out what to include in order to get that to work. > > > Any ideas? > > > > > > settings.navigateTo landed very recently (8a9687b5) so you may just have > to > > rebase. You should only have to include /route.html. > > Yeah, the blob in the diff is old -- just rebase and check `git branch > --contains 8a9687b5`. > > > > > > > > > > I haven't done i18n yet, that can be next. > > > > https://codereview.chromium.org/2200463002/ > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to chromium-reviews+unsubscribe@chromium.org. > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/a11y_page/a11y_page.html (right): https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/a11y_page/a11y_page.html:4: <if expr="chromeos"> this <if> should be below the rest of the imports, separated by whitespace, e.g.: https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/basic_... https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/a11y_page/a11y_page.html:7: <link rel="import" href="/settings_page/settings_animated_pages.html"> this & settings_subpage.html should also be in the <if expr="chromeos"> https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/a11y_page/a11y_page.html:22: <paper-toggle-button id="optionsInMenuToggle" import paper-toggle-button (cros only) https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/a11y_page/a11y_page.html:23: checked="{{prefs.settings.a11y.enable_menu}}"> .value <settings-checkbox> is a control that takes the pref object. here, you just want to bind to the boolean value itself. https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/a11y_page/a11y_page.html:23: checked="{{prefs.settings.a11y.enable_menu}}"> it looks like this can be controlled by policy: https://cs.chromium.org/chromium/src/chrome/browser/policy/configuration_poli... if that's the case, we'll need to support UI for that, and disabling the control. Look at the stuff we have for settings-checkbox -- if we're going to use paper-toggle-button for prefs, we should probably make a similar settings-toggle-button. Dan, agree? That's something we would do separately, either before this CL or after. https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/a11y_page/a11y_page.html:28: on-tap="onManageAccessibilityFeaturesTap_"> nit: 4-space indent from previous line, don't align https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/a11y_page/compiled_resources2.gyp (right): https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/a11y_page/compiled_resources2.gyp:9: '../compiled_resources2.gyp:route', nit: put after <(DEPTH) and <(EXTERNS_GYP) https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/a11y_page/compiled_resources2.gyp:10: '<(DEPTH)/ui/webui/resources/js/compiled_resources2.gyp:load_time_data', remove https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/a11y_page/compiled_resources2.gyp:11: '<(DEPTH)/ui/webui/resources/js/compiled_resources2.gyp:assert', remove? assert isn't used https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/a11y_page/compiled_resources2.gyp:19: '../compiled_resources2.gyp:route', same https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/a11y_page/compiled_resources2.gyp:21: '<(DEPTH)/ui/webui/resources/js/compiled_resources2.gyp:assert', also unused? https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/a11y_page/manage_a11y_page.html (right): https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/a11y_page/manage_a11y_page.html:3: <link rel="import" href="/route.html"> import /i18n_setup.html for load_time_data.js (include-what-you-use helps with testing) https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/a11y_page/manage_a11y_page.html:12: h3 { nit: blank line between rules https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/a11y_page/manage_a11y_page.html:13: color: rgb(90, 90, 90); Note: I asked in the bug whether this <h3> should be a common style. https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/a11y_page/manage_a11y_page.html:36: label="$i18n{chromeVoxLabel}"> 4-space indents, throughout https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/a11y_page/manage_a11y_page.html:38: <paper-icon-button icon="settings:settings" on-tap="onChromeVoxSettingsTap_"> wrap long lines, throughout https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/a11y_page/manage_a11y_page.html:112: <select value="{{prefs.settings.a11y.autoclick_delay_ms::change}}"> feel like upgrading this to a settings-dropdown-menu? :-D https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/a11y_page/manage_a11y_page.html:153: nit: remove blank line https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/a11y_page/manage_a11y_page.html:155: nit: remove blank line https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/a11y_page/manage_a11y_page.js (right): https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/a11y_page/manage_a11y_page.js:37: 'chrome-extension://mndnfokpggljbaajbnioimlmbfngpief/chromevox/background/options.html'); wrap also, should we do this in C++? https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/settings/font_ha... https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/a11y_page/manage_a11y_page.js:69: nit: remove blank line https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/advanced_page/advanced_page.html (right): https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/advanced_page/advanced_page.html:92: <settings-a11y-page prefs="{{prefs}}" current-route="{{currentRoute}}"> wrap (before current-route) https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/icons.html (right): https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/icons.html:96: <g id="settings"><path d="M19.43 12.98c.04-.32.07-.64.07-.98s-.03-.66-.07-.98l2.11-1.65c.19-.15.24-.42.12-.64l-2-3.46c-.12-.22-.39-.3-.61-.22l-2.49 1c-.52-.4-1.08-.73-1.69-.98l-.38-2.65C14.46 2.18 14.25 2 14 2h-4c-.25 0-.46.18-.49.42l-.38 2.65c-.61.25-1.17.59-1.69.98l-2.49-1c-.23-.09-.49 0-.61.22l-2 3.46c-.13.22-.07.49.12.64l2.11 1.65c-.04.32-.07.65-.07.98s.03.66.07.98l-2.11 1.65c-.19.15-.24.42-.12.64l2 3.46c.12.22.39.3.61.22l2.49-1c.52.4 1.08.73 1.69.98l.38 2.65c.03.24.24.42.49.42h4c.25 0 .46-.18.49-.42l.38-2.65c.61-.25 1.17-.59 1.69-.98l2.49 1c.23.09.49 0 .61-.22l2-3.46c.12-.22.07-.49-.12-.64l-2.11-1.65zM12 15.5c-1.93 0-3.5-1.57-3.5-3.5s1.57-3.5 3.5-3.5 3.5 1.57 3.5 3.5-1.57 3.5-3.5 3.5z"/></g> this already exists in cr_elements/icons.html which we import in settings, use icon="cr:settings" https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/settings_resources.grd (right): https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_resources.grd:24: <structure name="IDR_SETTINGS_MANAGE_A11Y_PAGE_JS" these should be <if expr="chromeos">
Feedback addressed and i18n done https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/a11y_page/a11y_page.html (right): https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/a11y_page/a11y_page.html:4: <if expr="chromeos"> On 2016/07/29 at 22:14:20, michaelpg wrote: > this <if> should be below the rest of the imports, separated by whitespace, e.g.: https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/basic_... Done https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/a11y_page/a11y_page.html:7: <link rel="import" href="/settings_page/settings_animated_pages.html"> On 2016/07/29 at 22:14:20, michaelpg wrote: > this & settings_subpage.html should also be in the <if expr="chromeos"> Done https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/a11y_page/a11y_page.html:22: <paper-toggle-button id="optionsInMenuToggle" On 2016/07/29 at 22:14:20, michaelpg wrote: > import paper-toggle-button (cros only) Done https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/a11y_page/a11y_page.html:23: checked="{{prefs.settings.a11y.enable_menu}}"> On 2016/07/29 at 22:14:20, michaelpg wrote: > .value > > <settings-checkbox> is a control that takes the pref object. here, you just want to bind to the boolean value itself. Done https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/a11y_page/a11y_page.html:28: on-tap="onManageAccessibilityFeaturesTap_"> On 2016/07/29 at 22:14:20, michaelpg wrote: > nit: 4-space indent from previous line, don't align Done https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/a11y_page/compiled_resources2.gyp (right): https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/a11y_page/compiled_resources2.gyp:9: '../compiled_resources2.gyp:route', On 2016/07/29 at 22:14:20, michaelpg wrote: > nit: put after <(DEPTH) and <(EXTERNS_GYP) Done https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/a11y_page/compiled_resources2.gyp:10: '<(DEPTH)/ui/webui/resources/js/compiled_resources2.gyp:load_time_data', On 2016/07/29 at 22:14:20, michaelpg wrote: > remove Done https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/a11y_page/compiled_resources2.gyp:11: '<(DEPTH)/ui/webui/resources/js/compiled_resources2.gyp:assert', On 2016/07/29 at 22:14:20, michaelpg wrote: > remove? assert isn't used Done https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/a11y_page/compiled_resources2.gyp:19: '../compiled_resources2.gyp:route', On 2016/07/29 at 22:14:20, michaelpg wrote: > same Done https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/a11y_page/compiled_resources2.gyp:21: '<(DEPTH)/ui/webui/resources/js/compiled_resources2.gyp:assert', On 2016/07/29 at 22:14:20, michaelpg wrote: > also unused? Done https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/a11y_page/manage_a11y_page.html (right): https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/a11y_page/manage_a11y_page.html:3: <link rel="import" href="/route.html"> On 2016/07/29 at 22:14:20, michaelpg wrote: > import /i18n_setup.html for load_time_data.js (include-what-you-use helps with testing) Done https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/a11y_page/manage_a11y_page.html:12: h3 { On 2016/07/29 at 22:14:20, michaelpg wrote: > nit: blank line between rules Done https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/a11y_page/manage_a11y_page.html:13: color: rgb(90, 90, 90); On 2016/07/29 at 22:14:21, michaelpg wrote: > Note: I asked in the bug whether this <h3> should be a common style. This definitely seems like it ought to be a heading. We should be using more semantic HTML tags where possible. https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/a11y_page/manage_a11y_page.html:36: label="$i18n{chromeVoxLabel}"> On 2016/07/29 at 22:14:21, michaelpg wrote: > 4-space indents, throughout Done https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/a11y_page/manage_a11y_page.html:38: <paper-icon-button icon="settings:settings" on-tap="onChromeVoxSettingsTap_"> On 2016/07/29 at 22:14:21, michaelpg wrote: > wrap long lines, throughout Done https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/a11y_page/manage_a11y_page.html:112: <select value="{{prefs.settings.a11y.autoclick_delay_ms::change}}"> On 2016/07/29 at 22:14:20, michaelpg wrote: > feel like upgrading this to a settings-dropdown-menu? :-D Looks like that was done in parallel. :) https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/a11y_page/manage_a11y_page.html:153: On 2016/07/29 at 22:14:21, michaelpg wrote: > nit: remove blank line Done https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/a11y_page/manage_a11y_page.html:155: On 2016/07/29 at 22:14:20, michaelpg wrote: > nit: remove blank line Done https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/a11y_page/manage_a11y_page.js (right): https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/a11y_page/manage_a11y_page.js:37: 'chrome-extension://mndnfokpggljbaajbnioimlmbfngpief/chromevox/background/options.html'); On 2016/07/29 at 22:14:21, michaelpg wrote: > wrap > > also, should we do this in C++? https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/settings/font_ha... Good idea, done - and thanks for pointing me to an example to follow. https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/a11y_page/manage_a11y_page.js:69: On 2016/07/29 at 22:14:21, michaelpg wrote: > nit: remove blank line Done https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/advanced_page/advanced_page.html (right): https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/advanced_page/advanced_page.html:92: <settings-a11y-page prefs="{{prefs}}" current-route="{{currentRoute}}"> On 2016/07/29 at 22:14:21, michaelpg wrote: > wrap (before current-route) Done https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/icons.html (right): https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/icons.html:96: <g id="settings"><path d="M19.43 12.98c.04-.32.07-.64.07-.98s-.03-.66-.07-.98l2.11-1.65c.19-.15.24-.42.12-.64l-2-3.46c-.12-.22-.39-.3-.61-.22l-2.49 1c-.52-.4-1.08-.73-1.69-.98l-.38-2.65C14.46 2.18 14.25 2 14 2h-4c-.25 0-.46.18-.49.42l-.38 2.65c-.61.25-1.17.59-1.69.98l-2.49-1c-.23-.09-.49 0-.61.22l-2 3.46c-.13.22-.07.49.12.64l2.11 1.65c-.04.32-.07.65-.07.98s.03.66.07.98l-2.11 1.65c-.19.15-.24.42-.12.64l2 3.46c.12.22.39.3.61.22l2.49-1c.52.4 1.08.73 1.69.98l.38 2.65c.03.24.24.42.49.42h4c.25 0 .46-.18.49-.42l.38-2.65c.61-.25 1.17-.59 1.69-.98l2.49 1c.23.09.49 0 .61-.22l2-3.46c.12-.22.07-.49-.12-.64l-2.11-1.65zM12 15.5c-1.93 0-3.5-1.57-3.5-3.5s1.57-3.5 3.5-3.5 3.5 1.57 3.5 3.5-1.57 3.5-3.5 3.5z"/></g> On 2016/07/29 at 22:14:21, michaelpg wrote: > this already exists in cr_elements/icons.html which we import in settings, use icon="cr:settings" Thanks https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/settings_resources.grd (right): https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_resources.grd:24: <structure name="IDR_SETTINGS_MANAGE_A11Y_PAGE_JS" On 2016/07/29 at 22:14:21, michaelpg wrote: > these should be <if expr="chromeos"> Done
can you run this somewhere other than ChromeOS to check that it works? https://codereview.chromium.org/2200463002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/a11y_page/a11y_page.html (right): https://codereview.chromium.org/2200463002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/a11y_page/a11y_page.html:8: <link rel="import" href="chrome://resources/polymer/v1_0/paper-button/paper-button.html"> duplicated https://codereview.chromium.org/2200463002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/a11y_page/a11y_page.html:22: <div class="settings-box"> class="settings-box first" https://codereview.chromium.org/2200463002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/a11y_page/a11y_page.js (left): https://codereview.chromium.org/2200463002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/a11y_page/a11y_page.js:63: onMoreFeaturesTap_: function() { you deleted this (still needed by the non-CrOS version) https://codereview.chromium.org/2200463002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/a11y_page/manage_a11y_page.js (right): https://codereview.chromium.org/2200463002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/a11y_page/manage_a11y_page.js:22: <if expr="chromeos"> isn't this whole page only <if cros>? can you just remove this <if>?
Thanks! Tested on Linux with chromeos=0 also. I had to change the code that opens the "manage accessibility" subpage since I last updated this changelist, is settings.navigateTo() the correct API now? https://codereview.chromium.org/2200463002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/a11y_page/a11y_page.html (right): https://codereview.chromium.org/2200463002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/a11y_page/a11y_page.html:8: <link rel="import" href="chrome://resources/polymer/v1_0/paper-button/paper-button.html"> On 2016/08/01 at 20:37:03, Dan Beam wrote: > duplicated Done https://codereview.chromium.org/2200463002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/a11y_page/a11y_page.html:22: <div class="settings-box"> On 2016/08/01 at 20:37:03, Dan Beam wrote: > class="settings-box first" Done https://codereview.chromium.org/2200463002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/a11y_page/a11y_page.js (left): https://codereview.chromium.org/2200463002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/a11y_page/a11y_page.js:63: onMoreFeaturesTap_: function() { On 2016/08/01 at 20:37:03, Dan Beam wrote: > you deleted this (still needed by the non-CrOS version) Good catch, done. https://codereview.chromium.org/2200463002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/a11y_page/manage_a11y_page.js (right): https://codereview.chromium.org/2200463002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/a11y_page/manage_a11y_page.js:22: <if expr="chromeos"> On 2016/08/01 at 20:37:03, Dan Beam wrote: > isn't this whole page only <if cros>? can you just remove this <if>? You're right, done
https://codereview.chromium.org/2200463002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/a11y_page/manage_a11y_page.html (right): https://codereview.chromium.org/2200463002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/a11y_page/manage_a11y_page.html:40: label="$i18n{chromeVoxLabel}"> nit: 4\s for HTML continuations https://codereview.chromium.org/2200463002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/chromeos/accessibility_handler.cc (right): https://codereview.chromium.org/2200463002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/chromeos/accessibility_handler.cc:13: #include "chrome/browser/ui/webui/settings_utils.h" what are you using settings_utils.h for? https://codereview.chromium.org/2200463002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/chromeos/accessibility_handler.cc:16: #include "extensions/browser/extension_registry.h" what are you using extension_registry.h for? https://codereview.chromium.org/2200463002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/chromeos/accessibility_handler.cc:38: extensions::ExtensionSystem::Get(profile_)->extension_service(); nit: can the extension system or service ever be null here? https://codereview.chromium.org/2200463002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/chromeos/accessibility_handler.cc:42: service->GetInstalledExtension(extension_misc::kChromeVoxExtensionId); nit: can we use Extension* chrome_vox = ExtensionRegistry::Get(profile_)->GetExtensionById( extension_misc::kChromeVoxExtensionId, extensions::ExtensionRegistry::ENABLED); if (!chrome_vox) return; to do the enabled and existence check all at once? https://codereview.chromium.org/2200463002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/chromeos/accessibility_handler.h (right): https://codereview.chromium.org/2200463002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/chromeos/accessibility_handler.h:31: void HandleShowChromeVoxSettings(const base::ListValue* args); nit: forward declare base::ListValue https://codereview.chromium.org/2200463002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/chromeos/accessibility_handler.h:31: void HandleShowChromeVoxSettings(const base::ListValue* args); please make this handler private
The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run
https://codereview.chromium.org/2200463002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/a11y_page/manage_a11y_page.html (right): https://codereview.chromium.org/2200463002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/a11y_page/manage_a11y_page.html:40: label="$i18n{chromeVoxLabel}"> On 2016/08/02 at 05:18:47, Dan Beam wrote: > nit: 4\s for HTML continuations Fixed https://codereview.chromium.org/2200463002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/chromeos/accessibility_handler.cc (right): https://codereview.chromium.org/2200463002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/chromeos/accessibility_handler.cc:13: #include "chrome/browser/ui/webui/settings_utils.h" On 2016/08/02 at 05:18:47, Dan Beam wrote: > what are you using settings_utils.h for? Removed. https://codereview.chromium.org/2200463002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/chromeos/accessibility_handler.cc:16: #include "extensions/browser/extension_registry.h" On 2016/08/02 at 05:18:47, Dan Beam wrote: > what are you using extension_registry.h for? Kept this in due to your suggestion below, and removed the includes of extension_service and extension_system instead. https://codereview.chromium.org/2200463002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/chromeos/accessibility_handler.cc:38: extensions::ExtensionSystem::Get(profile_)->extension_service(); On 2016/08/02 at 05:18:47, Dan Beam wrote: > nit: can the extension system or service ever be null here? I don't believe so, anywhere else in the code doesn't ever expect it to be null. I added a DCHECK. https://codereview.chromium.org/2200463002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/chromeos/accessibility_handler.cc:42: service->GetInstalledExtension(extension_misc::kChromeVoxExtensionId); On 2016/08/02 at 05:18:47, Dan Beam wrote: > nit: can we use > > Extension* chrome_vox = ExtensionRegistry::Get(profile_)->GetExtensionById( > extension_misc::kChromeVoxExtensionId, > extensions::ExtensionRegistry::ENABLED); > if (!chrome_vox) > return; > > to do the enabled and existence check all at once? Sure, that's cleaner. Thanks. https://codereview.chromium.org/2200463002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/chromeos/accessibility_handler.h (right): https://codereview.chromium.org/2200463002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/chromeos/accessibility_handler.h:31: void HandleShowChromeVoxSettings(const base::ListValue* args); On 2016/08/02 at 05:18:47, Dan Beam wrote: > please make this handler private Done
On 2016/08/02 03:07:02, dmazzoni wrote: > Thanks! > > Tested on Linux with chromeos=0 also. > > I had to change the code that opens the "manage accessibility" subpage since I > last updated this changelist, is settings.navigateTo() the correct API now? yep > > https://codereview.chromium.org/2200463002/diff/40001/chrome/browser/resource... > File chrome/browser/resources/settings/a11y_page/a11y_page.html (right): > > https://codereview.chromium.org/2200463002/diff/40001/chrome/browser/resource... > chrome/browser/resources/settings/a11y_page/a11y_page.html:8: <link rel="import" > href="chrome://resources/polymer/v1_0/paper-button/paper-button.html"> > On 2016/08/01 at 20:37:03, Dan Beam wrote: > > duplicated > > Done > > https://codereview.chromium.org/2200463002/diff/40001/chrome/browser/resource... > chrome/browser/resources/settings/a11y_page/a11y_page.html:22: <div > class="settings-box"> > On 2016/08/01 at 20:37:03, Dan Beam wrote: > > class="settings-box first" > > Done > > https://codereview.chromium.org/2200463002/diff/40001/chrome/browser/resource... > File chrome/browser/resources/settings/a11y_page/a11y_page.js (left): > > https://codereview.chromium.org/2200463002/diff/40001/chrome/browser/resource... > chrome/browser/resources/settings/a11y_page/a11y_page.js:63: onMoreFeaturesTap_: > function() { > On 2016/08/01 at 20:37:03, Dan Beam wrote: > > you deleted this (still needed by the non-CrOS version) > > Good catch, done. > > https://codereview.chromium.org/2200463002/diff/40001/chrome/browser/resource... > File chrome/browser/resources/settings/a11y_page/manage_a11y_page.js (right): > > https://codereview.chromium.org/2200463002/diff/40001/chrome/browser/resource... > chrome/browser/resources/settings/a11y_page/manage_a11y_page.js:22: <if > expr="chromeos"> > On 2016/08/01 at 20:37:03, Dan Beam wrote: > > isn't this whole page only <if cros>? can you just remove this <if>? > > You're right, done
lgtm https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/a11y_page/manage_a11y_page.html (right): https://codereview.chromium.org/2200463002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/a11y_page/manage_a11y_page.html:112: <select value="{{prefs.settings.a11y.autoclick_delay_ms::change}}"> On 2016/08/01 18:45:23, dmazzoni wrote: > On 2016/07/29 at 22:14:20, michaelpg wrote: > > feel like upgrading this to a settings-dropdown-menu? :-D > > Looks like that was done in parallel. :) Oh... I reviewed that, too! https://codereview.chromium.org/2200463002/diff/40001/chrome/app/chromeos_str... File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/2200463002/diff/40001/chrome/app/chromeos_str... chrome/app/chromeos_strings.grdp:2539: <message name="IDS_OPTIONS_SETTINGS_ACCESSIBILITY_MOUSE_AND_TOUCHPAD_HEADING" desc="In the settings tab, the heading for the section on mouse and touch pad settings."> nits: "touchpad" here & below https://codereview.chromium.org/2200463002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/a11y_page/a11y_page.html (right): https://codereview.chromium.org/2200463002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/a11y_page/a11y_page.html:4: <link rel="import" href="/i18n_setup.html"> nit: alphabetize (also, technically looks cros-only) https://codereview.chromium.org/2200463002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/a11y_page/a11y_page.js (right): https://codereview.chromium.org/2200463002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/a11y_page/a11y_page.js:8: * a couple of simple settings or links on most platforms, and a link to nit: remove "a link to" https://codereview.chromium.org/2200463002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/a11y_page/a11y_page.js:8: * a couple of simple settings or links on most platforms, and a link to "a couple of simple settings or links on most platforms" => a single button/link to the store?
https://codereview.chromium.org/2200463002/diff/40001/chrome/app/chromeos_str... File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/2200463002/diff/40001/chrome/app/chromeos_str... chrome/app/chromeos_strings.grdp:2539: <message name="IDS_OPTIONS_SETTINGS_ACCESSIBILITY_MOUSE_AND_TOUCHPAD_HEADING" desc="In the settings tab, the heading for the section on mouse and touch pad settings."> On 2016/08/02 at 17:38:29, michaelpg wrote: > nits: "touchpad" here & below Done https://codereview.chromium.org/2200463002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/a11y_page/a11y_page.html (right): https://codereview.chromium.org/2200463002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/a11y_page/a11y_page.html:4: <link rel="import" href="/i18n_setup.html"> On 2016/08/02 at 17:38:29, michaelpg wrote: > nit: alphabetize (also, technically looks cros-only) Fixed, but it is needed for non-cros https://codereview.chromium.org/2200463002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/a11y_page/a11y_page.js (right): https://codereview.chromium.org/2200463002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/a11y_page/a11y_page.js:8: * a couple of simple settings or links on most platforms, and a link to On 2016/08/02 at 17:38:29, michaelpg wrote: > "a couple of simple settings or links on most platforms" => a single button/link to the store? Fixed both
The CQ bit was checked by dmazzoni@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaelpg@chromium.org Link to the patchset: https://codereview.chromium.org/2200463002/#ps100001 (title: "Fix final nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2200463002/diff/100001/chrome/app/chromeos_st... File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/2200463002/diff/100001/chrome/app/chromeos_st... chrome/app/chromeos_strings.grdp:2503: <message name="IDS_OPTIONS_SETTINGS_ACCESSIBILITY_SELECT_TO_SPEAK_DESCRIPTION" desc="In the settings tab, the description of an option to hold a key and click to speak any on-screen text out loud."> why are these strings here rather than in chrome/app/settings_strings.grdp? https://codereview.chromium.org/2200463002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/a11y_page/manage_a11y_page.html (right): https://codereview.chromium.org/2200463002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/a11y_page/manage_a11y_page.html:10: div.indented { nit: div doesn't help this selector much https://codereview.chromium.org/2200463002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/a11y_page/manage_a11y_page.html:15: color: rgb(90, 90, 90); where is this color coming from? is this a --google or --paper color? https://codereview.chromium.org/2200463002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/a11y_page/manage_a11y_page.html:57: pref="{{prefs.settings.a11y.high_contrast_enabled}}"> wrong indent https://codereview.chromium.org/2200463002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_resources.grd (right): https://codereview.chromium.org/2200463002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/settings_resources.grd:32: flattenhtml="true" why do you need to flatten? it's probably better not to
The CQ bit was unchecked by michaelpg@chromium.org
On 2016/08/02 18:46:32, michaelpg wrote: > The CQ bit was unchecked by mailto:michaelpg@chromium.org cancelled CQ per dan's feedback
Wasn't sure about the strings file, see below. https://codereview.chromium.org/2200463002/diff/100001/chrome/app/chromeos_st... File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/2200463002/diff/100001/chrome/app/chromeos_st... chrome/app/chromeos_strings.grdp:2503: <message name="IDS_OPTIONS_SETTINGS_ACCESSIBILITY_SELECT_TO_SPEAK_DESCRIPTION" desc="In the settings tab, the description of an option to hold a key and click to speak any on-screen text out loud."> On 2016/08/02 at 18:43:03, Dan Beam wrote: > why are these strings here rather than in chrome/app/settings_strings.grdp? Some of these strings are used by the old options page, can that page access chrome/app/settings_strings.grdp too? It looks like there are >400 strings matching IDS_OPTIONS_SETTINGS in this file. Is this just historical or is there a rhyme or reason to where to put settings strings? https://codereview.chromium.org/2200463002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/a11y_page/manage_a11y_page.html (right): https://codereview.chromium.org/2200463002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/a11y_page/manage_a11y_page.html:10: div.indented { On 2016/08/02 at 18:43:03, Dan Beam wrote: > nit: div doesn't help this selector much Done https://codereview.chromium.org/2200463002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/a11y_page/manage_a11y_page.html:15: color: rgb(90, 90, 90); On 2016/08/02 at 18:43:03, Dan Beam wrote: > where is this color coming from? is this a --google or --paper color? Got this from the mock. Changed to --settings-nav-grey, which matches the style of the <h2> in settings_subpage. https://codereview.chromium.org/2200463002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/a11y_page/manage_a11y_page.html:57: pref="{{prefs.settings.a11y.high_contrast_enabled}}"> On 2016/08/02 at 18:43:03, Dan Beam wrote: > wrong indent Fixed https://codereview.chromium.org/2200463002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_resources.grd (right): https://codereview.chromium.org/2200463002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/settings_resources.grd:32: flattenhtml="true" On 2016/08/02 at 18:43:03, Dan Beam wrote: > why do you need to flatten? it's probably better not to I was just copying from above. Do you want to get rid of it from the rules for a11y_page, too?
https://codereview.chromium.org/2200463002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_resources.grd (right): https://codereview.chromium.org/2200463002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/settings_resources.grd:32: flattenhtml="true" On 2016/08/03 19:21:48, dmazzoni wrote: > On 2016/08/02 at 18:43:03, Dan Beam wrote: > > why do you need to flatten? it's probably better not to > > I was just copying from above. Do you want to get rid of > it from the rules for a11y_page, too? * preprocess="true" in grd processes <if> and <include> * flattenhtml="true" does this ^ AND a whole bunch of magical inlining * allowexternalscript="true" is a dependent option that only has an effect when flattenhtml="true", I believe, so it's also unlikely to be needed. a11y_page.html has <if> nodes, therefore it needs one of {preprocess,flattenhtml}; preprocess does less and is preferred though I wouldn't muddy your CL with this. manage_a11y_page.{html,js} have no <if> nor <include>, so don't use these options there.
On 2016/08/03 at 21:19:46, dbeam wrote:
> a11y_page.html has <if> nodes, therefore it needs one of
{preprocess,flattenhtml}; preprocess does less and is preferred though I
wouldn't muddy your CL with this.
>
> manage_a11y_page.{html,js} have no <if> nor <include>, so don't use these
options there.
Thanks.
I got rid of flattenhtml, so this should be good.
Any thoughts on where to put the strings?
lgtm w/nits https://codereview.chromium.org/2200463002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/a11y_page/manage_a11y_page.html (right): https://codereview.chromium.org/2200463002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/a11y_page/manage_a11y_page.html:47: <div class="settings-box first indented"> i don't think this should be a "first" class https://codereview.chromium.org/2200463002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/a11y_page/manage_a11y_page.html:145: label="$i18n{monoAudioLabel}"> wrong indent https://codereview.chromium.org/2200463002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/a11y_page/manage_a11y_page.js (right): https://codereview.chromium.org/2200463002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/a11y_page/manage_a11y_page.js:26: showExperimentalFeatures_: { why did you move this above autoClickDelayOptions_? https://codereview.chromium.org/2200463002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_resources.grd (right): https://codereview.chromium.org/2200463002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/settings_resources.grd:31: allowexternalscript="true" /> nit: I don't think you need allowexternalscript="true" either https://codereview.chromium.org/2200463002/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/chromeos/accessibility_handler.cc (right): https://codereview.chromium.org/2200463002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/chromeos/accessibility_handler.cc:7: #include "base/bind.h" nit: #include "base/bind_helpers.h" for base::Unretained()
The CQ bit was checked by dmazzoni@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaelpg@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2200463002/#ps140001 (title: "Address last feedback from dbeam")
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 #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Reorganize accessibility settings. Based on the mock in the bug, put the accessibility settings into a subpage, organize them by category, and add additional helpful links. BUG=616802 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Reorganize accessibility settings. Based on the mock in the bug, put the accessibility settings into a subpage, organize them by category, and add additional helpful links. BUG=616802 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/2fa2772be4152c0291ff745f19bd3ace41bf82cd Cr-Commit-Position: refs/heads/master@{#409924} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/2fa2772be4152c0291ff745f19bd3ace41bf82cd Cr-Commit-Position: refs/heads/master@{#409924}
Message was sent while issue was closed.
https://codereview.chromium.org/2200463002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/a11y_page/manage_a11y_page.html (right): https://codereview.chromium.org/2200463002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/a11y_page/manage_a11y_page.html:47: <div class="settings-box first indented"> On 2016/08/04 at 17:57:05, Dan Beam wrote: > i don't think this should be a "first" class Switched to a new class no-top-border instead https://codereview.chromium.org/2200463002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/a11y_page/manage_a11y_page.html:145: label="$i18n{monoAudioLabel}"> On 2016/08/04 at 17:57:05, Dan Beam wrote: > wrong indent Done https://codereview.chromium.org/2200463002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/a11y_page/manage_a11y_page.js (right): https://codereview.chromium.org/2200463002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/a11y_page/manage_a11y_page.js:26: showExperimentalFeatures_: { On 2016/08/04 at 17:57:05, Dan Beam wrote: > why did you move this above autoClickDelayOptions_? No reason. Moved it back for a shorter diff. https://codereview.chromium.org/2200463002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_resources.grd (right): https://codereview.chromium.org/2200463002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/settings_resources.grd:31: allowexternalscript="true" /> On 2016/08/04 at 17:57:05, Dan Beam wrote: > nit: I don't think you need allowexternalscript="true" either Done https://codereview.chromium.org/2200463002/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/chromeos/accessibility_handler.cc (right): https://codereview.chromium.org/2200463002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/chromeos/accessibility_handler.cc:7: #include "base/bind.h" On 2016/08/04 at 17:57:05, Dan Beam wrote: > nit: #include "base/bind_helpers.h" > > for base::Unretained() Done |
