Optionally clear PlatformCanvas instances.

Remove redundant memory clears when constructing BitmapPlatformDevice instances on Mac and Windows.  PlatformCanvas construction is showing up as a performance bottleneck due to unnecessary initialization.

The change moves the clear to the call sites where it is necessary.

Note:  On Linux, cairo always allocates an initialized surface, so there is no way to bypass the performance penalty.

BUG=112009
TEST=All of them

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=127196

[Jeff Timanus, 2012-02-22 19:08:40]

Alok & Steve, PTAL.

This change is pretty straight-forward:  I added some flags to specify if the
PlatformCanvas should be initialized to 0 during construction.  

I'm still waiting to confirm that I have not introduced any layout-test
regressions - the signal to noise on the failures is quite low.

[alokp, 2012-02-22 21:35:54]

I have two main issues with this patch:

1. We have painstakingly removed unnecessary references to PlatformDevice from
Chrome, which this patch seems to undo to a certain extent.

2. Why does INITIALIZE need to be specified at construction time? Can't the call
sites themselves call eraseARGB themselves.

I would just move all eraseARGB calls from the constructor to the call sites
that need them.

[Jeff Timanus, 2012-02-22 21:53:55]

On 2012/02/22 21:35:54, Alok Priyadarshi wrote:
> I have two main issues with this patch:
> 
> 1. We have painstakingly removed unnecessary references to PlatformDevice from
> Chrome, which this patch seems to undo to a certain extent.

All of the modifications made in this CL were previously making explicit use of
a Platform* instance, so there isn't that much more pollution of PlatformDevice
throughout Chrome.

I placed the enums in PlatformDevice, because it is all implementers of that
interface (and PlatformCanvas) that make use of them.  The enums could also
easily be placed in the skia namespace, so that we avoid extra mention of
PlatformDevice.

> 
> 2. Why does INITIALIZE need to be specified at construction time? Can't the
call
> sites themselves call eraseARGB themselves.

That's a good point.  The flag could be removed, and the erase be applied at
construction time.

The one problem there is that there would be a performance hit on Linux.  On
Linux, cairo always initializes new surfaces to 0.

If the callers were to explicitly call eraseARGB, then linux would suffer from a
double-clear.  The solution would be to conditionally clear, based on the
platform, but I'm not sure that's much better than the solution I have here.

> 
> I would just move all eraseARGB calls from the constructor to the call sites
> that need them.

[alokp, 2012-02-22 22:03:43]

On 2012/02/22 21:53:55, Jeff Timanus wrote:
> On 2012/02/22 21:35:54, Alok Priyadarshi wrote:
> > I have two main issues with this patch:
> > 
> > 1. We have painstakingly removed unnecessary references to PlatformDevice
from
> > Chrome, which this patch seems to undo to a certain extent.
> 
> All of the modifications made in this CL were previously making explicit use
of
> a Platform* instance, so there isn't that much more pollution of
PlatformDevice
> throughout Chrome.
> 
> I placed the enums in PlatformDevice, because it is all implementers of that
> interface (and PlatformCanvas) that make use of them.  

I saw PlatformDevice:: enum being used not just in the implementors but
throughout the patch.

> 
> > 
> > 2. Why does INITIALIZE need to be specified at construction time? Can't the
> call
> > sites themselves call eraseARGB themselves.
> 
> That's a good point.  The flag could be removed, and the erase be applied at
> construction time.
> 
> The one problem there is that there would be a performance hit on Linux.  On
> Linux, cairo always initializes new surfaces to 0.
> 
> If the callers were to explicitly call eraseARGB, then linux would suffer from
a
> double-clear.  The solution would be to conditionally clear, based on the
> platform, but I'm not sure that's much better than the solution I have here.
> 
> > 
> > I would just move all eraseARGB calls from the constructor to the call sites
> > that need them.

I see. Do you know if these double-clears would be in performance-critical path?

[Jeff Timanus, 2012-02-23 16:05:54]

On 2012/02/22 22:03:43, Alok Priyadarshi wrote:
> On 2012/02/22 21:53:55, Jeff Timanus wrote:
> > On 2012/02/22 21:35:54, Alok Priyadarshi wrote:
> > > I have two main issues with this patch:
> > > 
> > > 1. We have painstakingly removed unnecessary references to PlatformDevice
> from
> > > Chrome, which this patch seems to undo to a certain extent.
> > 
> > All of the modifications made in this CL were previously making explicit use
> of
> > a Platform* instance, so there isn't that much more pollution of
> PlatformDevice
> > throughout Chrome.
> > 
> > I placed the enums in PlatformDevice, because it is all implementers of that
> > interface (and PlatformCanvas) that make use of them.  
> 
> I saw PlatformDevice:: enum being used not just in the implementors but
> throughout the patch.

Sorry I wasn't clearer earlier.  All use-points of the enums are already making
use of a PlatformCanvas instance.  The flags could be placed there.  I put them
in PlatformDevice, because they are also needed in all of the various flavours
of devices.

