Issue 11416205: Move EnableSpdy from HttpNetworkLayer to IOThread.

Issue 11416205: Move EnableSpdy from HttpNetworkLayer to IOThread. (Closed)

Created:
8 years ago by Ryan Hamilton

Modified:
8 years ago

Reviewers:
Nico, sleevi (please use rsleevi)

CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

More Reviews

Description

Move EnableSpdy from HttpNetworkLayer to IOThread. Add HttpNetworkLayer::ForceAlternateProtocol() BUG=162571 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170373

Patch Set 1 #

Total comments: 12

Patch Set 2 : Fix Comments #

Patch Set 3 : Rebase #

Created: 8 years ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+89 lines, -72 lines)			Patch
M	chrome/browser/chrome_benchmarking_message_filter.cc	View		2 chunks	+1 line, -2 lines	0 comments	Download
M	chrome/browser/io_thread.h	View	1	1 chunk	+17 lines, -0 lines	0 comments	Download
M	chrome/browser/io_thread.cc	View	1 2	2 chunks	+63 lines, -1 line	0 comments	Download
M	net/http/http_network_layer.h	View		1 chunk	+3 lines, -9 lines	0 comments	Download
M	net/http/http_network_layer.cc	View		1 chunk	+5 lines, -60 lines	0 comments	Download

Messages

Total messages: 11 (0 generated)

Expand Messages | Collapse Messages

Ryan Sleevi

Overall, this seems right - command-line parsing should not be in net/ (which it indirectly/effectively ...

8 years ago (2012-11-27 20:06:37 UTC) #2

Ryan Hamilton

You asked these questions, but I don't think I really understand them. 1) Why is ...

8 years ago (2012-11-27 20:52:53 UTC) #3

You asked these questions, but I don't think I really understand them.

1) Why is this necessary (for the associated bug) to move up to chrome/ layer?

I don't know.  What's the alternative?  The HttpNetworkSession is constructed in
IOThread::Init() and that seems to be the place where we're doing command line
parsing.  If we want to move statics to Params, then this seems logical.  But I
don't pretend to understand the theory behind this in any great detail.  Is
there a better options?

2) Are other content/ embedders going to want to use equivalent functionality.
That is, are there common combinations/settings that we should be encouraging be
set holistically (which the string/command-line does), as opposed to requiring
other consumers to determine what exact combination of HttpStreamFactory flags
to set?

What is a "content/ embedder"?  Frankly, none of these options should be used by
anyone ever :>  (With the exception of people attempting to debug SPDY protocool
issues).  In any case, the static methods on HttpStreamFactory will be going
away as part of this effort so I think this question may well be moot?

https://codereview.chromium.org/11416205/diff/1/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/11416205/diff/1/chrome/browser/io_thread.cc#n...
chrome/browser/io_thread.cc:681: static const char kSingleDomain[] =
"single-domain";
On 2012/11/27 20:06:38, Ryan Sleevi wrote:
> You have many more options documented here than in the header. Is this
> intentional (eg: legacy support?) If so, you should clarify this in a
> file-comment, perhaps re-ordering these.
> 
> Otherwise, you should update the header level comment.

No idea what the history here is.  I've updated the comment to reflect the code.

https://codereview.chromium.org/11416205/diff/1/chrome/browser/io_thread.cc#n...
chrome/browser/io_thread.cc:721: LOG(ERROR) << "FORCING SINGLE DOMAIN";
On 2012/11/27 20:06:38, Ryan Sleevi wrote:
> Does this *really* need to be a LOG(ERROR)?

Changed to DLOG(INFO).

https://codereview.chromium.org/11416205/diff/1/chrome/browser/io_thread.cc#n...
chrome/browser/io_thread.cc:727: continue;
On 2012/11/27 20:06:38, Ryan Sleevi wrote:
> So ",foo" is a valid option string?

Actually that appears to crash.  I've fixed a bug in this method as a result. 
It doesn't seem harmful to allow. An extra trailing or leading "," in the
command line argument.  That being said, if you prefer to remove this case I'm
happy to  fold into the case below.

https://codereview.chromium.org/11416205/diff/1/chrome/browser/io_thread.cc#n...
chrome/browser/io_thread.cc:729: LOG(DFATAL) << "Unrecognized spdy option: " <<
option;
On 2012/11/27 20:06:38, Ryan Sleevi wrote:
> Should you just rewrite this as LOG(ERROR)? Trying to understand under what
> situations an end user would ever benefit from the LOG(FATAL) crashing here,
> since it's only enabled for debug

I'm happy to change this, but in my experience with --use-spdy I would much
prefer a crash when I typo the option because then I know right away that it's
not going what I want.  Otherwise, I might perform some testing and only later
realize that I used the wrong command line option and my tests were invalid.

