Switch to use net::FilterSourceStream from net::Filter

Switch to use net::FilterSourceStream from net::Filter

This CL switches filter implementation from net::Filter
to the new pull-based interface, net::FilterSourceStream.

BUG=474859
Committed: https://crrev.com/28a187760807a40049084627465be82a5d1e3118
Cr-Commit-Position: refs/heads/master@{#427358}

[xunjieli, 2016-09-27 14:45:03]

Patchset #3 (id:40001) has been deleted

[xunjieli, 2016-09-27 14:45:52]

Patchset #2 (id:20001) has been deleted

[xunjieli, 2016-09-27 14:48:53]

The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-27 14:49:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xunjieli, 2016-09-27 14:50:47]

Description was changed from

==========
Switch to use net::FilterSourceStream from net::Filter

BUG=
==========

to

==========
Switch to use net::FilterSourceStream from net::Filter

This CL switches filter implementation from net::Filter
to the new pull-based interface, net::FilterSourceStream.

BUG=474859
==========

[commit-bot: I haz the power, 2016-09-27 15:09:54]

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

[commit-bot: I haz the power, 2016-09-27 15:09:55]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[xunjieli, 2016-09-27 16:38:37]

The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-27 16:38:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-27 17:25:52]

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

[commit-bot: I haz the power, 2016-09-27 17:25:53]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[xunjieli, 2016-09-27 17:56:17]

xunjieli@chromium.org changed reviewers:
+ mmenke@chromium.org

[xunjieli, 2016-09-27 17:56:18]

Matt, The Gzip and Sdch filters are in review. I thought I'd send this out in
the meantime so you have more time to review. Thanks!

[mmenke, 2016-09-27 19:13:22]

This is looking pretty good to me, don't think it's going to need too many
changes before landing.

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

