Pin dictionaries from being deleted while request is outstanding.

Pin dictionaries from being deleted while request is outstanding.

This CL also removed scoped_refptr<> from the SdchManager interface.

BUG=383407
R=bnc@chromium.org

Committed: https://crrev.com/81f6075625a14b2574974a9836f696d397dfdfe7
Cr-Commit-Position: refs/heads/master@{#305247}

[Randy Smith (Not in Mondays), 2014-11-12 20:21:31]

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

[Randy Smith (Not in Mondays), 2014-11-12 20:21:31]

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

[Randy Smith (Not in Mondays), 2014-11-12 20:21:32]

Ryan, Bence: PTAL?  

Bence, you're primary reviewer.

Ryan: All I'm looking for from you is a glance at the structure I used to remove
scoped_refptr<> from the SdchManager interface and to pin down dictionaries over
the lifetime of a request.  This is basically all in sdch_manager.h.  The thing
I don't like is having to declare DictionaryWrapper in the header file; it's a
pure implementation detail.  However, it's a pure implementation detail used in
the private data members of SdchManager and DictionarySet, so it's in that
header.  I could get around that problem if I declared SdchManager and
DictionarySet as ABCs and had separate classes for the implementation, but I
decided that that was enough boilerplate to not be worth it to save 20 lines in
a header file.

While I'd like to have your eyes go over that change, I'm comfortable enough
with the architecture that I'm ok doing that with a TBR if you're busy this
week.  If you should happen to object to this before Bence and I finish the
primary review, just give me a heads up and I'll wait.

[Randy Smith (Not in Mondays), 2014-11-12 20:23:33]

Whoops, so busy with the note to Ryan, I forgot the note I had queued in my mind
to Bence :-}.

Bence: There's one portion of this CL I don't plan to land as is, which is the
conversion of CanUse over to return a ProblemCodes.  There's a CL in process
(landed and reverted) which did a much more comprehensive job on the return
codes (http://codereview.chromium.org/423813002; landed and reverted, author is
working on a relanding) and I'll either wait for that CL to land, or if you and
I finish up earlier, I'll include the return code refactor from it in this CL.

[Bence, 2014-11-12 21:46:26]

So far, so good, modulo nits.  I would like to see the version that you plan to
land with the return code refactor (or one rebased on that other CL).

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

https://codereview.chromium.org/711753003/diff/100001/net/base/sdch_manager.h...
net/base/sdch_manager.h:101: UNADVERTISED_SDCH_DICTIONARY_USED_CACHED = 52,
Last value should be 53 instead of 52.

https://codereview.chromium.org/711753003/diff/100001/net/base/sdch_manager.h...
net/base/sdch_manager.h:216: // DictionarySet and SdchManager.   This class
should not be used
Two spaces should be enough between two sentences.

https://codereview.chromium.org/711753003/diff/100001/net/filter/filter.h
File net/filter/filter.h (right):

https://codereview.chromium.org/711753003/diff/100001/net/filter/filter.h#new...
net/filter/filter.h:119: SdchDictionariesAdvertised() const = 0;
From the Google C++ style guide:  "If you break after the return type of a
function declaration or definition, do not indent."

[Randy Smith (Not in Mondays), 2014-11-13 22:11:03]

Completely fair with regard to the rebase.  I expect the other CL to land in a
day or so, so I'm going to hold off on the rebase until after it does.  This PS
has your comments incorporated, but feel free to wait for the rebase PS to
re-review.

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

https://codereview.chromium.org/711753003/diff/100001/net/base/sdch_manager.h...
net/base/sdch_manager.h:101: UNADVERTISED_SDCH_DICTIONARY_USED_CACHED = 52,
On 2014/11/12 21:46:26, Bence wrote:
> Last value should be 53 instead of 52.

Whoops; thanks for the catch.

https://codereview.chromium.org/711753003/diff/100001/net/base/sdch_manager.h...
net/base/sdch_manager.h:216: // DictionarySet and SdchManager.   This class
should not be used
On 2014/11/12 21:46:26, Bence wrote:
> Two spaces should be enough between two sentences.

Actually, Ryan's badgered me down to 1 :-}.  Not sure how that happened.  Done.

https://codereview.chromium.org/711753003/diff/100001/net/filter/filter.h
File net/filter/filter.h (right):

https://codereview.chromium.org/711753003/diff/100001/net/filter/filter.h#new...
net/filter/filter.h:119: SdchDictionariesAdvertised() const = 0;
On 2014/11/12 21:46:26, Bence wrote:
> From the Google C++ style guide:  "If you break after the return type of a
> function declaration or definition, do not indent."

Moot; fits on one line anyway :-}.  Fixed.

[Ryan Sleevi, 2014-11-14 03:21:26]

Sorry for delays. A lot of design-level comments on the
DictionarySet/DictionaryMap, as you probably anticipated :)

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

https://codereview.chromium.org/711753003/diff/120001/net/base/sdch_manager.c...
net/base/sdch_manager.cc:200: return base::Time::Now() > expiration_;
Whenever people do this, they tend to then need to screw with the clock in order
to cover the whole "expired" test case.

