Add verbose logging for native accessibility events.

Add verbose logging for native accessibility events.

Introduce a new class BrowserAccessibilityEvent and
use it to encapsulate more information about
native accessibility events, and optionally log
information about events sent and also those that
are discarded (and why). This will make it easier to
debug problems with accessibility events in the future.

BUG=607703
Committed: https://crrev.com/9cd13b2cd1f50c5660b434c40070363e28ece434
Cr-Commit-Position: refs/heads/master@{#390867}

[dmazzoni, 2016-04-26 23:04:05]

dmazzoni@chromium.org changed reviewers:
+ nektar@chromium.org

[dmazzoni, 2016-04-26 23:04:05]

This is incomplete, but I wanted to send this to you now to give you an idea of
the direction I was thinking of going with this. Feedback welcome.

We can start by verbose logging, and then follow up with
an option to capture the same logs in
chrome://accessibility

[chromium-reviews, 2016-04-28 15:49:31]

I read through the change and it appears great.


-- 
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.

[dmazzoni, 2016-04-28 16:14:55]

Great, thanks. Two more things:

1. Do you think it makes more sense for BrowserAccessibilityEvent to be the
object that actually fires the event, or to leave it in
BrowserAccessibilityManager, or no preference?
2. I'm thinking of moving this code to ui/accessibility/platform instead,
call it AXPlatformEvent. That would allow us to share this code for Views
too since it might be useful to log focus events coming from the toolbar

Either way, I'll try to have a change ready to go today


On Thu, Apr 28, 2016 at 8:49 AM Nektarios Paisios <nektar@google.com> wrote:

> I read through the change and it appears great.
>
>
>

-- 
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.

[chromium-reviews, 2016-04-28 16:26:33]

On 4/28/2016 12:14 PM, Dominic Mazzoni wrote:
> Great, thanks. Two more things:
>
> 1. Do you think it makes more sense for BrowserAccessibilityEvent to 
> be the object that actually fires the event, or to leave it in 
> BrowserAccessibilityManager, or no preference?

I would prefer if this new class would fire the events, but there are 
two things that would make your job harder. One is that you would have 
to create subclasses for each platform and the second is that if we 
decide to move the new classes to ui/accessibility you would need to 
create two sets of subclasses.
Or, do you think we could somehow have common platform specific code for 
events between views and the renderer?
Perhaps add another source that is named "FROM_VIEWS"?
I am in favor of making the managers simpler. A lot of their code now 
deals with event dispatching. It would be great if we could isolate that 
outside the managers.

> 2. I'm thinking of moving this code to ui/accessibility/platform 
> instead, call it AXPlatformEvent. That would allow us to share this 
> code for Views too since it might be useful to log focus events coming 
> from the toolbar
>
I fully agree with moving the code to ui/accessibility.
We would have to do it sometime in the future and there is no better 
time than now.
Nektarios.

-- 
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.

[dmazzoni, 2016-04-28 16:47:07]

Sounds good. I think it's worth creating subclasses for each platform,
except for now I think Android will be different because it has to go via
Java.

On Thu, Apr 28, 2016 at 9:26 AM 'Nektarios Paisios' via Chromium-reviews <
chromium-reviews@chromium.org> wrote:

> On 4/28/2016 12:14 PM, Dominic Mazzoni wrote:
> > Great, thanks. Two more things:
> >
> > 1. Do you think it makes more sense for BrowserAccessibilityEvent to
> > be the object that actually fires the event, or to leave it in
> > BrowserAccessibilityManager, or no preference?
>
> I would prefer if this new class would fire the events, but there are
> two things that would make your job harder. One is that you would have
> to create subclasses for each platform and the second is that if we
> decide to move the new classes to ui/accessibility you would need to
> create two sets of subclasses.
> Or, do you think we could somehow have common platform specific code for
> events between views and the renderer?
> Perhaps add another source that is named "FROM_VIEWS"?
> I am in favor of making the managers simpler. A lot of their code now
> deals with event dispatching. It would be great if we could isolate that
> outside the managers.
>
> > 2. I'm thinking of moving this code to ui/accessibility/platform
> > instead, call it AXPlatformEvent. That would allow us to share this
> > code for Views too since it might be useful to log focus events coming
> > from the toolbar
> >
> I fully agree with moving the code to ui/accessibility.
> We would have to do it sometime in the future and there is no better
> time than now.
> Nektarios.
>
> --
> 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.
>
>

-- 
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.

[dmazzoni, 2016-04-28 22:04:58]

Description was changed from

==========
NOT FOR REVIEW. In-progress BrowserAccessibilityEvent

BUG=none
==========

to

==========
Add verbose logging for native accessibility events.

Introduce a new class BrowserAccessibilityEvent and
use it to encapsulate more information about
native accessibility events, and optionally log
information about events sent and also those that
are discarded (and why). This will make it easier to
debug problems with accessibility events in the future.

BUG=607703
==========

[dmazzoni, 2016-04-28 22:06:33]

OK, ready for review.

I want to move to ui/accessibility in a follow-up patch
because this is long enough.

[nektarios, 2016-04-29 20:24:19]

+protected:
+  Source source_;
+  ui::AXEvent event_type_;
+  BrowserAccessibility* node_;
+  BrowserAccessibility* original_node_;

Shouldn't all these be private? Subclasses could use set_source and set_node.
I prefer to use the word "target" for the "node" and "source" for
"original_node".
I'd prefer if all BrowserAccessibility* variables were declared as const. We are
not modifying them, only the pointer, so const should work.

+#include "content/browser/accessibility/browser_accessibility_event.h"

The header file for a specific cc file goes at the very top.
Shouldn't you also include <string>?


+    case FailedBecauseDetached:

I prefer
case FailedBecauseObjectIsDetached?

Also, should enum constants be all caps or use  Pascal casing?

+bool BrowserAccessibilityEvent::FailedToSend(Result result) {

Could you use a default switch case and  put NOT_REACHED there?

VLOG(1) << "Accessibility event ...

What does the 1 constant stand for? DEBUG?
Also, I prefer using one string type for everything. Could you concadenate
everything into a message std::string, instead of creating a mixture of
std::strings and const char* strs?


+  static bool FailedToSend(Result result);

I think that we can now use enum classes and put such methods directly on the
enum, but feel free not to do that if you find it cumbersome.


+  // Construct a new accessibility event that should needs to be sent via a

// needs to be sent ...?


+  // Synchronously try to send the native accessibility event notification
+  // and return a result indicating the outcome. This deletes the event.

Why does it say the event is deleted?

+#include "content/browser/accessibility/browser_accessibility_event_win.h"
Header include goes to the top.

+BrowserAccessibilityEvent::Result BrowserAccessibilityEventWin::Send() {
+  DCHECK(node_->manager());

node_->instance_active()?

+  // It doesn't make sense to fire a REORDER event on a leaf node; that
+  // happens when the node has internal children line inline text boxes.

The word "line" is not needed.

+      SendWinAccessibilityEvent(BrowserAccessibilityEventWin* event);

I prefer Fire instead of Send. It would also have a similar name to
CanFireEvents and FireFocusEvent.
I think it should be a const method.

+void BrowserAccessibilityWin::SendNativeEvent(LONG win_event_type) {

FireNativeEvent?
Shouldn't it be const too?


+++ b/content/content_browser.gypi

Shouldn't you also modify the .gn file?

[dmazzoni, 2016-04-29 21:55:48]

On 2016/04/29 20:24:19, nektarios wrote:
> Shouldn't all these be private? Subclasses could use set_source and set_node.

Done

> I prefer to use the word "target" for the "node" and "source" for
> "original_node".

OK, done, but I made it target and original_target, since we already have
"source" that means something different.

> I'd prefer if all BrowserAccessibility* variables were declared as const. We
are
> not modifying them, only the pointer, so const should work.

Done.

> +#include "content/browser/accessibility/browser_accessibility_event.h"
> 
> The header file for a specific cc file goes at the very top.
> Shouldn't you also include <string>?

Done.

> 
> +    case FailedBecauseDetached:
> 
> I prefer
> case FailedBecauseObjectIsDetached?

Done

> Also, should enum constants be all caps or use  Pascal casing?

I think this is allowed...but they may change to caps if I move them
to ax_enums.

> +bool BrowserAccessibilityEvent::FailedToSend(Result result) {
> 
> Could you use a default switch case and  put NOT_REACHED there?

It's better this way, with no default, because it's a compiler error
if we forget to add all of the cases.

> VLOG(1) << "Accessibility event ...
> 
> What does the 1 constant stand for? DEBUG?

verbose level. You can add --v=1 for verbose-1 which is a lot of logs, but
much better is --vmodule="*browser_accessibility_event*=1", which says to
log all level-1 verbose logs, but only from this file.

> Also, I prefer using one string type for everything. Could you concadenate
> everything into a message std::string, instead of creating a mixture of
> std::strings and const char* strs?

I think it's better to use const char* for string literals, it avoids creating
objects unnecessarily.

> 
> +  static bool FailedToSend(Result result);
> 
> I think that we can now use enum classes and put such methods directly on the
> enum, but feel free not to do that if you find it cumbersome.

Oh! I'll consider that for a follow-up.

> +  // Construct a new accessibility event that should needs to be sent via a
> 
> // needs to be sent ...?

Fixed

> +  // Synchronously try to send the native accessibility event notification
> +  // and return a result indicating the outcome. This deletes the event.
> 
> Why does it say the event is deleted?

So you can write Event::Create(...)->Send() and not worry about deleting it,
it deletes itself.

> +#include "content/browser/accessibility/browser_accessibility_event_win.h"
> Header include goes to the top.

Done.

> +BrowserAccessibilityEvent::Result BrowserAccessibilityEventWin::Send() {
> +  DCHECK(node_->manager());
> 
> node_->instance_active()?

Only needed for external APIs like IAccessible. We don't have any pointers
to inactive nodes internally.

> +  // It doesn't make sense to fire a REORDER event on a leaf node; that
> +  // happens when the node has internal children line inline text boxes.
> 
> The word "line" is not needed.

Done

> +      SendWinAccessibilityEvent(BrowserAccessibilityEventWin* event);
> 
> I prefer Fire instead of Send. It would also have a similar name to
> CanFireEvents and FireFocusEvent.
> I think it should be a const method.

Done.

> +void BrowserAccessibilityWin::SendNativeEvent(LONG win_event_type) {
> 
> FireNativeEvent?
> Shouldn't it be const too?

Done

> 
> +++ b/content/content_browser.gypi
> 
> Shouldn't you also modify the .gn file?

Not needed - both gyp and gn import the same .gypi file

[nektarios, 2016-04-29 21:59:52]

lgtm

[chromium-reviews, 2016-04-29 23:44:20]

I'd prefer to call the Send method, SendAndDestroy or better force the 
caller to delete the class manually.
I am concerned that people might not read the comment and not realize 
that the class auto-deletes itself.
Feel free to do it in a followup patch if you like.

-- 
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.

[dmazzoni, 2016-04-30 23:09:59]

I want to make it clear, but I was thinking of modeling it
after ipc/ipc_sender.h, which is used throughout Chrome, where the sender
takes ownership once you call Send().

On Fri, Apr 29, 2016 at 4:44 PM 'Nektarios Paisios' via Chromium-reviews <
chromium-reviews@chromium.org> wrote:

> I'd prefer to call the Send method, SendAndDestroy or better force the
> caller to delete the class manually.
> I am concerned that people might not read the comment and not realize
> that the class auto-deletes itself.
> Feel free to do it in a followup patch if you like.
>
> --
> 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.
>
>

-- 
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.

[dmazzoni, 2016-04-30 23:39:58]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-04-30 23:39:58]

The patchset sent to the CQ was uploaded after l-g-t-m from nektar@chromium.org
Link to the patchset: https://codereview.chromium.org/1925473002/#ps120001
(title: "address all feedback")

[commit-bot: I haz the power, 2016-04-30 23:40:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1925473002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1925473002/120001

[commit-bot: I haz the power, 2016-05-01 00:08:08]

Description was changed from

==========
Add verbose logging for native accessibility events.

Introduce a new class BrowserAccessibilityEvent and
use it to encapsulate more information about
native accessibility events, and optionally log
information about events sent and also those that
are discarded (and why). This will make it easier to
debug problems with accessibility events in the future.

BUG=607703
==========

to

==========
Add verbose logging for native accessibility events.

Introduce a new class BrowserAccessibilityEvent and
use it to encapsulate more information about
native accessibility events, and optionally log
information about events sent and also those that
are discarded (and why). This will make it easier to
debug problems with accessibility events in the future.

BUG=607703
==========

[commit-bot: I haz the power, 2016-05-01 00:08:09]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-05-01 00:09:30]

Description was changed from

==========
Add verbose logging for native accessibility events.

Introduce a new class BrowserAccessibilityEvent and
use it to encapsulate more information about
native accessibility events, and optionally log
information about events sent and also those that
are discarded (and why). This will make it easier to
debug problems with accessibility events in the future.

BUG=607703
==========

to

==========
Add verbose logging for native accessibility events.

Introduce a new class BrowserAccessibilityEvent and
use it to encapsulate more information about
native accessibility events, and optionally log
information about events sent and also those that
are discarded (and why). This will make it easier to
debug problems with accessibility events in the future.

BUG=607703

Committed: https://crrev.com/9cd13b2cd1f50c5660b434c40070363e28ece434
Cr-Commit-Position: refs/heads/master@{#390867}
==========

[commit-bot: I haz the power, 2016-05-01 00:09:31]

Patchset 7 (id:??) landed as
https://crrev.com/9cd13b2cd1f50c5660b434c40070363e28ece434
Cr-Commit-Position: refs/heads/master@{#390867}