hide Config8888 entirely

hide Config8888 entirely

BUG=skia:

Committed: http://code.google.com/p/skia/source/detail?r=13865

[reed1, 2014-03-18 21:22:28]

have not looked at tests or benches yet

[bsalomon, 2014-03-19 12:36:48]

https://codereview.chromium.org/203993002/diff/1/include/core/SkColorPriv.h
File include/core/SkColorPriv.h (right):

https://codereview.chromium.org/203993002/diff/1/include/core/SkColorPriv.h#n...
include/core/SkColorPriv.h:21: #define SkASSERT_IS_BYTE(x)     SkASSERT(0 ==
((x) & 0xFF))
Isn't this asserting that the lower 8 are zero?

https://codereview.chromium.org/203993002/diff/1/include/core/SkColorPriv.h#n...
include/core/SkColorPriv.h:63: #define SK_PMCOLOR_IS_RGBA
It'd be nice to allow the user (SkUserConfig.h, gyp file, ...) to specify these
directly instead of the SHIFT values.

In the transition time we'd have to check for either and if both are defined
verify that they're consistent.

https://codereview.chromium.org/203993002/diff/1/src/core/SkConfig8888.h
File src/core/SkConfig8888.h (right):

https://codereview.chromium.org/203993002/diff/1/src/core/SkConfig8888.h#newc...
src/core/SkConfig8888.h:13: struct SkPixelInfo {
One alternative

template <typename PixelPtr>
struct SkTPixelInfo {
    SkColorType fColorType;
    SkAlphaType fAlphaType;
    size_t      fRowBytes;
    PixelPtr   fPixels;
}

typedef SkTPixelInfo<void*> SkDstPixelInfo;
typedef SkTPixelInfo<const void*> SkSrcPixelInfo;

https://codereview.chromium.org/203993002/diff/1/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

https://codereview.chromium.org/203993002/diff/1/src/gpu/GrContext.cpp#newcod...
src/gpu/GrContext.cpp:1268: static SkColorType swapColorType32(SkColorType ct) {
In the Gr code we're mostly consistent about naming statics with _ rather than
camel, e.g. swap_colortype32 or swap_color_type_32... or something  :)

[reed1, 2014-03-19 17:59:57]

https://codereview.chromium.org/203993002/diff/1/include/core/SkColorPriv.h
File include/core/SkColorPriv.h (right):

https://codereview.chromium.org/203993002/diff/1/include/core/SkColorPriv.h#n...
include/core/SkColorPriv.h:21: #define SkASSERT_IS_BYTE(x)     SkASSERT(0 ==
((x) & 0xFF))
On 2014/03/19 12:36:48, bsalomon wrote:
> Isn't this asserting that the lower 8 are zero?

Doh!

https://codereview.chromium.org/203993002/diff/1/include/core/SkColorPriv.h#n...
include/core/SkColorPriv.h:63: #define SK_PMCOLOR_IS_RGBA
On 2014/03/19 12:36:48, bsalomon wrote:
> It'd be nice to allow the user (SkUserConfig.h, gyp file, ...) to specify
these
> directly instead of the <span id="abbrex-spnAbbr_27630" class="abbrex-Abbr
abbrex-Abbr_SHIFT abbrex-AbbrUnknown abbrex-AbbrUnknownVisible">SHIFT</span>
values.
> 
> In the transition time we'd have to check for either and if both are defined
> verify that they're consistent.

I can try adding checks now that if they've specified PMCOLOR_IS already, that
everything makes sense. I agree that, down the road, we may want to *only*
expose that as a settable define.

https://codereview.chromium.org/203993002/diff/1/src/core/SkConfig8888.h
File src/core/SkConfig8888.h (right):

https://codereview.chromium.org/203993002/diff/1/src/core/SkConfig8888.h#newc...
src/core/SkConfig8888.h:13: struct SkPixelInfo {
On 2014/03/19 12:36:48, bsalomon wrote:
> One alternative
> 
> template <typename PixelPtr>
> struct SkTPixelInfo {
>     SkColorType fColorType;
>     SkAlphaType fAlphaType;
>     size_t      fRowBytes;
>     PixelPtr   fPixels;
> }
> 
> typedef SkTPixelInfo<void*> SkDstPixelInfo;
> typedef SkTPixelInfo<const void*> SkSrcPixelInfo;

I'll play around with this.

https://codereview.chromium.org/203993002/diff/1/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

https://codereview.chromium.org/203993002/diff/1/src/gpu/GrContext.cpp#newcod...
src/gpu/GrContext.cpp:1268: static SkColorType swapColorType32(SkColorType ct) {
On 2014/03/19 12:36:48, bsalomon wrote:
> In the Gr code we're mostly consistent about naming statics with _ rather than
> camel, e.g. swap_colortype32 or swap_color_type_32... or something  :)

I think that's true in general everywhere, so I'll correct it here.

[reed1, 2014-03-19 19:28:41]

ptal

[bsalomon, 2014-03-19 19:48:28]

lgtm w/ q about double checkPixel.

https://codereview.chromium.org/203993002/diff/40001/tests/WritePixelsTest.cpp
File tests/WritePixelsTest.cpp (right):

https://codereview.chromium.org/203993002/diff/40001/tests/WritePixelsTest.cp...
tests/WritePixelsTest.cpp:236: check = checkPixel(bmpPMColor, canvasPixel, mul);
debugging code? I'm tempted to say leave it in as I've put this in myself more
than once.

[reed1, 2014-03-19 20:23:18]

https://codereview.chromium.org/203993002/diff/40001/tests/WritePixelsTest.cpp
File tests/WritePixelsTest.cpp (right):

https://codereview.chromium.org/203993002/diff/40001/tests/WritePixelsTest.cp...
tests/WritePixelsTest.cpp:236: check = checkPixel(bmpPMColor, canvasPixel, mul);
On 2014/03/19 19:48:28, bsalomon wrote:
> debugging code? I'm tempted to say leave it in as I've put this in myself more
> than once.

Yes, that was for local debugging.

Wish that sort of thing could be automatically available in some mode.

I think I'll remove for now for now, as it may be confusing to another reader,
but lets do try to find a canonical way to do this.

[reed1, 2014-03-19 20:25:09]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-03-19 20:25:14]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/reed@google.com/203993002/60001

[commit-bot: I haz the power, 2014-03-19 21:20:40]

Change committed as 13865

[jcgregorio, 2014-03-20 03:32:42]

On 2014/03/19 21:20:40, I haz the power (commit-bot) wrote:
> Change committed as 13865

This change breaks the build, in particular when compiling chrome:

../../third_party/skia/src/core/SkBitmapDevice.cpp: In member function ‘virtual
bool SkBitmapDevice::onReadPixels(const SkBitmap&, int, int,
 SkCanvas::Config8888)’:
../../third_party/skia/src/core/SkBitmapDevice.cpp:199:78: error:
‘SkCopyBitmapToConfig8888’ was not declared in this scope

See:
http://108.170.217.252:10115/builders/Canary-Chrome-Win7-Ninja-x86-SharedLib/...

Unfortunately 5 subsequent CLs landed before I could close the tree, so I'm not
even sure a revert would cleanly apply at this point.