cc: Moved contents_swizzled out of TileManager

cc: Moved contents_swizzled out of TileManager

Drawing info now has everything it needs to calculate
the contents_swizzled instead of it being set in
TileManager in two spots.

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

[vmpstr, 2013-04-04 22:30:00]

Please take a look.

https://codereview.chromium.org/13665003/diff/1/cc/resources/managed_tile_sta...
File cc/resources/managed_tile_state.h (right):

https://codereview.chromium.org/13665003/diff/1/cc/resources/managed_tile_sta...
cc/resources/managed_tile_state.h:55: return
!PlatformColor::SameComponentOrder(resource_format_);
Should we change PlatformColor functions to be hacker_style? They seem to be
fairly small functions all defined in the header.

[enne (OOO), 2013-04-04 22:37:28]

https://codereview.chromium.org/13665003/diff/1/cc/resources/managed_tile_sta...
File cc/resources/managed_tile_state.h (right):

https://codereview.chromium.org/13665003/diff/1/cc/resources/managed_tile_sta...
cc/resources/managed_tile_state.h:55: return
!PlatformColor::SameComponentOrder(resource_format_);
On 2013/04/04 22:30:00, vmpstr wrote:
> Should we change PlatformColor functions to be hacker_style? They seem to be
> fairly small functions all defined in the header.

I'm pretty indifferent on this.  hacker style is optional.

https://codereview.chromium.org/13665003/diff/1/cc/resources/managed_tile_sta...
cc/resources/managed_tile_state.h:98: GLenum resource_format_;
Is this different than resource_->format()?

[vmpstr, 2013-04-04 22:46:35]

On 2013/04/04 22:37:28, enne wrote:
>
https://codereview.chromium.org/13665003/diff/1/cc/resources/managed_tile_sta...
> File cc/resources/managed_tile_state.h (right):
> 
>
https://codereview.chromium.org/13665003/diff/1/cc/resources/managed_tile_sta...
> cc/resources/managed_tile_state.h:55: return
> !PlatformColor::SameComponentOrder(resource_format_);
> On 2013/04/04 22:30:00, vmpstr wrote:
> > Should we change PlatformColor functions to be hacker_style? They seem to be
> > fairly small functions all defined in the header.
> 
> I'm pretty indifferent on this.  hacker style is optional.
> 
>
https://codereview.chromium.org/13665003/diff/1/cc/resources/managed_tile_sta...
> cc/resources/managed_tile_state.h:98: GLenum resource_format_;
> Is this different than resource_->format()?

It's the same, but resource_ is initialized in TileManager using
resource_format_. That is, resource_format_ comes first, then it is used to
initialize resource_ if needed

[enne (OOO), 2013-04-04 23:14:49]

On 2013/04/04 22:46:35, vmpstr wrote:
> On 2013/04/04 22:37:28, enne wrote:
>
https://codereview.chromium.org/13665003/diff/1/cc/resources/managed_tile_sta...
> > cc/resources/managed_tile_state.h:98: GLenum resource_format_;
> > Is this different than resource_->format()?
> 
> It's the same, but resource_ is initialized in TileManager using
> resource_format_. That is, resource_format_ comes first, then it is used to
> initialize resource_ if needed

Sure, but you also assign unconditionally assign it to be GL_RGBA in
managed_tile_state.cc, so I'm not sure what value storing it separately gets
you?  You could just move that constant to tile manager and you wouldn't need to
store it.  Is it ever not that value, or am I missing some code?

[vmpstr, 2013-04-05 16:53:21]

> Sure, but you also assign unconditionally assign it to be GL_RGBA in
> managed_tile_state.cc, so I'm not sure what value storing it separately gets
> you?  You could just move that constant to tile manager and you wouldn't need
to
> store it.  Is it ever not that value, or am I missing some code?

That's a good point. I've removed more references to the format, so it should
now be localized to DrawingInfo. I can't remove it all together and put a
constant in tile_manager, because of the rasterize_on_demand mode. In that mode,
a resource is never created, but we still check if the contents are swizzled.

[enne (OOO), 2013-04-05 17:12:15]

lgtm

[commit-bot: I haz the power, 2013-04-05 17:18:21]

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

[commit-bot: I haz the power, 2013-04-05 18:43:30]

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

[commit-bot: I haz the power, 2013-04-06 00:45:31]

Change committed as 192675