Use unique IDs for accessibility nodes on Android

Use unique IDs for accessibility nodes on Android

Shortly we're going to switch to having a separate accessibility tree for
each frame. That would break Android because Android only has a single
virtual view hierarchy, and we were using node IDs as the virtual view IDs,
which are only unique relative to one tree.

We already had unique accessibility IDs for Windows, so refactor that code
into a cross-platform unique ID per accessibility node, both in the
Views code and Content code, and then switch Android to use those unique IDs
instead of node IDs.

BUG=532249
TBR=nasko

Committed: https://crrev.com/7324d7f6bd40b3d60bc71610494abcca22cf6a69
Cr-Commit-Position: refs/heads/master@{#379775}

[dmazzoni, 2016-03-03 23:56:59]

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

[nektarios, 2016-03-07 16:03:14]

1. Why is the hashtable with the IDs stored in BrowserAccessibility and not in
BrowserAccessibilityManager? I thought that all data that has to do with the
unified cross-frame tree is stored in the *managers.
2. It appears that we still have two tables, one for views and one for Web
content. Wouldn't it create a conflict ion Android? Or, Android doesn't use
Views at all.

+  return BrowserAccessibility::GetFromUniqueID(-child_id)
+      ->ToBrowserAccessibilityWin();
Shouldn't we first check if the child is non-null before we call
ToBrowserAccessibilityWin().

+typedef base::hash_map<int32_t, AXPlatformNode*> UniqueIdMap;
I prefer "using".
+using UniqueIdMap = base::hash_map<int32_t, AXPlatformNode*>;

+int32_t AXPlatformNode::GetNextUniqueId() {
+  static int32_t next_unique_id = 1;
I thought that static local variables were not allowed because they were not
thread safe or something?
Don't we use CR_LOCAL_STATIC or something similar?


+AXPlatformNode::AXPlatformNode() : unique_id_(GetNextUniqueId()) {
+  g_unique_id_map.Get().insert(std::make_pair(unique_id_, this));
Isn't it easier to say
g_unique_id_map.Get()[unique_id_] = this);

+  // to be a positive number. (It's stored in an int32_t because some platforms
+  // want to negate it.) This can be used when the id has to be unique across
I don't understand why int32 makes negations  easier.

[dmazzoni, 2016-03-07 16:33:12]

On 2016/03/07 16:03:14, nektarios wrote:
> 1. Why is the hashtable with the IDs stored in BrowserAccessibility and not in
> BrowserAccessibilityManager? I thought that all data that has to do with the
> unified cross-frame tree is stored in the *managers.

Not exactly. BrowserAccessibilityManager stores information about one frame
only.
Sometimes it walks across multiple frames, but all of its members are only about
one frame. Since the unique ID is global it made more sense to me to make it a
static of BrowserAccessibiltiyManager.

> 2. It appears that we still have two tables, one for views and one for Web
> content. Wouldn't it create a conflict ion Android? Or, Android doesn't use
> Views at all.

Android doesn't have Views at all, so there's no conflict. On Windows they
share the same ID so they won't overlap, but even still they belong to different
HWNDs there.

> +  return BrowserAccessibility::GetFromUniqueID(-child_id)
> +      ->ToBrowserAccessibilityWin();
> Shouldn't we first check if the child is non-null before we call
> ToBrowserAccessibilityWin().

ToBrowserAccessibilityWin is just a static_cast so it's safe.

> +typedef base::hash_map<int32_t, AXPlatformNode*> UniqueIdMap;
> I prefer "using".
> +using UniqueIdMap = base::hash_map<int32_t, AXPlatformNode*>;

Sure, done.

> +int32_t AXPlatformNode::GetNextUniqueId() {
> +  static int32_t next_unique_id = 1;
> I thought that static local variables were not allowed because they were not
> thread safe or something?
> Don't we use CR_LOCAL_STATIC or something similar?

The reason to use CR_LOCAL_STATIC is to avoid a global constructor and
destructor, so it's required to use it for objects. For plain old data like
ints and floats it's okay to just use a static.

> +AXPlatformNode::AXPlatformNode() : unique_id_(GetNextUniqueId()) {
> +  g_unique_id_map.Get().insert(std::make_pair(unique_id_, this));
> Isn't it easier to say
> g_unique_id_map.Get()[unique_id_] = this);
> 
> +  // to be a positive number. (It's stored in an int32_t because some
platforms
> +  // want to negate it.) This can be used when the id has to be unique across
> I don't understand why int32 makes negations  easier.

I meant int32_t as opposed to uint32_t. If we used uint32_t then it would be
obvious that it's not negative, but then we wouldn't be able to safely negate it
because we don't know if it could be larger than the largest signed integer.
I'll clarify.

[dmazzoni, 2016-03-07 16:37:59]

On 2016/03/07 16:33:12, dmazzoni wrote:
> Not exactly. BrowserAccessibilityManager stores information about one frame
> only.
> Sometimes it walks across multiple frames, but all of its members are only
about
> one frame. Since the unique ID is global it made more sense to me to make it a
> static of BrowserAccessibiltiyManager.

Oops, I meant to say a static of BrowserAccessibiltiy.

[chromium-reviews, 2016-03-07 16:42:47]

Android doesn't have Views at all, so there's no conflict. On Windows they
> share the same ID so they won't overlap, but even still they belong to 
> different
> HWNDs there.
>
What do you mean "On Windows they share the same id?" You mean the same 
hashtable of ids to object mappings? I thought we don't do that now. I 
saw two hashtables.
I acknowledge the different HWNDs though.

> > + return BrowserAccessibility::GetFromUniqueID(-child_id)
> > + ->ToBrowserAccessibilityWin();
> > Shouldn't we first check if the child is non-null before we call
> > ToBrowserAccessibilityWin().
>
> ToBrowserAccessibilityWin is just a static_cast so it's safe.
>
I know it's a static cast but we are calling a function 
(ToBrowserAccessibilityWin) in order to carry out this cast and we are 
calling it on a pointer that may be null?
>
>
> > +AXPlatformNode::AXPlatformNode() : unique_id_(GetNextUniqueId()) {
> > + g_unique_id_map.Get().insert(std::make_pair(unique_id_, this));
> > Isn't it easier to say
> > g_unique_id_map.Get()[unique_id_] = this);

Have you seen this?

-- 
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-03-07 17:11:49]

On Mon, Mar 7, 2016 at 8:42 AM Nektarios Paisios <nektar@google.com> wrote:

> Android doesn't have Views at all, so there's no conflict. On Windows they
>
> share the same ID so they won't overlap, but even still they belong to
> different
> HWNDs there.
>
> What do you mean "On Windows they share the same id?" You mean the same
> hashtable of ids to object mappings? I thought we don't do that now. I saw
> two hashtables.
>

Sorry, what I mean is that there's only one global function that returns
the next ID to use, even though there are two different hashes. So two
objects won't share the same ID across the two hashes.

In the future if we had a platform with views and web in the same native
window, we could solve that by having one global hash, but we'd need to
figure out what type the result was.  Let's cross that bridge when we come
to it.

ToBrowserAccessibilityWin is just a static_cast so it's safe.
>
> I know it's a static cast but we are calling a function
> (ToBrowserAccessibilityWin) in order to carry out this cast and we are
> calling it on a pointer that may be null?
>

Yep, it really is safe. You can cast a null pointer. It's only unsafe to
dereference it.

> unique_id_(GetNextUniqueId()) {
> > + g_unique_id_map.Get().insert(std::make_pair(unique_id_, this));
> > Isn't it easier to say
> > g_unique_id_map.Get()[unique_id_] = this);
>
> Have you seen this?
>

Sorry! You're right, fixed now.

-- 
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-03-07 17:25:08]

> Yep, it really is safe. You can cast a null pointer. It's only unsafe 
> to dereference it.
>

Yes, I know:
static_cast<BrowserAccessibilityWin*>(nullptr) will return nullptr.
But we might be doing:
nullptr->ToBrowserAccessibilityWin();
Is this guaranteed to be optimized away by the compiler to 
static_cast<BrowserAccessibilityWin*>(nullptr)?
What am I not getting?

-- 
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-03-07 17:56:14]

