Change (Pass)RefPtr<SkXxx> into sk_sp<SkXxx>.

Change (Pass)RefPtr<SkXxx> into sk_sp<SkXxx>.

This CL is motivated by 2 things:
1. General desire to use sk_sp<...> instead of RefPtr<...>
   (i.e. as given in a comment next to the fromSkSp/toSkSp functions)
2. Desire to allow rename_to_chrome_style tool to rename
   WTF::RefCountedBase::ref to Ref (while not renaming |ref| to |Ref|
   in Skia).

This CL contains the following broadly applied changes:
- s/\(Pass\|\)RefPtr<Sk/sk_sp<Sk/g
- Repeat the above for |const Sk| (i.e. const-qualified template parameter)
- Repeat the above for |WTF::RefPtr<Sk| (i.e. explicit namespace qualifier)
- Add #include "third_party/skia/include/core/SkRefCnt.h" if appropriate
- Replace calls to RefPtr<T>::clear() with calls to sk_sp<T>::reset()
- Replace calls to PassRefPtr<T>::release() with calls to std::move(x)
- Replace calls to adoptRef with calls to sk_sp<T>(T*) constructor
  or calls to sk_sp<T>::reset(T*) - both will adopt a ref-count
- Remove (now unnecessary) fromSkSp and toSkSp calls
- Add more std::move calls during a self review

This CL contains the following one-off changes:
- Tweak platform/CrossThreadCopier.h to mark sk_sp<SkRefCnt> as thread-safe
- Remove fromSkSp and toSkSp definitions from
  platform/graphics/skia/SkiaUtils.h

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

Committed: https://crrev.com/69969af74bfff920f5fdcc2aa547ea4af2d477d2
Cr-Commit-Position: refs/heads/master@{#416323}

[Łukasz Anforowicz, 2016-08-29 23:51:43]

Description was changed from

==========
Change (Pass)RefPtr<SkXxx> into sk_sp<SkXxx>.

This CL is motivated by 2 things:
1. General desire to use sk_sp<...> instead of RefPtr<...>
   (i.e. as given in a comment next to the fromSkSp/toSkSp functions)
2. Desire to allow rename_to_chrome_style tool to rename
   WTF::RefCountedBase::ref to Ref (while not renaming |ref| to |Ref|
   in Skia).

This CL contains the following broadly applied changes:
- s/\(Pass\|\)RefPtr<Sk/sk_sp<Sk/g
- Repeat the above for |const Sk| (i.e. const-qualified template parameter)
- Repeat the above for |WTF::RefPtr<Sk| (i.e. explicit namespace qualifier)
- Add #include "third_party/skia/include/core/SkRefCnt.h" if appropriate
- Replace calls to RefPtr<T>::clear() with calls to sk_sp<T>::reset()
- Replace calls to PassRefPtr<T>::release() with calls to std::move(x)
- Replace calls to adoptRef with calls to sk_sp<T>(T*) constructor
  or calls to sk_sp<T>::reset(T*) - both will adopt a ref-count
- Remove (now unnecessary) fromSkSp and toSkSp calls
- Add more std::move calls during a self review

This CL contains the following one-off changes:
- Tweak platform/CrossThreadCopier.h to mark sk_sp<T> as thread-safe
- Remove fromSkSp and toSkSp definitions from
  platform/graphics/skia/SkiaUtils.h

BUG=641014
==========

to

==========
Change (Pass)RefPtr<SkXxx> into sk_sp<SkXxx>.

This CL is motivated by 2 things:
1. General desire to use sk_sp<...> instead of RefPtr<...>
   (i.e. as given in a comment next to the fromSkSp/toSkSp functions)
2. Desire to allow rename_to_chrome_style tool to rename
   WTF::RefCountedBase::ref to Ref (while not renaming |ref| to |Ref|
   in Skia).

This CL contains the following broadly applied changes:
- s/\(Pass\|\)RefPtr<Sk/sk_sp<Sk/g
- Repeat the above for |const Sk| (i.e. const-qualified template parameter)
- Repeat the above for |WTF::RefPtr<Sk| (i.e. explicit namespace qualifier)
- Add #include "third_party/skia/include/core/SkRefCnt.h" if appropriate
- Replace calls to RefPtr<T>::clear() with calls to sk_sp<T>::reset()
- Replace calls to PassRefPtr<T>::release() with calls to std::move(x)
- Replace calls to adoptRef with calls to sk_sp<T>(T*) constructor
  or calls to sk_sp<T>::reset(T*) - both will adopt a ref-count
- Remove (now unnecessary) fromSkSp and toSkSp calls
- Add more std::move calls during a self review

This CL contains the following one-off changes:
- Tweak platform/CrossThreadCopier.h to mark sk_sp<T> as thread-safe
- Remove fromSkSp and toSkSp definitions from
  platform/graphics/skia/SkiaUtils.h

BUG=641014
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[Łukasz Anforowicz, 2016-08-29 23:51:44]

Description was changed from

==========
Change (Pass)RefPtr<SkXxx> into sk_sp<SkXxx>.

This CL is motivated by 2 things:
1. General desire to use sk_sp<...> instead of RefPtr<...>
   (i.e. as given in a comment next to the fromSkSp/toSkSp functions)
2. Desire to allow rename_to_chrome_style tool to rename
   WTF::RefCountedBase::ref to Ref (while not renaming |ref| to |Ref|
   in Skia).

This CL contains the following broadly applied changes:
- s/\(Pass\|\)RefPtr<Sk/sk_sp<Sk/g
- Repeat the above for |const Sk| (i.e. const-qualified template parameter)
- Repeat the above for |WTF::RefPtr<Sk| (i.e. explicit namespace qualifier)
- Add #include "third_party/skia/include/core/SkRefCnt.h" if appropriate
- Replace calls to RefPtr<T>::clear() with calls to sk_sp<T>::reset()
- Replace calls to PassRefPtr<T>::release() with calls to std::move(x)
- Replace calls to adoptRef with calls to sk_sp<T>(T*) constructor
  or calls to sk_sp<T>::reset(T*) - both will adopt a ref-count
- Remove (now unnecessary) fromSkSp and toSkSp calls
- Add more std::move calls during a self review

This CL contains the following one-off changes:
- Tweak platform/CrossThreadCopier.h to mark sk_sp<T> as thread-safe
- Remove fromSkSp and toSkSp definitions from
  platform/graphics/skia/SkiaUtils.h

BUG=641014
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Change (Pass)RefPtr<SkXxx> into sk_sp<SkXxx>.

This CL is motivated by 2 things:
1. General desire to use sk_sp<...> instead of RefPtr<...>
   (i.e. as given in a comment next to the fromSkSp/toSkSp functions)
2. Desire to allow rename_to_chrome_style tool to rename
   WTF::RefCountedBase::ref to Ref (while not renaming |ref| to |Ref|
   in Skia).

This CL contains the following broadly applied changes:
- s/\(Pass\|\)RefPtr<Sk/sk_sp<Sk/g
- Repeat the above for |const Sk| (i.e. const-qualified template parameter)
- Repeat the above for |WTF::RefPtr<Sk| (i.e. explicit namespace qualifier)
- Add #include "third_party/skia/include/core/SkRefCnt.h" if appropriate
- Replace calls to RefPtr<T>::clear() with calls to sk_sp<T>::reset()
- Replace calls to PassRefPtr<T>::release() with calls to std::move(x)
- Replace calls to adoptRef with calls to sk_sp<T>(T*) constructor
  or calls to sk_sp<T>::reset(T*) - both will adopt a ref-count
- Remove (now unnecessary) fromSkSp and toSkSp calls
- Add more std::move calls during a self review

This CL contains the following one-off changes:
- Tweak platform/CrossThreadCopier.h to mark sk_sp<T> as thread-safe
- Remove fromSkSp and toSkSp definitions from
  platform/graphics/skia/SkiaUtils.h

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

[Łukasz Anforowicz, 2016-08-30 21:52:58]

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

[commit-bot: I haz the power, 2016-08-30 21:53:48]

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-08-30 22:07:33]

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