https://codereview.chromium.org/11416205/diff/1/chrome/browser/io_thread.cc#n...
chrome/browser/io_thread.cc:730: }
On 2012/11/27 20:06:38, Ryan Sleevi wrote:
> It strikes me as odd that "off,ssl,no-ssl" is a perfectly valid string here.

Yes, this is a very strange piece of code.

https://codereview.chromium.org/11416205/diff/1/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

https://codereview.chromium.org/11416205/diff/1/chrome/browser/io_thread.h#ne...
chrome/browser/io_thread.h:174: // Without calling this function, SPDY is
disabled.  The mode can be:
On 2012/11/27 20:06:38, Ryan Sleevi wrote:
> This is... an odd default.
> 
> Why is SPDY disabled by default?

That comment appears to be incorrect.  I've revamped it.

Ryan Sleevi

On 2012/11/27 20:52:53, Ryan Hamilton wrote: > You asked these questions, but I don't think ...

8 years ago (2012-11-27 22:11:23 UTC) #4

On 2012/11/27 20:52:53, Ryan Hamilton wrote:
> You asked these questions, but I don't think I really understand them.
> 
> 1) Why is this necessary (for the associated bug) to move up to chrome/ layer?
> 
> I don't know.  What's the alternative?  The HttpNetworkSession is constructed
in
> IOThread::Init() and that seems to be the place where we're doing command line
> parsing.  If we want to move statics to Params, then this seems logical.  But
I
> don't pretend to understand the theory behind this in any great detail.  Is
> there a better options?

I don't know, I was more trying to find out the rationale. I think it's fine to
move into chrome/, but I wanted to make sure I understood the motivations.

> 
> 2) Are other content/ embedders going to want to use equivalent functionality.
> That is, are there common combinations/settings that we should be encouraging
be
> set holistically (which the string/command-line does), as opposed to requiring
> other consumers to determine what exact combination of HttpStreamFactory flags
> to set?
> 
> What is a "content/ embedder"?  Frankly, none of these options should be used
by
> anyone ever :>  (With the exception of people attempting to debug SPDY
protocool
> issues).  In any case, the static methods on HttpStreamFactory will be going
> away as part of this effort so I think this question may well be moot?

content-embedder = "user of the content/, but not chrome/"
eg:
(Official) Chrome for iOS. Chrome for Android. "Others"
(third-party) Chromium embedded framework.

These clients can already adjust the flags, by virtue of being responsible for
creating the IOThread that sets up the HttpStreamFactor(y/ies). However, if
these command-line flags are a better abstraction than the static methods (eg:
they logically group a collection of settings), then you might want to make the
individual settings private, and make the "collections" public as part of some
API (either an enum or encapsulation methods, like you do with
ForceAlternateProtocol), and just have the command-line parsing convert. If
they're not, or if it's a "to each their own", then I think this approach is
fine, but I wanted to make sure it was considered.

