introduce Props to surface

introduce Props to surface (work in progress)

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/3716fd067a5621bb94a6cb08d72afec8bf3aceda

[reed1, 2014-09-10 19:24:31]

didn't (yet) incorporate the gpu-specific flags into Props -- distance-fields,
clear-or-not.

thoughts?

[reed1, 2014-09-10 19:26:11]

reed@google.com changed reviewers:
+ bsalomon@google.com, jvanverth@google.com, robertphillips@google.com

[reed1, 2014-09-10 19:26:12]

Eventually I guess some of these props need to filter down to canvas and device
-- I'm just assuming that they should start in Surface (esp. if we want them to
be immutable, and not vary during the runtime of a surface or canvas).

[reed1, 2014-09-11 16:01:15]

reed@google.com changed reviewers:
+ bungeman@google.com, fmalita@google.com, vangelis@chromium.org

[reed1, 2014-09-11 21:27:22]

Now with plumbing down to rasterdevice.

Still need to understand how this meshes with DeviceProperties (merge? copy?)

That said, I think this is somewhat "correct" for the rasterdevice case, so PTAL

[reed1, 2014-09-15 20:04:46]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-09-15 20:04:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/551463004/80001

[commit-bot: I haz the power, 2014-09-15 20:04:58]

Note for Reviewers:
The CQ is waiting for an approval.
If you believe that the CL is not ready yet, or if you would like to L-G-T-M
with comments then please uncheck the CQ checkbox.

Waiting for LGTM from a valid reviewer till 2014-09-16 02:04 UTC

[reed1, 2014-09-15 20:08:35]

The CQ bit was unchecked by reed@google.com

[bsalomon, 2014-09-15 20:09:05]

Seems like this should supplant deviceproperties.

https://codereview.chromium.org/551463004/diff/80001/include/core/SkSurfacePr...
File include/core/SkSurfaceProps.h (right):

https://codereview.chromium.org/551463004/diff/80001/include/core/SkSurfacePr...
include/core/SkSurfaceProps.h:32: SkIPoint        fOrigin;
Origin needs some explanation.

I'm torn between specific disables and higher level info (e.g. types of
post-transforms that are going to be applied and some ppi-related info).

[bungeman-skia, 2014-09-15 20:47:13]

https://codereview.chromium.org/551463004/diff/80001/include/core/SkSurfacePr...
File include/core/SkSurfaceProps.h (right):

https://codereview.chromium.org/551463004/diff/80001/include/core/SkSurfacePr...
include/core/SkSurfaceProps.h:15: kLegacyLCD_SkPixelGeometry,
Throw this in at end and give big scary name and assert that it's never used
explicitly?

https://codereview.chromium.org/551463004/diff/80001/src/core/SkBitmapDevice.cpp
File src/core/SkBitmapDevice.cpp (right):

https://codereview.chromium.org/551463004/diff/80001/src/core/SkBitmapDevice....
src/core/SkBitmapDevice.cpp:386: return kN32_SkColorType != target.colorType();
comment that we do this because we don't have a blitter.

note that we do support path effect and fake bold with lcd text, this should
also include alternate fill styles.

The rasterizer and mask effect are implementation depended on why we don't do
them with lcd, it's just more code for something I doubt anyone cares that much
about.

The xfer mode I have no idea about.



I know this is in an in-between state, but the end goal is that the paint/font
has a bit on it that states 'exploit subpixel geometry' which is always passed
through to the scalar context (so that we know if to back form from lcd if
needed) and the scalar context is also told what the geometry actually is. Note
that all of these bits need to be in the scalar descriptor, because a request
for 'lcd on unknown' may look different from 'normal on unknown'.

[robertphillips, 2014-09-16 14:15:20]

https://codereview.chromium.org/551463004/diff/100001/include/core/SkSurface.h
File include/core/SkSurface.h (right):

https://codereview.chromium.org/551463004/diff/100001/include/core/SkSurface....
include/core/SkSurface.h:210: 
Why not "const Props&" ?

https://codereview.chromium.org/551463004/diff/100001/include/core/SkSurfaceP...
File include/core/SkSurfaceProps.h (right):

