Track ga.js through the cache.

Track ga.js through the cache.

Since ga.js would hit Google anyways, the privacy implication is minimal.
ga.js is a rather ubiquitous script, so that makes it a useful case study for cache policies.

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

[willchan no longer on Chromium, 2012-09-28 21:06:14]

I will ditch the field trial change before I land. Was just using that for
manual testing. I'll also redo the bits as we discussed offline. What other
histograms should I get?

[rvargas (doing something else), 2012-09-28 21:51:15]

The number of histograms look fine.

http://codereview.chromium.org/10986034/diff/1/net/http/infinite_cache.cc
File net/http/infinite_cache.cc (left):

http://codereview.chromium.org/10986034/diff/1/net/http/infinite_cache.cc#old...
net/http/infinite_cache.cc:258: resource_data_.reset(new ResourceData);
move this line up to line 250 (in this version).

http://codereview.chromium.org/10986034/diff/1/net/http/infinite_cache.cc
File net/http/infinite_cache.cc (right):

http://codereview.chromium.org/10986034/diff/1/net/http/infinite_cache.cc#new...
net/http/infinite_cache.cc:298: details.flags |= old_ga_js_values;
I knew there was a reason for passing DOOM_METHOD as a bool instead.

2 more bools? otherwise we need to have a mask of flags that can be set from
Sart()

[willchan no longer on Chromium, 2012-09-28 22:07:55]

http://codereview.chromium.org/10986034/diff/1/net/http/infinite_cache.cc
File net/http/infinite_cache.cc (left):

http://codereview.chromium.org/10986034/diff/1/net/http/infinite_cache.cc#old...
net/http/infinite_cache.cc:258: resource_data_.reset(new ResourceData);
On 2012/09/28 21:51:15, rvargas wrote:
> move this line up to line 250 (in this version).

I'm fine with that, but in the method != "GET" case, we're wasting memory then,
right?

http://codereview.chromium.org/10986034/diff/1/net/http/infinite_cache.cc
File net/http/infinite_cache.cc (right):

http://codereview.chromium.org/10986034/diff/1/net/http/infinite_cache.cc#new...
net/http/infinite_cache.cc:298: details.flags |= old_ga_js_values;
On 2012/09/28 21:51:15, rvargas wrote:
> I knew there was a reason for passing DOOM_METHOD as a bool instead.
> 
> 2 more bools? otherwise we need to have a mask of flags that can be set from
> Sart() 

I don't get it...why?

[willchan no longer on Chromium, 2012-09-28 22:21:07]

Btw, I've updated it now to redo the bits so there's one for HTTP and one for
HTTPS. Thx for the recommendation earlier.

[willchan no longer on Chromium, 2012-09-29 00:49:55]

OK, I've combined the |must_doom_entry_| into the bitmask.

[rvargas (doing something else), 2012-09-29 02:12:51]

time to remove field_trials?

http://codereview.chromium.org/10986034/diff/18001/net/http/infinite_cache.cc
File net/http/infinite_cache.cc (right):

http://codereview.chromium.org/10986034/diff/18001/net/http/infinite_cache.cc...
net/http/infinite_cache.cc:50: GA_JS_HTTPS = 1 << 9,  // 0 means HTTP, 1 means
HTTPS
stale comment.

http://codereview.chromium.org/10986034/diff/18001/net/http/infinite_cache.cc...
net/http/infinite_cache.cc:231: static const char kGaJsHttpUrl[] =
"http://www.google-analytics.com/ga.js";
this should go into the anonymous namespace

http://codereview.chromium.org/10986034/diff/18001/net/http/infinite_cache.cc...
net/http/infinite_cache.cc:255: scoped_ptr<ResourceData> resource_data(new
ResourceData);
there's no need for a local variable + swap. Just use resource_data_ directly

http://codereview.chromium.org/10986034/diff/18001/net/http/infinite_cache.cc...
net/http/infinite_cache.cc:262: } else {
no need for an else

http://codereview.chromium.org/10986034/diff/18001/net/http/infinite_cache.cc...
net/http/infinite_cache.cc:263: const std::string cache_key =
HttpUtil::SpecForRequest(request->url);
cache_->GenerateKey(request)

http://codereview.chromium.org/10986034/diff/18001/net/http/infinite_cache.cc...
net/http/infinite_cache.cc:265: resource_data->details.flags &= GA_JS_HTTP;
|= :)

