use new PixelSerializer API, remove legacy flag

use new PixelSerializer API, remove legacy flag

BUG=skia:3268
Committed: https://crrev.com/3921d94ff2b6802b552ac57d8fff2398de312296
Cr-Commit-Position: refs/heads/master@{#309358}

[reed1, 2014-12-19 19:30:15]

reed@google.com changed reviewers:
+ piman@chromium.org, scroggo@google.com

[reed1, 2014-12-19 19:30:29]

reed@google.com changed reviewers:
+ reed@google.com

[reed1, 2014-12-19 19:30:29]

need OWNER review

[scroggo, 2014-12-19 20:02:18]

https://codereview.chromium.org/802533005/diff/1/cc/resources/picture.cc
File cc/resources/picture.cc (right):

https://codereview.chromium.org/802533005/diff/1/cc/resources/picture.cc#newc...
cc/resources/picture.cc:45: return false;
This is a change in behavior. The old SkWriteBuffer::writeBitmap used the
existing decoded data if it was there.

I'm not aware of what the caller wants, but just wanted to point out that this
is different.

(The old EncodeBitmap function in gpu_benchmarking_extension.cc unnecessarily
checked refEncodedData, because an even earlier version of writeBitmap did not
call it first.)

https://codereview.chromium.org/802533005/diff/1/cc/resources/picture.cc#newc...
cc/resources/picture.cc:49: void* pixels,
Once https://codereview.chromium.org/788143007/ lands, this is const void*.
(Same for gpu_benchmarking_extension.cc.)

[piman, 2014-12-19 20:21:42]

piman@chromium.org changed reviewers:
+ enne@chromium.org

[piman, 2014-12-19 20:21:42]

LGTM+nits for content/

+enne for cc/

https://codereview.chromium.org/802533005/diff/1/content/renderer/gpu/gpu_ben...
File content/renderer/gpu/gpu_benchmarking_extension.cc (right):

https://codereview.chromium.org/802533005/diff/1/content/renderer/gpu/gpu_ben...
content/renderer/gpu/gpu_benchmarking_extension.cc:62: size_t rowBytes) override
{
nit: row_bytes

https://codereview.chromium.org/802533005/diff/1/content/renderer/gpu/gpu_ben...
content/renderer/gpu/gpu_benchmarking_extension.cc:65: if
(bm.installPixels(info, const_cast<void*>(pixels), rowBytes)) {
nit: no need for const_cast, pixels is already non-const.

[enne (OOO), 2014-12-19 21:15:14]

cc lgtm % scroggo

Is there a Skia bug for this work that can be linked to?

https://codereview.chromium.org/802533005/diff/1/cc/resources/picture.cc
File cc/resources/picture.cc (right):

https://codereview.chromium.org/802533005/diff/1/cc/resources/picture.cc#newc...
cc/resources/picture.cc:45: return false;
On 2014/12/19 at 20:02:18, scroggo wrote:
> This is a change in behavior. The old SkWriteBuffer::writeBitmap used the
existing decoded data if it was there.
> 
> I'm not aware of what the caller wants, but just wanted to point out that this
is different.
> 
> (The old EncodeBitmap function in gpu_benchmarking_extension.cc unnecessarily
checked refEncodedData, because an even earlier version of writeBitmap did not
call it first.)

If this can be done better, that'd be nice, but this is only used for putting
pictures into traces for FrameViewer.

https://codereview.chromium.org/802533005/diff/1/cc/resources/picture.cc#newc...
cc/resources/picture.cc:50: size_t rowBytes) override {
row_bytes

https://codereview.chromium.org/802533005/diff/1/cc/resources/picture.cc#newc...
cc/resources/picture.cc:66: return NULL;
nullptr

[reed1, 2014-12-19 21:30:02]

https://codereview.chromium.org/802533005/diff/1/cc/resources/picture.cc
File cc/resources/picture.cc (right):

https://codereview.chromium.org/802533005/diff/1/cc/resources/picture.cc#newc...
cc/resources/picture.cc:45: return false;
On 2014/12/19 20:02:18, scroggo wrote:
> This is a change in behavior. The old SkWriteBuffer::writeBitmap used the
> existing decoded data if it was there.
> 
> I'm not aware of what the caller wants, but just wanted to point out that this
> is different.
> 
> (The old EncodeBitmap function in gpu_benchmarking_extension.cc unnecessarily
> checked refEncodedData, because an even earlier version of writeBitmap did not
> call it first.)

Done.

https://codereview.chromium.org/802533005/diff/1/cc/resources/picture.cc#newc...
cc/resources/picture.cc:45: return false;
On 2014/12/19 21:15:13, enne wrote:
> On 2014/12/19 at 20:02:18, scroggo wrote:
> > This is a change in behavior. The old SkWriteBuffer::writeBitmap used the
> existing decoded data if it was there.
> > 
> > I'm not aware of what the caller wants, but just wanted to point out that
this
> is different.
> > 
> > (The old EncodeBitmap function in gpu_benchmarking_extension.cc
unnecessarily
> checked refEncodedData, because an even earlier version of writeBitmap did not
> call it first.)
> 
> If this can be done better, that'd be nice, but this is only used for putting
> pictures into traces for FrameViewer.

Acknowledged.

https://codereview.chromium.org/802533005/diff/1/cc/resources/picture.cc#newc...
cc/resources/picture.cc:49: void* pixels,
On 2014/12/19 20:02:18, scroggo wrote:
> Once https://codereview.chromium.org/788143007/ lands, this is const void*.
> (Same for gpu_benchmarking_extension.cc.)

Acknowledged.

https://codereview.chromium.org/802533005/diff/1/cc/resources/picture.cc#newc...
cc/resources/picture.cc:50: size_t rowBytes) override {
On 2014/12/19 21:15:13, enne wrote:
> row_bytes

Done.

https://codereview.chromium.org/802533005/diff/1/cc/resources/picture.cc#newc...
cc/resources/picture.cc:66: return NULL;
On 2014/12/19 21:15:13, enne wrote:
> nullptr

Done.

https://codereview.chromium.org/802533005/diff/1/content/renderer/gpu/gpu_ben...
File content/renderer/gpu/gpu_benchmarking_extension.cc (right):

https://codereview.chromium.org/802533005/diff/1/content/renderer/gpu/gpu_ben...
content/renderer/gpu/gpu_benchmarking_extension.cc:62: size_t rowBytes) override
{
On 2014/12/19 20:21:42, piman (Very slow to review) wrote:
> nit: row_bytes

Done.

https://codereview.chromium.org/802533005/diff/1/content/renderer/gpu/gpu_ben...
content/renderer/gpu/gpu_benchmarking_extension.cc:65: if
(bm.installPixels(info, const_cast<void*>(pixels), rowBytes)) {
On 2014/12/19 20:21:42, piman (Very slow to review) wrote:
> nit: no need for const_cast, pixels is already non-const.

A pending change in skia will require this cast (the signature will be become
const void* pixels). This will not land before that hits.

[reed1, 2014-12-20 17:45:38]

The CQ bit was checked by reed@google.com

[reed1, 2014-12-20 17:45:43]

The CQ bit was unchecked by reed@google.com

[reed1, 2014-12-20 17:47:44]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-12-20 17:48:30]

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

[commit-bot: I haz the power, 2014-12-20 17:53:40]

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

[commit-bot: I haz the power, 2014-12-20 17:53:41]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[reed1, 2014-12-20 17:54:41]

The CQ bit was checked by reed@google.com

[reed1, 2014-12-20 17:54:42]

lgtm

[commit-bot: I haz the power, 2014-12-20 17:55:33]

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

[commit-bot: I haz the power, 2014-12-20 19:54:14]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-12-20 19:55:02]

Patchset 4 (id:??) landed as
https://crrev.com/3921d94ff2b6802b552ac57d8fff2398de312296
Cr-Commit-Position: refs/heads/master@{#309358}