cc: Make tiling interest rect calc based on viewport.

cc: Make tiling interest rect calc based on viewport.

Current interest/eventually/live rect is calculated based
on  max_tiles_for_interest_area setting which is not the
best choice for determining the size. As part of this patch
tiling interest rect is calculated based on viewport area.
Which is multiplied by a constant factor. The factor is
currently 9, viewport dist in all directions.
Also, max_tiles_for_interest_area setting is removed
and its associated switch and entries in chrome://flags.

BUG=483520
Committed: https://crrev.com/6d84db54a27c036d77b0e4a26d42c158ee4a6add
Cr-Commit-Position: refs/heads/master@{#329870}

[sohanjg, 2015-05-05 12:54:46]

sohan.jyoti@samsung.com changed reviewers:
+ danakj@chromium.org, enne@chromium.org, vmpstr@chromium.org

[sohanjg, 2015-05-05 12:59:20]

Can you please take a look.
Its rough, just to have your thoughts, whether this is what we are looking for.

Tests needs to be looked at, and unused variables needs to be removed.

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

https://codereview.chromium.org/1126793002/diff/1/cc/layers/picture_layer_imp...
cc/layers/picture_layer_impl.cc:1159: this, 4 *
layer_tree_impl()->device_viewport_size().GetArea(),
instead of sending viewport area/tile area and then multiplying the same tile
area in PLT::ComputeTilePriorityRects, i am just sending the viewport area in
all 4 directions. will that be ok ?

actually we dont have tile size info here anyways till we create tiling and
decide the content bounds.

[vmpstr, 2015-05-05 20:48:11]

I think you're changing the right code (ie, that's kind of exactly what we
want). However, I think we need a bit more controlled way of specifying the
area.

We need at least a layer_tree_setting for something that we can change. It would
also be nice if it was smart enough... like if we're in the middle of a layer
that expands in all directions, then we can do 9 * viewport area (3 x 3), but in
the layer only expands up and down, then maybe 3 * viewport area is enough (1 x
3).

However, I'm not sure how to accomplish this and keep the code simple at the
same time. 

We also want to verify that telemetry and perf is more or less the same with
this change.

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

https://codereview.chromium.org/1126793002/diff/1/cc/layers/picture_layer_imp...
cc/layers/picture_layer_impl.cc:1159: this, 4 *
layer_tree_impl()->device_viewport_size().GetArea(),
On 2015/05/05 12:59:20, sohanjg wrote:
> instead of sending viewport area/tile area and then multiplying the same tile
> area in PLT::ComputeTilePriorityRects, i am just sending the viewport area in
> all 4 directions. will that be ok ?
> 
> actually we dont have tile size info here anyways till we create tiling and
> decide the content bounds.

Having a viewport distance in all directions would be 9 * viewport area, right?
(three viewports across three viewports down). Although I think in majority of
cases, there's no wiggle room for left and right. In either case, I think we
should send down here a multiplier (tiling_interest_area_viewport_multiplier or
something), since the viewport size can change without us creating a new tiling
set.

That is, we should use the multiplier in conjunction with the visible rect (that
we get in update tile priorities / compute tile priority rects), and figure out
what the new live tiles rect should be.

[sohanjg, 2015-05-06 15:01:59]

Just to summarize,
1. Removed the max_tiles_for_interest_area and added
tiling_interest_area_viewport_multiplier a new setting in layer tree, and use it
via PLI->PLTS->PLT in ComputeTilePriorityRects, to calc interest rect.

2. Resource, IDS changes for changing chrome://flags description.

3. Unit/perftest changes has been majorly to make sure interest rect is same as
before, we can have a look at them again.

Can you please take a look. thanks.

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

