Add Base64 codec to dart:convert.

Add Base64 codec to dart:convert.

This is a simple converter that only accepts and produces plain base-64 strings
with the default alphabet and no whitespace.

R=floitsch@google.com, sgjesse@google.com

Committed: https://github.com/dart-lang/sdk/commit/bd4c7a8a07f5fb92bc6370e815adf2b9cbce2ab4

[Lasse Reichstein Nielsen, 2015-09-28 12:54:30]

lrn@google.com changed reviewers:
+ floitsch@google.com, sgjesse@google.com

[kevmoo, 2015-09-28 15:14:46]

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

[kevmoo, 2015-09-28 15:14:46]

Why would we not put this in pkg/convert?

Is there a strong need to keep putting more in this core SDK lib?

[floitsch, 2015-09-28 16:08:28]

LGTM.

https://codereview.chromium.org/1370073002/diff/1/sdk/lib/convert/base64.dart
File sdk/lib/convert/base64.dart (right):

https://codereview.chromium.org/1370073002/diff/1/sdk/lib/convert/base64.dart...
sdk/lib/convert/base64.dart:58: var encoder = new _Base64Encoder();
Up to you if you want to add a type here.

https://codereview.chromium.org/1370073002/diff/1/sdk/lib/convert/base64.dart...
sdk/lib/convert/base64.dart:75: * Helper class for encoding bytes to BASE-64.
usage is not consistent. Sometimes BASE64. Here BASE-64.
I think I would prefer "Base64".

