cc: Low quality support for low res tiles

cc: Low quality support for low res tiles

This adds low quality for low res tiles. When a low res tile becomes
something else, then the reraster is scheduled.

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

[vmpstr, 2013-06-03 23:34:22]

PTAL.

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

https://codereview.chromium.org/15995033/diff/2001/cc/resources/tile_manager....
cc/resources/tile_manager.h:127: unsigned raster_flags;
This is here temporarily, until tasks become separate objects and we are no
longer limited by Bind argument number limit

[reveman, 2013-06-04 16:17:12]

I can't help to think that this would be cleaner if we moved the raster_task
(and picture_pile_analysis?) into TileVersion and added a
std::map<TileRasterFlags, TileVersion> to ManagedTileState instead of having one
ManagedTileState::tile_version.

https://codereview.chromium.org/15995033/diff/14/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/15995033/diff/14/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:476: unsigned required_raster_flags =
DetermineRasterFlags(tile);
What causes required_raster_flags to change and how is it triggering a call to
ManagedTiles?

https://codereview.chromium.org/15995033/diff/14/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:481: (current_raster_flags !=
required_raster_flags))) {
what if !mts.raster_task.is_null() but current_raster_flags !=
required_raster_flags. we want a new tile version in that case but can't
initiate one as there's already a pending raster task for this tile. when that
pending raster task completes, what will trigger this new task to be created?

https://codereview.chromium.org/15995033/diff/14/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:869: draw_filter = skia::AdoptRef(new
DisableLCDTextFilter);
How about low-quality when using lcd text? I don't think we should use a bit
mask unless we intend to support all combinations of it.

[vmpstr, 2013-06-04 16:58:58]

PTAL.

On 2013/06/04 16:17:12, David Reveman wrote:
> I can't help to think that this would be cleaner if we moved the raster_task
> (and picture_pile_analysis?) into TileVersion and added a
> std::map<TileRasterFlags, TileVersion> to ManagedTileState instead of having
one
> ManagedTileState::tile_version.

I kind of thing this is a bit of YANGI. I don't think we'll ever have more than
one new version we need to create at one time. That is we should never be
rastering more than one version at a time. If we do, we need to add some sort of
a metric like "this version is 'better' than this other version", since the
raster jobs can be completed in any order, and we should only keep the best
version. 

In any case, if we do decide to use some sort of a queue of versions, would you
be ok with it as a follow-up?

> 
> https://codereview.chromium.org/15995033/diff/14/cc/resources/tile_manager.cc
> File cc/resources/tile_manager.cc (right):
> 
>
https://codereview.chromium.org/15995033/diff/14/cc/resources/tile_manager.cc...
> cc/resources/tile_manager.cc:476: unsigned required_raster_flags =
> DetermineRasterFlags(tile);
> What causes required_raster_flags to change and how is it triggering a call to
> ManagedTiles?
> 
>
https://codereview.chromium.org/15995033/diff/14/cc/resources/tile_manager.cc...
> cc/resources/tile_manager.cc:481: (current_raster_flags !=
> required_raster_flags))) {
> what if !mts.raster_task.is_null() but current_raster_flags !=
> required_raster_flags. we want a new tile version in that case but can't
> initiate one as there's already a pending raster task for this tile. when that
> pending raster task completes, what will trigger this new task to be created?
> 
>
https://codereview.chromium.org/15995033/diff/14/cc/resources/tile_manager.cc...
> cc/resources/tile_manager.cc:869: draw_filter = skia::AdoptRef(new
> DisableLCDTextFilter);
> How about low-quality when using lcd text? I don't think we should use a bit
> mask unless we intend to support all combinations of it.

[vmpstr, 2013-06-04 16:59:08]

https://codereview.chromium.org/15995033/diff/14/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/15995033/diff/14/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:476: unsigned required_raster_flags =
DetermineRasterFlags(tile);
On 2013/06/04 16:17:12, David Reveman wrote:
> What causes required_raster_flags to change and how is it triggering a call to
> ManagedTiles?

the quality right now depends on the tile resolution, so if the resolution is
changed, then we call ManageTiles, right? I'm kind of basing it on this
assumption. Likewise with lcd text, it's currently immutable property on a
picture pile. When a tile is updated with a new picture, then we definitely
re-raster it.

https://codereview.chromium.org/15995033/diff/14/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:481: (current_raster_flags !=
required_raster_flags))) {
On 2013/06/04 16:17:12, David Reveman wrote:
> what if !mts.raster_task.is_null() but current_raster_flags !=
> required_raster_flags. we want a new tile version in that case but can't
> initiate one as there's already a pending raster task for this tile. when that
> pending raster task completes, what will trigger this new task to be created?

I've added a check in raster task completion.

https://codereview.chromium.org/15995033/diff/14/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:869: draw_filter = skia::AdoptRef(new
DisableLCDTextFilter);
On 2013/06/04 16:17:12, David Reveman wrote:
> How about low-quality when using lcd text? I don't think we should use a bit
> mask unless we intend to support all combinations of it.

