Scope and clean up uses of AccessibilityMode.

Scope and clean up uses of AccessibilityMode.

This CL removes the typedef |AccessibilityMode| and instead uses a class
which allows us to stop using the all UPPER CASE prefix
ACCESSIBILITY_MODE_FLAG_.

R=dmazzoni

BUG=None
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2694413006
Cr-Commit-Position: refs/heads/master@{#456614}
Committed: https://chromium.googlesource.com/chromium/src/+/cd3dad73b453f8a89731bc2a2cd72f3bd5def991

[dougt, 2017-02-16 18:44:36]

Description was changed from

==========
Scope and clean up uses of AccessibilityMode.

This CL removes the typedef |AccessibilityMode| and instead uses the enum class
which allows us to stop using the all UPPER CASE prefix
ACCESSIBILITY_MODE_FLAG_. Also this CL allows us to better validate the enum
value passed during IPC.

R=dmazzoni

BUG=None
==========

to

==========
Scope and clean up uses of AccessibilityMode.

This CL removes the typedef |AccessibilityMode| and instead uses the enum class
which allows us to stop using the all UPPER CASE prefix
ACCESSIBILITY_MODE_FLAG_. Also this CL allows us to better validate the enum
value passed during IPC.

R=dmazzoni

BUG=None
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[dougt, 2017-02-16 18:44:51]

The CQ bit was checked by dougt@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-16 18:45:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-16 19:13:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-16 19:13:34]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[dougt, 2017-02-16 21:40:39]

The CQ bit was checked by dougt@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-16 21:51:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dougt, 2017-02-16 23:58:40]

dmazzoni, ptal.

[commit-bot: I haz the power, 2017-02-17 00:05:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-17 00:05:22]

Dry run: This issue passed the CQ dry run.

[dmazzoni, 2017-02-17 00:21:50]

dmazzoni@chromium.org changed reviewers:
+ aboxhall@chromium.org, danakj@chromium.org

[dmazzoni, 2017-02-17 00:21:51]

lgtm

I like it! Much cleaner, the operator overloading is a nice trick.

I'll add Alice since she was the one who wrote this in the first
place and reviewed the last change I made, and then Dana,
who I know is up on C++11.

After that we'll need a couple of OWNERS rubber-stamps,
we'll do that last.

https://codereview.chromium.org/2694413006/diff/20001/content/common/accessib...
File content/common/accessibility_mode_enums.h (right):

https://codereview.chromium.org/2694413006/diff/20001/content/common/accessib...
content/common/accessibility_mode_enums.h:56: return
static_cast<AccessibilityMode>(int(a) | int(b));
nit: use static_cast<int>(a), etc.

https://codereview.chromium.org/2694413006/diff/20001/content/common/accessib...
content/common/accessibility_mode_enums.h:65: return
static_cast<AccessibilityMode>(int(a) ^ int(b));
same

https://codereview.chromium.org/2694413006/diff/20001/content/renderer/render...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/2694413006/diff/20001/content/renderer/render...
content/renderer/render_view_impl.cc:2507: (accessibility_mode &
AccessibilityMode::COMPLETE) ==
I think you can simplify this now, just (accessibility_mode ==
AccessibilityMode::COMPLETE) should work.

Probably a few other similar places.

[dmazzoni, 2017-02-17 00:23:54]

Dana, could you look at just this one file and let us know
if you have any concerns about the enum class and operator
overloading:

content/common/accessibility_mode_enums.h

Alice: thought you should be in the loop, too.

[danakj, 2017-02-17 00:30:39]

https://codereview.chromium.org/2694413006/diff/20001/content/common/accessib...
File content/common/accessibility_mode_enums.h (right):