https://codereview.chromium.org/1126793002/diff/1/cc/layers/picture_layer_imp...
cc/layers/picture_layer_impl.cc:1159: this, 4 *
layer_tree_impl()->device_viewport_size().GetArea(),
On 2015/05/05 20:48:11, vmpstr wrote:
> On 2015/05/05 12:59:20, sohanjg wrote:
> > instead of sending viewport area/tile area and then multiplying the same
tile
> > area in PLT::ComputeTilePriorityRects, i am just sending the viewport area
in
> > all 4 directions. will that be ok ?
> > 
> > actually we dont have tile size info here anyways till we create tiling and
> > decide the content bounds.
> 
> Having a viewport distance in all directions would be 9 * viewport area,
right?
> (three viewports across three viewports down). Although I think in majority of
> cases, there's no wiggle room for left and right. In either case, I think we
> should send down here a multiplier (tiling_interest_area_viewport_multiplier
or
> something), since the viewport size can change without us creating a new
tiling
> set.
> 
> That is, we should use the multiplier in conjunction with the visible rect
(that
> we get in update tile priorities / compute tile priority rects), and figure
out
> what the new live tiles rect should be.

Yeah right (* 9) , i somehow forgot its an area.
Maybe we can check for mobile viewport meta tag and reduce the multiplier (* 3)
in future, i'll leave a todo, if thats ok?

[vmpstr, 2015-05-06 17:57:29]

I'm ok with just keeping it at 9 if the perf shows that this is acceptable. Can
you run some telemetry tests to see what the results look like? I would
recommend thread_times.tough_compositor_cases and
smoothess.tough_scrolling_cases

https://codereview.chromium.org/1126793002/diff/20001/cc/base/switches.h
File cc/base/switches.h (right):

https://codereview.chromium.org/1126793002/diff/20001/cc/base/switches.h#newc...
cc/base/switches.h:28: CC_EXPORT extern const char
kTilingInterestAreaViewportMultiplier[];
I would say we remove this setting altogether, and add it back if necessary.

https://codereview.chromium.org/1126793002/diff/20001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/1126793002/diff/20001/cc/layers/picture_layer...
cc/layers/picture_layer_impl_unittest.cc:2558: // High res tiling should have 36
tiles (4x9 tile grid).
Can you verify manually that it is actually 4x9? I buy that there's less tiles
due to a smaller interest area, but I'm not sure I buy 4x9...

https://codereview.chromium.org/1126793002/diff/20001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling.cc (left):

https://codereview.chromium.org/1126793002/diff/20001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:594: gfx::Size tile_size =
tiling_data_.max_texture_size();
Unused?

https://codereview.chromium.org/1126793002/diff/20001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling.h (right):

https://codereview.chromium.org/1126793002/diff/20001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.h:311: const size_t
tiling_interest_area_viewport_multiplier_;
Let's make the multiplier be a float to allow for fractional multipliers

https://codereview.chromium.org/1126793002/diff/20001/cc/resources/tile_manag...
File cc/resources/tile_manager_unittest.cc (right):

https://codereview.chromium.org/1126793002/diff/20001/cc/resources/tile_manag...
cc/resources/tile_manager_unittest.cc:779: EXPECT_EQ(8, distance_decreasing);
I'd prefer if these numbers stayed the same, can you adjust the multiplier so
that the same amount of tiles is created instead?

https://codereview.chromium.org/1126793002/diff/20001/cc/resources/tile_manag...
cc/resources/tile_manager_unittest.cc:819: EXPECT_EQ(8, distance_decreasing);
Same here.

https://codereview.chromium.org/1126793002/diff/20001/cc/resources/tile_manag...
cc/resources/tile_manager_unittest.cc:1179:
settings.tiling_interest_area_viewport_multiplier = 36;
These numbers seem pretty random, why 36? Before it was 10000 to say
"everything" basically, but 36 seems awfully specific.

https://codereview.chromium.org/1126793002/diff/20001/cc/trees/layer_tree_set...
File cc/trees/layer_tree_settings.cc (right):

https://codereview.chromium.org/1126793002/diff/20001/cc/trees/layer_tree_set...
cc/trees/layer_tree_settings.cc:56: // Viewport distance in all directions.
I don't think you need this comment.