> 
> > 
> > > 
> > > 2. Why does INITIALIZE need to be specified at construction time? Can't
the
> > call
> > > sites themselves call eraseARGB themselves.
> > 
> > That's a good point.  The flag could be removed, and the erase be applied at
> > construction time.
> > 
> > The one problem there is that there would be a performance hit on Linux.  On
> > Linux, cairo always initializes new surfaces to 0.
> > 
> > If the callers were to explicitly call eraseARGB, then linux would suffer
from
> a
> > double-clear.  The solution would be to conditionally clear, based on the
> > platform, but I'm not sure that's much better than the solution I have here.
> > 
> > > 
> > > I would just move all eraseARGB calls from the constructor to the call
sites
> > > that need them.
> 
> I see. Do you know if these double-clears would be in performance-critical
path?

There are quite a few call-sites, so I can't state with certainty which of these
will be problematic wrt performance if there is a double-clear.  

I'll quickly review the call-sites that require the clear to see if they look
like hot-spots.

[Jeff Timanus, 2012-02-23 22:14:10]

After reviewing the locations where I found initialization to be necessary, it
seems that the platform differences would not be too painful.  

Most of the construction sites are already in platform-specific locations, so
there is no complexity around adding #if !LINUX to prevent double-clears.  The
only sites requiring this would be in CanvasSkia::CanvasSkia.

I'll upload that change, which should be much less intrusive.

http://codereview.chromium.org/9416017/diff/66/content/plugin/webplugin_proxy.cc
File content/plugin/webplugin_proxy.cc (right):

http://codereview.chromium.org/9416017/diff/66/content/plugin/webplugin_proxy...
content/plugin/webplugin_proxy.cc:486: skia::PlatformDevice::FLAGS_INITIALIZED,
This initialization is within a #if WIN block, so an explicit erase would be ok.

http://codereview.chromium.org/9416017/diff/66/skia/ext/bitmap_platform_devic...
File skia/ext/bitmap_platform_device_linux.cc (right):

http://codereview.chromium.org/9416017/diff/66/skia/ext/bitmap_platform_devic...
skia/ext/bitmap_platform_device_linux.cc:139: return
BitmapPlatformDevice::Create(width, height, GetDefaultFlags(isOpaque));
The various BitmapPlatformDevice::onCreateCompatibleDevice calls could clear the
buffer explicitly.  The linux port could skip the clear, to prevent the double
clear.

http://codereview.chromium.org/9416017/diff/66/skia/ext/vector_platform_devic...
File skia/ext/vector_platform_device_skia.cc (right):

http://codereview.chromium.org/9416017/diff/66/skia/ext/vector_platform_devic...
skia/ext/vector_platform_device_skia.cc:46: width(), height(), FLAGS_OPAQUE |
FLAGS_INITIALIZED, NULL);
The clear here can also be made conditional on the platform to minimize.

http://codereview.chromium.org/9416017/diff/66/ui/gfx/canvas_skia.cc
File ui/gfx/canvas_skia.cc (right):

http://codereview.chromium.org/9416017/diff/66/ui/gfx/canvas_skia.cc#newcode54
ui/gfx/canvas_skia.cc:54: skia::PlatformDevice::GetDefaultFlags(is_opaque))),
These clears would require a #if WIN/LINUX block to prevent a double-clear.

http://codereview.chromium.org/9416017/diff/66/ui/gfx/native_theme_win.cc
File ui/gfx/native_theme_win.cc (right):

http://codereview.chromium.org/9416017/diff/66/ui/gfx/native_theme_win.cc#new...
ui/gfx/native_theme_win.cc:434: skia::PlatformDevice::FLAGS_INITIALIZED));
This file is windows specific, so the initialization can be explicitly
performed.

http://codereview.chromium.org/9416017/diff/66/ui/gfx/surface/transport_dib_l...
File ui/gfx/surface/transport_dib_linux.cc (right):

http://codereview.chromium.org/9416017/diff/66/ui/gfx/surface/transport_dib_l...
ui/gfx/surface/transport_dib_linux.cc:100:
skia::PlatformDevice::FLAGS_INITIALIZED,
The canvas can be explicitly cleared in the various flavours of TransportDIB,
based on the platform.

http://codereview.chromium.org/9416017/diff/66/webkit/tools/test_shell/mac/we...
File webkit/tools/test_shell/mac/webwidget_host.mm (right):

http://codereview.chromium.org/9416017/diff/66/webkit/tools/test_shell/mac/we...
webkit/tools/test_shell/mac/webwidget_host.mm:179:
skia::PlatformDevice::FLAGS_INITIALIZED));
This clear can be performed explicitly on mac.

[Jeff Timanus, 2012-02-29 16:18:35]

Please have another look.  The change has now been simplified to call clear
where necessary.  

Because this change could expose subtle errors, I've ran all the layout tests
against the change, and against a reference no-op build:

Reference trys (no-change):
Win: http://build.chromium.org/p/tryserver.chromium/builders/win/builds/813
Win-Layout:
http://build.chromium.org/p/tryserver.chromium/builders/win_layout/builds/79
Linux: http://build.chromium.org/p/tryserver.chromium/builders/linux/builds/415
Linux-Layout:
http://build.chromium.org/p/tryserver.chromium/builders/linux_layout/builds/75
Mac: http://build.chromium.org/p/tryserver.chromium/builders/mac/builds/386
Mac-Layout:
http://build.chromium.org/p/tryserver.chromium/builders/mac_layout/builds/71

Trys:
Win (Success):
http://build.chromium.org/p/tryserver.chromium/builders/win/builds/784
Win-Layout:
http://build.chromium.org/p/tryserver.chromium/builders/win_layout/builds/69
Linux (Success):
http://build.chromium.org/p/tryserver.chromium/builders/linux/builds/392
Linux-Layout:
http://build.chromium.org/p/tryserver.chromium/builders/linux_layout/builds/65
Mac (Success):
http://build.chromium.org/p/tryserver.chromium/builders/mac/builds/362
Mac-Layout:
http://build.chromium.org/p/tryserver.chromium/builders/mac_layout/builds/61

There is a lot of noise here.  Comparing the no-op trys to the trys with my
change, the following tests look suspicious.

From the linux layout tests:
  fast/canvas/canvas-overloads-strokeText.html = TIMEOUT PASS
  compositing/masks/multiple-masks.html = IMAGE PASS
  svg/text/append-text-node-to-tspan.html = IMAGE PASS

From the mac layout tests:
  *  fast/encoding/invalid-xml.html = TEXT PASS
  media/track/track-cues-pause-on-exit.html = TEXT PASS
  transitions/transition-end-event-nested.html = TEXT PAS
  *  compositing/animation/busy-indicator.html = IMAGE PASS
  compositing/reflections/transform-inside-reflection.html = IMAGE PASS
  svg/text/bidi-reorder-value-lists.svg = IMAGE+TEXT PASS

From the windows layout tests:
  fast/events/panScroll-click-hyperlink.html = TEXT PASS
  fast/multicol/span/span-as-immediate-columns-child-removal.html = TEXT
IMAGE+TEXT
  compositing/masks/simple-composited-mask.html = IMAGE PASS
  css3/filters/effect-grayscale-hw.html = IMAGE PASS
  fast/images/destroyed-image-load-event.html = TIMEOUT PASS
  http/tests/loading/307-after-303-after-post.html = TIMEOUT PASS
  media/track/track-active-cues.html = TIMEOUT PASS

I reviewed all of these tests on the flakiness dashboard, and only those marked
* did not have a history of flakiness.  I doubt the invalid-xml test is related
to my change, but I'll double check the busy-indicator test.

[alokp, 2012-03-01 17:09:02]

looks better.

http://codereview.chromium.org/9416017/diff/39001/skia/ext/platform_canvas_un...
File skia/ext/platform_canvas_unittest.cc (right):

http://codereview.chromium.org/9416017/diff/39001/skia/ext/platform_canvas_un...
skia/ext/platform_canvas_unittest.cc:197: canvas.drawARGB(0, 0, 0, 0);
eraseARGB? and everywhere else.

[Jeff Timanus, 2012-03-01 18:35:49]

Patch 15 includes a file missing from the previous patch set:  Removal of the
clear for bitmap platform device on mac.

http://codereview.chromium.org/9416017/diff/39001/skia/ext/platform_canvas_un...
File skia/ext/platform_canvas_unittest.cc (right):

http://codereview.chromium.org/9416017/diff/39001/skia/ext/platform_canvas_un...
skia/ext/platform_canvas_unittest.cc:197: canvas.drawARGB(0, 0, 0, 0);
On 2012/03/01 17:09:02, Alok Priyadarshi wrote:
> eraseARGB? and everywhere else.

I was confused by this too.

SkCanvas (& PlatformCanvas) expose drawARGB.
SkDevice exposes clear(...).
SkBitmap exposes eraseARGB(...).

All of these routines perform the same operation.

[Jeff Timanus, 2012-03-02 16:54:40]

The try run for patch 15 introduced quite a few unexpected failures on mac.  

It's surprising, because the windows path does not have these failures, yet it
follows mostly the same code paths.

On 2012/03/01 18:35:49, Jeff Timanus wrote:
> Patch 15 includes a file missing from the previous patch set:  Removal of the
> clear for bitmap platform device on mac.
> 
>
http://codereview.chromium.org/9416017/diff/39001/skia/ext/platform_canvas_un...
> File skia/ext/platform_canvas_unittest.cc (right):
> 
>
http://codereview.chromium.org/9416017/diff/39001/skia/ext/platform_canvas_un...
> skia/ext/platform_canvas_unittest.cc:197: canvas.drawARGB(0, 0, 0, 0);
> On 2012/03/01 17:09:02, Alok Priyadarshi wrote:
> > eraseARGB? and everywhere else.
> 
> I was confused by this too.
> 
> SkCanvas (& PlatformCanvas) expose drawARGB.
> SkDevice exposes clear(...).
> SkBitmap exposes eraseARGB(...).
> 
> All of these routines perform the same operation.

