Add an accessibility API for events raised outside of the web content.

Add an accessibility API for events raised outside of the web content.

BUG=none
TEST=none

[Erik does not do reviews, 2009-12-18 19:43:53]

Since I suspect that the API changes I'm requesting will change a bunch of the
other code, I'll hold off on the rest of the review for now.

http://codereview.chromium.org/402099/diff/9050/9074
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/402099/diff/9050/9074#newcode209
chrome/common/extensions/api/extension_api.json:209: "id": "ControlInfo",
This type feels a bit muddled.  Many of the values only make sense for a
particular type and some aren't even necessary (their value is inferred by the
type).  I think we should have a few different explicit subtypes and use the
"choices" type as the sole member to allow one of the types to be specified.

http://codereview.chromium.org/402099/diff/9050/9074#newcode212
chrome/common/extensions/api/extension_api.json:212: "controlType": {"type":
"string", "description": "The type of the control, like TextBox or Button."},
this seems like it should be the enum type ("enum": ["TextBox", "Button", ...])
- it looks like we don't have any good examples of this in use

http://codereview.chromium.org/402099/diff/9050/9074#newcode230
chrome/common/extensions/api/extension_api.json:230: "nodocs": "true",
remove all of the nodocs params.  we already handle experimental APIs specially.

http://codereview.chromium.org/402099/diff/9050/9074#newcode231
chrome/common/extensions/api/extension_api.json:231: "customHandler": "true",
why is customHandler true?  I don't see custom hooks implemented.

http://codereview.chromium.org/402099/diff/9050/9074#newcode255
chrome/common/extensions/api/extension_api.json:255: "key": {"type": "integer",
"optional": false, "description": "The code of the key to press."},
remove "optional" field.  Also, specify that this is the same values as
event.keyCode (I'm assuming), so it's system dependent.

http://codereview.chromium.org/402099/diff/9050/9074#newcode266
chrome/common/extensions/api/extension_api.json:266: "name": "onOpen",
It seems weird to combine menus and windows in one event, even if they wind up
with effectively the same implementation.  I'd prefer onMenuOpen, onWindowOpen.

http://codereview.chromium.org/402099/diff/9050/9074#newcode314
chrome/common/extensions/api/extension_api.json:314: "description": "Fired when
a control is selected.",
Could you be more specific on the difference between "selected" and "focused"
here?  Those terms get overloaded a lot, and I expect the difference here is
subtle.

[dmazzoni at google, 2010-01-04 17:18:45]

On Fri, Dec 18, 2009 at 11:43 AM,  <erikkay@chromium.org> wrote:
> http://codereview.chromium.org/402099/diff/9050/9074#newcode209
> chrome/common/extensions/api/extension_api.json:209: "id":
> "ControlInfo",
> This type feels a bit muddled.  Many of the values only make sense for a
> particular type and some aren't even necessary (their value is inferred
> by the type).  I think we should have a few different explicit subtypes
> and use the "choices" type as the sole member to allow one of the types
> to be specified.

I'm struggling to figure out how the client is supposed to determine
the type of the returned object. Suppose I define ButtonInfo,
TextBoxInfo, etc. - all of these are then possible return values for
the onFocus handler.  The client can't use (typeof ctl) or
(ctl.constructor.name) - those just return "object".  The only way I
can see would be to have a property identifying the control type - but
this seems to defeat the purpose of using types, doesn't it?

Something else to consider: with this approach, if a new control type
is added later, extensions would need to be modified to support it.  A
single, more generic ControlInfo, where many of the fields are
optional, would allow an extension to handle that control just by
checking its properties.

Thoughts?

[Erik does not do reviews, 2010-01-07 23:39:10]

On Tue, Dec 29, 2009 at 2:19 PM, Dominic Mazzoni <dmazzoni@google.com>wrote:

> On Fri, Dec 18, 2009 at 11:43 AM,  <erikkay@chromium.org> wrote:
> > http://codereview.chromium.org/402099/diff/9050/9074#newcode209
> > chrome/common/extensions/api/extension_api.json:209: "id":
> > "ControlInfo",
> > This type feels a bit muddled.  Many of the values only make sense for a
> > particular type and some aren't even necessary (their value is inferred
> > by the type).  I think we should have a few different explicit subtypes
> > and use the "choices" type as the sole member to allow one of the types
> > to be specified.
>
> I'm struggling to figure out how the client is supposed to determine
> the type of the returned object. Suppose I define ButtonInfo,
> TextBoxInfo, etc. - all of these are then possible return values for
> the onFocus handler.  The client can't use (typeof ctl) or
> (ctl.constructor.name) - those just return "object".  The only way I
> can see would be to have a property identifying the control type - but
> this seems to defeat the purpose of using types, doesn't it?
>

This is a good point.  We consciously avoided making real types in our
design in order to avoid any perception that people were dealing with "live"
objects.  I could see benefits in this case though, and I think it might be
cool if these things were of type
"chrome.experimental.accessibility.TextField", etc.  I think it's something
we can consider in the future, but we definitely won't get to it in the
short term.

In the short term, I was assuming that you'd have a property field.  This
doesn't actually defeat the purpose of using types.  The main reason I was
interested in you splitting them up is for clarity particularly with the
generated docs. From an actual implementation standpoint, since objects are
just hashtables and since these fields are all optional anyway, there isn't
a real implementation difference.  The secondary benefit is that the type
information is more rigorously enforced (e.g. you can't have isPassword ==
true on a select element).

Incidentally, there's apparently a way to define some kind of type
inheritence in our schema as well, so you can declare a FormControl object
with a type field, and then have a bunch of subtypes that "extend" that
type.  I'll send you a sample of this in a bit.



> Something else to consider: with this approach, if a new control type
> is added later, extensions would need to be modified to support it.  A
> single, more generic ControlInfo, where many of the fields are
> optional, would allow an extension to handle that control just by
> checking its properties.
>
> Thoughts?
>

[Erik does not do reviews, 2010-01-07 23:46:07]

On Thu, Jan 7, 2010 at 3:38 PM, Erik Kay <erikkay@chromium.org> wrote:

> On Tue, Dec 29, 2009 at 2:19 PM, Dominic Mazzoni <dmazzoni@google.com>wrote:
>
>> On Fri, Dec 18, 2009 at 11:43 AM,  <erikkay@chromium.org> wrote:
>> > http://codereview.chromium.org/402099/diff/9050/9074#newcode209
>> > chrome/common/extensions/api/extension_api.json:209: "id":
>> > "ControlInfo",
>> > This type feels a bit muddled.  Many of the values only make sense for a
>> > particular type and some aren't even necessary (their value is inferred
>> > by the type).  I think we should have a few different explicit subtypes
>> > and use the "choices" type as the sole member to allow one of the types
>> > to be specified.
>>
>> I'm struggling to figure out how the client is supposed to determine
>> the type of the returned object. Suppose I define ButtonInfo,
>> TextBoxInfo, etc. - all of these are then possible return values for
>> the onFocus handler.  The client can't use (typeof ctl) or
>> (ctl.constructor.name) - those just return "object".  The only way I
>> can see would be to have a property identifying the control type - but
>> this seems to defeat the purpose of using types, doesn't it?
>>
>
> This is a good point.  We consciously avoided making real types in our
> design in order to avoid any perception that people were dealing with "live"
> objects.  I could see benefits in this case though, and I think it might be
> cool if these things were of type
> "chrome.experimental.accessibility.TextField", etc.  I think it's something
> we can consider in the future, but we definitely won't get to it in the
> short term.
>
> In the short term, I was assuming that you'd have a property field.  This
> doesn't actually defeat the purpose of using types.  The main reason I was
> interested in you splitting them up is for clarity particularly with the
> generated docs. From an actual implementation standpoint, since objects are
> just hashtables and since these fields are all optional anyway, there isn't
> a real implementation difference.  The secondary benefit is that the type
> information is more rigorously enforced (e.g. you can't have isPassword ==
> true on a select element).
>
> Incidentally, there's apparently a way to define some kind of type
> inheritence in our schema as well, so you can declare a FormControl object
> with a type field, and then have a bunch of subtypes that "extend" that
> type.  I'll send you a sample of this in a bit.
>

Actually, ignore this last comment.  "extends" turns out to be not as useful
as I thought (it winds up losing some of the type validation).


>
>
>
>> Something else to consider: with this approach, if a new control type
>> is added later, extensions would need to be modified to support it.  A
>> single, more generic ControlInfo, where many of the fields are
>> optional, would allow an extension to handle that control just by
>> checking its properties.
>>
>> Thoughts?
>>
>
>

[Erik does not do reviews, 2010-01-08 15:39:29]

On Thu, Jan 7, 2010 at 5:58 PM, Dominic Mazzoni <dmazzoni@google.com> wrote:

> On Thu, Jan 7, 2010 at 3:45 PM, Erik Kay <erikkay@chromium.org> wrote:
> >> Incidentally, there's apparently a way to define some kind of type
> >> inheritence in our schema as well, so you can declare a FormControl
> object
> >> with a type field, and then have a bunch of subtypes that "extend" that
> >> type.  I'll send you a sample of this in a bit.
> >
> > Actually, ignore this last comment.  "extends" turns out to be not as
> useful
> > as I thought (it winds up losing some of the type validation).
>
> So it sounds like I should use duck typing. I think it would be a
> really good idea for all of these objects to have several fields in
> common (more than just the type annotation) and perhaps even a
> hierarchy (for example, LocationBar could inherit from TextControl).
> Maybe we couldn't express the hierarchy in the .json file, but perhaps
> I could add it to the extension api documentation?


I agree inheritance would be nice for the docs.  Maybe we can come up with
some annotation that could help us on that front.  However, for now it looks
like this isn't possible.  Let's see what it looks like without.

Erik

[dmazzoni at google, 2010-01-08 18:12:00]

On Thu, Jan 7, 2010 at 3:45 PM, Erik Kay <erikkay@chromium.org> wrote:
>> Incidentally, there's apparently a way to define some kind of type
>> inheritence in our schema as well, so you can declare a FormControl object
>> with a type field, and then have a bunch of subtypes that "extend" that
>> type.  I'll send you a sample of this in a bit.
>
> Actually, ignore this last comment.  "extends" turns out to be not as useful
> as I thought (it winds up losing some of the type validation).

So it sounds like I should use duck typing. I think it would be a
really good idea for all of these objects to have several fields in
common (more than just the type annotation) and perhaps even a
hierarchy (for example, LocationBar could inherit from TextControl).
Maybe we couldn't express the hierarchy in the .json file, but perhaps
I could add it to the extension api documentation?

- Dominic

[dmazzoni at google, 2010-01-12 17:05:59]

I addressed your comments and refactored the code accordingly. It's
ready for another look.

My new browser test only passes when the
--enable-experimental-extension-apis  flag is set. (For some reason,
this has changed recently. A couple of weeks ago that flag didn't seem
to be needed.) What should I do about this?

More responses below:

On Fri, Dec 18, 2009 at 11:43 AM,  <erikkay@chromium.org> wrote:
> http://codereview.chromium.org/402099/diff/9050/9074#newcode209
> chrome/common/extensions/api/extension_api.json:209: "id":
> "ControlInfo",
> This type feels a bit muddled.  Many of the values only make sense for a
> particular type and some aren't even necessary (their value is inferred
> by the type).  I think we should have a few different explicit subtypes
> and use the "choices" type as the sole member to allow one of the types
> to be specified.

I don't think "choices" works here, but I now there's a specific type
for each type of control that can be focused.  The "details" that are
passed around in C++ now really use a different subclass for each
control type.

So far there's no need for a hierarchy; it seems to be clear enough to
have them just all inherit from one type. How does this look?