[sohanjg, 2015-05-07 14:38:02]

Patchset #3 (id:40001) has been deleted

[sohanjg, 2015-05-07 14:42:11]

I have attached the telemetry data in the bug, can you please take a look.
thanks :)

https://codereview.chromium.org/1126793002/diff/20001/cc/base/switches.h
File cc/base/switches.h (right):

https://codereview.chromium.org/1126793002/diff/20001/cc/base/switches.h#newc...
cc/base/switches.h:28: CC_EXPORT extern const char
kTilingInterestAreaViewportMultiplier[];
On 2015/05/06 17:57:29, vmpstr wrote:
> I would say we remove this setting altogether, and add it back if necessary.

Done.

https://codereview.chromium.org/1126793002/diff/20001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/1126793002/diff/20001/cc/layers/picture_layer...
cc/layers/picture_layer_impl_unittest.cc:2558: // High res tiling should have 36
tiles (4x9 tile grid).
On 2015/05/06 17:57:29, vmpstr wrote:
> Can you verify manually that it is actually 4x9? I buy that there's less tiles
> due to a smaller interest area, but I'm not sure I buy 4x9...

Hmm..for this test viewport is 1kX1k, tile size is 256X1002, so each viewport =
3.89 tiles (4, roughly), we have 9 viewport worth of interest area.

https://codereview.chromium.org/1126793002/diff/20001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling.cc (left):

https://codereview.chromium.org/1126793002/diff/20001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:594: gfx::Size tile_size =
tiling_data_.max_texture_size();
On 2015/05/06 17:57:29, vmpstr wrote:
> Unused?

Done.

https://codereview.chromium.org/1126793002/diff/20001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling.h (right):

https://codereview.chromium.org/1126793002/diff/20001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.h:311: const size_t
tiling_interest_area_viewport_multiplier_;
On 2015/05/06 17:57:29, vmpstr wrote:
> Let's make the multiplier be a float to allow for fractional multipliers

Done.

https://codereview.chromium.org/1126793002/diff/20001/cc/resources/tile_manag...
File cc/resources/tile_manager_unittest.cc (right):

https://codereview.chromium.org/1126793002/diff/20001/cc/resources/tile_manag...
cc/resources/tile_manager_unittest.cc:779: EXPECT_EQ(8, distance_decreasing);
On 2015/05/06 17:57:29, vmpstr wrote:
> I'd prefer if these numbers stayed the same, can you adjust the multiplier so
> that the same amount of tiles is created instead?

Hmm..we cant modify the LayerTreeSettings from this test, and we are not
explicitly creating PLTS, to pass the multiplier :/

any suggestions ?

https://codereview.chromium.org/1126793002/diff/20001/cc/resources/tile_manag...
cc/resources/tile_manager_unittest.cc:819: EXPECT_EQ(8, distance_decreasing);
On 2015/05/06 17:57:29, vmpstr wrote:
> Same here.

Acknowledged.

https://codereview.chromium.org/1126793002/diff/20001/cc/resources/tile_manag...
cc/resources/tile_manager_unittest.cc:1179:
settings.tiling_interest_area_viewport_multiplier = 36;
On 2015/05/06 17:57:29, vmpstr wrote:
> These numbers seem pretty random, why 36? Before it was 10000 to say
> "everything" basically, but 36 seems awfully specific.

Done.

I tried to keep the interest rect same as before in the test.
It was 10000 X 30 X 30, viewport here is 500 X 500, so i kept it 36 to be same
as before. But, 10000 should pass too, i'll change it.

https://codereview.chromium.org/1126793002/diff/20001/cc/trees/layer_tree_set...
File cc/trees/layer_tree_settings.cc (right):

https://codereview.chromium.org/1126793002/diff/20001/cc/trees/layer_tree_set...
cc/trees/layer_tree_settings.cc:56: // Viewport distance in all directions.
On 2015/05/06 17:57:29, vmpstr wrote:
> I don't think you need this comment.

