Separate BufferedDataSource and BufferedResourceLoader into two files.

Separate BufferedDataSource and BufferedResourceLoader into two files.

It's time to finally separate the huge monster files buffered_data_source.[h/cc] into two.

ericroman: There are some variables and short methods that both BufferedDataSource and BufferedResourceLoader rely on and that I have moved to src/net/http/http_util.h.  Could you please verify that this is an ok place to put them?

Also, appcache issues have been resolved (and approved by michaeln) so I've removed those comments.

BUG=None.
TEST=Compiles.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=69429

[scherkus (not reviewing), 2010-12-10 23:02:38]

is it also time to delete media_resource_loader_bridge_factory.h + friends?

http://codereview.chromium.org/5756004/diff/1/webkit/glue/media/buffered_data...
File webkit/glue/media/buffered_data_source.cc (right):

http://codereview.chromium.org/5756004/diff/1/webkit/glue/media/buffered_data...
webkit/glue/media/buffered_data_source.cc:7: #include
"webkit/glue/media/buffered_data_source.h"
can you move this header to the first line + fix any include dependencies?

http://codereview.chromium.org/5756004/diff/1/webkit/glue/media/buffered_data...
webkit/glue/media/buffered_data_source.cc:37: return url.SchemeIs(kHttpScheme)
|| url.SchemeIs(kHttpsScheme);
see if we can move this to net/http/http_util.h + same with other file

http://codereview.chromium.org/5756004/diff/1/webkit/glue/media/buffered_data...
webkit/glue/media/buffered_data_source.cc:48:
/////////////////////////////////////////////////////////////////////////////
nit: I'd remove this comment as it was used to separate the two classes.. but
given there's only one class now it's just noisy

http://codereview.chromium.org/5756004/diff/1/webkit/glue/media/buffered_reso...
File webkit/glue/media/buffered_resource_loader.cc (right):

http://codereview.chromium.org/5756004/diff/1/webkit/glue/media/buffered_reso...
webkit/glue/media/buffered_resource_loader.cc:12: #include
"webkit/glue/media/buffered_resource_loader.h"
can you move this header to the first line + fix any include dependencies?

http://codereview.chromium.org/5756004/diff/1/webkit/glue/media/buffered_reso...
webkit/glue/media/buffered_resource_loader.cc:48: // TODO(hclam): determine a
better value for this.
could you change the TODO to reference crbug.com/64571 to remove this?

http://codereview.chromium.org/5756004/diff/1/webkit/glue/media/buffered_reso...
webkit/glue/media/buffered_resource_loader.cc:64:
/////////////////////////////////////////////////////////////////////////////
ditto w.r.t. noisy comments

http://codereview.chromium.org/5756004/diff/1/webkit/glue/media/buffered_reso...
webkit/glue/media/buffered_resource_loader.cc:238: // BufferedResourceLoader,
WebKit::WebURLLoaderClient implementations.
ditto w.r.t. noisy comments

http://codereview.chromium.org/5756004/diff/1/webkit/glue/media/buffered_reso...
webkit/glue/media/buffered_resource_loader.cc:431: // BufferedResourceLoader,
private
ditto w.r.t. noisy comments

http://codereview.chromium.org/5756004/diff/1/webkit/glue/media/buffered_reso...
File webkit/glue/media/buffered_resource_loader.h (right):

http://codereview.chromium.org/5756004/diff/1/webkit/glue/media/buffered_reso...
webkit/glue/media/buffered_resource_loader.h:25:
/////////////////////////////////////////////////////////////////////////////
nit: same thing .. remove the ////////////... line and the following line that
repeats the class name

[Paweł Hajdan Jr., 2010-12-11 10:42:57]

Drive-by with a test comment. Please let me take another look before committing.

http://codereview.chromium.org/5756004/diff/1/webkit/glue/media/buffered_data...
File webkit/glue/media/buffered_data_source_unittest.cc (right):

http://codereview.chromium.org/5756004/diff/1/webkit/glue/media/buffered_data...
webkit/glue/media/buffered_data_source_unittest.cc:58: return
base::TimeDelta::FromMilliseconds(100);
There is base/test/test_timeouts for that. Please use it, and feel free to add a
new kind of timeout (a very short one) if needed.

[annacc, 2010-12-11 20:12:38]

