Remove Uint8ClampedArray use from ImageBuffer.h ImageDataBuffer

Remove Uint8ClampedArray use from ImageBuffer.h ImageDataBuffer

Per issue 235436, wtf/Uint8ClampedArray is moving to Core. Platform should
no longer use this type therefore. Replace Uint8ClampedArray use in helper
ImageDataBuffer and update the <canvas> "3d" toDataURL code paths.

Also, const-correct the encodePixel() inputPixel argument (the encoders do
not change the input pixels). Add a bug reference to the PNG encoder.

Filed issue 446853 about null data checking the "3d" toDataURL case. Noted
that the helper class could be removed eventually, perhaps by wrapping the
data in a temporary skia bitmap.

TBR=haraken@chromium.org
BUG=235436, 446853
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=188199

[Noel Gordon, 2015-01-06 11:51:06]

noel@chromium.org changed reviewers:
+ haraken@chromium.org, yukishiino@chromium.org

[haraken, 2015-01-06 12:08:45]

https://codereview.chromium.org/837643002/diff/1/Source/platform/image-encode...
File Source/platform/image-encoders/skia/PNGImageEncoder.cpp (right):

https://codereview.chromium.org/837643002/diff/1/Source/platform/image-encode...
Source/platform/image-encoders/skia/PNGImageEncoder.cpp:71: static bool
encodePixels(IntSize imageSize, const unsigned char* inputPixels, bool
premultiplied, Vector<unsigned char>* output)

Why do you add const and then do const_cast?

[Noel Gordon, 2015-01-06 13:54:57]

https://codereview.chromium.org/837643002/diff/1/Source/platform/image-encode...
File Source/platform/image-encoders/skia/PNGImageEncoder.cpp (right):

https://codereview.chromium.org/837643002/diff/1/Source/platform/image-encode...
Source/platform/image-encoders/skia/PNGImageEncoder.cpp:71: static bool
encodePixels(IntSize imageSize, const unsigned char* inputPixels, bool
premultiplied, Vector<unsigned char>* output)
To workaround libpng, which doesn't use const in its apis (they were defined at
a time when const was not so in vogue or well-supported across compilers).

[Yuki, 2015-01-06 14:57:48]

I've not yet taken a careful and deep look at your CL, but I wanted to tell you
very soon that we cannot use wtf/ArrayPiece in platform/ because we're planning
to remove wtf/ArrayBuffer and wtf/ArrayBufferView.

https://codereview.chromium.org/837643002/diff/1/Source/platform/graphics/Ima...
File Source/platform/graphics/ImageBuffer.h (right):

https://codereview.chromium.org/837643002/diff/1/Source/platform/graphics/Ima...
Source/platform/graphics/ImageBuffer.h:42: #include "wtf/ArrayPiece.h"
We're going to remove wtf/ArrayBuffer.h, wtf/ArrayBufferView.h and its
subclasses.

Since wtf/ArrayPiece depends on wtf/ArrayBuffer and wtf/ArrayBufferView, we
cannot use wtf/ArrayPiece, too.

[Noel Gordon, 2015-01-06 15:11:39]

On 2015/01/06 14:57:48, Yuki wrote:
> I've not yet taken a careful and deep look at your CL, but I wanted to tell
you
> very soon that we cannot use wtf/ArrayPiece in platform/ because we're
planning
> to remove wtf/ArrayBuffer and wtf/ArrayBufferView.

IC. ArrayPiece is a nice class.  So at remove time, how about we copy ArrayPiece
into a new DataPiece class say, remove the ArrayBuffer and ArrayBufferView
specializations from it, and leave DataPiece in WTF as a useful class ?

Of course, we could also just pass a raw pointer.

[Noel Gordon, 2015-01-06 16:26:11]

On 2015/01/06 15:11:39, noel gordon wrote:
> Of course, we could also just pass a raw pointer.

for example, patch set #2.

[Yuki, 2015-01-07 09:05:35]

1. I'm a little afraid of abuse of DataPiece / ArrayPiece.  Unlike ArrayBuffer
or ArrayBufferContents, XxxPiece does not own the data nor manage the lifetime
of the underlying object.  So we need to use it very carefully, and I'm afraid
of abuse.  I think XxxPiece is not that great in general.

For example, StringPiece is only used for function parameter types and it's a
caller's duty to keep the underlying object alive.  By using StringPiece, we
don't need to overload a function for const char*, const string&,
string::const_iterator, etc.  Since many functions take strings as arguments,
StringPiece pays for its potential danger of dangling pointers or pointing to
dead objects.