https://codereview.chromium.org/551463004/diff/100001/include/core/SkSurfaceP...
include/core/SkSurfaceProps.h:1: /*
2014 ?

https://codereview.chromium.org/551463004/diff/100001/include/core/SkSurfaceP...
include/core/SkSurfaceProps.h:7: 
SkSurfaceProps_DEFINED ?

https://codereview.chromium.org/551463004/diff/100001/include/core/SkSurfaceP...
include/core/SkSurfaceProps.h:12: 
// informative comment ?

https://codereview.chromium.org/551463004/diff/100001/include/core/SkSurfaceP...
include/core/SkSurfaceProps.h:21: 
// The surface properties class allows a client to specified the desired
properties of an SkSurface. It can control the:
//    origin - allowing the client to specify the coordinate space of the
surface (equivalent to a translate immediately after creation).
//    antialiasing - allowing the client to turn off AA for all draws to the
surface. Note that this overrides any AA flag passed to individual draw calls
...

?

https://codereview.chromium.org/551463004/diff/100001/src/core/SkBitmapDevice...
File src/core/SkBitmapDevice.cpp (right):

https://codereview.chromium.org/551463004/diff/100001/src/core/SkBitmapDevice...
src/core/SkBitmapDevice.cpp:59: 
Why this change?

https://codereview.chromium.org/551463004/diff/100001/src/core/SkBitmapDevice...
src/core/SkBitmapDevice.cpp:377: 
Rename to supports_lcd and switch sense?

https://codereview.chromium.org/551463004/diff/100001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/551463004/diff/100001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:75: return paint.getRasterizer() || paint.getPathEffect()
|| paint.isFakeBoldText() ||
Tab these two lines over?

https://codereview.chromium.org/551463004/diff/100001/src/image/SkSurface.cpp
File src/image/SkSurface.cpp (right):

https://codereview.chromium.org/551463004/diff/100001/src/image/SkSurface.cpp...
src/image/SkSurface.cpp:23: : INHERITED(width, height, props)
Shouldn't the '{' be on the line above?

https://codereview.chromium.org/551463004/diff/100001/src/image/SkSurface.cpp...
src/image/SkSurface.cpp:30: : INHERITED(info, props)
ditto?

https://codereview.chromium.org/551463004/diff/100001/src/image/SkSurface.cpp...
src/image/SkSurface.cpp:90: : fProps(props), fWidth(width), fHeight(height)
ditto?

https://codereview.chromium.org/551463004/diff/100001/src/image/SkSurface.cpp...
src/image/SkSurface.cpp:98: : fProps(props), fWidth(info.width()),
fHeight(info.height())
ditto?

https://codereview.chromium.org/551463004/diff/100001/src/image/SkSurface_Gpu...
File src/image/SkSurface_Gpu.cpp (right):

https://codereview.chromium.org/551463004/diff/100001/src/image/SkSurface_Gpu...
src/image/SkSurface_Gpu.cpp:39: : INHERITED(renderTarget->width(),
renderTarget->height(), Props())
line above ?

https://codereview.chromium.org/551463004/diff/100001/src/image/SkSurface_Ras...
File src/image/SkSurface_Raster.cpp (right):

https://codereview.chromium.org/551463004/diff/100001/src/image/SkSurface_Ras...
src/image/SkSurface_Raster.cpp:104: SkCanvas* SkSurface_Raster::onNewCanvas() {
const SkSurfacePropos& props = ?

[robertphillips, 2014-09-16 14:22:44]

This is going to require some pretty exhaustive unit testing.

[reed1, 2014-09-16 18:16:01]

https://codereview.chromium.org/551463004/diff/100001/include/core/SkSurface.h
File include/core/SkSurface.h (right):

https://codereview.chromium.org/551463004/diff/100001/include/core/SkSurface....
include/core/SkSurface.h:210: 
On 2014/09/16 14:15:17, robertphillips wrote:
> Why not "const Props&" ?

Done.

https://codereview.chromium.org/551463004/diff/100001/include/core/SkSurfaceP...
File include/core/SkSurfaceProps.h (right):

https://codereview.chromium.org/551463004/diff/100001/include/core/SkSurfaceP...
include/core/SkSurfaceProps.h:1: /*
On 2014/09/16 14:15:18, robertphillips wrote:
> 2014 ?

Done.

https://codereview.chromium.org/551463004/diff/100001/include/core/SkSurfaceP...
include/core/SkSurfaceProps.h:7: 
On 2014/09/16 14:15:18, robertphillips wrote:
> SkSurfaceProps_DEFINED ?

Done.

https://codereview.chromium.org/551463004/diff/100001/include/core/SkSurfaceP...
include/core/SkSurfaceProps.h:12: 
On 2014/09/16 14:15:17, robertphillips wrote:
> // informative comment ?

Done.

https://codereview.chromium.org/551463004/diff/100001/include/core/SkSurfaceP...
include/core/SkSurfaceProps.h:21: 
On 2014/09/16 14:15:18, robertphillips wrote:
> // The surface properties class allows a client to specified the desired
> properties of an SkSurface. It can control the:
> //    origin - allowing the client to specify the coordinate space of the
> surface (equivalent to a translate immediately after creation).
> //    antialiasing - allowing the client to turn off AA for all draws to the
> surface. Note that this overrides any AA flag passed to individual draw calls
> ...
> 
> ?

Done.

https://codereview.chromium.org/551463004/diff/100001/src/core/SkBitmapDevice...
File src/core/SkBitmapDevice.cpp (right):

https://codereview.chromium.org/551463004/diff/100001/src/core/SkBitmapDevice...
src/core/SkBitmapDevice.cpp:59: 
On 2014/09/16 14:15:18, robertphillips wrote:
> Why this change?

Historical edit/unedit. Fixed.

https://codereview.chromium.org/551463004/diff/100001/src/core/SkBitmapDevice...
src/core/SkBitmapDevice.cpp:377: 
On 2014/09/16 14:15:18, robertphillips wrote:
> Rename to supports_lcd and switch sense?

Done.

https://codereview.chromium.org/551463004/diff/100001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/551463004/diff/100001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:75: return paint.getRasterizer() || paint.getPathEffect()
|| paint.isFakeBoldText() ||
On 2014/09/16 14:15:18, robertphillips wrote:
> Tab these two lines over?

Done.

https://codereview.chromium.org/551463004/diff/100001/src/image/SkSurface.cpp
File src/image/SkSurface.cpp (right):

https://codereview.chromium.org/551463004/diff/100001/src/image/SkSurface.cpp...
src/image/SkSurface.cpp:23: : INHERITED(width, height, props)
On 2014/09/16 14:15:19, robertphillips wrote:
> Shouldn't the '{' be on the line above?

Actually, I think when we have indented initializers, the curly should be on the
left, otherwise the initializers stack on top of the first line of code.

https://codereview.chromium.org/551463004/diff/100001/src/image/SkSurface_Ras...
File src/image/SkSurface_Raster.cpp (right):

https://codereview.chromium.org/551463004/diff/100001/src/image/SkSurface_Ras...
src/image/SkSurface_Raster.cpp:104: SkCanvas* SkSurface_Raster::onNewCanvas() {
On 2014/09/16 14:15:19, robertphillips wrote:
> const SkSurfacePropos& props = ?

Done.

[bungeman-skia, 2014-09-16 19:18:54]

https://codereview.chromium.org/551463004/diff/120001/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

https://codereview.chromium.org/551463004/diff/120001/include/core/SkCanvas.h...
include/core/SkCanvas.h:139: SkCanvas(const SkBitmap& bitmap, const
SkSurfaceProps* = NULL);
This should still be explicit.

12.3.1 Conversion by constructor [class.conv.ctor]
A constructor declared without the function-specifier explicit specifies a
conversion from the types of its parameters to the type of its class. Such a
constructor is called a converting constructor. 

Example:
struct X {
  X(int);
  X(const char*, int =0);
};
void f(X arg) {
  X a = 1; // a = X(1)
  X b = "Jessie"; // b = X("Jessie",0)
  a = 2; // a = X(2)
  f(3); // f(X(3))
}

[reed1, 2014-09-16 19:38:47]

https://codereview.chromium.org/551463004/diff/120001/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

https://codereview.chromium.org/551463004/diff/120001/include/core/SkCanvas.h...
include/core/SkCanvas.h:139: SkCanvas(const SkBitmap& bitmap, const
SkSurfaceProps* = NULL);
On 2014/09/16 19:18:54, bungeman1 wrote:
> This should still be explicit.
> 
> 12.3.1 Conversion by constructor [class.conv.ctor]
> A constructor declared without the function-specifier explicit specifies a
> conversion from the types of its parameters to the type of its class. Such a
> constructor is called a converting constructor. 
> 
> Example:
> struct X {
>   X(int);
>   X(const char*, int =0);
> };
> void f(X arg) {
>   X a = 1; // a = X(1)
>   X b = "Jessie"; // b = X("Jessie",0)
>   a = 2; // a = X(2)
>   f(3); // f(X(3))
> }

Done.

[reed1, 2014-09-18 18:05:57]

PTAL

[reed1, 2014-09-18 18:11:04]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-09-18 18:11:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/551463004/280001

[commit-bot: I haz the power, 2014-09-18 18:11:21]

Note for Reviewers:
The CQ is waiting for an approval.
If you believe that the CL is not ready yet, or if you would like to L-G-T-M
with comments then please uncheck the CQ checkbox.

Waiting for LGTM from a valid reviewer till 2014-09-19 00:11 UTC

[commit-bot: I haz the power, 2014-09-18 18:18:00]

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

[commit-bot: I haz the power, 2014-09-18 18:18:01]

Try jobs failed on following builders:
  Test-Ubuntu13.10-GCE-NoGPU-x86_64-Debug-Trybot on tryserver.skia
(http://108.170.220.120:10117/builders/Test-Ubuntu13.10-GCE-NoGPU-x86_64-Debug...)

[bungeman-skia, 2014-09-18 18:31:21]

https://codereview.chromium.org/551463004/diff/280001/gm/surface.cpp
File gm/surface.cpp (right):

https://codereview.chromium.org/551463004/diff/280001/gm/surface.cpp#newcode83
gm/surface.cpp:83: for (int disallowAA = 0; disallowAA <= 1; ++disallowAA) {
for (bool aa = false; !aa; aa = true) {

[reed1, 2014-09-18 18:38:13]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-09-18 18:38:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/551463004/300001

[commit-bot: I haz the power, 2014-09-18 18:38:41]

Note for Reviewers:
The CQ is waiting for an approval.
If you believe that the CL is not ready yet, or if you would like to L-G-T-M
with comments then please uncheck the CQ checkbox.

Waiting for LGTM from a valid reviewer till 2014-09-19 00:38 UTC

[commit-bot: I haz the power, 2014-09-18 18:38:50]

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

[commit-bot: I haz the power, 2014-09-18 18:38:52]

Failed to apply patch for samplecode/SampleApp.cpp:
While running git apply --index -p1;
  error: patch failed: samplecode/SampleApp.cpp:275
  error: samplecode/SampleApp.cpp: patch does not apply

Patch:       samplecode/SampleApp.cpp
Index: samplecode/SampleApp.cpp
diff --git a/samplecode/SampleApp.cpp b/samplecode/SampleApp.cpp
index
9d7e727ce5e87d46347facaf9e36b007bd9eaafe..11aeddbbc7e221921804122a2e6237674cc7def7
100644
--- a/samplecode/SampleApp.cpp
+++ b/samplecode/SampleApp.cpp
@@ -275,7 +275,8 @@ public:
                                    SampleWindow* win) {
 #if SK_SUPPORT_GPU
         if (IsGpuDeviceType(dType) && fCurContext) {
-            SkAutoTUnref<SkBaseDevice>
device(SkGpuDevice::Create(fCurRenderTarget));
+            SkAutoTUnref<SkBaseDevice>
device(SkGpuDevice::Create(fCurRenderTarget,
+                                     
SkSurfaceProps(SkSurfaceProps::kLegacyFontHost_InitType)));
             return new SkCanvas(device);
         } else
 #endif

[bsalomon, 2014-09-18 18:43:33]

https://codereview.chromium.org/551463004/diff/280001/include/core/SkSurface.h
File include/core/SkSurface.h (right):

https://codereview.chromium.org/551463004/diff/280001/include/core/SkSurface....
include/core/SkSurface.h:108: static SkSurface* NewRenderTarget(GrContext*,
const SkImageInfo&, int sampleCount,
Can we deprecate the above version? What do you think of moving the clear bit
into Props?

https://codereview.chromium.org/551463004/diff/280001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/551463004/diff/280001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:271: fPaint = &fOrigPaint;
SkTCopyOnFirstWrite?

https://codereview.chromium.org/551463004/diff/280001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:300: SkPaint* paint = fLazyPaint.set(fOrigPaint);
fPaint->writable()->setFlags(fNewPaintFlags);

[reed1, 2014-09-18 21:03:12]

PTAL

https://codereview.chromium.org/551463004/diff/280001/include/core/SkSurface.h
File include/core/SkSurface.h (right):

https://codereview.chromium.org/551463004/diff/280001/include/core/SkSurface....
include/core/SkSurface.h:108: static SkSurface* NewRenderTarget(GrContext*,
const SkImageInfo&, int sampleCount,
On 2014/09/18 18:43:32, bsalomon wrote:
> Can we deprecate the above version? What do you think of moving the clear bit
> into Props?

Done.

https://codereview.chromium.org/551463004/diff/280001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/551463004/diff/280001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:271: fPaint = &fOrigPaint;
On 2014/09/18 18:43:33, bsalomon wrote:
> SkTCopyOnFirstWrite?

? fPaint is a pointer, so this line is free.

[bungeman-skia, 2014-09-18 21:14:10]

https://codereview.chromium.org/551463004/diff/320001/gm/surface.cpp
File gm/surface.cpp (right):

https://codereview.chromium.org/551463004/diff/320001/gm/surface.cpp#newcode79
gm/surface.cpp:79: kBGR_H_SkPixelGeometry,
Can we have at least one of the verticals here?

https://codereview.chromium.org/551463004/diff/320001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/551463004/diff/320001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:389: #include "SkDeviceProperties.h"
Is this being used here?

[bsalomon, 2014-09-18 21:20:24]

https://codereview.chromium.org/551463004/diff/280001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/551463004/diff/280001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:271: fPaint = &fOrigPaint;
On 2014/09/18 21:03:12, reed1 wrote:
> On 2014/09/18 18:43:33, bsalomon wrote:
> > SkTCopyOnFirstWrite?
> 
> ? fPaint is a pointer, so this line is free.

Init'ing a SkTCopyOnFirstWrite isn't costly and it captures the
point-at-const-or-modified-copy idiom.

[reed1, 2014-09-18 21:24:41]

https://codereview.chromium.org/551463004/diff/320001/gm/surface.cpp
File gm/surface.cpp (right):

https://codereview.chromium.org/551463004/diff/320001/gm/surface.cpp#newcode79
gm/surface.cpp:79: kBGR_H_SkPixelGeometry,
On 2014/09/18 21:14:10, bungeman1 wrote:
> Can we have at least one of the verticals here?

Done.

https://codereview.chromium.org/551463004/diff/320001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/551463004/diff/320001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:389: #include "SkDeviceProperties.h"
On 2014/09/18 21:14:10, bungeman1 wrote:
> Is this being used here?

Done.

[reed1, 2014-09-18 21:25:27]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-09-18 21:25:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/551463004/360001

[commit-bot: I haz the power, 2014-09-18 21:25:51]

Note for Reviewers:
The CQ is waiting for an approval.
If you believe that the CL is not ready yet, or if you would like to L-G-T-M
with comments then please uncheck the CQ checkbox.

Waiting for LGTM from a valid reviewer till 2014-09-19 03:25 UTC

[commit-bot: I haz the power, 2014-09-18 21:31:54]

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

[commit-bot: I haz the power, 2014-09-18 21:31:55]

Try jobs failed on following builders:
  Test-Ubuntu13.10-GCE-NoGPU-x86_64-Debug-Trybot on tryserver.skia
(http://108.170.220.120:10117/builders/Test-Ubuntu13.10-GCE-NoGPU-x86_64-Debug...)

[reed1, 2014-09-19 12:51:04]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-09-19 12:51:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/551463004/400001

[commit-bot: I haz the power, 2014-09-19 12:51:32]

Note for Reviewers:
The CQ is waiting for an approval.
If you believe that the CL is not ready yet, or if you would like to L-G-T-M
with comments then please uncheck the CQ checkbox.

Waiting for LGTM from a valid reviewer till 2014-09-19 18:51 UTC

[commit-bot: I haz the power, 2014-09-19 12:58:08]

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

[commit-bot: I haz the power, 2014-09-19 12:58:11]

Try jobs failed on following builders:
  Test-Ubuntu13.10-GCE-NoGPU-x86_64-Debug-Trybot on tryserver.skia
(http://108.170.220.120:10117/builders/Test-Ubuntu13.10-GCE-NoGPU-x86_64-Debug...)

[reed1, 2014-09-19 15:17:02]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-09-19 15:17:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/551463004/420001

[commit-bot: I haz the power, 2014-09-19 15:17:39]

Note for Reviewers:
The CQ is waiting for an approval.
If you believe that the CL is not ready yet, or if you would like to L-G-T-M
with comments then please uncheck the CQ checkbox.

Waiting for LGTM from a valid reviewer till 2014-09-19 21:17 UTC

[commit-bot: I haz the power, 2014-09-19 15:17:55]

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

[commit-bot: I haz the power, 2014-09-19 15:17:57]

Failed to apply patch for include/core/SkSurface.h:
While running git apply --index -p1;
  <stdin>:80: trailing whitespace.
      
  <stdin>:84: trailing whitespace.
      
  <stdin>:99: trailing whitespace.
      
  <stdin>:103: trailing whitespace.
      
  error: patch failed: include/core/SkSurface.h:52
  error: include/core/SkSurface.h: patch does not apply

Patch:       include/core/SkSurface.h
Index: include/core/SkSurface.h
diff --git a/include/core/SkSurface.h b/include/core/SkSurface.h
index
cb5133ae003e4d89de0f2f376b749977aee715f4..b1034d50bcc8d28f8abf877dba66a316863b4bd3
100644
--- a/include/core/SkSurface.h
+++ b/include/core/SkSurface.h
@@ -10,6 +10,9 @@
 
 #include "SkRefCnt.h"
 #include "SkImage.h"
+#include "SkSurfaceProps.h"
+
+//#define SK_SUPPORT_LEGACY_TEXTRENDERMODE
 
 class SkCanvas;
 class SkPaint;
@@ -35,7 +38,8 @@ public:
      *  If the requested surface cannot be created, or the request is not a
      *  supported configuration, NULL will be returned.
      */
