Enable chrome.automation.getDesktop on all aura platforms.

Enable chrome.automation.getDesktop on all aura platforms.

- on Chrome OS, continue to enumerate all Aura windows when building the desktop tree via the Ash shell.
- on all other Aura platforms, lazily hook up top level windows that have been created in our object cache. This works by only presenting cached windows as children of the desktop node. Since serialization walks up the tree, we are guaranteed windows in the cache whenever a descendant view has an event fired upon it.

- allow ChromeVox to traverse the desktop automation tree on all platforms.

TEST=build ChromeVox webstore extension. Run it on linux chromium. Verify views-based UI reads.

Committed: https://crrev.com/35b188a996e59c65787207818c7b90d70007f03d
Cr-Commit-Position: refs/heads/master@{#322666}

[David Tseng, 2015-03-13 18:51:56]

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

[David Tseng, 2015-03-13 18:53:22]

Ready for an initial look. There's an issue with the EnvObserver such that we
don't get the initial WindowTreeHost. Only after opening (or switching back to)
a window does the views-based windows get hooked up correctly. Still working on
that issue.

[dmazzoni, 2015-03-13 20:01:43]

looks fine, just one question

https://codereview.chromium.org/1003283002/diff/1/chrome/browser/ui/ash/acces...
File chrome/browser/ui/ash/accessibility/ax_root_obj_wrapper.cc (right):

https://codereview.chromium.org/1003283002/diff/1/chrome/browser/ui/ash/acces...
chrome/browser/ui/ash/accessibility/ax_root_obj_wrapper.cc:85: host_ = host;
Can't there be more than one WindowTreeHost, like if you have multiple monitors?

[sadrul, 2015-03-13 20:07:01]

sadrul@chromium.org changed reviewers:
+ oshima@chromium.org

[sadrul, 2015-03-13 20:07:01]

+oshima@

https://codereview.chromium.org/1003283002/diff/1/chrome/browser/ui/ash/acces...
File chrome/browser/ui/ash/accessibility/ax_root_obj_wrapper.cc (right):

https://codereview.chromium.org/1003283002/diff/1/chrome/browser/ui/ash/acces...
chrome/browser/ui/ash/accessibility/ax_root_obj_wrapper.cc:56: return;
This essentially means a WindowTreeHost was created before ash::Shell. Can you
clarify when that happens?

+oshima@ since I think it's a bug if this does in fact happen.

[oshima, 2015-03-13 20:40:03]

https://codereview.chromium.org/1003283002/diff/1/chrome/browser/ui/ash/acces...
File chrome/browser/ui/ash/accessibility/ax_root_obj_wrapper.cc (right):

https://codereview.chromium.org/1003283002/diff/1/chrome/browser/ui/ash/acces...
chrome/browser/ui/ash/accessibility/ax_root_obj_wrapper.cc:56: return;
On 2015/03/13 20:07:01, sadrul wrote:
> This essentially means a WindowTreeHost was created before ash::Shell. Can you
> clarify when that happens?
> 
> +oshima@ since I think it's a bug if this does in fact happen.

Yeah, this shouldn't happen (or you shouldn't try to access window before ash
shell is create, or after destroyed). Do you have a stack?

https://codereview.chromium.org/1003283002/diff/1/chrome/browser/ui/ash/acces...
chrome/browser/ui/ash/accessibility/ax_root_obj_wrapper.cc:85: host_ = host;
On 2015/03/13 20:01:43, dmazzoni wrote:
> Can't there be more than one WindowTreeHost, like if you have multiple
monitors?

yes, that's correct.

[David Tseng, 2015-03-13 21:33:53]

https://codereview.chromium.org/1003283002/diff/1/chrome/browser/ui/ash/acces...
File chrome/browser/ui/ash/accessibility/ax_root_obj_wrapper.cc (right):

https://codereview.chromium.org/1003283002/diff/1/chrome/browser/ui/ash/acces...
chrome/browser/ui/ash/accessibility/ax_root_obj_wrapper.cc:56: return;
On 2015/03/13 20:40:03, oshima wrote:
> On 2015/03/13 20:07:01, sadrul wrote:
> > This essentially means a WindowTreeHost was created before ash::Shell. Can
you
> > clarify when that happens?
> > 
> > +oshima@ since I think it's a bug if this does in fact happen.
> 
> Yeah, this shouldn't happen (or you shouldn't try to access window before ash
> shell is create, or after destroyed). Do you have a stack?

This code is meant to handle the linux/win case where USE_ASH is set but
shell::HasInstance() is false.

https://codereview.chromium.org/1003283002/diff/1/chrome/browser/ui/ash/acces...
chrome/browser/ui/ash/accessibility/ax_root_obj_wrapper.cc:85: host_ = host;
On 2015/03/13 20:40:03, oshima wrote:
> On 2015/03/13 20:01:43, dmazzoni wrote:
> > Can't there be more than one WindowTreeHost, like if you have multiple
> monitors?
> 
> yes, that's correct.

EnvObserver is how I'm keeping track of things. I can't use shell since it
doesn't get instantiated on linux (and maybe win). What's the canonical  way to
do otherwise?

[oshima, 2015-03-13 21:58:47]

