Introducing a net::GenerateMimeMultipartBoundary helper.

Introducing a net::GenerateMimeMultipartBoundary helper.

... and using the helper in places that had introduced separate
functions for generating a random Mime boundary (sometimes with
bugs - i.e. using '*' characters is disallowed by RFC 1341).

TBR=bartfab@chromium.org, stevet@chromium.org
BUG=575733
Committed: https://crrev.com/c644f37125ab27311d697af61e56c64d0e29027e
Cr-Commit-Position: refs/heads/master@{#368404}

[Łukasz Anforowicz, 2015-12-23 00:18:04]

lukasza@chromium.org changed reviewers:
+ rsleevi@chromium.org

[Łukasz Anforowicz, 2015-12-23 00:18:04]

rsleevi@, could you please take a look?  (focusing on changes to net/base/...
files that you are an owner of, but also evaluating the overall desirability /
correctness / risk of this change).

FWIW, this CL has been triggered by a CR comment some time ago here:
https://codereview.chromium.org/1386873003/diff/160001/content/common/frame_m...

[Ryan Sleevi, 2015-12-28 23:30:01]

Overall LG, although I haven't checked to see if there are any other places that
could benefit from this (ideally, we'd get them all in one CL)

The only question I have is with this new method, and the entropy provided,
whether or not it's statistically likely that we would reasonably have a
conflict, and whether it's better (in the name of efficiency) to not check the
data. I'm torn, but it seems that given that .find() is linear time, there might
be small gains skipping that check?

https://codereview.chromium.org/1547593002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/upload_job_impl.cc (right):

https://codereview.chromium.org/1547593002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/upload_job_impl.cc:224: } while (!found && retry
<= kMimeBoundaryRetries);
This seems overly conservative/unnecessary with your new change

https://codereview.chromium.org/1547593002/diff/60001/google_apis/drive/base_...
File google_apis/drive/base_requests.cc (right):

https://codereview.chromium.org/1547593002/diff/60001/google_apis/drive/base_...
google_apis/drive/base_requests.cc:219: boundary =
net::GenerateMimeMultipartBoundary();
Ditto as to whether needed?

https://codereview.chromium.org/1547593002/diff/60001/net/base/mime_util.cc
File net/base/mime_util.cc (right):

https://codereview.chromium.org/1547593002/diff/60001/net/base/mime_util.cc#n...
net/base/mime_util.cc:597: result += "----";
pedantry:
result.append(random_base64);
result.append("----");

No need to make the compiler try and figure out that the return value is unused
and can thus be avoided :)

https://codereview.chromium.org/1547593002/diff/60001/net/base/mime_util_unit...
File net/base/mime_util_unittest.cc (right):

https://codereview.chromium.org/1547593002/diff/60001/net/base/mime_util_unit...
net/base/mime_util_unittest.cc:282:
EXPECT_TRUE(boundary2.find("MultipartBoundary") != std::string::npos);
EXPECT_NE ?

[Łukasz Anforowicz, 2015-12-29 18:53:19]

Ryan, could you please take another look?

On 2015/12/28 23:30:01, Ryan Sleevi wrote:
> Overall LG, although I haven't checked to see if there are any other places
that
> could benefit from this (ideally, we'd get them all in one CL)

When trying to find places that could reuse net::GenerateMimeMultipartBoundary I
searched for:
- Callers of AddMultipartValueForUpload
- Case-insensitive "boundary" in .cc files


There are some places that could in theory reuse
net::GenerateMimeMultipartBoundary, but that I didn't modify:

- chrome/browser/media/webrtc_log_uploader.cc: global kMultipartBoundary
constant is used across multiple functions and I didn't want to complicate the
code with managing the lifetime of the boundary string

- chrome/browser/tracing/crash_service_uploader.cc: global kMultipartBoundary
constant (same value as above :-) seems desirable for crash-handling code - I
assume that crash handling code wants to minimize computations done in a
corrupted/crashing state and using a constant seems therefore appropriate

- breakpad/src/common/windows/http_upload.cc - not sure if depending on //net
and //base is ok here (//base might be needed for utf8->wstring translation
although I guess this translation can also be done using only std library).