I think I've addressed all your comments.  Also removed
media_resource_loader_bridge_factory and added a new timeout to test_timeouts as
requested.

http://codereview.chromium.org/5756004/diff/1/webkit/glue/media/buffered_data...
File webkit/glue/media/buffered_data_source.cc (right):

http://codereview.chromium.org/5756004/diff/1/webkit/glue/media/buffered_data...
webkit/glue/media/buffered_data_source.cc:7: #include
"webkit/glue/media/buffered_data_source.h"
On 2010/12/10 23:02:38, scherkus wrote:
> can you move this header to the first line + fix any include dependencies?

Done. (I think)

http://codereview.chromium.org/5756004/diff/1/webkit/glue/media/buffered_data...
webkit/glue/media/buffered_data_source.cc:37: return url.SchemeIs(kHttpScheme)
|| url.SchemeIs(kHttpsScheme);
On 2010/12/10 23:02:38, scherkus wrote:
> see if we can move this to net/http/http_util.h + same with other file

Done.

http://codereview.chromium.org/5756004/diff/1/webkit/glue/media/buffered_data...
webkit/glue/media/buffered_data_source.cc:48:
/////////////////////////////////////////////////////////////////////////////
On 2010/12/10 23:02:38, scherkus wrote:
> nit: I'd remove this comment as it was used to separate the two classes.. but
> given there's only one class now it's just noisy

Done.

http://codereview.chromium.org/5756004/diff/1/webkit/glue/media/buffered_data...
File webkit/glue/media/buffered_data_source_unittest.cc (right):

http://codereview.chromium.org/5756004/diff/1/webkit/glue/media/buffered_data...
webkit/glue/media/buffered_data_source_unittest.cc:58: return
base::TimeDelta::FromMilliseconds(100);
On 2010/12/11 10:43:00, Paweł Hajdan Jr. wrote:
> There is base/test/test_timeouts for that. Please use it, and feel free to add
a
> new kind of timeout (a very short one) if needed.

Done. But, you may want to double check it.

http://codereview.chromium.org/5756004/diff/1/webkit/glue/media/buffered_reso...
File webkit/glue/media/buffered_resource_loader.cc (right):

http://codereview.chromium.org/5756004/diff/1/webkit/glue/media/buffered_reso...
webkit/glue/media/buffered_resource_loader.cc:12: #include
"webkit/glue/media/buffered_resource_loader.h"
On 2010/12/10 23:02:38, scherkus wrote:
> can you move this header to the first line + fix any include dependencies?

Done.

http://codereview.chromium.org/5756004/diff/1/webkit/glue/media/buffered_reso...
webkit/glue/media/buffered_resource_loader.cc:48: // TODO(hclam): determine a
better value for this.
On 2010/12/10 23:02:38, scherkus wrote:
> could you change the TODO to reference crbug.com/64571 to remove this?

Done.

http://codereview.chromium.org/5756004/diff/1/webkit/glue/media/buffered_reso...
webkit/glue/media/buffered_resource_loader.cc:64:
/////////////////////////////////////////////////////////////////////////////
On 2010/12/10 23:02:38, scherkus wrote:
> ditto w.r.t. noisy comments

Done.

http://codereview.chromium.org/5756004/diff/1/webkit/glue/media/buffered_reso...
webkit/glue/media/buffered_resource_loader.cc:238: // BufferedResourceLoader,
WebKit::WebURLLoaderClient implementations.
On 2010/12/10 23:02:38, scherkus wrote:
> ditto w.r.t. noisy comments

Done.

http://codereview.chromium.org/5756004/diff/1/webkit/glue/media/buffered_reso...
webkit/glue/media/buffered_resource_loader.cc:431: // BufferedResourceLoader,
private
On 2010/12/10 23:02:38, scherkus wrote:
> ditto w.r.t. noisy comments

Done.

http://codereview.chromium.org/5756004/diff/1/webkit/glue/media/buffered_reso...
File webkit/glue/media/buffered_resource_loader.h (right):

http://codereview.chromium.org/5756004/diff/1/webkit/glue/media/buffered_reso...
webkit/glue/media/buffered_resource_loader.h:25:
/////////////////////////////////////////////////////////////////////////////
On 2010/12/10 23:02:38, scherkus wrote:
> nit: same thing .. remove the ////////////... line and the following line that
> repeats the class name

Done.

[Paweł Hajdan Jr., 2010-12-13 18:54:31]