https://codereview.chromium.org/1003283002/diff/1/chrome/browser/ui/ash/acces...
File chrome/browser/ui/ash/accessibility/ax_root_obj_wrapper.cc (right):

https://codereview.chromium.org/1003283002/diff/1/chrome/browser/ui/ash/acces...
chrome/browser/ui/ash/accessibility/ax_root_obj_wrapper.cc:56: return;
On 2015/03/13 21:33:53, David Tseng wrote:
> On 2015/03/13 20:40:03, oshima wrote:
> > On 2015/03/13 20:07:01, sadrul wrote:
> > > This essentially means a WindowTreeHost was created before ash::Shell. Can
> you
> > > clarify when that happens?
> > > 
> > > +oshima@ since I think it's a bug if this does in fact happen.
> > 
> > Yeah, this shouldn't happen (or you shouldn't try to access window before
ash
> > shell is create, or after destroyed). Do you have a stack?
> 
> This code is meant to handle the linux/win case where USE_ASH is set but
> shell::HasInstance() is false.

If that's the case, what you check screen type to tell if it's running on ash or
not.
(see gfx::Screen::GetScreenFor, and ScreenType in ui/gfx/screen_type_delegate.h)

https://codereview.chromium.org/1003283002/diff/1/chrome/browser/ui/ash/acces...
chrome/browser/ui/ash/accessibility/ax_root_obj_wrapper.cc:85: host_ = host;
On 2015/03/13 21:33:53, David Tseng wrote:
> On 2015/03/13 20:40:03, oshima wrote:
> > On 2015/03/13 20:01:43, dmazzoni wrote:
> > > Can't there be more than one WindowTreeHost, like if you have multiple
> > monitors?
> > 
> > yes, that's correct.
> 
> EnvObserver is how I'm keeping track of things. I can't use shell since it
> doesn't get instantiated on linux (and maybe win). What's the canonical  way
to
> do otherwise?

Can you explain how these wrapper objects are designed?

[David Tseng, 2015-03-16 17:00:52]

https://codereview.chromium.org/1003283002/diff/1/chrome/browser/ui/ash/acces...
File chrome/browser/ui/ash/accessibility/ax_root_obj_wrapper.cc (right):

https://codereview.chromium.org/1003283002/diff/1/chrome/browser/ui/ash/acces...
chrome/browser/ui/ash/accessibility/ax_root_obj_wrapper.cc:85: host_ = host;
On 2015/03/13 21:58:47, oshima wrote:
> On 2015/03/13 21:33:53, David Tseng wrote:
> > On 2015/03/13 20:40:03, oshima wrote:
> > > On 2015/03/13 20:01:43, dmazzoni wrote:
> > > > Can't there be more than one WindowTreeHost, like if you have multiple
> > > monitors?
> > > 
> > > yes, that's correct.
> > 
> > EnvObserver is how I'm keeping track of things. I can't use shell since it
> > doesn't get instantiated on linux (and maybe win). What's the canonical  way
> to
> > do otherwise?
> 
> Can you explain how these wrapper objects are designed?

These wrappers adapt various objects in Aura for use in the ui/accessibility
module which provides serialization and semantics for accessibility of UI.

For the case of Ash + shell, the AXRootObjWrapper acts as the root of all
windows. Unfortunately, I haven't found a way to enumerate all windows on
non-shell platforms. On ash shell (Chrome OS), I use shell::GetAllRootWindows.

[David Tseng, 2015-03-16 17:28:48]

https://codereview.chromium.org/1003283002/diff/1/chrome/browser/ui/ash/acces...
File chrome/browser/ui/ash/accessibility/ax_root_obj_wrapper.cc (right):

https://codereview.chromium.org/1003283002/diff/1/chrome/browser/ui/ash/acces...
chrome/browser/ui/ash/accessibility/ax_root_obj_wrapper.cc:56: return;
On 2015/03/13 21:58:47, oshima wrote:
> On 2015/03/13 21:33:53, David Tseng wrote:
> > On 2015/03/13 20:40:03, oshima wrote:
> > > On 2015/03/13 20:07:01, sadrul wrote:
> > > > This essentially means a WindowTreeHost was created before ash::Shell.
Can
> > you
> > > > clarify when that happens?
> > > > 
> > > > +oshima@ since I think it's a bug if this does in fact happen.
> > > 
> > > Yeah, this shouldn't happen (or you shouldn't try to access window before
> ash
> > > shell is create, or after destroyed). Do you have a stack?
> > 
> > This code is meant to handle the linux/win case where USE_ASH is set but
> > shell::HasInstance() is false.
> 
> If that's the case, what you check screen type to tell if it's running on ash
or
> not.
> (see gfx::Screen::GetScreenFor, and ScreenType in
ui/gfx/screen_type_delegate.h)

To be clear, I need to know if shell exists in order to decide whether I can use
shell::GetAllRootWindows. So, I don't think checking screen type above achieves
this.

[oshima, 2015-03-17 18:38:26]