https://codereview.chromium.org/2373003003/diff/80001/net/url_request/url_req...
net/url_request/url_request_http_job.cc:1162: }
This silently skips over unknown types, but keeps other known filters around. 
The old code did nothing whenever there was one or more unknown types.  So with
this "foo, gzip" is treated like "gzip", while before it was treated like "". 
Think we should keep the old behavior, or now (We could also fail in that case,
but think it's too big a change for this CL).  Please add a test for this.

https://codereview.chromium.org/2373003003/diff/80001/net/url_request/url_req...
File net/url_request/url_request_job.cc (right):

https://codereview.chromium.org/2373003003/diff/80001/net/url_request/url_req...
net/url_request/url_request_job.cc:139: std::string Description() const override
{ return ""; }
std::string() is preferred.  A naive compiler would turn this into
std::string(""), which is potentially fair bit more complicated than a
constructor that only returns empty strings.

https://codereview.chromium.org/2373003003/diff/80001/net/url_request/url_req...
net/url_request/url_request_job.cc:494: if (source_stream_ == nullptr) {
!source_stream_?  You're DCHECKing using that just above.

https://codereview.chromium.org/2373003003/diff/80001/net/url_request/url_req...
net/url_request/url_request_job.cc:693: // result == 0
May want to make this a DCHECK instead of a comment.

https://codereview.chromium.org/2373003003/diff/80001/net/url_request/url_req...
net/url_request/url_request_job.cc:734: if (request()->net_log().IsCapturing())
{
Hrm...This no longer has the filter check, so when there's no filter event,
we'll display the same bytes twice.  Doesn't seem like the end of the world if
we do that, but could just check source_stream_->type() !=
SourceStream::TYPE_NONE.

[xunjieli, 2016-09-27 19:31:22]

Patchset #4 (id:100001) has been deleted

[xunjieli, 2016-09-27 19:50:11]

The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run

[xunjieli, 2016-09-27 19:50:30]

Thanks for the review! PTAL.

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

https://codereview.chromium.org/2373003003/diff/80001/net/url_request/url_req...
net/url_request/url_request_http_job.cc:1162: }
On 2016/09/27 19:13:22, mmenke wrote:
> This silently skips over unknown types, but keeps other known filters around. 
> The old code did nothing whenever there was one or more unknown types.  So
with
> this "foo, gzip" is treated like "gzip", while before it was treated like "". 
> Think we should keep the old behavior, or now (We could also fail in that
case,
> but think it's too big a change for this CL).  Please add a test for this.

Done.

https://codereview.chromium.org/2373003003/diff/80001/net/url_request/url_req...
File net/url_request/url_request_job.cc (right):

https://codereview.chromium.org/2373003003/diff/80001/net/url_request/url_req...
net/url_request/url_request_job.cc:139: std::string Description() const override
{ return ""; }
On 2016/09/27 19:13:22, mmenke wrote:
> std::string() is preferred.  A naive compiler would turn this into
> std::string(""), which is potentially fair bit more complicated than a
> constructor that only returns empty strings.

Done.

https://codereview.chromium.org/2373003003/diff/80001/net/url_request/url_req...
net/url_request/url_request_job.cc:494: if (source_stream_ == nullptr) {
On 2016/09/27 19:13:22, mmenke wrote:
> !source_stream_?  You're DCHECKing using that just above.

Done.

https://codereview.chromium.org/2373003003/diff/80001/net/url_request/url_req...
net/url_request/url_request_job.cc:693: // result == 0
On 2016/09/27 19:13:22, mmenke wrote:
> May want to make this a DCHECK instead of a comment.

Done.

https://codereview.chromium.org/2373003003/diff/80001/net/url_request/url_req...
net/url_request/url_request_job.cc:734: if (request()->net_log().IsCapturing())
{
On 2016/09/27 19:13:22, mmenke wrote:
> Hrm...This no longer has the filter check, so when there's no filter event,
> we'll display the same bytes twice.  Doesn't seem like the end of the world if
> we do that, but could just check source_stream_->type() !=
> SourceStream::TYPE_NONE.

Done. Hmm.. Should we get rid of this block then?
URLRequestJob::SourceStreamReadComplete will always be invoked. If there's no
filter, we will have both URL_REQUEST_JOB_BYTES_READ and
URL_REQUEST_JOB_FILTERED_BYTES_READ. We can keep it as it is and clean up in the
future?

[commit-bot: I haz the power, 2016-09-27 19:50:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mmenke, 2016-09-27 20:33:01]

Expect to sign off on this after another pass tomorrow.

https://codereview.chromium.org/2373003003/diff/80001/net/url_request/url_req...
File net/url_request/url_request_job.cc (right):

https://codereview.chromium.org/2373003003/diff/80001/net/url_request/url_req...
net/url_request/url_request_job.cc:734: if (request()->net_log().IsCapturing())
{
On 2016/09/27 19:50:30, xunjieli wrote:
> On 2016/09/27 19:13:22, mmenke wrote:
> > Hrm...This no longer has the filter check, so when there's no filter event,
> > we'll display the same bytes twice.  Doesn't seem like the end of the world
if
> > we do that, but could just check source_stream_->type() !=
> > SourceStream::TYPE_NONE.
> 
> Done. Hmm.. Should we get rid of this block then?
> URLRequestJob::SourceStreamReadComplete will always be invoked. If there's no
> filter, we will have both URL_REQUEST_JOB_BYTES_READ and
> URL_REQUEST_JOB_FILTERED_BYTES_READ. We can keep it as it is and clean up in
the
> future?

I think the intention was to log both the filtered and unfiltered bytes, when
there's a filter, for filter debugging?

[xunjieli, 2016-09-27 20:42:55]

xunjieli@chromium.org changed reviewers:
+ dbeam@chromium.org

[xunjieli, 2016-09-27 20:42:56]

dbeam@chromium.org: Please review changes in 
content/browser/webui/url_data_manager_backend.cc

mmenke@: Thanks, Matt. Please take your time. I am fixing up the SDCH filter
which will take me a day or two.

https://codereview.chromium.org/2373003003/diff/80001/net/url_request/url_req...
File net/url_request/url_request_job.cc (right):

https://codereview.chromium.org/2373003003/diff/80001/net/url_request/url_req...
net/url_request/url_request_job.cc:734: if (request()->net_log().IsCapturing())
{
On 2016/09/27 20:33:01, mmenke wrote:
> On 2016/09/27 19:50:30, xunjieli wrote:
> > On 2016/09/27 19:13:22, mmenke wrote:
> > > Hrm...This no longer has the filter check, so when there's no filter
event,
> > > we'll display the same bytes twice.  Doesn't seem like the end of the
world
> if
> > > we do that, but could just check source_stream_->type() !=
> > > SourceStream::TYPE_NONE.
> > 
> > Done. Hmm.. Should we get rid of this block then?
> > URLRequestJob::SourceStreamReadComplete will always be invoked. If there's
no
> > filter, we will have both URL_REQUEST_JOB_BYTES_READ and
> > URL_REQUEST_JOB_FILTERED_BYTES_READ. We can keep it as it is and clean up in
> the
> > future?
> 
> I think the intention was to log both the filtered and unfiltered bytes, when
> there's a filter, for filter debugging?

Ah, sorry. You are right. I am not what I was thinking.. When there is no
filter, we won't log twice because we just added the type() != NONE check.

[commit-bot: I haz the power, 2016-09-27 20:44:30]

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

[commit-bot: I haz the power, 2016-09-27 20:44:31]

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_...)

[Dan Beam, 2016-09-27 22:53:10]

lgtm w/nit

https://codereview.chromium.org/2373003003/diff/120001/content/browser/webui/...
File content/browser/webui/url_data_manager_backend.cc (right):

https://codereview.chromium.org/2373003003/diff/120001/content/browser/webui/...
content/browser/webui/url_data_manager_backend.cc:396:
std::unique_ptr<net::SourceStream> URLRequestChromeJob::SetUpSourceStream() {
can't this more simply be:

if (is_gzipped_)
  return nullptr;

... reset of code ...

?

[xunjieli, 2016-09-27 23:43:17]

Thanks!

https://codereview.chromium.org/2373003003/diff/120001/content/browser/webui/...
File content/browser/webui/url_data_manager_backend.cc (right):

https://codereview.chromium.org/2373003003/diff/120001/content/browser/webui/...
content/browser/webui/url_data_manager_backend.cc:396:
std::unique_ptr<net::SourceStream> URLRequestChromeJob::SetUpSourceStream() {
On 2016/09/27 22:53:10, Dan Beam wrote:
> can't this more simply be:
> 
> if (is_gzipped_)
>   return nullptr;
> 
> ... reset of code ...
> 
> ?

We can't return nullptr in the new interface. We need to return the result of
net::URLRequestJob::SetUpSourceStream().

[mmenke, 2016-09-28 19:52:54]

Is this CL expected to break the SDCH blacklist behavior?

[mmenke, 2016-09-28 20:24:28]

Just holding off on the L-G-T-M for the SDCH test issue.

https://codereview.chromium.org/2373003003/diff/120001/net/url_request/url_re...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/2373003003/diff/120001/net/url_request/url_re...
net/url_request/url_request_http_job.cc:1202: default:
Think it's best not to have a default case.  That will give us a compile error
if a new SourceStream::Type is added, but a new case is not added.

https://codereview.chromium.org/2373003003/diff/120001/net/url_request/url_re...
File net/url_request/url_request_http_job_unittest.cc (right):

https://codereview.chromium.org/2373003003/diff/120001/net/url_request/url_re...
net/url_request/url_request_http_job_unittest.cc:87: bool
use_null_source_stream_;
While you're here, could you add DISALLOW_COPY_AND_ASSIGN?

https://codereview.chromium.org/2373003003/diff/120001/net/url_request/url_re...
net/url_request/url_request_http_job_unittest.cc:91: protected:
Suggest making the constructor public.  Doesn't fix anything, it's just standard
practice.

https://codereview.chromium.org/2373003003/diff/120001/net/url_request/url_re...
net/url_request/url_request_http_job_unittest.cc:127: request->SetPriority(LOW);
I don't think we care about priority?

https://codereview.chromium.org/2373003003/diff/120001/net/url_request/url_re...
net/url_request/url_request_http_job_unittest.cc:131:
EXPECT_FALSE(request->status().is_success());
EXPECT_EQ(OK, delegate_.request_status());

(I want to get rid of URLRequest::status(), since it encourages unsafe usage
patterns.  See https://crbug.com/651119)

https://codereview.chromium.org/2373003003/diff/120001/net/url_request/url_re...
net/url_request/url_request_http_job_unittest.cc:153: request->SetPriority(LOW);
I don't think we care about priority?

https://codereview.chromium.org/2373003003/diff/120001/net/url_request/url_re...
net/url_request/url_request_http_job_unittest.cc:157:
EXPECT_TRUE(request->status().is_success());
Use delegate_.request_status() instead.

https://codereview.chromium.org/2373003003/diff/120001/net/url_request/url_re...
File net/url_request/url_request_job_unittest.cc (right):

https://codereview.chromium.org/2373003003/diff/120001/net/url_request/url_re...
net/url_request/url_request_job_unittest.cc:213:
EXPECT_FALSE(d.request_failed());
Hrm...Looks like there's no way to check if a request is completed in
TestDelegate.  That seems rather unfortunate, and surprising...  Maybe add one? 
Set a bool in OnResponseCompleted, and then make the method check something like
"request_status_ == net::OK && response_completed_"?  Admittedly, then we'd have
a ton of tests that could use it, but aren't.  :(

https://codereview.chromium.org/2373003003/diff/120001/net/url_request/url_re...
net/url_request/url_request_job_unittest.cc:561:
EXPECT_THAT(req->status().error(), IsError(ERR_CONTENT_DECODING_FAILED));
See commend in other file about status().

https://codereview.chromium.org/2373003003/diff/120001/net/url_request/url_re...
net/url_request/url_request_job_unittest.cc:565:
RemoveMockTransaction(&kInvalidContentGZip_Transaction);
I assume we have a test for this case for the brotli filter, at a lower level? 
If we have the lower level one, don't need one here, just checking.

[xunjieli, 2016-09-30 16:01:08]

The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 16:01:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-30 16:36:41]

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

[commit-bot: I haz the power, 2016-09-30 16:36:42]

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...)

[xunjieli, 2016-09-30 17:01:45]

The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run

[xunjieli, 2016-09-30 17:02:19]

Patchset #5 (id:140001) has been deleted

[commit-bot: I haz the power, 2016-09-30 17:03:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xunjieli, 2016-09-30 17:28:24]

Thanks! Bots should be happy now. There is a bug in the SdchSourceStream on
meta-refresh, I have since fixed it and rebased this CL.

https://codereview.chromium.org/2373003003/diff/120001/net/url_request/url_re...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/2373003003/diff/120001/net/url_request/url_re...
net/url_request/url_request_http_job.cc:1202: default:
On 2016/09/28 20:24:27, mmenke wrote:
> Think it's best not to have a default case.  That will give us a compile error
> if a new SourceStream::Type is added, but a new case is not added.

Done.

https://codereview.chromium.org/2373003003/diff/120001/net/url_request/url_re...
File net/url_request/url_request_http_job_unittest.cc (right):

https://codereview.chromium.org/2373003003/diff/120001/net/url_request/url_re...
net/url_request/url_request_http_job_unittest.cc:87: bool
use_null_source_stream_;
On 2016/09/28 20:24:28, mmenke wrote:
> While you're here, could you add DISALLOW_COPY_AND_ASSIGN?

Done.

https://codereview.chromium.org/2373003003/diff/120001/net/url_request/url_re...
net/url_request/url_request_http_job_unittest.cc:91: protected:
On 2016/09/28 20:24:27, mmenke wrote:
> Suggest making the constructor public.  Doesn't fix anything, it's just
standard
> practice.

Done.

https://codereview.chromium.org/2373003003/diff/120001/net/url_request/url_re...
net/url_request/url_request_http_job_unittest.cc:127: request->SetPriority(LOW);
On 2016/09/28 20:24:28, mmenke wrote:
> I don't think we care about priority?

Done.

https://codereview.chromium.org/2373003003/diff/120001/net/url_request/url_re...
net/url_request/url_request_http_job_unittest.cc:131:
EXPECT_FALSE(request->status().is_success());
On 2016/09/28 20:24:27, mmenke wrote:
> EXPECT_EQ(OK, delegate_.request_status());
> 
> (I want to get rid of URLRequest::status(), since it encourages unsafe usage
> patterns.  See https://crbug.com/651119)

Done.

https://codereview.chromium.org/2373003003/diff/120001/net/url_request/url_re...
net/url_request/url_request_http_job_unittest.cc:153: request->SetPriority(LOW);
On 2016/09/28 20:24:28, mmenke wrote:
> I don't think we care about priority?

Done.

https://codereview.chromium.org/2373003003/diff/120001/net/url_request/url_re...
net/url_request/url_request_http_job_unittest.cc:157:
EXPECT_TRUE(request->status().is_success());
On 2016/09/28 20:24:28, mmenke wrote:
> Use delegate_.request_status() instead.

Done.

https://codereview.chromium.org/2373003003/diff/120001/net/url_request/url_re...
File net/url_request/url_request_job_unittest.cc (right):

https://codereview.chromium.org/2373003003/diff/120001/net/url_request/url_re...
net/url_request/url_request_job_unittest.cc:213:
EXPECT_FALSE(d.request_failed());
On 2016/09/28 20:24:28, mmenke wrote:
> Hrm...Looks like there's no way to check if a request is completed in
> TestDelegate.  That seems rather unfortunate, and surprising...  Maybe add
one? 
> Set a bool in OnResponseCompleted, and then make the method check something
like
> "request_status_ == net::OK && response_completed_"?  Admittedly, then we'd
have
> a ton of tests that could use it, but aren't.  :(

Done.

https://codereview.chromium.org/2373003003/diff/120001/net/url_request/url_re...
net/url_request/url_request_job_unittest.cc:561:
EXPECT_THAT(req->status().error(), IsError(ERR_CONTENT_DECODING_FAILED));
On 2016/09/28 20:24:28, mmenke wrote:
> See commend in other file about status().

Done.

https://codereview.chromium.org/2373003003/diff/120001/net/url_request/url_re...
net/url_request/url_request_job_unittest.cc:565:
RemoveMockTransaction(&kInvalidContentGZip_Transaction);
On 2016/09/28 20:24:28, mmenke wrote:
> I assume we have a test for this case for the brotli filter, at a lower level?

> If we have the lower level one, don't need one here, just checking.

There is a BrotliSourceStreamTest.DecodeCorruptedData, so I think we have the
invalid case covered for brotli.

[commit-bot: I haz the power, 2016-09-30 17:47:19]

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

[commit-bot: I haz the power, 2016-09-30 17:47:20]

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, 2016-09-30 17:51:53]

On 2016/09/30 17:28:24, xunjieli wrote:
> Thanks! Bots should be happy now. There is a bug in the SdchSourceStream on
> meta-refresh, I have since fixed it and rebased this CL.

Sorry, looks like the test isn't fixed. It passed locally though. I will take a
look.

[xunjieli, 2016-09-30 18:20:43]

The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 18:21:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-30 21:03:53]

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

[commit-bot: I haz the power, 2016-09-30 21:03:54]

Dry run: This issue passed the CQ dry run.

[xunjieli, 2016-09-30 21:08:56]

Tests have passed. Thanks for the patience!

On Fri, Sep 30, 2016, 1:52 PM <xunjieli@chromium.org> wrote:

> On 2016/09/30 17:28:24, xunjieli wrote:
> > Thanks! Bots should be happy now. There is a bug in the SdchSourceStream
> on
> > meta-refresh, I have since fixed it and rebased this CL.
>
> Sorry, looks like the test isn't fixed. It passed locally though. I will
> take a
> look.
>
>
> https://codereview.chromium.org/2373003003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mmenke, 2016-09-30 21:55:24]

What's the difference between sets 5 and 6?

On 2016/09/30 21:08:56, xunjieli wrote:
> Tests have passed. Thanks for the patience!
> 
> On Fri, Sep 30, 2016, 1:52 PM <mailto:xunjieli@chromium.org> wrote:
> 
> > On 2016/09/30 17:28:24, xunjieli wrote:
> > > Thanks! Bots should be happy now. There is a bug in the SdchSourceStream
> > on
> > > meta-refresh, I have since fixed it and rebased this CL.
> >
> > Sorry, looks like the test isn't fixed. It passed locally though. I will
> > take a
> > look.
> >
> >
> > https://codereview.chromium.org/2373003003/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[xunjieli, 2016-10-01 13:12:02]

On 2016/09/30 21:55:24, mmenke wrote:
> What's the difference between sets 5 and 6?

Sorry, should have clarified. I fixed the meta refresh bug in the dependent CL.
There is a link at the end of the patchset which points to the dependent CL
patchset. PS#5 depends on "Issue 2368433002 Patch 120001"; PS#6 depends on
"Issue 2368433002 Patch 140001". I didn't make any changes in this CL between
PS#5 and PS#6 other than rebasing on top of the dependent CL
(crrev.com/2368433002).

[mmenke, 2016-10-03 15:40:04]

LGTM, great to see these CLs nearing completion!  Your new API is so much
better.

[xunjieli, 2016-10-05 13:45:56]

The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-05 13:46:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-05 13:48:41]

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

[commit-bot: I haz the power, 2016-10-05 13:48:44]

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-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[xunjieli, 2016-10-05 13:49:48]

Patchset #7 (id:200001) has been deleted

[xunjieli, 2016-10-05 14:09:53]

Patchset #7 (id:220001) has been deleted

[xunjieli, 2016-10-05 14:10:49]

The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run

[xunjieli, 2016-10-05 14:10:58]

Patchset #7 (id:240001) has been deleted

[commit-bot: I haz the power, 2016-10-05 14:11:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-05 15:23:46]

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

[commit-bot: I haz the power, 2016-10-05 15:23:47]

Dry run: This issue passed the CQ dry run.

[xunjieli, 2016-10-10 19:24:28]

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

[xunjieli, 2016-10-10 19:35:33]

The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-10 19:36:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xunjieli, 2016-10-10 19:36:28]

+rdsmith@:

Hi Randy, PTAL at diffs between PS#6 and PS#7. Thanks!

[commit-bot: I haz the power, 2016-10-10 20:43:33]

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

[commit-bot: I haz the power, 2016-10-10 20:43:34]

Dry run: This issue passed the CQ dry run.

[xunjieli, 2016-10-12 13:58:03]

The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-12 13:58:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xunjieli, 2016-10-12 14:09:13]

Patchset #9 (id:300001) has been deleted

[xunjieli, 2016-10-12 14:09:36]

Patchset #8 (id:280001) has been deleted

[xunjieli, 2016-10-12 14:10:03]

On 2016/10/10 19:36:28, xunjieli wrote:
> +rdsmith@:
> 
> Hi Randy, PTAL at diffs between PS#6 and PS#7. Thanks!

Hi Randy, it's actually diffs between PS#6 and PS#8 now. Thanks!

[commit-bot: I haz the power, 2016-10-12 15:10:58]

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

[commit-bot: I haz the power, 2016-10-12 15:10:59]

Dry run: This issue passed the CQ dry run.

[xunjieli, 2016-10-13 17:48:12]

The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-13 17:48:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xunjieli, 2016-10-13 17:49:05]

On 2016/10/12 14:10:03, xunjieli wrote:
> On 2016/10/10 19:36:28, xunjieli wrote:
> > +rdsmith@:
> > 
> > Hi Randy, PTAL at diffs between PS#6 and PS#7. Thanks!
> 
> Hi Randy, it's actually diffs between PS#6 and PS#8 now. Thanks!

Hi Randy, it's actually diffs between PS#6 and PS#9 now (with the
URLRequestHttpJob calling DestroySourceStream() change). PTAL. Thanks!

[commit-bot: I haz the power, 2016-10-13 19:05:04]

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

[commit-bot: I haz the power, 2016-10-13 19:05:05]

Dry run: This issue passed the CQ dry run.

[Randy Smith (Not in Mondays), 2016-10-13 22:06:37]

So I looked through this and didn't find any problems, but I was basically
reviewing it in parallel with the SDCH CL, so I'd like to hold off stamping
until that one is good (in case there are changes that cascade into this CL). 
But: No comments at this time, which presumably means very few in the future
:-}.