Mostly nits.

http://codereview.chromium.org/5756004/diff/9001/base/test/test_switches.cc
File base/test/test_switches.cc (right):

http://codereview.chromium.org/5756004/diff/9001/base/test/test_switches.cc#n...
base/test/test_switches.cc:16: const char switches::kUiTestShortTimeout[] =
"ui-test-short-timeout";
nit: Remember to make a change here too.

http://codereview.chromium.org/5756004/diff/9001/base/test/test_switches.h
File base/test/test_switches.h (right):

http://codereview.chromium.org/5756004/diff/9001/base/test/test_switches.h#ne...
base/test/test_switches.h:17: extern const char kUiTestShortTimeout[];
nit: The "UiTest" part of the switch name is legacy. Could you rename it to
kTestTinyTimeout, and put it right above kTestLargeTimeout?

http://codereview.chromium.org/5756004/diff/9001/base/test/test_timeouts.h
File base/test/test_timeouts.h (right):

http://codereview.chromium.org/5756004/diff/9001/base/test/test_timeouts.h#ne...
base/test/test_timeouts.h:18: // Timeout for very short wait times, such and
resource loading.
nit: "such and" -> typo?

Please don't mention resource loading here. Could you rather say it's to be used
when the action is expected to finish "almost instantly"?

http://codereview.chromium.org/5756004/diff/9001/base/test/test_timeouts.h#ne...
base/test/test_timeouts.h:19: static int short_timeout_ms() { return
short_timeout_ms_; }
nit: Rename to tiny_timeout_ms.

[annacc, 2010-12-13 20:14:21]

http://codereview.chromium.org/5756004/diff/9001/base/test/test_switches.cc
File base/test/test_switches.cc (right):

http://codereview.chromium.org/5756004/diff/9001/base/test/test_switches.cc#n...
base/test/test_switches.cc:16: const char switches::kUiTestShortTimeout[] =
"ui-test-short-timeout";
On 2010/12/13 18:54:32, Paweł Hajdan Jr. wrote:
> nit: Remember to make a change here too.

Done.

http://codereview.chromium.org/5756004/diff/9001/base/test/test_switches.h
File base/test/test_switches.h (right):

http://codereview.chromium.org/5756004/diff/9001/base/test/test_switches.h#ne...
base/test/test_switches.h:17: extern const char kUiTestShortTimeout[];
On 2010/12/13 18:54:32, Paweł Hajdan Jr. wrote:
> nit: The "UiTest" part of the switch name is legacy. Could you rename it to
> kTestTinyTimeout, and put it right above kTestLargeTimeout?

Done. Except, I think you meant below (for alphabetical order), yes?

http://codereview.chromium.org/5756004/diff/9001/base/test/test_timeouts.h
File base/test/test_timeouts.h (right):

http://codereview.chromium.org/5756004/diff/9001/base/test/test_timeouts.h#ne...
base/test/test_timeouts.h:18: // Timeout for very short wait times, such and
resource loading.
On 2010/12/13 18:54:32, Paweł Hajdan Jr. wrote:
> nit: "such and" -> typo?
> 
> Please don't mention resource loading here. Could you rather say it's to be
used
> when the action is expected to finish "almost instantly"?

Done.

http://codereview.chromium.org/5756004/diff/9001/base/test/test_timeouts.h#ne...
base/test/test_timeouts.h:19: static int short_timeout_ms() { return
short_timeout_ms_; }
On 2010/12/13 18:54:32, Paweł Hajdan Jr. wrote:
> nit: Rename to tiny_timeout_ms.

Done.

[Paweł Hajdan Jr., 2010-12-13 20:46:37]

Code I commented in the drive-by LGTM. Thanks!

[annacc, 2010-12-14 18:35:58]

Is this CL ready to go?

[scherkus (not reviewing), 2010-12-14 18:48:19]

getting there!

try to find someone to look at your http_util.h stuff to make sure we're not
dumping in some random cruft

http://codereview.chromium.org/5756004/diff/17001/net/http/http_util.h
File net/http/http_util.h (right):

http://codereview.chromium.org/5756004/diff/17001/net/http/http_util.h#newcode66
net/http/http_util.h:66: const int kInitialReadBufferSize = 32768;
I don't think most of these constants belong here :(