[commit-bot: I haz the power, 2016-08-30 22:07:34]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Łukasz Anforowicz, 2016-08-30 23:12:35]

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

[commit-bot: I haz the power, 2016-08-30 23:13:03]

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-08-30 23:54:11]

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

[commit-bot: I haz the power, 2016-08-30 23:54:12]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Łukasz Anforowicz, 2016-08-31 00:58:44]

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

[commit-bot: I haz the power, 2016-08-31 00:59:13]

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-08-31 01:10:20]

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

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

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Łukasz Anforowicz, 2016-08-31 01:49:56]

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

[commit-bot: I haz the power, 2016-08-31 01:50:13]

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-08-31 02:33:51]

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

[commit-bot: I haz the power, 2016-08-31 02:33:52]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Łukasz Anforowicz, 2016-08-31 04:08:22]

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

[commit-bot: I haz the power, 2016-08-31 04:08:40]

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-08-31 05:46:08]

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

[commit-bot: I haz the power, 2016-08-31 05:46:09]

Dry run: This issue passed the CQ dry run.

[Łukasz Anforowicz, 2016-08-31 16:13:45]

Description was changed from

==========
Change (Pass)RefPtr<SkXxx> into sk_sp<SkXxx>.

This CL is motivated by 2 things:
1. General desire to use sk_sp<...> instead of RefPtr<...>
   (i.e. as given in a comment next to the fromSkSp/toSkSp functions)
2. Desire to allow rename_to_chrome_style tool to rename
   WTF::RefCountedBase::ref to Ref (while not renaming |ref| to |Ref|
   in Skia).

This CL contains the following broadly applied changes:
- s/\(Pass\|\)RefPtr<Sk/sk_sp<Sk/g
- Repeat the above for |const Sk| (i.e. const-qualified template parameter)
- Repeat the above for |WTF::RefPtr<Sk| (i.e. explicit namespace qualifier)
- Add #include "third_party/skia/include/core/SkRefCnt.h" if appropriate
- Replace calls to RefPtr<T>::clear() with calls to sk_sp<T>::reset()
- Replace calls to PassRefPtr<T>::release() with calls to std::move(x)
- Replace calls to adoptRef with calls to sk_sp<T>(T*) constructor
  or calls to sk_sp<T>::reset(T*) - both will adopt a ref-count
- Remove (now unnecessary) fromSkSp and toSkSp calls
- Add more std::move calls during a self review

This CL contains the following one-off changes:
- Tweak platform/CrossThreadCopier.h to mark sk_sp<T> as thread-safe
- Remove fromSkSp and toSkSp definitions from
  platform/graphics/skia/SkiaUtils.h

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

to

==========
Change (Pass)RefPtr<SkXxx> into sk_sp<SkXxx>.

This CL is motivated by 2 things:
1. General desire to use sk_sp<...> instead of RefPtr<...>
   (i.e. as given in a comment next to the fromSkSp/toSkSp functions)
2. Desire to allow rename_to_chrome_style tool to rename
   WTF::RefCountedBase::ref to Ref (while not renaming |ref| to |Ref|
   in Skia).

This CL contains the following broadly applied changes:
- s/\(Pass\|\)RefPtr<Sk/sk_sp<Sk/g
- Repeat the above for |const Sk| (i.e. const-qualified template parameter)
- Repeat the above for |WTF::RefPtr<Sk| (i.e. explicit namespace qualifier)
- Add #include "third_party/skia/include/core/SkRefCnt.h" if appropriate
- Replace calls to RefPtr<T>::clear() with calls to sk_sp<T>::reset()
- Replace calls to PassRefPtr<T>::release() with calls to std::move(x)
- Replace calls to adoptRef with calls to sk_sp<T>(T*) constructor
  or calls to sk_sp<T>::reset(T*) - both will adopt a ref-count
- Remove (now unnecessary) fromSkSp and toSkSp calls
- Add more std::move calls during a self review

This CL contains the following one-off changes:
- Tweak platform/CrossThreadCopier.h to mark sk_sp<SkRefCnt> as thread-safe
- Remove fromSkSp and toSkSp definitions from
  platform/graphics/skia/SkiaUtils.h

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

[Łukasz Anforowicz, 2016-08-31 17:02:30]

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

[commit-bot: I haz the power, 2016-08-31 17:02:55]

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

[Łukasz Anforowicz, 2016-08-31 17:08:42]

lukasza@chromium.org changed reviewers:
+ fmalita@chromium.org

[Łukasz Anforowicz, 2016-08-31 17:08:44]

fmalita@, could you please take a look?

[Stephen Chennney, 2016-08-31 17:21:04]

Description was changed from

==========
Change (Pass)RefPtr<SkXxx> into sk_sp<SkXxx>.

This CL is motivated by 2 things:
1. General desire to use sk_sp<...> instead of RefPtr<...>
   (i.e. as given in a comment next to the fromSkSp/toSkSp functions)
2. Desire to allow rename_to_chrome_style tool to rename
   WTF::RefCountedBase::ref to Ref (while not renaming |ref| to |Ref|
   in Skia).

This CL contains the following broadly applied changes:
- s/\(Pass\|\)RefPtr<Sk/sk_sp<Sk/g
- Repeat the above for |const Sk| (i.e. const-qualified template parameter)
- Repeat the above for |WTF::RefPtr<Sk| (i.e. explicit namespace qualifier)
- Add #include "third_party/skia/include/core/SkRefCnt.h" if appropriate
- Replace calls to RefPtr<T>::clear() with calls to sk_sp<T>::reset()
- Replace calls to PassRefPtr<T>::release() with calls to std::move(x)
- Replace calls to adoptRef with calls to sk_sp<T>(T*) constructor
  or calls to sk_sp<T>::reset(T*) - both will adopt a ref-count
- Remove (now unnecessary) fromSkSp and toSkSp calls
- Add more std::move calls during a self review

This CL contains the following one-off changes:
- Tweak platform/CrossThreadCopier.h to mark sk_sp<SkRefCnt> as thread-safe
- Remove fromSkSp and toSkSp definitions from
  platform/graphics/skia/SkiaUtils.h

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

to

==========
Change (Pass)RefPtr<SkXxx> into sk_sp<SkXxx>.

This CL is motivated by 2 things:
1. General desire to use sk_sp<...> instead of RefPtr<...>
   (i.e. as given in a comment next to the fromSkSp/toSkSp functions)
2. Desire to allow rename_to_chrome_style tool to rename
   WTF::RefCountedBase::ref to Ref (while not renaming |ref| to |Ref|
   in Skia).

This CL contains the following broadly applied changes:
- s/\(Pass\|\)RefPtr<Sk/sk_sp<Sk/g
- Repeat the above for |const Sk| (i.e. const-qualified template parameter)
- Repeat the above for |WTF::RefPtr<Sk| (i.e. explicit namespace qualifier)
- Add #include "third_party/skia/include/core/SkRefCnt.h" if appropriate
- Replace calls to RefPtr<T>::clear() with calls to sk_sp<T>::reset()
- Replace calls to PassRefPtr<T>::release() with calls to std::move(x)
- Replace calls to adoptRef with calls to sk_sp<T>(T*) constructor
  or calls to sk_sp<T>::reset(T*) - both will adopt a ref-count
- Remove (now unnecessary) fromSkSp and toSkSp calls
- Add more std::move calls during a self review

This CL contains the following one-off changes:
- Tweak platform/CrossThreadCopier.h to mark sk_sp<SkRefCnt> as thread-safe
- Remove fromSkSp and toSkSp definitions from
  platform/graphics/skia/SkiaUtils.h

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

[Stephen Chennney, 2016-08-31 17:21:04]

schenney@chromium.org changed reviewers:
+ jbroman@chromium.org

[haraken, 2016-08-31 17:28:01]

I support this change from the perspective of Blink's architecture.