Low quality filter disables lcd text as well. I prefer it as a bitmask, since
each of the bits can be determined independently. Wdyt?

[reveman, 2013-06-04 20:16:08]

https://codereview.chromium.org/15995033/diff/14/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/15995033/diff/14/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:476: unsigned required_raster_flags =
DetermineRasterFlags(tile);
On 2013/06/04 16:59:08, vmpstr wrote:
> On 2013/06/04 16:17:12, David Reveman wrote:
> > What causes required_raster_flags to change and how is it triggering a call
to
> > ManagedTiles?
> 
> the quality right now depends on the tile resolution, so if the resolution is
> changed, then we call ManageTiles, right? I'm kind of basing it on this
> assumption. Likewise with lcd text, it's currently immutable property on a
> picture pile. When a tile is updated with a new picture, then we definitely
> re-raster it.

How come TileRasterFlags is not just part of the TilePriority? Do you expect
some global state to affect this in the future?

https://codereview.chromium.org/15995033/diff/14/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:869: draw_filter = skia::AdoptRef(new
DisableLCDTextFilter);
On 2013/06/04 16:59:08, vmpstr wrote:
> On 2013/06/04 16:17:12, David Reveman wrote:
> > How about low-quality when using lcd text? I don't think we should use a bit
> > mask unless we intend to support all combinations of it.
> 
> Low quality filter disables lcd text as well. I prefer it as a bitmask, since
> each of the bits can be determined independently. Wdyt?

If they can be on/off independently but we don't support all combinations, then
you need to DCHECK that we don't end up with an invalid combination. The closer
to the source you can do that check the better and the less code we have where
invalid states can exist the better.