Would a
https://code.google.com/p/chromium/codesearch#chromium/src/base/time/clock.h&...
make more sense as an input here?

https://codereview.chromium.org/711753003/diff/120001/net/base/sdch_manager.c...
net/base/sdch_manager.cc:203:
//------------------------------------------------------------------------------
don't do this

https://codereview.chromium.org/711753003/diff/120001/net/base/sdch_manager.c...
net/base/sdch_manager.cc:210:
//------------------------------------------------------------------------------
don't do this

https://codereview.chromium.org/711753003/diff/120001/net/base/sdch_manager.c...
net/base/sdch_manager.cc:221:
client_hashes->append(it->second->dictionary()->client_hash());
Is it expected there will be a lot of client hashes? Is the client hash a fixed
size? If so, does it make sense to .reserve the (size + (fixed size * the
dictionaries_.size())) ?

(with a note indicating "Yes, I know this multiplication may overflow, but it's
just a reservation, and it'll blow up when it tries to append that much anyways)

https://codereview.chromium.org/711753003/diff/120001/net/base/sdch_manager.c...
net/base/sdch_manager.cc:241: DCHECK(dictionaries_.end() ==
dictionaries_.find(server_hash));
Is this really a DCHECK thing? Which is to say, is it programmer mistake, or can
the protocol itself cause this to happen?

https://codereview.chromium.org/711753003/diff/120001/net/base/sdch_manager.c...
net/base/sdch_manager.cc:246:
//------------------------------------------------------------------------------
:'(

https://codereview.chromium.org/711753003/diff/120001/net/base/sdch_manager.c...
net/base/sdch_manager.cc:429: it != dictionaries_.end(); ++it) {
Range-based for loops are allowed - https://chromium-cpp.appspot.com/

for (const auto& dictionary : dictionaries_) {
  if (dictionary->second->dictionary() ...)

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

https://codereview.chromium.org/711753003/diff/120001/net/base/sdch_manager.h...
net/base/sdch_manager.h:44: OK,
NO_PROBLEM? (to avoid confusion with net::OK)

https://codereview.chromium.org/711753003/diff/120001/net/base/sdch_manager.h...
net/base/sdch_manager.h:150: // dictionary.
Is this still true? By making this public, you create the possibility that
multiple Dictionary objects exist for a given dictionary text.

https://codereview.chromium.org/711753003/diff/120001/net/base/sdch_manager.h...
net/base/sdch_manager.h:178: // with the given URL.  Does not check expiration
(it's appropriate to
single space

https://codereview.chromium.org/711753003/diff/120001/net/base/sdch_manager.h...
net/base/sdch_manager.h:218: class DictionaryWrapper : public
base::RefCounted<DictionaryWrapper> {
Why not just RefCountedData?
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/ref_co...

Also, with your move from having the dictionary be a (mostly private) structure
to a (mostly public) structure, does it make sense to make the data members
copyable?

https://codereview.chromium.org/711753003/diff/120001/net/base/sdch_manager.h...
net/base/sdch_manager.h:221: DictionaryMap;
Why is this a typedef on the Wrapper? Seems to be an abstraction violation - it
belongs to the thing that maps, not to the wrapper itself

https://codereview.chromium.org/711753003/diff/120001/net/base/sdch_manager.h...
net/base/sdch_manager.h:243: void GetDictionaryClientHashList(std::string*
client_hashes) const;
Why do you append, rather than just returning the comma-separated list?
Premature optimization? It just strikes me as odd that this isn't just
std::string GetDictionaryClientHashList() const ...

https://codereview.chromium.org/711753003/diff/120001/net/base/sdch_manager.h...
net/base/sdch_manager.h:249: const SdchManager::Dictionary* Dictionary(const
std::string& hash) const;
Should you return a const-ref then? That usually helps programatically clarify
the lifetime is tied to the parent object (c.f iterators)

https://codereview.chromium.org/711753003/diff/120001/net/base/sdch_manager.h...
net/base/sdch_manager.h:257: static const char* kSdchDictionariesHandleDataKey;
private static? seems unnecessary?

https://codereview.chromium.org/711753003/diff/120001/net/base/sdch_manager.h...
net/base/sdch_manager.h:362: static scoped_ptr<DictionarySet>
CreateNullDictionarySetForTesting();
s/Null/Empty/ (as otherwise you'd just return a nullptr in the scoped_ptr)

https://codereview.chromium.org/711753003/diff/120001/net/base/sdch_manager.h...
net/base/sdch_manager.h:397: DictionaryWrapper::DictionaryMap dictionaries_;
see comment re: DictionaryMap typedef

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

https://codereview.chromium.org/711753003/diff/120001/net/base/sdch_manager_u...
net/base/sdch_manager_unittest.cc:614: "Domain: %s\nMax-age: 0\n\n",
dictionary_domain.c_str()));
why is a Max-age: 0 in the past?

Seems like it'd be == Now(), in which case, if you hit lines 625/626 with T=0,
you'd fail the test (aka flaky test)

https://codereview.chromium.org/711753003/diff/120001/net/filter/filter.h
File net/filter/filter.h (right):

https://codereview.chromium.org/711753003/diff/120001/net/filter/filter.h#new...
net/filter/filter.h:77: // time.  In addition, it only supports two filters,
sdch and gzip.
s/sdch/SDCH/

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

https://codereview.chromium.org/711753003/diff/120001/net/filter/sdch_filter....
net/filter/sdch_filter.cc:415: // involve evicting encoded resources on
dictionary removal.
Whenever someone adds a TODO like this, I like to ask - can you file a crbug for
this to explain more of the motivations?

[Randy Smith (Not in Mondays), 2014-11-17 17:04:20]

rdsmith@chromium.org changed required reviewers:
+ rsleevi@chromium.org

[Randy Smith (Not in Mondays), 2014-11-17 17:04:22]

Ok, ready for new review.

Bence: I've sync'd past the net-internals CL, so this is what I intend to land. 
PTAL?  Areas that have changed a bit since your first round: stuff touched by
Ryan's comments, the signature of GetDictionarySetByHash (a bit uncomfortable
here, as it's parallel to GetDictionarySet but doesn't match the other
SdchManager routines that return an SdchProblemCode), and
SdchFilter::InitializeDictionary() (modified control flow for error logging). 
PTAL?

Ryan: I definitely anticipated comments, but if I had expected quite this many I
wouldn't have threatened a TBR :-} (threat now removed; you're officially a
required reviewer now).  Thanks for the comments; enlightening and useful as
always.  Let me know what you think of my changes.

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

https://codereview.chromium.org/711753003/diff/120001/net/base/sdch_manager.c...
net/base/sdch_manager.cc:200: return base::Time::Now() > expiration_;
On 2014/11/14 03:21:25, Ryan Sleevi (OOO until 11-18) wrote:
> Whenever people do this, they tend to then need to screw with the clock in
order
> to cover the whole "expired" test case.
> 
> Would a
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/time/clock.h&...
> make more sense as an input here?

Nice; hadn't known about that class.  From grepping, I take the standard idiom
here as putting in a SetClockForTesting() method, and I've followed that.  
Done.

https://codereview.chromium.org/711753003/diff/120001/net/base/sdch_manager.c...
net/base/sdch_manager.cc:203:
//------------------------------------------------------------------------------
On 2014/11/14 03:21:24, Ryan Sleevi (OOO until 11-18) wrote:
> don't do this

Done.

https://codereview.chromium.org/711753003/diff/120001/net/base/sdch_manager.c...
net/base/sdch_manager.cc:210:
//------------------------------------------------------------------------------
On 2014/11/14 03:21:25, Ryan Sleevi (OOO until 11-18) wrote:
> don't do this

Done.

https://codereview.chromium.org/711753003/diff/120001/net/base/sdch_manager.c...
net/base/sdch_manager.cc:221:
client_hashes->append(it->second->dictionary()->client_hash());
On 2014/11/14 03:21:24, Ryan Sleevi (OOO until 11-18) wrote:
> Is it expected there will be a lot of client hashes? Is the client hash a
fixed
> size? If so, does it make sense to .reserve the (size + (fixed size * the
> dictionaries_.size())) ?
> 
> (with a note indicating "Yes, I know this multiplication may overflow, but
it's
> just a reservation, and it'll blow up when it tries to append that much
anyways)

The common case is just one hash, and if we ever have enough hashes to make
something like that make sense, we're in a world of hurt because that number of
dictionaries is going to start putting us under some serious memory pressure, at
least on mobile.  So my inclination is to leave it like it is, but I don't feel
strongly and am willing to add the reserve + comment if you'd prefer.

https://codereview.chromium.org/711753003/diff/120001/net/base/sdch_manager.c...
net/base/sdch_manager.cc:241: DCHECK(dictionaries_.end() ==
dictionaries_.find(server_hash));
On 2014/11/14 03:21:24, Ryan Sleevi (OOO until 11-18) wrote:
> Is this really a DCHECK thing? Which is to say, is it programmer mistake, or
can
> the protocol itself cause this to happen?

Yeah, it's a programmer mistake.  The only entity that calls
DictionarySet::AddDictionary() is the SdchManager in putting together the
DictionarySet, and it can't have more than one dictionary with a given server
hash.  That's not true at the SdchManager level (the server may have
dictionaries at two different URLs that have the same server hash) but the
SdchManager handles it.  