On 2015/03/16 17:28:48, David Tseng wrote:
>
https://codereview.chromium.org/1003283002/diff/1/chrome/browser/ui/ash/acces...
> File chrome/browser/ui/ash/accessibility/ax_root_obj_wrapper.cc (right):
> 
>
https://codereview.chromium.org/1003283002/diff/1/chrome/browser/ui/ash/acces...
> chrome/browser/ui/ash/accessibility/ax_root_obj_wrapper.cc:56: return;
> On 2015/03/13 21:58:47, oshima wrote:
> > On 2015/03/13 21:33:53, David Tseng wrote:
> > > On 2015/03/13 20:40:03, oshima wrote:
> > > > On 2015/03/13 20:07:01, sadrul wrote:
> > > > > This essentially means a WindowTreeHost was created before ash::Shell.
> Can
> > > you
> > > > > clarify when that happens?
> > > > > 
> > > > > +oshima@ since I think it's a bug if this does in fact happen.
> > > > 
> > > > Yeah, this shouldn't happen (or you shouldn't try to access window
before
> > ash
> > > > shell is create, or after destroyed). Do you have a stack?
> > > 
> > > This code is meant to handle the linux/win case where USE_ASH is set but
> > > shell::HasInstance() is false.
> > 
> > If that's the case, what you check screen type to tell if it's running on
ash
> or
> > not.
> > (see gfx::Screen::GetScreenFor, and ScreenType in
> ui/gfx/screen_type_delegate.h)
> 
> To be clear, I need to know if shell exists in order to decide whether I can
use
> shell::GetAllRootWindows. So, I don't think checking screen type above
achieves
> this.

My point is that you shouldn't use HasInstace to check if it's running on ash.
You should be able to just use Shell::GetInstance(),
or there must be an issue in initialization/shutdown process if you had to.

[David Tseng, 2015-03-19 21:13:15]

PTAL.

[oshima, 2015-03-20 22:31:43]

https://codereview.chromium.org/1003283002/diff/120001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/accessibility/ax_root_obj_wrapper.cc (right):

https://codereview.chromium.org/1003283002/diff/120001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/accessibility/ax_root_obj_wrapper.cc:52: #if
defined(OS_CHROMEOS)
I still don't quite understand why you need this.

When/how you're getting null shell? Do you have the stack trace?

The only way you can get null shell is when you run Windows in desktop mode
only. (no WinAsh) but If you're not on ash, this method shouldn't be called?

[David Tseng, 2015-03-20 22:59:10]

Linux Aura.

[16251:16251:0320/155716:FATAL:shell.cc(202)] Check failed: instance_. #0
0x7f8ac784392e base::debug::StackTrace::StackTrace()
#1 0x7f8ac96e75d3 ash::Shell::GetInstance()
#2 0x7f8ac956253d AXRootObjWrapper::GetChildren()
#3 0x7f8ac9561038 ui::AXTreeSerializer<>::SerializeChangedNodes()
#4 0x7f8ac956047a ui::AXTreeSerializer<>::SerializeChanges()
#5 0x7f8ac955fd47 AutomationManagerAsh::SendEvent()
#6 0x7f8ac955fc89 AutomationManagerAsh::Enable()
#7 0x7f8acb403bdf extensions::AutomationInternalEnableDesktopFunction::Run()
#8 0x7f8aca13cf33
extensions::ExtensionFunctionDispatcher::DispatchWithCallbackInternal()
#9 0x7f8aca13cb0a extensions::ExtensionFunctionDispatcher::Dispatch()
#10 0x7f8aca13ff15 extensions::ExtensionHost::OnMessageReceived()
#11 0x7f8ac98a9236 content::WebContentsImpl::OnMessageReceived()
#12 0x7f8ac9824344 content::RenderViewHostImpl::OnMessageReceived()
#13 0x7f8ac981999e content::RenderProcessHostImpl::OnMessageReceived()
#14 0x7f8ac7db948c IPC::ChannelProxy::Context::OnDispatchMessage()
#15 0x7f8ac78a31ae base::debug::TaskAnnotator::RunTask()
#16 0x7f8ac785c172 base::MessageLoop::RunTask()
#17 0x7f8ac785c309 base::MessageLoop::DeferOrRunPendingTask()
#18 0x7f8ac785c6cb base::MessageLoop::DoWork()
#19 0x7f8ac78a232a base::(anonymous namespace)::WorkSourceDispatch()
#20 0x7f8ac5d21e04 g_main_context_dispatch
#21 0x7f8ac5d22048 <unknown>
#22 0x7f8ac5d220ec g_main_context_iteration
#23 0x7f8ac78a2202 base::MessagePumpGlib::Run()
#24 0x7f8ac786c190 base::RunLoop::Run()
#25 0x7f8ac7569ed8 ChromeBrowserMainParts::MainMessageLoopRun()
#26 0x7f8ac9946d84 content::BrowserMainLoop::RunMainMessageLoopParts()
#27 0x7f8ac976700d content::BrowserMainRunnerImpl::Run()
#28 0x7f8ac9766c13 content::BrowserMain()
#29 0x7f8ac77f1b35 content::ContentMainRunnerImpl::Run()
#30 0x7f8ac77f0790 content::ContentMain()
#31 0x7f8ac72d0898 ChromeMain
#32 0x7f8ac0077ec5 __libc_start_main
#33 0x7f8ac72d075f <unknown>


