cc: ReadyToDraw notifications.

cc: ReadyToDraw notifications.

Send notifications when all visible tiles on the active tree are ready
to draw.

R=enne@chromium.org,reveman@chromium.org,brianderson@chromium.org,vmpstr@chromium.org
BUG=426501
Committed: https://crrev.com/dfac03e1bd435b80b3c6a588ca8dd98954073185
Cr-Commit-Position: refs/heads/master@{#303706}

[ernstm, 2014-10-23 21:28:22]

PTAL

[brianderson, 2014-10-24 01:34:04]

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

https://codereview.chromium.org/672283003/diff/1/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:391: if (task_set == REQUIRED_FOR_DRAW) {
Can a task_set be both required for draw and required for activation? How does
this make sure we get both notifications if the pending tree has no new tiles
compared to the active tree?

[ernstm, 2014-10-24 04:54:20]

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

https://codereview.chromium.org/672283003/diff/1/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:391: if (task_set == REQUIRED_FOR_DRAW) {
On 2014/10/24 01:34:04, brianderson wrote:
> Can a task_set be both required for draw and required for activation? How does
> this make sure we get both notifications if the pending tree has no new tiles
> compared to the active tree?

We get separate DidFinishRunningTasks notifications for each set that finishes. 
They are sent when all tasks in a set finished. If a set is empty (e.g. no tiles
required for activation), we still get a notification for it.

[vmpstr, 2014-10-24 18:02:24]

I'm wondering if there's a better way to capture the fact that activation and
draw checks are very similar to each other. Especially for the purposes of
plumbing, maybe we can have something like an enum or something like that that
tells us what we should check for. That could be cleaner, or maybe there is an
even cleaner way?

https://codereview.chromium.org/672283003/diff/1/cc/layers/picture_layer_impl.cc
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/672283003/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl.cc:1372: const char* action_name,
I don't think we need this. I'm not a fan of passing information like this just
so tracing is easier to interpret. We can just have an event that says "Tile
required, but not ready to draw"; we can see what it is required for from the
higher level trace.

https://codereview.chromium.org/672283003/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl.cc:1407: std::string event_name = "Tiles required
for " +
This would be constructed even when tracing is not enabled, which is something
we should avoid I think.

https://codereview.chromium.org/672283003/diff/1/cc/layers/picture_layer_impl.h
File cc/layers/picture_layer_impl.h (right):

https://codereview.chromium.org/672283003/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl.h:154: bool AllTilesRequiredAreReadyToDraw(
Can you put a comment here explaining what this function does (and what the
parameters are).

Also, I think this can be private.

https://codereview.chromium.org/672283003/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl.h:155: const char* event_name,
const std::string&?

https://codereview.chromium.org/672283003/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl.h:157: IsTileRequiredCallback) const;
this should be named as a variable, not a function.

https://codereview.chromium.org/672283003/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl.h:158: static bool
IsTileRequiredForActivation(PictureLayerTiling* tiling,
These should be anonymous in the implementation.

https://codereview.chromium.org/672283003/diff/1/cc/resources/picture_layer_t...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/672283003/diff/1/cc/resources/picture_layer_t...
cc/resources/picture_layer_tiling.cc:696: // Note that although this function
will determine whether tile is required
Can you remove "although" here (while here :) )

https://codereview.chromium.org/672283003/diff/1/cc/resources/picture_layer_t...
cc/resources/picture_layer_tiling.cc:740: // Note that although this function
will determine whether tile is required
Same here.

https://codereview.chromium.org/672283003/diff/1/cc/resources/picture_layer_t...
cc/resources/picture_layer_tiling.cc:745: // activation on tiles outside of the
viewport.
This is not correct, this should say something similar about draw.

https://codereview.chromium.org/672283003/diff/1/cc/resources/raster_worker_p...
File cc/resources/raster_worker_pool_perftest.cc (right):

https://codereview.chromium.org/672283003/diff/1/cc/resources/raster_worker_p...
cc/resources/raster_worker_pool_perftest.cc:171: REQUIRED_FOR_DRAW = 0,
Can you put this after activation, please? (alphabetical and old values
shouldn't change)

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

https://codereview.chromium.org/672283003/diff/1/cc/resources/tile.h#newcode187
cc/resources/tile.h:187: bool required_for_draw_;
Initialize in the ctor please.

https://codereview.chromium.org/672283003/diff/1/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/672283003/diff/1/cc/scheduler/scheduler.cc#ne...
cc/scheduler/scheduler.cc:201: // This is empty for now, until we take action
based on the notification.
Is there a TODO to go along or a bug number or something? If so, put that in
here please.

https://codereview.chromium.org/672283003/diff/1/cc/trees/single_thread_proxy.cc
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/672283003/diff/1/cc/trees/single_thread_proxy...
cc/trees/single_thread_proxy.cc:341: // Thread-only feature.
This would still be called though right? (if we're using single threaded impl
side)

[reveman, 2014-10-24 18:16:31]

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

https://codereview.chromium.org/672283003/diff/1/cc/resources/tile.h#newcode94
cc/resources/tile.h:94: bool required_for_draw() const { return
required_for_draw_; }
"required_for_draw" makes it sound like drawing is blocked until this tile has
been initialized. shouldn't it be "visible" instead? which is a concept that
already exists. having both "required_for_draw" and "visible" is confusing..

[enne (OOO), 2014-10-24 19:16:28]

https://codereview.chromium.org/672283003/diff/1/cc/resources/picture_layer_t...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/672283003/diff/1/cc/resources/picture_layer_t...
cc/resources/picture_layer_tiling.cc:745: // activation on tiles outside of the
viewport.
"activation", eh?

I know that this is a copy-paste from IsTileRequiredForActivation.  Could these
be renamed to be like IsTileRequiredForFoobarIfVisible to make it clear that
it's not the final determination?  I think that would help not need this
gigantic comment?

https://codereview.chromium.org/672283003/diff/1/cc/resources/raster_worker_p...
File cc/resources/raster_worker_pool_perftest.cc (right):

https://codereview.chromium.org/672283003/diff/1/cc/resources/raster_worker_p...
cc/resources/raster_worker_pool_perftest.cc:171: REQUIRED_FOR_DRAW = 0,
style nit: this is how enums are numbered automatically, no need for = x.

https://codereview.chromium.org/672283003/diff/1/cc/trees/single_thread_proxy.cc
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/672283003/diff/1/cc/trees/single_thread_proxy...
cc/trees/single_thread_proxy.cc:342: NOTREACHED();
This notification will still get called, so just make this a no-op.

[ernstm, 2014-10-27 21:13:57]

https://codereview.chromium.org/672283003/diff/1/cc/layers/picture_layer_impl.cc
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/672283003/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl.cc:1372: const char* action_name,
On 2014/10/24 18:02:23, vmpstr wrote:
> I don't think we need this. I'm not a fan of passing information like this
just
> so tracing is easier to interpret. We can just have an event that says "Tile
> required, but not ready to draw"; we can see what it is required for from the
> higher level trace.

Done.

https://codereview.chromium.org/672283003/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl.cc:1407: std::string event_name = "Tiles required
for " +
On 2014/10/24 18:02:23, vmpstr wrote:
> This would be constructed even when tracing is not enabled, which is something
> we should avoid I think.

Done.

https://codereview.chromium.org/672283003/diff/1/cc/layers/picture_layer_impl.h
File cc/layers/picture_layer_impl.h (right):

https://codereview.chromium.org/672283003/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl.h:155: const char* event_name,
On 2014/10/24 18:02:23, vmpstr wrote:
> const std::string&?

Removed.

https://codereview.chromium.org/672283003/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl.h:157: IsTileRequiredCallback) const;
On 2014/10/24 18:02:23, vmpstr wrote:
> this should be named as a variable, not a function.

Done.

https://codereview.chromium.org/672283003/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl.h:158: static bool
IsTileRequiredForActivation(PictureLayerTiling* tiling,
On 2014/10/24 18:02:23, vmpstr wrote:
> These should be anonymous in the implementation.

Done.

https://codereview.chromium.org/672283003/diff/1/cc/resources/picture_layer_t...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/672283003/diff/1/cc/resources/picture_layer_t...
cc/resources/picture_layer_tiling.cc:696: // Note that although this function
will determine whether tile is required
On 2014/10/24 18:02:24, vmpstr wrote:
> Can you remove "although" here (while here :) )

Done.

https://codereview.chromium.org/672283003/diff/1/cc/resources/picture_layer_t...
cc/resources/picture_layer_tiling.cc:740: // Note that although this function
will determine whether tile is required
On 2014/10/24 18:02:24, vmpstr wrote:
> Same here.

Done.

https://codereview.chromium.org/672283003/diff/1/cc/resources/picture_layer_t...
cc/resources/picture_layer_tiling.cc:745: // activation on tiles outside of the
viewport.
On 2014/10/24 19:16:28, enne wrote:
> "activation", eh?
> 
> I know that this is a copy-paste from IsTileRequiredForActivation.  Could
these
> be renamed to be like IsTileRequiredForFoobarIfVisible to make it clear that
> it's not the final determination?  I think that would help not need this
> gigantic comment?

Done.

https://codereview.chromium.org/672283003/diff/1/cc/resources/raster_worker_p...
File cc/resources/raster_worker_pool_perftest.cc (right):

https://codereview.chromium.org/672283003/diff/1/cc/resources/raster_worker_p...
cc/resources/raster_worker_pool_perftest.cc:171: REQUIRED_FOR_DRAW = 0,
On 2014/10/24 19:16:28, enne wrote:
> style nit: this is how enums are numbered automatically, no need for = x.

Done.

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

https://codereview.chromium.org/672283003/diff/1/cc/resources/tile.h#newcode94
cc/resources/tile.h:94: bool required_for_draw() const { return
required_for_draw_; }
On 2014/10/24 18:16:31, reveman wrote:
> "required_for_draw" makes it sound like drawing is blocked until this tile has
> been initialized. shouldn't it be "visible" instead? which is a concept that
> already exists. having both "required_for_draw" and "visible" is confusing..

It is tiles that are visible, high-res and on the active tree. Those are the
ones that are required for drawing without an approximation such as low-res or
checkerboard.

https://codereview.chromium.org/672283003/diff/1/cc/resources/tile.h#newcode187
cc/resources/tile.h:187: bool required_for_draw_;
On 2014/10/24 18:02:24, vmpstr wrote:
> Initialize in the ctor please.

Done.

https://codereview.chromium.org/672283003/diff/1/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/672283003/diff/1/cc/scheduler/scheduler.cc#ne...
cc/scheduler/scheduler.cc:201: // This is empty for now, until we take action
based on the notification.
On 2014/10/24 18:02:24, vmpstr wrote:
> Is there a TODO to go along or a bug number or something? If so, put that in
> here please.

Done.

https://codereview.chromium.org/672283003/diff/1/cc/trees/single_thread_proxy.cc
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/672283003/diff/1/cc/trees/single_thread_proxy...
cc/trees/single_thread_proxy.cc:341: // Thread-only feature.
On 2014/10/24 18:02:24, vmpstr wrote:
> This would still be called though right? (if we're using single threaded impl
> side)

Acknowledged.

https://codereview.chromium.org/672283003/diff/1/cc/trees/single_thread_proxy...
cc/trees/single_thread_proxy.cc:342: NOTREACHED();
On 2014/10/24 19:16:28, enne wrote:
> This notification will still get called, so just make this a no-op.

Done.

[reveman, 2014-10-28 15:52:25]

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

https://codereview.chromium.org/672283003/diff/20001/cc/resources/tile_manage...
cc/resources/tile_manager.h:50: // Called when all tiles required to draw are
ready.
nit: "marked as required..." to be consistent with comment above

https://codereview.chromium.org/672283003/diff/20001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/672283003/diff/20001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:202: // crbugs 352894, 383157, 421923.
Could we at least make this perform a UpdateVisibleTiles/CheckForCompletedTasks
check so that we redraw if we have initialized visible tiles?

[enne (OOO), 2014-10-28 19:07:47]

https://codereview.chromium.org/672283003/diff/20001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/672283003/diff/20001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:202: // crbugs 352894, 383157, 421923.
On 2014/10/28 at 15:52:24, reveman wrote:
> Could we at least make this perform a
UpdateVisibleTiles/CheckForCompletedTasks check so that we redraw if we have
initialized visible tiles?

+danakj

[reveman, 2014-10-28 19:20:10]

https://codereview.chromium.org/672283003/diff/20001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/672283003/diff/20001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:202: // crbugs 352894, 383157, 421923.
On 2014/10/28 15:52:24, reveman wrote:
> Could we at least make this perform a
UpdateVisibleTiles/CheckForCompletedTasks
> check so that we redraw if we have initialized visible tiles?

Never mind. I see that you already have this check in
TileManager::CheckIfReadyToDraw().

We might want to remove the scheduler plumbing from this patch as it's not used
yet and land the part up to the CheckForCompletedTasks in the TileManager for
now as that is useful on its own.

[brianderson, 2014-10-28 22:11:07]

Schduler parts lgtm.

https://codereview.chromium.org/672283003/diff/20001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/672283003/diff/20001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:202: // crbugs 352894, 383157, 421923.
On 2014/10/28 19:20:10, reveman wrote:
> On 2014/10/28 15:52:24, reveman wrote:
> > Could we at least make this perform a
> UpdateVisibleTiles/CheckForCompletedTasks
> > check so that we redraw if we have initialized visible tiles?
> 
> Never mind. I see that you already have this check in
> TileManager::CheckIfReadyToDraw().
> 
> We might want to remove the scheduler plumbing from this patch as it's not
used
> yet and land the part up to the CheckForCompletedTasks in the TileManager for
> now as that is useful on its own.

We'll use this notification in the Scheduler very soon, so keeping it in this
patch seems fine.

Somewhat related to the UpdateVisibleTiles stuff is
https://codereview.chromium.org/671653005.  Looks like Dana want's to move the
responsibility entirely over to the TileManager.

[ernstm, 2014-10-28 22:13:09]

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

https://codereview.chromium.org/672283003/diff/20001/cc/resources/tile_manage...
cc/resources/tile_manager.h:50: // Called when all tiles required to draw are
ready.
On 2014/10/28 15:52:24, reveman wrote:
> nit: "marked as required..." to be consistent with comment above

Done.

[ernstm, 2014-10-30 22:07:13]

On 2014/10/28 22:13:09, ernstm wrote:
>
https://codereview.chromium.org/672283003/diff/20001/cc/resources/tile_manager.h
> File cc/resources/tile_manager.h (right):
> 
>
https://codereview.chromium.org/672283003/diff/20001/cc/resources/tile_manage...
> cc/resources/tile_manager.h:50: // Called when all tiles required to draw are
> ready.
> On 2014/10/28 15:52:24, reveman wrote:
> > nit: "marked as required..." to be consistent with comment above
> 
> Done.

Enne, Dave, can we move forward with this patch?

[reveman, 2014-10-31 16:23:13]

just some cleanup requests. this looks good in general and I I think we should
land it % enne's review

https://codereview.chromium.org/672283003/diff/20001/cc/resources/tile_manage...
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/672283003/diff/20001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:398: }
Please replace all these "if (task_set == X) return" with one "switch
(task_set)" statement.

Also, please move the TRACE_EVENT to the top of the function instead of having
one for each case. If you need "task_set" to be a string in the trace rather
than an integer, add a simple "const char* TaskSetName(TaskSet task_set)"
utility function for that.

nit: please order the cases as the enum: ALL, REQUIRED_FOR_ACTIVATION,
REQUIRED_FOR_DRAW

[danakj, 2014-10-31 17:00:56]

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

[danakj, 2014-10-31 17:00:58]

https://codereview.chromium.org/672283003/diff/20001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/672283003/diff/20001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:1363: const
base::Callback<bool(PictureLayerTiling* tiling, const Tile*)>&
Can you give this callback type a name somewhere, so you don't have to repeat
its definition everywhere like this?

https://codereview.chromium.org/672283003/diff/20001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:1429:
base::Bind(&IsTileRequiredForActivationIfVisible));
this seems like a functor (function pointer) would be better suited for this,
like how STL classes work, than Bind/Callback which is much more heavyweight.

