Restructure SDCH layering to allow more separation (observer/1->[0,n]

Restructure SDCH layering to allow more separation (observer/1->[0,n]
relationship) between SdchManager and embedder classes.  This allows
cleanly setting up the embedder class after SdchManager construction and
tearing it down before SdchManager destruction.

BUG=414888
R=rsleevi@chromium.org

Committed: https://crrev.com/47c133b16551d5b2b24bb9536b696f2f283403e6
Cr-Commit-Position: refs/heads/master@{#303058}

[Randy Smith (Not in Mondays), 2014-10-20 22:16:09]

Ryan: As I'm sure you're aware, expressing opinions will sometimes mean that
people come back to you for opinions on the same topic :-}.  Willing to give
this CL a look?

One inline question for you, not about this CL but about a hypothetical future
one in which I deal with the authentication issues.

https://codereview.chromium.org/664263002/diff/1/net/base/sdch_observer.h
File net/base/sdch_observer.h (right):

https://codereview.chromium.org/664263002/diff/1/net/base/sdch_observer.h#new...
net/base/sdch_observer.h:23: virtual void OnGetDictionary(SdchManager* manager,
Question for future CLs, not this one: I had originally tried to make this
notification pass a URLRequest, so that the fetching could be done in the
authentication context that resulted in the Get-Dictionary notification (wasn't
planning to do the authentication code, just to make it possible to do it in the
future).  But the notification went through SdchManager, which isn't by net/
internal layering allowed to know about URLRequests.  I tried to cheat by
leaving it an opaque type just passed by reference at the SdchManager interface,
but then sdch_manager_unittest.cc needed to know about it.  I tried to make it a
simple test jig in sdch_manager_unittest.cc, and got linking errors.  At that
point I threw up my hands, said "I don't need this functionality for this CL",
and decided to ask for advice.  Any ideas?

If I go truth&beauty on this, I say that indirecting the Get-Dictionary
notification through the SdchManager is a bit hacky because it's the storage
object, and the Get-Dictionary notification has nothing to do with storage.  But
that suggests creating another per-BrowserContext/Profile object in the SDCH
subsystem who's only purpose is to indirect that notification, and then I have
to figure out what directory it goes in and who owns it.  That struck me as ugly
too.  So at the moment I'm thinking that the right implementation is pretty much
what I tried to do above, and accept that the SdchManager unit tests need to
know about things that aren't appropriate to know in net/base, and that's
basically ok.  But you may well have better ideas.

[Ryan Sleevi, 2014-10-20 22:42:05]

Perhaps it's residual flu, but I don't understand your truth & beauty problem.
Perhaps you could rephrase it and I might be able to take a second stab at
understanding?

Various style bits below.

https://codereview.chromium.org/664263002/diff/20001/net/base/sdch_manager.cc
File net/base/sdch_manager.cc (right):

https://codereview.chromium.org/664263002/diff/20001/net/base/sdch_manager.cc...
net/base/sdch_manager.cc:504: observer->RemoveRef();
This sort of manual ref-counting strikes me as all sorts of danger zone.

Why is it necessary to AddRef/RemoveRef? Why isn't the Observer simply unowned?

https://codereview.chromium.org/664263002/diff/20001/net/base/sdch_manager.h
File net/base/sdch_manager.h (right):

https://codereview.chromium.org/664263002/diff/20001/net/base/sdch_manager.h#...
net/base/sdch_manager.h:44: : public NON_EXPORTED_BASE(base::NonThreadSafe) {
Any chance we can remove this inheritance as part of the cleanup? :D

https://codereview.chromium.org/664263002/diff/20001/net/base/sdch_manager.h#...
net/base/sdch_manager.h:333: // Return true if fetch is legal.
Documentation needs more. I have trouble understanding what this is doing just
from reading the header.

https://codereview.chromium.org/664263002/diff/20001/net/base/sdch_observer.cc
File net/base/sdch_observer.cc (right):

https://codereview.chromium.org/664263002/diff/20001/net/base/sdch_observer.c...
net/base/sdch_observer.cc:14: // that they are interested in.
I'd rather be explicit and force classes to consume/ignore notifications. This
is in keeping in line with our avoidance of impl-inheritence.

https://codereview.chromium.org/664263002/diff/20001/net/base/sdch_observer.h
File net/base/sdch_observer.h (right):

https://codereview.chromium.org/664263002/diff/20001/net/base/sdch_observer.h...
net/base/sdch_observer.h:25: const GURL& dictionary_url);
Why aren't these pure virtual methods, if this is meant to be an interface base
class?

https://codereview.chromium.org/664263002/diff/20001/net/base/sdch_observer.h...
net/base/sdch_observer.h:32: SdchObserver();
STYLE: Why isn't this ctor, dtor, methods?

https://codereview.chromium.org/664263002/diff/20001/net/base/sdch_observer.h...
net/base/sdch_observer.h:42: int reference_count_;
This is all sorts of danger zone for me. I need a solid explanation why you're
inventing ref-count semantics here, for a class with a public destructor.

https://codereview.chromium.org/664263002/diff/20001/net/url_request/sdch_dic...
File net/url_request/sdch_dictionary_fetcher.h (right):

https://codereview.chromium.org/664263002/diff/20001/net/url_request/sdch_dic...
net/url_request/sdch_dictionary_fetcher.h:48: OnDictionaryFetchedCallback
callback);
Callbacks get passed as const-ref

https://codereview.chromium.org/664263002/diff/20001/net/url_request/sdch_dic...
net/url_request/sdch_dictionary_fetcher.h:52: virtual void Schedule(const GURL&
dictionary_url);
Why is this virtual, if it's no longer an override? Ditto line 55.

[Randy Smith (Not in Mondays), 2014-10-21 19:05:37]

Comments responded to/incorporated.  

WRT the truth&beauty question, maybe I need to tweak my rsleevi@ emulation;
wouldn't be the first time :-}.  Here's a layered breakdown:

1. Problem (is not and does not need to be addressed in this CL): Given the
embedder enough information when it is asked to fetch a dictionary that it can
figure out appropriate authentication methods.

2. Solution: Make the embedder aware of the URLRequest that resulted in the
dictionary fetch and let it figure out what authentication that implies.  

3. Attempted implementation: Make the signature of SdchObserver::OnGetDictionary
include a URLRequest&.

4. Problem with attempted implementation: The notification passes through the
SdchManager, which is in net/base, and hence can't know about URLRequest.

5. Attempted resolution to (4): Make signature of OnGetDictionary make a
reference to URLRequest, but just satisfy that with a forward declaration; no
need for sdch_manager.cc to #include url_request.h.

6. Problem with (5): No really good way to test the dispatch through
SdchManager.

7. Possible resolution to (6): Violate directory layering by having only the
sdch_manager_unittest.cc include url_request.h. (I ran into linking problems
trying to have a mock definition of URLRequest in sdch_manager_unittest.cc.)

My mental model was that you'd be fine with 1-3, find 4&5 questionable, and
actively object to (7).  If that's wrong (if 5&7 are ok) I'm good.  In my email
to you I presumed I was right, and gave you a sketch of my initial, blocked
thoughts as to how to resolve your assumed objection to (7), which was to step
back a step and say that the SdchManager is a storage object and shouldn't be
indirecting Response notifications, but that seeming to lead to a pretty
cardboard extra object that I had no idea where to put.  So I was asking you
what I should do.  But instead I'll ask: How's my rsleevi@ model? :-} :-|