[Justin Novosad, 2016-08-31 17:31:15]

sweet! \o/

lgtm

[Stephen Chennney, 2016-08-31 17:35:16]

schenney@chromium.org changed reviewers:
+ schenney@chromium.org

[Stephen Chennney, 2016-08-31 17:35:18]

Not LGTM, at least not yet.

I recall an explicit conversation about ref counting Skia objects in Blink, and
believe the current use of RefPtr and PassRefPtr is deliberate. Maybe jbroman@
can add more info?

Making a permanent change like this to make it easier to do a one-off style
conversion is not something I'm happy with. Now a huge number of people need to
understand Skia's ref counting behavior even when not dealing with Skia APIs.

[dcheng, 2016-08-31 17:40:08]

On 2016/08/31 17:35:18, Stephen Chennney wrote:
> Not LGTM, at least not yet.
> 
> I recall an explicit conversation about ref counting Skia objects in Blink,
and
> believe the current use of RefPtr and PassRefPtr is deliberate. Maybe jbroman@
> can add more info?
> 
> Making a permanent change like this to make it easier to do a one-off style
> conversion is not something I'm happy with. Now a huge number of people need
to
> understand Skia's ref counting behavior even when not dealing with Skia APIs.

1) I'm not sure what this means: what's special about Skia's ref counting
behavior here?
2) Does scoped_refptr work for Skia objects in Chromium? I don't think it does:
assuming this is true, I'd argue that using the same smart pointer type to hold
Skia types is a net improvement.

[haraken, 2016-08-31 17:53:36]

On 2016/08/31 17:40:08, dcheng wrote:
> On 2016/08/31 17:35:18, Stephen Chennney wrote:
> > Not LGTM, at least not yet.
> > 
> > I recall an explicit conversation about ref counting Skia objects in Blink,
> and
> > believe the current use of RefPtr and PassRefPtr is deliberate. Maybe
jbroman@
> > can add more info?
> > 
> > Making a permanent change like this to make it easier to do a one-off style
> > conversion is not something I'm happy with. Now a huge number of people need
> to
> > understand Skia's ref counting behavior even when not dealing with Skia
APIs.
> 
> 1) I'm not sure what this means: what's special about Skia's ref counting
> behavior here?
> 2) Does scoped_refptr work for Skia objects in Chromium? I don't think it
does:
> assuming this is true, I'd argue that using the same smart pointer type to
hold
> Skia types is a net improvement.

Also I think we have already converted RefPtrs in cc/ and ui/gfx/ with sk_sp.

[jbroman, 2016-08-31 18:10:19]

On 2016/08/31 at 17:35:18, schenney wrote:
> Not LGTM, at least not yet.
> 
> I recall an explicit conversation about ref counting Skia objects in Blink,
and believe the current use of RefPtr and PassRefPtr is deliberate. Maybe
jbroman@ can add more info?

Most of that discussion predates Skia even having a smart pointer type, where
the choice was between SkFoo* and RefPtr<SkFoo>. Now that Skia has seen the
smart-pointer light, switching to sk_sp<> seems reasonable, especially since
you'll need to deal with sk_sp at the API boundary with Skia anyhow.

I think Florin has started making such changes as this incrementally already.

> Making a permanent change like this to make it easier to do a one-off style
conversion is not something I'm happy with. Now a huge number of people need to
understand Skia's ref counting behavior even when not dealing with Skia APIs.

This change seems reasonable to me overall (assuming the mechanical changes are
correct; I haven't reviewed them).

[Stephen Chennney, 2016-08-31 18:15:30]

On 2016/08/31 17:53:36, haraken wrote:
> On 2016/08/31 17:40:08, dcheng wrote:
> > On 2016/08/31 17:35:18, Stephen Chennney wrote:
> > > Not LGTM, at least not yet.
> > > 
> > > I recall an explicit conversation about ref counting Skia objects in
Blink,
> > and
> > > believe the current use of RefPtr and PassRefPtr is deliberate. Maybe
> jbroman@
> > > can add more info?
> > > 
> > > Making a permanent change like this to make it easier to do a one-off
style
> > > conversion is not something I'm happy with. Now a huge number of people
need
> > to
> > > understand Skia's ref counting behavior even when not dealing with Skia
> APIs.
> > 
> > 1) I'm not sure what this means: what's special about Skia's ref counting
> > behavior here?

In some sense the question is exactly why I think there's cognitive overhead
to working with Skia ref counting when living almost exclusively in Blink land.
I don't know what's special, so I have to go and find out before I can make a
change. And I'm not likely to have to know often enough to keep my knowledge
up to date (although I would need to know more often if this lands).

> Also I think we have already converted RefPtrs in cc/ and ui/gfx/ with sk_sp.

Can someone confirm this? It's probably enough to change my mind.

[commit-bot: I haz the power, 2016-08-31 18:53:34]

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

[commit-bot: I haz the power, 2016-08-31 18:53:36]

Dry run: This issue passed the CQ dry run.

[Łukasz Anforowicz, 2016-08-31 19:08:10]

On 2016/08/31 18:15:30, Stephen Chennney wrote:
> On 2016/08/31 17:53:36, haraken wrote:
> > On 2016/08/31 17:40:08, dcheng wrote:
> > > On 2016/08/31 17:35:18, Stephen Chennney wrote:
> > > > Not LGTM, at least not yet.
> > > > 
> > > > I recall an explicit conversation about ref counting Skia objects in
> Blink,
> > > and
> > > > believe the current use of RefPtr and PassRefPtr is deliberate. Maybe
> > jbroman@
> > > > can add more info?
> > > > 
> > > > Making a permanent change like this to make it easier to do a one-off
> style
> > > > conversion is not something I'm happy with. Now a huge number of people
> need
> > > to
> > > > understand Skia's ref counting behavior even when not dealing with Skia
> > APIs.
> > > 
> > > 1) I'm not sure what this means: what's special about Skia's ref counting
> > > behavior here?
> 
> In some sense the question is exactly why I think there's cognitive overhead
> to working with Skia ref counting when living almost exclusively in Blink
land.
> I don't know what's special, so I have to go and find out before I can make a
> change. And I'm not likely to have to know often enough to keep my knowledge
> up to date (although I would need to know more often if this lands).
> 
> > Also I think we have already converted RefPtrs in cc/ and ui/gfx/ with
sk_sp.
> 
> Can someone confirm this? It's probably enough to change my mind.

There was https://codereview.chromium.org/1939143002 that touched cc/ and
ui/gfx/ and that changed skia::RefPtr<Sk...> to sk_sp<Sk...>.

[Stephen Chennney, 2016-08-31 20:32:31]

On 2016/08/31 19:08:10, Łukasz Anforowicz wrote:
> On 2016/08/31 18:15:30, Stephen Chennney wrote:
> > On 2016/08/31 17:53:36, haraken wrote:
> > > On 2016/08/31 17:40:08, dcheng wrote:
> > > > On 2016/08/31 17:35:18, Stephen Chennney wrote:
> > > > > Not LGTM, at least not yet.
> > > > > 
> > > > > I recall an explicit conversation about ref counting Skia objects in
> > Blink,
> > > > and
> > > > > believe the current use of RefPtr and PassRefPtr is deliberate. Maybe
> > > jbroman@
> > > > > can add more info?
> > > > > 
> > > > > Making a permanent change like this to make it easier to do a one-off
> > style
> > > > > conversion is not something I'm happy with. Now a huge number of
people
> > need
> > > > to
> > > > > understand Skia's ref counting behavior even when not dealing with
Skia
> > > APIs.
> > > > 
> > > > 1) I'm not sure what this means: what's special about Skia's ref
counting
> > > > behavior here?
> > 
> > In some sense the question is exactly why I think there's cognitive overhead
> > to working with Skia ref counting when living almost exclusively in Blink
> land.
> > I don't know what's special, so I have to go and find out before I can make
a
> > change. And I'm not likely to have to know often enough to keep my knowledge
> > up to date (although I would need to know more often if this lands).
> > 
> > > Also I think we have already converted RefPtrs in cc/ and ui/gfx/ with
> sk_sp.
> > 
> > Can someone confirm this? It's probably enough to change my mind.
> 
> There was https://codereview.chromium.org/1939143002 that touched cc/ and
> ui/gfx/ and that changed skia::RefPtr<Sk...> to sk_sp<Sk...>.

