Refactoring BufferedDataSource to work with WebURLLoader instead of a MediaResourceLoaderBridge.

Refactoring BufferedDataSource to work with WebURLLoader instead of a MediaResourceLoaderBridge. + 2 unit test modifications.

One thing to notice is that both buffered_data_source_unittest and simple_data_source_unittest need to have a way to inject a MockWebURLLoader into the BufferedResourceLoader and the SimpleDataSource.  In order to make sure a new one is not created during a Start(), I introduced the function SetURLLoaderForTest and keep_test_loader flag.

BUG=16751
TEST=
src/xcodebuild/Debug/test_shell_tests --gtest_filter=Buffered*
src/xcodebuild/Debug/test_shell_tests --gtest_filter=Simple*
src/webkit/tools/layout_tests/run_webkit_tests.sh --debug media
webkit/tools/layout_tests/run_webkit_tests.sh --debug http/tests/media
(but arg, the video-referer.html test still fails, any ideas?)

[scherkus (not reviewing), 2010-10-18 22:57:12]

looking good so far!  this is indeed a big (but very nice to have) refactor!

feel free to keep hacking on this for now, but don't forget about coding style
when you're ready for a more serious review:
http://dev.chromium.org/developers/coding-style

You can also run your patch through the lint tool to catch most style errors:
 $ git diff --name-only trunk | xargs cpplint.py

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

http://codereview.chromium.org/3863002/diff/1/3#newcode84
webkit/glue/media/buffered_data_source.cc:84: WebKit::WebURLLoader* url_loader,
when you using "using" statements you can drop the WebKit:: prefix (applies to
rest of code)

http://codereview.chromium.org/3863002/diff/1/3#newcode373
webkit/glue/media/buffered_data_source.cc:373: /** (annacc ?): How do I get the
header info? **/
same as above, you'll likely have to read the response info in
didReceiveResponse() and translate/save any relevant data to local variable
members

Check out WebURLResponse object, it should have enough info there

http://codereview.chromium.org/3863002/diff/1/3#newcode559
webkit/glue/media/buffered_data_source.cc:559: //if (start_callback_.get()) {
I don't think so as DoneStart() takes care of calling + resetting the callback

http://codereview.chromium.org/3863002/diff/1/3#newcode567
webkit/glue/media/buffered_data_source.cc:567: //  DoneStart(status.os_error());
yes I believe you'll have didRecieveResponse() in which you can save any
response data in that callback

http://codereview.chromium.org/3863002/diff/1/3#newcode577
webkit/glue/media/buffered_data_source.cc:577: /** (annacc ?): same as above,
don't know how to get status */
ditto

http://codereview.chromium.org/3863002/diff/1/3#newcode605
webkit/glue/media/buffered_data_source.cc:605: /** (annacc ?): - don't know if
this shuld be here **/
Each request either terminates with didFinishLoading() or didFail(), in which
case yes we should have some duplicated code for calling DoneStart() with an
error + cleaning up

http://codereview.chromium.org/3863002/diff/1/3#newcode816
webkit/glue/media/buffered_data_source.cc:816: WebURLRequest
request(frame_->url());
tabs!!! ack!!!  make sure xcode is set to use 2-space indents using spaces, not
tabs

[Alpha Left Google, 2010-10-18 23:55:18]

Looks pretty good.

After we have fixed this we'll need to handle the case of detaching WebFrame
from BufferedDataSource. I'll tell you where to start when this is finished.

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

