Explicit initialization of aura::Env for browser shell.

Explicit initialization of aura::Env for browser shell.

Most implementations (x11 aura with and without chromeos) do not explicitly
call aura::Env::GetInstance() to initialize it. These build work as is because
the call is done when performing other operations, such as adding observers.

In OZONE there are no such calls, so the OZONE build fails when trying to
create a compositor since the Compositor::Initialize hasn't been called.

This patch adds an explicit initialization for aura based builds to fix this
issue.

BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=227745

[Ben Goodger (Google), 2013-09-06 17:08:38]

https://codereview.chromium.org/23882007/diff/1/content/shell/browser/shell_a...
File content/shell/browser/shell_aura.cc (right):

https://codereview.chromium.org/23882007/diff/1/content/shell/browser/shell_a...
content/shell/browser/shell_aura.cc:293: aura::Env::GetInstance();
Perhaps we should have an explicit CreateInstance method and just require
everyone call it in their startup sequence. Then we can check instance_ in
GetInstance() to avoid this situation in the future.

[dnicoara, 2013-09-10 19:14:59]

I hope this is close to what you have in mind. Let me know if my comments don't
make sense, I'll add more info in that case.

https://codereview.chromium.org/23882007/diff/8001/ash/test/ash_test_helper.cc
File ash/test/ash_test_helper.cc (right):

https://codereview.chromium.org/23882007/diff/8001/ash/test/ash_test_helper.c...
ash/test/ash_test_helper.cc:114: aura::Env::CreateInstance();
This is needed since this function is being called before the shell
initialization.

https://codereview.chromium.org/23882007/diff/8001/ui/aura/env.cc
File ui/aura/env.cc (right):