LGTM to cancel my "not" because I'll be out tomorrow, but I still would strongly
prefer you get an lgtm from fmalita@ and someone else on the paint team.

From an architectural perspective my concern is that we don't bring Skia types
deep
into the blink stack. In evaluating this I believe we should ignore the "this
will
make code style conversion easier" and focus on long-term reasons.

[Łukasz Anforowicz, 2016-08-31 20:58:17]

On 2016/08/31 20:32:31, Stephen Chennney wrote:
> On 2016/08/31 19:08:10, Łukasz Anforowicz wrote:
> > On 2016/08/31 18:15:30, Stephen Chennney wrote:
> > > On 2016/08/31 17:53:36, haraken wrote:
> > > > On 2016/08/31 17:40:08, dcheng wrote:
> > > > > On 2016/08/31 17:35:18, Stephen Chennney wrote:
> > > > > > Not LGTM, at least not yet.
> > > > > > 
> > > > > > I recall an explicit conversation about ref counting Skia objects in
> > > Blink,
> > > > > and
> > > > > > believe the current use of RefPtr and PassRefPtr is deliberate.
Maybe
> > > > jbroman@
> > > > > > can add more info?
> > > > > > 
> > > > > > Making a permanent change like this to make it easier to do a
one-off
> > > style
> > > > > > conversion is not something I'm happy with. Now a huge number of
> people
> > > need
> > > > > to
> > > > > > understand Skia's ref counting behavior even when not dealing with
> Skia
> > > > APIs.
> > > > > 
> > > > > 1) I'm not sure what this means: what's special about Skia's ref
> counting
> > > > > behavior here?
> > > 
> > > In some sense the question is exactly why I think there's cognitive
overhead
> > > to working with Skia ref counting when living almost exclusively in Blink
> > land.
> > > I don't know what's special, so I have to go and find out before I can
make
> a
> > > change. And I'm not likely to have to know often enough to keep my
knowledge
> > > up to date (although I would need to know more often if this lands).
> > > 
> > > > Also I think we have already converted RefPtrs in cc/ and ui/gfx/ with
> > sk_sp.
> > > 
> > > Can someone confirm this? It's probably enough to change my mind.
> > 
> > There was https://codereview.chromium.org/1939143002 that touched cc/ and
> > ui/gfx/ and that changed skia::RefPtr<Sk...> to sk_sp<Sk...>.
> 
> LGTM to cancel my "not" because I'll be out tomorrow, but I still would
strongly
> prefer you get an lgtm from fmalita@ and someone else on the paint team.

Yes - I still need a review from fmalita@.  Not only to get an ack on the
overall direction of the changes, but also to have somebody look at the details
of this particular change - the change is not entirely mechanical and I will
appreciate a second set of eyes to double-check everything.  In particular - I
think the following places deserve extra scrutiny (although I do hope that I got
them all during self-review :-):

- places where sk_ref_sp is added
- places where adoptRef is replaced with sk_sp constructor and/or call to
sk_sp<T>::reset
- places where raw pointers are still used (i.e. some transitions from s_foo to
m_foo)

> From an architectural perspective my concern is that we don't bring Skia types
> deep
> into the blink stack. In evaluating this I believe we should ignore the "this
> will
> make code style conversion easier" and focus on long-term reasons.

Agreed :-)  OTOH, I think that the (relatively unimportant) conversion trouble
does highlight the architectural problems with using wtf::RefPtr to manage Skia
objects in the (more important) long term (i.e. IMO with correct layering, we
should be free to rename internal Blink code at any time;  IMO usage of
wtf::RefPtr<T> should be limited to wtf::RefCountedBase objects - wtf::RefPtr<T>
should be immune to changes in Skia and/or base::RefCountedBase semantics/API).

[pdr., 2016-08-31 21:00:38]

On 2016/08/31 at 20:32:31, schenney wrote:
> On 2016/08/31 19:08:10, Łukasz Anforowicz wrote:

One of Stephen's concerns is not knowing which type to use. Are there any real
differences between RefPtr and sk_sp that would surprise folks? One requires
RefCountedBase, the other SkRefCntBase, but they seem the same to me otherwise.
Is it fair to make a blanket statement like "blindly use sk_sp for skia types
(SkRefCntBase) and RefPtr for blink types (RefCountedBase)"?

[dcheng, 2016-08-31 21:36:59]

On 2016/08/31 21:00:38, pdr. wrote:
> On 2016/08/31 at 20:32:31, schenney wrote:
> > On 2016/08/31 19:08:10, Łukasz Anforowicz wrote:
> 
> One of Stephen's concerns is not knowing which type to use. Are there any real
> differences between RefPtr and sk_sp that would surprise folks? One requires
> RefCountedBase, the other SkRefCntBase, but they seem the same to me
otherwise.
> Is it fair to make a blanket statement like "blindly use sk_sp for skia types
> (SkRefCntBase) and RefPtr for blink types (RefCountedBase)"?

I think the main surprise here is that there's no PassRefPtr equivalent in
sk_sp: you have to remember to std::move() to avoid refcount churn.
However, this is no different from where Blink is headed: we're getting rid of
PassRefPtr and just using move semantics directly with RefPtr.

Once the rename is completed, the blanket statement will be enforced by virtue
of the fact that SkRefCntBase and RefCountedBase will no longer share the same
casing for the ref / unref methods.

[pdr., 2016-08-31 21:44:21]

On 2016/08/31 at 21:36:59, dcheng wrote:
> On 2016/08/31 21:00:38, pdr. wrote:
> > On 2016/08/31 at 20:32:31, schenney wrote:
> > > On 2016/08/31 19:08:10, Łukasz Anforowicz wrote:
> > 
> > One of Stephen's concerns is not knowing which type to use. Are there any
real
> > differences between RefPtr and sk_sp that would surprise folks? One requires
> > RefCountedBase, the other SkRefCntBase, but they seem the same to me
otherwise.
> > Is it fair to make a blanket statement like "blindly use sk_sp for skia
types
> > (SkRefCntBase) and RefPtr for blink types (RefCountedBase)"?
> 
> I think the main surprise here is that there's no PassRefPtr equivalent in
sk_sp: you have to remember to std::move() to avoid refcount churn.
> However, this is no different from where Blink is headed: we're getting rid of
PassRefPtr and just using move semantics directly with RefPtr.
> 
> Once the rename is completed, the blanket statement will be enforced by virtue
of the fact that SkRefCntBase and RefCountedBase will no longer share the same
casing for the ref / unref methods.

That sounds reasonable to me, LGTM here.

I'd like to give Stephen a chance to respond though. Do you mind waiting until
tomorrow?

[f(malita), 2016-09-01 03:55:39]

Initially, we tried not to push sk_sps too aggressively in Blink, mostly for the
reasons Stephen mentioned and because Blink's smart pointers were already
compatible with Skia types.  The idea was to ease sk_sps in, where they made
sense (API boundaries), and not try too hard to plumb them across layers.

(the story was different for the rest of Chromium which was relying on
Skia-specific wrappers -- skia::RefPtr -- so that became an immediate/obvious
target)

But in time it became clear that this hybrid approach has some drawbacks, and
bouncing back-and-forth between Skia's smart pointers and Blink's creates some
unneeded friction -- which could be avoided by sticking to a single smart ptr
domain.  And since all Skia-allocated objects ultimately end up consumed by
Skia, sk_sp is the obvious choice.

So I'm on board with this direction, and we've been gradually heading that way
for a while.