oshima@chromium.org writes:

>
https://codereview.chromium.org/1003283002/diff/120001/chrome/browser/ui/ash/...
> File chrome/browser/ui/ash/accessibility/ax_root_obj_wrapper.cc (right):
>
>
https://codereview.chromium.org/1003283002/diff/120001/chrome/browser/ui/ash/...
> chrome/browser/ui/ash/accessibility/ax_root_obj_wrapper.cc:52: #if
> defined(OS_CHROMEOS)
> I still don't quite understand why you need this.
>
> When/how you're getting null shell? Do you have the stack trace?
>
> The only way you can get null shell is when you run Windows in desktop
> mode only. (no WinAsh) but If you're not on ash, this method shouldn't
> be called?
>
> https://codereview.chromium.org/1003283002/


-- 
Foo X. Bar
http://www.example.com

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[oshima, 2015-03-20 23:16:47]

On 2015/03/20 22:59:10, David Tseng wrote:
> Linux Aura.
> 
> [16251:16251:0320/155716:FATAL:shell.cc(202)] Check failed: instance_. #0
> 0x7f8ac784392e base::debug::StackTrace::StackTrace()
> #1 0x7f8ac96e75d3 ash::Shell::GetInstance()
> #2 0x7f8ac956253d AXRootObjWrapper::GetChildren()
> #3 0x7f8ac9561038 ui::AXTreeSerializer<>::SerializeChangedNodes()
> #4 0x7f8ac956047a ui::AXTreeSerializer<>::SerializeChanges()
> #5 0x7f8ac955fd47 AutomationManagerAsh::SendEvent()
> #6 0x7f8ac955fc89 AutomationManagerAsh::Enable()
> #7 0x7f8acb403bdf extensions::AutomationInternalEnableDesktopFunction::Run()

According to this code:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

Enable() should be called only on ChromeOS build. Do you know why this is
enabled on linux aura as well?


> #8 0x7f8aca13cf33
> extensions::ExtensionFunctionDispatcher::DispatchWithCallbackInternal()
> #9 0x7f8aca13cb0a extensions::ExtensionFunctionDispatcher::Dispatch()
> #10 0x7f8aca13ff15 extensions::ExtensionHost::OnMessageReceived()
> #11 0x7f8ac98a9236 content::WebContentsImpl::OnMessageReceived()
> #12 0x7f8ac9824344 content::RenderViewHostImpl::OnMessageReceived()
> #13 0x7f8ac981999e content::RenderProcessHostImpl::OnMessageReceived()
> #14 0x7f8ac7db948c IPC::ChannelProxy::Context::OnDispatchMessage()
> #15 0x7f8ac78a31ae base::debug::TaskAnnotator::RunTask()
> #16 0x7f8ac785c172 base::MessageLoop::RunTask()
> #17 0x7f8ac785c309 base::MessageLoop::DeferOrRunPendingTask()
> #18 0x7f8ac785c6cb base::MessageLoop::DoWork()
> #19 0x7f8ac78a232a base::(anonymous namespace)::WorkSourceDispatch()
> #20 0x7f8ac5d21e04 g_main_context_dispatch
> #21 0x7f8ac5d22048 <unknown>
> #22 0x7f8ac5d220ec g_main_context_iteration
> #23 0x7f8ac78a2202 base::MessagePumpGlib::Run()
> #24 0x7f8ac786c190 base::RunLoop::Run()
> #25 0x7f8ac7569ed8 ChromeBrowserMainParts::MainMessageLoopRun()
> #26 0x7f8ac9946d84 content::BrowserMainLoop::RunMainMessageLoopParts()
> #27 0x7f8ac976700d content::BrowserMainRunnerImpl::Run()
> #28 0x7f8ac9766c13 content::BrowserMain()
> #29 0x7f8ac77f1b35 content::ContentMainRunnerImpl::Run()
> #30 0x7f8ac77f0790 content::ContentMain()
> #31 0x7f8ac72d0898 ChromeMain
> #32 0x7f8ac0077ec5 __libc_start_main
> #33 0x7f8ac72d075f <unknown>
> 
> 
> mailto:oshima@chromium.org writes:
> 
> >
>
https://codereview.chromium.org/1003283002/diff/120001/chrome/browser/ui/ash/...
> > File chrome/browser/ui/ash/accessibility/ax_root_obj_wrapper.cc (right):
> >
> >
>
https://codereview.chromium.org/1003283002/diff/120001/chrome/browser/ui/ash/...
> > chrome/browser/ui/ash/accessibility/ax_root_obj_wrapper.cc:52: #if
> > defined(OS_CHROMEOS)
> > I still don't quite understand why you need this.
> >
> > When/how you're getting null shell? Do you have the stack trace?
> >
> > The only way you can get null shell is when you run Windows in desktop
> > mode only. (no WinAsh) but If you're not on ash, this method shouldn't
> > be called?
> >
> > https://codereview.chromium.org/1003283002/
> 
> 
> -- 
> Foo X. Bar
> http://www.example.com
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[David Tseng, 2015-03-23 16:04:22]

oshima@chromium.org writes:

> On 2015/03/20 22:59:10, David Tseng wrote:
>> Linux Aura.
>
>> [16251:16251:0320/155716:FATAL:shell.cc(202)] Check failed: instance_. #0
>> 0x7f8ac784392e base::debug::StackTrace::StackTrace()
>> #1 0x7f8ac96e75d3 ash::Shell::GetInstance()
>> #2 0x7f8ac956253d AXRootObjWrapper::GetChildren()
>> #3 0x7f8ac9561038 ui::AXTreeSerializer<>::SerializeChangedNodes()
>> #4 0x7f8ac956047a ui::AXTreeSerializer<>::SerializeChanges()
>> #5 0x7f8ac955fd47 AutomationManagerAsh::SendEvent()
>> #6 0x7f8ac955fc89 AutomationManagerAsh::Enable()
>> #7 0x7f8acb403bdf  
>> extensions::AutomationInternalEnableDesktopFunction::Run()
>
> According to this code:
>
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
>
> Enable() should be called only on ChromeOS build. Do you know why this is
> enabled on linux aura as well?
>
That's the point of this change -- to make this extension api
(chrome.automation.getDesktop), available on aura.


>
>> #8 0x7f8aca13cf33
>> extensions::ExtensionFunctionDispatcher::DispatchWithCallbackInternal()
>> #9 0x7f8aca13cb0a extensions::ExtensionFunctionDispatcher::Dispatch()
>> #10 0x7f8aca13ff15 extensions::ExtensionHost::OnMessageReceived()
>> #11 0x7f8ac98a9236 content::WebContentsImpl::OnMessageReceived()
>> #12 0x7f8ac9824344 content::RenderViewHostImpl::OnMessageReceived()
>> #13 0x7f8ac981999e content::RenderProcessHostImpl::OnMessageReceived()
>> #14 0x7f8ac7db948c IPC::ChannelProxy::Context::OnDispatchMessage()
>> #15 0x7f8ac78a31ae base::debug::TaskAnnotator::RunTask()
>> #16 0x7f8ac785c172 base::MessageLoop::RunTask()
>> #17 0x7f8ac785c309 base::MessageLoop::DeferOrRunPendingTask()
>> #18 0x7f8ac785c6cb base::MessageLoop::DoWork()
>> #19 0x7f8ac78a232a base::(anonymous namespace)::WorkSourceDispatch()
>> #20 0x7f8ac5d21e04 g_main_context_dispatch
>> #21 0x7f8ac5d22048 <unknown>
>> #22 0x7f8ac5d220ec g_main_context_iteration
>> #23 0x7f8ac78a2202 base::MessagePumpGlib::Run()
>> #24 0x7f8ac786c190 base::RunLoop::Run()
>> #25 0x7f8ac7569ed8 ChromeBrowserMainParts::MainMessageLoopRun()
>> #26 0x7f8ac9946d84 content::BrowserMainLoop::RunMainMessageLoopParts()
>> #27 0x7f8ac976700d content::BrowserMainRunnerImpl::Run()
>> #28 0x7f8ac9766c13 content::BrowserMain()
>> #29 0x7f8ac77f1b35 content::ContentMainRunnerImpl::Run()
>> #30 0x7f8ac77f0790 content::ContentMain()
>> #31 0x7f8ac72d0898 ChromeMain
>> #32 0x7f8ac0077ec5 __libc_start_main
>> #33 0x7f8ac72d075f <unknown>
>
>
>> mailto:oshima@chromium.org writes:
>
>> >
>
>
https://codereview.chromium.org/1003283002/diff/120001/chrome/browser/ui/ash/...
>> > File chrome/browser/ui/ash/accessibility/ax_root_obj_wrapper.cc (right):
>> >
>> >
>
>
https://codereview.chromium.org/1003283002/diff/120001/chrome/browser/ui/ash/...
>> > chrome/browser/ui/ash/accessibility/ax_root_obj_wrapper.cc:52: #if
>> > defined(OS_CHROMEOS)
>> > I still don't quite understand why you need this.
>> >
>> > When/how you're getting null shell? Do you have the stack trace?
>> >
>> > The only way you can get null shell is when you run Windows in desktop
>> > mode only. (no WinAsh) but If you're not on ash, this method shouldn't
>> > be called?
>> >
>> > https://codereview.chromium.org/1003283002/
>
>
>> --
>> Foo X. Bar
>> http://www.example.com
>
>> To unsubscribe from this group and stop receiving emails from it, send an
> email
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>
>
> https://codereview.chromium.org/1003283002/
>
> To unsubscribe from this group and stop receiving emails from it, send an
email to chromium-reviews+unsubscribe@chromium.org.


-- 
Foo X. Bar
http://www.example.com

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[oshima, 2015-03-23 19:21:49]

