Open up SDCH for all sites, in preparation for latency tests...

Open up SDCH for all sites, in preparation for latency tests

Since the stability test is going well (so far) on
".google.com," this change will open up support for SDCH
compression to all sites. This will allow for more
international testing as well.

I tightened down the restrictions on who can set up a dictionary for
a given domain.  I'm pretty sure it is at least as restrictive as the
current SDCH spec.

I also supplied a default expiration time for using an SDCH
dictionary at 30 days (as per SDCH spec).

To be safer with the latency histograms, I also tightened the period of
time we measure, on the off chance that the page some-how asks for more
bytes after everything has been read/rendered.

r=openvcdiff,hunar

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

[Lincoln, 2008-11-14 21:55:30]

Great job as usual.

http://codereview.chromium.org/11009/diff/204/207
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/11009/diff/204/207#newcode532
Line 532: // Use default to "" so that all domains are supported.
tiny prepositional nit: to => of

http://codereview.chromium.org/11009/diff/204/207#newcode533
Line 533: std::string switch_domain("");  // Provide default test domain.
Remove the comment from this line; it appears to be covered by the preceding
line.

http://codereview.chromium.org/11009/diff/204/208
File net/base/sdch_filter_unittest.cc (right):

http://codereview.chromium.org/11009/diff/204/208#newcode595
Line 595: dictionary_text += " ";  // Cause the dictinary hash to be distinct.
I recommend breaking these out into separate test functions instead of trying to
reset and perform the add with a dictionary already in the cache.  Start with a
clean slate for each test.

http://codereview.chromium.org/11009/diff/204/206
File net/base/sdch_manager.cc (right):

http://codereview.chromium.org/11009/diff/204/206#newcode304
Line 304: size_t postfix_domain_index = referrer_url_host.rfind(domain);
I like the .rfind.  You might add a unit test to show why it is necessary, e.g.
match "www.www.www.www" against ".www.www".

http://codereview.chromium.org/11009/diff/204/206#newcode310
Line 310: end_of_host_index <= postfix_domain_index)
You could use "find_last_of" with a "pos" argument to make this clause
unnecessary.

http://codereview.chromium.org/11009/diff/204/206#newcode378
Line 378: return false;
Is there any provision to unload the dictionary from the cache when it expires?

[Lincoln, 2008-11-14 21:58:10]

P.S.  Would there be any benefit to leveraging the code in
net/base/registry_controlled_domain.h for some of your domain matching instead
of doing it by hand?  SDCH domain matching is based on cookie domain matching,
which I believe uses registry_controlled_domain.h.

[Lincoln, 2008-11-14 22:01:31]

Please disregard the P.S.  I should have seen that you are already using the
suggested module.  Sorry for the noise.

[jar (doing other things), 2008-11-14 23:43:55]

Please note additional change in sdch_filter.cc (to possibly 
get better latency numbers).

This version has responses to all of Lincoln's comments.

http://codereview.chromium.org/11009/diff/204/207
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/11009/diff/204/207#newcode532
Line 532: // Use default to "" so that all domains are supported.
On 2008/11/14 21:55:31, Lincoln wrote:
> tiny prepositional nit: to => of

Done.

http://codereview.chromium.org/11009/diff/204/207#newcode533
Line 533: std::string switch_domain("");  // Provide default test domain.
On 2008/11/14 21:55:31, Lincoln wrote:
> Remove the comment from this line; it appears to be covered by the preceding
> line.

Done.

http://codereview.chromium.org/11009/diff/204/208
File net/base/sdch_filter_unittest.cc (right):

http://codereview.chromium.org/11009/diff/204/208#newcode595
Line 595: dictionary_text += " ";  // Cause the dictinary hash to be distinct.
On 2008/11/14 21:55:31, Lincoln wrote:
> I recommend breaking these out into separate test functions instead of trying
to
> reset and perform the add with a dictionary already in the cache.  Start with
a
> clean slate for each test.

Done.

http://codereview.chromium.org/11009/diff/204/206
File net/base/sdch_manager.cc (right):

http://codereview.chromium.org/11009/diff/204/206#newcode304
Line 304: size_t postfix_domain_index = referrer_url_host.rfind(domain);
On 2008/11/14 21:55:31, Lincoln wrote:
> I like the .rfind.  You might add a unit test to show why it is necessary,
e.g.
> match "www.www.www.www" against ".www.www".

Done.

http://codereview.chromium.org/11009/diff/204/206#newcode310
Line 310: end_of_host_index <= postfix_domain_index)
On 2008/11/14 21:55:31, Lincoln wrote:
> You could use "find_last_of" with a "pos" argument to make this clause
> unnecessary.

You're correct.  Current code seems at least as clear (and I hate to have to
look up the meaning of the subtle overloads when reading code).  It is also not
performance critical...

http://codereview.chromium.org/11009/diff/204/206#newcode378
Line 378: return false;
On 2008/11/14 21:55:31, Lincoln wrote:
> Is there any provision to unload the dictionary from the cache when it
expires?

There is currently no provision, other than the fact that the dictionary is
discarded at shutdown.  With current dictionaries defaulting to 30 days, I would
also guess that the user will restart chrome long before the expiration.

I also have an outstanding question about this area of the SDCH spec (re:
max_age vs proposed expiration_date), and suspect it may change, so I'd rather
not put too much effort in this area until there is some resolution.

[Lincoln, 2008-11-15 01:37:02]

LGTM

http://codereview.chromium.org/11009/diff/217/412
File net/base/sdch_filter_unittest.cc (right):

http://codereview.chromium.org/11009/diff/217/412#newcode630
Line 630: // Fail the HD with D being the domain and H having a dot requirement.
Comment is not accurate -- this test should pass.

[huanr, 2008-11-17 21:43:28]

LGTM