LoFi integration test should use HasChromeProxyLoFi

LoFi integration test should use HasChromeProxyLoFi

BUG=449665
Committed: https://crrev.com/72ca80039efcea31c92af9ef069562bc80b02be5
Cr-Commit-Position: refs/heads/master@{#322033}

[megjablon, 2015-03-24 00:09:10]

megjablon@chromium.org changed reviewers:
+ sclittle@chromium.org

[sclittle, 2015-03-24 01:27:39]

https://codereview.chromium.org/998163005/diff/1/tools/chrome_proxy/integrati...
File tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py (right):

https://codereview.chromium.org/998163005/diff/1/tools/chrome_proxy/integrati...
tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:224: cl =
resp.content_length
Why write the "cl" variable here if it's overwritten in the next for loop?

https://codereview.chromium.org/998163005/diff/1/tools/chrome_proxy/integrati...
tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:225: resource =
resp.response.url
Why is this variable set here? Is this variable necessary?

https://codereview.chromium.org/998163005/diff/1/tools/chrome_proxy/integrati...
tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:226:
results.AddValue(scalar.ScalarValue(
While you're at it, could you also make it so that this test fails if
lo_fi_count is 0? (i.e. if zero responses came back)

https://codereview.chromium.org/998163005/diff/1/tools/chrome_proxy/integrati...
tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:229: for resp in
self.IterResponses(tab):
Could you combine these two for loops?

https://codereview.chromium.org/998163005/diff/1/tools/chrome_proxy/integrati...
tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:230: r =
resp.response
unused variable

https://codereview.chromium.org/998163005/diff/1/tools/chrome_proxy/integrati...
tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:239:
results.AddValue(scalar.ScalarValue(
Why report the last response's cl, ocl, and saving? Would it make more sense to
instead report the overall cl, ocl, and saving?

If you want to report the overall cl, ocl, and savings, you could call
super(ChromeProxyMetric, self).AddResults(tab, results) like
AddResultsForDataSaving does above; see
https://code.google.com/p/chromium/codesearch#chromium/src/tools/chrome_proxy...
for the implementation of that.

[megjablon, 2015-03-24 02:04:38]

Patchset #2 (id:20001) has been deleted

[megjablon, 2015-03-24 02:05:29]

https://codereview.chromium.org/998163005/diff/1/tools/chrome_proxy/integrati...
File tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py (right):

https://codereview.chromium.org/998163005/diff/1/tools/chrome_proxy/integrati...
tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:224: cl =
resp.content_length
On 2015/03/24 01:27:38, sclittle wrote:
> Why write the "cl" variable here if it's overwritten in the next for loop?

Done.

https://codereview.chromium.org/998163005/diff/1/tools/chrome_proxy/integrati...
tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:225: resource =
resp.response.url
On 2015/03/24 01:27:38, sclittle wrote:
> Why is this variable set here? Is this variable necessary?

Done.

https://codereview.chromium.org/998163005/diff/1/tools/chrome_proxy/integrati...
tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:226:
results.AddValue(scalar.ScalarValue(
On 2015/03/24 01:27:38, sclittle wrote:
> While you're at it, could you also make it so that this test fails if
> lo_fi_count is 0? (i.e. if zero responses came back)

Done.

https://codereview.chromium.org/998163005/diff/1/tools/chrome_proxy/integrati...
tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:229: for resp in
self.IterResponses(tab):
On 2015/03/24 01:27:38, sclittle wrote:
> Could you combine these two for loops?

Done.

https://codereview.chromium.org/998163005/diff/1/tools/chrome_proxy/integrati...
tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:230: r =
resp.response
On 2015/03/24 01:27:38, sclittle wrote:
> unused variable

Done.

https://codereview.chromium.org/998163005/diff/1/tools/chrome_proxy/integrati...
tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:239:
results.AddValue(scalar.ScalarValue(
On 2015/03/24 01:27:38, sclittle wrote:
> Why report the last response's cl, ocl, and saving? Would it make more sense
to
> instead report the overall cl, ocl, and saving?
> 
> If you want to report the overall cl, ocl, and savings, you could call
> super(ChromeProxyMetric, self).AddResults(tab, results) like
> AddResultsForDataSaving does above; see
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/chrome_proxy...
> for the implementation of that.

Done.

[sclittle, 2015-03-24 17:32:46]

https://codereview.chromium.org/998163005/diff/40001/tools/chrome_proxy/integ...
File tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py (right):

https://codereview.chromium.org/998163005/diff/40001/tools/chrome_proxy/integ...
tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:218: cl =
resp.content_length
Just use resp.response.url and resp.content_length inline; these r and cl
variables aren't needed.

[megjablon, 2015-03-24 18:05:18]

https://codereview.chromium.org/998163005/diff/40001/tools/chrome_proxy/integ...
File tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py (right):

https://codereview.chromium.org/998163005/diff/40001/tools/chrome_proxy/integ...
tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:218: cl =
resp.content_length
On 2015/03/24 17:32:46, sclittle wrote:
> Just use resp.response.url and resp.content_length inline; these r and cl
> variables aren't needed.

Done.

[sclittle, 2015-03-24 18:06:09]

lgtm

[megjablon, 2015-03-24 18:29:13]

The CQ bit was checked by megjablon@chromium.org

[commit-bot: I haz the power, 2015-03-24 18:29:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/998163005/60001

[commit-bot: I haz the power, 2015-03-24 18:35:29]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2015-03-24 18:36:23]

Patchset 3 (id:??) landed as
https://crrev.com/72ca80039efcea31c92af9ef069562bc80b02be5
Cr-Commit-Position: refs/heads/master@{#322033}