https://codereview.chromium.org/23882007/diff/8001/ui/aura/env.cc#newcode44
ui/aura/env.cc:44: if (!instance_) {
I would have liked to have this as a check and only allow calls to  this
function only when |instance_| hasn't been created unfortunately call ordering
in tests (see comment in ash_test_helper.cc) doesn't allow it. This would also
depend on moving the aura::Env::CreateInstance call in aura::RootWindow to
content::MinimalShell, though I think we don't want to do that since we want
creation as close as possible to first use.

https://codereview.chromium.org/23882007/diff/8001/ui/aura/root_window.cc
File ui/aura/root_window.cc (right):

https://codereview.chromium.org/23882007/diff/8001/ui/aura/root_window.cc#new...
ui/aura/root_window.cc:163: Env::CreateInstance();
On OZONE builds this is needed since the minimal_shell doesn't create an
instance like in ash.

[Ben Goodger (Google), 2013-09-10 19:21:03]

https://codereview.chromium.org/23882007/diff/8001/ui/aura/root_window.cc
File ui/aura/root_window.cc (right):

https://codereview.chromium.org/23882007/diff/8001/ui/aura/root_window.cc#new...
ui/aura/root_window.cc:163: Env::CreateInstance();
Why not? Not sure I like this.

[dnicoara, 2013-09-10 19:56:55]

https://codereview.chromium.org/23882007/diff/8001/ui/aura/root_window.cc
File ui/aura/root_window.cc (right):

https://codereview.chromium.org/23882007/diff/8001/ui/aura/root_window.cc#new...
ui/aura/root_window.cc:163: Env::CreateInstance();
On 2013/09/10 19:21:03, Ben Goodger (Google) wrote:
> Why not? Not sure I like this.

minimal_shell doesn't perform any aura calls, so it never needs to create the
aura Env. Should I just more this call there?

[Ben Goodger (Google), 2013-09-10 20:38:33]

On 2013/09/10 19:56:55, dnicoara wrote:
> https://codereview.chromium.org/23882007/diff/8001/ui/aura/root_window.cc
> File ui/aura/root_window.cc (right):
> 
>
https://codereview.chromium.org/23882007/diff/8001/ui/aura/root_window.cc#new...
> ui/aura/root_window.cc:163: Env::CreateInstance();
> On 2013/09/10 19:21:03, Ben Goodger (Google) wrote:
> > Why not? Not sure I like this.
> 
> minimal_shell doesn't perform any aura calls, so it never needs to create the
> aura Env. Should I just more this call there?
If minimal_shell doesn't do any aura stuff, who creates the RootWindow?

[dnicoara, 2013-09-10 20:42:25]

On 2013/09/10 20:38:33, Ben Goodger (Google) wrote:
> On 2013/09/10 19:56:55, dnicoara wrote:
> > https://codereview.chromium.org/23882007/diff/8001/ui/aura/root_window.cc
> > File ui/aura/root_window.cc (right):
> > 
> >
>
https://codereview.chromium.org/23882007/diff/8001/ui/aura/root_window.cc#new...
> > ui/aura/root_window.cc:163: Env::CreateInstance();
> > On 2013/09/10 19:21:03, Ben Goodger (Google) wrote:
> > > Why not? Not sure I like this.
> > 
> > minimal_shell doesn't perform any aura calls, so it never needs to create
the
> > aura Env. Should I just more this call there?
> If minimal_shell doesn't do any aura stuff, who creates the RootWindow?

I'm sorry, I didn't express myself correctly. It doesn't perform any aura
operation that requires explicit call to aura::Env::GetInstance(). It does
create the RootWindow which would need the compositor to be initialized (which
requires a call to aura::Env::CreateInstance()).

[Ben Goodger (Google), 2013-09-10 20:54:59]

On 2013/09/10 20:42:25, dnicoara wrote:
> I'm sorry, I didn't express myself correctly. It doesn't perform any aura
> operation that requires explicit call to aura::Env::GetInstance(). It does
> create the RootWindow which would need the compositor to be initialized (which
> requires a call to aura::Env::CreateInstance()).

OK. In this case then yes minimal_shell should call CreateInstance().

[dnicoara, 2013-09-10 21:03:52]

On 2013/09/10 20:54:59, Ben Goodger (Google) wrote:
> On 2013/09/10 20:42:25, dnicoara wrote:
> > I'm sorry, I didn't express myself correctly. It doesn't perform any aura
> > operation that requires explicit call to aura::Env::GetInstance(). It does
> > create the RootWindow which would need the compositor to be initialized
(which
> > requires a call to aura::Env::CreateInstance()).
> 
> OK. In this case then yes minimal_shell should call CreateInstance().

Done.

[Ben Goodger (Google), 2013-09-10 21:04:31]

lgtm

[dnicoara, 2013-09-17 17:53:16]

Ben, could you please take another look at the CL? I had to add a few more
changes for the Win tests and changes to the #ifdefs in aura::Env to compile on
Android, Mac and Windows.

Thank you!

[Ben Goodger (Google), 2013-09-17 20:39:03]

https://codereview.chromium.org/23882007/diff/48001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/23882007/diff/48001/ash/shell.cc#newcode233
ash/shell.cc:233: base::MessagePumpX11::Current()->AddDispatcherForRootWindow(
is this just the result of a rebase?

https://codereview.chromium.org/23882007/diff/48001/ui/views/widget/desktop_a...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right):

https://codereview.chromium.org/23882007/diff/48001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:270:
aura::Env::CreateInstance();
Seems like this should only be done once per process, but there can be many
DNWAs. Can't you move this to the startup sequence?

[dnicoara, 2013-09-17 21:01:27]

https://codereview.chromium.org/23882007/diff/48001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/23882007/diff/48001/ash/shell.cc#newcode233
ash/shell.cc:233: base::MessagePumpX11::Current()->AddDispatcherForRootWindow(
On 2013/09/17 20:39:04, Ben Goodger (Google) wrote:
> is this just the result of a rebase?

Yes.

https://codereview.chromium.org/23882007/diff/48001/ui/views/widget/desktop_a...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right):

https://codereview.chromium.org/23882007/diff/48001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:270:
aura::Env::CreateInstance();
On 2013/09/17 20:39:04, Ben Goodger (Google) wrote:
> Seems like this should only be done once per process, but there can be many
> DNWAs. Can't you move this to the startup sequence?

I see. It probably could be moved higher, but not too sure how high we want it.
I originally wanted to have it in the test SetUp, but that didn't work for these
tests.

The tests I was trying to fix are the failing ones in:
http://build.chromium.org/p/tryserver.chromium/builders/linux_aura/builds/75958

DesktopBrowserFrameAura would be another Aura specific class where it could go,
but it looks like it would have the same issue you mentioned above.

What would you recommend?

One stack trace is:

 [] aura::Env::GetInstance()
 [] aura::Window::Init()
 [] views::DesktopNativeWidgetAura::InitNativeWidget()
 [] DesktopBrowserFrameAura::InitNativeWidget()
 [] views::Widget::Init()
 [] BrowserFrame::InitBrowserFrame()
 [] BrowserWindow::CreateBrowserWindow()
 [] Browser::Browser()
 [] StartupBrowserCreatorImpl::OpenTabsInBrowser()
 [] StartupBrowserCreatorImpl::ProcessSpecifiedURLs()
 [] StartupBrowserCreatorImpl::ProcessStartupURLs()
 [] StartupBrowserCreatorImpl::ProcessLaunchURLs()
 [] StartupBrowserCreatorImpl::Launch()
 [] StartupBrowserCreator::LaunchBrowser()
 [] StartupBrowserCreator::ProcessCmdLineImpl()
 [] ChromeBrowserMainParts::PreMainMessageLoopRunImpl()
 [] ChromeBrowserMainParts::PreMainMessageLoopRun()
 [] content::BrowserMainLoop::PreMainMessageLoopRun()
 [] content::StartupTaskRunner::RunAllTasksNow()
 [] content::BrowserMainLoop::CreateStartupTasks()
 [] content::BrowserMainRunnerImpl::Initialize()
 [] content::BrowserMain()
 [] content::BrowserTestBase::SetUp()
 [] InProcessBrowserTest::SetUp()
 [] testing::Test::Run()
 [] testing::TestInfo::Run()
 [] testing::TestCase::Run()
 [] testing::internal::UnitTestImpl::RunAllTests()
 [] testing::UnitTest::Run()

[Ben Goodger (Google), 2013-09-17 21:55:15]

>  [] ChromeBrowserMainParts::PreMainMessageLoopRunImpl()

There's probably already other aura setup stuff here


>  [] ChromeBrowserMainParts::PreMainMessageLoopRun()

.. or maybe here.


-Ben

[dnicoara, 2013-10-01 13:56:54]

Added aura::Env::CreateInstance to FakeExternalTab to fix chrome_frame_net_test
on Windows.

Your initial suggestion was to use PreProfileInit in
chrome_browser_main_extra_parts_aura.cc, however that doesn't work since it
isn't in the call path due to the use of a custom content::ContentMainDelegate
(FakeMainDelegate).

[Ben Goodger (Google), 2013-10-01 23:21:49]

https://codereview.chromium.org/23882007/diff/92001/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/23882007/diff/92001/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main.cc:1474: aura::Env::CreateInstance();
If this needs to be in this position you should add a comment stating why it
needs to be in this precise spot.

[dnicoara, 2013-10-02 18:45:29]

https://codereview.chromium.org/23882007/diff/92001/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/23882007/diff/92001/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main.cc:1474: aura::Env::CreateInstance();
On 2013/10/01 23:21:49, Ben Goodger (Google) wrote:
> If this needs to be in this position you should add a comment stating why it
> needs to be in this precise spot.

Done. Let me know if it needs more detail.

[Ben Goodger (Google), 2013-10-03 18:10:18]

lgtm

[commit-bot: I haz the power, 2013-10-04 13:54:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dnicoara@chromium.org/23882007/111001

[commit-bot: I haz the power, 2013-10-04 14:47:35]

Retried try job too often on linux_chromeos for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[dnicoara, 2013-10-04 23:06:48]

Ben, could you please take another look. I had to rebase and move the
Env::CreateInstance call slightly earlier in the same method in ash/shell.cc to
fix linux_chromeos (due to recent changes).

Thank you,
Danny

[Ben Goodger (Google), 2013-10-08 23:38:09]

lgtm

[commit-bot: I haz the power, 2013-10-09 13:27:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dnicoara@chromium.org/23882007/148001

[commit-bot: I haz the power, 2013-10-09 16:19:12]

Change committed as 227745