https://codereview.chromium.org/664263002/diff/20001/net/base/sdch_manager.cc
File net/base/sdch_manager.cc (right):

https://codereview.chromium.org/664263002/diff/20001/net/base/sdch_manager.cc...
net/base/sdch_manager.cc:504: observer->RemoveRef();
On 2014/10/20 22:42:04, Ryan Sleevi wrote:
> This sort of manual ref-counting strikes me as all sorts of danger zone.
> 
> Why is it necessary to AddRef/RemoveRef? Why isn't the Observer simply
unowned?

First, I want to call out that the Add/RemoveRef() on the observer are private
methods that are not visible to inherited classes/part of the Observer
interface.  I.e. this complexity has limited scope, only existing within the
SdchObserver (base class) and SdchManager.  The reason for it is purely because
of the DCHECK in the base class destructor; it's easy for a class to add itself
as an observer and forget to remove oneself before destruction, which leads
directly to a use-after-free error.  

I can remove it if you want, but my judgement was that complexity limited to the
implementation which raised a clear flag in the case of a use-after-free error
was a reasonable tradeoff.  If you have a different judgement, I'd value your
explaining it to me, though as I say, I'm happy to remove it if you don't like
it.

https://codereview.chromium.org/664263002/diff/20001/net/base/sdch_manager.h
File net/base/sdch_manager.h (right):

https://codereview.chromium.org/664263002/diff/20001/net/base/sdch_manager.h#...
net/base/sdch_manager.h:44: : public NON_EXPORTED_BASE(base::NonThreadSafe) {
On 2014/10/20 22:42:04, Ryan Sleevi wrote:
> Any chance we can remove this inheritance as part of the cleanup? :D

I'm happy to, but I want to understand the rationale.   I would think assertions
that we're operating on a single thread would be good, and we can't use
content::BrowserThread::CurrentlyOn() in net/?

https://codereview.chromium.org/664263002/diff/20001/net/base/sdch_manager.h#...
net/base/sdch_manager.h:333: // Return true if fetch is legal.
On 2014/10/20 22:42:04, Ryan Sleevi wrote:
> Documentation needs more. I have trouble understanding what this is doing just
> from reading the header.

Done, but note that the change in this CL is making this function private when
before it was public.  So I might argue that no comment is needed, as it's no
longer part of the interface.

https://codereview.chromium.org/664263002/diff/20001/net/base/sdch_observer.cc
File net/base/sdch_observer.cc (right):

https://codereview.chromium.org/664263002/diff/20001/net/base/sdch_observer.c...
net/base/sdch_observer.cc:14: // that they are interested in.
On 2014/10/20 22:42:04, Ryan Sleevi wrote:
> I'd rather be explicit and force classes to consume/ignore notifications. This
> is in keeping in line with our avoidance of impl-inheritence.

Define "our"?  This pattern is all through the content API.  (More detail in
other comment on this issue.)

(Which translates to "Ok, but tell me one more time just so I know you've read
the counter-argument; I don't have strong feelings.")

https://codereview.chromium.org/664263002/diff/20001/net/base/sdch_observer.h
File net/base/sdch_observer.h (right):

https://codereview.chromium.org/664263002/diff/20001/net/base/sdch_observer.h...
net/base/sdch_observer.h:25: const GURL& dictionary_url);
On 2014/10/20 22:42:04, Ryan Sleevi wrote:
> Why aren't these pure virtual methods, if this is meant to be an interface
base
> class?

Ah, this issue :-}.  So I'm following where John Abd-El Malek ended up with the
content refactoring, which is that making things easy for the subclass is the
overarching virtue.  There may be subclasses of SdchObserver that care about
OnGetDictionary but not about OnClearDictionaries, or the reverse, and it's
certainly possible that there'll be subclasses that don't care about
"OnDictionaryUsed()" (which I'll be adding for doing prioritization, and
which'll fire a lot).  More abstractly: It isn't functionally necessary for an
observer to implement any particular interface, since this isn't a delegation
relationship; observers are subscribing for notifications they are interested
in, and not implementing a particular method is how a lack of interest is
indicated.

The counter-argument that I see is that in the current design, we really only
expect there to be a single SdchObserver, and it might be useful to get a
compilation error if it doesn't implement an observer interface, since that
might mean we can get rid of that observer interface.

https://codereview.chromium.org/664263002/diff/20001/net/base/sdch_observer.h...
net/base/sdch_observer.h:32: SdchObserver();
On 2014/10/20 22:42:04, Ryan Sleevi wrote:
> STYLE: Why isn't this ctor, dtor, methods?

No clue; maybe my fingers slipped? :-}.  Fixed.

https://codereview.chromium.org/664263002/diff/20001/net/base/sdch_observer.h...
net/base/sdch_observer.h:42: int reference_count_;
On 2014/10/20 22:42:04, Ryan Sleevi wrote:
> This is all sorts of danger zone for me. I need a solid explanation why you're
> inventing ref-count semantics here, for a class with a public destructor.

See response in SdchManager to the dual of this question.

https://codereview.chromium.org/664263002/diff/20001/net/url_request/sdch_dic...
File net/url_request/sdch_dictionary_fetcher.h (right):

https://codereview.chromium.org/664263002/diff/20001/net/url_request/sdch_dic...
net/url_request/sdch_dictionary_fetcher.h:48: OnDictionaryFetchedCallback
callback);
On 2014/10/20 22:42:04, Ryan Sleevi wrote:
> Callbacks get passed as const-ref