You can templatize the AllTilesRequiredAreReadyToDraw like

template <typename F>
bool AllTilesRequiredAreReadyToDraw(const F& is_tile_required_callback) const {
...
   ... is_tile_required_callback(tiling, tile) ...
...
}


And then just call it like
  return AllTilesRequiredAreReadyToDraw(&IsTileRequiredForActivationIfVisible);

https://codereview.chromium.org/672283003/diff/20001/cc/resources/picture_lay...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/672283003/diff/20001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling.cc:784: if (tree == PENDING_TREE)
{}

https://codereview.chromium.org/672283003/diff/20001/cc/resources/tile_manage...
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/672283003/diff/20001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:887: for
(std::vector<PictureLayerImpl*>::const_iterator it = layers.begin();
for (const auto& layer : layers)  ?

[danakj, 2014-10-31 17:07:55]

https://codereview.chromium.org/672283003/diff/20001/cc/resources/tile_manage...
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/672283003/diff/20001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:910: rasterizer_->CheckForCompletedTasks();
reveman: In my patch you said we shouldn't do this in the case where we're done
all the tiles because it's too heavy. Why is it okay here then? Shouldn't we
just tell client_ that it's ready to draw and have it UpdateVisibleTiles when it
actually does draw?

https://codereview.chromium.org/672283003/diff/20001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/672283003/diff/20001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:202: // crbugs 352894, 383157, 421923.
On 2014/10/28 22:11:07, brianderson wrote:
> On 2014/10/28 19:20:10, reveman wrote:
> > On 2014/10/28 15:52:24, reveman wrote:
> > > Could we at least make this perform a
> > UpdateVisibleTiles/CheckForCompletedTasks
> > > check so that we redraw if we have initialized visible tiles?
> > 
> > Never mind. I see that you already have this check in
> > TileManager::CheckIfReadyToDraw().
> > 
> > We might want to remove the scheduler plumbing from this patch as it's not
> used
> > yet and land the part up to the CheckForCompletedTasks in the TileManager
for
> > now as that is useful on its own.
> 
> We'll use this notification in the Scheduler very soon, so keeping it in this
> patch seems fine.
> 
> Somewhat related to the UpdateVisibleTiles stuff is
> https://codereview.chromium.org/671653005.  Looks like Dana want's to move the
> responsibility entirely over to the TileManager.

I think this plumbing can go away. We can just SetNeedsRedraw from LTHI when we
want to draw something.

[reveman, 2014-10-31 17:46:08]

https://codereview.chromium.org/672283003/diff/20001/cc/resources/tile_manage...
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/672283003/diff/20001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:887: for
(std::vector<PictureLayerImpl*>::const_iterator it = layers.begin();
On 2014/10/31 17:00:57, danakj wrote:
> for (const auto& layer : layers)  ?

yes, that would be nice. please update IsReadyToActivate() too so things are
consistent.

https://codereview.chromium.org/672283003/diff/20001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:910: rasterizer_->CheckForCompletedTasks();
On 2014/10/31 17:07:55, danakj wrote:
> reveman: In my patch you said we shouldn't do this in the case where we're
done
> all the tiles because it's too heavy. Why is it okay here then? Shouldn't we
> just tell client_ that it's ready to draw and have it UpdateVisibleTiles when
it
> actually does draw?

I don't think we have much of a choice here. We're not really ready to draw
here. CheckIfReadyToDraw() is only called as a result of all raster tasks that
were marked as required for draw and could be scheduled have completed. Some
ready to draw tasks might not have been scheduled. We need to call
TileManager::IsReadyToDraw() to determine if all tasks marked as required to
draw are ready. It would be hard to do this without calling
CheckForCompletedTasks().

FYI, CheckForCompletedTasks() will acquire the task graph runner lock and
process completion of tasks. It's a waste if the completion of these tasks are
not useful to us. In this case, I think they typically are.

[danakj, 2014-10-31 18:41:31]

I think I get what's going in this patch and it looks good overall. I've left my
last comment. If you can address that and my previous ones I'd be happy to give
this a stamp in place of enne@ who is unable to review for a few days, if you
like.

I'll rebase my UpdateVisibleTiles patch on top of this.

https://codereview.chromium.org/672283003/diff/20001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/672283003/diff/20001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:1363: const
base::Callback<bool(PictureLayerTiling* tiling, const Tile*)>&
On 2014/10/31 17:00:57, danakj wrote:
> Can you give this callback type a name somewhere, so you don't have to repeat
> its definition everywhere like this?

ignore this comment :)

https://codereview.chromium.org/672283003/diff/20001/cc/resources/rasterizer.h
File cc/resources/rasterizer.h (right):

https://codereview.chromium.org/672283003/diff/20001/cc/resources/rasterizer....
cc/resources/rasterizer.h:90: static const size_t kNumberOfTaskSets = 3;
can you leave a comment here pointing back to the NamedTaskSets as well?

[danakj, 2014-10-31 18:42:10]

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

https://codereview.chromium.org/672283003/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1334: client_->NotifyReadyToDraw();
Oh, since if you remove the scheduler plumbing this function is just empty, you
can stick a TODO(danakj) to do something here if you like, or just leave it
empty for now.

[reveman, 2014-10-31 19:38:52]

https://codereview.chromium.org/672283003/diff/20001/cc/resources/rasterizer.h
File cc/resources/rasterizer.h (right):

https://codereview.chromium.org/672283003/diff/20001/cc/resources/rasterizer....
cc/resources/rasterizer.h:90: static const size_t kNumberOfTaskSets = 3;
On 2014/10/31 18:41:31, danakj wrote:
> can you leave a comment here pointing back to the NamedTaskSets as well?

That would be a bit of a layer violation but fine for now if you think that's
important.

Btw, if you're adding a comment here, please add "TODO(reveman): Use template
specialization to make it easy for client code to check at compile time that the
number of supported task sets is correct."

[danakj, 2014-10-31 19:40:50]

https://codereview.chromium.org/672283003/diff/20001/cc/resources/rasterizer.h
File cc/resources/rasterizer.h (right):

https://codereview.chromium.org/672283003/diff/20001/cc/resources/rasterizer....
cc/resources/rasterizer.h:90: static const size_t kNumberOfTaskSets = 3;
On 2014/10/31 19:38:51, reveman wrote:
> On 2014/10/31 18:41:31, danakj wrote:
> > can you leave a comment here pointing back to the NamedTaskSets as well?
> 
> That would be a bit of a layer violation but fine for now if you think that's
> important.
> 
> Btw, if you're adding a comment here, please add "TODO(reveman): Use template
> specialization to make it easy for client code to check at compile time that
the
> number of supported task sets is correct."

The fact this number is tied to that enum is the layering violation, I think.
The comment just documents it, I'd like to have a comment so I can understand
what this constant is in the future :)

