Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
145283003:
Switch AccessibilityMode to be a bitmap (Closed)
|
Created:
6 years, 11 months ago by aboxhall Modified:
6 years, 10 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, nkostylev+watch_chromium.org, plundblad+watch_chromium.org, aboxhall+watch_chromium.org, jam, yoshiki+watch_chromium.org, yuzo+watch_chromium.org, joi+watch-content_chromium.org, darin-cc_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org, dtseng+watch_chromium.org, chromium-apps-reviews_chromium.org, miu+watch_chromium.org, stevenjb+watch_chromium.org, dmazzoni+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src@enable Visibility:
Public. |
DescriptionSwitch AccessibilityMode to be a bitmap.
This will allow us, in a follow-up CL, to add a new Accessibility mode: AccessibilityModeExtension, which may co-exist with the previously existing modes.
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=250837
Patch Set 1 : #
Total comments: 16
Patch Set 2 : Address dtseng's comments #
Total comments: 36
Patch Set 3 : dtseng review comments #Patch Set 4 : Remove SetAccessibilityModeRenderer; addressing other minor comments #Patch Set 5 : Fix incorrect accessibility mode constant values #Patch Set 6 : Fix compile error in Android code #Patch Set 7 : Fix broken logic in android code #Patch Set 8 : rebase #
Total comments: 2
Patch Set 9 : Set accessibility mode by constant, not unsigned int #Patch Set 10 : Minor tweak to accessibility_mode_helper.cc #Patch Set 11 : Tweak AccessibilityModeHelperTest #
Total comments: 12
Patch Set 12 : Removed AccessibilityModeFlagRenderer, etc. #
Total comments: 11
Patch Set 13 : Address (most) review comments #Patch Set 14 : Modify includes in accessibility_mode_helper #Patch Set 15 : s/BASE_EXPORT/CONTENT_EXPORT/ in accessibility_mode_helper #Patch Set 16 : Fix compile/test errors #Patch Set 17 : Add necessary includes on Windows #Patch Set 18 : Switch IPC value to be the enum, rather than unsigned int #
Total comments: 2
Patch Set 19 : Rebase and remove debugging cruft #Patch Set 20 : Switch back to AccessibilityMode enums in accessibility_ui.cc #Messages
Total messages: 42 (0 generated)
Some initial comments. https://codereview.chromium.org/145283003/diff/60001/content/browser/accessib... File content/browser/accessibility/browser_accessibility_state_impl.cc (right): https://codereview.chromium.org/145283003/diff/60001/content/browser/accessib... content/browser/accessibility/browser_accessibility_state_impl.cc:96: SetAccessibilityMode(0); Add a name for 0 as well. https://codereview.chromium.org/145283003/diff/60001/content/browser/accessib... content/browser/accessibility/browser_accessibility_state_impl.cc:153: if ((accessibility_mode_ & AccessibilityModeFlagRenderer) == on) This looks wrong; don't you want: ((accessibility_mode_ & AccessibilityModeFlagRenderer) == AccessibilityModeFlagRenderer) https://codereview.chromium.org/145283003/diff/60001/content/browser/renderer... File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/145283003/diff/60001/content/browser/renderer... content/browser/renderer_host/render_view_host_impl.cc:2086: if (accessibility_mode() & AccessibilityModeFlagPlatform && view_ && nit: one expression per line https://codereview.chromium.org/145283003/diff/60001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/145283003/diff/60001/content/browser/renderer... content/browser/renderer_host/render_widget_host_impl.cc:2108: if ((accessibility_mode_ & AccessibilityModeFlagRenderer) == on) Ditto. https://codereview.chromium.org/145283003/diff/60001/content/common/view_mess... File content/common/view_message_enums.h (left): https://codereview.chromium.org/145283003/diff/60001/content/common/view_mess... content/common/view_message_enums.h:59: AccessibilityModeOff, Let's keep an off defined in this enum as 0. https://codereview.chromium.org/145283003/diff/60001/content/common/view_mess... File content/common/view_message_enums.h (right): https://codereview.chromium.org/145283003/diff/60001/content/common/view_mess... content/common/view_message_enums.h:57: // WebKit accessibility is on and accessibility information is sent from the Blink https://codereview.chromium.org/145283003/diff/60001/content/common/view_mess... content/common/view_message_enums.h:59: AccessibilityModeFlagRenderer = 1 << 0, Isn't this bit implied for all (except off)? Why do we need this state? https://codereview.chromium.org/145283003/diff/60001/content/common/view_mess... content/common/view_message_enums.h:61: // Platform APIs are called for all accessibility updates and events. How about: Accessibility updates are processed to create platform trees and events are passed to platform APIs in the browser.
https://codereview.chromium.org/145283003/diff/60001/content/browser/accessib... File content/browser/accessibility/browser_accessibility_state_impl.cc (right): https://codereview.chromium.org/145283003/diff/60001/content/browser/accessib... content/browser/accessibility/browser_accessibility_state_impl.cc:96: SetAccessibilityMode(0); On 2014/01/23 18:44:19, David Tseng wrote: > Add a name for 0 as well. Done. https://codereview.chromium.org/145283003/diff/60001/content/browser/accessib... content/browser/accessibility/browser_accessibility_state_impl.cc:153: if ((accessibility_mode_ & AccessibilityModeFlagRenderer) == on) On 2014/01/23 18:44:19, David Tseng wrote: > This looks wrong; don't you want: > ((accessibility_mode_ & AccessibilityModeFlagRenderer) == > AccessibilityModeFlagRenderer) I don't think so. I want to check not whether it's on, but whether it's the same as the 'on' argument, so this is almost right. However, @jyasskin points out that this is going to cast |on| to an int (which will be 1) and then compare it, so it's still technically wrong (even though it does happen to work since, as you pointed out, AccessibilityModeFlagRenderer == 1); I have now cast it to a bool, so it should be right. https://codereview.chromium.org/145283003/diff/60001/content/browser/renderer... File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/145283003/diff/60001/content/browser/renderer... content/browser/renderer_host/render_view_host_impl.cc:2086: if (accessibility_mode() & AccessibilityModeFlagPlatform && view_ && On 2014/01/23 18:44:19, David Tseng wrote: > nit: one expression per line clang-format formats it this way! :( Fixed. https://codereview.chromium.org/145283003/diff/60001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/145283003/diff/60001/content/browser/renderer... content/browser/renderer_host/render_widget_host_impl.cc:2108: if ((accessibility_mode_ & AccessibilityModeFlagRenderer) == on) On 2014/01/23 18:44:19, David Tseng wrote: > Ditto. Fixed as previously. https://codereview.chromium.org/145283003/diff/60001/content/common/view_mess... File content/common/view_message_enums.h (left): https://codereview.chromium.org/145283003/diff/60001/content/common/view_mess... content/common/view_message_enums.h:59: AccessibilityModeOff, On 2014/01/23 18:44:19, David Tseng wrote: > Let's keep an off defined in this enum as 0. I added a separate constants enum as discussed. https://codereview.chromium.org/145283003/diff/60001/content/common/view_mess... File content/common/view_message_enums.h (right): https://codereview.chromium.org/145283003/diff/60001/content/common/view_mess... content/common/view_message_enums.h:57: // WebKit accessibility is on and accessibility information is sent from the On 2014/01/23 18:44:19, David Tseng wrote: > Blink Done. https://codereview.chromium.org/145283003/diff/60001/content/common/view_mess... content/common/view_message_enums.h:59: AccessibilityModeFlagRenderer = 1 << 0, On 2014/01/23 18:44:19, David Tseng wrote: > Isn't this bit implied for all (except off)? Why do we need this state? I just realised I totally misunderstood this comment earlier :) I agree it's not completely necessary, however it makes things like toggling accessibility in https://codereview.chromium.org/145283003/diff/60001/content/browser/accessib... more straightforward. I thought about renaming it to something like AccessibilityModeFlagEnabled, but on reflection I think this name is more accurate. You can, after all, have an accessibility mode which doesn't have this flag on, but maintains the state for the other flags for when it gets turned back on. https://codereview.chromium.org/145283003/diff/60001/content/common/view_mess... content/common/view_message_enums.h:61: // Platform APIs are called for all accessibility updates and events. On 2014/01/23 18:44:19, David Tseng wrote: > How about: > Accessibility updates are processed to create platform trees and events are > passed to platform APIs in the browser. Done.
https://codereview.chromium.org/145283003/diff/180001/content/browser/accessi... File content/browser/accessibility/accessibility_ui.cc (right): https://codereview.chromium.org/145283003/diff/180001/content/browser/accessi... content/browser/accessibility/accessibility_ui.cc:198: unsigned int new_mode = (mode & AccessibilityModeFlagRenderer This whole block is equivalent to xor. https://codereview.chromium.org/145283003/diff/180001/content/browser/accessi... File content/browser/accessibility/browser_accessibility_state_impl.cc (right): https://codereview.chromium.org/145283003/diff/180001/content/browser/accessi... content/browser/accessibility/browser_accessibility_state_impl.cc:149: if (bool(accessibility_mode_ & AccessibilityModeFlagRenderer) == on) Got it; this still looks cryptic because of the cast; I'd prefer being more xplicit. i.e. bool renderer_set = accessibility_mode_ & AccessibilityModeFlagRenderer == AccessibilityModeFlagRenderer; if (renderer_set == on)... https://codereview.chromium.org/145283003/diff/180001/content/browser/accessi... content/browser/accessibility/browser_accessibility_state_impl.cc:152: if (on) Ditto; xor https://codereview.chromium.org/145283003/diff/180001/content/browser/accessi... content/browser/accessibility/browser_accessibility_state_impl.cc:156: // Iterate over all RenderWidgetHosts, even swapped out ones in case Why not just call through to SetAccessibilityMode here? This is dupped from there anyway. https://codereview.chromium.org/145283003/diff/180001/content/browser/accessi... File content/browser/accessibility/browser_accessibility_state_impl.h (right): https://codereview.chromium.org/145283003/diff/180001/content/browser/accessi... content/browser/accessibility/browser_accessibility_state_impl.h:52: void SetRendererAccessibilityMode(bool on); Is this method just for convenience? https://codereview.chromium.org/145283003/diff/180001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/145283003/diff/180001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:2107: if (bool(accessibility_mode_ & AccessibilityModeFlagRenderer) == on) Ditto; cast; and xor. https://codereview.chromium.org/145283003/diff/180001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.h (right): https://codereview.chromium.org/145283003/diff/180001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.h:403: void SetRendererAccessibilityMode(bool on); nit: some naming inconsistency; (Set AccessibilityModeRenderer vs RendererAccessibilityMode). I'd prefer the former, to stay consistent. https://codereview.chromium.org/145283003/diff/180001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/145283003/diff/180001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:777: void WebContentsImpl::SetRendererAccessibilityMode(bool on) { Will we ever need to set accessibility mode other than renderer from this layer? https://codereview.chromium.org/145283003/diff/180001/content/common/view_mes... File content/common/view_message_enums.h (left): https://codereview.chromium.org/145283003/diff/180001/content/common/view_mes... content/common/view_message_enums.h:61: // WebKit accessibility is on, but only limited information about Blink https://codereview.chromium.org/145283003/diff/180001/content/common/view_mes... content/common/view_message_enums.h:62: // editable text nodes is sent to the browser process. Useful for s/is/are https://codereview.chromium.org/145283003/diff/180001/content/common/view_mes... File content/common/view_message_enums.h (right): https://codereview.chromium.org/145283003/diff/180001/content/common/view_mes... content/common/view_message_enums.h:81: AccessibilityModeEditableTextOnly Why can't we just include these in the above enum? i.e. complete is just 1<<0 | 1<<1 https://codereview.chromium.org/145283003/diff/180001/content/public/browser/... File content/public/browser/web_contents.h (right): https://codereview.chromium.org/145283003/diff/180001/content/public/browser/... content/public/browser/web_contents.h:243: virtual void SetRendererAccessibilityMode(bool on) = 0; Why not expose this method inside of BrowserAccessibilityState? https://codereview.chromium.org/145283003/diff/180001/content/renderer/render... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/145283003/diff/180001/content/renderer/render... content/renderer/render_view_impl.cc:843: accessibility_mode_(0), Define/use the enum here. https://codereview.chromium.org/145283003/diff/180001/content/renderer/render... content/renderer/render_view_impl.cc:5298: if (!(accessibility_mode_ & AccessibilityModeFlagRenderer)) You might want to check this. What happens to !0b0010 for example? (in this case, it works because AccessibilityModeRenderer == 1). https://codereview.chromium.org/145283003/diff/180001/content/renderer/render... content/renderer/render_view_impl.cc:6117: if (accessibility_mode_ & AccessibilityModeFlagRenderer && Please define and use accessibility complete in the accessibility mode enum.
Got through some comments, need to go get a bus now. https://codereview.chromium.org/145283003/diff/180001/content/browser/accessi... File content/browser/accessibility/accessibility_ui.cc (right): https://codereview.chromium.org/145283003/diff/180001/content/browser/accessi... content/browser/accessibility/accessibility_ui.cc:198: unsigned int new_mode = (mode & AccessibilityModeFlagRenderer On 2014/01/27 22:16:06, David Tseng wrote: > This whole block is equivalent to xor. Facepalm... Fixed. https://codereview.chromium.org/145283003/diff/180001/content/browser/accessi... File content/browser/accessibility/browser_accessibility_state_impl.cc (right): https://codereview.chromium.org/145283003/diff/180001/content/browser/accessi... content/browser/accessibility/browser_accessibility_state_impl.cc:152: if (on) On 2014/01/27 22:16:06, David Tseng wrote: > Ditto; xor Done. https://codereview.chromium.org/145283003/diff/180001/content/browser/accessi... content/browser/accessibility/browser_accessibility_state_impl.cc:156: // Iterate over all RenderWidgetHosts, even swapped out ones in case On 2014/01/27 22:16:06, David Tseng wrote: > Why not just call through to SetAccessibilityMode here? This is dupped from > there anyway. Done, that's much better. https://codereview.chromium.org/145283003/diff/180001/content/browser/accessi... File content/browser/accessibility/browser_accessibility_state_impl.h (right): https://codereview.chromium.org/145283003/diff/180001/content/browser/accessi... content/browser/accessibility/browser_accessibility_state_impl.h:52: void SetRendererAccessibilityMode(bool on); On 2014/01/27 22:16:06, David Tseng wrote: > Is this method just for convenience? Yes. Want me to remove it? https://codereview.chromium.org/145283003/diff/180001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/145283003/diff/180001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:2107: if (bool(accessibility_mode_ & AccessibilityModeFlagRenderer) == on) On 2014/01/27 22:16:06, David Tseng wrote: > Ditto; cast; > and > xor. Done. https://codereview.chromium.org/145283003/diff/180001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.h (right): https://codereview.chromium.org/145283003/diff/180001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.h:403: void SetRendererAccessibilityMode(bool on); On 2014/01/27 22:16:06, David Tseng wrote: > nit: some naming inconsistency; (Set AccessibilityModeRenderer vs > RendererAccessibilityMode). I'd prefer the former, to stay consistent. Done. https://codereview.chromium.org/145283003/diff/180001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/145283003/diff/180001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:777: void WebContentsImpl::SetRendererAccessibilityMode(bool on) { On 2014/01/27 22:16:06, David Tseng wrote: > Will we ever need to set accessibility mode other than renderer from this layer? Extension? https://codereview.chromium.org/145283003/diff/180001/content/common/view_mes... File content/common/view_message_enums.h (left): https://codereview.chromium.org/145283003/diff/180001/content/common/view_mes... content/common/view_message_enums.h:61: // WebKit accessibility is on, but only limited information about On 2014/01/27 22:16:06, David Tseng wrote: > Blink Done. https://codereview.chromium.org/145283003/diff/180001/content/common/view_mes... content/common/view_message_enums.h:62: // editable text nodes is sent to the browser process. Useful for On 2014/01/27 22:16:06, David Tseng wrote: > s/is/are Nope :) limited information (about editable text nodes) is sent... https://codereview.chromium.org/145283003/diff/180001/content/common/view_mes... File content/common/view_message_enums.h (right): https://codereview.chromium.org/145283003/diff/180001/content/common/view_mes... content/common/view_message_enums.h:81: AccessibilityModeEditableTextOnly On 2014/01/27 22:16:06, David Tseng wrote: > Why can't we just include these in the above enum? i.e. complete is just 1<<0 | > 1<<1 Huh, the constant values didn't make it in. I'd rather not mix up the two concepts (flags and constants). https://codereview.chromium.org/145283003/diff/180001/content/public/browser/... File content/public/browser/web_contents.h (right): https://codereview.chromium.org/145283003/diff/180001/content/public/browser/... content/public/browser/web_contents.h:243: virtual void SetRendererAccessibilityMode(bool on) = 0; On 2014/01/27 22:16:06, David Tseng wrote: > Why not expose this method inside of BrowserAccessibilityState? I guess we could, although obviously that wouldn't replace this (since this relates to a single WebContents). Should I do that?
https://codereview.chromium.org/145283003/diff/180001/content/common/view_mes... File content/common/view_message_enums.h (left): https://codereview.chromium.org/145283003/diff/180001/content/common/view_mes... content/common/view_message_enums.h:62: // editable text nodes is sent to the browser process. Useful for On 2014/01/28 00:18:42, aboxhall wrote: > On 2014/01/27 22:16:06, David Tseng wrote: > > s/is/are > > Nope :) > limited information (about editable text nodes) is sent... Oops; yep. Never liked passive voice I guess. https://codereview.chromium.org/145283003/diff/180001/content/common/view_mes... File content/common/view_message_enums.h (right): https://codereview.chromium.org/145283003/diff/180001/content/common/view_mes... content/common/view_message_enums.h:81: AccessibilityModeEditableTextOnly On 2014/01/28 00:18:42, aboxhall wrote: > On 2014/01/27 22:16:06, David Tseng wrote: > > Why can't we just include these in the above enum? i.e. complete is just 1<<0 > | > > 1<<1 > > Huh, the constant values didn't make it in. > > I'd rather not mix up the two concepts (flags and constants). I'm not sure I see the distinction. They're all modes right? Some modes are just composites of other modes. There's also some inconsistent usage of the constants vs the flags; could be kind of confusing. https://codereview.chromium.org/145283003/diff/180001/content/public/browser/... File content/public/browser/web_contents.h (right): https://codereview.chromium.org/145283003/diff/180001/content/public/browser/... content/public/browser/web_contents.h:243: virtual void SetRendererAccessibilityMode(bool on) = 0; On 2014/01/28 00:18:42, aboxhall wrote: > On 2014/01/27 22:16:06, David Tseng wrote: > > Why not expose this method inside of BrowserAccessibilityState? > > I guess we could, although obviously that wouldn't replace this (since this > relates to a single WebContents). Should I do that? I think we'll need both. BrowserAccessibilityState flips on everything. I also think we'll need the generic version (SetAccessibilityMode). Might be cleaner to just have SetAccessibilityMode.
https://codereview.chromium.org/145283003/diff/180001/content/common/view_mes... File content/common/view_message_enums.h (left): https://codereview.chromium.org/145283003/diff/180001/content/common/view_mes... content/common/view_message_enums.h:62: // editable text nodes is sent to the browser process. Useful for On 2014/01/28 00:48:45, David Tseng wrote: > On 2014/01/28 00:18:42, aboxhall wrote: > > On 2014/01/27 22:16:06, David Tseng wrote: > > > s/is/are > > > > Nope :) > > limited information (about editable text nodes) is sent... > > Oops; yep. Never liked passive voice I guess. Yeah, it's not the most elegant. https://codereview.chromium.org/145283003/diff/180001/content/common/view_mes... File content/common/view_message_enums.h (right): https://codereview.chromium.org/145283003/diff/180001/content/common/view_mes... content/common/view_message_enums.h:81: AccessibilityModeEditableTextOnly On 2014/01/28 00:48:45, David Tseng wrote: > On 2014/01/28 00:18:42, aboxhall wrote: > > On 2014/01/27 22:16:06, David Tseng wrote: > > > Why can't we just include these in the above enum? i.e. complete is just > 1<<0 > > | > > > 1<<1 > > > > Huh, the constant values didn't make it in. > > > > I'd rather not mix up the two concepts (flags and constants). > > I'm not sure I see the distinction. They're all modes right? Some modes are just > composites of other modes. > There's also some inconsistent usage of the constants vs the flags; could be > kind of confusing. Well, I don't think of the flags as modes, rather as components of modes. We shouldn't ever be using a flag in isolation as a mode, except as a shortcut to setting a mode with only that one flag set (which we pretty much never want to do anyway). https://codereview.chromium.org/145283003/diff/180001/content/public/browser/... File content/public/browser/web_contents.h (right): https://codereview.chromium.org/145283003/diff/180001/content/public/browser/... content/public/browser/web_contents.h:243: virtual void SetRendererAccessibilityMode(bool on) = 0; On 2014/01/28 00:48:45, David Tseng wrote: > On 2014/01/28 00:18:42, aboxhall wrote: > > On 2014/01/27 22:16:06, David Tseng wrote: > > > Why not expose this method inside of BrowserAccessibilityState? > > > > I guess we could, although obviously that wouldn't replace this (since this > > relates to a single WebContents). Should I do that? > > I think we'll need both. BrowserAccessibilityState flips on everything. > I also think we'll need the generic version (SetAccessibilityMode). > Might be cleaner to just have SetAccessibilityMode. On second thoughts, I might take this out for now - we will need something like it eventually but it doesn't belong in this change. And I agree with you on just having SetAccessibilityMode. https://codereview.chromium.org/145283003/diff/180001/content/renderer/render... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/145283003/diff/180001/content/renderer/render... content/renderer/render_view_impl.cc:843: accessibility_mode_(0), On 2014/01/27 22:16:06, David Tseng wrote: > Define/use the enum here. Done. https://codereview.chromium.org/145283003/diff/180001/content/renderer/render... content/renderer/render_view_impl.cc:5298: if (!(accessibility_mode_ & AccessibilityModeFlagRenderer)) On 2014/01/27 22:16:06, David Tseng wrote: > You might want to check this. What happens to !0b0010 for example? (in this > case, it works because AccessibilityModeRenderer == 1). In C++, "the value zero (for integral, floating-point, and unscoped enumeration) and the null pointer and the null pointer-to-member values become false. All other values become true." http://en.cppreference.com/w/cpp/language/implicit_cast#Boolean_conversions https://github.com/cplusplus/draft/blob/19ae5173cfd1614766da27504a9beeeffd3ee... ! forces an implicit conversion to bool for its operand: https://github.com/cplusplus/draft/blob/19ae5173cfd1614766da27504a9beeeffd3ee... (github links are for working draft standard, since that's the only freely available one. Hopefully this behaviour hasn't changed recently.) https://codereview.chromium.org/145283003/diff/180001/content/renderer/render... content/renderer/render_view_impl.cc:6117: if (accessibility_mode_ & AccessibilityModeFlagRenderer && On 2014/01/27 22:16:06, David Tseng wrote: > Please define and use accessibility complete in the accessibility mode enum. Done. Note that we explicitly don't want to check for == AccessibilityModeComplete since that will break when we add the extra flag for AccessibilityModeFlagExtension in a future CL and can potentially have {AccessibilityModeFlagRenderer & AccessibilityModeFlagPlatform & AccessibilityModeFlagExtension} here.
+jyasskin@ for C++ style/sanity.
dtseng, jyasskin: ping?
https://codereview.chromium.org/145283003/diff/180001/content/common/view_mes... File content/common/view_message_enums.h (right): https://codereview.chromium.org/145283003/diff/180001/content/common/view_mes... content/common/view_message_enums.h:81: AccessibilityModeEditableTextOnly On 2014/01/28 17:48:26, aboxhall wrote: > On 2014/01/28 00:48:45, David Tseng wrote: > > On 2014/01/28 00:18:42, aboxhall wrote: > > > On 2014/01/27 22:16:06, David Tseng wrote: > > > > Why can't we just include these in the above enum? i.e. complete is just > > 1<<0 > > > | > > > > 1<<1 > > > > > > Huh, the constant values didn't make it in. > > > > > > I'd rather not mix up the two concepts (flags and constants). > > > > I'm not sure I see the distinction. They're all modes right? Some modes are > just > > composites of other modes. > > There's also some inconsistent usage of the constants vs the flags; could be > > kind of confusing. > > Well, I don't think of the flags as modes, rather as components of modes. We > shouldn't ever be using a flag in isolation as a mode, except as a shortcut to > setting a mode with only that one flag set (which we pretty much never want to > do anyway). Ok; so, it sounds like we should just keep the integer bit mask hidden from all public API's right? So, SetAccessibilityMode should just take one of the constant values.
On 2014/01/29 17:59:53, David Tseng wrote: > https://codereview.chromium.org/145283003/diff/180001/content/common/view_mes... > File content/common/view_message_enums.h (right): > > https://codereview.chromium.org/145283003/diff/180001/content/common/view_mes... > content/common/view_message_enums.h:81: AccessibilityModeEditableTextOnly > On 2014/01/28 17:48:26, aboxhall wrote: > > On 2014/01/28 00:48:45, David Tseng wrote: > > > On 2014/01/28 00:18:42, aboxhall wrote: > > > > On 2014/01/27 22:16:06, David Tseng wrote: > > > > > Why can't we just include these in the above enum? i.e. complete is just > > > 1<<0 > > > > | > > > > > 1<<1 > > > > > > > > Huh, the constant values didn't make it in. > > > > > > > > I'd rather not mix up the two concepts (flags and constants). > > > > > > I'm not sure I see the distinction. They're all modes right? Some modes are > > just > > > composites of other modes. > > > There's also some inconsistent usage of the constants vs the flags; could be > > > kind of confusing. > > > > Well, I don't think of the flags as modes, rather as components of modes. We > > shouldn't ever be using a flag in isolation as a mode, except as a shortcut to > > setting a mode with only that one flag set (which we pretty much never want to > > do anyway). > > Ok; so, it sounds like we should just keep the integer bit mask hidden from all > public API's right? So, SetAccessibilityMode should just take one of the > constant values. I don't think so: as discussed previously, the mode may be some superposition of constants (e.g. AccessibilityModeComplete | AccessibilityModeExtension), so we definitely need to allow for that. As I see it, we have two options: - Leave it the way it is, and trust API users to do the right thing - Switch the public API to allow setting a single flag at a time
On 2014/01/30 00:43:34, aboxhall wrote: > On 2014/01/29 17:59:53, David Tseng wrote: > > > https://codereview.chromium.org/145283003/diff/180001/content/common/view_mes... > > File content/common/view_message_enums.h (right): > > > > > https://codereview.chromium.org/145283003/diff/180001/content/common/view_mes... > > content/common/view_message_enums.h:81: AccessibilityModeEditableTextOnly > > On 2014/01/28 17:48:26, aboxhall wrote: > > > On 2014/01/28 00:48:45, David Tseng wrote: > > > > On 2014/01/28 00:18:42, aboxhall wrote: > > > > > On 2014/01/27 22:16:06, David Tseng wrote: > > > > > > Why can't we just include these in the above enum? i.e. complete is > just > > > > 1<<0 > > > > > | > > > > > > 1<<1 > > > > > > > > > > Huh, the constant values didn't make it in. > > > > > > > > > > I'd rather not mix up the two concepts (flags and constants). > > > > > > > > I'm not sure I see the distinction. They're all modes right? Some modes > are > > > just > > > > composites of other modes. > > > > There's also some inconsistent usage of the constants vs the flags; could > be > > > > kind of confusing. > > > > > > Well, I don't think of the flags as modes, rather as components of modes. We > > > shouldn't ever be using a flag in isolation as a mode, except as a shortcut > to > > > setting a mode with only that one flag set (which we pretty much never want > to > > > do anyway). > > > > Ok; so, it sounds like we should just keep the integer bit mask hidden from > all > > public API's right? So, SetAccessibilityMode should just take one of the > > constant values. > > I don't think so: as discussed previously, the mode may be some superposition of > constants (e.g. AccessibilityModeComplete | AccessibilityModeExtension), so we > definitely need to allow for that. As I see it, we have two options: > - Leave it the way it is, and trust API users to do the right thing > - Switch the public API to allow setting a single flag at a time Or, have an API which allows only additive setting of modes, e.g. SetAccessibilityMode(AccessibilityModeExtension) will actually set accessibility_mode_ = accessibility_mode_ | AccessibilityModeExtension.
On 2014/01/30 00:46:10, aboxhall wrote: > On 2014/01/30 00:43:34, aboxhall wrote: > > On 2014/01/29 17:59:53, David Tseng wrote: > > > > > > https://codereview.chromium.org/145283003/diff/180001/content/common/view_mes... > > > File content/common/view_message_enums.h (right): > > > > > > > > > https://codereview.chromium.org/145283003/diff/180001/content/common/view_mes... > > > content/common/view_message_enums.h:81: AccessibilityModeEditableTextOnly > > > On 2014/01/28 17:48:26, aboxhall wrote: > > > > On 2014/01/28 00:48:45, David Tseng wrote: > > > > > On 2014/01/28 00:18:42, aboxhall wrote: > > > > > > On 2014/01/27 22:16:06, David Tseng wrote: > > > > > > > Why can't we just include these in the above enum? i.e. complete is > > just > > > > > 1<<0 > > > > > > | > > > > > > > 1<<1 > > > > > > > > > > > > Huh, the constant values didn't make it in. > > > > > > > > > > > > I'd rather not mix up the two concepts (flags and constants). > > > > > > > > > > I'm not sure I see the distinction. They're all modes right? Some modes > > are > > > > just > > > > > composites of other modes. > > > > > There's also some inconsistent usage of the constants vs the flags; > could > > be > > > > > kind of confusing. > > > > > > > > Well, I don't think of the flags as modes, rather as components of modes. > We > > > > shouldn't ever be using a flag in isolation as a mode, except as a > shortcut > > to > > > > setting a mode with only that one flag set (which we pretty much never > want > > to > > > > do anyway). > > > > > > Ok; so, it sounds like we should just keep the integer bit mask hidden from > > all > > > public API's right? So, SetAccessibilityMode should just take one of the > > > constant values. > > > > I don't think so: as discussed previously, the mode may be some superposition > of > > constants (e.g. AccessibilityModeComplete | AccessibilityModeExtension), so we > > definitely need to allow for that. As I see it, we have two options: > > - Leave it the way it is, and trust API users to do the right thing > > - Switch the public API to allow setting a single flag at a time > > Or, have an API which allows only additive setting of modes, e.g. > SetAccessibilityMode(AccessibilityModeExtension) will actually set > accessibility_mode_ = accessibility_mode_ | AccessibilityModeExtension. (dmazzoni: if you're lurking and feel like chiming in, go ahead :)
On 2014/01/30 01:09:40, aboxhall wrote: > On 2014/01/30 00:46:10, aboxhall wrote: > > On 2014/01/30 00:43:34, aboxhall wrote: > > > On 2014/01/29 17:59:53, David Tseng wrote: > > > > > > > > > > https://codereview.chromium.org/145283003/diff/180001/content/common/view_mes... > > > > File content/common/view_message_enums.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/145283003/diff/180001/content/common/view_mes... > > > > content/common/view_message_enums.h:81: AccessibilityModeEditableTextOnly > > > > On 2014/01/28 17:48:26, aboxhall wrote: > > > > > On 2014/01/28 00:48:45, David Tseng wrote: > > > > > > On 2014/01/28 00:18:42, aboxhall wrote: > > > > > > > On 2014/01/27 22:16:06, David Tseng wrote: > > > > > > > > Why can't we just include these in the above enum? i.e. complete > is > > > just > > > > > > 1<<0 > > > > > > > | > > > > > > > > 1<<1 > > > > > > > > > > > > > > Huh, the constant values didn't make it in. > > > > > > > > > > > > > > I'd rather not mix up the two concepts (flags and constants). > > > > > > > > > > > > I'm not sure I see the distinction. They're all modes right? Some > modes > > > are > > > > > just > > > > > > composites of other modes. > > > > > > There's also some inconsistent usage of the constants vs the flags; > > could > > > be > > > > > > kind of confusing. > > > > > > > > > > Well, I don't think of the flags as modes, rather as components of > modes. > > We > > > > > shouldn't ever be using a flag in isolation as a mode, except as a > > shortcut > > > to > > > > > setting a mode with only that one flag set (which we pretty much never > > want > > > to > > > > > do anyway). > > > > > > > > Ok; so, it sounds like we should just keep the integer bit mask hidden > from > > > all > > > > public API's right? So, SetAccessibilityMode should just take one of the > > > > constant values. > > > > > > I don't think so: as discussed previously, the mode may be some > superposition > > of > > > constants (e.g. AccessibilityModeComplete | AccessibilityModeExtension), so > we > > > definitely need to allow for that. Well, we could always define a constant for it AccessibilityModePlatformAndExtension. As I see it, we have two options: > > > - Leave it the way it is, and trust API users to do the right thing > > > - Switch the public API to allow setting a single flag at a time > > > > Or, have an API which allows only additive setting of modes, e.g. > > SetAccessibilityMode(AccessibilityModeExtension) will actually set > > accessibility_mode_ = accessibility_mode_ | AccessibilityModeExtension. > > (dmazzoni: if you're lurking and feel like chiming in, go ahead :) Additive seems to make sense, except we'd have trouble flipping modes off. Could take a bool to specify if the caller wants it set or unset.
So I think that's the "setting a single flag at a time" option? On Wed, Jan 29, 2014 at 7:51 PM, <dtseng@chromium.org> wrote: > On 2014/01/30 01:09:40, aboxhall wrote: > >> On 2014/01/30 00:46:10, aboxhall wrote: >> > On 2014/01/30 00:43:34, aboxhall wrote: >> > > On 2014/01/29 17:59:53, David Tseng wrote: >> > > > >> > > >> > >> > > https://codereview.chromium.org/145283003/diff/180001/ > content/common/view_message_enums.h > >> > > > File content/common/view_message_enums.h (right): >> > > > >> > > > >> > > >> > >> > > https://codereview.chromium.org/145283003/diff/180001/ > content/common/view_message_enums.h#newcode81 > >> > > > content/common/view_message_enums.h:81: >> > AccessibilityModeEditableTextOnly > >> > > > On 2014/01/28 17:48:26, aboxhall wrote: >> > > > > On 2014/01/28 00:48:45, David Tseng wrote: >> > > > > > On 2014/01/28 00:18:42, aboxhall wrote: >> > > > > > > On 2014/01/27 22:16:06, David Tseng wrote: >> > > > > > > > Why can't we just include these in the above enum? i.e. >> complete >> is >> > > just >> > > > > > 1<<0 >> > > > > > > | >> > > > > > > > 1<<1 >> > > > > > > >> > > > > > > Huh, the constant values didn't make it in. >> > > > > > > >> > > > > > > I'd rather not mix up the two concepts (flags and constants). >> > > > > > >> > > > > > I'm not sure I see the distinction. They're all modes right? >> Some >> modes >> > > are >> > > > > just >> > > > > > composites of other modes. >> > > > > > There's also some inconsistent usage of the constants vs the >> flags; >> > could >> > > be >> > > > > > kind of confusing. >> > > > > >> > > > > Well, I don't think of the flags as modes, rather as components of >> modes. >> > We >> > > > > shouldn't ever be using a flag in isolation as a mode, except as a >> > shortcut >> > > to >> > > > > setting a mode with only that one flag set (which we pretty much >> never >> > want >> > > to >> > > > > do anyway). >> > > > >> > > > Ok; so, it sounds like we should just keep the integer bit mask >> hidden >> from >> > > all >> > > > public API's right? So, SetAccessibilityMode should just take one >> of the >> > > > constant values. >> > > >> > > I don't think so: as discussed previously, the mode may be some >> superposition >> > of >> > > constants (e.g. AccessibilityModeComplete | >> AccessibilityModeExtension), >> > so > >> we >> > > definitely need to allow for that. >> > > Well, we could always define a constant for it > AccessibilityModePlatformAndExtension. > > > As I see it, we have two options: > >> > > - Leave it the way it is, and trust API users to do the right thing >> > > - Switch the public API to allow setting a single flag at a time >> > >> > Or, have an API which allows only additive setting of modes, e.g. >> > SetAccessibilityMode(AccessibilityModeExtension) will actually set >> > accessibility_mode_ = accessibility_mode_ | AccessibilityModeExtension. >> > > (dmazzoni: if you're lurking and feel like chiming in, go ahead :) >> > > Additive seems to make sense, except we'd have trouble flipping modes off. > Could > take a bool to specify if the caller wants it set or unset. > > > > https://codereview.chromium.org/145283003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I would vote for setting a constant at a time (constants being different than flags). this is a little tricky because if you have both text only and complete constants set, and unset complete, we need to make sure the renderer flag is still set; however, once all but one of text only and complete are set, unsetting would unset renderer. On Wed, Jan 29, 2014 at 7:55 PM, Alice Boxhall <aboxhall@google.com> wrote: So I think that's the "setting a single flag at a time" option? On Wed, Jan 29, 2014 at 7:51 PM, <dtseng@chromium.org> wrote: On 2014/01/30 01:09:40, aboxhall wrote: On 2014/01/30 00:46:10, aboxhall wrote: > On 2014/01/30 00:43:34, aboxhall wrote: > > On 2014/01/29 17:59:53, David Tseng wrote: > > > > > > https://codereview.chromium.org/145283003/diff/180001/content/common/view_mes... > > > File content/common/view_message_enums.h (right): > > > > > > > > > https://codereview.chromium.org/145283003/diff/180001/content/common/view_mes... > > > content/common/view_message_enums.h:81: AccessibilityModeEditableTextOnly > > > On 2014/01/28 17:48:26, aboxhall wrote: > > > > On 2014/01/28 00:48:45, David Tseng wrote: > > > > > On 2014/01/28 00:18:42, aboxhall wrote: > > > > > > On 2014/01/27 22:16:06, David Tseng wrote: > > > > > > > Why can't we just include these in the above enum? i.e. complete is > > just > > > > > 1<<0 > > > > > > | > > > > > > > 1<<1 > > > > > > > > > > > > Huh, the constant values didn't make it in. > > > > > > > > > > > > I'd rather not mix up the two concepts (flags and constants). > > > > > > > > > > I'm not sure I see the distinction. They're all modes right? Some modes > > are > > > > just > > > > > composites of other modes. > > > > > There's also some inconsistent usage of the constants vs the flags; > could > > be > > > > > kind of confusing. > > > > > > > > Well, I don't think of the flags as modes, rather as components of modes. > We > > > > shouldn't ever be using a flag in isolation as a mode, except as a > shortcut > > to > > > > setting a mode with only that one flag set (which we pretty much never > want > > to > > > > do anyway). > > > > > > Ok; so, it sounds like we should just keep the integer bit mask hidden from > > all > > > public API's right? So, SetAccessibilityMode should just take one of the > > > constant values. > > > > I don't think so: as discussed previously, the mode may be some superposition > of > > constants (e.g. AccessibilityModeComplete | AccessibilityModeExtension), so we > > definitely need to allow for that. Well, we could always define a constant for it AccessibilityModePlatformAndExtension. As I see it, we have two options: > > - Leave it the way it is, and trust API users to do the right thing > > - Switch the public API to allow setting a single flag at a time > > Or, have an API which allows only additive setting of modes, e.g. > SetAccessibilityMode(AccessibilityModeExtension) will actually set > accessibility_mode_ = accessibility_mode_ | AccessibilityModeExtension. (dmazzoni: if you're lurking and feel like chiming in, go ahead :) Additive seems to make sense, except we'd have trouble flipping modes off. Could take a bool to specify if the caller wants it set or unset. https://codereview.chromium.org/145283003/ https://codereview.chromium.org/145283003/diff/610001/content/common/view_mes... File content/common/view_message_enums.h (right): https://codereview.chromium.org/145283003/diff/610001/content/common/view_mes... content/common/view_message_enums.h:83: AccessibilityModeComplete | AccessibilityModeFlagEditableTextOnly This is a little confusing because text only isn't really a superset of complete (rather a subset). In the code, they're mutually exclusive; you can only be in text only mode or complete (not both). I also think we shouldn't mix constants and flags when defining constants
On 2014/01/31 18:38:22, David Tseng wrote: > I would vote for setting a constant at a time (constants being different than > flags). this is a little tricky because if you have both text only and complete > constants set, and unset complete, we need to make sure the renderer flag is > still set; however, once all but one of text only and complete are set, > unsetting would unset renderer. Alright, I'll give that a go, although I think I'll make an interface more like: AddAccessibilityMode(AccessibilityMode mode) RemoveAccessibilityMode(AccessibilityMode mode) I would explicitly like to avoid the situation where we essentially have to create a constant for every possible combination of accessibility mode flags and then write code like if (accessibility_mode_ == AccessibilityModeComplete) SetAccessibilityMode(AccessibilityModeCompleteAndExtension); else SetAccessibilityMode(AccessibilityModeExtension); -- this is precisely what I'm trying to avoid by creating the bitmasks in the first place. > Additive seems to make sense, except we'd have trouble flipping modes off. Could > take a bool to specify if the caller wants it set or unset. I'll make two separate methods instead. > https://codereview.chromium.org/145283003/ > > https://codereview.chromium.org/145283003/diff/610001/content/common/view_mes... > File content/common/view_message_enums.h (right): > > https://codereview.chromium.org/145283003/diff/610001/content/common/view_mes... > content/common/view_message_enums.h:83: AccessibilityModeComplete | > AccessibilityModeFlagEditableTextOnly > This is a little confusing because text only isn't really a superset of complete > (rather a subset). This was a misunderstanding on my part: I didn't realise AccessibilityModeEditableTextOnly didn't actually turn on Blink accessibility. In that case, this should probably be AccessibilityModeEditableTextOnly = AccessibilityModeFlagEditableTextOnly, since it doesn't depend on any of the other flags. Note that there may be a situation where we want both AccessibilityModeFlagEditableTextOnly and AccessibilityModeFlagRenderer and AccessibilityModeFlagExtension, if we want the platform to use EditableTextOnly mode but an extension requests access to automation. > In the code, they're mutually exclusive; you can only be in text only mode or > complete (not both). This doesn't imply otherwise; only that (I thought) you needed both renderer and platform accessibility on (i.e. the flags set in AccessibilityModeComplete), and to specify that only editable text/focus was to be included. > I also think we shouldn't mix constants and flags when defining constants Sure, I just wrote it that way for conciseness. I'll update it as per my comment above.
On Fri, Jan 31, 2014 at 6:18 PM, <aboxhall@chromium.org> wrote: > On 2014/01/31 18:38:22, David Tseng wrote: > >> I would vote for setting a constant at a time (constants being different >> than >> flags). this is a little tricky because if you have both text only and >> > complete > >> constants set, and unset complete, we need to make sure the renderer flag >> is >> still set; however, once all but one of text only and complete are set, >> unsetting would unset renderer. >> > > Alright, I'll give that a go, although I think I'll make an interface more > like: > > AddAccessibilityMode(AccessibilityMode mode) > RemoveAccessibilityMode(AccessibilityMode mode) > Agreed; I think this is good. > > I would explicitly like to avoid the situation where we essentially have to > create a constant for every possible combination of accessibility mode > flags and > then write code like > if (accessibility_mode_ == AccessibilityModeComplete) > SetAccessibilityMode(AccessibilityModeCompleteAndExtension); > else > SetAccessibilityMode(AccessibilityModeExtension); > > Agreed as well. I was just pointing out that the state machine for transitions is slightly more complicated with the renderer flag when removing accessibility modes. -- this is precisely what I'm trying to avoid by creating the bitmasks in > the > first place. > > > Additive seems to make sense, except we'd have trouble flipping modes off. >> > Could > >> take a bool to specify if the caller wants it set or unset. >> > > I'll make two separate methods instead. > > > https://codereview.chromium.org/145283003/ >> > > > https://codereview.chromium.org/145283003/diff/610001/ > content/common/view_message_enums.h > >> File content/common/view_message_enums.h (right): >> > > > https://codereview.chromium.org/145283003/diff/610001/ > content/common/view_message_enums.h#newcode83 > >> content/common/view_message_enums.h:83: AccessibilityModeComplete | >> AccessibilityModeFlagEditableTextOnly >> This is a little confusing because text only isn't really a superset of >> > complete > >> (rather a subset). >> > > This was a misunderstanding on my part: I didn't realise > AccessibilityModeEditableTextOnly didn't actually turn on Blink > accessibility. > In that case, this should probably be > AccessibilityModeEditableTextOnly = AccessibilityModeFlagEditableTextOnly, > since > it doesn't depend on any of the other flags. > > Note that there may be a situation where we want both > AccessibilityModeFlagEditableTextOnly and AccessibilityModeFlagRenderer > and > AccessibilityModeFlagExtension, if we want the platform to use > EditableTextOnly > mode but an extension requests access to automation. > > > In the code, they're mutually exclusive; you can only be in text only >> mode or >> complete (not both). >> > > This doesn't imply otherwise; only that (I thought) you needed both > renderer and > platform accessibility on (i.e. the flags set in > AccessibilityModeComplete), and > to specify that only editable text/focus was to be included. > > > I also think we shouldn't mix constants and flags when defining constants >> > > Sure, I just wrote it that way for conciseness. I'll update it as per my > comment > above. > > > https://codereview.chromium.org/145283003/ > > To unsubscribe from this group and stop receiving emails from it, send an > email to chromium-reviews+unsubscribe@chromium.org. > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
dtseng: I implemented Add/RemoveAccessibilityMode on BrowserAccessibilityStateImpl and RenderWidgetHostImpl; PTAL. https://codereview.chromium.org/145283003/diff/610001/content/common/view_mes... File content/common/view_message_enums.h (right): https://codereview.chromium.org/145283003/diff/610001/content/common/view_mes... content/common/view_message_enums.h:83: AccessibilityModeComplete | AccessibilityModeFlagEditableTextOnly On 2014/01/31 18:38:23, David Tseng wrote: > This is a little confusing because text only isn't really a superset of complete > (rather a subset). > > In the code, they're mutually exclusive; you can only be in text only mode or > complete (not both). > > I also think we shouldn't mix constants and flags when defining constants > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Done.
dtseng: I implemented Add/RemoveAccessibilityMode on BrowserAccessibilityStateImpl and RenderWidgetHostImpl; PTAL. https://codereview.chromium.org/145283003/diff/610001/content/common/view_mes... File content/common/view_message_enums.h (right): https://codereview.chromium.org/145283003/diff/610001/content/common/view_mes... content/common/view_message_enums.h:83: AccessibilityModeComplete | AccessibilityModeFlagEditableTextOnly On 2014/01/31 18:38:23, David Tseng wrote: > This is a little confusing because text only isn't really a superset of complete > (rather a subset). > > In the code, they're mutually exclusive; you can only be in text only mode or > complete (not both). > > I also think we shouldn't mix constants and flags when defining constants > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Done.
https://codereview.chromium.org/145283003/diff/1100001/content/browser/access... File content/browser/accessibility/accessibility_mode_helper.cc (right): https://codereview.chromium.org/145283003/diff/1100001/content/browser/access... content/browser/accessibility/accessibility_mode_helper.cc:11: return to | mode_to_add; Do we want to enforce which modes can be set at the same time? https://codereview.chromium.org/145283003/diff/1100001/content/browser/access... File content/browser/accessibility/accessibility_mode_helper_unittest.cc (right): https://codereview.chromium.org/145283003/diff/1100001/content/browser/access... content/browser/accessibility/accessibility_mode_helper_unittest.cc:33: AccessibilityModeComplete | AccessibilityModeEditableTextOnly, This isn't ever allowed I think. https://codereview.chromium.org/145283003/diff/1100001/content/browser/access... File content/browser/accessibility/browser_accessibility_state_impl.cc (right): https://codereview.chromium.org/145283003/diff/1100001/content/browser/access... content/browser/accessibility/browser_accessibility_state_impl.cc:135: content::AddAccessibilityMode(accessibility_mode_, mode); By setting text only, then complete, I would end up adding both text only and complete, which is a functional change from the current code. (see my comments about enforcement on add). https://codereview.chromium.org/145283003/diff/1100001/content/common/view_me... File content/common/view_message_enums.h (right): https://codereview.chromium.org/145283003/diff/1100001/content/common/view_me... content/common/view_message_enums.h:83: AccessibilityModeEditableTextOnly = AccessibilityModeFlagEditableTextOnly, I think you're expecting to also have the renderer bit set. https://codereview.chromium.org/145283003/diff/1100001/content/renderer/rende... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/145283003/diff/1100001/content/renderer/rende... content/renderer/render_view_impl.cc:5299: return; What about editable text only? This leads me to believe that the renderer bit is implied in all modes. https://codereview.chromium.org/145283003/diff/1100001/content/renderer/rende... File content/renderer/render_view_impl.h (right): https://codereview.chromium.org/145283003/diff/1100001/content/renderer/rende... content/renderer/render_view_impl.h:1377: // Only valid if |accessibility_mode_| has the AccessibilityModeFlagRenderer What about text only?
The EditableTextOnly issue is really confusing. What I tried to do when I initially created this CL was split the original constants (AccessibilityModeOff, AccessibilityModeComplete, AccessibilityModeEditableTextOnly) and the proposed new mode (which can co-exist with other modes) AccessibilityModeExtension into the parts which are common across modes and the parts which are unique to a particular mode. This is how I arrived at the flags: - Renderer, controlling whether Blink accessibility is enabled. This is required (or is it? RendererAccessibilityFocusOnly doesn't seem to turn on Blink accessibility) for any other accessibility flags to do anything. - Platform, controlling whether native platform APIs are called. This is explicitly not required for Extension accessibility mode, but may co-exist with it if someone is using external AT _and_ an extension requesting chrome.automation. - EditableTextOnly, controlling whether Platform accessibility is called with the full accessibility tree, or only focusable editable text information. - (in future) Extension, controlling whether accessibility tree information is sent over to any extension processes. This should be turned on whenever any extension requests chrome.automation and, in theory, turned off once all extensions which have requested it have stopped. So then we can re-implement the original modes as a combination of those flags: - AccessibilityModeOff is obviously 0000 - AccessibilityModeComplete is (I think) 0011 (Platform and Renderer) - AccessibilityModeEditableTextOnly is 0111 (EditableTextOnly, Platform and Renderer) - (in future) AccessibilityModeExtension is 1001 (Extension and Renderer) Note that there is _no_ flag which specifies "Complete"; it's implied by EditableTextOnly being _off_. Then you could have combinations: 1011 would imply Blink accessibility is on, full tree information is sent to the platform APIs _and_ to the extension 1111 would imply Blink accessibility is on, editable text/focus information is sent to the platform APIs and full accessibility tree information is sent to extensions. There are also, obviously, a bunch of combinations that don't make sense. This does however present a problem for adding/removing modes by constant, since AccessibilityModeEditableTextOnly is equivalent to AccessibilityModeComplete _plus_ the EditableTextOnly flag, so removing that constant will turn accessibility off completely (unless the Extension flag is set, which will cause the Renderer flag to stay on). So I'm leaning back towards allowing users to set flags individually, perhaps with some logic to ensure that they're not setting a mode which makes no sense (e.g. a DCHECK). What do you think is a sensible design here? https://codereview.chromium.org/145283003/diff/1100001/content/browser/access... File content/browser/accessibility/accessibility_mode_helper.cc (right): https://codereview.chromium.org/145283003/diff/1100001/content/browser/access... content/browser/accessibility/accessibility_mode_helper.cc:11: return to | mode_to_add; On 2014/02/04 20:25:51, David Tseng wrote: > Do we want to enforce which modes can be set at the same time? I think editable text only overrides the other modes. Note that AccessibilityModeComplete is just a shorthand for AccessibilityModeFlagRenderer | AccessibilityModeFlagPlatform. Complete is the default, i.e. the 0 value for AccessibilityModeFlagEditableTextOnly. https://codereview.chromium.org/145283003/diff/1100001/content/browser/access... File content/browser/accessibility/accessibility_mode_helper_unittest.cc (right): https://codereview.chromium.org/145283003/diff/1100001/content/browser/access... content/browser/accessibility/accessibility_mode_helper_unittest.cc:33: AccessibilityModeComplete | AccessibilityModeEditableTextOnly, On 2014/02/04 20:25:51, David Tseng wrote: > This isn't ever allowed I think. See comment elsewhere. https://codereview.chromium.org/145283003/diff/1100001/content/browser/access... File content/browser/accessibility/browser_accessibility_state_impl.cc (right): https://codereview.chromium.org/145283003/diff/1100001/content/browser/access... content/browser/accessibility/browser_accessibility_state_impl.cc:135: content::AddAccessibilityMode(accessibility_mode_, mode); On 2014/02/04 20:25:51, David Tseng wrote: > By setting text only, then complete, I would end up adding both text only and > complete, which is a functional change from the current code. (see my comments > about enforcement on add). Complete + text only should be considered to be text only, IMO. https://codereview.chromium.org/145283003/diff/1100001/content/common/view_me... File content/common/view_message_enums.h (right): https://codereview.chromium.org/145283003/diff/1100001/content/common/view_me... content/common/view_message_enums.h:83: AccessibilityModeEditableTextOnly = AccessibilityModeFlagEditableTextOnly, On 2014/02/04 20:25:51, David Tseng wrote: > I think you're expecting to also have the renderer bit set. I still can't make up my mind on this one. https://codereview.chromium.org/145283003/diff/1100001/content/renderer/rende... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/145283003/diff/1100001/content/renderer/rende... content/renderer/render_view_impl.cc:5299: return; On 2014/02/04 20:25:51, David Tseng wrote: > What about editable text only? This leads me to believe that the renderer bit is > implied in all modes. Ok, I think it'll be simpler if I go back to what I originally had. I'll do that and then we can see how that looks. https://codereview.chromium.org/145283003/diff/1100001/content/renderer/rende... File content/renderer/render_view_impl.h (right): https://codereview.chromium.org/145283003/diff/1100001/content/renderer/rende... content/renderer/render_view_impl.h:1377: // Only valid if |accessibility_mode_| has the AccessibilityModeFlagRenderer On 2014/02/04 20:25:51, David Tseng wrote: > What about text only? Depends whether text only includes renderer.
Ping?
On Wed, Feb 5, 2014 at 8:02 AM, <aboxhall@chromium.org> wrote: The EditableTextOnly issue is really confusing. What I tried to do when I initially created this CL was split the original constants (AccessibilityModeOff, AccessibilityModeComplete, AccessibilityModeEditableTextOnly) and the proposed new mode (which can co-exist with other modes) AccessibilityModeExtension into the parts which are common across modes and the parts which are unique to a particular mode. This is how I arrived at the flags: - Renderer, controlling whether Blink accessibility is enabled. This is required (or is it? RendererAccessibilityFocusOnly doesn't seem to turn on Blink accessibility) for any other accessibility flags to do anything. Yep; it fakes the renderer side, so I guess it's a toss up whether it has the renderer bit set. It just depends on whether you want the renderer bit to control the instantiation of the renderer side (or for it to represent turning on Blink accessibility). In the current cl, it looks like we're going for the former. - Platform, controlling whether native platform APIs are called. This is explicitly not required for Extension accessibility mode, but may co-exist with it if someone is using external AT _and_ an extension requesting chrome.automation. Agreed (and this isn't an issue in this cl). - EditableTextOnly, controlling whether Platform accessibility is called with the full accessibility tree, or only focusable editable text information. No real problems here, just the previously mentioned issues in the cl with the renderer bit. - (in future) Extension, controlling whether accessibility tree information is sent over to any extension processes. This should be turned on whenever any extension requests chrome.automation and, in theory, turned off once all extensions which have requested it have stopped. Agreed. So then we can re-implement the original modes as a combination of those flags: - AccessibilityModeOff is obviously 0000 - AccessibilityModeComplete is (I think) 0011 (Platform and Renderer) - AccessibilityModeEditableTextOnly is 0111 (EditableTextOnly, Platform and Renderer) Yeah. I mean, I really don't oppose any of these break downs. Just as long as we have a clear idea of what each bit controls. Platform/editable text only are very clear on the renderer side (they control the subclass of RendererAccessibility used). On the browser side, it's would be clear that extension controls whether we pass on updates to all extensions via automation. The platform flag controls whether we create BrowserAccessibilityManager. - (in future) AccessibilityModeExtension is 1001 (Extension and Renderer) Note that there is _no_ flag which specifies "Complete"; it's implied by EditableTextOnly being _off_. So, again, since we list renderer in every bitmap above, why do we need it? Then you could have combinations: 1011 would imply Blink accessibility is on, full tree information is sent to the platform APIs _and_ to the extension 1111 would imply Blink accessibility is on, editable text/focus information is sent to the platform APIs and full accessibility tree information is sent to extensions. At the moment, only RendererAccessibilityComplete does enables Blink accessibility. RendererAccessibilityFocusOnly fakes the renderer side tree, but do we want that distinction surfaced in the flags? If we ever implemented editable text only with Blink accessibility turned on, then I guess that could be important, but it seems unnecessary at the moment. There are also, obviously, a bunch of combinations that don't make sense. This does however present a problem for adding/removing modes by constant, since AccessibilityModeEditableTextOnly is equivalent to AccessibilityModeComplete _plus_ the EditableTextOnly flag, so removing that constant will turn accessibility off completely (unless the Extension flag is set, which will cause the Renderer flag to stay on). Yeah, this is why I initially thought a flat enum list would be easier to deal with since we explicitly list all combinations allowed. The current scheme is slightly complicated because of the renderer bit, so let's just get rid of it. So I'm leaning back towards allowing users to set flags individually, perhaps with some logic to ensure that they're not setting a mode which makes no sense (e.g. a DCHECK). What do you think is a sensible design here? https://codereview.chromium.org/145283003/diff/1100001/content/browser/access... File content/browser/accessibility/accessibility_mode_helper.cc (right): https://codereview.chromium.org/145283003/diff/1100001/content/browser/access... content/browser/accessibility/accessibility_mode_helper.cc:11: return to | mode_to_add; On 2014/02/04 20:25:51, David Tseng wrote: Do we want to enforce which modes can be set at the same time? I think editable text only overrides the other modes. Note that AccessibilityModeComplete is just a shorthand for AccessibilityModeFlagRenderer | AccessibilityModeFlagPlatform. Complete is the default, i.e. the 0 value for AccessibilityModeFlagEditableTextOnly. https://codereview.chromium.org/145283003/diff/1100001/content/browser/access... File content/browser/accessibility/accessibility_mode_helper_unittest.cc (right): https://codereview.chromium.org/145283003/diff/1100001/content/browser/access... content/browser/accessibility/accessibility_mode_helper_unittest.cc:33: AccessibilityModeComplete | AccessibilityModeEditableTextOnly, On 2014/02/04 20:25:51, David Tseng wrote: This isn't ever allowed I think. See comment elsewhere. https://codereview.chromium.org/145283003/diff/1100001/content/browser/access... File content/browser/accessibility/browser_accessibility_state_impl.cc (right): https://codereview.chromium.org/145283003/diff/1100001/content/browser/access... content/browser/accessibility/browser_accessibility_state_impl.cc:135: content::AddAccessibilityMode(accessibility_mode_, mode); On 2014/02/04 20:25:51, David Tseng wrote: By setting text only, then complete, I would end up adding both text only and complete, which is a functional change from the current code. (see my comments about enforcement on add). Complete + text only should be considered to be text only, IMO. https://codereview.chromium.org/145283003/diff/1100001/content/common/view_me... File content/common/view_message_enums.h (right): https://codereview.chromium.org/145283003/diff/1100001/content/common/view_me... content/common/view_message_enums.h:83: AccessibilityModeEditableTextOnly = AccessibilityModeFlagEditableTextOnly, On 2014/02/04 20:25:51, David Tseng wrote: I think you're expecting to also have the renderer bit set. I still can't make up my mind on this one. https://codereview.chromium.org/145283003/diff/1100001/content/renderer/rende... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/145283003/diff/1100001/content/renderer/rende... content/renderer/render_view_impl.cc:5299: return; On 2014/02/04 20:25:51, David Tseng wrote: What about editable text only? This leads me to believe that the renderer bit is implied in all modes. Ok, I think it'll be simpler if I go back to what I originally had. I'll do that and then we can see how that looks. https://codereview.chromium.org/145283003/diff/1100001/content/renderer/rende... File content/renderer/render_view_impl.h (right): https://codereview.chromium.org/145283003/diff/1100001/content/renderer/rende... content/renderer/render_view_impl.h:1377: // Only valid if |accessibility_mode_| has the AccessibilityModeFlagRenderer On 2014/02/04 20:25:51, David Tseng wrote: What about text only? Depends whether text only includes renderer. https://codereview.chromium.org/145283003/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Ok, I removed the renderer flag and switch the EditableTextOnly flag to invert its meaning (i.e. renamed to PlatformFullTree). I also made some changes to avoid turning off accessibility where the platform requires it. PTAL?
https://codereview.chromium.org/145283003/diff/1470001/content/renderer/rende... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/145283003/diff/1470001/content/renderer/rende... content/renderer/render_view_impl.cc:5181: if (accessibility_mode_ & AccessibilityModeFlagPlatformFullTree) Open question: what do we do here if we need extension accessibility _and_ FocusOnly accessibility for the platform?
Some more comments. https://codereview.chromium.org/145283003/diff/1470001/content/browser/access... File content/browser/accessibility/accessibility_mode_helper.cc (right): https://codereview.chromium.org/145283003/diff/1470001/content/browser/access... content/browser/accessibility/accessibility_mode_helper.cc:6: #include "content/browser/accessibility/accessibility_mode_helper.h" I think this goes above all other includes. https://codereview.chromium.org/145283003/diff/1470001/content/browser/access... content/browser/accessibility/accessibility_mode_helper.cc:23: !CommandLine::ForCurrentProcess()->HasSwitch( Seems unexpected to have the addition of this flag as a side effect of removal. Perhaps just prevent the removal of the editable text flag on Windows 8? https://codereview.chromium.org/145283003/diff/1470001/content/browser/access... content/browser/accessibility/accessibility_mode_helper.cc:31: AddAccessibilityModeTo(new_mode, AccessibilityModeComplete); Also, seems odd to have this as a side effect of removal. https://codereview.chromium.org/145283003/diff/1470001/content/browser/access... File content/browser/accessibility/browser_accessibility_state_impl.cc (right): https://codereview.chromium.org/145283003/diff/1470001/content/browser/access... content/browser/accessibility/browser_accessibility_state_impl.cc:91: switches::kForceRendererAccessibility)) { We should remove one of switches::kDisableRendererAccessibility)) and switches::kForceRendererAccessibility)) and replace with for-accessibility-to=on/off https://codereview.chromium.org/145283003/diff/1470001/content/browser/access... content/browser/accessibility/browser_accessibility_state_impl.cc:166: content::RemoveAccessibilityModeFrom(accessibility_mode_, mode); Seems like the command line switches related to forcing renderer accessibility should be handled at this layer. I think the checks should: checks to see if mode complete is being removed, if so, return without doing anything.
https://codereview.chromium.org/145283003/diff/1470001/content/browser/access... File content/browser/accessibility/accessibility_mode_helper.cc (right): https://codereview.chromium.org/145283003/diff/1470001/content/browser/access... content/browser/accessibility/accessibility_mode_helper.cc:6: #include "content/browser/accessibility/accessibility_mode_helper.h" On 2014/02/06 19:13:44, David Tseng wrote: > I think this goes above all other includes. Oops, done. https://codereview.chromium.org/145283003/diff/1470001/content/browser/access... content/browser/accessibility/accessibility_mode_helper.cc:23: !CommandLine::ForCurrentProcess()->HasSwitch( On 2014/02/06 19:13:44, David Tseng wrote: > Seems unexpected to have the addition of this flag as a side effect of removal. > Perhaps just prevent the removal of the editable text flag on Windows 8? Done, although note that this code is functionally the same (since it prevents you from ever getting into a state in which you would actually be adding it as opposed to keeping it). https://codereview.chromium.org/145283003/diff/1470001/content/browser/access... content/browser/accessibility/accessibility_mode_helper.cc:31: AddAccessibilityModeTo(new_mode, AccessibilityModeComplete); On 2014/02/06 19:13:44, David Tseng wrote: > Also, seems odd to have this as a side effect of removal. Ok, removed. https://codereview.chromium.org/145283003/diff/1470001/content/browser/access... File content/browser/accessibility/browser_accessibility_state_impl.cc (right): https://codereview.chromium.org/145283003/diff/1470001/content/browser/access... content/browser/accessibility/browser_accessibility_state_impl.cc:91: switches::kForceRendererAccessibility)) { On 2014/02/06 19:13:44, David Tseng wrote: > We should remove one of > switches::kDisableRendererAccessibility)) > and > switches::kForceRendererAccessibility)) > > and replace with > for-accessibility-to=on/off Possibly; do you want me to do it in this CL? https://codereview.chromium.org/145283003/diff/1470001/content/browser/access... content/browser/accessibility/browser_accessibility_state_impl.cc:166: content::RemoveAccessibilityModeFrom(accessibility_mode_, mode); On 2014/02/06 19:13:44, David Tseng wrote: > Seems like the command line switches related to forcing renderer accessibility > should be handled at this layer. I think the checks should: > checks to see if mode complete is being removed, if so, return without doing > anything. Done.
lgtm
I wonder whether you should introduce a new class instead of using a raw integer? A class could have the methods that are now global statics. Also, the ctor would ensure that it is always initialized with sane values. We're also loosening the security of the IPC field where we before checked that it was one of the valid values. Is it possible to keep at least some sanity checking in place?
On 2014/02/10 12:40:51, jochen wrote: > I wonder whether you should introduce a new class instead of using a raw > integer? A class could have the methods that are now global statics. Also, the > ctor would ensure that it is always initialized with sane values. > > We're also loosening the security of the IPC field where we before checked that > it was one of the valid values. Is it possible to keep at least some sanity > checking in place? Ok, I (finally) listened to dtseng's suggestion and switched it to be the enum throughout. I thought it was probably overkill to create a class, but I can do that if you still think it's a good idea.
lgtm please have someone from the security team look at the ipc bits https://codereview.chromium.org/145283003/diff/1870001/content/public/browser... File content/public/browser/browser_thread.h (right): https://codereview.chromium.org/145283003/diff/1870001/content/public/browser... content/public/browser/browser_thread.h:62: UI, // 0 unrelated change?
https://codereview.chromium.org/145283003/diff/1870001/content/public/browser... File content/public/browser/browser_thread.h (right): https://codereview.chromium.org/145283003/diff/1870001/content/public/browser... content/public/browser/browser_thread.h:62: UI, // 0 On 2014/02/12 09:56:07, jochen wrote: > unrelated change? Debugging cruft. Will remove.
On 2014/02/12 18:16:56, aboxhall wrote: > https://codereview.chromium.org/145283003/diff/1870001/content/public/browser... > File content/public/browser/browser_thread.h (right): > > https://codereview.chromium.org/145283003/diff/1870001/content/public/browser... > content/public/browser/browser_thread.h:62: UI, // 0 > On 2014/02/12 09:56:07, jochen wrote: > > unrelated change? > > Debugging cruft. Will remove. view_message_enums.h LGTM
The CQ bit was checked by aboxhall@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/aboxhall@chromium.org/145283003/1980001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by aboxhall@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/aboxhall@chromium.org/145283003/1980001
Message was sent while issue was closed.
Change committed as 250837 |