Ooops.  Thanks.  Done.

https://codereview.chromium.org/664263002/diff/20001/net/url_request/sdch_dic...
net/url_request/sdch_dictionary_fetcher.h:52: virtual void Schedule(const GURL&
dictionary_url);
On 2014/10/20 22:42:04, Ryan Sleevi wrote:
> Why is this virtual, if it's no longer an override? Ditto line 55.

Good catch.  Done.

[Ryan Sleevi, 2014-10-21 23:09:29]

So, I don't agree with 3, but that's precisely because of the problem you
highlight in 4.

I'm not sure there's value in it, or at least, I'm not sure it's the right
approach. If you are going to go down the route of being able to associate what
URLRequest triggered a given dictionary fetch, it's a bigger issue than
collating URLRequests, I believe.

Consider for example if I have two URLRequests that request /foo and /bar, and
both see a Get-Dictionary advertise /baz.

On the IO thread, it works such that
/foo request is made
  Get-Dictionary /baz
  SdchDictionaryFetcher::Schedule("/baz")
/bar request is made
  Get-Dictionary /baz
  SdchDictionaryFetcher::Schedule("/baz")

When /baz is requested, who is the responsible request? Arguably, both.

I don't have the bandwidth to help reason out the right layering any time soon
on this, unfortunately, and I wasn't looking to own some of this SDCH stuff. We
should find someone who is interested - or at least has better bandwidth to
reason about some of these design issues.

Nominally we could do some type-erasing smuggling via SupportsUserData, such
that there's some form of context object associated with the "/baz" request that
SupportsUserData, and the embedder can set a ChromeURLRequestContext key to
point to the 'original' (or set of original) requests in a way that's opaque to
the manager/fetcher. But that's a very off the cuff solution.

https://codereview.chromium.org/664263002/diff/20001/net/base/sdch_manager.cc
File net/base/sdch_manager.cc (right):

https://codereview.chromium.org/664263002/diff/20001/net/base/sdch_manager.cc...
net/base/sdch_manager.cc:504: observer->RemoveRef();
On 2014/10/21 19:05:36, rdsmith wrote:
> On 2014/10/20 22:42:04, Ryan Sleevi wrote:
> > This sort of manual ref-counting strikes me as all sorts of danger zone.
> > 
> > Why is it necessary to AddRef/RemoveRef? Why isn't the Observer simply
> unowned?
> 
> First, I want to call out that the Add/RemoveRef() on the observer are private
> methods that are not visible to inherited classes/part of the Observer
> interface.  I.e. this complexity has limited scope, only existing within the
> SdchObserver (base class) and SdchManager.  The reason for it is purely
because
> of the DCHECK in the base class destructor; it's easy for a class to add
itself
> as an observer and forget to remove oneself before destruction, which leads
> directly to a use-after-free error.  
> 
> I can remove it if you want, but my judgement was that complexity limited to
the
> implementation which raised a clear flag in the case of a use-after-free error
> was a reasonable tradeoff.  If you have a different judgement, I'd value your
> explaining it to me, though as I say, I'm happy to remove it if you don't like
> it.
> 

I have a different judgement. I dislike re-inventing these semantics for a
non-demonstrated problem. I'm all for removing footguns, but I'm not a fan of
'diagnostic' aids being littered in code. In particular, your implementation
doesn't adhere to what we'd "normally" expect from ref-counting (vis-a-vis
thread-safety), and comes with its own set of surprises. For example, does this
mean it's now valid to add the same observer multiple times?

The solution to this problem in terms of defense in depth is to use an RAII
pattern, not to use the ref-counting. Ensuring that deregistration is handled as
part of destruction, rather than has to happen before destruction, is a way to
guarantee the right rules are observed.

https://codereview.chromium.org/664263002/diff/20001/net/base/sdch_manager.h
File net/base/sdch_manager.h (right):