Done.

[vmpstr, 2015-05-07 17:21:39]

https://codereview.chromium.org/1126793002/diff/20001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/1126793002/diff/20001/cc/layers/picture_layer...
cc/layers/picture_layer_impl_unittest.cc:2558: // High res tiling should have 36
tiles (4x9 tile grid).
On 2015/05/07 14:42:10, sohanjg wrote:
> On 2015/05/06 17:57:29, vmpstr wrote:
> > Can you verify manually that it is actually 4x9? I buy that there's less
tiles
> > due to a smaller interest area, but I'm not sure I buy 4x9...
> 
> Hmm..for this test viewport is 1kX1k, tile size is 256X1002, so each viewport
=
> 3.89 tiles (4, roughly), we have 9 viewport worth of interest area.

There should be exactly 4 tiles in the viewport when accounting for border
pixels. Aside from that 4x9 doesn't refer to 4 tiles/viewport times 9 viewports.
4x9 says that there are 4 tiles across the tiling, and 9 tiles down, which is
what I don't really buy.

I think it might be 3x12 or something like that. Can you just print out all of
the tile indices (get active_layer_->HighResTiling()->AllTilesForTesting(), and
print tile->tiling_i_index() and tile->tiling_j_index() to see how they look on
a 2d grid)

https://codereview.chromium.org/1126793002/diff/20001/cc/resources/tile_manag...
File cc/resources/tile_manager_unittest.cc (right):

https://codereview.chromium.org/1126793002/diff/20001/cc/resources/tile_manag...
cc/resources/tile_manager_unittest.cc:779: EXPECT_EQ(8, distance_decreasing);
On 2015/05/07 14:42:10, sohanjg wrote:
> On 2015/05/06 17:57:29, vmpstr wrote:
> > I'd prefer if these numbers stayed the same, can you adjust the multiplier
so
> > that the same amount of tiles is created instead?
> 
> Hmm..we cant modify the LayerTreeSettings from this test, and we are not
> explicitly creating PLTS, to pass the multiplier :/
> 
> any suggestions ?

host_impl that is constructed for this test takes a LayerTreeSettings. Does
setting the multiplier to be high for all tests here work?

https://codereview.chromium.org/1126793002/diff/60001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling_unittest.cc (right):

https://codereview.chromium.org/1126793002/diff/60001/cc/resources/picture_la...
cc/resources/picture_layer_tiling_unittest.cc:770:
settings.tiling_interest_area_viewport_multiplier = 1600;
Why 1600?

https://codereview.chromium.org/1126793002/diff/60001/cc/resources/picture_la...
cc/resources/picture_layer_tiling_unittest.cc:1269:
settings.tiling_interest_area_viewport_multiplier = 4;
Why 4?

https://codereview.chromium.org/1126793002/diff/60001/cc/resources/picture_la...
cc/resources/picture_layer_tiling_unittest.cc:1724:
settings.tiling_interest_area_viewport_multiplier = 200;
Why 200?

https://codereview.chromium.org/1126793002/diff/60001/cc/resources/picture_la...
cc/resources/picture_layer_tiling_unittest.cc:1854:
settings.tiling_interest_area_viewport_multiplier = 10;
Is this line required at all? 10 is close to 9 which is the default?

https://codereview.chromium.org/1126793002/diff/60001/cc/resources/picture_la...
cc/resources/picture_layer_tiling_unittest.cc:1922:
settings.tiling_interest_area_viewport_multiplier = 10;
Is this line required at all? 10 is close to 9 which is the default?

[sohanjg, 2015-05-08 13:20:58]

Patchset #4 (id:80001) has been deleted

[sohanjg, 2015-05-08 13:26:10]

Please take a look, thanks.