I think I agree with transitioning all the call sites in 1 CL (and only breaking
up the CL into smaller ones if something blows up, which I think is relatively
unlikely).

> 
> The only question I have is with this new method, and the entropy provided,
> whether or not it's statistically likely that we would reasonably have a
> conflict, and whether it's better (in the name of efficiency) to not check the
> data. I'm torn, but it seems that given that .find() is linear time, there
might
> be small gains skipping that check?

Good point - that check is not really needed and removing it also has
readability / maintainability benefits.

https://codereview.chromium.org/1547593002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/upload_job_impl.cc (right):

https://codereview.chromium.org/1547593002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/upload_job_impl.cc:224: } while (!found && retry
<= kMimeBoundaryRetries);
On 2015/12/28 23:30:00, Ryan Sleevi wrote:
> This seems overly conservative/unnecessary with your new change

Done.

I think you have a good point wrt removing the checks of whether the mime
boundary appears in the parts themselves.

1. The checks do seem unnecessary:

   1.a. The checks are only done by a subset of callsites
   1.b. The new code has plenty of entropy:
        - new code = 8 bits * 30 = 240 bits of entropy
        - old upload_job_impl.cc = 6 bits * (32-8-8) = 96 bits of entropy
        - old base_requests.cc = 6 bits * 70 = 420 bits of entropy
        - random guid (Variant 2, Version 4 RFC 4122 GUID) = 122 bits of
entropy.  It would take about 3.27e17 random GUIDs for a 1% chance of collision.
That’s more than 326 million billion. That’s a lot. [1]

2. Removing the checks is beneficial:

   2.a. Removal will simplify the code (which is good for readability +
maintainability and good for binary size).

   2.b. Avoiding the runtime overhead of the checks can potentially yield small
improvement in performance.


[1] http://blog.stephencleary.com/2010/11/few-words-on-guids.html

https://codereview.chromium.org/1547593002/diff/60001/google_apis/drive/base_...
File google_apis/drive/base_requests.cc (right):

https://codereview.chromium.org/1547593002/diff/60001/google_apis/drive/base_...
google_apis/drive/base_requests.cc:219: boundary =
net::GenerateMimeMultipartBoundary();
On 2015/12/28 23:30:00, Ryan Sleevi wrote:
> Ditto as to whether needed?

Done.

https://codereview.chromium.org/1547593002/diff/60001/net/base/mime_util.cc
File net/base/mime_util.cc (right):

https://codereview.chromium.org/1547593002/diff/60001/net/base/mime_util.cc#n...
net/base/mime_util.cc:597: result += "----";
On 2015/12/28 23:30:00, Ryan Sleevi wrote:
> pedantry:
> result.append(random_base64);
> result.append("----");
> 
> No need to make the compiler try and figure out that the return value is
unused
> and can thus be avoided :)

Done.

1. I think "append" is slightly better for readability + I don't trust my C++
style taste (long time C# developer here).

2. OTOH, I see that "append" also returns a std::string& and in case of
chromium2/src/build/linux/debian_wheezy_amd64-sysroot/usr/include/c++/4.6/bits/basic_string.h
the += operator is inlined directly into append case:

      basic_string&
      operator+=(const basic_string& __str)
      { return this->append(__str); }
      ...
      basic_string&
      operator+=(const _CharT* __s)
      { return this->append(__s); }

https://codereview.chromium.org/1547593002/diff/60001/net/base/mime_util_unit...
File net/base/mime_util_unittest.cc (right):

https://codereview.chromium.org/1547593002/diff/60001/net/base/mime_util_unit...
net/base/mime_util_unittest.cc:282:
EXPECT_TRUE(boundary2.find("MultipartBoundary") != std::string::npos);
On 2015/12/28 23:30:00, Ryan Sleevi wrote:
> EXPECT_NE ?

Done + also tweaked the other asserts, added a few asserts and added some
comments.

[Ryan Sleevi, 2015-12-29 21:18:03]

LGTM. I was going to suggest some microoptimizations to preallocate the string
but... meh, that's just me being me (and I'd probably screw it up again - see
below)