My only objection is related to the size of the change - I would have preferred
smaller, more focused patches.  But since you have other good reasons for
getting RefPtr Skia wrappers out of the way, and have already made the effort, I
will gratefully take the CL in its current form.  Thanks for tackling this
inglorious task :)

Tangentially, we should also de-constify all Skia RefPtrs (now sk_sps) at some
point.  All the types we're wrapping are inherently immutable (SkImage,
SkPicture, SkTextBlob) -- so const-qualifying doesn't add any value, but it
introduces some friction when hitting Skia APIs.  E.g. we're hauling sk_sp<const
SkPicture> around, but SkShader::MakePictureShader() wants a sk_sp<SkPicture> so
we can't seamlessly move into it, or we can't call SkCanvas::drawPicture(const
sk_sp<SkPicture>&) and must use the raw version instead, etc.).  Definitely not
in this CL though :)

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right):

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:310:
surface->getCanvas()->drawImageRect(skiaImage.get(), drawSrcRect, drawDstRect,
&paint);
Nit: now that skiaImage is an sk_sp<SkImage>, there's no reason to use .get()
for the old style/raw ptr API anymore - you can use drawImageRect(const
sk_sp<SkImage>&, ...) instead.

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:312:
surface->getCanvas()->drawImage(skiaImage.get(), dstLeft, dstTop);
Ditto.

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:343:
surface->getCanvas()->drawImage(skImage.get(), 0, 0);
Ditto.

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:464:
listener->sendNewFrame(image);
Not new, but it would make sense to std::move(image) here.

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/SVGClipPainter.cpp (right):

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGClipPainter.cpp:142:
context.drawPicture(maskPicture.get());
Nit: .get() not needed anymore.

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/imagebitmap/ImageBitmapRenderingContext.cpp
(right):

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/imagebitmap/ImageBitmapRenderingContext.cpp:47:
surface->getCanvas()->drawImage(skImage.get(), 0, 0);
Nit: drop the .get(). it's cleaner.

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/DragImage.cpp (right):

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/DragImage.cpp:111:
canvas->drawImage(image.get(), 0, 0, &paint);
Ditto.

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/FontPlatformData.cpp (right):

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/FontPlatformData.cpp:137: :
m_typeface(tf)
std::move(tf)

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/TextBlob.h (right):

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/TextBlob.h:15: typedef sk_sp<const
SkTextBlob> PassTextBlobPtr;
Heh, this is now painfully obsolete.  Can you add a TODO(fmalita) to get rid of
it?

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/win/FontCacheSkiaWin.cpp (right):

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/win/FontCacheSkiaWin.cpp:104: if
(!m_fontManager.get())
Nit: no .get() needed.

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/graphics/AcceleratedStaticBitmapImage.cpp
(right):

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/AcceleratedStaticBitmapImage.cpp:25:
return adoptRef(new AcceleratedStaticBitmapImage(image));
std::move(image)

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/AcceleratedStaticBitmapImage.cpp:30:
return adoptRef(new AcceleratedStaticBitmapImage(image, std::move(grContext),
mailbox, syncToken));
Ditto.

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/BitmapImage.cpp (right):

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:376: return
StaticBitmapImage::create(firstFrame);
not new to this CL, but std::move(firstFrame) makes sense here.

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/BoxReflection.h (right):

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/BoxReflection.h:36: :
m_direction(direction), m_offset(offset), m_mask(mask) {}
std::move(mask)

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp
(right):

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp:303:
mailboxInfo.m_image = image;
std::move(image)

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp:316:
mailboxInfo.m_image = image;
std::move(image)

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp:617:
m_surface.reset();
Nit: I don't think this explicit reset is needed after move assignment. Maybe
DCHECK instead?

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/GraphicsContext.cpp (right):

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/GraphicsContext.cpp:323:
picturePaint.setImageFilter(SkPictureImageFilter::MakeForLocalSpace(picture,
sourceBounds, static_cast<SkFilterQuality>(imageInterpolationQuality())));
std::move(picture)

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp (right):

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp:344:
tracking.lastPaintedPicture = newPicture;
Not new to this CL (and prolly not very important for DCHECK_IS_ON), but
std::move(newPicture).

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/ImageBuffer.cpp (right):

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ImageBuffer.cpp:201: return
StaticBitmapImage::create(snapshot);
std:move(snapshot)

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/PaintGeneratedImage.h (right):

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/PaintGeneratedImage.h:20: return
adoptRef(new PaintGeneratedImage(picture, size));
std::move(picture)

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/PaintGeneratedImage.h:30: ,
m_picture(picture)
std::move(picture)

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/Pattern.cpp (right):

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/Pattern.cpp:48: return
PicturePattern::create(picture, repeatMode);
std::move(picture)

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/PicturePattern.cpp (right):

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/PicturePattern.cpp:16: return
adoptRef(new PicturePattern(picture, repeatMode));
std::move(picture)

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/PictureSnapshot.cpp (right):

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/PictureSnapshot.cpp:57: :
m_picture(picture)
std::move(picture)

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp (right):

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:27:
StaticBitmapImage::StaticBitmapImage(sk_sp<SkImage> image) : m_image(image)
std::move(image)

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/gpu/WebGLImageConversion.cpp
(right):

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/gpu/WebGLImageConversion.cpp:2189:
m_imagePixelLocker.emplace(skiaImage, info.alphaType(), kN32_SkColorType);
std::move(skiaImage)

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/skia/ImagePixelLocker.cpp
(right):

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/skia/ImagePixelLocker.cpp:29: :
m_image(image)
std::move(image)

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/SegmentReader.cpp
(right):

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/SegmentReader.cpp:106: :
m_roBuffer(buffer)
std::move(buffer)

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/SegmentReader.cpp:196: return
adoptRef(new ROBufferSegmentReader(buffer));
std::move(buffer)

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/linux/WebFontRendering.cpp (right):

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/linux/WebFontRendering.cpp:47:
FontCache::setFontManager(sk_sp<SkFontMgr>(fontMgr));
This looks iffy: prev RefPtr ctor is grabbing a new ref, while the current sk_sp
ctor is adopting.

We prolly need to use sk_ref_sp() instead.

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/win/WebFontRendering.cpp (right):

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/win/WebFontRendering.cpp:15:
FontCache::setFontManager(sk_sp<SkFontMgr>(fontMgr));
Ditto.

[Stephen Chennney, 2016-09-01 13:16:17]

Thanks for all the research and the back stories. I'm on board so feel free to
land after addressing fmalita's comments.

[Łukasz Anforowicz, 2016-09-01 18:18:38]

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

[commit-bot: I haz the power, 2016-09-01 18:19:04]

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-09-01 18:22:39]

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

[commit-bot: I haz the power, 2016-09-01 18:22:41]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Łukasz Anforowicz, 2016-09-01 18:38:40]

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

[commit-bot: I haz the power, 2016-09-01 18:38:59]

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-09-01 18:59:21]

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

[commit-bot: I haz the power, 2016-09-01 18:59:23]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Łukasz Anforowicz, 2016-09-01 19:33:59]

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

[commit-bot: I haz the power, 2016-09-01 19:34:32]

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

[Łukasz Anforowicz, 2016-09-01 20:50:59]

fmalita@, thanks for reviewing and finding extra opportunities to get rid of
.get() and/or to add std::move.  And I especially appreciate catching the
mistake I've made in web/win/WebFontRendering.cpp.  Can you take another look?

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right):

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:310:
surface->getCanvas()->drawImageRect(skiaImage.get(), drawSrcRect, drawDstRect,
&paint);
On 2016/09/01 03:55:37, f(malita) wrote:
> Nit: now that skiaImage is an sk_sp<SkImage>, there's no reason to use .get()
> for the old style/raw ptr API anymore - you can use drawImageRect(const
> sk_sp<SkImage>&, ...) instead.

Done.

I've also tried searching for

  $ find third_party/WebKit/Source -name *.cpp -or -name *.h \
    | xargs grep '\b\(m_sk\|sk\)[A-Z][a-zA-Z0-9_]*\.get()'