https://codereview.chromium.org/664263002/diff/20001/net/base/sdch_manager.h#...
net/base/sdch_manager.h:44: : public NON_EXPORTED_BASE(base::NonThreadSafe) {
On 2014/10/21 19:05:36, rdsmith wrote:
> On 2014/10/20 22:42:04, Ryan Sleevi wrote:
> > Any chance we can remove this inheritance as part of the cleanup? :D
> 
> I'm happy to, but I want to understand the rationale.   I would think
assertions
> that we're operating on a single thread would be good, and we can't use
> content::BrowserThread::CurrentlyOn() in net/?

Because generally trait-inheritance requires concrete inheritence, which becomes
a code smell.

This is where having a ThreadChecker member over exposing NonThreadSafe is
useful here.

(Having classes inheriting from NonThreadSafe caused issues with RefCounted
cleanups, because of how NonThreadSafe exposed its dtors. Multiple inheritance
messiness)

https://codereview.chromium.org/664263002/diff/20001/net/base/sdch_observer.cc
File net/base/sdch_observer.cc (right):

https://codereview.chromium.org/664263002/diff/20001/net/base/sdch_observer.c...
net/base/sdch_observer.cc:14: // that they are interested in.
On 2014/10/21 19:05:36, rdsmith wrote:
> On 2014/10/20 22:42:04, Ryan Sleevi wrote:
> > I'd rather be explicit and force classes to consume/ignore notifications.
This
> > is in keeping in line with our avoidance of impl-inheritence.
> 
> Define "our"?  This pattern is all through the content API.  (More detail in
> other comment on this issue.)
> 
> (Which translates to "Ok, but tell me one more time just so I know you've read
> the counter-argument; I don't have strong feelings.")

See
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Multiple_Inhe...
and then the definition of "Pure Interface" in
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Interfaces

[Randy Smith (Not in Mondays), 2014-10-23 20:57:07]

Comments incorporated; PTAL?  (I'm taking the URLRequest notification issue into
email since they aren't relevant to this CL, and getting less so as we talk.)

https://codereview.chromium.org/664263002/diff/20001/net/base/sdch_manager.cc
File net/base/sdch_manager.cc (right):

https://codereview.chromium.org/664263002/diff/20001/net/base/sdch_manager.cc...
net/base/sdch_manager.cc:504: observer->RemoveRef();
On 2014/10/21 23:09:29, Ryan Sleevi wrote:
> On 2014/10/21 19:05:36, rdsmith wrote:
> > On 2014/10/20 22:42:04, Ryan Sleevi wrote:
> > > This sort of manual ref-counting strikes me as all sorts of danger zone.
> > > 
> > > Why is it necessary to AddRef/RemoveRef? Why isn't the Observer simply
> > unowned?
> > 
> > First, I want to call out that the Add/RemoveRef() on the observer are
private
> > methods that are not visible to inherited classes/part of the Observer
> > interface.  I.e. this complexity has limited scope, only existing within the
> > SdchObserver (base class) and SdchManager.  The reason for it is purely
> because
> > of the DCHECK in the base class destructor; it's easy for a class to add
> itself
> > as an observer and forget to remove oneself before destruction, which leads
> > directly to a use-after-free error.  
> > 
> > I can remove it if you want, but my judgement was that complexity limited to
> the
> > implementation which raised a clear flag in the case of a use-after-free
error
> > was a reasonable tradeoff.  If you have a different judgement, I'd value
your
> > explaining it to me, though as I say, I'm happy to remove it if you don't
like
> > it.
> > 
> 
> I have a different judgement. I dislike re-inventing these semantics for a
> non-demonstrated problem. I'm all for removing footguns, but I'm not a fan of
> 'diagnostic' aids being littered in code. In particular, your implementation
> doesn't adhere to what we'd "normally" expect from ref-counting (vis-a-vis
> thread-safety), and comes with its own set of surprises. For example, does
this
> mean it's now valid to add the same observer multiple times?
> 
> The solution to this problem in terms of defense in depth is to use an RAII
> pattern, not to use the ref-counting. Ensuring that deregistration is handled
as
> part of destruction, rather than has to happen before destruction, is a way to
> guarantee the right rules are observed.

Done.  

(I tried to do an RAII pattern where SdchObserver construction took an
SdchManager pointer and auto-added itself, but that caused a lot of grief in the
tests, where you'd like the observer to own the manager, and possibly several
managers, so I dropped it :-J.  I could also imagine it triggering the
implementation inheritance issues you raised elsewhere, though if the RAII isn't
implemented in SdchObserver, it doesn't constrain the subclasses.)

https://codereview.chromium.org/664263002/diff/20001/net/base/sdch_manager.h
File net/base/sdch_manager.h (right):

https://codereview.chromium.org/664263002/diff/20001/net/base/sdch_manager.h#...
net/base/sdch_manager.h:44: : public NON_EXPORTED_BASE(base::NonThreadSafe) {
On 2014/10/21 23:09:29, Ryan Sleevi wrote:
> On 2014/10/21 19:05:36, rdsmith wrote:
> > On 2014/10/20 22:42:04, Ryan Sleevi wrote:
> > > Any chance we can remove this inheritance as part of the cleanup? :D
> > 
> > I'm happy to, but I want to understand the rationale.   I would think
> assertions
> > that we're operating on a single thread would be good, and we can't use
> > content::BrowserThread::CurrentlyOn() in net/?
> 
> Because generally trait-inheritance requires concrete inheritence, which
becomes
> a code smell.
> 
> This is where having a ThreadChecker member over exposing NonThreadSafe is
> useful here.
> 
> (Having classes inheriting from NonThreadSafe caused issues with RefCounted
> cleanups, because of how NonThreadSafe exposed its dtors. Multiple inheritance
> messiness)

Done.

https://codereview.chromium.org/664263002/diff/20001/net/base/sdch_observer.cc
File net/base/sdch_observer.cc (right):

https://codereview.chromium.org/664263002/diff/20001/net/base/sdch_observer.c...
net/base/sdch_observer.cc:14: // that they are interested in.
On 2014/10/21 23:09:29, Ryan Sleevi wrote:
> On 2014/10/21 19:05:36, rdsmith wrote:
> > On 2014/10/20 22:42:04, Ryan Sleevi wrote:
> > > I'd rather be explicit and force classes to consume/ignore notifications.
> This
> > > is in keeping in line with our avoidance of impl-inheritence.
> > 
> > Define "our"?  This pattern is all through the content API.  (More detail in
> > other comment on this issue.)
> > 
> > (Which translates to "Ok, but tell me one more time just so I know you've
read
> > the counter-argument; I don't have strong feelings.")
> 
> See
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Multiple_Inhe...
> and then the definition of "Pure Interface" in
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Interfaces
> 

Done.

[Randy Smith (Not in Mondays), 2014-10-28 19:41:42]

rdsmith@chromium.org changed reviewers:
+ bnc@chromium.org

[Randy Smith (Not in Mondays), 2014-10-28 19:41:43]

Ryan: When you get a chance, please see if the changes I've made are ok with you
on a net-architecture level.

Bence: Could you take a look at this CL when you have a chance (lower priority
than the iOS disable).   I'm using Ryan for larger scale net architecture
questions, and I'm hoping you can help me more with the SDCH internal structure.
 The CL is a restructuring of the embedder/SDCH relationship s.t. a) we can
avoid the dependency loop of url requests depending on SdchManager owning
SdchDictionaryFetcher depending on URLRequests (SdchManager doesn't depend on
observers of it; they can handle or ignore notifications as they wish), and b)
moves a bit closer to being able to expose URLRequests that result in dictionary
fetches to the embedder (for authentication & etc.)  Goal (b) isn't handled in
this CL and is still a bit off; see the interchange between Ryan and I in the
top-level comments :-}.

Thanks for doing this!

[Bence, 2014-10-29 15:46:31]

Please consider updating the Description with BUG=414888.

[Randy Smith (Not in Mondays), 2014-10-29 15:48:58]

On 2014/10/29 15:46:31, Bence wrote:
> Please consider updating the Description with BUG=414888.

Ooops; done.

[Randy Smith (Not in Mondays), 2014-10-31 17:24:24]

Bence, Ryan: Ping?  Just making sure this hasn't fallen off your radar screens.

[Bence, 2014-10-31 17:30:36]

On 2014/10/31 17:24:24, rdsmith wrote:
> Bence, Ryan: Ping?  Just making sure this hasn't fallen off your radar
screens.

LGTM

[Ryan Sleevi, 2014-11-04 21:40:45]

I have not reviewed the tests, assuming Bence has done so. If that's not the
case, please let me know and I'll take a closer look.

Overall, I think this is fine (having stepped away and paged this info back in).
If you do want to be able to associate with the URLRequestContexts, let me know,
but I suspect we can accomplish this via SupportsUserData and friends (or the
conceptual equivalent of a boost::any sort of type)

Mostly, the issues here are comment or style nits.

https://codereview.chromium.org/664263002/diff/100001/chrome/browser/net/chro...
File chrome/browser/net/chrome_sdch_policy.cc (right):

https://codereview.chromium.org/664263002/diff/100001/chrome/browser/net/chro...
chrome/browser/net/chrome_sdch_policy.cc:15: // of |fetcher_|.
1) I'm not sure what this comment is communicating that isn't "obvious" from the
code
.. but also

2) Is this true if fetcher_ were to PostTask something, and then this object
were deleted? The posted task would still be on the queue, indicating that a
WeakPtr should be used. Which then messes things up vis-a-vis WeakPtrFactory
(since the factory must be last), meaning you'd probably need fetcher_ to be
scoped_ptr rather than member.

Just the initial reaction from this code (not having dug into fetcher_ yet)

https://codereview.chromium.org/664263002/diff/100001/chrome/browser/net/sdch...
File chrome/browser/net/sdch_browsertest.cc (right):

https://codereview.chromium.org/664263002/diff/100001/chrome/browser/net/sdch...
chrome/browser/net/sdch_browsertest.cc:263: class SdchBrowserTest :
did git cl-format do this?

It seems that : public is the dominant form (see
https://code.google.com/p/chromium/codesearch#search/&q=%5Cs%5Cs:%5Cspublic%2...
 compared with
https://code.google.com/p/chromium/codesearch#search/&q=%5E%5Cw*class%5Cs%5Cw...
)

https://codereview.chromium.org/664263002/diff/100001/net/base/sdch_manager.h
File net/base/sdch_manager.h (right):

https://codereview.chromium.org/664263002/diff/100001/net/base/sdch_manager.h...
net/base/sdch_manager.h:39: class URLRequest;
Unneeded now, right?

https://codereview.chromium.org/664263002/diff/100001/net/base/sdch_manager.h...
net/base/sdch_manager.h:41:
//------------------------------------------------------------------------------
delete

https://codereview.chromium.org/664263002/diff/100001/net/base/sdch_manager.h...
net/base/sdch_manager.h:43: class NET_EXPORT SdchManager {
Documentation?

https://codereview.chromium.org/664263002/diff/100001/net/base/sdch_observer.h
File net/base/sdch_observer.h (right):

https://codereview.chromium.org/664263002/diff/100001/net/base/sdch_observer....
net/base/sdch_observer.h:18: class URLRequest;
Unnecessary, right?

https://codereview.chromium.org/664263002/diff/100001/net/base/sdch_observer....
net/base/sdch_observer.h:20: class NET_EXPORT SdchObserver {
Document?

Seems like lines 5-6 should be moved to 19/20

https://codereview.chromium.org/664263002/diff/100001/net/url_request/sdch_di...
File net/url_request/sdch_dictionary_fetcher.h (right):

https://codereview.chromium.org/664263002/diff/100001/net/url_request/sdch_di...
net/url_request/sdch_dictionary_fetcher.h:6: // to the net/ embedder for correct
implementaiton of authentication.
s/implementaiton/implementation/

https://codereview.chromium.org/664263002/diff/100001/net/url_request/sdch_di...
net/url_request/sdch_dictionary_fetcher.h:36: : public URLRequest::Delegate,
Compare with how you indented the other class. Pretty sure this is right.

[Bence, 2014-11-04 22:01:44]

On 2014/11/04 21:40:45, Ryan Sleevi wrote:
> I have not reviewed the tests, assuming Bence has done so. If that's not the
> case, please let me know and I'll take a closer look.
> 

Ryan:  I have indeed looked at the tests, and they look fine as far as I can
tell, but I would be grateful if you could also take a look at them.  Thank you.

[Randy Smith (Not in Mondays), 2014-11-05 20:35:02]

Ryan: I've incorporated your comments; PTAL?  

With regard to the whole URLRequest notification issue: I don't want to deal in
this CL, partially because I get a design smell from solutions that hide the
URLRequest type, and I want to see if I can find a clean solution that doesn't
do that.  And partially because my need for that information is a ways in the
future :-}.

Bence: Part of the reason I went looking for another reviewer was to lighten the
load on Ryan, so while I'd welcome him reviewing my tests, AFAIC, he's also
welcome to skip it based on your review.  Just trying to keep Ryan from drowning
under reviews.

https://codereview.chromium.org/664263002/diff/100001/chrome/browser/net/chro...
File chrome/browser/net/chrome_sdch_policy.cc (right):

https://codereview.chromium.org/664263002/diff/100001/chrome/browser/net/chro...
chrome/browser/net/chrome_sdch_policy.cc:15: // of |fetcher_|.
On 2014/11/04 21:40:44, Ryan Sleevi wrote:
> 1) I'm not sure what this comment is communicating that isn't "obvious" from
the
> code

Ok, just being over-paranoid about documenting ownership lifetimes.  Deleted. 

> .. but also
> 
> 2) Is this true if fetcher_ were to PostTask something, and then this object
> were deleted? The posted task would still be on the queue, indicating that a
> WeakPtr should be used. Which then messes things up vis-a-vis WeakPtrFactory
> (since the factory must be last), meaning you'd probably need fetcher_ to be
> scoped_ptr rather than member.