[reveman, 2014-10-31 20:06:06]

https://codereview.chromium.org/672283003/diff/20001/cc/resources/rasterizer.h
File cc/resources/rasterizer.h (right):

https://codereview.chromium.org/672283003/diff/20001/cc/resources/rasterizer....
cc/resources/rasterizer.h:90: static const size_t kNumberOfTaskSets = 3;
On 2014/10/31 19:40:50, danakj wrote:
> On 2014/10/31 19:38:51, reveman wrote:
> > On 2014/10/31 18:41:31, danakj wrote:
> > > can you leave a comment here pointing back to the NamedTaskSets as well?
> > 
> > That would be a bit of a layer violation but fine for now if you think
that's
> > important.
> > 
> > Btw, if you're adding a comment here, please add "TODO(reveman): Use
template
> > specialization to make it easy for client code to check at compile time that
> the
> > number of supported task sets is correct."
> 
> The fact this number is tied to that enum is the layering violation, I think.
> The comment just documents it, I'd like to have a comment so I can understand
> what this constant is in the future :)

I don't think there's a layering violation here. The Rasterizer interface
provides support for 3 task sets. It's completely up to the client to decide
what to use those for. The client code is broken if it's trying to use more task
sets than this limit. Note that this should ideally be a template parameter to
the Rasterizer class but it was too large of a change to do that in the initial
patch that implemented generic task sets.

