Fix incorrect use of AXState caused when refactoring two enums into one.

ui/accessibility/ax_view_state.h

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef UI_ACCESSIBILITY_AX_VIEW_STATE_H_
 #define UI_ACCESSIBILITY_AX_VIEW_STATE_H_
 
 #include "base/basictypes.h"
 #include "base/callback.h"
 #include "base/strings/string16.h"
 #include "ui/accessibility/ax_enums.h"
 #include "ui/accessibility/ax_export.h"
 
 namespace ui {
 
 ////////////////////////////////////////////////////////////////////////////////
 //
 // AXViewState
 //
 //   A cross-platform struct for storing the core accessibility information
 //   that should be provided about any UI view to assistive technology (AT).
 //
 ////////////////////////////////////////////////////////////////////////////////
 struct AX_EXPORT AXViewState {
  public:
   AXViewState();
   ~AXViewState();
 
+  // Convenience functions to set or check bits in |state|.
+  void SetStateFlag(ui::AXState state);

[mpichlinski, 2014/03/18 08:32:34]

This only adds flags, its for previous usage of |= operator, so it should be
AddStateFlag. In some places flags are overridden to one value by = operator, so
there should be another method SetStateFlag, which will do { state = (1 <<
flag); }.

[dmazzoni, 2014/03/22 14:28:01]

In typical programming and computer science jargon, when you have a bitmask
containing flags, you "set" a flag, or "set" a bit - you don't add it.

There should never be a place in the code where the whole flag bitmask is
overridden. The way AXViewState is used is that when the native views
accessibility system needs accessibility information about a view, it calls
View::GetAccessibleState and passes it an empty AXViewState. Each view
implements it and occasionally sets flags. Often a view class will call its
inherited
GetAccessibleState or delegate, but currently we don't ever need to clear/unset
a bit.

I didn't see any code that set "state" to a value because it wanted to
explicitly clear other flags, I just saw it where it set it to a particular
value because it assumed it was previously 0 so there'd be no difference.

[mpichlinski, 2014/03/24 08:43:41]

Done.

+  bool HasStateFlag(ui::AXState state) const;
+
   // The view's role, like button or list box.
   AXRole role;
 
   // The view's state, a bitmask containing fields such as checked
   // (for a checkbox) and protected (for a password text box).
   uint32 state;

[mpichlinski, 2014/03/18 08:32:34]

Hide it in private section, to prevent incorrect access. When access to state is
needed pass the whole AxViewState object, to keep state interface, or you may
create small template enum based state container (maybe something like this is
already in the code):
template<typename T>
class EnumBasedBitmask {
 public:
  EnumBasedBitmask() : mask_(0) { }
  void SetFlag(const T flag) { mask_ = (1 << flag); }
  void AddFlag(const T flag) { mask_ |= (1 << flag); }
  void ClearMask() { mask_ = 0; }
  bool HasFlag(const T flag) const { return !!(mask_ & (1 << flag)); }
 private:
  uint32 mask_;
}

[dmazzoni, 2014/03/22 14:28:01]

I don't think this complexity is needed. We don't currently have any use cases
for anything other than setting flags (according to the standard usage of "set"
a bit, which means |=) and checking them.

[mpichlinski, 2014/03/24 08:43:41]

Still hiding it in private section will prevent incorrect usage of state.

[dmazzoni, 2014/03/24 23:08:01]

Fair enough, done.

Now it should probably be a class instead of a struct. Maybe I'll use that as an
excuse to warn on other types of incorrect usage in
a future change.

 
   // The view's name / label.
   base::string16 name;
 
   // The view's value, for example the text content.
   base::string16 value;
 
   // The name of the default action if the user clicks on this view.
   base::string16 default_action;
 
@@ skipping 19 matching lines @@
   //
   // This callback is only valid for the lifetime of the view, and should
   // be a safe no-op if the view is deleted. Typically, accessible views
   // should use a WeakPtr when binding the callback.
   base::Callback<void(const base::string16&)> set_value_callback;
 };
 
 }  // namespace ui
 
 #endif  // UI_ACCESSIBILITY_AX_VIEW_STATE_H_