All true, but not a problem, since fetcher doesn't PostTask anything, which
behavior is specified and guaranteed in the constructor interface comment.
> 
> Just the initial reaction from this code (not having dug into fetcher_ yet)

https://codereview.chromium.org/664263002/diff/100001/chrome/browser/net/sdch...
File chrome/browser/net/sdch_browsertest.cc (right):

https://codereview.chromium.org/664263002/diff/100001/chrome/browser/net/sdch...
chrome/browser/net/sdch_browsertest.cc:263: class SdchBrowserTest :
On 2014/11/04 21:40:44, Ryan Sleevi wrote:
> did git cl-format do this?
> 
> It seems that : public is the dominant form (see
>
https://code.google.com/p/chromium/codesearch#search/&q=%5Cs%5Cs:%5Cspublic%2...
>  compared with
>
https://code.google.com/p/chromium/codesearch#search/&q=%5E%5Cw*class%5Cs%5Cw...
> )

No, this was my braino, and I agree with your suggested change.  I ran git cl
format over the whole thing (PS7->8), and while there's stuff in the results I
dislike (~SdchObserver :-{) there's nothing I hate enough to want to fight with
the formatter over it.  Consistency uber alles! :-J

https://codereview.chromium.org/664263002/diff/100001/net/base/sdch_manager.h
File net/base/sdch_manager.h (right):

https://codereview.chromium.org/664263002/diff/100001/net/base/sdch_manager.h...
net/base/sdch_manager.h:39: class URLRequest;
On 2014/11/04 21:40:44, Ryan Sleevi wrote:
> Unneeded now, right?

Right; thanks.

https://codereview.chromium.org/664263002/diff/100001/net/base/sdch_manager.h...
net/base/sdch_manager.h:41:
//------------------------------------------------------------------------------
On 2014/11/04 21:40:44, Ryan Sleevi wrote:
> delete

Done.

https://codereview.chromium.org/664263002/diff/100001/net/base/sdch_manager.h...
net/base/sdch_manager.h:43: class NET_EXPORT SdchManager {
On 2014/11/04 21:40:44, Ryan Sleevi wrote:
> Documentation?

What's your mental model for the documentation to be provided immediately
pre-class and the documentation to be provided at the top of the file (in a .h
file WRT its primary class)?  

I've taken your comment on SdchObserver as a tentative answer to this question
and moved the comment at the beginning of the file down to above the class (with
a deletion).  But let me know if you'd like something else.

https://codereview.chromium.org/664263002/diff/100001/net/base/sdch_observer.h
File net/base/sdch_observer.h (right):

https://codereview.chromium.org/664263002/diff/100001/net/base/sdch_observer....
net/base/sdch_observer.h:18: class URLRequest;
On 2014/11/04 21:40:44, Ryan Sleevi wrote:
> Unnecessary, right?

Done.

https://codereview.chromium.org/664263002/diff/100001/net/base/sdch_observer....
net/base/sdch_observer.h:20: class NET_EXPORT SdchObserver {
On 2014/11/04 21:40:44, Ryan Sleevi wrote:
> Document?
> 
> Seems like lines 5-6 should be moved to 19/20

Done.

https://codereview.chromium.org/664263002/diff/100001/net/url_request/sdch_di...
File net/url_request/sdch_dictionary_fetcher.h (right):

https://codereview.chromium.org/664263002/diff/100001/net/url_request/sdch_di...
net/url_request/sdch_dictionary_fetcher.h:6: // to the net/ embedder for correct
implementaiton of authentication.
On 2014/11/04 21:40:44, Ryan Sleevi wrote:
> s/implementaiton/implementation/

Done.

https://codereview.chromium.org/664263002/diff/100001/net/url_request/sdch_di...
net/url_request/sdch_dictionary_fetcher.h:36: : public URLRequest::Delegate,
On 2014/11/04 21:40:45, Ryan Sleevi wrote:
> Compare with how you indented the other class. Pretty sure this is right.

Agreed; believed made consistent via the git cl format sledgehammer.

[Ryan Sleevi, 2014-11-05 22:21:39]

You're in the home stretch - LGTM, mod a few remarks on tests.

https://codereview.chromium.org/664263002/diff/100001/net/base/sdch_manager.h
File net/base/sdch_manager.h (right):

https://codereview.chromium.org/664263002/diff/100001/net/base/sdch_manager.h...
net/base/sdch_manager.h:43: class NET_EXPORT SdchManager {
On 2014/11/05 20:35:01, rdsmith wrote:
> On 2014/11/04 21:40:44, Ryan Sleevi wrote:
> > Documentation?
> 
> What's your mental model for the documentation to be provided immediately
> pre-class and the documentation to be provided at the top of the file (in a .h
> file WRT its primary class)?  
> 
> I've taken your comment on SdchObserver as a tentative answer to this question
> and moved the comment at the beginning of the file down to above the class
(with
> a deletion).  But let me know if you'd like something else.

My mental model is to try and keep class-specific documentation as close to the
class it documents as possible. I see file level comments as generally only
being applicable to a file that has a number of classes or functions grouped by
some logical group. Files with multiple classes are pretty highly discouraged,
so it's only the "namespace with a bunch of helper functions" that I think
should (generally) get file-level comments.

https://codereview.chromium.org/664263002/diff/160001/net/base/sdch_manager_u...
File net/base/sdch_manager_unittest.cc (right):

https://codereview.chromium.org/664263002/diff/160001/net/base/sdch_manager_u...
net/base/sdch_manager_unittest.cc:600: TEST_F(SdchManagerTest,
GetDictionaryNotification) {
Pedantry: You don't test that _removing_ the observer causes no notifications to
be sent after removal.

https://codereview.chromium.org/664263002/diff/160001/net/url_request/sdch_di...
File net/url_request/sdch_dictionary_fetcher_unittest.cc (right):

https://codereview.chromium.org/664263002/diff/160001/net/url_request/sdch_di...
net/url_request/sdch_dictionary_fetcher_unittest.cc:91: DCHECK(!fetcher_.get());
DCHECK in tests is BAD! :)

ASSERT_FALSE(fetcher_.get()) if you go the SetUp route.

https://codereview.chromium.org/664263002/diff/160001/net/url_request/sdch_di...
net/url_request/sdch_dictionary_fetcher_unittest.cc:98:
base::Unretained(this))));
Is there a reason you need to do this in SetUp/TearDown, rather than CTor/DTor?

You don't appear to be using any of the ASSERTs that would be used during this.

[Bence, 2014-11-05 22:47:41]

On 2014/11/05 20:35:02, rdsmith wrote:
> Bence: Part of the reason I went looking for another reviewer was to lighten
the
> load on Ryan, so while I'd welcome him reviewing my tests, AFAIC, he's also
> welcome to skip it based on your review.  Just trying to keep Ryan from
drowning
> under reviews.

Okay, got it.  I am just not quite sure that I have ramped up to speed yet on
reviews.  Sorry.

[Randy Smith (Not in Mondays), 2014-11-05 22:56:16]

On 2014/11/05 22:47:41, Bence wrote:
> On 2014/11/05 20:35:02, rdsmith wrote:
> > Bence: Part of the reason I went looking for another reviewer was to lighten
> the
> > load on Ryan, so while I'd welcome him reviewing my tests, AFAIC, he's also
> > welcome to skip it based on your review.  Just trying to keep Ryan from
> drowning
> > under reviews.
> 
> Okay, got it.  I am just not quite sure that I have ramped up to speed yet on
> reviews.  Sorry.

No worries at all; I just wanted to give you the context for why I wasn't going
to wait for Ryan's test review if he didn't do one.

[Randy Smith (Not in Mondays), 2014-11-05 23:49:58]

rdsmith@chromium.org changed reviewers:
+ mmenke@chromium.org

[Randy Smith (Not in Mondays), 2014-11-05 23:50:00]

Thanks to you both!

Matt: Willing to stamp the changes to chrome/browser/profiles based on Bence and
Ryan's reviews?

https://codereview.chromium.org/664263002/diff/160001/net/base/sdch_manager_u...
File net/base/sdch_manager_unittest.cc (right):

https://codereview.chromium.org/664263002/diff/160001/net/base/sdch_manager_u...
net/base/sdch_manager_unittest.cc:600: TEST_F(SdchManagerTest,
GetDictionaryNotification) {
On 2014/11/05 22:21:39, Ryan Sleevi wrote:
> Pedantry: You don't test that _removing_ the observer causes no notifications
to
> be sent after removal.

Good point.  Done.

https://codereview.chromium.org/664263002/diff/160001/net/url_request/sdch_di...
File net/url_request/sdch_dictionary_fetcher_unittest.cc (right):

https://codereview.chromium.org/664263002/diff/160001/net/url_request/sdch_di...
net/url_request/sdch_dictionary_fetcher_unittest.cc:91: DCHECK(!fetcher_.get());
On 2014/11/05 22:21:39, Ryan Sleevi wrote:
> DCHECK in tests is BAD! :)
> 
> ASSERT_FALSE(fetcher_.get()) if you go the SetUp route.

Switching to the ctor makes this moot.

https://codereview.chromium.org/664263002/diff/160001/net/url_request/sdch_di...
net/url_request/sdch_dictionary_fetcher_unittest.cc:98:
base::Unretained(this))));
On 2014/11/05 22:21:39, Ryan Sleevi wrote:
> Is there a reason you need to do this in SetUp/TearDown, rather than
CTor/DTor?
> 
> You don't appear to be using any of the ASSERTs that would be used during
this.

