Plumb preferred raster scale for background images from Blink to cc layers.

WIP: plumb preferred raster scale for background images from Blink to cc layers.

BUG=652448
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/cde557b63b79a0b3a22253f125af6c209aa4bce1
Cr-Commit-Position: refs/heads/master@{#426255}

[chrishtr, 2016-10-10 18:34:02]

Description was changed from

==========
none


none


none


none


none


none


none


Merge branch 'master' into rasterscale


none

BUG=
==========

to

==========
none


none


none


none


none


none


none


Merge branch 'master' into rasterscale


none

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[chrishtr, 2016-10-10 18:34:02]

Description was changed from

==========
none


none


none


none


none


none


none


Merge branch 'master' into rasterscale


none

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
none


none


none


none


none


none


none


Merge branch 'master' into rasterscale


none

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[chrishtr, 2016-10-10 19:09:41]

Description was changed from

==========
none


none


none


none


none


none


none


Merge branch 'master' into rasterscale


none

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
WIP: plumb preferred raster scale for background images from Blink to cc layers.

BUG=652448
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[chrishtr, 2016-10-14 21:09:54]

chrishtr@chromium.org changed reviewers:
+ schenney@chromium.org, vmpstr@chromium.org

[chrishtr, 2016-10-14 21:09:54]

This plumbs a "preferred raster scale" value from Blink to cc. In a followup
patch, we'll
use that value to choose raster tile sets at a good resolution to maximize the
chance of
high-quality image rasterization.

[chrishtr, 2016-10-14 21:10:01]

The CQ bit was checked by chrishtr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-14 21:10:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vmpstr, 2016-10-14 21:22:34]

cc lgtm

https://codereview.chromium.org/2410513002/diff/200001/cc/layers/layer.h
File cc/layers/layer.h (right):

https://codereview.chromium.org/2410513002/diff/200001/cc/layers/layer.h#newc...
cc/layers/layer.h:691: bool has_preferred_raster_scale;
nit: move with other bools and : 1.

https://codereview.chromium.org/2410513002/diff/200001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebLayer.h (right):

https://codereview.chromium.org/2410513002/diff/200001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebLayer.h:223: virtual bool
hasPreferredRasterScale() = 0;
Is this one used? If not, please remove.

[chrishtr, 2016-10-14 21:35:09]

https://codereview.chromium.org/2410513002/diff/200001/cc/layers/layer.h
File cc/layers/layer.h (right):

https://codereview.chromium.org/2410513002/diff/200001/cc/layers/layer.h#newc...
cc/layers/layer.h:691: bool has_preferred_raster_scale;
On 2016/10/14 at 21:22:33, vmpstr wrote:
> nit: move with other bools and : 1.

Done.

https://codereview.chromium.org/2410513002/diff/200001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebLayer.h (right):

https://codereview.chromium.org/2410513002/diff/200001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebLayer.h:223: virtual bool
hasPreferredRasterScale() = 0;
On 2016/10/14 at 21:22:34, vmpstr wrote:
> Is this one used? If not, please remove.

Done.

[Stephen Chennney, 2016-10-17 14:31:30]

Update change description?

https://codereview.chromium.org/2410513002/diff/240001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/2410513002/diff/240001/cc/layers/layer.cc#new...
cc/layers/layer.cc:1783: inputs_.preferred_raster_scale = 1.0f;
Do we need a commit here too?

https://codereview.chromium.org/2410513002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/BackgroundImageGeometry.h (right):

https://codereview.chromium.org/2410513002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BackgroundImageGeometry.h:35: // in pixels
of the box to draw the image once.
"... is the size of each instance of the image, in pixels."

https://codereview.chromium.org/2410513002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/BoxPainter.cpp (right):

https://codereview.chromium.org/2410513002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BoxPainter.cpp:132: if
(paintLayer->compositingState() != PaintsIntoOwnBacking)
Move the check for paints into own backing up to where you are already checking
that the layer exists.

https://codereview.chromium.org/2410513002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BoxPainter.cpp:139: if (!obj.layer())
You don't need this. You check it again immediately.

https://codereview.chromium.org/2410513002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/2410513002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:180: // See
https://crbug.com/346236 for current status.
It seems this comment needs to move down.

[chrishtr, 2016-10-19 03:51:24]

The CQ bit was checked by chrishtr@chromium.org to run a CQ dry run

[chrishtr, 2016-10-19 03:51:36]

https://codereview.chromium.org/2410513002/diff/240001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/2410513002/diff/240001/cc/layers/layer.cc#new...
cc/layers/layer.cc:1783: inputs_.preferred_raster_scale = 1.0f;
On 2016/10/17 at 14:31:30, Stephen Chennney wrote:
> Do we need a commit here too?

Good catch, fixed.

https://codereview.chromium.org/2410513002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/BackgroundImageGeometry.h (right):

https://codereview.chromium.org/2410513002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BackgroundImageGeometry.h:35: // in pixels
of the box to draw the image once.
On 2016/10/17 at 14:31:30, Stephen Chennney wrote:
> "... is the size of each instance of the image, in pixels."

Isn't it in destination space though? So it isn't equal to the intrinsic image
size.

