Split ClipTransformRecorder into {Clip,Transform}Recorder.

Split ClipTransformRecorder into {Clip,Transform}Recorder.

BUG=529938
Committed: https://crrev.com/92a8676709834996882fcd07295e3523eb3ca89b
Cr-Commit-Position: refs/heads/master@{#361421}

[wkorman, 2015-11-23 23:07:23]

wkorman@chromium.org changed reviewers:
+ danakj@chromium.org, enne@chromium.org

[wkorman, 2015-11-23 23:07:23]

As a side effect of this split we lose the nice built in closer-order we had
when these were one class.

After http://crrev.com/1473453002 we can trust that Clip/Transform LIFO order is
maintained for EndClip/EndTransform. However with this change it is possible for
someone to write code that violates this for example:

ui::TransformRecorder transform_recorder(context);
ui::ClipRecorder clip_recorder(context);
clip_recorder.ClipRect(...);
transform_recorder.Transform(...);
// Clip
// Transform
// EndClip <-- wrong order
// EndTransform

We now rely on people ordering the instance declaration to match the potential
method calls:

ui::ClipRecorder clip_recorder(context);
ui::TransformRecorder transform_recorder(context);
clip_recorder.ClipRect(...);
transform_recorder.Transform(...);
// Clip
// Transform
// EndTransform
// EndClip

I think this is fine, just calling out.

[danakj, 2015-11-23 23:20:30]

LGTM

https://codereview.chromium.org/1470123002/diff/1/ui/compositor/clip_recorder.h
File ui/compositor/clip_recorder.h (right):

https://codereview.chromium.org/1470123002/diff/1/ui/compositor/clip_recorder...
ui/compositor/clip_recorder.h:24: // A class to provide scoped clips of painting
to a
use the full 80 cols

https://codereview.chromium.org/1470123002/diff/1/ui/compositor/paint_context.h
File ui/compositor/paint_context.h (right):

https://codereview.chromium.org/1470123002/diff/1/ui/compositor/paint_context...
ui/compositor/paint_context.h:24: class ClipRecorder;
forward declare TransformRecorder too

https://codereview.chromium.org/1470123002/diff/1/ui/compositor/transform_rec...
File ui/compositor/transform_recorder.h (right):

https://codereview.chromium.org/1470123002/diff/1/ui/compositor/transform_rec...
ui/compositor/transform_recorder.h:25: // DisplayItems added to the
DisplayItemList while this object is
is this wrapped at 80 or 72?

https://codereview.chromium.org/1470123002/diff/1/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/1470123002/diff/1/ui/views/view.cc#newcode784
ui/views/view.cc:784: if (!layer()) {
can you refactor this just slightly. it's important now that they are
constructed in the same order that the calls inside this block are made, which
isn't very obvious and could be messed up in the future. I suggest

// If the view is bac....
bool paint_relative_to_parent = !layer();

ui::ClipRecorder ...;
if (paint_relative_to_parent) {
  .. clip_recorder.ClipRect();
}

ui::TransformRecorder ...;
if (paint_relative_to_parent) {
  .. transform_recorder.Transform();
}

ie 2 blocks one for each RAII object.
}

[danakj, 2015-11-23 23:20:55]

danakj@chromium.org changed reviewers:
+ sky@chromium.org

[danakj, 2015-11-23 23:20:56]

+owner sky

[wkorman, 2015-11-23 23:38:08]

https://codereview.chromium.org/1470123002/diff/1/ui/compositor/clip_recorder.h
File ui/compositor/clip_recorder.h (right):

https://codereview.chromium.org/1470123002/diff/1/ui/compositor/clip_recorder...
ui/compositor/clip_recorder.h:24: // A class to provide scoped clips of painting
to a
On 2015/11/23 at 23:20:30, danakj (behind on reviews) wrote:
> use the full 80 cols

Done.

https://codereview.chromium.org/1470123002/diff/1/ui/compositor/paint_context.h
File ui/compositor/paint_context.h (right):

https://codereview.chromium.org/1470123002/diff/1/ui/compositor/paint_context...
ui/compositor/paint_context.h:24: class ClipRecorder;
On 2015/11/23 at 23:20:30, danakj (behind on reviews) wrote:
> forward declare TransformRecorder too

Done.

https://codereview.chromium.org/1470123002/diff/1/ui/compositor/transform_rec...
File ui/compositor/transform_recorder.h (right):

https://codereview.chromium.org/1470123002/diff/1/ui/compositor/transform_rec...
ui/compositor/transform_recorder.h:25: // DisplayItems added to the
DisplayItemList while this object is
On 2015/11/23 at 23:20:30, danakj (behind on reviews) wrote:
> is this wrapped at 80 or 72?

Done. I fixed my emacs config to address henceforth.

https://codereview.chromium.org/1470123002/diff/1/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/1470123002/diff/1/ui/views/view.cc#newcode784
ui/views/view.cc:784: if (!layer()) {
On 2015/11/23 at 23:20:30, danakj (behind on reviews) wrote:
> can you refactor this just slightly. it's important now that they are
constructed in the same order that the calls inside this block are made, which
isn't very obvious and could be messed up in the future. I suggest
> 
> // If the view is bac....
> bool paint_relative_to_parent = !layer();
> 
> ui::ClipRecorder ...;
> if (paint_relative_to_parent) {
>   .. clip_recorder.ClipRect();
> }
> 
> ui::TransformRecorder ...;
> if (paint_relative_to_parent) {
>   .. transform_recorder.Transform();
> }
> 
> ie 2 blocks one for each RAII object.
> }

Done.

[danakj, 2015-11-23 23:47:45]

Thanks LGTM

[sky, 2015-11-24 00:23:12]

LGTM

[wkorman, 2015-11-24 06:14:53]

The CQ bit was checked by wkorman@chromium.org

[wkorman, 2015-11-24 06:14:53]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1470123002/#ps40001
(title: "Sync to head.")

[commit-bot: I haz the power, 2015-11-24 06:15:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1470123002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1470123002/40001

[commit-bot: I haz the power, 2015-11-24 06:19:05]

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

[commit-bot: I haz the power, 2015-11-24 06:19:05]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[wkorman, 2015-11-24 06:27:13]

The CQ bit was checked by wkorman@chromium.org

[wkorman, 2015-11-24 06:27:13]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1470123002/#ps60001
(title: "Really sync to head.")

[commit-bot: I haz the power, 2015-11-24 06:28:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1470123002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1470123002/60001

[commit-bot: I haz the power, 2015-11-24 06:42:10]

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

[commit-bot: I haz the power, 2015-11-24 06:42:11]

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

[wkorman, 2015-11-24 07:06:34]

The CQ bit was checked by wkorman@chromium.org

[wkorman, 2015-11-24 07:06:34]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1470123002/#ps80001
(title: "Fix mus includes.")

[commit-bot: I haz the power, 2015-11-24 07:07:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1470123002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1470123002/80001

[commit-bot: I haz the power, 2015-11-24 07:35:40]

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

[commit-bot: I haz the power, 2015-11-24 07:35:41]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[wkorman, 2015-11-24 18:55:07]

The CQ bit was checked by wkorman@chromium.org

[wkorman, 2015-11-24 18:55:08]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1470123002/#ps100001
(title: "Initialize transformed to false.")

[commit-bot: I haz the power, 2015-11-24 18:57:02]

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

[commit-bot: I haz the power, 2015-11-24 20:06:42]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-11-24 20:07:59]

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