and there are a few more cases like this.  I've fixed this case:

- modules/imagebitmap/ImageBitmapRenderingContext.cpp:
surface->getCanvas()->drawImage(skImage.get(), ...) - fixed.

I've left the following cases unchanged:

- core/frame/ImageBitmap.cpp: Calls to flipSkImageVertically and
premulSkImageToUnPremul - I am leaving this unchanged - these are only
internal/local helpers + it feels best to keep the CL under review
contained/small

- platform/graphics/GraphicsContext.cpp: SkAnnotateLinkToDestination and
SkAnnotateNamedDestination - in both cases there only overloads in the Skia API
are ones that take a raw pointer to SkData.

- third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp - WebImageLayer
class only has an overload taking a raw pointer to SkImage.

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:312:
surface->getCanvas()->drawImage(skiaImage.get(), dstLeft, dstTop);
On 2016/09/01 03:55:37, f(malita) wrote:
> Ditto.

Done.

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:343:
surface->getCanvas()->drawImage(skImage.get(), 0, 0);
On 2016/09/01 03:55:37, f(malita) wrote:
> Ditto.

Done.

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:464:
listener->sendNewFrame(image);
On 2016/09/01 03:55:37, f(malita) wrote:
> Not new, but it would make sense to std::move(image) here.

But this is in a loop, so |image| can be reused, so we cannot move from |image|,
right?

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/SVGClipPainter.cpp (right):

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGClipPainter.cpp:142:
context.drawPicture(maskPicture.get());
On 2016/09/01 03:55:37, f(malita) wrote:
> Nit: .get() not needed anymore.

This won't work - AFAICT blink::GraphicsContext::drawPicture only has an
overload that takes a raw pointer.

../../third_party/WebKit/Source/core/paint/SVGClipPainter.cpp:142:25: error: no
viable conversion from 'sk_sp<SkPicture>' to 'const SkPicture *'
    context.drawPicture(maskPicture);
                        ^~~~~~~~~~~
../../third_party/skia/include/core/SkRefCnt.h:339:5: note: candidate function
    operator unspecified_bool_type() const { return this->get() ? &sk_sp::fPtr :
nullptr; }
    ^
../../third_party/WebKit/Source/platform/graphics/GraphicsContext.h:154:38:
note: passing argument to parameter here
    void drawPicture(const SkPicture*);

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/imagebitmap/ImageBitmapRenderingContext.cpp
(right):

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/imagebitmap/ImageBitmapRenderingContext.cpp:47:
surface->getCanvas()->drawImage(skImage.get(), 0, 0);
On 2016/09/01 03:55:37, f(malita) wrote:
> Nit: drop the .get(). it's cleaner.

Done.

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/DragImage.cpp (right):

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/DragImage.cpp:111:
canvas->drawImage(image.get(), 0, 0, &paint);
On 2016/09/01 03:55:38, f(malita) wrote:
> Ditto.

Done.

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/FontPlatformData.cpp (right):

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/FontPlatformData.cpp:137: :
m_typeface(tf)
On 2016/09/01 03:55:38, f(malita) wrote:
> std::move(tf)

Ooops - thanks for catching this.  Done.

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/TextBlob.h (right):

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/TextBlob.h:15: typedef sk_sp<const
SkTextBlob> PassTextBlobPtr;
On 2016/09/01 03:55:38, f(malita) wrote:
> Heh, this is now painfully obsolete.  Can you add a TODO(fmalita) to get rid
of
> it?

Done.

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/win/FontCacheSkiaWin.cpp (right):

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/win/FontCacheSkiaWin.cpp:104: if
(!m_fontManager.get())
On 2016/09/01 03:55:38, f(malita) wrote:
> Nit: no .get() needed.

Oh, right.  Done.

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/graphics/AcceleratedStaticBitmapImage.cpp
(right):

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/AcceleratedStaticBitmapImage.cpp:25:
return adoptRef(new AcceleratedStaticBitmapImage(image));
On 2016/09/01 03:55:38, f(malita) wrote:
> std::move(image)

Good catch.  Done.

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/AcceleratedStaticBitmapImage.cpp:30:
return adoptRef(new AcceleratedStaticBitmapImage(image, std::move(grContext),
mailbox, syncToken));
On 2016/09/01 03:55:38, f(malita) wrote:
> Ditto.

Good catch.  Done.

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/BitmapImage.cpp (right):

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:376: return
StaticBitmapImage::create(firstFrame);
On 2016/09/01 03:55:38, f(malita) wrote:
> not new to this CL, but std::move(firstFrame) makes sense here.

Good catch.  Done.

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/BoxReflection.h (right):

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/BoxReflection.h:36: :
m_direction(direction), m_offset(offset), m_mask(mask) {}
On 2016/09/01 03:55:38, f(malita) wrote:
> std::move(mask)

Done.

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp
(right):

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp:303:
mailboxInfo.m_image = image;
On 2016/09/01 03:55:38, f(malita) wrote:
> std::move(image)

Done.

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp:316:
mailboxInfo.m_image = image;
On 2016/09/01 03:55:38, f(malita) wrote:
> std::move(image)

Done.

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp:617:
m_surface.reset();
On 2016/09/01 03:55:38, f(malita) wrote:
> Nit: I don't think this explicit reset is needed after move assignment. Maybe
> DCHECK instead?

I wanted to preserve the old move semantics, but I felt uncomfortable moving
from something that is long-lived, because AFAIK std::move leaves its argument
in a somewhat undefined state - there is some discussion about this in
http://stackoverflow.com/questions/7027523/what-can-i-do-with-a-moved-from-ob....

