cc: split UpdateGpuRasterizationStatus() into two parts.

cc: split UpdateGpuRasterizationStatus() into two parts.

Make one part handle updating status (now private, and called
automatically when content or viewport params change), and one part
called on commit or renderer change.

BUG=464892
Committed: https://crrev.com/9c04acba8eacb5be020098cec7b4ec7c797844d7
Cr-Commit-Position: refs/heads/master@{#330170}

[Stephen White, 2015-05-13 19:44:17]

https://codereview.chromium.org/1134123005/diff/1/cc/layers/picture_layer_imp...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/1134123005/diff/1/cc/layers/picture_layer_imp...
cc/layers/picture_layer_impl_unittest.cc:2516:
host_impl_.set_has_gpu_rasterization_trigger(false);
Should we be calling UpdateTreeResourcesIfNeeded() here?

Test seems to pass without it; perhaps SetupDefaultTrees() (below) is enough?

https://codereview.chromium.org/1134123005/diff/1/cc/layers/picture_layer_imp...
cc/layers/picture_layer_impl_unittest.cc:2530:
host_impl_.set_content_is_suitable_for_gpu_rasterization(true);
Same here?

https://codereview.chromium.org/1134123005/diff/1/cc/layers/picture_layer_imp...
cc/layers/picture_layer_impl_unittest.cc:2540:
host_impl_.set_content_is_suitable_for_gpu_rasterization(true);
Same here?

https://codereview.chromium.org/1134123005/diff/1/cc/trees/layer_tree_host_im...
File cc/trees/layer_tree_host_impl.h (right):

https://codereview.chromium.org/1134123005/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.h:324: void set_has_gpu_rasterization_trigger(bool
flag) {
This should probably be CamelCased, now that it's no longer just a setter.

https://codereview.chromium.org/1134123005/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.h:328: void
set_content_is_suitable_for_gpu_rasterization(bool flag) {
Same here.

[danakj, 2015-05-13 19:50:58]

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

[danakj, 2015-05-13 19:50:58]

https://codereview.chromium.org/1134123005/diff/1/cc/trees/layer_tree_host_im...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/1134123005/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.cc:312: UpdateTreeResourcesIfNeeded();
I would UpdateGpuRasterizationStatus() here then UpdateTreeResourcesIfNeeded
right after, rather than doing it in the setters.

https://codereview.chromium.org/1134123005/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.cc:1629: if (!tree_resources_dirty_) {
We need to do the following stuff unconditionally when we change renderer. And
conditionally when we CommitComplete (if gpu raster changed).

https://codereview.chromium.org/1134123005/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.cc:2005: UpdateGpuRasterizationStatus();
Move this out to InitializeRenderer?

https://codereview.chromium.org/1134123005/diff/1/cc/trees/layer_tree_host_im...
File cc/trees/layer_tree_host_impl.h (right):

https://codereview.chromium.org/1134123005/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.h:324: void set_has_gpu_rasterization_trigger(bool
flag) {
this isn't a simple setter anymore, no hacker case

https://codereview.chromium.org/1134123005/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.h:328: void
set_content_is_suitable_for_gpu_rasterization(bool flag) {
dittos

https://codereview.chromium.org/1134123005/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.h:332: void UpdateTreeResourcesIfNeeded();
UpdateTreeResourcesForGpuRasterizationIfNeeded?

https://codereview.chromium.org/1134123005/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.h:653: bool tree_resources_dirty_;
_for_gpu_rasterization_

[danakj, 2015-05-13 19:53:45]

https://codereview.chromium.org/1134123005/diff/1/cc/trees/layer_tree_host_im...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/1134123005/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.cc:1629: if (!tree_resources_dirty_) {
On 2015/05/13 19:50:58, danakj wrote:
> We need to do the following stuff unconditionally when we change renderer. And
> conditionally when we CommitComplete (if gpu raster changed).

Oh no I'm mistaken, the unconditional of the following happens in
InitializeRenderer, this just does it a second time (which we could actually
avoid some of this if we made the decision earlier?)

[Stephen White, 2015-05-13 20:15:20]

Thanks for your review! New patch up.

https://codereview.chromium.org/1134123005/diff/1/cc/trees/layer_tree_host_im...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/1134123005/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.cc:312: UpdateTreeResourcesIfNeeded();
On 2015/05/13 19:50:58, danakj wrote:
> I would UpdateGpuRasterizationStatus() here then UpdateTreeResourcesIfNeeded
> right after, rather than doing it in the setters.

Having to remember to call UpdateGpuRasterizationStatus() seems 
error-prone. It seems safer to keep it automatically up-to-date, so 
that (e.g.) Victor can rely on gpu_rasterization_enabled_ being valid.

https://codereview.chromium.org/1134123005/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.cc:1629: if (!tree_resources_dirty_) {
On 2015/05/13 19:53:45, danakj wrote:
> On 2015/05/13 19:50:58, danakj wrote:
> > We need to do the following stuff unconditionally when we change renderer.
And
> > conditionally when we CommitComplete (if gpu raster changed).
> 
> Oh no I'm mistaken, the unconditional of the following happens in
> InitializeRenderer, this just does it a second time (which we could actually
> avoid some of this if we made the decision earlier?)

Yeah, it was conditional before in both cases. I've removed it from
CreateAndSetRenderer(), since it seems superfluous (see below).

https://codereview.chromium.org/1134123005/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.cc:2005: UpdateGpuRasterizationStatus();
On 2015/05/13 19:50:58, danakj wrote:
> Move this out to InitializeRenderer?

Done.

https://codereview.chromium.org/1134123005/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.cc:2006: UpdateTreeResourcesIfNeeded();
I see now that InitializeRenderer() is doing ReleaseTreeResoures() /
RecreateTreeResources() already, so this call seems superfluous. I've removed it
and all tests pass; let me know if I've missed something.

https://codereview.chromium.org/1134123005/diff/1/cc/trees/layer_tree_host_im...
File cc/trees/layer_tree_host_impl.h (right):

https://codereview.chromium.org/1134123005/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.h:324: void set_has_gpu_rasterization_trigger(bool
flag) {
On 2015/05/13 19:50:58, danakj wrote:
> this isn't a simple setter anymore, no hacker case

Done.

https://codereview.chromium.org/1134123005/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.h:328: void
set_content_is_suitable_for_gpu_rasterization(bool flag) {
On 2015/05/13 19:50:58, danakj wrote:
> dittos

Done.

https://codereview.chromium.org/1134123005/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.h:332: void UpdateTreeResourcesIfNeeded();
On 2015/05/13 19:50:58, danakj wrote:
> UpdateTreeResourcesForGpuRasterizationIfNeeded?

Done.

https://codereview.chromium.org/1134123005/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.h:653: bool tree_resources_dirty_;
On 2015/05/13 19:50:58, danakj wrote:
> _for_gpu_rasterization_

Done.

[danakj, 2015-05-13 20:21:58]

LGTM

https://codereview.chromium.org/1134123005/diff/20001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/1134123005/diff/20001/cc/layers/picture_layer...
cc/layers/picture_layer_impl_unittest.cc:2842:
host_impl_.UpdateTreeResourcesForGpuRasterizationIfNeeded();
is this needed?

https://codereview.chromium.org/1134123005/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/1134123005/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:312:
UpdateTreeResourcesForGpuRasterizationIfNeeded();
can you comment that LTH sets stuff and this is acting on it?

https://codereview.chromium.org/1134123005/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:1629: if
(!tree_resources_for_gpu_rasterization_dirty_) {
nit: no {}

[Stephen White, 2015-05-13 20:38:35]

https://codereview.chromium.org/1134123005/diff/20001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/1134123005/diff/20001/cc/layers/picture_layer...
cc/layers/picture_layer_impl_unittest.cc:2842:
host_impl_.UpdateTreeResourcesForGpuRasterizationIfNeeded();
On 2015/05/13 20:21:57, danakj wrote:
> is this needed?

Yes. The EXPECT_BOTH_EQ(HighResTiling()->contents_scale(), ...) tests
below all fail without it, I think because UTRFGRIF() updates the
tiling info / highres stuff?

https://codereview.chromium.org/1134123005/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/1134123005/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:312:
UpdateTreeResourcesForGpuRasterizationIfNeeded();
On 2015/05/13 20:21:57, danakj wrote:
> can you comment that LTH sets stuff and this is acting on it?

Done.

https://codereview.chromium.org/1134123005/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:1629: if
(!tree_resources_for_gpu_rasterization_dirty_) {
On 2015/05/13 20:21:57, danakj wrote:
> nit: no {}

Done.

[Stephen White, 2015-05-13 20:39:05]

The CQ bit was checked by senorblanco@chromium.org

[Stephen White, 2015-05-13 20:39:05]

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/1134123005/#ps40001
(title: "Fixes per review comments")

[Stephen White, 2015-05-13 20:40:31]

The CQ bit was unchecked by senorblanco@chromium.org

[commit-bot: I haz the power, 2015-05-13 20:40:32]

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

[Stephen White, 2015-05-15 18:53:30]

The CQ bit was checked by senorblanco@chromium.org

[Stephen White, 2015-05-15 18:53:30]

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/1134123005/#ps60001
(title: "Rebase to ToT")

[commit-bot: I haz the power, 2015-05-15 18:54:13]

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

[commit-bot: I haz the power, 2015-05-15 19:41:40]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-05-15 19:42:30]

Patchset 4 (id:??) landed as
https://crrev.com/9c04acba8eacb5be020098cec7b4ec7c797844d7
Cr-Commit-Position: refs/heads/master@{#330170}