[NaCl SDK] Add URLLoader fake pepper interface.

[NaCl SDK] Add URLLoader fake pepper interface.

BUG=244313, 245431
R=sbc@chromium.org

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

[Sam Clegg, 2013-11-20 20:00:53]

lgtm..

Did you try running the TOOLCHAIN=linux STANDALONE=1 build?

https://codereview.chromium.org/77423002/diff/40001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/mount_node_http.cc (right):

https://codereview.chromium.org/77423002/diff/40001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_http.cc:415: count = size -
offs;
So this function can no longer fail?  Might be worth changing
the signature accordingly.

Can you explain this part of the change a bit?

https://codereview.chromium.org/77423002/diff/40001/native_client_sdk/src/tes...
File
native_client_sdk/src/tests/nacl_io_test/fake_pepper_interface_url_loader.cc
(right):

https://codereview.chromium.org/77423002/diff/40001/native_client_sdk/src/tes...
native_client_sdk/src/tests/nacl_io_test/fake_pepper_interface_url_loader.cc:355:
core_interface_->resource_manager()->Get<FakeInstanceResource>(instance);
Seeing these long and duplicated "this" accessors at the beginning
of each method makes me to want to do something different...
maybe even a macro :(

https://codereview.chromium.org/77423002/diff/40001/native_client_sdk/src/tes...
File native_client_sdk/src/tests/nacl_io_test/fake_pepper_interface_url_loader.h
(right):

https://codereview.chromium.org/77423002/diff/40001/native_client_sdk/src/tes...
native_client_sdk/src/tests/nacl_io_test/fake_pepper_interface_url_loader.h:1:
// Copyright (c) 2013 The Chromium Authors. All rights reserved.
Should we put all the fake pepper code in a "fake_pepper" folder
rather than prefixing each file?

https://codereview.chromium.org/77423002/diff/40001/native_client_sdk/src/tes...
File native_client_sdk/src/tests/nacl_io_test/mount_http_test.cc (right):

https://codereview.chromium.org/77423002/diff/40001/native_client_sdk/src/tes...
native_client_sdk/src/tests/nacl_io_test/mount_http_test.cc:190: EXPECT_EQ(0,
mnt.ParseManifest(manifest));
Assert here and other places there ParseManifest is part
of the test setup?

https://codereview.chromium.org/77423002/diff/40001/native_client_sdk/src/tes...
native_client_sdk/src/tests/nacl_io_test/mount_http_test.cc:287:
EXPECT_TRUE(ppapi_.server_template()->AddEntity("file", contents, NULL));
Assert rather than expect here and other places where you use
AddEntity and AddError?

https://codereview.chromium.org/77423002/diff/40001/native_client_sdk/src/tes...
native_client_sdk/src/tests/nacl_io_test/mount_http_test.cc:310:
EXPECT_TRUE(ppapi_.server_template()->AddEntity("file", contents, NULL));
Assert here?

[binji, 2013-11-20 20:34:16]

https://codereview.chromium.org/77423002/diff/40001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/mount_node_http.cc (right):

https://codereview.chromium.org/77423002/diff/40001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_http.cc:415: count = size -
offs;
On 2013/11/20 20:00:54, Sam Clegg wrote:
> So this function can no longer fail?  Might be worth changing
> the signature accordingly.

Hm. I like that it has the same signature as the rest of the read functions
(actually, it should probably change to use HandleAttr instead of offset...)

> 
> Can you explain this part of the change a bit?

It was a bug that it was failing at all. It is not an error to read past the end
of the data, you should just get 0 bytes read.

https://codereview.chromium.org/77423002/diff/40001/native_client_sdk/src/tes...
File
native_client_sdk/src/tests/nacl_io_test/fake_pepper_interface_url_loader.cc
(right):

https://codereview.chromium.org/77423002/diff/40001/native_client_sdk/src/tes...
native_client_sdk/src/tests/nacl_io_test/fake_pepper_interface_url_loader.cc:355:
core_interface_->resource_manager()->Get<FakeInstanceResource>(instance);
On 2013/11/20 20:00:54, Sam Clegg wrote:
> Seeing these long and duplicated "this" accessors at the beginning
> of each method makes me to want to do something different...
> maybe even a macro :(

I could add a helper function. But I'd rather do it in another CL, because it
will affect html5 fake too.

https://codereview.chromium.org/77423002/diff/40001/native_client_sdk/src/tes...
File native_client_sdk/src/tests/nacl_io_test/fake_pepper_interface_url_loader.h
(right):

https://codereview.chromium.org/77423002/diff/40001/native_client_sdk/src/tes...
native_client_sdk/src/tests/nacl_io_test/fake_pepper_interface_url_loader.h:1:
// Copyright (c) 2013 The Chromium Authors. All rights reserved.
On 2013/11/20 20:00:54, Sam Clegg wrote:
> Should we put all the fake pepper code in a "fake_pepper" folder
> rather than prefixing each file?

I was thinking the same thing, though we'd still want to prefix the files; it
wouldn't make sense to be just "fake_url_loader", because that's not what it is.

https://codereview.chromium.org/77423002/diff/40001/native_client_sdk/src/tes...
File native_client_sdk/src/tests/nacl_io_test/mount_http_test.cc (right):

https://codereview.chromium.org/77423002/diff/40001/native_client_sdk/src/tes...
native_client_sdk/src/tests/nacl_io_test/mount_http_test.cc:190: EXPECT_EQ(0,
mnt.ParseManifest(manifest));
On 2013/11/20 20:00:54, Sam Clegg wrote:
> Assert here and other places there ParseManifest is part
> of the test setup?

Done.

https://codereview.chromium.org/77423002/diff/40001/native_client_sdk/src/tes...
native_client_sdk/src/tests/nacl_io_test/mount_http_test.cc:287:
EXPECT_TRUE(ppapi_.server_template()->AddEntity("file", contents, NULL));
On 2013/11/20 20:00:54, Sam Clegg wrote:
> Assert rather than expect here and other places where you use
> AddEntity and AddError?

Done.

https://codereview.chromium.org/77423002/diff/40001/native_client_sdk/src/tes...
native_client_sdk/src/tests/nacl_io_test/mount_http_test.cc:310:
EXPECT_TRUE(ppapi_.server_template()->AddEntity("file", contents, NULL));
On 2013/11/20 20:00:54, Sam Clegg wrote:
> Assert here?

Done.

[binji, 2013-11-20 23:30:48]

Committed patchset #3 manually as r236343 (presubmit successful).