https://codereview.chromium.org/2694413006/diff/20001/content/common/accessib...
content/common/accessibility_mode_enums.h:8: enum AccessibilityMode : int {
If this were an enum, then I think what you want is

enum class AccessibilityMode {
  ..
};

You don't need to specify the underlying type, this may only make it larger than
it needs to be. Unless you really require it to be an int which it doesn't seem
like?

However, this isn't so much an enum (a list of things of which you choose one)
as it is a scoped set of constants. Instead of an enum class you could do this

struct AccessibilityMode {
  enum { NATIVE_APIS = 1 << 0 };
  enum { WEB_CONTENTS = 1 << 1 };
  enum { INLINE_TEXT_BOXES = 1 << 2 };
  ..etc..
};

Then |&^ work on the constants still, and you get scoping, and you can't write a
switch statement on the enum for instance, which doesn't make sense here. And at
that point you could turn NATIVE_APIS into kNativeApis and so on, as these are
clearly constants now instead of an enum.

[danakj, 2017-02-17 00:31:27]

(I'm going to be OOO tomorrow so do what you think best with my advice but don't
block on me)

[aboxhall, 2017-02-17 00:53:34]

On 2017/02/17 00:30:39, danakj wrote:
>
https://codereview.chromium.org/2694413006/diff/20001/content/common/accessib...
> File content/common/accessibility_mode_enums.h (right):
> 
>
https://codereview.chromium.org/2694413006/diff/20001/content/common/accessib...
> content/common/accessibility_mode_enums.h:8: enum AccessibilityMode : int {
> If this were an enum, then I think what you want is
> 
> enum class AccessibilityMode {
>   ..
> };
> 
> You don't need to specify the underlying type, this may only make it larger
than
> it needs to be. Unless you really require it to be an int which it doesn't
seem
> like?
> 
> However, this isn't so much an enum (a list of things of which you choose one)
> as it is a scoped set of constants. Instead of an enum class you could do this
> 
> struct AccessibilityMode {
>   enum { NATIVE_APIS = 1 << 0 };
>   enum { WEB_CONTENTS = 1 << 1 };
>   enum { INLINE_TEXT_BOXES = 1 << 2 };
>   ..etc..
> };
> 
> Then |&^ work on the constants still, and you get scoping, and you can't write
a
> switch statement on the enum for instance, which doesn't make sense here. And
at
> that point you could turn NATIVE_APIS into kNativeApis and so on, as these are
> clearly constants now instead of an enum.

That sounds cool!

Current way also seems ok to me. This is a great cleanup.

[dougt, 2017-02-17 17:46:48]

https://codereview.chromium.org/2694413006/diff/20001/content/common/accessib...
File content/common/accessibility_mode_enums.h (right):

https://codereview.chromium.org/2694413006/diff/20001/content/common/accessib...
content/common/accessibility_mode_enums.h:8: enum AccessibilityMode : int {
I do see your point about "not being a list of things choose one".  I do think
it's a pretty common pattern to scope values, bit flags, ect., in an enum such
as I am doing here.

One of the values of staying with enums, or enum class as you suggest, is that
it is more strongly typed than just an int. For example, we have to be explicit
when assigning or passing values into a function that take a AccessibilityMode. 
In this CL, there are two places where this came up.  One, where we were passing
|0| into a method which we needed to change to be more explict (yes, good), and
another where we can set up IPC so that it does "some" validation on the input
before posting to the other process.

I would rather not just regress to using #define's or global constant values.

WDYT?

[danakj, 2017-02-17 17:50:58]

https://codereview.chromium.org/2694413006/diff/20001/content/common/accessib...
File content/common/accessibility_mode_enums.h (right):

https://codereview.chromium.org/2694413006/diff/20001/content/common/accessib...
content/common/accessibility_mode_enums.h:8: enum AccessibilityMode : int {
On 2017/02/17 17:46:48, dougt wrote:
> I do see your point about "not being a list of things choose one".  I do think
> it's a pretty common pattern to scope values, bit flags, ect., in an enum such
> as I am doing here.
> 
> One of the values of staying with enums, or enum class as you suggest, is that
> it is more strongly typed than just an int. For example, we have to be
explicit
> when assigning or passing values into a function that take a
AccessibilityMode. 
> In this CL, there are two places where this came up.  One, where we were
passing
> |0| into a method which we needed to change to be more explict (yes, good),
and
> another where we can set up IPC so that it does "some" validation on the input
> before posting to the other process.
> 
> I would rather not just regress to using #define's or global constant values.
> 
> WDYT?

That sounds fair, LGTM. I do suggest "enum class" tho and drop the ":int".

[dougt, 2017-02-17 22:30:12]

brettw@

Can you take a look at the changes switch over to the enum AccessibilityMode.
There are all mostly mechanical.

danakj@ wdyt?

https://codereview.chromium.org/2694413006/diff/20001/content/common/accessib...
File content/common/accessibility_mode_enums.h (right):

https://codereview.chromium.org/2694413006/diff/20001/content/common/accessib...
content/common/accessibility_mode_enums.h:8: enum AccessibilityMode : int {
On 2017/02/17 17:50:57, danakj wrote:
> On 2017/02/17 17:46:48, dougt wrote:
> > I do see your point about "not being a list of things choose one".  I do
think
> > it's a pretty common pattern to scope values, bit flags, ect., in an enum
such
> > as I am doing here.
> > 
> > One of the values of staying with enums, or enum class as you suggest, is
that
> > it is more strongly typed than just an int. For example, we have to be
> explicit
> > when assigning or passing values into a function that take a
> AccessibilityMode. 
> > In this CL, there are two places where this came up.  One, where we were
> passing
> > |0| into a method which we needed to change to be more explict (yes, good),
> and
> > another where we can set up IPC so that it does "some" validation on the
input
> > before posting to the other process.
> > 
> > I would rather not just regress to using #define's or global constant
values.
> > 
> > WDYT?
> 
> That sounds fair, LGTM. I do suggest "enum class" tho and drop the ":int".


Moving to enum class would mean many/all of our unitary operands woudldn't work
as is. For example:

 if (accessibility_mode & AccessibilityMode::NATIVE_APIS) {
    ....

Generates a invalid operands to binary expression compile time error. since the
result of the & is a class... and not something that can be used in an if ().

Maybe in the future we would convert this enum directly to a class and have
methods to check for various states.  But I am not sure the value of that yet.

https://codereview.chromium.org/2694413006/diff/20001/content/common/accessib...
content/common/accessibility_mode_enums.h:56: return
static_cast<AccessibilityMode>(int(a) | int(b));
On 2017/02/17 00:21:50, dmazzoni wrote:
> nit: use static_cast<int>(a), etc.

Done.

https://codereview.chromium.org/2694413006/diff/20001/content/common/accessib...
content/common/accessibility_mode_enums.h:65: return
static_cast<AccessibilityMode>(int(a) ^ int(b));
On 2017/02/17 00:21:50, dmazzoni wrote:
> same

Done.

[dougt, 2017-02-22 15:59:49]

After discussing with Dana and thinking about it a bit, I do not think enum
class is the right thing here. I think having values of the enum that are
composites of other values isn't really great.  What we can do instead is just
have a class called AccessibilityMode that holds the current mode value, and
offers some accessors (like has(), set(), toggle()) to make reasoning about the
value easier.

[danakj, 2017-02-22 16:02:46]

SGTM

On Wed, Feb 22, 2017 at 10:59 AM, <dougt@chromium.org> wrote:

> After discussing with Dana and thinking about it a bit, I do not think enum
> class is the right thing here. I think having values of the enum that are
> composites of other values isn't really great. What we can do instead is
> just
> have a class called AccessibilityMode that holds the current mode value,
> and
> offers some accessors (like has(), set(), toggle()) to make reasoning
> about the
> value easier.
>
> https://codereview.chromium.org/2694413006/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dougt, 2017-02-22 21:08:24]

The CQ bit was checked by dougt@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-22 21:08:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dougt, 2017-02-22 21:14:19]

danakj, can you look at this approach?

[danakj, 2017-02-22 21:27:56]

https://codereview.chromium.org/2694413006/diff/60001/content/common/accessib...
File content/common/accessibility_mode.h (right):

https://codereview.chromium.org/2694413006/diff/60001/content/common/accessib...
content/common/accessibility_mode.h:1: // Copyright 2014 The Chromium Authors.
All rights reserved.
2017

https://codereview.chromium.org/2694413006/diff/60001/content/common/accessib...
content/common/accessibility_mode.h:4: #ifndef
CONTENT_COMMON_ACCESSIBILITY_MODE_ENUMS_H_
white space around the #ifndef#define

https://codereview.chromium.org/2694413006/diff/60001/content/common/accessib...
content/common/accessibility_mode.h:7: // in sync with these two enums
These aren't enums anymore maybe clarify this comment

https://codereview.chromium.org/2694413006/diff/60001/content/common/accessib...
content/common/accessibility_mode.h:13: const static int OFF = 0;
constexpr instead of const.

kOff, kNativeApis, etc.

https://codereview.chromium.org/2694413006/diff/60001/content/common/accessib...
content/common/accessibility_mode.h:59: AccessibilityMode(int flag = OFF) :
flags_(flag) {}
explicit

https://codereview.chromium.org/2694413006/diff/60001/content/common/accessib...
content/common/accessibility_mode.h:62: int get() const { return flags_; }
do you need a get()? it breaks the encapsulation

https://codereview.chromium.org/2694413006/diff/60001/content/common/accessib...
content/common/accessibility_mode.h:64: void set(int flag) { flags_ |= flag; }
set is maybe an odd name here, you're getting a union of modes, or adding modes,
not setting (replacing) the modes

https://codereview.chromium.org/2694413006/diff/60001/content/common/accessib...
content/common/accessibility_mode.h:67: void clear() { set(OFF); }
this does nothing (a |= 0) == a

The OFF flag doesn't seem like it has a lot of value, and you could just use 0
instead in the places you need it (the constructor only?)

https://codereview.chromium.org/2694413006/diff/60001/content/common/accessib...
content/common/accessibility_mode.h:71: bool operator==(AccessibilityMode rhs)
const {
== should come with a != too

https://codereview.chromium.org/2694413006/diff/60001/content/common/accessib...
content/common/accessibility_mode.h:84: if (flags_ == rhs)
define operator!= in terms of !operator==(). so return !(*this == rhs)

comparing flags_ breaks the encapsulation again though. do you need them?

https://codereview.chromium.org/2694413006/diff/60001/content/common/accessib...
content/common/accessibility_mode.h:89: void operator=(int flags) { flags_ =
flags; }
i think you'd do better to have operator=(AccessibilityMode), this breaks
encapsulation and may do conversions you didn't intend

[aboxhall, 2017-02-22 22:05:50]

https://codereview.chromium.org/2694413006/diff/60001/content/common/accessib...
File content/common/accessibility_mode.h (right):

https://codereview.chromium.org/2694413006/diff/60001/content/common/accessib...
content/common/accessibility_mode.h:64: void set(int flag) { flags_ |= flag; }
On 2017/02/22 21:27:56, danakj wrote:
> set is maybe an odd name here, you're getting a union of modes, or adding
modes,
> not setting (replacing) the modes

Maybe set(int flag, bool state)?

https://codereview.chromium.org/2694413006/diff/60001/content/common/accessib...
content/common/accessibility_mode.h:67: void clear() { set(OFF); }
On 2017/02/22 21:27:56, danakj wrote:
> this does nothing (a |= 0) == a
> 
> The OFF flag doesn't seem like it has a lot of value, and you could just use 0
> instead in the places you need it (the constructor only?)

It's used in a bunch of places to avoid running code if accessibility is off.

[danakj, 2017-02-22 22:08:19]

https://codereview.chromium.org/2694413006/diff/60001/content/common/accessib...
File content/common/accessibility_mode.h (right):

https://codereview.chromium.org/2694413006/diff/60001/content/common/accessib...
content/common/accessibility_mode.h:64: void set(int flag) { flags_ |= flag; }
On 2017/02/22 22:05:50, aboxhall wrote:
> On 2017/02/22 21:27:56, danakj wrote:
> > set is maybe an odd name here, you're getting a union of modes, or adding
> modes,
> > not setting (replacing) the modes
> 
> Maybe set(int flag, bool state)?

If you go with a setter that can add or remove then toggle() could maybe go
away.

https://codereview.chromium.org/2694413006/diff/60001/content/common/accessib...
content/common/accessibility_mode.h:67: void clear() { set(OFF); }
On 2017/02/22 22:05:50, aboxhall wrote:
> On 2017/02/22 21:27:56, danakj wrote:
> > this does nothing (a |= 0) == a
> > 
> > The OFF flag doesn't seem like it has a lot of value, and you could just use
0
> > instead in the places you need it (the constructor only?)
> 
> It's used in a bunch of places to avoid running code if accessibility is off.

An empty() function here could do that for us.

[dougt, 2017-02-22 22:19:59]

https://codereview.chromium.org/2694413006/diff/60001/content/common/accessib...
File content/common/accessibility_mode.h (right):

https://codereview.chromium.org/2694413006/diff/60001/content/common/accessib...
content/common/accessibility_mode.h:1: // Copyright 2014 The Chromium Authors.
All rights reserved.
On 2017/02/22 21:27:56, danakj wrote:
> 2017

Done.

https://codereview.chromium.org/2694413006/diff/60001/content/common/accessib...
content/common/accessibility_mode.h:4: #ifndef
CONTENT_COMMON_ACCESSIBILITY_MODE_ENUMS_H_
On 2017/02/22 21:27:56, danakj wrote:
> white space around the #ifndef#define

Done.

https://codereview.chromium.org/2694413006/diff/60001/content/common/accessib...
content/common/accessibility_mode.h:7: // in sync with these two enums
On 2017/02/22 21:27:56, danakj wrote:
> These aren't enums anymore maybe clarify this comment

dropping comment.  Adding a PRESUBMIT in a follow up (Bug 692087).

https://codereview.chromium.org/2694413006/diff/60001/content/common/accessib...
content/common/accessibility_mode.h:13: const static int OFF = 0;
On 2017/02/22 21:27:56, danakj wrote:
> constexpr instead of const.
> 
> kOff, kNativeApis, etc.

Done.

https://codereview.chromium.org/2694413006/diff/60001/content/common/accessib...
content/common/accessibility_mode.h:13: const static int OFF = 0;
On 2017/02/22 21:27:56, danakj wrote:
> constexpr instead of const.
> 
> kOff, kNativeApis, etc.

Done.

https://codereview.chromium.org/2694413006/diff/60001/content/common/accessib...
content/common/accessibility_mode.h:59: AccessibilityMode(int flag = OFF) :
flags_(flag) {}
On 2017/02/22 21:27:56, danakj wrote:
> explicit

Done.

https://codereview.chromium.org/2694413006/diff/60001/content/common/accessib...
content/common/accessibility_mode.h:62: int get() const { return flags_; }
On 2017/02/22 21:27:56, danakj wrote:
> do you need a get()? it breaks the encapsulation

Sadly, for now yes.  The encapsulation gets broken in 3 places:

* When we IPC serialize (will got away with mojo)

* When we pass mode to chrome://accessibility (We could pass modes instead)

* Difference calculations for reporting. In browser_accessibility_state_impl.cc,
we want to know the absolute mode change and then report which flags have
changed.  Doing tihs on the raw int value is easier.  We could have a
difference() method that gives us the difference between two modes.

follow up for (2) and (3) ?

https://codereview.chromium.org/2694413006/diff/60001/content/common/accessib...
content/common/accessibility_mode.h:67: void clear() { set(OFF); }
On 2017/02/22 21:27:56, danakj wrote:
> this does nothing (a |= 0) == a

Removed.

> The OFF flag doesn't seem like it has a lot of value, and you could just use 0
> instead in the places you need it (the constructor only?)

What Alice said.

https://codereview.chromium.org/2694413006/diff/60001/content/common/accessib...
content/common/accessibility_mode.h:71: bool operator==(AccessibilityMode rhs)
const {
On 2017/02/22 21:27:56, danakj wrote:
> == should come with a != too

Done.

https://codereview.chromium.org/2694413006/diff/60001/content/common/accessib...
content/common/accessibility_mode.h:89: void operator=(int flags) { flags_ =
flags; }
On 2017/02/22 21:27:56, danakj wrote:
> i think you'd do better to have operator=(AccessibilityMode), this breaks
> encapsulation and may do conversions you didn't intend

What do you think if this also used:

[dougt, 2017-02-22 22:21:27]

https://codereview.chromium.org/2694413006/diff/60001/content/common/accessib...
File content/common/accessibility_mode.h (right):

https://codereview.chromium.org/2694413006/diff/60001/content/common/accessib...
content/common/accessibility_mode.h:89: void operator=(int flags) { flags_ =
flags; }
On 2017/02/22 22:19:58, dougt wrote:
> On 2017/02/22 21:27:56, danakj wrote:
> > i think you'd do better to have operator=(AccessibilityMode), this breaks
> > encapsulation and may do conversions you didn't intend
> 
> What do you think if this also used:

setMode(int mode, bool value)

[danakj, 2017-02-22 23:42:06]

Looks like it didn't upload.

[commit-bot: I haz the power, 2017-02-22 23:49:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-22 23:49:05]

Dry run: This issue passed the CQ dry run.

[dougt, 2017-02-23 05:49:09]

The CQ bit was checked by dougt@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-23 05:49:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dougt, 2017-02-23 05:51:29]

dmazzoni, danakj, aboxhall, please take another look.

[commit-bot: I haz the power, 2017-02-23 07:16:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-23 07:16:09]

Dry run: This issue passed the CQ dry run.

[dmazzoni, 2017-02-23 17:08:21]

A few minor nits but the main thing I see left is
to make more of a distinction between a *mode* and
a *mode flag*.

https://codereview.chromium.org/2694413006/diff/80001/content/browser/accessi...
File content/browser/accessibility/accessibility_ui.cc (right):

https://codereview.chromium.org/2694413006/diff/80001/content/browser/accessi...
content/browser/accessibility/accessibility_ui.cc:338: 
extremely minor nit: I wouldn't have a blank line here.
In fact I might prefer this:

web_contents->SetAccessibilityMode(
    AccessibilityMode(AccessibilityMode::kNativeAPIs |
                      AccessibilityMode::kWebContents));

https://codereview.chromium.org/2694413006/diff/80001/content/browser/accessi...
File content/browser/accessibility/hit_testing_browsertest.cc (right):

https://codereview.chromium.org/2694413006/diff/80001/content/browser/accessi...
content/browser/accessibility/hit_testing_browsertest.cc:34:
AccessibilityMode::kComplete,
This function must be taking an int, but it should be taking
an AccessibilityMode. See my other comment, we should have
constants for common AccessibilityModes that are of *type*
AccessibilityMode, rather than mode flags.

https://codereview.chromium.org/2694413006/diff/80001/content/common/accessib...
File content/common/accessibility_mode.h (right):

https://codereview.chromium.org/2694413006/diff/80001/content/common/accessib...
content/common/accessibility_mode.h:52: static constexpr int kComplete =
I would have made kComplete and kWebContentsOnly into
full AccessibilityModes rather than just additional constants.

Something like:

(outside of class)

const AccessibilityMode kAccessibilityModeComplete =
    AccessibilityMode(AccessibilityMode::kNativeAPIs |
                      ...);

That way you can pass it directly into a function that
expects an accessibility mode rather than always needing to
create one.

Same with kAccessibilityModeNone.

To put it another way: the constants inside the class are
*mode flags*, these constants are *modes*.

https://codereview.chromium.org/2694413006/diff/80001/content/common/accessib...
content/common/accessibility_mode.h:59: AccessibilityMode(int flag) :
flags_(flag) {}
nit: flag -> flags

https://codereview.chromium.org/2694413006/diff/80001/content/common/accessib...
content/common/accessibility_mode.h:61: bool hasMode(int flag) const { return
(flags_ & flag) > 0; }
No strong feelings either way, but I wonder if this should
be hasModeFlag?

The mode is all of the flags, each one is one mode flag.

[danakj, 2017-02-23 21:15:02]

I think what you're doing looks good, and aboxhall/dmazonni's feedback is good.
Thanks for the interesting discussions.

https://codereview.chromium.org/2694413006/diff/80001/content/common/accessib...
File content/common/accessibility_mode.h (right):

https://codereview.chromium.org/2694413006/diff/80001/content/common/accessib...
content/common/accessibility_mode.h:59: AccessibilityMode(int flag) :
flags_(flag) {}
explicit

https://codereview.chromium.org/2694413006/diff/80001/content/common/accessib...
content/common/accessibility_mode.h:61: bool hasMode(int flag) const { return
(flags_ & flag) > 0; }
On 2017/02/23 17:08:21, dmazzoni wrote:
> No strong feelings either way, but I wonder if this should
> be hasModeFlag?
> 
> The mode is all of the flags, each one is one mode flag.

+1. Also has_mode, not hasMode, is chromium style. Same throughout.

[dougt, 2017-02-26 22:46:00]

The CQ bit was checked by dougt@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-26 22:46:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-26 23:43:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-26 23:43:43]

Dry run: This issue passed the CQ dry run.

[dougt, 2017-02-27 03:12:57]

dmazzoni, danakj, aboxhall, PTAL.

Aaron, you make also want to take a look too.

https://codereview.chromium.org/2694413006/diff/80001/content/browser/accessi...
File content/browser/accessibility/accessibility_ui.cc (right):

https://codereview.chromium.org/2694413006/diff/80001/content/browser/accessi...
content/browser/accessibility/accessibility_ui.cc:338: 
On 2017/02/23 17:08:21, dmazzoni wrote:
> extremely minor nit: I wouldn't have a blank line here.
> In fact I might prefer this:
> 
> web_contents->SetAccessibilityMode(
>     AccessibilityMode(AccessibilityMode::kNativeAPIs |
>                       AccessibilityMode::kWebContents));

Done.

https://codereview.chromium.org/2694413006/diff/80001/content/common/accessib...
File content/common/accessibility_mode.h (right):

https://codereview.chromium.org/2694413006/diff/80001/content/common/accessib...
content/common/accessibility_mode.h:52: static constexpr int kComplete =
After discussion, we decided to try to remove the complex classes from this
header and define them where needed.  This also means we're going to be very
explict/verbose about what flags we use.

https://codereview.chromium.org/2694413006/diff/80001/content/common/accessib...
content/common/accessibility_mode.h:59: AccessibilityMode(int flag) :
flags_(flag) {}
On 2017/02/23 17:08:21, dmazzoni wrote:
> nit: flag -> flags

Done.

https://codereview.chromium.org/2694413006/diff/80001/content/common/accessib...
content/common/accessibility_mode.h:61: bool hasMode(int flag) const { return
(flags_ & flag) > 0; }
On 2017/02/23 17:08:21, dmazzoni wrote:
> No strong feelings either way, but I wonder if this should
> be hasModeFlag?
> 
> The mode is all of the flags, each one is one mode flag.

Then, we should also change setMode to setModeFlag?

[aleventhal, 2017-02-27 16:36:12]

On 2017/02/27 03:12:57, dougt wrote:
> dmazzoni, danakj, aboxhall, PTAL.
> 
> Aaron, you make also want to take a look too.
> 
>
https://codereview.chromium.org/2694413006/diff/80001/content/browser/accessi...
> File content/browser/accessibility/accessibility_ui.cc (right):
> 
>
https://codereview.chromium.org/2694413006/diff/80001/content/browser/accessi...
> content/browser/accessibility/accessibility_ui.cc:338: 
> On 2017/02/23 17:08:21, dmazzoni wrote:
> > extremely minor nit: I wouldn't have a blank line here.
> > In fact I might prefer this:
> > 
> > web_contents->SetAccessibilityMode(
> >     AccessibilityMode(AccessibilityMode::kNativeAPIs |
> >                       AccessibilityMode::kWebContents));
> 
> Done.
> 
>
https://codereview.chromium.org/2694413006/diff/80001/content/common/accessib...
> File content/common/accessibility_mode.h (right):
> 
>
https://codereview.chromium.org/2694413006/diff/80001/content/common/accessib...
> content/common/accessibility_mode.h:52: static constexpr int kComplete =
> After discussion, we decided to try to remove the complex classes from this
> header and define them where needed.  This also means we're going to be very
> explict/verbose about what flags we use.
> 
>
https://codereview.chromium.org/2694413006/diff/80001/content/common/accessib...
> content/common/accessibility_mode.h:59: AccessibilityMode(int flag) :
> flags_(flag) {}
> On 2017/02/23 17:08:21, dmazzoni wrote:
> > nit: flag -> flags
> 
> Done.
> 
>
https://codereview.chromium.org/2694413006/diff/80001/content/common/accessib...
> content/common/accessibility_mode.h:61: bool hasMode(int flag) const { return
> (flags_ & flag) > 0; }
> On 2017/02/23 17:08:21, dmazzoni wrote:
> > No strong feelings either way, but I wonder if this should
> > be hasModeFlag?
> > 
> > The mode is all of the flags, each one is one mode flag.
> 
> Then, we should also change setMode to setModeFlag?

Looks good. I didn't check every file, but I read over accessibility_mode.h and
gave some others a glance. Cool, thanks for CC'ing me.

[dmazzoni, 2017-02-27 23:54:20]

lgtm except for kOff

As a compromise maybe something more explicit like
kAllModeFlagsOff would be more clear

https://codereview.chromium.org/2694413006/diff/100001/content/common/accessi...
File content/common/accessibility_mode.h (right):

https://codereview.chromium.org/2694413006/diff/100001/content/common/accessi...
content/common/accessibility_mode.h:12: static constexpr int kOff = 0;
I still think kOff doesn't belong. There's no indication here that
it's a mode and not a mode flag.

Either it should be a const AccessibilityMode, or there should be
a function like AccessibilityMode::all_flags_off().

[dougt, 2017-02-28 02:21:55]

Description was changed from

==========
Scope and clean up uses of AccessibilityMode.

This CL removes the typedef |AccessibilityMode| and instead uses the enum class
which allows us to stop using the all UPPER CASE prefix
ACCESSIBILITY_MODE_FLAG_. Also this CL allows us to better validate the enum
value passed during IPC.

R=dmazzoni

BUG=None
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Scope and clean up uses of AccessibilityMode.

This CL removes the typedef |AccessibilityMode| and instead uses a class
which allows us to stop using the all UPPER CASE prefix
ACCESSIBILITY_MODE_FLAG_.

R=dmazzoni

BUG=None
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[dougt, 2017-02-28 04:01:54]

The CQ bit was checked by dougt@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-28 04:03:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-28 05:33:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-28 05:33:11]

Dry run: This issue passed the CQ dry run.

[dougt, 2017-02-28 19:06:36]

dmazzoni, wdyt?

[dmazzoni, 2017-02-28 19:07:50]

lgtm

[dougt, 2017-02-28 19:30:24]

dougt@chromium.org changed reviewers:
+ brettw@chromium.org

[dougt, 2017-02-28 19:30:25]

brettw, ptal.

[brettw, 2017-03-03 17:49:12]

lgtm

[dougt, 2017-03-03 18:36:00]

dougt@chromium.org changed reviewers:
+ dcheng@chromium.org

[dougt, 2017-03-03 18:36:01]

dcheng, PTAL at content/common/frame_manager.h

AccessiblityMode use to be a typedef to int. Now it's a class.

[dougt, 2017-03-03 18:36:03]

dcheng, PTAL at content/common/frame_manager.h

AccessiblityMode use to be a typedef to int. Now it's a class.

[dcheng, 2017-03-03 18:43:20]

https://codereview.chromium.org/2694413006/diff/120001/content/common/accessi...
File content/common/accessibility_mode.h (right):

https://codereview.chromium.org/2694413006/diff/120001/content/common/accessi...
content/common/accessibility_mode.h:59: int get_mode() const { return flags_; }
FWIW, lower-camel case things aren't usually prefixed with get_

https://codereview.chromium.org/2694413006/diff/120001/content/common/frame_m...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/2694413006/diff/120001/content/common/frame_m...
content/common/frame_messages.h:757:
IPC_MESSAGE_ROUTED1(FrameMsg_SetAccessibilityMode, int)
Would it make sense to just keep this typed as AccessibilityMode? Should be easy
to write a param traits for it.

(I guess if it's used as int in a lot of other places, then maybe it's OK to
leave it like this... but it seems like we went to a bit of trouble to have it
encapsulated inside a helper)

[dougt, 2017-03-03 18:54:38]

On 2017/03/03 18:43:20, dcheng wrote:
>
https://codereview.chromium.org/2694413006/diff/120001/content/common/accessi...
> File content/common/accessibility_mode.h (right):
> 
>
https://codereview.chromium.org/2694413006/diff/120001/content/common/accessi...
> content/common/accessibility_mode.h:59: int get_mode() const { return flags_;
}
> FWIW, lower-camel case things aren't usually prefixed with get_
> 
>
https://codereview.chromium.org/2694413006/diff/120001/content/common/frame_m...
> File content/common/frame_messages.h (right):
> 
>
https://codereview.chromium.org/2694413006/diff/120001/content/common/frame_m...
> content/common/frame_messages.h:757:
> IPC_MESSAGE_ROUTED1(FrameMsg_SetAccessibilityMode, int)
> Would it make sense to just keep this typed as AccessibilityMode? Should be
easy
> to write a param traits for it.
> 
> (I guess if it's used as int in a lot of other places, then maybe it's OK to
> leave it like this... but it seems like we went to a bit of trouble to have it
> encapsulated inside a helper)

Actually -- we want to get rid of get_mode() all together.  Access to the
underlying value is only used in three places:

* When we IPC serialize (will go away with mojo)

* When we pass mode to chrome://accessibility (We could pass modes instead)

* Difference calculations for reporting. In browser_accessibility_state_impl.cc,
we want to know the absolute mode change and then report which flags have
changed.  Doing this on the raw int value is easier.  We could have a
difference() method that gives us the difference between two modes.

I am inclined to just follow up on these.

[dcheng, 2017-03-03 23:57:55]

On 2017/03/03 18:54:38, dougt wrote:
> On 2017/03/03 18:43:20, dcheng wrote:
> >
>
https://codereview.chromium.org/2694413006/diff/120001/content/common/accessi...
> > File content/common/accessibility_mode.h (right):
> > 
> >
>
https://codereview.chromium.org/2694413006/diff/120001/content/common/accessi...
> > content/common/accessibility_mode.h:59: int get_mode() const { return
flags_;
> }
> > FWIW, lower-camel case things aren't usually prefixed with get_
> > 
> >
>
https://codereview.chromium.org/2694413006/diff/120001/content/common/frame_m...
> > File content/common/frame_messages.h (right):
> > 
> >
>
https://codereview.chromium.org/2694413006/diff/120001/content/common/frame_m...
> > content/common/frame_messages.h:757:
> > IPC_MESSAGE_ROUTED1(FrameMsg_SetAccessibilityMode, int)
> > Would it make sense to just keep this typed as AccessibilityMode? Should be
> easy
> > to write a param traits for it.
> > 
> > (I guess if it's used as int in a lot of other places, then maybe it's OK to
> > leave it like this... but it seems like we went to a bit of trouble to have
it
> > encapsulated inside a helper)
> 
> Actually -- we want to get rid of get_mode() all together.  Access to the
> underlying value is only used in three places:
> 
> * When we IPC serialize (will go away with mojo)
> 
> * When we pass mode to chrome://accessibility (We could pass modes instead)
> 
> * Difference calculations for reporting. In
browser_accessibility_state_impl.cc,
> we want to know the absolute mode change and then report which flags have
> changed.  Doing this on the raw int value is easier.  We could have a
> difference() method that gives us the difference between two modes.
> 
> I am inclined to just follow up on these.

I don't think this answers my question about using the AccessibilityMode type
directly in the IPC message though?

[dougt, 2017-03-04 04:39:37]

The CQ bit was checked by dougt@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-04 04:39:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dougt, 2017-03-04 04:46:00]

dcheng, what do you think about doing something like the following.

[commit-bot: I haz the power, 2017-03-04 05:32:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-04 05:32:38]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-03-07 08:41:48]

https://codereview.chromium.org/2694413006/diff/140001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2694413006/diff/140001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:175: const AccessibilityMode
kAccessibilityModeWebContent(
constexpr

(I'm a little surprised that this doesn't trigger any warnings about potential
static initializers)

https://codereview.chromium.org/2694413006/diff/140001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:1106:
AccessibilityMode::kHTML);
Would be nice not to have this duplicated in
https://codereview.chromium.org/2694413006/diff/140001/content/browser/androi...
as well.

Maybe just define the complete version as a constexpr in the AccessibilityMode
header and use it everywhere?

https://codereview.chromium.org/2694413006/diff/140001/content/common/content...
File content/common/content_param_traits.cc (right):

https://codereview.chromium.org/2694413006/diff/140001/content/common/content...
content/common/content_param_traits.cc:101: m->WriteBytes(&value,
sizeof(uint32_t));
Use IPC::WriteParam for type deduction (similarly, IPC::GetParamSize,
IPC::ReadParam)

[dcheng, 2017-03-07 08:42:33]

Also, for me, it helps if rebases and replies to review comments aren't uploaded
in the same PS; otherwise, it takes me a lot longer since I have to go through
the full diff again. Thanks!

[dougt, 2017-03-07 16:49:50]

The CQ bit was checked by dougt@chromium.org to run a CQ dry run

[dougt, 2017-03-07 16:50:17]

On 2017/03/07 08:42:33, dcheng wrote:
> Also, for me, it helps if rebases and replies to review comments aren't
uploaded
> in the same PS; otherwise, it takes me a lot longer since I have to go through
> the full diff again. Thanks!

Sorry!

[dougt, 2017-03-07 16:50:46]

dcheng, ptal.

https://codereview.chromium.org/2694413006/diff/140001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2694413006/diff/140001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:175: const AccessibilityMode
kAccessibilityModeWebContent(
On 2017/03/07 08:41:48, dcheng wrote:
> constexpr
> 
> (I'm a little surprised that this doesn't trigger any warnings about potential
> static initializers)

Yes, if it is a constexpr you do get an error/warning:

'AccessibilityMode' is not literal because it is not an aggregate and has no
constexpr constructors other than copy or move constructors

This is because constexpr implies that the value is known at complication time
-- but it's not in this case.

https://codereview.chromium.org/2694413006/diff/140001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:1106:
AccessibilityMode::kHTML);
On 2017/03/07 08:41:48, dcheng wrote:
> Would be nice not to have this duplicated in
>
https://codereview.chromium.org/2694413006/diff/140001/content/browser/androi...
> as well.
> 
> Maybe just define the complete version as a constexpr in the AccessibilityMode
> header and use it everywhere?

I considered doing that, but would that mean that every compilation unit that
includes this header would have a copy of these values. Probably doesn't matter
that much either way.  I sort of like it where we're very explict at the call
site on exactly what flags we want to use.  WDYT?

https://codereview.chromium.org/2694413006/diff/140001/content/common/content...
File content/common/content_param_traits.cc (right):

https://codereview.chromium.org/2694413006/diff/140001/content/common/content...
content/common/content_param_traits.cc:101: m->WriteBytes(&value,
sizeof(uint32_t));
On 2017/03/07 08:41:48, dcheng wrote:
> Use IPC::WriteParam for type deduction (similarly, IPC::GetParamSize,
> IPC::ReadParam)

\./

Cool trick. Done.

[commit-bot: I haz the power, 2017-03-07 16:51:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dcheng, 2017-03-07 18:05:13]

https://codereview.chromium.org/2694413006/diff/140001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2694413006/diff/140001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:175: const AccessibilityMode
kAccessibilityModeWebContent(
On 2017/03/07 16:50:46, dougt wrote:
> On 2017/03/07 08:41:48, dcheng wrote:
> > constexpr
> > 
> > (I'm a little surprised that this doesn't trigger any warnings about
potential
> > static initializers)
> 
> Yes, if it is a constexpr you do get an error/warning:
> 
> 'AccessibilityMode' is not literal because it is not an aggregate and has no
> constexpr constructors other than copy or move constructors
> 
> This is because constexpr implies that the value is known at complication time
> -- but it's not in this case.

If the constructor is constexpr as well, it should work.

https://codereview.chromium.org/2694413006/diff/140001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:1106:
AccessibilityMode::kHTML);
On 2017/03/07 16:50:46, dougt wrote:
> On 2017/03/07 08:41:48, dcheng wrote:
> > Would be nice not to have this duplicated in
> >
>
https://codereview.chromium.org/2694413006/diff/140001/content/browser/androi...
> > as well.
> > 
> > Maybe just define the complete version as a constexpr in the
AccessibilityMode
> > header and use it everywhere?
> 
> I considered doing that, but would that mean that every compilation unit that
> includes this header would have a copy of these values. Probably doesn't
matter
> that much either way.  I sort of like it where we're very explict at the call
> site on exactly what flags we want to use.  WDYT?
> 
> 

See my previous comment about constexpr. Explicit-ness is nice, but when all we
want is to make sure all the flags are included, I think it's better to have it
defined in one place (next to where we define the flags)

[commit-bot: I haz the power, 2017-03-07 19:25:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-07 19:25:47]

Dry run: This issue passed the CQ dry run.

[dougt, 2017-03-08 18:09:04]

The CQ bit was checked by dougt@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-08 18:09:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-08 18:37:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-08 18:37:07]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[dougt, 2017-03-08 22:24:10]

Patchset #10 (id:180001) has been deleted

[dougt, 2017-03-08 22:59:17]

The CQ bit was checked by dougt@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-08 22:59:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-08 23:28:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-08 23:28:12]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[dougt, 2017-03-08 23:36:58]