http://codereview.chromium.org/10986034/diff/18001/net/http/infinite_cache.cc...
net/http/infinite_cache.cc:272: CryptoHash(cache_->GenerateKey(request),
&resource_data_->key);
use cache_key here

http://codereview.chromium.org/10986034/diff/18001/net/http/infinite_cache.cc...
net/http/infinite_cache.cc:282: // Store the old ga.js flag values.
stale comment

[willchan no longer on Chromium, 2012-09-29 02:18:27]

http://codereview.chromium.org/10986034/diff/18001/net/http/infinite_cache.cc
File net/http/infinite_cache.cc (right):

http://codereview.chromium.org/10986034/diff/18001/net/http/infinite_cache.cc...
net/http/infinite_cache.cc:50: GA_JS_HTTPS = 1 << 9,  // 0 means HTTP, 1 means
HTTPS
On 2012/09/29 02:12:51, rvargas wrote:
> stale comment.

Done.

http://codereview.chromium.org/10986034/diff/18001/net/http/infinite_cache.cc...
net/http/infinite_cache.cc:231: static const char kGaJsHttpUrl[] =
"http://www.google-analytics.com/ga.js";
On 2012/09/29 02:12:51, rvargas wrote:
> this should go into the anonymous namespace

Done.

http://codereview.chromium.org/10986034/diff/18001/net/http/infinite_cache.cc...
net/http/infinite_cache.cc:255: scoped_ptr<ResourceData> resource_data(new
ResourceData);
On 2012/09/29 02:12:51, rvargas wrote:
> there's no need for a local variable + swap. Just use resource_data_ directly

Done.

http://codereview.chromium.org/10986034/diff/18001/net/http/infinite_cache.cc...
net/http/infinite_cache.cc:262: } else {
On 2012/09/29 02:12:51, rvargas wrote:
> no need for an else

Done.

http://codereview.chromium.org/10986034/diff/18001/net/http/infinite_cache.cc...
net/http/infinite_cache.cc:265: resource_data->details.flags &= GA_JS_HTTP;
On 2012/09/29 02:12:51, rvargas wrote:
> |= :)

Fixed earlier! I saw the test failures, thank god for tests :)

http://codereview.chromium.org/10986034/diff/18001/net/http/infinite_cache.cc...
net/http/infinite_cache.cc:272: CryptoHash(cache_->GenerateKey(request),
&resource_data_->key);
On 2012/09/29 02:12:51, rvargas wrote:
> use cache_key here

Done.

http://codereview.chromium.org/10986034/diff/18001/net/http/infinite_cache.cc...
net/http/infinite_cache.cc:282: // Store the old ga.js flag values.
On 2012/09/29 02:12:51, rvargas wrote:
> stale comment

Done.

[willchan no longer on Chromium, 2012-09-29 02:20:31]

OK, I've ditched the field trial too.

[rvargas (doing something else), 2012-09-29 02:28:15]

lgtm after fixing the boolean or.

http://codereview.chromium.org/10986034/diff/21003/net/http/infinite_cache.cc
File net/http/infinite_cache.cc (right):

http://codereview.chromium.org/10986034/diff/21003/net/http/infinite_cache.cc...
net/http/infinite_cache.cc:264: resource_data_->details.flags &= GA_JS_HTTP;
I meant this one (and the next one)

[willchan no longer on Chromium, 2012-09-29 02:31:21]

I'm going to send this to the CQ tonight since I've got approval now.

http://codereview.chromium.org/10986034/diff/21003/net/http/infinite_cache.cc
File net/http/infinite_cache.cc (right):

http://codereview.chromium.org/10986034/diff/21003/net/http/infinite_cache.cc...
net/http/infinite_cache.cc:264: resource_data_->details.flags &= GA_JS_HTTP;
On 2012/09/29 02:28:16, rvargas wrote:
> I meant this one (and the next one)

Argh, I fail. Thanks for the careful review. Done now.

[commit-bot: I haz the power, 2012-09-29 06:26:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/willchan@chromium.org/10986034/18002

[commit-bot: I haz the power, 2012-09-29 14:21:47]

Change committed as 159409