http://codereview.chromium.org/3863002/diff/1/3#newcode282
webkit/glue/media/buffered_data_source.cc:282: void
BufferedResourceLoader::willSendRequest(
Please read how willSendRequest is used
https://cs.corp.google.com/p#chrome/trunk/src/webkit/glue/weburlloader_impl.c...

http://codereview.chromium.org/3863002/diff/1/3#newcode287
webkit/glue/media/buffered_data_source.cc:287: url_loader_ = loader;
nit: indentation is 4 chars.

http://codereview.chromium.org/3863002/diff/1/3#newcode288
webkit/glue/media/buffered_data_source.cc:288: url_ = newRequest.url();
Don't save the new url yet, do so only after we have verified it.

http://codereview.chromium.org/3863002/diff/1/3#newcode293
webkit/glue/media/buffered_data_source.cc:293: return;
According to weburlloader_impl.cc we should set the url in the request to an
invalid value.

http://codereview.chromium.org/3863002/diff/1/3#newcode295
webkit/glue/media/buffered_data_source.cc:295: if
(!IsProtocolSupportedForMedia(url_)) {
According to weburlloader_impl.cc we should set the url in the request to an
invalid value.

http://codereview.chromium.org/3863002/diff/1/3#newcode298
webkit/glue/media/buffered_data_source.cc:298: }
nit: indentation is 2 chars.

http://codereview.chromium.org/3863002/diff/1/3#newcode410
webkit/glue/media/buffered_data_source.cc:410: //content_length_ =
info.content_length;
WebURLResponse should have equivalent information for these variables.

http://codereview.chromium.org/3863002/diff/1/3#newcode466
webkit/glue/media/buffered_data_source.cc:466: // Writes more data to |buffer_|.
nit: indentation is 2 chars.

http://codereview.chromium.org/3863002/diff/1/3#newcode567
webkit/glue/media/buffered_data_source.cc:567: //  DoneStart(status.os_error());
you can do DoneStart(net::OK); since didFinishLoading means success.

http://codereview.chromium.org/3863002/diff/1/3#newcode577
webkit/glue/media/buffered_data_source.cc:577: /** (annacc ?): same as above,
don't know how to get status */
didFinishLoading means success.

http://codereview.chromium.org/3863002/diff/1/3#newcode605
webkit/glue/media/buffered_data_source.cc:605: /** (annacc ?): - don't know if
this shuld be here **/
Copy most of logic OnCompleteRequest except this is the failure case.

http://codereview.chromium.org/3863002/diff/1/3#newcode781
webkit/glue/media/buffered_data_source.cc:781:
BufferedDataSource::BufferedDataSource(
Should have just one constructor.

http://codereview.chromium.org/3863002/diff/1/4
File webkit/glue/media/buffered_data_source.h (right):

http://codereview.chromium.org/3863002/diff/1/4#newcode42
webkit/glue/media/buffered_data_source.h:42: public
webkit_glue::ResourceLoaderBridge::Peer,
Can remove this interface.

http://codereview.chromium.org/3863002/diff/1/4#newcode53
webkit/glue/media/buffered_data_source.h:53: WebKit::WebURLLoader* url_loader,
nit: indentation is 4 chars.

http://codereview.chromium.org/3863002/diff/1/4#newcode288
webkit/glue/media/buffered_data_source.h:288: static media::FilterFactory*
CreateFactory(
Should have only one constructor.

http://codereview.chromium.org/3863002/diff/1/4#newcode326
webkit/glue/media/buffered_data_source.h:326: BufferedDataSource(
Should have only one constructor.

[annacc, 2010-11-18 06:18:58]

I think the last email did not include my comments.  Sorry for the double up...

Refactoring BufferedDataSource to work with WebURLLoader instead of a
MediaResourceLoaderBridge. + 2 unit test modifications.

One thing to notice is that both buffered_data_source_unittest and
simple_data_source_unittest need to have a way to inject a MockWebURLLoader into
the BufferedResourceLoader and the SimpleDataSource.  In order to make sure a
new one is not created during a Start(), I introduced the function
SetURLLoaderForTest and keep_test_loader flag.

BUG=16751
TEST=
src/xcodebuild/Debug/test_shell_tests --gtest_filter=Buffered*
src/xcodebuild/Debug/test_shell_tests --gtest_filter=Simple*
src/webkit/tools/layout_tests/run_webkit_tests.sh --debug media
webkit/tools/layout_tests/run_webkit_tests.sh --debug http/tests/media
(but arg, the video-referer.html test still fails, any ideas?)

[Alpha Left Google, 2010-11-19 22:53:40]

Wow! This is a big change!

Good job doing this and it looks very solid to me. I have some small comments
inline.

http://codereview.chromium.org/3863002/diff/18001/chrome/renderer/render_view.cc
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/3863002/diff/18001/chrome/renderer/render_view...
chrome/renderer/render_view.cc:2665: /*
Please add michaeln to review this block, it could be that WebFrame already
handles this.

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

http://codereview.chromium.org/3863002/diff/18001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.cc:263: //      
WebKit::WebURLLoaderClient implementations.
move this back to the previous line.

http://codereview.chromium.org/3863002/diff/18001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.cc:294: }
Add NOTIMPLEMENTED();

http://codereview.chromium.org/3863002/diff/18001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.cc:351: DCHECK(data_length > 0);
DCHECK_GT(data_length, 0);

http://codereview.chromium.org/3863002/diff/18001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.cc:384: }
Use NOTIMPLEMENTED(); instead.

http://codereview.chromium.org/3863002/diff/18001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.cc:388: DCHECK(!completed_);
Use NOTIMPLEMENTED(); instead.

http://codereview.chromium.org/3863002/diff/18001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.cc:558: bool
BufferedResourceLoader::ReadContentRanges(
Instead of adding this method what about use ReadContentRanges() in
MultipartResponseDelegate. You can add content_range_instance_size and test it.
It is best to avoid copying code.

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

http://codereview.chromium.org/3863002/diff/18001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.h:37: // web url loader and does the
actual resource loading.  This object does
web url loader -> WebURLLoader
remove the extra space.

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

http://codereview.chromium.org/3863002/diff/18001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source_unittest.cc:147:
WebString::fromUTF8(base::StringPrintf("bytes "
nit: indentation

http://codereview.chromium.org/3863002/diff/18001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source_unittest.cc:283: "%d-%d/%d", 1, 10,
1024)));
nit: indentation

http://codereview.chromium.org/3863002/diff/18001/webkit/glue/media/mock_webf...
File webkit/glue/media/mock_webframe.h (right):

http://codereview.chromium.org/3863002/diff/18001/webkit/glue/media/mock_webf...
webkit/glue/media/mock_webframe.h:5: #ifndef WEBKIT_GLUE_MEDIA_MOCK_WEBFRAME_H_
you can put this file to webkit/glue instead of inside media.

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

http://codereview.chromium.org/3863002/diff/18001/webkit/glue/media/simple_da...
webkit/glue/media/simple_data_source.cc:163: // Otherwise we should be
initializing and have created a bridge.
nit: we don't use bridge any more.

http://codereview.chromium.org/3863002/diff/18001/webkit/glue/media/simple_da...
webkit/glue/media/simple_data_source.cc:185: // Otherwise we should be
initializing and have created a bridge.
nit: we don't use bridge any more.

http://codereview.chromium.org/3863002/diff/18001/webkit/support/webkit_suppo...
File webkit/support/webkit_support.cc (right):

http://codereview.chromium.org/3863002/diff/18001/webkit/support/webkit_suppo...
webkit/support/webkit_support.cc:270: /*
please add michaeln to look at this block.

http://codereview.chromium.org/3863002/diff/18001/webkit/tools/test_shell/tes...
File webkit/tools/test_shell/test_shell.gypi (right):

http://codereview.chromium.org/3863002/diff/18001/webkit/tools/test_shell/tes...
webkit/tools/test_shell/test_shell.gypi:405: '../../glue/media/mock_webframe.h',
move this file to glue/mock_webframe.h.

http://codereview.chromium.org/3863002/diff/18001/webkit/tools/test_shell/tes...
File webkit/tools/test_shell/test_webview_delegate.cc (right):

http://codereview.chromium.org/3863002/diff/18001/webkit/tools/test_shell/tes...
webkit/tools/test_shell/test_webview_delegate.cc:729: /*
add michaeln to look at this code

[annacc, 2010-11-20 00:27:09]

+michaeln

Michael, could you please comment on the following:

render_view.cc line 2665
webkit_support.cc line 270
test_webview_delegate line 729

Thanks!
-Anna

[scherkus (not reviewing), 2010-11-22 21:27:29]

lots of style nits and overall looking good!

I'd probably axe the OnXXXDestroy() expectations and rely on valgrind instead
(makes our lives easier)

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

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.cc:131: if (frame == NULL)
do we know if frame should ever be NULL?

what's scary here is we won't execute start_callback, meaning the whole
operation will stall

it might be safe to add a DCHECK for frame to see if this condition can happen

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.cc:151:
WebString::fromUTF8(GenerateHeaders(
watch indentation here

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.cc:299: WebURLLoader* loader, const
WebURLResponse& response) {
2nd argument should be dropped to next line

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.cc:320: error =
net::ERR_INVALID_RESPONSE;
fix indentation here

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.cc:351: WebURLLoader* loader, const char*
data, int data_length) {
2nd and 3rd parameters should be dropped to next lines

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.cc:385: WebKit::WebURLLoader* loader, int
dataLength) {
ditto here

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.cc:390: WebURLLoader* loader, const char*
data, int data_length) {
ditto

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.cc:395: WebURLLoader* loader, double
finishTime) {
ditto

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.cc:561: int64 first_byte_position, int64
last_byte_position) {
ditto

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

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.h:22: #include
"third_party/WebKit/WebKit/chromium/public/WebFrame.h"
I'd also try to remove as many of these headers as possible and instead forward
declare any pointer-types

Not 100% sure but it looks like at a minimum you'll need WebURLLoaderClient
where as the rest can be forward declared like so:
namespace WebKit {
class WebFrame;
class WebURLLoader;
class WebURLRequest;
class WebURLResponse;
}  // namespace WebKit

Reason for doing this is helps compilation time

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.h:30: using WebKit::WebFrame;
I don't think you need this as it seems you always use WebKit::WebFrame as
opposed to simply WebFrame

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.h:51: const GURL& url,
the first param should be on same line as function name, with parameters wrapped
at the (

for example, look at the formatting of Start()

(we only do the "drop to next line + indent by 4" if an argument is reallllly
long)

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.h:247: bool keep_test_loader;
member variables should end with _

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.h:255: MessageLoop* render_loop,
ditto on argument style here

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

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source_unittest.cc:67: class TestWebFrameClient
: public WebFrameClient {
Is this used anywhere?  I did a quick check and didn't find any references

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source_unittest.cc:92:
~BufferedResourceLoaderTest() {
virtual

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source_unittest.cc:94: EXPECT_CALL(*frame_,
OnFrameDestroy())
these days I recommend against these kind of expectations

we can rely on valgrind to catch such errors and this way it'll simplify our
testing

I'd remove both OnFrameDestroy() and OnURLDestroy() from your mocks + tests

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source_unittest.cc:130:
WebString::fromUTF8(base::StringPrintf("%"
indentation

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source_unittest.cc:163:
.WillOnce(RequestCanceled(loader_));
indentation

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source_unittest.cc:283: "%d-%d/%d", 1, 10,
1024)));
indentation (this should be indented by an extra 4 spaces OR you could see if
base::StringPrintf can be moved to this line + and it'll all fit

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

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/simple_da...
webkit/glue/media/simple_data_source.cc:135: WebKit::WebURLLoader* loader, const
WebKit::WebURLResponse& response) {
drop 2nd arg onto next line

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/simple_da...
webkit/glue/media/simple_data_source.cc:141: WebKit::WebURLLoader* loader, int
dataLength) {
drop 2nd arg onto next line

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/simple_da...
webkit/glue/media/simple_data_source.cc:145: WebKit::WebURLLoader* loader, const
char* data, int data_length) {
drop 2nd/3rd arg onto next line

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/simple_da...
webkit/glue/media/simple_data_source.cc:151: WebKit::WebURLLoader* loader, const
char* data, int dataLength) {
drop 2nd/3rd arg onto next line

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/simple_da...
webkit/glue/media/simple_data_source.cc:155: WebKit::WebURLLoader* loader,
double finishTime) {
drop 2nd arg onto next line

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/simple_da...
File webkit/glue/media/simple_data_source.h (right):

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/simple_da...
webkit/glue/media/simple_data_source.h:19: #include
"third_party/WebKit/WebKit/chromium/public/WebFrame.h"
ditto here as in buffered_data_source.cc (removing headers and using forward
declares)

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/simple_da...
webkit/glue/media/simple_data_source.h:29: using WebKit::WebFrame;
ditto -- don't think you need this since you're always using WebKit:: prefix

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/simple_da...
webkit/glue/media/simple_data_source.h:37: MessageLoop* render_loop,
argument wrapping style.. in fact I think this whole thing can fit on a single
line

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/simple_da...
webkit/glue/media/simple_data_source.h:133: bool keep_test_loader;
member variables end with _

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/simple_da...
File webkit/glue/media/simple_data_source_unittest.cc (right):

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/simple_da...
webkit/glue/media/simple_data_source_unittest.cc:60: EXPECT_CALL(*frame_,
OnFrameDestroy())
try to use {} for non-one-liner ifs

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/mock_webframe.h
File webkit/glue/mock_webframe.h (right):

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/mock_webframe.h...
webkit/glue/mock_webframe.h:47: class MockWebFrame : public WebKit::WebFrame {
hrmm.. I'm not sure if this is possible but you might have been able to extend
WebKit::WebFrameImpl and use gmock to override the 3 methods we cared about

however I think that breaks the abstraction rules about "knowing" about
implementations.... hmm.....

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/mock_webframe.h...
webkit/glue/mock_webframe.h:344: private:
public/protected/private labels indented by 1 space

http://codereview.chromium.org/3863002/diff/27002/webkit/tools/test_shell/tes...
File webkit/tools/test_shell/test_webview_delegate.cc (right):

http://codereview.chromium.org/3863002/diff/27002/webkit/tools/test_shell/tes...
webkit/tools/test_shell/test_webview_delegate.cc:738: client,
collection.release(), frame,
all these args can fit on a single line

[annacc, 2010-11-24 21:34:44]

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

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.cc:131: if (frame == NULL)
On 2010/11/22 21:27:29, scherkus wrote:
> do we know if frame should ever be NULL?
> 
> what's scary here is we won't execute start_callback, meaning the whole
> operation will stall
> 
> it might be safe to add a DCHECK for frame to see if this condition can happen

|frame_| is detached by setting to NULL in Abort().  So, I figured if there was
any chance of a Start() getting accidentally called after an Abort(), we better
check for that.  Advice on how to better handle this is appreciated.

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.cc:151:
WebString::fromUTF8(GenerateHeaders(
On 2010/11/22 21:27:29, scherkus wrote:
> watch indentation here

Done.

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.cc:299: WebURLLoader* loader, const
WebURLResponse& response) {
On 2010/11/22 21:27:29, scherkus wrote:
> 2nd argument should be dropped to next line

Done.

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.cc:320: error =
net::ERR_INVALID_RESPONSE;
On 2010/11/22 21:27:29, scherkus wrote:
> fix indentation here

Done.

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.cc:351: WebURLLoader* loader, const char*
data, int data_length) {
On 2010/11/22 21:27:29, scherkus wrote:
> 2nd and 3rd parameters should be dropped to next lines

Done.

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.cc:385: WebKit::WebURLLoader* loader, int
dataLength) {
On 2010/11/22 21:27:29, scherkus wrote:
> ditto here

Done.

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.cc:390: WebURLLoader* loader, const char*
data, int data_length) {
On 2010/11/22 21:27:29, scherkus wrote:
> ditto

Done.

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.cc:395: WebURLLoader* loader, double
finishTime) {
On 2010/11/22 21:27:29, scherkus wrote:
> ditto

Done.

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.cc:561: int64 first_byte_position, int64
last_byte_position) {
On 2010/11/22 21:27:29, scherkus wrote:
> ditto

Done.

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

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.h:22: #include
"third_party/WebKit/WebKit/chromium/public/WebFrame.h"
On 2010/11/22 21:27:29, scherkus wrote:
> I'd also try to remove as many of these headers as possible and instead
forward
> declare any pointer-types
> 
> Not 100% sure but it looks like at a minimum you'll need WebURLLoaderClient
> where as the rest can be forward declared like so:
> namespace WebKit {
> class WebFrame;
> class WebURLLoader;
> class WebURLRequest;
> class WebURLResponse;
> }  // namespace WebKit
> 
> Reason for doing this is helps compilation time

Done for WebURLResponse.  All others are explicitly used.

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.h:30: using WebKit::WebFrame;
On 2010/11/22 21:27:29, scherkus wrote:
> I don't think you need this as it seems you always use WebKit::WebFrame as
> opposed to simply WebFrame

Done.

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.h:51: const GURL& url,
On 2010/11/22 21:27:29, scherkus wrote:
> the first param should be on same line as function name, with parameters
wrapped
> at the (
> 
> for example, look at the formatting of Start()
> 
> (we only do the "drop to next line + indent by 4" if an argument is reallllly
> long)

Done.

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.h:247: bool keep_test_loader;
On 2010/11/22 21:27:29, scherkus wrote:
> member variables should end with _

Done.

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.h:255: MessageLoop* render_loop,
On 2010/11/22 21:27:29, scherkus wrote:
> ditto on argument style here

Done.

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

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source_unittest.cc:67: class TestWebFrameClient
: public WebFrameClient {
On 2010/11/22 21:27:29, scherkus wrote:
> Is this used anywhere?  I did a quick check and didn't find any references

Ha!  Thanks, I decided against using it and then forgot to remove.  Done.

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source_unittest.cc:92:
~BufferedResourceLoaderTest() {
On 2010/11/22 21:27:29, scherkus wrote:
> virtual

Done.

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source_unittest.cc:94: EXPECT_CALL(*frame_,
OnFrameDestroy())
On 2010/11/22 21:27:29, scherkus wrote:
> these days I recommend against these kind of expectations
> 
> we can rely on valgrind to catch such errors and this way it'll simplify our
> testing
> 
> I'd remove both OnFrameDestroy() and OnURLDestroy() from your mocks + tests

Done.

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source_unittest.cc:130:
WebString::fromUTF8(base::StringPrintf("%"
On 2010/11/22 21:27:29, scherkus wrote:
> indentation

Done.

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source_unittest.cc:163:
.WillOnce(RequestCanceled(loader_));
On 2010/11/22 21:27:29, scherkus wrote:
> indentation

Done.

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source_unittest.cc:283: "%d-%d/%d", 1, 10,
1024)));
On 2010/11/22 21:27:29, scherkus wrote:
> indentation (this should be indented by an extra 4 spaces OR you could see if
> base::StringPrintf can be moved to this line + and it'll all fit

Done.

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

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/simple_da...
webkit/glue/media/simple_data_source.cc:135: WebKit::WebURLLoader* loader, const
WebKit::WebURLResponse& response) {
On 2010/11/22 21:27:29, scherkus wrote:
> drop 2nd arg onto next line

Done.

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/simple_da...
webkit/glue/media/simple_data_source.cc:141: WebKit::WebURLLoader* loader, int
dataLength) {
On 2010/11/22 21:27:29, scherkus wrote:
> drop 2nd arg onto next line

Done.

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/simple_da...
webkit/glue/media/simple_data_source.cc:145: WebKit::WebURLLoader* loader, const
char* data, int data_length) {
On 2010/11/22 21:27:29, scherkus wrote:
> drop 2nd/3rd arg onto next line

Done.

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/simple_da...
webkit/glue/media/simple_data_source.cc:151: WebKit::WebURLLoader* loader, const
char* data, int dataLength) {
On 2010/11/22 21:27:29, scherkus wrote:
> drop 2nd/3rd arg onto next line

Done.

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/simple_da...
webkit/glue/media/simple_data_source.cc:155: WebKit::WebURLLoader* loader,
double finishTime) {
On 2010/11/22 21:27:29, scherkus wrote:
> drop 2nd arg onto next line

Done.

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/simple_da...
File webkit/glue/media/simple_data_source.h (right):

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/simple_da...
webkit/glue/media/simple_data_source.h:19: #include
"third_party/WebKit/WebKit/chromium/public/WebFrame.h"
On 2010/11/22 21:27:29, scherkus wrote:
> ditto here as in buffered_data_source.cc (removing headers and using forward
> declares)

Nope, I really do need them explicitly.

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/simple_da...
webkit/glue/media/simple_data_source.h:29: using WebKit::WebFrame;
On 2010/11/22 21:27:29, scherkus wrote:
> ditto -- don't think you need this since you're always using WebKit:: prefix

Done.

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/simple_da...
webkit/glue/media/simple_data_source.h:37: MessageLoop* render_loop,
On 2010/11/22 21:27:29, scherkus wrote:
> argument wrapping style.. in fact I think this whole thing can fit on a single
> line

Done.

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/simple_da...
webkit/glue/media/simple_data_source.h:133: bool keep_test_loader;
On 2010/11/22 21:27:29, scherkus wrote:
> member variables end with _

Done.

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/simple_da...
File webkit/glue/media/simple_data_source_unittest.cc (right):

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/media/simple_da...
webkit/glue/media/simple_data_source_unittest.cc:60: EXPECT_CALL(*frame_,
OnFrameDestroy())
On 2010/11/22 21:27:29, scherkus wrote:
> try to use {} for non-one-liner ifs

Changed to no longer expect a OnDestroy().  Now just releases the web frame.

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/mock_webframe.h
File webkit/glue/mock_webframe.h (right):

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/mock_webframe.h...
webkit/glue/mock_webframe.h:47: class MockWebFrame : public WebKit::WebFrame {
On 2010/11/22 21:27:29, scherkus wrote:
> hrmm.. I'm not sure if this is possible but you might have been able to extend
> WebKit::WebFrameImpl and use gmock to override the 3 methods we cared about
> 
> however I think that breaks the abstraction rules about "knowing" about
> implementations.... hmm.....

Will leave as is and post to chromium-dev once it's submitted in case anyone
else might want to use it.

http://codereview.chromium.org/3863002/diff/27002/webkit/glue/mock_webframe.h...
webkit/glue/mock_webframe.h:344: private:
On 2010/11/22 21:27:29, scherkus wrote:
> public/protected/private labels indented by 1 space

Done.

http://codereview.chromium.org/3863002/diff/27002/webkit/tools/test_shell/tes...
File webkit/tools/test_shell/test_webview_delegate.cc (right):

http://codereview.chromium.org/3863002/diff/27002/webkit/tools/test_shell/tes...
webkit/tools/test_shell/test_webview_delegate.cc:738: client,
collection.release(), frame,
On 2010/11/22 21:27:29, scherkus wrote:
> all these args can fit on a single line

Done.

[Alpha Left Google, 2010-11-29 19:55:19]

LGTM.

[scherkus (not reviewing), 2010-11-30 20:05:39]

tiny nits, but we should also get that WebApplicationCacheHostImpl stuff sorted
out before committing

http://codereview.chromium.org/3863002/diff/52001/chrome/renderer/render_view.cc
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/3863002/diff/52001/chrome/renderer/render_view...
chrome/renderer/render_view.cc:2666: WebApplicationCacheHostImpl* appcache_host
=
has this been addressed yet?

I'd ping michaeln over IM

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

http://codereview.chromium.org/3863002/diff/52001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.cc:131: if (frame == NULL)
I'd make this a CHECK instead

tracing through the code it looks like all calls of Start() are protected by a
check of stopped_on_render_loop_, which is set to true the same time frame is
set to NULL

http://codereview.chromium.org/3863002/diff/52001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.cc:264: // BufferedResourceLoader,
nit: this comment can fit on a single line

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

http://codereview.chromium.org/3863002/diff/52001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.h:38: // WebUrlLoader and does the actual
resource loading. This object does
nit: WebUrlLoader -> WebURLLoader

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

http://codereview.chromium.org/3863002/diff/52001/webkit/glue/media/simple_da...
webkit/glue/media/simple_data_source.cc:124: url_loader_.reset(loader);
do we have to worry about keeping the test loader here?

http://codereview.chromium.org/3863002/diff/52001/webkit/glue/media/simple_da...
webkit/glue/media/simple_data_source.cc:144: }
NOTIMPLEMENTED() for these methods?

do we expect these to get called?

http://codereview.chromium.org/3863002/diff/52001/webkit/support/webkit_suppo...
File webkit/support/webkit_support.cc (right):

http://codereview.chromium.org/3863002/diff/52001/webkit/support/webkit_suppo...
webkit/support/webkit_support.cc:271: appcache::WebApplicationCacheHostImpl*
appcache_host =
ditto for this

http://codereview.chromium.org/3863002/diff/52001/webkit/support/webkit_suppo...
webkit/support/webkit_support.cc:279: client, collection.release(), frame,
nit: I think you can fit the params on a single line

http://codereview.chromium.org/3863002/diff/52001/webkit/tools/test_shell/tes...
File webkit/tools/test_shell/test_webview_delegate.cc (right):

http://codereview.chromium.org/3863002/diff/52001/webkit/tools/test_shell/tes...
webkit/tools/test_shell/test_webview_delegate.cc:730:
appcache::WebApplicationCacheHostImpl* appcache_host =
ditto

[annacc, 2010-11-30 21:51:44]

http://codereview.chromium.org/3863002/diff/52001/chrome/renderer/render_view.cc
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/3863002/diff/52001/chrome/renderer/render_view...
chrome/renderer/render_view.cc:2666: WebApplicationCacheHostImpl* appcache_host
=
On 2010/11/30 20:05:39, scherkus wrote:
> has this been addressed yet?
> 
> I'd ping michaeln over IM

Will do.

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

http://codereview.chromium.org/3863002/diff/52001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.cc:131: if (frame == NULL)
On 2010/11/30 20:05:39, scherkus wrote:
> I'd make this a CHECK instead
> 
> tracing through the code it looks like all calls of Start() are protected by a
> check of stopped_on_render_loop_, which is set to true the same time frame is
> set to NULL
> 

Done.

http://codereview.chromium.org/3863002/diff/52001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.cc:264: // BufferedResourceLoader,
On 2010/11/30 20:05:39, scherkus wrote:
> nit: this comment can fit on a single line

Done.

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

http://codereview.chromium.org/3863002/diff/52001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.h:38: // WebUrlLoader and does the actual
resource loading. This object does
On 2010/11/30 20:05:39, scherkus wrote:
> nit: WebUrlLoader -> WebURLLoader

Done.

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

http://codereview.chromium.org/3863002/diff/52001/webkit/glue/media/simple_da...
webkit/glue/media/simple_data_source.cc:124: url_loader_.reset(loader);
On 2010/11/30 20:05:39, scherkus wrote:
> do we have to worry about keeping the test loader here?

Probably not.  I've removed this line.

http://codereview.chromium.org/3863002/diff/52001/webkit/glue/media/simple_da...
webkit/glue/media/simple_data_source.cc:144: }
On 2010/11/30 20:05:39, scherkus wrote:
> NOTIMPLEMENTED() for these methods?
> 
> do we expect these to get called?

No. didSendData() is only used for uploading and didDownloadData() is only used
for downloading to a file, which I believe are not needed for
Simple/BufferedDataSource and resource loading.  But, correct me if I'm wrong. 
Added the NOTIMPLEMENTED()s.

http://codereview.chromium.org/3863002/diff/52001/webkit/support/webkit_suppo...
File webkit/support/webkit_support.cc (right):

http://codereview.chromium.org/3863002/diff/52001/webkit/support/webkit_suppo...
webkit/support/webkit_support.cc:271: appcache::WebApplicationCacheHostImpl*
appcache_host =
On 2010/11/30 20:05:39, scherkus wrote:
> ditto for this

Will do.

http://codereview.chromium.org/3863002/diff/52001/webkit/support/webkit_suppo...
webkit/support/webkit_support.cc:279: client, collection.release(), frame,
On 2010/11/30 20:05:39, scherkus wrote:
> nit: I think you can fit the params on a single line

Done.

http://codereview.chromium.org/3863002/diff/52001/webkit/tools/test_shell/tes...
File webkit/tools/test_shell/test_webview_delegate.cc (right):

http://codereview.chromium.org/3863002/diff/52001/webkit/tools/test_shell/tes...
webkit/tools/test_shell/test_webview_delegate.cc:730:
appcache::WebApplicationCacheHostImpl* appcache_host =
On 2010/11/30 20:05:39, scherkus wrote:
> ditto

Will do.

[scherkus (not reviewing), 2010-11-30 23:18:06]

one tiny indentation nit

when we get the appcache stuff sorted out let's check it in!

http://codereview.chromium.org/3863002/diff/84001/webkit/glue/mock_webframe.h
File webkit/glue/mock_webframe.h (right):

http://codereview.chromium.org/3863002/diff/84001/webkit/glue/mock_webframe.h...
webkit/glue/mock_webframe.h:212: return NULL;
nit: indentation

[annacc, 2010-11-30 23:32:53]

http://codereview.chromium.org/3863002/diff/84001/webkit/glue/mock_webframe.h
File webkit/glue/mock_webframe.h (right):

http://codereview.chromium.org/3863002/diff/84001/webkit/glue/mock_webframe.h...
webkit/glue/mock_webframe.h:212: return NULL;
On 2010/11/30 23:18:06, scherkus wrote:
> nit: indentation

Done.

[annacc, 2010-11-30 23:42:55]

Hi Michael,

Alpha suggested I ask you about our use of WebApplicationCacheHostImpl.  I'd
really appreciate your suggestions on how we should handle appcache.

There are only 3 relevant lines of code:
render_view.cc line 2819
webkit_support.cc line 271
test_webview_delegate line 730

Thanks so much!
-Anna

[scherkus (not reviewing), 2010-12-02 23:01:17]

Committed as r68094.