On 2015/03/23 16:04:22, David Tseng wrote:
> mailto:oshima@chromium.org writes:
> 
> > On 2015/03/20 22:59:10, David Tseng wrote:
> >> Linux Aura.
> >
> >> [16251:16251:0320/155716:FATAL:shell.cc(202)] Check failed: instance_. #0
> >> 0x7f8ac784392e base::debug::StackTrace::StackTrace()
> >> #1 0x7f8ac96e75d3 ash::Shell::GetInstance()
> >> #2 0x7f8ac956253d AXRootObjWrapper::GetChildren()
> >> #3 0x7f8ac9561038 ui::AXTreeSerializer<>::SerializeChangedNodes()
> >> #4 0x7f8ac956047a ui::AXTreeSerializer<>::SerializeChanges()
> >> #5 0x7f8ac955fd47 AutomationManagerAsh::SendEvent()
> >> #6 0x7f8ac955fc89 AutomationManagerAsh::Enable()
> >> #7 0x7f8acb403bdf  
> >> extensions::AutomationInternalEnableDesktopFunction::Run()
> >
> > According to this code:
> >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
> >
> > Enable() should be called only on ChromeOS build. Do you know why this is
> > enabled on linux aura as well?
> >
> That's the point of this change -- to make this extension api
> (chrome.automation.getDesktop), available on aura.
> 
> 
> >
> >> #8 0x7f8aca13cf33
> >> extensions::ExtensionFunctionDispatcher::DispatchWithCallbackInternal()
> >> #9 0x7f8aca13cb0a extensions::ExtensionFunctionDispatcher::Dispatch()
> >> #10 0x7f8aca13ff15 extensions::ExtensionHost::OnMessageReceived()
> >> #11 0x7f8ac98a9236 content::WebContentsImpl::OnMessageReceived()
> >> #12 0x7f8ac9824344 content::RenderViewHostImpl::OnMessageReceived()
> >> #13 0x7f8ac981999e content::RenderProcessHostImpl::OnMessageReceived()
> >> #14 0x7f8ac7db948c IPC::ChannelProxy::Context::OnDispatchMessage()
> >> #15 0x7f8ac78a31ae base::debug::TaskAnnotator::RunTask()
> >> #16 0x7f8ac785c172 base::MessageLoop::RunTask()
> >> #17 0x7f8ac785c309 base::MessageLoop::DeferOrRunPendingTask()
> >> #18 0x7f8ac785c6cb base::MessageLoop::DoWork()
> >> #19 0x7f8ac78a232a base::(anonymous namespace)::WorkSourceDispatch()
> >> #20 0x7f8ac5d21e04 g_main_context_dispatch
> >> #21 0x7f8ac5d22048 <unknown>
> >> #22 0x7f8ac5d220ec g_main_context_iteration
> >> #23 0x7f8ac78a2202 base::MessagePumpGlib::Run()
> >> #24 0x7f8ac786c190 base::RunLoop::Run()
> >> #25 0x7f8ac7569ed8 ChromeBrowserMainParts::MainMessageLoopRun()
> >> #26 0x7f8ac9946d84 content::BrowserMainLoop::RunMainMessageLoopParts()
> >> #27 0x7f8ac976700d content::BrowserMainRunnerImpl::Run()
> >> #28 0x7f8ac9766c13 content::BrowserMain()
> >> #29 0x7f8ac77f1b35 content::ContentMainRunnerImpl::Run()
> >> #30 0x7f8ac77f0790 content::ContentMain()
> >> #31 0x7f8ac72d0898 ChromeMain
> >> #32 0x7f8ac0077ec5 __libc_start_main
> >> #33 0x7f8ac72d075f <unknown>
> >
> >
> >> mailto:oshima@chromium.org writes:
> >
> >> >
> >
> >
>
https://codereview.chromium.org/1003283002/diff/120001/chrome/browser/ui/ash/...
> >> > File chrome/browser/ui/ash/accessibility/ax_root_obj_wrapper.cc (right):
> >> >
> >> >
> >
> >
>
https://codereview.chromium.org/1003283002/diff/120001/chrome/browser/ui/ash/...
> >> > chrome/browser/ui/ash/accessibility/ax_root_obj_wrapper.cc:52: #if
> >> > defined(OS_CHROMEOS)
> >> > I still don't quite understand why you need this.
> >> >
> >> > When/how you're getting null shell? Do you have the stack trace?
> >> >
> >> > The only way you can get null shell is when you run Windows in desktop
> >> > mode only. (no WinAsh) but If you're not on ash, this method shouldn't
> >> > be called?
> >> >
> >> > https://codereview.chromium.org/1003283002/
> >
> >
> >> --
> >> Foo X. Bar
> >> http://www.example.com
> >
> >> To unsubscribe from this group and stop receiving emails from it, send an
> > email
> >> to mailto:chromium-reviews+unsubscribe@chromium.org.
> >
> >
> > https://codereview.chromium.org/1003283002/
> >
> > To unsubscribe from this group and stop receiving emails from it, send an
> email to mailto:chromium-reviews+unsubscribe@chromium.org.
> 
> 
> -- 
> Foo X. Bar
> http://www.example.com
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

I think the issue is that you're using ash (c/b/ui/ash) code to initialize this
for aura, which is wrong.

You probably should refactor AutomationManagerAsh and use delegate to get all
root windows on aura and ash?

Note that on windows, ash and aura may co-exist, so you need be able to deal
with that.

[chromium-reviews, 2015-03-24 18:31:06]

PTAL. I went ahead and used your concerns as an excuse to eliminate
AutomationManagerAsh's dependency on shell entirely. I can move
AutomationManagerAsh to c/b/u/views if you'd like and rename to
AutomationManagerViews.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[David Tseng, 2015-03-25 18:05:54]

