Improve documentation on Filter and fix pipeline overwrite.

Improve documentation on Filter and fix pipeline overwrite.

BUG=418975
R=bnc@chromium.org

Committed: https://crrev.com/77ae84a416659e22c7d9a7a182fe7ba0335789e3
Cr-Commit-Position: refs/heads/master@{#302307}

[Randy Smith (Not in Mondays), 2014-10-29 17:54:30]

Bence: I'm going to do this CL in two stages.  The first stage is to improve the
documentation for net::Filter, and that's what the CL currently has in it.  I'm
now going to start working on a unit test to repro the problem in the bug, and
then put in the fix.  I figure the documentation review is separable from
reviewing the rest, so I'm tossing it into your queue, but I won't be blocked on
you until the unit test and fix get uploaded.  Review as you will :-}.

[Bence, 2014-10-29 20:00:24]

https://codereview.chromium.org/687263002/diff/1/net/filter/filter.cc
File net/filter/filter.cc (right):

https://codereview.chromium.org/687263002/diff/1/net/filter/filter.cc#newcode9
net/filter/filter.cc:9: // apply to the entire chain.
Should the part of this paragraph about how to use Filters in a chain not be
added to the header file instead, leaving only implementation details here?

https://codereview.chromium.org/687263002/diff/1/net/filter/filter.h
File net/filter/filter.h (right):

https://codereview.chromium.org/687263002/diff/1/net/filter/filter.h#newcode22
net/filter/filter.h:22: // repeatedly to get all the filtered data. After all
data have been fitlered
s/fitlered/filtered/ once you are here.  Thanks.

[Randy Smith (Not in Mondays), 2014-10-30 18:35:42]

Incorporated comments, created test, and applied fix.  PTAL?

https://codereview.chromium.org/687263002/diff/1/net/filter/filter.cc
File net/filter/filter.cc (right):

https://codereview.chromium.org/687263002/diff/1/net/filter/filter.cc#newcode9
net/filter/filter.cc:9: // apply to the entire chain.
On 2014/10/29 20:00:24, Bence wrote:
> Should the part of this paragraph about how to use Filters in a chain not be
> added to the header file instead, leaving only implementation details here?

I'm of two minds on this.  The issue is whether the filter chain functionality
is something that's exposed in the interface.  You can make an argument that
it's not; filter chains are setup by Filter::Factory and PrependNewFilter,
completely within this file.  On the other hand, we do pass the vector of
FilterTypes to Filter::Factory, and it's fairly clear that what it's doing is
creating a filter chain.  I still tend to think that the fact that it supports
chaining is more of an implementation detail than an interface detail--it's not
something callers worry about except for passing that vector of FilterTypes in. 
I've added some commentary to make that clear, but let me know if you disagree
with the basic judgement; as I say, I'm of two minds.

https://codereview.chromium.org/687263002/diff/1/net/filter/filter.h
File net/filter/filter.h (right):

https://codereview.chromium.org/687263002/diff/1/net/filter/filter.h#newcode22
net/filter/filter.h:22: // repeatedly to get all the filtered data. After all
data have been fitlered
On 2014/10/29 20:00:24, Bence wrote:
> s/fitlered/filtered/ once you are here.  Thanks.

Done.

[Bence, 2014-10-30 19:00:52]

1. I downloaded the patch and verified manually that the test indeed fails
without the one line fix, and passes with.  Just to make sure that you are
fixing the right bug and testing the right fix.

2. While trybot sadness mostly does not seem to be related to your patch, there
is at least one exception:
../../net/filter/filter_unittest.cc:415: error: undefined reference to 'bcmp'
at
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes....
 Do you need some #include for bcmp?

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

On 2014/10/30 19:00:52, Bence wrote:
> 1. I downloaded the patch and verified manually that the test indeed fails
> without the one line fix, and passes with.  Just to make sure that you are
> fixing the right bug and testing the right fix.

Thorough :-}.  I actually did something close to test drive development for this
CL; I wrote the test first, confirmed it failed, then put in the fix.  I cheated
a little bit, as I tweaked the test a lot to make sure I was targeting the
filter interface correctly, and then when it was still failing went "I wonder if
that's my bug?" and putting the change in fixed it :-}.

What I was really looking for from you, though, is making sure the code matches
the documentation and that you didn't see any potential gotchas I hadn't
considered in the fix; it's messing with a somewhat complex state diagram, and I
wanted another pair of eyes on that.  Just confirming: You looked at that too?

> 2. While trybot sadness mostly does not seem to be related to your patch,
there
> is at least one exception:
> ../../net/filter/filter_unittest.cc:415: error: undefined reference to 'bcmp'
> at
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes....
>  Do you need some #include for bcmp?

Nah, I need to use the right routine :-}.  The mem*s are the right cross
platform targets.  Done.  

PTAL?

[Bence, 2014-10-31 17:23:52]

On 2014/10/31 17:19:18, rdsmith wrote:
> What I was really looking for from you, though, is making sure the code
matches
> the documentation and that you didn't see any potential gotchas I hadn't
> considered in the fix; it's messing with a somewhat complex state diagram, and
I
> wanted another pair of eyes on that.  Just confirming: You looked at that too?

Okay, let me spend some private time with the state diagram.  I'll get back to
you.

[Bence, 2014-10-31 18:28:15]

LGTM.

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

The CQ bit was checked by rdsmith@chromium.org

[commit-bot: I haz the power, 2014-10-31 18:31:24]

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

[commit-bot: I haz the power, 2014-10-31 19:23:26]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-10-31 19:24:01]

Patchset 4 (id:??) landed as
https://crrev.com/77ae84a416659e22c7d9a7a182fe7ba0335789e3
Cr-Commit-Position: refs/heads/master@{#302307}