https://codereview.chromium.org/1547593002/diff/60001/net/base/mime_util.cc
File net/base/mime_util.cc (right):

https://codereview.chromium.org/1547593002/diff/60001/net/base/mime_util.cc#n...
net/base/mime_util.cc:597: result += "----";
On 2015/12/29 18:53:19, Łukasz Anforowicz wrote:
> 1. I think "append" is slightly better for readability + I don't trust my C++
> style taste (long time C# developer here).

StringBuilder ALL the things ;)

> 2. OTOH, I see that "append" also returns a std::string& and in case of
>
chromium2/src/build/linux/debian_wheezy_amd64-sysroot/usr/include/c++/4.6/bits/basic_string.h

Yeah, the C++ spec actually guarantees that += must be equivalent in complexity
to .append(); I should have paid attention that .append() also returns the
string&, so the net savings is... non-existent :)

So, in the future, pretend I didn't give you this advice and do what feels right
:)

[In an ideal world, we'd have StrCat in //base which offers even better
performance than multiple += or base::StringPrintF, through template
meta-programming, but that's one of those extreme optimizations; useful for
Google code]

[Łukasz Anforowicz, 2015-12-29 22:19:01]

lukasza@chromium.org changed reviewers:
+ vitalybuka@chromium.org

[Łukasz Anforowicz, 2015-12-29 22:19:02]

Vitaly, could you please take a look at:
chrome/common/cloud_print/...
chrome/service/cloud_print/...
cloud_print/...
?

For entropy considerations please see:
https://codereview.chromium.org/1547593002/diff/60001/chrome/browser/chromeos...

https://codereview.chromium.org/1547593002/diff/60001/net/base/mime_util.cc
File net/base/mime_util.cc (right):

https://codereview.chromium.org/1547593002/diff/60001/net/base/mime_util.cc#n...
net/base/mime_util.cc:597: result += "----";
On 2015/12/29 21:18:03, Ryan Sleevi wrote:
> On 2015/12/29 18:53:19, Łukasz Anforowicz wrote:
> > 1. I think "append" is slightly better for readability + I don't trust my
C++
> > style taste (long time C# developer here).
> 
> StringBuilder ALL the things ;)
> 
> > 2. OTOH, I see that "append" also returns a std::string& and in case of
> >
>
chromium2/src/build/linux/debian_wheezy_amd64-sysroot/usr/include/c++/4.6/bits/basic_string.h
> 
> Yeah, the C++ spec actually guarantees that += must be equivalent in
complexity
> to .append(); I should have paid attention that .append() also returns the
> string&, so the net savings is... non-existent :)
> 
> So, in the future, pretend I didn't give you this advice and do what feels
right
> :)
> 
> [In an ideal world, we'd have StrCat in //base which offers even better
> performance than multiple += or base::StringPrintF, through template
> meta-programming, but that's one of those extreme optimizations; useful for
> Google code]

Acknowledged.

[Vitaly Buka (NO REVIEWS), 2016-01-04 21:12:37]

lgtm

[Łukasz Anforowicz, 2016-01-04 21:45:26]

lukasza@chromium.org changed reviewers:
+ hashimoto@chromium.org, stevet@chromium.org

[Łukasz Anforowicz, 2016-01-04 21:45:27]

bartfab@, hashimoto@, rdsmith@, stevet@ - could you please do an OWNERS review?

bartfab@:
    chrome/browser/chromeos/policy/upload_job_impl.cc
    chrome/browser/chromeos/policy/upload_job_impl.h
    chrome/browser/chromeos/policy/upload_job_unittest.cc