Patchset #10 (id:200001) has been deleted

[dougt, 2017-03-09 22:56:03]

The CQ bit was checked by dougt@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-09 22:56:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dougt, 2017-03-09 23:22:24]

dmazzoni, please take another look.

dcheng, ptal.

[commit-bot: I haz the power, 2017-03-10 00:14:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-10 00:14:46]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-03-10 04:16:00]

LGTM

[dmazzoni, 2017-03-13 22:18:15]

still lgtm with nits

No need for additional review, it's a pain to keep maintaining
refactoring so please try to land now that everyone's happy!

https://codereview.chromium.org/2694413006/diff/220001/content/common/content...
File content/common/content_param_traits.cc (right):

https://codereview.chromium.org/2694413006/diff/220001/content/common/content...
content/common/content_param_traits.cc:114: std::string* l) {}
nit: any reason not to log this? kind of obscure but i don't see any reason not
to

https://codereview.chromium.org/2694413006/diff/220001/content/renderer/acces...
File content/renderer/accessibility/render_accessibility_impl_browsertest.cc
(right):

https://codereview.chromium.org/2694413006/diff/220001/content/renderer/acces...
content/renderer/accessibility/render_accessibility_impl_browsertest.cc:36:
AccessibilityMode::kNativeAPIs |
nit: should this be kAccessibilityModeComplete?

