Re: ServerPushDiscovery: Session and Learner classses in common. (issue970001)

[mdst...@google.com]

Initial comments


http://page-speed-codereview.appspot.com/970001/diff/1/mod_spdy/common/server_push_discovery_learner.cc
File mod_spdy/common/server_push_discovery_learner.cc (right):

http://page-speed-codereview.appspot.com/970001/diff/1/mod_spdy/common/server_push_discovery_learner.cc#newcode26
mod_spdy/common/server_push_discovery_learner.cc:26: bool EndsWith(const
std::string& url, const std::string& ending) {
Please nuke this an use an existing function/method from
base/strings/string_util.h or base/strings/string_piece.h instead.

http://page-speed-codereview.appspot.com/970001/diff/1/mod_spdy/common/server_push_discovery_learner.cc#newcode65
mod_spdy/common/server_push_discovery_learner.cc:65: int32_t priority =
ExtensionGetPriority(adjacent.adjacent_url);
Can you document what's going on with this priority selection code?

http://page-speed-codereview.appspot.com/970001/diff/1/mod_spdy/common/server_push_discovery_learner.cc#newcode67
mod_spdy/common/server_push_discovery_learner.cc:67: if (priority < 0)
Add braces

http://page-speed-codereview.appspot.com/970001/diff/1/mod_spdy/common/server_push_discovery_learner.cc#newcode70
mod_spdy/common/server_push_discovery_learner.cc:70: //std::cout <<
"PushYes: P" << priority << " " << hit_data.hit_count << "/"
Looks like this was for debugging; don't forget to remove it.

http://page-speed-codereview.appspot.com/970001/diff/1/mod_spdy/common/server_push_discovery_learner.h
File mod_spdy/common/server_push_discovery_learner.h (right):

http://page-speed-codereview.appspot.com/970001/diff/1/mod_spdy/common/server_push_discovery_learner.h#newcode43
mod_spdy/common/server_push_discovery_learner.h:43: struct
ServerPushDiscoveryUrlData {
This (and ServerPushDiscoveryAdjacentData) is essentially private to the
ServerPushDiscoverLearner implementation, right? I don't really like
having those private structs out here public. It'd be nice to at the
very least move them inside the ServerPushDiscoverLearner class, and to
the .cc file if possible (using pointers and forward declarations).

http://page-speed-codereview.appspot.com/970001/diff/1/mod_spdy/common/server_push_discovery_learner.h#newcode50
mod_spdy/common/server_push_discovery_learner.h:50: struct
ServerPushDiscoveryPush {
This should probably also be under ServerPushDiscoveryLearner (but still
public), possibly with a shorter name.

http://page-speed-codereview.appspot.com/970001/diff/1/mod_spdy/common/server_push_discovery_learner.h#newcode57
mod_spdy/common/server_push_discovery_learner.h:57: int priority;
s/int/net::SpdyPriority/ (you'll need to #include
"net/spdy/spdy_protocol.h")

http://page-speed-codereview.appspot.com/970001/diff/1/mod_spdy/common/server_push_discovery_learner.h#newcode69
mod_spdy/common/server_push_discovery_learner.h:69: void
AddFirstHit(const std::string& master_url);
Doc comment?

http://page-speed-codereview.appspot.com/970001/diff/1/mod_spdy/common/server_push_discovery_learner.h#newcode71
mod_spdy/common/server_push_discovery_learner.h:71: void
AddAdjacentHit(const std::string& master_url,
Doc comment?

http://page-speed-codereview.appspot.com/970001/diff/1/mod_spdy/common/server_push_discovery_session.cc
File mod_spdy/common/server_push_discovery_session.cc (right):

http://page-speed-codereview.appspot.com/970001/diff/1/mod_spdy/common/server_push_discovery_session.cc#newcode54
mod_spdy/common/server_push_discovery_session.cc:54: void
ServerPushDiscoverySessionPool::CleanExpired(int64_t request_time) {
Looks like the caller of this needs to be holding lock_. Can you add a
lock_.AssertAcquired(); at the top of the method?

http://page-speed-codereview.appspot.com/970001/diff/1/mod_spdy/common/server_push_discovery_session.cc#newcode58
mod_spdy/common/server_push_discovery_session.cc:58: if
(it->second.TimeFromInit(request_time) > kServerPushSessionTimeout)
Add braces

http://page-speed-codereview.appspot.com/970001/diff/1/mod_spdy/common/server_push_discovery_session.h
File mod_spdy/common/server_push_discovery_session.h (right):

http://page-speed-codereview.appspot.com/970001/diff/1/mod_spdy/common/server_push_discovery_session.h#newcode34
mod_spdy/common/server_push_discovery_session.h:34: class
ServerPushDiscoverySessionPool {
Please document the thread-safety of this class (one short sentence is
fine).

http://page-speed-codereview.appspot.com/970001/diff/1/mod_spdy/common/server_push_discovery_session.h#newcode40
mod_spdy/common/server_push_discovery_session.h:40: // is an objected
owned by the pool and must not be deleted by the caller.
s/objected/object/

http://page-speed-codereview.appspot.com/970001/diff/1/mod_spdy/common/server_push_discovery_session.h#newcode41
mod_spdy/common/server_push_discovery_session.h:41:
ServerPushDiscoverySession* GetExistingSession(int64_t session_id,
Can we have a typedef for session ids?

http://page-speed-codereview.appspot.com/970001/diff/1/mod_spdy/common/server_push_discovery_session.h#newcode45
mod_spdy/common/server_push_discovery_session.h:45: // took a SPDY push.
Returns the session_id for user agent storage.
What does it mean for a request to have taken a SPDY push?

http://page-speed-codereview.appspot.com/970001/diff/1/mod_spdy/common/server_push_discovery_session.h#newcode51
mod_spdy/common/server_push_discovery_session.h:51: void
CleanExpired(int64_t request_time);
Even though this is private, could you add a doc comment? And mention
that the caller is supposed to be holding the lock?

http://page-speed-codereview.appspot.com/970001/diff/1/mod_spdy/common/server_push_discovery_session.h#newcode63
mod_spdy/common/server_push_discovery_session.h:63: void
LogAccess(int64_t now) { last_access_ = now; }
The name LogAccess seems a little odd to me; this method doesn't log
anything in the usual sense.

http://page-speed-codereview.appspot.com/970001/