Making GraphicsContext the owner of PlatformContext.

Making GraphicsContext the owner of PlatformContext.

This will simplify future work.

R=jamesr@chromium.org
BUG=235468
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=149484

[jamesr, 2013-04-29 23:56:17]

lgtm with a few comments

https://codereview.chromium.org/14550002/diff/4001/Source/WebKit/chromium/src...
File Source/WebKit/chromium/src/painting/GraphicsContextBuilder.h (right):

https://codereview.chromium.org/14550002/diff/4001/Source/WebKit/chromium/src...
Source/WebKit/chromium/src/painting/GraphicsContextBuilder.h:40: class
GraphicsContextBuilder {
this class can definitely go away (as can WebCanvas, for that matter). I think
this existed just to paper over the differences between Skia and CG backed
canvases and the slightly awkward PlatformContext setup routine

https://codereview.chromium.org/14550002/diff/4001/Source/core/page/FrameView...
File Source/core/page/FrameView.cpp (right):

https://codereview.chromium.org/14550002/diff/4001/Source/core/page/FrameView...
Source/core/page/FrameView.cpp:2993: GraphicsContext context(0); // NULL canvas
to get a non-painting context.
woah, so this does a "paint" just to update random bits of state? weird!

https://codereview.chromium.org/14550002/diff/4001/Source/core/platform/graph...
File Source/core/platform/graphics/GraphicsContext.cpp (right):

https://codereview.chromium.org/14550002/diff/4001/Source/core/platform/graph...
Source/core/platform/graphics/GraphicsContext.cpp:73: delete m_data;
can you instead make m_data an OwnPtr<> since you're changing the type anyway?
or just make the storage inline if there's not a specific reason it should be
heap-allocated?

https://codereview.chromium.org/14550002/diff/4001/Source/core/platform/graph...
File Source/core/platform/graphics/GraphicsContext.h (right):

https://codereview.chromium.org/14550002/diff/4001/Source/core/platform/graph...
Source/core/platform/graphics/GraphicsContext.h:157: GraphicsContext(SkCanvas*);
nit: not your fault, but this c'tor should be explicit

[Stephen Chennney, 2013-04-30 00:37:45]

Thanks for the comments. A zillion tests are failing so clearly I'm doing
something wrong. :-(

I'll incorporate the comments into whatever fix I need for the tests.

[Stephen Chennney, 2013-04-30 19:08:43]

I've addressed all of Jame's concerns, and fixed the tests (I think). Locally I
got some failures with missing 3d contexts, to we'll see what happens on the try
bots.

[jamesr, 2013-04-30 19:25:16]

On 2013/04/30 19:08:43, Stephen Chenney wrote:
> I've addressed all of Jame's concerns, and fixed the tests (I think). Locally
I
> got some failures with missing 3d contexts, to we'll see what happens on the
try
> bots.

Mind summarizing what needed to change?

[Stephen Chennney, 2013-04-30 19:39:48]

On 2013/04/30 19:25:16, jamesr wrote:
> On 2013/04/30 19:08:43, Stephen Chenney wrote:
> > I've addressed all of Jame's concerns, and fixed the tests (I think).
Locally
> I
> > got some failures with missing 3d contexts, to we'll see what happens on the
> try
> > bots.
> 
> Mind summarizing what needed to change?

Sure.

1. Some of the ImageBuffer methods take a context for an argument; it's the
context for the image being copied into this image. I was using the wrong
context when getting the bitmap.

2. The accelerated allocation path was trying to use a null context (also in
ImageBuffer), so I moved the accelerated canvas flag setting to after the
context is created.

3. I made the PlatformContextSkia an OwnPtr in GraphicsContext.

4. I created a bug for the ridiculous use of paint to set scrollbar corner
properties.

[jamesr, 2013-04-30 19:57:08]

still looks awesome!

https://codereview.chromium.org/14550002/diff/15001/Source/core/page/FrameVie...
File Source/core/page/FrameView.cpp (right):

https://codereview.chromium.org/14550002/diff/15001/Source/core/page/FrameVie...
Source/core/page/FrameView.cpp:2993: // FIXME: THe use of paint seems like
overkill: crbug.com/236892
typo "THe"

https://codereview.chromium.org/14550002/diff/15001/Source/core/platform/grap...
File Source/core/platform/graphics/skia/ImageBufferSkia.cpp (right):

https://codereview.chromium.org/14550002/diff/15001/Source/core/platform/grap...
Source/core/platform/graphics/skia/ImageBufferSkia.cpp:153: bool isAccelerated =
false;
nit: isAccelerated is the same as renderingMode == Accelerated, so I think just
putting that in the setAccelerated() call on line 172 would be cleaner

[Stephen Chennney, 2013-04-30 23:09:52]

Committed patchset #4 manually as r149484 (presubmit successful).

[Stephen Chennney, 2013-04-30 23:10:44]

I took care of the nits before committing. Now onto the real show: merging
PlatformContextSkia and GraphicsContext.

On 2013/04/30 19:57:08, jamesr wrote:
> still looks awesome!
> 
>
https://codereview.chromium.org/14550002/diff/15001/Source/core/page/FrameVie...
> File Source/core/page/FrameView.cpp (right):
> 
>
https://codereview.chromium.org/14550002/diff/15001/Source/core/page/FrameVie...
> Source/core/page/FrameView.cpp:2993: // FIXME: THe use of paint seems like
> overkill: crbug.com/236892
> typo "THe"
> 
>
https://codereview.chromium.org/14550002/diff/15001/Source/core/platform/grap...
> File Source/core/platform/graphics/skia/ImageBufferSkia.cpp (right):
> 
>
https://codereview.chromium.org/14550002/diff/15001/Source/core/platform/grap...
> Source/core/platform/graphics/skia/ImageBufferSkia.cpp:153: bool isAccelerated
=
> false;
> nit: isAccelerated is the same as renderingMode == Accelerated, so I think
just
> putting that in the setAccelerated() call on line 172 would be cleaner