https://codereview.chromium.org/1126793002/diff/20001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/1126793002/diff/20001/cc/layers/picture_layer...
cc/layers/picture_layer_impl_unittest.cc:2558: // High res tiling should have 36
tiles (4x9 tile grid).
On 2015/05/07 17:21:38, vmpstr wrote:
> On 2015/05/07 14:42:10, sohanjg wrote:
> > On 2015/05/06 17:57:29, vmpstr wrote:
> > > Can you verify manually that it is actually 4x9? I buy that there's less
> tiles
> > > due to a smaller interest area, but I'm not sure I buy 4x9...
> > 
> > Hmm..for this test viewport is 1kX1k, tile size is 256X1002, so each
viewport
> =
> > 3.89 tiles (4, roughly), we have 9 viewport worth of interest area.
> 
> There should be exactly 4 tiles in the viewport when accounting for border
> pixels. Aside from that 4x9 doesn't refer to 4 tiles/viewport times 9
viewports.
> 4x9 says that there are 4 tiles across the tiling, and 9 tiles down, which is
> what I don't really buy.
> 
> I think it might be 3x12 or something like that. Can you just print out all of
> the tile indices (get active_layer_->HighResTiling()->AllTilesForTesting(),
and
> print tile->tiling_i_index() and tile->tiling_j_index() to see how they look
on
> a 2d grid)

Yes, you are right. My mistake.
The tile x,y does show it is a 3X12 layout.

https://codereview.chromium.org/1126793002/diff/20001/cc/resources/tile_manag...
File cc/resources/tile_manager_unittest.cc (right):

https://codereview.chromium.org/1126793002/diff/20001/cc/resources/tile_manag...
cc/resources/tile_manager_unittest.cc:779: EXPECT_EQ(8, distance_decreasing);
On 2015/05/07 17:21:38, vmpstr wrote:
> On 2015/05/07 14:42:10, sohanjg wrote:
> > On 2015/05/06 17:57:29, vmpstr wrote:
> > > I'd prefer if these numbers stayed the same, can you adjust the multiplier
> so
> > > that the same amount of tiles is created instead?
> > 
> > Hmm..we cant modify the LayerTreeSettings from this test, and we are not
> > explicitly creating PLTS, to pass the multiplier :/
> > 
> > any suggestions ?
> 
> host_impl that is constructed for this test takes a LayerTreeSettings. Does
> setting the multiplier to be high for all tests here work?

Done.

Works fine, thanks  :)

https://codereview.chromium.org/1126793002/diff/20001/cc/resources/tile_manag...
cc/resources/tile_manager_unittest.cc:819: EXPECT_EQ(8, distance_decreasing);
On 2015/05/07 14:42:10, sohanjg wrote:
> On 2015/05/06 17:57:29, vmpstr wrote:
> > Same here.
> 
> Acknowledged.

Done.

https://codereview.chromium.org/1126793002/diff/60001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling_unittest.cc (right):

https://codereview.chromium.org/1126793002/diff/60001/cc/resources/picture_la...
cc/resources/picture_layer_tiling_unittest.cc:770:
settings.tiling_interest_area_viewport_multiplier = 1600;
On 2015/05/07 17:21:39, vmpstr wrote:
> Why 1600?

Done.

I tried to keep the same interest rect as before this changes, so adjutsed
viewport multiplier accordingly.
Its fine if we keep the old values too, reverting them.

https://codereview.chromium.org/1126793002/diff/60001/cc/resources/picture_la...
cc/resources/picture_layer_tiling_unittest.cc:1269:
settings.tiling_interest_area_viewport_multiplier = 4;
On 2015/05/07 17:21:39, vmpstr wrote:
> Why 4?

Done.

same.

https://codereview.chromium.org/1126793002/diff/60001/cc/resources/picture_la...
cc/resources/picture_layer_tiling_unittest.cc:1724:
settings.tiling_interest_area_viewport_multiplier = 200;
On 2015/05/07 17:21:39, vmpstr wrote:
> Why 200?

Done.

same.