Sorry for not being clearer but I think at a minimum the only stuff that could
go here would be kHttpScheme, kHttpsScheme, kDataScheme and the two
IsXXXProtocol() methods

Everything else is exclusive to buffered data source/resource loader

I'd also try to find some one who works on src/net/ code to take a look

http://codereview.chromium.org/5756004/diff/17001/webkit/glue/media/buffered_...
File webkit/glue/media/buffered_data_source.cc (right):

http://codereview.chromium.org/5756004/diff/17001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.cc:13: using net::HttpUtil;
nit: alphabetize forward declares

http://codereview.chromium.org/5756004/diff/17001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.cc:96: return
IsProtocolSupportedForMedia(gurl) && !net::HttpUtil::IsDataProtocol(gurl);
>80 chars

http://codereview.chromium.org/5756004/diff/17001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.cc:377: streaming_ = (instance_size ==
net::kPositionNotSpecified) || !partial_response;
>80 chars

http://codereview.chromium.org/5756004/diff/17001/webkit/glue/media/buffered_...
File webkit/glue/media/buffered_data_source.h (right):

http://codereview.chromium.org/5756004/diff/17001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.h:13: #include
"webkit/glue/media/buffered_resource_loader.h"
nit: I don't think you actually need the header (I think you can get away with
forward declare)

http://codereview.chromium.org/5756004/diff/17001/webkit/glue/media/buffered_...
File webkit/glue/media/buffered_data_source_unittest.cc (right):

http://codereview.chromium.org/5756004/diff/17001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source_unittest.cc:62:
MOCK_METHOD2(CreateResourceLoader, BufferedResourceLoader*(
nit: I'd drop the BufferedResourceLoader*( ... part down to next line and line
up with the (

http://codereview.chromium.org/5756004/diff/17001/webkit/glue/media/buffered_...
File webkit/glue/media/buffered_resource_loader.cc (right):

http://codereview.chromium.org/5756004/diff/17001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_resource_loader.cc:32: : buffer_(new
media::SeekableBuffer(net::kBackwardCapcity, net::kForwardCapacity)),
>80 chars

http://codereview.chromium.org/5756004/diff/17001/webkit/glue/media/buffered_...
File webkit/glue/media/buffered_resource_loader.h (right):

http://codereview.chromium.org/5756004/diff/17001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_resource_loader.h:91: virtual int64 content_length()
{ return content_length_; }
avoid inlining virtual methods (i.e., move the bunch of these to .cc file)

it can generate a lot of virtual message dispatching cruft due to the inline

http://codereview.chromium.org/5756004/diff/17001/webkit/glue/media/simple_da...
File webkit/glue/media/simple_data_source.cc (right):

http://codereview.chromium.org/5756004/diff/17001/webkit/glue/media/simple_da...
webkit/glue/media/simple_data_source.cc:8: #include "net/base/load_flags.h"
nit: alphabetize includes

http://codereview.chromium.org/5756004/diff/17001/webkit/glue/media/simple_da...
webkit/glue/media/simple_data_source.cc:266:
host()->SetLoaded(url_.SchemeIsFile() || net::HttpUtil::IsDataProtocol(url_));
>80 chars

[annacc, 2010-12-14 21:10:17]

http://codereview.chromium.org/5756004/diff/17001/net/http/http_util.h
File net/http/http_util.h (right):

http://codereview.chromium.org/5756004/diff/17001/net/http/http_util.h#newcode66
net/http/http_util.h:66: const int kInitialReadBufferSize = 32768;
On 2010/12/14 18:48:19, scherkus wrote:
> I don't think most of these constants belong here :(
> 
> Sorry for not being clearer but I think at a minimum the only stuff that could
> go here would be kHttpScheme, kHttpsScheme, kDataScheme and the two
> IsXXXProtocol() methods
> 
> Everything else is exclusive to buffered data source/resource loader
> 
> I'd also try to find some one who works on src/net/ code to take a look

OK, I've moved the rest back where they belong, sorry for the confusion.  And
I'm adding ericroman to double check.

http://codereview.chromium.org/5756004/diff/17001/webkit/glue/media/buffered_...
File webkit/glue/media/buffered_data_source.cc (right):

http://codereview.chromium.org/5756004/diff/17001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.cc:13: using net::HttpUtil;
On 2010/12/14 18:48:19, scherkus wrote:
> nit: alphabetize forward declares

Done.

http://codereview.chromium.org/5756004/diff/17001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.cc:96: return
IsProtocolSupportedForMedia(gurl) && !net::HttpUtil::IsDataProtocol(gurl);
On 2010/12/14 18:48:19, scherkus wrote:
> >80 chars

Done.

http://codereview.chromium.org/5756004/diff/17001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.cc:377: streaming_ = (instance_size ==
net::kPositionNotSpecified) || !partial_response;
On 2010/12/14 18:48:19, scherkus wrote:
> >80 chars

Done.

http://codereview.chromium.org/5756004/diff/17001/webkit/glue/media/buffered_...
File webkit/glue/media/buffered_data_source.h (right):

http://codereview.chromium.org/5756004/diff/17001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.h:13: #include
"webkit/glue/media/buffered_resource_loader.h"
On 2010/12/14 18:48:19, scherkus wrote:
> nit: I don't think you actually need the header (I think you can get away with
> forward declare)