https://codereview.chromium.org/2694413006/diff/220001/content/renderer/rende...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/2694413006/diff/220001/content/renderer/rende...
content/renderer/render_frame_impl.h:1316: // Only valid if
|accessibility_mode_| has not flags.
nit: has no flags set?

[dougt, 2017-03-13 23:19:38]

The CQ bit was checked by dougt@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-13 23:21:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-14 00:36:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-14 00:36:52]

Dry run: Try jobs failed on following builders:
  linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)

[dougt, 2017-03-14 01:26:04]

The CQ bit was checked by dougt@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 01:26:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-14 02:12:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-14 02:13:02]

Dry run: This issue passed the CQ dry run.

[dougt, 2017-03-14 03:18:24]

https://codereview.chromium.org/2694413006/diff/220001/content/common/content...
File content/common/content_param_traits.cc (right):

https://codereview.chromium.org/2694413006/diff/220001/content/common/content...
content/common/content_param_traits.cc:114: std::string* l) {}
On 2017/03/13 22:18:15, dmazzoni wrote:
> nit: any reason not to log this? kind of obscure but i don't see any reason
not
> to

Acknowledged. I didn't really see a need yet. Fwiw, other protocols don't log
(for example MessagePort). Happy to do in a follow up.

https://codereview.chromium.org/2694413006/diff/220001/content/renderer/acces...
File content/renderer/accessibility/render_accessibility_impl_browsertest.cc
(right):

