moved upload progress logic from ResourceLoader to AsyncResourceHandler

moved upload progress logic from ResourceLoader to AsyncResourceHandler

ResourceDispatcherHostImpl no longer listens for UploadProgress ACKS from the renderer, instead AsyncResourceHandler takes care of those and the timing details about when to send progress reports

This means that all the ResourceLoaders that previously had stubs for upload progress can lose that method

BUG=498474
Committed: https://crrev.com/6684a66ad39c906f83f05a11c6e9ca7f9afac371
Cr-Commit-Position: refs/heads/master@{#346177}

[Charlie Harrison, 2015-08-21 19:12:49]

csharrison@chromium.org changed reviewers:
+ asanka@chromium.org, mmenke@chromium.org

[Charlie Harrison, 2015-08-24 20:04:53]

I simplified the tests, because things were getting pretty hairy trying to mock
an upload while still maintaining the http interface + redirects. I ended up
just making sure that the renderer receives at least two messages, on upload
start and on upload complete. This gives us a valid monotonically increasing
test that won't be zero-information.

[mmenke, 2015-08-26 19:13:13]

Looks good.  I'll probably file a bug about beefing up the tests, but think this
is worth landing independently of that.

https://codereview.chromium.org/1301103002/diff/120001/content/browser/loader...
File content/browser/loader/async_resource_handler.cc (right):

https://codereview.chromium.org/1301103002/diff/120001/content/browser/loader...
content/browser/loader/async_resource_handler.cc:131: void
AsyncResourceHandler::OnUploadProgressACK(int request_id) {
Definition order should match declaration order in the header.

https://codereview.chromium.org/1301103002/diff/120001/content/browser/loader...
content/browser/loader/async_resource_handler.cc:276: 
nit:  Remove blank line.

https://codereview.chromium.org/1301103002/diff/120001/content/browser/loader...
File content/browser/loader/async_resource_handler.h (right):

https://codereview.chromium.org/1301103002/diff/120001/content/browser/loader...
content/browser/loader/async_resource_handler.h:38: void ReportUploadProgress();
This should be private.

https://codereview.chromium.org/1301103002/diff/120001/content/browser/loader...
content/browser/loader/async_resource_handler.h:41: bool OnUploadProgress(uint64
position, uint64 size);
This is no longer part of the ResourceHandler, and should be private.  Or
better, let's just inline it in ReportUploadProgress.

https://codereview.chromium.org/1301103002/diff/120001/content/browser/loader...
File content/browser/loader/async_resource_handler_browsertest.cc (right):

https://codereview.chromium.org/1301103002/diff/120001/content/browser/loader...
content/browser/loader/async_resource_handler_browsertest.cc:1: // Copyright (c)
2015 The Chromium Authors. All rights reserved.
nit:  Don't use (c) in new files.

https://codereview.chromium.org/1301103002/diff/120001/content/browser/loader...
content/browser/loader/async_resource_handler_browsertest.cc:6: #include
"content/browser/loader/async_resource_handler.h"
This should go first, 1 line above the others.

https://codereview.chromium.org/1301103002/diff/120001/content/browser/loader...
content/browser/loader/async_resource_handler_browsertest.cc:19: const char
kRedirect[] = "/redirect";
These should go in the anonymous namespace

https://codereview.chromium.org/1301103002/diff/120001/content/browser/loader...
content/browser/loader/async_resource_handler_browsertest.cc:22: };
This should go below the anonymous namespace.

https://codereview.chromium.org/1301103002/diff/120001/content/browser/loader...
content/browser/loader/async_resource_handler_browsertest.cc:34: } // namespace
nit:  Should be:

namespace {

....

}  // namespace

https://codereview.chromium.org/1301103002/diff/120001/content/browser/loader...
content/browser/loader/async_resource_handler_browsertest.cc:40:
base::Bind(&SimplePost, kPost));
nit:  +2 indent

https://codereview.chromium.org/1301103002/diff/120001/content/browser/loader...
content/browser/loader/async_resource_handler_browsertest.cc:50: 
Remove blank line.

https://codereview.chromium.org/1301103002/diff/120001/content/browser/loader...
content/browser/loader/async_resource_handler_browsertest.cc:59: new
net::test_server::BasicHttpResponse());
nit:  +2 indent