I had thought that a single instance of the test object was used for all tests,
so I wanted to reset between tests.  But it works in the ctor/dtor, so I'm good
with doing it that way.  Done.

[Ryan Sleevi, 2014-11-06 00:03:45]

https://codereview.chromium.org/664263002/diff/160001/net/url_request/sdch_di...
File net/url_request/sdch_dictionary_fetcher_unittest.cc (right):

https://codereview.chromium.org/664263002/diff/160001/net/url_request/sdch_di...
net/url_request/sdch_dictionary_fetcher_unittest.cc:98:
base::Unretained(this))));
On 2014/11/05 23:50:00, rdsmith wrote:
> On 2014/11/05 22:21:39, Ryan Sleevi wrote:
> > Is there a reason you need to do this in SetUp/TearDown, rather than
> CTor/DTor?
> > 
> > You don't appear to be using any of the ASSERTs that would be used during
> this.
> 
> I had thought that a single instance of the test object was used for all
tests,
> so I wanted to reset between tests.  But it works in the ctor/dtor, so I'm
good
> with doing it that way.  Done.

Common mistake See
https://code.google.com/p/googletest/wiki/FAQ#Should_I_use_the_constructor/de...
for details :)

[mmenke, 2014-11-06 15:30:12]

Seems reasonable.

