Re: ServerPushDiscovery: Filter and configuration changes (issue1140001)

[mdst...@google.com]

Thanks for the CL. Some initial comments.


http://page-speed-codereview.appspot.com/1140001/diff/70001/install/common/spdy.conf.template
File install/common/spdy.conf.template (right):

http://page-speed-codereview.appspot.com/1140001/diff/70001/install/common/spdy.conf.template#newcode25
install/common/spdy.conf.template:25: # highly experimental feature and
off by default.
Nit: for consistency with the above paragraphs, add an extra blank "#"
line between the description paragraph and the commented-out config
directives.

http://page-speed-codereview.appspot.com/1140001/diff/70001/mod_spdy/apache/filters/server_push_discovery_filter.cc
File mod_spdy/apache/filters/server_push_discovery_filter.cc (right):

http://page-speed-codereview.appspot.com/1140001/diff/70001/mod_spdy/apache/filters/server_push_discovery_filter.cc#newcode30
mod_spdy/apache/filters/server_push_discovery_filter.cc:30: #include
"mod_spdy/apache/filters/util_cookies.c"
Yuck. Please don't do it this way.

We already have the Apache header files available. Why can't we just
#include "util_cookies.h", up next to #include "apr_strings.h"?

http://page-speed-codereview.appspot.com/1140001/diff/70001/mod_spdy/apache/filters/server_push_discovery_filter.cc#newcode44
mod_spdy/apache/filters/server_push_discovery_filter.cc:44: apr_status_t
status;
Nit: there's not really any need to declare status all the way up
here...

http://page-speed-codereview.appspot.com/1140001/diff/70001/mod_spdy/apache/filters/server_push_discovery_filter.cc#newcode61
mod_spdy/apache/filters/server_push_discovery_filter.cc:61: status =
ap_cookie_read(
...may as well just declare status here, when we actually initialize it.

http://page-speed-codereview.appspot.com/1140001/diff/70001/mod_spdy/apache/filters/server_push_discovery_filter.cc#newcode65
mod_spdy/apache/filters/server_push_discovery_filter.cc:65: session_id =
apr_atoi64(cookie_val);
It's not entirely clear to me how apr_atoi64 behaves if the string isn't
parsable (I think it sets errno, which we aren't checking here). I'd
mildly prefer to use e.g. base::StringToInt64 from #include
"base/strings/string_number_conversions.h", which has a clear API around
error handling and which we already use elsewhere (refer to
src/third_party/chromium/src/base/strings/string_number_conversions.h).

http://page-speed-codereview.appspot.com/1140001/diff/70001/mod_spdy/apache/filters/server_push_discovery_filter.cc#newcode92
mod_spdy/apache/filters/server_push_discovery_filter.cc:92:
"no-cache=\"set-cookie\"");
Nit: indentation

http://page-speed-codereview.appspot.com/1140001/diff/70001/mod_spdy/apache/filters/server_push_discovery_filter.cc#newcode111
mod_spdy/apache/filters/server_push_discovery_filter.cc:111: ss << "\""
<< it->adjacent_url << "\":" <<
Following the public Google style guide
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Streams),
we try to avoid using stringstreams. I'd prefer using
base::StringPrintf from #include "base/strings/stringprintf.h" here.

http://page-speed-codereview.appspot.com/1140001/diff/70001/mod_spdy/apache/filters/server_push_discovery_filter.h
File mod_spdy/apache/filters/server_push_discovery_filter.h (right):

http://page-speed-codereview.appspot.com/1140001/diff/70001/mod_spdy/apache/filters/server_push_discovery_filter.h#newcode24
mod_spdy/apache/filters/server_push_discovery_filter.h:24: #include
"base/strings/string_piece.h"
Nit: I think you can probably remove some of these includes.

http://page-speed-codereview.appspot.com/1140001/diff/70001/mod_spdy/apache/filters/server_push_discovery_filter.h#newcode32
mod_spdy/apache/filters/server_push_discovery_filter.h:32: void
ServerPushDiscoveryFilterFunc(
There should be unit tests for this filter (in a
server_push_discover_filter_test.cc file). You can take a look at the
tests for the other existing filters for ideas.

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

http://page-speed-codereview.appspot.com/1140001/diff/70001/mod_spdy/common/server_push_discovery_session.h#newcode75
mod_spdy/common/server_push_discovery_session.h:75: int64_t
TimeFromLastAccess(int64_t request_time) const {
Doc comment?

http://page-speed-codereview.appspot.com/1140001/diff/70001/mod_spdy/common/spdy_server_config.h
File mod_spdy/common/spdy_server_config.h (right):

http://page-speed-codereview.appspot.com/1140001/diff/70001/mod_spdy/common/spdy_server_config.h#newcode63
mod_spdy/common/spdy_server_config.h:63: // Sends server push discovery
debug headers to user agent.
s/Sends/Returns true if we should send/, or something along those lines
(this method doesn't actually send the headers!).

http://page-speed-codereview.appspot.com/1140001/diff/70001/mod_spdy/mod_spdy.cc
File mod_spdy/mod_spdy.cc (right):

http://page-speed-codereview.appspot.com/1140001/diff/70001/mod_spdy/mod_spdy.cc#newcode156
mod_spdy/mod_spdy.cc:156: ServerPushDiscoveryFilterFunc(
You've given these two functions the same name (so they are overloads of
each other), even though they're not really the same thing, which makes
me a little uncomfortable.

My inclination, for consistency with surrounding code if nothing else,
would be to simply drop the "Func" suffix from the 5-argument version
being called here.

http://page-speed-codereview.appspot.com/1140001/diff/70001/mod_spdy/mod_spdy.cc#newcode269
mod_spdy/mod_spdy.cc:269: if
(top_level_config->server_push_discovery_enabled()) {
I think there's a possibility here that server push discovery might be
disabled at the top level, but enabled for a particular virtual host.
In that case, the learner wouldn't get created here, but the filter
would still try to run, which probably wouldn't end well.

Maybe take a look at the code above that checks if mod_spdy is enabled
on any server_rec in the list.

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