-    static SkSurface* NewRasterDirect(const SkImageInfo&, void* pixels, size_t
rowBytes);
+    static SkSurface* NewRasterDirect(const SkImageInfo&, void* pixels, size_t
rowBytes,
+                                      const SkSurfaceProps* = NULL);
 
     /**
      *  The same as NewRasterDirect, but also accepts a call-back routine,
which is invoked
@@ -43,7 +47,7 @@ public:
      */
     static SkSurface* NewRasterDirectReleaseProc(const SkImageInfo&, void*
pixels, size_t rowBytes,
                                                  void (*releaseProc)(void*
pixels, void* context),
-                                                 void* context);
+                                                 void* context, const
SkSurfaceProps* = NULL);
 
     /**
      *  Return a new surface, with the memory for the pixels automatically
@@ -52,31 +56,17 @@ public:
      *  If the requested surface cannot be created, or the request is not a
      *  supported configuration, NULL will be returned.
      */
-    static SkSurface* NewRaster(const SkImageInfo&);
+    static SkSurface* NewRaster(const SkImageInfo&, const SkSurfaceProps* =
NULL);
 
     /**
      *  Helper version of NewRaster. It creates a SkImageInfo with the
      *  specified width and height, and populates the rest of info to match
      *  pixels in SkPMColor format.
      */