https://codereview.chromium.org/2694413006/diff/220001/content/renderer/acces...
content/renderer/accessibility/render_accessibility_impl_browsertest.cc:36:
AccessibilityMode::kNativeAPIs |
On 2017/03/13 22:18:15, dmazzoni wrote:
> nit: should this be kAccessibilityModeComplete?

Done.

https://codereview.chromium.org/2694413006/diff/220001/content/renderer/rende...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/2694413006/diff/220001/content/renderer/rende...
content/renderer/render_frame_impl.h:1316: // Only valid if
|accessibility_mode_| has not flags.
On 2017/03/13 22:18:15, dmazzoni wrote:
> nit: has no flags set?

Done.

[dougt, 2017-03-14 03:19:26]

The CQ bit was checked by dougt@chromium.org

[dougt, 2017-03-14 03:19:27]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
brettw@chromium.org, dmazzoni@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2694413006/#ps280001
(title: "merge")

[commit-bot: I haz the power, 2017-03-14 03:19:47]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-14 03:26:28]

CQ is committing da patch.

Bot data: {"patchset_id": 280001, "attempt_start_ts": 1489461566740880,
"parent_rev": "db0a348759ea9c97783c516344a1d96880ceb359", "commit_rev":
"cd3dad73b453f8a89731bc2a2cd72f3bd5def991"}

[commit-bot: I haz the power, 2017-03-14 03:27:26]

Description was changed from

==========
Scope and clean up uses of AccessibilityMode.

This CL removes the typedef |AccessibilityMode| and instead uses a class
which allows us to stop using the all UPPER CASE prefix
ACCESSIBILITY_MODE_FLAG_.

R=dmazzoni

BUG=None
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Scope and clean up uses of AccessibilityMode.

This CL removes the typedef |AccessibilityMode| and instead uses a class
which allows us to stop using the all UPPER CASE prefix
ACCESSIBILITY_MODE_FLAG_.

R=dmazzoni

BUG=None
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2694413006
Cr-Commit-Position: refs/heads/master@{#456614}
Committed:
https://chromium.googlesource.com/chromium/src/+/cd3dad73b453f8a89731bc2a2cd7...
==========

[commit-bot: I haz the power, 2017-03-14 03:27:34]

Committed patchset #13 (id:280001) as
https://chromium.googlesource.com/chromium/src/+/cd3dad73b453f8a89731bc2a2cd7...