Move AddMultipartValueForUpload to net/base/mime_util.h/cc.

Move AddMultipartValueForUpload to net/base/mime_util.h/cc.

Moved from cloud print since it is used from several places. This was triggered by https://chromiumcodereview.appspot.com/14329020/ were there also is a discussion about this move.

BUG=229829
R=scottbyer@chromium.org, sky@chromium.org, wtc@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=200521

[Henrik Grunell, 2013-05-14 12:52:24]

Moving a function in cloud print to mime_util.h/cc. It's already used in two
places and a third will be added soon, see linked CL. This move has been
discussed between joi@ and wtc@ in that CL.

sky@: chrome/browser/*
scottbyer@: chrome/common/* and chrome/service/*
wtc@: net/*

[Scott Byer, 2013-05-14 15:42:58]

lgtm

[sky, 2013-05-14 17:27:14]

chrome/browser LGTM

[wtc, 2013-05-14 18:45:29]

Patch set 2 LGTM.

1. We need to add a test to net_unittests, otherwise someone
who only works on src/net cannot easily detect a regression.

2. I suggest a way to avoid using base::StringPrintf().

https://codereview.chromium.org/15076008/diff/4001/chrome/browser/chrome_to_m...
File chrome/browser/chrome_to_mobile_service.cc (right):

https://codereview.chromium.org/15076008/diff/4001/chrome/browser/chrome_to_m...
chrome/browser/chrome_to_mobile_service.cc:590: post.append("--" + bound +
"--\r\n");

Optional: For completeness, should we also turn this into a
function in net/mime_util.h?

You can find other instances of this line if you search for
"Terminate the request body" in the other files in this CL.

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

https://codereview.chromium.org/15076008/diff/4001/net/base/mime_util.cc#newc...
net/base/mime_util.cc:17: #include "base/stringprintf.h"

I believe this can be avoided. See the comment below.

https://codereview.chromium.org/15076008/diff/4001/net/base/mime_util.cc#newc...
net/base/mime_util.cc:997: // First line is the boundary

Nit: add periods (.) to the end of sentences in comments.

https://codereview.chromium.org/15076008/diff/4001/net/base/mime_util.cc#newc...
net/base/mime_util.cc:1001: "name=\"%s\"\r\n", value_name.c_str()));

The only format specifier this function uses is %s, so it
doesn't seem necessary to use base::StringPrintf().

For example, this post_data->append() call can be rewritten as:
  post_data->append("Content-Disposition: form-data; name=\"" +
                    value_name + "\"\r\n");

or

  post_data->append("Content-Disposition: form-data; name=\"");
  post_data->append(value_name);
  post_data->append("\"\r\n");

[Henrik Grunell, 2013-05-15 11:49:30]

Code review fixes. Everyone please have another look (trivial changes).

https://chromiumcodereview.appspot.com/15076008/diff/4001/chrome/browser/chro...
File chrome/browser/chrome_to_mobile_service.cc (right):

https://chromiumcodereview.appspot.com/15076008/diff/4001/chrome/browser/chro...
chrome/browser/chrome_to_mobile_service.cc:590: post.append("--" + bound +
"--\r\n");
On 2013/05/14 18:45:29, wtc wrote:
> 
> Optional: For completeness, should we also turn this into a
> function in net/mime_util.h?
> 
> You can find other instances of this line if you search for
> "Terminate the request body" in the other files in this CL.

SGTM. Done.

https://chromiumcodereview.appspot.com/15076008/diff/4001/net/base/mime_util.cc
File net/base/mime_util.cc (right):

https://chromiumcodereview.appspot.com/15076008/diff/4001/net/base/mime_util....
net/base/mime_util.cc:17: #include "base/stringprintf.h"
On 2013/05/14 18:45:29, wtc wrote:
> 
> I believe this can be avoided. See the comment below.

Done.

https://chromiumcodereview.appspot.com/15076008/diff/4001/net/base/mime_util....
net/base/mime_util.cc:997: // First line is the boundary
On 2013/05/14 18:45:29, wtc wrote:
> 
> Nit: add periods (.) to the end of sentences in comments.

Done.

https://chromiumcodereview.appspot.com/15076008/diff/4001/net/base/mime_util....
net/base/mime_util.cc:1001: "name=\"%s\"\r\n", value_name.c_str()));
On 2013/05/14 18:45:29, wtc wrote:
> 
> The only format specifier this function uses is %s, so it
> doesn't seem necessary to use base::StringPrintf().
> 
> For example, this post_data->append() call can be rewritten as:
>   post_data->append("Content-Disposition: form-data; name=\"" +
>                     value_name + "\"\r\n");
> 
> or
> 
>   post_data->append("Content-Disposition: form-data; name=\"");
>   post_data->append(value_name);
>   post_data->append("\"\r\n");

Just copied the function over. Agree that's better. Done.

[Scott Byer, 2013-05-15 15:55:35]

My parts still LGTM

[wtc, 2013-05-15 17:01:35]

Patch set 3 LGTM. I suggest some changes.

1. Please add a unit test for the two new functions.
You can just add a "known answer test" that compares the
results with expected answers.

2. Minor coding style fixes for AddMultipartTerminateForUpload.

https://codereview.chromium.org/15076008/diff/16001/net/base/mime_util.h
File net/base/mime_util.h (right):

https://codereview.chromium.org/15076008/diff/16001/net/base/mime_util.h#newc...
net/base/mime_util.h:129: NET_EXPORT void AddMultipartTerminateForUpload(const
std::string& mime_boundary,

Please add a comment to describe what this function does.

Nit: "Terminate" is a verb. I suggest changing it to "Terminator"
or "FinalDelimiter". (That thing is called the "final boundary delimiter",
but that is a mouthful.)

[Henrik Grunell, 2013-05-16 08:35:45]

Code review fixes: added unit test, changed a function name and added a comment.

https://chromiumcodereview.appspot.com/15076008/diff/16001/net/base/mime_util.h
File net/base/mime_util.h (right):

https://chromiumcodereview.appspot.com/15076008/diff/16001/net/base/mime_util...
net/base/mime_util.h:129: NET_EXPORT void AddMultipartTerminateForUpload(const
std::string& mime_boundary,
On 2013/05/15 17:01:35, wtc wrote:
> 
> Please add a comment to describe what this function does.
> 
> Nit: "Terminate" is a verb. I suggest changing it to "Terminator"
> or "FinalDelimiter". (That thing is called the "final boundary delimiter",
> but that is a mouthful.)

Done.

[Henrik Grunell, 2013-05-16 08:59:59]

On 2013/05/16 08:35:45, Henrik Grunell wrote:
> Code review fixes: added unit test, changed a function name and added a
comment.

Created a new CL for the test (https://codereview.chromium.org/14934008/) and
removed it from here.

[commit-bot: I haz the power, 2013-05-16 09:00:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/grunell@chromium.org/15076008/37001

[commit-bot: I haz the power, 2013-05-16 09:52:54]

Retried try job too often on linux_chromeos for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[Henrik Grunell, 2013-05-16 12:59:26]

Committed patchset #7 manually as r200521 (presubmit successful).