Adds a base32 component.

Modelled on base/base64url.h.
Modifies chrome/installer/util/shell_util to use this component.
It will soon be used by components/certificate_transparency as well.

Discussion:
https://groups.google.com/a/chromium.org/forum/?pli=1#!topic/chromium-dev/pieHLJZZE7k

BUG=612439
Committed: https://crrev.com/633e8bc192437bdad37c49aa3d4024fe2d641c94
Cr-Commit-Position: refs/heads/master@{#398539}

[Rob Percival, 2016-05-27 17:45:35]

The CQ bit was checked by robpercival@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-27 17:46:17]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2017123002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2017123002/1

[commit-bot: I haz the power, 2016-05-27 19:00:40]

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

[commit-bot: I haz the power, 2016-05-27 19:00:41]

Dry run: This issue passed the CQ dry run.

[Rob Percival, 2016-06-01 13:24:50]

The CQ bit was checked by robpercival@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-01 13:25:09]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2017123002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2017123002/20001

[Rob Percival, 2016-06-01 13:27:45]

Description was changed from

==========
Refactor base32 encoding into a new component

BUG=612439
==========

to

==========
Adds a base32 component.

Modelled on base/base64url.h.
Modifies chrome/installer/util/shell_util to use this component.
It will soon be used by components/certificate_transparency as well.

BUG=612439
==========

[Rob Percival, 2016-06-01 13:29:14]

Description was changed from

==========
Adds a base32 component.

Modelled on base/base64url.h.
Modifies chrome/installer/util/shell_util to use this component.
It will soon be used by components/certificate_transparency as well.

BUG=612439
==========

to

==========
Adds a base32 component.

Modelled on base/base64url.h.
Modifies chrome/installer/util/shell_util to use this component.
It will soon be used by components/certificate_transparency as well.

Discussion:
https://groups.google.com/a/chromium.org/forum/?pli=1#!topic/chromium-dev/pie...

BUG=612439
==========

[commit-bot: I haz the power, 2016-06-01 13:30:38]

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

[commit-bot: I haz the power, 2016-06-01 13:30:39]

Dry run: Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Rob Percival, 2016-06-01 13:43:01]

The CQ bit was checked by robpercival@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-01 13:43:19]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2017123002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2017123002/40001

[Rob Percival, 2016-06-01 13:59:13]

The CQ bit was checked by robpercival@chromium.org to run a CQ dry run

[Rob Percival, 2016-06-01 13:59:25]

robpercival@chromium.org changed reviewers:
+ blundell@chromium.org

[commit-bot: I haz the power, 2016-06-01 13:59:41]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2017123002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2017123002/60001

[commit-bot: I haz the power, 2016-06-01 14:06:33]

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

[commit-bot: I haz the power, 2016-06-01 14:06:35]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Rob Percival, 2016-06-01 14:11:41]

The CQ bit was checked by robpercival@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-01 14:11:51]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2017123002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2017123002/80001

[Rob Percival, 2016-06-01 14:12:39]

robpercival@chromium.org changed reviewers:
+ sdefresne@chromium.org

[Rob Percival, 2016-06-01 14:12:39]

PTAL

[sdefresne, 2016-06-01 14:25:38]

https://codereview.chromium.org/2017123002/diff/80001/components/base32/base32.h
File components/base32/base32.h (right):

https://codereview.chromium.org/2017123002/diff/80001/components/base32/base3...
components/base32/base32.h:25: // encoded |*output|. |input| and |*output| may
not reference the same storage.
I think it should be "|input| and |*output| must not reference the same
storage." as by "may not" I understand that it is supported configuration.

https://codereview.chromium.org/2017123002/diff/80001/components/base32/base3...
File components/base32/base32_unittest.cc (right):

