Make View::Paint use ui::PaintRecorder to access PaintContext's canvas

Make View::Paint use ui::PaintRecorder to access PaintContext's canvas

This adds a ui::PaintRecorder that will be the gateway to the
PaintContext internals. The rules are:

1) Make a PaintRecorder when you want to access the canvas.
2) Can not nest PaintRecorders.

In the future PaintRecorder will also know how to deal with a
DisplayItemList and provider a Canvas that records to a DisplayItem
and each scoped PaintRecorder will produce DisplayItems to be cached.

For now, just restructuring code to go through PaintRecorder and not
nest recording.

R=sadrul, sky
BUG=466426
Committed: https://crrev.com/318e9c81d83a6862634b87b28e91fa1d4c4f8726
Cr-Commit-Position: refs/heads/master@{#323936}

[danakj, 2015-04-02 22:59:24]

Depends on: https://codereview.chromium.org/1055473003/ to avoid awkward early
outs.

[sky, 2015-04-02 23:43:09]

Are there any interesting changes here other than view.*?

https://codereview.chromium.org/1053143002/diff/40001/ui/compositor/paint_rec...
File ui/compositor/paint_recorder.h (right):

https://codereview.chromium.org/1053143002/diff/40001/ui/compositor/paint_rec...
ui/compositor/paint_recorder.h:24: PaintRecorder(const PaintContext& context);
explicit

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

https://codereview.chromium.org/1053143002/diff/40001/ui/views/view.cc#newcod...
ui/views/view.cc:825: ui::PaintRecorder recorder(context);
Why do you need this logic?

[danakj, 2015-04-03 00:27:36]

> Are there any interesting changes here other than view.*?

Oops, this included https://codereview.chromium.org/1057873004/ by accident it
should be a tiny CL. fixed.

https://codereview.chromium.org/1053143002/diff/40001/ui/compositor/paint_rec...
File ui/compositor/paint_recorder.h (right):

https://codereview.chromium.org/1053143002/diff/40001/ui/compositor/paint_rec...
ui/compositor/paint_recorder.h:24: PaintRecorder(const PaintContext& context);
On 2015/04/02 23:43:09, sky wrote:
> explicit

oops thanks.

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

https://codereview.chromium.org/1053143002/diff/40001/ui/views/view.cc#newcod...
ui/views/view.cc:825: ui::PaintRecorder recorder(context);
On 2015/04/02 23:43:09, sky wrote:
> Why do you need this logic?

This matches the save on l.788. We can't use ScopedCanvas because recordings can
not be nested.

[sky, 2015-04-03 14:53:59]

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

https://codereview.chromium.org/1053143002/diff/40001/ui/views/view.cc#newcod...
ui/views/view.cc:825: ui::PaintRecorder recorder(context);
On 2015/04/03 00:27:36, danakj wrote:
> On 2015/04/02 23:43:09, sky wrote:
> > Why do you need this logic?
> 
> This matches the save on l.788. We can't use ScopedCanvas because recordings
can
> not be nested.

Tricky, but error prone. How about a scoped_ptr<ScopedCanvas> defined on 780,
but set on 786 (where you have it now). I believe that would mean you can nuke
824-827.

https://codereview.chromium.org/1053143002/diff/60001/ui/compositor/composito...
File ui/compositor/compositor.gyp (right):

https://codereview.chromium.org/1053143002/diff/60001/ui/compositor/composito...
ui/compositor/compositor.gyp:65: 'paint_recorder.h',
nit: .cc first (you have in right in the gn file).

[danakj, 2015-04-03 16:15:35]

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

https://codereview.chromium.org/1053143002/diff/40001/ui/views/view.cc#newcod...
ui/views/view.cc:825: ui::PaintRecorder recorder(context);
On 2015/04/03 14:53:59, sky wrote:
> On 2015/04/03 00:27:36, danakj wrote:
> > On 2015/04/02 23:43:09, sky wrote:
> > > Why do you need this logic?
> > 
> > This matches the save on l.788. We can't use ScopedCanvas because recordings
> can
> > not be nested.
> 
> Tricky, but error prone. How about a scoped_ptr<ScopedCanvas> defined on 780,
> but set on 786 (where you have it now). I believe that would mean you can nuke
> 824-827.

The Canvas* here and at l786 are going to be different. Each scope of
PaintRecorder will have a different Canvas*.

One thing we could do is use a new RAII object that points at the paint context,
and makes a PaintRecorder and save/restores into it. But we need to cache the
display items in the correct order.. I think to do that, we need a longer lived
RAII object outside the scope of each PaintRecorder, and a ScopedSaveRestore
points at that. Nesting gets trickier than.

I agree it's more tricky with save/restore not on scope, but solutions around
this seem more complex than the problem. The issue is that the save and restore
have to be separate displayitems because there will be other displayitems in
between from children.

[danakj, 2015-04-03 16:19:53]

On 2015/04/03 16:15:35, danakj wrote:
> https://codereview.chromium.org/1053143002/diff/40001/ui/views/view.cc
> File ui/views/view.cc (right):
> 
>
https://codereview.chromium.org/1053143002/diff/40001/ui/views/view.cc#newcod...
> ui/views/view.cc:825: ui::PaintRecorder recorder(context);
> On 2015/04/03 14:53:59, sky wrote:
> > On 2015/04/03 00:27:36, danakj wrote:
> > > On 2015/04/02 23:43:09, sky wrote:
> > > > Why do you need this logic?
> > > 
> > > This matches the save on l.788. We can't use ScopedCanvas because
recordings
> > can
> > > not be nested.
> > 
> > Tricky, but error prone. How about a scoped_ptr<ScopedCanvas> defined on
780,
> > but set on 786 (where you have it now). I believe that would mean you can
nuke
> > 824-827.
> 
> The Canvas* here and at l786 are going to be different. Each scope of
> PaintRecorder will have a different Canvas*.
> 
> One thing we could do is use a new RAII object that points at the paint
context,
> and makes a PaintRecorder and save/restores into it. But we need to cache the
> display items in the correct order.. I think to do that, we need a longer
lived
> RAII object outside the scope of each PaintRecorder, and a ScopedSaveRestore
> points at that. Nesting gets trickier than.
> 
> I agree it's more tricky with save/restore not on scope, but solutions around
> this seem more complex than the problem. The issue is that the save and
restore
> have to be separate displayitems because there will be other displayitems in
> between from children.

I could add something just for DCHECKs but still require code in both places.
Like.

ScopedSaveRestoreChecker checker;

{
  PaintRecorder rec(..);
  checker.Save(rec);
}

..

{
  PaintRecorder rec(..);
  checker.Restore(rec);
}

Then removing Save or Restore but not the other could DCHECK. It's not super
amazing though?

[danakj, 2015-04-03 17:45:04]

On 2015/04/03 16:19:53, danakj wrote:
> On 2015/04/03 16:15:35, danakj wrote:
> > https://codereview.chromium.org/1053143002/diff/40001/ui/views/view.cc
> > File ui/views/view.cc (right):
> > 
> >
>
https://codereview.chromium.org/1053143002/diff/40001/ui/views/view.cc#newcod...
> > ui/views/view.cc:825: ui::PaintRecorder recorder(context);
> > On 2015/04/03 14:53:59, sky wrote:
> > > On 2015/04/03 00:27:36, danakj wrote:
> > > > On 2015/04/02 23:43:09, sky wrote:
> > > > > Why do you need this logic?
> > > > 
> > > > This matches the save on l.788. We can't use ScopedCanvas because
> recordings
> > > can
> > > > not be nested.
> > > 
> > > Tricky, but error prone. How about a scoped_ptr<ScopedCanvas> defined on
> 780,
> > > but set on 786 (where you have it now). I believe that would mean you can
> nuke
> > > 824-827.
> > 
> > The Canvas* here and at l786 are going to be different. Each scope of
> > PaintRecorder will have a different Canvas*.
> > 
> > One thing we could do is use a new RAII object that points at the paint
> context,
> > and makes a PaintRecorder and save/restores into it. But we need to cache
the
> > display items in the correct order.. I think to do that, we need a longer
> lived
> > RAII object outside the scope of each PaintRecorder, and a ScopedSaveRestore
> > points at that. Nesting gets trickier than.
> > 
> > I agree it's more tricky with save/restore not on scope, but solutions
around
> > this seem more complex than the problem. The issue is that the save and
> restore
> > have to be separate displayitems because there will be other displayitems in
> > between from children.
> 
> I could add something just for DCHECKs but still require code in both places.
> Like.
> 
> ScopedSaveRestoreChecker checker;
> 
> {
>   PaintRecorder rec(..);
>   checker.Save(rec);
> }
> 
> ..
> 
> {
>   PaintRecorder rec(..);
>   checker.Restore(rec);
> }
> 
> Then removing Save or Restore but not the other could DCHECK. It's not super
> amazing though?

Talked to pdr@ about what Blink does for this stuff, he pointed out they do not
bother trying to cache clip or transforms, and they do those through a different
recorder class. So then this would look something like:


{
  ClipRecorder clip(clip_rect...);
  TransformRecorder transform(GetTransform()...);

  {
     PaintRecorder recorder(context);
     ...
     OnPaint(context.canvas());
  }

  PaintChildren(context);
}

The ClipRecorder/TransformRecorder would not be cached every, but the work
computing them is presumably cheap enough to not care. They'd insert scoped
save/restores in the constructor/destructor.

Only PaintRecorder would have caching added to it in the future.

[danakj, 2015-04-03 18:28:56]

PTAL, added a ClipTransformRecorder.

[sky, 2015-04-03 20:46:44]

LGTM

[danakj, 2015-04-04 01:25:33]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2015-04-04 01:25:43]

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

[commit-bot: I haz the power, 2015-04-04 01:30:49]

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

[commit-bot: I haz the power, 2015-04-04 01:30:50]

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...)
  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_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[danakj, 2015-04-06 17:27:03]

The CQ bit was checked by danakj@chromium.org

[danakj, 2015-04-06 17:27:04]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1053143002/#ps180001
(title: "paintrecorder: rebase")

[commit-bot: I haz the power, 2015-04-06 17:27:25]

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

[commit-bot: I haz the power, 2015-04-06 19:52:51]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-04-06 20:05:23]

Patchset 10 (id:??) landed as
https://crrev.com/318e9c81d83a6862634b87b28e91fa1d4c4f8726
Cr-Commit-Position: refs/heads/master@{#323936}