Yes, it seems like that should be true, but the compiler complains. :(  Could it
be because it's used as a scoped_refptr?

http://codereview.chromium.org/5756004/diff/17001/webkit/glue/media/buffered_...
File webkit/glue/media/buffered_data_source_unittest.cc (right):

http://codereview.chromium.org/5756004/diff/17001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source_unittest.cc:62:
MOCK_METHOD2(CreateResourceLoader, BufferedResourceLoader*(
On 2010/12/14 18:48:19, scherkus wrote:
> nit: I'd drop the BufferedResourceLoader*( ... part down to next line and line
> up with the (

Done.

http://codereview.chromium.org/5756004/diff/17001/webkit/glue/media/buffered_...
File webkit/glue/media/buffered_resource_loader.cc (right):

http://codereview.chromium.org/5756004/diff/17001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_resource_loader.cc:32: : buffer_(new
media::SeekableBuffer(net::kBackwardCapcity, net::kForwardCapacity)),
On 2010/12/14 18:48:19, scherkus wrote:
> >80 chars

Done.

http://codereview.chromium.org/5756004/diff/17001/webkit/glue/media/buffered_...
File webkit/glue/media/buffered_resource_loader.h (right):

http://codereview.chromium.org/5756004/diff/17001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_resource_loader.h:91: virtual int64 content_length()
{ return content_length_; }
On 2010/12/14 18:48:19, scherkus wrote:
> avoid inlining virtual methods (i.e., move the bunch of these to .cc file)
> 
> it can generate a lot of virtual message dispatching cruft due to the inline

Done.

http://codereview.chromium.org/5756004/diff/17001/webkit/glue/media/simple_da...
File webkit/glue/media/simple_data_source.cc (right):

http://codereview.chromium.org/5756004/diff/17001/webkit/glue/media/simple_da...
webkit/glue/media/simple_data_source.cc:8: #include "net/base/load_flags.h"
On 2010/12/14 18:48:19, scherkus wrote:
> nit: alphabetize includes

Done.

http://codereview.chromium.org/5756004/diff/17001/webkit/glue/media/simple_da...
webkit/glue/media/simple_data_source.cc:266:
host()->SetLoaded(url_.SchemeIsFile() || net::HttpUtil::IsDataProtocol(url_));
On 2010/12/14 18:48:19, scherkus wrote:
> >80 chars

Done.

[annacc, 2010-12-14 21:30:44]

http://codereview.chromium.org/5756004/diff/17001/net/http/http_util.h
File net/http/http_util.h (right):

http://codereview.chromium.org/5756004/diff/17001/net/http/http_util.h#newcode66
net/http/http_util.h:66: const int kInitialReadBufferSize = 32768;
On 2010/12/14 21:10:17, annacc wrote:
> On 2010/12/14 18:48:19, scherkus wrote:
> > I don't think most of these constants belong here :(
> > 
> > Sorry for not being clearer but I think at a minimum the only stuff that
could
> > go here would be kHttpScheme, kHttpsScheme, kDataScheme and the two
> > IsXXXProtocol() methods
> > 
> > Everything else is exclusive to buffered data source/resource loader
> > 
> > I'd also try to find some one who works on src/net/ code to take a look
> 
> OK, I've moved the rest back where they belong, sorry for the confusion.  And
> I'm adding ericroman to double check.

Scratch the bit about adding ericroman, he's on vacation.  I've added erikwright
instead.

[scherkus (not reviewing), 2010-12-14 22:14:20]

willchan works on src/net/ stuff too I believe

[cbentzel, 2010-12-14 23:38:09]

On 2010/12/14 22:14:20, scherkus wrote:
> willchan works on src/net/ stuff too I believe

Quick driveby on http_util.h only

I'm not sure why net/http/http_util.h needed to be changed for this CL.
IsDataProtocol and IsHttpProtocol seem to add no value over calling directly on
the GURL.

[erikwright (departed), 2010-12-15 00:13:34]

I don't consider myself an authority on net, but I will provide my two cents.

1) It's not clear from "IsHttpProtocol" that either http or https return true.
While, for your use case, those might both be HTTP, other clients might consider
them separate protocols.

2) I don't think that the methods provide substantial value over doing the work
in-line. In fact, because of point (1), I think they could mislead a reader who
didn't follow to the implementation. So I would remove them.

3) If others felt that they _did_ belong here, at a minimum, move them to the
.cc, move the constants to an anonymous namespace in the .cc, remove gurl.h from
the .h (forward declare GURL), and move the declarations up to the top of the .h
(near the other functions operating on GURLs).

