Fix --host-rules command switch.

Fix --host-rules command switch.

BUG=78233
TEST=HostRulesTest.TestMap



Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=88568

[willchan no longer on Chromium, 2011-06-07 19:53:29]

Minor nits only. I'm cc'ing Pawel too in case he has an opinion on the test.

http://codereview.chromium.org/7024017/diff/6001/net/http/http_stream_factory...
File net/http/http_stream_factory_impl_job.cc (right):

http://codereview.chromium.org/7024017/diff/6001/net/http/http_stream_factory...
net/http/http_stream_factory_impl_job.cc:445: "url",
request_info_.url.GetOrigin().spec())));
I think that if the host mapping rules get applied, we should add a new NetLog
parameter saying what the modified url is. You'll need to make a new
EventParameters subclass:
http://codesearch.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/net/base/net_....
It's pretty simple, look at
http://codesearch.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/net/base/net_...
for an example. Basically, you need a second string parameter.

http://codereview.chromium.org/7024017/diff/6001/net/http/http_stream_factory...
File net/http/http_stream_factory_impl_job.h (right):

http://codereview.chromium.org/7024017/diff/6001/net/http/http_stream_factory...
net/http/http_stream_factory_impl_job.h:220: // The origin url we're trying to
reach.
Please add a comment noting how this may be modified.

[levin, 2011-06-08 00:37:25]

http://codereview.chromium.org/7024017/diff/6001/net/http/http_stream_factory...
File net/http/http_stream_factory_impl_job.cc (right):

http://codereview.chromium.org/7024017/diff/6001/net/http/http_stream_factory...
net/http/http_stream_factory_impl_job.cc:445: "url",
request_info_.url.GetOrigin().spec())));
On 2011/06/07 19:53:29, willchan wrote:
> I think that if the host mapping rules get applied, we should add a new NetLog
> parameter saying what the modified url is. You'll need to make a new
> EventParameters subclass:
>
http://codesearch.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/net/base/net_....
> It's pretty simple, look at
>
http://codesearch.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/net/base/net_...
> for an example. Basically, you need a second string parameter.

Am I suppose to call net_log_.AddEvent (and create a new event) within
DoResolveProxy? Do I do another Begin/EndEvent call? (that seems unlikely)

Should I remove this call to BeginEvent and move it to DoResolveProxy and add a
string to NetLogStringParameter?

I'm more than willing to do what you want, but I don't know enough about this
are to understand your instructions (even though you did point out some code).
My apologies.

[willchan no longer on Chromium, 2011-06-08 05:27:31]

Feel free to ask for more clarification here.

http://codereview.chromium.org/7024017/diff/6001/net/http/http_stream_factory...
File net/http/http_stream_factory_impl_job.cc (right):

http://codereview.chromium.org/7024017/diff/6001/net/http/http_stream_factory...
net/http/http_stream_factory_impl_job.cc:445: "url",
request_info_.url.GetOrigin().spec())));
On 2011/06/08 00:37:25, levin wrote:
> On 2011/06/07 19:53:29, willchan wrote:
> > I think that if the host mapping rules get applied, we should add a new
NetLog
> > parameter saying what the modified url is. You'll need to make a new
> > EventParameters subclass:
> >
>
http://codesearch.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/net/base/net_....
> > It's pretty simple, look at
> >
>
http://codesearch.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/net/base/net_...
> > for an example. Basically, you need a second string parameter.
> 
> Am I suppose to call net_log_.AddEvent (and create a new event) within
> DoResolveProxy? Do I do another Begin/EndEvent call? (that seems unlikely)
> 
> Should I remove this call to BeginEvent and move it to DoResolveProxy and add
a
> string to NetLogStringParameter?
> 
> I'm more than willing to do what you want, but I don't know enough about this
> are to understand your instructions (even though you did point out some code).
> My apologies.

Hah, no apologies needed. Obviously if you don't understand I didn't explain
well enough, you're not expected to know this code. So, no need to call
BeginEvent() again. This call here is great, except the 2nd parameter should
change. Currently, the second parameter is a NetLogStringParameter which is
logging a string value to the parameter named "url". You want to create a
subclass of NetLog::EventParameter which will log a value to "url" and also
"original_url".

And I think it's fine to move both the setting of |origin_| and |origin_url_| to
StartInternal().

[Paweł Hajdan Jr., 2011-06-08 09:05:54]

The test is good (LGTM). Some minor comments.

http://codereview.chromium.org/7024017/diff/6001/chrome/common/chrome_switche...
File chrome/common/chrome_switches_uitest.cc (right):

http://codereview.chromium.org/7024017/diff/6001/chrome/common/chrome_switche...
chrome/common/chrome_switches_uitest.cc:31: test_server_started_ =
test_server_.Start();
nit: Please add a TODO(phajdan.jr) to change this code when we can ask the test
server whether it started. This is a simple change, but I'd just like to
remember where we need it.

http://codereview.chromium.org/7024017/diff/6001/chrome/common/chrome_switche...
chrome/common/chrome_switches_uitest.cc:44: ASSERT_TRUE(browser.get()) <<
"GetBrowserWindow failed.";
nit: Generally there is no need for those "GetBrowserWindow failed" messages,
here and below.

[levin, 2011-06-08 21:57:57]

Ok, I believe that I have addressed all feedback.

Thanks!

[willchan no longer on Chromium, 2011-06-09 06:02:27]

LGTM

http://codereview.chromium.org/7024017/diff/9004/net/http/http_stream_factory...
File net/http/http_stream_factory_impl_job.cc (right):

http://codereview.chromium.org/7024017/diff/9004/net/http/http_stream_factory...
net/http/http_stream_factory_impl_job.cc:36: static
scoped_refptr<HttpStreamJobParameters> create(
Should be called Create(), not create().

[commit-bot: I haz the power, 2011-06-09 16:47:25]

Try job failure for 7024017-13007 on win for step compile:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2011-06-09 19:22:06]

Change committed as 88568