ui: Use skia::RefPtr<T> for implicit safe reference counting.

ui: Use skia::RefPtr<T> for implicit safe reference counting.

R=sky
BUG=163454
Depends on: https://codereview.chromium.org/11418217/
NOTRY=true

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=171107

[sky, 2012-11-29 23:54:55]

https://codereview.chromium.org/11299262/diff/1019/chrome/browser/ui/views/de...
File chrome/browser/ui/views/detachable_toolbar_view.cc (right):

https://codereview.chromium.org/11299262/diff/1019/chrome/browser/ui/views/de...
chrome/browser/ui/views/detachable_toolbar_view.cc:122: skia::RefPtr<SkShader>
shader;
Is there a reason for keeping this here and not at 126 where you create it?

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

https://codereview.chromium.org/11299262/diff/1019/ui/gfx/canvas.cc#newcode30
ui/gfx/canvas.cc:30: owned_canvas_(),
nit: remove this entirely.

https://codereview.chromium.org/11299262/diff/1019/ui/gfx/canvas.cc#newcode34
ui/gfx/canvas.cc:34: owned_canvas_ =
skia::AdoptRef(skia::CreatePlatformCanvas(pixel_size.width(),
Why do we need the extra AdoptRef step? Can't we just assign?

[danakj, 2012-11-30 00:12:21]

https://codereview.chromium.org/11299262/diff/1019/chrome/browser/ui/views/de...
File chrome/browser/ui/views/detachable_toolbar_view.cc (right):

https://codereview.chromium.org/11299262/diff/1019/chrome/browser/ui/views/de...
chrome/browser/ui/views/detachable_toolbar_view.cc:122: skia::RefPtr<SkShader>
shader;
On 2012/11/29 23:54:56, sky wrote:
> Is there a reason for keeping this here and not at 126 where you create it?

Nope, will move it.

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

https://codereview.chromium.org/11299262/diff/1019/ui/gfx/canvas.cc#newcode30
ui/gfx/canvas.cc:30: owned_canvas_(),
On 2012/11/29 23:54:56, sky wrote:
> nit: remove this entirely.

Removing owned_canvas is tricky, and makes this CL much larger. Because
sometimes it is empty even tho canvas_ is not.

I can do that though, is that what you want to see?

https://codereview.chromium.org/11299262/diff/1019/ui/gfx/canvas.cc#newcode34
ui/gfx/canvas.cc:34: owned_canvas_ =
skia::AdoptRef(skia::CreatePlatformCanvas(pixel_size.width(),
On 2012/11/29 23:54:56, sky wrote:
> Why do we need the extra AdoptRef step? Can't we just assign?

skia::CreatePlatformCanvas() returns an SkCanvas* still. We need some 3-sided
WebKit patching to fix it.

[sky, 2012-11-30 01:31:47]

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

https://codereview.chromium.org/11299262/diff/1019/ui/gfx/canvas.cc#newcode30
ui/gfx/canvas.cc:30: owned_canvas_(),
On 2012/11/30 00:12:21, danakj wrote:
> On 2012/11/29 23:54:56, sky wrote:
> > nit: remove this entirely.
> 
> Removing owned_canvas is tricky, and makes this CL much larger. Because
> sometimes it is empty even tho canvas_ is not.
> 
> I can do that though, is that what you want to see?

I actually just meant you can remove owned_canvas_ from the member initializer.

[danakj, 2012-11-30 01:32:39]

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

https://codereview.chromium.org/11299262/diff/1019/ui/gfx/canvas.cc#newcode30
ui/gfx/canvas.cc:30: owned_canvas_(),
On 2012/11/30 01:31:47, sky wrote:
> On 2012/11/30 00:12:21, danakj wrote:
> > On 2012/11/29 23:54:56, sky wrote:
> > > nit: remove this entirely.
> > 
> > Removing owned_canvas is tricky, and makes this CL much larger. Because
> > sometimes it is empty even tho canvas_ is not.
> > 
> > I can do that though, is that what you want to see?
> 
> I actually just meant you can remove owned_canvas_ from the member
initializer.

OOhh! ok :)

[danakj, 2012-11-30 01:36:28]

PTAL, done everything.

[sky, 2012-12-03 15:28:49]

https://codereview.chromium.org/11299262/diff/4002/ui/gfx/canvas_mac.mm
File ui/gfx/canvas_mac.mm (right):

https://codereview.chromium.org/11299262/diff/4002/ui/gfx/canvas_mac.mm#newco...
ui/gfx/canvas_mac.mm:62: skia::RefPtr<SkTypeface> typeface =
SkTypeface::CreateFromName(
Is it just me, or is it very mysterious as to when you AdoptRef and when you
don't?

. gfx::CreateGradientShader and skia::CreatePlatformCanvas you don't need to.
. skia::CreatePlatformCanvas you do.
. SkTypeface::CreateFromName you don't.

Can we make all these consistant? If not, I would hope that we're at least
consistent in a particular namespace.

[danakj, 2012-12-03 17:00:19]

https://codereview.chromium.org/11299262/diff/4002/ui/gfx/canvas_mac.mm
File ui/gfx/canvas_mac.mm (right):

https://codereview.chromium.org/11299262/diff/4002/ui/gfx/canvas_mac.mm#newco...
ui/gfx/canvas_mac.mm:62: skia::RefPtr<SkTypeface> typeface =
SkTypeface::CreateFromName(
On 2012/12/03 15:28:49, sky wrote:
> Is it just me, or is it very mysterious as to when you AdoptRef and when you
> don't?
> 
> . gfx::CreateGradientShader and skia::CreatePlatformCanvas you don't need to.
> . skia::CreatePlatformCanvas you do.
> . SkTypeface::CreateFromName you don't.
> 
> Can we make all these consistant? If not, I would hope that we're at least
> consistent in a particular namespace.

Oh, this should adopt. It's based on an old CL, and I can't throw this at bots
until the first one lands. I will fix.

The adopt-fu is much less mysterious, when you create a new skia object and it
returns a pointer, you adopt it there.

I've begun converting the skia:: apis to work with refptrs instead of raw
pointers whenever passing ownership, but CreatePlatformCanvas is tricky since it
is used in WebKit as well, so I didn't change it yet. It still returns a new raw
pointer, so it needs to be adopted by the caller.

[sky, 2012-12-03 18:08:05]

LGTM

[commit-bot: I haz the power, 2012-12-04 00:39:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/11299262/12003

[commit-bot: I haz the power, 2012-12-04 00:39:50]

Failed to apply patch for chrome/renderer/print_web_view_helper_linux.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/renderer/print_web_view_helper_linux.cc
  Hunk #1 FAILED at 16.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/renderer/print_web_view_helper_linux.cc.rej

Patch:       chrome/renderer/print_web_view_helper_linux.cc
Index: chrome/renderer/print_web_view_helper_linux.cc
diff --git a/chrome/renderer/print_web_view_helper_linux.cc
b/chrome/renderer/print_web_view_helper_linux.cc
index
ec7af6128e213b46a9094f5f15107586e70cabce..ca1de1e595ad0824336edb5c994e81a834d3e280
100644
--- a/chrome/renderer/print_web_view_helper_linux.cc
+++ b/chrome/renderer/print_web_view_helper_linux.cc
@@ -16,7 +16,6 @@
 #include "printing/page_size_margins.h"
 #include "skia/ext/platform_device.h"
 #include "skia/ext/vector_canvas.h"
-#include "third_party/skia/include/core/SkRefCnt.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebFrame.h"
 
 #if !defined(OS_CHROMEOS)

[commit-bot: I haz the power, 2012-12-05 00:16:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/11299262/7019

[commit-bot: I haz the power, 2012-12-05 01:00:56]

Change committed as 171107