https://codereview.chromium.org/2410513002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/BoxPainter.cpp (right):

https://codereview.chromium.org/2410513002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BoxPainter.cpp:132: if
(paintLayer->compositingState() != PaintsIntoOwnBacking)
On 2016/10/17 at 14:31:30, Stephen Chennney wrote:
> Move the check for paints into own backing up to where you are already
checking that the layer exists.

Removed the early-out above, since it's redundant.

https://codereview.chromium.org/2410513002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BoxPainter.cpp:139: if (!obj.layer())
On 2016/10/17 at 14:31:30, Stephen Chennney wrote:
> You don't need this. You check it again immediately.

Good point, removed.

[commit-bot: I haz the power, 2016-10-19 03:52:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-19 05:56:22]

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

[commit-bot: I haz the power, 2016-10-19 05:56:23]

Dry run: Exceeded global retry quota

[Stephen Chennney, 2016-10-19 14:48:15]

LGTM with one more comment pass needed before commit. I'm happy to let you
finalize the comments as you see fit.

https://codereview.chromium.org/2410513002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/BackgroundImageGeometry.h (right):

https://codereview.chromium.org/2410513002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BackgroundImageGeometry.h:35: // in pixels
of the box to draw the image once.
On 2016/10/19 03:51:36, chrishtr wrote:
> On 2016/10/17 at 14:31:30, Stephen Chennney wrote:
> > "... is the size of each instance of the image, in pixels."
> 
> Isn't it in destination space though? So it isn't equal to the intrinsic image
> size.

Right, my comment wasn't clear either. I also realize that this size has meaning
even if we are not tiling. Is this better? "tileSize() defines the size for the
background image to meet CSS background-size and background-repeat
requirements."

https://codereview.chromium.org/2410513002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BackgroundImageGeometry.h:39: // that tile
region.
Then this comment stays to make it clear we will resize to the tileSize.

https://codereview.chromium.org/2410513002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BackgroundImageGeometry.h:42: // direction
are the spaces between tiles, if the image is tiled.
There's already a comment below on spaceSize(). I don't think we need this new
one.

https://codereview.chromium.org/2410513002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BackgroundImageGeometry.h:45: const
LayoutPoint& phase() const { return m_phase; }
If we want to comment all of them, this one is "phase() represents the point in
the image that will appear at (0,0) in the destination space. The point is
defined in tileSize() coordinates."

[chrishtr, 2016-10-19 16:56:07]

https://codereview.chromium.org/2410513002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/BackgroundImageGeometry.h (right):

https://codereview.chromium.org/2410513002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BackgroundImageGeometry.h:35: // in pixels
of the box to draw the image once.
On 2016/10/19 at 14:48:15, Stephen Chennney wrote:
> On 2016/10/19 03:51:36, chrishtr wrote:
> > On 2016/10/17 at 14:31:30, Stephen Chennney wrote:
> > > "... is the size of each instance of the image, in pixels."
> > 
> > Isn't it in destination space though? So it isn't equal to the intrinsic
image
> > size.
> 
> Right, my comment wasn't clear either. I also realize that this size has
meaning even if we are not tiling. Is this better? "tileSize() defines the size
for the background image to meet CSS background-size and background-repeat
requirements."

I added a sentence referencing CSS as an example, and changed a few other words.
Hope that is clearer.

https://codereview.chromium.org/2410513002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BackgroundImageGeometry.h:39: // that tile
region.
On 2016/10/19 at 14:48:15, Stephen Chennney wrote:
> Then this comment stays to make it clear we will resize to the tileSize.

Ack.

https://codereview.chromium.org/2410513002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BackgroundImageGeometry.h:42: // direction
are the spaces between tiles, if the image is tiled.
On 2016/10/19 at 14:48:15, Stephen Chennney wrote:
> There's already a comment below on spaceSize(). I don't think we need this new
one.

Removed.

https://codereview.chromium.org/2410513002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BackgroundImageGeometry.h:45: const
LayoutPoint& phase() const { return m_phase; }
On 2016/10/19 at 14:48:15, Stephen Chennney wrote:
> If we want to comment all of them, this one is "phase() represents the point
in the image that will appear at (0,0) in the destination space. The point is
defined in tileSize() coordinates."

Done.

[chrishtr, 2016-10-19 16:56:09]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2016-10-19 16:56:09]

The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org,
schenney@chromium.org
Link to the patchset: https://codereview.chromium.org/2410513002/#ps300001
(title: "none")

[commit-bot: I haz the power, 2016-10-19 16:56:41]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-19 19:04:27]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2016-10-21 13:10:31]

Description was changed from

==========
WIP: plumb preferred raster scale for background images from Blink to cc layers.

BUG=652448
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
WIP: plumb preferred raster scale for background images from Blink to cc layers.

BUG=652448
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/cde557b63b79a0b3a22253f125af6c209aa4bce1
Cr-Commit-Position: refs/heads/master@{#426255}
==========

[commit-bot: I haz the power, 2016-10-21 13:10:33]

Patchset 16 (id:??) landed as
https://crrev.com/cde557b63b79a0b3a22253f125af6c209aa4bce1
Cr-Commit-Position: refs/heads/master@{#426255}