Bug 71846 – [chromium] Separate image encoding from dataURL construction

• Component: Canvas
• Status: RESOLVED FIXED

[noel gordon]

Remove the implicit assumption that a dataURL is the only desired output format of the image encoding phase.

[noel gordon]

Created attachment 114135 [details]
Patch

[Adam Barth]

Comment on attachment 114135 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=114135&action=review

> Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:366
> +    Vector<unsigned char>* encodedImage = static_cast<Vector<unsigned char>*>(output);

Crazy!  We can't be consistent about the type?

> Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:-390
> -    base64Encode(*reinterpret_cast<Vector<char>*>(&encodedImage), base64Data);

I see.  Frowns.

> Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:401
> +    return "data:" + mimeType + ";base64," + base64Data;

Should we CRASH or ASSERT or something that mimeType doesn't have a ; or a , character?

> Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:408
> +    Vector<char> encodedImage, base64Data;

WebKit doesn't usually use compound declarations.

[WebKit Review Bot]

Comment on attachment 114135 [details]
Patch

Attachment 114135 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10376193

[noel gordon]

Created attachment 114181 [details]
Patch: linux EWS build fix.

[noel gordon]

(In reply to comment #2)
> (From update of attachment 114135 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=114135&action=review
> 
> > Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:366
> > +    Vector<unsigned char>* encodedImage = static_cast<Vector<unsigned char>*>(output);
> 
> Crazy!  We can't be consistent about the type?
> 
> > Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:-390
> > -    base64Encode(*reinterpret_cast<Vector<char>*>(&encodedImage), base64Data);
> 
> I see.  Frowns.

The image encoders want unsigned char, the base64 encoder wants char, and we need to marry
the two.  Those reinterpret_cast gymnastics made me frown too.  Hoped that the static_cast
would be a little more sane, but the Linux EWS demands a reinterpret_cast *sigh*

[noel gordon]

(In reply to comment #2)
> > Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:401
> > +    return "data:" + mimeType + ";base64," + base64Data;
> 
> Should we CRASH or ASSERT or something that mimeType doesn't have a ; or a , character?

Good question, but no.  The ASSERT guards at the entry to these routines state:

   ASSERT(MIMETypeRegistry::isSupportedImageMIMETypeForEncoding(mimeType));

the expectation being that the mimeType is already canonical.  Caller must do the work to
make it so.

Only current caller is <canvas>.toDataURL(), it converts the mimeType to canonical & maps
bogus user input to "image/png" as required by the standard.

[noel gordon]

> > Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:408
> > +    Vector<char> encodedImage, base64Data;
> 
> WebKit doesn't usually use compound declarations.

Not something I'd usually do either, maybe ok here since there's no pointer confusion but
what I'd really like to write is

    return "data:" + mimeType + ";base64," + base64Encode(encodedImage);

and ditch the temporary variable.  Might be a reasonable subject for another patch.

[noel gordon]

> Only current caller is <canvas>.toDataURL(), it converts the mimeType to canonical & maps
> bogus user input to "image/png" as required by the standard.

I used "bogus" here in a technical sense

http://trac.webkit.org/browser/trunk/LayoutTests/canvas/philip/tests/toDataURL.bogustype.html

[WebKit Review Bot]

Comment on attachment 114181 [details]
Patch: linux EWS build fix.

Clearing flags on attachment: 114181

Committed r99763: <http://trac.webkit.org/changeset/99763>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.