[Findit] Support sending cookies for http requests.

[Findit] Support sending cookies for http requests.

BUG=
R=aarya@google.com, mbarbella@chromium.org

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

[stgao, 2014-08-14 23:41:55]

Hi Abhishek and Martin,

This CL is to support sending cookies to *.googlesource.com.

Thanks,
Shuotao Gao

[aarya, 2014-08-15 05:29:24]

lgtm

https://codereview.chromium.org/465403004/diff/1/tools/findit/https.py
File tools/findit/https.py (right):

https://codereview.chromium.org/465403004/diff/1/tools/findit/https.py#newcode1
tools/findit/https.py:1: # Copyright (c) 2014 The Chromium Authors. All rights
reserved.
Please rename this file to connection_handler or http_handler. https is a pretty
bad name for this file.

https://codereview.chromium.org/465403004/diff/1/tools/findit/https.py#newcod...
tools/findit/https.py:201: if __name__ == '__main__':
Please remove this test __main__ code.

[stgao, 2014-08-15 06:49:49]

https://codereview.chromium.org/465403004/diff/1/tools/findit/https.py
File tools/findit/https.py (right):

https://codereview.chromium.org/465403004/diff/1/tools/findit/https.py#newcode1
tools/findit/https.py:1: # Copyright (c) 2014 The Chromium Authors. All rights
reserved.
On 2014/08/15 05:29:23, aarya wrote:
> Please rename this file to connection_handler or http_handler. https is a
pretty
> bad name for this file.

This is more than connection_handler or http_handler.
Maybe rename to http_client?

https://codereview.chromium.org/465403004/diff/1/tools/findit/https.py#newcod...
tools/findit/https.py:201: if __name__ == '__main__':
On 2014/08/15 05:29:23, aarya wrote:
> Please remove this test __main__ code.

What's the reason to remove this code?
It is for testing.

[aarya, 2014-08-15 07:21:10]

On 2014/08/15 06:49:49, Shuotao wrote:
> https://codereview.chromium.org/465403004/diff/1/tools/findit/https.py
> File tools/findit/https.py (right):
> 
>
https://codereview.chromium.org/465403004/diff/1/tools/findit/https.py#newcode1
> tools/findit/https.py:1: # Copyright (c) 2014 The Chromium Authors. All rights
> reserved.
> On 2014/08/15 05:29:23, aarya wrote:
> > Please rename this file to connection_handler or http_handler. https is a
> pretty
> > bad name for this file.
> 
> This is more than connection_handler or http_handler.
> Maybe rename to http_client?

yes http_client is fine.

> 
>
https://codereview.chromium.org/465403004/diff/1/tools/findit/https.py#newcod...
> tools/findit/https.py:201: if __name__ == '__main__':
> On 2014/08/15 05:29:23, aarya wrote:
> > Please remove this test __main__ code.
> 
> What's the reason to remove this code?
> It is for testing.

We don't put test code in source files. if you need to test it, you can put in
local copy, but don't commit it.

[stgao, 2014-08-15 19:20:16]

Hi Abhishek, Martin,

I've addressed your comments and did some refactoring.
Would you mind another review?

Sorry, touched files are many, but code line change is little.

Thanks,
Shuotao Gao

[Martin Barbella, 2014-08-15 23:00:02]

lgtm

Feel free to commit this as-is. The comments are pretty minor, so they can be
addressed in another CL.

https://codereview.chromium.org/465403004/diff/60001/tools/findit/common/util...
File tools/findit/common/utils.py (right):

https://codereview.chromium.org/465403004/diff/60001/tools/findit/common/util...
tools/findit/common/utils.py:30: # TODO(stgao): return implementation for
appengine when running on appengine.
Are two implementations necessary? Isn't setting COOKIE_FILE good enough for
this to work on app engine without any additional differences?

https://codereview.chromium.org/465403004/diff/60001/tools/findit/crash_utils.py
File tools/findit/crash_utils.py (right):

https://codereview.chromium.org/465403004/diff/60001/tools/findit/crash_utils...
tools/findit/crash_utils.py:86: def NormalizePathLinux(path, parsed_deps):
Is this being called directly at the moment? There should probably be something
like NormalizePath which is used that calls whatever the platform specific
version of the function to do this is. Ideally, this should be implemented in a
way where it doesn't matter what platform we're running on.

