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

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

(Was broken by r253207.)

BUG=347450
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=259718

[mpichlinski, 2014-03-17 08:05:34]

Maybe it would be a good idea to hide state in AXViewState and expose interface
Has/Add/Set/Clear State which will do bit stuff, just as a compile time
protection for invalid use of AXState enum.

[dmazzoni, 2014-03-18 07:53:42]

On 2014/03/17 08:05:34, mpichlinski wrote:
> Maybe it would be a good idea to hide state in AXViewState and expose
interface
> Has/Add/Set/Clear State which will do bit stuff, just as a compile time
> protection for invalid use of AXState enum.

Good idea, how's this?

[mpichlinski, 2014-03-18 08:32:33]

https://codereview.chromium.org/196133012/diff/120001/ui/accessibility/ax_vie...
File ui/accessibility/ax_view_state.h (right):

https://codereview.chromium.org/196133012/diff/120001/ui/accessibility/ax_vie...
ui/accessibility/ax_view_state.h:30: void SetStateFlag(ui::AXState state);
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); }.

https://codereview.chromium.org/196133012/diff/120001/ui/accessibility/ax_vie...
ui/accessibility/ax_view_state.h:38: uint32 state;
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:00]

https://codereview.chromium.org/196133012/diff/120001/ui/accessibility/ax_vie...
File ui/accessibility/ax_view_state.h (right):

https://codereview.chromium.org/196133012/diff/120001/ui/accessibility/ax_vie...
ui/accessibility/ax_view_state.h:30: void SetStateFlag(ui::AXState state);
On 2014/03/18 08:32:34, mpichlinski wrote:
> 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); }.

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.

https://codereview.chromium.org/196133012/diff/120001/ui/accessibility/ax_vie...
ui/accessibility/ax_view_state.h:38: uint32 state;
On 2014/03/18 08:32:34, mpichlinski wrote:
> 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_;
> }

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:40]

https://codereview.chromium.org/196133012/diff/120001/ui/accessibility/ax_vie...
File ui/accessibility/ax_view_state.h (right):

https://codereview.chromium.org/196133012/diff/120001/ui/accessibility/ax_vie...
ui/accessibility/ax_view_state.h:30: void SetStateFlag(ui::AXState state);
On 2014/03/22 14:28:01, dmazzoni wrote:
> On 2014/03/18 08:32:34, mpichlinski wrote:
> > 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); }.
> 
> 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.

Done.

https://codereview.chromium.org/196133012/diff/120001/ui/accessibility/ax_vie...
ui/accessibility/ax_view_state.h:38: uint32 state;
On 2014/03/22 14:28:01, dmazzoni wrote:
> On 2014/03/18 08:32:34, mpichlinski wrote:
> > 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_;
> > }
> 
> 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.

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

[dmazzoni, 2014-03-24 23:08:00]

https://codereview.chromium.org/196133012/diff/120001/ui/accessibility/ax_vie...
File ui/accessibility/ax_view_state.h (right):

https://codereview.chromium.org/196133012/diff/120001/ui/accessibility/ax_vie...
ui/accessibility/ax_view_state.h:38: uint32 state;
On 2014/03/24 08:43:41, mpichlinski wrote:
> On 2014/03/22 14:28:01, dmazzoni wrote:
> > On 2014/03/18 08:32:34, mpichlinski wrote:
> > > 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_;
> > > }
> > 
> > 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.
> 
> Still hiding it in private section will prevent incorrect usage of state.

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.

[mpichlinski, 2014-03-25 07:45:59]

lgtm

[sky, 2014-03-26 14:29:12]

LGTM

https://codereview.chromium.org/196133012/diff/140001/chrome/browser/ui/views...
File chrome/browser/ui/views/download/download_item_view.cc (right):

https://codereview.chromium.org/196133012/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view.cc:498: if
(model_.IsDangerous()) {
nit: no {}

https://codereview.chromium.org/196133012/diff/140001/ui/accessibility/ax_vie...
File ui/accessibility/ax_view_state.h (right):

https://codereview.chromium.org/196133012/diff/140001/ui/accessibility/ax_vie...
ui/accessibility/ax_view_state.h:30: void SetStateFlag(ui::AXState state);
nit: I think AddStateFlag would be a better name here.

[dmazzoni, 2014-03-26 18:12:29]

https://codereview.chromium.org/196133012/diff/140001/chrome/browser/ui/views...
File chrome/browser/ui/views/download/download_item_view.cc (right):

https://codereview.chromium.org/196133012/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view.cc:498: if
(model_.IsDangerous()) {
On 2014/03/26 14:29:13, sky wrote:
> nit: no {}

Done.

https://codereview.chromium.org/196133012/diff/140001/ui/accessibility/ax_vie...
File ui/accessibility/ax_view_state.h (right):

https://codereview.chromium.org/196133012/diff/140001/ui/accessibility/ax_vie...
ui/accessibility/ax_view_state.h:30: void SetStateFlag(ui::AXState state);
On 2014/03/26 14:29:13, sky wrote:
> nit: I think AddStateFlag would be a better name here.

Done.

[dmazzoni, 2014-03-26 18:12:34]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2014-03-26 18:14:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/196133012/190001

[commit-bot: I haz the power, 2014-03-26 22:23:59]

Change committed as 259718