[xunjieli, 2016-10-14 18:46:42]

The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-14 18:47:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-14 18:51:30]

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

[commit-bot: I haz the power, 2016-10-14 18:51:31]

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-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Randy Smith (Not in Mondays), 2016-10-19 19:50:05]

LGTM.

[xunjieli, 2016-10-21 17:39:34]

The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-21 17:40:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-21 19:46:57]

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

[commit-bot: I haz the power, 2016-10-21 19:46:59]

Dry run: This issue passed the CQ dry run.

[mmenke, 2016-10-24 17:54:30]

[xunjieli]:  Planning on landing this today?

[xunjieli, 2016-10-24 17:56:44]

On 2016/10/24 17:54:30, mmenke wrote:
> [xunjieli]:  Planning on landing this today?

Ah, sorry, should have updated you. I found that there is a bug in SDCH filter,
and I uploaded a fix at https://codereview.chromium.org/2446443002/. I will wait
for Randy to review it. After that fix is landed, I think we are good to go.

[xunjieli, 2016-10-25 14:11:35]

The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-25 14:11:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xunjieli, 2016-10-25 14:38:39]

Thanks everyone for the review. I will be checking in this today. I did some
manual testing. Gzipped and Sdch encoded pages seem to be loading fine. We have
3 weeks before branch date, so I think there's still enough time for this to be
in Canary.