DictionarySet::AddDictionary() is a private method on DictionarySet only used by
the SdchManager, so I'm not inclined to add a comment to the header about this
interface requirement, but let me know if you disagree.

https://codereview.chromium.org/711753003/diff/120001/net/base/sdch_manager.c...
net/base/sdch_manager.cc:246:
//------------------------------------------------------------------------------
On 2014/11/14 03:21:24, Ryan Sleevi (OOO until 11-18) wrote:
> :'(

I'm tempted to make a comment about consistency within a file, but that'll
probably just produce more crystalline tears :-}.  I've removed all of these
lines in the file.

https://codereview.chromium.org/711753003/diff/120001/net/base/sdch_manager.c...
net/base/sdch_manager.cc:429: it != dictionaries_.end(); ++it) {
On 2014/11/14 03:21:24, Ryan Sleevi (OOO until 11-18) wrote:
> Range-based for loops are allowed - https://chromium-cpp.appspot.com/
> 
> for (const auto& dictionary : dictionaries_) {
>   if (dictionary->second->dictionary() ...)

Cool!  I like the syntax, so I went wild :-}.

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

https://codereview.chromium.org/711753003/diff/120001/net/base/sdch_manager.h...
net/base/sdch_manager.h:44: OK,
On 2014/11/14 03:21:25, Ryan Sleevi (OOO until 11-18) wrote:
> NO_PROBLEM? (to avoid confusion with net::OK)

I'm going to say that this issue will be moot by the next review you do, because
I'm going to be syncing with http://codereview.chromium.org/423813002 before
then and that pulls ProblemCodes out into its own header file and gives OK a
different name.  So if you look at the PS in which I respond to your comments,
this won't be fixed, but if you check the final PS of the CL, it should be.

https://codereview.chromium.org/711753003/diff/120001/net/base/sdch_manager.h...
net/base/sdch_manager.h:150: // dictionary.
On 2014/11/14 03:21:25, Ryan Sleevi (OOO until 11-18) wrote:
> Is this still true? By making this public, you create the possibility that
> multiple Dictionary objects exist for a given dictionary text.

Completely fair; now consumers can create their own Dictionary objects.  What is
true is that if consumers get a Dictionary through SdchManager, they'll always
get the same instance of the object for a given hash.  But thinking about where
to put that comment, I'm not sure it matters much for consumers--it's an
implementation detail.  Even if SdchManager returned each consumer their own
copy of the Dictionary, the consumers could use them fine.  So I just deleted
the comment.

https://codereview.chromium.org/711753003/diff/120001/net/base/sdch_manager.h...
net/base/sdch_manager.h:178: // with the given URL.  Does not check expiration
(it's appropriate to
On 2014/11/14 03:21:25, Ryan Sleevi (OOO until 11-18) wrote:
> single space

:-}.  Done (through file).

https://codereview.chromium.org/711753003/diff/120001/net/base/sdch_manager.h...
net/base/sdch_manager.h:218: class DictionaryWrapper : public
base::RefCounted<DictionaryWrapper> {
On 2014/11/14 03:21:25, Ryan Sleevi (OOO until 11-18) wrote:
> Why not just RefCountedData?
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/ref_co...
> 
> Also, with your move from having the dictionary be a (mostly private)
structure
> to a (mostly public) structure, does it make sense to make the data members
> copyable? 

Cool; thanks for pointing that type out to me.  That's quite close to what I
want.

Having said that, this comment caused me a bit of flailing.  The problem is that
I actually would prefer not to have a public copy constructor on the Dictionary
object, as I want to discourage users of the API from creating their own
instances of dictionaries stored in the manager; the dictionary sizes are large
enough that I don't want to encourage taking up the memory footprint.  But
RefCountedData<> requires a copy constructor or a default constructor + setters.
 I eventually settled on using RefCountedData<> with a private copy constructor,
but I'm not thrilled about that option either, since it means we're copying the
dictionary on creation.  But that's not that big a hit, and I decided it was
worth it for not having a custom class to do the work of RefCountedData.  

I'd be very surprised if you don't have any comments on the result; I look
forward to finding out what they are :-}.

https://codereview.chromium.org/711753003/diff/120001/net/base/sdch_manager.h...
net/base/sdch_manager.h:221: DictionaryMap;
On 2014/11/14 03:21:25, Ryan Sleevi (OOO until 11-18) wrote:
> Why is this a typedef on the Wrapper? Seems to be an abstraction violation -
it
> belongs to the thing that maps, not to the wrapper itself

Yeah, that's fair.  I was trying to avoid having the typedef in the public
section of SdchManager, as it's not part of the interface and I don't want the
consumers to have visibility into it, and I felt like putting it into
DictionaryWrapper hid it a bit.  But on reflection, that's silly, because it has
to be public in DictionaryWrapper so the other classes can see it.  In any case,
with the switch to RefCountedData<> it's back to being a public typedef.

https://codereview.chromium.org/711753003/diff/120001/net/base/sdch_manager.h...
net/base/sdch_manager.h:243: void GetDictionaryClientHashList(std::string*
client_hashes) const;
On 2014/11/14 03:21:25, Ryan Sleevi (OOO until 11-18) wrote:
> Why do you append, rather than just returning the comma-separated list?
> Premature optimization? It just strikes me as odd that this isn't just
> std::string GetDictionaryClientHashList() const ...

I think I was instinctively trying to avoid the performance cost of the copy,
but yeah, you're right, premature optimization.  Done.

https://codereview.chromium.org/711753003/diff/120001/net/base/sdch_manager.h...
net/base/sdch_manager.h:249: const SdchManager::Dictionary* Dictionary(const
std::string& hash) const;
On 2014/11/14 03:21:25, Ryan Sleevi (OOO until 11-18) wrote:
> Should you return a const-ref then? That usually helps programatically clarify
> the lifetime is tied to the parent object (c.f iterators)

I like that idea, and I agree that interface would communicate the lifetime in
an intuitive way.  But I tried to make this work in my mind, and I keep tripping
over "What do we return if a dictionary with that hash isn't present?"  We can
make it part of the interface contract not to do that (and add something into
the interface to probe for presence of hash), probably with DCHECKs, but I think
it's a likely enough misuse or the interface that we have to have some plan for
what it'd do in production (i.e. no DCHECKs).  And I don't think a CHECK is
right, and I can't think of anything else to do.  So after chewing on it a bit,
I decided to leave the interface the way it was, with comment.  Let me know what
you think.

https://codereview.chromium.org/711753003/diff/120001/net/base/sdch_manager.h...
net/base/sdch_manager.h:257: static const char* kSdchDictionariesHandleDataKey;
On 2014/11/14 03:21:25, Ryan Sleevi (OOO until 11-18) wrote:
> private static? seems unnecessary?

Whoops; serious leftover.  Gone.

https://codereview.chromium.org/711753003/diff/120001/net/base/sdch_manager.h...
net/base/sdch_manager.h:362: static scoped_ptr<DictionarySet>
CreateNullDictionarySetForTesting();
On 2014/11/14 03:21:25, Ryan Sleevi (OOO until 11-18) wrote:
> s/Null/Empty/ (as otherwise you'd just return a nullptr in the scoped_ptr)

Done.

https://codereview.chromium.org/711753003/diff/120001/net/base/sdch_manager.h...
net/base/sdch_manager.h:397: DictionaryWrapper::DictionaryMap dictionaries_;
On 2014/11/14 03:21:25, Ryan Sleevi (OOO until 11-18) wrote:
> see comment re: DictionaryMap typedef

Moot.

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

https://codereview.chromium.org/711753003/diff/120001/net/base/sdch_manager_u...
net/base/sdch_manager_unittest.cc:614: "Domain: %s\nMax-age: 0\n\n",
dictionary_domain.c_str()));
On 2014/11/14 03:21:25, Ryan Sleevi (OOO until 11-18) wrote:
> why is a Max-age: 0 in the past?
> 
> Seems like it'd be == Now(), in which case, if you hit lines 625/626 with T=0,
> you'd fail the test (aka flaky test)