[willchan no longer on Chromium, 2010-12-15 00:26:50]

Speaking for net/, I agree with cbentzel and erikwright.  Cheers.

On Tue, Dec 14, 2010 at 4:13 PM, <erikwright@chromium.org> wrote:

> I don't consider myself an authority on net, but I will provide my two
> cents.
>
> 1) It's not clear from "IsHttpProtocol" that either http or https return
> true.
> While, for your use case, those might both be HTTP, other clients might
> consider
> them separate protocols.
>
> 2) I don't think that the methods provide substantial value over doing the
> work
> in-line. In fact, because of point (1), I think they could mislead a reader
> who
> didn't follow to the implementation. So I would remove them.
>
> 3) If others felt that they _did_ belong here, at a minimum, move them to
> the
> .cc, move the constants to an anonymous namespace in the .cc, remove gurl.h
> from
> the .h (forward declare GURL), and move the declarations up to the top of
> the .h
> (near the other functions operating on GURLs).
>
>
> http://codereview.chromium.org/5756004/
>

[annacc, 2010-12-15 00:28:54]

OK, I will move these changes back into buffered data source / resource loader.

Thanks for the quick feedback!
-Anna

On 2010/12/15 00:26:50, willchan wrote:
> Speaking for net/, I agree with cbentzel and erikwright.  Cheers.
> 
> On Tue, Dec 14, 2010 at 4:13 PM, <mailto:erikwright@chromium.org> wrote:
> 
> > I don't consider myself an authority on net, but I will provide my two
> > cents.
> >
> > 1) It's not clear from "IsHttpProtocol" that either http or https return
> > true.
> > While, for your use case, those might both be HTTP, other clients might
> > consider
> > them separate protocols.
> >
> > 2) I don't think that the methods provide substantial value over doing the
> > work
> > in-line. In fact, because of point (1), I think they could mislead a reader
> > who
> > didn't follow to the implementation. So I would remove them.
> >
> > 3) If others felt that they _did_ belong here, at a minimum, move them to
> > the
> > .cc, move the constants to an anonymous namespace in the .cc, remove gurl.h
> > from
> > the .h (forward declare GURL), and move the declarations up to the top of
> > the .h
> > (near the other functions operating on GURLs).
> >
> >
> > http://codereview.chromium.org/5756004/
> >

[erikwright (departed), 2010-12-15 00:59:10]

Removing myself from reviewers list, as net/ is no longer affected.

[scherkus (not reviewing), 2010-12-15 16:00:36]

LGTM w/ nits

http://codereview.chromium.org/5756004/diff/17001/webkit/glue/media/buffered_...
File webkit/glue/media/buffered_data_source.h (right):