> 
> https://codereview.chromium.org/11416205/diff/1/chrome/browser/io_thread.cc
> File chrome/browser/io_thread.cc (right):
> 
>
https://codereview.chromium.org/11416205/diff/1/chrome/browser/io_thread.cc#n...
> chrome/browser/io_thread.cc:681: static const char kSingleDomain[] =
> "single-domain";
> On 2012/11/27 20:06:38, Ryan Sleevi wrote:
> > You have many more options documented here than in the header. Is this
> > intentional (eg: legacy support?) If so, you should clarify this in a
> > file-comment, perhaps re-ordering these.
> > 
> > Otherwise, you should update the header level comment.
> 
> No idea what the history here is.  I've updated the comment to reflect the
code.
> 
>
https://codereview.chromium.org/11416205/diff/1/chrome/browser/io_thread.cc#n...
> chrome/browser/io_thread.cc:721: LOG(ERROR) << "FORCING SINGLE DOMAIN";
> On 2012/11/27 20:06:38, Ryan Sleevi wrote:
> > Does this *really* need to be a LOG(ERROR)?
> 
> Changed to DLOG(INFO).
> 
>
https://codereview.chromium.org/11416205/diff/1/chrome/browser/io_thread.cc#n...
> chrome/browser/io_thread.cc:727: continue;
> On 2012/11/27 20:06:38, Ryan Sleevi wrote:
> > So ",foo" is a valid option string?
> 
> Actually that appears to crash.  I've fixed a bug in this method as a result. 
> It doesn't seem harmful to allow. An extra trailing or leading "," in the
> command line argument.  That being said, if you prefer to remove this case I'm
> happy to  fold into the case below.
> 
>
https://codereview.chromium.org/11416205/diff/1/chrome/browser/io_thread.cc#n...
> chrome/browser/io_thread.cc:729: LOG(DFATAL) << "Unrecognized spdy option: "
<<
> option;
> On 2012/11/27 20:06:38, Ryan Sleevi wrote:
> > Should you just rewrite this as LOG(ERROR)? Trying to understand under what
> > situations an end user would ever benefit from the LOG(FATAL) crashing here,
> > since it's only enabled for debug
> 
> I'm happy to change this, but in my experience with --use-spdy I would much
> prefer a crash when I typo the option because then I know right away that it's
> not going what I want.  Otherwise, I might perform some testing and only later
> realize that I used the wrong command line option and my tests were invalid.
> 
>
https://codereview.chromium.org/11416205/diff/1/chrome/browser/io_thread.cc#n...
> chrome/browser/io_thread.cc:730: }
> On 2012/11/27 20:06:38, Ryan Sleevi wrote:
> > It strikes me as odd that "off,ssl,no-ssl" is a perfectly valid string here.
> 
> Yes, this is a very strange piece of code.
> 
> https://codereview.chromium.org/11416205/diff/1/chrome/browser/io_thread.h
> File chrome/browser/io_thread.h (right):
> 
>
https://codereview.chromium.org/11416205/diff/1/chrome/browser/io_thread.h#ne...
> chrome/browser/io_thread.h:174: // Without calling this function, SPDY is
> disabled.  The mode can be:
> On 2012/11/27 20:06:38, Ryan Sleevi wrote:
> > This is... an odd default.
> > 
> > Why is SPDY disabled by default?
> 
> That comment appears to be incorrect.  I've revamped it.

Ryan Hamilton

On 2012/11/27 22:11:23, Ryan Sleevi wrote: > On 2012/11/27 20:52:53, Ryan Hamilton wrote: > > ...

8 years ago (2012-11-28 00:57:43 UTC) #6

On 2012/11/27 22:11:23, Ryan Sleevi wrote:
> On 2012/11/27 20:52:53, Ryan Hamilton wrote:
> > You asked these questions, but I don't think I really understand them.
> > 
> > 1) Why is this necessary (for the associated bug) to move up to chrome/
layer?
> > 
> > I don't know.  What's the alternative?  The HttpNetworkSession is
constructed
> in
> > IOThread::Init() and that seems to be the place where we're doing command
line
> > parsing.  If we want to move statics to Params, then this seems logical. 
But
> I
> > don't pretend to understand the theory behind this in any great detail.  Is
> > there a better options?
> 
> I don't know, I was more trying to find out the rationale. I think it's fine
to
> move into chrome/, but I wanted to make sure I understood the motivations.
> 
> > 
> > 2) Are other content/ embedders going to want to use equivalent
functionality.
> > That is, are there common combinations/settings that we should be
encouraging
> be
> > set holistically (which the string/command-line does), as opposed to
requiring
> > other consumers to determine what exact combination of HttpStreamFactory
flags
> > to set?
> > 
> > What is a "content/ embedder"?  Frankly, none of these options should be
used
> by
> > anyone ever :>  (With the exception of people attempting to debug SPDY
> protocool
> > issues).  In any case, the static methods on HttpStreamFactory will be going
> > away as part of this effort so I think this question may well be moot?
> 
> content-embedder = "user of the content/, but not chrome/"
> eg:
> (Official) Chrome for iOS. Chrome for Android. "Others"
> (third-party) Chromium embedded framework.
> 
> These clients can already adjust the flags, by virtue of being responsible for
> creating the IOThread that sets up the HttpStreamFactor(y/ies). However, if
> these command-line flags are a better abstraction than the static methods (eg:
> they logically group a collection of settings), then you might want to make
the
> individual settings private, and make the "collections" public as part of some
> API (either an enum or encapsulation methods, like you do with
> ForceAlternateProtocol), and just have the command-line parsing convert. If
> they're not, or if it's a "to each their own", then I think this approach is
> fine, but I wanted to make sure it was considered.

Ah. I see.  I think that these static methods should disappear completely.  This
CL is part of that effort.  I think that none of these command line argument
should ever be set by an embedder, since they are all truly for debugging.  But
your point is well taken.  As I work through this, if I find any that seem like
they make sense to be used externally, I will bundle them up in a public API.

Cool?

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rch@chromium.org/11416205/12

8 years ago (2012-11-29 20:13:43 UTC) #9

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rch@chromium.org/11416205/12

8 years ago (2012-11-30 02:06:56 UTC) #10

Message was sent while issue was closed.

Change committed as 170373

Expand Messages | Collapse Messages