https://codereview.chromium.org/465403004/diff/60001/tools/findit/crash_utils...
tools/findit/crash_utils.py:116: return (None, '', '')
Do the callees of this handle this case properly? It might be best to raise an
exception.

https://codereview.chromium.org/465403004/diff/60001/tools/findit/crash_utils...
tools/findit/crash_utils.py:435: 'The CL changes line %s of file %s from stack
%d.' %
If we are planning on using this output directly anywhere, we don't want to say
something like "from stack 2". This should be human-readable.

[stgao, 2014-08-15 23:11:03]

I will leave the code as it is right now.
For commnents on crash_utils.py, I will let Jason to handle them.

https://codereview.chromium.org/465403004/diff/60001/tools/findit/common/util...
File tools/findit/common/utils.py (right):

https://codereview.chromium.org/465403004/diff/60001/tools/findit/common/util...
tools/findit/common/utils.py:30: # TODO(stgao): return implementation for
appengine when running on appengine.
On 2014/08/15 23:00:02, mbarbella wrote:
> Are two implementations necessary? Isn't setting COOKIE_FILE good enough for
> this to work on app engine without any additional differences?

Yes, we need two different implementation.
On appengine, we don't have to sending cookie at all. The app is whitelised.

And we won't use urllib2. We will use url_fetch.

[Martin Barbella, 2014-08-15 23:17:43]

On 2014/08/15 23:11:03, Shuotao wrote:
> I will leave the code as it is right now.
> For commnents on crash_utils.py, I will let Jason to handle them.
> 
>
https://codereview.chromium.org/465403004/diff/60001/tools/findit/common/util...
> File tools/findit/common/utils.py (right):
> 
>
https://codereview.chromium.org/465403004/diff/60001/tools/findit/common/util...
> tools/findit/common/utils.py:30: # TODO(stgao): return implementation for
> appengine when running on appengine.
> On 2014/08/15 23:00:02, mbarbella wrote:
> > Are two implementations necessary? Isn't setting COOKIE_FILE good enough for
> > this to work on app engine without any additional differences?
> 
> Yes, we need two different implementation.
> On appengine, we don't have to sending cookie at all. The app is whitelised.
> 
> And we won't use urllib2. We will use url_fetch.

Ah, that makes sense. "Local" made me think of a user running the script
locally, not running on a bot in compute engine. In that case, maybe consider
changing the name.

[stgao, 2014-08-16 00:32:09]

Committed patchset #5 manually as 290064 (presubmit successful).

[stgao, 2014-08-16 00:33:29]

On 2014/08/15 23:17:43, mbarbella wrote:
> On 2014/08/15 23:11:03, Shuotao wrote:
> > I will leave the code as it is right now.
> > For commnents on crash_utils.py, I will let Jason to handle them.
> > 
> >
>
https://codereview.chromium.org/465403004/diff/60001/tools/findit/common/util...
> > File tools/findit/common/utils.py (right):
> > 
> >
>
https://codereview.chromium.org/465403004/diff/60001/tools/findit/common/util...
> > tools/findit/common/utils.py:30: # TODO(stgao): return implementation for
> > appengine when running on appengine.
> > On 2014/08/15 23:00:02, mbarbella wrote:
> > > Are two implementations necessary? Isn't setting COOKIE_FILE good enough
for
> > > this to work on app engine without any additional differences?
> > 
> > Yes, we need two different implementation.
> > On appengine, we don't have to sending cookie at all. The app is whitelised.
> > 
> > And we won't use urllib2. We will use url_fetch.
> 
> Ah, that makes sense. "Local" made me think of a user running the script
> locally, not running on a bot in compute engine. In that case, maybe consider
> changing the name.

Yes, I tried to think of a better name, but failed to.
Any nameing suggestion?

[Martin Barbella, 2014-08-18 17:53:02]

> Yes, I tried to think of a better name, but failed to.
> Any nameing suggestion?

Maybe HttpClientWithCookie since it would at least be describing the difference.
This still doesn't feel like a great name, but it should be less confusing than
HttpClientLocal.

[stgao, 2014-08-18 17:55:25]

On 2014/08/18 17:53:02, mbarbella wrote:
> > Yes, I tried to think of a better name, but failed to.
> > Any nameing suggestion?
> 
> Maybe HttpClientWithCookie since it would at least be describing the
difference.
> This still doesn't feel like a great name, but it should be less confusing
than
> HttpClientLocal.

Yes, that's better.

But it also includes the verification of certificate for https connection.
Will think about the name and do the change later.