[danakj, 2014-10-31 20:45:03]

https://codereview.chromium.org/672283003/diff/20001/cc/resources/rasterizer.h
File cc/resources/rasterizer.h (right):

https://codereview.chromium.org/672283003/diff/20001/cc/resources/rasterizer....
cc/resources/rasterizer.h:90: static const size_t kNumberOfTaskSets = 3;
On 2014/10/31 20:06:06, reveman wrote:
> On 2014/10/31 19:40:50, danakj wrote:
> > On 2014/10/31 19:38:51, reveman wrote:
> > > On 2014/10/31 18:41:31, danakj wrote:
> > > > can you leave a comment here pointing back to the NamedTaskSets as well?
> > > 
> > > That would be a bit of a layer violation but fine for now if you think
> that's
> > > important.
> > > 
> > > Btw, if you're adding a comment here, please add "TODO(reveman): Use
> template
> > > specialization to make it easy for client code to check at compile time
that
> > the
> > > number of supported task sets is correct."
> > 
> > The fact this number is tied to that enum is the layering violation, I
think.
> > The comment just documents it, I'd like to have a comment so I can
understand
> > what this constant is in the future :)
> 
> I don't think there's a layering violation here. The Rasterizer interface
> provides support for 3 task sets. It's completely up to the client to decide
> what to use those for. The client code is broken if it's trying to use more
task
> sets than this limit. Note that this should ideally be a template parameter to
> the Rasterizer class but it was too large of a change to do that in the
initial
> patch that implemented generic task sets.

Well, it only supports 3 because the TileManager wants 3 :) A template parameter
would let the constant live in the right spot, or a const member on the class
you pass to the constructor.

[reveman, 2014-10-31 21:23:06]

On 2014/10/31 20:45:03, danakj wrote:
> https://codereview.chromium.org/672283003/diff/20001/cc/resources/rasterizer.h
> File cc/resources/rasterizer.h (right):
> 
>
https://codereview.chromium.org/672283003/diff/20001/cc/resources/rasterizer....
> cc/resources/rasterizer.h:90: static const size_t kNumberOfTaskSets = 3;
> On 2014/10/31 20:06:06, reveman wrote:
> > On 2014/10/31 19:40:50, danakj wrote:
> > > On 2014/10/31 19:38:51, reveman wrote:
> > > > On 2014/10/31 18:41:31, danakj wrote:
> > > > > can you leave a comment here pointing back to the NamedTaskSets as
well?
> > > > 
> > > > That would be a bit of a layer violation but fine for now if you think
> > that's
> > > > important.
> > > > 
> > > > Btw, if you're adding a comment here, please add "TODO(reveman): Use
> > template
> > > > specialization to make it easy for client code to check at compile time
> that
> > > the
> > > > number of supported task sets is correct."
> > > 
> > > The fact this number is tied to that enum is the layering violation, I
> think.
> > > The comment just documents it, I'd like to have a comment so I can
> understand
> > > what this constant is in the future :)
> > 
> > I don't think there's a layering violation here. The Rasterizer interface
> > provides support for 3 task sets. It's completely up to the client to decide
> > what to use those for. The client code is broken if it's trying to use more
> task
> > sets than this limit. Note that this should ideally be a template parameter
to
> > the Rasterizer class but it was too large of a change to do that in the
> initial
> > patch that implemented generic task sets.
> 
> Well, it only supports 3 because the TileManager wants 3 :)

We can make it 10 and still keep the tile manager happy :) But sure, it's a bit
of a layering violation. Similar to base::Bind only supporting 7 arguments
because chrome wants 7. Difference is that in the base::Bind case you get a
compile time failure if you use it in an unsupported way. Here you don't. That's
what I'd like to fix sometime soon.

> A template parameter
> would let the constant live in the right spot, or a const member on the class
> you pass to the constructor.

FYI, a const member wouldn't allow use to use std::bitset which is a requirement
for good performance.

[brianderson, 2014-10-31 23:04:24]

https://codereview.chromium.org/672283003/diff/20001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/672283003/diff/20001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:202: // crbugs 352894, 383157, 421923.
On 2014/10/31 17:07:55, danakj wrote:
> On 2014/10/28 22:11:07, brianderson wrote:
> > On 2014/10/28 19:20:10, reveman wrote:
> > > On 2014/10/28 15:52:24, reveman wrote:
> > > > Could we at least make this perform a
> > > UpdateVisibleTiles/CheckForCompletedTasks
> > > > check so that we redraw if we have initialized visible tiles?
> > > 
> > > Never mind. I see that you already have this check in
> > > TileManager::CheckIfReadyToDraw().
> > > 
> > > We might want to remove the scheduler plumbing from this patch as it's not
> > used
> > > yet and land the part up to the CheckForCompletedTasks in the TileManager
> for
> > > now as that is useful on its own.
> > 
> > We'll use this notification in the Scheduler very soon, so keeping it in
this
> > patch seems fine.
> > 
> > Somewhat related to the UpdateVisibleTiles stuff is
> > https://codereview.chromium.org/671653005.  Looks like Dana want's to move
the
> > responsibility entirely over to the TileManager.
> 
> I think this plumbing can go away. We can just SetNeedsRedraw from LTHI when
we
> want to draw something.