https://codereview.chromium.org/1126793002/diff/60001/cc/resources/picture_la...
cc/resources/picture_layer_tiling_unittest.cc:1854:
settings.tiling_interest_area_viewport_multiplier = 10;
On 2015/05/07 17:21:39, vmpstr wrote:
> Is this line required at all? 10 is close to 9 which is the default?

Done.

Not reqd :)

https://codereview.chromium.org/1126793002/diff/60001/cc/resources/picture_la...
cc/resources/picture_layer_tiling_unittest.cc:1922:
settings.tiling_interest_area_viewport_multiplier = 10;
On 2015/05/07 17:21:39, vmpstr wrote:
> Is this line required at all? 10 is close to 9 which is the default?

Done.

[sohanjg, 2015-05-11 17:37:39]

On 2015/05/08 13:26:10, sohanjg wrote:
> Please take a look, thanks.
> 
>
https://codereview.chromium.org/1126793002/diff/20001/cc/layers/picture_layer...
> File cc/layers/picture_layer_impl_unittest.cc (right):
> 
>
https://codereview.chromium.org/1126793002/diff/20001/cc/layers/picture_layer...
> cc/layers/picture_layer_impl_unittest.cc:2558: // High res tiling should have
36
> tiles (4x9 tile grid).
> On 2015/05/07 17:21:38, vmpstr wrote:
> > On 2015/05/07 14:42:10, sohanjg wrote:
> > > On 2015/05/06 17:57:29, vmpstr wrote:
> > > > Can you verify manually that it is actually 4x9? I buy that there's less
> > tiles
> > > > due to a smaller interest area, but I'm not sure I buy 4x9...
> > > 
> > > Hmm..for this test viewport is 1kX1k, tile size is 256X1002, so each
> viewport
> > =
> > > 3.89 tiles (4, roughly), we have 9 viewport worth of interest area.
> > 
> > There should be exactly 4 tiles in the viewport when accounting for border
> > pixels. Aside from that 4x9 doesn't refer to 4 tiles/viewport times 9
> viewports.
> > 4x9 says that there are 4 tiles across the tiling, and 9 tiles down, which
is
> > what I don't really buy.
> > 
> > I think it might be 3x12 or something like that. Can you just print out all
of
> > the tile indices (get active_layer_->HighResTiling()->AllTilesForTesting(),
> and
> > print tile->tiling_i_index() and tile->tiling_j_index() to see how they look
> on
> > a 2d grid)
> 
> Yes, you are right. My mistake.
> The tile x,y does show it is a 3X12 layout.
> 
>
https://codereview.chromium.org/1126793002/diff/20001/cc/resources/tile_manag...
> File cc/resources/tile_manager_unittest.cc (right):
> 
>
https://codereview.chromium.org/1126793002/diff/20001/cc/resources/tile_manag...
> cc/resources/tile_manager_unittest.cc:779: EXPECT_EQ(8, distance_decreasing);
> On 2015/05/07 17:21:38, vmpstr wrote:
> > On 2015/05/07 14:42:10, sohanjg wrote:
> > > On 2015/05/06 17:57:29, vmpstr wrote:
> > > > I'd prefer if these numbers stayed the same, can you adjust the
multiplier
> > so
> > > > that the same amount of tiles is created instead?
> > > 
> > > Hmm..we cant modify the LayerTreeSettings from this test, and we are not
> > > explicitly creating PLTS, to pass the multiplier :/
> > > 
> > > any suggestions ?
> > 
> > host_impl that is constructed for this test takes a LayerTreeSettings. Does
> > setting the multiplier to be high for all tests here work?
> 
> Done.
> 
> Works fine, thanks  :)
> 
>
https://codereview.chromium.org/1126793002/diff/20001/cc/resources/tile_manag...
> cc/resources/tile_manager_unittest.cc:819: EXPECT_EQ(8, distance_decreasing);
> On 2015/05/07 14:42:10, sohanjg wrote:
> > On 2015/05/06 17:57:29, vmpstr wrote:
> > > Same here.
> > 
> > Acknowledged.
> 
> Done.
> 
>
https://codereview.chromium.org/1126793002/diff/60001/cc/resources/picture_la...
> File cc/resources/picture_layer_tiling_unittest.cc (right):
> 
>
https://codereview.chromium.org/1126793002/diff/60001/cc/resources/picture_la...
> cc/resources/picture_layer_tiling_unittest.cc:770:
> settings.tiling_interest_area_viewport_multiplier = 1600;
> On 2015/05/07 17:21:39, vmpstr wrote:
> > Why 1600?
> 
> Done.
> 
> I tried to keep the same interest rect as before this changes, so adjutsed
> viewport multiplier accordingly.
> Its fine if we keep the old values too, reverting them.
> 
>
https://codereview.chromium.org/1126793002/diff/60001/cc/resources/picture_la...
> cc/resources/picture_layer_tiling_unittest.cc:1269:
> settings.tiling_interest_area_viewport_multiplier = 4;
> On 2015/05/07 17:21:39, vmpstr wrote:
> > Why 4?
> 
> Done.
> 
> same.
> 
>
https://codereview.chromium.org/1126793002/diff/60001/cc/resources/picture_la...
> cc/resources/picture_layer_tiling_unittest.cc:1724:
> settings.tiling_interest_area_viewport_multiplier = 200;
> On 2015/05/07 17:21:39, vmpstr wrote:
> > Why 200?
> 
> Done.
> 
> same.
> 
>
https://codereview.chromium.org/1126793002/diff/60001/cc/resources/picture_la...
> cc/resources/picture_layer_tiling_unittest.cc:1854:
> settings.tiling_interest_area_viewport_multiplier = 10;
> On 2015/05/07 17:21:39, vmpstr wrote:
> > Is this line required at all? 10 is close to 9 which is the default?
> 
> Done.
> 
> Not reqd :)
> 
>
https://codereview.chromium.org/1126793002/diff/60001/cc/resources/picture_la...
> cc/resources/picture_layer_tiling_unittest.cc:1922:
> settings.tiling_interest_area_viewport_multiplier = 10;
> On 2015/05/07 17:21:39, vmpstr wrote:
> > Is this line required at all? 10 is close to 9 which is the default?
> 
> Done.