[xunjieli, 2016-10-25 14:38:52]

The CQ bit was unchecked by xunjieli@chromium.org

[xunjieli, 2016-10-25 14:39:05]

The CQ bit was checked by xunjieli@chromium.org

[xunjieli, 2016-10-25 14:39:06]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org,
mmenke@chromium.org, rdsmith@chromium.org
Link to the patchset: https://codereview.chromium.org/2373003003/#ps400001
(title: "rebased onto sdch fix")

[commit-bot: I haz the power, 2016-10-25 14:39:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-25 15:30:31]

Description was changed from

==========
Switch to use net::FilterSourceStream from net::Filter

This CL switches filter implementation from net::Filter
to the new pull-based interface, net::FilterSourceStream.

BUG=474859
==========

to

==========
Switch to use net::FilterSourceStream from net::Filter

This CL switches filter implementation from net::Filter
to the new pull-based interface, net::FilterSourceStream.

BUG=474859
==========

[commit-bot: I haz the power, 2016-10-25 15:30:37]

Committed patchset #12 (id:400001)

[commit-bot: I haz the power, 2016-10-25 15:43:52]

Description was changed from

==========
Switch to use net::FilterSourceStream from net::Filter

This CL switches filter implementation from net::Filter
to the new pull-based interface, net::FilterSourceStream.

BUG=474859
==========

to

==========
Switch to use net::FilterSourceStream from net::Filter

This CL switches filter implementation from net::Filter
to the new pull-based interface, net::FilterSourceStream.

BUG=474859

Committed: https://crrev.com/28a187760807a40049084627465be82a5d1e3118
Cr-Commit-Position: refs/heads/master@{#427358}
==========

[commit-bot: I haz the power, 2016-10-25 15:43:54]

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