Make ByteStream independent from DownloadInterruptReason

Make ByteStream independent from DownloadInterruptReason

Change status type to int to make it portable to other component.

After this change, ByteStream could be moved under src/net/base/ or
src/base directory.

- Comment fix (MaybePostToPeer -> Write)

BUG=169957
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=215265

[sky, 2013-07-15 18:02:18]

I'm not too familiar with this code. Is there someone else you can get to review
that knows it better?

[tyoshino (SeeGerritForStatus), 2013-07-17 03:50:03]

+rdsmith as a main reviewer

[Randy Smith (Not in Mondays), 2013-07-17 11:02:20]

I'm OOO today, but I would like to review this code; I'll try to get to it
tonight or early tomorrow morning. 

One question (without looking at the code): When I looked at this issue (when I
was originally writing this code) the option I was considering was to make
ByteStream a template class on the type of the error variable (I was going to
call it "out of line information" or something like that).  I didn't go that
direction because it didn't have any other customers than downloads and I didn't
want to do the added complexity on spec, but now that there's some interest
maybe that's a direction worth looking at?

[tyoshino (SeeGerritForStatus), 2013-07-17 18:39:40]

On 2013/07/17 11:02:20, rdsmith wrote:
> One question (without looking at the code): When I looked at this issue (when
I
> was originally writing this code) the option I was considering was to make
> ByteStream a template class on the type of the error variable (I was going to
> call it "out of line information" or something like that).  I didn't go that
> direction because it didn't have any other customers than downloads and I
didn't
> want to do the added complexity on spec, but now that there's some interest
> maybe that's a direction worth looking at?

I also considered that option. I haven't tried but it seemed to need a bit big
work since
implementation is hidden inside .cc file. Maybe we could separate code not
involving
status code change to ByteStream.*Base classes and then derive template classes.

For my use case, we just need binary information, success or fail. So, I'm fine
with
int.

[Randy Smith (Not in Mondays), 2013-07-18 16:09:43]

On 2013/07/17 18:39:40, tyoshino wrote:
> On 2013/07/17 11:02:20, rdsmith wrote:
> > One question (without looking at the code): When I looked at this issue
(when
> I
> > was originally writing this code) the option I was considering was to make
> > ByteStream a template class on the type of the error variable (I was going
to
> > call it "out of line information" or something like that).  I didn't go that
> > direction because it didn't have any other customers than downloads and I
> didn't
> > want to do the added complexity on spec, but now that there's some interest
> > maybe that's a direction worth looking at?
> 
> I also considered that option. I haven't tried but it seemed to need a bit big
> work since
> implementation is hidden inside .cc file. Maybe we could separate code not
> involving
> status code change to ByteStream.*Base classes and then derive template
classes.
> 
> For my use case, we just need binary information, success or fail. So, I'm
fine
> with
> int.

I apologize for the persnicketiness, but I don't think I am.   I think it likely
that the two ends of the ByteStream will be generally be used in completely
separate code modules, and so anyone who uses ByteStream will have to (via
comments and education) keep those two separate ends in sync.  And that won't be
very easy.  If we write it as a template class, the compiler will do that job
for us, for all future implementations.  And I don't think it'll be "that hard"
(I know, famous last words, especially since I'm asking you to do it) to change
it over to a template class.  I'm good with the *Base class idea (as long as any
casting is kept hidden from the consumer), or simply with thrown the
sledgehammer at it and templatizing everything, substituting
DownloadInterruptReason -> T everywhere.  

Extra added bonus would be to make the only requirement on T be that it be
copyable (i.e. not require that it be castable to/from int) so that the consumer
could use it as a general out of band end-of-transmission information transfer. 
(I'm not requiring this, just putting it up as a "would be nice".  If int
castability is required, I do want it to fail at compile time if T can't be
casted to an int.)

