Make SdchManager per-profile.

Make SdchManager per-profile.

This will both allow SDCH dictionaries to be cached (as they can use the
cache associated with the profile) and will provide privacy protection
between different profiles (the existing of a dictionary in one profile
will not be leaked to another profile).

BUG=374914
R=jar@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277160

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277322

[Randy Smith (Not in Mondays), 2014-05-26 21:02:28]

This CL is dependent on https://codereview.chromium.org/292063002. 

Jim: Could you give me early feedback on this CL?  It's not done yet (tests are
crashing) but it compiles and embodies my basic intent and I'm about to start a
week of bug triage and cleanup work, so I think getting initial responses to it
would be useful.

Still not done:
* Hooking into browsing data removal.
* Writing a simple browser test to test end-to-end plumbing. 

From my perspective, the biggest change I made was to shift dispatch of the
dictionary fetch from the URLRequestHttpJob destructor to when the dictionary
was noticed.  I did that because I don't have access to the request (and hence
the request context, and hence the SdchManager) in the destructor.  I couldn't
figure out why the fetch was in the destructor, but I presume there was a reason
:-}, so I'm specifically looking for feedback on that change.  And anything else
you notice that you want to comment on (high level--I'm not ready yet for
detailed review).

[jar (doing other things), 2014-05-27 17:14:22]

Top level comments (before I read the CL):

