Update desktop tree when Aura windows are removed.

Update desktop tree when Aura windows are removed.

Since the Automation API utilizes wrapper objects (necessary as it keeps its own
id map and cache) and the tree structure contains views, widgets, windows, and a
fake desktop root, we were not tied to the same lifetime as the wrapped
objects. This cl makes it so we do the needed cleanup when windows and widgets
get removed.

In particular, we try to observe as early as possible every observer method that
causes a widget or window to be detached. We still need the additional methods
(Destroy*) because accessibility events might be fired between Destroying* and
Destroyed*.

The bug in ChromeVox where time gets scrapped from the desktop tree revealed the issue and required we update the client tree with a children changed event on the parent of a removed window. Other platforms do not have this problem because they talk to the browser process directly whereas ChromeVox talks to the desktop tree in its rendere extension process.

BUG=641734
TEST=AutomationApiTest and manual test with ChromeVox on status tray; verify views/widgets get detached from the desktop tree in js when the status tray is hidden
Additionally, other native windows update ChromeVox appropriately when closed. Ctrl+F find, blluetooth dialog, etc.

Committed: https://crrev.com/75025da30515c2abfa9ce587ca7b0aedaad7ea4e
Cr-Commit-Position: refs/heads/master@{#415871}

[David Tseng, 2016-08-31 15:15:06]

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

[commit-bot: I haz the power, 2016-08-31 15:15:31]

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

[David Tseng, 2016-08-31 15:21:16]

Description was changed from

==========
Update desktop tree when Aura windows are removed.

Since the Automation API utilizes wrapper objects (necessary as it keeps its own
id map and cache) and the tree structure contains views, widgets, windows, and a
fake desktop root, we were not tied to the same lifetime as the wrapped
objects. This cl makes it so we do the needed cleanup when windows and widgets
get removed.

In particular, we try to observe as early as possible every observer method that
causes a widget or window to be detached. We still need the additional methods
(Destroy*) because accessibility events might be fired between Destroying* and
Destroyed*.

BUG=
==========

to

==========
Update desktop tree when Aura windows are removed.

Since the Automation API utilizes wrapper objects (necessary as it keeps its own
id map and cache) and the tree structure contains views, widgets, windows, and a
fake desktop root, we were not tied to the same lifetime as the wrapped
objects. This cl makes it so we do the needed cleanup when windows and widgets
get removed.

In particular, we try to observe as early as possible every observer method that
causes a widget or window to be detached. We still need the additional methods
(Destroy*) because accessibility events might be fired between Destroying* and
Destroyed*.

The bug in ChromeVox where time gets scrapped from the desktop tree revealed the
issue and required we update the client tree with a children changed event on
the parent of a removed window. Other platforms do not have this problem because
they talk to the browser process directly whereas ChromeVox talks to the desktop
tree in its rendere extension process.

BUG=641734
TEST=AutomationApiTest and manual test with ChromeVox on status tray; verify
views/widgets get detached from the desktop tree in js when the status tray is
hidden
==========

[David Tseng, 2016-08-31 15:21:16]

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

[David Tseng, 2016-08-31 15:26:14]

Description was changed from

==========
Update desktop tree when Aura windows are removed.

Since the Automation API utilizes wrapper objects (necessary as it keeps its own
id map and cache) and the tree structure contains views, widgets, windows, and a
fake desktop root, we were not tied to the same lifetime as the wrapped
objects. This cl makes it so we do the needed cleanup when windows and widgets
get removed.

In particular, we try to observe as early as possible every observer method that
causes a widget or window to be detached. We still need the additional methods
(Destroy*) because accessibility events might be fired between Destroying* and
Destroyed*.

The bug in ChromeVox where time gets scrapped from the desktop tree revealed the
issue and required we update the client tree with a children changed event on
the parent of a removed window. Other platforms do not have this problem because
they talk to the browser process directly whereas ChromeVox talks to the desktop
tree in its rendere extension process.

BUG=641734
TEST=AutomationApiTest and manual test with ChromeVox on status tray; verify
views/widgets get detached from the desktop tree in js when the status tray is
hidden
==========

to

==========
Update desktop tree when Aura windows are removed.

Since the Automation API utilizes wrapper objects (necessary as it keeps its own
id map and cache) and the tree structure contains views, widgets, windows, and a
fake desktop root, we were not tied to the same lifetime as the wrapped
objects. This cl makes it so we do the needed cleanup when windows and widgets
get removed.

In particular, we try to observe as early as possible every observer method that
causes a widget or window to be detached. We still need the additional methods
(Destroy*) because accessibility events might be fired between Destroying* and
Destroyed*.

The bug in ChromeVox where time gets scrapped from the desktop tree revealed the
issue and required we update the client tree with a children changed event on
the parent of a removed window. Other platforms do not have this problem because
they talk to the browser process directly whereas ChromeVox talks to the desktop
tree in its rendere extension process.

BUG=641734
TEST=AutomationApiTest and manual test with ChromeVox on status tray; verify
views/widgets get detached from the desktop tree in js when the status tray is
hidden
Additionally, other native windows update ChromeVox appropriately when closed.
Ctrl+F find, blluetooth dialog, etc.
==========

[commit-bot: I haz the power, 2016-08-31 16:16:17]

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

[commit-bot: I haz the power, 2016-08-31 16:16:18]

Dry run: Exceeded global retry quota

[dmazzoni, 2016-08-31 16:33:50]

lgtm, just some suggestions for readability

https://codereview.chromium.org/2295183003/diff/1/chrome/browser/ui/aura/acce...
File chrome/browser/ui/aura/accessibility/automation_manager_aura.cc (right):

https://codereview.chromium.org/2295183003/diff/1/chrome/browser/ui/aura/acce...
chrome/browser/ui/aura/accessibility/automation_manager_aura.cc:131: context =
g_browser_process->profile_manager()->GetLastUsedProfile();
This is done in two places. I think it'd be better if you just passed nullptr
instead of |context| to SendEvent, and did the same from
AutomationManagerAura::Enable, then have SendEvent be the one to call
GetLastUsedProfile if |context| is nullptr.

https://codereview.chromium.org/2295183003/diff/1/ui/views/accessibility/ax_a...
File ui/views/accessibility/ax_aura_obj_cache.cc (right):

https://codereview.chromium.org/2295183003/diff/1/ui/views/accessibility/ax_a...
ui/views/accessibility/ax_aura_obj_cache.cc:80: if (id != -1) {
This check isn't necessary, right? You can still call Get(-1) and it will return
nullptr.

I think this would be more clear, if RemoveInternal doesn't affect |obj|:

AXAuraObjWrapper* obj = Get(id);
RemoveInternal(window, window_to_id_map_);
if (notify && delegate_)
  delegate_->OnChildWindowRemoved(obj ? obj->GetParent() : nullptr);

Alternatively, how about:

AXAuraObjWrapper* window_obj = Get(id);
AXAuraObjWrapper* window_parent_obj = window_obj ? window_obj->GetParent() :
nullptr;
RemoveInternal(window, window_to_id_map_);
if (notify && delegate_)
  delegate_->OnChildWindowRemoved(window_parent_obj);

[David Tseng, 2016-08-31 21:06:22]

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

[David Tseng, 2016-08-31 21:06:42]

https://codereview.chromium.org/2295183003/diff/1/chrome/browser/ui/aura/acce...
File chrome/browser/ui/aura/accessibility/automation_manager_aura.cc (right):

https://codereview.chromium.org/2295183003/diff/1/chrome/browser/ui/aura/acce...
chrome/browser/ui/aura/accessibility/automation_manager_aura.cc:131: context =
g_browser_process->profile_manager()->GetLastUsedProfile();
On 2016/08/31 16:33:50, dmazzoni wrote:
> This is done in two places. I think it'd be better if you just passed nullptr
> instead of |context| to SendEvent, and did the same from
> AutomationManagerAura::Enable, then have SendEvent be the one to call
> GetLastUsedProfile if |context| is nullptr.

Done.

https://codereview.chromium.org/2295183003/diff/1/ui/views/accessibility/ax_a...
File ui/views/accessibility/ax_aura_obj_cache.cc (right):

https://codereview.chromium.org/2295183003/diff/1/ui/views/accessibility/ax_a...
ui/views/accessibility/ax_aura_obj_cache.cc:80: if (id != -1) {
On 2016/08/31 16:33:50, dmazzoni wrote:
> This check isn't necessary, right? You can still call Get(-1) and it will
return
> nullptr.
> 
> I think this would be more clear, if RemoveInternal doesn't affect |obj|:
> 
> AXAuraObjWrapper* obj = Get(id);
> RemoveInternal(window, window_to_id_map_);
> if (notify && delegate_)
>   delegate_->OnChildWindowRemoved(obj ? obj->GetParent() : nullptr);
> 
> Alternatively, how about:
> 
> AXAuraObjWrapper* window_obj = Get(id);
> AXAuraObjWrapper* window_parent_obj = window_obj ? window_obj->GetParent() :
> nullptr;
> RemoveInternal(window, window_to_id_map_);
> if (notify && delegate_)
>   delegate_->OnChildWindowRemoved(window_parent_obj);
> 

Done.

[commit-bot: I haz the power, 2016-08-31 21:06:53]

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

[commit-bot: I haz the power, 2016-08-31 22:03:10]

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

[commit-bot: I haz the power, 2016-08-31 22:03:11]

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

[David Tseng, 2016-08-31 23:15:11]

The CQ bit was checked by dtseng@chromium.org

[David Tseng, 2016-08-31 23:15:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/2295183003/#ps40001
(title: "Fix test.")

[commit-bot: I haz the power, 2016-08-31 23:15:54]

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

[commit-bot: I haz the power, 2016-09-01 00:22:18]

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

[commit-bot: I haz the power, 2016-09-01 00:22:19]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[David Tseng, 2016-09-01 02:51:26]

The CQ bit was checked by dtseng@chromium.org

[commit-bot: I haz the power, 2016-09-01 02:51:48]

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

[commit-bot: I haz the power, 2016-09-01 03:21:32]

Description was changed from

==========
Update desktop tree when Aura windows are removed.

Since the Automation API utilizes wrapper objects (necessary as it keeps its own
id map and cache) and the tree structure contains views, widgets, windows, and a
fake desktop root, we were not tied to the same lifetime as the wrapped
objects. This cl makes it so we do the needed cleanup when windows and widgets
get removed.

In particular, we try to observe as early as possible every observer method that
causes a widget or window to be detached. We still need the additional methods
(Destroy*) because accessibility events might be fired between Destroying* and
Destroyed*.

The bug in ChromeVox where time gets scrapped from the desktop tree revealed the
issue and required we update the client tree with a children changed event on
the parent of a removed window. Other platforms do not have this problem because
they talk to the browser process directly whereas ChromeVox talks to the desktop
tree in its rendere extension process.

BUG=641734
TEST=AutomationApiTest and manual test with ChromeVox on status tray; verify
views/widgets get detached from the desktop tree in js when the status tray is
hidden
Additionally, other native windows update ChromeVox appropriately when closed.
Ctrl+F find, blluetooth dialog, etc.
==========

to

==========
Update desktop tree when Aura windows are removed.

Since the Automation API utilizes wrapper objects (necessary as it keeps its own
id map and cache) and the tree structure contains views, widgets, windows, and a
fake desktop root, we were not tied to the same lifetime as the wrapped
objects. This cl makes it so we do the needed cleanup when windows and widgets
get removed.

In particular, we try to observe as early as possible every observer method that
causes a widget or window to be detached. We still need the additional methods
(Destroy*) because accessibility events might be fired between Destroying* and
Destroyed*.

The bug in ChromeVox where time gets scrapped from the desktop tree revealed the
issue and required we update the client tree with a children changed event on
the parent of a removed window. Other platforms do not have this problem because
they talk to the browser process directly whereas ChromeVox talks to the desktop
tree in its rendere extension process.

BUG=641734
TEST=AutomationApiTest and manual test with ChromeVox on status tray; verify
views/widgets get detached from the desktop tree in js when the status tray is
hidden
Additionally, other native windows update ChromeVox appropriately when closed.
Ctrl+F find, blluetooth dialog, etc.
==========

[commit-bot: I haz the power, 2016-09-01 03:21:33]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-01 03:23:37]

Description was changed from

==========
Update desktop tree when Aura windows are removed.

Since the Automation API utilizes wrapper objects (necessary as it keeps its own
id map and cache) and the tree structure contains views, widgets, windows, and a
fake desktop root, we were not tied to the same lifetime as the wrapped
objects. This cl makes it so we do the needed cleanup when windows and widgets
get removed.

In particular, we try to observe as early as possible every observer method that
causes a widget or window to be detached. We still need the additional methods
(Destroy*) because accessibility events might be fired between Destroying* and
Destroyed*.

The bug in ChromeVox where time gets scrapped from the desktop tree revealed the
issue and required we update the client tree with a children changed event on
the parent of a removed window. Other platforms do not have this problem because
they talk to the browser process directly whereas ChromeVox talks to the desktop
tree in its rendere extension process.

BUG=641734
TEST=AutomationApiTest and manual test with ChromeVox on status tray; verify
views/widgets get detached from the desktop tree in js when the status tray is
hidden
Additionally, other native windows update ChromeVox appropriately when closed.
Ctrl+F find, blluetooth dialog, etc.
==========

to

==========
Update desktop tree when Aura windows are removed.

Since the Automation API utilizes wrapper objects (necessary as it keeps its own
id map and cache) and the tree structure contains views, widgets, windows, and a
fake desktop root, we were not tied to the same lifetime as the wrapped
objects. This cl makes it so we do the needed cleanup when windows and widgets
get removed.

In particular, we try to observe as early as possible every observer method that
causes a widget or window to be detached. We still need the additional methods
(Destroy*) because accessibility events might be fired between Destroying* and
Destroyed*.

The bug in ChromeVox where time gets scrapped from the desktop tree revealed the
issue and required we update the client tree with a children changed event on
the parent of a removed window. Other platforms do not have this problem because
they talk to the browser process directly whereas ChromeVox talks to the desktop
tree in its rendere extension process.

BUG=641734
TEST=AutomationApiTest and manual test with ChromeVox on status tray; verify
views/widgets get detached from the desktop tree in js when the status tray is
hidden
Additionally, other native windows update ChromeVox appropriately when closed.
Ctrl+F find, blluetooth dialog, etc.

Committed: https://crrev.com/75025da30515c2abfa9ce587ca7b0aedaad7ea4e
Cr-Commit-Position: refs/heads/master@{#415871}
==========

[commit-bot: I haz the power, 2016-09-01 03:23:38]

Patchset 3 (id:??) landed as
https://crrev.com/75025da30515c2abfa9ce587ca7b0aedaad7ea4e
Cr-Commit-Position: refs/heads/master@{#415871}