http://codereview.chromium.org/5756004/diff/17001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.h:13: #include
"webkit/glue/media/buffered_resource_loader.h"
On 2010/12/14 21:10:17, annacc wrote:
> On 2010/12/14 18:48:19, scherkus wrote:
> > nit: I don't think you actually need the header (I think you can get away
with
> > forward declare)
> 
> Yes, it seems like that should be true, but the compiler complains. :(  Could
it
> be because it's used as a scoped_refptr?
> 

should be fine but let's not worry about it for now

http://codereview.chromium.org/5756004/diff/65001/webkit/glue/media/buffered_...
File webkit/glue/media/buffered_data_source.cc (right):

http://codereview.chromium.org/5756004/diff/65001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.cc:19: const int64 kPositionNotSpecified
= -1;
I think we should avoid duplicating this but I have a plan of attack!

kPositionNotSpecified is unique to BufferedResourceLoader (notice all the
comments where it says "pass in -1" or "returns -1") ... we should move the
declaration of kPositionNotSpecified to BufferedResourceLoader's header file and
update the comments to say kPositionNotSpecified instead of -1

Feel free to do this in follow up CL if you want to get this in first

http://codereview.chromium.org/5756004/diff/65001/webkit/glue/media/buffered_...
File webkit/glue/media/buffered_resource_loader.h (right):

http://codereview.chromium.org/5756004/diff/65001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_resource_loader.h:80: virtual int64
GetBufferedFirstBytePosition();
no implementation and looks like no usage.. remove from here + mock I guess!

http://codereview.chromium.org/5756004/diff/65001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_resource_loader.h:84: virtual int64
GetBufferedLastBytePosition();
ditto!

[annacc, 2010-12-15 18:34:03]

http://codereview.chromium.org/5756004/diff/65001/webkit/glue/media/buffered_...
File webkit/glue/media/buffered_data_source.cc (right):

http://codereview.chromium.org/5756004/diff/65001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.cc:19: const int64 kPositionNotSpecified
= -1;
On 2010/12/15 16:00:36, scherkus wrote:
> I think we should avoid duplicating this but I have a plan of attack!
> 
> kPositionNotSpecified is unique to BufferedResourceLoader (notice all the
> comments where it says "pass in -1" or "returns -1") ... we should move the
> declaration of kPositionNotSpecified to BufferedResourceLoader's header file
and
> update the comments to say kPositionNotSpecified instead of -1
> 
> Feel free to do this in follow up CL if you want to get this in first

Done.  

How do you feel about moving kHttpScheme, kHttpsScheme, kDataScheme, and/or the
two Is[Http/Data]Prototcol's to BufferedResourceLoader's header file as well?

http://codereview.chromium.org/5756004/diff/65001/webkit/glue/media/buffered_...
File webkit/glue/media/buffered_resource_loader.h (right):

http://codereview.chromium.org/5756004/diff/65001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_resource_loader.h:84: virtual int64
GetBufferedLastBytePosition();
On 2010/12/15 16:00:36, scherkus wrote:
> ditto!

GetBufferedLastBytePosition() is used in
BufferedDataSource::NetworkEventCallback() line 577.  Should I get rid of
GetBufferedFirstBytePosition (will it be needed someday?) or keep both?

[scherkus (not reviewing), 2010-12-15 18:42:45]

http://codereview.chromium.org/5756004/diff/65001/webkit/glue/media/buffered_...
File webkit/glue/media/buffered_data_source.cc (right):

http://codereview.chromium.org/5756004/diff/65001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.cc:19: const int64 kPositionNotSpecified
= -1;
On 2010/12/15 18:34:03, annacc wrote:
> On 2010/12/15 16:00:36, scherkus wrote:
> > I think we should avoid duplicating this but I have a plan of attack!
> > 
> > kPositionNotSpecified is unique to BufferedResourceLoader (notice all the
> > comments where it says "pass in -1" or "returns -1") ... we should move the
> > declaration of kPositionNotSpecified to BufferedResourceLoader's header file
> and
> > update the comments to say kPositionNotSpecified instead of -1
> > 
> > Feel free to do this in follow up CL if you want to get this in first
> 
> Done.  
> 
> How do you feel about moving kHttpScheme, kHttpsScheme, kDataScheme, and/or
the
> two Is[Http/Data]Prototcol's to BufferedResourceLoader's header file as well?

pretty indifferent.. as the other reviewers said these functions could
technically be inlined + removed if we wanted to

http://codereview.chromium.org/5756004/diff/65001/webkit/glue/media/buffered_...
File webkit/glue/media/buffered_resource_loader.h (right):

http://codereview.chromium.org/5756004/diff/65001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_resource_loader.h:84: virtual int64
GetBufferedLastBytePosition();
On 2010/12/15 18:34:03, annacc wrote:
> On 2010/12/15 16:00:36, scherkus wrote:
> > ditto!
> 
> GetBufferedLastBytePosition() is used in
> BufferedDataSource::NetworkEventCallback() line 577.  Should I get rid of
> GetBufferedFirstBytePosition (will it be needed someday?) or keep both?

ah missed that reference!

ok remove GetBufferedFirstBytePosition() and leave in
GetBufferedLastBytePosition()

although I will say that name is a bit verbose... what about simply
GetBufferedPosition()?

[annacc, 2010-12-15 19:19:49]

Done and done.  Removed GetBufferedFirstBytePosition() and renamed
GetBufferedLastBytePosition() to GetBufferedPosition().  Inlined and removed
Is...Protocol()s and moved k...Scheme's to buffered_resource_loader.h.

On 2010/12/15 18:42:45, scherkus wrote:
>
http://codereview.chromium.org/5756004/diff/65001/webkit/glue/media/buffered_...
> File webkit/glue/media/buffered_data_source.cc (right):
> 
>
http://codereview.chromium.org/5756004/diff/65001/webkit/glue/media/buffered_...
> webkit/glue/media/buffered_data_source.cc:19: const int64
kPositionNotSpecified
> = -1;
> On 2010/12/15 18:34:03, annacc wrote:
> > On 2010/12/15 16:00:36, scherkus wrote:
> > > I think we should avoid duplicating this but I have a plan of attack!
> > > 
> > > kPositionNotSpecified is unique to BufferedResourceLoader (notice all the
> > > comments where it says "pass in -1" or "returns -1") ... we should move
the
> > > declaration of kPositionNotSpecified to BufferedResourceLoader's header
file
> > and
> > > update the comments to say kPositionNotSpecified instead of -1
> > > 
> > > Feel free to do this in follow up CL if you want to get this in first
> > 
> > Done.  
> > 
> > How do you feel about moving kHttpScheme, kHttpsScheme, kDataScheme, and/or
> the
> > two Is[Http/Data]Prototcol's to BufferedResourceLoader's header file as
well?
> 
> pretty indifferent.. as the other reviewers said these functions could
> technically be inlined + removed if we wanted to
> 
>
http://codereview.chromium.org/5756004/diff/65001/webkit/glue/media/buffered_...
> File webkit/glue/media/buffered_resource_loader.h (right):
> 
>
http://codereview.chromium.org/5756004/diff/65001/webkit/glue/media/buffered_...
> webkit/glue/media/buffered_resource_loader.h:84: virtual int64
> GetBufferedLastBytePosition();
> On 2010/12/15 18:34:03, annacc wrote:
> > On 2010/12/15 16:00:36, scherkus wrote:
> > > ditto!
> > 
> > GetBufferedLastBytePosition() is used in
> > BufferedDataSource::NetworkEventCallback() line 577.  Should I get rid of
> > GetBufferedFirstBytePosition (will it be needed someday?) or keep both?
> 
> ah missed that reference!
> 
> ok remove GetBufferedFirstBytePosition() and leave in
> GetBufferedLastBytePosition()
> 
> although I will say that name is a bit verbose... what about simply
> GetBufferedPosition()?

[annacc, 2010-12-16 17:59:13]

friendly ping.

[scherkus (not reviewing), 2010-12-16 18:09:42]

LGTM w/ nits

For future reference (because I don't think I explained this before!) when
someone says LGTM but has a few tiny details (like in this review or my previous
"LGTM w/ nits") it's OK to fix the details and go ahead and commit

Basically it's a way of reducing an additional round trip if the things to be
fixed are very tiny and non-controversial

http://codereview.chromium.org/5756004/diff/104001/webkit/glue/media/buffered...
File webkit/glue/media/buffered_data_source.cc (right):

http://codereview.chromium.org/5756004/diff/104001/webkit/glue/media/buffered...
webkit/glue/media/buffered_data_source.cc:208: loader_ =
CreateResourceLoader(kPositionNotSpecified, kPositionNotSpecified);
>80 chars

http://codereview.chromium.org/5756004/diff/104001/webkit/glue/media/buffered...
webkit/glue/media/buffered_data_source.cc:404: loader_ =
CreateResourceLoader(kPositionNotSpecified, kPositionNotSpecified);
> 80 chars

[annacc, 2010-12-16 18:51:49]

Committed revision 69429