-    static SkSurface* NewRasterPMColor(int width, int height) {
-        return NewRaster(SkImageInfo::MakeN32Premul(width, height));
+    static SkSurface* NewRasterPMColor(int width, int height, const
SkSurfaceProps* props = NULL) {
+        return NewRaster(SkImageInfo::MakeN32Premul(width, height), props);
     }
 
-    /**
-     *  Text rendering modes that can be passed to NewRenderTarget*
-     */
-    enum TextRenderMode {
-        /**
-         *  This will use the standard text rendering method
-         */
-        kStandard_TextRenderMode,
-        /**
-         *  This will use signed distance fields for text rendering when
possible
-         */
-        kDistanceField_TextRenderMode,
-    };
-
     enum RenderTargetFlags {
         kNone_RenderTargetFlag      = 0x0,
         /*
@@ -89,17 +79,29 @@ public:
     /**
      *  Return a new surface using the specified render target.
      */
-    static SkSurface* NewRenderTargetDirect(GrRenderTarget*,
-                                            TextRenderMode trm =
kStandard_TextRenderMode,
-                                            RenderTargetFlags flags =
kNone_RenderTargetFlag);
-
+    static SkSurface* NewRenderTargetDirect(GrRenderTarget*, const
SkSurfaceProps*,
+                                            RenderTargetFlags);
+    
+    static SkSurface* NewRenderTargetDirect(GrRenderTarget* target) {
+        return NewRenderTargetDirect(target, NULL, kNone_RenderTargetFlag);
+    }
+    
     /**
      *  Return a new surface whose contents will be drawn to an offscreen
      *  render target, allocated by the surface.
      */
-    static SkSurface* NewRenderTarget(GrContext*, const SkImageInfo&, int
sampleCount = 0,
-                                      TextRenderMode trm =
kStandard_TextRenderMode,
-                                      RenderTargetFlags flags =
kNone_RenderTargetFlag);
+    static SkSurface* NewRenderTarget(GrContext*, const SkImageInfo&, int
sampleCount,
+                                      const SkSurfaceProps*,
+                                      RenderTargetFlags =
kNone_RenderTargetFlag);
+
+    static SkSurface* NewRenderTarget(GrContext* gr, const SkImageInfo& info) {
+        return NewRenderTarget(gr, info, 0, NULL, kNone_RenderTargetFlag);
+    }
+    
+    static SkSurface* NewRenderTarget(GrContext* gr, const SkImageInfo& info,
int sampleCount) {
+        return NewRenderTarget(gr, info, sampleCount, NULL,
kNone_RenderTargetFlag);
+    }
+    
 
     /**
      *  Return a new surface whose contents will be drawn to an offscreen
@@ -113,9 +115,36 @@ public:
      *  Note: Scratch textures count against the GrContext's cached resource
      *  budget.
      */
-    static SkSurface* NewScratchRenderTarget(GrContext*, const SkImageInfo&,
int sampleCount = 0,
-                                             TextRenderMode trm =
kStandard_TextRenderMode,
+    static SkSurface* NewScratchRenderTarget(GrContext*, const SkImageInfo&,
int sampleCount,
+                                             const SkSurfaceProps*,
RenderTargetFlags);
+
+    static SkSurface* NewScratchRenderTarget(GrContext* gr, const SkImageInfo&
info) {
+        return NewScratchRenderTarget(gr, info, 0, NULL,
kNone_RenderTargetFlag);
+    }
+
+#ifdef SK_SUPPORT_LEGACY_TEXTRENDERMODE
+    /**
+     *  Text rendering modes that can be passed to NewRenderTarget*
+     */
+    enum TextRenderMode {
+        /**
+         *  This will use the standard text rendering method
+         */
+        kStandard_TextRenderMode,
+        /**
+         *  This will use signed distance fields for text rendering when
possible
+         */
+        kDistanceField_TextRenderMode,
+    };
+    static SkSurface* NewRenderTargetDirect(GrRenderTarget*, TextRenderMode,
+                                            RenderTargetFlags flags =
kNone_RenderTargetFlag);
+    static SkSurface* NewRenderTarget(GrContext*, const SkImageInfo&, int
sampleCount,
+                                      TextRenderMode,
+                                      RenderTargetFlags flags =
kNone_RenderTargetFlag);
+    static SkSurface* NewScratchRenderTarget(GrContext*, const SkImageInfo&,
int sampleCount,
+                                             TextRenderMode,
                                              RenderTargetFlags flags =
kNone_RenderTargetFlag);
+#endif
 
     int width() const { return fWidth; }
     int height() const { return fHeight; }
@@ -204,9 +233,11 @@ public:
      */
     const void* peekPixels(SkImageInfo* info, size_t* rowBytes);
 
+    const SkSurfaceProps& props() const { return fProps; }
+
 protected:
-    SkSurface(int width, int height);
-    SkSurface(const SkImageInfo&);
+    SkSurface(int width, int height, const SkSurfaceProps*);
+    SkSurface(const SkImageInfo&, const SkSurfaceProps*);
 
     // called by subclass if their contents have changed
     void dirtyGenerationID() {
@@ -214,9 +245,10 @@ protected:
     }
 
 private:
-    const int   fWidth;
-    const int   fHeight;
-    uint32_t    fGenerationID;
+    const SkSurfaceProps fProps;
+    const int            fWidth;
+    const int            fHeight;
+    uint32_t             fGenerationID;
 
     typedef SkRefCnt INHERITED;
 };

[reed1, 2014-09-19 17:42:13]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-09-19 17:42:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/551463004/440001

[commit-bot: I haz the power, 2014-09-19 17:42:58]

Note for Reviewers:
The CQ is waiting for an approval.
If you believe that the CL is not ready yet, or if you would like to L-G-T-M
with comments then please uncheck the CQ checkbox.

Waiting for LGTM from a valid reviewer till 2014-09-19 23:42 UTC

[commit-bot: I haz the power, 2014-09-19 17:46:43]

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

[commit-bot: I haz the power, 2014-09-19 17:46:45]

Try jobs failed on following builders:
  Test-Ubuntu13.10-GCE-NoGPU-x86_64-Debug-Trybot on tryserver.skia
(http://108.170.220.120:10117/builders/Test-Ubuntu13.10-GCE-NoGPU-x86_64-Debug...)

[reed1, 2014-09-19 17:57:28]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-09-19 17:57:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/551463004/460001

[commit-bot: I haz the power, 2014-09-19 17:57:59]

Note for Reviewers:
The CQ is waiting for an approval.
If you believe that the CL is not ready yet, or if you would like to L-G-T-M
with comments then please uncheck the CQ checkbox.

Waiting for LGTM from a valid reviewer till 2014-09-19 23:57 UTC

[bsalomon, 2014-09-19 17:58:23]

I'd say more but I'm too busy playing with my beach ball.

https://codereview.chromium.org/551463004/diff/440001/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

https://codereview.chromium.org/551463004/diff/440001/include/core/SkCanvas.h...
include/core/SkCanvas.h:206: *  props are passed to the new surface.
It seems little confusing that a canvas has surface properties, since as far as
the public API is concerned it doesn't.

https://codereview.chromium.org/551463004/diff/440001/include/core/SkSurface.h
File include/core/SkSurface.h (right):

https://codereview.chromium.org/551463004/diff/440001/include/core/SkSurface....
include/core/SkSurface.h:86: static SkSurface* NewRenderTarget(GrContext* gr,
const SkImageInfo& info) {
Will we get rid of this one?

https://codereview.chromium.org/551463004/diff/440001/include/core/SkSurface....
include/core/SkSurface.h:105: static SkSurface*
NewScratchRenderTarget(GrContext* gr, const SkImageInfo& info) {
and this one?

[reed1, 2014-09-19 18:04:49]

https://codereview.chromium.org/551463004/diff/440001/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

https://codereview.chromium.org/551463004/diff/440001/include/core/SkCanvas.h...
include/core/SkCanvas.h:206: *  props are passed to the new surface.
On 2014/09/19 17:58:23, bsalomon wrote:
> It seems little confusing that a canvas has surface properties, since as far
as
> the public API is concerned it doesn't.

But a canvas can (should?) be made from a surface, and passing NULL means create
a surface that is "compatible" with the one that created this canvas. If the
canvas has only "default" props, then so will the newly created surface.

https://codereview.chromium.org/551463004/diff/440001/include/core/SkSurface.h
File include/core/SkSurface.h (right):

https://codereview.chromium.org/551463004/diff/440001/include/core/SkSurface....
include/core/SkSurface.h:86: static SkSurface* NewRenderTarget(GrContext* gr,
const SkImageInfo& info) {
On 2014/09/19 17:58:23, bsalomon wrote:
> Will we get rid of this one?

We previously had this variant, with lots of default args. I just wanted to
capture that more explicitly. Are you suggesting we migrate clients away from
having this option?

[bsalomon, 2014-09-19 18:21:06]

https://codereview.chromium.org/551463004/diff/440001/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

https://codereview.chromium.org/551463004/diff/440001/include/core/SkCanvas.h...
include/core/SkCanvas.h:206: *  props are passed to the new surface.
On 2014/09/19 18:04:49, reed1 wrote:
> On 2014/09/19 17:58:23, bsalomon wrote:
> > It seems little confusing that a canvas has surface properties, since as far
> as
> > the public API is concerned it doesn't.
> 
> But a canvas can (should?) be made from a surface, and passing NULL means
create
> a surface that is "compatible" with the one that created this canvas. If the
> canvas has only "default" props, then so will the newly created surface.

Something like this?

"""
If surfaceprops is specified, those are passed to the new surface. Otherwise,
the new
surface inherits the properties of the surface that owns this canvas. If this
canvas is not owned by a surface it will receive the default surface properties.
"""

https://codereview.chromium.org/551463004/diff/440001/include/core/SkSurface.h
File include/core/SkSurface.h (right):

https://codereview.chromium.org/551463004/diff/440001/include/core/SkSurface....
include/core/SkSurface.h:86: static SkSurface* NewRenderTarget(GrContext* gr,
const SkImageInfo& info) {
On 2014/09/19 18:04:49, reed1 wrote:
> On 2014/09/19 17:58:23, bsalomon wrote:
> > Will we get rid of this one?
> 
> We previously had this variant, with lots of default args. I just wanted to
> capture that more explicitly. Are you suggesting we migrate clients away from
> having this option?

Personally, I'd rather have fewer factories at the expense of making callers say
how many samples they want. I expect over time the use of MSAA will grow and
just assuming 0 will be less common than it is today.

[robertphillips, 2014-09-19 18:25:37]

https://codereview.chromium.org/551463004/diff/440001/gm/surface.cpp
File gm/surface.cpp (right):

https://codereview.chromium.org/551463004/diff/440001/gm/surface.cpp#newcode15
gm/surface.cpp:15: 
make_linear_gradient ?

https://codereview.chromium.org/551463004/diff/440001/gm/surface.cpp#newcode58
gm/surface.cpp:58: 
// Exercise SkSurfaceProps variants. This renders a test scene for four
AA/dithering
// combinations (AA/Dither, AA/noDither, noAA/Dither, noAA/noDither) crossed
with
// 5 pixel geometry options.

?

https://codereview.chromium.org/551463004/diff/440001/gm/surface.cpp#newcode63
gm/surface.cpp:63: protected:
SK_OVERRIDE(s) ?
virtual on onShortName ?

https://codereview.chromium.org/551463004/diff/440001/include/core/SkSurfaceP...
File include/core/SkSurfaceProps.h (right):

https://codereview.chromium.org/551463004/diff/440001/include/core/SkSurfaceP...
include/core/SkSurfaceProps.h:27: *  Describes properties and constraints of a
given SkSurface. The rendering engine can parse these
optimize.

https://codereview.chromium.org/551463004/diff/440001/include/core/SkSurfaceP...
include/core/SkSurfaceProps.h:37: };
Just one each public/private block ?

https://codereview.chromium.org/551463004/diff/440001/include/core/SkSurfaceP...
include/core/SkSurfaceProps.h:52: 
aaDisallowed?
ditherDisallowed?

https://codereview.chromium.org/551463004/diff/440001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/551463004/diff/440001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:463: 
Why not CopyOrLegacy?

https://codereview.chromium.org/551463004/diff/440001/src/image/SkSurface.cpp
File src/image/SkSurface.cpp (right):

https://codereview.chromium.org/551463004/diff/440001/src/image/SkSurface.cpp...
src/image/SkSurface.cpp:117: 
Use CopyOrLegacy?

https://codereview.chromium.org/551463004/diff/440001/src/image/SkSurface.cpp...
src/image/SkSurface.cpp:198: 
overlength

[bungeman-skia, 2014-09-19 18:29:51]

https://codereview.chromium.org/551463004/diff/460001/src/core/SkDeviceProper...
File src/core/SkDeviceProperties.h (right):

https://codereview.chromium.org/551463004/diff/460001/src/core/SkDeviceProper...
src/core/SkDeviceProperties.h:10: SkDeviceProperties(InitType);
If this is only ever supposed to be passed the one value, why take the value?

https://codereview.chromium.org/551463004/diff/460001/src/core/SkPaint.cpp
File src/core/SkPaint.cpp (right):

https://codereview.chromium.org/551463004/diff/460001/src/core/SkPaint.cpp#ne...
src/core/SkPaint.cpp:1545:
SkDeviceProperties::SkDeviceProperties(SkDeviceProperties::InitType)
This is only used by SkBaseDevice, why is it here?

https://codereview.chromium.org/551463004/diff/460001/src/core/SkPaint.cpp#ne...
src/core/SkPaint.cpp:1624: ? deviceProperties->fPixelGeometry
nit: this '?' and ':' should line up with the '=' above.

[bungeman-skia, 2014-09-19 18:59:12]

https://codereview.chromium.org/551463004/diff/460001/include/core/SkSurfaceP...
File include/core/SkSurfaceProps.h (right):

https://codereview.chromium.org/551463004/diff/460001/include/core/SkSurfaceP...
include/core/SkSurfaceProps.h:19: kUnknown_SkPixelGeometry,
I'd like this one (Unknown) to come last, simply because every time I look at
this I want the low bit to be RGB/BGR and the next bit to be H/V. The fact that
they are almost like that is driving me bananas.

https://codereview.chromium.org/551463004/diff/460001/include/core/SkSurfaceP...
include/core/SkSurfaceProps.h:47: // pixel geometry cannot be
kLegacyLCD_SkPixelGeometry
kLegacyLCD_SkPixelGeometry doesn't exist anymore?

https://codereview.chromium.org/551463004/diff/460001/include/core/SkSurfaceP...
include/core/SkSurfaceProps.h:55: bool useDistanceFieldFonts() const { return
SkToBool(fFlags & kUseDistanceFieldFonts_Flag); }
It seems like these should be named 'disallowsX' and 'usesX' so that call sites
read better and makes it more obvious that these are getters and not magic
setters.

[reed1, 2014-09-19 19:16:35]

https://codereview.chromium.org/551463004/diff/440001/gm/surface.cpp
File gm/surface.cpp (right):

https://codereview.chromium.org/551463004/diff/440001/gm/surface.cpp#newcode15
gm/surface.cpp:15: 
On 2014/09/19 18:25:36, robertphillips wrote:
> make_linear_gradient ?

hmm, not important to the caller, so I chose to keep it flexible.

https://codereview.chromium.org/551463004/diff/440001/gm/surface.cpp#newcode58
gm/surface.cpp:58: 
On 2014/09/19 18:25:36, robertphillips wrote:
> // Exercise SkSurfaceProps variants. This renders a test scene for four
> AA/dithering
> // combinations (AA/Dither, AA/noDither, noAA/Dither, noAA/noDither) crossed
> with
> // 5 pixel geometry options.
> 
> ?

Done.

https://codereview.chromium.org/551463004/diff/440001/gm/surface.cpp#newcode63
gm/surface.cpp:63: protected:
On 2014/09/19 18:25:36, robertphillips wrote:
> SK_OVERRIDE(s) ?
> virtual on onShortName ?

Done.

https://codereview.chromium.org/551463004/diff/440001/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

https://codereview.chromium.org/551463004/diff/440001/include/core/SkCanvas.h...
include/core/SkCanvas.h:206: *  props are passed to the new surface.
On 2014/09/19 18:21:05, bsalomon wrote:
> On 2014/09/19 18:04:49, reed1 wrote:
> > On 2014/09/19 17:58:23, bsalomon wrote:
> > > It seems little confusing that a canvas has surface properties, since as
far
> > as
> > > the public API is concerned it doesn't.
> > 
> > But a canvas can (should?) be made from a surface, and passing NULL means
> create
> > a surface that is "compatible" with the one that created this canvas. If the
> > canvas has only "default" props, then so will the newly created surface.
> 
> Something like this?
> 
> """
> If surfaceprops is specified, those are passed to the new surface. Otherwise,
> the new
> surface inherits the properties of the surface that owns this canvas. If this
> canvas is not owned by a surface it will receive the default surface
properties.
> """
> 

Done.

https://codereview.chromium.org/551463004/diff/440001/include/core/SkSurface.h
File include/core/SkSurface.h (right):

https://codereview.chromium.org/551463004/diff/440001/include/core/SkSurface....
include/core/SkSurface.h:86: static SkSurface* NewRenderTarget(GrContext* gr,
const SkImageInfo& info) {
On 2014/09/19 18:21:05, bsalomon wrote:
> On 2014/09/19 18:04:49, reed1 wrote:
> > On 2014/09/19 17:58:23, bsalomon wrote:
> > > Will we get rid of this one?
> > 
> > We previously had this variant, with lots of default args. I just wanted to
> > capture that more explicitly. Are you suggesting we migrate clients away
from
> > having this option?
> 
> Personally, I'd rather have fewer factories at the expense of making callers
say
> how many samples they want. I expect over time the use of MSAA will grow and
> just assuming 0 will be less common than it is today.

No problem with that, but that will affect existing call sites in chrome, so I'd
prefer for that sort of orthogonal change to happen afterwards.

https://codereview.chromium.org/551463004/diff/440001/include/core/SkSurfaceP...
File include/core/SkSurfaceProps.h (right):

https://codereview.chromium.org/551463004/diff/440001/include/core/SkSurfaceP...
include/core/SkSurfaceProps.h:27: *  Describes properties and constraints of a
given SkSurface. The rendering engine can parse these
On 2014/09/19 18:25:36, robertphillips wrote:
> optimize.

Done.

https://codereview.chromium.org/551463004/diff/440001/include/core/SkSurfaceP...
include/core/SkSurfaceProps.h:37: };
On 2014/09/19 18:25:37, robertphillips wrote:
> Just one each public/private block ?

Done.

https://codereview.chromium.org/551463004/diff/440001/include/core/SkSurfaceP...
include/core/SkSurfaceProps.h:52: 
On 2014/09/19 18:25:36, robertphillips wrote:
> aaDisallowed?
> ditherDisallowed?

when we don't "name" the field for the getter, we add isFoo, so I'll do that.

https://codereview.chromium.org/551463004/diff/440001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/551463004/diff/440001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:463: 
On 2014/09/19 18:25:37, robertphillips wrote:
> Why not CopyOrLegacy?

Done.

https://codereview.chromium.org/551463004/diff/440001/src/image/SkSurface.cpp
File src/image/SkSurface.cpp (right):

https://codereview.chromium.org/551463004/diff/440001/src/image/SkSurface.cpp...
src/image/SkSurface.cpp:117: 
On 2014/09/19 18:25:37, robertphillips wrote:
> Use CopyOrLegacy?

Done.

https://codereview.chromium.org/551463004/diff/460001/include/core/SkSurfaceP...
File include/core/SkSurfaceProps.h (right):

https://codereview.chromium.org/551463004/diff/460001/include/core/SkSurfaceP...
include/core/SkSurfaceProps.h:19: kUnknown_SkPixelGeometry,
On 2014/09/19 18:59:12, bungeman1 wrote:
> I'd like this one (Unknown) to come last, simply because every time I look at
> this I want the low bit to be RGB/BGR and the next bit to be H/V. The fact
that
> they are almost like that is driving me bananas.

I'd rather add helpers to answer those predicates. Having the caller know the
order is too fragile (and you can't distinguish between unknown and RGB by
looking at the low bit alone).

https://codereview.chromium.org/551463004/diff/460001/include/core/SkSurfaceP...
include/core/SkSurfaceProps.h:47: // pixel geometry cannot be
kLegacyLCD_SkPixelGeometry
On 2014/09/19 18:59:11, bungeman1 wrote:
> kLegacyLCD_SkPixelGeometry doesn't exist anymore?

Done.

https://codereview.chromium.org/551463004/diff/460001/include/core/SkSurfaceP...
include/core/SkSurfaceProps.h:55: bool useDistanceFieldFonts() const { return
SkToBool(fFlags & kUseDistanceFieldFonts_Flag); }
On 2014/09/19 18:59:12, bungeman1 wrote:
> It seems like these should be named 'disallowsX' and 'usesX' so that call
sites
> read better and makes it more obvious that these are getters and not magic
> setters.

isDisallowAA -- to match the pattern we use in SkPaint.h and other places

https://codereview.chromium.org/551463004/diff/460001/src/core/SkDeviceProper...
File src/core/SkDeviceProperties.h (right):

https://codereview.chromium.org/551463004/diff/460001/src/core/SkDeviceProper...
src/core/SkDeviceProperties.h:10: SkDeviceProperties(InitType);
On 2014/09/19 18:29:51, bungeman1 wrote:
> If this is only ever supposed to be passed the one value, why take the value?

I want to not have a default constructor, so I can find (fix/remove) all
occurrences now and down the road.

https://codereview.chromium.org/551463004/diff/460001/src/core/SkPaint.cpp
File src/core/SkPaint.cpp (right):

https://codereview.chromium.org/551463004/diff/460001/src/core/SkPaint.cpp#ne...
src/core/SkPaint.cpp:1545:
SkDeviceProperties::SkDeviceProperties(SkDeviceProperties::InitType)
On 2014/09/19 18:29:51, bungeman1 wrote:
> This is only used by SkBaseDevice, why is it here?

will remove

https://codereview.chromium.org/551463004/diff/460001/src/core/SkPaint.cpp#ne...
src/core/SkPaint.cpp:1624: ? deviceProperties->fPixelGeometry
On 2014/09/19 18:29:51, bungeman1 wrote:
> nit: this '?' and ':' should line up with the '=' above.

Done.

[bsalomon, 2014-09-19 19:25:22]

lgtm

https://codereview.chromium.org/551463004/diff/440001/include/core/SkSurface.h
File include/core/SkSurface.h (right):

https://codereview.chromium.org/551463004/diff/440001/include/core/SkSurface....
include/core/SkSurface.h:86: static SkSurface* NewRenderTarget(GrContext* gr,
const SkImageInfo& info) {
On 2014/09/19 19:16:33, reed1 wrote:
> On 2014/09/19 18:21:05, bsalomon wrote:
> > On 2014/09/19 18:04:49, reed1 wrote:
> > > On 2014/09/19 17:58:23, bsalomon wrote:
> > > > Will we get rid of this one?
> > > 
> > > We previously had this variant, with lots of default args. I just wanted
to
> > > capture that more explicitly. Are you suggesting we migrate clients away
> from
> > > having this option?
> > 
> > Personally, I'd rather have fewer factories at the expense of making callers
> say
> > how many samples they want. I expect over time the use of MSAA will grow and
> > just assuming 0 will be less common than it is today.
> 
> No problem with that, but that will affect existing call sites in chrome, so
I'd
> prefer for that sort of orthogonal change to happen afterwards.

sgtm

[bungeman-skia, 2014-09-19 19:35:13]

https://codereview.chromium.org/551463004/diff/480001/include/core/SkSurfaceP...
File include/core/SkSurfaceProps.h (right):

https://codereview.chromium.org/551463004/diff/480001/include/core/SkSurfaceP...
include/core/SkSurfaceProps.h:11: #include "SkPoint.h"
Is this being used?

[bungeman-skia, 2014-09-19 19:52:24]

https://codereview.chromium.org/551463004/diff/480001/src/core/SkDeviceProper...
File src/core/SkDeviceProperties.h (right):

https://codereview.chromium.org/551463004/diff/480001/src/core/SkDeviceProper...
src/core/SkDeviceProperties.h:22: bool useBGR() const {
Is this needed anymore, this is the same as SkPixelGeometryIsBGR?

[reed1, 2014-09-19 20:34:21]

https://codereview.chromium.org/551463004/diff/480001/include/core/SkSurfaceP...
File include/core/SkSurfaceProps.h (right):

https://codereview.chromium.org/551463004/diff/480001/include/core/SkSurfaceP...
include/core/SkSurfaceProps.h:11: #include "SkPoint.h"
On 2014/09/19 19:35:12, bungeman1 wrote:
> Is this being used?

Done.

https://codereview.chromium.org/551463004/diff/480001/src/core/SkDeviceProper...
File src/core/SkDeviceProperties.h (right):

https://codereview.chromium.org/551463004/diff/480001/src/core/SkDeviceProper...
src/core/SkDeviceProperties.h:22: bool useBGR() const {
On 2014/09/19 19:52:24, bungeman1 wrote:
> Is this needed anymore, this is the same as SkPixelGeometryIsBGR?

Done.

[bungeman-skia, 2014-09-19 20:51:17]

Can't say I can keep all of this in my head, but lgtm.

[reed2, 2014-09-20 02:50:01]

The CQ bit was checked by reed@chromium.org

[commit-bot: I haz the power, 2014-09-20 02:50:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/551463004/500001

[commit-bot: I haz the power, 2014-09-20 02:53:40]

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

[commit-bot: I haz the power, 2014-09-20 02:53:42]

Try jobs failed on following builders:
  Test-Ubuntu13.10-GCE-NoGPU-x86_64-Debug-Trybot on tryserver.skia
(http://108.170.220.120:10117/builders/Test-Ubuntu13.10-GCE-NoGPU-x86_64-Debug...)
  Test-Ubuntu13.10-GCE-NoGPU-x86_64-Release-Shared-Trybot on tryserver.skia
(http://108.170.220.120:10117/builders/Test-Ubuntu13.10-GCE-NoGPU-x86_64-Relea...)

[reed1, 2014-09-21 16:30:05]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-09-21 16:30:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/551463004/520001

[commit-bot: I haz the power, 2014-09-21 16:40:04]

Committed patchset #27 (id:520001) as 3716fd067a5621bb94a6cb08d72afec8bf3aceda

[reed2, 2014-09-21 17:23:24]

A revert of this CL (patchset #27 id:520001) has been created in
https://codereview.chromium.org/583773004/ by reed@chromium.org.

The reason for reverting is: Broke call site in WebKit.

[bungeman-skia, 2014-09-22 18:06:19]

On 2014/09/21 17:23:24, reed2 wrote:
> A revert of this CL (patchset #27 id:520001) has been created in
> https://codereview.chromium.org/583773004/ by mailto:reed@chromium.org.
> 
> The reason for reverting is: Broke call site in WebKit.

CL continued in https://codereview.chromium.org/588143004 .