The dictionary fetch was probably initiated in the destructor to minimize the
chance that the fetch would interfere available bandwidth for the resource that
mentioned the dictionary.  Note that we still added a short delay... but it
would have been MUCH better to find a time of quiescence to download the
dictionary... and MUCH better if we had a "background" download that would
back-away from using the link if other resources were requested (e.g., a "low
priority" download).

FWIW: Moving to use separate profiles is probably nice... but I expect the much
bigger changes will be to have an async decompression API that allows for
fetching of a dictionary from disk; and having the ability to write decompressed
content to a disk, so that we don't need the dictionary for do additional/future
decompression (deciphering?) of the (cached) SDCH content.

[jar (doing other things), 2014-05-27 17:53:09]

I looked through the CL... and it all looks reasonable so far.

As you do move to having profile-based dictionaries, you should think about how
much memory is reasonable to consume for the cached dictionaries.  You might
also ponder a way to tune this by platform, such as being more conservative
about memory use on Mobile.

https://codereview.chromium.org/298063006/diff/1/chrome/browser/net/sdch_dict...
File chrome/browser/net/sdch_dictionary_fetcher.h (left):

https://codereview.chromium.org/298063006/diff/1/chrome/browser/net/sdch_dict...
chrome/browser/net/sdch_dictionary_fetcher.h:37: static void Shutdown();
Given that the dictionary fetch is very async... I would expect that you'll need
some shutdown call to abandon such a pending fetch.

https://codereview.chromium.org/298063006/diff/1/net/base/sdch_manager.cc
File net/base/sdch_manager.cc (left):

https://codereview.chromium.org/298063006/diff/1/net/base/sdch_manager.cc#old...
net/base/sdch_manager.cc:56: if
(!SdchManager::Global()->IsInSupportedDomain(target_url))
It was nice to have a way to have users "turn off" SDCH if this go to hell in a
handbasket.   You might make sure you have such a flag, and you might even want
to be able to toggle it via Finch.

It is VERY easy to break a LOT of critical pages (google search?) with any error
in this code... and you may well appreciate a way to broadly disable it.

[Randy Smith (Not in Mondays), 2014-06-04 20:39:44]

On 2014/05/27 17:14:22, jar wrote:

> FWIW: Moving to use separate profiles is probably nice... but I expect the
much
> bigger changes will be to have an async decompression API that allows for
> fetching of a dictionary from disk; and having the ability to write
decompressed
> content to a disk, so that we don't need the dictionary for do
additional/future
> decompression (deciphering?) of the (cached) SDCH content.

Huh.  I hadn't worked out that we cached the encoded content before this.  What
keeps that from breaking horribly if we try and read from the same URL at a
later time without having the appropriate dictionary in place?

The goals for this change are twofold.  One is privacy, which I think it does
accomplish, and the other is caching of the SDCH dictionaries, which it helps a
lot with (they'll be fetched through a profile URLRequestContextGetter(), and
hence end up in cache).  I'm going to get them the rest of the way persistent in
http://crbug.com/374915, which will preload from cache a persisted list of SDCH
dictionary URLs.

[Randy Smith (Not in Mondays), 2014-06-04 22:04:52]

> As you do move to having profile-based dictionaries, you should think 
> about how much memory is reasonable to consume for the cached 
> dictionaries.  You might also ponder a way to tune this by platform, 
> such as being more conservative about memory use on Mobile.

Yep.  Planning on doing that in http://crbug.com/374917, with a future
unscheduled follow-on in http://crbug.com/374916.

https://codereview.chromium.org/298063006/diff/1/chrome/browser/net/sdch_dict...
File chrome/browser/net/sdch_dictionary_fetcher.h (left):

https://codereview.chromium.org/298063006/diff/1/chrome/browser/net/sdch_dict...
chrome/browser/net/sdch_dictionary_fetcher.h:37: static void Shutdown();
On 2014/05/27 17:53:09, jar wrote:
> Given that the dictionary fetch is very async... I would expect that you'll
need
> some shutdown call to abandon such a pending fetch.

I believe what I currently have is equivalent to the old global code with regard
to shutdown; do you disagree with that claim, or do you think that I need to add
something to shutdown pending fetches that wasn't in the original code?

Support for above statements: In the original code,
BrowserProcessImpl::StartTearDown called SdchDictionaryFetcher::Shutdown(),
which called SdchManager::Shutdown(), which reset the SdchDictionaryFetcher
scoped_ptr<> to null, which triggered the SdchDictionaryFetcher() destructor. 
In the current code, when the ProfileImplIOData is torn down (which I think is
before BrowserProcessImpl::StartTearDown, as it's per-profile), that results in
SdchManager destruction, which will result in SdchFetcher scoped_ptr<> nulling,
which will result in SdchDictionaryFetcher destruction.

I believe that SdchDictionaryFetcher destruction results in cancellation, since
it results in URLFetcher destruction, which results in calling
URLFetcherCore::Stop().

https://codereview.chromium.org/298063006/diff/1/net/base/sdch_manager.cc
File net/base/sdch_manager.cc (left):

https://codereview.chromium.org/298063006/diff/1/net/base/sdch_manager.cc#old...
net/base/sdch_manager.cc:56: if
(!SdchManager::Global()->IsInSupportedDomain(target_url))
On 2014/05/27 17:53:09, jar wrote:
> It was nice to have a way to have users "turn off" SDCH if this go to hell in
a
> handbasket.   You might make sure you have such a flag, and you might even
want
> to be able to toggle it via Finch.
> 
> It is VERY easy to break a LOT of critical pages (google search?) with any
error
> in this code... and you may well appreciate a way to broadly disable it.

Fair, and I'll look into it, but I'm confused as to why that comment is made on
this line?  The removal of this line is just a refactor; I put it in instead
just before the only call to CanAdvertise() below.

https://codereview.chromium.org/298063006/diff/1/net/base/sdch_manager.cc#old...
net/base/sdch_manager.cc:56: if
(!SdchManager::Global()->IsInSupportedDomain(target_url))
On 2014/05/27 17:53:09, jar wrote:
> It was nice to have a way to have users "turn off" SDCH if this go to hell in
a
> handbasket.   You might make sure you have such a flag, and you might even
want
> to be able to toggle it via Finch.
> 
> It is VERY easy to break a LOT of critical pages (google search?) with any
error
> in this code... and you may well appreciate a way to broadly disable it.

I've looked into this, and I don't believe I'm changing anything in this space
in this refactor.   EnableSdchSupport(false) will turn SDCH off (by making
IsInSupportedDomain() return false unilaterally), and that can be set from Finch
if we really need to.

[Randy Smith (Not in Mondays), 2014-06-09 18:55:20]

Jim: I think I'm ready for a real review.  Things I feel a bit funny about (so
that you get a fair chance to block them):

* I'm relying on the PostTask delay to avoid bandwidth contention for dictionary
fetch, despite having moved the fetch dispatch earlier in the URLRequestHttpJob.
 In my ideal world, I'd switch the fetcher to use IDLE priority, but that runs
into either adding priority to URLFetcher or rewriting SDCH to use URLRequest
(see my recent post to net-dev).
* I feel like this should have a simple end-to-end browsertest, but to do that,
I need to figure out how to create an SDCH dictionary in the test, which seems
non-trivial.  I'm going to (attempt to :-}) duck this for this CL on the basis
that this is just a refactor, and hope to come back to it when I have a chance
and have learned more.

WDYT?

[jar (doing other things), 2014-06-09 22:30:15]

On 2014/06/09 18:55:20, rdsmith wrote:
> Jim: I think I'm ready for a real review.  Things I feel a bit funny about (so
> that you get a fair chance to block them):
> 
> * I'm relying on the PostTask delay to avoid bandwidth contention for
dictionary
> fetch, despite having moved the fetch dispatch earlier in the
URLRequestHttpJob.
>  In my ideal world, I'd switch the fetcher to use IDLE priority, but that runs
> into either adding priority to URLFetcher or rewriting SDCH to use URLRequest
> (see my recent post to net-dev).
> * I feel like this should have a simple end-to-end browsertest, but to do
that,
> I need to figure out how to create an SDCH dictionary in the test, which seems
> non-trivial.  I'm going to (attempt to :-}) duck this for this CL on the basis
> that this is just a refactor, and hope to come back to it when I have a chance
> and have learned more.
> 
> WDYT?

1: Yes.... I had always wished there was a way to request a "background
download," but the PostTask was the "poors man's implementation."  Doing better
would be more work, and you can see if the optimization is justified by
use/cost.

2: End-to-end tests would be very nice.  You wouldn't need a "big" dictionary,
and could probably craft something very small.  My guess is a bit of manually
twiddling with Vcdiff would produce a test example.  There *might* be tests of
this sort hiding in some older work on SDCH... maybe you could even grow
something out of the current unit tests (which I think have some examples of
dictionaries and compression).   ...but I'm fine with looking at this as a
refactor.

