Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1662763002:
[ON HOLD] Implement pull-based design for content decoding (Closed)
Description[ON HOLD] Implement pull-based design for content decoding
This CL implements a pull-based design for content decoding.
This CL is based on an earlier prototype CL
(crrev.com/1274863002 by ellyjones@).
The design for this pull-based content decoding is proposed
in crbug.com/474858.
BUG=474859
Patch Set 1 : #
Total comments: 26
Patch Set 2 : Refactor common logic #
Total comments: 81
Patch Set 3 : Address comments #
Total comments: 58
Patch Set 4 : Remove new buffer class per Matt's suggestion #Patch Set 5 : Address most of the comments. Still working on Sdch. #Patch Set 6 : Mostly working. Need to address UMA and logs #Patch Set 7 : fix content_decoder_tool #Patch Set 8 : NET_EXPORT_PRIVATE #Patch Set 9 : Remove IOBuffer change #Patch Set 10 : hopefully this will compile #Patch Set 11 : change method signature #Patch Set 12 : fix compile on mac #
Total comments: 56
Patch Set 13 : Fix components_unittests #
Total comments: 25
Patch Set 14 : Address comments #
Total comments: 48
Patch Set 15 : address Matt's comments #
Total comments: 18
Patch Set 16 : Address Matt's comments #
Total comments: 2
Patch Set 17 : add filter_source_stream_unittest.cc and address other comments #
Total comments: 44
Patch Set 18 : Address Randy's comments #Patch Set 19 : change to upstream/downstream #Patch Set 20 : rebased #Depends on Patchset: Messages
Total messages: 129 (84 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== temp BUG= ========== to ========== Implement pull-based design for net::Filter This CL implements a pull-based design for net::Filter. The design is proposed in crbug.com/474858, and based on an earlier prototype CL (crrev.com/1274863002 by ellyjones@). BUG=474859 ==========
Patchset #1 (id:20001) has been deleted
xunjieli@chromium.org changed reviewers: + rdsmith@chromium.org
Hey Randy, here's a first draft. Not quite ready for review yet, though tests are passing. I'd like to talk about how to go forward from here. Since the change is fairly large, I want to make sure I won't break too many things when this land. If you have a moment, please grab me.
Matt: I've cc'd you in because I'd like your eyes to pass over the architectural
musings I write below at some point. I think it's very reasonable for you to
hold off on any level of review on this until Helen and I have got something
we're both comfortable for at the code level, so this is just an FYI.
Helen: I took at look at this. Let's talk tomorrow. The quick summary is:
* The overall approach seems good.
* The only way I can think of to not land this all at once is to create a gasket
from the old filter interface to the new filter interface, land the new
interface, then transition the filters over one at a time. My current opinion
is that that's probably not worth it--that instead we should just try and test
this still really well and accept that we may have a couple of land/revert steps
in our future :-J. But if you want to explore that option, I'm ok with it.
* There are several things I'd like to explore changing at the mid-level of the
architecture (i.e. nothing big picture, but not implementation details either).
I'm happy to hash this through with you further along in the code review, but
I'll mention them here in case you want to think about them. Note that I'm
*not* assuming these are all worth doing, they're just all directions I want to
think about.
** Is it worth it to find a way to have the scoped_ptr<StreamSource> that each
class implements handled by the base class?
** Is it worth it to find a way to indirect each classes Read() method through
the base class, and have the base class implement GetReadBytes().
** Is there a way to avoid WeakPtr<> access both ways back and forth between
URLRequestJob and URLRequestJobStreamSource? Note that this is a tricky
question that's very dependent on the lifetime guarantees among the StreamSource
chain, and the whole point of the URLRequestJobStreamSource gasket is matching
up lifetime issues, so the answer here may well be "no".
** (Not in this CL, but you and Matt are probably the right people to share the
idea with) Is it worthwhile changing the URLRequestJob base classes into
contained classes, which could then be owned by the URLRequestJobStreamSource?
This might simplify ownership issues.
** There's a pattern in the Gzip stream source (only one I read through for this
level of review) that basically goes "Loop { decompress what we have stored ->
output. Read more data from input" until either we get an IO pending from the
input source or run out of output space. I'd think this pattern would be common
to pretty much all StreamSources; I wonder if there's a way to put it into the
base class.
https://codereview.chromium.org/1662763002/diff/60001/net/docs/filter.md
File net/docs/filter.md (right):
https://codereview.chromium.org/1662763002/diff/60001/net/docs/filter.md#newc...
net/docs/filter.md:13: with "X <-- Y" meaning "Y reads bytes from X":
Hmmm. For what it's worth, when I see an arrow between two letters, I think of
the arrow direction as the direction the data moves in, i.e. that "X <-- Y"
would be "X reads bytes from Y".
https://codereview.chromium.org/1662763002/diff/60001/net/filter/gzip_stream_...
File net/filter/gzip_stream_source.cc (right):
https://codereview.chromium.org/1662763002/diff/60001/net/filter/gzip_stream_...
net/filter/gzip_stream_source.cc:104: size_t previous_bytes_read;
nit: Initialize to zero.
https://codereview.chromium.org/1662763002/diff/60001/net/url_request/url_req...
File net/url_request/url_request_job.cc (right):
https://codereview.chromium.org/1662763002/diff/60001/net/url_request/url_req...
net/url_request/url_request_job.cc:70: // last filter in the chain.
nit: Extra comment line.
https://codereview.chromium.org/1662763002/diff/60001/net/url_request/url_req...
net/url_request/url_request_job.cc:87: friend class
base::RefCounted<URLRequestJobStreamSource>;
I'm confused--the pattern I'm used to is that if a class is refcounted it
inherits from RefCounted<classname> and has a friend declaration like this.
What does it mean to not inherit but have the friend decl?
https://codereview.chromium.org/1662763002/diff/60001/net/url_request/url_req...
net/url_request/url_request_job.cc:120: int bytes_read_raw;
nit: I'd init this to zero.
https://codereview.chromium.org/1662763002/diff/60001/net/url_request/url_req...
net/url_request/url_request_job.cc:229: // |Read| above for the contract of this
method.
nit: I think the documentation for |Read| is in the header file.
https://codereview.chromium.org/1662763002/diff/60001/net/url_request/url_req...
net/url_request/url_request_job.cc:780: return source_ ?
source_->GetBytesOutput() : 0;
nit: Comment line indicating when source will be null and why zero is an
appropriate response. (I'm guessing this is only true before we start reading,
in which case 0 is the right response, I just think a comment's worth doing.)
https://codereview.chromium.org/1662763002/diff/60001/net/url_request/url_req...
net/url_request/url_request_job.cc:802: // OnRawReadComplete() callback in the
event that the read completes
nit: I think this comment is actually referring to ReadRawDataComplete(), but
it's not clear because there's a (commented out)
URLRequestJob::OnRawReadComplete and an active
URLRequestJobStreamSource::OnRawReadComplete. It'd be worthwhile making the
reference precise (by either qualifying with the class name or disambiguating
the method names) and making sure it's correct.
On 2016/02/08 23:28:42, Randy Smith - Not in Fridays wrote:
> Matt: I've cc'd you in because I'd like your eyes to pass over the
architectural
> musings I write below at some point. I think it's very reasonable for you to
> hold off on any level of review on this until Helen and I have got something
> we're both comfortable for at the code level, so this is just an FYI.
>
> Helen: I took at look at this. Let's talk tomorrow. The quick summary is:
> * The overall approach seems good.
>
> * The only way I can think of to not land this all at once is to create a
gasket
> from the old filter interface to the new filter interface, land the new
> interface, then transition the filters over one at a time. My current opinion
> is that that's probably not worth it--that instead we should just try and test
> this still really well and accept that we may have a couple of land/revert
steps
> in our future :-J. But if you want to explore that option, I'm ok with it.
>
> * There are several things I'd like to explore changing at the mid-level of
the
> architecture (i.e. nothing big picture, but not implementation details
either).
> I'm happy to hash this through with you further along in the code review, but
> I'll mention them here in case you want to think about them. Note that I'm
> *not* assuming these are all worth doing, they're just all directions I want
to
> think about.
> ** Is it worth it to find a way to have the scoped_ptr<StreamSource> that each
> class implements handled by the base class?
> ** Is it worth it to find a way to indirect each classes Read() method through
> the base class, and have the base class implement GetReadBytes().
> ** Is there a way to avoid WeakPtr<> access both ways back and forth between
> URLRequestJob and URLRequestJobStreamSource? Note that this is a tricky
> question that's very dependent on the lifetime guarantees among the
StreamSource
> chain, and the whole point of the URLRequestJobStreamSource gasket is matching
> up lifetime issues, so the answer here may well be "no".
> ** (Not in this CL, but you and Matt are probably the right people to share
the
> idea with) Is it worthwhile changing the URLRequestJob base classes into
> contained classes, which could then be owned by the URLRequestJobStreamSource?
> This might simplify ownership issues.
> ** There's a pattern in the Gzip stream source (only one I read through for
this
> level of review) that basically goes "Loop { decompress what we have stored ->
> output. Read more data from input" until either we get an IO pending from the
> input source or run out of output space. I'd think this pattern would be
common
> to pretty much all StreamSources; I wonder if there's a way to put it into the
> base class.
>
> https://codereview.chromium.org/1662763002/diff/60001/net/docs/filter.md
> File net/docs/filter.md (right):
>
>
https://codereview.chromium.org/1662763002/diff/60001/net/docs/filter.md#newc...
> net/docs/filter.md:13: with "X <-- Y" meaning "Y reads bytes from X":
> Hmmm. For what it's worth, when I see an arrow between two letters, I think
of
> the arrow direction as the direction the data moves in, i.e. that "X <-- Y"
> would be "X reads bytes from Y".
>
>
https://codereview.chromium.org/1662763002/diff/60001/net/filter/gzip_stream_...
> File net/filter/gzip_stream_source.cc (right):
>
>
https://codereview.chromium.org/1662763002/diff/60001/net/filter/gzip_stream_...
> net/filter/gzip_stream_source.cc:104: size_t previous_bytes_read;
> nit: Initialize to zero.
>
>
https://codereview.chromium.org/1662763002/diff/60001/net/url_request/url_req...
> File net/url_request/url_request_job.cc (right):
>
>
https://codereview.chromium.org/1662763002/diff/60001/net/url_request/url_req...
> net/url_request/url_request_job.cc:70: // last filter in the chain.
> nit: Extra comment line.
>
>
https://codereview.chromium.org/1662763002/diff/60001/net/url_request/url_req...
> net/url_request/url_request_job.cc:87: friend class
> base::RefCounted<URLRequestJobStreamSource>;
> I'm confused--the pattern I'm used to is that if a class is refcounted it
> inherits from RefCounted<classname> and has a friend declaration like this.
> What does it mean to not inherit but have the friend decl?
>
>
https://codereview.chromium.org/1662763002/diff/60001/net/url_request/url_req...
> net/url_request/url_request_job.cc:120: int bytes_read_raw;
> nit: I'd init this to zero.
>
>
https://codereview.chromium.org/1662763002/diff/60001/net/url_request/url_req...
> net/url_request/url_request_job.cc:229: // |Read| above for the contract of
this
> method.
> nit: I think the documentation for |Read| is in the header file.
>
>
https://codereview.chromium.org/1662763002/diff/60001/net/url_request/url_req...
> net/url_request/url_request_job.cc:780: return source_ ?
> source_->GetBytesOutput() : 0;
> nit: Comment line indicating when source will be null and why zero is an
> appropriate response. (I'm guessing this is only true before we start
reading,
> in which case 0 is the right response, I just think a comment's worth doing.)
>
>
https://codereview.chromium.org/1662763002/diff/60001/net/url_request/url_req...
> net/url_request/url_request_job.cc:802: // OnRawReadComplete() callback in the
> event that the read completes
> nit: I think this comment is actually referring to ReadRawDataComplete(), but
> it's not clear because there's a (commented out)
> URLRequestJob::OnRawReadComplete and an active
> URLRequestJobStreamSource::OnRawReadComplete. It'd be worthwhile making the
> reference precise (by either qualifying with the class name or disambiguating
> the method names) and making sure it's correct.
Oh, I should also say: Feel free to ignore the nits above--they're just the kind
of thing I noticed while I was doing a high level review and didn't cost much to
write down. A lot of that code may change due to rearchitecture, so if you
don't want to pay attention to that level of feedback at the moment, that's
fine.
Sorry I didn't chime in on this CL earlier - been in my queue, but I keep on getting sidetracked. Hope to have some feedback today, possibly tomorrow.
On 2016/02/18 16:26:59, mmenke wrote: > Sorry I didn't chime in on this CL earlier - been in my queue, but I keep on > getting sidetracked. Hope to have some feedback today, possibly tomorrow. So remember that you're explicitly granted the option to wait until Helen & I are happy (or at least until Helen wants to bring you in) since I cc'd you just to make sure your eyes passed over some architectural musing at some point; see c#6. Your comments are, as always, welcome, but I don't think required yet.
On 2016/02/08 23:28:42, Randy Smith - Not in Fridays wrote:
> Matt: I've cc'd you in because I'd like your eyes to pass over the
architectural
> musings I write below at some point. I think it's very reasonable for you to
> hold off on any level of review on this until Helen and I have got something
> we're both comfortable for at the code level, so this is just an FYI.
>
> Helen: I took at look at this. Let's talk tomorrow. The quick summary is:
> * The overall approach seems good.
>
> * The only way I can think of to not land this all at once is to create a
gasket
> from the old filter interface to the new filter interface, land the new
> interface, then transition the filters over one at a time. My current opinion
> is that that's probably not worth it--that instead we should just try and test
> this still really well and accept that we may have a couple of land/revert
steps
> in our future :-J. But if you want to explore that option, I'm ok with it.
>
> * There are several things I'd like to explore changing at the mid-level of
the
> architecture (i.e. nothing big picture, but not implementation details
either).
> I'm happy to hash this through with you further along in the code review, but
> I'll mention them here in case you want to think about them. Note that I'm
> *not* assuming these are all worth doing, they're just all directions I want
to
> think about.
> ** Is it worth it to find a way to have the scoped_ptr<StreamSource> that each
> class implements handled by the base class?
> ** Is it worth it to find a way to indirect each classes Read() method through
> the base class, and have the base class implement GetReadBytes().
> ** Is there a way to avoid WeakPtr<> access both ways back and forth between
> URLRequestJob and URLRequestJobStreamSource? Note that this is a tricky
> question that's very dependent on the lifetime guarantees among the
StreamSource
> chain, and the whole point of the URLRequestJobStreamSource gasket is matching
> up lifetime issues, so the answer here may well be "no".
> ** (Not in this CL, but you and Matt are probably the right people to share
the
> idea with) Is it worthwhile changing the URLRequestJob base classes into
> contained classes, which could then be owned by the URLRequestJobStreamSource?
> This might simplify ownership issues.
Not sure this really helps, since something still has to own the
NetworkTransaction, and we still need to know the number of raw bytes received.
We could just make the URLRequestJob itself own everything, rather than having
each entry in the chain own the next entry, I suppose.
> ** There's a pattern in the Gzip stream source (only one I read through for
this
> level of review) that basically goes "Loop { decompress what we have stored ->
> output. Read more data from input" until either we get an IO pending from the
> input source or run out of output space. I'd think this pattern would be
common
> to pretty much all StreamSources; I wonder if there's a way to put it into the
> base class.
The seems like a really good idea to me. They also all need to allocate a
buffer on construction, though not sure URLRequestJob quite fits into it. If we
exclude that class, they all also create their own buffers, too.
>
> https://codereview.chromium.org/1662763002/diff/60001/net/docs/filter.md
> File net/docs/filter.md (right):
>
>
https://codereview.chromium.org/1662763002/diff/60001/net/docs/filter.md#newc...
> net/docs/filter.md:13: with "X <-- Y" meaning "Y reads bytes from X":
> Hmmm. For what it's worth, when I see an arrow between two letters, I think
of
> the arrow direction as the direction the data moves in, i.e. that "X <-- Y"
> would be "X reads bytes from Y".
>
>
https://codereview.chromium.org/1662763002/diff/60001/net/filter/gzip_stream_...
> File net/filter/gzip_stream_source.cc (right):
>
>
https://codereview.chromium.org/1662763002/diff/60001/net/filter/gzip_stream_...
> net/filter/gzip_stream_source.cc:104: size_t previous_bytes_read;
> nit: Initialize to zero.
>
>
https://codereview.chromium.org/1662763002/diff/60001/net/url_request/url_req...
> File net/url_request/url_request_job.cc (right):
>
>
https://codereview.chromium.org/1662763002/diff/60001/net/url_request/url_req...
> net/url_request/url_request_job.cc:70: // last filter in the chain.
> nit: Extra comment line.
>
>
https://codereview.chromium.org/1662763002/diff/60001/net/url_request/url_req...
> net/url_request/url_request_job.cc:87: friend class
> base::RefCounted<URLRequestJobStreamSource>;
> I'm confused--the pattern I'm used to is that if a class is refcounted it
> inherits from RefCounted<classname> and has a friend declaration like this.
> What does it mean to not inherit but have the friend decl?
>
>
https://codereview.chromium.org/1662763002/diff/60001/net/url_request/url_req...
> net/url_request/url_request_job.cc:120: int bytes_read_raw;
> nit: I'd init this to zero.
>
>
https://codereview.chromium.org/1662763002/diff/60001/net/url_request/url_req...
> net/url_request/url_request_job.cc:229: // |Read| above for the contract of
this
> method.
> nit: I think the documentation for |Read| is in the header file.
>
>
https://codereview.chromium.org/1662763002/diff/60001/net/url_request/url_req...
> net/url_request/url_request_job.cc:780: return source_ ?
> source_->GetBytesOutput() : 0;
> nit: Comment line indicating when source will be null and why zero is an
> appropriate response. (I'm guessing this is only true before we start
reading,
> in which case 0 is the right response, I just think a comment's worth doing.)
>
>
https://codereview.chromium.org/1662763002/diff/60001/net/url_request/url_req...
> net/url_request/url_request_job.cc:802: // OnRawReadComplete() callback in the
> event that the read completes
> nit: I think this comment is actually referring to ReadRawDataComplete(), but
> it's not clear because there's a (commented out)
> URLRequestJob::OnRawReadComplete and an active
> URLRequestJobStreamSource::OnRawReadComplete. It'd be worthwhile making the
> reference precise (by either qualifying with the class name or disambiguating
> the method names) and making sure it's correct.
mmenke@chromium.org changed reviewers: + mmenke@chromium.org
https://codereview.chromium.org/1662763002/diff/60001/net/filter/stream_sourc... File net/filter/stream_source.cc (right): https://codereview.chromium.org/1662763002/diff/60001/net/filter/stream_sourc... net/filter/stream_source.cc:91: current = BuildSource(std::move(current), type, sdch_delegate); if current ever ends up null after a BuildSource call, should we return give up, and return a NULL filter? That's what the current code does. https://codereview.chromium.org/1662763002/diff/60001/net/filter/stream_source.h File net/filter/stream_source.h (right): https://codereview.chromium.org/1662763002/diff/60001/net/filter/stream_sourc... net/filter/stream_source.h:22: virtual ~StreamSource(){}; Remove semi-colon https://codereview.chromium.org/1662763002/diff/60001/net/filter/stream_sourc... net/filter/stream_source.h:44: virtual size_t GetBytesOutput() const = 0; Is this method really needed? Can't the URLRequestJob just track how many bytes its read both before and after all filers have been applied? Seems simpler than having near identical byte tracking code in all filter implementations, and testing them all seems like a pain. https://codereview.chromium.org/1662763002/diff/60001/net/url_request/url_req... File net/url_request/url_request_job.cc (right): https://codereview.chromium.org/1662763002/diff/60001/net/url_request/url_req... net/url_request/url_request_job.cc:73: public base::SupportsWeakPtr<URLRequestJobStreamSource> { I don't think you ever vend any weak pointers, so WeakPtr not needed? https://codereview.chromium.org/1662763002/diff/60001/net/url_request/url_req... File net/url_request/url_request_job.h (right): https://codereview.chromium.org/1662763002/diff/60001/net/url_request/url_req... net/url_request/url_request_job.h:365: friend class URLRequestJobStreamSource; Seems like this should just be an inner class instead. Then you don't need it to be a friend, or in the global namespace.
Patchset #3 (id:80001) has been deleted
Thanks for the feedback! I think I have incorporated the comments where I can. Specifically I refactored the common parts shared by the stream sources. Sorry it took me a long time to reply -- I got sidetracked by another project. PTAL. https://codereview.chromium.org/1662763002/diff/60001/net/docs/filter.md File net/docs/filter.md (right): https://codereview.chromium.org/1662763002/diff/60001/net/docs/filter.md#newc... net/docs/filter.md:13: with "X <-- Y" meaning "Y reads bytes from X": On 2016/02/08 23:28:42, Randy Smith - Not in Fridays wrote: > Hmmm. For what it's worth, when I see an arrow between two letters, I think of > the arrow direction as the direction the data moves in, i.e. that "X <-- Y" > would be "X reads bytes from Y". Done. https://codereview.chromium.org/1662763002/diff/60001/net/filter/gzip_stream_... File net/filter/gzip_stream_source.cc (right): https://codereview.chromium.org/1662763002/diff/60001/net/filter/gzip_stream_... net/filter/gzip_stream_source.cc:104: size_t previous_bytes_read; On 2016/02/08 23:28:42, Randy Smith - Not in Fridays wrote: > nit: Initialize to zero. Done. https://codereview.chromium.org/1662763002/diff/60001/net/filter/stream_sourc... File net/filter/stream_source.cc (right): https://codereview.chromium.org/1662763002/diff/60001/net/filter/stream_sourc... net/filter/stream_source.cc:91: current = BuildSource(std::move(current), type, sdch_delegate); On 2016/02/18 22:58:28, mmenke wrote: > if current ever ends up null after a BuildSource call, should we return give up, > and return a NULL filter? That's what the current code does. Done. https://codereview.chromium.org/1662763002/diff/60001/net/filter/stream_source.h File net/filter/stream_source.h (right): https://codereview.chromium.org/1662763002/diff/60001/net/filter/stream_sourc... net/filter/stream_source.h:22: virtual ~StreamSource(){}; On 2016/02/18 22:58:28, mmenke wrote: > Remove semi-colon Done. https://codereview.chromium.org/1662763002/diff/60001/net/url_request/url_req... File net/url_request/url_request_job.cc (right): https://codereview.chromium.org/1662763002/diff/60001/net/url_request/url_req... net/url_request/url_request_job.cc:70: // last filter in the chain. On 2016/02/08 23:28:42, Randy Smith - Not in Fridays wrote: > nit: Extra comment line. Done. Removed. Used that last comment to remind myself. No longer needed. https://codereview.chromium.org/1662763002/diff/60001/net/url_request/url_req... net/url_request/url_request_job.cc:73: public base::SupportsWeakPtr<URLRequestJobStreamSource> { On 2016/02/18 22:58:28, mmenke wrote: > I don't think you ever vend any weak pointers, so WeakPtr not needed? Done. https://codereview.chromium.org/1662763002/diff/60001/net/url_request/url_req... net/url_request/url_request_job.cc:87: friend class base::RefCounted<URLRequestJobStreamSource>; On 2016/02/08 23:28:42, Randy Smith - Not in Fridays wrote: > I'm confused--the pattern I'm used to is that if a class is refcounted it > inherits from RefCounted<classname> and has a friend declaration like this. > What does it mean to not inherit but have the friend decl? Done. https://codereview.chromium.org/1662763002/diff/60001/net/url_request/url_req... net/url_request/url_request_job.cc:120: int bytes_read_raw; On 2016/02/08 23:28:42, Randy Smith - Not in Fridays wrote: > nit: I'd init this to zero. Done. https://codereview.chromium.org/1662763002/diff/60001/net/url_request/url_req... net/url_request/url_request_job.cc:229: // |Read| above for the contract of this method. On 2016/02/08 23:28:42, Randy Smith - Not in Fridays wrote: > nit: I think the documentation for |Read| is in the header file. Done. https://codereview.chromium.org/1662763002/diff/60001/net/url_request/url_req... net/url_request/url_request_job.cc:780: return source_ ? source_->GetBytesOutput() : 0; On 2016/02/08 23:28:42, Randy Smith - Not in Fridays wrote: > nit: Comment line indicating when source will be null and why zero is an > appropriate response. (I'm guessing this is only true before we start reading, > in which case 0 is the right response, I just think a comment's worth doing.) Done. Changed so that we are tracking postfilter_bytes_read in URLRequestJob itself. https://codereview.chromium.org/1662763002/diff/60001/net/url_request/url_req... net/url_request/url_request_job.cc:802: // OnRawReadComplete() callback in the event that the read completes On 2016/02/08 23:28:42, Randy Smith - Not in Fridays wrote: > nit: I think this comment is actually referring to ReadRawDataComplete(), but > it's not clear because there's a (commented out) > URLRequestJob::OnRawReadComplete and an active > URLRequestJobStreamSource::OnRawReadComplete. It'd be worthwhile making the > reference precise (by either qualifying with the class name or disambiguating > the method names) and making sure it's correct. Done. Good catch! https://codereview.chromium.org/1662763002/diff/60001/net/url_request/url_req... File net/url_request/url_request_job.h (right): https://codereview.chromium.org/1662763002/diff/60001/net/url_request/url_req... net/url_request/url_request_job.h:365: friend class URLRequestJobStreamSource; On 2016/02/18 22:58:28, mmenke wrote: > Seems like this should just be an inner class instead. Then you don't need it > to be a friend, or in the global namespace. Done.
My apologies, but I won't be able to do a re-review until Monday or Tuesday. If you need to move fast on this, I'm ok with Matt as single reviewer, but if you can wait, I'd like to be involved. https://codereview.chromium.org/1662763002/diff/60001/net/filter/stream_sourc... File net/filter/stream_source.cc (right): https://codereview.chromium.org/1662763002/diff/60001/net/filter/stream_sourc... net/filter/stream_source.cc:91: current = BuildSource(std::move(current), type, sdch_delegate); On 2016/02/18 22:58:28, mmenke wrote: > if current ever ends up null after a BuildSource call, should we return give up, > and return a NULL filter? That's what the current code does. I've managed to lose track of the debate as to what the right net/ API surface way to deal with a bad Content-Encoding: header is? I'd vote for failing a request with CONTENT_DECODING_FAILED. IMO: If that's what we currently do, we should preserve that behavior; if it isn't, we should change to that behavior in a separate CL and preserve it in this one.
On 2016/03/03 23:05:48, Randy Smith - Not in Fridays wrote: > My apologies, but I won't be able to do a re-review until Monday or Tuesday. If > you need to move fast on this, I'm ok with Matt as single reviewer, but if you > can wait, I'd like to be involved. Thanks! Please take your time. I am not in a hurry. (There are still a couple of TODOs left by Elly in the new SDCH code that I need to figure out what they mean.)
On 2016/03/03 23:07:45, xunjieli wrote: > On 2016/03/03 23:05:48, Randy Smith - Not in Fridays wrote: > > My apologies, but I won't be able to do a re-review until Monday or Tuesday. > If > > you need to move fast on this, I'm ok with Matt as single reviewer, but if you > > can wait, I'd like to be involved. > > Thanks! Please take your time. I am not in a hurry. (There are still a couple of > TODOs left by Elly in the new SDCH code that I need to figure out what they > mean.) Given the size of this CL, I think it's extremely unlikely I'll sign it off on it before you get a chance to re-review it, Randy. :)
Nothing close to a full review. https://codereview.chromium.org/1662763002/diff/100001/net/filter/gzip_stream... File net/filter/gzip_stream_source.cc (right): https://codereview.chromium.org/1662763002/diff/100001/net/filter/gzip_stream... net/filter/gzip_stream_source.cc:125: // Sometime misconfigured servers omit the zlib header, relying on clients to Sometimes https://codereview.chromium.org/1662763002/diff/100001/net/filter/gzip_stream... File net/filter/gzip_stream_source.h (right): https://codereview.chromium.org/1662763002/diff/100001/net/filter/gzip_stream... net/filter/gzip_stream_source.h:9: typedef struct z_stream_s z_stream; I don't think we want this in the global namespace. Maybe just use zstream_s? Or could move it into the private section of GzipStreamSource. Also, I think the C++0x11 way of doing this is "using z_stream = z_stream_s;" https://codereview.chromium.org/1662763002/diff/100001/net/filter/gzip_stream... net/filter/gzip_stream_source.h:22: GzipStreamSource(scoped_ptr<StreamSource> previous); explicit https://codereview.chromium.org/1662763002/diff/100001/net/filter/gzip_stream... net/filter/gzip_stream_source.h:25: bool Init(GzipStreamSourceMode mode, bool gzip_fallback); These arguments need to be documented. https://codereview.chromium.org/1662763002/diff/100001/net/filter/gzip_stream... net/filter/gzip_stream_source.h:79: bool gzip_fallback_unchecked_; These need to be documented. Also, "unchecked" is not a great name for a bool - double negatives are confusing. https://codereview.chromium.org/1662763002/diff/100001/net/filter/stream_sour... File net/filter/stream_source.cc (right): https://codereview.chromium.org/1662763002/diff/100001/net/filter/stream_sour... net/filter/stream_source.cc:39: } If we see an unrecognized scheme, we should just give up, right? https://codereview.chromium.org/1662763002/diff/100001/net/filter/stream_sour... net/filter/stream_source.cc:45: case StreamSource::SOURCE_BROTLI: We presumably won't change these often, but I suggest a separate file with a list of these, and then include it in two other files: One with the enum class, and one that converts it to a string. Less likely to break when adding a filter. https://codereview.chromium.org/1662763002/diff/100001/net/filter/stream_sour... net/filter/stream_source.cc:59: } NOTREACHED()? https://codereview.chromium.org/1662763002/diff/100001/net/filter/stream_sour... net/filter/stream_source.cc:63: scoped_ptr<StreamSource> BuildSource(scoped_ptr<StreamSource> current, Randy may have an opinion here, but I think it's a little weird to have stream_source dependent on (almost) all of its subclasses. May make sense to have a separate file to handle construction. Or not, don't feel strongly about this. https://codereview.chromium.org/1662763002/diff/100001/net/filter/stream_sour... net/filter/stream_source.cc:69: current = std::move(next); overwriting current with next seems weird. Suggest renaming current to previous, and reutrning next. https://codereview.chromium.org/1662763002/diff/100001/net/filter/stream_sour... net/filter/stream_source.cc:74: current = std::move(next); On failure, just return NULL? I'd also like to see tests for the failure case here (Not sure we can make a gzip init failure, but I assume we can make an SDCH one) https://codereview.chromium.org/1662763002/diff/100001/net/filter/stream_sour... net/filter/stream_source.cc:85: current = std::move(next); On failure, just return NULL? https://codereview.chromium.org/1662763002/diff/100001/net/filter/stream_sour... net/filter/stream_source.cc:124: break; This previous_ magic (And the magic in the constructor) seems weird. Suggest either making this method part of a StreamSource subclass that the URLRequestJob's StreamSource does not inherit from, or having URLRequestJob's StreamSource class override Read. The first option seems to better get rid of the previous_ maginc in the constructor. https://codereview.chromium.org/1662763002/diff/100001/net/filter/stream_sour... net/filter/stream_source.cc:149: pending_read_buffer_ = dest_buffer; Hrm...seems a little weird to set these when this is called from StreamSource::OnReadComplete. Could just make them OnReadComplete arguments and base::Bind them. We're already doing that for dest_buffer, actually, and doesn't seem like we need it both as an argument to OnReadComplete, and as a member of |this|. Randy may disagree on style here. https://codereview.chromium.org/1662763002/diff/100001/net/filter/stream_sour... net/filter/stream_source.cc:176: if (!callback_.is_null()) { Why check this here, but not in the "error != OK" case? Also, suggest combining this and the error != OK case. (Know I suggested splitting up combined cases elsewhere, but here they share more code, and the checks are a little simpler) https://codereview.chromium.org/1662763002/diff/100001/net/filter/stream_sour... net/filter/stream_source.cc:196: SdchStreamSourceDelegate* sdch_delegate) { Suggest just passing in the headers, and walking through them here. That means we don't have to assembly type_names. And in the future, we can grab the SDCH dictionary from it, too, instead of using the sdch_delegate to get it. Beyond that, we take in a set of headers, walk through them, make strings, walk through them, get types, split based on type, and actually do stuff based on them. Can we just go straight from headers to to filters? Still may need types for logging purposes, though, I suppose. https://codereview.chromium.org/1662763002/diff/100001/net/filter/stream_sour... File net/filter/stream_source.h (right): https://codereview.chromium.org/1662763002/diff/100001/net/filter/stream_sour... net/filter/stream_source.h:38: virtual ~StreamSource(); nit: Blank line after destructor https://codereview.chromium.org/1662763002/diff/100001/net/url_request/url_re... File net/url_request/url_request_job.cc (left): https://codereview.chromium.org/1662763002/diff/100001/net/url_request/url_re... net/url_request/url_request_job.cc:753: filter_->type(), Filter::FILTER_TYPE_MAX); Are we just getting rid of this histogram? https://codereview.chromium.org/1662763002/diff/100001/net/url_request/url_re... net/url_request/url_request_job.cc:769: filtered_read_buffer_->data()); Think we want to keep this log event. https://codereview.chromium.org/1662763002/diff/100001/net/url_request/url_re... File net/url_request/url_request_job.cc (right): https://codereview.chromium.org/1662763002/diff/100001/net/url_request/url_re... net/url_request/url_request_job.cc:95: DCHECK(job); If we can DCHECK on it, I don't think it needs to be a weak ptr. https://codereview.chromium.org/1662763002/diff/100001/net/url_request/url_re... net/url_request/url_request_job.cc:581: cb.Run(OK, bytes_read); Can just use: base::ResetAndReturn(read_raw_callback_).Run(OK, bytes_read); https://codereview.chromium.org/1662763002/diff/100001/net/url_request/url_re... net/url_request/url_request_job.cc:596: #endif Should remove all the commented out code. https://codereview.chromium.org/1662763002/diff/100001/net/url_request/url_re... net/url_request/url_request_job.cc:733: return postfilter_bytes_read_; Making prefilter_bytes_read use a checked_cast, and this one not, seems weird. We should probably make these return size_t's at some point, to be consistent, anyways, but that's beyond the scope of this CL. https://codereview.chromium.org/1662763002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1662763002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:3770: +</histogram> I don't think this belongs in this CL.
I've focussed on the StreamSource abstraction in this review; once we have that basically settled, I'll widen my focus. https://codereview.chromium.org/1662763002/diff/100001/net/filter/block_buffer.h File net/filter/block_buffer.h (right): https://codereview.chromium.org/1662763002/diff/100001/net/filter/block_buffe... net/filter/block_buffer.h:19: // its previous block is not completely drained. suggestion: Put these last two requirements in terms of the methods on the class? I think that means "Note that it is an error to drain more bytes than are available in the BlockBuffer (i.e. calling WasDrained() with |filled_bytes > bytes_left()|) or to refill the BlockBuffer when its previous block is not completely drained (|hasMoreBytes()| is true). https://codereview.chromium.org/1662763002/diff/100001/net/filter/block_buffe... net/filter/block_buffer.h:32: IOBuffer* buffer() { return buffer_.get(); } I think it's important to document the distinction between bytes() and buffer(). I presume that bytes() points to the next byte that may be read/drained from the buffer, and buffer() to the full buffer (that will be written into on filling)? https://codereview.chromium.org/1662763002/diff/100001/net/filter/stream_sour... File net/filter/stream_source.cc (right): https://codereview.chromium.org/1662763002/diff/100001/net/filter/stream_sour... net/filter/stream_source.cc:39: } On 2016/03/04 21:15:57, mmenke wrote: > If we see an unrecognized scheme, we should just give up, right? Define "give up"? The current behavior (which I don't like, but think we should change in a separate CL) appears to me to be that if we fail to recognize any encoding type, we don't do any filtering at all but let the request go through. I'd recommend keeping that behavior in this CL (or changing it in a dependent CL), which I think requires changing this code. https://codereview.chromium.org/1662763002/diff/100001/net/filter/stream_sour... net/filter/stream_source.cc:63: scoped_ptr<StreamSource> BuildSource(scoped_ptr<StreamSource> current, On 2016/03/04 21:15:57, mmenke wrote: > Randy may have an opinion here, but I think it's a little weird to have > stream_source dependent on (almost) all of its subclasses. May make sense to > have a separate file to handle construction. Or not, don't feel strongly about > this. This matches the (IMO bad for the reasons you articulate) pattern that currently exists for filter.cc. I'd be happy to see it changed in this CL, but ok if it's not; I think it would take some real work to actually change it (since I think that means pretty much removing awareness of the StreamSource type from StreamSource). If we change it, I'd vote more in favor of putting the build function in either url_request_job.cc or url_request_http_job.cc (probably url_request_job.cc). https://codereview.chromium.org/1662763002/diff/100001/net/filter/stream_sour... net/filter/stream_source.cc:97: buffer_.reset(new BlockBuffer()); Could get rid of this magic here by allocating it on first read. https://codereview.chromium.org/1662763002/diff/100001/net/filter/stream_sour... net/filter/stream_source.cc:117: (error != OK && error != ERR_IO_PENDING)) Is ReadInternal *allowed* to return ERR_IO_PENDING? I guess it's valid for the first filter in the stream, but any others? If not (i.e. if we deal with the first filter in some other fashion as discussed elsewhere in these reviews) we should probably DCHECK() against ERR_IO_PENDING and document its illegality in the header file. https://codereview.chromium.org/1662763002/diff/100001/net/filter/stream_sour... net/filter/stream_source.cc:124: break; On 2016/03/04 21:15:57, mmenke wrote: > This previous_ magic (And the magic in the constructor) seems weird. Suggest > either making this method part of a StreamSource subclass that the > URLRequestJob's StreamSource does not inherit from, or having URLRequestJob's > StreamSource class override Read. > > The first option seems to better get rid of the previous_ maginc in the > constructor. Based on my current understanding, I'd prefer the second option with the block_buffer_ being allocated on first read. But that does mean that the Read() method has to be documented to be dependent on a non-null previous_ value passed to the constructor. https://codereview.chromium.org/1662763002/diff/100001/net/filter/stream_sour... net/filter/stream_source.cc:130: base::Bind(&StreamSource::OnReadComplete, base::Unretained(this), nit, knee-jerk response: Use of Unretained should have a comment as to why it's safe. https://codereview.chromium.org/1662763002/diff/100001/net/filter/stream_sour... net/filter/stream_source.cc:134: // Decompress already failed to return any more data, this source is also nit: I think you mean ReadInternal, not Decompress. https://codereview.chromium.org/1662763002/diff/100001/net/filter/stream_sour... net/filter/stream_source.cc:149: pending_read_buffer_ = dest_buffer; On 2016/03/04 21:15:57, mmenke wrote: > Hrm...seems a little weird to set these when this is called from > StreamSource::OnReadComplete. Could just make them OnReadComplete arguments and > base::Bind them. We're already doing that for dest_buffer, actually, and > doesn't seem like we need it both as an argument to OnReadComplete, and as a > member of |this|. > > Randy may disagree on style here. No, I think I agree. I'd vote for making the callback hold the reference (which is closer to where/how it's going to be used) and getting rid of pending_read_buffer_. https://codereview.chromium.org/1662763002/diff/100001/net/filter/stream_sour... net/filter/stream_source.cc:185: // Recurse. Read runs the callback if completes synchronously, I think this comment is wrong? The code is calling the callback on synchronous return below, and I don't see Read() calling it above. https://codereview.chromium.org/1662763002/diff/100001/net/filter/stream_sour... File net/filter/stream_source.h (right): https://codereview.chromium.org/1662763002/diff/100001/net/filter/stream_sour... net/filter/stream_source.h:47: // when done. This comment reads to me like an implementation comment, not an interface comment. If you agree with that statement, let's move this comment to the .cc file. The comment here should be everything that a consumer of this class needs to know, and no more; I think that's more on the order of "This call initiates a read from the filter chain starting at this filter." followed with the below information. https://codereview.chromium.org/1662763002/diff/100001/net/filter/stream_sour... net/filter/stream_source.h:67: void OnReadComplete(IOBuffer* dest_buffer, I'm fairly disturbed (read: Please don't do this :-}) by the common name of OnReadComplete() for routines in both the base and derived classes without an inheritance relationship; that seems like it's inviting code readers to confuse which method is actually being called. https://codereview.chromium.org/1662763002/diff/100001/net/filter/stream_sour... net/filter/stream_source.h:70: size_t bytes_read); Why isn't this protected:? I'm not sure which interface I prefer, but I'll call out for your consideration that, since this feels to me like part of the interface to ReadInternal, it might be a cleaner interface to just pass ReadInternal a callback rather than make subclasses know about a special entry point. ETA: Actually, it doesn't look to me like subclasses ever call this, other than through the callback passed to Read(), which is a fine interface. Maybe the comment above is just wrong? (And this should be private?) Apologies if there's something about the intricacies of how this is used I'm missing. https://codereview.chromium.org/1662763002/diff/100001/net/filter/stream_sour... net/filter/stream_source.h:91: OnReadCompleteCallback callback_; nit: Comment about when non-null? (Presumably when there's a read outstanding.) https://codereview.chromium.org/1662763002/diff/100001/net/filter/stream_sour... net/filter/stream_source.h:93: scoped_ptr<BlockBuffer> buffer_; nit: Comment about how used? https://codereview.chromium.org/1662763002/diff/100001/net/filter/stream_sour... net/filter/stream_source.h:93: scoped_ptr<BlockBuffer> buffer_; Did you consider making this an explicit part of the ReadInternal interface (and hence a private data member)? I think I'd prefer that unless it's a real PITA, since it makes the base/derived class interface more explicit (it's just ReadInternal). https://codereview.chromium.org/1662763002/diff/100001/net/filter/stream_sour... net/filter/stream_source.h:99: // finishing asynchronously. This comment contradicts the one above on OnReadComplete (though I think this one is accurate). Having said that, would it be reasonable to make an explicit callback argument to this interface and remove the magic of reaching into the base class to access callback_? If we can make the base/derived class interface very explicit and in one place I think that's architectural goodness. https://codereview.chromium.org/1662763002/diff/100001/net/url_request/url_re... File net/url_request/url_request_job.cc (left): https://codereview.chromium.org/1662763002/diff/100001/net/url_request/url_re... net/url_request/url_request_job.cc:753: filter_->type(), Filter::FILTER_TYPE_MAX); On 2016/03/04 21:15:57, mmenke wrote: > Are we just getting rid of this histogram? I'd like to keep this histogram; it's a pain when we see a CONTENT_DECODING_FAILED spike and can't figure out which code to look at. Especially as we have a new filter coming in.
https://codereview.chromium.org/1662763002/diff/100001/net/filter/stream_sour... File net/filter/stream_source.cc (right): https://codereview.chromium.org/1662763002/diff/100001/net/filter/stream_sour... net/filter/stream_source.cc:39: } On 2016/03/09 23:03:56, Randy Smith - Not in Fridays wrote: > On 2016/03/04 21:15:57, mmenke wrote: > > If we see an unrecognized scheme, we should just give up, right? > > Define "give up"? The current behavior (which I don't like, but think we should > change in a separate CL) appears to me to be that if we fail to recognize any > encoding type, we don't do any filtering at all but let the request go through. > I'd recommend keeping that behavior in this CL (or changing it in a dependent > CL), which I think requires changing this code. That's what I meant by "give up". :)
Helen: Any idea when you'll be getting back to this? I think we should have some fuzzers for our filters, and it seems best for all concerned to only do that after this CL lands.
On 2016/03/25 20:25:38, mmenke wrote: > Helen: Any idea when you'll be getting back to this? I think we should have > some fuzzers for our filters, and it seems best for all concerned to only do > that after this CL lands. I think I can get back to this in one more week from today, which will be the first week of April.
On 2016/03/25 20:28:10, xunjieli wrote: > On 2016/03/25 20:25:38, mmenke wrote: > > Helen: Any idea when you'll be getting back to this? I think we should have > > some fuzzers for our filters, and it seems best for all concerned to only do > > that after this CL lands. > > I think I can get back to this in one more week from today, which will be the > first week of April. I am very sorry about the delay. Looks like I won't be able to get back to it this week :( But it is my top priority to get this done in Q2.
On 2016/04/04 18:47:27, xunjieli wrote: > On 2016/03/25 20:28:10, xunjieli wrote: > > On 2016/03/25 20:25:38, mmenke wrote: > > > Helen: Any idea when you'll be getting back to this? I think we should > have > > > some fuzzers for our filters, and it seems best for all concerned to only do > > > that after this CL lands. > > > > I think I can get back to this in one more week from today, which will be the > > first week of April. > > I am very sorry about the delay. Looks like I won't be able to get back to it > this week :( > But it is my top priority to get this done in Q2. No problem. I've had plenty of time to do a pass over more of the CL, and haven't gotten to it, myself.
This CL is in the "important but not urgent" category, which makes me not worry about delays, but want to make very sure it doesn't actually fall off your radar screen. So "high priority in Q2" SGTM. Thanks for the update.
Thanks! I think I have addressed all comments except those about UMA stats gathering. I have a few questions on SdchStreamSource, SdchStreamSourceDelegate, and SdchPolicyDelegate. I will look into those. https://codereview.chromium.org/1662763002/diff/100001/net/filter/block_buffer.h File net/filter/block_buffer.h (right): https://codereview.chromium.org/1662763002/diff/100001/net/filter/block_buffe... net/filter/block_buffer.h:19: // its previous block is not completely drained. On 2016/03/09 23:03:56, Randy Smith - Not in Fridays wrote: > suggestion: Put these last two requirements in terms of the methods on the > class? I think that means "Note that it is an error to drain more bytes than > are available in the BlockBuffer (i.e. calling WasDrained() with |filled_bytes > > bytes_left()|) or to refill the BlockBuffer when its previous block is not > completely drained (|hasMoreBytes()| is true). Done. https://codereview.chromium.org/1662763002/diff/100001/net/filter/block_buffe... net/filter/block_buffer.h:32: IOBuffer* buffer() { return buffer_.get(); } On 2016/03/09 23:03:55, Randy Smith - Not in Fridays wrote: > I think it's important to document the distinction between bytes() and buffer(). > I presume that bytes() points to the next byte that may be read/drained from > the buffer, and buffer() to the full buffer (that will be written into on > filling)? Done. https://codereview.chromium.org/1662763002/diff/100001/net/filter/gzip_stream... File net/filter/gzip_stream_source.cc (right): https://codereview.chromium.org/1662763002/diff/100001/net/filter/gzip_stream... net/filter/gzip_stream_source.cc:125: // Sometime misconfigured servers omit the zlib header, relying on clients to On 2016/03/04 21:15:56, mmenke wrote: > Sometimes Done. https://codereview.chromium.org/1662763002/diff/100001/net/filter/gzip_stream... File net/filter/gzip_stream_source.h (right): https://codereview.chromium.org/1662763002/diff/100001/net/filter/gzip_stream... net/filter/gzip_stream_source.h:9: typedef struct z_stream_s z_stream; On 2016/03/04 21:15:56, mmenke wrote: > I don't think we want this in the global namespace. Maybe just use zstream_s? > Or could move it into the private section of GzipStreamSource. > > Also, I think the C++0x11 way of doing this is "using z_stream = z_stream_s;" I tried this suggestion, but the code failed to compile. Can we tackle this in a separate CL? This line is copied over from gzip_filter.h https://codereview.chromium.org/1662763002/diff/100001/net/filter/gzip_stream... net/filter/gzip_stream_source.h:22: GzipStreamSource(scoped_ptr<StreamSource> previous); On 2016/03/04 21:15:56, mmenke wrote: > explicit Done. https://codereview.chromium.org/1662763002/diff/100001/net/filter/gzip_stream... net/filter/gzip_stream_source.h:25: bool Init(GzipStreamSourceMode mode, bool gzip_fallback); On 2016/03/04 21:15:56, mmenke wrote: > These arguments need to be documented. Done. https://codereview.chromium.org/1662763002/diff/100001/net/filter/gzip_stream... net/filter/gzip_stream_source.h:79: bool gzip_fallback_unchecked_; On 2016/03/04 21:15:56, mmenke wrote: > These need to be documented. Also, "unchecked" is not a great name for a bool - > double negatives are confusing. Done. https://codereview.chromium.org/1662763002/diff/100001/net/filter/stream_sour... File net/filter/stream_source.cc (right): https://codereview.chromium.org/1662763002/diff/100001/net/filter/stream_sour... net/filter/stream_source.cc:39: } On 2016/03/09 23:11:50, mmenke wrote: > On 2016/03/09 23:03:56, Randy Smith - Not in Fridays wrote: > > On 2016/03/04 21:15:57, mmenke wrote: > > > If we see an unrecognized scheme, we should just give up, right? > > > > Define "give up"? The current behavior (which I don't like, but think we > should > > change in a separate CL) appears to me to be that if we fail to recognize any > > encoding type, we don't do any filtering at all but let the request go > through. > > I'd recommend keeping that behavior in this CL (or changing it in a dependent > > CL), which I think requires changing this code. > > That's what I meant by "give up". :) Acknowledged. https://codereview.chromium.org/1662763002/diff/100001/net/filter/stream_sour... net/filter/stream_source.cc:45: case StreamSource::SOURCE_BROTLI: On 2016/03/04 21:15:57, mmenke wrote: > We presumably won't change these often, but I suggest a separate file with a > list of these, and then include it in two other files: One with the enum class, > and one that converts it to a string. Less likely to break when adding a > filter. Done. https://codereview.chromium.org/1662763002/diff/100001/net/filter/stream_sour... net/filter/stream_source.cc:59: } On 2016/03/04 21:15:56, mmenke wrote: > NOTREACHED()? Done. https://codereview.chromium.org/1662763002/diff/100001/net/filter/stream_sour... net/filter/stream_source.cc:63: scoped_ptr<StreamSource> BuildSource(scoped_ptr<StreamSource> current, On 2016/03/09 23:03:56, Randy Smith - Not in Fridays wrote: > On 2016/03/04 21:15:57, mmenke wrote: > > Randy may have an opinion here, but I think it's a little weird to have > > stream_source dependent on (almost) all of its subclasses. May make sense to > > have a separate file to handle construction. Or not, don't feel strongly > about > > this. > > This matches the (IMO bad for the reasons you articulate) pattern that currently > exists for filter.cc. I'd be happy to see it changed in this CL, but ok if it's > not; I think it would take some real work to actually change it (since I think > that means pretty much removing awareness of the StreamSource type from > StreamSource). If we change it, I'd vote more in favor of putting the build > function in either url_request_job.cc or url_request_http_job.cc (probably > url_request_job.cc). Done. I moved them to a util file. https://codereview.chromium.org/1662763002/diff/100001/net/filter/stream_sour... net/filter/stream_source.cc:69: current = std::move(next); On 2016/03/04 21:15:57, mmenke wrote: > overwriting current with next seems weird. Suggest renaming current to > previous, and reutrning next. Done. https://codereview.chromium.org/1662763002/diff/100001/net/filter/stream_sour... net/filter/stream_source.cc:74: current = std::move(next); On 2016/03/04 21:15:57, mmenke wrote: > On failure, just return NULL? I'd also like to see tests for the failure case > here (Not sure we can make a gzip init failure, but I assume we can make an SDCH > one) Partially DONE. How do we make SDCH's init() fail? Do we mock out that method? I am not quite sure how to do that. https://codereview.chromium.org/1662763002/diff/100001/net/filter/stream_sour... net/filter/stream_source.cc:85: current = std::move(next); On 2016/03/04 21:15:56, mmenke wrote: > On failure, just return NULL? Done. https://codereview.chromium.org/1662763002/diff/100001/net/filter/stream_sour... net/filter/stream_source.cc:97: buffer_.reset(new BlockBuffer()); On 2016/03/09 23:03:56, Randy Smith - Not in Fridays wrote: > Could get rid of this magic here by allocating it on first read. Done. https://codereview.chromium.org/1662763002/diff/100001/net/filter/stream_sour... net/filter/stream_source.cc:117: (error != OK && error != ERR_IO_PENDING)) On 2016/03/09 23:03:56, Randy Smith - Not in Fridays wrote: > Is ReadInternal *allowed* to return ERR_IO_PENDING? I guess it's valid for the > first filter in the stream, but any others? If not (i.e. if we deal with the > first filter in some other fashion as discussed elsewhere in these reviews) we > should probably DCHECK() against ERR_IO_PENDING and document its illegality in > the header file. Done. https://codereview.chromium.org/1662763002/diff/100001/net/filter/stream_sour... net/filter/stream_source.cc:124: break; On 2016/03/09 23:03:56, Randy Smith - Not in Fridays wrote: > On 2016/03/04 21:15:57, mmenke wrote: > > This previous_ magic (And the magic in the constructor) seems weird. Suggest > > either making this method part of a StreamSource subclass that the > > URLRequestJob's StreamSource does not inherit from, or having URLRequestJob's > > StreamSource class override Read. > > > > The first option seems to better get rid of the previous_ maginc in the > > constructor. > > Based on my current understanding, I'd prefer the second option with the > block_buffer_ being allocated on first read. But that does mean that the Read() > method has to be documented to be dependent on a non-null previous_ value passed > to the constructor. Done. https://codereview.chromium.org/1662763002/diff/100001/net/filter/stream_sour... net/filter/stream_source.cc:124: break; On 2016/03/04 21:15:57, mmenke wrote: > This previous_ magic (And the magic in the constructor) seems weird. Suggest > either making this method part of a StreamSource subclass that the > URLRequestJob's StreamSource does not inherit from, or having URLRequestJob's > StreamSource class override Read. > > The first option seems to better get rid of the previous_ maginc in the > constructor. Done. https://codereview.chromium.org/1662763002/diff/100001/net/filter/stream_sour... net/filter/stream_source.cc:130: base::Bind(&StreamSource::OnReadComplete, base::Unretained(this), On 2016/03/09 23:03:56, Randy Smith - Not in Fridays wrote: > nit, knee-jerk response: Use of Unretained should have a comment as to why it's > safe. Done. https://codereview.chromium.org/1662763002/diff/100001/net/filter/stream_sour... net/filter/stream_source.cc:134: // Decompress already failed to return any more data, this source is also On 2016/03/09 23:03:56, Randy Smith - Not in Fridays wrote: > nit: I think you mean ReadInternal, not Decompress. Done. https://codereview.chromium.org/1662763002/diff/100001/net/filter/stream_sour... net/filter/stream_source.cc:149: pending_read_buffer_ = dest_buffer; On 2016/03/09 23:03:56, Randy Smith - Not in Fridays wrote: > On 2016/03/04 21:15:57, mmenke wrote: > > Hrm...seems a little weird to set these when this is called from > > StreamSource::OnReadComplete. Could just make them OnReadComplete arguments > and > > base::Bind them. We're already doing that for dest_buffer, actually, and > > doesn't seem like we need it both as an argument to OnReadComplete, and as a > > member of |this|. > > > > Randy may disagree on style here. > > No, I think I agree. I'd vote for making the callback hold the reference (which > is closer to where/how it's going to be used) and getting rid of > pending_read_buffer_. Done. https://codereview.chromium.org/1662763002/diff/100001/net/filter/stream_sour... net/filter/stream_source.cc:149: pending_read_buffer_ = dest_buffer; On 2016/03/04 21:15:57, mmenke wrote: > Hrm...seems a little weird to set these when this is called from > StreamSource::OnReadComplete. Could just make them OnReadComplete arguments and > base::Bind them. We're already doing that for dest_buffer, actually, and > doesn't seem like we need it both as an argument to OnReadComplete, and as a > member of |this|. > > Randy may disagree on style here. Done. https://codereview.chromium.org/1662763002/diff/100001/net/filter/stream_sour... net/filter/stream_source.cc:176: if (!callback_.is_null()) { On 2016/03/04 21:15:56, mmenke wrote: > Why check this here, but not in the "error != OK" case? Also, suggest combining > this and the error != OK case. (Know I suggested splitting up combined cases > elsewhere, but here they share more code, and the checks are a little simpler) Done. https://codereview.chromium.org/1662763002/diff/100001/net/filter/stream_sour... net/filter/stream_source.cc:185: // Recurse. Read runs the callback if completes synchronously, On 2016/03/09 23:03:56, Randy Smith - Not in Fridays wrote: > I think this comment is wrong? The code is calling the callback on synchronous > return below, and I don't see Read() calling it above. Done. https://codereview.chromium.org/1662763002/diff/100001/net/filter/stream_sour... net/filter/stream_source.cc:196: SdchStreamSourceDelegate* sdch_delegate) { On 2016/03/04 21:15:56, mmenke wrote: > Suggest just passing in the headers, and walking through them here. That means > we don't have to assembly type_names. And in the future, we can grab the SDCH > dictionary from it, too, instead of using the sdch_delegate to get it. > > Beyond that, we take in a set of headers, walk through them, make strings, walk > through them, get types, split based on type, and actually do stuff based on > them. Can we just go straight from headers to to filters? Still may need types > for logging purposes, though, I suppose. Done. https://codereview.chromium.org/1662763002/diff/100001/net/filter/stream_sour... File net/filter/stream_source.h (right): https://codereview.chromium.org/1662763002/diff/100001/net/filter/stream_sour... net/filter/stream_source.h:38: virtual ~StreamSource(); On 2016/03/04 21:15:57, mmenke wrote: > nit: Blank line after destructor Done. https://codereview.chromium.org/1662763002/diff/100001/net/filter/stream_sour... net/filter/stream_source.h:47: // when done. On 2016/03/09 23:03:56, Randy Smith - Not in Fridays wrote: > This comment reads to me like an implementation comment, not an interface > comment. If you agree with that statement, let's move this comment to the .cc > file. The comment here should be everything that a consumer of this class needs > to know, and no more; I think that's more on the order of "This call initiates a > read from the filter chain starting at this filter." followed with the below > information. Done. https://codereview.chromium.org/1662763002/diff/100001/net/filter/stream_sour... net/filter/stream_source.h:91: OnReadCompleteCallback callback_; On 2016/03/09 23:03:56, Randy Smith - Not in Fridays wrote: > nit: Comment about when non-null? (Presumably when there's a read outstanding.) Done. https://codereview.chromium.org/1662763002/diff/100001/net/filter/stream_sour... net/filter/stream_source.h:93: scoped_ptr<BlockBuffer> buffer_; On 2016/03/09 23:03:56, Randy Smith - Not in Fridays wrote: > nit: Comment about how used? Done. https://codereview.chromium.org/1662763002/diff/100001/net/filter/stream_sour... net/filter/stream_source.h:93: scoped_ptr<BlockBuffer> buffer_; On 2016/03/09 23:03:56, Randy Smith - Not in Fridays wrote: > Did you consider making this an explicit part of the ReadInternal interface (and > hence a private data member)? I think I'd prefer that unless it's a real PITA, > since it makes the base/derived class interface more explicit (it's just > ReadInternal). |buffer_| is used for reading from |previous_|, but sub classes also consume bytes from it in anyway they like. For instance, gzip stream source drains header bytes from |buffer_|, so it can't be just a part of ReadInternal interface and a private data member. https://codereview.chromium.org/1662763002/diff/100001/net/url_request/url_re... File net/url_request/url_request_job.cc (left): https://codereview.chromium.org/1662763002/diff/100001/net/url_request/url_re... net/url_request/url_request_job.cc:753: filter_->type(), Filter::FILTER_TYPE_MAX); On 2016/03/09 23:03:56, Randy Smith - Not in Fridays wrote: > On 2016/03/04 21:15:57, mmenke wrote: > > Are we just getting rid of this histogram? > > I'd like to keep this histogram; it's a pain when we see a > CONTENT_DECODING_FAILED spike and can't figure out which code to look at. > Especially as we have a new filter coming in. Done. moved to stream_source.cc so we can log the type of the stream source. https://codereview.chromium.org/1662763002/diff/100001/net/url_request/url_re... net/url_request/url_request_job.cc:769: filtered_read_buffer_->data()); On 2016/03/04 21:15:57, mmenke wrote: > Think we want to keep this log event. Done. https://codereview.chromium.org/1662763002/diff/100001/net/url_request/url_re... File net/url_request/url_request_job.cc (right): https://codereview.chromium.org/1662763002/diff/100001/net/url_request/url_re... net/url_request/url_request_job.cc:95: DCHECK(job); On 2016/03/04 21:15:57, mmenke wrote: > If we can DCHECK on it, I don't think it needs to be a weak ptr. Done. https://codereview.chromium.org/1662763002/diff/100001/net/url_request/url_re... net/url_request/url_request_job.cc:581: cb.Run(OK, bytes_read); On 2016/03/04 21:15:57, mmenke wrote: > Can just use: > > base::ResetAndReturn(read_raw_callback_).Run(OK, bytes_read); Done. https://codereview.chromium.org/1662763002/diff/100001/net/url_request/url_re... net/url_request/url_request_job.cc:596: #endif On 2016/03/04 21:15:57, mmenke wrote: > Should remove all the commented out code. Done. https://codereview.chromium.org/1662763002/diff/100001/net/url_request/url_re... net/url_request/url_request_job.cc:733: return postfilter_bytes_read_; On 2016/03/04 21:15:57, mmenke wrote: > Making prefilter_bytes_read use a checked_cast, and this one not, seems weird. > We should probably make these return size_t's at some point, to be consistent, > anyways, but that's beyond the scope of this CL. Done. https://codereview.chromium.org/1662763002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1662763002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:3770: +</histogram> On 2016/03/04 21:15:57, mmenke wrote: > I don't think this belongs in this CL. Done.
Helen: Here's my next round of review. Most of my comments are on the base
abstractions, but I did take a look at the SDCH code. My analysis of the
SdchStreamSourceDelegate is that it's an attempt to separate out normal SDCH
processing (pretty much everything you'd think would go into a filter) from all
the stuff that is sorta weird about SDCH (getting the dictionary is weird
because it's based on what we advertised, not on a central store, except we fall
back to the central store if what we advertised doesn't work, and we've got some
"interesting" :-{ error handling code). And I basically like that separation.
But my gut reaction is that it's got too many levels to it--SdchPolicyDelegate
inherits from SdchStreamSourceDelegate, but delegates some further questions to
the SdchPolicyDelegate::Context, which URLRequestHttpJob eventually will export
a class that inherits from and expands the information.
I'd like to explore another option. I like the idea of keeping the SDCH ugly
cruft separate from the SdchStreamSource and separate from URLRequestHttpJob, so
I'd like to keep SdchPolicyDelegate. But what I'd *like* to see if we can do is
initialize SdchPolicyDelegate with all the information it needs to make it's
policy decisions and not having the subclass context and the (implied; it isn't
there in the code yet) link back to URLRequestHttpJob. This may well require
pulling the creation of the filter chain up into URLRequestHttpJob, as the
information about the dictionaries advertised is only available there.
SdchStreamSource would then have a std::unique_ptr<SdchStreamSource::Delegate>,
which would actually be a SdchPolicyDelegate.
If that seems like too much of a hassle, we'll need to come up with some other
way to get information from the URLRequestHttpJob over to the SdchStreamSource
(which used to be done through FilterContext); I'm not certain offhand what that
would be, just that it would need to be plumbed through filter creation. Which
strikes me as architecturally ugly anyway, so I'm in favor of hoisting it up
into
URLRequestHttpJob. But I'm open to alternatives :-}.
https://codereview.chromium.org/1662763002/diff/120001/net/filter/block_buffer.h
File net/filter/block_buffer.h (right):
https://codereview.chromium.org/1662763002/diff/120001/net/filter/block_buffe...
net/filter/block_buffer.h:23: bool HasMoreBytes() const;
Personal preference, therefore suggestion: I find .h files easier to read when,
if there are comments describing methods, there's a single blank line between
the methods, so it's easy to visually associate the comments with the method.
https://codereview.chromium.org/1662763002/diff/120001/net/filter/block_buffe...
net/filter/block_buffer.h:24: // Marks that the buffer has been refilled
|filled_bytes| bytes. It is illegal
nit, suggestion: I think this would be clearer with something like "Marks that
|filled_bytes| bytes has been written into the buffer starting at location
buffer(). This resets the bytes() pointer to buffer(). It is illegal to refill
the buffer when |bytes_left() != 0|.
https://codereview.chromium.org/1662763002/diff/120001/net/filter/block_buffe...
net/filter/block_buffer.h:35: // Returns the pointer to the next byte that may
be read or drained.
nit, suggestion: I'd leave out the "or drained"; in fact, I'd suggest rewriting
these descriptions to not refer to draining bytes, but instead refer to having
read them and thus updating the bytes() pointer.
https://codereview.chromium.org/1662763002/diff/120001/net/filter/block_buffe...
net/filter/block_buffer.h:37: // Returns the number of bytes that are remaining.
nit: I'd use the same language as above in the comment to WasDrained; i.e. use
"remaining" there or "available" here. (I think the words mean the same thing
in this context.)
https://codereview.chromium.org/1662763002/diff/120001/net/filter/block_buffe...
net/filter/block_buffer.h:45: };
Should there be a DISALLOW_COPY_AND_ASSIGN?
https://codereview.chromium.org/1662763002/diff/120001/net/filter/filter_stre...
File net/filter/filter_stream_source.cc (right):
https://codereview.chromium.org/1662763002/diff/120001/net/filter/filter_stre...
net/filter/filter_stream_source.cc:53: if ((error == OK && single_bytes_read >
0) || error != OK)
I believe what this means is that this function will only change *bytes_read
once, because if single_bytes_read is non-zero, the function exits here. If
that's accurate, I'd suggest getting rid of single_bytes_read and just passing
bytes_read into ReadInternal() for simplicity (one less variable, and one that I
experience as having a confusing name).
https://codereview.chromium.org/1662763002/diff/120001/net/filter/filter_stre...
File net/filter/filter_stream_source.h (right):
https://codereview.chromium.org/1662763002/diff/120001/net/filter/filter_stre...
net/filter/filter_stream_source.h:17: class FilterStreamSource : public
StreamSource {
nit: comment as to use.
https://codereview.chromium.org/1662763002/diff/120001/net/filter/filter_stre...
net/filter/filter_stream_source.h:45: virtual net::Error ReadInternal(IOBuffer*
dest_buffer,
I think this would be cleaner if it took an input argument (buffer_, presumably)
so that buffer_ didn't need to be protected and used by all the derived classes.
(Possibly buffer_ should be declared here, but I haven't looked at the
non-filter StreamSource yet, so I'm not sure.)
https://codereview.chromium.org/1662763002/diff/120001/net/filter/sdch_stream...
File net/filter/sdch_stream_source_delegate.h (right):
https://codereview.chromium.org/1662763002/diff/120001/net/filter/sdch_stream...
net/filter/sdch_stream_source_delegate.h:18: class SdchStreamSourceDelegate {
Suggestion: My inclination would be to define this as SdchStreamSource::Delegate
in the sdch_stream_source.h file. I think it's bound tightly with that
interface, and should be part of it.
https://codereview.chromium.org/1662763002/diff/120001/net/filter/stream_sour...
File net/filter/stream_source.h (right):
https://codereview.chromium.org/1662763002/diff/120001/net/filter/stream_sour...
net/filter/stream_source.h:42: // - Returns an Error other than ERR_IO_PENDING
nit: or OK.
https://codereview.chromium.org/1662763002/diff/120001/net/filter/stream_sour...
net/filter/stream_source.h:44: // - Writes bytes into |*dest_buffer|
nit: Presumably only if it did return OK for both of these.
https://codereview.chromium.org/1662763002/diff/120001/net/filter/stream_sour...
net/filter/stream_source.h:50: // of bytes read, which are them placed into
|*dest_buffer|.
nit, typo: "them" -> "then".
Presuming that's what you meant to type, I think it implies that the bytes are
placed after the callback is called, when I think they should be placed before
the callback is called. Maybe "... and a count of bytes read and written into
|*dest_buffer|."?
https://codereview.chromium.org/1662763002/diff/120001/net/filter/stream_sour...
net/filter/stream_source.h:67: // scoped_refptr<IOBuffer> pending_read_buffer_;
nit: Should these lines be removed?
https://codereview.chromium.org/1662763002/diff/120001/net/filter/stream_sour...
File net/filter/stream_source_util.h (right):
https://codereview.chromium.org/1662763002/diff/120001/net/filter/stream_sour...
net/filter/stream_source_util.h:22: // with |delegate|.
? There is no |delegate| in the argument list (and since it's static, that
means it's not there).
https://codereview.chromium.org/1662763002/diff/120001/net/filter/stream_sour...
net/filter/stream_source_util.h:25: HttpResponseHeaders* headers);
I have some urge to pull this functionality up into URLRequestHttpJob; it's
pretty HTTP specific, so it doesn't strike me as out of place there, and I think
it likely that with that and a few other tweaks we can break the circular
dependency of StreamSource -> all different types of StreamSources ->
StreamSource. But it's only vaguely related to this CL, so feel free to look
into doing it or not as you choose.
https://codereview.chromium.org/1662763002/diff/120001/net/filter/stream_sour...
net/filter/stream_source_util.h:28: static std::string
SourceTypeAsString(StreamSource::SourceType type);
I suspect (haven't traced through the code) that this functionality could be
replaced by a virtual function on StreamSource, which would again be a nice
dependency breakage.
https://codereview.chromium.org/1662763002/diff/120001/net/filter/stream_sour...
net/filter/stream_source_util.h:32: static std::string
OrderedStreamSourceList(StreamSource* source);
And similarly, I'd recommend having a virtual method on StreamSource that
returns a string describing the stream, and a non-virtual method on StreamSource
that concatenates the result of that methods with the result of calling itself
on previous_.
https://codereview.chromium.org/1662763002/diff/120001/net/url_request/url_re...
File net/url_request/url_request_job.cc (right):
https://codereview.chromium.org/1662763002/diff/120001/net/url_request/url_re...
net/url_request/url_request_job.cc:164: *bytes_read = bytes_read_n;
Why not just pass bytes_read to source_->Read()?
https://codereview.chromium.org/1662763002/diff/120001/net/url_request/url_re...
net/url_request/url_request_job.cc:187: void
URLRequestJob::SourceReadComplete(Error error, size_t bytes_read) {
nit, idea (i.e. not even as much force as suggestion): I wonder if it might be
worthwhile to try and combine this logic with the trailing logic in Read()
above. It's doing basically the same thing, which is translating underlying
filter chain information into SetStatus/NotifyDone/NotifyReadCompleted calls.
https://codereview.chromium.org/1662763002/diff/120001/net/url_request/url_re...
net/url_request/url_request_job.cc:512: }
nit: Why the curly braces?
https://codereview.chromium.org/1662763002/diff/120001/net/url_request/url_re...
net/url_request/url_request_job.cc:566: DCHECK(!read_raw_callback_.is_null());
Why only if error == OK? Don't we want to pass the error back up the filter
chain regardless? I mean, the filters probably won't modify the error, but I
sorta feel like they should have the option to do so, and maybe to do some
cleanup too.
https://codereview.chromium.org/1662763002/diff/120001/net/url_request/url_re...
File net/url_request/url_request_job.h (right):
https://codereview.chromium.org/1662763002/diff/120001/net/url_request/url_re...
net/url_request/url_request_job.h:351: class URLRequestJobStreamSource;
I don't see this class referred to in the header file; is there any need to
declare it here?
Plan to do a more complete review Monday. https://codereview.chromium.org/1662763002/diff/120001/net/filter/block_buffer.h File net/filter/block_buffer.h (right): https://codereview.chromium.org/1662763002/diff/120001/net/filter/block_buffe... net/filter/block_buffer.h:18: class BlockBuffer { Can we just use DrainableIOBuffer instead? https://codereview.chromium.org/1662763002/diff/120001/net/filter/filter_stre... File net/filter/filter_stream_source.cc (right): https://codereview.chromium.org/1662763002/diff/120001/net/filter/filter_stre... net/filter/filter_stream_source.cc:39: while (error == OK) { Hrm...Wonder if a do loop approach would be a little clearer. The API to talk to the subclass is pretty much the same, but the transitions may be a little clearer. What do you two think? Read() { DCHECK_EQ(NONE, next_state_); // Start with the filter. It tells us it needs data if it doesn't have any yet. next_state_ = FILTER_DATA; return DoLoop(OK); } DoLoop(result) { ... switch(state){ case READ_DATA: result = DoReadData(); break; case READ_DATA_COMPLETE: result = DoReadDataComplete(); break; case FILTER_DATA: result = DoFilterData(); break; case FILTER_DATA_COMPLETE: result = DoFilterDataComplete(); break; } .. } int DoReadData() { next_state_ = READ_DATA_COMPLETE; return previous_->Read(); } int DoReadDataComplete(int result) { // No more data left. May have to check with the filter if it has to have more data, // or may always be an error, not sure. if (result == OK) return ... // error (If we go an ERR_IO_PENDING, we don't if (result < 0) return result; AddPrefilterData(read_buffer_, result); next_state_ = FILTER_DATA; return result; } // |result| is number of bytes read, if previous state was READ_DATA_COMPLETE, OK if // previous state was FILTER_DATA_COMPLETE (In which case we're coming directly from a Read call). int DoFilterData(int result) { next_state_ = FILTER_DATA_COMPLETE; // Pass entire buffer to subclass. Make read_buffer a DrainableIOBuffer, so subclass can call its consume method. // Call even when read_buffer is empty, before going back to read more data, to make sure we need more data, and // that there's no left over decoded bytes to get. return FilterData(read_buffer.get(), write_buffer_, write_buffer_length_); } int DoFilterDataComplete(int result) { if (result == OK) { // Done filtering or needs more data. Need a method to distinguish the two. if (NeedsMoreData()) next_state_ = READ_DATA; else next_state_ = DONE; return OK; } // Failed. if (result < 0) { next_state_ = DONE; return result; } // next_state_ defaults to NONE. return result; } https://codereview.chromium.org/1662763002/diff/120001/net/filter/filter_stre... net/filter/filter_stream_source.cc:57: DCHECK(!buffer_ || !buffer_->HasMoreBytes()); nit: I'd say it's not worth DCHECKing on buffer_ here, or below. The second value in the DCHECK will crash is buffer_ is null, anyways. https://codereview.chromium.org/1662763002/diff/120001/net/filter/filter_stre... File net/filter/filter_stream_source.h (right): https://codereview.chromium.org/1662763002/diff/120001/net/filter/filter_stre... net/filter/filter_stream_source.h:19: FilterStreamSource(SourceType type, std::unique_ptr<StreamSource> previous); Hrm...Wonder about "next" vs "previous". We could say "data flows from the previous one to the next one", which is the name scheme you're using. Or we could say "the read command comes in the previous one, and is passed to the next one", which is the actual way the logic works. Maybe there's a clearer name? I dunno. https://codereview.chromium.org/1662763002/diff/120001/net/filter/filter_stre... net/filter/filter_stream_source.h:47: size_t* bytes_read) = 0; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/1662763002/diff/120001/net/filter/stream_sour... File net/filter/stream_source.h (right): https://codereview.chromium.org/1662763002/diff/120001/net/filter/stream_sour... net/filter/stream_source.h:62: protected: protected fields aren't allowed. https://codereview.chromium.org/1662763002/diff/120001/net/filter/stream_sour... net/filter/stream_source.h:72: private: DISALLOW_COPY_AND_ASSIGN
Patchset #1 (id:40001) has been deleted
Description was changed from ========== Implement pull-based design for net::Filter This CL implements a pull-based design for net::Filter. The design is proposed in crbug.com/474858, and based on an earlier prototype CL (crrev.com/1274863002 by ellyjones@). BUG=474859 ========== to ========== Implement pull-based design for net::Filter This CL implements a pull-based design for net::Filter. This CL is based on an earlier prototype CL (crrev.com/1274863002 by ellyjones@). The design for this pull-based net::Filter is proposed in crbug.com/474858. BUG=474859 ==========
Sorry, completely forgot about this. I'll do another round today or tomorrow.
On 2016/06/15 16:26:42, mmenke wrote: > Sorry, completely forgot about this. I'll do another round today or tomorrow. (And for the record, Helen suggested I hold off until she uploads her next patch set)
Patchset #6 (id:180001) has been deleted
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
Patchset #8 (id:240001) has been deleted
Thank you both very much for the review. I believe I have addressed all of them. I've put "Acknowledged" for comments that no longer apply. net_unittests are passing, and I have tested this manually. SDCH is working as expected for google and amazon pages. I will work on adding more test cases in the meanwhile. https://codereview.chromium.org/1662763002/diff/120001/net/filter/block_buffer.h File net/filter/block_buffer.h (right): https://codereview.chromium.org/1662763002/diff/120001/net/filter/block_buffe... net/filter/block_buffer.h:18: class BlockBuffer { On 2016/04/29 19:10:54, mmenke wrote: > Can we just use DrainableIOBuffer instead? Done. https://codereview.chromium.org/1662763002/diff/120001/net/filter/block_buffe... net/filter/block_buffer.h:23: bool HasMoreBytes() const; On 2016/04/26 21:54:01, Randy Smith - Not in Fridays wrote: > Personal preference, therefore suggestion: I find .h files easier to read when, > if there are comments describing methods, there's a single blank line between > the methods, so it's easy to visually associate the comments with the method. Acknowledged. I got rid of this class per Matt's suggestion on using DrainableIOBuffer directly. https://codereview.chromium.org/1662763002/diff/120001/net/filter/block_buffe... net/filter/block_buffer.h:24: // Marks that the buffer has been refilled |filled_bytes| bytes. It is illegal On 2016/04/26 21:54:01, Randy Smith - Not in Fridays wrote: > nit, suggestion: I think this would be clearer with something like "Marks that > |filled_bytes| bytes has been written into the buffer starting at location > buffer(). This resets the bytes() pointer to buffer(). It is illegal to refill > the buffer when |bytes_left() != 0|. Acknowledged. https://codereview.chromium.org/1662763002/diff/120001/net/filter/block_buffe... net/filter/block_buffer.h:35: // Returns the pointer to the next byte that may be read or drained. On 2016/04/26 21:54:01, Randy Smith - Not in Fridays wrote: > nit, suggestion: I'd leave out the "or drained"; in fact, I'd suggest rewriting > these descriptions to not refer to draining bytes, but instead refer to having > read them and thus updating the bytes() pointer. Acknowledged. https://codereview.chromium.org/1662763002/diff/120001/net/filter/block_buffe... net/filter/block_buffer.h:37: // Returns the number of bytes that are remaining. On 2016/04/26 21:54:01, Randy Smith - Not in Fridays wrote: > nit: I'd use the same language as above in the comment to WasDrained; i.e. use > "remaining" there or "available" here. (I think the words mean the same thing > in this context.) Acknowledged. https://codereview.chromium.org/1662763002/diff/120001/net/filter/block_buffe... net/filter/block_buffer.h:45: }; On 2016/04/26 21:54:01, Randy Smith - Not in Fridays wrote: > Should there be a DISALLOW_COPY_AND_ASSIGN? Acknowledged. https://codereview.chromium.org/1662763002/diff/120001/net/filter/filter_stre... File net/filter/filter_stream_source.cc (right): https://codereview.chromium.org/1662763002/diff/120001/net/filter/filter_stre... net/filter/filter_stream_source.cc:39: while (error == OK) { On 2016/04/29 19:10:54, mmenke wrote: > Hrm...Wonder if a do loop approach would be a little clearer. The API to talk > to the subclass is pretty much the same, but the transitions may be a little > clearer. What do you two think? > > Read() { > DCHECK_EQ(NONE, next_state_); > > // Start with the filter. It tells us it needs data if it doesn't have any > yet. > next_state_ = FILTER_DATA; > > return DoLoop(OK); > } > > DoLoop(result) { > ... > switch(state){ > case READ_DATA: > result = DoReadData(); > break; > case READ_DATA_COMPLETE: > result = DoReadDataComplete(); > break; > case FILTER_DATA: > result = DoFilterData(); > break; > case FILTER_DATA_COMPLETE: > result = DoFilterDataComplete(); > break; > } > .. > } > > int DoReadData() { > next_state_ = READ_DATA_COMPLETE; > return previous_->Read(); > } > > int DoReadDataComplete(int result) { > // No more data left. May have to check with the filter if it has to have more > data, > // or may always be an error, not sure. > if (result == OK) > return ... > > // error (If we go an ERR_IO_PENDING, we don't > if (result < 0) > return result; > > AddPrefilterData(read_buffer_, result); > next_state_ = FILTER_DATA; > return result; > } > > // |result| is number of bytes read, if previous state was READ_DATA_COMPLETE, > OK if > // previous state was FILTER_DATA_COMPLETE (In which case we're coming directly > from a Read call). > int DoFilterData(int result) { > next_state_ = FILTER_DATA_COMPLETE; > // Pass entire buffer to subclass. Make read_buffer a DrainableIOBuffer, so > subclass can call its consume method. > // Call even when read_buffer is empty, before going back to read more data, > to make sure we need more data, and > // that there's no left over decoded bytes to get. > return FilterData(read_buffer.get(), write_buffer_, write_buffer_length_); > } > > int DoFilterDataComplete(int result) { > if (result == OK) { > // Done filtering or needs more data. Need a method to distinguish the two. > if (NeedsMoreData()) > next_state_ = READ_DATA; > else > next_state_ = DONE; > return OK; > } > > // Failed. > if (result < 0) { > next_state_ = DONE; > return result; > } > > // next_state_ defaults to NONE. > > return result; > } Done. https://codereview.chromium.org/1662763002/diff/120001/net/filter/filter_stre... net/filter/filter_stream_source.cc:53: if ((error == OK && single_bytes_read > 0) || error != OK) On 2016/04/26 21:54:01, Randy Smith - Not in Fridays wrote: > I believe what this means is that this function will only change *bytes_read > once, because if single_bytes_read is non-zero, the function exits here. If > that's accurate, I'd suggest getting rid of single_bytes_read and just passing > bytes_read into ReadInternal() for simplicity (one less variable, and one that I > experience as having a confusing name). Done. https://codereview.chromium.org/1662763002/diff/120001/net/filter/filter_stre... net/filter/filter_stream_source.cc:57: DCHECK(!buffer_ || !buffer_->HasMoreBytes()); On 2016/04/29 19:10:54, mmenke wrote: > nit: I'd say it's not worth DCHECKing on buffer_ here, or below. The second > value in the DCHECK will crash is buffer_ is null, anyways. Done. https://codereview.chromium.org/1662763002/diff/120001/net/filter/filter_stre... File net/filter/filter_stream_source.h (right): https://codereview.chromium.org/1662763002/diff/120001/net/filter/filter_stre... net/filter/filter_stream_source.h:17: class FilterStreamSource : public StreamSource { On 2016/04/26 21:54:01, Randy Smith - Not in Fridays wrote: > nit: comment as to use. Done. https://codereview.chromium.org/1662763002/diff/120001/net/filter/filter_stre... net/filter/filter_stream_source.h:19: FilterStreamSource(SourceType type, std::unique_ptr<StreamSource> previous); On 2016/04/29 19:10:54, mmenke wrote: > Hrm...Wonder about "next" vs "previous". > > We could say "data flows from the previous one to the next one", which is the > name scheme you're using. > > Or we could say "the read command comes in the previous one, and is passed to > the next one", which is the actual way the logic works. > > Maybe there's a clearer name? I dunno. Done. I have added a comment. I think |previous| is reasonable in the sense that this is a pull-based design where data is requested from the previous source in the filter chain. The read command actually comes from the next source in the filter chain. https://codereview.chromium.org/1662763002/diff/120001/net/filter/filter_stre... net/filter/filter_stream_source.h:45: virtual net::Error ReadInternal(IOBuffer* dest_buffer, On 2016/04/26 21:54:01, Randy Smith - Not in Fridays wrote: > I think this would be cleaner if it took an input argument (buffer_, presumably) > so that buffer_ didn't need to be protected and used by all the derived classes. > (Possibly buffer_ should be declared here, but I haven't looked at the > non-filter StreamSource yet, so I'm not sure.) Done. https://codereview.chromium.org/1662763002/diff/120001/net/filter/filter_stre... net/filter/filter_stream_source.h:47: size_t* bytes_read) = 0; On 2016/04/29 19:10:54, mmenke wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/1662763002/diff/120001/net/filter/sdch_stream... File net/filter/sdch_stream_source_delegate.h (right): https://codereview.chromium.org/1662763002/diff/120001/net/filter/sdch_stream... net/filter/sdch_stream_source_delegate.h:18: class SdchStreamSourceDelegate { On 2016/04/26 21:54:01, Randy Smith - Not in Fridays wrote: > Suggestion: My inclination would be to define this as SdchStreamSource::Delegate > in the sdch_stream_source.h file. I think it's bound tightly with that > interface, and should be part of it. Done. https://codereview.chromium.org/1662763002/diff/120001/net/filter/stream_sour... File net/filter/stream_source.h (right): https://codereview.chromium.org/1662763002/diff/120001/net/filter/stream_sour... net/filter/stream_source.h:42: // - Returns an Error other than ERR_IO_PENDING On 2016/04/26 21:54:01, Randy Smith - Not in Fridays wrote: > nit: or OK. Done. https://codereview.chromium.org/1662763002/diff/120001/net/filter/stream_sour... net/filter/stream_source.h:44: // - Writes bytes into |*dest_buffer| On 2016/04/26 21:54:01, Randy Smith - Not in Fridays wrote: > nit: Presumably only if it did return OK for both of these. Done. https://codereview.chromium.org/1662763002/diff/120001/net/filter/stream_sour... net/filter/stream_source.h:50: // of bytes read, which are them placed into |*dest_buffer|. On 2016/04/26 21:54:01, Randy Smith - Not in Fridays wrote: > nit, typo: "them" -> "then". > > Presuming that's what you meant to type, I think it implies that the bytes are > placed after the callback is called, when I think they should be placed before > the callback is called. Maybe "... and a count of bytes read and written into > |*dest_buffer|."? Done. https://codereview.chromium.org/1662763002/diff/120001/net/filter/stream_sour... net/filter/stream_source.h:62: protected: On 2016/04/29 19:10:54, mmenke wrote: > protected fields aren't allowed. Done. https://codereview.chromium.org/1662763002/diff/120001/net/filter/stream_sour... net/filter/stream_source.h:67: // scoped_refptr<IOBuffer> pending_read_buffer_; On 2016/04/26 21:54:01, Randy Smith - Not in Fridays wrote: > nit: Should these lines be removed? Done. https://codereview.chromium.org/1662763002/diff/120001/net/filter/stream_sour... net/filter/stream_source.h:72: private: On 2016/04/29 19:10:54, mmenke wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/1662763002/diff/120001/net/filter/stream_sour... File net/filter/stream_source_util.h (right): https://codereview.chromium.org/1662763002/diff/120001/net/filter/stream_sour... net/filter/stream_source_util.h:22: // with |delegate|. On 2016/04/26 21:54:02, Randy Smith - Not in Fridays wrote: > ? There is no |delegate| in the argument list (and since it's static, that > means it's not there). Done. https://codereview.chromium.org/1662763002/diff/120001/net/filter/stream_sour... net/filter/stream_source_util.h:25: HttpResponseHeaders* headers); On 2016/04/26 21:54:01, Randy Smith - Not in Fridays wrote: > I have some urge to pull this functionality up into URLRequestHttpJob; it's > pretty HTTP specific, so it doesn't strike me as out of place there, and I think > it likely that with that and a few other tweaks we can break the circular > dependency of StreamSource -> all different types of StreamSources -> > StreamSource. But it's only vaguely related to this CL, so feel free to look > into doing it or not as you choose. Done. https://codereview.chromium.org/1662763002/diff/120001/net/filter/stream_sour... net/filter/stream_source_util.h:28: static std::string SourceTypeAsString(StreamSource::SourceType type); On 2016/04/26 21:54:02, Randy Smith - Not in Fridays wrote: > I suspect (haven't traced through the code) that this functionality could be > replaced by a virtual function on StreamSource, which would again be a nice > dependency breakage. Done. https://codereview.chromium.org/1662763002/diff/120001/net/filter/stream_sour... net/filter/stream_source_util.h:32: static std::string OrderedStreamSourceList(StreamSource* source); On 2016/04/26 21:54:02, Randy Smith - Not in Fridays wrote: > And similarly, I'd recommend having a virtual method on StreamSource that > returns a string describing the stream, and a non-virtual method on StreamSource > that concatenates the result of that methods with the result of calling itself > on previous_. Done. https://codereview.chromium.org/1662763002/diff/120001/net/url_request/url_re... File net/url_request/url_request_job.cc (right): https://codereview.chromium.org/1662763002/diff/120001/net/url_request/url_re... net/url_request/url_request_job.cc:164: *bytes_read = bytes_read_n; On 2016/04/26 21:54:02, Randy Smith - Not in Fridays wrote: > Why not just pass bytes_read to source_->Read()? Done. https://codereview.chromium.org/1662763002/diff/120001/net/url_request/url_re... net/url_request/url_request_job.cc:187: void URLRequestJob::SourceReadComplete(Error error, size_t bytes_read) { On 2016/04/26 21:54:02, Randy Smith - Not in Fridays wrote: > nit, idea (i.e. not even as much force as suggestion): I wonder if it might be > worthwhile to try and combine this logic with the trailing logic in Read() > above. It's doing basically the same thing, which is translating underlying > filter chain information into SetStatus/NotifyDone/NotifyReadCompleted calls. Done. https://codereview.chromium.org/1662763002/diff/120001/net/url_request/url_re... net/url_request/url_request_job.cc:512: } On 2016/04/26 21:54:02, Randy Smith - Not in Fridays wrote: > nit: Why the curly braces? Done. https://codereview.chromium.org/1662763002/diff/120001/net/url_request/url_re... net/url_request/url_request_job.cc:566: DCHECK(!read_raw_callback_.is_null()); On 2016/04/26 21:54:02, Randy Smith - Not in Fridays wrote: > Why only if error == OK? Don't we want to pass the error back up the filter > chain regardless? I mean, the filters probably won't modify the error, but I > sorta feel like they should have the option to do so, and maybe to do some > cleanup too. Done. https://codereview.chromium.org/1662763002/diff/120001/net/url_request/url_re... File net/url_request/url_request_job.h (right): https://codereview.chromium.org/1662763002/diff/120001/net/url_request/url_re... net/url_request/url_request_job.h:351: class URLRequestJobStreamSource; On 2016/04/26 21:54:02, Randy Smith - Not in Fridays wrote: > I don't see this class referred to in the header file; is there any need to > declare it here? The declaration here is needed because this class needs to access the private method (ReadRawDataHelper) of the enclosing class.
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
xunjieli@chromium.org changed reviewers: + eustas@chromium.org
Patchset #13 (id:360001) has been deleted
Patchset #7 (id:220001) has been deleted
+eustas@: PTAL at Brotli-related code. If you have any suggestions on other parts of the code, I am more than happy to address them too. Thanks!
Patchset #14 (id:420001) has been deleted
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
A bunch of nits, strewn across your last two CLs (oops!). I'm planning to carefully review URLRequest[Http]Job, and the [Filter]StreamSource code, and leave the reviews of the individual filters to Randy. https://codereview.chromium.org/1662763002/diff/400001/net/filter/brotli_stre... File net/filter/brotli_stream_source.cc (right): https://codereview.chromium.org/1662763002/diff/400001/net/filter/brotli_stre... net/filter/brotli_stream_source.cc:20: class BrotliStreamSource : public FilterStreamSource { This should be in an anonymous namespace. https://codereview.chromium.org/1662763002/diff/400001/net/filter/brotli_stre... net/filter/brotli_stream_source.cc:22: BrotliStreamSource(std::unique_ptr<StreamSource> previous) explicit https://codereview.chromium.org/1662763002/diff/400001/net/filter/brotli_stre... net/filter/brotli_stream_source.cc:30: CHECK(brotli_state_); include base/logging.h https://codereview.chromium.org/1662763002/diff/400001/net/filter/brotli_stre... net/filter/brotli_stream_source.cc:36: DCHECK(used_memory_ == 0); DCHECK_EQ https://codereview.chromium.org/1662763002/diff/400001/net/filter/brotli_stre... net/filter/brotli_stream_source.cc:52: static const int64_t kMaxKb = 1 << (kBuckets / 3); // 64MiB in KiB Think it's preferred not to use static on local constants. (I assume you're copying old code here, but may as wlel remove it now) https://codereview.chromium.org/1662763002/diff/400001/net/filter/brotli_stre... File net/filter/brotli_stream_source.h (right): https://codereview.chromium.org/1662763002/diff/400001/net/filter/brotli_stre... net/filter/brotli_stream_source.h:6: #define NET_FILTER_BROTLI_STREAM_SOURCE_H__ Style is just to use one underscore in header guards, isn't it? https://codereview.chromium.org/1662763002/diff/400001/net/filter/brotli_stre... net/filter/brotli_stream_source.h:10: #include "base/macros.h" Not used here. Move to the cc file? https://codereview.chromium.org/1662763002/diff/400001/net/filter/brotli_stre... net/filter/brotli_stream_source.h:14: #include "third_party/brotli/dec/decode.h" Not used here, move to the cc file? https://codereview.chromium.org/1662763002/diff/400001/net/filter/filter_stre... File net/filter/filter_stream_source.cc (right): https://codereview.chromium.org/1662763002/diff/400001/net/filter/filter_stre... net/filter/filter_stream_source.cc:56: return previous_->OrderedTypeStringList() + "," + GetTypeAsString(); So will this be something like "NONE,GZIP" or "NONE,GZIP,SDCH"? Can we get rid of the NONE? https://codereview.chromium.org/1662763002/diff/400001/net/filter/filter_stre... File net/filter/filter_stream_source.h (right): https://codereview.chromium.org/1662763002/diff/400001/net/filter/filter_stre... net/filter/filter_stream_source.h:6: #define NET_FILTER_FILTER_STREAM_SOURCE_H +_ (x3) https://codereview.chromium.org/1662763002/diff/400001/net/filter/gzip_stream... File net/filter/gzip_stream_source.h (right): https://codereview.chromium.org/1662763002/diff/400001/net/filter/gzip_stream... net/filter/gzip_stream_source.h:30: GzipStreamSourceMode mode); nit: explicit not needed. https://codereview.chromium.org/1662763002/diff/400001/net/filter/gzip_stream... net/filter/gzip_stream_source.h:33: bool Init() override; FilterStreamSource implementation. https://codereview.chromium.org/1662763002/diff/400001/net/filter/gzip_stream... net/filter/gzip_stream_source.h:33: bool Init() override; Randy (And you) may disagree, but I think requiring a constructor call and then an init call to get something into a usable state is a bit ugly. It means you have an extra state - created, but uninitialized, which you expose to the world. Sometimes, that's needed (Like if init has to happen asynchronously, on another thread). But in general, if not needed, I think making the constructor and Init private, and having a unique_ptr<FilterSource> Create() method is preferable. Don't feel too strongly about it, though, so feel free to keep as is. Also fine to say "Meh, this CL has gone on long enough, maybe later". https://codereview.chromium.org/1662763002/diff/400001/net/filter/gzip_stream... net/filter/gzip_stream_source.h:36: // StreamSource implementation FilterStreamSource, actually. https://codereview.chromium.org/1662763002/diff/400001/net/filter/gzip_stream... net/filter/gzip_stream_source.h:77: scoped_refptr<IOBuffer> pending_read_buffer_; include base/memory/ref_counted.h, net/base/io_buffer.h https://codereview.chromium.org/1662763002/diff/400001/net/filter/gzip_stream... net/filter/gzip_stream_source.h:97: size_t gzip_footer_bytes_left_; DISALLOW_COPY_AND_ASSIGN / include macros.h https://codereview.chromium.org/1662763002/diff/400001/net/filter/mock_stream... File net/filter/mock_stream_source.h (right): https://codereview.chromium.org/1662763002/diff/400001/net/filter/mock_stream... net/filter/mock_stream_source.h:6: #define NET_FILTER_MOCK_STREAM_SOURCE_H +_ (x3) https://codereview.chromium.org/1662763002/diff/400001/net/filter/mock_stream... net/filter/mock_stream_source.h:17: class MockStreamSource : public StreamSource { Think this needs a description https://codereview.chromium.org/1662763002/diff/400001/net/filter/mock_stream... net/filter/mock_stream_source.h:32: void AddReadResult(const char* data, size_t len, Error error, bool sync); include net/base/net_errors.h https://codereview.chromium.org/1662763002/diff/400001/net/filter/mock_stream... net/filter/mock_stream_source.h:39: QueuedResult(const char* data, size_t len, Error error, bool sync); nit: Add blank line before member variables. https://codereview.chromium.org/1662763002/diff/400001/net/filter/mock_stream... net/filter/mock_stream_source.h:43: bool sync; Can these all be constant? Fine if not. Could even use std::queue<const QueuedResult> results_; instead, if that works. https://codereview.chromium.org/1662763002/diff/400001/net/filter/mock_stream... net/filter/mock_stream_source.h:48: IOBuffer* dest_buffer_; scoped_refptr? (+include header for it) https://codereview.chromium.org/1662763002/diff/400001/net/filter/sdch_policy... File net/filter/sdch_policy_delegate.h (right): https://codereview.chromium.org/1662763002/diff/400001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.h:6: #define NET_FILTER_SDCH_POLICY_DELEGATE_H +_ (x3) https://codereview.chromium.org/1662763002/diff/400001/net/filter/sdch_stream... File net/filter/sdch_stream_source.h (right): https://codereview.chromium.org/1662763002/diff/400001/net/filter/sdch_stream... net/filter/sdch_stream_source.h:6: #define NET_FILTER_SDCH_STREAM_SOURCE_H +_ (x3) https://codereview.chromium.org/1662763002/diff/400001/net/filter/stream_sour... File net/filter/stream_source.h (right): https://codereview.chromium.org/1662763002/diff/400001/net/filter/stream_sour... net/filter/stream_source.h:6: #define NET_FILTER_STREAM_SOURCE_H +_ (x3) https://codereview.chromium.org/1662763002/diff/400001/net/filter/stream_sour... net/filter/stream_source.h:34: StreamSource(SourceType type); explicit https://codereview.chromium.org/1662763002/diff/400001/net/filter/stream_sour... net/filter/stream_source.h:56: virtual std::string OrderedTypeStringList() const; include <string> https://codereview.chromium.org/1662763002/diff/400001/net/url_request/url_re... File net/url_request/url_request_file_job.cc (right): https://codereview.chromium.org/1662763002/diff/400001/net/url_request/url_re... net/url_request/url_request_file_job.cc:25: #include "base/memory/ptr_util.h" What's this being used for? https://codereview.chromium.org/1662763002/diff/440001/net/url_request/url_re... File net/url_request/url_request_http_job.cc (right): https://codereview.chromium.org/1662763002/diff/440001/net/url_request/url_re... net/url_request/url_request_http_job.cc:1229: for (std::vector<StreamSource::SourceType>::reverse_iterator riter = r_iter? riter seems too much a misspelling of writer. https://codereview.chromium.org/1662763002/diff/440001/net/url_request/url_re... File net/url_request/url_request_job.cc (right): https://codereview.chromium.org/1662763002/diff/440001/net/url_request/url_re... net/url_request/url_request_job.cc:124: URLRequestJobStreamSource(URLRequestJob* job) explicit https://codereview.chromium.org/1662763002/diff/440001/net/url_request/url_re... net/url_request/url_request_job.cc:136: // the filter chain. Is this strictly needed? If callback_ is bound to a method on the higher layer StreamSource, |this| can only be deleted if that higher layer StreamSource was deleted, and it should presumably be using a weak ptr. Otherwise, it's owned by the URLRequestJob itself, which also uses a weak ptr (And never deletes its StreamSource, anyways)... Am I missing something? May still be worth doing this, just want to know if I'm missing something. https://codereview.chromium.org/1662763002/diff/440001/net/url_request/url_re... net/url_request/url_request_job.cc:154: base::WeakPtrFactory<URLRequestJobStreamSource> weak_factory_; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/1662763002/diff/440001/net/url_request/url_re... net/url_request/url_request_job.cc:301: return std::move(source); std::move not needed. Should probably just be: return base::MakeUnique<URLRequestJobStreamSource>(this);, and include ptr_util.h. https://codereview.chromium.org/1662763002/diff/440001/net/url_request/url_re... net/url_request/url_request_job.cc:553: if (source_->type() == StreamSource::TYPE_NONE) { SetupSource() can return nullptr on failure. Need to support that (And have tests?) https://codereview.chromium.org/1662763002/diff/440001/net/url_request/url_re... File net/url_request/url_request_job.h (right): https://codereview.chromium.org/1662763002/diff/440001/net/url_request/url_re... net/url_request/url_request_job.h:51: typedef base::Callback<void(net::Error, size_t)> ReadRawCompleteCallback; Is this even used? If not, remove it. If so, need to include Callback header, should have a blank line after it, need to remove net::, and should include net_errors.h. https://codereview.chromium.org/1662763002/diff/440001/net/url_request/url_re... net/url_request/url_request_job.h:150: // or nullptr if no StreamSource should be used. This class takes ownership of "... or nullptr on error"? https://codereview.chromium.org/1662763002/diff/440001/net/url_request/url_re... net/url_request/url_request_job.h:152: virtual std::unique_ptr<StreamSource> SetupSource(); This should be private, right? It's an old comment, but should also remove "This class takes ownership of..." https://codereview.chromium.org/1662763002/diff/440001/net/url_request/url_re... net/url_request/url_request_job.h:431: // None null if ReadRawData() returned ERR_IO_PENDING, and the read has not Non-null if...
Helen: I'm feeling snowed by some high priority reviews and personal stuff, and am about to head out on vacation for a week. Given how long this CL has been in progress :-}, how much will it mess you up if I put off this review until I get back?
On 2016/07/21 18:19:52, Randy Smith - Not in Fridays wrote: > Helen: I'm feeling snowed by some high priority reviews and personal stuff, and > am about to head out on vacation for a week. Given how long this CL has been in > progress :-}, how much will it mess you up if I put off this review until I get > back? Please take your time. It will take me a while to address Matt's comments anyway, and a week is very short :) I am not in a hurry. I think I need to write a couple more unit tests. If I can get this landed before this year end, I will be very happy.
https://codereview.chromium.org/1662763002/diff/440001/net/docs/filter.md File net/docs/filter.md (right): https://codereview.chromium.org/1662763002/diff/440001/net/docs/filter.md#new... net/docs/filter.md:7: * brotili (handling "br" Content-Encoding) s/brotili/brotli https://codereview.chromium.org/1662763002/diff/440001/net/filter/brotli_stre... File net/filter/brotli_stream_source.cc (right): https://codereview.chromium.org/1662763002/diff/440001/net/filter/brotli_stre... net/filter/brotli_stream_source.cc:39: "BrotliFilter.Status", static_cast<int>(decoding_status_), This file seems to be based on old version of brotli_filter (some UMA's are missing)
Patchset #15 (id:460001) has been deleted
Patchset #12 (id:380001) has been deleted
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Patchset #15 (id:500001) has been deleted
Patchset #14 (id:480001) has been deleted
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Patchset #14 (id:520001) has been deleted
Thanks a lot for the comments mmenke@ and eustas@! I have uploaded a new patch. PTAL. https://codereview.chromium.org/1662763002/diff/400001/net/filter/brotli_stre... File net/filter/brotli_stream_source.cc (right): https://codereview.chromium.org/1662763002/diff/400001/net/filter/brotli_stre... net/filter/brotli_stream_source.cc:20: class BrotliStreamSource : public FilterStreamSource { On 2016/07/21 18:14:07, mmenke wrote: > This should be in an anonymous namespace. Done. https://codereview.chromium.org/1662763002/diff/400001/net/filter/brotli_stre... net/filter/brotli_stream_source.cc:22: BrotliStreamSource(std::unique_ptr<StreamSource> previous) On 2016/07/21 18:14:07, mmenke wrote: > explicit Done. https://codereview.chromium.org/1662763002/diff/400001/net/filter/brotli_stre... net/filter/brotli_stream_source.cc:30: CHECK(brotli_state_); On 2016/07/21 18:14:07, mmenke wrote: > include base/logging.h Done. https://codereview.chromium.org/1662763002/diff/400001/net/filter/brotli_stre... net/filter/brotli_stream_source.cc:36: DCHECK(used_memory_ == 0); On 2016/07/21 18:14:08, mmenke wrote: > DCHECK_EQ Done. https://codereview.chromium.org/1662763002/diff/400001/net/filter/brotli_stre... net/filter/brotli_stream_source.cc:52: static const int64_t kMaxKb = 1 << (kBuckets / 3); // 64MiB in KiB On 2016/07/21 18:14:07, mmenke wrote: > Think it's preferred not to use static on local constants. (I assume you're > copying old code here, but may as wlel remove it now) Done. https://codereview.chromium.org/1662763002/diff/400001/net/filter/brotli_stre... File net/filter/brotli_stream_source.h (right): https://codereview.chromium.org/1662763002/diff/400001/net/filter/brotli_stre... net/filter/brotli_stream_source.h:6: #define NET_FILTER_BROTLI_STREAM_SOURCE_H__ On 2016/07/21 18:14:08, mmenke wrote: > Style is just to use one underscore in header guards, isn't it? Done. Thanks for catching it. Probably a remnant of incorrect find-and-replace. https://codereview.chromium.org/1662763002/diff/400001/net/filter/brotli_stre... net/filter/brotli_stream_source.h:10: #include "base/macros.h" On 2016/07/21 18:14:08, mmenke wrote: > Not used here. Move to the cc file? Done. https://codereview.chromium.org/1662763002/diff/400001/net/filter/brotli_stre... net/filter/brotli_stream_source.h:14: #include "third_party/brotli/dec/decode.h" On 2016/07/21 18:14:08, mmenke wrote: > Not used here, move to the cc file? Done. https://codereview.chromium.org/1662763002/diff/400001/net/filter/filter_stre... File net/filter/filter_stream_source.cc (right): https://codereview.chromium.org/1662763002/diff/400001/net/filter/filter_stre... net/filter/filter_stream_source.cc:56: return previous_->OrderedTypeStringList() + "," + GetTypeAsString(); On 2016/07/21 18:14:08, mmenke wrote: > So will this be something like "NONE,GZIP" or "NONE,GZIP,SDCH"? Can we get rid > of the NONE? Done. https://codereview.chromium.org/1662763002/diff/400001/net/filter/filter_stre... File net/filter/filter_stream_source.h (right): https://codereview.chromium.org/1662763002/diff/400001/net/filter/filter_stre... net/filter/filter_stream_source.h:6: #define NET_FILTER_FILTER_STREAM_SOURCE_H On 2016/07/21 18:14:08, mmenke wrote: > +_ (x3) Done. https://codereview.chromium.org/1662763002/diff/400001/net/filter/gzip_stream... File net/filter/gzip_stream_source.h (right): https://codereview.chromium.org/1662763002/diff/400001/net/filter/gzip_stream... net/filter/gzip_stream_source.h:30: GzipStreamSourceMode mode); On 2016/07/21 18:14:08, mmenke wrote: > nit: explicit not needed. Done. https://codereview.chromium.org/1662763002/diff/400001/net/filter/gzip_stream... net/filter/gzip_stream_source.h:33: bool Init() override; On 2016/07/21 18:14:08, mmenke wrote: > FilterStreamSource implementation. Done. https://codereview.chromium.org/1662763002/diff/400001/net/filter/gzip_stream... net/filter/gzip_stream_source.h:33: bool Init() override; On 2016/07/21 18:14:08, mmenke wrote: > Randy (And you) may disagree, but I think requiring a constructor call and then > an init call to get something into a usable state is a bit ugly. It means you > have an extra state - created, but uninitialized, which you expose to the world. > Sometimes, that's needed (Like if init has to happen asynchronously, on another > thread). > > But in general, if not needed, I think making the constructor and Init private, > and having a unique_ptr<FilterSource> Create() method is preferable. Don't feel > too strongly about it, though, so feel free to keep as is. Also fine to say > "Meh, this CL has gone on long enough, maybe later". Done. Thanks for the excellent suggestion! I couldn't agree more that having an extra state is confusing. I forgot a few times to call Init() in tests, which resulted in runtime crashes. https://codereview.chromium.org/1662763002/diff/400001/net/filter/gzip_stream... net/filter/gzip_stream_source.h:36: // StreamSource implementation On 2016/07/21 18:14:08, mmenke wrote: > FilterStreamSource, actually. Done. https://codereview.chromium.org/1662763002/diff/400001/net/filter/gzip_stream... net/filter/gzip_stream_source.h:77: scoped_refptr<IOBuffer> pending_read_buffer_; On 2016/07/21 18:14:08, mmenke wrote: > include base/memory/ref_counted.h, net/base/io_buffer.h Done. https://codereview.chromium.org/1662763002/diff/400001/net/filter/gzip_stream... net/filter/gzip_stream_source.h:97: size_t gzip_footer_bytes_left_; On 2016/07/21 18:14:08, mmenke wrote: > DISALLOW_COPY_AND_ASSIGN / include macros.h Done. https://codereview.chromium.org/1662763002/diff/400001/net/filter/mock_stream... File net/filter/mock_stream_source.h (right): https://codereview.chromium.org/1662763002/diff/400001/net/filter/mock_stream... net/filter/mock_stream_source.h:6: #define NET_FILTER_MOCK_STREAM_SOURCE_H On 2016/07/21 18:14:08, mmenke wrote: > +_ (x3) Done. https://codereview.chromium.org/1662763002/diff/400001/net/filter/mock_stream... net/filter/mock_stream_source.h:17: class MockStreamSource : public StreamSource { On 2016/07/21 18:14:08, mmenke wrote: > Think this needs a description Done. https://codereview.chromium.org/1662763002/diff/400001/net/filter/mock_stream... net/filter/mock_stream_source.h:32: void AddReadResult(const char* data, size_t len, Error error, bool sync); On 2016/07/21 18:14:09, mmenke wrote: > include net/base/net_errors.h Done. https://codereview.chromium.org/1662763002/diff/400001/net/filter/mock_stream... net/filter/mock_stream_source.h:39: QueuedResult(const char* data, size_t len, Error error, bool sync); On 2016/07/21 18:14:09, mmenke wrote: > nit: Add blank line before member variables. Done. https://codereview.chromium.org/1662763002/diff/400001/net/filter/mock_stream... net/filter/mock_stream_source.h:43: bool sync; On 2016/07/21 18:14:08, mmenke wrote: > Can these all be constant? Fine if not. Could even use std::queue<const > QueuedResult> results_; instead, if that works. Done. I couldn't get "std::queue<const QueuedResult>" to compile though. https://codereview.chromium.org/1662763002/diff/400001/net/filter/mock_stream... net/filter/mock_stream_source.h:48: IOBuffer* dest_buffer_; On 2016/07/21 18:14:09, mmenke wrote: > scoped_refptr? (+include header for it) Done. https://codereview.chromium.org/1662763002/diff/400001/net/filter/sdch_policy... File net/filter/sdch_policy_delegate.h (right): https://codereview.chromium.org/1662763002/diff/400001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.h:6: #define NET_FILTER_SDCH_POLICY_DELEGATE_H On 2016/07/21 18:14:09, mmenke wrote: > +_ (x3) Done. https://codereview.chromium.org/1662763002/diff/400001/net/filter/sdch_stream... File net/filter/sdch_stream_source.h (right): https://codereview.chromium.org/1662763002/diff/400001/net/filter/sdch_stream... net/filter/sdch_stream_source.h:6: #define NET_FILTER_SDCH_STREAM_SOURCE_H On 2016/07/21 18:14:09, mmenke wrote: > +_ (x3) Done. https://codereview.chromium.org/1662763002/diff/400001/net/filter/stream_sour... File net/filter/stream_source.h (right): https://codereview.chromium.org/1662763002/diff/400001/net/filter/stream_sour... net/filter/stream_source.h:6: #define NET_FILTER_STREAM_SOURCE_H On 2016/07/21 18:14:09, mmenke wrote: > +_ (x3) Done. https://codereview.chromium.org/1662763002/diff/400001/net/filter/stream_sour... net/filter/stream_source.h:34: StreamSource(SourceType type); On 2016/07/21 18:14:09, mmenke wrote: > explicit Done. https://codereview.chromium.org/1662763002/diff/400001/net/filter/stream_sour... net/filter/stream_source.h:56: virtual std::string OrderedTypeStringList() const; On 2016/07/21 18:14:09, mmenke wrote: > include <string> Done. https://codereview.chromium.org/1662763002/diff/400001/net/url_request/url_re... File net/url_request/url_request_file_job.cc (right): https://codereview.chromium.org/1662763002/diff/400001/net/url_request/url_re... net/url_request/url_request_file_job.cc:25: #include "base/memory/ptr_util.h" On 2016/07/21 18:14:09, mmenke wrote: > What's this being used for? Done. https://codereview.chromium.org/1662763002/diff/440001/net/docs/filter.md File net/docs/filter.md (right): https://codereview.chromium.org/1662763002/diff/440001/net/docs/filter.md#new... net/docs/filter.md:7: * brotili (handling "br" Content-Encoding) On 2016/07/22 12:29:19, eustas wrote: > s/brotili/brotli Done. https://codereview.chromium.org/1662763002/diff/440001/net/filter/brotli_stre... File net/filter/brotli_stream_source.cc (right): https://codereview.chromium.org/1662763002/diff/440001/net/filter/brotli_stre... net/filter/brotli_stream_source.cc:39: "BrotliFilter.Status", static_cast<int>(decoding_status_), On 2016/07/22 12:29:19, eustas wrote: > This file seems to be based on old version of brotli_filter (some UMA's are > missing) Done. Thanks for letting me know. I've rebased this on the latest change. https://codereview.chromium.org/1662763002/diff/440001/net/url_request/url_re... File net/url_request/url_request_http_job.cc (right): https://codereview.chromium.org/1662763002/diff/440001/net/url_request/url_re... net/url_request/url_request_http_job.cc:1229: for (std::vector<StreamSource::SourceType>::reverse_iterator riter = On 2016/07/21 18:14:09, mmenke wrote: > r_iter? riter seems too much a misspelling of writer. Done. https://codereview.chromium.org/1662763002/diff/440001/net/url_request/url_re... File net/url_request/url_request_job.cc (right): https://codereview.chromium.org/1662763002/diff/440001/net/url_request/url_re... net/url_request/url_request_job.cc:124: URLRequestJobStreamSource(URLRequestJob* job) On 2016/07/21 18:14:09, mmenke wrote: > explicit Done. https://codereview.chromium.org/1662763002/diff/440001/net/url_request/url_re... net/url_request/url_request_job.cc:136: // the filter chain. On 2016/07/21 18:14:09, mmenke wrote: > Is this strictly needed? If callback_ is bound to a method on the higher layer > StreamSource, |this| can only be deleted if that higher layer StreamSource was > deleted, and it should presumably be using a weak ptr. Otherwise, it's owned by > the URLRequestJob itself, which also uses a weak ptr (And never deletes its > StreamSource, anyways)... Am I missing something? > > May still be worth doing this, just want to know if I'm missing something. Done. Sorry, you are right. It is not needed. I think I put it there when I was trying to debug something and didn't revisit it afterwards. Thanks for pointing it out. https://codereview.chromium.org/1662763002/diff/440001/net/url_request/url_re... net/url_request/url_request_job.cc:154: base::WeakPtrFactory<URLRequestJobStreamSource> weak_factory_; On 2016/07/21 18:14:09, mmenke wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/1662763002/diff/440001/net/url_request/url_re... net/url_request/url_request_job.cc:301: return std::move(source); On 2016/07/21 18:14:09, mmenke wrote: > std::move not needed. > > Should probably just be: return > base::MakeUnique<URLRequestJobStreamSource>(this);, and include ptr_util.h. Done. https://codereview.chromium.org/1662763002/diff/440001/net/url_request/url_re... net/url_request/url_request_job.cc:553: if (source_->type() == StreamSource::TYPE_NONE) { On 2016/07/21 18:14:09, mmenke wrote: > SetupSource() can return nullptr on failure. Need to support that (And have > tests?) Done. I added a test in url_request_http_job_unittest.cc to test the case where SetupSource() returns nullptr. https://codereview.chromium.org/1662763002/diff/440001/net/url_request/url_re... File net/url_request/url_request_job.h (right): https://codereview.chromium.org/1662763002/diff/440001/net/url_request/url_re... net/url_request/url_request_job.h:51: typedef base::Callback<void(net::Error, size_t)> ReadRawCompleteCallback; On 2016/07/21 18:14:10, mmenke wrote: > Is this even used? > > If not, remove it. > > If so, need to include Callback header, should have a blank line after it, need > to remove net::, and should include net_errors.h. Done. Sorry, it isn't used. https://codereview.chromium.org/1662763002/diff/440001/net/url_request/url_re... net/url_request/url_request_job.h:150: // or nullptr if no StreamSource should be used. This class takes ownership of On 2016/07/21 18:14:10, mmenke wrote: > "... or nullptr on error"? Done. https://codereview.chromium.org/1662763002/diff/440001/net/url_request/url_re... net/url_request/url_request_job.h:152: virtual std::unique_ptr<StreamSource> SetupSource(); On 2016/07/21 18:14:10, mmenke wrote: > This should be private, right? It's an old comment, but should also remove > "This class takes ownership of..." Do you mean protected instead? The subclasses (http_job and file_job) need to access parent class's implementation. https://codereview.chromium.org/1662763002/diff/440001/net/url_request/url_re... net/url_request/url_request_job.h:431: // None null if ReadRawData() returned ERR_IO_PENDING, and the read has not On 2016/07/21 18:14:10, mmenke wrote: > Non-null if... Done.
https://codereview.chromium.org/1662763002/diff/440001/net/url_request/url_re... File net/url_request/url_request_job.h (right): https://codereview.chromium.org/1662763002/diff/440001/net/url_request/url_re... net/url_request/url_request_job.h:152: virtual std::unique_ptr<StreamSource> SetupSource(); On 2016/07/27 20:32:05, xunjieli wrote: > On 2016/07/21 18:14:10, mmenke wrote: > > This should be private, right? It's an old comment, but should also remove > > "This class takes ownership of..." > > Do you mean protected instead? The subclasses (http_job and file_job) need to > access parent class's implementation. Oops...I didn't realize subclasses recursively called this on the parent class, to create the raw stream source. Yes, protected then. That's...err...exactly what I meant! https://codereview.chromium.org/1662763002/diff/540001/net/filter/filter_stre... File net/filter/filter_stream_source.cc (right): https://codereview.chromium.org/1662763002/diff/540001/net/filter/filter_stre... net/filter/filter_stream_source.cc:38: input_buffer_ = new IOBufferWithSize(kBufferSize); I don't think we need this as a member - can just use drainable_input_buffer_, and update it as needed. https://codereview.chromium.org/1662763002/diff/540001/net/filter/filter_stre... net/filter/filter_stream_source.cc:96: previous_->Read(input_buffer_.get(), kBufferSize, input_buffer_ -> drainable_input_buffer_ (And call drainable_input_buffer_.SetOffset(0) before the Read call) https://codereview.chromium.org/1662763002/diff/540001/net/filter/filter_stre... net/filter/filter_stream_source.cc:110: input_buffer_.get(), base::checked_cast<size_t>(result)); Can remove this, if you follow my other suggestions on drainable_input_buffer_ management. https://codereview.chromium.org/1662763002/diff/540001/net/filter/filter_stre... net/filter/filter_stream_source.cc:113: previous_eof_reached_ = true; Hrm...Should we set this if result < 0? Doesn't really matter, just not sure we should consider error the same as eof. https://codereview.chromium.org/1662763002/diff/540001/net/filter/filter_stre... net/filter/filter_stream_source.cc:127: CHECK_LE(0, result); All of these CHECKs should be DCHECKs to avoid bloating up the binary, unless we're particularly concerned about them. Then should have a TODO about changing them all to DCHECKs. https://codereview.chromium.org/1662763002/diff/540001/net/filter/filter_stre... net/filter/filter_stream_source.cc:143: DCHECK_NE(ERR_IO_PENDING, result); Should we just merge this with the previous state, since we don't allow DoFilterData to return ERR_IO_PENDING? Doesn't really matter all that much. https://codereview.chromium.org/1662763002/diff/540001/net/filter/filter_stre... net/filter/filter_stream_source.cc:145: if (result == OK && !previous_eof_reached_) Think this line could use a comment. https://codereview.chromium.org/1662763002/diff/540001/net/url_request/url_re... File net/url_request/url_request_http_job.cc (right): https://codereview.chromium.org/1662763002/diff/540001/net/url_request/url_re... net/url_request/url_request_http_job.cc:1232: std::unique_ptr<FilterStreamSource> next = nullptr; nit: nullptr not needed. https://codereview.chromium.org/1662763002/diff/540001/net/url_request/url_re... net/url_request/url_request_http_job.cc:1453: weak_factory_.GetWeakPtr())); Is there a reason for switching to a weak_factory_ here? We own the transaction. https://codereview.chromium.org/1662763002/diff/540001/net/url_request/url_re... File net/url_request/url_request_http_job_unittest.cc (right): https://codereview.chromium.org/1662763002/diff/540001/net/url_request/url_re... net/url_request/url_request_http_job_unittest.cc:63: TestURLRequestHttpJob(URLRequest* request, bool use_null_source) May be simpler to make a setter for use_null_source, rather than creating a separate test fixture with it. Doesn't really matter https://codereview.chromium.org/1662763002/diff/540001/net/url_request/url_re... File net/url_request/url_request_job.cc (right): https://codereview.chromium.org/1662763002/diff/540001/net/url_request/url_re... net/url_request/url_request_job.cc:126: : StreamSource(StreamSource::TYPE_NONE), job_(job) {} DCHECK(job_);? Since we never modify it, DCHECK seems more useful here, if we want to include one. https://codereview.chromium.org/1662763002/diff/540001/net/url_request/url_re... net/url_request/url_request_job.cc:139: URLRequestJob* job_; Maybe URLRequestJob* const job_? (Meaning the pointer is const, not the job_) https://codereview.chromium.org/1662763002/diff/540001/net/url_request/url_re... net/url_request/url_request_job.cc:225: pending_read_buffer_ = nullptr; Optional: May be a little cleaner as: if (result > 0) { // AddByteTransferEvent code goes here } pending_read_buffer_ = nullptr; // All other logic goes here. if (result > 0) ... It's no less code, but it makes clearer that the pending_read_buffer_ is done on all branches, and seems marginally less regression prone to me. https://codereview.chromium.org/1662763002/diff/540001/net/url_request/url_re... net/url_request/url_request_job.cc:228: request_->NotifyReadCompleted(result); I think early return is a little better in each of these cases - we really don't want to share any code at the end of these branches, and these cases are very mutually exclusive, so think it's the preferred pattern, in this case. https://codereview.chromium.org/1662763002/diff/540001/net/url_request/url_re... net/url_request/url_request_job.cc:540: NotifyDone(URLRequestStatus(URLRequestStatus::FAILED, ERR_FAILED)); Could we add an error code for this case? Don't need to add any error text, but a clear way to know about this failure would be useful in identifying the rare cases when it happens. https://codereview.chromium.org/1662763002/diff/540001/net/url_request/url_re... net/url_request/url_request_job.cc:542: } nit: Suggest a blank line here. https://codereview.chromium.org/1662763002/diff/540001/net/url_request/url_re... net/url_request/url_request_job.cc:749: read_raw_callback_ = callback; Should we use a callback, or just keep a pointer to the URLRequestJobStreamSource? Can't use ResetAndReturn or DCHECKs with the latter (Speaking of those DCHECKs...this method could use one that it's not set), but it makes the code clearer. https://codereview.chromium.org/1662763002/diff/540001/net/url_request/url_re... File net/url_request/url_request_job.h (right): https://codereview.chromium.org/1662763002/diff/540001/net/url_request/url_re... net/url_request/url_request_job.h:239: const GURL& redirect_destination); nit: Blank line between unrelated methods. https://codereview.chromium.org/1662763002/diff/540001/net/url_request/url_re... net/url_request/url_request_job.h:310: virtual std::unique_ptr<StreamSource> SetupSource(); nit: SetUpSource() (Setup is a noun, Set Up functions as a verb) https://codereview.chromium.org/1662763002/diff/540001/net/url_request/url_re... net/url_request/url_request_job.h:310: virtual std::unique_ptr<StreamSource> SetupSource(); Can this be const? Not a big deal if not, but I assume it has no side effects? https://codereview.chromium.org/1662763002/diff/540001/net/url_request/url_re... net/url_request/url_request_job.h:310: virtual std::unique_ptr<StreamSource> SetupSource(); Also, should this be SetUpStream or SetUpStreamSource? "Stream" seems more important the "Source" to me. I know what a Stream is. I'm not sure what a "Source" is (It could be a connection, for instance). If Stream is the more important term, could even rename StreamSource to SourceStream? Don't have strong opinions here, just thinking out loud. On the down side, there's at least one other SourceStream class in Chrome, though in a different namespace. There are also BlahStreamSource classes as well, though there's no other StreamSource I can find. https://codereview.chromium.org/1662763002/diff/540001/net/url_request/url_re... net/url_request/url_request_job.h:396: // The first StreamSource of the StreamSource chain used. Using "first" here is inconsistent with using "previous_" to mean the next filter closer to the HttpTransaction in FilterStreamSource. source_->previous() should be null, if |source_| is the "first", but in this case, it's the "second" filter. I'd replace "previous_" with "next_", because I think of things in terms of URLRequest->next_layer->next_layer->.... (top/right/first -> ... -> bottom/left/last). Calling this one the "last" or the "bottom" source seems really weird to me. I suppose calling this top, and considering |previous_| to mean "lower" kinda works, but seems less natural to me. https://codereview.chromium.org/1662763002/diff/540001/net/url_request/url_re... net/url_request/url_request_job.h:438: size_t postfilter_bytes_read_; BUG: These need to be int64_t's. If we're on a 32-bit system, we can still end up downloading 4+ GB files. (Main reason not to use uint64_t is that all the other sizes are int64_t's, and changing all that is beyond the scope of this CL). Suppose we could test that, if we didn't actually write anything to the IOBuffer and claimed we did, though testing that case may be overkill. https://codereview.chromium.org/1662763002/diff/540001/net/url_request/url_re... File net/url_request/url_request_job_unittest.cc (right): https://codereview.chromium.org/1662763002/diff/540001/net/url_request/url_re... net/url_request/url_request_job_unittest.cc:190: EXPECT_TRUE(network_layer.done_reading_called()); Should we check the final result of the transaction? A lot of these tests don't, which seems weird.
Patchset #15 (id:560001) has been deleted
Thank you very much for the review! I've uploaded a new patch to address the comments. PTAL. https://codereview.chromium.org/1662763002/diff/540001/net/filter/filter_stre... File net/filter/filter_stream_source.cc (right): https://codereview.chromium.org/1662763002/diff/540001/net/filter/filter_stre... net/filter/filter_stream_source.cc:38: input_buffer_ = new IOBufferWithSize(kBufferSize); On 2016/07/28 18:40:13, mmenke wrote: > I don't think we need this as a member - can just use drainable_input_buffer_, > and update it as needed. Acknowledged. Talked offline. Because DrainableIOBuffer doesn't have a way to set |size| when it is refilled, we can't go with this approach. https://codereview.chromium.org/1662763002/diff/540001/net/filter/filter_stre... net/filter/filter_stream_source.cc:96: previous_->Read(input_buffer_.get(), kBufferSize, On 2016/07/28 18:40:13, mmenke wrote: > input_buffer_ -> drainable_input_buffer_ (And call > drainable_input_buffer_.SetOffset(0) before the Read call) Acknowledged. https://codereview.chromium.org/1662763002/diff/540001/net/filter/filter_stre... net/filter/filter_stream_source.cc:110: input_buffer_.get(), base::checked_cast<size_t>(result)); On 2016/07/28 18:40:13, mmenke wrote: > Can remove this, if you follow my other suggestions on drainable_input_buffer_ > management. Acknowledged. https://codereview.chromium.org/1662763002/diff/540001/net/filter/filter_stre... net/filter/filter_stream_source.cc:113: previous_eof_reached_ = true; On 2016/07/28 18:40:12, mmenke wrote: > Hrm...Should we set this if result < 0? Doesn't really matter, just not sure we > should consider error the same as eof. Done. Yes, we need to set this variable if result < 0, so we do not do previous_->Read() in the future if either an error occurs or eof is reached. I renamed this variable. Hopefully that is clearer? https://codereview.chromium.org/1662763002/diff/540001/net/filter/filter_stre... net/filter/filter_stream_source.cc:127: CHECK_LE(0, result); On 2016/07/28 18:40:12, mmenke wrote: > All of these CHECKs should be DCHECKs to avoid bloating up the binary, unless > we're particularly concerned about them. Then should have a TODO about changing > them all to DCHECKs. Done. https://codereview.chromium.org/1662763002/diff/540001/net/filter/filter_stre... net/filter/filter_stream_source.cc:143: DCHECK_NE(ERR_IO_PENDING, result); On 2016/07/28 18:40:12, mmenke wrote: > Should we just merge this with the previous state, since we don't allow > DoFilterData to return ERR_IO_PENDING? Doesn't really matter all that much. Done. Great idea! https://codereview.chromium.org/1662763002/diff/540001/net/filter/filter_stre... net/filter/filter_stream_source.cc:145: if (result == OK && !previous_eof_reached_) On 2016/07/28 18:40:12, mmenke wrote: > Think this line could use a comment. Done. https://codereview.chromium.org/1662763002/diff/540001/net/url_request/url_re... File net/url_request/url_request_http_job.cc (right): https://codereview.chromium.org/1662763002/diff/540001/net/url_request/url_re... net/url_request/url_request_http_job.cc:1232: std::unique_ptr<FilterStreamSource> next = nullptr; On 2016/07/28 18:40:13, mmenke wrote: > nit: nullptr not needed. Done. https://codereview.chromium.org/1662763002/diff/540001/net/url_request/url_re... net/url_request/url_request_http_job.cc:1453: weak_factory_.GetWeakPtr())); On 2016/07/28 18:40:13, mmenke wrote: > Is there a reason for switching to a weak_factory_ here? We own the > transaction. Done. No reason. I think I changed that during debugging and forgot to change it back. Thanks for pointing out! https://codereview.chromium.org/1662763002/diff/540001/net/url_request/url_re... File net/url_request/url_request_http_job_unittest.cc (right): https://codereview.chromium.org/1662763002/diff/540001/net/url_request/url_re... net/url_request/url_request_http_job_unittest.cc:63: TestURLRequestHttpJob(URLRequest* request, bool use_null_source) On 2016/07/28 18:40:13, mmenke wrote: > May be simpler to make a setter for use_null_source, rather than creating a > separate test fixture with it. Doesn't really matter Done. https://codereview.chromium.org/1662763002/diff/540001/net/url_request/url_re... File net/url_request/url_request_job.cc (right): https://codereview.chromium.org/1662763002/diff/540001/net/url_request/url_re... net/url_request/url_request_job.cc:126: : StreamSource(StreamSource::TYPE_NONE), job_(job) {} On 2016/07/28 18:40:13, mmenke wrote: > DCHECK(job_);? Since we never modify it, DCHECK seems more useful here, if we > want to include one. Done. https://codereview.chromium.org/1662763002/diff/540001/net/url_request/url_re... net/url_request/url_request_job.cc:139: URLRequestJob* job_; On 2016/07/28 18:40:13, mmenke wrote: > Maybe URLRequestJob* const job_? (Meaning the pointer is const, not the job_) Done. https://codereview.chromium.org/1662763002/diff/540001/net/url_request/url_re... net/url_request/url_request_job.cc:225: pending_read_buffer_ = nullptr; On 2016/07/28 18:40:13, mmenke wrote: > Optional: May be a little cleaner as: > > if (result > 0) { > // AddByteTransferEvent code goes here > } > pending_read_buffer_ = nullptr; > > // All other logic goes here. > if (result > 0) ... > > It's no less code, but it makes clearer that the pending_read_buffer_ is done on > all branches, and seems marginally less regression prone to me. Done. great suggestion! https://codereview.chromium.org/1662763002/diff/540001/net/url_request/url_re... net/url_request/url_request_job.cc:228: request_->NotifyReadCompleted(result); On 2016/07/28 18:40:13, mmenke wrote: > I think early return is a little better in each of these cases - we really don't > want to share any code at the end of these branches, and these cases are very > mutually exclusive, so think it's the preferred pattern, in this case. Done. https://codereview.chromium.org/1662763002/diff/540001/net/url_request/url_re... net/url_request/url_request_job.cc:540: NotifyDone(URLRequestStatus(URLRequestStatus::FAILED, ERR_FAILED)); On 2016/07/28 18:40:13, mmenke wrote: > Could we add an error code for this case? Don't need to add any error text, but > a clear way to know about this failure would be useful in identifying the rare > cases when it happens. Done. https://codereview.chromium.org/1662763002/diff/540001/net/url_request/url_re... net/url_request/url_request_job.cc:542: } On 2016/07/28 18:40:13, mmenke wrote: > nit: Suggest a blank line here. Done. https://codereview.chromium.org/1662763002/diff/540001/net/url_request/url_re... net/url_request/url_request_job.cc:749: read_raw_callback_ = callback; On 2016/07/28 18:40:13, mmenke wrote: > Should we use a callback, or just keep a pointer to the > URLRequestJobStreamSource? Can't use ResetAndReturn or DCHECKs with the latter > (Speaking of those DCHECKs...this method could use one that it's not set), but > it makes the code clearer. Acknowledged. Talked offline and we decided to leave this as it is. This is because URLRequestJob indirectly owns URLRequestJobSteamSource, the callback here is the callback passed in by the SourceStream above URLRequestJobStreamSource. https://codereview.chromium.org/1662763002/diff/540001/net/url_request/url_re... File net/url_request/url_request_job.h (right): https://codereview.chromium.org/1662763002/diff/540001/net/url_request/url_re... net/url_request/url_request_job.h:239: const GURL& redirect_destination); On 2016/07/28 18:40:13, mmenke wrote: > nit: Blank line between unrelated methods. Done. https://codereview.chromium.org/1662763002/diff/540001/net/url_request/url_re... net/url_request/url_request_job.h:310: virtual std::unique_ptr<StreamSource> SetupSource(); On 2016/07/28 18:40:14, mmenke wrote: > nit: SetUpSource() > > (Setup is a noun, Set Up functions as a verb) Done. https://codereview.chromium.org/1662763002/diff/540001/net/url_request/url_re... net/url_request/url_request_job.h:310: virtual std::unique_ptr<StreamSource> SetupSource(); On 2016/07/28 18:40:14, mmenke wrote: > Also, should this be SetUpStream or SetUpStreamSource? "Stream" seems more > important the "Source" to me. I know what a Stream is. I'm not sure what a > "Source" is (It could be a connection, for instance). > > If Stream is the more important term, could even rename StreamSource to > SourceStream? Don't have strong opinions here, just thinking out loud. On the > down side, there's at least one other SourceStream class in Chrome, though in a > different namespace. There are also BlahStreamSource classes as well, though > there's no other StreamSource I can find. Done. I agree with you. I like SourceStream better. https://codereview.chromium.org/1662763002/diff/540001/net/url_request/url_re... net/url_request/url_request_job.h:310: virtual std::unique_ptr<StreamSource> SetupSource(); On 2016/07/28 18:40:13, mmenke wrote: > Can this be const? Not a big deal if not, but I assume it has no side effects? It can't be const because when URLRequestHttpJob creates the StreamSource, it might need to create a SdchSourceStreamDelegate and make it a member variable. Other than that, it doesn't have any side effect. https://codereview.chromium.org/1662763002/diff/540001/net/url_request/url_re... net/url_request/url_request_job.h:396: // The first StreamSource of the StreamSource chain used. On 2016/07/28 18:40:14, mmenke wrote: > Using "first" here is inconsistent with using "previous_" to mean the next > filter closer to the HttpTransaction in FilterStreamSource. source_->previous() > should be null, if |source_| is the "first", but in this case, it's the "second" > filter. > > I'd replace "previous_" with "next_", because I think of things in terms of > URLRequest->next_layer->next_layer->.... (top/right/first -> ... -> > bottom/left/last). > > Calling this one the "last" or the "bottom" source seems really weird to me. I > suppose calling this top, and considering |previous_| to mean "lower" kinda > works, but seems less natural to me. Done. https://codereview.chromium.org/1662763002/diff/540001/net/url_request/url_re... net/url_request/url_request_job.h:438: size_t postfilter_bytes_read_; On 2016/07/28 18:40:13, mmenke wrote: > BUG: These need to be int64_t's. If we're on a 32-bit system, we can still end > up downloading 4+ GB files. (Main reason not to use uint64_t is that all the > other sizes are int64_t's, and changing all that is beyond the scope of this > CL). > > Suppose we could test that, if we didn't actually write anything to the IOBuffer > and claimed we did, though testing that case may be overkill. Done. I've reverted these to the originals which are int64_t's. Thanks for pointing that out! https://codereview.chromium.org/1662763002/diff/540001/net/url_request/url_re... File net/url_request/url_request_job_unittest.cc (right): https://codereview.chromium.org/1662763002/diff/540001/net/url_request/url_re... net/url_request/url_request_job_unittest.cc:190: EXPECT_TRUE(network_layer.done_reading_called()); On 2016/07/28 18:40:14, mmenke wrote: > Should we check the final result of the transaction? A lot of these tests > don't, which seems weird. Done.
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
A couple quick comments. Think this will be my last set of comments, I'll leave the specific filters to the other reviewers. https://codereview.chromium.org/1662763002/diff/580001/net/filter/filter_sour... File net/filter/filter_source_stream.cc (right): https://codereview.chromium.org/1662763002/diff/580001/net/filter/filter_sour... net/filter/filter_source_stream.cc:39: } nit: Remove braces. https://codereview.chromium.org/1662763002/diff/580001/net/filter/filter_sour... net/filter/filter_source_stream.cc:51: return static_cast<Error>(rv); cast not needed. https://codereview.chromium.org/1662763002/diff/580001/net/filter/filter_sour... net/filter/filter_source_stream.cc:58: } nit: Remove braces https://codereview.chromium.org/1662763002/diff/580001/net/filter/filter_sour... net/filter/filter_source_stream.cc:108: input_buffer_.get(), base::checked_cast<size_t>(result)); static_cast (We have the > OK check just above, so the checked_cast seems redundant) https://codereview.chromium.org/1662763002/diff/580001/net/filter/filter_sour... net/filter/filter_source_stream.cc:156: DCHECK(!callback_.is_null()); Suggest putting this up next to DCHECK_NE(ERR_IO_PENDING, result); - it's one of the prerequisites on the state machine when calling this method. Up to you, though. Largely a matter of personal preference. https://codereview.chromium.org/1662763002/diff/580001/net/filter/filter_sour... File net/filter/filter_source_stream.h (right): https://codereview.chromium.org/1662763002/diff/580001/net/filter/filter_sour... net/filter/filter_source_stream.h:94: bool next_end_reached_; next_stream_end_reached_? (Since we also have next_state_, and next can also be an adjective describing end, instead of a noun, seems a bit confusing) https://codereview.chromium.org/1662763002/diff/580001/net/filter/gzip_source... File net/filter/gzip_source_stream.cc (right): https://codereview.chromium.org/1662763002/diff/580001/net/filter/gzip_source... net/filter/gzip_source_stream.cc:35: } nit: Remove braces. https://codereview.chromium.org/1662763002/diff/580001/net/filter/gzip_source... net/filter/gzip_source_stream.cc:223: char d = input_buffer->data()[0]; nit: Prefer meaningful variable names. first_byte? Could even just remove the variable. https://codereview.chromium.org/1662763002/diff/580001/net/filter/gzip_source... File net/filter/gzip_source_stream.h (right): https://codereview.chromium.org/1662763002/diff/580001/net/filter/gzip_source... net/filter/gzip_source_stream.h:17: typedef struct z_stream_s z_stream; Can we get this out of the global namespace, move it into GzipSourceStream or something?
Description was changed from ========== Implement pull-based design for net::Filter This CL implements a pull-based design for net::Filter. This CL is based on an earlier prototype CL (crrev.com/1274863002 by ellyjones@). The design for this pull-based net::Filter is proposed in crbug.com/474858. BUG=474859 ========== to ========== Implement pull-based design for content decoding This CL implements a pull-based design for content decoding. This CL is based on an earlier prototype CL (crrev.com/1274863002 by ellyjones@). The design for this pull-based content decoding is proposed in crbug.com/474858. BUG=474859 ==========
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thank you very much for the review! I have addressed all comments expect the one about moving "typedef struct z_stream_s z_stream;" out of global namespace. PTAL. https://codereview.chromium.org/1662763002/diff/580001/net/filter/filter_sour... File net/filter/filter_source_stream.cc (right): https://codereview.chromium.org/1662763002/diff/580001/net/filter/filter_sour... net/filter/filter_source_stream.cc:39: } On 2016/08/01 21:55:30, mmenke wrote: > nit: Remove braces. Done. https://codereview.chromium.org/1662763002/diff/580001/net/filter/filter_sour... net/filter/filter_source_stream.cc:51: return static_cast<Error>(rv); On 2016/08/01 21:55:30, mmenke wrote: > cast not needed. Done. https://codereview.chromium.org/1662763002/diff/580001/net/filter/filter_sour... net/filter/filter_source_stream.cc:58: } On 2016/08/01 21:55:30, mmenke wrote: > nit: Remove braces Done. https://codereview.chromium.org/1662763002/diff/580001/net/filter/filter_sour... net/filter/filter_source_stream.cc:108: input_buffer_.get(), base::checked_cast<size_t>(result)); On 2016/08/01 21:55:30, mmenke wrote: > static_cast (We have the > OK check just above, so the checked_cast seems > redundant) Done. https://codereview.chromium.org/1662763002/diff/580001/net/filter/filter_sour... net/filter/filter_source_stream.cc:156: DCHECK(!callback_.is_null()); On 2016/08/01 21:55:30, mmenke wrote: > Suggest putting this up next to DCHECK_NE(ERR_IO_PENDING, result); - it's one of > the prerequisites on the state machine when calling this method. Up to you, > though. Largely a matter of personal preference. Done. https://codereview.chromium.org/1662763002/diff/580001/net/filter/filter_sour... File net/filter/filter_source_stream.h (right): https://codereview.chromium.org/1662763002/diff/580001/net/filter/filter_sour... net/filter/filter_source_stream.h:94: bool next_end_reached_; On 2016/08/01 21:55:30, mmenke wrote: > next_stream_end_reached_? (Since we also have next_state_, and next can also be > an adjective describing end, instead of a noun, seems a bit confusing) Done. https://codereview.chromium.org/1662763002/diff/580001/net/filter/gzip_source... File net/filter/gzip_source_stream.cc (right): https://codereview.chromium.org/1662763002/diff/580001/net/filter/gzip_source... net/filter/gzip_source_stream.cc:35: } On 2016/08/01 21:55:30, mmenke wrote: > nit: Remove braces. Done. https://codereview.chromium.org/1662763002/diff/580001/net/filter/gzip_source... net/filter/gzip_source_stream.cc:223: char d = input_buffer->data()[0]; On 2016/08/01 21:55:30, mmenke wrote: > nit: Prefer meaningful variable names. first_byte? Could even just remove the > variable. Done. https://codereview.chromium.org/1662763002/diff/580001/net/filter/gzip_source... File net/filter/gzip_source_stream.h (right): https://codereview.chromium.org/1662763002/diff/580001/net/filter/gzip_source... net/filter/gzip_source_stream.h:17: typedef struct z_stream_s z_stream; On 2016/08/01 21:55:30, mmenke wrote: > Can we get this out of the global namespace, move it into GzipSourceStream or > something? It fails to compile if I move it elsewhere. I think this is needed so we can forward declare z_stream_s and somehow it has to be in the global namespace.
Hello mmenke@, rdsmith@, eustas@: M54 is going to be cut on 08/25. The change is risky, so I think we can aim to land this change in M55 to get enough bake time in Canary. I will be OOO from 08/22-29. If you give me l-g-t-m before then, I will wait to land it after the branch point and when I am back. Thanks for the review!
https://codereview.chromium.org/1662763002/diff/600001/net/filter/brotli_sour... File net/filter/brotli_source_stream.cc (right): https://codereview.chromium.org/1662763002/diff/600001/net/filter/brotli_sour... net/filter/brotli_source_stream.cc:126: decoding_status_ = DecodingStatus::DECODING_IN_PROGRESS; I suppose BROTLI_RESULT_NEEDS_MORE_OUTPUT should not cause decoding_status_ change. Otherwise, wrong filter status may be reported in destructor.
Should we have tests for FilterStreamSource, to make sure we have all the cases covered (DoFilterData call returns no bytes, return bytes on two (three?) successive calls, etc). We presumably should have tests for those cases for individual filters, anyways, so may well not be worth the effort, just thought I'd bring it up.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #17 (id:620001) has been deleted
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
> Should we have tests for FilterStreamSource, to make sure we have all the cases > covered (DoFilterData call returns no bytes, return bytes on two (three?) > successive calls, etc). > We presumably should have tests for those cases for individual filters, anyways, > so may well not be worth the effort, just thought I'd bring it up. Done. Thanks for the suggestion to add the the tests! I found one bug by writing unit tests for FilterSourceStream. The bug is that we were not passing the EOF read from |next| to FilterData(). I have updated filter_source_stream.cc accordingly. PTAL. https://codereview.chromium.org/1662763002/diff/600001/net/filter/brotli_sour... File net/filter/brotli_source_stream.cc (right): https://codereview.chromium.org/1662763002/diff/600001/net/filter/brotli_sour... net/filter/brotli_source_stream.cc:126: decoding_status_ = DecodingStatus::DECODING_IN_PROGRESS; On 2016/08/02 14:43:33, eustas wrote: > I suppose BROTLI_RESULT_NEEDS_MORE_OUTPUT should not cause decoding_status_ > change. Otherwise, wrong filter status may be reported in destructor. Done. Good catch!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Initial comments on the core classes ({Filter,}SourceStream, URLRequestJob, a
few parts of URLRequestHttpJob).
Very nice! I really like how this is coming together. I'm publishing these
comments now before I get to the rest of the CL because I think they stand on
their own and other comments on other classes are likely to be independent. I
will keep reviewing the CL; you should feel free to integrate these comments or
wait for my next round as you wish. Having said that, next round is probably
next week :-J.
https://codereview.chromium.org/1662763002/diff/640001/net/filter/filter_sour...
File net/filter/filter_source_stream.cc (right):
https://codereview.chromium.org/1662763002/diff/640001/net/filter/filter_sour...
net/filter/filter_source_stream.cc:39: input_buffer_ = new
IOBufferWithSize(kBufferSize);
Why not in the constructor?
https://codereview.chromium.org/1662763002/diff/640001/net/filter/filter_sour...
net/filter/filter_source_stream.cc:145: // If can still read data from
|next_stream_| and filter did not return any
nit, rephrasing: "If data can still be read from |next_stream_| ..."
https://codereview.chromium.org/1662763002/diff/640001/net/filter/filter_sour...
net/filter/filter_source_stream.cc:148: next_state_ = STATE_READ_DATA;
Would an assertion that drainable_input_buffer_ has nothing left in it be
appropriate here?
https://codereview.chromium.org/1662763002/diff/640001/net/filter/filter_sour...
File net/filter/filter_source_stream.h (right):
https://codereview.chromium.org/1662763002/diff/640001/net/filter/filter_sour...
net/filter/filter_source_stream.h:22: // stream from which undecoded input can
be read. Except the last stream in
Suggestion: I'm struggling around the nomenclature used to express stream
relationships/ordering. Specifically, I'm not sure that "next"/"previous"
communicates well the actual relationship between the streams is (I don't
remember if you were involved in the design discussions around whether a sink
stream should own its source stream, or a source stream should own its sync
stream--we settled on a sink stream owning its source stream, but that was a
live design issue for a while). So I'm tempted to suggest a change of
nomenclature in that wherever there are descriptive words referring to the
relationship between streams, instead of using next/previous use source/sink.
That would involve a lot of changes of names, but I think would make the code
easier to understand for future readers. If you do this, it might be reasonable
to refer to "the last stream" might be referred to as "the ultimate source
stream", or it might be left as "the last stream" with the context being enough
for readers to figure out that it's all the way to the source in the chain, as
opposed to all the way to the sink.
Up to you, but I wanted to at least raise the question.
https://codereview.chromium.org/1662763002/diff/640001/net/filter/filter_sour...
net/filter/filter_source_stream.h:60: // Helper method used as a callback when
|next_stream_| is to complete a read
nit: Did you meant "... |next_stream_->Read()| is called to complete ..."?
https://codereview.chromium.org/1662763002/diff/640001/net/filter/filter_sour...
net/filter/filter_source_stream.h:62: void OnNextReadCompleted(int result);
nit, suggestion: I find these classes easier to read when there's a vertical
whitespace between methods that have comments next to them.
https://codereview.chromium.org/1662763002/diff/640001/net/filter/filter_sour...
net/filter/filter_source_stream.h:83: scoped_refptr<IOBuffer> input_buffer_;
nit, suggestion: Same as above--a line of vertical whitespace here?
https://codereview.chromium.org/1662763002/diff/640001/net/filter/source_stre...
File net/filter/source_stream.h (right):
https://codereview.chromium.org/1662763002/diff/640001/net/filter/source_stre...
net/filter/source_stream.h:44: // - Calls |callback| when it does complete,
with an error code and a count
nit, suggestion "an error code *or* a count"?
https://codereview.chromium.org/1662763002/diff/640001/net/filter/source_stre...
net/filter/source_stream.h:53: virtual std::string OrderedTypeStringList()
const;
Suggestion: If I'm reading the code correctly, the reason this isn't a pure
virtual is because URLRequestJobSourceStream doesn't implement it, and hence
ends up using this implementation. Would it make sense to move this
implementation into URLRJSS and make this a pure virtual?
https://codereview.chromium.org/1662763002/diff/640001/net/filter/source_stre...
net/filter/source_stream.h:53: virtual std::string OrderedTypeStringList()
const;
Why is this named OrderedTypeStringList()? I'm not sure what that name means.
https://codereview.chromium.org/1662763002/diff/640001/net/url_request/url_re...
File net/url_request/url_request_http_job.cc (right):
https://codereview.chromium.org/1662763002/diff/640001/net/url_request/url_re...
net/url_request/url_request_http_job.cc:1212:
types.push_back(SourceStream::TYPE_SDCH);
I may be misreading the CL (I haven't read all of it, and this is a tricky thing
to tease out without codesearch having indexed the source) but it looks to me as
if the primary use of the SourceStream type is in this function and the one
following it. If you agree, could you see what would be required to hoist that
declaration out of SourceStream and up into this file, and leave source_stream.h
not being dependent on knowing about the different types of stream sources?
If it's a big deal, another CL is fine (maybe I'll even just do that after you
land this one). But it would be a bit cleaner for the ABC to not have to know
about all the possible instantiations of its interface.
https://codereview.chromium.org/1662763002/diff/640001/net/url_request/url_re...
File net/url_request/url_request_job.cc (right):
https://codereview.chromium.org/1662763002/diff/640001/net/url_request/url_re...
net/url_request/url_request_job.cc:119: // SourceStreams own the previous
SourceStream in the chain, but the ultimate
nit, suggested rephrasing: "Each SourceStream owns the previous SourceStream
...".
https://codereview.chromium.org/1662763002/diff/640001/net/url_request/url_re...
net/url_request/url_request_job.cc:121: // is a proxy for URLRequestJob that is
owned by the first filter (in dataflow
nit, suggestion: " ... first stream (in ..."?
https://codereview.chromium.org/1662763002/diff/640001/net/url_request/url_re...
net/url_request/url_request_job.cc:141: URLRequestJob* const job_;
nit: Comment on why it's safe to keep a raw pointer.
https://codereview.chromium.org/1662763002/diff/640001/net/url_request/url_re...
net/url_request/url_request_job.cc:206: *bytes_read = result;
nitty nit, suggestion: I'd find the grouping in this function slightly more
intuitive if this if was at the end next to the "return result >= OK", since
this is setting an output parameter for the function which is basically a return
through the arguments list. Up to you, though.
https://codereview.chromium.org/1662763002/diff/640001/net/url_request/url_re...
net/url_request/url_request_job.cc:217: void
URLRequestJob::SourceReadComplete(bool synchronous, int result) {
nit, suggestion: Change name to "StreamReadComplete" or
"SourceStreamReadComplete" just to be very clear how the function is used?
https://codereview.chromium.org/1662763002/diff/640001/net/url_request/url_re...
net/url_request/url_request_job.cc:575: void
URLRequestJob::ReadRawDataComplete(int result) {
nit, suggestion: Name this function in parallel fashion to "SourceReadComplete"
(whatever name you settle on for that; see other comment). These are each
functions that are passed as callbacks or called directly in the synchronous
case for different Reads from this code, and I'm inclined to think that making
that parallelism clear in the name would be useful. Up to you, though.
https://codereview.chromium.org/1662763002/diff/640001/net/url_request/url_re...
net/url_request/url_request_job.cc:748: GatherRawReadStats(result);
Would you be willing to put in a TODO() to cleanup the interface with the
subclasses? I'm specifically feeling like what we're ideally like is to have
ReadRawData() take a callback, which would be a wrap of ReadRawDataComplete()
around the callback, as opposed to hard-coding the subclasses to call
ReadRawDataComplete(). If you don't agree, skip the TODO; figuring out the
right design to fix that wart is beyond the scope of this CL. But if you do
agree, I wouldn't mind leaving a breadcrumb about fixing it to come back to.
https://codereview.chromium.org/1662763002/diff/640001/net/url_request/url_re...
net/url_request/url_request_job.cc:761: void
URLRequestJob::GatherRawReadStats(int bytes_read) {
nit, suggestion: This routine is a bit misleadingly named; it mostly gathers the
read statistics, but it also nulls out raw_read_buffer_, so there's a way in
which what is it is just the stuff that needs to always be called (synchronously
or asynchronously) on completion of ReadRawData. Maybe change
ReadRawDataComplete->ReadRawDataCallback, and change this to
ReadRawDataComplete? (Up to you.)
https://codereview.chromium.org/1662763002/diff/640001/net/url_request/url_re...
File net/url_request/url_request_job.h (right):
https://codereview.chromium.org/1662763002/diff/640001/net/url_request/url_re...
net/url_request/url_request_job.h:249: void SourceReadComplete(bool synchronous,
int result);
Why is this protected rather than private?
Thanks for the review! PTAL. https://codereview.chromium.org/1662763002/diff/640001/net/filter/filter_sour... File net/filter/filter_source_stream.cc (right): https://codereview.chromium.org/1662763002/diff/640001/net/filter/filter_sour... net/filter/filter_source_stream.cc:39: input_buffer_ = new IOBufferWithSize(kBufferSize); On 2016/08/09 20:28:30, Randy Smith - Not in Fridays wrote: > Why not in the constructor? This is so that we don't initialize one unnecessarily. For instance, if caller cancels the request before reading the response body. There's no need to allocate a 32kB buffer in that case. https://codereview.chromium.org/1662763002/diff/640001/net/filter/filter_sour... net/filter/filter_source_stream.cc:145: // If can still read data from |next_stream_| and filter did not return any On 2016/08/09 20:28:30, Randy Smith - Not in Fridays wrote: > nit, rephrasing: "If data can still be read from |next_stream_| ..." Done. https://codereview.chromium.org/1662763002/diff/640001/net/filter/filter_sour... net/filter/filter_source_stream.cc:148: next_state_ = STATE_READ_DATA; On 2016/08/09 20:28:30, Randy Smith - Not in Fridays wrote: > Would an assertion that drainable_input_buffer_ has nothing left in it be > appropriate here? DoReadData() already has a DCHECK. https://codereview.chromium.org/1662763002/diff/640001/net/filter/filter_sour... File net/filter/filter_source_stream.h (right): https://codereview.chromium.org/1662763002/diff/640001/net/filter/filter_sour... net/filter/filter_source_stream.h:22: // stream from which undecoded input can be read. Except the last stream in On 2016/08/09 20:28:30, Randy Smith - Not in Fridays wrote: > Suggestion: I'm struggling around the nomenclature used to express stream > relationships/ordering. Specifically, I'm not sure that "next"/"previous" > communicates well the actual relationship between the streams is (I don't > remember if you were involved in the design discussions around whether a sink > stream should own its source stream, or a source stream should own its sync > stream--we settled on a sink stream owning its source stream, but that was a > live design issue for a while). So I'm tempted to suggest a change of > nomenclature in that wherever there are descriptive words referring to the > relationship between streams, instead of using next/previous use source/sink. > That would involve a lot of changes of names, but I think would make the code > easier to understand for future readers. If you do this, it might be reasonable > to refer to "the last stream" might be referred to as "the ultimate source > stream", or it might be left as "the last stream" with the context being enough > for readers to figure out that it's all the way to the source in the chain, as > opposed to all the way to the sink. > > Up to you, but I wanted to at least raise the question. Thanks for the suggestion. I have considered it, but I don't see a clear way to change the nomenclature to source/sink. The classes have "source" in their names. If we refer |this| as "sink", and |next| as "source", I find it counter-intuitive. Furthermore, a "sink" should provide a drain method and not a read method. Maybe I am missing something, since I wasn't involved in the initial design discussion. Let's talk about this in person. I've put it on your calendar, please feel free to re-schedule. https://codereview.chromium.org/1662763002/diff/640001/net/filter/filter_sour... net/filter/filter_source_stream.h:60: // Helper method used as a callback when |next_stream_| is to complete a read On 2016/08/09 20:28:30, Randy Smith - Not in Fridays wrote: > nit: Did you meant "... |next_stream_->Read()| is called to complete ..."? Done. I rephrased. Please let me know if it's clearer. https://codereview.chromium.org/1662763002/diff/640001/net/filter/filter_sour... net/filter/filter_source_stream.h:62: void OnNextReadCompleted(int result); On 2016/08/09 20:28:30, Randy Smith - Not in Fridays wrote: > nit, suggestion: I find these classes easier to read when there's a vertical > whitespace between methods that have comments next to them. Done. https://codereview.chromium.org/1662763002/diff/640001/net/filter/filter_sour... net/filter/filter_source_stream.h:83: scoped_refptr<IOBuffer> input_buffer_; On 2016/08/09 20:28:30, Randy Smith - Not in Fridays wrote: > nit, suggestion: Same as above--a line of vertical whitespace here? Done. https://codereview.chromium.org/1662763002/diff/640001/net/filter/source_stre... File net/filter/source_stream.h (right): https://codereview.chromium.org/1662763002/diff/640001/net/filter/source_stre... net/filter/source_stream.h:44: // - Calls |callback| when it does complete, with an error code and a count On 2016/08/09 20:28:30, Randy Smith - Not in Fridays wrote: > nit, suggestion "an error code *or* a count"? Done. https://codereview.chromium.org/1662763002/diff/640001/net/filter/source_stre... net/filter/source_stream.h:53: virtual std::string OrderedTypeStringList() const; On 2016/08/09 20:28:30, Randy Smith - Not in Fridays wrote: > Why is this named OrderedTypeStringList()? I'm not sure what that name means. This is moved over from the original code. The original name is: std::string OrderedFilterList() const; https://cs.chromium.org/chromium/src/net/filter/filter.h?rcl=0&l=238 https://codereview.chromium.org/1662763002/diff/640001/net/filter/source_stre... net/filter/source_stream.h:53: virtual std::string OrderedTypeStringList() const; On 2016/08/09 20:28:30, Randy Smith - Not in Fridays wrote: > Suggestion: If I'm reading the code correctly, the reason this isn't a pure > virtual is because URLRequestJobSourceStream doesn't implement it, and hence > ends up using this implementation. Would it make sense to move this > implementation into URLRJSS and make this a pure virtual? Done. https://codereview.chromium.org/1662763002/diff/640001/net/url_request/url_re... File net/url_request/url_request_http_job.cc (right): https://codereview.chromium.org/1662763002/diff/640001/net/url_request/url_re... net/url_request/url_request_http_job.cc:1212: types.push_back(SourceStream::TYPE_SDCH); On 2016/08/09 20:28:30, Randy Smith - Not in Fridays wrote: > I may be misreading the CL (I haven't read all of it, and this is a tricky thing > to tease out without codesearch having indexed the source) but it looks to me as > if the primary use of the SourceStream type is in this function and the one > following it. If you agree, could you see what would be required to hoist that > declaration out of SourceStream and up into this file, and leave source_stream.h > not being dependent on knowing about the different types of stream sources? > > If it's a big deal, another CL is fine (maybe I'll even just do that after you > land this one). But it would be a bit cleaner for the ABC to not have to know > about all the possible instantiations of its interface. Let's do that in another CL. Each SourceStream needs to know it's own type for logging purpose. I don't think it will be straightforward to hoist the type entirely out of source_stream.h. It seems naturally to associate a type with individual source stream, rather than having a list of them in url_request_http_job.cc. But I agree the type isn't used as much right now. https://codereview.chromium.org/1662763002/diff/640001/net/url_request/url_re... File net/url_request/url_request_job.cc (right): https://codereview.chromium.org/1662763002/diff/640001/net/url_request/url_re... net/url_request/url_request_job.cc:119: // SourceStreams own the previous SourceStream in the chain, but the ultimate On 2016/08/09 20:28:31, Randy Smith - Not in Fridays wrote: > nit, suggested rephrasing: "Each SourceStream owns the previous SourceStream > ...". Done. https://codereview.chromium.org/1662763002/diff/640001/net/url_request/url_re... net/url_request/url_request_job.cc:121: // is a proxy for URLRequestJob that is owned by the first filter (in dataflow On 2016/08/09 20:28:30, Randy Smith - Not in Fridays wrote: > nit, suggestion: " ... first stream (in ..."? Done. https://codereview.chromium.org/1662763002/diff/640001/net/url_request/url_re... net/url_request/url_request_job.cc:141: URLRequestJob* const job_; On 2016/08/09 20:28:31, Randy Smith - Not in Fridays wrote: > nit: Comment on why it's safe to keep a raw pointer. Done. https://codereview.chromium.org/1662763002/diff/640001/net/url_request/url_re... net/url_request/url_request_job.cc:206: *bytes_read = result; On 2016/08/09 20:28:31, Randy Smith - Not in Fridays wrote: > nitty nit, suggestion: I'd find the grouping in this function slightly more > intuitive if this if was at the end next to the "return result >= OK", since > this is setting an output parameter for the function which is basically a return > through the arguments list. Up to you, though. Done. https://codereview.chromium.org/1662763002/diff/640001/net/url_request/url_re... net/url_request/url_request_job.cc:217: void URLRequestJob::SourceReadComplete(bool synchronous, int result) { On 2016/08/09 20:28:30, Randy Smith - Not in Fridays wrote: > nit, suggestion: Change name to "StreamReadComplete" or > "SourceStreamReadComplete" just to be very clear how the function is used? Done. https://codereview.chromium.org/1662763002/diff/640001/net/url_request/url_re... net/url_request/url_request_job.cc:575: void URLRequestJob::ReadRawDataComplete(int result) { On 2016/08/09 20:28:30, Randy Smith - Not in Fridays wrote: > nit, suggestion: Name this function in parallel fashion to "SourceReadComplete" > (whatever name you settle on for that; see other comment). These are each > functions that are passed as callbacks or called directly in the synchronous > case for different Reads from this code, and I'm inclined to think that making > that parallelism clear in the name would be useful. Up to you, though. Acknowledged. Let's do it in a follow-up? This one is used by all sorts of sub classes of URLRequestJob. https://codereview.chromium.org/1662763002/diff/640001/net/url_request/url_re... net/url_request/url_request_job.cc:748: GatherRawReadStats(result); On 2016/08/09 20:28:31, Randy Smith - Not in Fridays wrote: > Would you be willing to put in a TODO() to cleanup the interface with the > subclasses? I'm specifically feeling like what we're ideally like is to have > ReadRawData() take a callback, which would be a wrap of ReadRawDataComplete() > around the callback, as opposed to hard-coding the subclasses to call > ReadRawDataComplete(). If you don't agree, skip the TODO; figuring out the > right design to fix that wart is beyond the scope of this CL. But if you do > agree, I wouldn't mind leaving a breadcrumb about fixing it to come back to. Done. https://codereview.chromium.org/1662763002/diff/640001/net/url_request/url_re... net/url_request/url_request_job.cc:761: void URLRequestJob::GatherRawReadStats(int bytes_read) { On 2016/08/09 20:28:30, Randy Smith - Not in Fridays wrote: > nit, suggestion: This routine is a bit misleadingly named; it mostly gathers the > read statistics, but it also nulls out raw_read_buffer_, so there's a way in > which what is it is just the stuff that needs to always be called (synchronously > or asynchronously) on completion of ReadRawData. Maybe change > ReadRawDataComplete->ReadRawDataCallback, and change this to > ReadRawDataComplete? (Up to you.) Acknowledged. ReadRawDataComplete is used by all sorts of subclasses. Let's handle the renaming in a follow-up. https://codereview.chromium.org/1662763002/diff/640001/net/url_request/url_re... File net/url_request/url_request_job.h (right): https://codereview.chromium.org/1662763002/diff/640001/net/url_request/url_re... net/url_request/url_request_job.h:249: void SourceReadComplete(bool synchronous, int result); On 2016/08/09 20:28:31, Randy Smith - Not in Fridays wrote: > Why is this protected rather than private? Done.
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Hrm...For landing this, should we land one SourceStream class at a time, and then the CL that hooks it all up? Reverting and relanding such a bit CL seems problematic.
Thanks very much for the review! I've uploaded a new patch which changed the terminology of previous/next to upstream/downstream that is suggested by rdsmith@. I agree the comments are easier to understand with the new name. Thanks Randy for the suggestions. I will create separate CLs to break up this change per mmenke@'s suggestion.
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Patchset #21 (id:710001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
Responses to comments here; now moving to review 2251853002, which is presumably where the action is :-}. https://codereview.chromium.org/1662763002/diff/640001/net/filter/filter_sour... File net/filter/filter_source_stream.cc (right): https://codereview.chromium.org/1662763002/diff/640001/net/filter/filter_sour... net/filter/filter_source_stream.cc:39: input_buffer_ = new IOBufferWithSize(kBufferSize); On 2016/08/15 15:19:07, xunjieli(OOO-8.21-8.26) wrote: > On 2016/08/09 20:28:30, Randy Smith - Not in Fridays wrote: > > Why not in the constructor? > > This is so that we don't initialize one unnecessarily. For instance, if caller > cancels the request before reading the response body. There's no need to > allocate a 32kB buffer in that case. Hmmm. Ok. My shoot-from-the-hip is that it's a premature optimization (i.e. that that case won't occur often enough to be a real issue). But I don't feel strongly. Put in a comment, though? https://codereview.chromium.org/1662763002/diff/640001/net/filter/filter_sour... net/filter/filter_source_stream.cc:148: next_state_ = STATE_READ_DATA; On 2016/08/15 15:19:07, xunjieli(OOO-8.21-8.26) wrote: > On 2016/08/09 20:28:30, Randy Smith - Not in Fridays wrote: > > Would an assertion that drainable_input_buffer_ has nothing left in it be > > appropriate here? > > DoReadData() already has a DCHECK. Acknowledged. https://codereview.chromium.org/1662763002/diff/640001/net/url_request/url_re... File net/url_request/url_request_job.cc (right): https://codereview.chromium.org/1662763002/diff/640001/net/url_request/url_re... net/url_request/url_request_job.cc:575: void URLRequestJob::ReadRawDataComplete(int result) { On 2016/08/15 15:19:08, xunjieli(OOO-8.21-8.26) wrote: > On 2016/08/09 20:28:30, Randy Smith - Not in Fridays wrote: > > nit, suggestion: Name this function in parallel fashion to > "SourceReadComplete" > > (whatever name you settle on for that; see other comment). These are each > > functions that are passed as callbacks or called directly in the synchronous > > case for different Reads from this code, and I'm inclined to think that making > > that parallelism clear in the name would be useful. Up to you, though. > > Acknowledged. Let's do it in a follow-up? This one is used by all sorts of sub > classes of URLRequestJob. Sure, that's fine. Let's keep a list of followup ideas somewhere, though. It's fine if we choose not to do them, and basically fine if we don't get to them, but I'd hate to *forget* them :-}.
Followup on naming, on this CL since it's where we were having the original discussion. https://codereview.chromium.org/1662763002/diff/640001/net/filter/source_stre... File net/filter/source_stream.h (right): https://codereview.chromium.org/1662763002/diff/640001/net/filter/source_stre... net/filter/source_stream.h:53: virtual std::string OrderedTypeStringList() const; On 2016/08/15 15:19:08, xunjieli(OOO-8.21-8.26) wrote: > On 2016/08/09 20:28:30, Randy Smith - Not in Fridays wrote: > > Why is this named OrderedTypeStringList()? I'm not sure what that name means. > > This is moved over from the original code. > The original name is: > > std::string OrderedFilterList() const; > > https://cs.chromium.org/chromium/src/net/filter/filter.h?rcl=0&l=238 Yeah, but that code was more explicit about filters being in a chain. This code is being a little more abstract; at each level the SourceStream looks opaque to what calls it; it's just a producer of bytes. And this is really nothing more than a string that describes the stream. So I'd vote in favor of just calling it "Description()" and commenting that this is for UMA and net logging. ETA: Actually, it looks like it's used for testing as well (which scares me a bit, but I'm not going to worry about that for now).
Description was changed from ========== Implement pull-based design for content decoding This CL implements a pull-based design for content decoding. This CL is based on an earlier prototype CL (crrev.com/1274863002 by ellyjones@). The design for this pull-based content decoding is proposed in crbug.com/474858. BUG=474859 ========== to ========== [ON HOLD] Implement pull-based design for content decoding This CL implements a pull-based design for content decoding. This CL is based on an earlier prototype CL (crrev.com/1274863002 by ellyjones@). The design for this pull-based content decoding is proposed in crbug.com/474858. BUG=474859 ========== |