https://codereview.chromium.org/15995033/diff/10001/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/15995033/diff/10001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:409: unsigned
TileManager::DetermineRasterFlags(const Tile* tile) const {
does this need to be a member function? if not, consider moving it to anonymous
namespace.

https://codereview.chromium.org/15995033/diff/10001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:757: client_->ScheduleManageTiles();
how will this work with https://codereview.chromium.org/15855008/ ?

what if this task was canceled? and wouldn't we want this canceled and the task
with the correct raster flags to be scheduled asap instead of having to wait for
this to complete?

[vmpstr, 2013-06-04 22:26:04]

PTAL. The new version uses a map of raster mode -> tile version. The only
complicated bit here is the forced upload. It forces any first tile version that
it can (in most cases that should just be the one and only version that is being
processed).

https://codereview.chromium.org/15995033/diff/10001/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/15995033/diff/10001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:409: unsigned
TileManager::DetermineRasterFlags(const Tile* tile) const {
On 2013/06/04 20:16:08, David Reveman wrote:
> does this need to be a member function? if not, consider moving it to
anonymous
> namespace.

It's a member because it's using Tile's friend TileManager to get access to
managed state

[reveman, 2013-06-05 14:25:13]

https://codereview.chromium.org/15995033/diff/19001/cc/resources/managed_tile...
File cc/resources/managed_tile_state.h (right):

https://codereview.chromium.org/15995033/diff/19001/cc/resources/managed_tile...
cc/resources/managed_tile_state.h:125: TileVersion tile_version;
Why not just replace |tile_version| with:
typedef std::map<TileRasterMode, TileVersion> TileVersionMap;
TileVersionMap tile_versions;
or maybe:
TileVersion tile_versions[NUM_RASTER_MODES];

The tile manager code would essentially be the same as before expect anywhere
you access |tile_version| you would do tile_versions[raster_mode] instead.

"TileVersion tile_versions[NUM_RASTER_MODES]" is probably much better than using
std::map. The advantage of the map would be that you could save some memory when
we have a lot of different tile versions but that's not the case right now and
the simple array is likely much more efficient.

https://codereview.chromium.org/15995033/diff/19001/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/15995033/diff/19001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:497: TileRasterMode required_raster_mode =
DetermineRasterMode(tile);
What if you compute new raster mode as the best of the old and the currently
desired mode? We add ManagedTileState::raster_mode which is updated as a
separate task of ManageTiles. Ideally in a DetermineRasterModeForTiles()
function.

Existing tile_version related code here and in ScheduleTasks would be the same
as before but s/tile_version/tile_versions[mts.raster_mode]/.

Wdyt?

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

https://codereview.chromium.org/15995033/diff/19001/cc/resources/tile_manager...
cc/resources/tile_manager.h:30: INVALID_RASTER_MODE = 0,
Do we need this enum?

[vmpstr, 2013-06-05 16:33:34]

PTAL. I addressed most comments in the new patch.

[reveman, 2013-06-05 18:01:32]

I think this is starting to look very good.

https://codereview.chromium.org/15995033/diff/29001/cc/resources/managed_tile...
File cc/resources/managed_tile_state.h (right):

https://codereview.chromium.org/15995033/diff/29001/cc/resources/managed_tile...
cc/resources/managed_tile_state.h:122: PicturePileImpl::Analysis
picture_pile_analysis;
no need to do anything about it in this patch but do we still need
|picture_pile_analyzed| and |picture_pile_analysis|?

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

https://codereview.chromium.org/15995033/diff/29001/cc/resources/tile.h#newco...
cc/resources/tile.h:65: const ManagedTileState::TileVersion& tile_version()
const {
tile_version() magically returning the tile version that is preferred for
drawing is a bit weird.

How about we add:
bool Tile::IsReadyToDraw() and
TileRasterMode Tile::GetRasterModeThatIsReadyToDraw();
or just:
bool Tile::IsReadyToDraw(TileRasterMode* ready_to_draw_mode);

and make these tile_version() functions take a TileRasterMode as parameter and
just return tile_versions[mode]?

this way you also make sure there's no old tile_version() call sites you forgot
that need to be adjusted.

https://codereview.chromium.org/15995033/diff/29001/cc/resources/tile.h#newco...
cc/resources/tile.h:107: if
(!managed_state().tile_versions[mode].raster_task_.is_null())
can you remove this and always call Reset instead?

https://codereview.chromium.org/15995033/diff/29001/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (left):

https://codereview.chromium.org/15995033/diff/29001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:315: // Setting |forced_upload_| to true makes this
tile ready to draw.
lets keep this comment. just s/this tile/this tile version/

https://codereview.chromium.org/15995033/diff/29001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:693: DCHECK(!mts.raster_task.is_null());
please keep this DCHECK.

https://codereview.chromium.org/15995033/diff/29001/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/15995033/diff/29001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:467: mts.raster_mode = DetermineRasterMode(tile);
Please DCHECK that new raster_mode is not worse than last as that would cause
unnecessary raster.

https://codereview.chromium.org/15995033/diff/29001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:493: }
nit: {} not required

https://codereview.chromium.org/15995033/diff/29001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:522: if (!tile_version.resource_ &&
should the "!tile_version.resource_" be here? seems like IsReadyToDraw should be
enough.

https://codereview.chromium.org/15995033/diff/29001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:537: mts.tile_versions[mts.raster_mode];
Do you need a TileRasterMode loop here to free all resources rather than just
tile_versions[mts.raster_mode]?

https://codereview.chromium.org/15995033/diff/29001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:759: tile_version.resource_ = resource.Pass();
can you do this below the was_canceled check as before? I think it's cleaner to
not temporarily set resource_ when canceled or solid.

https://codereview.chromium.org/15995033/diff/29001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:760: tile_version.forced_upload_ = false;
this is new. do we need it?

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

https://codereview.chromium.org/15995033/diff/29001/cc/resources/tile_manager...
cc/resources/tile_manager.h:32: HIGH_QUALITY_RASTER_MODE = 2,
Maybe reverse the order of these so you can iterate from 0 to find best possible
tile version. Might also be good to add a comment here about them being in order
of what is preferred to draw.

https://codereview.chromium.org/15995033/diff/29001/cc/resources/tile_manager...
cc/resources/tile_manager.h:33: NUM_RASTER_MODES
"NUM_RASTER_MODES = 3" to be consistent with TileManagerBin below.

[vmpstr, 2013-06-06 01:54:20]

PTAL.

https://codereview.chromium.org/15995033/diff/29001/cc/resources/managed_tile...
File cc/resources/managed_tile_state.h (right):

https://codereview.chromium.org/15995033/diff/29001/cc/resources/managed_tile...
cc/resources/managed_tile_state.h:122: PicturePileImpl::Analysis
picture_pile_analysis;
On 2013/06/05 18:01:32, David Reveman wrote:
> no need to do anything about it in this patch but do we still need
> |picture_pile_analyzed| and |picture_pile_analysis|?

We might have to add "has_text" to the mts or tile version, but yeah I think we
can clean this up as a follow-up

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

https://codereview.chromium.org/15995033/diff/29001/cc/resources/tile.h#newco...
cc/resources/tile.h:65: const ManagedTileState::TileVersion& tile_version()
const {
On 2013/06/05 18:01:32, David Reveman wrote:
> tile_version() magically returning the tile version that is preferred for
> drawing is a bit weird.
> 
> How about we add:
> bool Tile::IsReadyToDraw() and
> TileRasterMode Tile::GetRasterModeThatIsReadyToDraw();
> or just:
> bool Tile::IsReadyToDraw(TileRasterMode* ready_to_draw_mode);
> 
> and make these tile_version() functions take a TileRasterMode as parameter and
> just return tile_versions[mode]?
> 
> this way you also make sure there's no old tile_version() call sites you
forgot
> that need to be adjusted.

Done.

https://codereview.chromium.org/15995033/diff/29001/cc/resources/tile.h#newco...
cc/resources/tile.h:107: if
(!managed_state().tile_versions[mode].raster_task_.is_null())
On 2013/06/05 18:01:32, David Reveman wrote:
> can you remove this and always call Reset instead?

Done.

https://codereview.chromium.org/15995033/diff/29001/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (left):

https://codereview.chromium.org/15995033/diff/29001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:315: // Setting |forced_upload_| to true makes this
tile ready to draw.
On 2013/06/05 18:01:32, David Reveman wrote:
> lets keep this comment. just s/this tile/this tile version/

Done.

https://codereview.chromium.org/15995033/diff/29001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:693: DCHECK(!mts.raster_task.is_null());
On 2013/06/05 18:01:32, David Reveman wrote:
> please keep this DCHECK.

Done.

https://codereview.chromium.org/15995033/diff/29001/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/15995033/diff/29001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:467: mts.raster_mode = DetermineRasterMode(tile);
On 2013/06/05 18:01:32, David Reveman wrote:
> Please DCHECK that new raster_mode is not worse than last as that would cause
> unnecessary raster. 

Turns out that when you switch tabs, the inactive one becomes a worse raster
mode. I've added a check for that instead, so we always keep the better version
if we have it.

https://codereview.chromium.org/15995033/diff/29001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:493: }
On 2013/06/05 18:01:32, David Reveman wrote:
> nit: {} not required

Done.

https://codereview.chromium.org/15995033/diff/29001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:522: if (!tile_version.resource_ &&
On 2013/06/05 18:01:32, David Reveman wrote:
> should the "!tile_version.resource_" be here? seems like IsReadyToDraw should
be
> enough.

Done.

https://codereview.chromium.org/15995033/diff/29001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:537: mts.tile_versions[mts.raster_mode];
On 2013/06/05 18:01:32, David Reveman wrote:
> Do you need a TileRasterMode loop here to free all resources rather than just
> tile_versions[mts.raster_mode]?

Done.

https://codereview.chromium.org/15995033/diff/29001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:759: tile_version.resource_ = resource.Pass();
On 2013/06/05 18:01:32, David Reveman wrote:
> can you do this below the was_canceled check as before? I think it's cleaner
to
> not temporarily set resource_ when canceled or solid.

Done.

https://codereview.chromium.org/15995033/diff/29001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:760: tile_version.forced_upload_ = false;
On 2013/06/05 18:01:32, David Reveman wrote:
> this is new. do we need it?

I don't really see where the flag is reset back in here... If anything, this
just makes sure that the state is consistent since we know that at that point
the resource isn't forced. Right?

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

https://codereview.chromium.org/15995033/diff/29001/cc/resources/tile_manager...
cc/resources/tile_manager.h:32: HIGH_QUALITY_RASTER_MODE = 2,
On 2013/06/05 18:01:32, David Reveman wrote:
> Maybe reverse the order of these so you can iterate from 0 to find best
possible
> tile version. Might also be good to add a comment here about them being in
order
> of what is preferred to draw.

Done.

https://codereview.chromium.org/15995033/diff/29001/cc/resources/tile_manager...
cc/resources/tile_manager.h:33: NUM_RASTER_MODES
On 2013/06/05 18:01:32, David Reveman wrote:
> "NUM_RASTER_MODES = 3" to be consistent with TileManagerBin below.

Done.

[vmpstr, 2013-06-06 02:08:35]

https://codereview.chromium.org/15995033/diff/39001/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/15995033/diff/39001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:479: if (tile->IsReadyToDraw(&ready_mode) &&
(ready_mode > desired_mode))
That should read ready_mode < desired_mode. I'll fix that in the morning

[reveman, 2013-06-06 06:02:02]

https://codereview.chromium.org/15995033/diff/29001/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/15995033/diff/29001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:760: tile_version.forced_upload_ = false;
On 2013/06/06 01:54:20, vmpstr wrote:
> On 2013/06/05 18:01:32, David Reveman wrote:
> > this is new. do we need it?
> 
> I don't really see where the flag is reset back in here... If anything, this
> just makes sure that the state is consistent since we know that at that point
> the resource isn't forced. Right?

Good call. This might be causing some of the rendering issues we're currently
seeing so I moved it to a separate patch that I'd like to land first.

https://codereview.chromium.org/15995033/diff/39001/cc/layers/picture_layer_i...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/15995033/diff/39001/cc/layers/picture_layer_i...
cc/layers/picture_layer_impl.cc:233: iter->IsReadyToDraw(&raster_mode);
can we consolidate this with the IsReadyToDraw call above?

https://codereview.chromium.org/15995033/diff/39001/cc/resources/managed_tile...
File cc/resources/managed_tile_state.cc (right):

https://codereview.chromium.org/15995033/diff/39001/cc/resources/managed_tile...
cc/resources/managed_tile_state.cc:14: :
raster_mode(static_cast<TileRasterMode>(0)),
raster_mode(LOW_QUALITY_RASTER_MODE) ?

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

https://codereview.chromium.org/15995033/diff/39001/cc/resources/tile.h#newco...
cc/resources/tile.h:112: }
nit: no need for {}

https://codereview.chromium.org/15995033/diff/39001/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (left):

https://codereview.chromium.org/15995033/diff/39001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:565: DCHECK(!tile->tile_version().resource_);
Why has this DCHECK been removed?

https://codereview.chromium.org/15995033/diff/39001/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/15995033/diff/39001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:366: if
(!mts.tile_versions[mode].requires_resource())
use one if statement with && instead

https://codereview.chromium.org/15995033/diff/39001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:479: if (tile->IsReadyToDraw(&ready_mode) &&
(ready_mode > desired_mode))
On 2013/06/06 02:08:35, vmpstr wrote:
> That should read ready_mode < desired_mode. I'll fix that in the morning

why IsReadyToDraw? If you prevent new tasks from being dispatched when one is
pending you need to make sure these tasks are dispatched later, which I think is
messy.

Doesn't this get exactly the result we want:
mts.raster_mode = min(mts.raster_mode, DetermineRasterMode(tile));

which is to cancel of any undesired tasks and immediately dispatch a raster task
for the desired tile version.

https://codereview.chromium.org/15995033/diff/39001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:562: if (has_memory) {
is this conditional necessary?

https://codereview.chromium.org/15995033/diff/39001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:617: }
maybe move the inner part of this loop to a FreeResourceForTile(Tile* tile,
TileRasterMode mode) function.

[vmpstr, 2013-06-06 15:20:21]

PTAL.

https://codereview.chromium.org/15995033/diff/39001/cc/layers/picture_layer_i...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/15995033/diff/39001/cc/layers/picture_layer_i...
cc/layers/picture_layer_impl.cc:233: iter->IsReadyToDraw(&raster_mode);
On 2013/06/06 06:02:02, David Reveman wrote:
> can we consolidate this with the IsReadyToDraw call above?

Done.

https://codereview.chromium.org/15995033/diff/39001/cc/resources/managed_tile...
File cc/resources/managed_tile_state.cc (right):

https://codereview.chromium.org/15995033/diff/39001/cc/resources/managed_tile...
cc/resources/managed_tile_state.cc:14: :
raster_mode(static_cast<TileRasterMode>(0)),
On 2013/06/06 06:02:02, David Reveman wrote:
> raster_mode(LOW_QUALITY_RASTER_MODE) ?

Done.

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

https://codereview.chromium.org/15995033/diff/39001/cc/resources/tile.h#newco...
cc/resources/tile.h:112: }
On 2013/06/06 06:02:02, David Reveman wrote:
> nit: no need for {}