If you think I'm being unreasonable, I'm happy to pull in one of the folks (Will
Chan, or (I think, but need to check, Fred Akalin) who supervised my initial
implementation and defer to their opinion.  But I suspect they'll agree with me.

In terms of destination, I think base/ is the best place (though we'll need to
pull in a base/ reviewer), but net/base would be easier to get it into (since
I'm a reviewer for that :-}).

[tyoshino (SeeGerritForStatus), 2013-07-25 07:25:05]

On 2013/07/18 16:09:43, rdsmith wrote:
...
> very easy.  If we write it as a template class, the compiler will do that job
> for us, for all future implementations.  And I don't think it'll be "that
hard"
> (I know, famous last words, especially since I'm asking you to do it) to
change

Yes. That's a big benefit but also making templatizing hard.
Since TransferData touches lifetimeflag, etc, to templatize status, it's sad but
we need to move LifetimeFlag, ContentVector, ... to .h file.

> it over to a template class.  I'm good with the *Base class idea (as long as
any
> casting is kept hidden from the consumer), or simply with thrown the
> sledgehammer at it and templatizing everything, substituting
> DownloadInterruptReason -> T everywhere.  

I tried Base class idea, but sorry it was also not feasible. I'm gonna move lots
of
stuff to .h file.

> 
> Extra added bonus would be to make the only requirement on T be that it be
> copyable (i.e. not require that it be castable to/from int) so that the
consumer
> could use it as a general out of band end-of-transmission information
transfer. 
> (I'm not requiring this, just putting it up as a "would be nice".  If int
> castability is required, I do want it to fail at compile time if T can't be
> casted to an int.)
> 
> If you think I'm being unreasonable, I'm happy to pull in one of the folks
(Will
> Chan, or (I think, but need to check, Fred Akalin) who supervised my initial
> implementation and defer to their opinion.  But I suspect they'll agree with
me.
> 
> In terms of destination, I think base/ is the best place (though we'll need to
> pull in a base/ reviewer), but net/base would be easier to get it into (since
> I'm a reviewer for that :-}).

I thought so but needs to find an alternative for IOBuffer used in it.

[tyoshino (SeeGerritForStatus), 2013-07-26 05:31:13]

It looks like we need to put most of implementation in .h file. I tried
separating Status independent code as much as possible, but it's hard. Creating
a base class in .h also doesn't help much. Are you fine with this approach?

[tyoshino (SeeGerritForStatus), 2013-07-29 07:18:41]

Done. It's not templatized.

-sky +jam for OWNERS review as I've made some changes out of content/browser.

PTAL, rdsmith, please.

[tyoshino (SeeGerritForStatus), 2013-07-29 07:19:08]

On 2013/07/29 07:18:41, tyoshino wrote:
> Done. It's not templatized.

s/not/now/

[tyoshino (SeeGerritForStatus), 2013-07-29 13:56:04]

It's good if we could enclose implementation details in unnamed namespace, but
it's prohibited by the style guide.

[jam, 2013-07-29 16:22:28]

The plan is to get rid of src/webkit. Everything there is moving to content.

Why do you want to move this?

[Randy Smith (Not in Mondays), 2013-07-29 18:01:10]

I'll do a full review in the next day or two, dependent on the results of your
discussion with jam@ (specifically, I'd love to see the templatization change,
but if the code you want to use this from is being moved to content, you
probably don't need this CL at all).  

One quick note: If we want to get rid of IOBuffer, we can.  The only thing
standing in the way of just making that a scoped pointer to an array of bytes
was issue 131272, which has been fixed.  So I think we could switch this code
over to using a more generic type if we wanted.

[tyoshino (SeeGerritForStatus), 2013-07-30 05:12:49]

Moving src/webkit to content likely to happen soon.

But regardless of the plan, I think it's useful to templatize it. It's not
content specific code by nature.

[jam, 2013-07-30 15:54:46]

On 2013/07/30 05:12:49, tyoshino wrote:
> Moving src/webkit to content likely to happen soon.
> 
> But regardless of the plan, I think it's useful to templatize it. It's not
> content specific code by nature.

What's the reason for templatizing it? Can you provide more background?

[Randy Smith (Not in Mondays), 2013-07-30 16:20:04]

On 2013/07/30 15:54:46, jam wrote:
> On 2013/07/30 05:12:49, tyoshino wrote:
> > Moving src/webkit to content likely to happen soon.
> > 
> > But regardless of the plan, I think it's useful to templatize it. It's not
> > content specific code by nature.
> 
> What's the reason for templatizing it? Can you provide more background?

John: As previously written, it's downloads specific, but in a very narrow way:
It transmits a DownloadInterruptReason as an error code.  It's a noticeably more
general class if that download dependency can be removed.  I had thought about
templatizing it when I first wrote it, but mission creep :-}.  tyoshino@ wants
to use it for a non-downloads purpose (into which I have not probed heavily). 
He and I discussed changing DownloadInterruptReason over to an int, but I was
opposed to that idea because in general the two ends of a ByteStream are going
to be in completely different sections of the code, so I thought having compiler
support for making sure the error code was used the same on both ends had some
value.  And templatizing will also allow more complex data than just 32 bits to
be sent on stream completion.  tyoshino@ was kind enough to humor me and do the
templatization.

[Randy Smith (Not in Mondays), 2013-07-30 17:43:59]

In doing my first quick read-through of this, I realize that I don't have
confidence in my ability to properly format and layout template based classes. 
So I'll want to pull in someone to critique that aspect of this change (ajwong@
would be my default, just because I know he did Bind/Callback/etc. and hence
must be steeped in this stuff).  But we probably want to postpone doing that
after we've finished the discussion with John about to template or not to
template :-}.

In the meantime, let me ask: Did you consider using a -inl.h file (See
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#The_-inl.h_Files
) for the guts of the template implementation?  That would be a way to at least
visually separate the implementation from the interface.

https://codereview.chromium.org/18284005/diff/29022/content/browser/byte_stre...
File content/browser/byte_stream.h (left):

https://codereview.chromium.org/18284005/diff/29022/content/browser/byte_stre...
content/browser/byte_stream.h:187: CONTENT_EXPORT void CreateByteStream(
I'm surprised that removing CONTENT_EXPORT works; I thought that was needed for
test linking.

https://codereview.chromium.org/18284005/diff/29022/content/browser/byte_stre...
File content/browser/byte_stream.h (right):

https://codereview.chromium.org/18284005/diff/29022/content/browser/byte_stre...
content/browser/byte_stream.h:38: // are indicated to the sink via the
GetStatus() method.
Suggestion: Change "Errors" to "Operation status"; GetStatus() can signal
success as well as failure.

https://codereview.chromium.org/18284005/diff/29022/content/browser/byte_stre...
content/browser/byte_stream.h:66: //      scoped_ptr<ByteStreamWriter<status
type> > writer;
Could you change this to StatusType?  My eye keeps tripping as I read the code,
because I keep expecting the whitespace to be significant and I can't parse the
code if it is.

https://codereview.chromium.org/18284005/diff/29022/content/browser/byte_stre...
content/browser/byte_stream.h:124: template <typename StatusType>
We need to document the requirements on StatusType (POD?  Requires copy
constructor and assignment operator?) somewhere.

[tyoshino (SeeGerritForStatus), 2013-07-30 18:55:59]

https://codereview.chromium.org/18284005/diff/29022/content/browser/byte_stre...
File content/browser/byte_stream.h (left):

https://codereview.chromium.org/18284005/diff/29022/content/browser/byte_stre...
content/browser/byte_stream.h:187: CONTENT_EXPORT void CreateByteStream(
On 2013/07/30 17:43:59, rdsmith wrote:
> I'm surprised that removing CONTENT_EXPORT works; I thought that was needed
for
> test linking.

As we have implementation in .h file (to instantiate implementation of template
functions/classes), each object file will contain implementation in themselves
(with weak symbol). So, we don't have to instruct compilers to export and import
symbol. If we use CONTENT_EXPORT, it's marked with dllimport in non-content
object files, but we don't instantiate corresponding implementations with
dllexport annotation. So, DLL fails to resolve it.

https://codereview.chromium.org/18284005/diff/29022/content/browser/byte_stre...
File content/browser/byte_stream.h (right):

https://codereview.chromium.org/18284005/diff/29022/content/browser/byte_stre...
content/browser/byte_stream.h:38: // are indicated to the sink via the
GetStatus() method.
On 2013/07/30 17:43:59, rdsmith wrote:
> Suggestion: Change "Errors" to "Operation status"; GetStatus() can signal
> success as well as failure.

Done.

https://codereview.chromium.org/18284005/diff/29022/content/browser/byte_stre...
content/browser/byte_stream.h:66: //      scoped_ptr<ByteStreamWriter<status
type> > writer;
On 2013/07/30 17:43:59, rdsmith wrote:
> Could you change this to StatusType?  My eye keeps tripping as I read the
code,
> because I keep expecting the whitespace to be significant and I can't parse
the
> code if it is.

Done.

https://codereview.chromium.org/18284005/diff/29022/content/browser/byte_stre...
content/browser/byte_stream.h:124: template <typename StatusType>
On 2013/07/30 17:43:59, rdsmith wrote:
> We need to document the requirements on StatusType (POD?  Requires copy
> constructor and assignment operator?) somewhere.

Added around L38

[tyoshino (SeeGerritForStatus), 2013-07-30 19:01:17]

On 2013/07/30 17:43:59, rdsmith wrote:
> In doing my first quick read-through of this, I realize that I don't have
> confidence in my ability to properly format and layout template based classes.

> So I'll want to pull in someone to critique that aspect of this change
(ajwong@
> would be my default, just because I know he did Bind/Callback/etc. and hence
> must be steeped in this stuff).  But we probably want to postpone doing that
> after we've finished the discussion with John about to template or not to
> template :-}.
> 
> In the meantime, let me ask: Did you consider using a -inl.h file (See
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#The_-inl.h_Files
> ) for the guts of the template implementation?  That would be a way to at
least
> visually separate the implementation from the interface.

It seems that use of -inl.h is not common in Chromium. But maybe it could
improve readability. I'd like to consult ajwong@ about this point, too (and,
also whether we should use "namespace internal" or something to isolate impl
details).

[jam, 2013-07-31 15:05:12]

On 2013/07/30 16:20:04, rdsmith wrote:
> On 2013/07/30 15:54:46, jam wrote:
> > On 2013/07/30 05:12:49, tyoshino wrote:
> > > Moving src/webkit to content likely to happen soon.
> > > 
> > > But regardless of the plan, I think it's useful to templatize it. It's not
> > > content specific code by nature.
> > 
> > What's the reason for templatizing it? Can you provide more background?
> 
> John: As previously written, it's downloads specific, but in a very narrow
way:
> It transmits a DownloadInterruptReason as an error code.  It's a noticeably
more
> general class if that download dependency can be removed.  I had thought about
> templatizing it when I first wrote it, but mission creep :-}.  tyoshino@ wants
> to use it for a non-downloads purpose (into which I have not probed heavily). 
> He and I discussed changing DownloadInterruptReason over to an int, but I was
> opposed to that idea because in general the two ends of a ByteStream are going
> to be in completely different sections of the code, so I thought having
compiler
> support for making sure the error code was used the same on both ends had some
> value.  And templatizing will also allow more complex data than just 32 bits
to
> be sent on stream completion.  tyoshino@ was kind enough to humor me and do
the
> templatization.

I see. When I look at the file with the templates
https://codereview.chromium.org/18284005/diff/48001/content/browser/byte_stre...
it seems that the readability has suffered because of this transition. that's
ignoring code blowups for having all this stuff in the header. i realize this
work has already been done, but it seems to me that an int would be quite a bit
more readable than a template

[Randy Smith (Not in Mondays), 2013-07-31 15:24:59]

I'd very much prefer not using int, just because I care more about the consumer
code than the ByteStream code in terms of maintainability (as it's a utility
class).  I would think the readability would be about the same with a .h/-inl.h
split as it would with a .h/.cc split--can you say why you think it would suffer
in that mode?

[jam, 2013-07-31 17:07:17]

On 2013/07/31 15:24:59, rdsmith wrote:
> I'd very much prefer not using int, just because I care more about the
consumer
> code than the ByteStream code in terms of maintainability (as it's a utility
> class).  I would think the readability would be about the same with a
.h/-inl.h
> split as it would with a .h/.cc split--can you say why you think it would
suffer
> in that mode?

I think templates are best used when there's no other way of doing it, i.e.
bind, IPCs. As is, it makes reading ByteStream (even from a consumer's POV)
harder. In this case, there are alternative means. why would the client code be
less readable if an int is used?

[Randy Smith (Not in Mondays), 2013-07-31 17:30:50]

On 2013/07/31 17:07:17, jam wrote:
> On 2013/07/31 15:24:59, rdsmith wrote:
> > I'd very much prefer not using int, just because I care more about the
> consumer
> > code than the ByteStream code in terms of maintainability (as it's a utility
> > class).  I would think the readability would be about the same with a
> .h/-inl.h
> > split as it would with a .h/.cc split--can you say why you think it would
> suffer
> > in that mode?
> 
> I think templates are best used when there's no other way of doing it, i.e.
> bind, IPCs. As is, it makes reading ByteStream (even from a consumer's POV)
> harder. In this case, there are alternative means. why would the client code
be
> less readable if an int is used?

It's not about readability, it's about giving up the compile-time check on the
type of the error status.  I think that's valuable, especially because the code
that writes to the ByteStream and reads from the ByteStream are generally in
separate files and don't have a lot to do with each other (that separation was
the point of creating the class, actually).

[jam, 2013-07-31 17:36:51]

On 2013/07/31 17:30:50, rdsmith wrote:
> On 2013/07/31 17:07:17, jam wrote:
> > On 2013/07/31 15:24:59, rdsmith wrote:
> > > I'd very much prefer not using int, just because I care more about the
> > consumer
> > > code than the ByteStream code in terms of maintainability (as it's a
utility
> > > class).  I would think the readability would be about the same with a
> > .h/-inl.h
> > > split as it would with a .h/.cc split--can you say why you think it would
> > suffer
> > > in that mode?
> > 
> > I think templates are best used when there's no other way of doing it, i.e.
> > bind, IPCs. As is, it makes reading ByteStream (even from a consumer's POV)
> > harder. In this case, there are alternative means. why would the client code
> be
> > less readable if an int is used?
> 
> It's not about readability, it's about giving up the compile-time check on the
> type of the error status.

These are related, it's a tradeoff depending on exactly how much value the
compile time check gives in order for this readability decrease. Can you
elaborate more on what kind of checks you're talking about? Could there be other
ways of doing this check, i.e. have the creator pass in a callback about which
value ranges are valid, and that can be called in dchecks.

As another point of reference, I asked brettw for his feedback on this change
and he also said we should avoid the template.


>  I think that's valuable, especially because the code
> that writes to the ByteStream and reads from the ByteStream are generally in
> separate files and don't have a lot to do with each other (that separation was
> the point of creating the class, actually).

[Randy Smith (Not in Mondays), 2013-07-31 18:05:40]

On 2013/07/31 17:36:51, jam wrote:
> On 2013/07/31 17:30:50, rdsmith wrote:
> > On 2013/07/31 17:07:17, jam wrote:
> > > On 2013/07/31 15:24:59, rdsmith wrote:
> > > > I'd very much prefer not using int, just because I care more about the
> > > consumer
> > > > code than the ByteStream code in terms of maintainability (as it's a
> utility
> > > > class).  I would think the readability would be about the same with a
> > > .h/-inl.h
> > > > split as it would with a .h/.cc split--can you say why you think it
would
> > > suffer
> > > > in that mode?
> > > 
> > > I think templates are best used when there's no other way of doing it,
i.e.
> > > bind, IPCs. As is, it makes reading ByteStream (even from a consumer's
POV)
> > > harder. In this case, there are alternative means. why would the client
code
> > be
> > > less readable if an int is used?
> > 
> > It's not about readability, it's about giving up the compile-time check on
the
> > type of the error status.
> 
> These are related, it's a tradeoff depending on exactly how much value the
> compile time check gives in order for this readability decrease. Can you
> elaborate more on what kind of checks you're talking about? Could there be
other
> ways of doing this check, i.e. have the creator pass in a callback about which
> value ranges are valid, and that can be called in dchecks.
> 
> As another point of reference, I asked brettw for his feedback on this change
> and he also said we should avoid the template.
> 
> 
> >  I think that's valuable, especially because the code
> > that writes to the ByteStream and reads from the ByteStream are generally in
> > separate files and don't have a lot to do with each other (that separation
was
> > the point of creating the class, actually).

John and I talked, and he convinced me that the value of having a compile time
check, at least in the current usage model, wasn't really that high.  At least
for downloads, it's really unlikely that the code on the two sides of the
ByteStream are going to mutate apart to use different error values.  So I'll
accept casting, and agree to go with using int instead of
DownloadInterruptReason.

 Takuyo-san: My apologies for making you do the work of templatizing and have
that then not need to be used.  Thanks for bearing with John and I as we worked
through what the best implementation should be.  I'm ok with using an "int".

[tyoshino (SeeGerritForStatus), 2013-07-31 19:32:00]

On 2013/07/31 18:05:40, rdsmith wrote:
> John and I talked, and he convinced me that the value of having a compile time
> check, at least in the current usage model, wasn't really that high.  At least
> for downloads, it's really unlikely that the code on the two sides of the
> ByteStream are going to mutate apart to use different error values.  So I'll
> accept casting, and agree to go with using int instead of
> DownloadInterruptReason.

OK. Reverted and merged one change made to address your comment on ps13.

> 
>  Takuyo-san: My apologies for making you do the work of templatizing and have
> that then not need to be used.  Thanks for bearing with John and I as we
worked
> through what the best implementation should be.  I'm ok with using an "int".

I learned a lot such as symbol handling details from this work :)

