Remove SkDevice and SkBaseDevice outside skia/ext/

We're deprecating SkDevice usage in Chromium; this change should
isolate it to skia/ext/ for simpler refactoring.

I took the opportunity to also change calls to the deprecated
SkCanvas::getDeviceSize() to the preferred replacement,
SkCanvas::getBaseLayerSize().

R=fmalita@chromium.org,junov@chromium.org
BUG=543755
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/15223e977e59eafbb2476c7224641adc1589d860
Cr-Commit-Position: refs/heads/master@{#361196}

[Tom Hudson, 2015-11-03 19:19:49]

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

[commit-bot: I haz the power, 2015-11-03 19:21:17]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1432443004/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1432443004/1

[commit-bot: I haz the power, 2015-11-03 20:17:06]

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

[commit-bot: I haz the power, 2015-11-03 20:17:06]

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

[Tom Hudson, 2015-11-04 16:00:53]

Description was changed from

==========
Remove SkDevice and SkBaseDevice outside skia/ext/

We're deprecating SkDevice usage in Chromium; this change should
isolate it to skia/ext/ for simpler refactoring.

Possible performance impact in WebKit: we're now returning a *copy* of
the pixels in an ImageBufferSurface to enforce the read-only contract.

Will also need review from jam@

R=fmalita@chromium.org
CC=halcanary@chromium.org
BUG=543755
==========

to

==========
We're deprecating SkDevice usage in Chromium; this change should
isolate it to skia/ext/ for simpler refactoring.

Possible performance impact in WebKit: we're now returning a *copy* of
the pixels in an ImageBufferSurface to enforce the read-only contract.

Will also need review from jam@

R=fmalita@chromium.org
CC=halcanary@chromium.org
BUG=543755
==========

[Tom Hudson, 2015-11-04 16:00:53]

tomhudson@chromium.org changed reviewers:
- tomhudson@chromium.org

[Tom Hudson, 2015-11-04 16:01:20]

Florin, PTAL - I'd also welcome suggestions where to put the ReadPixels() code
in WebKit to avoid cut-and-paste; in Chromium it's part of src/skia/ext/'s
skia:: global namespace, which is a bit yucky.

Asan failure looks like a bot infrastructure issue.

[Justin Novosad, 2015-11-04 16:29:36]

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

[Justin Novosad, 2015-11-04 16:29:36]

https://codereview.chromium.org/1432443004/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/ImageBufferSurface.cpp (right):

https://codereview.chromium.org/1432443004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/ImageBufferSurface.cpp:45: SkBitmap
ReadPixels(SkCanvas* canvas)
Blink style: 'readPixels'

https://codereview.chromium.org/1432443004/diff/1/third_party/WebKit/Source/p...
File
third_party/WebKit/Source/platform/graphics/UnacceleratedImageBufferSurface.cpp
(right):

https://codereview.chromium.org/1432443004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/UnacceleratedImageBufferSurface.cpp:43:
SkBitmap ReadPixels(SkCanvas* canvas)
This is copied in two places, perhaps it should go in SkiaUtils.h/cpp

https://codereview.chromium.org/1432443004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/UnacceleratedImageBufferSurface.cpp:79:
return ReadPixels(m_surface->getCanvas());
I am surprised this does not break any layout tests (toDataURL on a webgl
canvas, drawing webgl to a 2D canvas, etc.). I did not expect
SkCanvas::readPixels to return an SkBitmap that references the surface's
backing, I would have expected it to make a copy (as implied by the 'read'
verb).  Are we garanteed this will continue to work going forward?

IMHO, I think the calls sites that use this should be re-written to use
ImageBufferSurface::canvas() for writing to the surface. An we should just get
rid of this method entirely.

[Tom Hudson, 2015-11-04 16:43:27]

On 2015/11/04 16:29:36, Justin Novosad wrote:
>
https://codereview.chromium.org/1432443004/diff/1/third_party/WebKit/Source/p...
> File third_party/WebKit/Source/platform/graphics/ImageBufferSurface.cpp
(right):
> 
>
https://codereview.chromium.org/1432443004/diff/1/third_party/WebKit/Source/p...
> third_party/WebKit/Source/platform/graphics/ImageBufferSurface.cpp:45:
SkBitmap
> ReadPixels(SkCanvas* canvas)
> Blink style: 'readPixels'
> 
>
https://codereview.chromium.org/1432443004/diff/1/third_party/WebKit/Source/p...
> File
>
third_party/WebKit/Source/platform/graphics/UnacceleratedImageBufferSurface.cpp
> (right):
> 
>
https://codereview.chromium.org/1432443004/diff/1/third_party/WebKit/Source/p...
>
third_party/WebKit/Source/platform/graphics/UnacceleratedImageBufferSurface.cpp:43:
> SkBitmap ReadPixels(SkCanvas* canvas)
> This is copied in two places, perhaps it should go in SkiaUtils.h/cpp

Perfect, that's what I was looking for.

>
https://codereview.chromium.org/1432443004/diff/1/third_party/WebKit/Source/p...
>
third_party/WebKit/Source/platform/graphics/UnacceleratedImageBufferSurface.cpp:79:
> return ReadPixels(m_surface->getCanvas());
> I am surprised this does not break any layout tests (toDataURL on a webgl
> canvas, drawing webgl to a 2D canvas, etc.). I did not expect
> SkCanvas::readPixels to return an SkBitmap that references the surface's
> backing, I would have expected it to make a copy (as implied by the 'read'
> verb).  Are we garanteed this will continue to work going forward?

readPixels() does make a copy; I hadn't copied the documentation from Chromium's
skia/ext/platform_canvas.h because as I said I didn't like the cut-and-paste and
wanted to find somewhere to put it.

Going forward, by the time we break this we should have migrated everything to
SkSurface/SkImage :)