NotifyReadyToDraw will indicate that we can draw without checkerboarding.
SetNeedsRedraw only indicates that we have something new to draw without any
guarantees about checkerboarding.

Once the scheduler has NotifyReadyToDraw, I'm imaging the scheduler will do the
following:
1) Change AdjustBeginImplFrameDeadline to not post the deadline immediately.
2) Then trigger the deadline early only once we get the NotifyReadyToDraw
signal.

Today, we get away without the above because of prepainting. Newly visible tiles
are usually ready. With Ganesh, we plan to move away from prepainting and it
will become much more important to check for checkerboarding before we draw.

[danakj, 2014-11-03 16:11:12]

https://codereview.chromium.org/672283003/diff/20001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/672283003/diff/20001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:202: // crbugs 352894, 383157, 421923.
On 2014/10/31 23:04:24, brianderson wrote:
> On 2014/10/31 17:07:55, danakj wrote:
> > On 2014/10/28 22:11:07, brianderson wrote:
> > > On 2014/10/28 19:20:10, reveman wrote:
> > > > On 2014/10/28 15:52:24, reveman wrote:
> > > > > Could we at least make this perform a
> > > > UpdateVisibleTiles/CheckForCompletedTasks
> > > > > check so that we redraw if we have initialized visible tiles?
> > > > 
> > > > Never mind. I see that you already have this check in
> > > > TileManager::CheckIfReadyToDraw().
> > > > 
> > > > We might want to remove the scheduler plumbing from this patch as it's
not
> > > used
> > > > yet and land the part up to the CheckForCompletedTasks in the
TileManager
> > for
> > > > now as that is useful on its own.
> > > 
> > > We'll use this notification in the Scheduler very soon, so keeping it in
> this
> > > patch seems fine.
> > > 
> > > Somewhat related to the UpdateVisibleTiles stuff is
> > > https://codereview.chromium.org/671653005.  Looks like Dana want's to move
> the
> > > responsibility entirely over to the TileManager.
> > 
> > I think this plumbing can go away. We can just SetNeedsRedraw from LTHI when
> we
> > want to draw something.
> 
> NotifyReadyToDraw will indicate that we can draw without checkerboarding.
> SetNeedsRedraw only indicates that we have something new to draw without any
> guarantees about checkerboarding.
> 
> Once the scheduler has NotifyReadyToDraw, I'm imaging the scheduler will do
the
> following:
> 1) Change AdjustBeginImplFrameDeadline to not post the deadline immediately.
> 2) Then trigger the deadline early only once we get the NotifyReadyToDraw
> signal.
> 
> Today, we get away without the above because of prepainting. Newly visible
tiles
> are usually ready. With Ganesh, we plan to move away from prepainting and it
> will become much more important to check for checkerboarding before we draw.

Interesting, how does that play out with other sources of SetNeedsRedraw besides
invalidation? Such as a layer moving on the screen causing damage and calling
SetNeedsRedraw?

[brianderson, 2014-11-03 21:27:29]

On 2014/11/03 16:11:12, danakj wrote:
> https://codereview.chromium.org/672283003/diff/20001/cc/scheduler/scheduler.cc
> File cc/scheduler/scheduler.cc (right):
> 
>
https://codereview.chromium.org/672283003/diff/20001/cc/scheduler/scheduler.c...
> cc/scheduler/scheduler.cc:202: // crbugs 352894, 383157, 421923.
> On 2014/10/31 23:04:24, brianderson wrote:
> > On 2014/10/31 17:07:55, danakj wrote:
> > > On 2014/10/28 22:11:07, brianderson wrote:
> > > > On 2014/10/28 19:20:10, reveman wrote:
> > > > > On 2014/10/28 15:52:24, reveman wrote:
> > > > > > Could we at least make this perform a
> > > > > UpdateVisibleTiles/CheckForCompletedTasks
> > > > > > check so that we redraw if we have initialized visible tiles?
> > > > > 
> > > > > Never mind. I see that you already have this check in
> > > > > TileManager::CheckIfReadyToDraw().
> > > > > 
> > > > > We might want to remove the scheduler plumbing from this patch as it's
> not
> > > > used
> > > > > yet and land the part up to the CheckForCompletedTasks in the
> TileManager
> > > for
> > > > > now as that is useful on its own.
> > > > 
> > > > We'll use this notification in the Scheduler very soon, so keeping it in
> > this
> > > > patch seems fine.
> > > > 
> > > > Somewhat related to the UpdateVisibleTiles stuff is
> > > > https://codereview.chromium.org/671653005.  Looks like Dana want's to
move
> > the
> > > > responsibility entirely over to the TileManager.
> > > 
> > > I think this plumbing can go away. We can just SetNeedsRedraw from LTHI
when
> > we
> > > want to draw something.
> > 
> > NotifyReadyToDraw will indicate that we can draw without checkerboarding.
> > SetNeedsRedraw only indicates that we have something new to draw without any
> > guarantees about checkerboarding.
> > 
> > Once the scheduler has NotifyReadyToDraw, I'm imaging the scheduler will do
> the
> > following:
> > 1) Change AdjustBeginImplFrameDeadline to not post the deadline immediately.
> > 2) Then trigger the deadline early only once we get the NotifyReadyToDraw
> > signal.
> > 
> > Today, we get away without the above because of prepainting. Newly visible
> tiles
> > are usually ready. With Ganesh, we plan to move away from prepainting and it
> > will become much more important to check for checkerboarding before we draw.
> 
> Interesting, how does that play out with other sources of SetNeedsRedraw
besides
> invalidation? Such as a layer moving on the screen causing damage and calling
> SetNeedsRedraw?

The intent is to wait for NotifyReadyToDraw in those (and all) cases, until we
reach the deadline. At the deadline, we may still decide to draw with
checkerboard.

[ernstm, 2014-11-03 22:34:42]

All comments should be addressed by the latest patch. I've left the scheduler
plumbing in the patch, because Brian has immediate use for it.

https://codereview.chromium.org/672283003/diff/20001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/672283003/diff/20001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:1429:
base::Bind(&IsTileRequiredForActivationIfVisible));
On 2014/10/31 17:00:57, danakj wrote:
> this seems like a functor (function pointer) would be better suited for this,
> like how STL classes work, than Bind/Callback which is much more heavyweight.
> 
> You can templatize the AllTilesRequiredAreReadyToDraw like
> 
> template <typename F>
> bool AllTilesRequiredAreReadyToDraw(const F& is_tile_required_callback) const
{
> ...
>    ... is_tile_required_callback(tiling, tile) ...
> ...
> }
> 
> 
> And then just call it like
>   return
AllTilesRequiredAreReadyToDraw(&IsTileRequiredForActivationIfVisible);

Done.

https://codereview.chromium.org/672283003/diff/20001/cc/resources/picture_lay...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/672283003/diff/20001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling.cc:784: if (tree == PENDING_TREE)
On 2014/10/31 17:00:57, danakj wrote:
> {}

Done.

