LayerTreeHost should not modify the LayerTreeSettings object. Instead it should use a temporary va…

LayerTreeHost should not modify the LayerTreeSettings object.

Instead it should use a temporary variable wherever needed and
leave the settings object const.  This patch removes the last
instance of this behavior and makes the settings object const.

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

[danakj, 2013-09-17 21:11:19]

LGTM w/ one nit. Thanks!

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

https://codereview.chromium.org/23463042/diff/1/cc/trees/layer_tree_host.cc#n...
cc/trees/layer_tree_host.cc:1080: !settings_.impl_side_painting) {
indenting is off, run "git cl format" before you upload?

[atreat, 2013-09-17 22:04:48]

Fixed the formatting.  I've also installed clang-format so I can do 'git cl
format' from now on :)

[commit-bot: I haz the power, 2013-09-17 22:10:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/adam.treat@samsung.com/23463042/6001

[commit-bot: I haz the power, 2013-09-17 22:31:50]

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

[danakj, 2013-09-17 22:41:40]

Looks like you have some work to do:

adam.treat@samsung.com is not in AUTHORS file. If you are a new contributor,
please visit
http://www.chromium.org/developers/contributing-code and read the "Legal"
section
If you are a chromite, verify the contributor signed the CLA.

[danakj, 2013-09-17 22:44:23]

https://chromiumcodereview.appspot.com/23463042/diff/6001/cc/trees/layer_tree...
File cc/trees/layer_tree_host.cc (right):

https://chromiumcodereview.appspot.com/23463042/diff/6001/cc/trees/layer_tree...
cc/trees/layer_tree_host.cc:1078: size_t max_partial_texture_updates =
settings_.max_partial_texture_updates;
Oh, this should be 0 if the if statement below is false. Set it to 0 here, and
use the settings_ value inside the std::min statement on 1081. (Sorry for
missing that change in logic)

[atreat, 2013-09-18 14:20:06]

Sorry about the change in logic.  I've fixed as you suggested.  Laszlo Gombos
has requested to add me as part of Samsung's contributors agreement and that
should be done soon hopefully.  I figured I should wait to update the AUTHORS
file for that process to finish.

[danakj, 2013-09-18 14:34:16]

Ok thanks, patchset 3 LGTM

[atreat, 2013-09-30 18:04:38]

On 2013/09/18 14:34:16, danakj wrote:
> Ok thanks, patchset 3 LGTM

Hello.  I think the contributors agreement is all figured out.  My name has been
added to Samsung's roster.  I've also changed the AUTHORS file and it is in the
queue to commit.  Can you CQ this now?

[danakj, 2013-09-30 19:32:04]

On 2013/09/30 18:04:38, atreat wrote:
> On 2013/09/18 14:34:16, danakj wrote:
> > Ok thanks, patchset 3 LGTM
> 
> Hello.  I think the contributors agreement is all figured out.  My name has
been
> added to Samsung's roster.  I've also changed the AUTHORS file and it is in
the
> queue to commit.  Can you CQ this now?

I'll try, but that probably needs to land first. Can you cc me on it?

[commit-bot: I haz the power, 2013-09-30 19:32:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/adam.treat@samsung.com/23463042/6002

[commit-bot: I haz the power, 2013-09-30 19:34:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/adam.treat@samsung.com/23463042/6002

[atreat, 2013-09-30 19:57:25]

On 2013/09/30 19:32:04, danakj wrote:
> On 2013/09/30 18:04:38, atreat wrote:
> > On 2013/09/18 14:34:16, danakj wrote:
> > > Ok thanks, patchset 3 LGTM
> > 
> > Hello.  I think the contributors agreement is all figured out.  My name has
> been
> > added to Samsung's roster.  I've also changed the AUTHORS file and it is in
> the
> > queue to commit.  Can you CQ this now?
> 
> I'll try, but that probably needs to land first. Can you cc me on it?

Done. https://codereview.chromium.org/25314002/

[commit-bot: I haz the power, 2013-09-30 22:38:42]

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

[atreat, 2013-10-01 23:17:47]

On 2013/09/30 22:38:42, I haz the power (commit-bot) wrote:
> Retried try job too often on linux_rel for step(s) cc_unittests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

Ping.  Try again?

[commit-bot: I haz the power, 2013-10-01 23:20:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/adam.treat@samsung.com/23463042/6002

[commit-bot: I haz the power, 2013-10-02 00:18:39]

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

[commit-bot: I haz the power, 2013-10-02 00:58:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/adam.treat@samsung.com/23463042/6002

[commit-bot: I haz the power, 2013-10-02 01:12:32]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[danakj, 2013-10-02 05:34:06]

These unit test failures need to be addressed.
On Oct 1, 2013 8:18 PM, <commit-bot@chromium.org> wrote:

> Retried try job too often on linux_rel for step(s) cc_unittests
> http://build.chromium.org/p/**tryserver.chromium/**
>
buildstatus?builder=linux_rel&**number=174278<http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&number=174278>
>
>
https://codereview.chromium.**org/23463042/<https://codereview.chromium.org/2...
>

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

[atreat, 2013-10-02 16:52:05]

On 2013/10/02 05:34:06, danakj wrote:
> These unit test failures need to be addressed.

Working on it now.  Thanks.

[atreat, 2013-10-02 19:18:47]

I changed the patch around to take into account
LayerTreeHost::buffered_updates() so that it continues to return the same value
and no change in behavior.

Also, I added a new member variable to the class instead of just putting the
logic for calculating this number inside of
LayerTreeHost::MaxPartialTextureUpdates() because some thread (not the main one)
calls buffered_updates() and apparently the logic contains calls that can only
be made on the main thread.

[danakj, 2013-10-03 17:42:25]

LGTM w/ 2 things.

Btw buffered_updates() should be called from TiledLayer::Update() which is done
on the main thread.

https://codereview.chromium.org/23463042/diff/72001/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/23463042/diff/72001/cc/trees/layer_tree_host....
cc/trees/layer_tree_host.cc:133:
max_partial_texture_updates_(settings_.max_partial_texture_updates),
Can we initialize this to 0?

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

https://codereview.chromium.org/23463042/diff/72001/cc/trees/layer_tree_host....
cc/trees/layer_tree_host.h:275: size_t MaxPartialTextureUpdates() const {
rename to max_partial_texture_updates()

[danakj, 2013-10-03 18:49:23]

With https://codereview.chromium.org/25912003/ I think this can go back to
mostly the previous version, without the extra member variable.

[atreat, 2013-10-04 17:14:47]

This revision takes into account your recent changes and restores the original
idea, but now passes all base_unittests and cc_unittests.

[danakj, 2013-10-04 17:19:15]

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

https://codereview.chromium.org/23463042/diff/80001/cc/trees/layer_tree_host....
cc/trees/layer_tree_host.h:276: size_t max_partial_texture_updates() const;
Cool! Sorry to be confusing, but since this function is not just returning a
variable anymore, it should go back to its other form as
MaxPartialTextureUpdates(). Other than that, LGTM!

[commit-bot: I haz the power, 2013-10-04 17:36:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/adam.treat@samsung.com/23463042/88001

[commit-bot: I haz the power, 2013-10-05 00:27:03]

Change committed as 227149