Does the CQ dry run actually run layout tests? Or is there something additional
I should be doing to trigger them?

> IMHO, I think the calls sites that use this should be re-written to use
> ImageBufferSurface::canvas() for writing to the surface. An we should just get
> rid of this method entirely.

Interesting suggestion. DrawingBuffer::paintFramebufferToCanvas() might be good
place to experiment. Would it be reasonable to land this to enable Skia cleanup
in parallel with working there to get rid of the deprecated*() functions?

[Justin Novosad, 2015-11-04 19:09:25]

On 2015/11/04 16:43:27, Tom Hudson wrote:
> On 2015/11/04 16:29:36, Justin Novosad wrote:

> > IMHO, I think the calls sites that use this should be re-written to use
> > ImageBufferSurface::canvas() for writing to the surface. An we should just
get
> > rid of this method entirely.
> 
> Interesting suggestion. DrawingBuffer::paintFramebufferToCanvas() might be
good
> place to experiment. Would it be reasonable to land this to enable Skia
cleanup
> in parallel with working there to get rid of the deprecated*() functions?

Only if this CL does not break anything. I am pretty confident this should be
breaking stuff as the CL stands right now, but the layout tests are passing. I
don't understand what is going on.

The layout test are being triggered. If you look at the *_chromium_rel_ng bots,
they are running webkit_tests. And the test are passing.
I am going to apply this patch locally to try and figure out what is going on...

[Justin Novosad, 2015-11-04 21:35:16]

On 2015/11/04 19:09:25, Justin Novosad wrote:


It looks like UnacceleratedImageBufferSurface::deprecatedBitmapForOverwrite() is
dead code (still referenced, but never called).

[Tom Hudson, 2015-11-04 21:51:42]

On 2015/11/04 21:35:16, Justin Novosad wrote:
> On 2015/11/04 19:09:25, Justin Novosad wrote:
> 
> 
> It looks like UnacceleratedImageBufferSurface::deprecatedBitmapForOverwrite()
is
> dead code (still referenced, but never called).

Because the only caller is DrawingBuffer::paintFramebufferToCanvas(), which is
always working with an AcceleratedImageBufferSurface?

[Justin Novosad, 2015-11-04 22:34:44]