https://codereview.chromium.org/672283003/diff/20001/cc/resources/rasterizer.h
File cc/resources/rasterizer.h (right):

https://codereview.chromium.org/672283003/diff/20001/cc/resources/rasterizer....
cc/resources/rasterizer.h:90: static const size_t kNumberOfTaskSets = 3;
On 2014/10/31 20:45:03, danakj wrote:
> On 2014/10/31 20:06:06, reveman wrote:
> > On 2014/10/31 19:40:50, danakj wrote:
> > > On 2014/10/31 19:38:51, reveman wrote:
> > > > On 2014/10/31 18:41:31, danakj wrote:
> > > > > can you leave a comment here pointing back to the NamedTaskSets as
well?
> > > > 
> > > > That would be a bit of a layer violation but fine for now if you think
> > that's
> > > > important.
> > > > 
> > > > Btw, if you're adding a comment here, please add "TODO(reveman): Use
> > template
> > > > specialization to make it easy for client code to check at compile time
> that
> > > the
> > > > number of supported task sets is correct."
> > > 
> > > The fact this number is tied to that enum is the layering violation, I
> think.
> > > The comment just documents it, I'd like to have a comment so I can
> understand
> > > what this constant is in the future :)
> > 
> > I don't think there's a layering violation here. The Rasterizer interface
> > provides support for 3 task sets. It's completely up to the client to decide
> > what to use those for. The client code is broken if it's trying to use more
> task
> > sets than this limit. Note that this should ideally be a template parameter
to
> > the Rasterizer class but it was too large of a change to do that in the
> initial
> > patch that implemented generic task sets.
> 
> Well, it only supports 3 because the TileManager wants 3 :) A template
parameter
> would let the constant live in the right spot, or a const member on the class
> you pass to the constructor.

Done. I've added the comment pointing back to TileManager::NamedTaskSet and the
TODO for template specialization.

https://codereview.chromium.org/672283003/diff/20001/cc/resources/tile_manage...
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/672283003/diff/20001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:398: }
On 2014/10/31 16:23:12, reveman wrote:
> Please replace all these "if (task_set == X) return" with one "switch
> (task_set)" statement.
> 
> Also, please move the TRACE_EVENT to the top of the function instead of having
> one for each case. If you need "task_set" to be a string in the trace rather
> than an integer, add a simple "const char* TaskSetName(TaskSet task_set)"
> utility function for that.
> 
> nit: please order the cases as the enum: ALL, REQUIRED_FOR_ACTIVATION,
> REQUIRED_FOR_DRAW

Done.

