Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1406993010:
Crop urls in Data Saver feedback reports (Closed)
DescriptionCrop urls in Data Saver feedback reports
For reports where the last bypass happened more than 5 minutes ago,
crop the bypass URL to the host only.
BUG=514773
Committed: https://crrev.com/29624d080735e62de722def7fffb3e2e3fae0a8a
Cr-Commit-Position: refs/heads/master@{#357891}
Patch Set 1 #
Total comments: 9
Patch Set 2 : sclittle comments #
Total comments: 8
Patch Set 3 : comments #
Total comments: 2
Patch Set 4 : test 4 minutes #
Messages
Total messages: 13 (2 generated)
megjablon@chromium.org changed reviewers: + sclittle@chromium.org
PTAL, thanks!
https://chromiumcodereview.appspot.com/1406993010/diff/1/components/data_redu... File components/data_reduction_proxy/core/common/data_reduction_proxy_event_store.cc (right): https://chromiumcodereview.appspot.com/1406993010/diff/1/components/data_redu... components/data_reduction_proxy/core/common/data_reduction_proxy_event_store.cc:22: const int kLastBypassTimeDeltaMS = 300000; For clarity, could you change this to be in minutes and add a comment explaining what this is for? You can use base::TimeDelta::FromMinutes when comparing the times below. https://chromiumcodereview.appspot.com/1406993010/diff/1/components/data_redu... components/data_reduction_proxy/core/common/data_reduction_proxy_event_store.cc:254: bool host_only = true; nit: could you use a more descriptive name here? e.g. truncate_url_to_host or something. https://chromiumcodereview.appspot.com/1406993010/diff/1/components/data_redu... components/data_reduction_proxy/core/common/data_reduction_proxy_event_store.cc:258: int64 bypass_time; nit: #include <stdint.h> and use int64_t here instead of int64. https://chromiumcodereview.appspot.com/1406993010/diff/1/components/data_redu... components/data_reduction_proxy/core/common/data_reduction_proxy_event_store.cc:258: int64 bypass_time; nit: It's not obvious here what the units are, could you rename this to variable to bypass_ticks_ms or something? https://chromiumcodereview.appspot.com/1406993010/diff/1/components/data_redu... components/data_reduction_proxy/core/common/data_reduction_proxy_event_store.cc:264: if (now - bypass_time < kLastBypassTimeDeltaMS) For clarity, instead of comparing raw millisecond counts, could you just compare them as TimeDeltas? e.g. base::TimeTicks bypass_ticks = base::TimeTicks() + base::TimeDelta::FromMilliseconds(bypass_time_ms); if (base::TimeTicks::Now() - bypass_ticks < base::TimeDelta::FromMinutes(kLastBypassTimeDeltaMinutes)) { ...
https://chromiumcodereview.appspot.com/1406993010/diff/1/components/data_redu... File components/data_reduction_proxy/core/common/data_reduction_proxy_event_store.cc (right): https://chromiumcodereview.appspot.com/1406993010/diff/1/components/data_redu... components/data_reduction_proxy/core/common/data_reduction_proxy_event_store.cc:22: const int kLastBypassTimeDeltaMS = 300000; On 2015/11/03 01:50:46, sclittle wrote: > For clarity, could you change this to be in minutes and add a comment explaining > what this is for? > > You can use base::TimeDelta::FromMinutes when comparing the times below. Done. https://chromiumcodereview.appspot.com/1406993010/diff/1/components/data_redu... components/data_reduction_proxy/core/common/data_reduction_proxy_event_store.cc:254: bool host_only = true; On 2015/11/03 01:50:45, sclittle wrote: > nit: could you use a more descriptive name here? e.g. truncate_url_to_host or > something. Done. https://chromiumcodereview.appspot.com/1406993010/diff/1/components/data_redu... components/data_reduction_proxy/core/common/data_reduction_proxy_event_store.cc:258: int64 bypass_time; On 2015/11/03 01:50:46, sclittle wrote: > nit: It's not obvious here what the units are, could you rename this to variable > to bypass_ticks_ms or something? Done. https://chromiumcodereview.appspot.com/1406993010/diff/1/components/data_redu... components/data_reduction_proxy/core/common/data_reduction_proxy_event_store.cc:264: if (now - bypass_time < kLastBypassTimeDeltaMS) On 2015/11/03 01:50:46, sclittle wrote: > For clarity, instead of comparing raw millisecond counts, could you just compare > them as TimeDeltas? e.g. > > base::TimeTicks bypass_ticks = base::TimeTicks() + > base::TimeDelta::FromMilliseconds(bypass_time_ms); > > if (base::TimeTicks::Now() - bypass_ticks < > base::TimeDelta::FromMinutes(kLastBypassTimeDeltaMinutes)) { > ... Done.
https://chromiumcodereview.appspot.com/1406993010/diff/20001/components/data_... File components/data_reduction_proxy/core/common/data_reduction_proxy_event_store.cc (right): https://chromiumcodereview.appspot.com/1406993010/diff/20001/components/data_... components/data_reduction_proxy/core/common/data_reduction_proxy_event_store.cc:7: #include <stdint.h> micro nit: please add blank line between <stdint.h> and <vector> includes, the convention is to have a blank line between C-style and C++-style includes like these. https://chromiumcodereview.appspot.com/1406993010/diff/20001/components/data_... components/data_reduction_proxy/core/common/data_reduction_proxy_event_store.cc:271: truncate_url_to_host = false; This comparison looks backward - did you mean "true"? https://chromiumcodereview.appspot.com/1406993010/diff/20001/components/data_... File components/data_reduction_proxy/core/common/data_reduction_proxy_event_store_unittest.cc (right): https://chromiumcodereview.appspot.com/1406993010/diff/20001/components/data_... components/data_reduction_proxy/core/common/data_reduction_proxy_event_store_unittest.cc:280: delta_time -= 300000; Testing 5 minutes exactly seems ambiguous - could you instead test a time that's clearly larger than 5 min and a time that's clearly smaller than 5 min?
https://chromiumcodereview.appspot.com/1406993010/diff/20001/components/data_... File components/data_reduction_proxy/core/common/data_reduction_proxy_event_store.cc (right): https://chromiumcodereview.appspot.com/1406993010/diff/20001/components/data_... components/data_reduction_proxy/core/common/data_reduction_proxy_event_store.cc:7: #include <stdint.h> On 2015/11/04 01:41:33, sclittle wrote: > micro nit: please add blank line between <stdint.h> and <vector> includes, the > convention is to have a blank line between C-style and C++-style includes like > these. Done. https://chromiumcodereview.appspot.com/1406993010/diff/20001/components/data_... components/data_reduction_proxy/core/common/data_reduction_proxy_event_store.cc:271: truncate_url_to_host = false; On 2015/11/04 01:41:33, sclittle wrote: > This comparison looks backward - did you mean "true"? No, this is correct. If the last bypass happened in the last five minutes, we don't truncate to host, but keep the full url (minus the query). When the last_bypass time was greater than 5 minutes ago, or we don't know the bypass time, we only use the host. https://chromiumcodereview.appspot.com/1406993010/diff/20001/components/data_... File components/data_reduction_proxy/core/common/data_reduction_proxy_event_store_unittest.cc (right): https://chromiumcodereview.appspot.com/1406993010/diff/20001/components/data_... components/data_reduction_proxy/core/common/data_reduction_proxy_event_store_unittest.cc:280: delta_time -= 300000; On 2015/11/04 01:41:33, sclittle wrote: > Testing 5 minutes exactly seems ambiguous - could you instead test a time that's > clearly larger than 5 min and a time that's clearly smaller than 5 min? Done.
LGTM % nits https://chromiumcodereview.appspot.com/1406993010/diff/20001/components/data_... File components/data_reduction_proxy/core/common/data_reduction_proxy_event_store.cc (right): https://chromiumcodereview.appspot.com/1406993010/diff/20001/components/data_... components/data_reduction_proxy/core/common/data_reduction_proxy_event_store.cc:271: truncate_url_to_host = false; On 2015/11/04 19:41:07, megjablon wrote: > On 2015/11/04 01:41:33, sclittle wrote: > > This comparison looks backward - did you mean "true"? > > No, this is correct. If the last bypass happened in the last five minutes, we > don't truncate to host, but keep the full url (minus the query). When the > last_bypass time was greater than 5 minutes ago, or we don't know the bypass > time, we only use the host. OK, then the comment above this if statement is backwards, please fix. https://chromiumcodereview.appspot.com/1406993010/diff/40001/components/data_... File components/data_reduction_proxy/core/common/data_reduction_proxy_event_store_unittest.cc (right): https://chromiumcodereview.appspot.com/1406993010/diff/40001/components/data_... components/data_reduction_proxy/core/common/data_reduction_proxy_event_store_unittest.cc:279: base::TimeTicks::Now() - base::TimeDelta::FromMinutes(6)); Could we also test that 4 minutes isn't truncated?
https://chromiumcodereview.appspot.com/1406993010/diff/20001/components/data_... File components/data_reduction_proxy/core/common/data_reduction_proxy_event_store.cc (right): https://chromiumcodereview.appspot.com/1406993010/diff/20001/components/data_... components/data_reduction_proxy/core/common/data_reduction_proxy_event_store.cc:271: truncate_url_to_host = false; On 2015/11/04 19:47:10, sclittle wrote: > On 2015/11/04 19:41:07, megjablon wrote: > > On 2015/11/04 01:41:33, sclittle wrote: > > > This comparison looks backward - did you mean "true"? > > > > No, this is correct. If the last bypass happened in the last five minutes, we > > don't truncate to host, but keep the full url (minus the query). When the > > last_bypass time was greater than 5 minutes ago, or we don't know the bypass > > time, we only use the host. > > OK, then the comment above this if statement is backwards, please fix. Ah you're right. My bad. https://chromiumcodereview.appspot.com/1406993010/diff/40001/components/data_... File components/data_reduction_proxy/core/common/data_reduction_proxy_event_store_unittest.cc (right): https://chromiumcodereview.appspot.com/1406993010/diff/40001/components/data_... components/data_reduction_proxy/core/common/data_reduction_proxy_event_store_unittest.cc:279: base::TimeTicks::Now() - base::TimeDelta::FromMinutes(6)); On 2015/11/04 19:47:10, sclittle wrote: > Could we also test that 4 minutes isn't truncated? We test that above. Switched from Now to 4 minutes.
Cool, still LGTM.
The CQ bit was checked by megjablon@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1406993010/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1406993010/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/29624d080735e62de722def7fffb3e2e3fae0a8a Cr-Commit-Position: refs/heads/master@{#357891} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