Done.

https://codereview.chromium.org/15995033/diff/39001/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (left):

https://codereview.chromium.org/15995033/diff/39001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:565: DCHECK(!tile->tile_version().resource_);
On 2013/06/06 06:02:02, David Reveman wrote:
> Why has this DCHECK been removed?

Whoops. Added it back.

https://codereview.chromium.org/15995033/diff/39001/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/15995033/diff/39001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:366: if
(!mts.tile_versions[mode].requires_resource())
On 2013/06/06 06:02:02, David Reveman wrote:
> use one if statement with && instead

Done.

https://codereview.chromium.org/15995033/diff/39001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:479: if (tile->IsReadyToDraw(&ready_mode) &&
(ready_mode > desired_mode))
On 2013/06/06 06:02:02, David Reveman wrote:
> On 2013/06/06 02:08:35, vmpstr wrote:
> > That should read ready_mode < desired_mode. I'll fix that in the morning
> 
> why IsReadyToDraw? If you prevent new tasks from being dispatched when one is
> pending you need to make sure these tasks are dispatched later, which I think
is
> messy.
> 
> Doesn't this get exactly the result we want:
> mts.raster_mode = min(mts.raster_mode, DetermineRasterMode(tile));
> 
> which is to cancel of any undesired tasks and immediately dispatch a raster
task
> for the desired tile version. 

