Android: Adjust tile-size using 'real' resolution in JB-MR1

Android: Adjust tile-size using 'real' resolution in JB-MR1

We can now get the real screen resolution which makes
choosing tile size easier. This also uses 384 for FHD
resolutions (uses less CPU on GPU/CC threads) and adjusts
the tile size some certain screen resolutions just barely
straddle an extra tile.

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

[epenner, 2013-12-20 04:25:56]

Ptal.

[no sievers, 2013-12-20 19:24:24]

https://codereview.chromium.org/119763002/diff/1/content/browser/gpu/gpu_data...
File content/browser/gpu/gpu_data_manager_impl_private.cc (right):

https://codereview.chromium.org/119763002/diff/1/content/browser/gpu/gpu_data...
content/browser/gpu/gpu_data_manager_impl_private.cc:254: int display_width =
info.GetRealDisplayWidth(real_size_supported);
I don't see these functions in ui/gfx/android/device_display_info.h, do they
depend on another patch?

https://codereview.chromium.org/119763002/diff/1/content/browser/gpu/gpu_data...
content/browser/gpu/gpu_data_manager_impl_private.cc:259: if
(real_size_supported) {
where does that ever get set to |true|?

https://codereview.chromium.org/119763002/diff/1/content/browser/gpu/gpu_data...
content/browser/gpu/gpu_data_manager_impl_private.cc:262: if (portrait_width >
768 || landscape_width > 1280)
nit: is it more readable to maybe put all of this incl. the logic in 267-273 in
a single if-else-if-else-if-... statement?

if (portrait_width > 1200 || landscape_width > 1920)
  default_tile_size = 512;
else if (portrait_width == 1200 && landscape_width > 1280)
  default_tile_size = 416;  
else if (portrait_width > 768 || landscape_width > 1280)
  default_tile_size = 384;
else if (portrait_width == 768)
  default_tile_size = 288;

I hope I got it right :)

[epennerAtGoogle, 2013-12-20 19:34:21]

https://codereview.chromium.org/119763002/diff/1/content/browser/gpu/gpu_data...
File content/browser/gpu/gpu_data_manager_impl_private.cc (right):

https://codereview.chromium.org/119763002/diff/1/content/browser/gpu/gpu_data...
content/browser/gpu/gpu_data_manager_impl_private.cc:254: int display_width =
info.GetRealDisplayWidth(real_size_supported);
> depend on another patch?

Yep:
https://codereview.chromium.org/93933016/

https://codereview.chromium.org/119763002/diff/1/content/browser/gpu/gpu_data...
content/browser/gpu/gpu_data_manager_impl_private.cc:259: if
(real_size_supported) {
> where does that ever get set to |true|?

It's an out-param by reference. I could use a pointer if that would make it more
clear (I don't recall the guideline for which is better).

https://codereview.chromium.org/119763002/diff/1/content/browser/gpu/gpu_data...
content/browser/gpu/gpu_data_manager_impl_private.cc:262: if (portrait_width >
768 || landscape_width > 1280)
I'm not sure about that one. That combines several bits of 'conceptual' logic
into one logical statement. I'd rather this be a bit longer with each adjustment
clearly explained.  For example, where would the comments go in your version?

[no sievers, 2013-12-20 19:43:33]

https://codereview.chromium.org/119763002/diff/1/content/browser/gpu/gpu_data...
File content/browser/gpu/gpu_data_manager_impl_private.cc (right):

https://codereview.chromium.org/119763002/diff/1/content/browser/gpu/gpu_data...
content/browser/gpu/gpu_data_manager_impl_private.cc:259: if
(real_size_supported) {
On 2013/12/20 19:34:22, epennerAtGoogle wrote:
> > where does that ever get set to |true|?
> 
> It's an out-param by reference. I could use a pointer if that would make it
more
> clear (I don't recall the guideline for which is better).

I think per style guide it's const-ref or pointer if modified.

https://codereview.chromium.org/119763002/diff/1/content/browser/gpu/gpu_data...
content/browser/gpu/gpu_data_manager_impl_private.cc:262: if (portrait_width >
768 || landscape_width > 1280)
On 2013/12/20 19:34:22, epennerAtGoogle wrote:
> I'm not sure about that one. That combines several bits of 'conceptual' logic
> into one logical statement. I'd rather this be a bit longer with each
adjustment
> clearly explained. 

I just find it a bit easier to read, because to follow your code we might
actually change the size twice, i.e. you would have to go back from the logic
further down to above to piece it together. But I don't feel strongly, if you
prefer it this way it's fine, you are the one tweaking the logic anyways :)

> For example, where would the comments go in your version?
You could just put it Into each branch.

[epenner, 2013-12-20 23:00:53]

https://codereview.chromium.org/119763002/diff/1/content/browser/gpu/gpu_data...
File content/browser/gpu/gpu_data_manager_impl_private.cc (right):

https://codereview.chromium.org/119763002/diff/1/content/browser/gpu/gpu_data...
content/browser/gpu/gpu_data_manager_impl_private.cc:259: if
(real_size_supported) {
On 2013/12/20 19:43:34, sievers wrote:
> On 2013/12/20 19:34:22, epennerAtGoogle wrote:
> > > where does that ever get set to |true|?
> > 
> > It's an out-param by reference. I could use a pointer if that would make it
> more
> > clear (I don't recall the guideline for which is better).
> 
> I think per style guide it's const-ref or pointer if modified.

Done.

https://codereview.chromium.org/119763002/diff/1/content/browser/gpu/gpu_data...
content/browser/gpu/gpu_data_manager_impl_private.cc:262: if (portrait_width >
768 || landscape_width > 1280)
I get your point and we do change it more than once. If you look below, we might
even change it 3 times on IMG! :P

I'll see if I can clean it up when I rebase on the real_size_supported change.
But currently I favor redundancy over concise logic here, since each step
corresponds to a conceptual reason why we are making these changes.

[no sievers, 2013-12-20 23:03:48]

On 2013/12/20 23:00:53, epenner wrote:
>
https://codereview.chromium.org/119763002/diff/1/content/browser/gpu/gpu_data...
> File content/browser/gpu/gpu_data_manager_impl_private.cc (right):
> 
>
https://codereview.chromium.org/119763002/diff/1/content/browser/gpu/gpu_data...
> content/browser/gpu/gpu_data_manager_impl_private.cc:259: if
> (real_size_supported) {
> On 2013/12/20 19:43:34, sievers wrote:
> > On 2013/12/20 19:34:22, epennerAtGoogle wrote:
> > > > where does that ever get set to |true|?
> > > 
> > > It's an out-param by reference. I could use a pointer if that would make
it
> > more
> > > clear (I don't recall the guideline for which is better).
> > 
> > I think per style guide it's const-ref or pointer if modified.
> 
> Done.
> 
>
https://codereview.chromium.org/119763002/diff/1/content/browser/gpu/gpu_data...
> content/browser/gpu/gpu_data_manager_impl_private.cc:262: if (portrait_width >
> 768 || landscape_width > 1280)
> I get your point and we do change it more than once. If you look below, we
might
> even change it 3 times on IMG! :P
> 
> I'll see if I can clean it up when I rebase on the real_size_supported change.
> But currently I favor redundancy over concise logic here, since each step
> corresponds to a conceptual reason why we are making these changes.

Sounds good. LGTM

[Zhenyao Mo, 2014-01-07 02:36:30]

LGTM

[commit-bot: I haz the power, 2014-01-10 00:07:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/epenner@chromium.org/119763002/130001

[commit-bot: I haz the power, 2014-01-10 02:24:56]

Change committed as 244048