Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1159093002:
Implement a Base64 codec (Closed)
|
Created:
5 years, 6 months ago by Alexander Ivanov Modified:
5 years, 6 months ago CC:
reviews_dartlang.org Base URL:
https://github.com/dart-lang/crypto.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionImplement a Base64 codec
Implement a Base64 codec similar to codecs in 'dart:convert'.
`CryptoUtils.bytesToBase64` and `CryptoUtils.base64StringToBytes`
remain for backward compatibility, but they internally invoke
`Base64.encode` and `Base64.decode` respectively
BUG=
R=lrn@google.com, sgjesse@google.com
Committed: https://github.com/dart-lang/crypto/commit/3a2b31228cfe03880af829798ffe16d05f37f6e3
Patch Set 1 #
Total comments: 59
Patch Set 2 : Address review comments #
Total comments: 14
Patch Set 3 : Address some more stylistic/documentation comments from the codereview. #
Total comments: 22
Patch Set 4 : Check safe/unsafe characters in decoder for consistency #
Total comments: 6
Patch Set 5 : Remove newlines after dart-docs #
Messages
Total messages: 14 (1 generated)
alivanov@google.com changed reviewers: + lrn@google.com, sgjesse@google.com
First comments, purely cosmetic. https://codereview.chromium.org/1159093002/diff/1/lib/crypto.dart File lib/crypto.dart (right): https://codereview.chromium.org/1159093002/diff/1/lib/crypto.dart#newcode22 lib/crypto.dart:22: part 'src/base64.dart'; We may want to move base64 to dart:convert at some point, so this is great! :) https://codereview.chromium.org/1159093002/diff/1/lib/src/base64.dart File lib/src/base64.dart (right): https://codereview.chromium.org/1159093002/diff/1/lib/src/base64.dart#newcode3 lib/src/base64.dart:3: const Base64Codec Base64 = const Base64Codec(); Name it either "BASE64" or "base64" (style guide for constants give the two options). The "new" style is to use lowerCamelCase for constants, the old style was UPPER_UNDERLINE style. Most current codecs use old style. https://codereview.chromium.org/1159093002/diff/1/lib/src/base64.dart#newcode29 lib/src/base64.dart:29: const int LINE_LENGTH = 76; These should probably be private. https://codereview.chromium.org/1159093002/diff/1/lib/src/base64.dart#newcode40 lib/src/base64.dart:40: * Instantiates a new [Base64Codec]. Document the parameters, say what they do and what the default is if they are omitted. https://codereview.chromium.org/1159093002/diff/1/lib/src/base64.dart#newcode43 lib/src/base64.dart:43: No empty line here. https://codereview.chromium.org/1159093002/diff/1/lib/src/base64.dart#newcode44 lib/src/base64.dart:44: const Base64Codec({ bool urlSafe: false, bool addLineSeparator: false }) No space between "{" and "bool" (or before "}"). https://codereview.chromium.org/1159093002/diff/1/lib/src/base64.dart#newcode45 lib/src/base64.dart:45: : _urlSafe = urlSafe, indent four spaces before :, then one space after. https://codereview.chromium.org/1159093002/diff/1/lib/src/base64.dart#newcode51 lib/src/base64.dart:51: { bool urlSafe, Again no space after '{' or before '}' (becaue style guide says so). https://codereview.chromium.org/1159093002/diff/1/lib/src/base64.dart#newcode55 lib/src/base64.dart:55: return new Base64Encoder(urlSafe: urlSafe, addLineSeparator: addLineSeparator) Long line, break it somewhere. https://codereview.chromium.org/1159093002/diff/1/lib/src/base64.dart#newcode59 lib/src/base64.dart:59: Base64Encoder get encoder => new Base64Encoder(urlSafe: _urlSafe, addLineSeparator: _addLineSeparator); Long line. https://codereview.chromium.org/1159093002/diff/1/lib/src/base64.dart#newcode83 lib/src/base64.dart:83: int outputLen = ((len ~/ 3) * 4) + ((remainderLength > 0) ? 4 : 0); This seems too complex. Let's look at that later :) https://codereview.chromium.org/1159093002/diff/1/lib/src/base64.dart#newcode147 lib/src/base64.dart:147: : _encoder = new Base64Encoder(urlSafe: urlSafe, addLineSeparator: addLineSeparator); long line, indent : by four. https://codereview.chromium.org/1159093002/diff/1/lib/src/base64.dart#newcode150 lib/src/base64.dart:150: We usually don't have an empty line at the beginning of functions. https://codereview.chromium.org/1159093002/diff/1/lib/src/base64.dart#newcode153 lib/src/base64.dart:153: if(chunk.length + _buffer_count >= 3) { space between "if" and "(". More cases below. https://codereview.chromium.org/1159093002/diff/1/lib/src/base64.dart#newcode155 lib/src/base64.dart:155: var decodable = new List.from(_buffer.sublist(0, _buffer_count))..addAll(chunk.sublist(0, remainder)); long line https://codereview.chromium.org/1159093002/diff/1/lib/src/base64.dart#newcode160 lib/src/base64.dart:160: else { Else goes on the same line as '}'. https://codereview.chromium.org/1159093002/diff/1/lib/src/base64.dart#newcode232 lib/src/base64.dart:232: if(sink is! ByteConversionSink) { Indent by 2 only. https://codereview.chromium.org/1159093002/diff/1/lib/src/base64.dart#newcode249 lib/src/base64.dart:249: int next_buffer_length = (chunk.length + _buffer.length) % 4; We use camelCase for variables, so "nextBufferLength". https://codereview.chromium.org/1159093002/diff/1/lib/src/base64.dart#newcode265 lib/src/base64.dart:265: throw new FormatException("Size of Base 64 input must be a multiple of 4 : ${_buffer.toString()}"); Use other arguments to FormatException (source and maybe position): throw new FormatException("Size of Base64 input must be multiplier of 4", _buffer, _buffer.length); https://codereview.chromium.org/1159093002/diff/1/lib/src/crypto_utils.dart File lib/src/crypto_utils.dart (right): https://codereview.chromium.org/1159093002/diff/1/lib/src/crypto_utils.dart#n... lib/src/crypto_utils.dart:19: return Base64.encode(bytes, urlSafe: urlSafe, addLineSeparator: addLineSeparator); long line. https://codereview.chromium.org/1159093002/diff/1/test/base64_test.dart File test/base64_test.dart (right): https://codereview.chromium.org/1159093002/diff/1/test/base64_test.dart#newco... test/base64_test.dart:34: [[102, 102], [111, 102], [111, 111, 102, 111, 111, 98, 102, 111, 111, 98, 97, 102, 111, 111, 98, 97, 114]]; long line. https://codereview.chromium.org/1159093002/diff/1/test/base64_test.dart#newco... test/base64_test.dart:38: const ['YmFz', 'ZTY', '0I', 'GRlY29kZXI=']; indent by four. https://codereview.chromium.org/1159093002/diff/1/test/base64_test.dart#newco... test/base64_test.dart:142: .transform(Base64.decoder).toList(); indent by four (or up to other "."). https://codereview.chromium.org/1159093002/diff/1/test/base64_test.dart#newco... test/base64_test.dart:212: // gst Bad edit?
As an update to Lasses comments I forgot to tell you about the Dart style guide, see https://www.dartlang.org/articles/style-guide/. It doesn't cover all the conventions we have, but most.
https://codereview.chromium.org/1159093002/diff/1/lib/src/base64.dart File lib/src/base64.dart (right): https://codereview.chromium.org/1159093002/diff/1/lib/src/base64.dart#newcode65 lib/src/base64.dart:65: class Base64Encoder extends Converter<List<int>, String> { Could generally use more comments, but ok for now. https://codereview.chromium.org/1159093002/diff/1/lib/src/base64.dart#newcode144 lib/src/base64.dart:144: int _buffer_count = 0; _bufferCount. https://codereview.chromium.org/1159093002/diff/1/lib/src/base64.dart#newcode156 lib/src/base64.dart:156: _buffer.setRange(0, next_buffer_length, chunk.sublist(remainder)); Use the fourth argument to setRange instead of creating a new list: _buffer.setRange(0, next_buffer_length, chunk, remainder); https://codereview.chromium.org/1159093002/diff/1/lib/src/base64.dart#newcode158 lib/src/base64.dart:158: _outSink.add(_encoder.convert(decodable)); The current thing works, which is great, so this is just an idea for a later optimization: Consider letting the encoder have a private (maybe static) method that does the conversion, where you can pass start and end indices and the bytes decoded so far. Then you don't need to create a new list for calling convert. https://codereview.chromium.org/1159093002/diff/1/lib/src/base64.dart#newcode161 lib/src/base64.dart:161: _buffer.setRange(_buffer_count, _buffer_count + chunk.length, chunk); _buffer_cunt + chunk.length => nextBufferLength https://codereview.chromium.org/1159093002/diff/1/test/base64_test.dart File test/base64_test.dart (right): https://codereview.chromium.org/1159093002/diff/1/test/base64_test.dart#newco... test/base64_test.dart:131: _STREAMING_DECODED_ZEROES); I would like a test of chunked encoding/decoding that has chunks of any size (0-3) following each other, including such nonsense as ["A", "" , "", "A", "="]. Basically, take a string like ["ABCD"] and split it into all possible decompositions (["ABC", "D"],["AB","CD"],["A","BCD"],["AB","C","D"],["A","BC","D"],["A","B","CD"],["A","B","C","D"]], and add 0-2 empty chunks between the other chunks, and see that they all give the right result when decoding. Do similar for encoding.
https://codereview.chromium.org/1159093002/diff/1/lib/src/base64.dart File lib/src/base64.dart (right): https://codereview.chromium.org/1159093002/diff/1/lib/src/base64.dart#newcode3 lib/src/base64.dart:3: const Base64Codec Base64 = const Base64Codec(); On 2015/05/29 06:48:41, Lasse Reichstein Nielsen wrote: > Name it either "BASE64" or "base64" (style guide for constants give the two > options). > The "new" style is to use lowerCamelCase for constants, the old style was > UPPER_UNDERLINE style. Most current codecs use old style. Done. https://codereview.chromium.org/1159093002/diff/1/lib/src/base64.dart#newcode29 lib/src/base64.dart:29: const int LINE_LENGTH = 76; On 2015/05/29 06:48:42, Lasse Reichstein Nielsen wrote: > These should probably be private. Done. https://codereview.chromium.org/1159093002/diff/1/lib/src/base64.dart#newcode40 lib/src/base64.dart:40: * Instantiates a new [Base64Codec]. On 2015/05/29 06:48:42, Lasse Reichstein Nielsen wrote: > Document the parameters, say what they do and what the default is if they are > omitted. Done. https://codereview.chromium.org/1159093002/diff/1/lib/src/base64.dart#newcode43 lib/src/base64.dart:43: On 2015/05/29 06:48:42, Lasse Reichstein Nielsen wrote: > No empty line here. Done. https://codereview.chromium.org/1159093002/diff/1/lib/src/base64.dart#newcode44 lib/src/base64.dart:44: const Base64Codec({ bool urlSafe: false, bool addLineSeparator: false }) On 2015/05/29 06:48:41, Lasse Reichstein Nielsen wrote: > No space between "{" and "bool" (or before "}"). Done. https://codereview.chromium.org/1159093002/diff/1/lib/src/base64.dart#newcode45 lib/src/base64.dart:45: : _urlSafe = urlSafe, On 2015/05/29 06:48:42, Lasse Reichstein Nielsen wrote: > indent four spaces before :, then one space after. Done. https://codereview.chromium.org/1159093002/diff/1/lib/src/base64.dart#newcode51 lib/src/base64.dart:51: { bool urlSafe, On 2015/05/29 06:48:42, Lasse Reichstein Nielsen wrote: > Again no space after '{' or before '}' (becaue style guide says so). Done. https://codereview.chromium.org/1159093002/diff/1/lib/src/base64.dart#newcode55 lib/src/base64.dart:55: return new Base64Encoder(urlSafe: urlSafe, addLineSeparator: addLineSeparator) On 2015/05/29 06:48:42, Lasse Reichstein Nielsen wrote: > Long line, break it somewhere. Done. https://codereview.chromium.org/1159093002/diff/1/lib/src/base64.dart#newcode59 lib/src/base64.dart:59: Base64Encoder get encoder => new Base64Encoder(urlSafe: _urlSafe, addLineSeparator: _addLineSeparator); On 2015/05/29 06:48:42, Lasse Reichstein Nielsen wrote: > Long line. Done. https://codereview.chromium.org/1159093002/diff/1/lib/src/base64.dart#newcode65 lib/src/base64.dart:65: class Base64Encoder extends Converter<List<int>, String> { On 2015/05/29 07:15:15, Lasse Reichstein Nielsen wrote: > Could generally use more comments, but ok for now. Done. https://codereview.chromium.org/1159093002/diff/1/lib/src/base64.dart#newcode83 lib/src/base64.dart:83: int outputLen = ((len ~/ 3) * 4) + ((remainderLength > 0) ? 4 : 0); On 2015/05/29 06:48:41, Lasse Reichstein Nielsen wrote: > This seems too complex. Let's look at that later :) Done. https://codereview.chromium.org/1159093002/diff/1/lib/src/base64.dart#newcode144 lib/src/base64.dart:144: int _buffer_count = 0; On 2015/05/29 07:15:16, Lasse Reichstein Nielsen wrote: > _bufferCount. Done. https://codereview.chromium.org/1159093002/diff/1/lib/src/base64.dart#newcode147 lib/src/base64.dart:147: : _encoder = new Base64Encoder(urlSafe: urlSafe, addLineSeparator: addLineSeparator); On 2015/05/29 06:48:42, Lasse Reichstein Nielsen wrote: > long line, indent : by four. Done. https://codereview.chromium.org/1159093002/diff/1/lib/src/base64.dart#newcode150 lib/src/base64.dart:150: On 2015/05/29 06:48:42, Lasse Reichstein Nielsen wrote: > We usually don't have an empty line at the beginning of functions. Done. https://codereview.chromium.org/1159093002/diff/1/lib/src/base64.dart#newcode153 lib/src/base64.dart:153: if(chunk.length + _buffer_count >= 3) { On 2015/05/29 06:48:42, Lasse Reichstein Nielsen wrote: > space between "if" and "(". More cases below. Done. https://codereview.chromium.org/1159093002/diff/1/lib/src/base64.dart#newcode155 lib/src/base64.dart:155: var decodable = new List.from(_buffer.sublist(0, _buffer_count))..addAll(chunk.sublist(0, remainder)); On 2015/05/29 06:48:42, Lasse Reichstein Nielsen wrote: > long line Done. https://codereview.chromium.org/1159093002/diff/1/lib/src/base64.dart#newcode156 lib/src/base64.dart:156: _buffer.setRange(0, next_buffer_length, chunk.sublist(remainder)); On 2015/05/29 07:15:16, Lasse Reichstein Nielsen wrote: > Use the fourth argument to setRange instead of creating a new list: > _buffer.setRange(0, next_buffer_length, chunk, remainder); Done. https://codereview.chromium.org/1159093002/diff/1/lib/src/base64.dart#newcode158 lib/src/base64.dart:158: _outSink.add(_encoder.convert(decodable)); On 2015/05/29 07:15:16, Lasse Reichstein Nielsen wrote: > The current thing works, which is great, so this is just an idea for a later > optimization: > Consider letting the encoder have a private (maybe static) method that does the > conversion, where you can pass start and end indices and the bytes decoded so > far. Then you don't need to create a new list for calling convert. Done. https://codereview.chromium.org/1159093002/diff/1/lib/src/base64.dart#newcode160 lib/src/base64.dart:160: else { On 2015/05/29 06:48:42, Lasse Reichstein Nielsen wrote: > Else goes on the same line as '}'. Done. https://codereview.chromium.org/1159093002/diff/1/lib/src/base64.dart#newcode161 lib/src/base64.dart:161: _buffer.setRange(_buffer_count, _buffer_count + chunk.length, chunk); On 2015/05/29 07:15:16, Lasse Reichstein Nielsen wrote: > _buffer_cunt + chunk.length => nextBufferLength Done. https://codereview.chromium.org/1159093002/diff/1/lib/src/base64.dart#newcode232 lib/src/base64.dart:232: if(sink is! ByteConversionSink) { On 2015/05/29 06:48:41, Lasse Reichstein Nielsen wrote: > Indent by 2 only. Done. https://codereview.chromium.org/1159093002/diff/1/lib/src/base64.dart#newcode249 lib/src/base64.dart:249: int next_buffer_length = (chunk.length + _buffer.length) % 4; On 2015/05/29 06:48:42, Lasse Reichstein Nielsen wrote: > We use camelCase for variables, so "nextBufferLength". Done. https://codereview.chromium.org/1159093002/diff/1/lib/src/base64.dart#newcode265 lib/src/base64.dart:265: throw new FormatException("Size of Base 64 input must be a multiple of 4 : ${_buffer.toString()}"); On 2015/05/29 06:48:41, Lasse Reichstein Nielsen wrote: > Use other arguments to FormatException (source and maybe position): > throw new FormatException("Size of Base64 input must be multiplier of 4", > _buffer, _buffer.length); Done. https://codereview.chromium.org/1159093002/diff/1/lib/src/crypto_utils.dart File lib/src/crypto_utils.dart (right): https://codereview.chromium.org/1159093002/diff/1/lib/src/crypto_utils.dart#n... lib/src/crypto_utils.dart:19: return Base64.encode(bytes, urlSafe: urlSafe, addLineSeparator: addLineSeparator); On 2015/05/29 06:48:42, Lasse Reichstein Nielsen wrote: > long line. Done. https://codereview.chromium.org/1159093002/diff/1/test/base64_test.dart File test/base64_test.dart (right): https://codereview.chromium.org/1159093002/diff/1/test/base64_test.dart#newco... test/base64_test.dart:34: [[102, 102], [111, 102], [111, 111, 102, 111, 111, 98, 102, 111, 111, 98, 97, 102, 111, 111, 98, 97, 114]]; On 2015/05/29 06:48:43, Lasse Reichstein Nielsen wrote: > long line. Done. https://codereview.chromium.org/1159093002/diff/1/test/base64_test.dart#newco... test/base64_test.dart:38: const ['YmFz', 'ZTY', '0I', 'GRlY29kZXI=']; On 2015/05/29 06:48:43, Lasse Reichstein Nielsen wrote: > indent by four. Done. https://codereview.chromium.org/1159093002/diff/1/test/base64_test.dart#newco... test/base64_test.dart:131: _STREAMING_DECODED_ZEROES); On 2015/05/29 07:15:16, Lasse Reichstein Nielsen wrote: > I would like a test of chunked encoding/decoding that has chunks of any size > (0-3) following each other, including such nonsense as ["A", "" , "", "A", "="]. > Basically, take a string like ["ABCD"] and split it into all possible > decompositions (["ABC", > "D"],["AB","CD"],["A","BCD"],["AB","C","D"],["A","BC","D"],["A","B","CD"],["A","B","C","D"]], > and add 0-2 empty chunks between the other chunks, and see that they all give > the right result when decoding. > Do similar for encoding. > Done. https://codereview.chromium.org/1159093002/diff/1/test/base64_test.dart#newco... test/base64_test.dart:142: .transform(Base64.decoder).toList(); On 2015/05/29 06:48:43, Lasse Reichstein Nielsen wrote: > indent by four (or up to other "."). Done. https://codereview.chromium.org/1159093002/diff/1/test/base64_test.dart#newco... test/base64_test.dart:212: // gst On 2015/05/29 06:48:43, Lasse Reichstein Nielsen wrote: > Bad edit? yes
LGTM. Good stuff! https://codereview.chromium.org/1159093002/diff/20001/lib/crypto.dart File lib/crypto.dart (right): https://codereview.chromium.org/1159093002/diff/20001/lib/crypto.dart#newcode2 lib/crypto.dart:2: Extra line doesn't belong here. https://codereview.chromium.org/1159093002/diff/20001/lib/src/base64.dart File lib/src/base64.dart (right): https://codereview.chromium.org/1159093002/diff/20001/lib/src/base64.dart#new... lib/src/base64.dart:51: * If it is `true` `encode` adds an optional line separator(CR + LF) If `addLineSeparator` is true, `encode` adds ... speace between "separator" and "(". https://codereview.chromium.org/1159093002/diff/20001/test/base64_test.dart File test/base64_test.dart (right): https://codereview.chromium.org/1159093002/diff/20001/test/base64_test.dart#n... test/base64_test.dart:117: BASE64.encode(_LONG_LINE.codeUnits, addLineSeparator : true), This may now fit on the previous line. https://codereview.chromium.org/1159093002/diff/20001/test/base64_test.dart#n... test/base64_test.dart:151: await new Stream.fromIterable(_STREAMING_DECODER_INPUT) Indent "await" by four. https://codereview.chromium.org/1159093002/diff/20001/test/base64_test.dart#n... test/base64_test.dart:155: _STREAMING_DECODED); Indent this line to match the await. Ditto for expect below. https://codereview.chromium.org/1159093002/diff/20001/test/base64_test.dart#n... test/base64_test.dart:169: }(), throwsFormatException); I think the value for the expect can be just: new Stream.fromIterable(['ABz']).transform(BASE64.decoder).toList() That itself returns a future, so there is no need to wrap it in an async function that returns a future with the same error. https://codereview.chromium.org/1159093002/diff/20001/test/base64_test.dart#n... test/base64_test.dart:211: var streamed_result = await new Stream.fromIterable(dec) streamedResult :)
LGTM, with comments Nice work! https://codereview.chromium.org/1159093002/diff/40001/lib/src/base64.dart File lib/src/base64.dart (right): https://codereview.chromium.org/1159093002/diff/40001/lib/src/base64.dart#new... lib/src/base64.dart:46: * the [encoder] and [encode] use '-' instead of '+' and '_' instead of '/'. Please add a paragraph stating what the default value for urlSafe is. https://codereview.chromium.org/1159093002/diff/40001/lib/src/base64.dart#new... lib/src/base64.dart:52: * optional line separator (CR + LF) for each 76 char output. Please add a paragraph stating what the default value for addLineSeparator is. Should the default value be true? Or should the default value be the same as urlSafe, as it seems like the most common use is either 'non-URL with line separation (e.g. for reading/writing files) and URL without line separation (e.g. data: URL). However this might be more difficult to explain. https://codereview.chromium.org/1159093002/diff/40001/lib/src/base64.dart#new... lib/src/base64.dart:52: * optional line separator (CR + LF) for each 76 char output. Should the line length be configurable. According to http://en.wikipedia.org/wiki/Base64 PEM files have a max line length of 64 characters. https://codereview.chromium.org/1159093002/diff/40001/lib/src/base64.dart#new... lib/src/base64.dart:75: Base64Decoder get decoder => new Base64Decoder(); The decoder does not take _urlSafe. Should it and only accept either x and / or - and _? https://codereview.chromium.org/1159093002/diff/40001/lib/src/base64.dart#new... lib/src/base64.dart:87: const Base64Encoder({bool urlSafe: false, bool addLineSeparator: false}) As this is also public it should have the same information as the Base64Codec constructor. https://codereview.chromium.org/1159093002/diff/40001/lib/src/base64.dart#new... lib/src/base64.dart:111: int baseOutputLength = ((length ~/3) * 4); nit: Space before 3. https://codereview.chromium.org/1159093002/diff/40001/lib/src/base64.dart#new... lib/src/base64.dart:123: int x = ((bytes[i++] << 16) & 0xFFFFFF) | nit: Maybe write 0x00FFFFFF for "completeness". https://codereview.chromium.org/1159093002/diff/40001/lib/src/base64.dart#new... lib/src/base64.dart:129: out[j++] = lookup.codeUnitAt(x & 0x3f); nit: Please be consistent 0x3f -> 0x3F https://codereview.chromium.org/1159093002/diff/40001/test/base64_test.dart File test/base64_test.dart (right): https://codereview.chromium.org/1159093002/diff/40001/test/base64_test.dart#n... test/base64_test.dart:22: test('streaming decoder for malformed input', _testStreamingDecoderForMalformedInput); nit: Long line. https://codereview.chromium.org/1159093002/diff/40001/test/base64_test.dart#n... test/base64_test.dart:108: void _testEncoder() { Please keep the tests using CryptoUtils as long as we support it. That is have both this and an equivalent test that uses CryptoUtils. https://codereview.chromium.org/1159093002/diff/40001/test/base64_test.dart#n... test/base64_test.dart:221: expect(BASE64.encode(dec, urlSafe: true), '-_A='); I am not sure here. Related to the padding character http://en.wikipedia.org/wiki/Base64 says "must be percent-encoded in URL". Is that something we should do, or at least have an option for?
https://codereview.chromium.org/1159093002/diff/20001/lib/crypto.dart File lib/crypto.dart (right): https://codereview.chromium.org/1159093002/diff/20001/lib/crypto.dart#newcode2 lib/crypto.dart:2: On 2015/05/29 14:15:41, Lasse Reichstein Nielsen wrote: > Extra line doesn't belong here. Done. https://codereview.chromium.org/1159093002/diff/20001/lib/src/base64.dart File lib/src/base64.dart (right): https://codereview.chromium.org/1159093002/diff/20001/lib/src/base64.dart#new... lib/src/base64.dart:51: * If it is `true` `encode` adds an optional line separator(CR + LF) On 2015/05/29 14:15:42, Lasse Reichstein Nielsen wrote: > If `addLineSeparator` is true, `encode` adds ... > speace between "separator" and "(". Done. https://codereview.chromium.org/1159093002/diff/20001/test/base64_test.dart File test/base64_test.dart (right): https://codereview.chromium.org/1159093002/diff/20001/test/base64_test.dart#n... test/base64_test.dart:117: BASE64.encode(_LONG_LINE.codeUnits, addLineSeparator : true), On 2015/05/29 14:15:42, Lasse Reichstein Nielsen wrote: > This may now fit on the previous line. Done. https://codereview.chromium.org/1159093002/diff/20001/test/base64_test.dart#n... test/base64_test.dart:151: await new Stream.fromIterable(_STREAMING_DECODER_INPUT) On 2015/05/29 14:15:42, Lasse Reichstein Nielsen wrote: > Indent "await" by four. Done. https://codereview.chromium.org/1159093002/diff/20001/test/base64_test.dart#n... test/base64_test.dart:155: _STREAMING_DECODED); On 2015/05/29 14:15:42, Lasse Reichstein Nielsen wrote: > Indent this line to match the await. > Ditto for expect below. Done. https://codereview.chromium.org/1159093002/diff/20001/test/base64_test.dart#n... test/base64_test.dart:169: }(), throwsFormatException); On 2015/05/29 14:15:42, Lasse Reichstein Nielsen wrote: > I think the value for the expect can be just: > new Stream.fromIterable(['ABz']).transform(BASE64.decoder).toList() > That itself returns a future, so there is no need to wrap it in an async > function that returns a future with the same error. oh, absolutely https://codereview.chromium.org/1159093002/diff/20001/test/base64_test.dart#n... test/base64_test.dart:211: var streamed_result = await new Stream.fromIterable(dec) On 2015/05/29 14:15:42, Lasse Reichstein Nielsen wrote: > streamedResult :) Done. https://codereview.chromium.org/1159093002/diff/40001/lib/src/base64.dart File lib/src/base64.dart (right): https://codereview.chromium.org/1159093002/diff/40001/lib/src/base64.dart#new... lib/src/base64.dart:75: Base64Decoder get decoder => new Base64Decoder(); On 2015/06/01 12:20:59, Søren Gjesse wrote: > The decoder does not take _urlSafe. Should it and only accept either x and / or > - and _? Done.
https://codereview.chromium.org/1159093002/diff/40001/lib/src/base64.dart File lib/src/base64.dart (right): https://codereview.chromium.org/1159093002/diff/40001/lib/src/base64.dart#new... lib/src/base64.dart:46: * the [encoder] and [encode] use '-' instead of '+' and '_' instead of '/'. On 2015/06/01 12:20:58, Søren Gjesse wrote: > Please add a paragraph stating what the default value for urlSafe is. Done. https://codereview.chromium.org/1159093002/diff/40001/lib/src/base64.dart#new... lib/src/base64.dart:52: * optional line separator (CR + LF) for each 76 char output. On 2015/06/01 12:20:59, Søren Gjesse wrote: > Please add a paragraph stating what the default value for addLineSeparator is. > > Should the default value be true? Or should the default value be the same as > urlSafe, as it seems like the most common use is either 'non-URL with line > separation (e.g. for reading/writing files) and URL without line separation > (e.g. data: URL). However this might be more difficult to explain. Done. https://codereview.chromium.org/1159093002/diff/40001/lib/src/base64.dart#new... lib/src/base64.dart:52: * optional line separator (CR + LF) for each 76 char output. On 2015/06/01 12:20:58, Søren Gjesse wrote: > Should the line length be configurable. According to > http://en.wikipedia.org/wiki/Base64 PEM files have a max line length of 64 > characters. We can do that in a separate change. https://codereview.chromium.org/1159093002/diff/40001/lib/src/base64.dart#new... lib/src/base64.dart:87: const Base64Encoder({bool urlSafe: false, bool addLineSeparator: false}) On 2015/06/01 12:20:59, Søren Gjesse wrote: > As this is also public it should have the same information as the Base64Codec > constructor. Done. https://codereview.chromium.org/1159093002/diff/40001/lib/src/base64.dart#new... lib/src/base64.dart:111: int baseOutputLength = ((length ~/3) * 4); On 2015/06/01 12:20:59, Søren Gjesse wrote: > nit: Space before 3. Done. https://codereview.chromium.org/1159093002/diff/40001/lib/src/base64.dart#new... lib/src/base64.dart:123: int x = ((bytes[i++] << 16) & 0xFFFFFF) | On 2015/06/01 12:20:58, Søren Gjesse wrote: > nit: Maybe write 0x00FFFFFF for "completeness". Done. https://codereview.chromium.org/1159093002/diff/40001/lib/src/base64.dart#new... lib/src/base64.dart:129: out[j++] = lookup.codeUnitAt(x & 0x3f); On 2015/06/01 12:20:59, Søren Gjesse wrote: > nit: Please be consistent 0x3f -> 0x3F Done. https://codereview.chromium.org/1159093002/diff/40001/test/base64_test.dart File test/base64_test.dart (right): https://codereview.chromium.org/1159093002/diff/40001/test/base64_test.dart#n... test/base64_test.dart:22: test('streaming decoder for malformed input', _testStreamingDecoderForMalformedInput); On 2015/06/01 12:20:59, Søren Gjesse wrote: > nit: Long line. Done. https://codereview.chromium.org/1159093002/diff/40001/test/base64_test.dart#n... test/base64_test.dart:108: void _testEncoder() { On 2015/06/01 12:20:59, Søren Gjesse wrote: > Please keep the tests using CryptoUtils as long as we support it. > > That is have both this and an equivalent test that uses CryptoUtils. I've added tests confirming that the old API is working as expected. https://codereview.chromium.org/1159093002/diff/40001/test/base64_test.dart#n... test/base64_test.dart:221: expect(BASE64.encode(dec, urlSafe: true), '-_A='); On 2015/06/01 12:20:59, Søren Gjesse wrote: > I am not sure here. Related to the padding character > http://en.wikipedia.org/wiki/Base64 says "must be percent-encoded in URL". > > Is that something we should do, or at least have an option for? We add the option to encode the padding character in another change
Just a few more comments. https://codereview.chromium.org/1159093002/diff/50001/lib/src/base64.dart File lib/src/base64.dart (right): https://codereview.chromium.org/1159093002/diff/50001/lib/src/base64.dart#new... lib/src/base64.dart:113: nit: Please remove the empty line after the dartdoc comment. https://codereview.chromium.org/1159093002/diff/50001/lib/src/base64.dart#new... lib/src/base64.dart:244: nit: Please remove the empty line after the dartdoc comment. https://codereview.chromium.org/1159093002/diff/50001/lib/src/base64.dart#new... lib/src/base64.dart:245: const Base64Decoder(); Having thought about this, I think the decode should take the optional named urlSafe argument, and use that for checking instead of the auto-detection of the encoding.
https://codereview.chromium.org/1159093002/diff/50001/lib/src/base64.dart File lib/src/base64.dart (right): https://codereview.chromium.org/1159093002/diff/50001/lib/src/base64.dart#new... lib/src/base64.dart:113: On 2015/06/03 09:20:58, Søren Gjesse wrote: > nit: Please remove the empty line after the dartdoc comment. Done. https://codereview.chromium.org/1159093002/diff/50001/lib/src/base64.dart#new... lib/src/base64.dart:244: On 2015/06/03 09:20:58, Søren Gjesse wrote: > nit: Please remove the empty line after the dartdoc comment. Done. https://codereview.chromium.org/1159093002/diff/50001/lib/src/base64.dart#new... lib/src/base64.dart:245: const Base64Decoder(); On 2015/06/03 09:20:58, Søren Gjesse wrote: > Having thought about this, I think the decode should take the optional named > urlSafe argument, and use that for checking instead of the auto-detection of the > encoding. We decided to discuss it in another change.
Message was sent while issue was closed.
Committed patchset #5 (id:70001) manually as 3a2b31228cfe03880af829798ffe16d05f37f6e3 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
