Add the ability for DevTools to wrap network transactions.

Add the ability for DevTools to wrap network transactions.

The eventual goal is to allow it to simulate flaky network connections.

Docs:
https://docs.google.com/a/google.com/document/d/1SkuWqLYIUpMDI05n7vgx8zdkunyUsxGs9tvUPDOqEJY/edit
https://docs.google.com/a/google.com/document/d/19O_UeUfCFZg5cEnH5jkkZ6_tqsfaLQLYUEIGyXw3oTo/edit

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

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

[szym, 2014-02-27 16:12:36]

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

https://codereview.chromium.org/182993003/diff/1/net/http/http_network_layer....
net/http/http_network_layer.h:55: class TransactionFactory {
This is confusing. HttpNetworkLayer is a HttpTransactionFactory, but a
HttpNetworkLayer::TransactionFactory can override transaction creation.

If you want to control individual transactions, why not swap HttpNetworkLayer in
the HttpCache?

Another idea is to create a dedicated internal HTTP header which instructs the
HttpNetworkTransaction to fail. This is how "Disable cache (while DevTools is
open)" is implemented -- using "Cache-control: no-cache".

[mmenke, 2014-02-27 16:32:32]

On 2014/02/27 16:12:36, szym wrote:
> https://codereview.chromium.org/182993003/diff/1/net/http/http_network_layer.h
> File net/http/http_network_layer.h (right):
> 
>
https://codereview.chromium.org/182993003/diff/1/net/http/http_network_layer....
> net/http/http_network_layer.h:55: class TransactionFactory {
> This is confusing. HttpNetworkLayer is a HttpTransactionFactory, but a
> HttpNetworkLayer::TransactionFactory can override transaction creation.
> 
> If you want to control individual transactions, why not swap HttpNetworkLayer
in
> the HttpCache?
> 
> Another idea is to create a dedicated internal HTTP header which instructs the
> HttpNetworkTransaction to fail. This is how "Disable cache (while DevTools is
> open)" is implemented -- using "Cache-control: no-cache".

I agree that the naming is very confusing, and think adding globals should be
avoided when we reasonably can do so.

I agree with Paul that it would be better to swap the HttpNetworkLayer that the
HttpCache uses.  I think it would be simplest to do this during construction,
rather than mucking with stuff at runtime, and during the normal course of
events, just pass calls through to the real HttpNetworkLayer unmodified.

I'm not a big fan of having an internal header idea.

[eustas, 2014-02-28 09:47:18]

On 2014/02/27 16:12:36, szym wrote:
> https://codereview.chromium.org/182993003/diff/1/net/http/http_network_layer.h
> File net/http/http_network_layer.h (right):
> 
>
https://codereview.chromium.org/182993003/diff/1/net/http/http_network_layer....
> net/http/http_network_layer.h:55: class TransactionFactory {
> This is confusing. HttpNetworkLayer is a HttpTransactionFactory, but a
> HttpNetworkLayer::TransactionFactory can override transaction creation.
> 
> If you want to control individual transactions, why not swap HttpNetworkLayer
in
> the HttpCache?
> 
> Another idea is to create a dedicated internal HTTP header which instructs the
> HttpNetworkTransaction to fail. This is how "Disable cache (while DevTools is
> open)" is implemented -- using "Cache-control: no-cache".

AFAIK we can't replace HttpTransactionFactory, once it is in use.
There is a dozen of places where HttpNetworkLayer is instantiated,
so replacing its instances sounds like hell.

If we could use internal HTTP header we could implement this feature entirely in
Blink.
The problem is that there is plenty of network requests that are issued by
browser and
can't be easily instrumented.

[eustas, 2014-02-28 10:11:21]

On 2014/02/27 16:32:32, mmenke wrote:
> On 2014/02/27 16:12:36, szym wrote:
> >
https://codereview.chromium.org/182993003/diff/1/net/http/http_network_layer.h
> > File net/http/http_network_layer.h (right):
> > 
> >
>
https://codereview.chromium.org/182993003/diff/1/net/http/http_network_layer....
> > net/http/http_network_layer.h:55: class TransactionFactory {
> > This is confusing. HttpNetworkLayer is a HttpTransactionFactory, but a
> > HttpNetworkLayer::TransactionFactory can override transaction creation.
> > 
> > If you want to control individual transactions, why not swap
HttpNetworkLayer
> in
> > the HttpCache?
> > 
> > Another idea is to create a dedicated internal HTTP header which instructs
the
> > HttpNetworkTransaction to fail. This is how "Disable cache (while DevTools
is
> > open)" is implemented -- using "Cache-control: no-cache".
> 
> I agree that the naming is very confusing, and think adding globals should be
> avoided when we reasonably can do so.
> 
> I agree with Paul that it would be better to swap the HttpNetworkLayer that
the
> HttpCache uses.  I think it would be simplest to do this during construction,
> rather than mucking with stuff at runtime, and during the normal course of
> events, just pass calls through to the real HttpNetworkLayer unmodified.
> 
> I'm not a big fan of having an internal header idea.

HttpCache has 3 constructors and dozen and a half instantiations.
I've tried to choose the narrowest point of hack...

We can go this way, but:
1) it will break "lowest overhead" rule: inactive DevTools shouldn't affect
browser
2) it will break scoping: net/ shouldn't depend on content/browser/devtools
3) can we be sure that all network transactions are covered?

[mmenke, 2014-02-28 16:32:05]

On 2014/02/28 10:11:21, eustas wrote:
> On 2014/02/27 16:32:32, mmenke wrote:
> > On 2014/02/27 16:12:36, szym wrote:
> > >
> https://codereview.chromium.org/182993003/diff/1/net/http/http_network_layer.h
> > > File net/http/http_network_layer.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/182993003/diff/1/net/http/http_network_layer....
> > > net/http/http_network_layer.h:55: class TransactionFactory {
> > > This is confusing. HttpNetworkLayer is a HttpTransactionFactory, but a
> > > HttpNetworkLayer::TransactionFactory can override transaction creation.
> > > 
> > > If you want to control individual transactions, why not swap
> HttpNetworkLayer
> > in
> > > the HttpCache?
> > > 
> > > Another idea is to create a dedicated internal HTTP header which instructs
> the
> > > HttpNetworkTransaction to fail. This is how "Disable cache (while DevTools
> is
> > > open)" is implemented -- using "Cache-control: no-cache".
> > 
> > I agree that the naming is very confusing, and think adding globals should
be
> > avoided when we reasonably can do so.
> > 
> > I agree with Paul that it would be better to swap the HttpNetworkLayer that
> the
> > HttpCache uses.  I think it would be simplest to do this during
construction,
> > rather than mucking with stuff at runtime, and during the normal course of
> > events, just pass calls through to the real HttpNetworkLayer unmodified.
> > 
> > I'm not a big fan of having an internal header idea.
> 
> HttpCache has 3 constructors and dozen and a half instantiations.
> I've tried to choose the narrowest point of hack...
> 
> We can go this way, but:
> 1) it will break "lowest overhead" rule: inactive DevTools shouldn't affect
> browser
> 2) it will break scoping: net/ shouldn't depend on content/browser/devtools
> 3) can we be sure that all network transactions are covered?

1)  I prefer to pick based on "lowest mucking with net" overhead, and adding API
bloat and a global seems to be worse, in this respect.  Adding a global is
rarely the right solution.  I'm certainly open to other ideas, but in general,
am not a fan of adding yet another API for hooking into net, when we can avoid
it.
2)  No net/ changes are needed, other than maybe making sure the cache's
3-argument constructor sets up QUIC (Currently looks like it doesn't do this,
which may be a bug, anyways).
3)  The only ones that matter are the ones in profile_io_data_impl and the OTR
equivalent (And maybe the aw_url_request_context_getter.h, if we're targetting
the Android WebView as well).

[mmenke, 2014-03-03 17:03:04]

Removing szym as a reviewer, as he's no longer at Google, unfortunately.

[eustas, 2014-03-05 10:07:53]

Here is the new plumbing that doesn't touch net/

[eustas, 2014-03-05 13:34:10]

jochen@chromium.org: Please review changes in content/public/browser

[mmenke, 2014-03-07 16:40:37]

On 2014/03/05 13:34:10, eustas wrote:
> jochen@chromium.org: Please review changes in content/public/browser

Sorry for the delay - a lot of reviews this week, and this dropped off my radar.
 I'll get to this later today.

[mmenke, 2014-03-07 19:38:08]

https://codereview.chromium.org/182993003/diff/40001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/182993003/diff/40001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:2428:
DevToolsNetworkController::SetBlockedDomains(profile, id, blocked_domains);
Presumably you're going to need to include a header for
DevToolsNetworkController.

https://codereview.chromium.org/182993003/diff/40001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/182993003/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl_io_data.cc:449:
network_layer->GetSession()->net_log(), main_backend);
This constructor doesn't initialize QUIC.  You should probably engage with the
QUIC team to ask how best to go about enabling it when using this constructor. 
Main issue is the QuicServerInfoFactoryAdaptor is owned by the cache, but used
by the session (Which is kinda weird).

https://codereview.chromium.org/182993003/diff/40001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/182993003/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/profile_io_data.cc:1120: void
ProfileIOData::InitializeNetworkTransactionFactory(
Initialize -> Create

https://codereview.chromium.org/182993003/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/profile_io_data.cc:1143: new
DevToolsNetworkController(new net::HttpNetworkSession(params)));
Rather than storing this in a local, this should return a
scoped_ptr<HttpTransactionFactory> (May need it as an unowned class variable as
well).

https://codereview.chromium.org/182993003/diff/40001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_io_data.h (right):

https://codereview.chromium.org/182993003/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/profile_io_data.h:187: }
How's this going to be used?

https://codereview.chromium.org/182993003/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/profile_io_data.h:358: net::HttpTransactionFactory*
network_transaction_factory() const {
Who's going to need access to this?

https://codereview.chromium.org/182993003/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/profile_io_data.h:360:
devtools_network_controller_.get());
static cast not needed, if DevtoolsNetworkController is a child of
HttpTransactionFactory.  Also, who's going to need this?

https://codereview.chromium.org/182993003/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/profile_io_data.h:556: mutable
scoped_ptr<DevToolsNetworkController> devtools_network_controller_;
The HttpCache will take ownership of the HttpTransactionFactory, so this should
not be a scoped_ptr.

https://codereview.chromium.org/182993003/diff/40001/content/public/browser/c...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/182993003/diff/40001/content/public/browser/c...
content/public/browser/content_browser_client.h:533: const
std::vector<std::string>& blocked_domains) {}
Why is this needed?  The DevToolsNetworkController will be in chrome, and the
call to it in content?  Don't suppose you can just put it all on one side of the
divide?

[mmenke, 2014-03-07 19:40:12]

Also, you should update your description.  "Tamper" is an intransitive verb, and
the description should mention devtools.

Maybe something like "Add the ability for devtools to wrap network transactions.
 The eventual goal is to allow it to simulate flaky network connections."

[mmenke, 2014-03-07 19:48:08]

Oh, and a couple other things:

Sorry for being so slow.  If I don't comment on a CL in a day or so, feel free
to ping me.

Also, you're currently passing a set of domains to tamper with.  I'm not sure
this works:  You don't know in advance, what domains a page load will touch.  I
think it'll be simplest to just muck with all connections, so suggest just using
a bool.

The media request context has its own cache, and its own network sessions. 
It'll also need to be wrapped with a devtools factory in the same way.  There's
also an extensions request context - I can't remember if it has its own cache or
not.  I don't think it does, but I'm not positive.

[eustas, 2014-03-11 08:51:49]

On 2014/03/07 19:40:12, mmenke wrote:
> Also, you should update your description.  "Tamper" is an intransitive verb,
and
> the description should mention devtools.
> 
> Maybe something like "Add the ability for devtools to wrap network
transactions.
>  The eventual goal is to allow it to simulate flaky network connections."

Sounds much better =) Thank you.

[eustas, 2014-03-11 12:06:41]

orry for slow response, we've celebrated local holidays in Russia.