On Mon, Mar 7, 2016 at 9:25 AM 'Nektarios Paisios' via Chromium-reviews <
chromium-reviews@chromium.org> wrote:

> Yes, I know:
> static_cast<BrowserAccessibilityWin*>(nullptr) will return nullptr.
> But we might be doing:
> nullptr->ToBrowserAccessibilityWin();
> Is this guaranteed to be optimized away by the compiler to
> static_cast<BrowserAccessibilityWin*>(nullptr)?
> What am I not getting?
>

You're right. I looked it up, and the consensus on StackOverflow seems to
be that it's technically undefined behavior but in practice nobody can
point to a case where it fails. I'll change this throughout but there are
nearly 150 uses so it's going to take me an hour or so

-- 
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-03-07 18:29:13]

LGTM


On Monday, March 7, 2016, Dominic Mazzoni <dmazzoni@chromium.org> wrote:

> On Mon, Mar 7, 2016 at 9:25 AM 'Nektarios Paisios' via Chromium-reviews <
> chromium-reviews@chromium.org
> <javascript:_e(%7B%7D,'cvml','chromium-reviews@chromium.org');>> wrote:
>
>> Yes, I know:
>> static_cast<BrowserAccessibilityWin*>(nullptr) will return nullptr.
>> But we might be doing:
>> nullptr->ToBrowserAccessibilityWin();
>> Is this guaranteed to be optimized away by the compiler to
>> static_cast<BrowserAccessibilityWin*>(nullptr)?
>> What am I not getting?
>>
>
> You're right. I looked it up, and the consensus on StackOverflow seems to
> be that it's technically undefined behavior but in practice nobody can
> point to a case where it fails. I'll change this throughout but there are
> nearly 150 uses so it's going to take me an hour or so
>
>
>