stevet@:
    components/search_engines/template_url.cc

rdsmith@:
    content/browser/download/mhtml_generation_manager.cc
    content/browser/download/mhtml_generation_manager.h

hashimoto@:
    google_apis/drive/base_requests.cc
    google_apis/drive/drive_api_requests_unittest.cc

If you are wondering about entropy changes or about the removal of the "retry"
code, please go here: 
https://codereview.chromium.org/1547593002/diff/60001/chrome/browser/chromeos...

[Łukasz Anforowicz, 2016-01-04 21:46:51]

lukasza@chromium.org changed reviewers:
+ bartfab@chromium.org, rdsmith@chromium.org

[Łukasz Anforowicz, 2016-01-04 21:46:52]

Adding bartfab@, hashimoto@, rdsmith@, stevet@ for real this time...

[hashimoto, 2016-01-05 07:16:09]

https://codereview.chromium.org/1547593002/diff/100001/google_apis/drive/base...
File google_apis/drive/base_requests.cc (left):

https://codereview.chromium.org/1547593002/diff/100001/google_apis/drive/base...
google_apis/drive/base_requests.cc:237: bool conflict_with_content = false;
The new implementation generates much less variety of boundary strings (62^70 is
2e77 times larger than 256^20).
Is it OK to remove this check?

BTW, why do we need to mimic IE/UnMHT when it results in only 256^20 possible
output strings?

[bartfab (slow), 2016-01-05 14:20:46]

https://codereview.chromium.org/1547593002/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/upload_job_impl.cc (left):

https://codereview.chromium.org/1547593002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/upload_job_impl.cc:259:
delegate_->OnFailure(CONTENT_ENCODING_ERROR);
Nit: Remove CONTENT_ENCODING_ERROR from upload_job.h. It is no longer used.

https://codereview.chromium.org/1547593002/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/upload_job_impl.cc (right):

https://codereview.chromium.org/1547593002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/upload_job_impl.cc:12: #include
"base/rand_util.h"
Nit: No longer used.

https://codereview.chromium.org/1547593002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/upload_job_impl.cc:35: const size_t
kMaxMimeBoundarySize = 70;
Nit: #include <stddef.h>

https://codereview.chromium.org/1547593002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/upload_job_impl.cc:78: bool
CheckIfDataContains(const std::string& chunk);
Nit: This method is no longer used.

https://codereview.chromium.org/1547593002/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/upload_job_impl.h (right):

https://codereview.chromium.org/1547593002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/upload_job_impl.h:8: #include <stddef.h>
Nit: No longer used.

[Randy Smith (Not in Mondays), 2016-01-05 19:14:01]

content/browser/download LGTM.

[Łukasz Anforowicz, 2016-01-05 19:16:49]

hashimoto@ + bartfab@ - thanks for reviewing and providing feedback - I tried
addressing it in the latest patchset.  Could you please take another look?

https://codereview.chromium.org/1547593002/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/upload_job_impl.cc (left):

https://codereview.chromium.org/1547593002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/upload_job_impl.cc:259:
delegate_->OnFailure(CONTENT_ENCODING_ERROR);
On 2016/01/05 14:20:45, bartfab (slow) wrote:
> Nit: Remove CONTENT_ENCODING_ERROR from upload_job.h. It is no longer used.

Done.

https://codereview.chromium.org/1547593002/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/upload_job_impl.cc (right):

https://codereview.chromium.org/1547593002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/upload_job_impl.cc:12: #include
"base/rand_util.h"
On 2016/01/05 14:20:45, bartfab (slow) wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/1547593002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/upload_job_impl.cc:35: const size_t
kMaxMimeBoundarySize = 70;
On 2016/01/05 14:20:45, bartfab (slow) wrote:
> Nit: #include <stddef.h>

Done.

https://codereview.chromium.org/1547593002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/upload_job_impl.cc:78: bool
CheckIfDataContains(const std::string& chunk);
On 2016/01/05 14:20:45, bartfab (slow) wrote:
> Nit: This method is no longer used.

