Split up streams logic to prepare for PlzNavigate RDH changes.

Split up streams logic to prepare for PlzNavigate RDH changes.

This was split out from https://codereview.chromium.org/519533002/

StreamHandle is now split into a StreamHandle and StreamInfo. StreamHandle only
manages the stream URL. StreamInfo also contains the headers and other metadata
that the streams_private mechanism uses.

StreamResourceHandler is also split out into a StreamWriter so other resource
handlers may also use it.

BUG=376015
Committed: https://crrev.com/61814fd5bf4f1febc1c41b608dcbcd8a1e46e2c0
Cr-Commit-Position: refs/heads/master@{#299745}

[davidben, 2014-10-03 16:52:38]

davidben@chromium.org changed reviewers:
+ zork@chromium.org

[davidben, 2014-10-03 16:52:39]

This is the streams refactoring split out from the original CL.

https://codereview.chromium.org/625993002/diff/20001/content/browser/loader/s...
File content/browser/loader/stream_writer.h (right):

https://codereview.chromium.org/625993002/diff/20001/content/browser/loader/s...
content/browser/loader/stream_writer.h:53: // support BufferedResourceHandler
which reads the initial chunk of data
This is not necessary for StreamResourceHandler, but NavigationResourceHandler
will want this property.

[davidben, 2014-10-03 20:57:16]

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

[davidben, 2014-10-03 20:57:16]

+mmenke for main (re-)review. It appears zork is OOO for the next week. It's the
same as what was reviewed in the original CL though, minus some stray forward
decls and includes I forgot to remove.

[mmenke, 2014-10-03 21:03:18]

On 2014/10/03 20:57:16, David Benjamin wrote:
> +mmenke for main (re-)review. It appears zork is OOO for the next week. It's
the
> same as what was reviewed in the original CL though, minus some stray forward
> decls and includes I forgot to remove.

Great, I'll try to get to this Monday.  If I don't, you may have been just as
well off going with Zork.  :)

[mmenke, 2014-10-06 19:46:49]

On 2014/10/03 21:03:18, mmenke wrote:
> On 2014/10/03 20:57:16, David Benjamin wrote:
> > +mmenke for main (re-)review. It appears zork is OOO for the next week. It's
> the
> > same as what was reviewed in the original CL though, minus some stray
forward
> > decls and includes I forgot to remove.
> 
> Great, I'll try to get to this Monday.  If I don't, you may have been just as
> well off going with Zork.  :)