My rule of thumb is: don't std::move is the object being moved from can be
reused, and it is not immediately obvious if it will be assigned to before or
after being reused.  Does it make sense?  Since this rule is violated above, I
wanted to make things clear/waterproof by adding the |reset| call (which should
be removed by the compiler anyway - it should be able to see that in the current
implementation of sk_sp<T>, the move constructor swaps with |oldSufrace| which
is null, and so the call to |reset| is redundant.

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/GraphicsContext.cpp (right):

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/GraphicsContext.cpp:323:
picturePaint.setImageFilter(SkPictureImageFilter::MakeForLocalSpace(picture,
sourceBounds, static_cast<SkFilterQuality>(imageInterpolationQuality())));
On 2016/09/01 03:55:38, f(malita) wrote:
> std::move(picture)

Done.

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp (right):

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp:344:
tracking.lastPaintedPicture = newPicture;
On 2016/09/01 03:55:38, f(malita) wrote:
> Not new to this CL (and prolly not very important for DCHECK_IS_ON), but
> std::move(newPicture).

Done.

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/ImageBuffer.cpp (right):

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ImageBuffer.cpp:201: return
StaticBitmapImage::create(snapshot);
On 2016/09/01 03:55:38, f(malita) wrote:
> std:move(snapshot)

Done.

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/PaintGeneratedImage.h (right):

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/PaintGeneratedImage.h:20: return
adoptRef(new PaintGeneratedImage(picture, size));
On 2016/09/01 03:55:38, f(malita) wrote:
> std::move(picture)

Done.

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/PaintGeneratedImage.h:30: ,
m_picture(picture)
On 2016/09/01 03:55:38, f(malita) wrote:
> std::move(picture)

Done.

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/Pattern.cpp (right):

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/Pattern.cpp:48: return
PicturePattern::create(picture, repeatMode);
On 2016/09/01 03:55:38, f(malita) wrote:
> std::move(picture)

Done.

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/PicturePattern.cpp (right):

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/PicturePattern.cpp:16: return
adoptRef(new PicturePattern(picture, repeatMode));
On 2016/09/01 03:55:38, f(malita) wrote:
> std::move(picture)

Done.

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/PictureSnapshot.cpp (right):

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/PictureSnapshot.cpp:57: :
m_picture(picture)
On 2016/09/01 03:55:38, f(malita) wrote:
> std::move(picture)

Done.

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp (right):

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:27:
StaticBitmapImage::StaticBitmapImage(sk_sp<SkImage> image) : m_image(image)
On 2016/09/01 03:55:38, f(malita) wrote:
> std::move(image)

Done.

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/gpu/WebGLImageConversion.cpp
(right):

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/gpu/WebGLImageConversion.cpp:2189:
m_imagePixelLocker.emplace(skiaImage, info.alphaType(), kN32_SkColorType);
On 2016/09/01 03:55:38, f(malita) wrote:
> std::move(skiaImage)

Done.

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/skia/ImagePixelLocker.cpp
(right):

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/skia/ImagePixelLocker.cpp:29: :
m_image(image)
On 2016/09/01 03:55:38, f(malita) wrote:
> std::move(image)

Done.

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/SegmentReader.cpp
(right):

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/SegmentReader.cpp:106: :
m_roBuffer(buffer)
On 2016/09/01 03:55:39, f(malita) wrote:
> std::move(buffer)

Done.

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/SegmentReader.cpp:196: return
adoptRef(new ROBufferSegmentReader(buffer));
On 2016/09/01 03:55:39, f(malita) wrote:
> std::move(buffer)

Done.

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/linux/WebFontRendering.cpp (right):

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/linux/WebFontRendering.cpp:47:
FontCache::setFontManager(sk_sp<SkFontMgr>(fontMgr));
On 2016/09/01 03:55:39, f(malita) wrote:
> This looks iffy: prev RefPtr ctor is grabbing a new ref, while the current
sk_sp
> ctor is adopting.
> 
> We prolly need to use sk_ref_sp() instead.

Done, although things are interesting here.  AFAICT, there is only a single
callsite of the linux version of this method:


Callsite #1: content/zygote/zygote_main_linux.cc:401:
blink::WebFontRendering::setSkiaFontManager(SkFontMgr_New_Android(&custom));

Note that SkFontMgr_New_Android returns a raw pointer to a newly
constructed/allocated object that needs to be *adopted*:
third_party/skia/src/ports/SkFontMgr_android.cpp:
SkFontMgr* SkFontMgr_New_Android(const SkFontMgr_Android_CustomFonts* custom) {
...
    return new SkFontMgr_Android(custom);
}


The callsite below is for the windows implementation (which I've admittedly
broken in the same way...):
Callsite #2: content/child/dwrite_font_proxy/dwrite_font_proxy_init_win.cc:75:
  sk_sp<SkFontMgr> skia_font_manager(SkFontMgr_New_DirectWrite(
      factory.Get(), g_font_collection.Get(), font_fallback.Get()));
  blink::WebFontRendering::setSkiaFontManager(skia_font_manager.get());
  // Add an extra ref for SetDefaultSkiaFactory, which keeps a ref but doesn't
  // addref.
  skia_font_manager->ref();
  SetDefaultSkiaFactory(skia_font_manager.get());


In the case of callsite #2, it does seem that we need to get an extra ref-count
inside blink::WebFontRendering::setSkiaFontManager.  Therefore, as you've
suggested, I've added sk_ref_sp (to preserve the old behavior) in both the linux
and the win version.

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/win/WebFontRendering.cpp (right):

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/win/WebFontRendering.cpp:15:
FontCache::setFontManager(sk_sp<SkFontMgr>(fontMgr));
On 2016/09/01 03:55:39, f(malita) wrote:
> Ditto.

Done.

[f(malita), 2016-09-01 21:03:43]

Thanks, LGTM.

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:464:
listener->sendNewFrame(image);
On 2016/09/01 20:50:57, Łukasz Anforowicz wrote:
> On 2016/09/01 03:55:37, f(malita) wrote:
> > Not new, but it would make sense to std::move(image) here.
> 
> But this is in a loop, so |image| can be reused, so we cannot move from
|image|,
> right?

Acknowledged.

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/SVGClipPainter.cpp (right):

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGClipPainter.cpp:142:
context.drawPicture(maskPicture.get());
On 2016/09/01 20:50:57, Łukasz Anforowicz wrote:
> On 2016/09/01 03:55:37, f(malita) wrote:
> > Nit: .get() not needed anymore.
> 
> This won't work - AFAICT blink::GraphicsContext::drawPicture only has an
> overload that takes a raw pointer.
> 
> ../../third_party/WebKit/Source/core/paint/SVGClipPainter.cpp:142:25: error:
no
> viable conversion from 'sk_sp<SkPicture>' to 'const SkPicture *'
>     context.drawPicture(maskPicture);
>                         ^~~~~~~~~~~
> ../../third_party/skia/include/core/SkRefCnt.h:339:5: note: candidate function
>     operator unspecified_bool_type() const { return this->get() ? &sk_sp::fPtr
:
> nullptr; }
>     ^
> ../../third_party/WebKit/Source/platform/graphics/GraphicsContext.h:154:38:
> note: passing argument to parameter here
>     void drawPicture(const SkPicture*);

Doh, yeah, I was thinking we're drawing to an SkCanvas.

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp
(right):

https://codereview.chromium.org/2290903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp:617:
m_surface.reset();
On 2016/09/01 20:50:58, Łukasz Anforowicz wrote:
> On 2016/09/01 03:55:38, f(malita) wrote:
> > Nit: I don't think this explicit reset is needed after move assignment.
Maybe
> > DCHECK instead?
> 
> I wanted to preserve the old move semantics, but I felt uncomfortable moving
> from something that is long-lived, because AFAIK std::move leaves its argument
> in a somewhat undefined state - there is some discussion about this in
>
http://stackoverflow.com/questions/7027523/what-can-i-do-with-a-moved-from-ob....
> 
> My rule of thumb is: don't std::move is the object being moved from can be
> reused, and it is not immediately obvious if it will be assigned to before or
> after being reused.  Does it make sense?  Since this rule is violated above, I
> wanted to make things clear/waterproof by adding the |reset| call (which
should
> be removed by the compiler anyway - it should be able to see that in the
current
> implementation of sk_sp<T>, the move constructor swaps with |oldSufrace| which
> is null, and so the call to |reset| is redundant.

Acknowledged.

[commit-bot: I haz the power, 2016-09-01 21:16:51]

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

[commit-bot: I haz the power, 2016-09-01 21:16:53]

Dry run: This issue passed the CQ dry run.

[Łukasz Anforowicz, 2016-09-01 21:23:47]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2016-09-01 21:23:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
schenney@chromium.org, pdr@chromium.org, junov@chromium.org
Link to the patchset: https://codereview.chromium.org/2290903002/#ps180001
(title: "Fixing issues caused by rebasing.")

[commit-bot: I haz the power, 2016-09-01 21:24:12]

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-09-01 21:31:04]

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

[commit-bot: I haz the power, 2016-09-01 21:31:07]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Łukasz Anforowicz, 2016-09-01 21:45:54]

lukasza@chromium.org changed reviewers:
+ mcasas@chromium.org, zmo@chromium.org

[Łukasz Anforowicz, 2016-09-01 21:45:56]

+zmo@ for third_party/WebKit/Source/modules/webgl/OWNERS review
+mcasas@ for third_party/WebKit/Source/modules/mediacapturefromelement/OWNERS
review

[Zhenyao Mo, 2016-09-02 00:38:10]

On 2016/09/01 21:45:56, Łukasz Anforowicz wrote:
> +zmo@ for third_party/WebKit/Source/modules/webgl/OWNERS review
> +mcasas@ for third_party/WebKit/Source/modules/mediacapturefromelement/OWNERS
> review

modules/webgl LGTM

[mcasas, 2016-09-02 01:52:10]

On 2016/09/02 00:38:10, Zhenyao Mo wrote:
> On 2016/09/01 21:45:56, Łukasz Anforowicz wrote:
> > +zmo@ for third_party/WebKit/Source/modules/webgl/OWNERS review
> > +mcasas@ for
third_party/WebKit/Source/modules/mediacapturefromelement/OWNERS
> > review
> 
> modules/webgl LGTM

