Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2905373002:
Set keys for SwitchAccessEventHandler to capture using accessibiltyPrivate API (Closed)
|
Created:
3 years, 6 months ago by elichtenberg Modified:
3 years, 6 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, alemate+watch_chromium.org, oshima+watch_chromium.org, aboxhall+watch_chromium.org, jlklein+watch-closure_chromium.org, nektar+watch_chromium.org, yuzo+watch_chromium.org, dougt+watch_chromium.org, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, vitalyp+closure_chromium.org, chromium-apps-reviews_chromium.org, arv+watch_chromium.org, davemoore+watch_chromium.org, je_julie, dbeam+watch-closure_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionSet keys for SwitchAccessEventHandler to capture using accessibiltyPrivate API
BUG=593885
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2905373002
Cr-Commit-Position: refs/heads/master@{#476793}
Committed: https://chromium.googlesource.com/chromium/src/+/a108c8ab965c5fa52da3472f04cf507437e6ce37
Patch Set 1 #Patch Set 2 : Updated extension histograms #
Total comments: 12
Patch Set 3 : Fixed extension histograms #Patch Set 4 : Responded to comments #
Total comments: 2
Patch Set 5 : Responded to comment #
Total comments: 27
Patch Set 6 : Responded to comments #
Total comments: 4
Patch Set 7 : Responded to comments #
Total comments: 4
Patch Set 8 : Responded to comment #Dependent Patchsets: Messages
Total messages: 55 (34 generated)
Description was changed from ========== Set keys for SwitchAccessEventHandler to capture using accessibiltyPrivate API BUG=593885 ========== to ========== Set keys for SwitchAccessEventHandler to capture using accessibiltyPrivate API BUG=593885 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by elichtenberg@google.com 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...
elichtenberg@google.com changed reviewers: + dmazzoni@chromium.org, dtseng@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by elichtenberg@google.com 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...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by elichtenberg@google.com 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...
https://codereview.chromium.org/2905373002/diff/20001/chrome/browser/accessib... File chrome/browser/accessibility/accessibility_extension_api.cc (right): https://codereview.chromium.org/2905373002/diff/20001/chrome/browser/accessib... chrome/browser/accessibility/accessibility_extension_api.cc:176: std::vector<int> key_codes; Is this supposed to be ordered or are you maybe looking for a set? https://codereview.chromium.org/2905373002/diff/20001/chrome/browser/accessib... chrome/browser/accessibility/accessibility_extension_api.cc:180: if (key_code >= 48 && key_code <= 90) You should be able to grab key codes from a number of headers in the Chrome source. https://codereview.chromium.org/2905373002/diff/20001/chrome/browser/accessib... chrome/browser/accessibility/accessibility_extension_api.cc:185: chromeos::AccessibilityManager::Get(); Can be null. https://codereview.chromium.org/2905373002/diff/20001/chrome/browser/accessib... chrome/browser/accessibility/accessibility_extension_api.cc:187: return RespondNow(NoArguments()); You should probably respond with an error for various cases above. https://codereview.chromium.org/2905373002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/accessibility/switch_access_event_handler.cc (right): https://codereview.chromium.org/2905373002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/switch_access_event_handler.cc:36: if (std::find(captured_keys_.begin(), captured_keys_.end(), key_code) != Do you care that this will capture as an example, 1 and shift+1? https://codereview.chromium.org/2905373002/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/switch_access/keyboard_handler.js (right): https://codereview.chromium.org/2905373002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/switch_access/keyboard_handler.js:33: [49, 50, 51, 52, 54, 55, 56, 57]); Use ''.charCodeAt rather than hard coding the key codes (e.g. '1'.charCodeAt(0) == 49).
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 elichtenberg@google.com to run a CQ dry run
https://codereview.chromium.org/2905373002/diff/20001/chrome/browser/accessib... File chrome/browser/accessibility/accessibility_extension_api.cc (right): https://codereview.chromium.org/2905373002/diff/20001/chrome/browser/accessib... chrome/browser/accessibility/accessibility_extension_api.cc:176: std::vector<int> key_codes; On 2017/05/26 22:10:31, David Tseng wrote: > Is this supposed to be ordered or are you maybe looking for a set? Yep, I did mean to do a set. Thanks! https://codereview.chromium.org/2905373002/diff/20001/chrome/browser/accessib... chrome/browser/accessibility/accessibility_extension_api.cc:180: if (key_code >= 48 && key_code <= 90) On 2017/05/26 22:10:31, David Tseng wrote: > You should be able to grab key codes from a number of headers in the Chrome > source. Done. https://codereview.chromium.org/2905373002/diff/20001/chrome/browser/accessib... chrome/browser/accessibility/accessibility_extension_api.cc:185: chromeos::AccessibilityManager::Get(); On 2017/05/26 22:10:31, David Tseng wrote: > Can be null. Done. https://codereview.chromium.org/2905373002/diff/20001/chrome/browser/accessib... chrome/browser/accessibility/accessibility_extension_api.cc:187: return RespondNow(NoArguments()); On 2017/05/26 22:10:31, David Tseng wrote: > You should probably respond with an error for various cases above. What cases are you thinking of? Like if the key codes list is empty or has non-alphanumeric characters? Maybe we should discuss this offline. https://codereview.chromium.org/2905373002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/accessibility/switch_access_event_handler.cc (right): https://codereview.chromium.org/2905373002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/switch_access_event_handler.cc:36: if (std::find(captured_keys_.begin(), captured_keys_.end(), key_code) != On 2017/05/26 22:10:31, David Tseng wrote: > Do you care that this will capture as an example, 1 and shift+1? I'm fine with that. I don't see any reason to make it more restrictive for now. https://codereview.chromium.org/2905373002/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/switch_access/keyboard_handler.js (right): https://codereview.chromium.org/2905373002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/switch_access/keyboard_handler.js:33: [49, 50, 51, 52, 54, 55, 56, 57]); On 2017/05/26 22:10:31, David Tseng wrote: > Use ''.charCodeAt rather than hard coding the key codes (e.g. '1'.charCodeAt(0) > == 49). Done.
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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
lgtm https://codereview.chromium.org/2905373002/diff/60001/chrome/browser/accessib... File chrome/browser/accessibility/accessibility_extension_api.cc (right): https://codereview.chromium.org/2905373002/diff/60001/chrome/browser/accessib... chrome/browser/accessibility/accessibility_extension_api.cc:180: key_codes_list->GetInteger(i, &key_code); GetInteger returns true on success, so put EXTENSION_FUNCTION_VALIDATE around this so that it fails if you try to pass something other than an integer
dmazzoni@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
+devlin for extensions API
The CQ bit was checked by elichtenberg@google.com 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...
https://codereview.chromium.org/2905373002/diff/60001/chrome/browser/accessib... File chrome/browser/accessibility/accessibility_extension_api.cc (right): https://codereview.chromium.org/2905373002/diff/60001/chrome/browser/accessib... chrome/browser/accessibility/accessibility_extension_api.cc:180: key_codes_list->GetInteger(i, &key_code); On 2017/05/30 15:09:13, dmazzoni wrote: > GetInteger returns true on success, so put EXTENSION_FUNCTION_VALIDATE around > this so that it fails if you try to pass something other than an integer Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2905373002/diff/80001/chrome/browser/accessib... File chrome/browser/accessibility/accessibility_extension_api.cc (right): https://codereview.chromium.org/2905373002/diff/80001/chrome/browser/accessib... chrome/browser/accessibility/accessibility_extension_api.cc:171: #if defined(OS_CHROMEOS) Is there a reason to have this function present on non-chromeos platforms, rather than making it only available on ChromeOS? https://codereview.chromium.org/2905373002/diff/80001/chrome/browser/accessib... chrome/browser/accessibility/accessibility_extension_api.cc:172: base::ListValue* key_codes_list = NULL; nit: nullptr in new code, though possibly moot with below. https://codereview.chromium.org/2905373002/diff/80001/chrome/browser/accessib... chrome/browser/accessibility/accessibility_extension_api.cc:173: EXTENSION_FUNCTION_VALIDATE(args_->GetSize() == 1); Any reason to not use the generated extension parsing? https://codereview.chromium.org/2905373002/diff/80001/chrome/browser/accessib... chrome/browser/accessibility/accessibility_extension_api.cc:179: int key_code; nit: initialize https://codereview.chromium.org/2905373002/diff/80001/chrome/browser/accessib... chrome/browser/accessibility/accessibility_extension_api.cc:182: key_codes.insert(key_code); should there be an error if the key is out of bounds? (See also comment in .json) https://codereview.chromium.org/2905373002/diff/80001/chrome/browser/accessib... chrome/browser/accessibility/accessibility_extension_api.cc:187: if (manager) When can this be null? Should we be returning an error? https://codereview.chromium.org/2905373002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/accessibility/switch_access_event_handler.h (right): https://codereview.chromium.org/2905373002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/switch_access_event_handler.h:8: #include <string> not needed https://codereview.chromium.org/2905373002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/switch_access_event_handler.h:28: void SetKeysToCapture(const std::unordered_set<int>& key_codes); Any reason to use std::unordered_set over std::set? https://codereview.chromium.org/2905373002/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/switch_access/keyboard_handler.js (right): https://codereview.chromium.org/2905373002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/switch_access/keyboard_handler.js:33: ['1'.charCodeAt(0), '2'.charCodeAt(0), '3'.charCodeAt(0), optional: to me, this would potentially be more readable (and less prone to error) as: // Capture keycodes for keys 1 through 4, and 6 through 9. var keys = ['1', '2', '3', '4', '6', '7', '8', '9'].map(function(key) { return key.charCodeAt(0); }); But up to you. https://codereview.chromium.org/2905373002/diff/80001/chrome/common/extension... File chrome/common/extensions/api/accessibility_private.json (right): https://codereview.chromium.org/2905373002/diff/80001/chrome/common/extension... chrome/common/extensions/api/accessibility_private.json:112: "items": { "type": "integer" }, note: we support min and max values, so if we know these should always be between 0 and 'z', we could hard-code those values here with a comment and it would be checked js-side.
https://codereview.chromium.org/2905373002/diff/80001/chrome/browser/accessib... File chrome/browser/accessibility/accessibility_extension_api.cc (right): https://codereview.chromium.org/2905373002/diff/80001/chrome/browser/accessib... chrome/browser/accessibility/accessibility_extension_api.cc:171: #if defined(OS_CHROMEOS) On 2017/05/30 19:22:03, Devlin wrote: > Is there a reason to have this function present on non-chromeos platforms, > rather than making it only available on ChromeOS? Good point. Eli, I think you want "platforms": ["chromeos"] in the .json file
The CQ bit was checked by elichtenberg@google.com 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...
https://codereview.chromium.org/2905373002/diff/80001/chrome/browser/accessib... File chrome/browser/accessibility/accessibility_extension_api.cc (right): https://codereview.chromium.org/2905373002/diff/80001/chrome/browser/accessib... chrome/browser/accessibility/accessibility_extension_api.cc:171: #if defined(OS_CHROMEOS) On 2017/05/30 19:34:02, dmazzoni wrote: > On 2017/05/30 19:22:03, Devlin wrote: > > Is there a reason to have this function present on non-chromeos platforms, > > rather than making it only available on ChromeOS? > > Good point. Eli, I think you want "platforms": ["chromeos"] in the .json file Done. https://codereview.chromium.org/2905373002/diff/80001/chrome/browser/accessib... chrome/browser/accessibility/accessibility_extension_api.cc:172: base::ListValue* key_codes_list = NULL; On 2017/05/30 19:22:03, Devlin wrote: > nit: nullptr in new code, though possibly moot with below. Done. https://codereview.chromium.org/2905373002/diff/80001/chrome/browser/accessib... chrome/browser/accessibility/accessibility_extension_api.cc:173: EXTENSION_FUNCTION_VALIDATE(args_->GetSize() == 1); On 2017/05/30 19:22:03, Devlin wrote: > Any reason to not use the generated extension parsing? What exactly do you mean by generated extension parsing? I'm not sure what to change. Are you saying there's some other way of getting the size of args or getting the list of key codes without using GetSize or GetList? https://codereview.chromium.org/2905373002/diff/80001/chrome/browser/accessib... chrome/browser/accessibility/accessibility_extension_api.cc:179: int key_code; On 2017/05/30 19:22:03, Devlin wrote: > nit: initialize Done. https://codereview.chromium.org/2905373002/diff/80001/chrome/browser/accessib... chrome/browser/accessibility/accessibility_extension_api.cc:182: key_codes.insert(key_code); On 2017/05/30 19:22:03, Devlin wrote: > should there be an error if the key is out of bounds? (See also comment in > .json) Changing it to have an error instead of just filtering out invalid keys. https://codereview.chromium.org/2905373002/diff/80001/chrome/browser/accessib... chrome/browser/accessibility/accessibility_extension_api.cc:187: if (manager) On 2017/05/30 19:22:03, Devlin wrote: > When can this be null? Should we be returning an error? It can be null when the system is shutting down, but we shouldn't need to return an error then. https://codereview.chromium.org/2905373002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/accessibility/switch_access_event_handler.h (right): https://codereview.chromium.org/2905373002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/switch_access_event_handler.h:8: #include <string> On 2017/05/30 19:22:03, Devlin wrote: > not needed Done. https://codereview.chromium.org/2905373002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/switch_access_event_handler.h:28: void SetKeysToCapture(const std::unordered_set<int>& key_codes); On 2017/05/30 19:22:03, Devlin wrote: > Any reason to use std::unordered_set over std::set? I believe unordered_sets are faster than sets. I think unordered_sets are hashsets, and sets are binary search trees. https://codereview.chromium.org/2905373002/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/switch_access/keyboard_handler.js (right): https://codereview.chromium.org/2905373002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/switch_access/keyboard_handler.js:33: ['1'.charCodeAt(0), '2'.charCodeAt(0), '3'.charCodeAt(0), On 2017/05/30 19:22:03, Devlin wrote: > optional: to me, this would potentially be more readable (and less prone to > error) as: > // Capture keycodes for keys 1 through 4, and 6 through 9. > var keys = ['1', '2', '3', '4', '6', '7', '8', '9'].map(function(key) { return > key.charCodeAt(0); }); > > But up to you. Good idea. I changed it to an arrow function too, so that should also make it cleaner. https://codereview.chromium.org/2905373002/diff/80001/chrome/common/extension... File chrome/common/extensions/api/accessibility_private.json (right): https://codereview.chromium.org/2905373002/diff/80001/chrome/common/extension... chrome/common/extensions/api/accessibility_private.json:112: "items": { "type": "integer" }, On 2017/05/30 19:22:03, Devlin wrote: > note: we support min and max values, so if we know these should always be > between 0 and 'z', we could hard-code those values here with a comment and it > would be checked js-side. Would it be better to just do this check in accessibility_extension_api.cc? Because that way, we can get the key codes from headers, rather than hard coding the key codes. Also, we might expand to other keys later, so just having a minimum and maximum key code might not work later on.
(just responding to questions; will take another look at the latest patch set tomorrow) https://codereview.chromium.org/2905373002/diff/80001/chrome/browser/accessib... File chrome/browser/accessibility/accessibility_extension_api.cc (right): https://codereview.chromium.org/2905373002/diff/80001/chrome/browser/accessib... chrome/browser/accessibility/accessibility_extension_api.cc:173: EXTENSION_FUNCTION_VALIDATE(args_->GetSize() == 1); On 2017/05/31 00:58:43, elichtenberg wrote: > On 2017/05/30 19:22:03, Devlin wrote: > > Any reason to not use the generated extension parsing? > > What exactly do you mean by generated extension parsing? I'm not sure what to > change. Are you saying there's some other way of getting the size of args or > getting the list of key codes without using GetSize or GetList? We auto-generate typed structs and parsing code for extension APIs. The generated files are, in this case, <out_dir>/gen/chrome/common/api/accessibility_private.[h|cc]. You can use those to parse the input params, doing something like: std::unique_ptr<accessibility_private::SetSwitchAccessKeys::Params> params = accessibility_private::SetSwitchAccessKeys::Params::Create(*args_); EXTENSION_FUNCTION_VALIDATE(params); and then params contains a std::vector<int>, rather than needing to iterate over the list value. This approach is generally less error-prone, since it doesn't rely on argument indices, etc. https://codereview.chromium.org/2905373002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/accessibility/switch_access_event_handler.h (right): https://codereview.chromium.org/2905373002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/switch_access_event_handler.h:28: void SetKeysToCapture(const std::unordered_set<int>& key_codes); On 2017/05/31 00:58:43, elichtenberg wrote: > On 2017/05/30 19:22:03, Devlin wrote: > > Any reason to use std::unordered_set over std::set? > > I believe unordered_sets are faster than sets. I think unordered_sets are > hashsets, and sets are binary search trees. "faster" depends entirely on data set size. The algorithmic lookup complexity is faster, but for small data sets (like this), a regular std::set is likely faster, and certainly less memory (a vector would be better yet, but doesn't have the de-duping/counting niceties of a set). Unless there's a very concrete reason to use the unordered counterparts (i.e., known to be large data sets and performance-sensitive code), prefer using the vanilla std::set. (The same goes for std::map vs std::unordered_map.) I think brettw@ wrote up a nice post in chromium-dev that highlights this, along with some more scientific measurements, awhile back. https://codereview.chromium.org/2905373002/diff/80001/chrome/common/extension... File chrome/common/extensions/api/accessibility_private.json (right): https://codereview.chromium.org/2905373002/diff/80001/chrome/common/extension... chrome/common/extensions/api/accessibility_private.json:112: "items": { "type": "integer" }, On 2017/05/31 00:58:44, elichtenberg wrote: > On 2017/05/30 19:22:03, Devlin wrote: > > note: we support min and max values, so if we know these should always be > > between 0 and 'z', we could hard-code those values here with a comment and it > > would be checked js-side. > > Would it be better to just do this check in accessibility_extension_api.cc? > Because that way, we can get the key codes from headers, rather than hard coding > the key codes. Also, we might expand to other keys later, so just having a > minimum and maximum key code might not work later on. The advantage to having the minimum specified here is that it will be validated synchronously in the renderer, so if someone tried to do setSwitchAccessKeys([-1]), it would immediately throw an error. This is often nicer than throwing an error asynchronously in runtime.lastError, which is what happens when we return the result from the browser process. But, as you noted, this means we can't import the exact values from the headers. Given this is a private API, I'm fine either way.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
A few last comments + the ones from the previous patch set. https://codereview.chromium.org/2905373002/diff/100001/chrome/browser/accessi... File chrome/browser/accessibility/accessibility_extension_api.h (right): https://codereview.chromium.org/2905373002/diff/100001/chrome/browser/accessi... chrome/browser/accessibility/accessibility_extension_api.h:53: // API function that sets the keys to be captured by Switch Access. We should be able to have this declaration only on CrOS. https://codereview.chromium.org/2905373002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/chromeos/switch_access/keyboard_handler.js (right): https://codereview.chromium.org/2905373002/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/switch_access/keyboard_handler.js:33: key => key.charCodeAt(0)); Note: arrow functions (last I checked) weren't available in all versions of iOS. I'm assuming this code is only run on CrOS; is that accurate?
https://codereview.chromium.org/2905373002/diff/80001/chrome/browser/accessib... File chrome/browser/accessibility/accessibility_extension_api.cc (right): https://codereview.chromium.org/2905373002/diff/80001/chrome/browser/accessib... chrome/browser/accessibility/accessibility_extension_api.cc:173: EXTENSION_FUNCTION_VALIDATE(args_->GetSize() == 1); On 2017/05/31 01:33:32, Devlin wrote: > On 2017/05/31 00:58:43, elichtenberg wrote: > > On 2017/05/30 19:22:03, Devlin wrote: > > > Any reason to not use the generated extension parsing? > > > > What exactly do you mean by generated extension parsing? I'm not sure what to > > change. Are you saying there's some other way of getting the size of args or > > getting the list of key codes without using GetSize or GetList? > > We auto-generate typed structs and parsing code for extension APIs. The > generated files are, in this case, > <out_dir>/gen/chrome/common/api/accessibility_private.[h|cc]. You can use those > to parse the input params, doing something like: > std::unique_ptr<accessibility_private::SetSwitchAccessKeys::Params> params = > accessibility_private::SetSwitchAccessKeys::Params::Create(*args_); > EXTENSION_FUNCTION_VALIDATE(params); > > and then params contains a std::vector<int>, rather than needing to iterate over > the list value. This approach is generally less error-prone, since it doesn't > rely on argument indices, etc. Got it. Made the change. https://codereview.chromium.org/2905373002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/accessibility/switch_access_event_handler.h (right): https://codereview.chromium.org/2905373002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/switch_access_event_handler.h:28: void SetKeysToCapture(const std::unordered_set<int>& key_codes); On 2017/05/31 01:33:32, Devlin wrote: > On 2017/05/31 00:58:43, elichtenberg wrote: > > On 2017/05/30 19:22:03, Devlin wrote: > > > Any reason to use std::unordered_set over std::set? > > > > I believe unordered_sets are faster than sets. I think unordered_sets are > > hashsets, and sets are binary search trees. > > "faster" depends entirely on data set size. The algorithmic lookup complexity > is faster, but for small data sets (like this), a regular std::set is likely > faster, and certainly less memory (a vector would be better yet, but doesn't > have the de-duping/counting niceties of a set). Unless there's a very concrete > reason to use the unordered counterparts (i.e., known to be large data sets and > performance-sensitive code), prefer using the vanilla std::set. (The same goes > for std::map vs std::unordered_map.) > > I think brettw@ wrote up a nice post in chromium-dev that highlights this, along > with some more scientific measurements, awhile back. Fair point. Changed it to std::set. https://codereview.chromium.org/2905373002/diff/80001/chrome/common/extension... File chrome/common/extensions/api/accessibility_private.json (right): https://codereview.chromium.org/2905373002/diff/80001/chrome/common/extension... chrome/common/extensions/api/accessibility_private.json:112: "items": { "type": "integer" }, On 2017/05/31 01:33:32, Devlin wrote: > On 2017/05/31 00:58:44, elichtenberg wrote: > > On 2017/05/30 19:22:03, Devlin wrote: > > > note: we support min and max values, so if we know these should always be > > > between 0 and 'z', we could hard-code those values here with a comment and > it > > > would be checked js-side. > > > > Would it be better to just do this check in accessibility_extension_api.cc? > > Because that way, we can get the key codes from headers, rather than hard > coding > > the key codes. Also, we might expand to other keys later, so just having a > > minimum and maximum key code might not work later on. > > The advantage to having the minimum specified here is that it will be validated > synchronously in the renderer, so if someone tried to do > setSwitchAccessKeys([-1]), it would immediately throw an error. This is often > nicer than throwing an error asynchronously in runtime.lastError, which is what > happens when we return the result from the browser process. But, as you noted, > this means we can't import the exact values from the headers. Given this is a > private API, I'm fine either way. Good points. I ended up setting min and max values. https://codereview.chromium.org/2905373002/diff/100001/chrome/browser/accessi... File chrome/browser/accessibility/accessibility_extension_api.h (right): https://codereview.chromium.org/2905373002/diff/100001/chrome/browser/accessi... chrome/browser/accessibility/accessibility_extension_api.h:53: // API function that sets the keys to be captured by Switch Access. On 2017/05/31 20:41:15, Devlin wrote: > We should be able to have this declaration only on CrOS. Done. https://codereview.chromium.org/2905373002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/chromeos/switch_access/keyboard_handler.js (right): https://codereview.chromium.org/2905373002/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/switch_access/keyboard_handler.js:33: key => key.charCodeAt(0)); On 2017/05/31 20:41:15, Devlin wrote: > Note: arrow functions (last I checked) weren't available in all versions of iOS. > I'm assuming this code is only run on CrOS; is that accurate? Yep, this is just run on CrOS.
nice, lgtm https://codereview.chromium.org/2905373002/diff/120001/chrome/browser/accessi... File chrome/browser/accessibility/accessibility_extension_api.cc (right): https://codereview.chromium.org/2905373002/diff/120001/chrome/browser/accessi... chrome/browser/accessibility/accessibility_extension_api.cc:175: std::vector<int> key_codes_list = params->key_codes; No need to copy this; prefer const std::vector<int>& (or just use params->key_codes; it isn't used frequently and isn't that unreadable). https://codereview.chromium.org/2905373002/diff/120001/chrome/browser/accessi... chrome/browser/accessibility/accessibility_extension_api.cc:179: for (size_t i = 0; i < key_codes_list.size(); ++i) { prefer: for (auto key_code : key_codes_list) { EXTENSION_FUNCTION_VALIDATE(...); key_codes.insert(key_code); }
https://codereview.chromium.org/2905373002/diff/120001/chrome/browser/accessi... File chrome/browser/accessibility/accessibility_extension_api.cc (right): https://codereview.chromium.org/2905373002/diff/120001/chrome/browser/accessi... chrome/browser/accessibility/accessibility_extension_api.cc:175: std::vector<int> key_codes_list = params->key_codes; On 2017/06/02 02:02:36, Devlin wrote: > No need to copy this; prefer const std::vector<int>& (or just use > params->key_codes; it isn't used frequently and isn't that unreadable). Done. https://codereview.chromium.org/2905373002/diff/120001/chrome/browser/accessi... chrome/browser/accessibility/accessibility_extension_api.cc:179: for (size_t i = 0; i < key_codes_list.size(); ++i) { On 2017/06/02 02:02:36, Devlin wrote: > prefer: > for (auto key_code : key_codes_list) { > EXTENSION_FUNCTION_VALIDATE(...); > key_codes.insert(key_code); > } Done.
The CQ bit was checked by elichtenberg@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from dmazzoni@chromium.org, rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2905373002/#ps140001 (title: "Responded to comment")
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
elichtenberg@google.com changed reviewers: + mpearson@chromium.org
enums.xml lgtm
The CQ bit was checked by elichtenberg@google.com
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": 140001, "attempt_start_ts": 1496430547119170,
"parent_rev": "6a2b369b452ea9b4de023ab1d0bdf508beb702cf", "commit_rev":
"a108c8ab965c5fa52da3472f04cf507437e6ce37"}
Message was sent while issue was closed.
Description was changed from ========== Set keys for SwitchAccessEventHandler to capture using accessibiltyPrivate API BUG=593885 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Set keys for SwitchAccessEventHandler to capture using accessibiltyPrivate API BUG=593885 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2905373002 Cr-Commit-Position: refs/heads/master@{#476793} Committed: https://chromium.googlesource.com/chromium/src/+/a108c8ab965c5fa52da3472f04cf... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/a108c8ab965c5fa52da3472f04cf... |