[jar (doing other things), 2014-06-09 23:03:02]

Looks very nice.  Small comments below.

Thanks!

https://codereview.chromium.org/298063006/diff/40001/chrome/browser/net/sdch_...
File chrome/browser/net/sdch_dictionary_fetcher.h (right):

https://codereview.chromium.org/298063006/diff/40001/chrome/browser/net/sdch_...
chrome/browser/net/sdch_dictionary_fetcher.h:33: // this object.  Usually this
is done by making sure the manager
nit: Rather then then passive voice of "Usually this is done..." I think the
comment should indicate how this is achieved in the current implementation.

https://codereview.chromium.org/298063006/diff/40001/chrome/browser/net/sdch_...
chrome/browser/net/sdch_dictionary_fetcher.h:62: net::SdchManager* manager_;
nit: can/should this be const?

https://codereview.chromium.org/298063006/diff/40001/net/filter/mock_filter_c...
File net/filter/mock_filter_context.h (right):

https://codereview.chromium.org/298063006/diff/40001/net/filter/mock_filter_c...
net/filter/mock_filter_context.h:35: URLRequestContext*
GetModifiableURLRequestContext() { return context_.get(); }
nit: nice to use const on such accessors.

https://codereview.chromium.org/298063006/diff/40001/net/filter/sdch_filter.cc
File net/filter/sdch_filter.cc (right):