On 2015/11/04 21:51:42, Tom Hudson wrote:
> On 2015/11/04 21:35:16, Justin Novosad wrote:
> > On 2015/11/04 19:09:25, Justin Novosad wrote:
> > 
> > 
> > It looks like
UnacceleratedImageBufferSurface::deprecatedBitmapForOverwrite()
> is
> > dead code (still referenced, but never called).
> 
> Because the only caller is DrawingBuffer::paintFramebufferToCanvas(), which is
> always working with an AcceleratedImageBufferSurface?

That is what it looks like. We have a fallback code path that never gets
exercised.  I created a cleanup CL right here:
https://codereview.chromium.org/1407133017/

[f(malita), 2015-11-05 13:53:26]

https://codereview.chromium.org/1432443004/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/ImageBufferSurface.cpp (right):

https://codereview.chromium.org/1432443004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/ImageBufferSurface.cpp:99: return
ReadPixels(canvas());
This looks like we're just trying to make the compiler happy, so I'd vote to
return Bitmap() and get rid of ReadPixels().

Then again, IIUC this is all moot after Justin's deprecatedBitmapForOverwrite
removal.

https://codereview.chromium.org/1432443004/diff/1/third_party/WebKit/Source/p...
File
third_party/WebKit/Source/platform/graphics/UnacceleratedImageBufferSurface.cpp
(right):

https://codereview.chromium.org/1432443004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/UnacceleratedImageBufferSurface.cpp:79:
return ReadPixels(m_surface->getCanvas());
On 2015/11/04 16:29:36, Justin Novosad wrote:
> I am surprised this does not break any layout tests (toDataURL on a webgl
> canvas, drawing webgl to a 2D canvas, etc.). I did not expect
> SkCanvas::readPixels to return an SkBitmap that references the surface's
> backing, I would have expected it to make a copy (as implied by the 'read'
> verb).  Are we garanteed this will continue to work going forward?
> 
> IMHO, I think the calls sites that use this should be re-written to use
> ImageBufferSurface::canvas() for writing to the surface.

I was just about to suggest returning a SkSurface instead of SkBitmap.  A canvas
would work too.

> An we should just get
> rid of this method entirely.

Even better, thanks for doing that :)

[Tom Hudson, 2015-11-05 22:21:57]

I'll consider this CL blocked until Justin lands
https://codereview.chromium.org/1407133017/, and then rebase on top of that.

[Tom Hudson, 2015-11-13 20:33:03]

Description was changed from

==========
We're deprecating SkDevice usage in Chromium; this change should
isolate it to skia/ext/ for simpler refactoring.

Possible performance impact in WebKit: we're now returning a *copy* of
the pixels in an ImageBufferSurface to enforce the read-only contract.

Will also need review from jam@

R=fmalita@chromium.org
CC=halcanary@chromium.org
BUG=543755
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
We're deprecating SkDevice usage in Chromium; this change should
isolate it to skia/ext/ for simpler refactoring.

I took the opportunity to also change calls to the deprecated
SkCanvas::getDeviceSize() to the preferred replacement,
SkCanvas::getBaseLayerSize().

Will also need review from danakj@

R=fmalita@chromium.org,junov@chromium.org
BUG=543755
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Tom Hudson, 2015-11-13 20:33:58]

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

[commit-bot: I haz the power, 2015-11-13 20:37:24]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1432443004/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1432443004/20001

[Tom Hudson, 2015-11-13 20:38:25]

Rebased and expanded a bit since Justin's cleanup patch landed.

The diff has a couple of removals of #include
"mojo/public/cpp/system/data_pipe.h" that presubmit was complaining about; I
assume those showed up in conflicts during the merge/rebase, since they aren't
needed for building.

I'm a bit torn between lumping the ui/base/clipboard/clipboard_win.cc and
content/renderer/npapi/webplugin_delegate_proxy.cc changes in here and isolating
them to their own CLs.

[commit-bot: I haz the power, 2015-11-13 21:06:58]

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

[commit-bot: I haz the power, 2015-11-13 21:06:58]

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

[Tom Hudson, 2015-11-18 16:39:39]

tomhudson@chromium.org changed reviewers:
+ danakj@chromium.org, dcheng@chromium.org, dnicoara@chromium.org,
msw@chromium.org, piman@chromium.org, sky@chromium.org, thestig@chromium.org

