cc: Add LayerTreeHost::InitParams for LayerTreeHost creation.

cc: Add LayerTreeHost::InitParams for LayerTreeHost creation.

Instead of passing in a number of things to the LayerTreeHost constructor,
use a struct to simplify the code a bit, and make this more readable. This
also makes it easy to maintain a development branch that adds new things.

TBR=jam@ for API usage update in android_webview/, components/, and content/

BUG=436952
Committed: https://crrev.com/6780f3daea93a14cb81698025685c860b8f41db5
Cr-Commit-Position: refs/heads/master@{#329164}

[sadrul, 2015-05-06 20:50:14]

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

[sadrul, 2015-05-06 20:50:15]

Hi! This patch is incomplete; I still need to update the tests to use the
InitParams, but I wanted to get some feedback on whether this is something that
would make sense. (the driving desire is to make it easy to maintain changes
such as https://codereview.chromium.org/1014993002/ in a separate branch, and
also readability).

[danakj, 2015-05-06 21:00:48]

https://codereview.chromium.org/1126253005/diff/1/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/1126253005/diff/1/cc/trees/layer_tree_host.cc...
cc/trees/layer_tree_host.cc:59: LayerTreeHost::InitParams::~InitParams() {
whitespace

https://codereview.chromium.org/1126253005/diff/1/cc/trees/layer_tree_host.h
File cc/trees/layer_tree_host.h (right):

https://codereview.chromium.org/1126253005/diff/1/cc/trees/layer_tree_host.h#...
cc/trees/layer_tree_host.h:81: mutable scoped_ptr<BeginFrameSource>
external_begin_frame_source;
:( mutable.

this should be a movable type so we can pass/move it by value to LTH instead of
by const&, and then we don't need mutable here.

but in the absence of allowing std::move() how passing it as a (nonconst)
InitParams*, with a TODO to std::move() by value?

[sadrul, 2015-05-06 23:35:43]

https://codereview.chromium.org/1126253005/diff/1/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/1126253005/diff/1/cc/trees/layer_tree_host.cc...
cc/trees/layer_tree_host.cc:59: LayerTreeHost::InitParams::~InitParams() {
On 2015/05/06 21:00:48, danakj wrote:
> whitespace

Done.

https://codereview.chromium.org/1126253005/diff/1/cc/trees/layer_tree_host.h
File cc/trees/layer_tree_host.h (right):

https://codereview.chromium.org/1126253005/diff/1/cc/trees/layer_tree_host.h#...
cc/trees/layer_tree_host.h:81: mutable scoped_ptr<BeginFrameSource>
external_begin_frame_source;
On 2015/05/06 21:00:48, danakj wrote:
> :( mutable.
> 
> this should be a movable type so we can pass/move it by value to LTH instead
of
> by const&, and then we don't need mutable here.
> 
> but in the absence of allowing std::move() how passing it as a (nonconst)
> InitParams*, with a TODO to std::move() by value?

Yep, makes sense. Done.

[sadrul, 2015-05-06 23:39:01]

Sidenote: I took this opportunity to replace some uses of
base::MessageLoopProxy::current() in the affected code with
base::ThreadTaskRunnerHandle::Get() in the spirit of fix-as-you-go. I can remove
that from here if you would prefer.

[danakj, 2015-05-07 00:59:56]

https://codereview.chromium.org/1126253005/diff/10005/cc/test/fake_layer_tree...
File cc/test/fake_layer_tree_host.cc (right):

https://codereview.chromium.org/1126253005/diff/10005/cc/test/fake_layer_tree...
cc/test/fake_layer_tree_host.cc:12: host_impl_(params->settings ?
*params->settings : LayerTreeSettings(),
not a fan, this is a bit unobvious. i'd rather the settings always given to the
InitParams.

it /could/ even be a const& if you passed it to the constructor. if you want, no
strong feelings here.

https://codereview.chromium.org/1126253005/diff/10005/cc/test/layer_tree_test.cc
File cc/test/layer_tree_test.cc (right):

https://codereview.chromium.org/1126253005/diff/10005/cc/test/layer_tree_test...
cc/test/layer_tree_test.cc:457: LayerTreeHost::InitParams* params) {
I think maybe I prefer passing in all the things here, constructing the Params
inside this method, and then it doesn't happen that we pass
externalbeginframesource to something twice.. etc. That'd help make things a bit
more obvious.

alternately pull the EBFS out of the params into a local before passing on the
params.

https://codereview.chromium.org/1126253005/diff/10005/cc/test/layer_tree_test...
cc/test/layer_tree_test.cc:652: 
nit: no whitespace

https://codereview.chromium.org/1126253005/diff/10005/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/1126253005/diff/10005/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:95: settings_(params->settings ? *params->settings :
LayerTreeSettings()),
similar, not a fan of this

https://codereview.chromium.org/1126253005/diff/10005/components/html_viewer/...
File components/html_viewer/web_layer_tree_view_impl.cc (right):

https://codereview.chromium.org/1126253005/diff/10005/components/html_viewer/...
components/html_viewer/web_layer_tree_view_impl.cc:43:
params.shared_bitmap_manager = shared_bitmap_manager;
you're not using the settings here, can you double check the other places?

https://codereview.chromium.org/1126253005/diff/10005/content/renderer/gpu/re...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/1126253005/diff/10005/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.cc:486: params.task_graph_runner =
task_graph_runner;
no settings here either

[sadrul, 2015-05-07 04:39:26]

https://codereview.chromium.org/1126253005/diff/10005/cc/test/fake_layer_tree...
File cc/test/fake_layer_tree_host.cc (right):

https://codereview.chromium.org/1126253005/diff/10005/cc/test/fake_layer_tree...
cc/test/fake_layer_tree_host.cc:12: host_impl_(params->settings ?
*params->settings : LayerTreeSettings(),
On 2015/05/07 00:59:56, danakj wrote:
> not a fan, this is a bit unobvious. i'd rather the settings always given to
the
> InitParams.
> 
> it /could/ even be a const& if you passed it to the constructor. if you want,
no
> strong feelings here.

Removed the NULL check (and kept settings as a ptr).

https://codereview.chromium.org/1126253005/diff/10005/cc/test/layer_tree_test.cc
File cc/test/layer_tree_test.cc (right):

https://codereview.chromium.org/1126253005/diff/10005/cc/test/layer_tree_test...
cc/test/layer_tree_test.cc:457: LayerTreeHost::InitParams* params) {
On 2015/05/07 00:59:56, danakj wrote:
> I think maybe I prefer passing in all the things here, constructing the Params
> inside this method, and then it doesn't happen that we pass
> externalbeginframesource to something twice.. etc. That'd help make things a
bit
> more obvious.

Done.

> 
> alternately pull the EBFS out of the params into a local before passing on the
> params.

https://codereview.chromium.org/1126253005/diff/10005/cc/test/layer_tree_test...
cc/test/layer_tree_test.cc:652: 
On 2015/05/07 00:59:56, danakj wrote:
> nit: no whitespace

Done.

https://codereview.chromium.org/1126253005/diff/10005/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/1126253005/diff/10005/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:95: settings_(params->settings ? *params->settings :
LayerTreeSettings()),
On 2015/05/07 00:59:56, danakj wrote:
> similar, not a fan of this

Removed this.

https://codereview.chromium.org/1126253005/diff/10005/components/html_viewer/...
File components/html_viewer/web_layer_tree_view_impl.cc (right):

https://codereview.chromium.org/1126253005/diff/10005/components/html_viewer/...
components/html_viewer/web_layer_tree_view_impl.cc:43:
params.shared_bitmap_manager = shared_bitmap_manager;
On 2015/05/07 00:59:56, danakj wrote:
> you're not using the settings here, can you double check the other places?

Fixed. (the default set of trybots didn't catch this :( There are some bots on
tryserver.client.mojo master though, I have triggered them to make sure this is
caught)

https://codereview.chromium.org/1126253005/diff/10005/content/renderer/gpu/re...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/1126253005/diff/10005/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.cc:486: params.task_graph_runner =
task_graph_runner;
On 2015/05/07 00:59:56, danakj wrote:
> no settings here either

Fixed (thankfully, lots of try failures from this omission).

https://codereview.chromium.org/1126253005/diff/70001/cc/test/layer_tree_test.cc
File cc/test/layer_tree_test.cc (right):

https://codereview.chromium.org/1126253005/diff/70001/cc/test/layer_tree_test...
cc/test/layer_tree_test.cc:513: task_graph_runner_(params->task_graph_runner),
Can you double check this a bit? The old code sends nullptr for
shared_bitmap_manager, gpu_memory_buffer_manager, and task_graph_runner to the
LayerTreeHost ctor, but sets the duplicate-member variables here. With this
change, both the LTH ctor and the local memebers are set. The tests don't seem
to mind, so it's a bit unclear if the old behaviour was deliberate or not.

[danakj, 2015-05-07 18:28:32]

LGTM

https://codereview.chromium.org/1126253005/diff/70001/cc/test/layer_tree_test.cc
File cc/test/layer_tree_test.cc (right):

https://codereview.chromium.org/1126253005/diff/70001/cc/test/layer_tree_test...
cc/test/layer_tree_test.cc:469: params.main_task_runner = main_task_runner;
this is similar, we used to not pass it to the constructor, and only pass it to
the Initialize, maybe we should keep that

https://codereview.chromium.org/1126253005/diff/70001/cc/test/layer_tree_test...
cc/test/layer_tree_test.cc:513: task_graph_runner_(params->task_graph_runner),
On 2015/05/07 04:39:26, sadrul wrote:
> Can you double check this a bit? The old code sends nullptr for
> shared_bitmap_manager, gpu_memory_buffer_manager, and task_graph_runner to the
> LayerTreeHost ctor, but sets the duplicate-member variables here. With this
> change, both the LTH ctor and the local memebers are set. The tests don't seem
> to mind, so it's a bit unclear if the old behaviour was deliberate or not.

Right, passing them to the LTH is pointless, cuz they are only stored there to
pass to the LTHI when it's created.
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre...

But we override that method and pass them from the test harness on L491.

We could stop storing them in the harness and have the above method pull them
off the LTH and pass them to the LTHI like the real method does, then they
aren't stored in 2 places.

Or we could explain all of this in some comments here, which would be nice.

[sadrul, 2015-05-07 22:47:43]

https://codereview.chromium.org/1126253005/diff/70001/cc/test/layer_tree_test.cc
File cc/test/layer_tree_test.cc (right):

https://codereview.chromium.org/1126253005/diff/70001/cc/test/layer_tree_test...
cc/test/layer_tree_test.cc:469: params.main_task_runner = main_task_runner;
On 2015/05/07 18:28:32, danakj wrote:
> this is similar, we used to not pass it to the constructor, and only pass it
to
> the Initialize, maybe we should keep that

Done (not setting main_task_runner anymore).

https://codereview.chromium.org/1126253005/diff/70001/cc/test/layer_tree_test...
cc/test/layer_tree_test.cc:513: task_graph_runner_(params->task_graph_runner),
On 2015/05/07 18:28:32, danakj wrote:
> On 2015/05/07 04:39:26, sadrul wrote:
> > Can you double check this a bit? The old code sends nullptr for
> > shared_bitmap_manager, gpu_memory_buffer_manager, and task_graph_runner to
the
> > LayerTreeHost ctor, but sets the duplicate-member variables here. With this
> > change, both the LTH ctor and the local memebers are set. The tests don't
seem
> > to mind, so it's a bit unclear if the old behaviour was deliberate or not.
> 
> Right, passing them to the LTH is pointless, cuz they are only stored there to
> pass to the LTHI when it's created.
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre...
> 
> But we override that method and pass them from the test harness on L491.
> 
> We could stop storing them in the harness and have the above method pull them
> off the LTH and pass them to the LTHI like the real method does, then they
> aren't stored in 2 places.

Thanks for explaining! I have made this change.

> 
> Or we could explain all of this in some comments here, which would be nice.

[sadrul, 2015-05-07 22:49:54]

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

[sadrul, 2015-05-07 22:49:55]

+jam@ for changes in android_webview, components, and contents.

[danakj, 2015-05-07 23:22:04]

LGTM

https://codereview.chromium.org/1126253005/diff/90001/cc/trees/layer_tree_host.h
File cc/trees/layer_tree_host.h (right):

https://codereview.chromium.org/1126253005/diff/90001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.h:333: SharedBitmapManager* shared_bitmap_manager()
const {
can you comment that these are only valid until the LTHI is created?

[sadrul, 2015-05-08 00:53:31]

https://codereview.chromium.org/1126253005/diff/90001/cc/trees/layer_tree_host.h
File cc/trees/layer_tree_host.h (right):

https://codereview.chromium.org/1126253005/diff/90001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.h:333: SharedBitmapManager* shared_bitmap_manager()
const {
On 2015/05/07 23:22:04, danakj wrote:
> can you comment that these are only valid until the LTHI is created?

Done.

[jam, 2015-05-11 15:00:43]

On 2015/05/07 22:49:55, sadrul wrote:
> +jam@ for changes in android_webview, components, and contents.

per
https://www.chromium.org/developers/owners-files#TOC-When-to-use-To-Be-Review...,
you can TBR owners of files which use an API as long as you get owners review
for where the API is defined.

[sadrul, 2015-05-11 15:06:11]

The CQ bit was checked by sadrul@chromium.org

[sadrul, 2015-05-11 15:06:11]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/1126253005/#ps110001
(title: ".")

[commit-bot: I haz the power, 2015-05-11 15:06:33]

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

[commit-bot: I haz the power, 2015-05-11 17:02:01]

Committed patchset #7 (id:110001)

[commit-bot: I haz the power, 2015-05-11 17:02:57]

Patchset 7 (id:??) landed as
https://crrev.com/6780f3daea93a14cb81698025685c860b8f41db5
Cr-Commit-Position: refs/heads/master@{#329164}