[Jeff Timanus, 2012-03-14 21:53:41]

Steve, can you please have a look at this change?  Alok is out for the next
couple of weeks.

The state of the tests seems to be so poor that I can't get any green layout
passes I'm reviewing all of the failures now, and none of them seem to be
related to my change.

Jeff

[Jeff Timanus, 2012-03-14 23:29:43]

On 2012/03/14 21:53:41, Jeff Timanus wrote:
> Steve, can you please have a look at this change?  Alok is out for the next
> couple of weeks.
> 
> The state of the tests seems to be so poor that I can't get any green layout
> passes I'm reviewing all of the failures now, and none of them seem to be
> related to my change.

I reviewed all of the test failures.  Here is a summary:

Win:  http://build.chromium.org/p/tryserver.chromium/builders/win/builds/2074
browser_tests:
[105788:105932:0313/230906:9217000:ERROR:window_impl.cc(54)] Failed to
unregister class Chrome_WidgetWin_0. Error = 1412
webkit_unit_tests: Same failures as on a reference no-change try run.
Win Layout:
http://build.chromium.org/p/tryserver.chromium/builders/win_layout/builds/144
Linker error.

------------------------------------------

Mac:  Success!!!
Mac Layout:
http://build.chromium.org/p/tryserver.chromium/builders/mac_layout/builds/118
Only test with no flakiness history:  Timeout: plugins/update-widgets-crash.html


------------------------------------------

Linux: 
http://build.chromium.org/p/tryserver.chromium/builders/linux_layout/builds/129
Failures in ipc & WebSocket which are not related.
Linux Layout: 
http://build.chromium.org/p/tryserver.chromium/builders/linux_layout/builds/129 
Known flakes.

All of the failures appear to be flakes.  The windows browser_tests failures do
look to be flakes.  They all report the same error, as indicated above.  I'm
running another try pass now to see if they are repro.

> 
> Jeff

[vandebo (ex-Chrome), 2012-03-14 23:38:29]

Generally LG, just a few questions.  I buy the premise that if the erase'd color
was ever visible, we'd see green artifacts in debug mode.  Of course that
doesn't apply to layers.  There are a few non-layer places where you've added
erase calls, so if you can explain why we weren't seeing green there before,
we'll be all set.

https://chromiumcodereview.appspot.com/9416017/diff/85001/skia/ext/vector_pla...
File skia/ext/vector_platform_device_skia.cc (left):

https://chromiumcodereview.appspot.com/9416017/diff/85001/skia/ext/vector_pla...
skia/ext/vector_platform_device_skia.cc:56: SkCanvas
canvas(raster_surface_.get());
nit: Why is/was this here?

https://chromiumcodereview.appspot.com/9416017/diff/85001/skia/ext/vector_pla...
File skia/ext/vector_platform_device_skia.cc (right):

https://chromiumcodereview.appspot.com/9416017/diff/85001/skia/ext/vector_pla...
skia/ext/vector_platform_device_skia.cc:56: #if defined(OS_WIN) ||
defined(OS_MACOSX)
I comment wouldn't hurt here.

https://chromiumcodereview.appspot.com/9416017/diff/85001/ui/gfx/canvas_skia.cc
File ui/gfx/canvas_skia.cc (right):

https://chromiumcodereview.appspot.com/9416017/diff/85001/ui/gfx/canvas_skia....
ui/gfx/canvas_skia.cc:58: owned_canvas_->clear(SkColorSetARGB(0, 0, 0, 0));
Why is this clear needed - this doesn't appear to be for a layer?  A comment
wouldn't hurt.

https://chromiumcodereview.appspot.com/9416017/diff/85001/webkit/tools/test_s...
File webkit/tools/test_shell/mac/webwidget_host.mm (right):

https://chromiumcodereview.appspot.com/9416017/diff/85001/webkit/tools/test_s...
webkit/tools/test_shell/mac/webwidget_host.mm:178:
canvas_->clear(SkColorSetARGB(0, 0, 0, 0));
Is this one necessary?

[Jeff Timanus, 2012-03-15 19:01:28]

Thanks for the review.  In general, I preferred preserving the existing
behaviour when examining call sites for the need to clear to 0.

https://chromiumcodereview.appspot.com/9416017/diff/85001/skia/ext/vector_pla...
File skia/ext/vector_platform_device_skia.cc (left):

https://chromiumcodereview.appspot.com/9416017/diff/85001/skia/ext/vector_pla...
skia/ext/vector_platform_device_skia.cc:56: SkCanvas
canvas(raster_surface_.get());
On 2012/03/14 23:38:29, vandebo wrote:
> nit: Why is/was this here?