https://codereview.chromium.org/672283003/diff/20001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:887: for
(std::vector<PictureLayerImpl*>::const_iterator it = layers.begin();
On 2014/10/31 17:00:57, danakj wrote:
> for (const auto& layer : layers)  ?

Done.

https://codereview.chromium.org/672283003/diff/20001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:910: rasterizer_->CheckForCompletedTasks();
On 2014/10/31 17:46:08, reveman wrote:
> On 2014/10/31 17:07:55, danakj wrote:
> > reveman: In my patch you said we shouldn't do this in the case where we're
> done
> > all the tiles because it's too heavy. Why is it okay here then? Shouldn't we
> > just tell client_ that it's ready to draw and have it UpdateVisibleTiles
when
> it
> > actually does draw?
> 
> I don't think we have much of a choice here. We're not really ready to draw
> here. CheckIfReadyToDraw() is only called as a result of all raster tasks that
> were marked as required for draw and could be scheduled have completed. Some
> ready to draw tasks might not have been scheduled. We need to call
> TileManager::IsReadyToDraw() to determine if all tasks marked as required to
> draw are ready. It would be hard to do this without calling
> CheckForCompletedTasks().
> 
> FYI, CheckForCompletedTasks() will acquire the task graph runner lock and
> process completion of tasks. It's a waste if the completion of these tasks are
> not useful to us. In this case, I think they typically are.

Acknowledged.

[vmpstr, 2014-11-03 23:07:00]

lgtm % other people. Please move the anonymous functions to the top of the file,
and feel free to ignore the rest of the comments.

https://codereview.chromium.org/672283003/diff/40001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/672283003/diff/40001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:1408: template <typename F>
nit: I think F == bool (*)(PictureLayerTiling*, const Tile*) in both cases where
this is used, but feel free to leave it as in, I guess.

For bonus points (or extra complexity :P), we can

typedef bool(PictureLayerTiling::*TileRequirementCheck)(const Tile*); 
then take const TileRequirementCheck& is_tile_required (or keep template
<typename F> instead of these two steps),

and pass &PictureLayerTiling::IsTileRequiredForActivationIfVisible or
&PictureLayerTiling::IsTileRequiredForDrawIfVisible, then do

if ((tiling->*is_tile_required_callback)(tile) && !tile->IsReadyToDraw)

thus eliminating the need for the two functions.

*runs to hide* This is probably too complicated, so feel free to ignore all of
it. We could use a lambda here but I don't think those are allowed yet?

https://codereview.chromium.org/672283003/diff/40001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:1453: namespace {
nit: I prefer anonymous namespace to be at the top of the file (there is already
a namespace there, I think, it's just a matter of putting these in there).

[reveman, 2014-11-03 23:55:53]

lgtm % other reviewers

https://codereview.chromium.org/672283003/diff/40001/cc/resources/tile_manage...
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/672283003/diff/40001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:336: TaskSetName(task_set));
hm, did cl format output change recently as this is not the format of the rest
of the file? annoying to see things becoming inconsistent. You can ignore this
but I would prefer if this was kept consistent with existing code in this file
for now and we'd reformat the whole file in a follow up patch if this actually
the new correct format.

[ernstm, 2014-11-04 00:21:55]

https://codereview.chromium.org/672283003/diff/40001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/672283003/diff/40001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:1408: template <typename F>
On 2014/11/03 23:07:00, vmpstr wrote:
> nit: I think F == bool (*)(PictureLayerTiling*, const Tile*) in both cases
where
> this is used, but feel free to leave it as in, I guess.
> 
> For bonus points (or extra complexity :P), we can
> 
> typedef bool(PictureLayerTiling::*TileRequirementCheck)(const Tile*); 
> then take const TileRequirementCheck& is_tile_required (or keep template
> <typename F> instead of these two steps),
> 
> and pass &PictureLayerTiling::IsTileRequiredForActivationIfVisible or
> &PictureLayerTiling::IsTileRequiredForDrawIfVisible, then do
> 
> if ((tiling->*is_tile_required_callback)(tile) && !tile->IsReadyToDraw)
> 
> thus eliminating the need for the two functions.
> 
> *runs to hide* This is probably too complicated, so feel free to ignore all of
> it. We could use a lambda here but I don't think those are allowed yet?

Done. This is what I originally wanted to do, but I didn't know about operator
->*. Thanks!

I've dropped the const reference to function pointer and pass the function
pointer by value instead. Does the const reference offer any benefits in this
situation?

https://codereview.chromium.org/672283003/diff/40001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:1453: namespace {
On 2014/11/03 23:07:00, vmpstr wrote:
> nit: I prefer anonymous namespace to be at the top of the file (there is
already
> a namespace there, I think, it's just a matter of putting these in there). 

Done.

https://codereview.chromium.org/672283003/diff/40001/cc/resources/tile_manage...
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/672283003/diff/40001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:336: TaskSetName(task_set));
On 2014/11/03 23:55:53, reveman wrote:
> hm, did cl format output change recently as this is not the format of the rest
> of the file? annoying to see things becoming inconsistent. You can ignore this
> but I would prefer if this was kept consistent with existing code in this file
> for now and we'd reformat the whole file in a follow up patch if this actually
> the new correct format.

Done. I've reformatted it and ignored the pre-submit warning.

[danakj, 2014-11-04 22:02:59]

https://codereview.chromium.org/672283003/diff/80001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.h (right):

https://codereview.chromium.org/672283003/diff/80001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.h:156: typedef bool
(PictureLayerTiling::*TileRequirementCheck)(const Tile*) const;
can you do this with using instead of typedef? the project as a whole now
prefers that in new code.

https://codereview.chromium.org/672283003/diff/80001/cc/resources/tile_manage...
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/672283003/diff/80001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:419: default:
don't use default unless you have to, it avoids compiler errors if we add a new
enum value. just put NOTREACHED() below the switch statement.

[danakj, 2014-11-04 22:06:13]

https://codereview.chromium.org/672283003/diff/80001/cc/resources/tile_manage...
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/672283003/diff/80001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:206: default:
same here, no default please, move it below the switch

[danakj, 2014-11-04 22:09:52]

On 2014/11/03 21:27:29, brianderson wrote:
> The intent is to wait for NotifyReadyToDraw in those (and all) cases, until we
> reach the deadline. At the deadline, we may still decide to draw with
> checkerboard.

So what will be calling NotifyReadyToDraw in the case where we have nothing new
to raster? I don't see that anywhere in here. Is that an explicit followup? Do
you have an idea how you want it to work?

[ernstm, 2014-11-04 22:41:15]

https://codereview.chromium.org/672283003/diff/80001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.h (right):

https://codereview.chromium.org/672283003/diff/80001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.h:156: typedef bool
(PictureLayerTiling::*TileRequirementCheck)(const Tile*) const;
On 2014/11/04 22:02:58, danakj wrote:
> can you do this with using instead of typedef? the project as a whole now
> prefers that in new code.

Done.

https://codereview.chromium.org/672283003/diff/80001/cc/resources/tile_manage...
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/672283003/diff/80001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:206: default:
On 2014/11/04 22:06:13, danakj wrote:
> same here, no default please, move it below the switch

Done.

https://codereview.chromium.org/672283003/diff/80001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:419: default:
On 2014/11/04 22:02:59, danakj wrote:
> don't use default unless you have to, it avoids compiler errors if we add a
new
> enum value. just put NOTREACHED() below the switch statement.

Done.

[reveman, 2014-11-04 22:42:16]

On 2014/11/04 22:09:52, danakj wrote:
> On 2014/11/03 21:27:29, brianderson wrote:
> > The intent is to wait for NotifyReadyToDraw in those (and all) cases, until
we
> > reach the deadline. At the deadline, we may still decide to draw with
> > checkerboard.
> 
> So what will be calling NotifyReadyToDraw in the case where we have nothing
new
> to raster? I don't see that anywhere in here. Is that an explicit followup? Do
> you have an idea how you want it to work?

NotifyReadyToDraw will always be called as a result of a call to ManageTiles
(assuming we reach a state where all tiles marked as required to draw are
initialized). If there's no raster work that needs to be done before we reach
this state then this notification is going to be generated almost immediately
after the ManageTiles call.

[danakj, 2014-11-04 22:44:03]

On 2014/11/04 22:42:16, reveman wrote:
> On 2014/11/04 22:09:52, danakj wrote:
> > On 2014/11/03 21:27:29, brianderson wrote:
> > > The intent is to wait for NotifyReadyToDraw in those (and all) cases,
until
> we
> > > reach the deadline. At the deadline, we may still decide to draw with
> > > checkerboard.
> > 
> > So what will be calling NotifyReadyToDraw in the case where we have nothing
> new
> > to raster? I don't see that anywhere in here. Is that an explicit followup?
Do
> > you have an idea how you want it to work?
> 
> NotifyReadyToDraw will always be called as a result of a call to ManageTiles
> (assuming we reach a state where all tiles marked as required to draw are
> initialized). If there's no raster work that needs to be done before we reach
> this state then this notification is going to be generated almost immediately
> after the ManageTiles call.

Oh okay great! Is there any tests that verify this happens?

[reveman, 2014-11-04 23:02:29]

On 2014/11/04 22:44:03, danakj wrote:
> On 2014/11/04 22:42:16, reveman wrote:
> > On 2014/11/04 22:09:52, danakj wrote:
> > > On 2014/11/03 21:27:29, brianderson wrote:
> > > > The intent is to wait for NotifyReadyToDraw in those (and all) cases,
> until
> > we
> > > > reach the deadline. At the deadline, we may still decide to draw with
> > > > checkerboard.
> > > 
> > > So what will be calling NotifyReadyToDraw in the case where we have
nothing
> > new
> > > to raster? I don't see that anywhere in here. Is that an explicit
followup?
> Do
> > > you have an idea how you want it to work?
> > 
> > NotifyReadyToDraw will always be called as a result of a call to ManageTiles
> > (assuming we reach a state where all tiles marked as required to draw are
> > initialized). If there's no raster work that needs to be done before we
reach
> > this state then this notification is going to be generated almost
immediately
> > after the ManageTiles call.
> 
> Oh okay great! Is there any tests that verify this happens?

The lower level task-set notification system is tested as part of our raster
worker pool unit tests.  I don't think there's anything at the tile manager
level to explicitly verify that these Notify* client functions are actually
called. Adding something for that wouldn't hurt.

[enne (OOO), 2014-11-06 01:18:58]

In general, this all looks good to me % some tile manager tests like reveman
describes.  If we're going to depend on these signals to kick off drawing in the
future, then we should make sure to at least smoke test that the TileManager
issues them.

[danakj, 2014-11-07 15:53:52]

https://codereview.chromium.org/672283003/diff/100001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/672283003/diff/100001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:1454: TRACE_EVENT0("cc",
The equivalent traces were removed from TOT, so you can remove this

https://codereview.chromium.org/672283003/diff/100001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:1465: TRACE_EVENT0("cc",
"PictureLayerImpl::AllTilesRequiredForDrawAreReadyToDraw");
and this.

[ernstm, 2014-11-07 18:44:54]

Added a smoke test for ReadyToActivate and ReadyToDraw signals.

https://codereview.chromium.org/672283003/diff/100001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/672283003/diff/100001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:1454: TRACE_EVENT0("cc",
On 2014/11/07 15:53:52, danakj wrote:
> The equivalent traces were removed from TOT, so you can remove this

Done.

https://codereview.chromium.org/672283003/diff/100001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:1465: TRACE_EVENT0("cc",
"PictureLayerImpl::AllTilesRequiredForDrawAreReadyToDraw");
On 2014/11/07 15:53:52, danakj wrote:
> and this.

Done.

[danakj, 2014-11-07 19:08:19]

https://codereview.chromium.org/672283003/diff/120001/cc/resources/tile_manag...
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/672283003/diff/120001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:865: TRACE_EVENT0("cc",
"TileManager::IsReadyToActivate");
Should we add a similar trace in IsReadyToDraw()?

https://codereview.chromium.org/672283003/diff/120001/cc/test/layer_tree_test.h
File cc/test/layer_tree_test.h (right):