Done.  Thanks for catching this and other places where I've left code that could
be removed further.  I'll wear a cone of shame for a day... :-/

https://codereview.chromium.org/1547593002/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/upload_job_impl.h (right):

https://codereview.chromium.org/1547593002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/upload_job_impl.h:8: #include <stddef.h>
On 2016/01/05 14:20:45, bartfab (slow) wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/1547593002/diff/100001/google_apis/drive/base...
File google_apis/drive/base_requests.cc (left):

https://codereview.chromium.org/1547593002/diff/100001/google_apis/drive/base...
google_apis/drive/base_requests.cc:237: bool conflict_with_content = false;
On 2016/01/05 07:16:09, hashimoto wrote:
> The new implementation generates much less variety of boundary strings (62^70
is
> 2e77 times larger than 256^20).

The new implementation is generating 256^30 strings (30 random bytes in
random_values array which is equivalent to 40 base64 characters so 64^40).

I am open to increasing the entropy to 256^45 (by removing "MultipartBoundary--"
substring [to make room for additional 20 base64 characters] and therefore
slightly hurting readability of the generated mime documents), but I think that
shouldn't be necessary - I think that reintroducing the retries below should be
sufficient.

> Is it OK to remove this check?
> 
The probability of collisions is quite low (see the comment in [1] for more
details), but I can see that argument that 1) GDrive is potentially dealing with
encrypted files (i.e. effectively random content) and 2) GDrive is dealing with
long files (i.e. lots of potential collisions) and 3) this check-and-retry is
infinite (so it really is significantly changing the behavior, unlike the
at-most-3-retries-code in the other place).

FWIW, I've put the check-and-retry code back.

[1]
https://codereview.chromium.org/1547593002/diff/60001/chrome/browser/chromeos...

> BTW, why do we need to mimic IE/UnMHT when it results in only 256^20 possible
> output strings?

The boundary marker is not very similar to IE/UnMHT anymore.  OTOH, many
implementations used "-" characters as a prefix and suffix, so I want to retain
this (in case some third party consumers depend on this).  Additionally the
dashes and the "MultipartBoundary" substring help make the marker human-readable
(i.e. make reading the generated MHTML files easier for humans / developers).

[hashimoto, 2016-01-07 05:15:48]

https://codereview.chromium.org/1547593002/diff/100001/google_apis/drive/base...
File google_apis/drive/base_requests.cc (left):

https://codereview.chromium.org/1547593002/diff/100001/google_apis/drive/base...
google_apis/drive/base_requests.cc:237: bool conflict_with_content = false;
On 2016/01/05 19:16:49, Łukasz Anforowicz wrote:
> On 2016/01/05 07:16:09, hashimoto wrote:
> > The new implementation generates much less variety of boundary strings
(62^70
> is
> > 2e77 times larger than 256^20).
> 
> The new implementation is generating 256^30 strings (30 random bytes in
> random_values array which is equivalent to 40 base64 characters so 64^40).
> 
> I am open to increasing the entropy to 256^45 (by removing
"MultipartBoundary--"
> substring [to make room for additional 20 base64 characters] and therefore
> slightly hurting readability of the generated mime documents), but I think
that
> shouldn't be necessary - I think that reintroducing the retries below should
be
> sufficient.
> 
> > Is it OK to remove this check?
> > 
> The probability of collisions is quite low (see the comment in [1] for more
> details), but I can see that argument that 1) GDrive is potentially dealing
with
> encrypted files (i.e. effectively random content) and 2) GDrive is dealing
with
> long files (i.e. lots of potential collisions) and 3) this check-and-retry is
> infinite (so it really is significantly changing the behavior, unlike the
> at-most-3-retries-code in the other place).
> 
> FWIW, I've put the check-and-retry code back.
lgtm
> 
> [1]
>
https://codereview.chromium.org/1547593002/diff/60001/chrome/browser/chromeos...
> 
> > BTW, why do we need to mimic IE/UnMHT when it results in only 256^20
possible
> > output strings?
> 
> The boundary marker is not very similar to IE/UnMHT anymore.  OTOH, many
> implementations used "-" characters as a prefix and suffix, so I want to
retain
> this (in case some third party consumers depend on this).  Additionally the
> dashes and the "MultipartBoundary" substring help make the marker
human-readable
> (i.e. make reading the generated MHTML files easier for humans / developers).
Speaking of compatibility, Blink's implementation doesn't add "----" suffix, and
the comment there says "However the following characters, though legal, cause
some sites to fail: (),./:=+".
(https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So...)
Probably better to avoid using /=+ for boundary strings?