https://codereview.chromium.org/1370073002/diff/1/sdk/lib/convert/base64.dart...
sdk/lib/convert/base64.dart:77: class _Base64Encoder {
I'm not a fan of reusing a class as a mixin.

I would prefer having a _Base64Mixin and a separate _Base64Encoder.
In that case I would also make the _getBuffer abstract in the Mixin.

https://codereview.chromium.org/1370073002/diff/1/sdk/lib/convert/base64.dart...
sdk/lib/convert/base64.dart:78: /** Intermediiate state shift of the bits stored
in the state. */
Intermediate

/// Shift-count to extract the value that is stored in the state.

https://codereview.chromium.org/1370073002/diff/1/sdk/lib/convert/base64.dart...
sdk/lib/convert/base64.dart:80: /** Intermediate state encoding of the number of
bytes stored in the state. */
/// Mask to extract the XYZ count from the state.

https://codereview.chromium.org/1370073002/diff/1/sdk/lib/convert/base64.dart...
sdk/lib/convert/base64.dart:83: int _state = 0;
Might be nice to just write getters and setters.

int get _bufferedBytesCount => _state & 3;
int get _bufferedBytes => _state >> 2;
void _buffer(int value, int byteCount) {
  assert(value < ...);
  assert(byteCount < ...);
  _state = ((value << byteCount) | byteCount) & ...;
}

https://codereview.chromium.org/1370073002/diff/1/sdk/lib/convert/base64.dart...
sdk/lib/convert/base64.dart:85: Uint8List _getBuffer(int bufferLength) => new
Uint8List(bufferLength);
_createBuffer

I do realize that "_buffer()" in my comment above is too similar to this
"buffer". I don't have a better suggestion yet, though.

https://codereview.chromium.org/1370073002/diff/1/sdk/lib/convert/base64.dart...
sdk/lib/convert/base64.dart:101: var output = _getBuffer(bufferLength);
type if you want to.

https://codereview.chromium.org/1370073002/diff/1/sdk/lib/convert/base64.dart...
sdk/lib/convert/base64.dart:104: // If the input plus the data in _state is
still less than three bytes,
-still-

https://codereview.chromium.org/1370073002/diff/1/sdk/lib/convert/base64.dart...
sdk/lib/convert/base64.dart:105: // there may not be any output.
s/may not be any/is no

https://codereview.chromium.org/1370073002/diff/1/sdk/lib/convert/base64.dart...
sdk/lib/convert/base64.dart:115: int byteOr = 0;
// The input must be a list of bytes.
// All input integers are ored into [byteOr] and a later check verifies that
this was indeed the case.

https://codereview.chromium.org/1370073002/diff/1/sdk/lib/convert/base64.dart...
sdk/lib/convert/base64.dart:120: count--;
I find "count" to be misleading (but it's not too bad).

https://codereview.chromium.org/1370073002/diff/1/sdk/lib/convert/base64.dart...
sdk/lib/convert/base64.dart:134: if (byteOr >= 0 && byteOr <= 255) {
I almost prefer to have the error-case guarded:

if (byteOr < 0 || byteOr > 255) {
  // There was an invalid ...
  ...
}

if (isLast && count < 3) {
...
}

https://codereview.chromium.org/1370073002/diff/1/sdk/lib/convert/base64.dart...
sdk/lib/convert/base64.dart:258: var decoder = new _Base64Decoder();
type if you want.

https://codereview.chromium.org/1370073002/diff/1/sdk/lib/convert/base64.dart...
sdk/lib/convert/base64.dart:273: static const int _valueShift = 2;
Same as for the encoder.

https://codereview.chromium.org/1370073002/diff/1/sdk/lib/convert/base64.dart...
sdk/lib/convert/base64.dart:312: * contains six bits per seend character.
seen

https://codereview.chromium.org/1370073002/diff/1/sdk/lib/convert/base64.dart...
sdk/lib/convert/base64.dart:318: * means that a four-character block has not
been completed.
mean

https://codereview.chromium.org/1370073002/diff/1/sdk/lib/convert/base64.dart...
sdk/lib/convert/base64.dart:369: int charOr = 0;
Add comment, what the charOr is for.

https://codereview.chromium.org/1370073002/diff/1/sdk/lib/convert/base64.dart...
sdk/lib/convert/base64.dart:371: var char = input.codeUnitAt(i);
type if you want.

https://codereview.chromium.org/1370073002/diff/1/sdk/lib/convert/base64.dart...
sdk/lib/convert/base64.dart:375: bits = ((bits << 6) | code);
magic "6".

https://codereview.chromium.org/1370073002/diff/1/sdk/lib/convert/base64.dart...
sdk/lib/convert/base64.dart:375: bits = ((bits << 6) | code);
Add & to make it easier for the VM to optimize.

https://codereview.chromium.org/1370073002/diff/1/sdk/lib/convert/base64.dart...
sdk/lib/convert/base64.dart:406: if (charOr >= 0 && charOr <= asciiMax) {
Same as for the encoder.

[Søren Gjesse, 2015-09-28 17:09:29]

On 2015/09/28 15:14:46, kevmoo wrote:
> Why would we not put this in pkg/convert?
> 
> Is there a strong need to keep putting more in this core SDK lib?

To be able to handle Base64 encoded data: URIs a Base64 decoder is required in
the core libraries.

Lasse and I have discussed this back and forth, and the implementation in
dart:convert will be as simple as possible with very few options, and then
package:convert can extend with all kinds of bells and whistles.

[kevmoo, 2015-09-28 17:16:43]

On 2015/09/28 17:09:29, Søren Gjesse wrote:
> On 2015/09/28 15:14:46, kevmoo wrote:
> > Why would we not put this in pkg/convert?
> > 
> > Is there a strong need to keep putting more in this core SDK lib?
> 
> To be able to handle Base64 encoded data: URIs a Base64 decoder is required in
> the core libraries.
> 
> Lasse and I have discussed this back and forth, and the implementation in
> dart:convert will be as simple as possible with very few options, and then
> package:convert can extend with all kinds of bells and whistles.

Awesome – thanks for the context.

We also have base64 code in convert. Would be great to align these so – DRY
(don't repeat yourself)

[kevmoo, 2015-09-28 17:16:54]

On 2015/09/28 17:16:43, kevmoo wrote:
> On 2015/09/28 17:09:29, Søren Gjesse wrote:
> > On 2015/09/28 15:14:46, kevmoo wrote:
> > > Why would we not put this in pkg/convert?
> > > 
> > > Is there a strong need to keep putting more in this core SDK lib?
> > 
> > To be able to handle Base64 encoded data: URIs a Base64 decoder is required
in
> > the core libraries.
> > 
> > Lasse and I have discussed this back and forth, and the implementation in
> > dart:convert will be as simple as possible with very few options, and then
> > package:convert can extend with all kinds of bells and whistles.
> 
> Awesome – thanks for the context.
> 
> We also have base64 code in convert. Would be great to align these so – DRY
> (don't repeat yourself)

er, not convert – crypto

[Søren Gjesse, 2015-09-28 17:18:52]

lgtm

https://codereview.chromium.org/1370073002/diff/1/sdk/lib/convert/base64.dart
File sdk/lib/convert/base64.dart (right):

https://codereview.chromium.org/1370073002/diff/1/sdk/lib/convert/base64.dart...
sdk/lib/convert/base64.dart:25: * The default encoding alphabet.
This is not just "the default encoding alphabet" it is "the (only) encoding
alpahbet".

https://codereview.chromium.org/1370073002/diff/1/sdk/lib/convert/base64.dart...
sdk/lib/convert/base64.dart:42: String get name => "base64";
Uppercase B?

https://codereview.chromium.org/1370073002/diff/1/sdk/lib/convert/base64.dart...
sdk/lib/convert/base64.dart:162: if (count == 1) {
Maybe this could be

  if (count == 2) {
    output[outputIndex++] =
       _base64Alphabet.codeUnitAt((bits >> 10) & _sixBitMask);
  }
  // the >> 4
  // the << 2
  // one padding
  if (count == 1) {
    // an additional padding
  }

https://codereview.chromium.org/1370073002/diff/1/sdk/lib/convert/base64.dart...
sdk/lib/convert/base64.dart:203: /**
Shouldn't be dartdoc.

[sra1, 2015-09-28 17:29:41]

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

[sra1, 2015-09-28 17:29:42]

DBC

https://codereview.chromium.org/1370073002/diff/1/sdk/lib/convert/base64.dart
File sdk/lib/convert/base64.dart (right):

https://codereview.chromium.org/1370073002/diff/1/sdk/lib/convert/base64.dart...
sdk/lib/convert/base64.dart:14: * any invalid characters in the input to
decoding.
Is this encoding/decoding compatible with btoa()/atob() provided by the browser?
Is it possible to use the browser to improve performance or reduce the size of
the compiled conversion?

https://codereview.chromium.org/1370073002/diff/1/sdk/lib/convert/base64.dart...
sdk/lib/convert/base64.dart:261: return buffer;
If you are OK with an unmodifiable result, the a browser implementation is
essentially


    List<int> convert(String input) =>
      JS('String', 'atob(#)', input).codeUnits;

[Lasse Reichstein Nielsen, 2015-09-29 10:31:05]

https://codereview.chromium.org/1370073002/diff/1/sdk/lib/convert/base64.dart
File sdk/lib/convert/base64.dart (right):

https://codereview.chromium.org/1370073002/diff/1/sdk/lib/convert/base64.dart...
sdk/lib/convert/base64.dart:14: * any invalid characters in the input to
decoding.
Compatible, but not identical. 
The atob conversion is more lenient (allows invalid characters and missing
padding) which I deliberately omitted from this version of the decoder to keep
it simple.

https://codereview.chromium.org/1370073002/diff/1/sdk/lib/convert/base64.dart...
sdk/lib/convert/base64.dart:22: const Base64Codec BASE64 = const Base64Codec();
This naming is *only* for consistency. I'd prefer to call it base64 if possible.

https://codereview.chromium.org/1370073002/diff/1/sdk/lib/convert/base64.dart...
sdk/lib/convert/base64.dart:25: * The default encoding alphabet.
Rewording.
It is the default alphabet for base64 encoding (in general), and the only
alphabet supported by this library.
Well, if you follow RFC 4648, you shouldn't call using the other alphabets
base64 encoding.

https://codereview.chromium.org/1370073002/diff/1/sdk/lib/convert/base64.dart...
sdk/lib/convert/base64.dart:42: String get name => "base64";
I should remove it. This is not an "Encoding", just a Codec, so there is no name
getter.
Encodings are generally named with lower-case letters, e.g., "utf-8" and
"us-ascii" (even if it is horrible to behold).

https://codereview.chromium.org/1370073002/diff/1/sdk/lib/convert/base64.dart...
sdk/lib/convert/base64.dart:58: var encoder = new _Base64Encoder();
Not really worth it.

https://codereview.chromium.org/1370073002/diff/1/sdk/lib/convert/base64.dart...
sdk/lib/convert/base64.dart:75: * Helper class for encoding bytes to BASE-64.
I think the name should be "base64" or "base 64 encoding".
This is what is used by both RFC 2045 and RFC 4648 as the name of the encoding
(with Base64 used only if it appears in Title Case).

It is actually being used incorrectly here as a name for the output of base64
conversion, not the conversion itself.

https://codereview.chromium.org/1370073002/diff/1/sdk/lib/convert/base64.dart...
sdk/lib/convert/base64.dart:77: class _Base64Encoder {
That would be pretty redundant for a private helper class.
If it helps, you can think of it as using the mixin as a class, not the class as
a mixin.

Another option is to not mix it into the chunked conversion sink, but instead
have it as a member. Mixin it in just avoids one redirection per call and saves
a little memory (maybe -  I don't know if the mixin application itself uses a
significant amount of memory due to duplication, in which case the extra word
per instance is probably not going to make up for it).

That will still need two classes instead of one since I can't overload
_getBuffer in the class itself.

https://codereview.chromium.org/1370073002/diff/1/sdk/lib/convert/base64.dart...
sdk/lib/convert/base64.dart:78: /** Intermediiate state shift of the bits stored
in the state. */
On 2015/09/28 16:08:28, floitsch wrote:
> Intermediate
> 
> /// Shift-count to extract the value that is stored in the state.

Done.

https://codereview.chromium.org/1370073002/diff/1/sdk/lib/convert/base64.dart...
sdk/lib/convert/base64.dart:80: /** Intermediate state encoding of the number of
bytes stored in the state. */
On 2015/09/28 16:08:28, floitsch wrote:
> /// Mask to extract the XYZ count from the state.

Done.

https://codereview.chromium.org/1370073002/diff/1/sdk/lib/convert/base64.dart...
sdk/lib/convert/base64.dart:83: int _state = 0;
I use the extraction in static methods, so I added extract function instead:
stateBits(int state) => state >> _valueShift;

https://codereview.chromium.org/1370073002/diff/1/sdk/lib/convert/base64.dart...
sdk/lib/convert/base64.dart:85: Uint8List _getBuffer(int bufferLength) => new
Uint8List(bufferLength);
Yes, "buffer" as a verb is not going to work, it's too traditionally a noun.
Again I use it in a static setting, so I went with "encodeState(int count, int
bits) => ..."

https://codereview.chromium.org/1370073002/diff/1/sdk/lib/convert/base64.dart...
sdk/lib/convert/base64.dart:104: // If the input plus the data in _state is
still less than three bytes,
On 2015/09/28 16:08:28, floitsch wrote:
> -still-

Done.

https://codereview.chromium.org/1370073002/diff/1/sdk/lib/convert/base64.dart...
sdk/lib/convert/base64.dart:105: // there may not be any output.
Unless isLast is true, in which case there is output (and the buffer size has
been increased accordingly above so we don't get here).

But reworded anyway - if we get here, that is the case.

https://codereview.chromium.org/1370073002/diff/1/sdk/lib/convert/base64.dart...
sdk/lib/convert/base64.dart:115: int byteOr = 0;
On 2015/09/28 16:08:28, floitsch wrote:
> // The input must be a list of bytes.
> // All input integers are ored into [byteOr] and a later check verifies that
> this was indeed the case.

Done.

https://codereview.chromium.org/1370073002/diff/1/sdk/lib/convert/base64.dart...
sdk/lib/convert/base64.dart:120: count--;
Renamed to "expectedChars".

https://codereview.chromium.org/1370073002/diff/1/sdk/lib/convert/base64.dart...
sdk/lib/convert/base64.dart:134: if (byteOr >= 0 && byteOr <= 255) {
I like to keep the non-error flow connected if possible.

(It's also a micro-optimization. CPUs will predict a forward branch as
not-taken, so the non-error branch should come first - I don't expect the
compiler to reorder the cases).

https://codereview.chromium.org/1370073002/diff/1/sdk/lib/convert/base64.dart...
sdk/lib/convert/base64.dart:162: if (count == 1) {
On 2015/09/28 17:18:52, Søren Gjesse wrote:
> Maybe this could be
> 
>   if (count == 2) {
>     output[outputIndex++] =
>        _base64Alphabet.codeUnitAt((bits >> 10) & _sixBitMask);
>   }
>   // the >> 4
>   // the << 2

It's >> 2, << 4 for one of the branches, so that doesn't generalize.

>   // one padding
>   if (count == 1) {
>     // an additional padding
>   }

I could move the last padding store out of the branch, but I prefer to not split
of the blocks.

https://codereview.chromium.org/1370073002/diff/1/sdk/lib/convert/base64.dart...
sdk/lib/convert/base64.dart:203: /**
Why not?
An editor should still show it to you, even if it's not in the public API.

https://codereview.chromium.org/1370073002/diff/1/sdk/lib/convert/base64.dart...
sdk/lib/convert/base64.dart:261: return buffer;
I probably want it to be a Uint8List.
Still, it should be possible to do
  new Uint8List.fromList(....codeUnits)
It's even optimized in the VM (which won't help dart2js).

https://codereview.chromium.org/1370073002/diff/1/sdk/lib/convert/base64.dart...
sdk/lib/convert/base64.dart:273: static const int _valueShift = 2;
On 2015/09/28 16:08:28, floitsch wrote:
> Same as for the encoder.

Done.

https://codereview.chromium.org/1370073002/diff/1/sdk/lib/convert/base64.dart...
sdk/lib/convert/base64.dart:312: * contains six bits per seend character.
On 2015/09/28 16:08:28, floitsch wrote:
> seen

Done.

https://codereview.chromium.org/1370073002/diff/1/sdk/lib/convert/base64.dart...
sdk/lib/convert/base64.dart:318: * means that a four-character block has not
been completed.
On 2015/09/28 16:08:28, floitsch wrote:
> mean

Done.

https://codereview.chromium.org/1370073002/diff/1/sdk/lib/convert/base64.dart...
sdk/lib/convert/base64.dart:369: int charOr = 0;
On 2015/09/28 16:08:28, floitsch wrote:
> Add comment, what the charOr is for.

Done.

https://codereview.chromium.org/1370073002/diff/1/sdk/lib/convert/base64.dart...
sdk/lib/convert/base64.dart:375: bits = ((bits << 6) | code);
named it "bitsPerCharacter".

https://codereview.chromium.org/1370073002/diff/1/sdk/lib/convert/base64.dart...
sdk/lib/convert/base64.dart:375: bits = ((bits << 6) | code);
On 2015/09/28 16:08:28, floitsch wrote:
> Add & to make it easier for the VM to optimize.

Done.

https://codereview.chromium.org/1370073002/diff/1/sdk/lib/convert/base64.dart...
sdk/lib/convert/base64.dart:406: if (charOr >= 0 && charOr <= asciiMax) {
Same answer.

[Lasse Reichstein Nielsen, 2015-09-29 11:50:30]

Committed patchset #3 (id:40001) manually as
bd4c7a8a07f5fb92bc6370e815adf2b9cbce2ab4 (presubmit successful).