Devtools tracks network requests and will propose users list of domains to
block.
This will allow more scenarios (like simulating server-side outage)

Also user could block TLDs.

I'll add instrumentation for media (and extensions) soon.

https://codereview.chromium.org/182993003/diff/40001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/182993003/diff/40001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:2428:
DevToolsNetworkController::SetBlockedDomains(profile, id, blocked_domains);
On 2014/03/07 19:38:08, mmenke wrote:
> Presumably you're going to need to include a header for
> DevToolsNetworkController.

I've tried to reduce review scope in this CL.
I'll add remaining parts of the change.

https://codereview.chromium.org/182993003/diff/40001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/182993003/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/profile_io_data.cc:1120: void
ProfileIOData::InitializeNetworkTransactionFactory(
On 2014/03/07 19:38:08, mmenke wrote:
> Initialize -> Create

Replaced with CreateMainHttpFactory that constructs HttpCache.

https://codereview.chromium.org/182993003/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/profile_io_data.cc:1143: new
DevToolsNetworkController(new net::HttpNetworkSession(params)));
On 2014/03/07 19:38:08, mmenke wrote:
> Rather than storing this in a local, this should return a
> scoped_ptr<HttpTransactionFactory> (May need it as an unowned class variable
as
> well).

Done.

https://codereview.chromium.org/182993003/diff/40001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_io_data.h (right):

https://codereview.chromium.org/182993003/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/profile_io_data.h:187: }
On 2014/03/07 19:38:08, mmenke wrote:
> How's this going to be used?

This will be used to communicate with controller from devtools.

https://codereview.chromium.org/182993003/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/profile_io_data.h:358: net::HttpTransactionFactory*
network_transaction_factory() const {
On 2014/03/07 19:38:08, mmenke wrote:
> Who's going to need access to this?

this is protected method, so it is used by profile & offline profile io data
impls.

Removed.

https://codereview.chromium.org/182993003/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/profile_io_data.h:360:
devtools_network_controller_.get());
On 2014/03/07 19:38:08, mmenke wrote:
> static cast not needed, if DevtoolsNetworkController is a child of
> HttpTransactionFactory.  Also, who's going to need this?

This method was added to encapsulate the nature of constructed transaction
factory. profile_io_data implementations shouldn't know about it.

Removed.

https://codereview.chromium.org/182993003/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/profile_io_data.h:556: mutable
scoped_ptr<DevToolsNetworkController> devtools_network_controller_;
On 2014/03/07 19:38:08, mmenke wrote:
> The HttpCache will take ownership of the HttpTransactionFactory, so this
should
> not be a scoped_ptr.

Oops, didn't notice.

Fixed.

https://codereview.chromium.org/182993003/diff/40001/content/public/browser/c...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/182993003/diff/40001/content/public/browser/c...
content/public/browser/content_browser_client.h:533: const
std::vector<std::string>& blocked_domains) {}
On 2014/03/07 19:38:08, mmenke wrote:
> Why is this needed?  The DevToolsNetworkController will be in chrome, and the
> call to it in content?  Don't suppose you can just put it all on one side of
the
> divide?