The telemetry bots seems to be green with 9 as viewport multiplier, should we
plan to reduce it further ?

[vmpstr, 2015-05-11 17:52:24]

> The telemetry bots seems to be green with 9 as viewport multiplier, should we
> plan to reduce it further ?

I think we're more concerned with the performance on several of the tests.
Telemetry bots only do some basic testing AFAIK. That's not really indicative of
whether this patch is affecting performance.

[vmpstr, 2015-05-12 00:35:30]

Can you add a description and remove "[WIP]" from the subject please?

[vmpstr, 2015-05-12 00:42:35]

On 2015/05/12 00:35:30, vmpstr wrote:
> Can you add a description and remove "[WIP]" from the subject please?

Other than that lgtm. You'll need to get owner's approvals for chrome/ and
content/ changes before landing.

[vmpstr, 2015-05-12 00:43:35]

On 2015/05/12 00:42:35, vmpstr wrote:
> On 2015/05/12 00:35:30, vmpstr wrote:
> > Can you add a description and remove "[WIP]" from the subject please?
> 
> Other than that lgtm. You'll need to get owner's approvals for chrome/ and
> content/ changes before landing.

(or just content, I guess)

[sohanjg, 2015-05-12 05:36:20]

On 2015/05/12 00:43:35, vmpstr wrote:
> On 2015/05/12 00:42:35, vmpstr wrote:
> > On 2015/05/12 00:35:30, vmpstr wrote:
> > > Can you add a description and remove "[WIP]" from the subject please?
> > 
> > Other than that lgtm. You'll need to get owner's approvals for chrome/ and
> > content/ changes before landing.
> 
> (or just content, I guess)

sievers@: Can you pls have a look at content/