Good point, changed this to min

https://codereview.chromium.org/15995033/diff/39001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:562: if (has_memory) {
On 2013/06/06 06:02:02, David Reveman wrote:
> is this conditional necessary?

I changed this a bit. I prefer to still keep this type of conditional, since I
want to keep the behavior the same as before. I want to take a closer look at
this part of the code in a follow-up.

https://codereview.chromium.org/15995033/diff/39001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:617: }
On 2013/06/06 06:02:02, David Reveman wrote:
> maybe move the inner part of this loop to a FreeResourceForTile(Tile* tile,
> TileRasterMode mode) function.

Done.

[reveman, 2013-06-06 15:40:13]

I like it. lgtm.

https://codereview.chromium.org/15995033/diff/44001/cc/resources/managed_tile...
File cc/resources/managed_tile_state.cc (right):

https://codereview.chromium.org/15995033/diff/44001/cc/resources/managed_tile...
cc/resources/managed_tile_state.cc:63:
tile_versions[raster_mode].resource_.get() != 0);
nit: we should probably check all tile versions here but I guess it depends on
how you interpret "has_resource"

https://codereview.chromium.org/15995033/diff/44001/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/15995033/diff/44001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:780:
DCHECK(!mts.tile_versions[raster_mode].raster_task_.is_null());
nit: DCHECK(!tile_version.raster_task_.is_null()) instead