I noticed this, and was also confused.  I left it as is, because I wanted to
minimize any impact this change may have had.  

It was introduced when the PDF back-end was brought into chrome.  See
http://src.chromium.org/viewvc/chrome?view=rev&revision=80873

https://chromiumcodereview.appspot.com/9416017/diff/85001/skia/ext/vector_pla...
File skia/ext/vector_platform_device_skia.cc (right):

https://chromiumcodereview.appspot.com/9416017/diff/85001/skia/ext/vector_pla...
skia/ext/vector_platform_device_skia.cc:56: #if defined(OS_WIN) ||
defined(OS_MACOSX)
On 2012/03/14 23:38:29, vandebo wrote:
> I comment wouldn't hurt here.

Done.

https://chromiumcodereview.appspot.com/9416017/diff/85001/ui/gfx/canvas_skia.cc
File ui/gfx/canvas_skia.cc (right):

https://chromiumcodereview.appspot.com/9416017/diff/85001/ui/gfx/canvas_skia....
ui/gfx/canvas_skia.cc:58: owned_canvas_->clear(SkColorSetARGB(0, 0, 0, 0));
On 2012/03/14 23:38:29, vandebo wrote:
> Why is this clear needed - this doesn't appear to be for a layer?  A comment
> wouldn't hurt.

I put a clear here to preserve existing behaviour.  I wanted to limit the change
to all users of skia/ext classes, and didn't want to propagate 'uninitalized'
semantics to users of ui/gfx.

Comment added.

https://chromiumcodereview.appspot.com/9416017/diff/85001/webkit/tools/test_s...
File webkit/tools/test_shell/mac/webwidget_host.mm (right):

https://chromiumcodereview.appspot.com/9416017/diff/85001/webkit/tools/test_s...
webkit/tools/test_shell/mac/webwidget_host.mm:178:
canvas_->clear(SkColorSetARGB(0, 0, 0, 0));
On 2012/03/14 23:38:29, vandebo wrote:
> Is this one necessary?

I added a clear here because it wasn't clear to me that the code below will
guarantee that the entire canvas is painted.

It's possible that it could be removed, but I wanted to preserve existing
behaviour.

[Jeff Timanus, 2012-03-15 19:13:05]

On 2012/03/14 23:29:43, Jeff Timanus wrote:
> On 2012/03/14 21:53:41, Jeff Timanus wrote:
> > Steve, can you please have a look at this change?  Alok is out for the next
> > couple of weeks.
> > 
> > The state of the tests seems to be so poor that I can't get any green layout
> > passes I'm reviewing all of the failures now, and none of them seem to be
> > related to my change.
> 
> I reviewed all of the test failures.  Here is a summary:
> 
> Win:  http://build.chromium.org/p/tryserver.chromium/builders/win/builds/2074
> browser_tests:
> [105788:105932:0313/230906:9217000:ERROR:window_impl.cc(54)] Failed to
> unregister class Chrome_WidgetWin_0. Error = 1412
> webkit_unit_tests: Same failures as on a reference no-change try run.
> Win Layout:
> http://build.chromium.org/p/tryserver.chromium/builders/win_layout/builds/144
> Linker error.
> 
> ------------------------------------------
> 
> Mac:  Success!!!
> Mac Layout:
> http://build.chromium.org/p/tryserver.chromium/builders/mac_layout/builds/118
> Only test with no flakiness history:  Timeout:
plugins/update-widgets-crash.html
> 
> 
> ------------------------------------------
> 
> Linux: 
>
http://build.chromium.org/p/tryserver.chromium/builders/linux_layout/builds/129
> Failures in ipc & WebSocket which are not related.
> Linux Layout: 
>
http://build.chromium.org/p/tryserver.chromium/builders/linux_layout/builds/129 
> Known flakes.
> 
> All of the failures appear to be flakes.  The windows browser_tests failures
do
> look to be flakes.  They all report the same error, as indicated above.  I'm
> running another try pass now to see if they are repro.

I ran another try bot job on Win, and there are still failures at
window_impl.cc(54), but for a different, and much larger set of tests.  I
strongly feel that these are flakes.

Build summary:
http://build.chromium.org/p/tryserver.chromium/builders/win/builds/2292


> 
> > 
> > Jeff

[vandebo (ex-Chrome), 2012-03-15 20:40:56]

LGTM if you're confident that the try failures aren't caused by these changes.

https://chromiumcodereview.appspot.com/9416017/diff/85001/skia/ext/vector_pla...
File skia/ext/vector_platform_device_skia.cc (left):

https://chromiumcodereview.appspot.com/9416017/diff/85001/skia/ext/vector_pla...
skia/ext/vector_platform_device_skia.cc:56: SkCanvas
canvas(raster_surface_.get());
On 2012/03/15 19:01:28, Jeff Timanus wrote:
> On 2012/03/14 23:38:29, vandebo wrote:
> > nit: Why is/was this here?
> 
> I noticed this, and was also confused.  I left it as is, because I wanted to
> minimize any impact this change may have had.  
> 
> It was introduced when the PDF back-end was brought into chrome.  See
> http://src.chromium.org/viewvc/chrome?view=rev&revision=80873