https://codereview.chromium.org/672283003/diff/120001/cc/test/layer_tree_test...
cc/test/layer_tree_test.h:241: #define
SINGLE_THREAD_DIRECT_RENDERER_IMPL_TEST_F(TEST_FIXTURE_NAME)        \
You could just override the LayerTreeSettings to enable impl side painting if
the test needs to be only with implside, which is what we've done elsewhere till
now:
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre...

https://codereview.chromium.org/672283003/diff/120001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/672283003/diff/120001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:85: EndTest();
It's much nicer to understand the test if there is only one EndTest() in it.

https://codereview.chromium.org/672283003/diff/120001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:95:
EXPECT_TRUE(did_notify_ready_to_activate_);
I was kinda expecting a test that was more long the lines of "we don't notify
ready to draw until all the raster work is done, then we do", rather than just
"a notify will happen at some point if we commit". Would that be possible?

[enne (OOO), 2014-11-07 19:12:02]

If it helps, the test cases I think would be most useful would be:
(1) there is non-zero raster work; we get a ready to draw notification; verify
raster work is done
(2) there is zero raster worker; verify we still get a ready to draw
notification

[ernstm, 2014-11-08 01:57:27]

I add tests that create raster tasks. What's missing is a good way to cause
rasterization of active tree tiles. Right now the
LayerTreeHostTestReadyToDrawNonEmpty test relies on the fact that some tiles in
single threaded mode get flagged as REQUIRED_FOR_DRAW. I'll look more into that
next week.

https://codereview.chromium.org/672283003/diff/120001/cc/resources/tile_manag...
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/672283003/diff/120001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:865: TRACE_EVENT0("cc",
"TileManager::IsReadyToActivate");
On 2014/11/07 19:08:18, danakj wrote:
> Should we add a similar trace in IsReadyToDraw()?

Done.

https://codereview.chromium.org/672283003/diff/120001/cc/test/layer_tree_test.h
File cc/test/layer_tree_test.h (right):

https://codereview.chromium.org/672283003/diff/120001/cc/test/layer_tree_test...
cc/test/layer_tree_test.h:241: #define
SINGLE_THREAD_DIRECT_RENDERER_IMPL_TEST_F(TEST_FIXTURE_NAME)        \
On 2014/11/07 19:08:18, danakj wrote:
> You could just override the LayerTreeSettings to enable impl side painting if
> the test needs to be only with implside, which is what we've done elsewhere
till
> now:
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre...

We shouldn't override the settings to force imple-side painting, if the test
says MainThreadPaint.  This would be confusing and it also causes redundant test
runs on the bots. Having separate macros for each case avoids these problems.

https://codereview.chromium.org/672283003/diff/120001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/672283003/diff/120001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:85: EndTest();
On 2014/11/07 19:08:18, danakj wrote:
> It's much nicer to understand the test if there is only one EndTest() in it.

Done. I created separate tests for ReadyToActivate and for ReadyToDraw.

[enne (OOO), 2014-11-10 20:02:12]

lgtm with a suggestions.

https://codereview.chromium.org/672283003/diff/140001/cc/test/layer_tree_test.h
File cc/test/layer_tree_test.h (right):

https://codereview.chromium.org/672283003/diff/140001/cc/test/layer_tree_test...
cc/test/layer_tree_test.h:249:
SINGLE_THREAD_DIRECT_RENDERER_IMPL_TEST_F(TEST_FIXTURE_NAME);
nit: no semicolon here to force callers to have a semicolon.

https://codereview.chromium.org/672283003/diff/140001/cc/test/layer_tree_test...
cc/test/layer_tree_test.h:295:
MULTI_THREAD_DIRECT_RENDERER_IMPL_TEST_F(TEST_FIXTURE_NAME);
nit: same thing here

https://codereview.chromium.org/672283003/diff/140001/cc/test/layer_tree_test...
cc/test/layer_tree_test.h:312:
MULTI_THREAD_DELEGATING_RENDERER_IMPL_TEST_F(TEST_FIXTURE_NAME);
nit: and here

https://codereview.chromium.org/672283003/diff/140001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/672283003/diff/140001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:129: class LayerTreeHostTestReadyToDraw :
public LayerTreeHostTest {
Can you add some DCHECK that verifies that this is empty (and maybe name the
test that way too?)

https://codereview.chromium.org/672283003/diff/140001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:162: class
LayerTreeHostTestReadyToDrawNonEmpty
Can you add some DCHECK that verifies that this is non-empty?

[ernstm, 2014-11-11 01:36:39]

The latest patch checks the number of required tiles in the unit tests. I had to
duplicate some code from PictureLayerImpl::AllTilesRequiredAreReadyToDraw into
FakePictureLayerImpl::CountTilesRequired. It would be possible to share more
code, if we used a base::callback and more parameters in
PictureLayerImpl::AllTilesRequiredAreReadyToDraw, but we wanted to avoid that.

https://codereview.chromium.org/672283003/diff/140001/cc/test/layer_tree_test.h
File cc/test/layer_tree_test.h (right):

https://codereview.chromium.org/672283003/diff/140001/cc/test/layer_tree_test...
cc/test/layer_tree_test.h:249:
SINGLE_THREAD_DIRECT_RENDERER_IMPL_TEST_F(TEST_FIXTURE_NAME);
On 2014/11/10 20:02:12, enne wrote:
> nit: no semicolon here to force callers to have a semicolon.

Done.

https://codereview.chromium.org/672283003/diff/140001/cc/test/layer_tree_test...
cc/test/layer_tree_test.h:295:
MULTI_THREAD_DIRECT_RENDERER_IMPL_TEST_F(TEST_FIXTURE_NAME);
On 2014/11/10 20:02:12, enne wrote:
> nit: same thing here

Done.

https://codereview.chromium.org/672283003/diff/140001/cc/test/layer_tree_test...
cc/test/layer_tree_test.h:312:
MULTI_THREAD_DELEGATING_RENDERER_IMPL_TEST_F(TEST_FIXTURE_NAME);
On 2014/11/10 20:02:12, enne wrote:
> nit: and here

Done.

https://codereview.chromium.org/672283003/diff/140001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/672283003/diff/140001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:129: class LayerTreeHostTestReadyToDraw :
public LayerTreeHostTest {
On 2014/11/10 20:02:12, enne wrote:
> Can you add some DCHECK that verifies that this is empty (and maybe name the
> test that way too?)

Done.

https://codereview.chromium.org/672283003/diff/140001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:162: class
LayerTreeHostTestReadyToDrawNonEmpty
On 2014/11/10 20:02:12, enne wrote:
> Can you add some DCHECK that verifies that this is non-empty?

Done.

[ernstm, 2014-11-11 01:37:35]

The CQ bit was checked by ernstm@chromium.org

[commit-bot: I haz the power, 2014-11-11 01:38:30]

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

[commit-bot: I haz the power, 2014-11-11 02:29:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-11 02:29:15]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ernstm, 2014-11-11 18:28:30]

The CQ bit was checked by ernstm@chromium.org

[commit-bot: I haz the power, 2014-11-11 18:29:29]

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

[commit-bot: I haz the power, 2014-11-11 20:18:14]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2014-11-11 20:19:04]

Patchset 10 (id:??) landed as
https://crrev.com/dfac03e1bd435b80b3c6a588ca8dd98954073185
Cr-Commit-Position: refs/heads/master@{#303706}