StringPiece is so common that most developers know how to use it and when they
shouldn't use it.  However, DataPiece and/or ArrayPiece wouldn't be that common,
so I'm afraid of abuse or misuse.  I'm suspicious if DataPiece and/or ArrayPiece
would pay for its potential danger.

I prefer limited use of DataPiece / ArrayPiece to wide use of them.


2. I don't understand why ImageDataBuffer is defined in
platform/graphics/ImageBuffer.h.  My understanding is as follows.

2-1. ImageDataBuffer has nothing to do with ImageData.  ImageDataBuffer can be
defined independent from ImageBuffer and ImageData.  ImageDataToDataURL() is
also independent from ImageBuffer and ImageData.

2-2. XXXImageEncoder::encode() takes 3 parameters; image data, quality, output
bytes.  The image data can be represented by SkBitmap or a pair of {void*,
size_t}.  So we could have two overloaded functions as:
    encode(const SkBitmap&, int quality, Vector)
    encode(const void*, size_t, int quality, Vector)
However, when we'd like to use a template parameter for the image data, it was
inconvenient that the former takes 3 arguments and the latter takes 4 arguments.
 So we'd like the latter to take 3 arguments as:
    encode({void*, size_t}, int quality, Vector)
where {void*, size_t} is a single type.  This was named ImageDataBuffer.

I think it was reasonable to introduce {void*, size_t} to use template.  My
question is why it was NOT defined in XxxImageEncoder?

Suppose that we had a common base class ImageEncoderBase for XxxxImageEncoder
such as:
    class ImageEncoderBase {
        virtual bool encode(SkBitMap, ...) = 0;
        virtual bool encode({void*, size_t}, ...) = 0;
    };
I think it's natural that this class (or header) defines a type for {void*,
size_t}.

Actually we don't have such a base class as an interface though, we should
define a type for {void*, size_t} at a layer of XxxImageEncoder or a lower
layer.

My understanding about layers is:
    layer of core/html/HTMLCanvasElement
    layer of platform/graphics/ImageBuffer
    layer of platform/image-encoders/skia/XxxImageEncoder
    layer of wtf/
in the order from upper layer to lower layer.  Lower layer provides an interface
to upper layer, and upper layer uses lower layer through the interface.

Why don't you define a type for {void*, size_t} at the layer of XxxImageEncoder
or below?  Type {void*, size_t} must be an interface of XxxImageEncoder, why
it's defined in an upper layer of XxxImageEncoder?  It seems quite strange.

Ignoring a discussion if it's okay to introduce DataPiece or not, I think it's
reasonable that wtf/DataPiece is used to represent {void*, size_t} from a point
of view of layering, because wtf/ is a lower layer of XxxImageEncoder.

[Noel Gordon, 2015-01-07 10:51:08]

Interesting discussion.  DataPiece was just an idea, and there'd sure be pros /
cons.  Somewhat orthogonal now, since I uploaded patch-set #2. I have green
bubbles. Could we review #2 please?
 
> 2. I don't understand why ImageDataBuffer is defined in
platform/graphics/ImageBuffer.h.

It was added to WebKit for the reasons listed in ChangeLog available at
trac.webkit.org, jbauman@chromium.org was the patch author from memory.

[Yuki, 2015-01-07 13:00:57]

Although I was not able to find a ChangeLog of ImageDataBuffer, I think it's a
good chance to refactor ImageDataBuffer.  Let's move it out into a proper layer.
 I don't see any good reason to keep ImageDataBuffer in ImageBuffer.h.

https://codereview.chromium.org/837643002/diff/20001/Source/platform/graphics...
File Source/platform/graphics/ImageBuffer.h (right):