https://codereview.chromium.org/1301103002/diff/120001/content/browser/loader...
content/browser/loader/async_resource_handler_browsertest.cc:75: }
Suggest putting this all in the anonymous namespace up top - that's a more
common pattern.

https://codereview.chromium.org/1301103002/diff/120001/content/browser/loader...
content/browser/loader/async_resource_handler_browsertest.cc:83:
base::Bind(&SimplePostRedirect, kRedirect));
nit:  +2 indent

https://codereview.chromium.org/1301103002/diff/120001/content/browser/loader...
content/browser/loader/async_resource_handler_browsertest.cc:93: 
Remove blank line

https://codereview.chromium.org/1301103002/diff/120001/content/test/data/asyn...
File content/test/data/async_resource_handler.html (right):

https://codereview.chromium.org/1301103002/diff/120001/content/test/data/asyn...
content/test/data/async_resource_handler.html:1: <html>
Suggest putting this in a new directory content/test/data/loader/

https://codereview.chromium.org/1301103002/diff/120001/content/test/data/asyn...
content/test/data/async_resource_handler.html:7: var completed_upload = false;
Javascript value should use javascriptNamingStyle.  I have no idea why it's
different than C++ naming style.  :(

https://codereview.chromium.org/1301103002/diff/120001/content/test/data/asyn...
content/test/data/async_resource_handler.html:9: var uReq;
Suspect you copied this from another test, but please use more meaningful
variable names.

https://codereview.chromium.org/1301103002/diff/120001/content/test/data/asyn...
content/test/data/async_resource_handler.html:11: // build a long string, fast
nit:  "Build a long string, fast."

https://codereview.chromium.org/1301103002/diff/120001/content/test/data/asyn...
content/test/data/async_resource_handler.html:17: data += data + data; // length
*= 3
Suggest just data += data, for simplicity.  Or data = data + data + data for
clarity (As-is, it's easy to miss it's +=), but I don't think the *3 really
gives us much.

https://codereview.chromium.org/1301103002/diff/120001/content/test/data/asyn...
content/test/data/async_resource_handler.html:21: function progressListener(e) {
If this is called after completedUpload, it should be a failure, right?

https://codereview.chromium.org/1301103002/diff/120001/content/test/data/asyn...
content/test/data/async_resource_handler.html:22: if (e.lengthComputable) {
Can this ever be false?

https://codereview.chromium.org/1301103002/diff/120001/content/test/data/asyn...
content/test/data/async_resource_handler.html:24: var percent = e.loaded /
e.total;
Can check e.total here, too.

https://codereview.chromium.org/1301103002/diff/120001/content/test/data/asyn...
content/test/data/async_resource_handler.html:26: backwards_progress = true;
Should we ever get the same value multiple times?

https://codereview.chromium.org/1301103002/diff/120001/content/test/data/asyn...
content/test/data/async_resource_handler.html:26: backwards_progress = true;
percent should be <= 1, right?

https://codereview.chromium.org/1301103002/diff/120001/content/test/data/asyn...
content/test/data/async_resource_handler.html:27: progress = percent;
suggest last_seen_progress / current_progress - it's clearer than progress /
percent.

Or last_seen_percent / current_percent.  Just want names that more clearly
relate.

https://codereview.chromium.org/1301103002/diff/120001/content/test/data/asyn...
content/test/data/async_resource_handler.html:39: this.responseText == "hello";
Are we guaranteed to have progress == 1 at this point?

[Charlie Harrison, 2015-08-27 15:11:55]

On 2015/08/26 19:13:13, mmenke wrote:
> Looks good.  I'll probably file a bug about beefing up the tests, but think
this
> is worth landing independently of that.
> 
>
https://codereview.chromium.org/1301103002/diff/120001/content/browser/loader...
> File content/browser/loader/async_resource_handler.cc (right):
> 
>
https://codereview.chromium.org/1301103002/diff/120001/content/browser/loader...
> content/browser/loader/async_resource_handler.cc:131: void
> AsyncResourceHandler::OnUploadProgressACK(int request_id) {
> Definition order should match declaration order in the header.
> 
>
https://codereview.chromium.org/1301103002/diff/120001/content/browser/loader...
> content/browser/loader/async_resource_handler.cc:276: 
> nit:  Remove blank line.
> 
>
https://codereview.chromium.org/1301103002/diff/120001/content/browser/loader...
> File content/browser/loader/async_resource_handler.h (right):
> 
>
https://codereview.chromium.org/1301103002/diff/120001/content/browser/loader...
> content/browser/loader/async_resource_handler.h:38: void
ReportUploadProgress();
> This should be private.
> 
>
https://codereview.chromium.org/1301103002/diff/120001/content/browser/loader...
> content/browser/loader/async_resource_handler.h:41: bool
OnUploadProgress(uint64
> position, uint64 size);
> This is no longer part of the ResourceHandler, and should be private.  Or
> better, let's just inline it in ReportUploadProgress.
> 
>
https://codereview.chromium.org/1301103002/diff/120001/content/browser/loader...
> File content/browser/loader/async_resource_handler_browsertest.cc (right):
> 
>
https://codereview.chromium.org/1301103002/diff/120001/content/browser/loader...
> content/browser/loader/async_resource_handler_browsertest.cc:1: // Copyright
(c)
> 2015 The Chromium Authors. All rights reserved.
> nit:  Don't use (c) in new files.
> 
>
https://codereview.chromium.org/1301103002/diff/120001/content/browser/loader...
> content/browser/loader/async_resource_handler_browsertest.cc:6: #include
> "content/browser/loader/async_resource_handler.h"
> This should go first, 1 line above the others.
> 
>
https://codereview.chromium.org/1301103002/diff/120001/content/browser/loader...
> content/browser/loader/async_resource_handler_browsertest.cc:19: const char
> kRedirect[] = "/redirect";
> These should go in the anonymous namespace
> 
>
https://codereview.chromium.org/1301103002/diff/120001/content/browser/loader...
> content/browser/loader/async_resource_handler_browsertest.cc:22: };
> This should go below the anonymous namespace.
> 
>
https://codereview.chromium.org/1301103002/diff/120001/content/browser/loader...
> content/browser/loader/async_resource_handler_browsertest.cc:34: } //
namespace
> nit:  Should be:
> 
> namespace {
> 
> ....
> 
> }  // namespace
> 
>
https://codereview.chromium.org/1301103002/diff/120001/content/browser/loader...
> content/browser/loader/async_resource_handler_browsertest.cc:40:
> base::Bind(&SimplePost, kPost));
> nit:  +2 indent
> 
>
https://codereview.chromium.org/1301103002/diff/120001/content/browser/loader...
> content/browser/loader/async_resource_handler_browsertest.cc:50: 
> Remove blank line.
> 
>
https://codereview.chromium.org/1301103002/diff/120001/content/browser/loader...
> content/browser/loader/async_resource_handler_browsertest.cc:59: new
> net::test_server::BasicHttpResponse());
> nit:  +2 indent
> 
>
https://codereview.chromium.org/1301103002/diff/120001/content/browser/loader...
> content/browser/loader/async_resource_handler_browsertest.cc:75: }
> Suggest putting this all in the anonymous namespace up top - that's a more
> common pattern.
> 
>
https://codereview.chromium.org/1301103002/diff/120001/content/browser/loader...
> content/browser/loader/async_resource_handler_browsertest.cc:83:
> base::Bind(&SimplePostRedirect, kRedirect));
> nit:  +2 indent
> 
>
https://codereview.chromium.org/1301103002/diff/120001/content/browser/loader...
> content/browser/loader/async_resource_handler_browsertest.cc:93: 
> Remove blank line
> 
>
https://codereview.chromium.org/1301103002/diff/120001/content/test/data/asyn...
> File content/test/data/async_resource_handler.html (right):
> 
>
https://codereview.chromium.org/1301103002/diff/120001/content/test/data/asyn...
> content/test/data/async_resource_handler.html:1: <html>
> Suggest putting this in a new directory content/test/data/loader/
> 
>
https://codereview.chromium.org/1301103002/diff/120001/content/test/data/asyn...
> content/test/data/async_resource_handler.html:7: var completed_upload = false;
> Javascript value should use javascriptNamingStyle.  I have no idea why it's
> different than C++ naming style.  :(
> 
>
https://codereview.chromium.org/1301103002/diff/120001/content/test/data/asyn...
> content/test/data/async_resource_handler.html:9: var uReq;
> Suspect you copied this from another test, but please use more meaningful
> variable names.
> 
>
https://codereview.chromium.org/1301103002/diff/120001/content/test/data/asyn...
> content/test/data/async_resource_handler.html:11: // build a long string, fast
> nit:  "Build a long string, fast."
> 
>
https://codereview.chromium.org/1301103002/diff/120001/content/test/data/asyn...
> content/test/data/async_resource_handler.html:17: data += data + data; //
length
> *= 3
> Suggest just data += data, for simplicity.  Or data = data + data + data for
> clarity (As-is, it's easy to miss it's +=), but I don't think the *3 really
> gives us much.
> 
>
https://codereview.chromium.org/1301103002/diff/120001/content/test/data/asyn...
> content/test/data/async_resource_handler.html:21: function progressListener(e)
{
> If this is called after completedUpload, it should be a failure, right?
> 
>
https://codereview.chromium.org/1301103002/diff/120001/content/test/data/asyn...
> content/test/data/async_resource_handler.html:22: if (e.lengthComputable) {
> Can this ever be false?
> 
>
https://codereview.chromium.org/1301103002/diff/120001/content/test/data/asyn...
> content/test/data/async_resource_handler.html:24: var percent = e.loaded /
> e.total;
> Can check e.total here, too.
> 
>
https://codereview.chromium.org/1301103002/diff/120001/content/test/data/asyn...
> content/test/data/async_resource_handler.html:26: backwards_progress = true;
> Should we ever get the same value multiple times?
> 
>
https://codereview.chromium.org/1301103002/diff/120001/content/test/data/asyn...
> content/test/data/async_resource_handler.html:26: backwards_progress = true;
> percent should be <= 1, right?
> 
>
https://codereview.chromium.org/1301103002/diff/120001/content/test/data/asyn...
> content/test/data/async_resource_handler.html:27: progress = percent;
> suggest last_seen_progress / current_progress - it's clearer than progress /
> percent.
> 
> Or last_seen_percent / current_percent.  Just want names that more clearly
> relate.
> 
>
https://codereview.chromium.org/1301103002/diff/120001/content/test/data/asyn...
> content/test/data/async_resource_handler.html:39: this.responseText ==
"hello";
> Are we guaranteed to have progress == 1 at this point?

Thanks for the comments, I changed the upload progress conditions a little after
going over http://www.w3.org/TR/XMLHttpRequest/#make-progress-notifications. We
don't actually need to have completedUpload fire before onFinished.

[mmenke, 2015-08-27 17:21:29]

Looking really good - nice to get all this stuff in a single file.

https://codereview.chromium.org/1301103002/diff/160001/content/browser/loader...
File content/browser/loader/async_resource_handler.cc (right):

https://codereview.chromium.org/1301103002/diff/160001/content/browser/loader...
content/browser/loader/async_resource_handler.cc:160: if (filter) {
A bit unrelated to this CL, but if filter is false, we should just early return.
 Doesn't really matter, but setting waiting_for_upload_progress_ack_ to true
seems really weird, since we never sent the message we claim to be waiting for
in the first place.

Could call progress_timer_.Stop(), too, but I don't think that really matters.

https://codereview.chromium.org/1301103002/diff/160001/content/browser/loader...
File content/browser/loader/async_resource_handler_browsertest.cc (right):

https://codereview.chromium.org/1301103002/diff/160001/content/browser/loader...
content/browser/loader/async_resource_handler_browsertest.cc:19: namespace {
nit:  Blank line after start and before end of namespaces.

https://codereview.chromium.org/1301103002/diff/160001/content/browser/loader...
content/browser/loader/async_resource_handler_browsertest.cc:21: const char
kRedirect[] = "/redirect";
Maybe  kPostPath / kRedirectPostPath?  We post to both paths, afterall

https://codereview.chromium.org/1301103002/diff/160001/content/browser/loader...
content/browser/loader/async_resource_handler_browsertest.cc:23:
scoped_ptr<net::test_server::HttpResponse> SimplePost(
include base/memory/scoped_ptr.h

https://codereview.chromium.org/1301103002/diff/160001/content/browser/loader...
content/browser/loader/async_resource_handler_browsertest.cc:23:
scoped_ptr<net::test_server::HttpResponse> SimplePost(
include net/test/embedded_test_server/http_response.h

https://codereview.chromium.org/1301103002/diff/160001/content/browser/loader...
content/browser/loader/async_resource_handler_browsertest.cc:23:
scoped_ptr<net::test_server::HttpResponse> SimplePost(
Do we even need this method?  SimplePostRedirect works whenever this does as
well.  It also checks payload size, as an added bonus.

https://codereview.chromium.org/1301103002/diff/160001/content/browser/loader...
content/browser/loader/async_resource_handler_browsertest.cc:24: const
std::string& path,
include string.h

https://codereview.chromium.org/1301103002/diff/160001/content/browser/loader...
content/browser/loader/async_resource_handler_browsertest.cc:25: const
net::test_server::HttpRequest& request) {
net/test/embedded_test_server/http_request.h

https://codereview.chromium.org/1301103002/diff/160001/content/browser/loader...
content/browser/loader/async_resource_handler_browsertest.cc:33: const size_t
payload_size = 28697814; // 2*3^15
nit:  Two spaces between code and comments.

https://codereview.chromium.org/1301103002/diff/160001/content/browser/loader...
content/browser/loader/async_resource_handler_browsertest.cc:33: const size_t
payload_size = 28697814; // 2*3^15
kPayloadSize

https://codereview.chromium.org/1301103002/diff/160001/content/browser/loader...
content/browser/loader/async_resource_handler_browsertest.cc:34:
scoped_ptr<net::test_server::HttpResponse> SimplePostRedirect(
HandlePostAndRedirectURLs?

https://codereview.chromium.org/1301103002/diff/160001/content/browser/loader...
content/browser/loader/async_resource_handler_browsertest.cc:43:
EXPECT_EQ(request.content.length(), payload_size);;
Should explicitly include gtest.h

https://codereview.chromium.org/1301103002/diff/160001/content/browser/loader...
content/browser/loader/async_resource_handler_browsertest.cc:55: }  // namespace
nit:  Blank line after start and before end of namespaces.

https://codereview.chromium.org/1301103002/diff/160001/content/browser/loader...
content/browser/loader/async_resource_handler_browsertest.cc:61:
net::test_server::EmbeddedTestServer *test_server = embedded_test_server();
* should be before the space.

https://codereview.chromium.org/1301103002/diff/160001/content/browser/loader...
content/browser/loader/async_resource_handler_browsertest.cc:79:
net::test_server::EmbeddedTestServer *test_server = embedded_test_server();
* should be before the space.

https://codereview.chromium.org/1301103002/diff/160001/content/test/data/load...
File content/test/data/loader/async_resource_handler.html (right):

https://codereview.chromium.org/1301103002/diff/160001/content/test/data/load...
content/test/data/loader/async_resource_handler.html:4: var testSuccess= false;
Currently doesn't need to be a global.

https://codereview.chromium.org/1301103002/diff/160001/content/test/data/load...
content/test/data/loader/async_resource_handler.html:6: var ranProgressHandler=
false;
nit: space before =

https://codereview.chromium.org/1301103002/diff/160001/content/test/data/load...
content/test/data/loader/async_resource_handler.html:11: // build a long string,
fast.
nit:  capitalize Build

https://codereview.chromium.org/1301103002/diff/160001/content/test/data/load...
content/test/data/loader/async_resource_handler.html:27: progress > e.total)
We're testing for a ton of failure cases here.  I think it would make for better
failure information if we create a list of failure reasons, if any.  Then at the
end of the test, if it's empty, send "success", and if it's not, send it.  The
browser process reads it as a string, and EXPECT_EQs it with success.  Then on
failure, we learn exactly what went wrong.

Something like:

if (completedUpload)
  failureList.push("Received progress after finished.");
if (progress <= lastSeenProgress)
  failureList.push(...);

And then at the end of onFinished:

// Check for more failure reasons first.
...
var result = failureList.join("\r\n");
if (failureList.length == 0) {
  result = "success"
}
window.domAutomationController.send(result);

WDYT?

https://codereview.chromium.org/1301103002/diff/160001/content/test/data/load...
content/test/data/loader/async_resource_handler.html:28:
window.domAutomationController.send(false);
nit:  Use braces when the if condition or body takes more than one line.

[Charlie Harrison, 2015-08-27 18:43:53]

https://codereview.chromium.org/1301103002/diff/160001/content/test/data/load...
File content/test/data/loader/async_resource_handler.html (right):

https://codereview.chromium.org/1301103002/diff/160001/content/test/data/load...
content/test/data/loader/async_resource_handler.html:27: progress > e.total)
On 2015/08/27 17:21:29, mmenke wrote:
> We're testing for a ton of failure cases here.  I think it would make for
better
> failure information if we create a list of failure reasons, if any.  Then at
the
> end of the test, if it's empty, send "success", and if it's not, send it.  The
> browser process reads it as a string, and EXPECT_EQs it with success.  Then on
> failure, we learn exactly what went wrong.
> 
> Something like:
> 
> if (completedUpload)
>   failureList.push("Received progress after finished.");
> if (progress <= lastSeenProgress)
>   failureList.push(...);
> 
> And then at the end of onFinished:
> 
> // Check for more failure reasons first.
> ...
> var result = failureList.join("\r\n");
> if (failureList.length == 0) {
>   result = "success"
> }
> window.domAutomationController.send(result);
> 
> WDYT?

Great idea, otherwise this test would be very difficult to debug.

[Charlie Harrison, 2015-08-27 22:22:12]

csharrison@chromium.org changed reviewers:
- asanka@chromium.org

[mmenke, 2015-08-27 22:46:57]

LGTM!

https://codereview.chromium.org/1301103002/diff/220001/content/browser/loader...
File content/browser/loader/async_resource_handler_browsertest.cc (right):

https://codereview.chromium.org/1301103002/diff/220001/content/browser/loader...
content/browser/loader/async_resource_handler_browsertest.cc:69:
EXPECT_EQ(js_result, "");
I think it's a better practice to send "success" on success - relying on sending
an empty string on success just seems more regression prone.

https://codereview.chromium.org/1301103002/diff/220001/content/test/data/load...
File content/test/data/loader/async_resource_handler.html (right):

https://codereview.chromium.org/1301103002/diff/220001/content/test/data/load...
content/test/data/loader/async_resource_handler.html:12: var data = "yo";
nit:  Single quotes are actually preferred in Javascript (So one can embed HTML
in Javascript)

Goes for this whole file.

https://codereview.chromium.org/1301103002/diff/220001/content/test/data/load...
content/test/data/loader/async_resource_handler.html:56:
failureList.push("Receieved responseText: '" + this.responseText +"'. Expected
'hello'");
nit:  lines should be at most 80 characters (And you'll need to add braces).

[Charlie Harrison, 2015-08-28 13:14:35]

The CQ bit was checked by csharrison@chromium.org

[Charlie Harrison, 2015-08-28 13:14:35]

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/1301103002/#ps240001
(title: "single quotes, nits, and return success on success")

[commit-bot: I haz the power, 2015-08-28 13:14:49]

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

[commit-bot: I haz the power, 2015-08-28 13:22:38]

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

[commit-bot: I haz the power, 2015-08-28 13:22:39]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Charlie Harrison, 2015-08-28 13:24:23]

csharrison@chromium.org changed reviewers:
+ davidben@chromium.org

[Charlie Harrison, 2015-08-28 13:24:24]

Hey David can you review the download/ files?

[davidben, 2015-08-28 17:14:02]

content/browser/download lgtm. Let me know if you want me to look at
content/browser/loader as well, but otherwise I'm deferring to mmenke's review.

[Charlie Harrison, 2015-08-28 17:33:32]

The CQ bit was checked by csharrison@chromium.org

[commit-bot: I haz the power, 2015-08-28 17:33:45]

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

[commit-bot: I haz the power, 2015-08-28 17:41:07]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2015-08-28 17:42:38]

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