You and the Windows try bots are telling me the same thing :-}.  Used the clock
suggestion to fix this.

https://codereview.chromium.org/711753003/diff/120001/net/filter/filter.h
File net/filter/filter.h (right):

https://codereview.chromium.org/711753003/diff/120001/net/filter/filter.h#new...
net/filter/filter.h:77: // time.  In addition, it only supports two filters,
sdch and gzip.
On 2014/11/14 03:21:25, Ryan Sleevi (OOO until 11-18) wrote:
> s/sdch/SDCH/

Done.

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

https://codereview.chromium.org/711753003/diff/120001/net/filter/sdch_filter....
net/filter/sdch_filter.cc:415: // involve evicting encoded resources on
dictionary removal.
On 2014/11/14 03:21:25, Ryan Sleevi (OOO until 11-18) wrote:
> Whenever someone adds a TODO like this, I like to ask - can you file a crbug
for
> this to explain more of the motivations?

I've added a pointer to the relevant bug in the TODO, but I'm not sure how much
more background is there (the bug consists of a pointer to a net-dev discussion,
and a follow-on note from me about the evicting encoded resources suggestion
that Matt made in recent email tome).  Let me know if you want more.

[Bence, 2014-11-17 20:38:04]

LGTM % nits.  Sorry for late response.

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