Ahh - in that revision, the device is cleared to black using the canvas.  At
some point that was removed.

[commit-bot: I haz the power, 2012-03-15 21:06:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/twiz@chromium.org/9416017/86017

[commit-bot: I haz the power, 2012-03-15 21:06:30]

Presubmit check for 9416017-86017 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    webkit
    ui/gfx

Presubmit checks took 1.3s to calculate.

[Jeff Timanus, 2012-03-15 21:24:17]

James & Mike:  Can you please LGTM stamp the changes in webkit, and ui/gfx.

Thanks,

Jeff

[jamesr, 2012-03-15 21:25:51]

webkit/ LGTM

[msw, 2012-03-15 21:53:07]

+Alexei, who is/knows a better reviewer for this CL.
Sorry, I'm really only a ui/gfx OWNER for RenderText.

https://chromiumcodereview.appspot.com/9416017/diff/86017/ui/gfx/canvas_skia.cc
File ui/gfx/canvas_skia.cc (right):

https://chromiumcodereview.appspot.com/9416017/diff/86017/ui/gfx/canvas_skia....
ui/gfx/canvas_skia.cc:58: // skia::PlatformCanvas instances are initialized to 0
by Cairo on Linux, but
Should this be done in Skia / SkCanvas?

https://chromiumcodereview.appspot.com/9416017/diff/86017/ui/gfx/canvas_skia....
ui/gfx/canvas_skia.cc:60: owned_canvas_->clear(SkColorSetARGB(0, 0, 0, 0));
Can this be skipped for is_opaque == true or false?
What's the perf impact here?
I presume it's better than the status quo, given the bug.

https://chromiumcodereview.appspot.com/9416017/diff/86017/ui/gfx/canvas_skia_...
File ui/gfx/canvas_skia_win.cc (right):

https://chromiumcodereview.appspot.com/9416017/diff/86017/ui/gfx/canvas_skia_...
ui/gfx/canvas_skia_win.cc:554: gradient_canvas->clear(SkColorSetARGB(0, 0, 0,
0));
Is this needed by other direct SkCanvas users too?
It seems like this ought to be in the SkCanvas impl.

[Jeff Timanus, 2012-03-15 22:26:53]

Mike, you are right about the possibility of skipping the clear based on
is_opaque.

At one point, I had switched on this value, but I felt that it was an
abstraction leak.  The is_opaque flag's purpose is to inform skia of the usage
intent of the bitmap.  

I chose to follow the behaviour of the mac port:  Always clear when there is no
shared handle/context given, which is the case when constructing PlatformCanvas
objects.

https://chromiumcodereview.appspot.com/9416017/diff/86017/ui/gfx/canvas_skia.cc
File ui/gfx/canvas_skia.cc (right):

https://chromiumcodereview.appspot.com/9416017/diff/86017/ui/gfx/canvas_skia....
ui/gfx/canvas_skia.cc:58: // skia::PlatformCanvas instances are initialized to 0
by Cairo on Linux, but
On 2012/03/15 21:53:07, msw wrote:
> Should this be done in Skia / SkCanvas?

SkCanvas used to construct initialized 0, but that has since changed.  The
assumption is that SkCanvas instances are not initialized.  This change carries
those semantics to skia/ext.

https://chromiumcodereview.appspot.com/9416017/diff/86017/ui/gfx/canvas_skia....
ui/gfx/canvas_skia.cc:60: owned_canvas_->clear(SkColorSetARGB(0, 0, 0, 0));
On 2012/03/15 21:53:07, msw wrote:
> Can this be skipped for is_opaque == true or false?
> What's the perf impact here?
> I presume it's better than the status quo, given the bug.

You're right, but the old behaviour was a little confusing.

On Mac, a PlatformCanvas would always be constructed with a cleared canvas,
regardless of the is_opaque argument.

On Win, the clear would only happen if !is_opaque. So there is a potential for a
perf hit here on Win.

The hit is on the order of ~2-3 ms (from observed results).

https://chromiumcodereview.appspot.com/9416017/diff/86017/ui/gfx/canvas_skia_...
File ui/gfx/canvas_skia_win.cc (right):

https://chromiumcodereview.appspot.com/9416017/diff/86017/ui/gfx/canvas_skia_...
ui/gfx/canvas_skia_win.cc:554: gradient_canvas->clear(SkColorSetARGB(0, 0, 0,
0));
On 2012/03/15 21:53:07, msw wrote:
> Is this needed by other direct SkCanvas users too?
> It seems like this ought to be in the SkCanvas impl.

I searched through all users of SkCanvas (and assoc.), and found that only the
sites in this change needed the clear.

Putting it in the impl forces the perf impact for all canvas users, even those
that don't need the clear.  An earlier revision of this change introduced a
construction argument to request the clear, but I found that since there are so
few sites requiring the clear, it was easier to simply assume that
PlatformCanvas instances are not initialized.

[Alexei Svitkine (slow), 2012-03-15 22:49:10]

In addition to my comment below, could you add comments to the constructors and
Create() functions documenting that the contents are not cleared on Mac and
Windows?

https://chromiumcodereview.appspot.com/9416017/diff/86017/skia/ext/vector_pla...
File skia/ext/vector_platform_device_skia.cc (right):

https://chromiumcodereview.appspot.com/9416017/diff/86017/skia/ext/vector_pla...
skia/ext/vector_platform_device_skia.cc:60: canvas.clear(SkColorSetARGB(0, 0, 0,
0));
This whole function is kind of nasty.

Could you add a BitmapPlatformDevice::CreateAndClear(int width, int height, bool
opaque); function that will do the right thing on each platform?

[Jeff Timanus, 2012-03-16 03:10:31]

Alexei, I added the function you suggested, and made use of it where applicable.

I also converted the clear logic to that of the windows port:  If !is_opaque,
then the backing bitmap is cleared to erase 0, otherwise the bitmap remains
uninitialized.

Please take another look.

(Try-bots pending.)

https://chromiumcodereview.appspot.com/9416017/diff/86017/skia/ext/vector_pla...
File skia/ext/vector_platform_device_skia.cc (right):

https://chromiumcodereview.appspot.com/9416017/diff/86017/skia/ext/vector_pla...
skia/ext/vector_platform_device_skia.cc:60: canvas.clear(SkColorSetARGB(0, 0, 0,
0));
On 2012/03/15 22:49:11, Alexei Svitkine wrote:
> This whole function is kind of nasty.
> 
> Could you add a BitmapPlatformDevice::CreateAndClear(int width, int height,
bool
> opaque); function that will do the right thing on each platform?

Done.

[Alexei Svitkine (slow), 2012-03-16 05:57:56]

LGTM. Thanks!

https://chromiumcodereview.appspot.com/9416017/diff/83003/skia/ext/bitmap_pla...
File skia/ext/bitmap_platform_device_linux.h (right):

https://chromiumcodereview.appspot.com/9416017/diff/83003/skia/ext/bitmap_pla...
skia/ext/bitmap_platform_device_linux.h:74: // Constructs a device with size
|width| * |height|.with contents initialized
Replace the . after |height| with a space.

[msw, 2012-03-16 07:13:22]

I don't fully understand this code, so I'll defer approval to Alexei and others.
I noted some nits and confusing bits; hope that helps; sorry I can't help more.

https://chromiumcodereview.appspot.com/9416017/diff/83003/skia/ext/bitmap_pla...
File skia/ext/bitmap_platform_device_android.cc (right):

https://chromiumcodereview.appspot.com/9416017/diff/83003/skia/ext/bitmap_pla...
skia/ext/bitmap_platform_device_android.cc:40: } else {
nit: this could now be "else if (!bitmap...)" and you can drop the braces.

https://chromiumcodereview.appspot.com/9416017/diff/83003/skia/ext/bitmap_pla...
File skia/ext/bitmap_platform_device_android.h (right):

https://chromiumcodereview.appspot.com/9416017/diff/83003/skia/ext/bitmap_pla...
skia/ext/bitmap_platform_device_android.h:30: static BitmapPlatformDevice*
CreateAndClear(int width, int height,
nit: one line per arg if they don't all fit (here and elsewhere).
it seems like that rule isn't popular here; ignore me if i'm wrong.

https://chromiumcodereview.appspot.com/9416017/diff/83003/skia/ext/bitmap_pla...
skia/ext/bitmap_platform_device_android.h:33: // This doesn't take ownership of
|data|. If |data| is null, the bitmap
It appears the bitmap is not initialized regardless of |data|'s value.

https://chromiumcodereview.appspot.com/9416017/diff/83003/skia/ext/bitmap_pla...
File skia/ext/bitmap_platform_device_linux.h (right):

https://chromiumcodereview.appspot.com/9416017/diff/83003/skia/ext/bitmap_pla...
skia/ext/bitmap_platform_device_linux.h:85: // This doesn't take ownership of
|data|. If |data| is NULL, the contents
is this contingent on |data|'s value? sounds like it's always cleared.

https://chromiumcodereview.appspot.com/9416017/diff/83003/skia/ext/bitmap_pla...
File skia/ext/bitmap_platform_device_win.cc (right):

https://chromiumcodereview.appspot.com/9416017/diff/83003/skia/ext/bitmap_pla...
skia/ext/bitmap_platform_device_win.cc:132: if (!shared_section) {
nit: combine nested ifs, drop braces, put the whole block in the #ifndef.

https://chromiumcodereview.appspot.com/9416017/diff/83003/skia/ext/bitmap_pla...
File skia/ext/bitmap_platform_device_win.h (right):

https://chromiumcodereview.appspot.com/9416017/diff/83003/skia/ext/bitmap_pla...
skia/ext/bitmap_platform_device_win.h:38: // If shared_section is null, the
bitmap backing store is not initialized.
nit: vertical bars around |shared_section|.

https://chromiumcodereview.appspot.com/9416017/diff/83003/skia/ext/bitmap_pla...
skia/ext/bitmap_platform_device_win.h:46: // Create a BitmapPlatformDevice wit
no shared section.  The bitmap is not
spelling nit: with

[Jeff Timanus, 2012-03-16 15:52:41]

https://chromiumcodereview.appspot.com/9416017/diff/83003/skia/ext/bitmap_pla...
File skia/ext/bitmap_platform_device_android.cc (right):

https://chromiumcodereview.appspot.com/9416017/diff/83003/skia/ext/bitmap_pla...
skia/ext/bitmap_platform_device_android.cc:40: } else {
On 2012/03/16 07:13:22, msw wrote:
> nit: this could now be "else if (!bitmap...)" and you can drop the braces.

Done.

https://chromiumcodereview.appspot.com/9416017/diff/83003/skia/ext/bitmap_pla...
File skia/ext/bitmap_platform_device_android.h (right):

https://chromiumcodereview.appspot.com/9416017/diff/83003/skia/ext/bitmap_pla...
skia/ext/bitmap_platform_device_android.h:30: static BitmapPlatformDevice*
CreateAndClear(int width, int height,
On 2012/03/16 07:13:22, msw wrote:
> nit: one line per arg if they don't all fit (here and elsewhere).
> it seems like that rule isn't popular here; ignore me if i'm wrong.

I'd prefer to keep with the existing style in this file.  In general, the
skia/ext code doesn't always follow the coding standards consistently.

https://chromiumcodereview.appspot.com/9416017/diff/83003/skia/ext/bitmap_pla...
skia/ext/bitmap_platform_device_android.h:33: // This doesn't take ownership of
|data|. If |data| is null, the bitmap
On 2012/03/16 07:13:22, msw wrote:
> It appears the bitmap is not initialized regardless of |data|'s value.

If |data| is non-null, the bitmap has the pixel data of |data|.  The old
behaviour would clear the bitmap to 0.  The comment makes it very explicit that
if you don't give an initial value, the contents are undefined.

https://chromiumcodereview.appspot.com/9416017/diff/83003/skia/ext/bitmap_pla...
File skia/ext/bitmap_platform_device_linux.h (right):

https://chromiumcodereview.appspot.com/9416017/diff/83003/skia/ext/bitmap_pla...
skia/ext/bitmap_platform_device_linux.h:74: // Constructs a device with size
|width| * |height|.with contents initialized
On 2012/03/16 05:57:56, Alexei Svitkine wrote:
> Replace the . after |height| with a space.

Done.

https://chromiumcodereview.appspot.com/9416017/diff/83003/skia/ext/bitmap_pla...
skia/ext/bitmap_platform_device_linux.h:85: // This doesn't take ownership of
|data|. If |data| is NULL, the contents
On 2012/03/16 07:13:22, msw wrote:
> is this contingent on |data|'s value? sounds like it's always cleared.

My understanding of this code is that if |data| is non-null, then the bitmap
will have the pixel-data contents of |data|.  Otherwise, a 0-initialized bitmap
is created.

https://chromiumcodereview.appspot.com/9416017/diff/83003/skia/ext/bitmap_pla...
File skia/ext/bitmap_platform_device_win.cc (right):

https://chromiumcodereview.appspot.com/9416017/diff/83003/skia/ext/bitmap_pla...
skia/ext/bitmap_platform_device_win.cc:132: if (!shared_section) {
On 2012/03/16 07:13:22, msw wrote:
> nit: combine nested ifs, drop braces, put the whole block in the #ifndef.

Done.

https://chromiumcodereview.appspot.com/9416017/diff/83003/skia/ext/bitmap_pla...
File skia/ext/bitmap_platform_device_win.h (right):

https://chromiumcodereview.appspot.com/9416017/diff/83003/skia/ext/bitmap_pla...
skia/ext/bitmap_platform_device_win.h:38: // If shared_section is null, the
bitmap backing store is not initialized.
On 2012/03/16 07:13:22, msw wrote:
> nit: vertical bars around |shared_section|.

Done.

https://chromiumcodereview.appspot.com/9416017/diff/83003/skia/ext/bitmap_pla...
skia/ext/bitmap_platform_device_win.h:46: // Create a BitmapPlatformDevice wit
no shared section.  The bitmap is not
On 2012/03/16 07:13:22, msw wrote:
> spelling nit: with

Done.

[commit-bot: I haz the power, 2012-03-16 15:53:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/twiz@chromium.org/9416017/92010

[commit-bot: I haz the power, 2012-03-16 17:33:19]

Change committed as 127196