[sohanjg, 2015-05-12 05:36:36]

sohan.jyoti@samsung.com changed reviewers:
+ sievers@chromium.org

[vmpstr, 2015-05-13 16:10:22]

vmpstr@chromium.org changed reviewers:
+ piman@chromium.org

[vmpstr, 2015-05-13 16:10:53]

+piman, please take a look at content/

[piman, 2015-05-13 16:45:27]

lgtm

[sohanjg, 2015-05-13 16:52:26]

The CQ bit was checked by sohan.jyoti@samsung.com

[commit-bot: I haz the power, 2015-05-13 16:53:01]

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

[commit-bot: I haz the power, 2015-05-13 17:25:55]

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

[commit-bot: I haz the power, 2015-05-13 17:25:56]

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

[sohanjg, 2015-05-14 06:19:28]

The CQ bit was checked by sohan.jyoti@samsung.com to run a CQ dry run

[sohanjg, 2015-05-14 06:19:28]

The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org,
piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1126793002/#ps120001
(title: "remove switch entry from chromeos cmdline.")

[commit-bot: I haz the power, 2015-05-14 06:19:49]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1126793002/120001

[sohanjg, 2015-05-14 07:04:52]

On 2015/05/14 06:19:49, I haz the power (commit-bot) wrote:
> Dry run: CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1126793002/120001

Missed removing kMaxTilesForInterestArea from chrome_restart_request.cc, hence
the chromeOS build issues.
Removed it.

[commit-bot: I haz the power, 2015-05-14 07:12:32]

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

[commit-bot: I haz the power, 2015-05-14 07:12:33]

Dry run: This issue passed the CQ dry run.

[vmpstr, 2015-05-14 17:15:06]

On 2015/05/14 07:04:52, sohanjg wrote:
> On 2015/05/14 06:19:49, I haz the power (commit-bot) wrote:
> > Dry run: CQ is trying da patch. Follow status at
> >  https://chromium-cq-status.appspot.com/patch-status/1126793002/120001
> 
> Missed removing kMaxTilesForInterestArea from chrome_restart_request.cc, hence
> the chromeOS build issues.
> Removed it.

lgtm

[sohanjg, 2015-05-14 17:26:41]

The CQ bit was checked by sohan.jyoti@samsung.com

[commit-bot: I haz the power, 2015-05-14 17:28:04]

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

[commit-bot: I haz the power, 2015-05-14 17:37:29]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2015-05-14 17:39:03]

Patchset 5 (id:??) landed as
https://crrev.com/6d84db54a27c036d77b0e4a26d42c158ee4a6add
Cr-Commit-Position: refs/heads/master@{#329870}

[sohanjg, 2015-11-18 10:25:05]

Description was changed from

==========
cc: Make tiling interest rect calc based on viewport.

Current interest/eventually/live rect is calculated based
on  max_tiles_for_interest_area setting which is not the
best choice for determining the size. As part of this patch
tiling interest rect is calculated based on viewport area.
Which is multiplied by a constant factor. The factor is
currently 9, viewport dist in all directions.
Also, max_tiles_for_interest_area setting is removed
and its associated switch and entries in chrome://flags.

BUG=483520

Committed: https://crrev.com/6d84db54a27c036d77b0e4a26d42c158ee4a6add
Cr-Commit-Position: refs/heads/master@{#329870}
==========

to

==========
cc: Make tiling interest rect calc based on viewport.

Current interest/eventually/live rect is calculated based
on  max_tiles_for_interest_area setting which is not the
best choice for determining the size. As part of this patch
tiling interest rect is calculated based on viewport area.
Which is multiplied by a constant factor. The factor is
currently 9, viewport dist in all directions.
Also, max_tiles_for_interest_area setting is removed
and its associated switch and entries in chrome://flags.

BUG=483520

Committed: https://crrev.com/6d84db54a27c036d77b0e4a26d42c158ee4a6add
Cr-Commit-Position: refs/heads/master@{#329870}
==========