[Tom Hudson, 2015-11-18 16:39:39]

danakj@: please review cc/, content/browser/compositor/, 
msw@: please review chrome/browser/, components/html_viewer/
piman@: please review content/renderer/
dcheng@: please review ui/base/clipboard/
dnicoara@: please review ui/ozone/platform/drm/gpu/
sky@: please review ui/views/
thestig@: please review printing/

[Tom Hudson, 2015-11-18 16:42:22]

Description was changed from

==========
We're deprecating SkDevice usage in Chromium; this change should
isolate it to skia/ext/ for simpler refactoring.

I took the opportunity to also change calls to the deprecated
SkCanvas::getDeviceSize() to the preferred replacement,
SkCanvas::getBaseLayerSize().

Will also need review from danakj@

R=fmalita@chromium.org,junov@chromium.org
BUG=543755
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
We're deprecating SkDevice usage in Chromium; this change should
isolate it to skia/ext/ for simpler refactoring.

I took the opportunity to also change calls to the deprecated
SkCanvas::getDeviceSize() to the preferred replacement,
SkCanvas::getBaseLayerSize().

R=fmalita@chromium.org,junov@chromium.org
BUG=543755
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[dnicoara, 2015-11-18 16:58:17]

ui/ozone lgtm

[Justin Novosad, 2015-11-18 17:05:24]

third_party/WebKit lgtm

[Lei Zhang, 2015-11-18 17:36:36]

printing/ lgtm

https://codereview.chromium.org/1432443004/diff/80001/printing/metafile_skia_...
File printing/metafile_skia_wrapper.cc (right):

https://codereview.chromium.org/1432443004/diff/80001/printing/metafile_skia_...
printing/metafile_skia_wrapper.cc:10: class SkCanvas;
Is this even needed?

[Tom Hudson, 2015-11-18 17:50:56]

tomhudson@chromium.org changed reviewers:
- tomhudson@chromium.org

[Tom Hudson, 2015-11-18 17:50:56]

https://codereview.chromium.org/1432443004/diff/80001/printing/metafile_skia_...
File printing/metafile_skia_wrapper.cc (right):

https://codereview.chromium.org/1432443004/diff/80001/printing/metafile_skia_...
printing/metafile_skia_wrapper.cc:10: class SkCanvas;
On 2015/11/18 17:36:36, Lei Zhang wrote:
> Is this even needed?

Technically not, since we include platform_device which includes SkBitmapDevice
which includes SkCanvas, but (1) I'm trying to get rid of platform_device
dependencies as well and (2)
http://dev.chromium.org/developers/coding-style#TOC-Forward-Declarations-vs.-...
led me to think this was the better end-state.

[dcheng, 2015-11-18 18:10:35]

https://codereview.chromium.org/1432443004/diff/80001/ui/base/clipboard/clipb...
File ui/base/clipboard/clipboard_win.cc (right):