DevTools host lives in content, so I don't think we can place all on the one
side =(

[mmenke, 2014-03-11 21:16:52]

Just skimmed over the devtools stuff.

https://codereview.chromium.org/182993003/diff/60001/chrome/browser/devtools/...
File chrome/browser/devtools/devtools_network_controller.cc (right):

https://codereview.chromium.org/182993003/diff/60001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_network_controller.cc:19: , weak_factory_(this)
{
nit:  Comma goes on previous line

https://codereview.chromium.org/182993003/diff/60001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_network_controller.cc:25: void
DevToolsNetworkController::SetBlockedDomains(
nit:  "// static"

https://codereview.chromium.org/182993003/diff/60001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_network_controller.cc:31:
profile->GetResourceContext())->devtools_network_controller();
We really shouldn't be dereferencing IO thread objects on the UI thread.  I
suppose you can't just do all this on the IO thread?

https://codereview.chromium.org/182993003/diff/60001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_network_controller.cc:43: void
DevToolsNetworkController::RemoveTransaction(const std::string& id) {
I think using "id" for both transactions and blocked domains is a bit confusing.
 I suggest calling them transaction_id, and rule_id, or somesuch, for
readability.

https://codereview.chromium.org/182993003/diff/60001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_network_controller.cc:97: return false;
Use two space indent.

https://codereview.chromium.org/182993003/diff/60001/chrome/browser/devtools/...
File chrome/browser/devtools/devtools_network_controller.h (right):

https://codereview.chromium.org/182993003/diff/60001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_network_controller.h:21: } // namespace net
nit:  Two spaces before comment.

https://codereview.chromium.org/182993003/diff/60001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_network_controller.h:25: } // namespace content
nit:  Two spaces before comment.

https://codereview.chromium.org/182993003/diff/60001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_network_controller.h:27: //
DevToolsNetworkController wraps and tracks HttpNetworkTrancactions.
nit:  HttpNetworkTrancactions -> HttpNetworkTransactions

https://codereview.chromium.org/182993003/diff/60001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_network_controller.h:38: const
std::vector<std::string>& blocked_domains);
I am a little concerned that this is working at the domain level.  Seems like we
could very well not know all the domains a page will use, and then miss some. 
Also means we will be affecting other pages in a manner that is unclear.

ServiceWorker may well make things even more confusion, once implemented, since
removes the real domains being requested yet another level from what devtools
knows.  Could consider using render frame IDs, but that has the same issue with
ServiceWorker (And also issues with WebWorkers, and who knows what else).

https://codereview.chromium.org/182993003/diff/60001/chrome/browser/devtools/...
File chrome/browser/devtools/devtools_network_transaction.cc (right):

https://codereview.chromium.org/182993003/diff/60001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_network_transaction.cc:14: const
base::WeakPtr<DevToolsNetworkController>& controller,
This is a weak ptr because devtools is destroyed before objects that own
NetworkTransactions that it wraps?

https://codereview.chromium.org/182993003/diff/60001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_network_transaction.cc:17: ,
id_(base::GenerateGUID())
Do these have to be strings?  Numeric IDs are much shorter...  I would suggest
using NetLog IDs, but two NetworkTransactions can have the same ID (If a
URLRequest is redirected, for instance, both transactions will share a net log
ID)

https://codereview.chromium.org/182993003/diff/60001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_network_transaction.cc:18: ,
proxy_callback_(base::Bind(&DevToolsNetworkTransaction::OnCallback,
Commas go on previous line.

https://codereview.chromium.org/182993003/diff/60001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_network_transaction.cc:20:
network_transaction_.swap(network_transaction);
Can set this in your initializer list: 
network_transaction_(network_transaction.Pass())

https://codereview.chromium.org/182993003/diff/60001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_network_transaction.cc:25:
controller_->RemoveTransaction(GetId());
I think it's much cleaner to have the same class add and remove the transaction,
so I suggest adding the transaction in the constructor, rather than in the
controller.

https://codereview.chromium.org/182993003/diff/60001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_network_transaction.cc:28: std::string
DevToolsNetworkTransaction::GetId() {
const std::string&?

https://codereview.chromium.org/182993003/diff/60001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_network_transaction.cc:135:
net::QuicServerInfo* quic_server_info) {}
Should probably just call into network_transaction_'s equivalent function, for
consistency.

https://codereview.chromium.org/182993003/diff/60001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/182993003/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/profile_io_data.cc:1134: net::HttpCache*
ProfileIOData::CreateMainHttpFactory(
suggest passing this as a scoped_ptr<net::HttpCache>, to make ownership
semantics clear.

https://codereview.chromium.org/182993003/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/profile_io_data.cc:1162: 
nit:  Remove blank line.

https://codereview.chromium.org/182993003/diff/60001/content/renderer/devtool...
File content/renderer/devtools/devtools_agent.h (right):

https://codereview.chromium.org/182993003/diff/60001/content/renderer/devtool...
content/renderer/devtools/devtools_agent.h:60: const
blink::WebVector<blink::WebString>& blocked_domains);
nit:  +2 indent.

https://codereview.chromium.org/182993003/diff/60001/content/renderer/devtool...
content/renderer/devtools/devtools_agent.h:60: const
blink::WebVector<blink::WebString>& blocked_domains);
Should all of these be OVERRIDE, or is that a problem when doing the whole
two-sided Blink/Chrome patch dance?

[eustas, 2014-03-12 15:38:33]

https://codereview.chromium.org/182993003/diff/60001/chrome/browser/devtools/...
File chrome/browser/devtools/devtools_network_controller.cc (right):

https://codereview.chromium.org/182993003/diff/60001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_network_controller.cc:19: , weak_factory_(this)
{
On 2014/03/11 21:16:53, mmenke wrote:
> nit:  Comma goes on previous line

Done.

https://codereview.chromium.org/182993003/diff/60001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_network_controller.cc:25: void
DevToolsNetworkController::SetBlockedDomains(
On 2014/03/11 21:16:53, mmenke wrote:
> nit:  "// static"

Done.

https://codereview.chromium.org/182993003/diff/60001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_network_controller.cc:31:
profile->GetResourceContext())->devtools_network_controller();
On 2014/03/11 21:16:53, mmenke wrote:
> We really shouldn't be dereferencing IO thread objects on the UI thread.  I
> suppose you can't just do all this on the IO thread?

Done.

https://codereview.chromium.org/182993003/diff/60001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_network_controller.cc:43: void
DevToolsNetworkController::RemoveTransaction(const std::string& id) {
On 2014/03/11 21:16:53, mmenke wrote:
> I think using "id" for both transactions and blocked domains is a bit
confusing.
>  I suggest calling them transaction_id, and rule_id, or somesuch, for
> readability.

Done.

https://codereview.chromium.org/182993003/diff/60001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_network_controller.cc:97: return false;
On 2014/03/11 21:16:53, mmenke wrote:
> Use two space indent.

ooops. fixed.

https://codereview.chromium.org/182993003/diff/60001/chrome/browser/devtools/...
File chrome/browser/devtools/devtools_network_controller.h (right):

https://codereview.chromium.org/182993003/diff/60001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_network_controller.h:21: } // namespace net
On 2014/03/11 21:16:53, mmenke wrote:
> nit:  Two spaces before comment.

Done.

https://codereview.chromium.org/182993003/diff/60001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_network_controller.h:25: } // namespace content
On 2014/03/11 21:16:53, mmenke wrote:
> nit:  Two spaces before comment.

Done.

https://codereview.chromium.org/182993003/diff/60001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_network_controller.h:27: //
DevToolsNetworkController wraps and tracks HttpNetworkTrancactions.
On 2014/03/11 21:16:53, mmenke wrote:
> nit:  HttpNetworkTrancactions -> HttpNetworkTransactions

Done.

https://codereview.chromium.org/182993003/diff/60001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_network_controller.h:38: const
std::vector<std::string>& blocked_domains);
On 2014/03/11 21:16:53, mmenke wrote:
> I am a little concerned that this is working at the domain level.  Seems like
we
> could very well not know all the domains a page will use, and then miss some. 
> Also means we will be affecting other pages in a manner that is unclear.
> 
> ServiceWorker may well make things even more confusion, once implemented,
since
> removes the real domains being requested yet another level from what devtools
> knows.  Could consider using render frame IDs, but that has the same issue
with
> ServiceWorker (And also issues with WebWorkers, and who knows what else).

Service Worker and AppCache are the main reasons why we didn't implement
blocking on Blink side. The same reason doesn't allow blocking by renderer id =(

On the first stage we could avoid domains (blacklists) with predefined
whitelists (to allow remote debugging).

https://codereview.chromium.org/182993003/diff/60001/chrome/browser/devtools/...
File chrome/browser/devtools/devtools_network_transaction.cc (right):

https://codereview.chromium.org/182993003/diff/60001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_network_transaction.cc:14: const
base::WeakPtr<DevToolsNetworkController>& controller,
On 2014/03/11 21:16:53, mmenke wrote:
> This is a weak ptr because devtools is destroyed before objects that own
> NetworkTransactions that it wraps?

that was so in 1-st take. removed.

https://codereview.chromium.org/182993003/diff/60001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_network_transaction.cc:17: ,
id_(base::GenerateGUID())
On 2014/03/11 21:16:53, mmenke wrote:
> Do these have to be strings?  Numeric IDs are much shorter...  I would suggest
> using NetLog IDs, but two NetworkTransactions can have the same ID (If a
> URLRequest is redirected, for instance, both transactions will share a net log
> ID)

Done.

https://codereview.chromium.org/182993003/diff/60001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_network_transaction.cc:18: ,
proxy_callback_(base::Bind(&DevToolsNetworkTransaction::OnCallback,
On 2014/03/11 21:16:53, mmenke wrote:
> Commas go on previous line.

Done.

https://codereview.chromium.org/182993003/diff/60001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_network_transaction.cc:20:
network_transaction_.swap(network_transaction);
On 2014/03/11 21:16:53, mmenke wrote:
> Can set this in your initializer list: 
> network_transaction_(network_transaction.Pass())

Cool! thanks.

https://codereview.chromium.org/182993003/diff/60001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_network_transaction.cc:25:
controller_->RemoveTransaction(GetId());
On 2014/03/11 21:16:53, mmenke wrote:
> I think it's much cleaner to have the same class add and remove the
transaction,
> so I suggest adding the transaction in the constructor, rather than in the
> controller.

Done.

https://codereview.chromium.org/182993003/diff/60001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_network_transaction.cc:28: std::string
DevToolsNetworkTransaction::GetId() {
On 2014/03/11 21:16:53, mmenke wrote:
> const std::string&?

removed.

https://codereview.chromium.org/182993003/diff/60001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_network_transaction.cc:135:
net::QuicServerInfo* quic_server_info) {}
On 2014/03/11 21:16:53, mmenke wrote:
> Should probably just call into network_transaction_'s equivalent function, for
> consistency.

oops. didn't notice that one. done.

https://codereview.chromium.org/182993003/diff/60001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/182993003/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/profile_io_data.cc:1134: net::HttpCache*
ProfileIOData::CreateMainHttpFactory(
On 2014/03/11 21:16:53, mmenke wrote:
> suggest passing this as a scoped_ptr<net::HttpCache>, to make ownership
> semantics clear.

Done.

https://codereview.chromium.org/182993003/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/profile_io_data.cc:1162: 
On 2014/03/11 21:16:53, mmenke wrote:
> nit:  Remove blank line.

Done.

https://codereview.chromium.org/182993003/diff/60001/content/renderer/devtool...
File content/renderer/devtools/devtools_agent.h (right):

https://codereview.chromium.org/182993003/diff/60001/content/renderer/devtool...
content/renderer/devtools/devtools_agent.h:60: const
blink::WebVector<blink::WebString>& blocked_domains);
On 2014/03/11 21:16:53, mmenke wrote:
> nit:  +2 indent.

Done.

https://codereview.chromium.org/182993003/diff/60001/content/renderer/devtool...
content/renderer/devtools/devtools_agent.h:60: const
blink::WebVector<blink::WebString>& blocked_domains);
On 2014/03/11 21:16:53, mmenke wrote:
> Should all of these be OVERRIDE, or is that a problem when doing the whole
> two-sided Blink/Chrome patch dance?

Surely they should. I've added todo note.

[eustas, 2014-03-12 15:41:12]

Tomorrow I'm going to split network controller and transaction factory.
This way we will be able to construct multiple factories.

[mmenke, 2014-03-12 15:59:26]

https://codereview.chromium.org/182993003/diff/60001/chrome/browser/devtools/...
File chrome/browser/devtools/devtools_network_controller.h (right):

https://codereview.chromium.org/182993003/diff/60001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_network_controller.h:38: const
std::vector<std::string>& blocked_domains);
On 2014/03/12 15:38:33, eustas wrote:
> On 2014/03/11 21:16:53, mmenke wrote:
> > I am a little concerned that this is working at the domain level.  Seems
like
> we
> > could very well not know all the domains a page will use, and then miss
some. 
> > Also means we will be affecting other pages in a manner that is unclear.
> > 
> > ServiceWorker may well make things even more confusion, once implemented,
> since
> > removes the real domains being requested yet another level from what
devtools
> > knows.  Could consider using render frame IDs, but that has the same issue
> with
> > ServiceWorker (And also issues with WebWorkers, and who knows what else).
> 
> Service Worker and AppCache are the main reasons why we didn't implement
> blocking on Blink side. The same reason doesn't allow blocking by renderer id
=(
> 
> On the first stage we could avoid domains (blacklists) with predefined
> whitelists (to allow remote debugging).

I had completely forgotten about remote debugging.  Whitelists sound reasonable
to me, assuming the machine being debugged is the one acting as the client (If
it's being used as a server, the connection wouldn't be using
HttpNetworkTransactions).

https://codereview.chromium.org/182993003/diff/80001/chrome/browser/net/netwo...
File chrome/browser/net/network_controller.cc (right):

https://codereview.chromium.org/182993003/diff/80001/chrome/browser/net/netwo...
chrome/browser/net/network_controller.cc:34: 
nit:  Remove extra blank line.

https://codereview.chromium.org/182993003/diff/80001/chrome/browser/net/netwo...
chrome/browser/net/network_controller.cc:45: profile,
Actually, should do profile->GetResourceContext() on the UI thread, and
everything else on the IO (Sorry for not being clearer).  Still find it icky to
even acknowledge the existence of the ResourceContext on the UI thread, but
don't see a way around it.

https://codereview.chromium.org/182993003/diff/80001/chrome/browser/net/netwo...
File chrome/browser/net/network_controller.h (right):

https://codereview.chromium.org/182993003/diff/80001/chrome/browser/net/netwo...
chrome/browser/net/network_controller.h:1: // Copyright (c) 2014 The Chromium
Authors. All rights reserved.
nit:  New files shouldn't use the "(c)"

https://codereview.chromium.org/182993003/diff/80001/chrome/browser/net/netwo...
chrome/browser/net/network_controller.h:7: 
nit:  include <vector> and <string>

https://codereview.chromium.org/182993003/diff/80001/chrome/browser/net/netwo...
chrome/browser/net/network_controller.h:19: enum RequestPriority;
Don't forward declare enums.  This is C++11, so not yet allowed.  Include the
header instead.

https://codereview.chromium.org/182993003/diff/80001/chrome/browser/net/netwo...
chrome/browser/net/network_controller.h:19: enum RequestPriority;
Should probably include HttpCache in this list.

https://codereview.chromium.org/182993003/diff/80001/chrome/browser/net/netwo...
chrome/browser/net/network_controller.h:73:
DISALLOW_COPY_AND_ASSIGN(NetworkController);
Should include the headers for this and OVERRIDE (macros.h is the way to get
them both now, I believe).

https://codereview.chromium.org/182993003/diff/80001/chrome/browser/net/netwo...
File chrome/browser/net/network_transaction.h (right):

https://codereview.chromium.org/182993003/diff/80001/chrome/browser/net/netwo...
chrome/browser/net/network_transaction.h:31: class NET_EXPORT NetworkTransaction
: public net::HttpTransaction {
Why are you adding all this stuff to browser/net instead of devtools now?

[eustas, 2014-03-13 13:11:09]

https://codereview.chromium.org/182993003/diff/80001/chrome/browser/net/netwo...
File chrome/browser/net/network_controller.cc (right):

https://codereview.chromium.org/182993003/diff/80001/chrome/browser/net/netwo...
chrome/browser/net/network_controller.cc:34: 
On 2014/03/12 15:59:27, mmenke wrote:
> nit:  Remove extra blank line.

Done.

https://codereview.chromium.org/182993003/diff/80001/chrome/browser/net/netwo...
chrome/browser/net/network_controller.cc:45: profile,
On 2014/03/12 15:59:27, mmenke wrote:
> Actually, should do profile->GetResourceContext() on the UI thread, and
> everything else on the IO (Sorry for not being clearer).  Still find it icky
to
> even acknowledge the existence of the ResourceContext on the UI thread, but
> don't see a way around it.

Done.

https://codereview.chromium.org/182993003/diff/80001/chrome/browser/net/netwo...
File chrome/browser/net/network_controller.h (right):

https://codereview.chromium.org/182993003/diff/80001/chrome/browser/net/netwo...
chrome/browser/net/network_controller.h:1: // Copyright (c) 2014 The Chromium
Authors. All rights reserved.
On 2014/03/12 15:59:27, mmenke wrote:
> nit:  New files shouldn't use the "(c)"

Done.

https://codereview.chromium.org/182993003/diff/80001/chrome/browser/net/netwo...
chrome/browser/net/network_controller.h:7: 
On 2014/03/12 15:59:27, mmenke wrote:
> nit:  include <vector> and <string>

Done.

https://codereview.chromium.org/182993003/diff/80001/chrome/browser/net/netwo...
chrome/browser/net/network_controller.h:19: enum RequestPriority;
On 2014/03/12 15:59:27, mmenke wrote:
> Don't forward declare enums.  This is C++11, so not yet allowed.  Include the
> header instead.

Done.

https://codereview.chromium.org/182993003/diff/80001/chrome/browser/net/netwo...
chrome/browser/net/network_controller.h:19: enum RequestPriority;
On 2014/03/12 15:59:27, mmenke wrote:
> Should probably include HttpCache in this list.

Done.

https://codereview.chromium.org/182993003/diff/80001/chrome/browser/net/netwo...
chrome/browser/net/network_controller.h:73:
DISALLOW_COPY_AND_ASSIGN(NetworkController);
On 2014/03/12 15:59:27, mmenke wrote:
> Should include the headers for this and OVERRIDE (macros.h is the way to get
> them both now, I believe).

Done.

https://codereview.chromium.org/182993003/diff/80001/chrome/browser/net/netwo...
File chrome/browser/net/network_transaction.h (right):

https://codereview.chromium.org/182993003/diff/80001/chrome/browser/net/netwo...
chrome/browser/net/network_transaction.h:31: class NET_EXPORT NetworkTransaction
: public net::HttpTransaction {
On 2014/03/12 15:59:27, mmenke wrote:
> Why are you adding all this stuff to browser/net instead of devtools now?

We (DevTools) hesitate to be the only client of the stack that intercepts all
network requests.

We hope that network controller will be reused by extensions and policies.

[mmenke, 2014-03-13 15:33:33]

Quick comments, I'll do a more thorough review later today.

https://codereview.chromium.org/182993003/diff/80001/chrome/browser/net/netwo...
File chrome/browser/net/network_transaction.h (right):

https://codereview.chromium.org/182993003/diff/80001/chrome/browser/net/netwo...
chrome/browser/net/network_transaction.h:31: class NET_EXPORT NetworkTransaction
: public net::HttpTransaction {
On 2014/03/13 13:11:09, eustas wrote:
> On 2014/03/12 15:59:27, mmenke wrote:
> > Why are you adding all this stuff to browser/net instead of devtools now?
> 
> We (DevTools) hesitate to be the only client of the stack that intercepts all
> network requests.
> 
> We hope that network controller will be reused by extensions and policies.

Generally interception is done at the URLRequestJob layer, or in the
NetworkDelegate/URLRequest::Delegate layers.  I don't believe anything else will
need to intercept at the HttpNetworkTransaction layer.  Or do you know something
I don't?

In general, I prefer not to expand the commonly used interception methods.  I
consider this sort of a one-off need to intercept here.

https://codereview.chromium.org/182993003/diff/90001/chrome/browser/net/netwo...
File chrome/browser/net/network_transaction.cc (right):

https://codereview.chromium.org/182993003/diff/90001/chrome/browser/net/netwo...
chrome/browser/net/network_transaction.cc:23:
NetworkTransaction::NetworkTransaction(
In general, I think it's a really bad idea for two classes to share a name, but
be in different namespaces or have different parent classes.  It makes grepping
through the code much more painful.

[mmenke, 2014-03-13 20:57:40]

This looks pretty reasonable, other than my earlier concerns.

https://codereview.chromium.org/182993003/diff/130001/chrome/browser/net/netw...
File chrome/browser/net/network_transaction.h (right):

https://codereview.chromium.org/182993003/diff/130001/chrome/browser/net/netw...
chrome/browser/net/network_transaction.h:39: const GURL& GetURL();
nit:  const GURL& url() const { return url_; }

[eustas, 2014-03-26 14:06:57]

rtenneti@ please take a look at HttpCache changes.

[eustas, 2014-03-26 14:09:09]

I'm sorry for soooo long intermission. PTAL.

[mmenke, 2014-03-26 14:28:03]

On 2014/03/26 14:09:09, eustas wrote:
> I'm sorry for soooo long intermission. PTAL.

Not a problem.  I have a lot of commitments today, so may not get to it until
tomorrow.

[mmenke, 2014-03-31 18:10:19]

Sorry again for slowness, having issues with a large queue.  I think you're
going to want a fair number of unit tests with DevToolsNetworkTransaction,
either with a fake or a real DevToolsNetworkController.

https://codereview.chromium.org/182993003/diff/150001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_network_transaction.cc (right):

https://codereview.chromium.org/182993003/diff/150001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_transaction.cc:19: }
I don't think we need a global for this - suggest just adding a function
controller_->GenerateTransactionId().  Or are you concerned about multiple
controllers using the same ID?  I don't think that's the issue, since these are
per-profile, and ChromeContentBrowserClient::DisableNetwork looks up the
corresponding profile.

https://codereview.chromium.org/182993003/diff/150001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_transaction.cc:21: }  // namrspace
nit:  r->e

https://codereview.chromium.org/182993003/diff/150001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_transaction.cc:41: }
nit:  This should probably be inlined and named url()

https://codereview.chromium.org/182993003/diff/150001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_transaction.cc:43: void
DevToolsNetworkTransaction::OnCallback(int rv) {
If Stop() has been called, then callback will already be NULL, and so we
shouldn't try to call it.  My suggestion is just to delete the real transaction
in Stop().

Should unit test this case, too.

https://codereview.chromium.org/182993003/diff/150001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_transaction.cc:62: if (controller_ &&
controller_->IsBlockedURL(url_))
"IsBlockedURL" / "Stop" should have similar names, to indicate that they're
related.  I think it would be a bit clearer to rename them something like
"UrlShouldFail" and "Fail".  "Blocked" has other meanings elsewhere (RDH uses it
for requests that are paused until an interestitial completes), and "Stop" is
used when requests are cancelled when they're no longer needed.  Fail is a bit
less ambiguous, though there are lots of potential errors to fail with, of
course.

https://codereview.chromium.org/182993003/diff/150001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_network_transaction.h (right):

https://codereview.chromium.org/182993003/diff/150001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_transaction.h:31: class NET_EXPORT
DevToolsNetworkTransaction : public net::HttpTransaction {
Should more heavily comment this class.

https://codereview.chromium.org/182993003/diff/150001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_network_transaction_factory.cc (right):

https://codereview.chromium.org/182993003/diff/150001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_transaction_factory.cc:11: #include
"net/http/http_network_session.h"
I don't think we need this, since we only work through the NetworkLayer?

https://codereview.chromium.org/182993003/diff/150001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_transaction_factory.cc:33: new
DevToolsNetworkTransaction(controller_, network_transaction));
network_transaction.Pass()?

https://codereview.chromium.org/182993003/diff/150001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_network_transaction_factory.h (right):

https://codereview.chromium.org/182993003/diff/150001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_transaction_factory.h:22: class
DevToolsNetworkTransactionFactory: public net::HttpTransactionFactory {
nit:  Space before colon

https://codereview.chromium.org/182993003/diff/150001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_transaction_factory.h:24: public:
nit:  No blank line before public

https://codereview.chromium.org/182993003/diff/150001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/182993003/diff/150001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:477: main_http_factory_ =
main_cache.PassAs<net::HttpTransactionFactory>();
Think "main_http_factory_.reset(main_cache.release())" is much more common than
this.

https://codereview.chromium.org/182993003/diff/150001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/182993003/diff/150001/content/public/browser/...
content/public/browser/content_browser_client.h:545: // Disables / reenables
network.
Think this needs a comment on what "id" is.

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

https://codereview.chromium.org/182993003/diff/150001/net/http/http_cache.h#n...
net/http/http_cache.h:142: bool setup_netowork_session);
nit:  -o

https://codereview.chromium.org/182993003/diff/150001/net/http/http_cache.h#n...
net/http/http_cache.h:142: bool setup_netowork_session);
Hrm...I find the bool really ugly, but I don't see an alternative.

https://codereview.chromium.org/182993003/diff/150001/net/http/mock_http_cach...
File net/http/mock_http_cache.cc (right):

https://codereview.chromium.org/182993003/diff/150001/net/http/mock_http_cach...
net/http/mock_http_cache.cc:501: false) {
nit:  This should probably line up with the first "new"

[mmenke, 2014-03-31 18:10:57]

Oh, and I think this looks fine - most of these are just nits.

[eustas, 2014-04-18 12:33:18]

https://codereview.chromium.org/182993003/diff/150001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_network_transaction.cc (right):

https://codereview.chromium.org/182993003/diff/150001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_transaction.cc:19: }
On 2014/03/31 18:10:19, mmenke wrote:
> I don't think we need a global for this - suggest just adding a function
> controller_->GenerateTransactionId().  Or are you concerned about multiple
> controllers using the same ID?  I don't think that's the issue, since these
are
> per-profile, and ChromeContentBrowserClient::DisableNetwork looks up the
> corresponding profile.

Done.

https://codereview.chromium.org/182993003/diff/150001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_transaction.cc:21: }  // namrspace
On 2014/03/31 18:10:20, mmenke wrote:
> nit:  r->e

Done.

https://codereview.chromium.org/182993003/diff/150001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_transaction.cc:41: }
On 2014/03/31 18:10:20, mmenke wrote:
> nit:  This should probably be inlined and named url()

Done.

https://codereview.chromium.org/182993003/diff/150001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_transaction.cc:43: void
DevToolsNetworkTransaction::OnCallback(int rv) {
On 2014/03/31 18:10:20, mmenke wrote:
> If Stop() has been called, then callback will already be NULL, and so we
> shouldn't try to call it.  My suggestion is just to delete the real
transaction
> in Stop().
> 
> Should unit test this case, too.

After request is "failed" clients still would require some information from
transaction (load timing info, total received bytes).

I've added guard, so callback will not be invoked after request is "failed".

https://codereview.chromium.org/182993003/diff/150001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_transaction.cc:62: if (controller_ &&
controller_->IsBlockedURL(url_))
On 2014/03/31 18:10:20, mmenke wrote:
> "IsBlockedURL" / "Stop" should have similar names, to indicate that they're
> related.  I think it would be a bit clearer to rename them something like
> "UrlShouldFail" and "Fail".  "Blocked" has other meanings elsewhere (RDH uses
it
> for requests that are paused until an interestitial completes), and "Stop" is
> used when requests are cancelled when they're no longer needed.  Fail is a bit
> less ambiguous, though there are lots of potential errors to fail with, of
> course.

Done.

https://codereview.chromium.org/182993003/diff/150001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_network_transaction.h (right):

https://codereview.chromium.org/182993003/diff/150001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_transaction.h:31: class NET_EXPORT
DevToolsNetworkTransaction : public net::HttpTransaction {
On 2014/03/31 18:10:20, mmenke wrote:
> Should more heavily comment this class.

Done.

https://codereview.chromium.org/182993003/diff/150001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_network_transaction_factory.cc (right):

https://codereview.chromium.org/182993003/diff/150001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_transaction_factory.cc:11: #include
"net/http/http_network_session.h"
On 2014/03/31 18:10:20, mmenke wrote:
> I don't think we need this, since we only work through the NetworkLayer?

Done.

https://codereview.chromium.org/182993003/diff/150001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_transaction_factory.cc:33: new
DevToolsNetworkTransaction(controller_, network_transaction));
On 2014/03/31 18:10:20, mmenke wrote:
> network_transaction.Pass()?

Done.

https://codereview.chromium.org/182993003/diff/150001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_network_transaction_factory.h (right):

https://codereview.chromium.org/182993003/diff/150001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_transaction_factory.h:22: class
DevToolsNetworkTransactionFactory: public net::HttpTransactionFactory {
On 2014/03/31 18:10:20, mmenke wrote:
> nit:  Space before colon

Done.

https://codereview.chromium.org/182993003/diff/150001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_transaction_factory.h:24: public:
On 2014/03/31 18:10:20, mmenke wrote:
> nit:  No blank line before public

Done.

https://codereview.chromium.org/182993003/diff/150001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/182993003/diff/150001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:477: main_http_factory_ =
main_cache.PassAs<net::HttpTransactionFactory>();
On 2014/03/31 18:10:20, mmenke wrote:
> Think "main_http_factory_.reset(main_cache.release())" is much more common
than
> this.

Done.

https://codereview.chromium.org/182993003/diff/150001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/182993003/diff/150001/content/public/browser/...
content/public/browser/content_browser_client.h:545: // Disables / reenables
network.
On 2014/03/31 18:10:20, mmenke wrote:
> Think this needs a comment on what "id" is.

Done.

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

https://codereview.chromium.org/182993003/diff/150001/net/http/http_cache.h#n...
net/http/http_cache.h:142: bool setup_netowork_session);
On 2014/03/31 18:10:20, mmenke wrote:
> nit:  -o

Done.

https://codereview.chromium.org/182993003/diff/150001/net/http/http_cache.h#n...
net/http/http_cache.h:142: bool setup_netowork_session);
On 2014/03/31 18:10:20, mmenke wrote:
> Hrm...I find the bool really ugly, but I don't see an alternative.

Yeah, that looks really ugly =(

https://codereview.chromium.org/182993003/diff/150001/net/http/mock_http_cach...
File net/http/mock_http_cache.cc (right):

https://codereview.chromium.org/182993003/diff/150001/net/http/mock_http_cach...
net/http/mock_http_cache.cc:501: false) {
On 2014/03/31 18:10:20, mmenke wrote:
> nit:  This should probably line up with the first "new"

Done.

[mmenke, 2014-04-18 15:41:42]

https://codereview.chromium.org/182993003/diff/210001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_network_controller.cc (right):

https://codereview.chromium.org/182993003/diff/210001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_controller.cc:30: void
DevToolsNetworkController::RemoveTransaction(uint64 transaction_id) {
Hrm...Are you planning on using the IDs for anything?  Currently don't need
them.  Could just keep the transactions around in a std::set instead.

https://codereview.chromium.org/182993003/diff/210001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_controller.cc:31:
transactions_.erase(transaction_id);
Suggect DCHECKing it was in the list (This won't be true, unless you make the
modifications suggested below).

https://codereview.chromium.org/182993003/diff/210001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_controller.cc:59:
resource_context)->network_controller();
nit:  +2 indent.

https://codereview.chromium.org/182993003/diff/210001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_controller.cc:80: return;
I guess you're thinking about adding different options, and keeping track of
options on a per-client basis?  Otherwise, the following would be much simpler:

if (!disable_network) {
  client_set_.erase(client);
  return;
}

client_set_.insert(client);

https://codereview.chromium.org/182993003/diff/210001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_controller.cc:88: transaction->Fail();
This seems potentially re-entrant - failing a transaction could result in
creating a new one.  Or (theoretically) destroying one, for that matter.

One simple fix is to put together another set of transactions that should fail,
leaving the original list alone.

Then walk through the new list (No need to erase anything from it), and for each
element, check if it's still in the list, and fail it if so (Could check
ShouldFail again, but I don't think that can change from cancelling earlier
requests).

https://codereview.chromium.org/182993003/diff/210001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_controller.cc:96:
CHECK(transaction->started());
Should generally use DCHECKs instead of CHECKs, to avoid bloating up binaries.

https://codereview.chromium.org/182993003/diff/210001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_controller.cc:96:
CHECK(transaction->started());
Also, I think the started check makes more sense as an if(!started) here, and a
DCHECK in the transactions Fail() function.

https://codereview.chromium.org/182993003/diff/210001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_controller.cc:97: if
(clients_set_.size() == 0)
Use empty() instead, for performance reasons.

https://codereview.chromium.org/182993003/diff/210001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_network_controller.h (right):

https://codereview.chromium.org/182993003/diff/210001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_controller.h:40: const std::string&
client_id,
Think you need to document what a client ID is.

https://codereview.chromium.org/182993003/diff/210001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_network_controller_unittest.cc (right):

https://codereview.chromium.org/182993003/diff/210001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_controller_unittest.cc:16: namespace {
nit:  Blank line after namespace, and before end of namespace.

https://codereview.chromium.org/182993003/diff/210001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_controller_unittest.cc:26: private:
nit:  Blank line before private.

https://codereview.chromium.org/182993003/diff/210001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_controller_unittest.cc:76:
scoped_refptr<net::IOBuffer> buffer_;
Include ref_counted.h

https://codereview.chromium.org/182993003/diff/210001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_controller_unittest.cc:78: } //
namespace
nit:  2 spaces between code and comment.

https://codereview.chromium.org/182993003/diff/210001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_controller_unittest.cc:78: } //
namespace
Couldn't this go at the end of the file?

https://codereview.chromium.org/182993003/diff/210001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_network_transaction.cc (right):

https://codereview.chromium.org/182993003/diff/210001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_transaction.cc:41: CHECK(!failed_);
DCHECK(started_)?

https://codereview.chromium.org/182993003/diff/210001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_transaction.cc:41: CHECK(!failed_);
I don't think this is true - if we're waiting for something to call into the
NetworkTransaction, could result in failing it twice, if a client is quickly
added, removed, and added again.

https://codereview.chromium.org/182993003/diff/210001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_transaction.cc:45: if
(!callback.is_null())
optional:  Could move this check up 2 lines, and earlier return if callback is
NULL, rather than moving a NULL callback into a temporary and resetting it. 
Think it results in slightly clearer code.  Any performance differences is
irrelevant, of course.

https://codereview.chromium.org/182993003/diff/210001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_transaction.cc:58: if
(controller_->ShouldFail(this))
failed_ = true?

https://codereview.chromium.org/182993003/diff/210001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_transaction.cc:104: CHECK(!failed_);
This check looks to be wrong - we could be waiting for the next call to Read to
happen when Fail() is called.

This applies to all the checks in this file.  We should have tests for this
case.

https://codereview.chromium.org/182993003/diff/210001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_network_transaction.h (right):

https://codereview.chromium.org/182993003/diff/210001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_transaction.h:31: //
DevToolsNetworkTransaction is a "decorator" for network transaction. All
wrapper is the more commonly used term.

[eustas, 2014-04-21 11:34:58]

https://codereview.chromium.org/182993003/diff/210001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_network_controller.cc (right):

https://codereview.chromium.org/182993003/diff/210001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_controller.cc:30: void
DevToolsNetworkController::RemoveTransaction(uint64 transaction_id) {
GooOn 2014/04/18 15:41:42, mmenke wrote:
> Hrm...Are you planning on using the IDs for anything?  Currently don't need
> them.  Could just keep the transactions around in a std::set instead.

Good point! Done.

https://codereview.chromium.org/182993003/diff/210001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_controller.cc:31:
transactions_.erase(transaction_id);
On 2014/04/18 15:41:42, mmenke wrote:
> Suggect DCHECKing it was in the list (This won't be true, unless you make the
> modifications suggested below).

Done.

https://codereview.chromium.org/182993003/diff/210001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_controller.cc:59:
resource_context)->network_controller();
On 2014/04/18 15:41:42, mmenke wrote:
> nit:  +2 indent.

Done.

https://codereview.chromium.org/182993003/diff/210001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_controller.cc:80: return;
On 2014/04/18 15:41:42, mmenke wrote:
> I guess you're thinking about adding different options, and keeping track of
> options on a per-client basis?  Otherwise, the following would be much
simpler:
> 
> if (!disable_network) {
>   client_set_.erase(client);
>   return;
> }
> 
> client_set_.insert(client);

Initially there was an idea to blacklist requests by URL.

But, I think, you are right; for the first iteration we should use simpler
implementation.

https://codereview.chromium.org/182993003/diff/210001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_controller.cc:88: transaction->Fail();
On 2014/04/18 15:41:42, mmenke wrote:
> This seems potentially re-entrant - failing a transaction could result in
> creating a new one.  Or (theoretically) destroying one, for that matter.
> 
> One simple fix is to put together another set of transactions that should
fail,
> leaving the original list alone.
> 
> Then walk through the new list (No need to erase anything from it), and for
each
> element, check if it's still in the list, and fail it if so (Could check
> ShouldFail again, but I don't think that can change from cancelling earlier
> requests).

Done.

https://codereview.chromium.org/182993003/diff/210001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_controller.cc:96:
CHECK(transaction->started());
On 2014/04/18 15:41:42, mmenke wrote:
> Should generally use DCHECKs instead of CHECKs, to avoid bloating up binaries.

Done.

https://codereview.chromium.org/182993003/diff/210001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_controller.cc:96:
CHECK(transaction->started());
On 2014/04/18 15:41:42, mmenke wrote:
> Also, I think the started check makes more sense as an if(!started) here, and
a
> DCHECK in the transactions Fail() function.

Done. I've used CHECK to avoid building debug binaries =)

https://codereview.chromium.org/182993003/diff/210001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_controller.cc:97: if
(clients_set_.size() == 0)
On 2014/04/18 15:41:42, mmenke wrote:
> Use empty() instead, for performance reasons.

Done.

https://codereview.chromium.org/182993003/diff/210001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_network_controller.h (right):

https://codereview.chromium.org/182993003/diff/210001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_controller.h:40: const std::string&
client_id,
On 2014/04/18 15:41:42, mmenke wrote:
> Think you need to document what a client ID is.

Done.

https://codereview.chromium.org/182993003/diff/210001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_network_controller_unittest.cc (right):

https://codereview.chromium.org/182993003/diff/210001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_controller_unittest.cc:16: namespace {
On 2014/04/18 15:41:42, mmenke wrote:
> nit:  Blank line after namespace, and before end of namespace.

Done.

https://codereview.chromium.org/182993003/diff/210001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_controller_unittest.cc:26: private:
On 2014/04/18 15:41:42, mmenke wrote:
> nit:  Blank line before private.

Done.

https://codereview.chromium.org/182993003/diff/210001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_controller_unittest.cc:76:
scoped_refptr<net::IOBuffer> buffer_;
On 2014/04/18 15:41:42, mmenke wrote:
> Include ref_counted.h

Done.

https://codereview.chromium.org/182993003/diff/210001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_controller_unittest.cc:78: } //
namespace
On 2014/04/18 15:41:42, mmenke wrote:
> nit:  2 spaces between code and comment.

Done.

https://codereview.chromium.org/182993003/diff/210001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_controller_unittest.cc:78: } //
namespace
On 2014/04/18 15:41:42, mmenke wrote:
> Couldn't this go at the end of the file?

I can split definition/declaration, but this won't make a huge difference - only
20 lines out of 60 could be moved. Unfortunately, cpp wants fully defined
classes when we address their members.

https://codereview.chromium.org/182993003/diff/210001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_network_transaction.cc (right):

https://codereview.chromium.org/182993003/diff/210001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_transaction.cc:41: CHECK(!failed_);
On 2014/04/18 15:41:42, mmenke wrote:
> DCHECK(started_)?

Done.

https://codereview.chromium.org/182993003/diff/210001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_transaction.cc:41: CHECK(!failed_);
On 2014/04/18 15:41:42, mmenke wrote:
> I don't think this is true - if we're waiting for something to call into the
> NetworkTransaction, could result in failing it twice, if a client is quickly
> added, removed, and added again.

I've added "if" in controller, so in wouldn't fail already failed transactions.

https://codereview.chromium.org/182993003/diff/210001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_transaction.cc:45: if
(!callback.is_null())
On 2014/04/18 15:41:42, mmenke wrote:
> optional:  Could move this check up 2 lines, and earlier return if callback is
> NULL, rather than moving a NULL callback into a temporary and resetting it. 
> Think it results in slightly clearer code.  Any performance differences is
> irrelevant, of course.

Done.

https://codereview.chromium.org/182993003/diff/210001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_transaction.cc:58: if
(controller_->ShouldFail(this))
On 2014/04/18 15:41:42, mmenke wrote:
> failed_ = true?

Surely. Done.

https://codereview.chromium.org/182993003/diff/210001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_transaction.cc:104: CHECK(!failed_);
On 2014/04/18 15:41:42, mmenke wrote:
> This check looks to be wrong - we could be waiting for the next call to Read
to
> happen when Fail() is called.
> 
> This applies to all the checks in this file.  We should have tests for this
> case.

Done.

https://codereview.chromium.org/182993003/diff/210001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_network_transaction.h (right):

https://codereview.chromium.org/182993003/diff/210001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_transaction.h:31: //
DevToolsNetworkTransaction is a "decorator" for network transaction. All
On 2014/04/18 15:41:42, mmenke wrote:
> wrapper is the more commonly used term.

Done.

[mmenke, 2014-04-21 17:16:50]

https://codereview.chromium.org/182993003/diff/230001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_network_controller.cc (right):

https://codereview.chromium.org/182993003/diff/230001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_controller.cc:71:
std::vector<DevToolsNetworkTransaction*> transactions(transactions_.size());
Suggest calling this old_transactions, for clarity.

https://codereview.chromium.org/182993003/diff/230001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_controller.cc:72:
std::copy(transactions_.begin(), transactions_.end(), transactions.begin());
Is there a reason you're not just using:

std::set<DevToolsNetworkTransaction*> transactions(transactions_);  and walking
through that?

https://codereview.chromium.org/182993003/diff/230001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_controller.cc:75:
DevToolsNetworkTransaction* transaction = *it++;
No need for the extra complexity now - Can just make this a for loop, and always
use *it directly.

https://codereview.chromium.org/182993003/diff/230001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_network_controller.h (right):

https://codereview.chromium.org/182993003/diff/230001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_controller.h:19: class BrowserContext;
This doesn't seem to be used.

https://codereview.chromium.org/182993003/diff/230001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_controller.h:21: }  // namespace
content
optional nit:  I don't believe the close namespace comment is generally used in
forward declarations.

https://codereview.chromium.org/182993003/diff/230001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_controller.h:25: 
nit:  There's generally no linebreak here

https://codereview.chromium.org/182993003/diff/230001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_controller.h:34: // Disables /
reenables network. |id| should be DevToolsAgentHost GUID.
nit:  id -> client_id

https://codereview.chromium.org/182993003/diff/230001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_network_controller_unittest.cc (right):

https://codereview.chromium.org/182993003/diff/230001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_controller_unittest.cc:19: class
TestCallback {
Shouldn't indent code in a namespace.

https://codereview.chromium.org/182993003/diff/230001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_controller_unittest.cc:54: return
transaction_->Start(
nit:  -2 indent

https://codereview.chromium.org/182993003/diff/230001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_controller_unittest.cc:82: }  //
namespace
On 2014/04/21 11:34:59, eustas wrote:
> On 2014/04/18 15:41:42, mmenke wrote:
> > Couldn't this go at the end of the file?
> 
> I can split definition/declaration, but this won't make a huge difference -
only
> 20 lines out of 60 could be moved. Unfortunately, cpp wants fully defined
> classes when we address their members.

Sorry, should have been clearer - can the rest of the file also go in the
anonymous namespace?

Shouldn't be conflicts in names of tests anyways, but there's no need for them
to be in the global namespace, unless they are friends of classes in other
files.

https://codereview.chromium.org/182993003/diff/230001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_controller_unittest.cc:88: std::string
test_id = "42";
These should probably be:

const char kTestId[] = "42";

(Or maybe kTestClientId to be more explicit)

https://codereview.chromium.org/182993003/diff/230001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_controller_unittest.cc:146:
base::RunLoop().RunUntilIdle();
Think this is worth a comment (Just something along the lines of the
HttpTransaction should complete successfully, but the DevToolsTransaction should
ignore it)

https://codereview.chromium.org/182993003/diff/230001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_controller_unittest.cc:155:
TEST(DevToolsNetworkControllerTest, ReadAfterFail) {
This shouldn't happen - should test when a fail with no callback pending
instead.

Should run Start(), run the message loop and make sure you get back success. 
Then shut off the network, then read, and make sure you get
ERR_INTERNET_DISCONNECTED.

https://codereview.chromium.org/182993003/diff/230001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_controller_unittest.cc:167:
TEST(DevToolsNetworkControllerTest, BypassDevToolsRequests) {
nit:  Think this name is confusing.  Maybe AllowsDevToolsRequests?  Or
IgnoresDevToolsRequests?

https://codereview.chromium.org/182993003/diff/230001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_network_transaction.cc (right):

https://codereview.chromium.org/182993003/diff/230001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_transaction.cc:55: url_ =
request_info->url;
Can't we just keep a pointer to request_info, like other classes do?

https://codereview.chromium.org/182993003/diff/230001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_transaction.cc:57:
request_info->extra_headers.HasHeader("X-DevTools-Request-Initiator");
This header should probably be in a shared header.  What requests does devtools
initiate, anyways?  Remote debugging?

https://codereview.chromium.org/182993003/diff/230001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_transaction.cc:136: {
Believe that the way this is generally handled is a line break after the return
type, with the second line not indented.

https://codereview.chromium.org/182993003/diff/230001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_transaction.cc:169: // TODO:
investigate.
Are you planning on investigating before landing this CL?  If you want to land
it with the TODO, should be a little clearer, and use TODO(eustas):  <blah>. 
That's not a guarantee that you'll investigate, but it's a pointer to who knows
about the TODO.

https://codereview.chromium.org/182993003/diff/230001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_network_transaction.h (right):

https://codereview.chromium.org/182993003/diff/230001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_transaction.h:29: } // namespace net
nit:  two spaces before comment or remove comment.

https://codereview.chromium.org/182993003/diff/230001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_transaction.h:40:
scoped_ptr<net::HttpTransaction> network_transaction);
include scoped_ptr.h

https://codereview.chromium.org/182993003/diff/230001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_network_transaction_factory.cc (right):

https://codereview.chromium.org/182993003/diff/230001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_transaction_factory.cc:9: #include
"net/base/request_priority.h"
Nit:  Not needed, since it's in the header.

https://codereview.chromium.org/182993003/diff/230001/content/browser/devtool...
File content/browser/devtools/render_view_devtools_agent_host.cc (right):

https://codereview.chromium.org/182993003/diff/230001/content/browser/devtool...
content/browser/devtools/render_view_devtools_agent_host.cc:170: {
nit:  The open brace should be on the previous line.

[eustas, 2014-04-22 14:23:09]

https://codereview.chromium.org/182993003/diff/230001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_network_controller.cc (right):

https://codereview.chromium.org/182993003/diff/230001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_controller.cc:71:
std::vector<DevToolsNetworkTransaction*> transactions(transactions_.size());
On 2014/04/21 17:16:51, mmenke wrote:
> Suggest calling this old_transactions, for clarity.

Done.

https://codereview.chromium.org/182993003/diff/230001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_controller.cc:72:
std::copy(transactions_.begin(), transactions_.end(), transactions.begin());
On 2014/04/21 17:16:51, mmenke wrote:
> Is there a reason you're not just using:
> 
> std::set<DevToolsNetworkTransaction*> transactions(transactions_);  and
walking
> through that?

No reason, just old java habits =)
Done.

https://codereview.chromium.org/182993003/diff/230001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_controller.cc:75:
DevToolsNetworkTransaction* transaction = *it++;
On 2014/04/21 17:16:51, mmenke wrote:
> No need for the extra complexity now - Can just make this a for loop, and
always
> use *it directly.

Done.

https://codereview.chromium.org/182993003/diff/230001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_network_controller.h (right):

https://codereview.chromium.org/182993003/diff/230001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_controller.h:19: class BrowserContext;
On 2014/04/21 17:16:51, mmenke wrote:
> This doesn't seem to be used.

Done.

https://codereview.chromium.org/182993003/diff/230001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_controller.h:21: }  // namespace
content
On 2014/04/21 17:16:51, mmenke wrote:
> optional nit:  I don't believe the close namespace comment is generally used
in
> forward declarations.

Done.

https://codereview.chromium.org/182993003/diff/230001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_controller.h:25: 
On 2014/04/21 17:16:51, mmenke wrote:
> nit:  There's generally no linebreak here

Done.

https://codereview.chromium.org/182993003/diff/230001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_controller.h:34: // Disables /
reenables network. |id| should be DevToolsAgentHost GUID.
On 2014/04/21 17:16:51, mmenke wrote:
> nit:  id -> client_id

Done.

https://codereview.chromium.org/182993003/diff/230001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_network_controller_unittest.cc (right):

https://codereview.chromium.org/182993003/diff/230001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_controller_unittest.cc:19: class
TestCallback {
On 2014/04/21 17:16:51, mmenke wrote:
> Shouldn't indent code in a namespace.

Done.

https://codereview.chromium.org/182993003/diff/230001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_controller_unittest.cc:54: return
transaction_->Start(
On 2014/04/21 17:16:51, mmenke wrote:
> nit:  -2 indent

Done.

https://codereview.chromium.org/182993003/diff/230001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_controller_unittest.cc:82: }  //
namespace
On 2014/04/21 17:16:51, mmenke wrote:
> On 2014/04/21 11:34:59, eustas wrote:
> > On 2014/04/18 15:41:42, mmenke wrote:
> > > Couldn't this go at the end of the file?
> > 
> > I can split definition/declaration, but this won't make a huge difference -
> only
> > 20 lines out of 60 could be moved. Unfortunately, cpp wants fully defined
> > classes when we address their members.
> 
> Sorry, should have been clearer - can the rest of the file also go in the
> anonymous namespace?
> 
> Shouldn't be conflicts in names of tests anyways, but there's no need for them
> to be in the global namespace, unless they are friends of classes in other
> files.

Done.

https://codereview.chromium.org/182993003/diff/230001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_controller_unittest.cc:88: std::string
test_id = "42";
On 2014/04/21 17:16:51, mmenke wrote:
> These should probably be:
> 
> const char kTestId[] = "42";
> 
> (Or maybe kTestClientId to be more explicit)

Done.

https://codereview.chromium.org/182993003/diff/230001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_controller_unittest.cc:146:
base::RunLoop().RunUntilIdle();
On 2014/04/21 17:16:51, mmenke wrote:
> Think this is worth a comment (Just something along the lines of the
> HttpTransaction should complete successfully, but the DevToolsTransaction
should
> ignore it)

Done.

https://codereview.chromium.org/182993003/diff/230001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_controller_unittest.cc:155:
TEST(DevToolsNetworkControllerTest, ReadAfterFail) {
On 2014/04/21 17:16:51, mmenke wrote:
> This shouldn't happen - should test when a fail with no callback pending
> instead.
> 
> Should run Start(), run the message loop and make sure you get back success. 
> Then shut off the network, then read, and make sure you get
> ERR_INTERNET_DISCONNECTED.

Done.

https://codereview.chromium.org/182993003/diff/230001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_controller_unittest.cc:167:
TEST(DevToolsNetworkControllerTest, BypassDevToolsRequests) {
On 2014/04/21 17:16:51, mmenke wrote:
> nit:  Think this name is confusing.  Maybe AllowsDevToolsRequests?  Or
> IgnoresDevToolsRequests?

Done.

https://codereview.chromium.org/182993003/diff/230001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_network_transaction.cc (right):

https://codereview.chromium.org/182993003/diff/230001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_transaction.cc:55: url_ =
request_info->url;
On 2014/04/21 17:16:51, mmenke wrote:
> Can't we just keep a pointer to request_info, like other classes do?

Good idea. Done.

https://codereview.chromium.org/182993003/diff/230001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_transaction.cc:57:
request_info->extra_headers.HasHeader("X-DevTools-Request-Initiator");
On 2014/04/21 17:16:51, mmenke wrote:
> This header should probably be in a shared header.  What requests does
devtools
> initiate, anyways?  Remote debugging?

Remote debugging is no longer a case.
The main case now is fetching of SourceMaps.

I haven't found any strings in shared headers, so I think there should be
reasons why developers don't add them =(

https://codereview.chromium.org/182993003/diff/230001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_transaction.cc:136: {
On 2014/04/21 17:16:51, mmenke wrote:
> Believe that the way this is generally handled is a line break after the
return
> type, with the second line not indented.

Done.

https://codereview.chromium.org/182993003/diff/230001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_transaction.cc:169: // TODO:
investigate.
On 2014/04/21 17:16:51, mmenke wrote:
> Are you planning on investigating before landing this CL?  If you want to land
> it with the TODO, should be a little clearer, and use TODO(eustas):  <blah>. 
> That's not a guarantee that you'll investigate, but it's a pointer to who
knows
> about the TODO.

Resolved.

https://codereview.chromium.org/182993003/diff/230001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_network_transaction.h (right):

https://codereview.chromium.org/182993003/diff/230001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_transaction.h:29: } // namespace net
On 2014/04/21 17:16:51, mmenke wrote:
> nit:  two spaces before comment or remove comment.

Done.

https://codereview.chromium.org/182993003/diff/230001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_transaction.h:40:
scoped_ptr<net::HttpTransaction> network_transaction);
On 2014/04/21 17:16:51, mmenke wrote:
> include scoped_ptr.h

Done.

https://codereview.chromium.org/182993003/diff/230001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_network_transaction_factory.cc (right):

https://codereview.chromium.org/182993003/diff/230001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_transaction_factory.cc:9: #include
"net/base/request_priority.h"
On 2014/04/21 17:16:51, mmenke wrote:
> Nit:  Not needed, since it's in the header.

Surely. Done.

https://codereview.chromium.org/182993003/diff/230001/content/browser/devtool...
File content/browser/devtools/render_view_devtools_agent_host.cc (right):

https://codereview.chromium.org/182993003/diff/230001/content/browser/devtool...
content/browser/devtools/render_view_devtools_agent_host.cc:170: {
On 2014/04/21 17:16:51, mmenke wrote:
> nit:  The open brace should be on the previous line.

Done.

[mmenke, 2014-04-22 14:37:35]

This LGTM, but I have only skimmed over the content/ changes to hook this up, so
should have someone more familiar with how devtools is generally hooked up
review that (fairly small) part of the CL.

Sorry for all the rounds of review.

https://codereview.chromium.org/182993003/diff/250001/content/browser/devtool...
File content/browser/devtools/render_view_devtools_agent_host.cc (right):

https://codereview.chromium.org/182993003/diff/250001/content/browser/devtool...
content/browser/devtools/render_view_devtools_agent_host.cc:469: bool
disable_network) {
nit:  +2 indent.

[eustas, 2014-04-22 14:51:38]

On 2014/04/22 14:37:35, mmenke wrote:
> This LGTM, but I have only skimmed over the content/ changes to hook this up,
so
> should have someone more familiar with how devtools is generally hooked up
> review that (fairly small) part of the CL.
> 
> Sorry for all the rounds of review.
> 
>
https://codereview.chromium.org/182993003/diff/250001/content/browser/devtool...
> File content/browser/devtools/render_view_devtools_agent_host.cc (right):
> 
>
https://codereview.chromium.org/182993003/diff/250001/content/browser/devtool...
> content/browser/devtools/render_view_devtools_agent_host.cc:469: bool
> disable_network) {
> nit:  +2 indent.

Thank you very much for your reviews!

[eustas, 2014-04-22 14:52:54]

@pfeldman please take a look at DevTools code.

[eustas, 2014-04-23 07:55:48]

https://codereview.chromium.org/182993003/diff/250001/content/browser/devtool...
File content/browser/devtools/render_view_devtools_agent_host.cc (right):

https://codereview.chromium.org/182993003/diff/250001/content/browser/devtool...
content/browser/devtools/render_view_devtools_agent_host.cc:469: bool
disable_network) {
On 2014/04/22 14:37:35, mmenke wrote:
> nit:  +2 indent.

Done.

[eustas, 2014-04-23 08:08:56]

kenrb@ please take a look at  content/common/devtools_messages.h

[jochen (gone - plz use gerrit), 2014-04-23 09:42:06]

https://codereview.chromium.org/182993003/diff/270001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/182993003/diff/270001/content/public/browser/...
content/public/browser/content_browser_client.h:543: virtual void
DisableNetwork(
Can we come up with a more descriptive name?

It's only disabling the network if disable == true, right? And what is the
relation to the RenderViewHost? Is it supposed to only disable networking for
that RVH?

https://codereview.chromium.org/182993003/diff/270001/content/public/browser/...
content/public/browser/content_browser_client.h:545: const std::string& id,
maybe devtools_client_id?

[eustas, 2014-04-23 12:33:06]

https://codereview.chromium.org/182993003/diff/270001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/182993003/diff/270001/content/public/browser/...
content/public/browser/content_browser_client.h:543: virtual void
DisableNetwork(
On 2014/04/23 09:42:07, jochen wrote:
> Can we come up with a more descriptive name?
> 
> It's only disabling the network if disable == true, right? And what is the
> relation to the RenderViewHost? Is it supposed to only disable networking for
> that RVH?

Surely, but I got stuck choosing name...
Here are some thoughts:

EnableNetwork (sounds better, but less intuitive)

EmulateNetworkConnectivityLoss

SetNetworkEmulationConfiguration (and pass struct, that contain User-Agent
overrides, etc.)


Network is disabled for Profile (BrowserContext); network is disabled if there
is at least one client that requested it.

RVH is used to access BrowserContext.

https://codereview.chromium.org/182993003/diff/270001/content/public/browser/...
content/public/browser/content_browser_client.h:545: const std::string& id,
On 2014/04/23 09:42:07, jochen wrote:
> maybe devtools_client_id?

devtools_client_id would be confusing.

For now I'll change it simply to client_id.

I'll change it to devtools_agent_id if you like.

[jochen (gone - plz use gerrit), 2014-04-23 12:39:29]

+jam for content api

https://codereview.chromium.org/182993003/diff/270001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/182993003/diff/270001/content/public/browser/...
content/public/browser/content_browser_client.h:543: virtual void
DisableNetwork(
On 2014/04/23 12:33:08, eustas wrote:
> On 2014/04/23 09:42:07, jochen wrote:
> > Can we come up with a more descriptive name?
> > 
> > It's only disabling the network if disable == true, right? And what is the
> > relation to the RenderViewHost? Is it supposed to only disable networking
for
> > that RVH?
> 
> Surely, but I got stuck choosing name...
> Here are some thoughts:
> 
> EnableNetwork (sounds better, but less intuitive)
> 
> EmulateNetworkConnectivityLoss
> 
> SetNetworkEmulationConfiguration (and pass struct, that contain User-Agent
> overrides, etc.)

I think I like that one.

Or SetNetworkState() or similar if you don't want to go for the struct right
away.

> 
> 
> Network is disabled for Profile (BrowserContext); network is disabled if there
> is at least one client that requested it.
> 
> RVH is used to access BrowserContext.

hum, why isn't the method on BrowserContext then? Or ResourceContext even since
apparently most of the work is done on the io thread anyway?

[pfeldman, 2014-04-23 13:00:27]

https://codereview.chromium.org/182993003/diff/290001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.h (right):

https://codereview.chromium.org/182993003/diff/290001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.h:241: virtual void DisableNetwork(
RequestDisableNetwork(content::RenderViewHost* requester, ...

https://codereview.chromium.org/182993003/diff/290001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_network_controller.cc (right):

https://codereview.chromium.org/182993003/diff/290001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_controller.cc:27:
transactions_.insert(transaction);
DCHECK every method on the IO thread please.

https://codereview.chromium.org/182993003/diff/290001/content/common/devtools...
File content/common/devtools_messages.h (right):

https://codereview.chromium.org/182993003/diff/290001/content/common/devtools...
content/common/devtools_messages.h:121:
IPC_MESSAGE_ROUTED1(DevToolsHostMsg_DisableNetwork,
This one (and the two above), should not be initiated by the renderer.
Otherwise, breaking into the renderer can disable browser network. You should
intercept them on the browser level instead (see RendererOverridesHandler).

[jam, 2014-04-23 19:48:30]

https://codereview.chromium.org/182993003/diff/270001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/182993003/diff/270001/content/public/browser/...
content/public/browser/content_browser_client.h:543: virtual void
DisableNetwork(
On 2014/04/23 12:39:29, jochen wrote:
> On 2014/04/23 12:33:08, eustas wrote:
> > On 2014/04/23 09:42:07, jochen wrote:
> > > Can we come up with a more descriptive name?
> > > 
> > > It's only disabling the network if disable == true, right? And what is the
> > > relation to the RenderViewHost? Is it supposed to only disable networking
> for
> > > that RVH?
> > 
> > Surely, but I got stuck choosing name...
> > Here are some thoughts:
> > 
> > EnableNetwork (sounds better, but less intuitive)
> > 
> > EmulateNetworkConnectivityLoss
> > 
> > SetNetworkEmulationConfiguration (and pass struct, that contain User-Agent
> > overrides, etc.)
> 
> I think I like that one.
> 
> Or SetNetworkState() or similar if you don't want to go for the struct right
> away.
> 
> > 
> > 
> > Network is disabled for Profile (BrowserContext); network is disabled if
there
> > is at least one client that requested it.
> > 
> > RVH is used to access BrowserContext.
> 
> hum, why isn't the method on BrowserContext then? Or ResourceContext even
since
> apparently most of the work is done on the io thread anyway?

+1: ContentBrowserClient is a last resort embedder API, when there's no other
context to call stuff on. we haven't been diligent on ensuring this; but now i'm
trying to in other cls. so please, instead of calling CBC with a BrowserContext
so that chrome translates to a ResourceContext and then calls a method on the IO
thread, just call a ResourceContext method directly from content.

[eustas, 2014-04-29 08:14:41]

https://codereview.chromium.org/182993003/diff/290001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.h (right):

https://codereview.chromium.org/182993003/diff/290001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.h:241: virtual void DisableNetwork(
On 2014/04/23 13:00:28, pfeldman wrote:
> RequestDisableNetwork(content::RenderViewHost* requester, ...

Removed.

https://codereview.chromium.org/182993003/diff/290001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_network_controller.cc (right):

https://codereview.chromium.org/182993003/diff/290001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_controller.cc:27:
transactions_.insert(transaction);
On 2014/04/23 13:00:28, pfeldman wrote:
> DCHECK every method on the IO thread please.

Done. But I've used thread checker to avoid compexity in unit tests.

https://codereview.chromium.org/182993003/diff/290001/content/common/devtools...
File content/common/devtools_messages.h (right):

https://codereview.chromium.org/182993003/diff/290001/content/common/devtools...
content/common/devtools_messages.h:121:
IPC_MESSAGE_ROUTED1(DevToolsHostMsg_DisableNetwork,
On 2014/04/23 13:00:28, pfeldman wrote:
> This one (and the two above), should not be initiated by the renderer.
> Otherwise, breaking into the renderer can disable browser network. You should
> intercept them on the browser level instead (see RendererOverridesHandler).

Done.

[jochen (gone - plz use gerrit), 2014-04-29 13:35:50]

content lgtm

[ramant (doing other things), 2014-04-29 21:52:46]

lgtm

http_cache.h, http_cache.cc changes LGTM

[rvargas (doing something else), 2014-04-29 23:40:44]

I just skimmed through the bug, design docs and parts of this CL and I have a
(maybe dumb) high level question.

I'm a little concerned about having yet another component whose purpose is to
filter and muck with network requests, with a completely new hook mechanism.

Couldn't this be implemented using the current delegation/extension mechanisms?

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

https://codereview.chromium.org/182993003/diff/310001/net/http/http_cache.cc#...
net/http/http_cache.cc:1003: if
(session->params().enable_quic_persist_server_info) {
couldn't we use this condition to avoid having an explicit bool on the
constructor?

https://codereview.chromium.org/182993003/diff/310001/net/http/http_cache.cc#...
net/http/http_cache.cc:1004: DCHECK(!quic_server_info_factory_.get());
nit: shouldn't need get() here

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

https://codereview.chromium.org/182993003/diff/310001/net/http/http_cache.h#n...
net/http/http_cache.h:139: HttpCache(HttpTransactionFactory* network_layer,
This constructor is intended for unit tests but it seems like now it is used
from production code. At the very least the comment is now stale, but more
importantly, it would be nice to not increase the numbers of ways to initialize
this class.

https://codereview.chromium.org/182993003/diff/310001/net/http/http_cache.h#n...
net/http/http_cache.h:142: bool setup_network_session);
It is not clear how a caller should know what to pass here.

https://codereview.chromium.org/182993003/diff/310001/net/http/http_cache.h#n...
net/http/http_cache.h:360: void SetupNetworkSession(HttpNetworkSession*
session);
If what this does is to configure quick server info, it may be better to use a
name that reflects that. SetupNetworkSession is kind of vague and it's not clear
why we'll end up with a valid |session| that has not been setup.

[eustas, 2014-05-07 13:46:58]

On 2014/04/29 23:40:44, rvargas wrote:
> I just skimmed through the bug, design docs and parts of this CL and I have a
> (maybe dumb) high level question.
> 
> I'm a little concerned about having yet another component whose purpose is to
> filter and muck with network requests, with a completely new hook mechanism.
> 
> Couldn't this be implemented using the current delegation/extension
mechanisms?

Disabling network is only the first step.

We would like to add some kind of traffic shaping, latency control, etc.
This doesn't seem to be possible to implement with current mechanisms.

[eustas, 2014-05-07 13:47:16]

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

https://codereview.chromium.org/182993003/diff/310001/net/http/http_cache.cc#...
net/http/http_cache.cc:1003: if
(session->params().enable_quic_persist_server_info) {
On 2014/04/29 23:40:45, rvargas wrote:
> couldn't we use this condition to avoid having an explicit bool on the
> constructor?

Unfortunately, this condition doesn't (and shouldn't) change between HttpCache
instantiations.

I'll add supplimential method to QuicStreamFactory instead.

https://codereview.chromium.org/182993003/diff/310001/net/http/http_cache.cc#...
net/http/http_cache.cc:1004: DCHECK(!quic_server_info_factory_.get());
On 2014/04/29 23:40:45, rvargas wrote:
> nit: shouldn't need get() here

Done.

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

https://codereview.chromium.org/182993003/diff/310001/net/http/http_cache.h#n...
net/http/http_cache.h:139: HttpCache(HttpTransactionFactory* network_layer,
On 2014/04/29 23:40:45, rvargas wrote:
> This constructor is intended for unit tests but it seems like now it is used
> from production code. At the very least the comment is now stale, but more
> importantly, it would be nice to not increase the numbers of ways to
initialize
> this class.

It is possible to remove constructor that takes params and backend.
It is referenced from 5 locations.

Also I can remove all references from tests to this constructor (because tests
can use SetHttpNetworkTransactionFactoryForTesting
to substitute network layer).

Just let me know I'll be glad to address it in separate patch.

https://codereview.chromium.org/182993003/diff/310001/net/http/http_cache.h#n...
net/http/http_cache.h:142: bool setup_network_session);
On 2014/04/29 23:40:45, rvargas wrote:
> It is not clear how a caller should know what to pass here.

Reverted.

https://codereview.chromium.org/182993003/diff/310001/net/http/http_cache.h#n...
net/http/http_cache.h:360: void SetupNetworkSession(HttpNetworkSession*
session);
On 2014/04/29 23:40:45, rvargas wrote:
> If what this does is to configure quick server info, it may be better to use a
> name that reflects that. SetupNetworkSession is kind of vague and it's not
clear
> why we'll end up with a valid |session| that has not been setup.

Done.

[rvargas (doing something else), 2014-05-07 20:22:30]

On 2014/05/07 13:46:58, eustas wrote:
> On 2014/04/29 23:40:44, rvargas wrote:
> > I just skimmed through the bug, design docs and parts of this CL and I have
a
> > (maybe dumb) high level question.
> > 
> > I'm a little concerned about having yet another component whose purpose is
to
> > filter and muck with network requests, with a completely new hook mechanism.
> > 
> > Couldn't this be implemented using the current delegation/extension
> mechanisms?
> 
> Disabling network is only the first step.
> 
> We would like to add some kind of traffic shaping, latency control, etc.
> This doesn't seem to be possible to implement with current mechanisms.

I commented on the design doc (that may be a better place to consider this).

[rvargas (doing something else), 2014-05-07 20:22:56]

Looks better now, thanks.

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

https://codereview.chromium.org/182993003/diff/310001/net/http/http_cache.h#n...
net/http/http_cache.h:139: HttpCache(HttpTransactionFactory* network_layer,
On 2014/05/07 13:47:17, eustas wrote:
> On 2014/04/29 23:40:45, rvargas wrote:
> > This constructor is intended for unit tests but it seems like now it is used
> > from production code. At the very least the comment is now stale, but more
> > importantly, it would be nice to not increase the numbers of ways to
> initialize
> > this class.
> 
> It is possible to remove constructor that takes params and backend.
> It is referenced from 5 locations.
> 
> Also I can remove all references from tests to this constructor (because tests
> can use SetHttpNetworkTransactionFactoryForTesting
> to substitute network layer).
> 
> Just let me know I'll be glad to address it in separate patch.

I don't have an issue with tests using the same constructor that is used by
production code.

What we have today is two constructors that are used from Chrome so I would
prefer to not increase that number.

[jam, 2014-05-09 17:08:14]

btw I'm curious: is there no way for chrome to register its own handlers for
commands? I thought the devtools protocol was layered in a way that an embedder
of content could respond to its own custom commands. because otherwise it seems
odd for content to know about a command regarding networking, but not disable it
itself.

[pfeldman, 2014-05-09 17:29:36]

On 2014/05/09 17:08:14, jam wrote:
> btw I'm curious: is there no way for chrome to register its own handlers for
> commands? I thought the devtools protocol was layered in a way that an
embedder
> of content could respond to its own custom commands. because otherwise it
seems
> odd for content to know about a command regarding networking, but not disable
it
> itself.

No, we don't have such layering (yet). DevTools protocol command hits content
right away; in fact, remote debugging server itself is implemented in content.
And so far it worked well for us since remote debugging was enabled on the
platform level + we didn't have domains that needed to be served outside of
content/.

But what you are saying makes total sense and there is nothing preventing us
from going that way. As a start, we could give embedder the right to process the
command first, while keeping devtools manager and remote debugging
implementation in content/. But I wonder if you want us to go further and move
devtools manager and remote debugging out of content into components or
something. Not sure it makes sense, just thinking out loud.

[pfeldman, 2014-05-09 17:48:30]

https://codereview.chromium.org/251653003/#msg25 mentions a new
DevToolsManagerDelegate that could be used to make embedder dispatch the command
first.

[rvargas (doing something else), 2014-05-12 23:49:57]

net lgtm after renaming the file.

https://codereview.chromium.org/182993003/diff/330001/net/net.gyp
File net/net.gyp (right):

https://codereview.chromium.org/182993003/diff/330001/net/net.gyp#newcode829
net/net.gyp:829: 'http/http_transaction_unittest.cc',
nit: We should rename this file as it doesn't have unit tests and now it doesn't
live in the unit tests target.

[eustas, 2014-05-13 11:39:54]

https://codereview.chromium.org/182993003/diff/330001/net/net.gyp
File net/net.gyp (right):

https://codereview.chromium.org/182993003/diff/330001/net/net.gyp#newcode829
net/net.gyp:829: 'http/http_transaction_unittest.cc',
On 2014/05/12 23:49:58, rvargas wrote:
> nit: We should rename this file as it doesn't have unit tests and now it
doesn't
> live in the unit tests target.

Initially I had the same idea.
But this change touces 14 files.

So, let's do it in distinct patch:
https://codereview.chromium.org/280763003

[jam, 2014-05-13 21:26:55]

On 2014/05/09 17:29:36, pfeldman wrote:
> On 2014/05/09 17:08:14, jam wrote:
> > btw I'm curious: is there no way for chrome to register its own handlers for
> > commands? I thought the devtools protocol was layered in a way that an
> embedder
> > of content could respond to its own custom commands. because otherwise it
> seems
> > odd for content to know about a command regarding networking, but not
disable
> it
> > itself.
> 
> No, we don't have such layering (yet). DevTools protocol command hits content
> right away; in fact, remote debugging server itself is implemented in content.
> And so far it worked well for us since remote debugging was enabled on the
> platform level + we didn't have domains that needed to be served outside of
> content/.
> 
> But what you are saying makes total sense and there is nothing preventing us
> from going that way. As a start, we could give embedder the right to process
the
> command first, while keeping devtools manager and remote debugging
> implementation in content/. But I wonder if you want us to go further and move
> devtools manager and remote debugging out of content into components or
> something. Not sure it makes sense, just thinking out loud.

since DevToolsManagerDelegate landed, can we use that now in this cl?

[eustas, 2014-05-14 14:49:34]

> > As a start, we could give embedder the right to process the
> > command first.
>
> since DevToolsManagerDelegate landed, can we use that now in this cl?

Just to make sure I understood correctly: 
 DevToolsManagerDelegate should become a devtools protocol command handler?

[pfeldman, 2014-05-14 15:50:48]

> Just to make sure I understood correctly: 
>  DevToolsManagerDelegate should become a devtools protocol command handler?

Sort of, let me file a separate bug where we work on it:
https://code.google.com/p/chromium/issues/detail?id=373325

[eustas, 2014-05-19 15:00:45]

On 2014/05/14 15:50:48, pfeldman wrote:
> > Just to make sure I understood correctly: 
> >  DevToolsManagerDelegate should become a devtools protocol command handler?
> 
> Sort of, let me file a separate bug where we work on it:
> https://code.google.com/p/chromium/issues/detail?id=373325

Rebased upon https://codereview.chromium.org/290873002/

I'm going to add constants generation to remove a bit of boilerplate code in
delegate implementation.

[jam, 2014-05-21 16:32:16]

On 2014/05/19 15:00:45, eustas wrote:
> On 2014/05/14 15:50:48, pfeldman wrote:
> > > Just to make sure I understood correctly: 
> > >  DevToolsManagerDelegate should become a devtools protocol command
handler?
> > 
> > Sort of, let me file a separate bug where we work on it:
> > https://code.google.com/p/chromium/issues/detail?id=373325
> 
> Rebased upon https://codereview.chromium.org/290873002/
> 
> I'm going to add constants generation to remove a bit of boilerplate code in
> delegate implementation.

content shouldn't change now right?

[jam, 2014-05-21 16:32:17]

On 2014/05/19 15:00:45, eustas wrote:
> On 2014/05/14 15:50:48, pfeldman wrote:
> > > Just to make sure I understood correctly: 
> > >  DevToolsManagerDelegate should become a devtools protocol command
handler?
> > 
> > Sort of, let me file a separate bug where we work on it:
> > https://code.google.com/p/chromium/issues/detail?id=373325
> 
> Rebased upon https://codereview.chromium.org/290873002/
> 
> I'm going to add constants generation to remove a bit of boilerplate code in
> delegate implementation.

content shouldn't change now right?

[eustas, 2014-05-21 17:44:52]

On 2014/05/21 16:32:17, jam wrote:
> On 2014/05/19 15:00:45, eustas wrote:
> > On 2014/05/14 15:50:48, pfeldman wrote:
> > > > Just to make sure I understood correctly: 
> > > >  DevToolsManagerDelegate should become a devtools protocol command
> handler?
> > > 
> > > Sort of, let me file a separate bug where we work on it:
> > > https://code.google.com/p/chromium/issues/detail?id=373325
> > 
> > Rebased upon https://codereview.chromium.org/290873002/
> > 
> > I'm going to add constants generation to remove a bit of boilerplate code in
> > delegate implementation.
> 
> content shouldn't change now right?

Good point! I'll remove it tomorrow.

[eustas, 2014-05-22 08:33:36]

On 2014/05/21 16:32:17, jam wrote:
> On 2014/05/19 15:00:45, eustas wrote:
> > On 2014/05/14 15:50:48, pfeldman wrote:
> > > > Just to make sure I understood correctly: 
> > > >  DevToolsManagerDelegate should become a devtools protocol command
> handler?
> > > 
> > > Sort of, let me file a separate bug where we work on it:
> > > https://code.google.com/p/chromium/issues/detail?id=373325
> > 
> > Rebased upon https://codereview.chromium.org/290873002/
> > 
> > I'm going to add constants generation to remove a bit of boilerplate code in
> > delegate implementation.
> 
> content shouldn't change now right?

Done. PTAL.

[eustas, 2014-05-28 10:24:57]

The CQ bit was checked by eustas@chromium.org

[commit-bot: I haz the power, 2014-05-28 10:26:00]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/eustas@chromium.org/182993003/390001

[commit-bot: I haz the power, 2014-05-28 12:12:48]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/12974)

[commit-bot: I haz the power, 2014-05-28 12:23:10]

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

[commit-bot: I haz the power, 2014-05-28 12:23:11]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)

[eustas, 2014-05-28 12:32:45]

The CQ bit was checked by eustas@chromium.org

[commit-bot: I haz the power, 2014-05-28 12:33:45]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/eustas@chromium.org/182993003/410001

[eustas, 2014-05-28 12:59:28]

The CQ bit was checked by eustas@chromium.org

[commit-bot: I haz the power, 2014-05-28 13:00:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/eustas@chromium.org/182993003/430001

[eustas, 2014-05-28 13:01:51]

The CQ bit was checked by eustas@chromium.org

[commit-bot: I haz the power, 2014-05-28 13:02:13]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/eustas@chromium.org/182993003/430001

[eustas, 2014-05-28 14:08:44]

The CQ bit was checked by eustas@chromium.org

[commit-bot: I haz the power, 2014-05-28 14:09:07]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/eustas@chromium.org/182993003/450001

[commit-bot: I haz the power, 2014-05-28 18:21:59]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-28 19:43:25]

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

[commit-bot: I haz the power, 2014-05-28 19:43:26]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[eustas, 2014-05-29 06:04:59]

The CQ bit was checked by eustas@chromium.org

[commit-bot: I haz the power, 2014-05-29 06:06:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/eustas@chromium.org/182993003/470001

[commit-bot: I haz the power, 2014-05-29 07:58:27]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-05-29 08:38:02]

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

[commit-bot: I haz the power, 2014-05-29 08:38:03]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)

[eustas, 2014-05-29 09:06:30]

The CQ bit was checked by eustas@chromium.org

[commit-bot: I haz the power, 2014-05-29 09:08:47]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/eustas@chromium.org/182993003/470001

[commit-bot: I haz the power, 2014-05-29 20:52:06]

Change committed as 273584

[eustas, 2014-06-02 08:38:46]

The CQ bit was checked by eustas@chromium.org

[commit-bot: I haz the power, 2014-06-02 08:39:01]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/eustas@chromium.org/182993003/510001

[eustas, 2014-06-02 08:39:29]

The CQ bit was unchecked by eustas@chromium.org

[eustas, 2014-06-02 08:50:54]

The CQ bit was checked by eustas@chromium.org

[commit-bot: I haz the power, 2014-06-02 08:51:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/eustas@chromium.org/182993003/530001

[commit-bot: I haz the power, 2014-06-02 12:53:12]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-06-02 16:03:18]

Change committed as 274253