https://codereview.chromium.org/711753003/diff/180001/net/base/sdch_manager.h...
net/base/sdch_manager.h:126: // DISALLOW_COPY_AND_ASSIGN(Dictionary) by hand
since the copy constructor
For clarity, consider writing
Dictionary(const Dictionary&) = default;
explicitly, now that it's allowed.

https://codereview.chromium.org/711753003/diff/180001/net/base/sdch_manager.h...
net/base/sdch_manager.h:126: // DISALLOW_COPY_AND_ASSIGN(Dictionary) by hand
since the copy constructor
Well, then you don't disallow COPY after all, only ASSIGN, right?  For clarity,
consider just writing
// The copy constructor is needed by RefCountedData<>.

https://codereview.chromium.org/711753003/diff/180001/net/base/sdch_manager.h...
net/base/sdch_manager.h:128: void operator=(const Dictionary&);
Consider writing
void operator=(const Dictionary&) = delete;
to state that you do not intend to define it at all.  I think this might also
give you a compiler error instead of a linker error should you mistakenly use
it, though I might be mistaken on that.

[Randy Smith (Not in Mondays), 2014-11-18 22:22:42]

Bence: I don't consider your comments delayed; by my count they were 3h 34m
after I posted, which is fine response time :-}.  Let me know if you have any
issues with my response to your comments.

Ryan: Ping?

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

