cc: Refactor additional code from BuildRasterQueue.

cc: Refactor additional code from BuildRasterQueue.

This patch removes additional sets in BuildRasterQueue and moves it
to a separate client function that is called by the tile manager.
Having this as a separate function explicitly allows us to control the
setting instead of implicitly relying on the fact that every time we
rebuild a queue this setting might change.

R=danakj
BUG=451147
Committed: https://crrev.com/4973f5bd100f56946f26a2547ecd242f0ca06ce3
Cr-Commit-Position: refs/heads/master@{#314177}

[vmpstr, 2015-01-26 22:08:35]

Please take a look.

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

https://codereview.chromium.org/880693002/diff/1/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:367: client_->SetIsLikelyToRequireADraw(
I think this is the only spot that makes sense we add this call. Alternatively,
we can add it to every spot that rebuilds the raster queue. That, however, is
iffy since some of the calls are almost certainly will set this to false
(scheduling more work after the initial batch is done) and some don't really
make sense to me (building it with intent of marking tiles rasterize on demand).

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

https://codereview.chromium.org/880693002/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1218: bool is_likely_to_require_a_draw) {
Since we're updating this only in certain cases, does it make sense for this to
be SetIsLikelyToRequireADraw() { is_likely_to_require_a_draw_ = true; } and have
the caller have if(...) SetIsLikelyToRequireADraw? Wdyt?

[vmpstr, 2015-01-29 21:56:38]

I've rebased this, PTAL.

[danakj, 2015-01-29 22:33:20]

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

https://codereview.chromium.org/880693002/diff/1/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:367: client_->SetIsLikelyToRequireADraw(
On 2015/01/26 22:08:35, vmpstr wrote:
> I think this is the only spot that makes sense we add this call.
Alternatively,
> we can add it to every spot that rebuilds the raster queue. That, however, is
> iffy since some of the calls are almost certainly will set this to false
> (scheduling more work after the initial batch is done) and some don't really
> make sense to me (building it with intent of marking tiles rasterize on
demand).
> 

Once the initial batch of work is done, if the top of the queue is not required,
we should tell the LTHI so and stop queuing frames tho?

https://codereview.chromium.org/880693002/diff/40001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/880693002/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1609: // Optimistically schedule a draw, as a
tile required for draw is at the top
maybe update this comment? the top tile thing isn't part of this file anymore.

[vmpstr, 2015-01-29 22:48:09]

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

https://codereview.chromium.org/880693002/diff/1/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:367: client_->SetIsLikelyToRequireADraw(
On 2015/01/29 22:33:20, danakj wrote:
> On 2015/01/26 22:08:35, vmpstr wrote:
> > I think this is the only spot that makes sense we add this call.
> Alternatively,
> > we can add it to every spot that rebuilds the raster queue. That, however,
is
> > iffy since some of the calls are almost certainly will set this to false
> > (scheduling more work after the initial batch is done) and some don't really
> > make sense to me (building it with intent of marking tiles rasterize on
> demand).
> > 
> 
> Once the initial batch of work is done, if the top of the queue is not
required,
> we should tell the LTHI so and stop queuing frames tho?

Well.. I don't think so... I mean when we draw, we will reset the flag until its
set again. So suppose the situation is that we queue 1 tiles that is required to
draw, so we SetIsLikelyToRequireDraw but then it finishes really quickly and we
get to the reschedule and find that the top tile is not required. I think in
that case, we should leave the flag whatever it was, no? If we had it at
is_likely_to_draw_ = true, then that's still the case since we finished a now
tile and didn't have a draw yet, and if it's false then leave it at false.

https://codereview.chromium.org/880693002/diff/40001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/880693002/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1609: // Optimistically schedule a draw, as a
tile required for draw is at the top
On 2015/01/29 22:33:20, danakj wrote:
> maybe update this comment? the top tile thing isn't part of this file anymore.

Done.

[danakj, 2015-01-29 23:05:34]

lgtm

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

https://codereview.chromium.org/880693002/diff/1/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:367: client_->SetIsLikelyToRequireADraw(
On 2015/01/29 22:48:09, vmpstr wrote:
> On 2015/01/29 22:33:20, danakj wrote:
> > On 2015/01/26 22:08:35, vmpstr wrote:
> > > I think this is the only spot that makes sense we add this call.
> > Alternatively,
> > > we can add it to every spot that rebuilds the raster queue. That, however,
> is
> > > iffy since some of the calls are almost certainly will set this to false
> > > (scheduling more work after the initial batch is done) and some don't
really
> > > make sense to me (building it with intent of marking tiles rasterize on
> > demand).
> > > 
> > 
> > Once the initial batch of work is done, if the top of the queue is not
> required,
> > we should tell the LTHI so and stop queuing frames tho?
> 
> Well.. I don't think so... I mean when we draw, we will reset the flag until
its
> set again. So suppose the situation is that we queue 1 tiles that is required
to
> draw, so we SetIsLikelyToRequireDraw but then it finishes really quickly and
we
> get to the reschedule and find that the top tile is not required. I think in
> that case, we should leave the flag whatever it was, no? If we had it at
> is_likely_to_draw_ = true, then that's still the case since we finished a now
> tile and didn't have a draw yet, and if it's false then leave it at false.

well, i'm not sure it matters but take a look at LTHI::NotifyReadyToDraw(). When
a tile becomes ready, we SetNeedsRedraw then and stop doing it in
WillBeginFrame. If there was only one tile then we'd have already set it to
false. So I guess this is good. Less codepaths to follow with same result.

[vmpstr, 2015-02-02 17:42:54]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2015-02-02 17:43:30]

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

[commit-bot: I haz the power, 2015-02-02 19:27:59]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-02-02 19:29:00]

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