https://codereview.chromium.org/15995033/diff/44001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:782: tile_version.forced_upload_ = false;
I prefer if we land this fix separately. I created this cl for that:
https://codereview.chromium.org/16439015/

[vmpstr, 2013-06-06 18:20:00]

+jamesr for content/renderer OWNER's review. Please take a look.

[jamesr, 2013-06-06 18:25:37]

lgtm

[commit-bot: I haz the power, 2013-06-06 23:02:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vmpstr@chromium.org/15995033/54001

[commit-bot: I haz the power, 2013-06-06 23:56:49]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-06-07 00:38:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vmpstr@chromium.org/15995033/73001

[commit-bot: I haz the power, 2013-06-07 06:41:40]

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

[commit-bot: I haz the power, 2013-06-07 06:48:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vmpstr@chromium.org/15995033/73001

[commit-bot: I haz the power, 2013-06-07 08:11:52]

Change committed as 204747

[Tom Hudson, 2013-06-07 12:14:41]

There was discussion on IRC of reverting this because it's hitting
warning-as-error in FreeUnusedResourcesForTile() on a ChromeOS build: if
Tile::IsReadyToDraw() returns false, used_mode is uninitialized. It looks like
it's an overzealous warning, but will probably need to be fixed up anyway.

[tyoshino (SeeGerritForStatus), 2013-06-07 12:51:47]

On 2013/06/07 12:14:41, Tom Hudson wrote:
> There was discussion on IRC of reverting this because it's hitting
> warning-as-error in FreeUnusedResourcesForTile() on a ChromeOS build: if
> Tile::IsReadyToDraw() returns false, used_mode is uninitialized. It looks like
> it's an overzealous warning, but will probably need to be fixed up anyway.

Fixed. Please review https://codereview.chromium.org/16268021/