third_party/WebKit/Source/modules/mediacapturefromelement/ lgtm

[Łukasz Anforowicz, 2016-09-02 15:10:59]

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

[commit-bot: I haz the power, 2016-09-02 15:11:15]

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-09-02 15:15:09]

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

[commit-bot: I haz the power, 2016-09-02 15:15:10]

Dry run: This issue passed the CQ dry run.

[Łukasz Anforowicz, 2016-09-02 17:10:21]

The CQ bit was checked by lukasza@chromium.org

[commit-bot: I haz the power, 2016-09-02 17:11:06]

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-09-02 17:19:07]

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

[commit-bot: I haz the power, 2016-09-02 17:19:09]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Łukasz Anforowicz, 2016-09-02 17:43:15]

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

[commit-bot: I haz the power, 2016-09-02 17:43:33]

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

[Łukasz Anforowicz, 2016-09-02 17:54:23]

The CQ bit was unchecked by lukasza@chromium.org

[Łukasz Anforowicz, 2016-09-02 17:54:32]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2016-09-02 17:54:33]

The patchset sent to the CQ was uploaded after l-g-t-m from junov@chromium.org,
zmo@chromium.org, mcasas@chromium.org, fmalita@chromium.org,
schenney@chromium.org, pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/2290903002/#ps200001
(title: "Rebasing...")

[commit-bot: I haz the power, 2016-09-02 17:55:13]

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-09-02 19:50:57]

Description was changed from

==========
Change (Pass)RefPtr<SkXxx> into sk_sp<SkXxx>.

This CL is motivated by 2 things:
1. General desire to use sk_sp<...> instead of RefPtr<...>
   (i.e. as given in a comment next to the fromSkSp/toSkSp functions)
2. Desire to allow rename_to_chrome_style tool to rename
   WTF::RefCountedBase::ref to Ref (while not renaming |ref| to |Ref|
   in Skia).

This CL contains the following broadly applied changes:
- s/\(Pass\|\)RefPtr<Sk/sk_sp<Sk/g
- Repeat the above for |const Sk| (i.e. const-qualified template parameter)
- Repeat the above for |WTF::RefPtr<Sk| (i.e. explicit namespace qualifier)
- Add #include "third_party/skia/include/core/SkRefCnt.h" if appropriate
- Replace calls to RefPtr<T>::clear() with calls to sk_sp<T>::reset()
- Replace calls to PassRefPtr<T>::release() with calls to std::move(x)
- Replace calls to adoptRef with calls to sk_sp<T>(T*) constructor
  or calls to sk_sp<T>::reset(T*) - both will adopt a ref-count
- Remove (now unnecessary) fromSkSp and toSkSp calls
- Add more std::move calls during a self review

This CL contains the following one-off changes:
- Tweak platform/CrossThreadCopier.h to mark sk_sp<SkRefCnt> as thread-safe
- Remove fromSkSp and toSkSp definitions from
  platform/graphics/skia/SkiaUtils.h

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

to

==========
Change (Pass)RefPtr<SkXxx> into sk_sp<SkXxx>.

This CL is motivated by 2 things:
1. General desire to use sk_sp<...> instead of RefPtr<...>
   (i.e. as given in a comment next to the fromSkSp/toSkSp functions)
2. Desire to allow rename_to_chrome_style tool to rename
   WTF::RefCountedBase::ref to Ref (while not renaming |ref| to |Ref|
   in Skia).

This CL contains the following broadly applied changes:
- s/\(Pass\|\)RefPtr<Sk/sk_sp<Sk/g
- Repeat the above for |const Sk| (i.e. const-qualified template parameter)
- Repeat the above for |WTF::RefPtr<Sk| (i.e. explicit namespace qualifier)
- Add #include "third_party/skia/include/core/SkRefCnt.h" if appropriate
- Replace calls to RefPtr<T>::clear() with calls to sk_sp<T>::reset()
- Replace calls to PassRefPtr<T>::release() with calls to std::move(x)
- Replace calls to adoptRef with calls to sk_sp<T>(T*) constructor
  or calls to sk_sp<T>::reset(T*) - both will adopt a ref-count
- Remove (now unnecessary) fromSkSp and toSkSp calls
- Add more std::move calls during a self review

This CL contains the following one-off changes:
- Tweak platform/CrossThreadCopier.h to mark sk_sp<SkRefCnt> as thread-safe
- Remove fromSkSp and toSkSp definitions from
  platform/graphics/skia/SkiaUtils.h

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

[commit-bot: I haz the power, 2016-09-02 19:51:00]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-09-02 19:53:05]

Description was changed from

==========
Change (Pass)RefPtr<SkXxx> into sk_sp<SkXxx>.

This CL is motivated by 2 things:
1. General desire to use sk_sp<...> instead of RefPtr<...>
   (i.e. as given in a comment next to the fromSkSp/toSkSp functions)
2. Desire to allow rename_to_chrome_style tool to rename
   WTF::RefCountedBase::ref to Ref (while not renaming |ref| to |Ref|
   in Skia).

This CL contains the following broadly applied changes:
- s/\(Pass\|\)RefPtr<Sk/sk_sp<Sk/g
- Repeat the above for |const Sk| (i.e. const-qualified template parameter)
- Repeat the above for |WTF::RefPtr<Sk| (i.e. explicit namespace qualifier)
- Add #include "third_party/skia/include/core/SkRefCnt.h" if appropriate
- Replace calls to RefPtr<T>::clear() with calls to sk_sp<T>::reset()
- Replace calls to PassRefPtr<T>::release() with calls to std::move(x)
- Replace calls to adoptRef with calls to sk_sp<T>(T*) constructor
  or calls to sk_sp<T>::reset(T*) - both will adopt a ref-count
- Remove (now unnecessary) fromSkSp and toSkSp calls
- Add more std::move calls during a self review

This CL contains the following one-off changes:
- Tweak platform/CrossThreadCopier.h to mark sk_sp<SkRefCnt> as thread-safe
- Remove fromSkSp and toSkSp definitions from
  platform/graphics/skia/SkiaUtils.h

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

to

==========
Change (Pass)RefPtr<SkXxx> into sk_sp<SkXxx>.

This CL is motivated by 2 things:
1. General desire to use sk_sp<...> instead of RefPtr<...>
   (i.e. as given in a comment next to the fromSkSp/toSkSp functions)
2. Desire to allow rename_to_chrome_style tool to rename
   WTF::RefCountedBase::ref to Ref (while not renaming |ref| to |Ref|
   in Skia).

This CL contains the following broadly applied changes:
- s/\(Pass\|\)RefPtr<Sk/sk_sp<Sk/g
- Repeat the above for |const Sk| (i.e. const-qualified template parameter)
- Repeat the above for |WTF::RefPtr<Sk| (i.e. explicit namespace qualifier)
- Add #include "third_party/skia/include/core/SkRefCnt.h" if appropriate
- Replace calls to RefPtr<T>::clear() with calls to sk_sp<T>::reset()
- Replace calls to PassRefPtr<T>::release() with calls to std::move(x)
- Replace calls to adoptRef with calls to sk_sp<T>(T*) constructor
  or calls to sk_sp<T>::reset(T*) - both will adopt a ref-count
- Remove (now unnecessary) fromSkSp and toSkSp calls
- Add more std::move calls during a self review

This CL contains the following one-off changes:
- Tweak platform/CrossThreadCopier.h to mark sk_sp<SkRefCnt> as thread-safe
- Remove fromSkSp and toSkSp definitions from
  platform/graphics/skia/SkiaUtils.h

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

Committed: https://crrev.com/69969af74bfff920f5fdcc2aa547ea4af2d477d2
Cr-Commit-Position: refs/heads/master@{#416323}
==========

[commit-bot: I haz the power, 2016-09-02 19:53:08]

Patchset 11 (id:??) landed as
https://crrev.com/69969af74bfff920f5fdcc2aa547ea4af2d477d2
Cr-Commit-Position: refs/heads/master@{#416323}