cc: Fix memory limit edge cases for SetIsLikelyToRequireADraw.

cc: Fix memory limit edge cases for SetIsLikelyToRequireADraw.

Whenever TileManager reassigns memory it should call
SetIsLikelyToRequireADraw on it's client. This happens when it checks if
there are more tiles that need to be rasterized after all raster tasks
are done. Furthermore when synchronously rasterizing tiles TileManager
should call SetIsLikelyToRequireADraw(false) on it's client. It's
necessary to update the client (LayerTreeHostImpl) otherwise it could
end up drawing again and again without any real work being done.

BUG=439275
Committed: https://crrev.com/1110f019ddad7bc371299384c6c0706708d3295c
Cr-Commit-Position: refs/heads/master@{#322637}

[sunnyps, 2015-03-27 00:15:41]

sunnyps@chromium.org changed reviewers:
+ danakj@chromium.org, vmpstr@chromium.org

[sunnyps, 2015-03-27 00:15:41]

PTAL. This is needed for my webview CL:
https://codereview.chromium.org/817603002/

[vmpstr, 2015-03-27 17:23:28]

https://codereview.chromium.org/1035043002/diff/1/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/1035043002/diff/1/cc/resources/tile_manager.c...
cc/resources/tile_manager.cc:432: client_->SetIsLikelyToRequireADraw(false);
Just FYI, this function is kind of slated for removal. crbug.com/457860

https://codereview.chromium.org/1035043002/diff/1/cc/resources/tile_manager.c...
cc/resources/tile_manager.cc:885: CheckIfMoreTilesNeedToBePrepared();
Feel free to inline this. (We don't typically implement ForTesting functionality
in .cc)

https://codereview.chromium.org/1035043002/diff/1/cc/resources/tile_manager.c...
cc/resources/tile_manager.cc:959: void
TileManager::CheckIfMoreTilesNeedToBePrepared() {
Kind of thinking out loud, but this function and PrepareTiles function share
quite a bit of code, and it would probably be nice to refactor at some point :)

https://codereview.chromium.org/1035043002/diff/1/cc/resources/tile_manager.h
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/1035043002/diff/1/cc/resources/tile_manager.h...
cc/resources/tile_manager.h:183: bool IsReadyToActivate() const;
While here, can you move these to be above the for-testing functions and group
all of the for testing functions near the end of public:. Also can you add a
comment along the lines of "the rest of the public interface here is used for
testing." Also, maybe we should change our tests to have a FakeTileManager that
promotes protected functions to public, just so this becomes cleaner (this is
unrelated to your patch).

https://codereview.chromium.org/1035043002/diff/1/cc/resources/tile_manager_u...
File cc/resources/tile_manager_unittest.cc (right):

https://codereview.chromium.org/1035043002/diff/1/cc/resources/tile_manager_u...
cc/resources/tile_manager_unittest.cc:1336: ->TileTaskRunnerForTesting()
This is kind of an overkill to get the tile task runner from tile manager just
to check the resource format... Is there another way to get this or is this the
best place?

[sunnyps, 2015-03-27 18:38:04]

Addressed comments. PTAL.

https://codereview.chromium.org/1035043002/diff/1/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/1035043002/diff/1/cc/resources/tile_manager.c...
cc/resources/tile_manager.cc:432: client_->SetIsLikelyToRequireADraw(false);
On 2015/03/27 17:23:28, vmpstr wrote:
> Just FYI, this function is kind of slated for removal. crbug.com/457860

Yeah, would be nice to keep the test while it's alive.

https://codereview.chromium.org/1035043002/diff/1/cc/resources/tile_manager.c...
cc/resources/tile_manager.cc:885: CheckIfMoreTilesNeedToBePrepared();
On 2015/03/27 17:23:28, vmpstr wrote:
> Feel free to inline this. (We don't typically implement ForTesting
functionality
> in .cc)

Done.

https://codereview.chromium.org/1035043002/diff/1/cc/resources/tile_manager.c...
cc/resources/tile_manager.cc:959: void
TileManager::CheckIfMoreTilesNeedToBePrepared() {
On 2015/03/27 17:23:28, vmpstr wrote:
> Kind of thinking out loud, but this function and PrepareTiles function share
> quite a bit of code, and it would probably be nice to refactor at some point
:)

Acknowledged.

https://codereview.chromium.org/1035043002/diff/1/cc/resources/tile_manager.h
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/1035043002/diff/1/cc/resources/tile_manager.h...
cc/resources/tile_manager.h:183: bool IsReadyToActivate() const;
On 2015/03/27 17:23:28, vmpstr wrote:
> While here, can you move these to be above the for-testing functions and group
> all of the for testing functions near the end of public:. Also can you add a
> comment along the lines of "the rest of the public interface here is used for
> testing." Also, maybe we should change our tests to have a FakeTileManager
that
> promotes protected functions to public, just so this becomes cleaner (this is
> unrelated to your patch).

Done.

https://codereview.chromium.org/1035043002/diff/1/cc/resources/tile_manager_u...
File cc/resources/tile_manager_unittest.cc (right):

https://codereview.chromium.org/1035043002/diff/1/cc/resources/tile_manager_u...
cc/resources/tile_manager_unittest.cc:1336: ->TileTaskRunnerForTesting()
On 2015/03/27 17:23:28, vmpstr wrote:
> This is kind of an overkill to get the tile task runner from tile manager just
> to check the resource format... Is there another way to get this or is this
the
> best place?

Done. Using ResourceProvider::best_texture_format for this. The intent here is
to communicate what the numbers mean if the test ever breaks.

[vmpstr, 2015-03-27 19:02:27]

lgtm

https://codereview.chromium.org/1035043002/diff/20001/cc/resources/tile_manag...
File cc/resources/tile_manager_unittest.cc (right):

https://codereview.chromium.org/1035043002/diff/20001/cc/resources/tile_manag...
cc/resources/tile_manager_unittest.cc:1334:
EXPECT_EQ(queue->Top()->desired_texture_size(), gfx::Size(256, 256));
minor nit: for EXPECT* macros, the expected value typically goes first, and the
thing you're testing goes second.

The reason this matters is if the test fails, you get output that looks like:

Expected value:
gfx::Size(10, 10)
Actual value:
gfx::Size(256, 256)

But in your case, 256 is the expected, and actual is 10. 
However, throughout our tests, both ways are kind of used, so this just for
future reference more than anything.

[danakj, 2015-03-27 19:04:55]

https://codereview.chromium.org/1035043002/diff/20001/cc/resources/tile_manag...
File cc/resources/tile_manager_unittest.cc (right):

https://codereview.chromium.org/1035043002/diff/20001/cc/resources/tile_manag...
cc/resources/tile_manager_unittest.cc:1334:
EXPECT_EQ(queue->Top()->desired_texture_size(), gfx::Size(256, 256));
On 2015/03/27 19:02:27, vmpstr wrote:
> minor nit: for EXPECT* macros, the expected value typically goes first, and
the
> thing you're testing goes second.

nit on the nit: Just for EXPECT_EQ and EXPECT_NE.

[sunnyps, 2015-03-27 19:09:39]

On 2015/03/27 19:02:27, vmpstr wrote:
> lgtm
> 
>
https://codereview.chromium.org/1035043002/diff/20001/cc/resources/tile_manag...
> File cc/resources/tile_manager_unittest.cc (right):
> 
>
https://codereview.chromium.org/1035043002/diff/20001/cc/resources/tile_manag...
> cc/resources/tile_manager_unittest.cc:1334:
> EXPECT_EQ(queue->Top()->desired_texture_size(), gfx::Size(256, 256));
> minor nit: for EXPECT* macros, the expected value typically goes first, and
the
> thing you're testing goes second.
> 
> The reason this matters is if the test fails, you get output that looks like:
> 
> Expected value:
> gfx::Size(10, 10)
> Actual value:
> gfx::Size(256, 256)
> 
> But in your case, 256 is the expected, and actual is 10. 
> However, throughout our tests, both ways are kind of used, so this just for
> future reference more than anything.

Done. Thanks for the tip.

[sunnyps, 2015-03-27 19:26:43]

The CQ bit was checked by sunnyps@chromium.org

[sunnyps, 2015-03-27 19:26:43]

The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org
Link to the patchset: https://codereview.chromium.org/1035043002/#ps60001
(title: "Address comments.")

[commit-bot: I haz the power, 2015-03-27 19:26:58]

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

[commit-bot: I haz the power, 2015-03-27 20:37:21]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-03-27 20:37:57]

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