[Łukasz Anforowicz, 2016-01-07 19:04:12]

lukasza@chromium.org changed reviewers:
+ haraken@chromium.org

[Łukasz Anforowicz, 2016-01-07 19:04:13]

bartfab@, stevet@, haraken@ - could you please do an OWNERS review?  hashimoto@
- thanks for reviewing.

bartfab@:
    chrome/browser/chromeos/policy/upload_job_impl.cc
    chrome/browser/chromeos/policy/upload_job_impl.h
    chrome/browser/chromeos/policy/upload_job_unittest.cc

stevet@:
    components/search_engines/template_url.cc

haraken@ (just need to rubberstamp a comment):
    third_party/WebKit/Source/platform/network/FormDataEncoder.cpp

https://codereview.chromium.org/1547593002/diff/100001/google_apis/drive/base...
File google_apis/drive/base_requests.cc (left):

https://codereview.chromium.org/1547593002/diff/100001/google_apis/drive/base...
google_apis/drive/base_requests.cc:237: bool conflict_with_content = false;
On 2016/01/07 05:15:48, hashimoto wrote:
> [...snip...]
> Speaking of compatibility, Blink's implementation doesn't add "----" suffix,
and
> the comment there says "However the following characters, though legal, cause
> some sites to fail: (),./:=+".
>
(https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So...)
> Probably better to avoid using /=+ for boundary strings?

Thank you very much for pointing this out.  This does seem important and I
wasn't aware of this.  I guess this means I should back out of using
base::RandBytes + base::Base64Encode (rather than patch it up with
std::string::replace or something like this) and instead reuse code similar to
what was used in google_apis/drive/base_requests.cc (and in a few other places).

Done.

BTW: By removing the weird base64 size dependencies, this change also allows to
increase the entropy by increasing the characters count from 67 to 69.

[haraken, 2016-01-07 23:37:19]

WebKit LGTM

[Łukasz Anforowicz, 2016-01-08 17:53:37]

Description was changed from

==========
Introducing a net::GenerateMimeMultipartBoundary helper.

... and using the helper in places that had introduced separate
functions for generating a random Mime boundary (sometimes with
bugs - i.e. using '*' characters is disallowed by RFC 1341).

BUG=
==========

to

==========
Introducing a net::GenerateMimeMultipartBoundary helper.

... and using the helper in places that had introduced separate
functions for generating a random Mime boundary (sometimes with
bugs - i.e. using '*' characters is disallowed by RFC 1341).

TBR=bartfab@chromium.org, stevet@chromium.org
BUG=575733
==========

[Łukasz Anforowicz, 2016-01-08 17:56:30]

I plan to TBR the rest of the files - I think I've addressed bartfab@'s feedback
(mostly nits) in patchset #7 on Tuesday and I think that changes in
components/search_engines/template_url.cc are sufficiently similar to other
changes in this CL to not require an explicit OWNERS sign-off from stevet@.

[Łukasz Anforowicz, 2016-01-08 17:58:22]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2016-01-08 17:58:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
rsleevi@chromium.org, vitalybuka@chromium.org, rdsmith@chromium.org,
hashimoto@chromium.org, haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1547593002/#ps180001
(title: "Rebasing...")