https://codereview.chromium.org/2017123002/diff/80001/components/base32/base3...
components/base32/base32_unittest.cc:23: Base32Encode(test_str.substr(0, i),
Base32EncodePolicy::OMIT_PADDING,
nit: Can you instead do the following to avoid allocating a new string?

Base32Encode(base::StringPiece(test_str.data(), i), ...)

https://codereview.chromium.org/2017123002/diff/80001/components/base32/base3...
components/base32/base32_unittest.cc:40: Base32Encode(test_str.substr(0, i),
Base32EncodePolicy::INCLUDE_PADDING,
ditto

https://codereview.chromium.org/2017123002/diff/80001/components/base32/base3...
components/base32/base32_unittest.cc:57: TEST(Base32Test,
EncodesEmptyStringAsEmptyString) {
Both EncodesRfcTestVectorsCorrectlyWithoutPadding and
EncodesRfcTestVectorsCorrectlyWithPadding already tests that an empty string is
encoded to an empty string.

I think this test checks that the output is cleared before encoding, so maybe
rename the test to ClearOutputOnEncoding?

[Rob Percival, 2016-06-01 14:37:57]

https://codereview.chromium.org/2017123002/diff/80001/components/base32/base32.h
File components/base32/base32.h (right):

https://codereview.chromium.org/2017123002/diff/80001/components/base32/base3...
components/base32/base32.h:25: // encoded |*output|. |input| and |*output| may
not reference the same storage.
On 2016/06/01 14:25:38, sdefresne wrote:
> I think it should be "|input| and |*output| must not reference the same
> storage." as by "may not" I understand that it is supported configuration.

Done.

https://codereview.chromium.org/2017123002/diff/80001/components/base32/base3...
File components/base32/base32_unittest.cc (right):

https://codereview.chromium.org/2017123002/diff/80001/components/base32/base3...
components/base32/base32_unittest.cc:23: Base32Encode(test_str.substr(0, i),
Base32EncodePolicy::OMIT_PADDING,
On 2016/06/01 14:25:38, sdefresne wrote:
> nit: Can you instead do the following to avoid allocating a new string?
> 
> Base32Encode(base::StringPiece(test_str.data(), i), ...)

Done. With it being test code, I had just opted for the most readable method.

https://codereview.chromium.org/2017123002/diff/80001/components/base32/base3...
components/base32/base32_unittest.cc:40: Base32Encode(test_str.substr(0, i),
Base32EncodePolicy::INCLUDE_PADDING,
On 2016/06/01 14:25:38, sdefresne wrote:
> ditto

Done.

https://codereview.chromium.org/2017123002/diff/80001/components/base32/base3...
components/base32/base32_unittest.cc:57: TEST(Base32Test,
EncodesEmptyStringAsEmptyString) {
On 2016/06/01 14:25:38, sdefresne wrote:
> Both EncodesRfcTestVectorsCorrectlyWithoutPadding and
> EncodesRfcTestVectorsCorrectlyWithPadding already tests that an empty string
is
> encoded to an empty string.
> 
> I think this test checks that the output is cleared before encoding, so maybe
> rename the test to ClearOutputOnEncoding?

Done.

[sdefresne, 2016-06-01 14:45:10]

https://codereview.chromium.org/2017123002/diff/100001/components/base32/base...
File components/base32/base32.h (right):

https://codereview.chromium.org/2017123002/diff/100001/components/base32/base...
components/base32/base32.h:26: void Base32Encode(base::StringPiece input,
It looks like this class is usually passed by const-reference, so maybe do it
here too

void Base32Encode(const base::StringPiece& input, ...);

It also looks strange to have this class return a std::string by pointer. The
compiler should generally be able to do return value optimisation for such a
class, so why not have the simpler interface?

std::string Base32Encode(const base::StringPiece& input, Base32EncodePolicy
policy);

[Rob Percival, 2016-06-01 14:49:52]

https://codereview.chromium.org/2017123002/diff/100001/components/base32/base...
File components/base32/base32.h (right):

https://codereview.chromium.org/2017123002/diff/100001/components/base32/base...
components/base32/base32.h:26: void Base32Encode(base::StringPiece input,
On 2016/06/01 14:45:10, sdefresne wrote:
> It looks like this class is usually passed by const-reference, so maybe do it
> here too
> 
> void Base32Encode(const base::StringPiece& input, ...);
> 
> It also looks strange to have this class return a std::string by pointer. The
> compiler should generally be able to do return value optimisation for such a
> class, so why not have the simpler interface?
> 
> std::string Base32Encode(const base::StringPiece& input, Base32EncodePolicy
> policy);
> 

RE StringPiece const-ref: base/strings/string_piece.h says to pass it by value
unless circumstances require otherwise
(https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/strin...).

RE returning string: I thought you might say that - I'd actually prefer
returning a string like that. I only did it this way to match the
Base64UrlEncode function signature
(https://code.google.com/p/chromium/codesearch#chromium/src/base/base64url.h&l=30),
but I'm happy to change it.

[sdefresne, 2016-06-01 14:57:30]

https://codereview.chromium.org/2017123002/diff/100001/components/base32/base...
File components/base32/base32.h (right):

https://codereview.chromium.org/2017123002/diff/100001/components/base32/base...
components/base32/base32.h:26: void Base32Encode(base::StringPiece input,
On 2016/06/01 14:49:52, Rob Percival wrote:
> On 2016/06/01 14:45:10, sdefresne wrote:
> > It looks like this class is usually passed by const-reference, so maybe do
it
> > here too
> > 
> > void Base32Encode(const base::StringPiece& input, ...);
> > 
> > It also looks strange to have this class return a std::string by pointer.
The
> > compiler should generally be able to do return value optimisation for such a
> > class, so why not have the simpler interface?
> > 
> > std::string Base32Encode(const base::StringPiece& input, Base32EncodePolicy
> > policy);
> > 
> 
> RE StringPiece const-ref: base/strings/string_piece.h says to pass it by value
> unless circumstances require otherwise
>
(https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/strin...).

Ack. I was looking for that very comment thank you finding it.

> RE returning string: I thought you might say that - I'd actually prefer
> returning a string like that. I only did it this way to match the
> Base64UrlEncode function signature
>
(https://code.google.com/p/chromium/codesearch#chromium/src/base/base64url.h&l=30),
> but I'm happy to change it.

I'd prefer to have the function return a std::string if possible even if it make
the signature differ from Base64UrlEncode (I think Base64UrlEncode takes output
by pointer because it support input and output to point to the same storage
which is not the case here).

[Rob Percival, 2016-06-01 15:04:02]

The CQ bit was checked by robpercival@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-01 15:04:19]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2017123002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2017123002/140001

[Rob Percival, 2016-06-01 15:04:40]

https://codereview.chromium.org/2017123002/diff/100001/components/base32/base...
File components/base32/base32.h (right):

https://codereview.chromium.org/2017123002/diff/100001/components/base32/base...
components/base32/base32.h:26: void Base32Encode(base::StringPiece input,
On 2016/06/01 14:57:30, sdefresne wrote:
> On 2016/06/01 14:49:52, Rob Percival wrote:
> > On 2016/06/01 14:45:10, sdefresne wrote:
> > > It looks like this class is usually passed by const-reference, so maybe do
> it
> > > here too
> > > 
> > > void Base32Encode(const base::StringPiece& input, ...);
> > > 
> > > It also looks strange to have this class return a std::string by pointer.
> The
> > > compiler should generally be able to do return value optimisation for such
a
> > > class, so why not have the simpler interface?
> > > 
> > > std::string Base32Encode(const base::StringPiece& input,
Base32EncodePolicy
> > > policy);
> > > 
> > 
> > RE StringPiece const-ref: base/strings/string_piece.h says to pass it by
value
> > unless circumstances require otherwise
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/strin...).
> 
> Ack. I was looking for that very comment thank you finding it.
> 
> > RE returning string: I thought you might say that - I'd actually prefer
> > returning a string like that. I only did it this way to match the
> > Base64UrlEncode function signature
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/base/base64url.h&l=30),
> > but I'm happy to change it.
> 
> I'd prefer to have the function return a std::string if possible even if it
make
> the signature differ from Base64UrlEncode (I think Base64UrlEncode takes
output
> by pointer because it support input and output to point to the same storage
> which is not the case here).

Done.

[sdefresne, 2016-06-01 16:16:17]

lgtm

[Rob Percival, 2016-06-01 16:16:29]

robpercival@chromium.org changed reviewers:
+ gab@chromium.org
- blundell@chromium.org

[commit-bot: I haz the power, 2016-06-01 16:36:12]

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

[commit-bot: I haz the power, 2016-06-01 16:36:13]

Dry run: This issue passed the CQ dry run.

[Rob Percival, 2016-06-02 06:26:39]

robpercival@chromium.org changed required reviewers:
+ gab@chromium.org

[Rob Percival, 2016-06-02 06:26:40]

PTAL @gab

[Rob Percival, 2016-06-02 19:15:28]

Description was changed from

==========
Adds a base32 component.

Modelled on base/base64url.h.
Modifies chrome/installer/util/shell_util to use this component.
It will soon be used by components/certificate_transparency as well.

Discussion:
https://groups.google.com/a/chromium.org/forum/?pli=1#!topic/chromium-dev/pie...

BUG=612439
==========

to

==========
Modelled on base/base64url.h.
Modifies chrome/installer/util/shell_util to use this component.
It will soon be used by components/certificate_transparency as well.

Discussion:
https://groups.google.com/a/chromium.org/forum/?pli=1#!topic/chromium-dev/pie...

BUG=612439
==========

[gab, 2016-06-03 18:46:11]

Thanks for doing this, first pass below

https://codereview.chromium.org/2017123002/diff/160001/components/base32/base...
File components/base32/base32.cc (right):

https://codereview.chromium.org/2017123002/diff/160001/components/base32/base...
components/base32/base32.cc:13: const char kEncoding[] =
"ABCDEFGHIJKLMNOPQRSTUVWXYZ234567";
constexpr

https://codereview.chromium.org/2017123002/diff/160001/components/base32/base...
components/base32/base32.cc:17: std::string output;
if (input.size() > std::numeric_limits<size_t>::max() / 8) {
  NOTREACHED() << "Input is too large and would overflow size computation
below.";
  return std::string();
}

// Per RFC4648, the output is formed of 8 characters per 40 bits of input and
// another 8 characters for the last group of [1,39] bits in the input.
// That is: ceil(input.size() * 8.0 / 40.0) * 8 ==
//          ceil(input.size() / 5.0) * 8 ==
//          ((input.size() + 4) / 5) * 8.
const size_t padded_length = ((input.size() + 4) / 5) * 8;

// When no padding is used, the output is exactly 1 character per 5 bits of
// input and one more for the last [1,4] bits.
// That is: ceil(input.size() * 8.0 / 5.0) ==
//          (input.size() * 8 + 4) / 5.
const size_t unpadded_length = (input.size() * 8 + 4) / 5;


std::string output;
output.reserve(encoded_length);

https://codereview.chromium.org/2017123002/diff/160001/components/base32/base...
components/base32/base32.cc:19: // Transform 5 input bytes into 8 output bytes
at a time.
s/output bytes/characters/ (or "encoded characters")

https://codereview.chromium.org/2017123002/diff/160001/components/base32/base...
components/base32/base32.cc:21: for (size_t input_offset = 0; input_offset <
input.size();
0U

https://codereview.chromium.org/2017123002/diff/160001/components/base32/base...
components/base32/base32.cc:22: input_offset += 5) {
5U

https://codereview.chromium.org/2017123002/diff/160001/components/base32/base...
components/base32/base32.cc:23: // Bits of input left to encode.
// Bits of input left to encode in the current 40 bits (or less if at end)
block.

https://codereview.chromium.org/2017123002/diff/160001/components/base32/base...
components/base32/base32.cc:25: std::min(size_t(5), input.size() - input_offset)
* 8;
s/size_t(5)/5U/

https://codereview.chromium.org/2017123002/diff/160001/components/base32/base...
components/base32/base32.cc:27: // 40 bits is used because it is the lowest
common multiple of 5 and 8.
I'd remove the second sentence, no need to justify, that's the spec anyways. So
perhaps:

// A buffer for up to 40 bits of input that is consumed 5 bits at a time per
RFC4648.

https://codereview.chromium.org/2017123002/diff/160001/components/base32/base...
components/base32/base32.cc:30: // Load 8 bits at a time into the buffer.
s/Load 8 bits at a time./Fill the buffer, 8 bits at a time./ ? (the current
comment somehow made me think you would only load 8 bits in this pass of the
loop)

https://codereview.chromium.org/2017123002/diff/160001/components/base32/base...
components/base32/base32.cc:32: for (size_t i = 0; i < bits_remaining / 8; ++i)
{
If |bits_remaining == 39| then 39 / 8 is 4 and you will only fill 32 bits in the
input_group.

(Need more tests to cover that use case?)

This algorithm appears more complex then the one that was in ShellUtil, I think
we can re-use that one with a slight tweak to handle padding. i.e. (plus tweak
to avoid special casing single byte inputs -- still need to return early on
empty inputs but that's okay):

// A bit stream which will be read from the left and appended to from the 
// right as it's emptied. 
uint16_t bit_stream = (static_cast<uint8_t>(input[0]) << 8); 
size_t next_byte_index = 1; 
int free_bits = 8;
while (free_bits < 16) {  
  // Extract the 5 leftmost bits in the stream  
  output.push_back(kEncoding[(bit_stream & 0xf800) >> 11]);  
  bit_stream <<= 5; 
  free_bits += 5; 

  // If there is enough room in the bit stream, inject another byte (if there 
  // are any left...).  
  if (free_bits >= 8 && next_byte_index < size) { 
    free_bits -= 8; 
    bit_stream += static_cast<uint8_t>(input[next_byte_index++]) << free_bits;  
  } 
}

if (policy == Base32EncodePolicy::INCLUDE_PADDING) {
  output.append(padded_length - unpadded_length,'=');
}

https://codereview.chromium.org/2017123002/diff/160001/components/base32/base...
components/base32/base32.cc:54: 
#include "base/logging.h"

DCHECK_EQ(encoded_length, output.size());

https://codereview.chromium.org/2017123002/diff/160001/components/base32/base...
File components/base32/base32.h (right):

https://codereview.chromium.org/2017123002/diff/160001/components/base32/base...
components/base32/base32.h:17: // Remove the trailing padding from the output.
// Omit trailing padding to the output. Such an output will not be decodable but
its size is guaranteed to be |max(8, ceil(input.size() * 8.0 / 5.0))|.

[Rob Percival, 2016-06-04 06:20:24]

robpercival@chromium.org changed required reviewers:
- gab@chromium.org

[Rob Percival, 2016-06-04 06:20:25]

https://codereview.chromium.org/2017123002/diff/160001/components/base32/base...
File components/base32/base32.cc (right):

https://codereview.chromium.org/2017123002/diff/160001/components/base32/base...
components/base32/base32.cc:13: const char kEncoding[] =
"ABCDEFGHIJKLMNOPQRSTUVWXYZ234567";
On 2016/06/03 18:46:11, gab wrote:
> constexpr

Done.

https://codereview.chromium.org/2017123002/diff/160001/components/base32/base...
components/base32/base32.cc:17: std::string output;
On 2016/06/03 18:46:11, gab wrote:
> if (input.size() > std::numeric_limits<size_t>::max() / 8) {
>   NOTREACHED() << "Input is too large and would overflow size computation
> below.";
>   return std::string();
> }
> 
> // Per RFC4648, the output is formed of 8 characters per 40 bits of input and
> // another 8 characters for the last group of [1,39] bits in the input.
> // That is: ceil(input.size() * 8.0 / 40.0) * 8 ==
> //          ceil(input.size() / 5.0) * 8 ==
> //          ((input.size() + 4) / 5) * 8.
> const size_t padded_length = ((input.size() + 4) / 5) * 8;
> 
> // When no padding is used, the output is exactly 1 character per 5 bits of
> // input and one more for the last [1,4] bits.
> // That is: ceil(input.size() * 8.0 / 5.0) ==
> //          (input.size() * 8 + 4) / 5.
> const size_t unpadded_length = (input.size() * 8 + 4) / 5;
> 
> 
> std::string output;
> output.reserve(encoded_length);

Done. An alternative would be letting the caller reserve capacity in the output
string instead (and make it an out parameter, rather than a return value). I
only suggest this because the two prospective users of this component both only
encode fixed-length input (MD5 hashes and SHA256 hashes, respectively), so this
code adds unnecessary complexity. I'm happy with either.

https://codereview.chromium.org/2017123002/diff/160001/components/base32/base...
components/base32/base32.cc:19: // Transform 5 input bytes into 8 output bytes
at a time.
On 2016/06/03 18:46:11, gab wrote:
> s/output bytes/characters/ (or "encoded characters")

Acknowledged.

https://codereview.chromium.org/2017123002/diff/160001/components/base32/base...
components/base32/base32.cc:21: for (size_t input_offset = 0; input_offset <
input.size();
On 2016/06/03 18:46:11, gab wrote:
> 0U

What effect does this have?

https://codereview.chromium.org/2017123002/diff/160001/components/base32/base...
components/base32/base32.cc:22: input_offset += 5) {
On 2016/06/03 18:46:11, gab wrote:
> 5U

What effect does this have?

https://codereview.chromium.org/2017123002/diff/160001/components/base32/base...
components/base32/base32.cc:23: // Bits of input left to encode.
On 2016/06/03 18:46:11, gab wrote:
> // Bits of input left to encode in the current 40 bits (or less if at end)
> block.

Acknowledged.

https://codereview.chromium.org/2017123002/diff/160001/components/base32/base...
components/base32/base32.cc:25: std::min(size_t(5), input.size() - input_offset)
* 8;
On 2016/06/03 18:46:11, gab wrote:
> s/size_t(5)/5U/

Is this semantically better or just shorter?

https://codereview.chromium.org/2017123002/diff/160001/components/base32/base...
components/base32/base32.cc:27: // 40 bits is used because it is the lowest
common multiple of 5 and 8.
On 2016/06/03 18:46:11, gab wrote:
> I'd remove the second sentence, no need to justify, that's the spec anyways.
So
> perhaps:
> 
> // A buffer for up to 40 bits of input that is consumed 5 bits at a time per
> RFC4648.

Acknowledged.

https://codereview.chromium.org/2017123002/diff/160001/components/base32/base...
components/base32/base32.cc:30: // Load 8 bits at a time into the buffer.
On 2016/06/03 18:46:11, gab wrote:
> s/Load 8 bits at a time./Fill the buffer, 8 bits at a time./ ? (the current
> comment somehow made me think you would only load 8 bits in this pass of the
> loop)

Acknowledged.

https://codereview.chromium.org/2017123002/diff/160001/components/base32/base...
components/base32/base32.cc:32: for (size_t i = 0; i < bits_remaining / 8; ++i)
{
On 2016/06/03 18:46:11, gab wrote:
> If |bits_remaining == 39| then 39 / 8 is 4 and you will only fill 32 bits in
the
> input_group.
> 
> (Need more tests to cover that use case?)

|bits_remaining| is always a multiple of 8 at this point, so truncation is not
possible.

> This algorithm appears more complex then the one that was in ShellUtil, I
think
> we can re-use that one with a slight tweak to handle padding. i.e. (plus tweak
> to avoid special casing single byte inputs -- still need to return early on
> empty inputs but that's okay):
> 
> // A bit stream which will be read from the left and appended to from the 
> // right as it's emptied. 
> uint16_t bit_stream = (static_cast<uint8_t>(input[0]) << 8); 
> size_t next_byte_index = 1; 
> int free_bits = 8;
> while (free_bits < 16) {  
>   // Extract the 5 leftmost bits in the stream  
>   output.push_back(kEncoding[(bit_stream & 0xf800) >> 11]);  
>   bit_stream <<= 5; 
>   free_bits += 5; 
> 
>   // If there is enough room in the bit stream, inject another byte (if there 
>   // are any left...).  
>   if (free_bits >= 8 && next_byte_index < size) { 
>     free_bits -= 8; 
>     bit_stream += static_cast<uint8_t>(input[next_byte_index++]) << free_bits;
 
>   } 
> }
> 
> if (policy == Base32EncodePolicy::INCLUDE_PADDING) {
>   output.append(padded_length - unpadded_length,'=');
> }

I suppose it is a bit more complicated. It's closer to how the RFC describes
doing it (in groups of 40 bits), but simplicity trumps that I think. I'll swap
in this algorithm.

https://codereview.chromium.org/2017123002/diff/160001/components/base32/base...
components/base32/base32.cc:54: 
On 2016/06/03 18:46:11, gab wrote:
> #include "base/logging.h"
> 
> DCHECK_EQ(encoded_length, output.size()); 

Done.

https://codereview.chromium.org/2017123002/diff/160001/components/base32/base...
File components/base32/base32.h (right):

https://codereview.chromium.org/2017123002/diff/160001/components/base32/base...
components/base32/base32.h:17: // Remove the trailing padding from the output.
On 2016/06/03 18:46:11, gab wrote:
> // Omit trailing padding to the output. Such an output will not be decodable
but
> its size is guaranteed to be |max(8, ceil(input.size() * 8.0 / 5.0))|.

That isn't quite accurate. It is decodable, if you know what size the input was
(in fact, I'm counting on that for my use of this encoder). Also, in the case of
|input| being empty, the output will also be empty (so it does not have a
minimum size of 8 bytes as stated). Is there much value to stating what the size
will be anyway?

By the way, should I update base/base64url.h as well? This enum was more or less
copy-and-pasted from there.

[Rob Percival, 2016-06-04 06:21:53]

The CQ bit was checked by robpercival@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-04 06:22:09]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2017123002/180001

[commit-bot: I haz the power, 2016-06-04 07:55:20]

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

[commit-bot: I haz the power, 2016-06-04 07:55:22]

Dry run: This issue passed the CQ dry run.

[gab, 2016-06-07 01:24:10]

lgtm w/ comments, thanks!

https://codereview.chromium.org/2017123002/diff/160001/components/base32/base...
File components/base32/base32.cc (right):

https://codereview.chromium.org/2017123002/diff/160001/components/base32/base...
components/base32/base32.cc:21: for (size_t input_offset = 0; input_offset <
input.size();
On 2016/06/04 06:20:25, Rob Percival wrote:
> On 2016/06/03 18:46:11, gab wrote:
> > 0U
> 
> What effect does this have?

Explicitly declares this constant as unsigned. I guess it's not required because
there is an implicit conversion at compile-time otherwise anyways, just a habit
of mine to be explicit about types.

https://codereview.chromium.org/2017123002/diff/160001/components/base32/base...
components/base32/base32.cc:25: std::min(size_t(5), input.size() - input_offset)
* 8;
On 2016/06/04 06:20:25, Rob Percival wrote:
> On 2016/06/03 18:46:11, gab wrote:
> > s/size_t(5)/5U/
> 
> Is this semantically better or just shorter?

It's more readable IMO because it's clearly a constant whereas size_t(5) is
constructor syntax. I assume you only do size_t(5) to avoid the signed/unsigned
warnings , 5U does just that.

https://codereview.chromium.org/2017123002/diff/160001/components/base32/base...
File components/base32/base32.h (right):

https://codereview.chromium.org/2017123002/diff/160001/components/base32/base...
components/base32/base32.h:17: // Remove the trailing padding from the output.
On 2016/06/04 06:20:25, Rob Percival wrote:
> On 2016/06/03 18:46:11, gab wrote:
> > // Omit trailing padding to the output. Such an output will not be decodable
> but
> > its size is guaranteed to be |max(8, ceil(input.size() * 8.0 / 5.0))|.
> 
> That isn't quite accurate. It is decodable, if you know what size the input
was
> (in fact, I'm counting on that for my use of this encoder). Also, in the case
of
> |input| being empty, the output will also be empty (so it does not have a
> minimum size of 8 bytes as stated). Is there much value to stating what the
size
> will be anyway?

Your latest version lg to me.

> 
> By the way, should I update base/base64url.h as well? This enum was more or
less
> copy-and-pasted from there.

If you want to but I don't feel strongly.

https://codereview.chromium.org/2017123002/diff/180001/components/base32/base...
File components/base32/base32_unittest.cc (right):

https://codereview.chromium.org/2017123002/diff/180001/components/base32/base...
components/base32/base32_unittest.cc:15: const std::string test_str = "foobar";
s/const std::string/constexpr char[]/

here and in all tests below

[Rob Percival, 2016-06-07 23:09:36]

The CQ bit was checked by robpercival@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-07 23:10:08]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2017123002/200001

[Rob Percival, 2016-06-07 23:11:15]

PTAL again sdefresne, as the code has changed significantly since your LGTM.

https://codereview.chromium.org/2017123002/diff/180001/components/base32/base...
File components/base32/base32_unittest.cc (right):

https://codereview.chromium.org/2017123002/diff/180001/components/base32/base...
components/base32/base32_unittest.cc:15: const std::string test_str = "foobar";
On 2016/06/07 01:24:10, gab wrote:
> s/const std::string/constexpr char[]/
> 
> here and in all tests below

Done.

[commit-bot: I haz the power, 2016-06-07 23:48:08]

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

[commit-bot: I haz the power, 2016-06-07 23:48:09]

Dry run: Try jobs failed on following builders:
  ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)

[sdefresne, 2016-06-08 09:22:35]

still lgtm (once the windows build failure is fixed)

[Rob Percival, 2016-06-08 12:15:41]

On 2016/06/08 09:22:35, sdefresne wrote:
> still lgtm (once the windows build failure is fixed)

Ah someone has very recently added another use of the old base32 encode
function. I've migrated that to use this component as well.

[Rob Percival, 2016-06-08 12:15:56]

The CQ bit was checked by robpercival@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-08 12:16:14]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2017123002/240001

[commit-bot: I haz the power, 2016-06-08 13:18:02]

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

[commit-bot: I haz the power, 2016-06-08 13:18:03]

Dry run: This issue passed the CQ dry run.

[Rob Percival, 2016-06-08 13:36:16]

The CQ bit was checked by robpercival@chromium.org

[Rob Percival, 2016-06-08 13:36:17]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org,
sdefresne@chromium.org
Link to the patchset: https://codereview.chromium.org/2017123002/#ps240001
(title: "Migrates installer/setup/user_hive_visitor.cc to use base32 component")

[commit-bot: I haz the power, 2016-06-08 13:36:32]

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

[commit-bot: I haz the power, 2016-06-08 13:45:23]

Description was changed from

==========
Modelled on base/base64url.h.
Modifies chrome/installer/util/shell_util to use this component.
It will soon be used by components/certificate_transparency as well.

Discussion:
https://groups.google.com/a/chromium.org/forum/?pli=1#!topic/chromium-dev/pie...

BUG=612439
==========

to

==========
Modelled on base/base64url.h.
Modifies chrome/installer/util/shell_util to use this component.
It will soon be used by components/certificate_transparency as well.

Discussion:
https://groups.google.com/a/chromium.org/forum/?pli=1#!topic/chromium-dev/pie...

BUG=612439
==========

[commit-bot: I haz the power, 2016-06-08 13:45:24]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-06-08 13:48:13]

Description was changed from

==========
Modelled on base/base64url.h.
Modifies chrome/installer/util/shell_util to use this component.
It will soon be used by components/certificate_transparency as well.

Discussion:
https://groups.google.com/a/chromium.org/forum/?pli=1#!topic/chromium-dev/pie...

BUG=612439
==========

to

==========
Modelled on base/base64url.h.
Modifies chrome/installer/util/shell_util to use this component.
It will soon be used by components/certificate_transparency as well.

Discussion:
https://groups.google.com/a/chromium.org/forum/?pli=1#!topic/chromium-dev/pie...

BUG=612439

Committed: https://crrev.com/633e8bc192437bdad37c49aa3d4024fe2d641c94
Cr-Commit-Position: refs/heads/master@{#398539}
==========

[commit-bot: I haz the power, 2016-06-08 13:48:15]

Patchset 13 (id:??) landed as
https://crrev.com/633e8bc192437bdad37c49aa3d4024fe2d641c94
Cr-Commit-Position: refs/heads/master@{#398539}

[jam, 2016-06-28 17:17:09]

Why would we create a component for 2 files instead of putting these in base/ by
the other decoding functions?

[Rob Percival, 2016-06-28 17:26:54]

On 2016/06/28 17:17:09, jam wrote:
> Why would we create a component for 2 files instead of putting these in base/
by
> the other decoding functions?

Check out the discussion linked in the description.