Move LayoutProvider initialization to later in the startup process.

Move LayoutProvider initialization to later in the startup process.

CreateLayoutProvider() relies on ui::MaterialDesignController::Intialize() having already been called. That function cannot be called until the command-line arguments are available, which happens when PreCreateThreads() is called.

BUG=687349
Review-Url: https://codereview.chromium.org/2818123002
Cr-Commit-Position: refs/heads/master@{#464777}
Committed: https://chromium.googlesource.com/chromium/src/+/d61d7204eab9c8d63173a8fbadeaac9a2e816fae

[kylix_rd, 2017-04-14 17:11:52]

Description was changed from

==========
Move LayoutProvider initialization to later in the startup process.

CreateLayoutProvider() relies on ui::MaterialDesignController::Intialize()
having already been called. That function cannot be called until the
command-line arguments are available, which happens when PreCreateThreads() is
called.

BUG=687349
==========

to

==========
Move LayoutProvider initialization to later in the startup process.

CreateLayoutProvider() relies on ui::MaterialDesignController::Intialize()
having already been called. That function cannot be called until the
command-line arguments are available, which happens when PreCreateThreads() is
called.

BUG=687349
==========

[kylix_rd, 2017-04-14 17:11:52]

kylixrd@chromium.org changed reviewers:
+ pkasting@chromium.org

[kylix_rd, 2017-04-14 17:12:19]

This should fix the problem Bret reported.

[Peter Kasting, 2017-04-14 18:14:32]

https://codereview.chromium.org/2818123002/diff/1/chrome/browser/ui/views/chr...
File chrome/browser/ui/views/chrome_browser_main_extra_parts_views.cc (right):

https://codereview.chromium.org/2818123002/diff/1/chrome/browser/ui/views/chr...
chrome/browser/ui/views/chrome_browser_main_extra_parts_views.cc:91: if
(!views::LayoutProvider::Get())
Is it safe to move the ViewsDelegate creation here too?  If not, I'm worried
that the same code that tries to access that object in the intervening window
could try to access this object, and crash.

Also, if somehow it is safe to keep these separated and code can't crash, this
at least deserves a comment about why it has to be here and not there.

[kylix_rd, 2017-04-14 18:35:26]

https://codereview.chromium.org/2818123002/diff/1/chrome/browser/ui/views/chr...
File chrome/browser/ui/views/chrome_browser_main_extra_parts_views.cc (right):

https://codereview.chromium.org/2818123002/diff/1/chrome/browser/ui/views/chr...
chrome/browser/ui/views/chrome_browser_main_extra_parts_views.cc:91: if
(!views::LayoutProvider::Get())
On 2017/04/14 18:14:32, Peter Kasting wrote:
> Is it safe to move the ViewsDelegate creation here too?  If not, I'm worried
> that the same code that tries to access that object in the intervening window
> could try to access this object, and crash.
> 
> Also, if somehow it is safe to keep these separated and code can't crash, this
> at least deserves a comment about why it has to be here and not there.

I cannot directly answer that question, but my reasoning is as follows:

Any code that relied on ui::MaterialDesignController::Initialize() having been
called would also behave improperly (albeit, it wouldn't likely crash).
Additionally, in the previous model where the LayoutDelegate was lazily created,
would not the same issue Bret outlined have happened?

Even the main browser window initialization must rely on
ui::MaterialDesignController::Initialize() already being called to test the
mode.

I added the creation of the LayoutProvider() into ToolkitInitialized() only
because it previously had been part of ViewsDelegate (along with the underlying
lazily create LayoutDelegate).

[Peter Kasting, 2017-04-14 18:44:50]

LGTM

https://codereview.chromium.org/2818123002/diff/1/chrome/browser/ui/views/chr...
File chrome/browser/ui/views/chrome_browser_main_extra_parts_views.cc (right):

https://codereview.chromium.org/2818123002/diff/1/chrome/browser/ui/views/chr...
chrome/browser/ui/views/chrome_browser_main_extra_parts_views.cc:91: if
(!views::LayoutProvider::Get())
On 2017/04/14 18:35:26, kylix_rd wrote:
> On 2017/04/14 18:14:32, Peter Kasting wrote:
> > Is it safe to move the ViewsDelegate creation here too?  If not, I'm worried
> > that the same code that tries to access that object in the intervening
window
> > could try to access this object, and crash.
> > 
> > Also, if somehow it is safe to keep these separated and code can't crash,
this
> > at least deserves a comment about why it has to be here and not there.
> 
> I cannot directly answer that question, but my reasoning is as follows:
> 
> Any code that relied on ui::MaterialDesignController::Initialize() having been
> called would also behave improperly (albeit, it wouldn't likely crash).
> Additionally, in the previous model where the LayoutDelegate was lazily
created,
> would not the same issue Bret outlined have happened?
> 
> Even the main browser window initialization must rely on
> ui::MaterialDesignController::Initialize() already being called to test the
> mode.

That sounds like an argument that we should, in fact, move the ViewsDelegate
creation later.

I think we should try it, but not in this patch.  I added a comment on bug
691894 about this.

https://codereview.chromium.org/2818123002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/chrome_browser_main_extra_parts_views.cc (right):

https://codereview.chromium.org/2818123002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_browser_main_extra_parts_views.cc:91: // The
layout_provider_ must be intialized here because it relies on
Nit: "here, and not in ToolkitInitialized(),"?

https://codereview.chromium.org/2818123002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_browser_main_extra_parts_views.cc:92: //
ui::MaterialDesignController::Intialize() having already been called.
Nit: Maybe add

TODO(pkasting): Try to move ViewsDelegate creation here as well; see
https://crbug.com/691894#c1

[kylix_rd, 2017-04-14 18:50:17]

The CQ bit was checked by kylixrd@chromium.org

[kylix_rd, 2017-04-14 18:50:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2818123002/#ps40001
(title: "Updated comments")

[commit-bot: I haz the power, 2017-04-14 18:50:44]

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, 2017-04-14 19:36:36]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1492195817272880,
"parent_rev": "03e181ac938ea7034d4f983327f1b71e85549f9e", "commit_rev":
"d61d7204eab9c8d63173a8fbadeaac9a2e816fae"}

[commit-bot: I haz the power, 2017-04-14 19:37:25]

Description was changed from

==========
Move LayoutProvider initialization to later in the startup process.

CreateLayoutProvider() relies on ui::MaterialDesignController::Intialize()
having already been called. That function cannot be called until the
command-line arguments are available, which happens when PreCreateThreads() is
called.

BUG=687349
==========

to

==========
Move LayoutProvider initialization to later in the startup process.

CreateLayoutProvider() relies on ui::MaterialDesignController::Intialize()
having already been called. That function cannot be called until the
command-line arguments are available, which happens when PreCreateThreads() is
called.

BUG=687349

Review-Url: https://codereview.chromium.org/2818123002
Cr-Commit-Position: refs/heads/master@{#464777}
Committed:
https://chromium.googlesource.com/chromium/src/+/d61d7204eab9c8d63173a8fbadea...
==========

[commit-bot: I haz the power, 2017-04-14 19:37:26]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/d61d7204eab9c8d63173a8fbadea...