https://codereview.chromium.org/298063006/diff/40001/net/filter/sdch_filter.c...
net/filter/sdch_filter.cc:58: UMA_HISTOGRAM_COUNTS("Sdch3.PartialBytesIn",
nit: I went and looked at these histogtrams.  The dashboard indicated that they
are unowned.... but you *might* want to take ownership of some/all of these. 
Especially interesting is the: Sdch3.ProblemCodes_4 

This will be a place where you can see how your code is working in the field (
:-/.... assuming you don't get direction from crash reports).

https://codereview.chromium.org/298063006/diff/40001/net/filter/sdch_filter_u...
File net/filter/sdch_filter_unittest.cc (right):

https://codereview.chromium.org/298063006/diff/40001/net/filter/sdch_filter_u...
net/filter/sdch_filter_unittest.cc:576: TEST_F(SdchFilterTest,
CanStillDecodeHttp) {
Given that this CL introduces the use of multiple managers... it would be nice
to show that you can use more than one manager, and that changes in one
(blacklisting?) only blocks that manager, while the other manager is still
functional.

https://codereview.chromium.org/298063006/diff/40001/net/url_request/url_requ...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/298063006/diff/40001/net/url_request/url_requ...
net/url_request/url_request_http_job.cc:308: SdchManager*
manager(request()->context()->sdch_manager());
nit: in this context, just as in line 463, a better name might be sdch_manager.

https://codereview.chromium.org/298063006/diff/40001/net/url_request/url_requ...
net/url_request/url_request_http_job.cc:326: SdchManager* manager =
request()->context()->sdch_manager();
Didn't you just get this on line 308... and validate it on line 309?  Can you we
use that here?

[Randy Smith (Not in Mondays), 2014-06-10 20:38:41]

Thanks for the review!  I've tried to address all comments; let me know what you
think.

https://codereview.chromium.org/298063006/diff/40001/chrome/browser/net/sdch_...
File chrome/browser/net/sdch_dictionary_fetcher.h (right):

https://codereview.chromium.org/298063006/diff/40001/chrome/browser/net/sdch_...
chrome/browser/net/sdch_dictionary_fetcher.h:33: // this object.  Usually this
is done by making sure the manager
On 2014/06/09 23:03:01, jar wrote:
> nit: Rather then then passive voice of "Usually this is done..." I think the
> comment should indicate how this is achieved in the current implementation.

Done.

https://codereview.chromium.org/298063006/diff/40001/chrome/browser/net/sdch_...
chrome/browser/net/sdch_dictionary_fetcher.h:62: net::SdchManager* manager_;
On 2014/06/09 23:03:01, jar wrote:
> nit: can/should this be const?

I'm inclined to say "no"; the method that's used on the manager from this class
is AddSdchDictionary(), which to me changes the state of the manager and hence
can't really be consider conceptually const.  But if you have an argument in the
other direction, I'd value hearing it.

https://codereview.chromium.org/298063006/diff/40001/net/filter/mock_filter_c...
File net/filter/mock_filter_context.h (right):

https://codereview.chromium.org/298063006/diff/40001/net/filter/mock_filter_c...
net/filter/mock_filter_context.h:35: URLRequestContext*
GetModifiableURLRequestContext() { return context_.get(); }
On 2014/06/09 23:03:01, jar wrote:
> nit: nice to use const on such accessors.

Actually, the whole point of this accessor is that it's not const; thus the
"Modifiable" portion of the accessor name.  The const version is part of the
base class, and is declared below.  This accessor is used to set the SdchManager
on the URLRequestContext, so I didn't think it was reasonable to make it const. 
But I'm interested in an alternate perspective if you'd like to pitch it.

https://codereview.chromium.org/298063006/diff/40001/net/filter/sdch_filter.cc
File net/filter/sdch_filter.cc (right):

https://codereview.chromium.org/298063006/diff/40001/net/filter/sdch_filter.c...
net/filter/sdch_filter.cc:58: UMA_HISTOGRAM_COUNTS("Sdch3.PartialBytesIn",
On 2014/06/09 23:03:01, jar wrote:
> nit: I went and looked at these histogtrams.  The dashboard indicated that
they
> are unowned.... but you *might* want to take ownership of some/all of these. 
> Especially interesting is the: Sdch3.ProblemCodes_4 

Will do, but in a separate CL.  Confirming: I do that by adding an "<owner>"
field in histograms.xml?  Also, I'm not seeing the histogram in
tools/metrics/histograms.xml; does that mean they're internal and should be
moved external?

> This will be a place where you can see how your code is working in the field (
> :-/.... assuming you don't get direction from crash reports).

Yes, I need to engage with how to evaluate this stuff in the field; I haven't
done that yet.

https://codereview.chromium.org/298063006/diff/40001/net/filter/sdch_filter_u...
File net/filter/sdch_filter_unittest.cc (right):

https://codereview.chromium.org/298063006/diff/40001/net/filter/sdch_filter_u...
net/filter/sdch_filter_unittest.cc:576: TEST_F(SdchFilterTest,
CanStillDecodeHttp) {
On 2014/06/09 23:03:01, jar wrote:
> Given that this CL introduces the use of multiple managers... it would be nice
> to show that you can use more than one manager, and that changes in one
> (blacklisting?) only blocks that manager, while the other manager is still
> functional.

I did this with dictionaries, as that struck me as the most relevant state to
check.  Done.

https://codereview.chromium.org/298063006/diff/40001/net/url_request/url_requ...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/298063006/diff/40001/net/url_request/url_requ...
net/url_request/url_request_http_job.cc:308: SdchManager*
manager(request()->context()->sdch_manager());
On 2014/06/09 23:03:01, jar wrote:
> nit: in this context, just as in line 463, a better name might be
sdch_manager.

Done.

https://codereview.chromium.org/298063006/diff/40001/net/url_request/url_requ...
net/url_request/url_request_http_job.cc:326: SdchManager* manager =
request()->context()->sdch_manager();
On 2014/06/09 23:03:01, jar wrote:
> Didn't you just get this on line 308... and validate it on line 309?  Can you
we
> use that here?

Whoops; copy/paste error.  Done.

[jar (doing other things), 2014-06-11 20:04:43]

Just two nits below.... 

Tell me if I'm still missing something. They are not super critical... but
probably worth agreeing on.

Thanks!

https://codereview.chromium.org/298063006/diff/40001/chrome/browser/net/sdch_...
File chrome/browser/net/sdch_dictionary_fetcher.h (right):

https://codereview.chromium.org/298063006/diff/40001/chrome/browser/net/sdch_...
chrome/browser/net/sdch_dictionary_fetcher.h:62: net::SdchManager* manager_;
On 2014/06/10 20:38:41, rdsmith wrote:
> On 2014/06/09 23:03:01, jar wrote:
> > nit: can/should this be const?
> 
> I'm inclined to say "no"; the method that's used on the manager from this
class
> is AddSdchDictionary(), which to me changes the state of the manager and hence
> can't really be consider conceptually const.  But if you have an argument in
the
> other direction, I'd value hearing it.

To be sure I was clear, I was asking for:
const net::SdchManager* manager_;
and not asking for:
net::SdchManager* const manager_;

const on a pointer is only meant to clarify that the pointed-to-object is always
the same instance, not that the instance state won't change.  This (for example)
means you never have to wonder if the pointer is null.

I use const in cases like this as a matter of course, when I know the pointer
can be established in the constructor, and (at least the pointer would) remain
intact during the life of the object. 

Quoting from the guide:
"Consider making data members const whenever they do not need to be modified
after construction."

In this case, the data member is a pointer (not the instance that is pointed
to).

https://codereview.chromium.org/298063006/diff/40001/net/filter/mock_filter_c...
File net/filter/mock_filter_context.h (right):

https://codereview.chromium.org/298063006/diff/40001/net/filter/mock_filter_c...
net/filter/mock_filter_context.h:35: URLRequestContext*
GetModifiableURLRequestContext() { return context_.get(); }
I wasn't asking for the return value to be const, but rather the method to be
noted as const (i.e., this call does not change the instance's state):

URLRequestContext* GetModifiableURLRequestContext() const {
  return context_.get();
}

Quoting from the style guide:
"Declare methods to be const whenever possible. Accessors should almost always
be const. "

...or am I misunderstanding??

https://codereview.chromium.org/298063006/diff/40001/net/filter/sdch_filter.cc
File net/filter/sdch_filter.cc (right):

https://codereview.chromium.org/298063006/diff/40001/net/filter/sdch_filter.c...
net/filter/sdch_filter.cc:58: UMA_HISTOGRAM_COUNTS("Sdch3.PartialBytesIn",
+1 with your plan, inluding <owner> field.

On 2014/06/10 20:38:41, rdsmith wrote:
> On 2014/06/09 23:03:01, jar wrote:
> > nit: I went and looked at these histogtrams.  The dashboard indicated that
> they
> > are unowned.... but you *might* want to take ownership of some/all of these.

> > Especially interesting is the: Sdch3.ProblemCodes_4 
> 
> Will do, but in a separate CL.  Confirming: I do that by adding an "<owner>"
> field in histograms.xml?  Also, I'm not seeing the histogram in
> tools/metrics/histograms.xml; does that mean they're internal and should be
> moved external?
> 
> > This will be a place where you can see how your code is working in the field
(
> > :-/.... assuming you don't get direction from crash reports).
> 
> Yes, I need to engage with how to evaluate this stuff in the field; I haven't
> done that yet.
> 
>

[Randy Smith (Not in Mondays), 2014-06-11 20:43:38]

Jim: Comments incorporated; PTAL.

(Fair warning: I'll be doing a rebase and upload to get around try job problems,
so you may see a PS appear during your review.  But it'll only be rebase to a
different revision on trunk changes.)

https://codereview.chromium.org/298063006/diff/40001/chrome/browser/net/sdch_...
File chrome/browser/net/sdch_dictionary_fetcher.h (right):

https://codereview.chromium.org/298063006/diff/40001/chrome/browser/net/sdch_...
chrome/browser/net/sdch_dictionary_fetcher.h:62: net::SdchManager* manager_;
On 2014/06/11 20:04:43, jar wrote:
> On 2014/06/10 20:38:41, rdsmith wrote:
> > On 2014/06/09 23:03:01, jar wrote:
> > > nit: can/should this be const?
> > 
> > I'm inclined to say "no"; the method that's used on the manager from this
> class
> > is AddSdchDictionary(), which to me changes the state of the manager and
hence
> > can't really be consider conceptually const.  But if you have an argument in
> the
> > other direction, I'd value hearing it.
> 
> To be sure I was clear, I was asking for:
> const net::SdchManager* manager_;
> and not asking for:
> net::SdchManager* const manager_;
> 
> const on a pointer is only meant to clarify that the pointed-to-object is
always
> the same instance, not that the instance state won't change.  This (for
example)
> means you never have to wonder if the pointer is null.
> 
> I use const in cases like this as a matter of course, when I know the pointer
> can be established in the constructor, and (at least the pointer would) remain
> intact during the life of the object. 
> 
> Quoting from the guide:
> "Consider making data members const whenever they do not need to be modified
> after construction."
> 
> In this case, the data member is a pointer (not the instance that is pointed
> to).
> 

Sorry, that was just a braino on my part.  You're quite right.

Having said that, what's required for this to compile is the second form you
quote above, not the first.  That fits with my understanding from Stroustrap as
to how you indicate that the pointer is const rather than the underlying object,
so I went with it.  

Done.

https://codereview.chromium.org/298063006/diff/40001/net/filter/mock_filter_c...
File net/filter/mock_filter_context.h (right):

https://codereview.chromium.org/298063006/diff/40001/net/filter/mock_filter_c...
net/filter/mock_filter_context.h:35: URLRequestContext*
GetModifiableURLRequestContext() { return context_.get(); }
On 2014/06/11 20:04:43, jar wrote:
> I wasn't asking for the return value to be const, but rather the method to be
> noted as const (i.e., this call does not change the instance's state):
> 
> URLRequestContext* GetModifiableURLRequestContext() const {
>   return context_.get();
> }
> 
> Quoting from the style guide:
> "Declare methods to be const whenever possible. Accessors should almost always
> be const. "
> 
> ...or am I misunderstanding??

Sorry, another const parsing braino.  Done.

[jar (doing other things), 2014-06-12 15:26:58]

LGTM % white-space nit.

https://codereview.chromium.org/298063006/diff/100001/chrome/browser/net/sdch...
File chrome/browser/net/sdch_dictionary_fetcher.h (right):

https://codereview.chromium.org/298063006/diff/100001/chrome/browser/net/sdch...
chrome/browser/net/sdch_dictionary_fetcher.h:62: net::SdchManager * const
manager_;
nit: no space before asterisk

(...and sorry about my typo in original comment)

[Randy Smith (Not in Mondays), 2014-06-12 15:51:15]

Jim: Thanks!

Matt: Would you review/stamp the chrome/browser/profiles/* files?
Scott: Ditto for  chrome/browser/* (excluding profiles) files?

[mmenke, 2014-06-12 16:48:31]

https://codereview.chromium.org/298063006/diff/120001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/298063006/diff/120001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:534: new
net::TrivialURLRequestContextGetter(
This makes me sad, but doing something nicer seems beyond the scope of this CL.

https://codereview.chromium.org/298063006/diff/120001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:537:
content::BrowserThread::IO))));
Shouldn't you be calling main_context.set_sdch_manager?

https://codereview.chromium.org/298063006/diff/120001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:537:
content::BrowserThread::IO))));
What about the OTR profile?  It would end up using the in-memory cache, so I
think it would be safe to give its own manager, I assume.

[sky, 2014-06-12 17:06:22]

LGTM

https://codereview.chromium.org/298063006/diff/120001/chrome/browser/browser_...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/298063006/diff/120001/chrome/browser/browser_...
chrome/browser/browser_process_impl.cc:52: #include
"chrome/browser/net/sdch_dictionary_fetcher.h"
If you can, remove this include.

[Randy Smith (Not in Mondays), 2014-06-12 18:06:40]

Matt: PTAL?

https://codereview.chromium.org/298063006/diff/120001/chrome/browser/browser_...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/298063006/diff/120001/chrome/browser/browser_...
chrome/browser/browser_process_impl.cc:52: #include
"chrome/browser/net/sdch_dictionary_fetcher.h"
On 2014/06/12 17:06:22, sky wrote:
> If you can, remove this include.

Ah, thanks for catching this.  Done.

https://codereview.chromium.org/298063006/diff/120001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/298063006/diff/120001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:534: new
net::TrivialURLRequestContextGetter(
On 2014/06/12 16:48:31, mmenke wrote:
> This makes me sad, but doing something nicer seems beyond the scope of this
CL.

I chatted with Will as to whether or not this should be considered a reasonable
idiom, and his opinion was that it was a reasonable idiom
(https://codereview.chromium.org/292063002).  I don't know if that increases or
decreases your sadness :-}.

The "something nicer" would be to rewrite the dictionary fetcher to use
URLRequest rather than URLFetcher, which is a good idea for a couple of reasons,
not just this one.  But I suspect you understand why I don't want it on my
critical path :-J.

https://codereview.chromium.org/298063006/diff/120001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:537:
content::BrowserThread::IO))));
On 2014/06/12 16:48:31, mmenke wrote:
> Shouldn't you be calling main_context.set_sdch_manager?

Yikes.  Thank you.  Yes.  (And I won't land without at least smoke testing. 
This is a refactor, so I didn't add any tests, but SDCH clearly needs a browser
test.)

https://codereview.chromium.org/298063006/diff/120001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:537:
content::BrowserThread::IO))));
On 2014/06/12 16:48:31, mmenke wrote:
> What about the OTR profile?  It would end up using the in-memory cache, so I
> think it would be safe to give its own manager, I assume.

Tricky question.  I think the answer for this CL is "yes", but there's a privacy
issue that'll show up coming down the pike around this.  We're going to need to
limit memory consumed by SDCH on a global basis (http://crbug.com/374917) and
that'll provide some information leakage between profiles.  But I'll worry about
this issue at that point.  Done.

[mmenke, 2014-06-12 18:11:16]

Profiles LGTM, though yea, a browser test seems like a good idea.

https://codereview.chromium.org/298063006/diff/120001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/298063006/diff/120001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:534: new
net::TrivialURLRequestContextGetter(
On 2014/06/12 18:06:40, rdsmith wrote:
> On 2014/06/12 16:48:31, mmenke wrote:
> > This makes me sad, but doing something nicer seems beyond the scope of this
> CL.
> 
> I chatted with Will as to whether or not this should be considered a
reasonable
> idiom, and his opinion was that it was a reasonable idiom
> (https://codereview.chromium.org/292063002).  I don't know if that increases
or
> decreases your sadness :-}.
> 
> The "something nicer" would be to rewrite the dictionary fetcher to use
> URLRequest rather than URLFetcher, which is a good idea for a couple of
reasons,
> not just this one.  But I suspect you understand why I don't want it on my
> critical path :-J.

Actually, I'd be fine with using the real main request context getter - my issue
here is just with creating another one.  I'm fine with using URLFetcher for this
(For some value of "fine" - URLFetchers don't, by default, limit body sizes, so
anyone who feels like crashing Chrome may be able to do so).

https://codereview.chromium.org/298063006/diff/140001/chrome/browser/profiles...
File chrome/browser/profiles/off_the_record_profile_io_data.h (right):

https://codereview.chromium.org/298063006/diff/140001/chrome/browser/profiles...
chrome/browser/profiles/off_the_record_profile_io_data.h:150: mutable
scoped_ptr<net::SdchManager> sdch_manager_;
nit:  Need to forward declare this.

[Randy Smith (Not in Mondays), 2014-06-12 20:03:12]

Thanks, Matt!

Jim, I'm afraid I'd like another round of review on PS9; I made several changes
to get around problems in smoke testing :-{.

https://codereview.chromium.org/298063006/diff/140001/chrome/browser/profiles...
File chrome/browser/profiles/off_the_record_profile_io_data.h (right):

https://codereview.chromium.org/298063006/diff/140001/chrome/browser/profiles...
chrome/browser/profiles/off_the_record_profile_io_data.h:150: mutable
scoped_ptr<net::SdchManager> sdch_manager_;
On 2014/06/12 18:11:16, mmenke wrote:
> nit:  Need to forward declare this.

Done.

[jar (doing other things), 2014-06-13 18:28:49]

LGTM % white space nit

https://codereview.chromium.org/298063006/diff/160001/net/filter/sdch_filter.h
File net/filter/sdch_filter.h (right):

https://codereview.chromium.org/298063006/diff/160001/net/filter/sdch_filter....
net/filter/sdch_filter.h:103: const URLRequestContext * const
url_request_context_;
nit: no space before the asterisk. (like lines 43, 44, 66).

The style guide specifically asks that you not put a space on both sides.... and
leaves either side as allowed... but...

Chromium tends to standardize on putting the space *only* after the asterisk
(despite the fact that the parsing binds the asterisk more tightly to what is on
its right :-/).

[Randy Smith (Not in Mondays), 2014-06-13 19:40:12]

Thanks!

https://codereview.chromium.org/298063006/diff/160001/net/filter/sdch_filter.h
File net/filter/sdch_filter.h (right):

https://codereview.chromium.org/298063006/diff/160001/net/filter/sdch_filter....
net/filter/sdch_filter.h:103: const URLRequestContext * const
url_request_context_;
On 2014/06/13 18:28:48, jar wrote:
> nit: no space before the asterisk. (like lines 43, 44, 66).
> 
> The style guide specifically asks that you not put a space on both sides....
and
> leaves either side as allowed... but...
> 
> Chromium tends to standardize on putting the space *only* after the asterisk
> (despite the fact that the parsing binds the asterisk more tightly to what is
on
> its right :-/).

Whoops; done.

[Randy Smith (Not in Mondays), 2014-06-13 19:40:19]

The CQ bit was checked by rdsmith@chromium.org

[commit-bot: I haz the power, 2014-06-13 19:41:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rdsmith@chromium.org/298063006/180001

[commit-bot: I haz the power, 2014-06-14 02:49:17]

Change committed as 277160

[viettrungluu, 2014-06-14 07:20:52]

On 2014/06/14 02:49:17, I haz the power (commit-bot) wrote:
> Change committed as 277160

Reverted r277185 due to LSan failure:

http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te...

Indirect leak of 34 byte(s) in 1 object(s) allocated from:
#0 0x512b7b in operator new(unsigned long)
/usr/local/google/work/chromium/src/third_party/llvm/projects/compiler-rt/lib/asan/asan_new_delete.cc:55
#1 0x7f9bbc3a6739 in __gnu_cxx::new_allocator<char>::allocate(unsigned long,
void const*)
/build/buildd/gcc-4.6-4.6.3/build/x86_64-linux-gnu/libstdc++-v3/include/ext/new_allocator.h:92
#2 0x7f9bbc3a5d2c in std::string::_Rep::_S_create(unsigned long, unsigned long,
std::allocator<char> const&)
/build/buildd/gcc-4.6-4.6.3/build/x86_64-linux-gnu/libstdc++-v3/include/bits/basic_string.tcc:609
#3 0x7f9bbc3a5f04 in std::string::_Rep::_M_clone(std::allocator<char> const&,
unsigned long)
/build/buildd/gcc-4.6-4.6.3/build/x86_64-linux-gnu/libstdc++-v3/include/bits/basic_string.tcc:631
#4 0x7f9bbc3a2c48 in std::string::reserve(unsigned long)
/build/buildd/gcc-4.6-4.6.3/build/x86_64-linux-gnu/libstdc++-v3/include/bits/basic_string.tcc:512
#5 0x7f9bbc3a3391 in std::string::append(unsigned long, char)
/build/buildd/gcc-4.6-4.6.3/build/x86_64-linux-gnu/libstdc++-v3/include/bits/basic_string.tcc:290
#6 0x7f9bbc3a2aeb in std::string::resize(unsigned long, char)
/build/buildd/gcc-4.6-4.6.3/build/x86_64-linux-gnu/libstdc++-v3/include/bits/basic_string.tcc:647
#7 0x31d6642 in resize
/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/bits/basic_string.h:749
#8 0x31d6642 in base::Base64Encode(base::BasicStringPiece<std::string> const&,
std::string*) base/base64.cc:13
#9 0x34a1d05 in net::SdchManager::UrlSafeBase64Encode(std::string const&,
std::string*) net/base/sdch_manager.cc:541
#10 0x34a1124 in net::SdchManager::GenerateHash(std::string const&,
std::string*, std::string*) net/base/sdch_manager.cc:508
#11 0x349facb in net::SdchManager::AddSdchDictionary(std::string const&, GURL
const&) net/base/sdch_manager.cc:363
#12 0x803e06 in net::SdchManagerTest_CanUseMultipleDictionaries_Test::TestBody()
net/base/sdch_manager_unittest.cc:403
[...]

[Randy Smith (Not in Mondays), 2014-06-15 01:59:07]

Viet-Trung: Willing to give me a stamp for the change made to avoid the leak?

[mmenke, 2014-06-15 02:50:33]

On 2014/06/15 01:59:07, rdsmith wrote:
> Viet-Trung: Willing to give me a stamp for the change made to avoid the leak?

I'm happy to do that...  But...umm...  What does patch set 11 do, exactly?  It's
just a merge, right?  I'm not following the description.

[Randy Smith (Not in Mondays), 2014-06-15 03:06:49]

On 2014/06/15 02:50:33, mmenke wrote:
> On 2014/06/15 01:59:07, rdsmith wrote:
> > Viet-Trung: Willing to give me a stamp for the change made to avoid the
leak?
> 
> I'm happy to do that...  But...umm...  What does patch set 11 do, exactly? 
It's
> just a merge, right?  I'm not following the description.

PS10's a merge.  PS11 converts a bare pointer into a scoped_ptr<> in the new
test I added for multiple SdchManagers.  (Best memory leak is obvious memory
leak?  :-J.)

[Randy Smith (Not in Mondays), 2014-06-15 03:07:24]

On 2014/06/15 03:06:49, rdsmith wrote:
> On 2014/06/15 02:50:33, mmenke wrote:
> > On 2014/06/15 01:59:07, rdsmith wrote:
> > > Viet-Trung: Willing to give me a stamp for the change made to avoid the
> leak?
> > 
> > I'm happy to do that...  But...umm...  What does patch set 11 do, exactly? 
> It's
> > just a merge, right?  I'm not following the description.
> 
> PS10's a merge.  PS11 converts a bare pointer into a scoped_ptr<> in the new
> test I added for multiple SdchManagers.  (Best memory leak is obvious memory
> leak?  :-J.)

Whoops.  PS11's a merge, PS12 does the conversion.

[mmenke, 2014-06-15 03:12:36]

On 2014/06/15 03:07:24, rdsmith wrote:
> On 2014/06/15 03:06:49, rdsmith wrote:
> > On 2014/06/15 02:50:33, mmenke wrote:
> > > On 2014/06/15 01:59:07, rdsmith wrote:
> > > > Viet-Trung: Willing to give me a stamp for the change made to avoid the
> > leak?
> > > 
> > > I'm happy to do that...  But...umm...  What does patch set 11 do, exactly?

> > It's
> > > just a merge, right?  I'm not following the description.
> > 
> > PS10's a merge.  PS11 converts a bare pointer into a scoped_ptr<> in the new
> > test I added for multiple SdchManagers.  (Best memory leak is obvious memory
> > leak?  :-J.)
> 
> Whoops.  PS11's a merge, PS12 does the conversion.

PS12 LGTM, but is there any reason it needs a new dictionary?  The test fixture
is re-created for each test, so can just use sdch_manager(), no?

[mmenke, 2014-06-15 03:13:58]

On 2014/06/15 03:12:36, mmenke wrote:
> On 2014/06/15 03:07:24, rdsmith wrote:
> > On 2014/06/15 03:06:49, rdsmith wrote:
> > > On 2014/06/15 02:50:33, mmenke wrote:
> > > > On 2014/06/15 01:59:07, rdsmith wrote:
> > > > > Viet-Trung: Willing to give me a stamp for the change made to avoid
the
> > > leak?
> > > > 
> > > > I'm happy to do that...  But...umm...  What does patch set 11 do,
exactly?
> 
> > > It's
> > > > just a merge, right?  I'm not following the description.
> > > 
> > > PS10's a merge.  PS11 converts a bare pointer into a scoped_ptr<> in the
new
> > > test I added for multiple SdchManagers.  (Best memory leak is obvious
memory
> > > leak?  :-J.)
> > 
> > Whoops.  PS11's a merge, PS12 does the conversion.
> 
> PS12 LGTM, but is there any reason it needs a new dictionary?  The test
fixture
> is re-created for each test, so can just use sdch_manager(), no?

Oh, nevermind...Was misreading "second_manager" as "sdch_manager"....Ok, makes
sense (Though doesn't need to be a scoped_ptr.  Could just stick it on the
stack...Less risk of leaks).  LGTM, regardless.

[Randy Smith (Not in Mondays), 2014-06-15 13:20:09]

On 2014/06/15 03:13:58, mmenke wrote:
> On 2014/06/15 03:12:36, mmenke wrote:
> > On 2014/06/15 03:07:24, rdsmith wrote:
> > > On 2014/06/15 03:06:49, rdsmith wrote:
> > > > On 2014/06/15 02:50:33, mmenke wrote:
> > > > > On 2014/06/15 01:59:07, rdsmith wrote:
> > > > > > Viet-Trung: Willing to give me a stamp for the change made to avoid
> the
> > > > leak?
> > > > > 
> > > > > I'm happy to do that...  But...umm...  What does patch set 11 do,
> exactly?
> > 
> > > > It's
> > > > > just a merge, right?  I'm not following the description.
> > > > 
> > > > PS10's a merge.  PS11 converts a bare pointer into a scoped_ptr<> in the
> new
> > > > test I added for multiple SdchManagers.  (Best memory leak is obvious
> memory
> > > > leak?  :-J.)
> > > 
> > > Whoops.  PS11's a merge, PS12 does the conversion.
> > 
> > PS12 LGTM, but is there any reason it needs a new dictionary?  The test
> fixture
> > is re-created for each test, so can just use sdch_manager(), no?
> 
> Oh, nevermind...Was misreading "second_manager" as "sdch_manager"....Ok, makes
> sense (Though doesn't need to be a scoped_ptr.  Could just stick it on the
> stack...Less risk of leaks).  LGTM, regardless.

Ah, good point.  Done.  (Also changed the test name, as it's multiple managers,
not multiple dictionaries, but I don't think that requires a re-stamp.)

[Randy Smith (Not in Mondays), 2014-06-15 13:20:17]

The CQ bit was checked by rdsmith@chromium.org

[commit-bot: I haz the power, 2014-06-15 13:20:46]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rdsmith@chromium.org/298063006/240001

[commit-bot: I haz the power, 2014-06-15 16:02:30]

Change committed as 277322