ui: Eliminate allocating gfx::Canvas on the heap for every view.

ui: Eliminate allocating gfx::Canvas on the heap for every view.

PaintRecorder needs to present a gfx::Canvas instead of an SkCanvas.
Currently it does so via CreateCanvasWithoutScaling which returns
a new Canvas allocated on the heap. But mallocs are terrible, so
instead make it a constructor and have the canvas allocated as part
of ui::PaintRecorder on the stack.

This changes the time to record a single tab loading spinner from
0.138818 ms to 0.124487 ms per frame for a 10% reduction in time.

R=sky
BUG=466426
Committed: https://crrev.com/8fa739b657b48f54ee96e2187158570101623f94
Cr-Commit-Position: refs/heads/master@{#329315}

[danakj, 2015-05-11 23:10:16]

https://codereview.chromium.org/1124223010/diff/1/ui/compositor/paint_recorde...
File ui/compositor/paint_recorder.cc (right):

https://codereview.chromium.org/1124223010/diff/1/ui/compositor/paint_recorde...
ui/compositor/paint_recorder.cc:25: ? context.canvas_->sk_canvas()
This is all a little bit crazy, but will go away/get better when we eliminate
the canvas path. It's sad that this has to all be written in the initializer
like this but constructors /o\. I didn't want to add another constructor to
gfx::Canvas that allowed it to be null, that seemed slightly less good than some
temporary crazy edge cases for git cl format to figure out.

[sky, 2015-05-11 23:37:34]

https://codereview.chromium.org/1124223010/diff/20001/ui/compositor/paint_rec...
File ui/compositor/paint_recorder.cc (right):

https://codereview.chromium.org/1124223010/diff/20001/ui/compositor/paint_rec...
ui/compositor/paint_recorder.cc:23: // the recorder_ so no  need to store a
RefPtr to it on this class.
nit: you've got two spaces after 'no'.

https://codereview.chromium.org/1124223010/diff/20001/ui/compositor/paint_rec...
ui/compositor/paint_recorder.cc:33: canvas_(context.canvas_ ? context.canvas_ :
&owned_canvas_),
Shouldn't we Save() in the constructor and Restore() in the destructor? If not,
isn't any state installed on the shared canvas permanent?

[danakj, 2015-05-11 23:42:14]

https://codereview.chromium.org/1124223010/diff/20001/ui/compositor/paint_rec...
File ui/compositor/paint_recorder.cc (right):

https://codereview.chromium.org/1124223010/diff/20001/ui/compositor/paint_rec...
ui/compositor/paint_recorder.cc:23: // the recorder_ so no  need to store a
RefPtr to it on this class.
On 2015/05/11 23:37:34, sky wrote:
> nit: you've got two spaces after 'no'.

Done.

https://codereview.chromium.org/1124223010/diff/20001/ui/compositor/paint_rec...
ui/compositor/paint_recorder.cc:33: canvas_(context.canvas_ ? context.canvas_ :
&owned_canvas_),
On 2015/05/11 23:37:34, sky wrote:
> Shouldn't we Save() in the constructor and Restore() in the destructor? If
not,
> isn't any state installed on the shared canvas permanent?

We use a ScopedCanvas out in View::Paint. Though we could move that here and it
can go away when we turn on slimming paint, cuz it's wasted overhead. I'll send
a separate CL with numbers if it has any impact.

[danakj, 2015-05-11 23:42:53]

https://codereview.chromium.org/1124223010/diff/20001/ui/gfx/canvas.cc
File ui/gfx/canvas.cc (right):

https://codereview.chromium.org/1124223010/diff/20001/ui/gfx/canvas.cc#newcode24
ui/gfx/canvas.cc:24: #include "ui/gfx/canvas_skia_paint.h"
Oops, included a rebase here, not part of my changes.

[sky, 2015-05-11 23:45:52]

Can you at least add a comment as to why it isn't necessary.

[sky, 2015-05-11 23:46:04]

LGTM

[danakj, 2015-05-11 23:54:29]

On 2015/05/11 23:45:52, sky wrote:
> Can you at least add a comment as to why it isn't necessary.

Done.

[danakj, 2015-05-11 23:54:34]

The CQ bit was checked by danakj@chromium.org

[danakj, 2015-05-11 23:54:34]

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/1124223010/#ps60001
(title: "canvasstack: comment")

[commit-bot: I haz the power, 2015-05-11 23:57:34]

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

[danakj, 2015-05-12 00:26:11]

On 2015/05/11 23:42:14, danakj wrote:
> We use a ScopedCanvas out in View::Paint. Though we could move that here and
it
> can go away when we turn on slimming paint, cuz it's wasted overhead. I'll
send
> a separate CL with numbers if it has any impact.

Removing the save/restore with slimming paint is not noticeable outside of
noise.

[danakj, 2015-05-12 00:26:47]

The CQ bit was unchecked by danakj@chromium.org

[danakj, 2015-05-12 00:27:08]

The CQ bit was checked by danakj@chromium.org

[danakj, 2015-05-12 00:27:08]

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/1124223010/#ps80001
(title: "canvasstack: addTODO")

[commit-bot: I haz the power, 2015-05-12 00:29:10]

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

[commit-bot: I haz the power, 2015-05-12 02:10:21]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-05-12 02:11:40]

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