[tyoshino (SeeGerritForStatus), 2013-07-31 19:59:29]

Done rebasing. Ready

[Randy Smith (Not in Mondays), 2013-07-31 22:34:41]

LGTM with nit.

https://codereview.chromium.org/18284005/diff/52002/content/browser/byte_stre...
File content/browser/byte_stream.h (right):

https://codereview.chromium.org/18284005/diff/52002/content/browser/byte_stre...
content/browser/byte_stream.h:37: // are indicated to the sink via the
GetStatus() method.
nit: I think I'd like to put in a bit more detail, including a warning. 
Something like: "Operation status at the source is indicated to the sink via an
int passed to the Close() method and returned from the GetStatus() method. 
Source and sink must agree on the interpretation of this int."

[tyoshino (SeeGerritForStatus), 2013-08-01 03:46:40]

https://codereview.chromium.org/18284005/diff/52002/content/browser/byte_stre...
File content/browser/byte_stream.h (right):

https://codereview.chromium.org/18284005/diff/52002/content/browser/byte_stre...
content/browser/byte_stream.h:37: // are indicated to the sink via the
GetStatus() method.
On 2013/07/31 22:34:42, rdsmith wrote:
> nit: I think I'd like to put in a bit more detail, including a warning. 
> Something like: "Operation status at the source is indicated to the sink via
an
> int passed to the Close() method and returned from the GetStatus() method. 
> Source and sink must agree on the interpretation of this int."

