Plumb network stack information about existence of cached copy

Plumb network stack information about existence of cached copy
through to error page.  Specifically, add a "stale_copy_in_cache" argument to all of (ordered from Browser->Renderer):
* ResourceMsg_RequestComplete IPC message.
* ResourceDispatcher::OnRequestComplete
* ResourceLoaderBridge::Peer::OnCompletedRequest.
* All subclasses of RLB::P::OnCompleted Request, including WebURLLoaderImpl::context::OnCompletedRequest.
* Blink WebURLError and ResourceError classes (https://codereview.chromium.org/138493002).
* LocalizedError::GetStrings.

This is a paired commit with the blink CL
https://codereview.chromium.org/138493002.  That CL must be landed before this one.

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

[Randy Smith (Not in Mondays), 2014-01-14 19:17:24]

I'd like to request an initial, high level review of this CL.  (I know I'll need
more reviewers before landing, I'm just trying to get feedback on the areas that
I think are mostly likely to require large changes.)  

James: Could you give me feedback on webkit/* and the paired blink patch?  Go
gentle on me; this is my first time touching blink code :-}.

Ricardo: net/http_cache.*.  That change is to allow interposition for testing
purposes between the cache and network layers.  I could have tested this CL in a
different way (I think no-cache followed by a CONNECTION_REFUSED would do it)
but I'll eventually need something like what I've put in this CL for testing
specifically offline related errors, so I decided to do it earlier rather than
later.

John: ipc/* and content/common/resource_messages.h.  The basic question I'm
wondering about is whether I should have structurized
ResourceMsg_RequestComplete rather than adding extra arguments to various IPC
macros.  (I'm asking you because you and Darin are the overlaps in OWNERS
between ipc/* and content/common; feel free to redirect me if you're not a good
person, but I'd like to find one person to consider those two changes together.)

Thanks very much in advance ...

[jam, 2014-01-15 02:35:39]

On 2014/01/14 19:17:24, rdsmith wrote:
> I'd like to request an initial, high level review of this CL.  (I know I'll
need
> more reviewers before landing, I'm just trying to get feedback on the areas
that
> I think are mostly likely to require large changes.)  
> 
> James: Could you give me feedback on webkit/* and the paired blink patch?  Go
> gentle on me; this is my first time touching blink code :-}.
> 
> Ricardo: net/http_cache.*.  That change is to allow interposition for testing
> purposes between the cache and network layers.  I could have tested this CL in
a
> different way (I think no-cache followed by a CONNECTION_REFUSED would do it)
> but I'll eventually need something like what I've put in this CL for testing
> specifically offline related errors, so I decided to do it earlier rather than
> later.
> 
> John: ipc/* and content/common/resource_messages.h.  The basic question I'm
> wondering about is whether I should have structurized
> ResourceMsg_RequestComplete rather than adding extra arguments to various IPC
> macros.

yep, please add a struct. for background, a long time ago we drew the line at 5
arguments and then said anything past that use a struct.

>  (I'm asking you because you and Darin are the overlaps in OWNERS
> between ipc/* and content/common; feel free to redirect me if you're not a
good
> person, but I'd like to find one person to consider those two changes
together.)
> 
> Thanks very much in advance ...

[rvargas (doing something else), 2014-01-15 03:26:28]

https://codereview.chromium.org/138513002/diff/1/net/http/http_cache.h
File net/http/http_cache.h (right):

https://codereview.chromium.org/138513002/diff/1/net/http/http_cache.h#newcod...
net/http/http_cache.h:198: // Reset the network layer to allow for browser tests
that probe
nit: don't mention browser tests

https://codereview.chromium.org/138513002/diff/1/net/http/http_cache.h#newcod...
net/http/http_cache.h:201: scoped_ptr<HttpTransactionFactory>
SetNetworkLayerForTesting(
I'm wondering what do you envision for the returned network layer. That means
that I'm a little uncomfortable with giving away the old value, in part because
the use requires that no requests should be in flight when the network is
switched.

So I'm guessing that you want to create a browser, grab the cache right away and
replace the network for the lifetime of that browser.

[Randy Smith (Not in Mondays), 2014-01-15 19:07:06]

On 2014/01/14 19:17:24, rdsmith wrote:
> I'd like to request an initial, high level review of this CL.  (I know I'll
need
> more reviewers before landing, I'm just trying to get feedback on the areas
that
> I think are mostly likely to require large changes.)  
> 
> James: Could you give me feedback on webkit/* and the paired blink patch?  Go
> gentle on me; this is my first time touching blink code :-}.

Tony: Could you take a look at webkit/* and the paired blink patch for high
level comments?  If you're not the right person to ask, any chance you could
suggest someone else? 

(I'm choosing reviewers based on who is in both webkit/OWNERS and
third_party/WebKit/Source/platform/OWNERS.  I asked jamesr@, but he indicated he
wasn't a good choice.)

> 
> Ricardo: net/http_cache.*.  That change is to allow interposition for testing
> purposes between the cache and network layers.  I could have tested this CL in
a
> different way (I think no-cache followed by a CONNECTION_REFUSED would do it)
> but I'll eventually need something like what I've put in this CL for testing
> specifically offline related errors, so I decided to do it earlier rather than
> later.
> 
> John: ipc/* and content/common/resource_messages.h.  The basic question I'm
> wondering about is whether I should have structurized
> ResourceMsg_RequestComplete rather than adding extra arguments to various IPC
> macros.  (I'm asking you because you and Darin are the overlaps in OWNERS
> between ipc/* and content/common; feel free to redirect me if you're not a
good
> person, but I'd like to find one person to consider those two changes
together.)
> 
> Thanks very much in advance ...

[jamesr, 2014-01-15 19:12:02]

You'll have to find the right owner, but I think the site isolation folks are
most up to date on navigation issues at this level.  You could try asking
japhet@, nasko@, or abarth@ for some pointers.

[Randy Smith (Not in Mondays), 2014-01-15 19:14:43]

Incorporated Ricardo's comment.

https://codereview.chromium.org/138513002/diff/1/net/http/http_cache.h
File net/http/http_cache.h (right):

https://codereview.chromium.org/138513002/diff/1/net/http/http_cache.h#newcod...
net/http/http_cache.h:198: // Reset the network layer to allow for browser tests
that probe
On 2014/01/15 03:26:29, rvargas wrote:
> nit: don't mention browser tests

Done.

https://codereview.chromium.org/138513002/diff/1/net/http/http_cache.h#newcod...
net/http/http_cache.h:201: scoped_ptr<HttpTransactionFactory>
SetNetworkLayerForTesting(
On 2014/01/15 03:26:29, rvargas wrote:
> I'm wondering what do you envision for the returned network layer. That means
> that I'm a little uncomfortable with giving away the old value, in part
because
> the use requires that no requests should be in flight when the network is
> switched.
> 
> So I'm guessing that you want to create a browser, grab the cache right away
and
> replace the network for the lifetime of that browser.

So that would work for this instance, and I'm willing to do that if you'd like. 
But the use case I had envisioned for possible/later tests was for the new
HttpTransactionFactory to act as a filter, sometimes interception
HttpTransaction creation requests, and sometimes passing them through to the
original factory.  This interface would enable that usage.

[Randy Smith (Not in Mondays), 2014-01-15 19:17:07]

Nate: Would you be willing to take a high-level look at webkit/* in this CL and
all of https://codereview.chromium.org/138493002/ and let me know what you
think?  This is my first time venturing into blink, so I'd like to get an early
sanity check.

[rvargas (doing something else), 2014-01-15 19:39:45]

https://codereview.chromium.org/138513002/diff/1/net/http/http_cache.h
File net/http/http_cache.h (right):

https://codereview.chromium.org/138513002/diff/1/net/http/http_cache.h#newcod...
net/http/http_cache.h:201: scoped_ptr<HttpTransactionFactory>
SetNetworkLayerForTesting(
On 2014/01/15 19:14:43, rdsmith wrote:
> On 2014/01/15 03:26:29, rvargas wrote:
> > I'm wondering what do you envision for the returned network layer. That
means
> > that I'm a little uncomfortable with giving away the old value, in part
> because
> > the use requires that no requests should be in flight when the network is
> > switched.
> > 
> > So I'm guessing that you want to create a browser, grab the cache right away
> and
> > replace the network for the lifetime of that browser.
> 
> So that would work for this instance, and I'm willing to do that if you'd
like. 
> But the use case I had envisioned for possible/later tests was for the new
> HttpTransactionFactory to act as a filter, sometimes interception
> HttpTransaction creation requests, and sometimes passing them through to the
> original factory.  This interface would enable that usage.

ah, ok. That works.

[Randy Smith (Not in Mondays), 2014-01-23 19:48:20]

Ok, I'd like to officially request reviews for this code.  

Elly (ellyjones@): Could you look over the whole CL at a high level and make
sure it looks good?  Don't worry about any details you don't want to--I'm trying
to cover all the individual pieces with the reviewers below. 

Matt (mmenke@): Could you look at error_page_browsertest.cc,
render_view_browsertest.cc, */test/data/*, and localized_error.*,
net_error_helper.*?  I'll also ask John to review render_view_browsertest.cc,
but the code's pretty much copied from e_p_b.cc, so I think your looking at it
also makes sense.

John (jam@): Could you look at the changes to ResourceMsg_RequestComplete, as
well as the render view browser test stuff above and the required changes to
ContentBrowserTest for that?  That's resources_messages.h (obviously), as well
as async_resource_handler.cc, resource_dispatcher_host_{impl,unittest}.cc,
render_view_browsertest.cc, and content/test/content_browser_test.*.  I'm also
asking Chris Evans (below) for a security review of resource_messages.h, but I
expect that to be pro forma, so I'm looking for architectural comments from you.

Adam (abarth@): Could you look at webkit/child/* as well as
https://codereview.chromium.org/138493002/?  I know you're not an OWNER of
webkit/*, but you are for the blink CL, and thus I think the right place to
start for the webkit/* stuff.  I'll get an owner stamp once you ok it. 

Chris (cevans@): Could you look at resource_messages.h for security review?  I
believe this is pro-forma, since all I'm doing is structurizing and adding a
bool that was blessed by silence (and discussion with Parisa) in the security
review requested in http://crbug.com/308233.

Ricardo (rvargas@): Could you look at net/*?  I broke TestNetworkTransaction out
into its own file since it was being used at both content and chrome levels.

All: If anyone feels competent to bless the pure interface propagation changes
from the changes to ResourceLoaderBridge::Peer::OnCompletedRequest, let me know.
 I figure I'm probably going to need to get a final stamp from someone who owns
the top of the tree for those, *and* I think they're trivial, but there's no one
on the list above who seems the best to ask to do that.

Thanks very much in advance!

[mmenke, 2014-01-23 20:40:51]

Quick nits - I think this approach to testing looks reasonable, though I haven't
dug into the browser tests.

https://codereview.chromium.org/138513002/diff/560001/chrome/browser/errorpag...
File chrome/browser/errorpage_browsertest.cc (right):

https://codereview.chromium.org/138513002/diff/560001/chrome/browser/errorpag...
chrome/browser/errorpage_browsertest.cc:158: enum net::Error err) {
nit:  "enum" not needed.

https://codereview.chromium.org/138513002/diff/560001/chrome/browser/errorpag...
chrome/browser/errorpage_browsertest.cc:158: enum net::Error err) {
nit:  err -> error ("err", while not an uncommon abbreviation, isn't much used
for network errors)

https://codereview.chromium.org/138513002/diff/560001/net/http/http_cache.cc
File net/http/http_cache.cc (right):

https://codereview.chromium.org/138513002/diff/560001/net/http/http_cache.cc#...
net/http/http_cache.cc:471: scoped_ptr<HttpTransactionFactory>
new_network_layer) {
Suggest calling this "SetHttpNetworkTransactionFactoryForTesting". 
"NetworkLayer" is too weird (We also aren't setting the FtpTransactionFactory,
so it's not the entire network layer).

I know it's called network_layer_ internally, but I've always found that
confusing.

https://codereview.chromium.org/138513002/diff/560001/net/http/test_network_t...
File net/http/test_network_transaction.cc (right):

https://codereview.chromium.org/138513002/diff/560001/net/http/test_network_t...
net/http/test_network_transaction.cc:9: #include "base/memory/scoped_ptr.h"
This is needed in the header file, rather than here.

https://codereview.chromium.org/138513002/diff/560001/net/http/test_network_t...
net/http/test_network_transaction.cc:13: #include "net/http/http_cache.h"
Don't need this, though may want to forward declare it in the header.

https://codereview.chromium.org/138513002/diff/560001/net/http/test_network_t...
net/http/test_network_transaction.cc:14: #include
"net/url_request/url_request_context_getter.h"
Not needed

https://codereview.chromium.org/138513002/diff/560001/net/http/test_network_t...
net/http/test_network_transaction.cc:92: load_timing_info->send_end =
base::TimeTicks::Now();
Suggest just:

*load_timing_info = LoadTimingInfo();

https://codereview.chromium.org/138513002/diff/560001/net/http/test_network_t...
net/http/test_network_transaction.cc:130: return NULL;                        //
TODO(rdsmith): Is this really safe?
Can't we just pass in a real session?  Seems the safest thing to do, and at
least when we have the cache, we can get one from it.

https://codereview.chromium.org/138513002/diff/560001/net/http/test_network_t...
File net/http/test_network_transaction.h (right):

https://codereview.chromium.org/138513002/diff/560001/net/http/test_network_t...
net/http/test_network_transaction.h:1: // Copyright (c) 2014 The Chromium
Authors. All rights reserved.
nit:  No (c) in new files.

https://codereview.chromium.org/138513002/diff/560001/net/http/test_network_t...
net/http/test_network_transaction.h:1: // Copyright (c) 2014 The Chromium
Authors. All rights reserved.
Think this file is more about the factory than the transaction, suggest naming
after the factory instead.

https://codereview.chromium.org/138513002/diff/560001/net/http/test_network_t...
net/http/test_network_transaction.h:28: class NET_EXPORT TestNetworkTransaction
: public HttpTransaction {
This doesn't need to be exported, or even in the header, for now.

https://codereview.chromium.org/138513002/diff/560001/net/http/test_network_t...
net/http/test_network_transaction.h:28: class NET_EXPORT TestNetworkTransaction
: public HttpTransaction {
Maybe FailingHttpTransaction?

https://codereview.chromium.org/138513002/diff/560001/net/http/test_network_t...
net/http/test_network_transaction.h:30: explicit TestNetworkTransaction(enum
Error err);
--enum
err->error

https://codereview.chromium.org/138513002/diff/560001/net/http/test_network_t...
net/http/test_network_transaction.h:70: // |err| must not be net::OK.
Don't think the "net::" is needed.

https://codereview.chromium.org/138513002/diff/560001/net/http/test_network_t...
net/http/test_network_transaction.h:71: class NET_EXPORT
TestNetworkTransactionFactory : public HttpTransactionFactory {
This should have a clearer name.  FailingNetworkTransactionFactory?  Actually,
these are just HttpTransactions, nothing networky about them.... 
FailingHttpTransactionFactory?

https://codereview.chromium.org/138513002/diff/560001/net/http/test_network_t...
net/http/test_network_transaction.h:73: TestNetworkTransactionFactory(enum Error
err);
--enum
err->error

https://codereview.chromium.org/138513002/diff/560001/net/http/test_network_t...
net/http/test_network_transaction.h:84: enum Error err_;
--enum, and err_ -> error_.

[mmenke, 2014-01-23 20:59:18]

https://codereview.chromium.org/138513002/diff/560001/net/http/test_network_t...
File net/http/test_network_transaction.cc (right):

https://codereview.chromium.org/138513002/diff/560001/net/http/test_network_t...
net/http/test_network_transaction.cc:19: DCHECK_NE(OK, err);
This should be a CHECK, so it surfaces during release builds as well.

Also should probably be a CHECK_LT(err, OK); (Or GT, if you prefer - I think LE
is a little more intuitive, but up to you).

https://codereview.chromium.org/138513002/diff/560001/net/http/test_network_t...
net/http/test_network_transaction.cc:19: DCHECK_NE(OK, err);
Should have a similar check on creation of the factory.  Makes for a clearer
stack trace.

[Randy Smith (Not in Mondays), 2014-01-23 22:12:32]

Matt: I'm responding early (before uploading a new PS or incorporating all
comments) because there's one of your comments I'd specifically like Ricardo's
input on.  I don't disagree with any of your remaining comments, I just want to
incorporate them at the same time as any comments Ricardo has.

https://codereview.chromium.org/138513002/diff/560001/chrome/browser/errorpag...
File chrome/browser/errorpage_browsertest.cc (right):

https://codereview.chromium.org/138513002/diff/560001/chrome/browser/errorpag...
chrome/browser/errorpage_browsertest.cc:158: enum net::Error err) {
On 2014/01/23 20:40:52, mmenke wrote:
> nit:  "enum" not needed.

Done.

https://codereview.chromium.org/138513002/diff/560001/chrome/browser/errorpag...
chrome/browser/errorpage_browsertest.cc:158: enum net::Error err) {
On 2014/01/23 20:40:52, mmenke wrote:
> nit:  err -> error ("err", while not an uncommon abbreviation, isn't much used
> for network errors)

Done.

https://codereview.chromium.org/138513002/diff/560001/net/http/test_network_t...
File net/http/test_network_transaction.cc (right):

https://codereview.chromium.org/138513002/diff/560001/net/http/test_network_t...
net/http/test_network_transaction.cc:9: #include "base/memory/scoped_ptr.h"
On 2014/01/23 20:40:52, mmenke wrote:
> This is needed in the header file, rather than here.

Done.

https://codereview.chromium.org/138513002/diff/560001/net/http/test_network_t...
net/http/test_network_transaction.cc:13: #include "net/http/http_cache.h"
On 2014/01/23 20:40:52, mmenke wrote:
> Don't need this, though may want to forward declare it in the header.

Done.

https://codereview.chromium.org/138513002/diff/560001/net/http/test_network_t...
net/http/test_network_transaction.cc:14: #include
"net/url_request/url_request_context_getter.h"
On 2014/01/23 20:40:52, mmenke wrote:
> Not needed

Done.

https://codereview.chromium.org/138513002/diff/560001/net/http/test_network_t...
net/http/test_network_transaction.cc:19: DCHECK_NE(OK, err);
On 2014/01/23 20:59:19, mmenke wrote:
> This should be a CHECK, so it surfaces during release builds as well.

Done.

> Also should probably be a CHECK_LT(err, OK); (Or GT, if you prefer - I think
LE
> is a little more intuitive, but up to you).

Yeah, I've seen people argue for constant first in these comparisons, and it
seems to be our predominant idiom, so I generally do that, even though it looks
a bit weird to me (i.e. I'm doing GT).

https://codereview.chromium.org/138513002/diff/560001/net/http/test_network_t...
net/http/test_network_transaction.cc:19: DCHECK_NE(OK, err);
On 2014/01/23 20:59:19, mmenke wrote:
> Should have a similar check on creation of the factory.  Makes for a clearer
> stack trace.

Quite right.  Done.

https://codereview.chromium.org/138513002/diff/560001/net/http/test_network_t...
net/http/test_network_transaction.cc:92: load_timing_info->send_end =
base::TimeTicks::Now();
On 2014/01/23 20:40:52, mmenke wrote:
> Suggest just:
> 
> *load_timing_info = LoadTimingInfo();

I'm wary of doing that, just because I don't know how the load timing info is
used, and it wouldn't match the effect of
MockNetworkTransaction::GetLoadTimingInfo() (socket_reused would be false). 
Ricardo, do you have an opinion?

https://codereview.chromium.org/138513002/diff/560001/net/http/test_network_t...
File net/http/test_network_transaction.h (right):

https://codereview.chromium.org/138513002/diff/560001/net/http/test_network_t...
net/http/test_network_transaction.h:1: // Copyright (c) 2014 The Chromium
Authors. All rights reserved.
On 2014/01/23 20:40:52, mmenke wrote:
> nit:  No (c) in new files.

Done.

https://codereview.chromium.org/138513002/diff/560001/net/http/test_network_t...
net/http/test_network_transaction.h:30: explicit TestNetworkTransaction(enum
Error err);
On 2014/01/23 20:40:52, mmenke wrote:
> --enum
> err->error

Done.

https://codereview.chromium.org/138513002/diff/560001/net/http/test_network_t...
net/http/test_network_transaction.h:70: // |err| must not be net::OK.
On 2014/01/23 20:40:52, mmenke wrote:
> Don't think the "net::" is needed.

Done.

https://codereview.chromium.org/138513002/diff/560001/net/http/test_network_t...
net/http/test_network_transaction.h:73: TestNetworkTransactionFactory(enum Error
err);
On 2014/01/23 20:40:52, mmenke wrote:
> --enum
> err->error

Done.

https://codereview.chromium.org/138513002/diff/560001/net/http/test_network_t...
net/http/test_network_transaction.h:84: enum Error err_;
On 2014/01/23 20:40:52, mmenke wrote:
> --enum, and err_ -> error_.

Done.

[mmenke, 2014-01-23 22:30:13]

https://codereview.chromium.org/138513002/diff/560001/net/http/test_network_t...
File net/http/test_network_transaction.cc (right):

https://codereview.chromium.org/138513002/diff/560001/net/http/test_network_t...
net/http/test_network_transaction.cc:92: load_timing_info->send_end =
base::TimeTicks::Now();
On 2014/01/23 22:12:33, rdsmith wrote:
> On 2014/01/23 20:40:52, mmenke wrote:
> > Suggest just:
> > 
> > *load_timing_info = LoadTimingInfo();
> 
> I'm wary of doing that, just because I don't know how the load timing info is
> used, and it wouldn't match the effect of
> MockNetworkTransaction::GetLoadTimingInfo() (socket_reused would be false). 
> Ricardo, do you have an opinion?

It's for HAR file generation, the navigation timing API, and the resource timing
API.  Actually, we can just do nothing and return false, since we never go a
stream (Real HttpNetworkTransactions do that, too).

[rvargas (doing something else), 2014-01-23 23:24:48]

https://codereview.chromium.org/138513002/diff/560001/net/http/http_cache.h
File net/http/http_cache.h (right):

https://codereview.chromium.org/138513002/diff/560001/net/http/http_cache.h#n...
net/http/http_cache.h:198: // Reset the network layer to allow for tests that
probe
nit: resets

https://codereview.chromium.org/138513002/diff/560001/net/http/test_network_t...
File net/http/test_network_transaction.cc (right):

https://codereview.chromium.org/138513002/diff/560001/net/http/test_network_t...
net/http/test_network_transaction.cc:19: DCHECK_NE(OK, err);
On 2014/01/23 22:12:33, rdsmith wrote:
> On 2014/01/23 20:59:19, mmenke wrote:
> > This should be a CHECK, so it surfaces during release builds as well.
> 
> Done.
> 
> > Also should probably be a CHECK_LT(err, OK); (Or GT, if you prefer - I think
> LE
> > is a little more intuitive, but up to you).
> 
> Yeah, I've seen people argue for constant first in these comparisons, and it
> seems to be our predominant idiom, so I generally do that, even though it
looks
> a bit weird to me (i.e. I'm doing GT).

I would actually argue for readability as opposed to constant first. Constant
first applies only to test macros (EXPECT_).

But I'm actually curious about why preferring GT or LT vs NE; it seems like the
only value that we don't want here is OK so IMO we should check against that.

https://codereview.chromium.org/138513002/diff/560001/net/http/test_network_t...
net/http/test_network_transaction.cc:93: return true;
don't do anything and return false?

[mmenke, 2014-01-23 23:27:17]

https://codereview.chromium.org/138513002/diff/560001/net/http/test_network_t...
File net/http/test_network_transaction.cc (right):

https://codereview.chromium.org/138513002/diff/560001/net/http/test_network_t...
net/http/test_network_transaction.cc:19: DCHECK_NE(OK, err);
On 2014/01/23 23:24:49, rvargas wrote:
> On 2014/01/23 22:12:33, rdsmith wrote:
> > On 2014/01/23 20:59:19, mmenke wrote:
> > > This should be a CHECK, so it surfaces during release builds as well.
> > 
> > Done.
> > 
> > > Also should probably be a CHECK_LT(err, OK); (Or GT, if you prefer - I
think
> > LE
> > > is a little more intuitive, but up to you).
> > 
> > Yeah, I've seen people argue for constant first in these comparisons, and it
> > seems to be our predominant idiom, so I generally do that, even though it
> looks
> > a bit weird to me (i.e. I'm doing GT).
> 
> I would actually argue for readability as opposed to constant first. Constant
> first applies only to test macros (EXPECT_).
> 
> But I'm actually curious about why preferring GT or LT vs NE; it seems like
the
> only value that we don't want here is OK so IMO we should check against that.

Positive values generally indicate success.  What happens when a function that
returns OK success and a negative value on failure instead returns a positive
value seems like something we should avoid worrying about, hence we shouldn't
allow positive values here.

[rvargas (doing something else), 2014-01-24 02:09:59]

https://codereview.chromium.org/138513002/diff/560001/net/http/test_network_t...
File net/http/test_network_transaction.cc (right):

https://codereview.chromium.org/138513002/diff/560001/net/http/test_network_t...
net/http/test_network_transaction.cc:19: DCHECK_NE(OK, err);
On 2014/01/23 23:27:18, mmenke wrote:
> On 2014/01/23 23:24:49, rvargas wrote:
> > On 2014/01/23 22:12:33, rdsmith wrote:
> > > On 2014/01/23 20:59:19, mmenke wrote:
> > > > This should be a CHECK, so it surfaces during release builds as well.
> > > 
> > > Done.
> > > 
> > > > Also should probably be a CHECK_LT(err, OK); (Or GT, if you prefer - I
> think
> > > LE
> > > > is a little more intuitive, but up to you).
> > > 
> > > Yeah, I've seen people argue for constant first in these comparisons, and
it
> > > seems to be our predominant idiom, so I generally do that, even though it
> > looks
> > > a bit weird to me (i.e. I'm doing GT).
> > 
> > I would actually argue for readability as opposed to constant first.
Constant
> > first applies only to test macros (EXPECT_).
> > 
> > But I'm actually curious about why preferring GT or LT vs NE; it seems like
> the
> > only value that we don't want here is OK so IMO we should check against
that.
> 
> Positive values generally indicate success.  What happens when a function that
> returns OK success and a negative value on failure instead returns a positive
> value seems like something we should avoid worrying about, hence we shouldn't
> allow positive values here.

That doesn't match the definition of Start (the method that is supposed to
return this error code). Start returns a plain error code not error code |
bytes.

[rvargas (doing something else), 2014-01-24 16:18:41]

https://codereview.chromium.org/138513002/diff/560001/net/http/test_network_t...
File net/http/test_network_transaction.cc (right):

https://codereview.chromium.org/138513002/diff/560001/net/http/test_network_t...
net/http/test_network_transaction.cc:19: DCHECK_NE(OK, err);
On 2014/01/24 02:10:00, rvargas wrote:
> On 2014/01/23 23:27:18, mmenke wrote:
> > On 2014/01/23 23:24:49, rvargas wrote:
> > > On 2014/01/23 22:12:33, rdsmith wrote:
> > > > On 2014/01/23 20:59:19, mmenke wrote:
> > > > > This should be a CHECK, so it surfaces during release builds as well.
> > > > 
> > > > Done.
> > > > 
> > > > > Also should probably be a CHECK_LT(err, OK); (Or GT, if you prefer - I
> > think
> > > > LE
> > > > > is a little more intuitive, but up to you).
> > > > 
> > > > Yeah, I've seen people argue for constant first in these comparisons,
and
> it
> > > > seems to be our predominant idiom, so I generally do that, even though
it
> > > looks
> > > > a bit weird to me (i.e. I'm doing GT).
> > > 
> > > I would actually argue for readability as opposed to constant first.
> Constant
> > > first applies only to test macros (EXPECT_).
> > > 
> > > But I'm actually curious about why preferring GT or LT vs NE; it seems
like
> > the
> > > only value that we don't want here is OK so IMO we should check against
> that.
> > 
> > Positive values generally indicate success.  What happens when a function
that
> > returns OK success and a negative value on failure instead returns a
positive
> > value seems like something we should avoid worrying about, hence we
shouldn't
> > allow positive values here.
> 
> That doesn't match the definition of Start (the method that is supposed to
> return this error code). Start returns a plain error code not error code |
> bytes.

I guess what I said doesn't make a lot of sense, so let me try again:

If this checks that from the error codes that Start can return the caller uses a
failure one, then EQ looks better to me. If we also want to check that the
caller is not using a return code that doesn't make any sense at all, then LT
makes sense, but reading that code trows me off as to what is the intention of
the check if an explicit error code is used. In that case, I think LT(error, 0)
is a better check for "this is an error condition".

Also, this would be a programmer error (improper use of the interface) so I
think it should be a DCHECK, not a CHECK... we want to fail tests, not crash
them (more of a philosophical point because in practice release bots enable
DCHECKs).

[Randy Smith (Not in Mondays), 2014-01-25 21:11:30]

Incorporated all comments made so far by Matt and Ricardo.  Waiting on comments
from Elly, John, and Chris.  Adam: I just noticed I hadn't included you on the
original reviewers list; sorry about that.  My request to you from c#9: 

'Adam (abarth@): Could you look at webkit/child/* as well as
https://codereview.chromium.org/138493002/?  I know you're not an OWNER of
webkit/*, but you are for the blink CL, and thus I think the right place to
start for the webkit/* stuff.  I'll get an owner stamp once you ok it. '

https://codereview.chromium.org/138513002/diff/560001/net/http/http_cache.cc
File net/http/http_cache.cc (right):

https://codereview.chromium.org/138513002/diff/560001/net/http/http_cache.cc#...
net/http/http_cache.cc:471: scoped_ptr<HttpTransactionFactory>
new_network_layer) {
On 2014/01/23 20:40:52, mmenke wrote:
> Suggest calling this "SetHttpNetworkTransactionFactoryForTesting". 
> "NetworkLayer" is too weird (We also aren't setting the FtpTransactionFactory,
> so it's not the entire network layer).
> 
> I know it's called network_layer_ internally, but I've always found that
> confusing.

Done.

https://codereview.chromium.org/138513002/diff/560001/net/http/http_cache.h
File net/http/http_cache.h (right):

https://codereview.chromium.org/138513002/diff/560001/net/http/http_cache.h#n...
net/http/http_cache.h:198: // Reset the network layer to allow for tests that
probe
On 2014/01/23 23:24:49, rvargas wrote:
> nit: resets

Done.

https://codereview.chromium.org/138513002/diff/560001/net/http/test_network_t...
File net/http/test_network_transaction.cc (right):

https://codereview.chromium.org/138513002/diff/560001/net/http/test_network_t...
net/http/test_network_transaction.cc:19: DCHECK_NE(OK, err);
On 2014/01/24 16:18:42, rvargas wrote:
> On 2014/01/24 02:10:00, rvargas wrote:
> > On 2014/01/23 23:27:18, mmenke wrote:
> > > On 2014/01/23 23:24:49, rvargas wrote:
> > > > On 2014/01/23 22:12:33, rdsmith wrote:
> > > > > On 2014/01/23 20:59:19, mmenke wrote:
> > > > > > This should be a CHECK, so it surfaces during release builds as
well.
> > > > > 
> > > > > Done.
> > > > > 
> > > > > > Also should probably be a CHECK_LT(err, OK); (Or GT, if you prefer -
I
> > > think
> > > > > LE
> > > > > > is a little more intuitive, but up to you).
> > > > > 
> > > > > Yeah, I've seen people argue for constant first in these comparisons,
> and
> > it
> > > > > seems to be our predominant idiom, so I generally do that, even though
> it
> > > > looks
> > > > > a bit weird to me (i.e. I'm doing GT).
> > > > 
> > > > I would actually argue for readability as opposed to constant first.
> > Constant
> > > > first applies only to test macros (EXPECT_).
> > > > 
> > > > But I'm actually curious about why preferring GT or LT vs NE; it seems
> like
> > > the
> > > > only value that we don't want here is OK so IMO we should check against
> > that.
> > > 
> > > Positive values generally indicate success.  What happens when a function
> that
> > > returns OK success and a negative value on failure instead returns a
> positive
> > > value seems like something we should avoid worrying about, hence we
> shouldn't
> > > allow positive values here.
> > 
> > That doesn't match the definition of Start (the method that is supposed to
> > return this error code). Start returns a plain error code not error code |
> > bytes.
> 
> I guess what I said doesn't make a lot of sense, so let me try again:
> 
> If this checks that from the error codes that Start can return the caller uses
a
> failure one, then EQ looks better to me. If we also want to check that the
> caller is not using a return code that doesn't make any sense at all, then LT
> makes sense, but reading that code trows me off as to what is the intention of
> the check if an explicit error code is used. In that case, I think LT(error,
0)
> is a better check for "this is an error condition".
> 
> Also, this would be a programmer error (improper use of the interface) so I
> think it should be a DCHECK, not a CHECK... we want to fail tests, not crash
> them (more of a philosophical point because in practice release bots enable
> DCHECKs).

With regard to the ordering, I'm happy to yield to reviewer preference to put
the variable first.

With regard to LT verse NE I'm going to say that leaving theoretical arguments
aside, I want to catch the largest category of inputs that don't make sense
given the intent of the class, and use LT.

With regard to DCHECK vs. CHECK, I'm going to say that given that the release
bots enable DCHECK, it doesn't actually matter.  In the context of it not
mattering, I'm going to go with DCHECK, as my internal sense is that CHECK
should only be used in cases where you're clear it's preferable that chrome
provide the horrible UE of crashing rather than trying to limp along in some
fashion (even though this code will never show up in user run code).  However, I
don't really care; I'm just picking one of the options, and if one of you cares
strongly and the other doesn't, I'm happy to go with that person.  

If it turns out that the release bots didn't enable DCHECK, I'd use CHECK.

https://codereview.chromium.org/138513002/diff/560001/net/http/test_network_t...
net/http/test_network_transaction.cc:92: load_timing_info->send_end =
base::TimeTicks::Now();
On 2014/01/23 22:30:14, mmenke wrote:
> On 2014/01/23 22:12:33, rdsmith wrote:
> > On 2014/01/23 20:40:52, mmenke wrote:
> > > Suggest just:
> > > 
> > > *load_timing_info = LoadTimingInfo();
> > 
> > I'm wary of doing that, just because I don't know how the load timing info
is
> > used, and it wouldn't match the effect of
> > MockNetworkTransaction::GetLoadTimingInfo() (socket_reused would be false). 
> > Ricardo, do you have an opinion?
> 
> It's for HAR file generation, the navigation timing API, and the resource
timing
> API.  Actually, we can just do nothing and return false, since we never go a
> stream (Real HttpNetworkTransactions do that, too).

returning false.

https://codereview.chromium.org/138513002/diff/560001/net/http/test_network_t...
net/http/test_network_transaction.cc:93: return true;
On 2014/01/23 23:24:49, rvargas wrote:
> don't do anything and return false?

Done.

https://codereview.chromium.org/138513002/diff/560001/net/http/test_network_t...
net/http/test_network_transaction.cc:130: return NULL;                        //
TODO(rdsmith): Is this really safe?
On 2014/01/23 20:40:52, mmenke wrote:
> Can't we just pass in a real session?  Seems the safest thing to do, and at
> least when we have the cache, we can get one from it.

Done.

https://codereview.chromium.org/138513002/diff/560001/net/http/test_network_t...
File net/http/test_network_transaction.h (right):

https://codereview.chromium.org/138513002/diff/560001/net/http/test_network_t...
net/http/test_network_transaction.h:1: // Copyright (c) 2014 The Chromium
Authors. All rights reserved.
On 2014/01/23 20:40:52, mmenke wrote:
> Think this file is more about the factory than the transaction, suggest naming
> after the factory instead.

Done.

https://codereview.chromium.org/138513002/diff/560001/net/http/test_network_t...
net/http/test_network_transaction.h:28: class NET_EXPORT TestNetworkTransaction
: public HttpTransaction {
On 2014/01/23 20:40:52, mmenke wrote:
> This doesn't need to be exported, or even in the header, for now.

Done.

https://codereview.chromium.org/138513002/diff/560001/net/http/test_network_t...
net/http/test_network_transaction.h:28: class NET_EXPORT TestNetworkTransaction
: public HttpTransaction {
On 2014/01/23 20:40:52, mmenke wrote:
> Maybe FailingHttpTransaction?

Done.

https://codereview.chromium.org/138513002/diff/560001/net/http/test_network_t...
net/http/test_network_transaction.h:71: class NET_EXPORT
TestNetworkTransactionFactory : public HttpTransactionFactory {
On 2014/01/23 20:40:52, mmenke wrote:
> This should have a clearer name.  FailingNetworkTransactionFactory?  Actually,
> these are just HttpTransactions, nothing networky about them.... 
> FailingHttpTransactionFactory?

Done.

[rvargas (doing something else), 2014-01-28 01:37:33]

net LGTM

[Randy Smith (Not in Mondays), 2014-01-28 16:29:31]

John Abd-El Malek, Chris Evans: Ping?

[mmenke, 2014-01-28 17:27:22]

https://codereview.chromium.org/138513002/diff/730001/chrome/browser/errorpag...
File chrome/browser/errorpage_browsertest.cc (right):

https://codereview.chromium.org/138513002/diff/730001/chrome/browser/errorpag...
chrome/browser/errorpage_browsertest.cc:163:
scoped_ptr<net::FailingHttpTransactionFactory> factory(
nit:  Can just make this a HttpTransactionFactory, and then pass with
factory.Pass().

https://codereview.chromium.org/138513002/diff/730001/chrome/browser/errorpag...
chrome/browser/errorpage_browsertest.cc:166: // need to restore the old state.
nit:  Don't use "we" in comments.

https://codereview.chromium.org/138513002/diff/730001/chrome/browser/errorpag...
chrome/browser/errorpage_browsertest.cc:426: EXPECT_TRUE(result);
optional:  It's simpler to do:

ASSERT_TRUE(content::ExecuteScriptAndExtractBool(...));
EXPECT_TRUE(result);

Though I suppose that doesn't separate out the cases where the script fails to
run, and where we don't get a bool.  I don't think I've ever seen the first
happen before, though.

https://codereview.chromium.org/138513002/diff/730001/chrome/browser/errorpag...
chrome/browser/errorpage_browsertest.cc:440: EXPECT_FALSE(result);
Suggest making this into a function.  4 copies seems a bit much.

https://codereview.chromium.org/138513002/diff/730001/chrome/browser/errorpag...
chrome/browser/errorpage_browsertest.cc:556: browser(), test_url, 1);
If Link Doctor is failing properly, this should be 2.

https://codereview.chromium.org/138513002/diff/730001/chrome/common/localized...
File chrome/common/localized_error.cc (right):

https://codereview.chromium.org/138513002/diff/730001/chrome/common/localized...
chrome/common/localized_error.cc:562:
error_strings->SetBoolean("staleCopyInCache", stale_copy_in_cache);
From a UI consistency standpoint, may want to add an entry to the suggestions
array, rather than a bool.  OF course, you aren't hooking that up yet, so could
leave worrying about that for another CL.

https://codereview.chromium.org/138513002/diff/730001/chrome/common/localized...
chrome/common/localized_error.cc:562:
error_strings->SetBoolean("staleCopyInCache", stale_copy_in_cache);
Just a heads up that we may want to, at least temporarily, disable any option
this adds when showing Link Doctor suggestions (Link Doctor suggestions are
probably going to use a different format from the current suggestions, so it's
just a matter of making the UI look reasonable, not some opposition to mixing
the suggestions).

https://codereview.chromium.org/138513002/diff/730001/net/http/failing_http_t...
File net/http/failing_http_transaction_factory.h (right):

https://codereview.chromium.org/138513002/diff/730001/net/http/failing_http_t...
net/http/failing_http_transaction_factory.h:9: #include
"net/base/completion_callback.h"
Not needed.

https://codereview.chromium.org/138513002/diff/730001/net/http/failing_http_t...
net/http/failing_http_transaction_factory.h:25: struct HttpRequestInfo;
Should probably add a forward declaration of HttpNetworkSession.

https://codereview.chromium.org/138513002/diff/730001/net/http/failing_http_t...
net/http/failing_http_transaction_factory.h:25: struct HttpRequestInfo;
Of all these forward declarations, only HttpCache is needed.  Some of the others
should  probably go into the cc file, though.

https://codereview.chromium.org/138513002/diff/730001/net/http/failing_http_t...
net/http/failing_http_transaction_factory.h:28: // |err| must not be OK.
Think it's generally a bad idea to mention function arguments in class
description (And it's no longer "err", for that matter).  Suggest something
like:  "Creates transactions that always (asynchronously) return a specified
error.  The error is returned asynchronously, just after the transaction is
started."

https://codereview.chromium.org/138513002/diff/730001/net/http/failing_http_t...
net/http/failing_http_transaction_factory.h:40: virtual HttpNetworkSession*
GetSession() OVERRIDE;
include platform_specific for OVERRIDE.

[Randy Smith (Not in Mondays), 2014-01-28 22:52:58]

Incorporated Matt's comments.  Matt, just confirming that you took a look a
render_view_browsertest.cc?

https://codereview.chromium.org/138513002/diff/730001/chrome/browser/errorpag...
File chrome/browser/errorpage_browsertest.cc (right):

https://codereview.chromium.org/138513002/diff/730001/chrome/browser/errorpag...
chrome/browser/errorpage_browsertest.cc:163:
scoped_ptr<net::FailingHttpTransactionFactory> factory(
On 2014/01/28 17:27:23, mmenke wrote:
> nit:  Can just make this a HttpTransactionFactory, and then pass with
> factory.Pass().

Done.

https://codereview.chromium.org/138513002/diff/730001/chrome/browser/errorpag...
chrome/browser/errorpage_browsertest.cc:166: // need to restore the old state.
On 2014/01/28 17:27:23, mmenke wrote:
> nit:  Don't use "we" in comments.

Done.

https://codereview.chromium.org/138513002/diff/730001/chrome/browser/errorpag...
chrome/browser/errorpage_browsertest.cc:426: EXPECT_TRUE(result);
On 2014/01/28 17:27:23, mmenke wrote:
> optional:  It's simpler to do:
> 
> ASSERT_TRUE(content::ExecuteScriptAndExtractBool(...));
> EXPECT_TRUE(result);
> 
> Though I suppose that doesn't separate out the cases where the script fails to
> run, and where we don't get a bool.  I don't think I've ever seen the first
> happen before, though.

So it turns out that ExecuteScriptAndGetValue() and ExecuteScriptAndExtractBool
use very different methods to wait for the response to come back from the
renderer, and if I just do a straight replacement, the test hangs.  I believe
what's going on is that ExecuteScriptAndExtractBool uses a DOMOperationObserver
to wait for the response, and that the other uses of
ExecuteScriptAndExtractBool() call domAutomationController.send(), which trips
that observer.  

I'm willing to shift over to that method if you'd like, and part of me thinks I
should (for compatibility with the rest of the file) but having dug into the two
pathways a bit, I find myself preferring ExecuteScriptAndGetValue() because it
doesn't rely on DomAutomationController, only on mechanisms inside
RenderViewHostImpl (and presumably RenderView).  So I figured I'd report back
and let you make the call :-}.

https://codereview.chromium.org/138513002/diff/730001/chrome/browser/errorpag...
chrome/browser/errorpage_browsertest.cc:440: EXPECT_FALSE(result);
On 2014/01/28 17:27:23, mmenke wrote:
> Suggest making this into a function.  4 copies seems a bit much.

Done.

https://codereview.chromium.org/138513002/diff/730001/chrome/browser/errorpag...
chrome/browser/errorpage_browsertest.cc:556: browser(), test_url, 1);
On 2014/01/28 17:27:23, mmenke wrote:
> If Link Doctor is failing properly, this should be 2.

Done.

https://codereview.chromium.org/138513002/diff/730001/chrome/common/localized...
File chrome/common/localized_error.cc (right):

https://codereview.chromium.org/138513002/diff/730001/chrome/common/localized...
chrome/common/localized_error.cc:562:
error_strings->SetBoolean("staleCopyInCache", stale_copy_in_cache);
On 2014/01/28 17:27:23, mmenke wrote:
> From a UI consistency standpoint, may want to add an entry to the suggestions
> array, rather than a bool.  OF course, you aren't hooking that up yet, so
could
> leave worrying about that for another CL.

As discussed offline, ignoring for now.

https://codereview.chromium.org/138513002/diff/730001/chrome/common/localized...
chrome/common/localized_error.cc:562:
error_strings->SetBoolean("staleCopyInCache", stale_copy_in_cache);
On 2014/01/28 17:27:23, mmenke wrote:
> Just a heads up that we may want to, at least temporarily, disable any option
> this adds when showing Link Doctor suggestions (Link Doctor suggestions are
> probably going to use a different format from the current suggestions, so it's
> just a matter of making the UI look reasonable, not some opposition to mixing
> the suggestions).

Based on our discussion, I think this is moot.

https://codereview.chromium.org/138513002/diff/730001/net/http/failing_http_t...
File net/http/failing_http_transaction_factory.h (right):

https://codereview.chromium.org/138513002/diff/730001/net/http/failing_http_t...
net/http/failing_http_transaction_factory.h:9: #include
"net/base/completion_callback.h"
On 2014/01/28 17:27:23, mmenke wrote:
> Not needed.

Done.

https://codereview.chromium.org/138513002/diff/730001/net/http/failing_http_t...
net/http/failing_http_transaction_factory.h:25: struct HttpRequestInfo;
On 2014/01/28 17:27:23, mmenke wrote:
> Should probably add a forward declaration of HttpNetworkSession.

Done.

https://codereview.chromium.org/138513002/diff/730001/net/http/failing_http_t...
net/http/failing_http_transaction_factory.h:25: struct HttpRequestInfo;
On 2014/01/28 17:27:23, mmenke wrote:
> Of all these forward declarations, only HttpCache is needed.  Some of the
others
> should  probably go into the cc file, though.

Right, because the transaction declaration used to be here.  Good point.  Done.

https://codereview.chromium.org/138513002/diff/730001/net/http/failing_http_t...
net/http/failing_http_transaction_factory.h:28: // |err| must not be OK.
On 2014/01/28 17:27:23, mmenke wrote:
> Think it's generally a bad idea to mention function arguments in class
> description (And it's no longer "err", for that matter).  Suggest something
> like:  "Creates transactions that always (asynchronously) return a specified
> error.  The error is returned asynchronously, just after the transaction is
> started."

SGTM.  Done.

https://codereview.chromium.org/138513002/diff/730001/net/http/failing_http_t...
net/http/failing_http_transaction_factory.h:40: virtual HttpNetworkSession*
GetSession() OVERRIDE;
On 2014/01/28 17:27:23, mmenke wrote:
> include platform_specific for OVERRIDE.

I think you meant compiler_specific.h, so I included that.  Done.

[mmenke, 2014-01-28 23:10:21]

Your latest patch doesn't look to have been uploaded successfully.  I forgot to
look at the other browsertest, I'll do that tomorrow.  Thanks for catching that
oversight.

https://codereview.chromium.org/138513002/diff/730001/chrome/browser/errorpag...
File chrome/browser/errorpage_browsertest.cc (right):

https://codereview.chromium.org/138513002/diff/730001/chrome/browser/errorpag...
chrome/browser/errorpage_browsertest.cc:426: EXPECT_TRUE(result);
On 2014/01/28 22:52:59, rdsmith wrote:
> On 2014/01/28 17:27:23, mmenke wrote:
> > optional:  It's simpler to do:
> > 
> > ASSERT_TRUE(content::ExecuteScriptAndExtractBool(...));
> > EXPECT_TRUE(result);
> > 
> > Though I suppose that doesn't separate out the cases where the script fails
to
> > run, and where we don't get a bool.  I don't think I've ever seen the first
> > happen before, though.
> 
> So it turns out that ExecuteScriptAndGetValue() and
ExecuteScriptAndExtractBool
> use very different methods to wait for the response to come back from the
> renderer, and if I just do a straight replacement, the test hangs.  I believe
> what's going on is that ExecuteScriptAndExtractBool uses a
DOMOperationObserver
> to wait for the response, and that the other uses of
> ExecuteScriptAndExtractBool() call domAutomationController.send(), which trips
> that observer.  
> 
> I'm willing to shift over to that method if you'd like, and part of me thinks
I
> should (for compatibility with the rest of the file) but having dug into the
two
> pathways a bit, I find myself preferring ExecuteScriptAndGetValue() because it
> doesn't rely on DomAutomationController, only on mechanisms inside
> RenderViewHostImpl (and presumably RenderView).  So I figured I'd report back
> and let you make the call :-}.

My general feeling on this is that ExecuteScriptAndExtractBool is closer to what
we want.  If there's a problem with its implementation, then rather than not use
it, we should modify it to use content::ExecuteScriptAndGetValue behind the
scenes.

In order to use it, you need to replace the "return" statements with
"chrome.send(result)".  Sorry I didn't mention that, I had just assumed they
used the same mechanism.

https://codereview.chromium.org/138513002/diff/730001/net/http/failing_http_t...
File net/http/failing_http_transaction_factory.h (right):

https://codereview.chromium.org/138513002/diff/730001/net/http/failing_http_t...
net/http/failing_http_transaction_factory.h:40: virtual HttpNetworkSession*
GetSession() OVERRIDE;
On 2014/01/28 22:52:59, rdsmith wrote:
> On 2014/01/28 17:27:23, mmenke wrote:
> > include platform_specific for OVERRIDE.
> 
> I think you meant compiler_specific.h, so I included that.  Done.

Gah...compiler_specific was named platform_specific 2 or 3 years ago.  You think
I'd have gotten used to the new name by now.

[mmenke, 2014-01-29 16:14:33]

Just nits

https://codereview.chromium.org/138513002/diff/770001/content/renderer/render...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/138513002/diff/770001/content/renderer/render...
content/renderer/render_view_browsertest.cc:335: }
Is this function needed?

https://codereview.chromium.org/138513002/diff/770001/content/renderer/render...
content/renderer/render_view_browsertest.cc:368: class RenderViewBrowserTest :
public ContentBrowserTest {
optional:  For large files like this, I think it makes future CLs a little
simpler if the test fixture and relevant classes are defined just above the
tests that use them.

https://codereview.chromium.org/138513002/diff/770001/content/renderer/render...
content/renderer/render_view_browsertest.cc:374: // of a test
ContentRendererClient.
nit:  Per style guide, comments should be sentences.  Suggest just adding
"Needed" in front.  For some reason, I'm fine with the class level comment, but
this one just seems weird.

https://codereview.chromium.org/138513002/diff/770001/content/renderer/render...
content/renderer/render_view_browsertest.cc:403: // |*error| will be set to that
value.
Suggest just saying "... in this case, |*error_code| and
|*stale_cache_entry_present| will be updated appropriately."

https://codereview.chromium.org/138513002/diff/770001/content/renderer/render...
content/renderer/render_view_browsertest.cc:439: // need to restore the old
state.
nit: --we

https://codereview.chromium.org/138513002/diff/770001/content/renderer/render...
content/renderer/render_view_browsertest.cc:453: void
BackendClearCache(disk_cache::Backend** backend,
optional:  Could we just make backend a scoped_ptr<disk_cache::Backend*>?

https://codereview.chromium.org/138513002/diff/770001/content/renderer/render...
content/renderer/render_view_browsertest.cc:463: return;
Return not needed.

https://codereview.chromium.org/138513002/diff/770001/content/renderer/render...
content/renderer/render_view_browsertest.cc:484: }  // namespace
I'm assuming the anonymous namespace can't just be extended to the end of the
file.

https://codereview.chromium.org/138513002/diff/770001/content/renderer/render...
content/renderer/render_view_browsertest.cc:2426: // cache.
nit:  "Load cache with entry" -> "Load page" or "Load URL"?

https://codereview.chromium.org/138513002/diff/770001/content/renderer/render...
content/renderer/render_view_browsertest.cc:2449:
ASSERT_TRUE(stale_cache_entry_present);
Should these two be expects instead?  Just thinking that if the error code is
different, knowing the value of stale_cache_entry_present may still be useful.

https://codereview.chromium.org/138513002/diff/770001/content/renderer/render...
content/renderer/render_view_browsertest.cc:2462: stale_cache_entry_present =
true;
optional:  Rather than initialize these here (and above), maybe initialize them
in GetLatestErrorFromRendererClient?

https://codereview.chromium.org/138513002/diff/770001/content/renderer/render...
content/renderer/render_view_browsertest.cc:2466:
ASSERT_FALSE(stale_cache_entry_present);
Should these two be expects instead?

https://codereview.chromium.org/138513002/diff/770001/content/test/content_br...
File content/test/content_browser_test.cc (right):

https://codereview.chromium.org/138513002/diff/770001/content/test/content_br...
content/test/content_browser_test.cc:71: scoped_ptr<ContentRendererClient>(new
ShellContentRendererClient));
optional:  I prefer keeping initialization order constant - I know it shouldn't
matter, but I'd be happier if this were:

if (command_line->HasSwitch(switches::kSingleProcess)) {
  if (!single_process_renderer_client_)
    single_process_renderer_client_.reset(new ShellContentRendererClient);
  SetRendererClientForTesting(single_process_renderer_client_.get());
}

And then SetContentRendererClient just set single_process_renderer_client_ (And
maybe DCHECKed it wasn't already set).

[Randy Smith (Not in Mondays), 2014-01-29 21:14:09]

https://codereview.chromium.org/138513002/diff/730001/chrome/browser/errorpag...
File chrome/browser/errorpage_browsertest.cc (right):

https://codereview.chromium.org/138513002/diff/730001/chrome/browser/errorpag...
chrome/browser/errorpage_browsertest.cc:426: EXPECT_TRUE(result);
On 2014/01/28 23:10:22, mmenke wrote:
> On 2014/01/28 22:52:59, rdsmith wrote:
> > On 2014/01/28 17:27:23, mmenke wrote:
> > > optional:  It's simpler to do:
> > > 
> > > ASSERT_TRUE(content::ExecuteScriptAndExtractBool(...));
> > > EXPECT_TRUE(result);
> > > 
> > > Though I suppose that doesn't separate out the cases where the script
fails
> to
> > > run, and where we don't get a bool.  I don't think I've ever seen the
first
> > > happen before, though.
> > 
> > So it turns out that ExecuteScriptAndGetValue() and
> ExecuteScriptAndExtractBool
> > use very different methods to wait for the response to come back from the
> > renderer, and if I just do a straight replacement, the test hangs.  I
believe
> > what's going on is that ExecuteScriptAndExtractBool uses a
> DOMOperationObserver
> > to wait for the response, and that the other uses of
> > ExecuteScriptAndExtractBool() call domAutomationController.send(), which
trips
> > that observer.  
> > 
> > I'm willing to shift over to that method if you'd like, and part of me
thinks
> I
> > should (for compatibility with the rest of the file) but having dug into the
> two
> > pathways a bit, I find myself preferring ExecuteScriptAndGetValue() because
it
> > doesn't rely on DomAutomationController, only on mechanisms inside
> > RenderViewHostImpl (and presumably RenderView).  So I figured I'd report
back
> > and let you make the call :-}.
> 
> My general feeling on this is that ExecuteScriptAndExtractBool is closer to
what
> we want.  If there's a problem with its implementation, then rather than not
use
> it, we should modify it to use content::ExecuteScriptAndGetValue behind the
> scenes.
> 
> In order to use it, you need to replace the "return" statements with
> "chrome.send(result)".  Sorry I didn't mention that, I had just assumed they
> used the same mechanism.

This makes sense.  Done.  (I used ExecuteScriptAndExtractString, since I really
didn't want to lose the distinction between "variable not there" and one of the
values of the variable.)

https://codereview.chromium.org/138513002/diff/770001/content/renderer/render...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/138513002/diff/770001/content/renderer/render...
content/renderer/render_view_browsertest.cc:335: }
On 2014/01/29 16:14:35, mmenke wrote:
> Is this function needed?

Nope; copy and paste error.  Done.

https://codereview.chromium.org/138513002/diff/770001/content/renderer/render...
content/renderer/render_view_browsertest.cc:368: class RenderViewBrowserTest :
public ContentBrowserTest {
On 2014/01/29 16:14:35, mmenke wrote:
> optional:  For large files like this, I think it makes future CLs a little
> simpler if the test fixture and relevant classes are defined just above the
> tests that use them.

Done.

https://codereview.chromium.org/138513002/diff/770001/content/renderer/render...
content/renderer/render_view_browsertest.cc:374: // of a test
ContentRendererClient.
On 2014/01/29 16:14:35, mmenke wrote:
> nit:  Per style guide, comments should be sentences.  Suggest just adding
> "Needed" in front.  For some reason, I'm fine with the class level comment,
but
> this one just seems weird.

Done.

https://codereview.chromium.org/138513002/diff/770001/content/renderer/render...
content/renderer/render_view_browsertest.cc:403: // |*error| will be set to that
value.
On 2014/01/29 16:14:35, mmenke wrote:
> Suggest just saying "... in this case, |*error_code| and
> |*stale_cache_entry_present| will be updated appropriately."

Done.

https://codereview.chromium.org/138513002/diff/770001/content/renderer/render...
content/renderer/render_view_browsertest.cc:439: // need to restore the old
state.
On 2014/01/29 16:14:35, mmenke wrote:
> nit: --we

Done, but I'll note that when I searched the file for instances of the word
"we", I found several that I hadn't added :-}.

https://codereview.chromium.org/138513002/diff/770001/content/renderer/render...
content/renderer/render_view_browsertest.cc:453: void
BackendClearCache(disk_cache::Backend** backend,
On 2014/01/29 16:14:35, mmenke wrote:
> optional:  Could we just make backend a scoped_ptr<disk_cache::Backend*>?

Done.

https://codereview.chromium.org/138513002/diff/770001/content/renderer/render...
content/renderer/render_view_browsertest.cc:463: return;
On 2014/01/29 16:14:35, mmenke wrote:
> Return not needed.

Done.

https://codereview.chromium.org/138513002/diff/770001/content/renderer/render...
content/renderer/render_view_browsertest.cc:484: }  // namespace
On 2014/01/29 16:14:35, mmenke wrote:
> I'm assuming the anonymous namespace can't just be extended to the end of the
> file.

I've seen both idioms, and have a very small preference for the idiom that
leaves the test in the exposed namespace as a symbol of "these are the meat of
the file".  Having said that, I basically don't care and would be happy to
extend it, but that wasn't the idiom in this file when I got here, so I'm taking
the "leave things the way you found them" attitude.  If the file OWNER wants me
to extend it, I'm perfectly fine with that.

https://codereview.chromium.org/138513002/diff/770001/content/renderer/render...
content/renderer/render_view_browsertest.cc:2426: // cache.
On 2014/01/29 16:14:35, mmenke wrote:
> nit:  "Load cache with entry" -> "Load page" or "Load URL"?

Done.

https://codereview.chromium.org/138513002/diff/770001/content/renderer/render...
content/renderer/render_view_browsertest.cc:2449:
ASSERT_TRUE(stale_cache_entry_present);
On 2014/01/29 16:14:35, mmenke wrote:
> Should these two be expects instead?  Just thinking that if the error code is
> different, knowing the value of stale_cache_entry_present may still be useful.

Done.

https://codereview.chromium.org/138513002/diff/770001/content/renderer/render...
content/renderer/render_view_browsertest.cc:2462: stale_cache_entry_present =
true;
On 2014/01/29 16:14:35, mmenke wrote:
> optional:  Rather than initialize these here (and above), maybe initialize
them
> in GetLatestErrorFromRendererClient?

The problem is that it's not clear what to initialize them to.  The current code
initializes them to values that are different than the ones expected, implicitly
to guard against GetLatestErrorFromRendererClient() not setting the values.  I'd
be basically happy to have the test code rely on
GetLatestErrorFromRendererClient() always either setting the values or returning
false, but that'd either leave me with declaring variables without initializing
them above (which feels a bit icky) or setting them to values that are relevant
(opposite expected) for the first probe but not resetting them for the second
probe.  Given that set of options, I think the current set/reset is best, but if
you have other feelings I'd be interested.

https://codereview.chromium.org/138513002/diff/770001/content/renderer/render...
content/renderer/render_view_browsertest.cc:2466:
ASSERT_FALSE(stale_cache_entry_present);
On 2014/01/29 16:14:35, mmenke wrote:
> Should these two be expects instead?

Done.

https://codereview.chromium.org/138513002/diff/770001/content/test/content_br...
File content/test/content_browser_test.cc (right):

https://codereview.chromium.org/138513002/diff/770001/content/test/content_br...
content/test/content_browser_test.cc:71: scoped_ptr<ContentRendererClient>(new
ShellContentRendererClient));
On 2014/01/29 16:14:35, mmenke wrote:
> optional:  I prefer keeping initialization order constant - I know it
shouldn't
> matter, but I'd be happier if this were:
> 
> if (command_line->HasSwitch(switches::kSingleProcess)) {
>   if (!single_process_renderer_client_)
>     single_process_renderer_client_.reset(new ShellContentRendererClient);
>   SetRendererClientForTesting(single_process_renderer_client_.get());
> }
> 
> And then SetContentRendererClient just set single_process_renderer_client_
(And
> maybe DCHECKed it wasn't already set).

Done with some extra DHCECKs for paranoia.

[Randy Smith (Not in Mondays), 2014-01-29 22:06:37]

Replace Chris (Evans) with Chris (Palmer) for security review.

[Randy Smith (Not in Mondays), 2014-01-29 22:07:54]

On 2014/01/29 22:06:37, rdsmith wrote:
> Replace Chris (Evans) with Chris (Palmer) for security review.

Copying reviewer-specific requests from c#9:

Chris (palmer@): Could you look at resource_messages.h for security review?  I
believe this is pro-forma, since all I'm doing is structurizing and adding a
bool that was blessed by silence (and discussion with Parisa) in the security
review requested in http://crbug.com/308233.

[Randy Smith (Not in Mondays), 2014-01-29 22:17:50]

James (jamesr@): I know you said in the review for
https://codereview.chromium.org/138493002 that you weren't the right person for
navigation, but Adam has now reviewed and approved that CL, so now the webkit/
review for this CL is just making sure it's compatible with 138493002 (which
compilation mostly proves) and deciding if you're comfortable with my adding the
obvious bool to OnCompletedRequest().  Would you be willing to review just that
aspect of this CL?

[mmenke, 2014-01-29 22:18:22]

LGTM

https://codereview.chromium.org/138513002/diff/770001/content/renderer/render...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/138513002/diff/770001/content/renderer/render...
content/renderer/render_view_browsertest.cc:439: // need to restore the old
state.
On 2014/01/29 21:14:11, rdsmith wrote:
> On 2014/01/29 16:14:35, mmenke wrote:
> > nit: --we
> 
> Done, but I'll note that when I searched the file for instances of the word
> "we", I found several that I hadn't added :-}.

Not worth worrying about.  I don't think it's a huge deal (And don't feel too
strongly about it myself), but enough people have complained about me using it
that I figure it's best to try and minimize adding more instances of it.

https://codereview.chromium.org/138513002/diff/840001/chrome/browser/errorpag...
File chrome/browser/errorpage_browsertest.cc (right):

https://codereview.chromium.org/138513002/diff/840001/chrome/browser/errorpag...
chrome/browser/errorpage_browsertest.cc:101: " 
domAutomationController.send('absent');\n"
nit:  +2 indent.

https://codereview.chromium.org/138513002/diff/840001/content/renderer/render...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/138513002/diff/840001/content/renderer/render...
content/renderer/render_view_browsertest.cc:411: // The call completed
synchronously, so it is our job to call the callback.
nit:  --our.  :)

[Randy Smith (Not in Mondays), 2014-01-29 22:50:44]

Thanks!

https://codereview.chromium.org/138513002/diff/840001/chrome/browser/errorpag...
File chrome/browser/errorpage_browsertest.cc (right):

https://codereview.chromium.org/138513002/diff/840001/chrome/browser/errorpag...
chrome/browser/errorpage_browsertest.cc:101: " 
domAutomationController.send('absent');\n"
On 2014/01/29 22:18:24, mmenke wrote:
> nit:  +2 indent.

Done.

https://codereview.chromium.org/138513002/diff/840001/content/renderer/render...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/138513002/diff/840001/content/renderer/render...
content/renderer/render_view_browsertest.cc:411: // The call completed
synchronously, so it is our job to call the callback.
On 2014/01/29 22:18:24, mmenke wrote:
> nit:  --our.  :)

Done.

[jamesr, 2014-01-29 22:54:59]

webkit/ lgtm

[palmer, 2014-01-29 23:22:46]

IPC security LGTM.

https://codereview.chromium.org/138513002/diff/840001/content/common/resource...
File content/common/resource_messages.h (right):

https://codereview.chromium.org/138513002/diff/840001/content/common/resource...
content/common/resource_messages.h:224: // A copy of the data requested exists
in the cache
Nit: Period at the end of the sentence. :)

[jam, 2014-01-30 18:49:25]

https://codereview.chromium.org/138513002/diff/880001/content/renderer/render...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/138513002/diff/880001/content/renderer/render...
content/renderer/render_view_browsertest.cc:2369: SetContentRendererClient(
I think we can avoid the contentbrowsertest changes. You can call
ContentBrowserTest::Setup first, and then call SetRendererClientForTesting
directly here?

[Randy Smith (Not in Mondays), 2014-01-30 19:10:47]

https://codereview.chromium.org/138513002/diff/880001/content/renderer/render...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/138513002/diff/880001/content/renderer/render...
content/renderer/render_view_browsertest.cc:2369: SetContentRendererClient(
On 2014/01/30 18:49:27, jam wrote:
> I think we can avoid the contentbrowsertest changes. You can call
> ContentBrowserTest::Setup first, and then call SetRendererClientForTesting
> directly here?

ContentBrowserTest::SetUp() calls BrowserTestBase::SetUp(), which calls
BrowserMain().  So nothing you do after calling ContentBrowserTest::SetUp()
matters; the test will have run.  

I looked into doing setup/teardown at the beginning and end of the test, but the
constructor for ShellContentRendererClient DCHECKs if something is already
present in g_renderer_client and assigns itself to that value.  If I respect
that (which I'm tempted not too--setting oneself to a global variable inside
ones own constructor?--but it's not my code), that means we can only construct a
single instance of the content renderer client.  So that needs to be constructed
and assigned before ContentBrowserTest::SetUp(), which I think means changes to
ContentBrowserTest::SetUp().  

But if you have an alternative I'm not seeing, I'm up for it.

[jam, 2014-01-30 19:27:16]

https://codereview.chromium.org/138513002/diff/880001/content/renderer/render...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/138513002/diff/880001/content/renderer/render...
content/renderer/render_view_browsertest.cc:2369: SetContentRendererClient(
On 2014/01/30 19:10:47, rdsmith wrote:
> On 2014/01/30 18:49:27, jam wrote:
> > I think we can avoid the contentbrowsertest changes. You can call
> > ContentBrowserTest::Setup first, and then call SetRendererClientForTesting
> > directly here?
> 
> ContentBrowserTest::SetUp() calls BrowserTestBase::SetUp(), which calls
> BrowserMain().  So nothing you do after calling ContentBrowserTest::SetUp()
> matters; the test will have run.  
> 
> I looked into doing setup/teardown at the beginning and end of the test, but
the
> constructor for ShellContentRendererClient DCHECKs if something is already
> present in g_renderer_client and assigns itself to that value.  If I respect
> that (which I'm tempted not too--setting oneself to a global variable inside
> ones own constructor?--but it's not my code), that means we can only construct
a
> single instance of the content renderer client.  So that needs to be
constructed
> and assigned before ContentBrowserTest::SetUp(), which I think means changes
to
> ContentBrowserTest::SetUp().  
> 
> But if you have an alternative I'm not seeing, I'm up for it.

ah I see. How about we change ContentBrowserTest to not create a
ContentRendererClient if it finds ContentClient::get()->renderer() is set
already? that should be a simpler change

[Randy Smith (Not in Mondays), 2014-01-30 22:03:46]

https://codereview.chromium.org/138513002/diff/880001/content/renderer/render...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/138513002/diff/880001/content/renderer/render...
content/renderer/render_view_browsertest.cc:2369: SetContentRendererClient(
On 2014/01/30 19:27:18, jam wrote:
> On 2014/01/30 19:10:47, rdsmith wrote:
> > On 2014/01/30 18:49:27, jam wrote:
> > > I think we can avoid the contentbrowsertest changes. You can call
> > > ContentBrowserTest::Setup first, and then call SetRendererClientForTesting
> > > directly here?
> > 
> > ContentBrowserTest::SetUp() calls BrowserTestBase::SetUp(), which calls
> > BrowserMain().  So nothing you do after calling ContentBrowserTest::SetUp()
> > matters; the test will have run.  
> > 
> > I looked into doing setup/teardown at the beginning and end of the test, but
> the
> > constructor for ShellContentRendererClient DCHECKs if something is already
> > present in g_renderer_client and assigns itself to that value.  If I respect
> > that (which I'm tempted not too--setting oneself to a global variable inside
> > ones own constructor?--but it's not my code), that means we can only
construct
> a
> > single instance of the content renderer client.  So that needs to be
> constructed
> > and assigned before ContentBrowserTest::SetUp(), which I think means changes
> to
> > ContentBrowserTest::SetUp().  
> > 
> > But if you have an alternative I'm not seeing, I'm up for it.
> 
> ah I see. How about we change ContentBrowserTest to not create a
> ContentRendererClient if it finds ContentClient::get()->renderer() is set
> already? that should be a simpler change

So that interface doesn't exist, at least as far as my eyes and compilation
found.  There is GetContentClient(), but that appears to be only defined #if
defined(CONTENT_IMPLEMENTATION), and it looks to me like content_tests.gypi
#undef that define.  So I don't see a way to probe g_client->renderer_ from
content_browser_test.cc.  Am I missing a pathway in?

[Elly Fong-Jones, 2014-01-30 22:37:35]

lgtm

[jam, 2014-02-03 21:39:08]

On 2014/01/30 22:03:46, rdsmith wrote:
>
https://codereview.chromium.org/138513002/diff/880001/content/renderer/render...
> File content/renderer/render_view_browsertest.cc (right):
> 
>
https://codereview.chromium.org/138513002/diff/880001/content/renderer/render...
> content/renderer/render_view_browsertest.cc:2369: SetContentRendererClient(
> On 2014/01/30 19:27:18, jam wrote:
> > On 2014/01/30 19:10:47, rdsmith wrote:
> > > On 2014/01/30 18:49:27, jam wrote:
> > > > I think we can avoid the contentbrowsertest changes. You can call
> > > > ContentBrowserTest::Setup first, and then call
SetRendererClientForTesting
> > > > directly here?
> > > 
> > > ContentBrowserTest::SetUp() calls BrowserTestBase::SetUp(), which calls
> > > BrowserMain().  So nothing you do after calling
ContentBrowserTest::SetUp()
> > > matters; the test will have run.  
> > > 
> > > I looked into doing setup/teardown at the beginning and end of the test,
but
> > the
> > > constructor for ShellContentRendererClient DCHECKs if something is already
> > > present in g_renderer_client and assigns itself to that value.  If I
respect
> > > that (which I'm tempted not too--setting oneself to a global variable
inside
> > > ones own constructor?--but it's not my code), that means we can only
> construct
> > a
> > > single instance of the content renderer client.  So that needs to be
> > constructed
> > > and assigned before ContentBrowserTest::SetUp(), which I think means
changes
> > to
> > > ContentBrowserTest::SetUp().  
> > > 
> > > But if you have an alternative I'm not seeing, I'm up for it.
> > 
> > ah I see. How about we change ContentBrowserTest to not create a
> > ContentRendererClient if it finds ContentClient::get()->renderer() is set
> > already? that should be a simpler change
> 
> So that interface doesn't exist, at least as far as my eyes and compilation
> found.  There is GetContentClient(), but that appears to be only defined #if
> defined(CONTENT_IMPLEMENTATION), and it looks to me like content_tests.gypi
> #undef that define.  So I don't see a way to probe g_client->renderer_ from
> content_browser_test.cc.  Am I missing a pathway in?

you're right, I forgot about that. lgtm

[Randy Smith (Not in Mondays), 2014-02-04 19:03:15]

The CQ bit was checked by rdsmith@chromium.org

[Randy Smith (Not in Mondays), 2014-02-04 19:03:22]

The CQ bit was unchecked by rdsmith@chromium.org

[Randy Smith (Not in Mondays), 2014-02-04 19:59:52]

Thanks, all!

https://codereview.chromium.org/138513002/diff/840001/content/common/resource...
File content/common/resource_messages.h (right):

https://codereview.chromium.org/138513002/diff/840001/content/common/resource...
content/common/resource_messages.h:224: // A copy of the data requested exists
in the cache
On 2014/01/29 23:22:47, Chromium Palmer wrote:
> Nit: Period at the end of the sentence. :)

Done.

[Randy Smith (Not in Mondays), 2014-02-04 20:00:30]

The CQ bit was checked by rdsmith@chromium.org

[commit-bot: I haz the power, 2014-02-04 20:13:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdsmith@chromium.org/138513002/1010001

[commit-bot: I haz the power, 2014-02-04 20:53:56]

Retried try job too often on win_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2014-02-04 20:59:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdsmith@chromium.org/138513002/1010001

[commit-bot: I haz the power, 2014-02-05 00:57:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdsmith@chromium.org/138513002/1010001

[commit-bot: I haz the power, 2014-02-05 05:19:46]

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

[commit-bot: I haz the power, 2014-02-05 05:19:47]

Retried try job too often on win_rel for step(s) telemetry_perf_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Randy Smith (Not in Mondays), 2014-02-05 17:53:59]

The CQ bit was checked by rdsmith@chromium.org

[commit-bot: I haz the power, 2014-02-05 17:59:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdsmith@chromium.org/138513002/1010001

[commit-bot: I haz the power, 2014-02-05 21:40:08]

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

[commit-bot: I haz the power, 2014-02-05 21:40:09]

Retried try job too often on win_rel for step(s) telemetry_perf_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Randy Smith (Not in Mondays), 2014-02-06 22:17:58]

The CQ bit was checked by rdsmith@chromium.org

[commit-bot: I haz the power, 2014-02-06 22:20:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdsmith@chromium.org/138513002/1010001

[commit-bot: I haz the power, 2014-02-06 23:08:40]

Change committed as 249527