Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2550433002:
Add GetResponses() and related funcionality (Closed)
|
Created:
4 years ago by Robert Ogden Modified:
4 years ago Reviewers:
sclittle CC:
chromium-reviews, tbansal+watch-data-reduction-proxy_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd GetResponses() and related funcionality
BUG=669956
Committed: https://crrev.com/2d6c4e692dc0aaabf2d5ffd7f362cd8c4a68dab7
Cr-Commit-Position: refs/heads/master@{#436691}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Better comments and doc. Simpler HTTPResponse init. #
Total comments: 4
Patch Set 3 : Fix doc. Remove proxy scheme. #
Messages
Total messages: 15 (6 generated)
Description was changed from ========== Add GetResponses() and related funcionality BUG= ========== to ========== Add GetResponses() and related funcionality BUG=669956 ==========
robertogden@chromium.org changed reviewers: + sclittle@chromium.org
https://codereview.chromium.org/2550433002/diff/1/tools/chrome_proxy/webdrive... File tools/chrome_proxy/webdriver/common.py (right): https://codereview.chromium.org/2550433002/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/common.py:38: # TODO(robertogden) make a logging statement here style nit: You should have a colon after the "TODO(robertogden)" and all comments should be complete sentences. Also, could you clarify what you mean by "make a logging statement here" in the comment? https://codereview.chromium.org/2550433002/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/common.py:194: def GetPerformanceLogs(self, method_filter=r'Network\.responseReceived'): For all of these class and function definitions, you should be following the Google Python style guide, e.g. starting with a single summary line. See the style guide for more details: https://google.github.io/styleguide/pyguide.html?showone=Comments#Comments You can fix the style of the existing code here in a separate CL if you'd like, but eventually it should follow the style guide. https://codereview.chromium.org/2550433002/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/common.py:205: return all_messages nit: This is minor, but if you don't need the entire list of logs at once, you might consider making this function a generator (e.g. using "yield" instead of "return"), so that each yielded value will be iterated over. Same with the GetHTTPResponses() below. https://codereview.chromium.org/2550433002/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/common.py:223: return HTTPResponse(response_headers, request_headers, url, protocol, nit: Instead of making so many local variables, could you just pass in the dict lookups directly to the constructor here? e.g. return HTTPResponse(response_dict['headers'], ...) https://codereview.chromium.org/2550433002/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/common.py:286: return self._protocol == 'https' What do you intent |UsedHTTPS| to be used for? I haven't confirmed this, but I'd guess that it checks if the fetched URL used HTTPS, regardless of whether or not the secure proxy was used. IIRC we have a of couple tests (e.g. HTTPFallbackToDirect or something) that check that the secure or insecure proxy is specifically being used. If those are what you want to use |UsedHTTPS| for, then could you confirm that it does what you want it to do?
https://codereview.chromium.org/2550433002/diff/1/tools/chrome_proxy/webdrive... File tools/chrome_proxy/webdriver/common.py (right): https://codereview.chromium.org/2550433002/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/common.py:38: # TODO(robertogden) make a logging statement here On 2016/12/06 00:09:49, sclittle wrote: > style nit: You should have a colon after the "TODO(robertogden)" and all > comments should be complete sentences. > > Also, could you clarify what you mean by "make a logging statement here" in the > comment? Done. https://codereview.chromium.org/2550433002/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/common.py:194: def GetPerformanceLogs(self, method_filter=r'Network\.responseReceived'): On 2016/12/06 00:09:49, sclittle wrote: > For all of these class and function definitions, you should be following the > Google Python style guide, e.g. starting with a single summary line. > > See the style guide for more details: > https://google.github.io/styleguide/pyguide.html?showone=Comments#Comments > > You can fix the style of the existing code here in a separate CL if you'd like, > but eventually it should follow the style guide. Done. https://codereview.chromium.org/2550433002/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/common.py:205: return all_messages On 2016/12/06 00:09:49, sclittle wrote: > nit: This is minor, but if you don't need the entire list of logs at once, you > might consider making this function a generator (e.g. using "yield" instead of > "return"), so that each yielded value will be iterated over. > > Same with the GetHTTPResponses() below. Thought about that too. Kept it like this so that the len() function would still work, which seems like it could be useful with all the counting and checking we did in Telemetry. https://codereview.chromium.org/2550433002/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/common.py:223: return HTTPResponse(response_headers, request_headers, url, protocol, On 2016/12/06 00:09:49, sclittle wrote: > nit: Instead of making so many local variables, could you just pass in the dict > lookups directly to the constructor here? > > e.g. return HTTPResponse(response_dict['headers'], ...) Done. https://codereview.chromium.org/2550433002/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/common.py:286: return self._protocol == 'https' On 2016/12/06 00:09:49, sclittle wrote: > What do you intent |UsedHTTPS| to be used for? I haven't confirmed this, but I'd > guess that it checks if the fetched URL used HTTPS, regardless of whether or not > the secure proxy was used. > > IIRC we have a of couple tests (e.g. HTTPFallbackToDirect or something) that > check that the secure or insecure proxy is specifically being used. > > If those are what you want to use |UsedHTTPS| for, then could you confirm that > it does what you want it to do? Correct, there are a number of tests where we need to check for HTTPS vs HTTP. and yes, this is working as intended.
https://codereview.chromium.org/2550433002/diff/20001/tools/chrome_proxy/webd... File tools/chrome_proxy/webdriver/common.py (right): https://codereview.chromium.org/2550433002/diff/20001/tools/chrome_proxy/webd... tools/chrome_proxy/webdriver/common.py:24: Parses the given command line arguments. tiny, tiny nit: Thanks for changing the style here! Could you also move the one-line summary up a line, so that it's on the same line as the triple quotes? Same with the other function/class comments. https://google.github.io/styleguide/pyguide.html?showone=Comments#Comments https://codereview.chromium.org/2550433002/diff/20001/tools/chrome_proxy/webd... tools/chrome_proxy/webdriver/common.py:347: return self._protocol == 'https' As per offline discussion, it looks like this returns the protocol of the page, not the protocol that the proxy used. I'd recommend removing this for now and adding support for determining secure proxy vs. insecure proxy in a later CL.
https://codereview.chromium.org/2550433002/diff/20001/tools/chrome_proxy/webd... File tools/chrome_proxy/webdriver/common.py (right): https://codereview.chromium.org/2550433002/diff/20001/tools/chrome_proxy/webd... tools/chrome_proxy/webdriver/common.py:24: Parses the given command line arguments. On 2016/12/06 18:11:30, sclittle wrote: > tiny, tiny nit: Thanks for changing the style here! Could you also move the > one-line summary up a line, so that it's on the same line as the triple quotes? > Same with the other function/class comments. > > https://google.github.io/styleguide/pyguide.html?showone=Comments#Comments Done. https://codereview.chromium.org/2550433002/diff/20001/tools/chrome_proxy/webd... tools/chrome_proxy/webdriver/common.py:347: return self._protocol == 'https' On 2016/12/06 18:11:30, sclittle wrote: > As per offline discussion, it looks like this returns the protocol of the page, > not the protocol that the proxy used. > > I'd recommend removing this for now and adding support for determining secure > proxy vs. insecure proxy in a later CL. Done.
lgtm
The CQ bit was checked by robertogden@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1481053435166510,
"parent_rev": "8e32403fcda513202afd141d97f8d902311100f3", "commit_rev":
"92ddd2b90f5e01ca494d0c19f67577873400436d"}
Message was sent while issue was closed.
Description was changed from ========== Add GetResponses() and related funcionality BUG=669956 ========== to ========== Add GetResponses() and related funcionality BUG=669956 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Add GetResponses() and related funcionality BUG=669956 ========== to ========== Add GetResponses() and related funcionality BUG=669956 Committed: https://crrev.com/2d6c4e692dc0aaabf2d5ffd7f362cd8c4a68dab7 Cr-Commit-Position: refs/heads/master@{#436691} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/2d6c4e692dc0aaabf2d5ffd7f362cd8c4a68dab7 Cr-Commit-Position: refs/heads/master@{#436691} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