[commit-bot: I haz the power, 2016-01-08 17:59:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1547593002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1547593002/180001

[commit-bot: I haz the power, 2016-01-08 19:35:12]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-01-08 19:36:09]

Description was changed from

==========
Introducing a net::GenerateMimeMultipartBoundary helper.

... and using the helper in places that had introduced separate
functions for generating a random Mime boundary (sometimes with
bugs - i.e. using '*' characters is disallowed by RFC 1341).

TBR=bartfab@chromium.org, stevet@chromium.org
BUG=575733
==========

to

==========
Introducing a net::GenerateMimeMultipartBoundary helper.

... and using the helper in places that had introduced separate
functions for generating a random Mime boundary (sometimes with
bugs - i.e. using '*' characters is disallowed by RFC 1341).

TBR=bartfab@chromium.org, stevet@chromium.org
BUG=575733

Committed: https://crrev.com/c644f37125ab27311d697af61e56c64d0e29027e
Cr-Commit-Position: refs/heads/master@{#368404}
==========

[commit-bot: I haz the power, 2016-01-08 19:36:10]

Patchset 10 (id:??) landed as
https://crrev.com/c644f37125ab27311d697af61e56c64d0e29027e
Cr-Commit-Position: refs/heads/master@{#368404}

[Ryan Sleevi, 2016-01-08 19:55:04]

https://codereview.chromium.org/1547593002/diff/180001/net/base/mime_util.cc
File net/base/mime_util.cc (right):

https://codereview.chromium.org/1547593002/diff/180001/net/base/mime_util.cc#...
net/base/mime_util.cc:520: // to fail: (),./:=+
Sorry, I've been at a conference this week and haven't been able to stay on top
of code reviews.

I appreciate that there's concern, but I generally dislike magic comments and
behaviour like this, because it doesn't have a bug, data, or measurements, so it
becomes secret incantation without quantifiable metrics.

Filed https://code.google.com/p/chromium/issues/detail?id=575779 to track this;
would you mind in a follow-up CL adding a pointer to that bug from this code, so
that we can make sure the reasoning/logic is tracked

https://codereview.chromium.org/1547593002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/network/FormDataEncoder.cpp (right):

https://codereview.chromium.org/1547593002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/network/FormDataEncoder.cpp:102: //
TODO(lukasza): Reuse net::GenerateMimeMultipartBoundary instead.
I would have objected to this TODO - The Blink code shouldn't be pointing at
//net code.

[Łukasz Anforowicz, 2016-01-08 21:32:06]

On 2016/01/08 19:55:04, Ryan Sleevi wrote:
> https://codereview.chromium.org/1547593002/diff/180001/net/base/mime_util.cc
> File net/base/mime_util.cc (right):
> 
>
https://codereview.chromium.org/1547593002/diff/180001/net/base/mime_util.cc#...
> net/base/mime_util.cc:520: // to fail: (),./:=+
> Sorry, I've been at a conference this week and haven't been able to stay on
top
> of code reviews.
> 
> I appreciate that there's concern, but I generally dislike magic comments and
> behaviour like this, because it doesn't have a bug, data, or measurements, so
it
> becomes secret incantation without quantifiable metrics.
> 
> Filed https://code.google.com/p/chromium/issues/detail?id=575779 to track
this;
> would you mind in a follow-up CL adding a pointer to that bug from this code,
so
> that we can make sure the reasoning/logic is tracked
> 
>
https://codereview.chromium.org/1547593002/diff/180001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/platform/network/FormDataEncoder.cpp (right):
> 
>
https://codereview.chromium.org/1547593002/diff/180001/third_party/WebKit/Sou...
> third_party/WebKit/Source/platform/network/FormDataEncoder.cpp:102: //
> TODO(lukasza): Reuse net::GenerateMimeMultipartBoundary instead.
> I would have objected to this TODO - The Blink code shouldn't be pointing at
> //net code.

Following-up in https://codereview.chromium.org/1573623002/