https://codereview.chromium.org/1432443004/diff/80001/ui/base/clipboard/clipb...
ui/base/clipboard/clipboard_win.cc:176: void MakeBitmapOpaque(SkPixmap pixmap) {
I can't tell from the header comments. Is SkPixmap a cheap handle type that's
easy to copy, and all copies refer to the same pixel store?

[piman, 2015-11-18 18:33:12]

content/ lgtm.

[piman, 2015-11-18 18:33:12]

content/ lgtm.

[msw, 2015-11-18 19:44:29]

chrome/browser/ui and components/html_viewer/ lgtm

[sky, 2015-11-18 21:03:41]

LGTM

[Tom Hudson, 2015-11-20 15:20:47]

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

[Tom Hudson, 2015-11-20 15:20:48]

https://codereview.chromium.org/1432443004/diff/80001/ui/base/clipboard/clipb...
File ui/base/clipboard/clipboard_win.cc (right):

https://codereview.chromium.org/1432443004/diff/80001/ui/base/clipboard/clipb...
ui/base/clipboard/clipboard_win.cc:176: void MakeBitmapOpaque(SkPixmap pixmap) {
On 2015/11/18 18:10:35, dcheng wrote:
> I can't tell from the header comments. Is SkPixmap a cheap handle type that's
> easy to copy, and all copies refer to the same pixel store?

Yep! (Sorry that I missed responding to this.)

I'd like to get more intent into the header comments, and just haven't figured
out what the important context is. Suggestions welcome.

[dcheng, 2015-11-20 19:25:27]

https://codereview.chromium.org/1432443004/diff/80001/ui/base/clipboard/clipb...
File ui/base/clipboard/clipboard_win.cc (right):

https://codereview.chromium.org/1432443004/diff/80001/ui/base/clipboard/clipb...
ui/base/clipboard/clipboard_win.cc:176: void MakeBitmapOpaque(SkPixmap pixmap) {
On 2015/11/20 at 15:20:48, Tom Hudson wrote:
> On 2015/11/18 18:10:35, dcheng wrote:
> > I can't tell from the header comments. Is SkPixmap a cheap handle type
that's
> > easy to copy, and all copies refer to the same pixel store?
> 
> Yep! (Sorry that I missed responding to this.)
> 
> I'd like to get more intent into the header comments, and just haven't figured
out what the important context is. Suggestions welcome.

Compare:
https://code.google.com/p/chromium/codesearch#search/&q=%5B(,%5DSkPixmap%5B*%...

vs

https://code.google.com/p/chromium/codesearch#search/&q=%5B(,%5DSkPixmap%5B%2...

It seems like passing it by value is non-existent. Is there any advantage to
pass by value? It seems like it'd require a memcpy where pass-by-pointer would
be free-ish.

[Tom Hudson, 2015-11-20 20:09:10]

https://codereview.chromium.org/1432443004/diff/80001/ui/base/clipboard/clipb...
File ui/base/clipboard/clipboard_win.cc (right):

https://codereview.chromium.org/1432443004/diff/80001/ui/base/clipboard/clipb...
ui/base/clipboard/clipboard_win.cc:176: void MakeBitmapOpaque(SkPixmap pixmap) {
On 2015/11/20 19:25:27, dcheng wrote:
> 
> It seems like passing it by value is non-existent. Is there any advantage to
> pass by value? It seems like it'd require a memcpy where pass-by-pointer would
> be free-ish.

D'oh, yes, done.

There are lots of places around here where I have to pass copies of SkBitmaps
that we used to pass by reference, but here we can perfectly well pass a pointer
to the SkPixmap.

(This doesn't require a memcpy of the actual pixels, but yes, it was copying the
several-words-big struct, and with the API changes we do have to do a bit more
of that.)

[dcheng, 2015-11-20 20:12:26]

ui/base/clipboard lgtm

[tomhudson, 2015-11-23 15:05:43]

danakj@: ping? You're the only unlimited reviewer shared between cc/ and
content/browser/compositor/, and the changes to those directories in this CL
ought to be trivial.

[piman, 2015-11-23 19:35:49]

On 2015/11/23 15:05:43, tomhudson wrote:
> danakj@: ping? You're the only unlimited reviewer shared between cc/ and
> content/browser/compositor/, and the changes to those directories in this CL
> ought to be trivial.

LGTM for cc

[Tom Hudson, 2015-11-23 20:00:01]

The CQ bit was checked by tomhudson@chromium.org

[Tom Hudson, 2015-11-23 20:00:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
dnicoara@chromium.org, sky@chromium.org, thestig@chromium.org,
junov@chromium.org, msw@chromium.org
Link to the patchset: https://codereview.chromium.org/1432443004/#ps100001
(title: "pass SkPixmap by pointer to MakeBitmapOpaque")

[commit-bot: I haz the power, 2015-11-23 20:05:46]

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

[danakj, 2015-11-23 21:11:39]

On 2015/11/23 15:05:43, tomhudson wrote:
> danakj@: ping? You're the only unlimited reviewer shared between cc/ and
> content/browser/compositor/, and the changes to those directories in this CL
> ought to be trivial.

Oh, sorry this was the first time I saw this published and noticed I'm a
reviewer. LGTM

[commit-bot: I haz the power, 2015-11-23 21:53:02]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-11-23 21:54:04]

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