> http://codereview.chromium.org/402099/diff/9050/9074#newcode212
> chrome/common/extensions/api/extension_api.json:212: "controlType":
> {"type": "string", "description": "The type of the control, like TextBox
> or Button."},
> this seems like it should be the enum type ("enum": ["TextBox",
> "Button", ...]) - it looks like we don't have any good examples of this
> in use

I have the type correspond to the name of the subclass now.  Does this
work, or would you prefer an enum?

> http://codereview.chromium.org/402099/diff/9050/9074#newcode230
> chrome/common/extensions/api/extension_api.json:230: "nodocs": "true",
> remove all of the nodocs params.  we already handle experimental APIs
> specially.

Done.

> http://codereview.chromium.org/402099/diff/9050/9074#newcode231
> chrome/common/extensions/api/extension_api.json:231: "customHandler":
> "true",
> why is customHandler true?  I don't see custom hooks implemented.

Removed.

> http://codereview.chromium.org/402099/diff/9050/9074#newcode255
> chrome/common/extensions/api/extension_api.json:255: "key": {"type":
> "integer", "optional": false, "description": "The code of the key to
> press."},
> remove "optional" field.  Also, specify that this is the same values as
> event.keyCode (I'm assuming), so it's system dependent.

Done.

> http://codereview.chromium.org/402099/diff/9050/9074#newcode266
> chrome/common/extensions/api/extension_api.json:266: "name": "onOpen",
> It seems weird to combine menus and windows in one event, even if they
> wind up with effectively the same implementation.  I'd prefer
> onMenuOpen, onWindowOpen.

Sure. I've implemented onOpenWindow for now, and I'll add onOpenMenu,
etc. later.

> http://codereview.chromium.org/402099/diff/9050/9074#newcode314
> chrome/common/extensions/api/extension_api.json:314: "description":
> "Fired when a control is selected.",
> Could you be more specific on the difference between "selected" and
> "focused" here?  Those terms get overloaded a lot, and I expect the
> difference here is subtle.

Sure, I changed it to onControlFocus and onControlAction, and added a
more clear description for onControlAction.

Thanks,
- Dominic

> http://codereview.chromium.org/402099
>

[Erik does not do reviews, 2010-01-13 00:35:28]

On Tue, Jan 12, 2010 at 9:05 AM, Dominic Mazzoni <dmazzoni@google.com>wrote:

> I addressed your comments and refactored the code accordingly. It's
> ready for another look.
>
> My new browser test only passes when the
> --enable-experimental-extension-apis  flag is set. (For some reason,
> this has changed recently. A couple of weeks ago that flag didn't seem
> to be needed.) What should I do about this?
>
> More responses below:
>
> On Fri, Dec 18, 2009 at 11:43 AM,  <erikkay@chromium.org> wrote:
> > http://codereview.chromium.org/402099/diff/9050/9074#newcode209
> > chrome/common/extensions/api/extension_api.json:209: "id":
> > "ControlInfo",
> > This type feels a bit muddled.  Many of the values only make sense for a
> > particular type and some aren't even necessary (their value is inferred
> > by the type).  I think we should have a few different explicit subtypes
> > and use the "choices" type as the sole member to allow one of the types
> > to be specified.
>
> I don't think "choices" works here, but I now there's a specific type
> for each type of control that can be focused.  The "details" that are
> passed around in C++ now really use a different subclass for each
> control type.
>

I don't understand why choices doesn't work.  Without using choices, the
schema can't enforce the parameters correctly.


> So far there's no need for a hierarchy; it seems to be clear enough to
> have them just all inherit from one type. How does this look?
>
> > http://codereview.chromium.org/402099/diff/9050/9074#newcode212
> > chrome/common/extensions/api/extension_api.json:212: "controlType":
> > {"type": "string", "description": "The type of the control, like TextBox
> > or Button."},
> > this seems like it should be the enum type ("enum": ["TextBox",
> > "Button", ...]) - it looks like we don't have any good examples of this
> > in use
>
> I have the type correspond to the name of the subclass now.  Does this
> work, or would you prefer an enum?
>

I'd prefer an enum.  That way the schema can enforce that it's correct.


> http://codereview.chromium.org/402099/diff/9050/9074#newcode230
> > chrome/common/extensions/api/extension_api.json:230: "nodocs": "true",
> > remove all of the nodocs params.  we already handle experimental APIs
> > specially.
>
> Done.
>
> > http://codereview.chromium.org/402099/diff/9050/9074#newcode231
> > chrome/common/extensions/api/extension_api.json:231: "customHandler":
> > "true",
> > why is customHandler true?  I don't see custom hooks implemented.
>
> Removed.
>
> > http://codereview.chromium.org/402099/diff/9050/9074#newcode255
> > chrome/common/extensions/api/extension_api.json:255: "key": {"type":
> > "integer", "optional": false, "description": "The code of the key to
> > press."},
> > remove "optional" field.  Also, specify that this is the same values as
> > event.keyCode (I'm assuming), so it's system dependent.
>
> Done.
>
> > http://codereview.chromium.org/402099/diff/9050/9074#newcode266
> > chrome/common/extensions/api/extension_api.json:266: "name": "onOpen",
> > It seems weird to combine menus and windows in one event, even if they
> > wind up with effectively the same implementation.  I'd prefer
> > onMenuOpen, onWindowOpen.
>
> Sure. I've implemented onOpenWindow for now, and I'll add onOpenMenu,
> etc. later.
>
> > http://codereview.chromium.org/402099/diff/9050/9074#newcode314
> > chrome/common/extensions/api/extension_api.json:314: "description":
> > "Fired when a control is selected.",
> > Could you be more specific on the difference between "selected" and
> > "focused" here?  Those terms get overloaded a lot, and I expect the
> > difference here is subtle.
>
> Sure, I changed it to onControlFocus and onControlAction, and added a
> more clear description for onControlAction.
>
> Thanks,
> - Dominic
>
> > http://codereview.chromium.org/402099
> >
>

[dmazzoni at google, 2010-01-13 21:15:36]

On Tue, Jan 12, 2010 at 4:35 PM, Erik Kay <erikkay@chromium.org> wrote:
>> I don't think "choices" works here, but I now there's a specific type
>> for each type of control that can be focused.  The "details" that are
>> passed around in C++ now really use a different subclass for each
>> control type.
>
> I don't understand why choices doesn't work.  Without using choices, the
> schema can't enforce the parameters correctly.

> I'd prefer an enum.  That way the schema can enforce that it's correct.

Great, I added both choices and enum.  Everything works, and the
schema enforcement is great, but here's one thing I'm not sure about:
for the parent class "AccessibilityControl", controlType is an enum
with 8 possible values.  For a subclass like "AccessibilityButton", I
made controlType into an enum with only one possible value:
"AccessibilityButton". Does that make sense to you?

It's uploaded now, take a look when you have a chance.

- Dominic

http://codereview.chromium.org/402099

[Erik does not do reviews, 2010-01-13 22:19:48]

On Wed, Jan 13, 2010 at 1:15 PM, Dominic Mazzoni <dmazzoni@google.com>wrote:

> On Tue, Jan 12, 2010 at 4:35 PM, Erik Kay <erikkay@chromium.org> wrote:
> >> I don't think "choices" works here, but I now there's a specific type
> >> for each type of control that can be focused.  The "details" that are
> >> passed around in C++ now really use a different subclass for each
> >> control type.
> >
> > I don't understand why choices doesn't work.  Without using choices, the
> > schema can't enforce the parameters correctly.
>
> > I'd prefer an enum.  That way the schema can enforce that it's correct.
>
> Great, I added both choices and enum.  Everything works, and the
> schema enforcement is great, but here's one thing I'm not sure about:
> for the parent class "AccessibilityControl", controlType is an enum
> with 8 possible values.  For a subclass like "AccessibilityButton", I
> made controlType into an enum with only one possible value:
> "AccessibilityButton". Does that make sense to you?
>
> It's uploaded now, take a look when you have a chance.
>
> - Dominic
>
> http://codereview.chromium.org/402099
>

Sorry, I think I wasn't clear enough.  I was thinking more of "has-a"
composition:

+    "types": [
+      {
+        "id": "AccessibilityControl",
+        "type": "object",
+        "description": "Parent class for accessibility information about
the current focused control.",
+        "properties": {
+          "controlType": {
+            "type": "string",
+            "description": "The type of this control.",
+            "enum": [
+              "AccessibilityControl",
+              "AccessibilityButton",
+              "AccessibilityLink",
+              "AccessibilityRadioButton",
+              "AccessibilityCheckbox",
+              "AccessibilityTab",
+              "AccessibilityComboBox",
+              "AccessibilityTextBox"
+            ]
+          },
+          {
+            "name": "control",
+            "description": "Details of the currently focused control.",
+            "choices": [
+              { "$ref": "AccessibilityControl" },
+              { "$ref": "AccessibilityButton" },
+              { "$ref": "AccessibilityLink" },
+              { "$ref": "AccessibilityRadioButton" },
+              { "$ref": "AccessibilityCheckbox" },
+              { "$ref": "AccessibilityTab" },
+              { "$ref": "AccessibilityComboBox" },
+              { "$ref": "AccessibilityTextBox" },
+            ]
+          }
+          "controlName": {"type": "string", "description": "The name of the
control, like OK or Password."},
+        }
+      },

Then for the methods, you just take an AccessibilityControl and for the
subclasses, you don't need the type or name.  The downside of this approach
is that methods where it only makes sense to have a subset of the control
types wouldn't get enforced.  It looks like this would only affect
onTextChanged.

Erik

[dmazzoni at google, 2010-01-13 23:01:59]

On Wed, Jan 13, 2010 at 2:19 PM, Erik Kay <erikkay@chromium.org> wrote:
> Sorry, I think I wasn't clear enough.  I was thinking more of "has-a"
> composition:

Ah, ok.  Check it out now, is this more like what you had in mind?

- Dominic

http://codereview.chromium.org/402099

[Erik does not do reviews, 2010-01-13 23:10:39]

http://codereview.chromium.org/402099/diff/24001/24007
File chrome/browser/extensions/extension_accessibility_api.h (right):

http://codereview.chromium.org/402099/diff/24001/24007#newcode37
chrome/browser/extensions/extension_accessibility_api.h:37: void
AccessibilityOnOpenWindow(const AccessibilityWindowInfo* details);
comment above these methods.  Any reason why they all need "Accessibility" in
front of them?  They're already in a class with that name, so I'd assume that's
clear.

http://codereview.chromium.org/402099/diff/24001/24007#newcode50
chrome/browser/extensions/extension_accessibility_api.h:50: // Registered
profiles.
what it the map key?  add this to the comment.

http://codereview.chromium.org/402099/diff/24001/24007#newcode57
chrome/browser/extensions/extension_accessibility_api.h:57: class
GetFocusedControlFunction : public SyncExtensionFunction {
comments above all classes

http://codereview.chromium.org/402099/diff/24001/24007#newcode58
chrome/browser/extensions/extension_accessibility_api.h:58:
~GetFocusedControlFunction() {}
both of these destructors should be virtual

http://codereview.chromium.org/402099/diff/24001/24026
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/402099/diff/24001/24026#newcode222
chrome/common/extensions/api/extension_api.json:222: "controlType": {"type":
"string", "description": "Will be 'Button'."},
if you add a "choices" field to "Control", then you don't have to replicate
these fields in each of the sub-types.

http://codereview.chromium.org/402099/diff/24001/24029
File chrome/common/notification_type.h (right):

http://codereview.chromium.org/402099/diff/24001/24029#newcode802
chrome/common/notification_type.h:802: ACCESSIBILITY_ONOPENWINDOW,
naming nits:
- remove "ON" from the notification name
- put "_" between each word
- object first, verb second
- always include verb (ACCESSIBILITY_TEXT_CHANGED)

e.g. ACCESSIBILITY_WINDOW_OPENED

http://codereview.chromium.org/402099/diff/24001/24003
File chrome/test/data/extensions/api_test/accessibility/test.js (right):

http://codereview.chromium.org/402099/diff/24001/24003#newcode26
chrome/test/data/extensions/api_test/accessibility/test.js:26:
setTimeout(chrome.test.fail, 5000);
this isn't necessary.  the tests have a built-in timeout

http://codereview.chromium.org/402099/diff/28001/25019
File chrome/browser/extensions/extension_accessibility_api.cc (right):

http://codereview.chromium.org/402099/diff/28001/25019#newcode24
chrome/browser/extensions/extension_accessibility_api.cc:24: std::string
ControlInfoToJsonString(const AccessibilityControlInfo* info) {
add comment to this method ("encodes as an array with a single dictionary
member...")

http://codereview.chromium.org/402099/diff/28001/25019#newcode41
chrome/browser/extensions/extension_accessibility_api.cc:41: // TODO: how could
we return "null" from getFocusedControl instead of
nit: newlines above comments
nit: TODO(dmazzoni): How...

http://codereview.chromium.org/402099/diff/28001/25019#newcode42
chrome/browser/extensions/extension_accessibility_api.cc:42: // needing to have
a "unknown" value populated?
"null" is a legal value in JSON, why can't you just use that?

http://codereview.chromium.org/402099/diff/28001/25019#newcode73
chrome/browser/extensions/extension_accessibility_api.cc:73: if
(profiles_.find(source.map_key()) == profiles_.end()) {
nit: remove {} around one-line blocks

http://codereview.chromium.org/402099/diff/28001/25019#newcode106
chrome/browser/extensions/extension_accessibility_api.cc:106: args.Append(new
StringValue(info->name()));
why not just pass the whole object?  maybe name is the only useful field now,
but we may add more in the future.  Plus this would make the API more consistent
with the other events.

http://codereview.chromium.org/402099/diff/28001/25019#newcode161
chrome/browser/extensions/extension_accessibility_api.cc:161: bool
SimulateKeyPressFunction::RunImpl() {
I'm definitely nervous about this one since it leads to platform-dependent
extension code.  Maybe we could come up with a version that takes a char-code
and encourage its use ahead of this one?  There will always be cases where
key-code is necessary, but it would be good if they were more rare.

http://codereview.chromium.org/402099/diff/28001/25017
File chrome/browser/extensions/extension_accessibility_api_constants.cc (right):

http://codereview.chromium.org/402099/diff/28001/25017#newcode25
chrome/browser/extensions/extension_accessibility_api_constants.cc:25: const
char kOnOpenWindow[] = "experimental.accessibility.onOpenWindow";
event names should be onNounVerbed (onWindowOpened)

http://codereview.chromium.org/402099/diff/28001/25017#newcode29
chrome/browser/extensions/extension_accessibility_api_constants.cc:29: const
char kOnText[] = "experimental.accessibility.onText";
this should be onTextChanged

http://codereview.chromium.org/402099/diff/28001/25035
File chrome/browser/profile.h (right):

http://codereview.chromium.org/402099/diff/28001/25035#newcode391
chrome/browser/profile.h:391: void ResumeAccessibilityEvents() {
DCHECK that it doesn't go negative

http://codereview.chromium.org/402099/diff/28001/25035#newcode404
chrome/browser/profile.h:404: int accessibility_pause_level_;
add comment to the variable. I would change "level" to "count".

http://codereview.chromium.org/402099/diff/28001/25023
File chrome/browser/views/location_bar_view.cc (right):

http://codereview.chromium.org/402099/diff/28001/25023#newcode496
chrome/browser/views/location_bar_view.cc:496: details.name = "LocationBar";
If these are a fixed set of well-known names, then it seems like they should be
an enum as well, both for doc and for validation purposes.

http://codereview.chromium.org/402099/diff/28001/25038
File chrome/common/accessibility_events.h (right):

http://codereview.chromium.org/402099/diff/28001/25038#newcode16
chrome/common/accessibility_events.h:16: void SendAccessibilityNotification(
comment

http://codereview.chromium.org/402099/diff/28001/25038#newcode37
chrome/common/accessibility_events.h:37: // The name of the control, like "OK"
or "Password".
nit: newline above comment

http://codereview.chromium.org/402099/diff/28001/25038#newcode41
chrome/common/accessibility_events.h:41: class AccessibilityWindowInfo : public
AccessibilityControlInfo {
all classes need comments

http://codereview.chromium.org/402099/diff/28001/25037
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/402099/diff/28001/25037#newcode449
chrome/common/extensions/api/extension_api.json:449: "name": "onText",
this should be onTextChanged

[Erik does not do reviews, 2010-01-13 23:12:44]

On Wed, Jan 13, 2010 at 3:01 PM, Dominic Mazzoni <dmazzoni@google.com>wrote:

> On Wed, Jan 13, 2010 at 2:19 PM, Erik Kay <erikkay@chromium.org> wrote:
> > Sorry, I think I wasn't clear enough.  I was thinking more of "has-a"
> > composition:
>
> Ah, ok.  Check it out now, is this more like what you had in mind?
>

Yep.  That's it.  Thanks!  I also finally finished the rest of the review,
so now there's more for you to do. :)


>
> - Dominic
>
> http://codereview.chromium.org/402099
>

[dmazzoni at google, 2010-01-13 23:31:00]

On Wed, Jan 13, 2010 at 3:12 PM, Erik Kay <erikkay@chromium.org> wrote:
> Yep.  That's it.  Thanks!  I also finally finished the rest of the review,
> so now there's more for you to do. :)

Thanks!

The only thing I don't quite understand is the concern about the key
code: aren't key codes supposed to be cross-platform? I know there are
a lot of differences in key events on different platforms and between
different browsers, but I really thought that all of the standard key
codes you'd need to interact with the browser (printable ascii, tab,
enter, arrow keys, etc.) are the same on all platforms. If not, what's
an example of a difference?

Maybe the problem is that my current implementation is passing the key
code as-is to gtk, which may not work for many corner cases? I think
this should be passed through a routine to convert JavaScript standard
key codes to platform-specific codes transparently.

- Dominic

[Erik does not do reviews, 2010-01-14 00:00:14]

On Wed, Jan 13, 2010 at 3:30 PM, Dominic Mazzoni <dmazzoni@google.com>wrote:

> On Wed, Jan 13, 2010 at 3:12 PM, Erik Kay <erikkay@chromium.org> wrote:
> > Yep.  That's it.  Thanks!  I also finally finished the rest of the
> review,
> > so now there's more for you to do. :)
>
> Thanks!
>
> The only thing I don't quite understand is the concern about the key
> code: aren't key codes supposed to be cross-platform? I know there are
> a lot of differences in key events on different platforms and between
> different browsers, but I really thought that all of the standard key
> codes you'd need to interact with the browser (printable ascii, tab,
> enter, arrow keys, etc.) are the same on all platforms. If not, what's
> an example of a difference?
>

Take a look at keyboard_code_conversion_gtk.cc.


Maybe the problem is that my current implementation is passing the key
> code as-is to gtk, which may not work for many corner cases? I think
> this should be passed through a routine to convert JavaScript standard
> key codes to platform-specific codes transparently.


To be honest, I don't think that JS keycodes are standardized either,
although perhaps they are across different versions of Chrome.  This isn't
something I know a lot about.  I just know that keycode handling has
historically been very inconsistent across browsers and OSs.  There are a
few rants floating around the net about this:
http://unixpapa.com/js/key.html

I'm not sure who the right person is to ask about this.  Maybe Darin?

Erik

[dmazzoni at google, 2010-01-14 17:24:22]

On Wed, Jan 13, 2010 at 3:12 PM, Erik Kay <erikkay@chromium.org> wrote:
> Yep.  That's it.  Thanks!  I also finally finished the rest of the review,
> so now there's more for you to do. :)

I'm stuck trying to get this to work, I'm getting this error:

[11826:11826:90742389481:FATAL:chrome/renderer/extensions/event_bindings.cc(308)]
Check failed: false. Event validation error during
experimental.accessibility.onControlFocus -- TypeError: Cannot use
'in' operator to search for 'isPassword' in undefined

Here's the JSON it's trying to validate:

{"details":{"isPassword":false,"selectionEnd":0,"selectionStart":0,"value":""},"name":"LocationBar","type":"textbox"}

The error is being triggered by v8/src/runtime.js, but I'm not sure
how to get a stack trace.  Any idea what I'm doing wrong, or any tips
on how to debug it?

- Dominic

[dmazzoni at google, 2010-01-14 17:31:08]

On Thu, Jan 14, 2010 at 9:24 AM, Dominic Mazzoni <dmazzoni@google.com> wrote:
> On Wed, Jan 13, 2010 at 3:12 PM, Erik Kay <erikkay@chromium.org> wrote:
>> Yep.  That's it.  Thanks!  I also finally finished the rest of the review,
>> so now there's more for you to do. :)
>
> I'm stuck trying to get this to work, I'm getting this error:

To clarify, this problem happened after switching to "has-a"
composition...I think I'm missing some detail in the json schema but I
don't know what.

- Dominic

>
[11826:11826:90742389481:FATAL:chrome/renderer/extensions/event_bindings.cc(308)]
> Check failed: false. Event validation error during
> experimental.accessibility.onControlFocus -- TypeError: Cannot use
> 'in' operator to search for 'isPassword' in undefined
>
> Here's the JSON it's trying to validate:
>
>
{"details":{"isPassword":false,"selectionEnd":0,"selectionStart":0,"value":""},"name":"LocationBar","type":"textbox"}
>
> The error is being triggered by v8/src/runtime.js, but I'm not sure
> how to get a stack trace.  Any idea what I'm doing wrong, or any tips
> on how to debug it?
>
> - Dominic
>

[dmazzoni at google, 2010-01-14 18:01:47]

On Thu, Jan 14, 2010 at 9:30 AM, Dominic Mazzoni <dmazzoni@google.com> wrote:
> To clarify, this problem happened after switching to "has-a"
> composition...I think I'm missing some detail in the json schema but I
> don't know what.

Thanks to Rafael for finding my mistake!  It works now, and I'll
update again when I've finished addressing the rest of the comments.

- Dominic

[Evan Stade, 2010-01-14 20:10:58]

I request more comments

http://codereview.chromium.org/402099/diff/29001/30022
File chrome/browser/gtk/accessibility_events_gtk.cc (right):

http://codereview.chromium.org/402099/diff/29001/30022#newcode1
chrome/browser/gtk/accessibility_events_gtk.cc:1: // Copyright (c) 2010 The
Chromium Authors. All rights reserved.
filename should be closer to the classname, i.e. accessibility_event_router_gtk

http://codereview.chromium.org/402099/diff/29001/30022#newcode14
chrome/browser/gtk/accessibility_events_gtk.cc:14: gboolean
OnRadioButtonFocused(GtkToggleButton* toggle_button,
these should be in an anonymous namespace

http://codereview.chromium.org/402099/diff/29001/30022#newcode216
chrome/browser/gtk/accessibility_events_gtk.cc:216: // Connect up any automatic
buttons in the dialog's "action area"
final .

http://codereview.chromium.org/402099/diff/29001/30022#newcode220
chrome/browser/gtk/accessibility_events_gtk.cc:220: while (child_iter) {
this looks an awful lot like a for loop...

http://codereview.chromium.org/402099/diff/29001/30012
File chrome/browser/gtk/accessibility_events_gtk.h (right):

http://codereview.chromium.org/402099/diff/29001/30012#newcode17
chrome/browser/gtk/accessibility_events_gtk.h:17: class AccessibilityEventRouter
{
comments on this class

http://codereview.chromium.org/402099/diff/29001/30017
File chrome/browser/gtk/options/general_page_gtk.cc (right):

http://codereview.chromium.org/402099/diff/29001/30017#newcode321
chrome/browser/gtk/options/general_page_gtk.cc:321:
accessibility_event_router_.ConnectCheckbox(homepage_show_home_button_checkbox_);
80 char width

http://codereview.chromium.org/402099/diff/29001/30017#newcode360
chrome/browser/gtk/options/general_page_gtk.cc:360:
accessibility_event_router_.ConnectButton(default_search_manage_engines_button_);
80

http://codereview.chromium.org/402099/diff/29001/30020
File chrome/browser/gtk/options/options_window_gtk.cc (right):

http://codereview.chromium.org/402099/diff/29001/30020#newcode215
chrome/browser/gtk/options/options_window_gtk.cc:215:
profile->PauseAccessibilityEvents();
comments please. Why do you need to pause?

http://codereview.chromium.org/402099/diff/29001/30020#newcode232
chrome/browser/gtk/options/options_window_gtk.cc:232:
NotificationType::ACCESSIBILITY_ONOPENWINDOW,
i believe the notification naming style would be ACCESSIBILITY_ON_OPEN_WINDOW

[dmazzoni at google, 2010-01-15 17:30:24]

On Wed, Jan 13, 2010 at 3:10 PM,  <erikkay@chromium.org> wrote:
>
> http://codereview.chromium.org/402099/diff/24001/24007
> File chrome/browser/extensions/extension_accessibility_api.h (right):
>
> http://codereview.chromium.org/402099/diff/24001/24007#newcode37
> chrome/browser/extensions/extension_accessibility_api.h:37: void
> AccessibilityOnOpenWindow(const AccessibilityWindowInfo* details);
> comment above these methods.  Any reason why they all need
> "Accessibility" in front of them?  They're already in a class with that
> name, so I'd assume that's clear.

Removed.

> http://codereview.chromium.org/402099/diff/24001/24007#newcode50
> chrome/browser/extensions/extension_accessibility_api.h:50: //
> Registered profiles.
> what it the map key?  add this to the comment.

Done

> http://codereview.chromium.org/402099/diff/24001/24007#newcode57
> chrome/browser/extensions/extension_accessibility_api.h:57: class
> GetFocusedControlFunction : public SyncExtensionFunction {
> comments above all classes

Done.

> http://codereview.chromium.org/402099/diff/24001/24007#newcode58
> chrome/browser/extensions/extension_accessibility_api.h:58:
> ~GetFocusedControlFunction() {}
> both of these destructors should be virtual

Done

> http://codereview.chromium.org/402099/diff/24001/24026
> File chrome/common/extensions/api/extension_api.json (right):
>
> http://codereview.chromium.org/402099/diff/24001/24026#newcode222
> chrome/common/extensions/api/extension_api.json:222: "controlType":
> {"type": "string", "description": "Will be 'Button'."},
> if you add a "choices" field to "Control", then you don't have to
> replicate these fields in each of the sub-types.

Looks much cleaner now!

> http://codereview.chromium.org/402099/diff/24001/24029
> File chrome/common/notification_type.h (right):
>
> http://codereview.chromium.org/402099/diff/24001/24029#newcode802
> chrome/common/notification_type.h:802: ACCESSIBILITY_ONOPENWINDOW,
> naming nits:
> - remove "ON" from the notification name
> - put "_" between each word
> - object first, verb second
> - always include verb (ACCESSIBILITY_TEXT_CHANGED)
>
> e.g. ACCESSIBILITY_WINDOW_OPENED

Changed throughout.

I'm happy with all except ON_CONTROL_ACTION - is there a verb that
applies to pressing a button, toggling a checkbox, or changing the
value of a combo box?

> http://codereview.chromium.org/402099/diff/24001/24003
> File chrome/test/data/extensions/api_test/accessibility/test.js (right):
>
> http://codereview.chromium.org/402099/diff/24001/24003#newcode26
> chrome/test/data/extensions/api_test/accessibility/test.js:26:
> setTimeout(chrome.test.fail, 5000);
> this isn't necessary.  the tests have a built-in timeout

Removed.  It must be a really long timeout by default in debug mode,
because I waited a while and assumed it would never exit!  :)

> http://codereview.chromium.org/402099/diff/28001/25019
> File chrome/browser/extensions/extension_accessibility_api.cc (right):
>
> http://codereview.chromium.org/402099/diff/28001/25019#newcode24
> chrome/browser/extensions/extension_accessibility_api.cc:24: std::string
> ControlInfoToJsonString(const AccessibilityControlInfo* info) {
> add comment to this method ("encodes as an array with a single
> dictionary member...")

Done.

> http://codereview.chromium.org/402099/diff/28001/25019#newcode41
> chrome/browser/extensions/extension_accessibility_api.cc:41: // TODO:
> how could we return "null" from getFocusedControl instead of
> nit: newlines above comments
> nit: TODO(dmazzoni): How...
>
> http://codereview.chromium.org/402099/diff/28001/25019#newcode42
> chrome/browser/extensions/extension_accessibility_api.cc:42: // needing
> to have a "unknown" value populated?
> "null" is a legal value in JSON, why can't you just use that?

I can return null if I mark the argument to the callback in
getFocusedControl as optional. Here's the problem if it's not
optional: if you try to return Value::CreateNullValue() from an api
function that returns its result via a callback, then 'null' gets
turned into an empty list in extension_process_bindings.js:80: "var
callbackArgs = response ? [JSON.parse(response)] : [];" and then
json_schema.js complains because one argument is required to the
callback function.

I went ahead and made that callback parameter optional. What do you think?

> http://codereview.chromium.org/402099/diff/28001/25019#newcode73
> chrome/browser/extensions/extension_accessibility_api.cc:73: if
> (profiles_.find(source.map_key()) == profiles_.end()) {
> nit: remove {} around one-line blocks

Done

> http://codereview.chromium.org/402099/diff/28001/25019#newcode106
> chrome/browser/extensions/extension_accessibility_api.cc:106:
> args.Append(new StringValue(info->name()));
> why not just pass the whole object?  maybe name is the only useful field
> now, but we may add more in the future.  Plus this would make the API
> more consistent with the other events.

Sure, it's more consistent now.

> http://codereview.chromium.org/402099/diff/28001/25019#newcode161
> chrome/browser/extensions/extension_accessibility_api.cc:161: bool
> SimulateKeyPressFunction::RunImpl() {
> I'm definitely nervous about this one since it leads to
> platform-dependent extension code.  Maybe we could come up with a
> version that takes a char-code and encourage its use ahead of this one?
> There will always be cases where key-code is necessary, but it would be
> good if they were more rare.

I've decided to take automation out of this changelist so that we can
finish this one and get it submitted while still debating automation.

> http://codereview.chromium.org/402099/diff/28001/25017
> File chrome/browser/extensions/extension_accessibility_api_constants.cc
> (right):
>
> http://codereview.chromium.org/402099/diff/28001/25017#newcode25
> chrome/browser/extensions/extension_accessibility_api_constants.cc:25:
> const char kOnOpenWindow[] = "experimental.accessibility.onOpenWindow";
> event names should be onNounVerbed (onWindowOpened)

Done

> http://codereview.chromium.org/402099/diff/28001/25017#newcode29
> chrome/browser/extensions/extension_accessibility_api_constants.cc:29:
> const char kOnText[] = "experimental.accessibility.onText";
> this should be onTextChanged
>
> http://codereview.chromium.org/402099/diff/28001/25035
> File chrome/browser/profile.h (right):
>
> http://codereview.chromium.org/402099/diff/28001/25035#newcode391
> chrome/browser/profile.h:391: void ResumeAccessibilityEvents() {
> DCHECK that it doesn't go negative

Done

> http://codereview.chromium.org/402099/diff/28001/25035#newcode404
> chrome/browser/profile.h:404: int accessibility_pause_level_;
> add comment to the variable. I would change "level" to "count".

I think "level" is more correct.  Counts don't tend to go up and down
normally.  See if it's more clear with the comments.

If you still don't like "level", maybe we can find a third term.

> http://codereview.chromium.org/402099/diff/28001/25023
> File chrome/browser/views/location_bar_view.cc (right):
>
> http://codereview.chromium.org/402099/diff/28001/25023#newcode496
> chrome/browser/views/location_bar_view.cc:496: details.name =
> "LocationBar";
> If these are a fixed set of well-known names, then it seems like they
> should be an enum as well, both for doc and for validation purposes.

So, the vast majority of controls will get their name directly from
the translatable string used for the label on the control in the GUI.
The exception will be a small number of controls like the Location Bar
that don't have a visible label or tooltip containing their name. I'm
not sure I want an accessibility extension to treat those any
differently for that reason - if a control acts like a TextBox they
should treat it like every other TextBox.

Now, if the LocationBar has different semantics, then we should give
it its own control type.  But I don't think the control _name_ should
ever be an enum, because we'd have to update the api constantly every
time we add a new control, forcing extension authors to catch up.

I fixed the code by making this a string constant rather than hard-coded.

> http://codereview.chromium.org/402099/diff/28001/25038
> File chrome/common/accessibility_events.h (right):
>
> http://codereview.chromium.org/402099/diff/28001/25038#newcode16
> chrome/common/accessibility_events.h:16: void
> SendAccessibilityNotification(
> comment

Done

> http://codereview.chromium.org/402099/diff/28001/25038#newcode37
> chrome/common/accessibility_events.h:37: // The name of the control,
> like "OK" or "Password".
> nit: newline above comment

Done

> http://codereview.chromium.org/402099/diff/28001/25038#newcode41
> chrome/common/accessibility_events.h:41: class AccessibilityWindowInfo :
> public AccessibilityControlInfo {
> all classes need comments

All commented now.

> http://codereview.chromium.org/402099/diff/28001/25037
> File chrome/common/extensions/api/extension_api.json (right):
>
> http://codereview.chromium.org/402099/diff/28001/25037#newcode449
> chrome/common/extensions/api/extension_api.json:449: "name": "onText",
> this should be onTextChanged

Fixed.

> http://codereview.chromium.org/402099

[dmazzoni at google, 2010-01-15 17:41:02]

On Thu, Jan 14, 2010 at 12:10 PM,  <estade@chromium.org> wrote:
> I request more comments

I fully commented accessibility_event_router_gtk.h and
accessibility_event_router_gtk.cc and the other places mentioned.

> http://codereview.chromium.org/402099/diff/29001/30022
> File chrome/browser/gtk/accessibility_events_gtk.cc (right):
>
> http://codereview.chromium.org/402099/diff/29001/30022#newcode1
> chrome/browser/gtk/accessibility_events_gtk.cc:1: // Copyright (c) 2010
> The Chromium Authors. All rights reserved.
> filename should be closer to the classname, i.e.
> accessibility_event_router_gtk

Changed.

> http://codereview.chromium.org/402099/diff/29001/30022#newcode14
> chrome/browser/gtk/accessibility_events_gtk.cc:14: gboolean
> OnRadioButtonFocused(GtkToggleButton* toggle_button,
> these should be in an anonymous namespace

Done.

> http://codereview.chromium.org/402099/diff/29001/30022#newcode216
> chrome/browser/gtk/accessibility_events_gtk.cc:216: // Connect up any
> automatic buttons in the dialog's "action area"
> final .

Final punctuation added; hopefully everywhere if I was careful.

> http://codereview.chromium.org/402099/diff/29001/30022#newcode220
> chrome/browser/gtk/accessibility_events_gtk.cc:220: while (child_iter) {
> this looks an awful lot like a for loop...

Sure.

> http://codereview.chromium.org/402099/diff/29001/30012
> File chrome/browser/gtk/accessibility_events_gtk.h (right):
>
> http://codereview.chromium.org/402099/diff/29001/30012#newcode17
> chrome/browser/gtk/accessibility_events_gtk.h:17: class
> AccessibilityEventRouter {
> comments on this class

Added.

> http://codereview.chromium.org/402099/diff/29001/30017
> File chrome/browser/gtk/options/general_page_gtk.cc (right):
>
> http://codereview.chromium.org/402099/diff/29001/30017#newcode321
> chrome/browser/gtk/options/general_page_gtk.cc:321:
>
accessibility_event_router_.ConnectCheckbox(homepage_show_home_button_checkbox_);
> 80 char width
>
> http://codereview.chromium.org/402099/diff/29001/30017#newcode360
> chrome/browser/gtk/options/general_page_gtk.cc:360:
>
accessibility_event_router_.ConnectButton(default_search_manage_engines_button_);
> 80

Fixed both.  (Do the lint checks not catch this?)

> http://codereview.chromium.org/402099/diff/29001/30020
> File chrome/browser/gtk/options/options_window_gtk.cc (right):
>
> http://codereview.chromium.org/402099/diff/29001/30020#newcode215
> chrome/browser/gtk/options/options_window_gtk.cc:215:
> profile->PauseAccessibilityEvents();
> comments please. Why do you need to pause?

Added more comments and made the scope even smaller.

> http://codereview.chromium.org/402099/diff/29001/30020#newcode232
> chrome/browser/gtk/options/options_window_gtk.cc:232:
> NotificationType::ACCESSIBILITY_ONOPENWINDOW,
> i believe the notification naming style would be
> ACCESSIBILITY_ON_OPEN_WINDOW

I renamed most of these based on Erik's suggestions.

> http://codereview.chromium.org/402099

[Evan Stade, 2010-01-15 20:42:35]

http://codereview.chromium.org/402099/diff/20014/20025
File chrome/browser/gtk/accessibility_event_router_gtk.h (right):

http://codereview.chromium.org/402099/diff/20014/20025#newcode27
chrome/browser/gtk/accessibility_event_router_gtk.h:27: // dialog does.
thanks, this makes it really clear.

A concern I have with this class is that certain widgets in the UI have signal
handlers attached to provide special behavior, which may return TRUE in order to
stop the default handler. This will also prevent the signal being sent to the
AccessibilityEventRouter, so it's important to make sure this class connects
first. You should note that in the comment here.

Perhaps to avoid hard-to-find bugs, you can make Connect* return the widget it
was passed to allow the pattern:

GtkWidget* button = a11y_event_router_.ConnectButton(gtk_button_new());

or maybe make some convenience functions such as

GtkWidget* button = a11y_event_router_.NewAccessibleButton();

http://codereview.chromium.org/402099/diff/20014/20025#newcode41
chrome/browser/gtk/accessibility_event_router_gtk.h:41: // Connect to a
checkbox, but using the given string_id instead of
nit: variable names in comments go inside |s

[Erik does not do reviews, 2010-01-15 21:47:55]

In the future, please reply to comments using Rietveld's inline reply feature
rather than using email.  It's much harder to have a conversation this way (and
it's too easy to lose your work like I just did).

On 2010/01/15 17:30:24, Dominic Mazzoni wrote:
> > http://codereview.chromium.org/402099/diff/24001/24029
> > File chrome/common/notification_type.h (right):
> >
> > http://codereview.chromium.org/402099/diff/24001/24029#newcode802
> > chrome/common/notification_type.h:802: ACCESSIBILITY_ONOPENWINDOW,
> > naming nits:
> > - remove "ON" from the notification name
> > - put "_" between each word
> > - object first, verb second
> > - always include verb (ACCESSIBILITY_TEXT_CHANGED)
> >
> > e.g. ACCESSIBILITY_WINDOW_OPENED
> 
> Changed throughout.
> 
> I'm happy with all except ON_CONTROL_ACTION - is there a verb that
> applies to pressing a button, toggling a checkbox, or changing the
> value of a combo box?

It's OK with me, but if you'd prefer a stronger verb, try
ON_CONTROL_ACTION_PERFORMED.


> > http://codereview.chromium.org/402099/diff/24001/24003
> > File chrome/test/data/extensions/api_test/accessibility/test.js (right):
> >
> > http://codereview.chromium.org/402099/diff/24001/24003#newcode26
> > chrome/test/data/extensions/api_test/accessibility/test.js:26:
> > setTimeout(chrome.test.fail, 5000);
> > this isn't necessary.  the tests have a built-in timeout
> 
> Removed.  It must be a really long timeout by default in debug mode,
> because I waited a while and assumed it would never exit!  :)

I think it's 30 seconds.  If it isn't working, let me know.

> > http://codereview.chromium.org/402099/diff/28001/25019#newcode41
> > chrome/browser/extensions/extension_accessibility_api.cc:41: // TODO:
> > how could we return "null" from getFocusedControl instead of
> > nit: newlines above comments
> > nit: TODO(dmazzoni): How...
> >
> > http://codereview.chromium.org/402099/diff/28001/25019#newcode42
> > chrome/browser/extensions/extension_accessibility_api.cc:42: // needing
> > to have a "unknown" value populated?
> > "null" is a legal value in JSON, why can't you just use that?
> 
> I can return null if I mark the argument to the callback in
> getFocusedControl as optional. Here's the problem if it's not
> optional: if you try to return Value::CreateNullValue() from an api
> function that returns its result via a callback, then 'null' gets
> turned into an empty list in extension_process_bindings.js:80: "var
> callbackArgs = response ? [JSON.parse(response)] : [];" and then
> json_schema.js complains because one argument is required to the
> callback function.
> 
> I went ahead and made that callback parameter optional. What do you think?

That seems OK to me.  Can you check to see what Rafael thinks?



> > http://codereview.chromium.org/402099/diff/28001/25019#newcode161
> > chrome/browser/extensions/extension_accessibility_api.cc:161: bool
> > SimulateKeyPressFunction::RunImpl() {
> > I'm definitely nervous about this one since it leads to
> > platform-dependent extension code.  Maybe we could come up with a
> > version that takes a char-code and encourage its use ahead of this one?
> > There will always be cases where key-code is necessary, but it would be
> > good if they were more rare.
> 
> I've decided to take automation out of this changelist so that we can
> finish this one and get it submitted while still debating automation.

Good idea.

> > http://codereview.chromium.org/402099/diff/28001/25035#newcode404
> > chrome/browser/profile.h:404: int accessibility_pause_level_;
> > add comment to the variable. I would change "level" to "count".
> 
> I think "level" is more correct.  Counts don't tend to go up and down
> normally.  See if it's more clear with the comments.
> 
> If you still don't like "level", maybe we can find a third term.

With the comment, it's OK to me.


> > http://codereview.chromium.org/402099/diff/28001/25023
> > File chrome/browser/views/location_bar_view.cc (right):
> >
> > http://codereview.chromium.org/402099/diff/28001/25023#newcode496
> > chrome/browser/views/location_bar_view.cc:496: details.name =
> > "LocationBar";
> > If these are a fixed set of well-known names, then it seems like they
> > should be an enum as well, both for doc and for validation purposes.
> 
> So, the vast majority of controls will get their name directly from
> the translatable string used for the label on the control in the GUI.
> The exception will be a small number of controls like the Location Bar
> that don't have a visible label or tooltip containing their name. I'm
> not sure I want an accessibility extension to treat those any
> differently for that reason - if a control acts like a TextBox they
> should treat it like every other TextBox.
> 
> Now, if the LocationBar has different semantics, then we should give
> it its own control type.  But I don't think the control _name_ should
> ever be an enum, because we'd have to update the api constantly every
> time we add a new control, forcing extension authors to catch up.
> 
> I fixed the code by making this a string constant rather than hard-coded.

Given this, I don't see how extension authors are supposed to use this API.  How
do they test to see if you're on a particular control?  They'd need to know all
of the different language translations.  Or is this meant to be treated opaquely
(for screen reader purposes)?  If so, then you'll need to change the code to use
pull the string out of localized data rather than a hard-coded string constant.

Erik

[Erik does not do reviews, 2010-01-15 21:48:04]

http://codereview.chromium.org/402099/diff/20014/20021
File chrome/browser/extensions/extension_accessibility_api.cc (right):

http://codereview.chromium.org/402099/diff/20014/20021#newcode112
chrome/browser/extensions/extension_accessibility_api.cc:112:
printf("onControlFocus: %s\n", json_args.c_str());
if you want to keep this, switch to use LOG

http://codereview.chromium.org/402099/diff/20014/20021#newcode147
chrome/browser/extensions/extension_accessibility_api.cc:147:
printf("GetFocusedControlFunction::RunImpl returning junk\n");
ditto

http://codereview.chromium.org/402099/diff/20014/20020
File chrome/browser/extensions/extension_accessibility_api.h (right):

http://codereview.chromium.org/402099/diff/20014/20020#newcode56
chrome/browser/extensions/extension_accessibility_api.h:56: // Registered
profiles - map from the NotificationSource map_key to
sorry, maybe I'm being dense, but I still don't know what the NotificationSource
map_key is.  What are you trying to do here?

[dmazzoni at google, 2010-01-19 19:08:56]

One response for Evan:

http://codereview.chromium.org/402099/diff/20014/20025
File chrome/browser/gtk/accessibility_event_router_gtk.h (right):

http://codereview.chromium.org/402099/diff/20014/20025#newcode27
chrome/browser/gtk/accessibility_event_router_gtk.h:27: // dialog does.
On 2010/01/15 20:42:36, Evan Stade wrote:
> A concern I have with this class is that certain widgets in the UI have signal
> handlers attached to provide special behavior, which may return TRUE in order
to
> stop the default handler. This will also prevent the signal being sent to the
> AccessibilityEventRouter, so it's important to make sure this class connects
> first. You should note that in the comment here.

Good catch...

Or, could I make it even more foolproof by disconnecting any existing signal
handlers, then reconnecting them after adding the accessibility handlers?

It looks like g_signal_handler_find might let me find existing signal
handlers...

[Evan Stade, 2010-01-19 19:16:32]

I don't know that you'd be able to re-connect them though.

[dmazzoni at google, 2010-01-19 19:17:52]

And one for Erik:

On 2010/01/15 21:47:55, Erik Kay wrote:
> In the future, please reply to comments using Rietveld's inline reply feature
> rather than using email.  It's much harder to have a conversation this way
(and
> it's too easy to lose your work like I just did).

Oops, sorry!

> > Now, if the LocationBar has different semantics, then we should give
> > it its own control type.  But I don't think the control _name_ should
> > ever be an enum, because we'd have to update the api constantly every
> > time we add a new control, forcing extension authors to catch up.
> > 
> > I fixed the code by making this a string constant rather than hard-coded.
> 
> Given this, I don't see how extension authors are supposed to use this API. 
How
> do they test to see if you're on a particular control?  They'd need to know
all
> of the different language translations.  Or is this meant to be treated
opaquely
> (for screen reader purposes)?  If so, then you'll need to change the code to
use
> pull the string out of localized data rather than a hard-coded string
constant.

To a first approximation, yes, I expect an accessibility extension should treat
the names of controls opaquely - to read them aloud using text-to-speech, for
example, without interpreting anything other than the type of control.  I'll
make this a localized string.

However, you bring up a good point: supposing someone does want to check for a
specific control (and this may also be useful for unit testing) - should we
provide a unique name for each control that they could test for?  Rather than a
new enum (which would have to be updated every time a new control is added, and
would make this api enormous), it'd be nice if we could leverage any existing
unique id that's associated with each string.

The string ids aren't stable, are they?  What about returning the English
version of each name, in addition to the localized version?  That way the
English name could be used for unambiguously identifying a particular control.

I think we could live without this, though.

- Dominic

[dmazzoni at google, 2010-01-19 19:20:39]

On 2010/01/19 19:16:32, Evan Stade wrote:
> I don't know that you'd be able to re-connect them though.

Not sure. I'll write a quick test program. If it does work, I'll write a unit
test for this specifically. If not, I like your idea of returning GtkControl*
from the Connect methods.

[Erik does not do reviews, 2010-01-19 22:10:25]

On Tue, Jan 19, 2010 at 11:17 AM, <dmazzoni@google.com> wrote:

> And one for Erik:
>
>
> On 2010/01/15 21:47:55, Erik Kay wrote:
>
>> In the future, please reply to comments using Rietveld's inline reply
>> feature
>> rather than using email.  It's much harder to have a conversation this way
>>
> (and
>
>> it's too easy to lose your work like I just did).
>>
>
> Oops, sorry!
>
>
>  > Now, if the LocationBar has different semantics, then we should give
>> > it its own control type.  But I don't think the control _name_ should
>> > ever be an enum, because we'd have to update the api constantly every
>> > time we add a new control, forcing extension authors to catch up.
>> >
>> > I fixed the code by making this a string constant rather than
>> hard-coded.
>>
>
>  Given this, I don't see how extension authors are supposed to use this
>> API.
>>
> How
>
>> do they test to see if you're on a particular control?  They'd need to
>> know
>>
> all
>
>> of the different language translations.  Or is this meant to be treated
>>
> opaquely
>
>> (for screen reader purposes)?  If so, then you'll need to change the code
>> to
>>
> use
>
>> pull the string out of localized data rather than a hard-coded string
>>
> constant.
>
> To a first approximation, yes, I expect an accessibility extension should
> treat
> the names of controls opaquely - to read them aloud using text-to-speech,
> for
> example, without interpreting anything other than the type of control.
>  I'll
> make this a localized string.
>
> However, you bring up a good point: supposing someone does want to check
> for a
> specific control (and this may also be useful for unit testing) - should we
> provide a unique name for each control that they could test for?  Rather
> than a
> new enum (which would have to be updated every time a new control is added,
> and
> would make this api enormous), it'd be nice if we could leverage any
> existing
> unique id that's associated with each string.
>
> The string ids aren't stable, are they?  What about returning the English
> version of each name, in addition to the localized version?  That way the
> English name could be used for unambiguously identifying a particular
> control.
>
> I think we could live without this, though.
>

Unfortunately, I don't think we can count on the English name being stable
either.  I think we'd need an enum.  In the interest of keeping this change
short and landing it in finite time, I'm happy postponing that work though.
:)

In the meantime, I think the docs and API names may need to be clarified so
that users don't use these strings in this way.

Erik



>
> - Dominic
>
>
>
> http://codereview.chromium.org/402099
>

[dmazzoni at google, 2010-01-19 22:17:22]

On 2010/01/19 22:10:25, Erik Kay wrote:
> Unfortunately, I don't think we can count on the English name being stable
> either.  I think we'd need an enum.  In the interest of keeping this change
> short and landing it in finite time, I'm happy postponing that work though.
> :)
> 
> In the meantime, I think the docs and API names may need to be clarified so
> that users don't use these strings in this way.

Sounds good, perhaps I'll explicitly mark such strings as "localized" to make it
clear that you can't count on the value being the same on all systems.

[dmazzoni at google, 2010-01-19 22:37:03]

On 2010/01/19 19:16:32, Evan Stade wrote:
> I don't know that you'd be able to re-connect them though.

Another idea: what if I used g_signal_add_emission_hook to register a global,
generic callback for each type of event (focused, clicked, etc.)? Emission hooks
are called before signal handlers, even if registered after.  This would
eliminate the need for many of the calls to Connect after each widget is
constructed now - it'd only be used to provide extra information when needed.

[Evan Stade, 2010-01-19 22:59:06]

On Tue, Jan 19, 2010 at 2:37 PM, <dmazzoni@google.com> wrote:

> On 2010/01/19 19:16:32, Evan Stade wrote:
>
>> I don't know that you'd be able to re-connect them though.
>>
>
> Another idea: what if I used g_signal_add_emission_hook to register a
> global,
> generic callback for each type of event (focused, clicked, etc.)? Emission
> hooks
> are called before signal handlers, even if registered after. This would

eliminate the need for many of the calls to Connect after each widget is
> constructed now - it'd only be used to provide extra information when
> needed.


that sounds like a big win, but without a call to Connect, how would you
know if the instance should be an accessible control or not?


>
>
>
> http://codereview.chromium.org/402099
>

[dmazzoni at google, 2010-01-20 00:31:49]

On 2010/01/19 22:59:06, Evan Stade wrote:
> > Another idea: what if I used g_signal_add_emission_hook to register a
> 
> that sounds like a big win, but without a call to Connect, how would you
> know if the instance should be an accessible control or not?

How about: 1 call to connect for a single parent widget, which says "all
descendants of this widget should be accessible by default".

Then we provide functions to deal with the exceptional cases.

I'm trying this now.

[Evan Stade, 2010-01-20 00:50:34]

as long as the implementation doesn't have negative impact on performance, that
sounds ok

[dmazzoni at google, 2010-01-21 22:50:40]

Ready for another look.

Erik, I finished addressing all of your comments. The map of registered Profiles
wasn't needed (sorry, I borrowed this from a different extension api when I
first started and confused myself), and I found an existing localized string for
the location bar.

Evan, I finished refactoring everything based on global event hooks.

Previously my api_test relied on sending keyboard events in order to change the
focus; now I can't do this because we took keyboard automation out of this
changelist. Some ideas:
1. Commit this as-is without an api_test, but add the api_test soon once
keyboard automation is checked in.
2. Add a gtk-specific browser_test to fully test the api instead of an api_test.
3. Add a couple of very specific functions to the extension api, for example
focusLocationBar(), just enough to make a basic api_test possible.

Thoughts?

Thanks,
Dominic

[dmazzoni at google, 2010-01-26 22:28:47]

Re-synced and uploaded.

[Erik does not do reviews, 2010-01-27 01:09:23]

LGTM

Given the size of this CL and the fact that it's all in the experimental bucket,
my vote is for you to come back with the api_test in a later CL.

My gut says to wait until you have keyboard automation figured out rather than
adding one-off APIs, but I could be convinced the other way.

http://codereview.chromium.org/402099/diff/24072/35019
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/402099/diff/24072/35019#newcode220
chrome/common/extensions/api/extension_api.json:220: "description": "The name of
the object, like OK or Password."
This is the localized name, only suitable for screen readers or display right? 
Please add this to the description here.

[Evan Stade, 2010-01-27 01:35:15]

> Given the size of this CL and the fact that it's all in the experimental
bucket,
> my vote is for you to come back with the api_test in a later CL.

agreed

sorry I didn't see this. Quick question: can a user disable accessibility? All
those map enumerations look rather heavyweight for something that happens on
every event, whether the widget is accessible or not, whether the user has
accessibility on or not.

[dmazzoni at google, 2010-01-27 20:35:34]

http://codereview.chromium.org/402099/diff/24072/35019
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/402099/diff/24072/35019#newcode220
chrome/common/extensions/api/extension_api.json:220: "description": "The name of
the object, like OK or Password."
On 2010/01/27 01:09:25, Erik Kay wrote:
> This is the localized name, only suitable for screen readers or display right?

> Please add this to the description here.

Done.

[dmazzoni at google, 2010-01-27 20:39:33]

On 2010/01/27 01:35:15, Evan Stade wrote:
> sorry I didn't see this. Quick question: can a user disable accessibility? All
> those map enumerations look rather heavyweight for something that happens on
> every event, whether the widget is accessible or not, whether the user has
> accessibility on or not.

I didn't notice any slowdowns, but this is a good point.

To solve this, I added a new API function, setAccessibilityEnabled. Until an
extension sets that to true, no event handlers are registered and no
notifications are posted anywhere.  When you call
setAccessibilityEnabled(false), GTK removes the event emission hooks.

[Evan Stade, 2010-01-27 20:47:39]

thanks, LGTM!

http://codereview.chromium.org/402099/diff/24072/35009
File chrome/browser/gtk/accessibility_event_router_gtk.cc (right):

http://codereview.chromium.org/402099/diff/24072/35009#newcode24
chrome/browser/gtk/accessibility_event_router_gtk.cc:24:
((AccessibilityEventRouter *)user_data)->DispatchAccessibilityNotification(
nit: c++ style casts

http://codereview.chromium.org/402099/diff/24072/35008
File chrome/browser/gtk/accessibility_event_router_gtk.h (right):

http://codereview.chromium.org/402099/diff/24072/35008#newcode27
chrome/browser/gtk/accessibility_event_router_gtk.h:27: }
nit:  // namespace __gnu_cxx

http://codereview.chromium.org/402099/diff/28049/33005
File chrome/browser/extensions/extension_accessibility_api.h (right):

http://codereview.chromium.org/402099/diff/28049/33005#newcode5
chrome/browser/extensions/extension_accessibility_api.h:5: #ifndef
CHROME_BROWSER_EXTENSIONS_EXTENSION_ACCESSIBILITY_API_H__
single trailing underscore

[dmazzoni at google, 2010-01-27 21:55:11]

Thanks!  I think I'm done...I accidentally broke the Stubs api_test in an svn
merge but that's now fixed.  This will be ready to submit when the trybots
report success.

http://codereview.chromium.org/402099/diff/24072/35009
File chrome/browser/gtk/accessibility_event_router_gtk.cc (right):

http://codereview.chromium.org/402099/diff/24072/35009#newcode24
chrome/browser/gtk/accessibility_event_router_gtk.cc:24:
((AccessibilityEventRouter *)user_data)->DispatchAccessibilityNotification(
On 2010/01/27 20:47:40, Evan Stade wrote:
> nit: c++ style casts

Done.

http://codereview.chromium.org/402099/diff/24072/35008
File chrome/browser/gtk/accessibility_event_router_gtk.h (right):

http://codereview.chromium.org/402099/diff/24072/35008#newcode27
chrome/browser/gtk/accessibility_event_router_gtk.h:27: }
On 2010/01/27 20:47:40, Evan Stade wrote:
> nit:  // namespace __gnu_cxx

Done.

http://codereview.chromium.org/402099/diff/28049/33005
File chrome/browser/extensions/extension_accessibility_api.h (right):

http://codereview.chromium.org/402099/diff/28049/33005#newcode5
chrome/browser/extensions/extension_accessibility_api.h:5: #ifndef
CHROME_BROWSER_EXTENSIONS_EXTENSION_ACCESSIBILITY_API_H__
On 2010/01/27 20:47:40, Evan Stade wrote:
> single trailing underscore

Done.

[dmazzoni at google, 2010-01-28 16:46:26]

I fixed the chromeos build problem and got a trybot success for: win, mac,
linux, linux_view, linux_valgrind, linux_chromeos.