https://codereview.chromium.org/711753003/diff/180001/net/base/sdch_manager.h...
net/base/sdch_manager.h:126: // DISALLOW_COPY_AND_ASSIGN(Dictionary) by hand
since the copy constructor
On 2014/11/17 20:38:04, Bence wrote:
> Well, then you don't disallow COPY after all, only ASSIGN, right?  For
clarity,
> consider just writing
> // The copy constructor is needed by RefCountedData<>.
> 

So as far as any public user of this class is concerned, I am disallowing copy
and assign; an attempt to do either of those things from outside of this class
or a class that's a friend to this class will fail.  I'll admit this may be
confusing shorthand for that; I'll try to write it out a bit more clearly.

https://codereview.chromium.org/711753003/diff/180001/net/base/sdch_manager.h...
net/base/sdch_manager.h:126: // DISALLOW_COPY_AND_ASSIGN(Dictionary) by hand
since the copy constructor
On 2014/11/17 20:38:04, Bence wrote:
> For clarity, consider writing
> Dictionary(const Dictionary&) = default;
> explicitly, now that it's allowed.

I don't think that that will work with the scoped_ptr<base::Clock> clock_;
element I added at Ryan's suggestion.

https://codereview.chromium.org/711753003/diff/180001/net/base/sdch_manager.h...
net/base/sdch_manager.h:128: void operator=(const Dictionary&);
On 2014/11/17 20:38:04, Bence wrote:
> Consider writing
> void operator=(const Dictionary&) = delete;
> to state that you do not intend to define it at all.  I think this might also
> give you a compiler error instead of a linker error should you mistakenly use
> it, though I might be mistaken on that.

Nice.  Thank you.  Done.

[Bence, 2014-11-19 14:26:50]

Thanks.

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

https://codereview.chromium.org/711753003/diff/180001/net/base/sdch_manager.h...
net/base/sdch_manager.h:126: // DISALLOW_COPY_AND_ASSIGN(Dictionary) by hand
since the copy constructor
On 2014/11/18 22:22:42, rdsmith wrote:
> On 2014/11/17 20:38:04, Bence wrote:
> > Well, then you don't disallow COPY after all, only ASSIGN, right?  For
> clarity,
> > consider just writing
> > // The copy constructor is needed by RefCountedData<>.
> > 
> 
> So as far as any public user of this class is concerned, I am disallowing copy
> and assign; an attempt to do either of those things from outside of this class
> or a class that's a friend to this class will fail.  I'll admit this may be
> confusing shorthand for that; I'll try to write it out a bit more clearly.  

Of course, these are private members, you're right.

https://codereview.chromium.org/711753003/diff/180001/net/base/sdch_manager.h...
net/base/sdch_manager.h:126: // DISALLOW_COPY_AND_ASSIGN(Dictionary) by hand
since the copy constructor
On 2014/11/18 22:22:42, rdsmith wrote:
> On 2014/11/17 20:38:04, Bence wrote:
> > For clarity, consider writing
> > Dictionary(const Dictionary&) = default;
> > explicitly, now that it's allowed.
> 
> I don't think that that will work with the scoped_ptr<base::Clock> clock_;
> element I added at Ryan's suggestion.

Hm okay.  I wouldn't have guessed that it would not work, but if so, don't do
it.

[Randy Smith (Not in Mondays), 2014-11-19 14:45:05]

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

https://codereview.chromium.org/711753003/diff/180001/net/base/sdch_manager.h...
net/base/sdch_manager.h:126: // DISALLOW_COPY_AND_ASSIGN(Dictionary) by hand
since the copy constructor
On 2014/11/19 14:26:50, Bence wrote:
> On 2014/11/18 22:22:42, rdsmith wrote:
> > On 2014/11/17 20:38:04, Bence wrote:
> > > For clarity, consider writing
> > > Dictionary(const Dictionary&) = default;
> > > explicitly, now that it's allowed.
> > 
> > I don't think that that will work with the scoped_ptr<base::Clock> clock_;
> > element I added at Ryan's suggestion.
> 
> Hm okay.  I wouldn't have guessed that it would not work, but if so, don't do
> it.