-- 
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-03-07 19:00:05]

Thanks for the review! I replaced all of the ToBrowserAccessibilityXXX methods
with global functions across three platforms. I suspect I'll have a couple of
compile errors to fix in the try jobs but otherwise this should be ready to
land.

[nektarios, 2016-03-07 19:06:53]

lgtm

[dmazzoni, 2016-03-08 07:08:50]

Description was changed from

==========
Use unique IDs for accessibility nodes on Android

Shortly we're going to switch to having a separate accessibility tree for
each frame. That would break Android because Android only has a single
virtual view hierarchy, and we were using node IDs as the virtual view IDs,
which are only unique relative to one tree.

We already had unique accessibility IDs for Windows, so refactor that code
into a cross-platform unique ID per accessibility node, both in the
Views code and Content code, and then switch Android to use those unique IDs
instead of node IDs.

BUG=532249
==========

to

==========
Use unique IDs for accessibility nodes on Android

Shortly we're going to switch to having a separate accessibility tree for
each frame. That would break Android because Android only has a single
virtual view hierarchy, and we were using node IDs as the virtual view IDs,
which are only unique relative to one tree.

We already had unique accessibility IDs for Windows, so refactor that code
into a cross-platform unique ID per accessibility node, both in the
Views code and Content code, and then switch Android to use those unique IDs
instead of node IDs.

BUG=532249
TBR=nasko
==========

[dmazzoni, 2016-03-08 07:08:50]

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

[dmazzoni, 2016-03-08 07:09:08]

+tbr:nasko for trivial changes to content/browser/renderer_host

[dmazzoni, 2016-03-08 07:09:14]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-03-08 07:09:14]

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/1762143002/#ps200001
(title: "Fix null obj deref in DCHECK")

[commit-bot: I haz the power, 2016-03-08 07:09:27]

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

[commit-bot: I haz the power, 2016-03-08 07:36:42]

Description was changed from

==========
Use unique IDs for accessibility nodes on Android

Shortly we're going to switch to having a separate accessibility tree for
each frame. That would break Android because Android only has a single
virtual view hierarchy, and we were using node IDs as the virtual view IDs,
which are only unique relative to one tree.

We already had unique accessibility IDs for Windows, so refactor that code
into a cross-platform unique ID per accessibility node, both in the
Views code and Content code, and then switch Android to use those unique IDs
instead of node IDs.

BUG=532249
TBR=nasko
==========

to

==========
Use unique IDs for accessibility nodes on Android

Shortly we're going to switch to having a separate accessibility tree for
each frame. That would break Android because Android only has a single
virtual view hierarchy, and we were using node IDs as the virtual view IDs,
which are only unique relative to one tree.

We already had unique accessibility IDs for Windows, so refactor that code
into a cross-platform unique ID per accessibility node, both in the
Views code and Content code, and then switch Android to use those unique IDs
instead of node IDs.

BUG=532249
TBR=nasko
==========

[commit-bot: I haz the power, 2016-03-08 07:36:43]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-03-08 07:37:50]

Description was changed from

==========
Use unique IDs for accessibility nodes on Android

Shortly we're going to switch to having a separate accessibility tree for
each frame. That would break Android because Android only has a single
virtual view hierarchy, and we were using node IDs as the virtual view IDs,
which are only unique relative to one tree.

We already had unique accessibility IDs for Windows, so refactor that code
into a cross-platform unique ID per accessibility node, both in the
Views code and Content code, and then switch Android to use those unique IDs
instead of node IDs.

BUG=532249
TBR=nasko
==========

to

==========
Use unique IDs for accessibility nodes on Android

Shortly we're going to switch to having a separate accessibility tree for
each frame. That would break Android because Android only has a single
virtual view hierarchy, and we were using node IDs as the virtual view IDs,
which are only unique relative to one tree.

We already had unique accessibility IDs for Windows, so refactor that code
into a cross-platform unique ID per accessibility node, both in the
Views code and Content code, and then switch Android to use those unique IDs
instead of node IDs.

BUG=532249
TBR=nasko

Committed: https://crrev.com/7324d7f6bd40b3d60bc71610494abcca22cf6a69
Cr-Commit-Position: refs/heads/master@{#379775}
==========

[commit-bot: I haz the power, 2016-03-08 07:37:51]

Patchset 11 (id:??) landed as
https://crrev.com/7324d7f6bd40b3d60bc71610494abcca22cf6a69
Cr-Commit-Position: refs/heads/master@{#379775}

[dmazzoni, 2016-05-17 16:37:15]

Great fix, I think that would explain the behavior you're seeing. Thanks.

I've asked for a comment below so it doesn't look like a bug later,
but feel free to commit once you add that.

[dmazzoni, 2016-05-17 16:37:52]

Oops, that last comment was intended for:
https://codereview.chromium.org/1978223002/