https://codereview.chromium.org/837643002/diff/20001/Source/platform/graphics...
Source/platform/graphics/ImageBuffer.h:163: struct ImageDataBuffer : public
IntSize {
ImageDataBuffer is not a size.  So we shouldn't inherit from IntSize.  Public
inheritance should be used only for is-a relationship, and never for has-a
relationship.

We don't want an ImageDataBuffer to be implicitly casted to IntSize.

https://codereview.chromium.org/837643002/diff/20001/Source/platform/image-en...
File Source/platform/image-encoders/skia/JPEGImageEncoder.cpp (right):

https://codereview.chromium.org/837643002/diff/20001/Source/platform/image-en...
Source/platform/image-encoders/skia/JPEGImageEncoder.cpp:212: return
encodePixels(IntSize(imageData.width(), imageData.height()), imageData.pixels(),
false, quality, output);
Why don't you use imageData.size()?

[Noel Gordon, 2015-01-07 13:27:52]

On 2015/01/07 13:00:57, Yuki wrote:
> Although I was not able to find a ChangeLog of ImageDataBuffer

I find when refactoring code, that's it often good to look up the source code
for change history,  and assess test coverage, bug reports, etc.  If a trac
search failed, one can google. A google searched for the author ImageDataBuffer
trac.webkit.org turned up https://bugs.webkit.org/show_bug.cgi?id=56238 for me.

[Noel Gordon, 2015-01-07 13:59:24]

On 2015/01/07 13:00:57, Yuki wrote:
> I think it's a good chance to refactor ImageDataBuffer.  Let's move it out
into a proper layer.
>  I don't see any good reason to keep ImageDataBuffer in ImageBuffer.h.

Yes, though that's t's not the subject of the current bug.  "One bug, one issue"
is better in practice I find.  It never goes well when we mix issues and we need
to revert, for example.  Perhaps file a separate issue about it if interested.

>
https://codereview.chromium.org/837643002/diff/20001/Source/platform/graphics...
> File Source/platform/graphics/ImageBuffer.h (right):
> 
>
https://codereview.chromium.org/837643002/diff/20001/Source/platform/graphics...
> Source/platform/graphics/ImageBuffer.h:163: struct ImageDataBuffer : public
> IntSize {
> ImageDataBuffer is not a size.  So we shouldn't inherit from IntSize.  Public
> inheritance should be used only for is-a relationship, and never for has-a
> relationship.

Sometimes you have to pick between the two, the choice is not so clear cut. 
ImageDataBuffer is a size here only for consistency (see below).

 > We don't want an ImageDataBuffer to be implicitly casted to IntSize.

Why?
 
>
https://codereview.chromium.org/837643002/diff/20001/Source/platform/image-en...
> File Source/platform/image-encoders/skia/JPEGImageEncoder.cpp (right):
> 
>
https://codereview.chromium.org/837643002/diff/20001/Source/platform/image-en...
> Source/platform/image-encoders/skia/JPEGImageEncoder.cpp:212: return
> encodePixels(IntSize(imageData.width(), imageData.height()),
imageData.pixels(),
> false, quality, output);
> Why don't you use imageData.size()?

To match the code for the canvas 2d case:  For example, refer to:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

I used size() here in one prototype of this patch.   I changed it to width() and
height() for consistency so the unfamiliar reader didn't have to wonder what a
size() is or it's orientation.  width / height are more familiar concepts when
dealing with images, at least to me, so on balance I preferred consistency.

[Yuki, 2015-01-07 14:11:33]

On 2015/01/07 13:27:52, noel gordon wrote:
> On 2015/01/07 13:00:57, Yuki wrote:
> > Although I was not able to find a ChangeLog of ImageDataBuffer
> 
> I find when refactoring code, that's it often good to look up the source code
> for change history,  and assess test coverage, bug reports, etc.  If a trac
> search failed, one can google. A google searched for the author
ImageDataBuffer
> http://trac.webkit.org turned up https://bugs.webkit.org/show_bug.cgi?id=56238
for me.

Thanks for the pointer.  I read the patch and it didn't introduce
ImageDataBuffer at that time.  However, I found that encoders used to take an
ImageData as an input.

I think this CL https://codereview.chromium.org/54653004 introduced
ImageDataBuffer.  It replaced ImageData with ImageDataBuffer.  Only the reason I
can see is that:
""""
> - the struct could be moved to ImageBuffer.h since it is only used for
encoding.

I like the second option, just moving the struct into ImageBuffer.
""""
and I see no deeper design discussion.

I think it's mostly a historical reason why we have ImageDataBuffer in
ImageBuffer.h.  It was ImageData, and then ImageDataBuffer.  I haven't found a
good reason to keep ImageDataBuffer in ImageBuffer.h.

[Noel Gordon, 2015-01-07 14:26:11]

On 2015/01/07 14:11:33, Yuki wrote:
> On 2015/01/07 13:27:52, noel gordon wrote:
> > On 2015/01/07 13:00:57, Yuki wrote:
> > > Although I was not able to find a ChangeLog of ImageDataBuffer
> > 
> > I find when refactoring code, that's it often good to look up the source
code
> > for change history,  and assess test coverage, bug reports, etc.  If a trac
> > search failed, one can google. A google searched for the author
> ImageDataBuffer
> > http://trac.webkit.org turned up
https://bugs.webkit.org/show_bug.cgi?id=56238
> for me.
> 
> Thanks for the pointer.  I read the patch and it didn't introduce
> ImageDataBuffer at that time.  However, I found that encoders used to take an
> ImageData as an input.

Yes, ImageDataToDataURL() was added at this time, which started the changes that
lead to ImageDataBuffer.  My apologies for any confusion. 

> I think this CL https://codereview.chromium.org/54653004 introduced
> ImageDataBuffer.  It replaced ImageData with ImageDataBuffer.  Only the reason
I
> can see is that:
> """"
> > - the struct could be moved to ImageBuffer.h since it is only used for
> encoding.
> 
> I like the second option, just moving the struct into ImageBuffer.
> """"

That's one possible option.

> and I see no deeper design discussion.
>
> I think it's mostly a historical reason why we have ImageDataBuffer in
> ImageBuffer.h.  It was ImageData, and then ImageDataBuffer.  I haven't found a
> good reason to keep ImageDataBuffer in ImageBuffer.h.

This is another.  Again not the subject of this bug, but good to know, and
better to handle in a separate bug I think.

[Noel Gordon, 2015-01-07 14:37:40]

Another question i had was about ImageBuffer.cpp.  The "2d" toDataURL path
checks the skia bitmap for null, but the "3d" tpDataURL path does not.  Seems
like a bug to me.  I filed
https://code.google.com/p/chromium/issues/detail?id=446853 about that.

[Yuki, 2015-01-07 14:42:06]

I agree with a plan to do a refactoring in a separate CL.  Could you handle it?

https://codereview.chromium.org/837643002/diff/20001/Source/platform/graphics...
File Source/platform/graphics/ImageBuffer.h (right):

https://codereview.chromium.org/837643002/diff/20001/Source/platform/graphics...
Source/platform/graphics/ImageBuffer.h:163: struct ImageDataBuffer : public
IntSize {
On 2015/01/07 13:00:57, Yuki wrote:
> ImageDataBuffer is not a size.  So we shouldn't inherit from IntSize.  Public
> inheritance should be used only for is-a relationship, and never for has-a
> relationship.
> 
> We don't want an ImageDataBuffer to be implicitly casted to IntSize.

It's simply because an image is not a size.  An image has a size (width and
height), image data, etc.  Like a house is not a door, but a house has doors and
windows.

For example,

    class Window {
        Window(String windowTitle, Size windowSize);
    };

    String title = "...";
    Image backgroundImage = ...;
    Window window = new Window(title, backgroundImage);

We'd like a compile error on the last line, because Window's ctor requires a
title and a window size, but we're passing a background image which we'd like to
show in the window.

If we'd like to create a window with the size of the image, then we should
explicitly write:

    Window window = new Window(title, backgroundImage.size());

Then, it's clearer for both of compilers and developers.

[Noel Gordon, 2015-01-07 14:44:16]

On 2015/01/07 14:11:33, Yuki wrote:
 
> I think this CL https://codereview.chromium.org/54653004 introduced
> ImageDataBuffer.  It replaced ImageData with ImageDataBuffer.  Only the reason
I
> can see is that:

""""
> - the struct could be moved to ImageBuffer.h since it is only used for
encoding.

I like the second option, just moving the struct into ImageBuffer.
""""

Yes, and according to their bug, they were trying break chrome into multiple
DLL's on windows (so we could compile chrome with MSVC as the code grew larger,
I beleive)
.

[Noel Gordon, 2015-01-07 15:09:32]

On 2015/01/07 14:42:06, Yuki wrote:
> I agree with a plan to do a refactoring in a separate CL.  Could you handle
it?

Well maybe if I could out of here. I have lots of bugs to fix.

>
https://codereview.chromium.org/837643002/diff/20001/Source/platform/graphics...
> File Source/platform/graphics/ImageBuffer.h (right):
> 
>
https://codereview.chromium.org/837643002/diff/20001/Source/platform/graphics...
> Source/platform/graphics/ImageBuffer.h:163: struct ImageDataBuffer : public
> IntSize {
> On 2015/01/07 13:00:57, Yuki wrote:
> > ImageDataBuffer is not a size.  So we shouldn't inherit from IntSize. 
Public
> > inheritance should be used only for is-a relationship, and never for has-a
> > relationship.
> > 
> > We don't want an ImageDataBuffer to be implicitly casted to IntSize.
> 
> It's simply because an image is not a size.  An image has a size (width and
> height), image data, etc.  Like a house is not a door, but a house has doors
and
> windows.

I am aware of the difference b/w image, doors and windows.  I mentioned that I
found I had to make a choice one way or another.  Both had merits, both had
cons.I  also knew I could use a cast to IntSize, but I did not use it in this
case  That was my choice, for consistency.

> For example,
> 
>     class Window {
>         Window(String windowTitle, Size windowSize);
>     };
> 
>     String title = "...";
>     Image backgroundImage = ...;
>     Window window = new Window(title, backgroundImage);
> 
> We'd like a compile error on the last line, because Window's ctor requires a
> title and a window size, but we're passing a background image which we'd like
to
> show in the window.
> 
> If we'd like to create a window with the size of the image, then we should
> explicitly write:
>
>     Window window = new Window(title, backgroundImage.size());

Window window = new Window(title, width, height) would be a possible api also
{1}

[1]
http://msdn.microsoft.com/en-us/library/windows/desktop/ms633534%28v=vs.85%29...

> Then, it's clearer for both of compilers and developers.

width and height are clearer to me in the image encoders, as I mentioned. 
ImageDataBuffer is not an ideal class, I think we agree on that point, and I
didn't invent it :)  It maybe needs moving / refactoring, but given unknown
future requirements, that would wold take longer. and should have a separate
bug.   I'm also curious as to why the discussion about it is needed.  We are
investigating a security bug, not redesigning the code.  I'd prefer to focus my
time on the security bug.  Refactoring can be done elsewhere.

[Yuki, 2015-01-08 04:32:39]

https://codereview.chromium.org/837643002/diff/20001/Source/platform/graphics...
File Source/platform/graphics/ImageBuffer.h (right):

https://codereview.chromium.org/837643002/diff/20001/Source/platform/graphics...
Source/platform/graphics/ImageBuffer.h:163: struct ImageDataBuffer : public
IntSize {
If ImageDataBuffer inherits from IntSize, ImageDataBufer can be implicitly
converted to IntSize, and it would cause bugs which are hard to realize.

This is as if we had
    class IntSize {
        IntSize(ImageDataBuffer);  // no "explicit" keyword
    }

This is dangerous, and it's possible to introduce severe bugs.  What I'm afraid
most is that this sort of bugs are hard to find out once it's introduced.

I don't know what consistency you're caring (SkBitmap does not inherit from
SkSize), but I think it's less important.  I don't have no choice on this point.
 This is a famous problem in OOD.

I agree with almost all of your points except for this point.  I'll send a
l-g-t-m once this gets fixed.

[Noel Gordon, 2015-01-08 05:29:37]

On 2015/01/08 04:32:39, Yuki wrote:
>
https://codereview.chromium.org/837643002/diff/20001/Source/platform/graphics...
> File Source/platform/graphics/ImageBuffer.h (right):
> 
>
https://codereview.chromium.org/837643002/diff/20001/Source/platform/graphics...
> Source/platform/graphics/ImageBuffer.h:163: struct ImageDataBuffer : public
> IntSize {
> If ImageDataBuffer inherits from IntSize, ImageDataBufer can be implicitly

ImageDataBuffer should be replaced with a SkBItmap, on a separate bug.

> converted to IntSize, and it would cause bugs which are hard to realize.
> This is as if we had
>     class IntSize {
>         IntSize(ImageDataBuffer);  // no "explicit" keyword
>     }
> 
> This is dangerous, and it's possible to introduce severe bugs.

Maybe, but we don't have this.  You are inventing scenarios that don't exist.

> What I'm afraid most is that this sort of bugs are hard to find out once it's
introduced.

and appear to be worried by most everything, re-reading this review. Worrying
never changes anything.

> I don't know what consistency you're caring (SkBitmap does not inherit from
> SkSize), but I think it's less important.

See above.

> I don't have no choice on this point. This is a famous problem in OOD.
> I agree with almost all of your points except for this point.  I'll send a
> l-g-t-m once this gets fixed.

You added a famous bug in our canvas implementation, we think.  Let's try fix
that first.

May I have a lgtm so we can get on with it.

[Yuki, 2015-01-08 05:44:44]

On 2015/01/08 05:29:37, noel gordon wrote:
> On 2015/01/08 04:32:39, Yuki wrote:
> >
>
https://codereview.chromium.org/837643002/diff/20001/Source/platform/graphics...
> > File Source/platform/graphics/ImageBuffer.h (right):
> > 
> >
>
https://codereview.chromium.org/837643002/diff/20001/Source/platform/graphics...
> > Source/platform/graphics/ImageBuffer.h:163: struct ImageDataBuffer : public
> > IntSize {
> > If ImageDataBuffer inherits from IntSize, ImageDataBufer can be implicitly
> 
> ImageDataBuffer should be replaced with a SkBItmap, on a separate bug.
> 
> > converted to IntSize, and it would cause bugs which are hard to realize.
> > This is as if we had
> >     class IntSize {
> >         IntSize(ImageDataBuffer);  // no "explicit" keyword
> >     }
> > 
> > This is dangerous, and it's possible to introduce severe bugs.
> 
> Maybe, but we don't have this.  You are inventing scenarios that don't exist.
> 
> > What I'm afraid most is that this sort of bugs are hard to find out once
it's
> introduced.
> 
> and appear to be worried by most everything, re-reading this review. Worrying
> never changes anything.
> 
> > I don't know what consistency you're caring (SkBitmap does not inherit from
> > SkSize), but I think it's less important.
> 
> See above.
> 
> > I don't have no choice on this point. This is a famous problem in OOD.
> > I agree with almost all of your points except for this point.  I'll send a
> > l-g-t-m once this gets fixed.
> 
> You added a famous bug in our canvas implementation, we think.  Let's try fix
> that first.
> 
> May I have a lgtm so we can get on with it.

In general, implicit type conversions are dangerous.  Why do you want implicit
type conversion here?  I'm not asking to change a thing.  The original code had
the size as a member.

[Noel Gordon, 2015-01-08 06:25:07]

I think I explained that already.

[Yuki, 2015-01-08 06:28:48]

On 2015/01/08 06:25:07, noel gordon wrote:
> I think I explained that already.

Did you mean that you want to write:
    return encodePixels(IntSize(imageData.width(), imageData.height()), imageDat
     a.pixels(), false, quality, output);
in platform/image-encoders/skia/XXXImageEncoder.cpp?

If that's the reason, we can add member functions width() and height() without
public inheritance.

[Noel Gordon, 2015-01-08 07:14:43]

On 2015/01/08 06:28:48, Yuki wrote:
> On 2015/01/08 06:25:07, noel gordon wrote:
> > I think I explained that already.
> 
> Did you mean that you want to write:
>     return encodePixels(IntSize(imageData.width(), imageData.height()),
imageDat
>      a.pixels(), false, quality, output);
> in platform/image-encoders/skia/XXXImageEncoder.cpp?

yes.
 
> If that's the reason, we can add member functions width() and height() without
> public inheritance.

yes, on another bug perhaps?  (got to head out for now, talk latter).

[Yuki, 2015-01-08 07:22:03]

On 2015/01/08 07:14:43, noel gordon wrote:
> On 2015/01/08 06:28:48, Yuki wrote:
> > On 2015/01/08 06:25:07, noel gordon wrote:
> > > I think I explained that already.
> > 
> > Did you mean that you want to write:
> >     return encodePixels(IntSize(imageData.width(), imageData.height()),
> imageDat
> >      a.pixels(), false, quality, output);
> > in platform/image-encoders/skia/XXXImageEncoder.cpp?
> 
> yes.
>  
> > If that's the reason, we can add member functions width() and height()
without
> > public inheritance.
> 
> yes, on another bug perhaps?  (got to head out for now, talk latter).

I'm really afraid of implicit conversions.  It shouldn't be a big deal.  Could
you kindly stop introducing public inheritance in this CL?  I'd appreciate your
cooperations and patience.

[Noel Gordon, 2015-01-09 15:39:31]

On 2015/01/08 07:22:03, Yuki wrote:
> I'm really afraid of implicit conversions.  It shouldn't be a big deal. 

As discussed off-line, couldn't submit here because you needed to revert another
patch, so we held off here.

> If that's the reason, we can add member functions width() and height() without
> public inheritance.

width() and height it is.  Also moved the toDataURL service into the class,
hopefully we can maybe get rid of the class eventually.

[Noel Gordon, 2015-01-09 15:45:00]

New patch uploaded.

[Noel Gordon, 2015-01-12 03:38:21]

All review comments addressed.  toDataURL security issues 445107 and 445743
resolved.

[Noel Gordon, 2015-01-12 03:39:05]

The CQ bit was checked by noel@chromium.org

[commit-bot: I haz the power, 2015-01-12 03:40:00]

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

[commit-bot: I haz the power, 2015-01-12 03:43:16]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=188199