The issue is the scoped_ptr, which, when you assign from it, zeroes the RHS. 
This shows up in compilation (I tried; didn't just rely on my gut) as the
default copy constructor taking a const argument, but there not being a const
version of the copy constructor for scoped_ptr<>.

[Bence, 2014-11-19 15:40:32]

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

https://codereview.chromium.org/711753003/diff/180001/net/base/sdch_manager.h...
net/base/sdch_manager.h:126: // DISALLOW_COPY_AND_ASSIGN(Dictionary) by hand
since the copy constructor
Sorry, I didn't realize that you are actually defining a non-default copy
constructor.

[Ryan Sleevi, 2014-11-20 22:50:30]

LGTM % mostly documentation & style nits.

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

https://codereview.chromium.org/711753003/diff/200001/net/base/sdch_manager.c...
net/base/sdch_manager.cc:464: auto it = dictionaries_.find(server_hash);
Should this be a const auto&? Doesn't seem like you mutate |it| at all, and it's
methods can/should be const.

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

https://codereview.chromium.org/711753003/diff/200001/net/base/sdch_manager.h...
net/base/sdch_manager.h:4: 
So this is probably where a file-level comment explaining the hierarchy
(Manager, Dictionary, DictionaryWrapper, DictionarySet) comes in handy, with
diagrams and the like, along with a bit of an explanation of the problem and why
there's the complexity.

https://codereview.chromium.org/711753003/diff/200001/net/base/sdch_manager.h...
net/base/sdch_manager.h:45: class DictionarySet;
You don't actually need to forward declare this here; this symbol is unused
until the full declaration is provided.

https://codereview.chromium.org/711753003/diff/200001/net/base/sdch_manager.h...
net/base/sdch_manager.h:124: scoped_ptr<base::Clock> clock_;
Do you want each dictionary to carry a Clock? Should you just have the Manager
hold the clock, since the Manager will outlive any Dictionaries, per contract,
and just have the Dictionary take a pointer to the clock?

I don't think this is too terribly expensive - it's just the difference between
allocating a vtable (since DefaultClock is an empty object, but has virtual
methods) & storing a pointer vs just storing a pointer.

https://codereview.chromium.org/711753003/diff/200001/net/base/sdch_manager.h...
net/base/sdch_manager.h:131: // macro.
Rather than this longer comment, which describes the less surprising part, I
would just add a comment around line 103

// Private copy-constructor to support RefCountedData<>, which requires
// that an object stored in it be either DefaultConstructible or
// CopyConstructible

These terms are borrowed from the C++ spec and it's notion of Concepts (
http://en.cppreference.com/w/cpp/concept )

I wouldn't say anything about that here for the =delete.

https://codereview.chromium.org/711753003/diff/200001/net/base/sdch_manager.h...
net/base/sdch_manager.h:154: typedef std::map<std::string,
scoped_refptr<base::RefCountedData<Dictionary>>>
typedefs go up top -
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O...

https://codereview.chromium.org/711753003/diff/200001/net/base/sdch_manager.h...
net/base/sdch_manager.h:170: const SdchManager::Dictionary* Dictionary(const
std::string& hash) const;
nit: Naming - FindDictionary, LookupDictionary, GetDictionary, etc?

https://codereview.chromium.org/711753003/diff/200001/net/base/sdch_manager.h...
net/base/sdch_manager.h:181: SdchManager::Dictionary>> dictionary);
const scoped_refptr<>&

only scoped_ptr<>'s get the move semantics.