https://codereview.chromium.org/664263002/diff/180001/chrome/browser/net/chro...
File chrome/browser/net/chrome_sdch_policy.cc (right):

https://codereview.chromium.org/664263002/diff/180001/chrome/browser/net/chro...
chrome/browser/net/chrome_sdch_policy.cc:10:
ChromeSdchPolicy::ChromeSdchPolicy(net::SdchManager* sdch_manager,
Why isn't this class in net/?

https://codereview.chromium.org/664263002/diff/180001/chrome/browser/net/chro...
File chrome/browser/net/chrome_sdch_policy.h (right):

https://codereview.chromium.org/664263002/diff/180001/chrome/browser/net/chro...
chrome/browser/net/chrome_sdch_policy.h:6: // TODO(rdsmith): Implement
dictionary prioritization.
nit:  Comments like this generally go above the class, not at the top of the
file.  I know that I, at least, always miss comments up here, because I've been
trained to ignore the license comments at the top of files.

https://codereview.chromium.org/664263002/diff/180001/chrome/browser/net/chro...
chrome/browser/net/chrome_sdch_policy.h:19: class ChromeSdchPolicy : public
net::SdchObserver {
I wonder about the name...  "Policy" indicates to me it does something, rather
than just forward everything along to another class.  I guess it's because of
your TODO about prioritization?

https://codereview.chromium.org/664263002/diff/180001/chrome/browser/net/chro...
chrome/browser/net/chrome_sdch_policy.h:25: virtual ~ChromeSdchPolicy();
nit:  -virtual +override

https://codereview.chromium.org/664263002/diff/180001/chrome/browser/net/chro...
chrome/browser/net/chrome_sdch_policy.h:27: void OnDictionaryFetched(const
std::string& dictionary_text,
#include  <string>

https://codereview.chromium.org/664263002/diff/180001/chrome/browser/net/chro...
chrome/browser/net/chrome_sdch_policy.h:32: const GURL& request_url,
Maybe forward declare GURL?  Know it's part of the interface, so it always has
to have been declared, but then, so is SdchManager.  Not something I have much
opinion one, as long as we're consistent.

https://codereview.chromium.org/664263002/diff/180001/chrome/browser/net/chro...
chrome/browser/net/chrome_sdch_policy.h:34: virtual void
OnClearDictionaries(net::SdchManager* manager) override;
nit:  -virtual +override (x2)

https://codereview.chromium.org/664263002/diff/180001/net/base/sdch_manager.h
File net/base/sdch_manager.h (right):

https://codereview.chromium.org/664263002/diff/180001/net/base/sdch_manager.h...
net/base/sdch_manager.h:5: 
nit:  Remove extra blank line.

https://codereview.chromium.org/664263002/diff/180001/net/base/sdch_manager.h...
net/base/sdch_manager.h:216: virtual ~SdchManager();
There aren't any subclasses of this, are there?

https://codereview.chromium.org/664263002/diff/180001/net/base/sdch_manager.h...
net/base/sdch_manager.h:268: void OnGetDictionary(const GURL& request_url, const
GURL& dictionary_url);
The reason for an observer-type rather than using an explicit delegate/fetcher
class is primarily for shutdown?  Or are their other reasons, too?

Not objecting, just trying to dig up context.

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

Incorporated comments; PTAL?

https://codereview.chromium.org/664263002/diff/180001/chrome/browser/net/chro...
File chrome/browser/net/chrome_sdch_policy.cc (right):

https://codereview.chromium.org/664263002/diff/180001/chrome/browser/net/chro...
chrome/browser/net/chrome_sdch_policy.cc:10:
ChromeSdchPolicy::ChromeSdchPolicy(net::SdchManager* sdch_manager,
On 2014/11/06 15:30:11, mmenke wrote:
> Why isn't this class in net/?

This CL is restructuring things so that I have a place to put policy
implementations, which is this class.  Policy decisions are appropriately made
in the embedder, not in net/.  I haven't put any meat in this class because this
CL was intended as pure refactoring.  Let me know if that doesn't make sense or
pass muster.

https://codereview.chromium.org/664263002/diff/180001/chrome/browser/net/chro...
File chrome/browser/net/chrome_sdch_policy.h (right):

https://codereview.chromium.org/664263002/diff/180001/chrome/browser/net/chro...
chrome/browser/net/chrome_sdch_policy.h:6: // TODO(rdsmith): Implement
dictionary prioritization.
On 2014/11/06 15:30:12, mmenke wrote:
> nit:  Comments like this generally go above the class, not at the top of the
> file.  I know that I, at least, always miss comments up here, because I've
been
> trained to ignore the license comments at the top of files.

Done.

https://codereview.chromium.org/664263002/diff/180001/chrome/browser/net/chro...
chrome/browser/net/chrome_sdch_policy.h:19: class ChromeSdchPolicy : public
net::SdchObserver {
On 2014/11/06 15:30:12, mmenke wrote:
> I wonder about the name...  "Policy" indicates to me it does something, rather
> than just forward everything along to another class.  I guess it's because of
> your TODO about prioritization?

Yeah.  The context here is that we want an embedder class that determines
policy, which this one will.  More pedantically, "blindly accept all dictionary
suggestions until full up" counts as a policy.  I hope to get a bit more nuanced
in future CLs :-}.

https://codereview.chromium.org/664263002/diff/180001/chrome/browser/net/chro...
chrome/browser/net/chrome_sdch_policy.h:25: virtual ~ChromeSdchPolicy();
On 2014/11/06 15:30:12, mmenke wrote:
> nit:  -virtual +override

Done.

https://codereview.chromium.org/664263002/diff/180001/chrome/browser/net/chro...
chrome/browser/net/chrome_sdch_policy.h:27: void OnDictionaryFetched(const
std::string& dictionary_text,
On 2014/11/06 15:30:12, mmenke wrote:
> #include  <string>

Done.

https://codereview.chromium.org/664263002/diff/180001/chrome/browser/net/chro...
chrome/browser/net/chrome_sdch_policy.h:32: const GURL& request_url,
On 2014/11/06 15:30:12, mmenke wrote:
> Maybe forward declare GURL?  Know it's part of the interface, so it always has
> to have been declared, but then, so is SdchManager.  Not something I have much
> opinion one, as long as we're consistent.

I think if we really want consistency, we'd need to get something into the style
guide, but what makes sense to me to aim for: If something's in the .h file, no
need to put it in the .cc file.  But the .h file should forward declare or
include everything that it needs.  I've tried to do that here; let me know if
you see any gaps (or object to the goal).

https://codereview.chromium.org/664263002/diff/180001/chrome/browser/net/chro...
chrome/browser/net/chrome_sdch_policy.h:34: virtual void
OnClearDictionaries(net::SdchManager* manager) override;
On 2014/11/06 15:30:11, mmenke wrote:
> nit:  -virtual +override (x2)

Done.

https://codereview.chromium.org/664263002/diff/180001/net/base/sdch_manager.h
File net/base/sdch_manager.h (right):

https://codereview.chromium.org/664263002/diff/180001/net/base/sdch_manager.h...
net/base/sdch_manager.h:5: 
On 2014/11/06 15:30:12, mmenke wrote:
> nit:  Remove extra blank line.

Done.

https://codereview.chromium.org/664263002/diff/180001/net/base/sdch_manager.h...
net/base/sdch_manager.h:216: virtual ~SdchManager();
On 2014/11/06 15:30:12, mmenke wrote:
> There aren't any subclasses of this, are there?

Not any more.  Done.

https://codereview.chromium.org/664263002/diff/180001/net/base/sdch_manager.h...
net/base/sdch_manager.h:268: void OnGetDictionary(const GURL& request_url, const
GURL& dictionary_url);
On 2014/11/06 15:30:12, mmenke wrote:
> The reason for an observer-type rather than using an explicit delegate/fetcher
> class is primarily for shutdown?  Or are their other reasons, too?
> 
> Not objecting, just trying to dig up context.

It's breaking dependencies.  That's primarily for shutdown, but helps a bit on
initialization too.  But it's mostly to make reasoning about dependencies
simpler. 

We had a conversation about this in an email thread entitled "SDCH dictionary
fetching, expiration, and layering", if you *really* want context :-}.  I think
the delegate/interface issue was discussed in the first 2-3 messages on that
thread.

[mmenke, 2014-11-06 16:44:59]

LGTM (Didn't delve into things, deferring to rsleevi and Bence for that, just
signing off on the the class divisions and how they're hooked up).

https://codereview.chromium.org/664263002/diff/180001/chrome/browser/net/chro...
File chrome/browser/net/chrome_sdch_policy.cc (right):

https://codereview.chromium.org/664263002/diff/180001/chrome/browser/net/chro...
chrome/browser/net/chrome_sdch_policy.cc:10:
ChromeSdchPolicy::ChromeSdchPolicy(net::SdchManager* sdch_manager,
On 2014/11/06 16:39:03, rdsmith wrote:
> On 2014/11/06 15:30:11, mmenke wrote:
> > Why isn't this class in net/?
> 
> This CL is restructuring things so that I have a place to put policy
> implementations, which is this class.  Policy decisions are appropriately made
> in the embedder, not in net/.  I haven't put any meat in this class because
this
> CL was intended as pure refactoring.  Let me know if that doesn't make sense
or
> pass muster.

My main concern is just whether we really want to make another implementation
just for CroNet.

[Randy Smith (Not in Mondays), 2014-11-06 18:13:30]

The CQ bit was checked by rdsmith@chromium.org

[commit-bot: I haz the power, 2014-11-06 18:15:54]

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

[commit-bot: I haz the power, 2014-11-06 19:02:39]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2014-11-06 19:03:22]

Patchset 12 (id:??) landed as
https://crrev.com/47c133b16551d5b2b24bb9536b696f2f283403e6
Cr-Commit-Position: refs/heads/master@{#303058}