Thanks, done.

[tyoshino (SeeGerritForStatus), 2013-08-01 03:51:03]

Merged typo fix I made.
https://codereview.chromium.org/18284005/diff2/79007:88001/content/browser/by...

PTAL, jam

[jam, 2013-08-01 16:32:23]

lgtm

https://codereview.chromium.org/18284005/diff/88001/content/browser/streams/s...
File content/browser/streams/stream.cc (right):

https://codereview.chromium.org/18284005/diff/88001/content/browser/streams/s...
content/browser/streams/stream.cc:75: writer_->Close(0 /* status */);
nit: here and in other places, I think the /* status */ isn't necessary

[tyoshino (SeeGerritForStatus), 2013-08-02 07:40:09]

https://codereview.chromium.org/18284005/diff/88001/content/browser/streams/s...
File content/browser/streams/stream.cc (right):

https://codereview.chromium.org/18284005/diff/88001/content/browser/streams/s...
content/browser/streams/stream.cc:75: writer_->Close(0 /* status */);
On 2013/08/01 16:32:23, jam wrote:
> nit: here and in other places, I think the /* status */ isn't necessary

Removed.

[commit-bot: I haz the power, 2013-08-02 07:41:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tyoshino@chromium.org/18284005/96001

[commit-bot: I haz the power, 2013-08-02 11:02:34]

Change committed as 215265