https://codereview.chromium.org/711753003/diff/200001/net/url_request/url_req...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/711753003/diff/200001/net/url_request/url_req...
net/url_request/url_request_http_job.cc:367: if (dictionaries_advertised_.get())
{
scoped_ptr implements Testable (it's only scoped_refptr<> that doesn't, and
dcheng is fixing that)

if (dictionaries_advertised) {

https://codereview.chromium.org/711753003/diff/200001/net/url_request/url_req...
net/url_request/url_request_http_job.cc:368: // We are wary of proxies that
discard or damage SDCH encoding.  If a server
*cough* s/.  If/. If/

https://codereview.chromium.org/711753003/diff/200001/net/url_request/url_req...
net/url_request/url_request_http_job.cc:554: if (dictionaries_advertised_.get()
&&
ditto testability

https://codereview.chromium.org/711753003/diff/200001/net/url_request/url_req...
net/url_request/url_request_http_job.cc:582: if (dictionaries_advertised_.get())
{
ditto Testability

[Randy Smith (Not in Mondays), 2014-11-20 23:44:29]

Thanks very much, Bence and Ryan!

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

https://codereview.chromium.org/711753003/diff/200001/net/base/sdch_manager.c...
net/base/sdch_manager.cc:464: auto it = dictionaries_.find(server_hash);
On 2014/11/20 22:50:29, Ryan Sleevi wrote:
> Should this be a const auto&? Doesn't seem like you mutate |it| at all, and
it's
> methods can/should be const.

Done.

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

https://codereview.chromium.org/711753003/diff/200001/net/base/sdch_manager.h...
net/base/sdch_manager.h:4: 
On 2014/11/20 22:50:29, Ryan Sleevi wrote:
> So this is probably where a file-level comment explaining the hierarchy
> (Manager, Dictionary, DictionaryWrapper, DictionarySet) comes in handy, with
> diagrams and the like, along with a bit of an explanation of the problem and
why
> there's the complexity.

Done.

(Side note: DictionaryWrapper doesn't exist anymore, I just apparently hadn't
finished killing it.)

https://codereview.chromium.org/711753003/diff/200001/net/base/sdch_manager.h...
net/base/sdch_manager.h:45: class DictionarySet;
On 2014/11/20 22:50:29, Ryan Sleevi wrote:
> You don't actually need to forward declare this here; this symbol is unused
> until the full declaration is provided.

Done.

https://codereview.chromium.org/711753003/diff/200001/net/base/sdch_manager.h...
net/base/sdch_manager.h:124: scoped_ptr<base::Clock> clock_;
On 2014/11/20 22:50:30, Ryan Sleevi wrote:
> Do you want each dictionary to carry a Clock? Should you just have the Manager
> hold the clock, since the Manager will outlive any Dictionaries, per contract,
> and just have the Dictionary take a pointer to the clock?

If there's a contract to that effect, I need to rewrite it--Dictionaries could
outlive the manager.  DictionarySets keep a scoped_refptr<> to any dictionaries,
as does the manager, so when the manager goes away, only the Dictionaries
without DictionarySet references will go away.  I didn't see such a contract
anywhere, but if you did, please point me at it.  I've added a quick clarifying
comment to the SdchManager class comment.

> I don't think this is too terribly expensive - it's just the difference
between
> allocating a vtable (since DefaultClock is an empty object, but has virtual
> methods) & storing a pointer vs just storing a pointer.

Good, because I'm reluctant to change it for the above reason :-}.

https://codereview.chromium.org/711753003/diff/200001/net/base/sdch_manager.h...
net/base/sdch_manager.h:131: // macro.
On 2014/11/20 22:50:29, Ryan Sleevi wrote:
> Rather than this longer comment, which describes the less surprising part, I
> would just add a comment around line 103
> 
> // Private copy-constructor to support RefCountedData<>, which requires
> // that an object stored in it be either DefaultConstructible or
> // CopyConstructible
> 
> These terms are borrowed from the C++ spec and it's notion of Concepts (
> http://en.cppreference.com/w/cpp/concept )
> 
> I wouldn't say anything about that here for the =delete.
> 

Done.  Having said that, I think it's a little confusing to have
DISALLOW_COPY_AND_ASSIGN functionality without that string appearing somewhere
in the class definition.  But I agree it's a lot of verbiage for a small amount
of possible confusion-remediation.

https://codereview.chromium.org/711753003/diff/200001/net/base/sdch_manager.h...
net/base/sdch_manager.h:154: typedef std::map<std::string,
scoped_refptr<base::RefCountedData<Dictionary>>>
On 2014/11/20 22:50:29, Ryan Sleevi wrote:
> typedefs go up top -
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O...

Done.

https://codereview.chromium.org/711753003/diff/200001/net/base/sdch_manager.h...
net/base/sdch_manager.h:170: const SdchManager::Dictionary* Dictionary(const
std::string& hash) const;
On 2014/11/20 22:50:29, Ryan Sleevi wrote:
> nit: Naming - FindDictionary, LookupDictionary, GetDictionary, etc?

*nod*  Changed to GetDictionary().

https://codereview.chromium.org/711753003/diff/200001/net/base/sdch_manager.h...
net/base/sdch_manager.h:181: SdchManager::Dictionary>> dictionary);
On 2014/11/20 22:50:29, Ryan Sleevi wrote:
> const scoped_refptr<>&
> 
> only scoped_ptr<>'s get the move semantics.

Done.

https://codereview.chromium.org/711753003/diff/200001/net/url_request/url_req...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/711753003/diff/200001/net/url_request/url_req...
net/url_request/url_request_http_job.cc:367: if (dictionaries_advertised_.get())
{
On 2014/11/20 22:50:30, Ryan Sleevi wrote:
> scoped_ptr implements Testable (it's only scoped_refptr<> that doesn't, and
> dcheng is fixing that)
> 
> if (dictionaries_advertised) {

Done.

https://codereview.chromium.org/711753003/diff/200001/net/url_request/url_req...
net/url_request/url_request_http_job.cc:368: // We are wary of proxies that
discard or damage SDCH encoding.  If a server
On 2014/11/20 22:50:30, Ryan Sleevi wrote:
> *cough* s/.  If/. If/

*embarrassed amused look* Done.  (Throughout CL.)

https://codereview.chromium.org/711753003/diff/200001/net/url_request/url_req...
net/url_request/url_request_http_job.cc:554: if (dictionaries_advertised_.get()
&&
On 2014/11/20 22:50:30, Ryan Sleevi wrote:
> ditto testability

Done.

https://codereview.chromium.org/711753003/diff/200001/net/url_request/url_req...
net/url_request/url_request_http_job.cc:582: if (dictionaries_advertised_.get())
{
On 2014/11/20 22:50:30, Ryan Sleevi wrote:
> ditto Testability

You realize that by not just saying also "elsewhere in file" you're training me
to rely on my reviewers to catch all the instances of the problem?  :-}  Done. 
(And yes, I've searched for all uses of it.)

[Randy Smith (Not in Mondays), 2014-11-20 23:44:36]

The CQ bit was checked by rdsmith@chromium.org

[commit-bot: I haz the power, 2014-11-20 23:46:25]

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

[commit-bot: I haz the power, 2014-11-20 23:55:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-20 23:55:44]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

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

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

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

Alexei, would you stamp histograms.xml?

[Alexei Svitkine (slow), 2014-11-21 18:18:06]

lgtm

[Randy Smith (Not in Mondays), 2014-11-21 18:29:12]

The CQ bit was checked by rdsmith@chromium.org

[commit-bot: I haz the power, 2014-11-21 18:31:14]

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

[commit-bot: I haz the power, 2014-11-21 18:35:24]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2014-11-21 18:36:06]

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