PTAL. Went ahead and moved c/b/u/ash/a/* to c/b/u/aura/a/ and renamed
accordingly.

A few notes:
- the ax_tree_source_aura_unittest has been left an ash-based test. I don't
see any equivalents for aura.
- I had to remove two tests. AXTreeSourceAuraTest Serialization can't be
easily hard coded now that the tree is dynamically loaded whenever an event
fires. Same goes for the ChromeVox desktop focus test which relied on the
tree being entirely present to do a find.
- the latest patchset contains a fix to AutomationManagerAura that was
uncovered by the overall change. Basically, a serialization of a tre could
cause further events to get fired. In particular, WebView fires a children
changed event when it first accesses its "child" web contents. This
resulted in what looked like out of order eventing; it was simply just
nested events causing us to send events in reverse. Added a pending events
queue to AutomationManagerAura to help mitigate this issue.

On Tue, Mar 24, 2015 at 11:31 AM, David Tseng <dtseng@google.com> wrote:

> PTAL. I went ahead and used your concerns as an excuse to eliminate
> AutomationManagerAsh's dependency on shell entirely. I can move
> AutomationManagerAsh to c/b/u/views if you'd like and rename to
> AutomationManagerViews.
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[oshima, 2015-03-25 22:16:53]

looks good to me overall.

On 2015/03/25 18:05:54, David Tseng wrote:
> PTAL. Went ahead and moved c/b/u/ash/a/* to c/b/u/aura/a/ and renamed
> accordingly.
> 
> A few notes:
> - the ax_tree_source_aura_unittest has been left an ash-based test. I don't
> see any equivalents for aura.

In that case, you can leave the test in ash directory.

> - I had to remove two tests. AXTreeSourceAuraTest Serialization can't be
> easily hard coded now that the tree is dynamically loaded whenever an event
> fires. Same goes for the ChromeVox desktop focus test which relied on the
> tree being entirely present to do a find.
> - the latest patchset contains a fix to AutomationManagerAura that was
> uncovered by the overall change. Basically, a serialization of a tre could
> cause further events to get fired. In particular, WebView fires a children
> changed event when it first accesses its "child" web contents. This
> resulted in what looked like out of order eventing; it was simply just
> nested events causing us to send events in reverse. Added a pending events
> queue to AutomationManagerAura to help mitigate this issue.

I'll leave these to other reviewers. 

> On Tue, Mar 24, 2015 at 11:31 AM, David Tseng <mailto:dtseng@google.com>
wrote:
> 
> > PTAL. I went ahead and used your concerns as an excuse to eliminate
> > AutomationManagerAsh's dependency on shell entirely. I can move
> > AutomationManagerAsh to c/b/u/views if you'd like and rename to
> > AutomationManagerViews.
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[David Tseng, 2015-03-26 16:25:16]

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

[David Tseng, 2015-03-26 16:25:17]

oshima, Moved test files back to c/b/u/ash/accessibility.
+ sky for c/b/u/v.
dmazzoni, think we're ready for you to take a look now.

[sky, 2015-03-26 17:47:59]

chrome/browser/ui/views/chrome_views_delegate.cc LGTM

[dmazzoni, 2015-03-26 18:44:58]

https://codereview.chromium.org/1003283002/diff/200001/chrome/browser/extensi...
File chrome/browser/extensions/api/automation/automation_apitest.cc (right):

https://codereview.chromium.org/1003283002/diff/200001/chrome/browser/extensi...
chrome/browser/extensions/api/automation/automation_apitest.cc:170: //
http://crbug.com/435449
nit: remove this line since the test isn't failing anymore

https://codereview.chromium.org/1003283002/diff/200001/chrome/browser/resourc...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/background_test.extjs
(left):

https://codereview.chromium.org/1003283002/diff/200001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background_test.extjs:73:
TEST_F('BackgroundTest', 'DesktopFocus', function() {
Why are we losing this test?

https://codereview.chromium.org/1003283002/diff/200001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/accessibility/ax_tree_source_aura_unittest.cc (left):

https://codereview.chromium.org/1003283002/diff/200001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/accessibility/ax_tree_source_aura_unittest.cc:105:
TEST_F(AXTreeSourceAshTest, Serialization) {
Are we losing this test too?

https://codereview.chromium.org/1003283002/diff/200001/chrome/browser/ui/aura...
File chrome/browser/ui/aura/accessibility/automation_manager_aura.cc (right):

https://codereview.chromium.org/1003283002/diff/200001/chrome/browser/ui/aura...
chrome/browser/ui/aura/accessibility/automation_manager_aura.cc:71:
processing_events_ = false;
Should you set it back to false after sending the pending events, below?

[David Tseng, 2015-03-26 23:20:48]

https://codereview.chromium.org/1003283002/diff/200001/chrome/browser/resourc...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/background_test.extjs
(left):

https://codereview.chromium.org/1003283002/diff/200001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background_test.extjs:73:
TEST_F('BackgroundTest', 'DesktopFocus', function() {
On 2015/03/26 18:44:58, dmazzoni wrote:
> Why are we losing this test?

The initial desktop tree is just the root. Until an event gets fired on
something like focus on the status tray, that's all we get. Once focus fires,
the serializer walks up to the window causing the AXAuraObjCache to cache that
window and then, find on an automation node will reach the status tray.

If you know how to focus the status tray from js, we could do that.

https://codereview.chromium.org/1003283002/diff/200001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/accessibility/ax_tree_source_aura_unittest.cc (left):

https://codereview.chromium.org/1003283002/diff/200001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/accessibility/ax_tree_source_aura_unittest.cc:105:
TEST_F(AXTreeSourceAshTest, Serialization) {
On 2015/03/26 18:44:58, dmazzoni wrote:
> Are we losing this test too?



Fixed this one.

https://codereview.chromium.org/1003283002/diff/200001/chrome/browser/ui/aura...
File chrome/browser/ui/aura/accessibility/automation_manager_aura.cc (right):

https://codereview.chromium.org/1003283002/diff/200001/chrome/browser/ui/aura...
chrome/browser/ui/aura/accessibility/automation_manager_aura.cc:71:
processing_events_ = false;
On 2015/03/26 18:44:58, dmazzoni wrote:
> Should you set it back to false after sending the pending events, below?

Done.

[dmazzoni, 2015-03-26 23:32:04]

lgtm

https://codereview.chromium.org/1003283002/diff/200001/chrome/browser/resourc...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/background_test.extjs
(left):

https://codereview.chromium.org/1003283002/diff/200001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background_test.extjs:73:
TEST_F('BackgroundTest', 'DesktopFocus', function() {
On 2015/03/26 23:20:47, David Tseng wrote:
> On 2015/03/26 18:44:58, dmazzoni wrote:
> > Why are we losing this test?
> 
> The initial desktop tree is just the root. Until an event gets fired on
> something like focus on the status tray, that's all we get. Once focus fires,
> the serializer walks up to the window causing the AXAuraObjCache to cache that
> window and then, find on an automation node will reach the status tray.
> 
> If you know how to focus the status tray from js, we could do that.

I know we can't iterate over all windows easily, but I wonder if we could do
something in C++ code when the desktop tree is first requested, like fire an
accessibility focus event on the currently focused view?

What do you think about disabling this test for now rather than deleting it? I
really would like to see tests for things like the status tray one way or
another.

[David Tseng, 2015-03-27 17:04:38]

https://codereview.chromium.org/1003283002/diff/200001/chrome/browser/resourc...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/background_test.extjs
(left):

https://codereview.chromium.org/1003283002/diff/200001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background_test.extjs:73:
TEST_F('BackgroundTest', 'DesktopFocus', function() {
On 2015/03/26 23:32:04, dmazzoni wrote:
> On 2015/03/26 23:20:47, David Tseng wrote:
> > On 2015/03/26 18:44:58, dmazzoni wrote:
> > > Why are we losing this test?
> > 
> > The initial desktop tree is just the root. Until an event gets fired on
> > something like focus on the status tray, that's all we get. Once focus
fires,
> > the serializer walks up to the window causing the AXAuraObjCache to cache
that
> > window and then, find on an automation node will reach the status tray.
> > 
> > If you know how to focus the status tray from js, we could do that.
> 
> I know we can't iterate over all windows easily, but I wonder if we could do
> something in C++ code when the desktop tree is first requested, like fire an
> accessibility focus event on the currently focused view?
> 
> What do you think about disabling this test for now rather than deleting it? I
> really would like to see tests for things like the status tray one way or
> another.

How about if we add a helper binding that can focus the status tray? I'm not
sure if it helps to keep a disabled test around though as long as we file a bug
for this.

I like the focus idea, but I don't think that would help in this case because we
need to have more native code mixed in among the js calls. This framework
doesn't really allow for that.

[dmazzoni, 2015-03-27 18:56:33]

>
> How about if we add a helper binding that can focus the status tray?


That sounds great.

I like the focus idea, but I don't think that would help in this case
> because we need to have more native code mixed in among the js calls.
> This framework doesn't really allow for that.


I meant to do that every time the desktop tree is requested, not just from
this test. It'd basically tickle the desktop tree and try to populate it
rather than leaving it empty.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[David Tseng, 2015-03-27 20:55:50]

The CQ bit was checked by dtseng@chromium.org

[David Tseng, 2015-03-27 20:55:50]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/1003283002/#ps310001
(title: "Actions.")

[commit-bot: I haz the power, 2015-03-27 20:56:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1003283002/310001

[commit-bot: I haz the power, 2015-03-27 22:56:37]

Committed patchset #17 (id:310001)

[commit-bot: I haz the power, 2015-03-27 22:57:37]

Patchset 17 (id:??) landed as
https://crrev.com/35b188a996e59c65787207818c7b90d70007f03d
Cr-Commit-Position: refs/heads/master@{#322666}

[scheib, 2015-03-28 00:23:41]

On 2015/03/27 22:57:37, I haz the power (commit-bot) wrote:
> Patchset 17 (id:??) landed as
> https://crrev.com/35b188a996e59c65787207818c7b90d70007f03d
> Cr-Commit-Position: refs/heads/master@{#322666}

Manually reverted in https://codereview.chromium.org/1040863002/