Make disable_low_res_tiling independent of is_gpu_rasterization_enable

Make disable_low_res_tiling independent of is_gpu_rasterization_enable

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

[piman, 2014-03-27 18:59:33]

lgtm

[enne (OOO), 2014-03-27 19:08:05]

lgtm

[vangelis, 2014-03-27 19:16:45]

One comment, otherwise the logic looks correct. 

lgtm

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

https://codereview.chromium.org/214593006/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl.h:127: return should_use_low_res_tiling_ &&
!ShouldUseGpuRasterization();
Bikeshedding a bit but since should_use_low_res_tiling_ is expected to have the
same value as the corresponding tree_impl setting (create_low_res_tiling),
shouldn't these two match for consistency?  Possibly rename it to
should_create_low_res_tiling ?

[Anton, 2014-03-28 10:51:15]

On 2014/03/27 19:16:45, vangelis wrote:
> One comment, otherwise the logic looks correct. 
> 
> lgtm
> 
>
https://codereview.chromium.org/214593006/diff/1/cc/layers/picture_layer_impl.h
> File cc/layers/picture_layer_impl.h (right):
> 
>
https://codereview.chromium.org/214593006/diff/1/cc/layers/picture_layer_impl...
> cc/layers/picture_layer_impl.h:127: return should_use_low_res_tiling_ &&
> !ShouldUseGpuRasterization();
> Bikeshedding a bit but since should_use_low_res_tiling_ is expected to have
the
> same value as the corresponding tree_impl setting (create_low_res_tiling),
> shouldn't these two match for consistency?  Possibly rename it to
> should_create_low_res_tiling ?

Yeah, bikeshedding, I should ignore this, so I will.

[Anton, 2014-03-28 10:52:47]

The CQ bit was checked by anton@chromium.org

[commit-bot: I haz the power, 2014-03-28 10:53:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/anton@chromium.org/214593006/1

[commit-bot: I haz the power, 2014-03-28 12:35:15]

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

[commit-bot: I haz the power, 2014-03-28 12:35:15]

Try jobs failed on following builders:
  tryserver.chromium on mac_chromium_rel

[Anton, 2014-03-28 14:29:06]

The CQ bit was checked by anton@chromium.org

[commit-bot: I haz the power, 2014-03-28 14:30:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/anton@chromium.org/214593006/1

[Vangelis Kokkevis, 2014-03-28 16:25:06]

On 2014/03/28 10:51:15, Anton wrote:
> On 2014/03/27 19:16:45, vangelis wrote:
> > One comment, otherwise the logic looks correct. 
> > 
> > lgtm
> > 
> >
>
https://codereview.chromium.org/214593006/diff/1/cc/layers/picture_layer_impl.h
> > File cc/layers/picture_layer_impl.h (right):
> > 
> >
>
https://codereview.chromium.org/214593006/diff/1/cc/layers/picture_layer_impl...
> > cc/layers/picture_layer_impl.h:127: return should_use_low_res_tiling_ &&
> > !ShouldUseGpuRasterization();
> > Bikeshedding a bit but since should_use_low_res_tiling_ is expected to have
> the
> > same value as the corresponding tree_impl setting (create_low_res_tiling),
> > shouldn't these two match for consistency?  Possibly rename it to
> > should_create_low_res_tiling ?
> 
> Yeah, bikeshedding, I should ignore this, so I will.

Glad to see you value consistency in the code.

[Anton, 2014-03-28 16:30:18]

"should" is dropped from the other flags in LayerTreeSettings so adding it
would make the code less consistent, not more.


On Fri, Mar 28, 2014 at 4:25 PM, <vangelis@chromium.org> wrote:

> On 2014/03/28 10:51:15, Anton wrote:
>
>> On 2014/03/27 19:16:45, vangelis wrote:
>> > One comment, otherwise the logic looks correct.
>> >
>> > lgtm
>> >
>> >
>>
>
> https://codereview.chromium.org/214593006/diff/1/cc/
> layers/picture_layer_impl.h
>
>> > File cc/layers/picture_layer_impl.h (right):
>> >
>> >
>>
>
> https://codereview.chromium.org/214593006/diff/1/cc/
> layers/picture_layer_impl.h#newcode127
>
>> > cc/layers/picture_layer_impl.h:127: return should_use_low_res_tiling_
>> &&
>> > !ShouldUseGpuRasterization();
>> > Bikeshedding a bit but since should_use_low_res_tiling_ is expected to
>> have
>> the
>> > same value as the corresponding tree_impl setting
>> (create_low_res_tiling),
>> > shouldn't these two match for consistency?  Possibly rename it to
>> > should_create_low_res_tiling ?
>>
>
>  Yeah, bikeshedding, I should ignore this, so I will.
>>
>
> Glad to see you value consistency in the code.
>
> https://codereview.chromium.org/214593006/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2014-03-28 16:35:26]

Change committed as 260160

[Vangelis Kokkevis, 2014-03-28 16:39:01]

On 2014/03/28 16:30:18, Anton wrote:
> "should" is dropped from the other flags in LayerTreeSettings so adding it
> would make the code less consistent, not more.
> 
> 

In case I wasn't clear as I talking about the difference between "use" and
"create" (use_low_res_tiling vs create_low_res_tiling) in the variable names.
Unless you see a semantic difference that needs to be highlighted, switching
from one to the other makes the code harder to follow.

[Anton, 2014-03-28 16:40:08]

"create" was added for another reviewer in earlier review.


On Fri, Mar 28, 2014 at 4:39 PM, <vangelis@chromium.org> wrote:

> On 2014/03/28 16:30:18, Anton wrote:
>
>> "should" is dropped from the other flags in LayerTreeSettings so adding it
>> would make the code less consistent, not more.
>>
>
>
>
> In case I wasn't clear as I talking about the difference between "use" and
> "create" (use_low_res_tiling vs create_low_res_tiling) in the variable
> names.
> Unless you see a semantic difference that needs to be highlighted,
> switching
> from one to the other makes the code harder to follow.
>
> https://codereview.chromium.org/214593006/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.