And I did not get to this today.  Next three days are the net stack meeting, and
Friday I have an interview and am heading out early (Parents arriving in town),
so looks like this may have to wait until next Monday.  :(

[mmenke, 2014-10-10 14:31:34]

On 2014/10/06 19:46:49, mmenke wrote:
> On 2014/10/03 21:03:18, mmenke wrote:
> > On 2014/10/03 20:57:16, David Benjamin wrote:
> > > +mmenke for main (re-)review. It appears zork is OOO for the next week.
It's
> > the
> > > same as what was reviewed in the original CL though, minus some stray
> forward
> > > decls and includes I forgot to remove.
> > 
> > Great, I'll try to get to this Monday.  If I don't, you may have been just
as
> > well off going with Zork.  :)
> 
> And I did not get to this today.  Next three days are the net stack meeting,
and
> Friday I have an interview and am heading out early (Parents arriving in
town),
> so looks like this may have to wait until next Monday.  :(

Forgot I'm taking first two days of next week off, so unless I manage to get to
this today....

[Zachary Kuznia, 2014-10-13 23:42:35]

https://chromiumcodereview.appspot.com/625993002/diff/310001/content/browser/...
File content/browser/loader/stream_writer.cc (right):

https://chromiumcodereview.appspot.com/625993002/diff/310001/content/browser/...
content/browser/loader/stream_writer.cc:17: StreamWriter::StreamWriter() {
Initialize controller_ to nullptr.

https://chromiumcodereview.appspot.com/625993002/diff/310001/content/browser/...
content/browser/loader/stream_writer.cc:38: int min_size) {
DCHECK_LE(min_size, kReadBufSize);

https://chromiumcodereview.appspot.com/625993002/diff/310001/content/browser/...
File content/browser/streams/stream_handle_impl.h (right):

https://chromiumcodereview.appspot.com/625993002/diff/310001/content/browser/...
content/browser/streams/stream_handle_impl.h:10: #include
"base/memory/ref_counted.h"
I think you can remove this line.

[davidben, 2014-10-14 18:37:15]

https://codereview.chromium.org/625993002/diff/310001/content/browser/loader/...
File content/browser/loader/stream_writer.cc (right):

https://codereview.chromium.org/625993002/diff/310001/content/browser/loader/...
content/browser/loader/stream_writer.cc:17: StreamWriter::StreamWriter() {
On 2014/10/13 23:42:35, Zachary Kuznia wrote:
> Initialize controller_ to nullptr.

Done.

https://codereview.chromium.org/625993002/diff/310001/content/browser/loader/...
content/browser/loader/stream_writer.cc:38: int min_size) {
On 2014/10/13 23:42:35, Zachary Kuznia wrote:
> DCHECK_LE(min_size, kReadBufSize);

Done.

https://codereview.chromium.org/625993002/diff/310001/content/browser/streams...
File content/browser/streams/stream_handle_impl.h (right):

https://codereview.chromium.org/625993002/diff/310001/content/browser/streams...
content/browser/streams/stream_handle_impl.h:10: #include
"base/memory/ref_counted.h"
On 2014/10/13 23:42:35, Zachary Kuznia wrote:
> I think you can remove this line.

Done. Also removed lock.h and the ResourceResponse forward decl. Remnant of
other changes.

[Zachary Kuznia, 2014-10-14 19:45:08]

lgtm

[davidben, 2014-10-14 19:48:37]

davidben@chromium.org changed reviewers:
+ jam@chromium.org

[davidben, 2014-10-14 19:48:38]

+jam for OWNERS. (chrome/browser/renderer_host is the missing one, but there's a
bunch of content/ stuff too.)

[jam, 2014-10-15 16:11:00]

On 2014/10/14 19:48:38, David Benjamin wrote:
> +jam for OWNERS. (chrome/browser/renderer_host is the missing one, 

lgtm

> but there's a bunch of content/ stuff too.)

you're an owner there so shouldn't be missing anything..

https://codereview.chromium.org/625993002/diff/410001/content/public/browser/...
File content/public/browser/stream_info.cc (right):

https://codereview.chromium.org/625993002/diff/410001/content/public/browser/...
content/public/browser/stream_info.cc:12: StreamInfo::StreamInfo() {}
nit: can you avoid this cc file and inline the constructor (shouldn't need a
d'tor I think) or will you get clang errors? we only add .cc files for structs
if clang complains

[davidben, 2014-10-15 16:12:29]

https://codereview.chromium.org/625993002/diff/410001/content/public/browser/...
File content/public/browser/stream_info.cc (right):

https://codereview.chromium.org/625993002/diff/410001/content/public/browser/...
content/public/browser/stream_info.cc:12: StreamInfo::StreamInfo() {}
On 2014/10/15 16:11:00, jam wrote:
> nit: can you avoid this cc file and inline the constructor (shouldn't need a
> d'tor I think) or will you get clang errors? we only add .cc files for structs
> if clang complains

The destructor isn't quite trivial because of the scoped_ptr and the
scoped_refptr. (Is that when clang complains?)

[davidben, 2014-10-15 19:28:54]

https://codereview.chromium.org/625993002/diff/410001/content/public/browser/...
File content/public/browser/stream_info.cc (right):

https://codereview.chromium.org/625993002/diff/410001/content/public/browser/...
content/public/browser/stream_info.cc:12: StreamInfo::StreamInfo() {}
On 2014/10/15 16:12:29, David Benjamin wrote:
> On 2014/10/15 16:11:00, jam wrote:
> > nit: can you avoid this cc file and inline the constructor (shouldn't need a
> > d'tor I think) or will you get clang errors? we only add .cc files for
structs
> > if clang complains
> 
> The destructor isn't quite trivial because of the scoped_ptr and the
> scoped_refptr. (Is that when clang complains?)

(Just checked with clang, and it does complain.)

[davidben, 2014-10-15 19:29:19]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2014-10-15 19:30:45]

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

[commit-bot: I haz the power, 2014-10-15 19:45:27]

Committed patchset #8 (id:410001)

[commit-bot: I haz the power, 2014-10-15 19:46:09]

Patchset 8 (id:??) landed as
https://crrev.com/61814fd5bf4f1febc1c41b608dcbcd8a1e46e2c0
Cr-Commit-Position: refs/heads/master@{#299745}

[mmenke, 2014-10-15 20:05:44]

Ah, well...guess it's not worth finishinh my review, now.  These were all pretty
minor.

https://codereview.chromium.org/625993002/diff/410001/content/browser/loader/...
File content/browser/loader/stream_resource_handler.h (right):

https://codereview.chromium.org/625993002/diff/410001/content/browser/loader/...
content/browser/loader/stream_resource_handler.h:21: // Redirect this resource
to a stream.
stream -> Stream

https://codereview.chromium.org/625993002/diff/410001/content/browser/loader/...
content/browser/loader/stream_resource_handler.h:21: // Redirect this resource
to a stream.
Maybe "resource" -> "resource response"?

Admittedly, the response is kinda implied by the fact this is a ResourceHandler,
but makes things a little clearer.

https://codereview.chromium.org/625993002/diff/410001/content/browser/loader/...
content/browser/loader/stream_resource_handler.h:67:
DISALLOW_COPY_AND_ASSIGN(StreamResourceHandler);
While you're here...  include base/macros.h?

https://codereview.chromium.org/625993002/diff/410001/content/browser/loader/...
File content/browser/loader/stream_writer.cc (right):

https://codereview.chromium.org/625993002/diff/410001/content/browser/loader/...
content/browser/loader/stream_writer.cc:41: DCHECK(buf);
include base/logging.h for DCHECK

https://codereview.chromium.org/625993002/diff/410001/content/browser/loader/...
content/browser/loader/stream_writer.cc:54: // We have more data to read.
While you're here, shouldn't use "we" in comments.

https://codereview.chromium.org/625993002/diff/410001/content/browser/loader/...
File content/browser/loader/stream_writer.h (right):

https://codereview.chromium.org/625993002/diff/410001/content/browser/loader/...
content/browser/loader/stream_writer.h:27: class StreamWriter : public
StreamWriteObserver {
Hrm...  This naming is really unfortunate.  Sounds like it watches itself... 
And then the Stream has a writer_ member, which is not this, but what the stream
writes to.  Making this more of a Stream's ByteStreamWriter Writer...Somehow,
though, StreamByteStreamWriterWriter doesn't seem like much of an improvement.

https://codereview.chromium.org/625993002/diff/410001/content/browser/loader/...
content/browser/loader/stream_writer.h:45: const GURL& origin);
nit:  Do these fit on one line?

https://codereview.chromium.org/625993002/diff/410001/content/browser/loader/...
content/browser/loader/stream_writer.h:72: virtual void OnClose(Stream* stream)
override;
I think the new hotness is to use override without the virtual?

https://codereview.chromium.org/625993002/diff/410001/content/public/browser/...
File content/public/browser/stream_info.h (right):

https://codereview.chromium.org/625993002/diff/410001/content/public/browser/...
content/public/browser/stream_info.h:7: 
#include <string>

https://codereview.chromium.org/625993002/diff/410001/content/public/browser/...
content/public/browser/stream_info.h:42: DISALLOW_COPY_AND_ASSIGN(StreamInfo);
DISALLOW_COPY_AND_ASSIGN should be private.

[davidben, 2014-10-15 20:10:13]

Oops. I'll resolve your comments in a follow-up.

[davidben, 2014-10-15 22:56:24]

On 2014/10/15 20:10:13, David Benjamin wrote:
> Oops. I'll resolve your comments in a follow-up.

...except I'm going to be out Thursday and Friday. I'll do this Monday.