Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2513423002:
[NaCl SDK] Implement FakeURLRequestInfoInterface::AppendDataToBody (Closed)
|
Created:
4 years, 1 month ago by chanpatorikku Modified:
4 years ago CC:
chromium-reviews, binji+watch_chromium.org, Sam Clegg Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[NaCl SDK] Implement FakeURLRequestInfoInterface::AppendDataToBody
AppendDataToBody is going to be called by the future code, which includes
a fake server. The fake server's going to respond requests sent from
GoogleDriveFs, a file system exposed to nacl_io.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_nacl_sdk;master.tryserver.chromium.mac:mac_nacl_sdk;master.tryserver.chromium.win:win_nacl_sdk
Committed: https://crrev.com/bc3878414c047c5485b4403c2cf682bee9e737da
Cr-Commit-Position: refs/heads/master@{#435228}
Patch Set 1 #
Total comments: 4
Patch Set 2 #Patch Set 3 #
Total comments: 8
Patch Set 4 #
Messages
Total messages: 26 (12 generated)
Description was changed from ========== [NaCl SDK] Implement FakeURLRequestInfoInterface::AppendDataToBody and FakeURLResponseInfoInterface::GetBodyAsFileRef Create FakeFileSystemResource, implement FakeURLRequestInfoInterface::AppendDataToBody, and implement FakeURLResponseInfoInterface::GetBodyAsFileRef. AppendDataToBody and GetBodyAsFileRef are going to be called by the future code, which includes a fake server. The fake server's going to respond requests sent from GoogleDriveFs, a file system exposed to nacl_io. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_nacl_sdk;master.tryserver.chromium.mac:mac_nacl_sdk;master.tryserver.chromium.win:win_nacl_sdk ========== to ========== [NaCl SDK] Implement FakeURLRequestInfoInterface::AppendDataToBody and FakeURLResponseInfoInterface::GetBodyAsFileRef Create FakeFileSystemResource, implement FakeURLRequestInfoInterface::AppendDataToBody, and implement FakeURLResponseInfoInterface::GetBodyAsFileRef. AppendDataToBody and GetBodyAsFileRef are going to be called by the future code, which includes a fake server. The fake server's going to respond requests sent from GoogleDriveFs, a file system exposed to nacl_io. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_nacl_sdk;master.tryserver.chromium.mac:mac_nacl_sdk;master.tryserver.chromium.win:win_nacl_sdk ==========
chanpatorikku@gmail.com changed reviewers: + binji@chromium.org, sbc@chromium.org
Description was changed from
==========
[NaCl SDK] Implement FakeURLRequestInfoInterface::AppendDataToBody and
FakeURLResponseInfoInterface::GetBodyAsFileRef
Create FakeFileSystemResource, implement
FakeURLRequestInfoInterface::AppendDataToBody, and implement
FakeURLResponseInfoInterface::GetBodyAsFileRef. AppendDataToBody and
GetBodyAsFileRef are going to be called by the future code, which includes
a fake server. The fake server's going to respond requests sent from
GoogleDriveFs, a file system exposed to nacl_io.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_nacl_sdk;master.tryserver.chromium.mac:mac_nacl_sdk;master.tryserver.chromium.win:win_nacl_sdk
==========
to
==========
[NaCl SDK] Implement FakeURLRequestInfoInterface::AppendDataToBody and
FakeURLResponseInfoInterface::GetBodyAsFileRef
Create FakeFileSystemResource in fake_pepper_interface_url_loader.{cc,h},
implement FakeURLRequestInfoInterface::AppendDataToBody, and implement
FakeURLResponseInfoInterface::GetBodyAsFileRef. AppendDataToBody and
GetBodyAsFileRef are going to be called by the future code, which includes
a fake server. The fake server's going to respond requests sent from
GoogleDriveFs, a file system exposed to nacl_io.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_nacl_sdk;master.tryserver.chromium.mac:mac_nacl_sdk;master.tryserver.chromium.win:win_nacl_sdk
==========
Description was changed from
==========
[NaCl SDK] Implement FakeURLRequestInfoInterface::AppendDataToBody and
FakeURLResponseInfoInterface::GetBodyAsFileRef
Create FakeFileSystemResource in fake_pepper_interface_url_loader.{cc,h},
implement FakeURLRequestInfoInterface::AppendDataToBody, and implement
FakeURLResponseInfoInterface::GetBodyAsFileRef. AppendDataToBody and
GetBodyAsFileRef are going to be called by the future code, which includes
a fake server. The fake server's going to respond requests sent from
GoogleDriveFs, a file system exposed to nacl_io.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_nacl_sdk;master.tryserver.chromium.mac:mac_nacl_sdk;master.tryserver.chromium.win:win_nacl_sdk
==========
to
==========
[NaCl SDK] Implement FakeURLRequestInfoInterface::AppendDataToBody and
FakeURLResponseInfoInterface::GetBodyAsFileRef
Create FakeFileSystemResource in fake_pepper_interface_url_loader.{cc,h},
implement FakeURLRequestInfoInterface::AppendDataToBody, and implement
FakeURLResponseInfoInterface::GetBodyAsFileRef. AppendDataToBody and
GetBodyAsFileRef are going to be called by the future code, which includes
a fake server. The fake server's going to respond requests sent from
GoogleDriveFs, a file system exposed to nacl_io.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_nacl_sdk;master.tryserver.chromium.mac:mac_nacl_sdk;master.tryserver.chromium.win:win_nacl_sdk
==========
Its another patch set before the PRs of GoogleDriveFs implementation code and test code > ptal
https://codereview.chromium.org/2513423002/diff/1/native_client_sdk/src/tests... File native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_pepper_interface_url_loader.cc (right): https://codereview.chromium.org/2513423002/diff/1/native_client_sdk/src/tests... native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_pepper_interface_url_loader.cc:560: char path_buffer[1024]; See https://developer.chrome.com/native-client/pepper_dev/cpp/classpp_1_1_u_r_l_r..., this file should only be valid if PP_URLREQUESTPROPERTY_STREAMTOFILE is set on the associated URLRequestInfo. Also, it shouldn't create a new fileref every time this is called. Instead, it should create it when first called, and reuse that value on subsequent calls. https://codereview.chromium.org/2513423002/diff/1/native_client_sdk/src/tests... File native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_util.h (right): https://codereview.chromium.org/2513423002/diff/1/native_client_sdk/src/tests... native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_util.h:62: std::string body; where is this assigned a value?
Description was changed from
==========
[NaCl SDK] Implement FakeURLRequestInfoInterface::AppendDataToBody and
FakeURLResponseInfoInterface::GetBodyAsFileRef
Create FakeFileSystemResource in fake_pepper_interface_url_loader.{cc,h},
implement FakeURLRequestInfoInterface::AppendDataToBody, and implement
FakeURLResponseInfoInterface::GetBodyAsFileRef. AppendDataToBody and
GetBodyAsFileRef are going to be called by the future code, which includes
a fake server. The fake server's going to respond requests sent from
GoogleDriveFs, a file system exposed to nacl_io.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_nacl_sdk;master.tryserver.chromium.mac:mac_nacl_sdk;master.tryserver.chromium.win:win_nacl_sdk
==========
to
==========
[NaCl SDK] Implement FakeURLRequestInfoInterface::AppendDataToBody
Implement FakeURLRequestInfoInterface::AppendDataToBody. AppendDataToBody is
going to be called by the future code, which includes
a fake server. The fake server's going to respond requests sent from
GoogleDriveFs, a file system exposed to nacl_io.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_nacl_sdk;master.tryserver.chromium.mac:mac_nacl_sdk;master.tryserver.chromium.win:win_nacl_sdk
==========
Description was changed from ========== [NaCl SDK] Implement FakeURLRequestInfoInterface::AppendDataToBody Implement FakeURLRequestInfoInterface::AppendDataToBody. AppendDataToBody is going to be called by the future code, which includes a fake server. The fake server's going to respond requests sent from GoogleDriveFs, a file system exposed to nacl_io. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_nacl_sdk;master.tryserver.chromium.mac:mac_nacl_sdk;master.tryserver.chromium.win:win_nacl_sdk ========== to ========== [NaCl SDK] Implement FakeURLRequestInfoInterface::AppendDataToBody AppendDataToBody is going to be called by the future code, which includes a fake server. The fake server's going to respond requests sent from GoogleDriveFs, a file system exposed to nacl_io. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_nacl_sdk;master.tryserver.chromium.mac:mac_nacl_sdk;master.tryserver.chromium.win:win_nacl_sdk ==========
Thanks for the comments. ptal https://codereview.chromium.org/2513423002/diff/1/native_client_sdk/src/tests... File native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_pepper_interface_url_loader.cc (right): https://codereview.chromium.org/2513423002/diff/1/native_client_sdk/src/tests... native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_pepper_interface_url_loader.cc:560: char path_buffer[1024]; On 2016/11/21 18:50:47, binji wrote: > See > https://developer.chrome.com/native-client/pepper_dev/cpp/classpp_1_1_u_r_l_r..., > this file should only be valid if PP_URLREQUESTPROPERTY_STREAMTOFILE is set on > the associated URLRequestInfo. > > Also, it shouldn't create a new fileref every time this is called. Instead, it > should create it when first called, and reuse that value on subsequent calls. Going to implement this in another CL. This CL is quickly changed to implement only FakeURLRequestInfoInterface::AppendDataToBody. The reason is that response_resource->body in FakeURLResponseInfoInterface::GetBodyAsFileRef is going to generate duplicate code with FakeURLLoaderEntity, which hurts the quality of the code base. HttpFsTest sends a request and gets a response as an instance of FakeURLResponseInfoResource and an instance of FakeURLLoaderEntity. The two instances are quickly set between the request and the response. The future code is also going to send a request and get a response. The response is going to be an instance of FakeURLResponseInfoResource and a std::string instance. The two instances are quickly set between the request and the response as well. a FakeURLLoaderEntity instance and a std::string instance have duplicate code, so the future code is going to create FakeDriveURLResponseInfoResource, which extends FakeURLResponseInfoResource and has a std::string variable named body. FakeURLResponseInfoInterface::GetBodyAsFileRef speedily reverts back to what it was. The future code is going to safely create FakeDriveURLResponseInfoInterface which extends FakeURLResponseInfoInterface and implements GetBodyAsFileRef. The future code is going to have a fake server. The fake server receives a request and quickly generates a response, which sets the value in the additional variable body in FakeDriveURLResponseInfoResource. Afterwards it is going to be safely used in GetBodyAsFileRef. https://codereview.chromium.org/2513423002/diff/1/native_client_sdk/src/tests... File native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_util.h (right): https://codereview.chromium.org/2513423002/diff/1/native_client_sdk/src/tests... native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_util.h:62: std::string body; On 2016/11/21 18:50:47, binji wrote: > where is this assigned a value? From the previous comment (please look at the previous comment for more details): The future code is going to have a fake server. The fake server receives a request and quickly generates a response, which sets the value in the additional variable body in FakeDriveURLResponseInfoResource.
Some long comment exists to explain why the change was made so more of your time could be focused on your primary work but not making a guess why the change was made. When you are not busy, ptal
Some long comment exists to explain why the change was made so more of your time could be focused on your primary work but not making a guess why the change was made. When you are not busy, ptal
I did not mean to mail double comments. Please ignore.
Patchset #3 (id:40001) has been deleted
My apology. After looking at the comment about GetBodyAsFileRef, I was going to check the implementation information about AppendDataAsBody (https://developer.chrome.com/native-client/pepper_dev/cpp/classpp_1_1_u_r_l_r...), but somehow I did not check and submitted patch set 2. Shouldn't have forgotten. patch set 3 is possessing code of setting contentlength as written in AppendDataAsBody. I can attempt to write a todo to myself to remember when there is a chance and incrementally update my work so I don't submit patch sets that are not very good. ptal
Sam, could you please take a look? I guess binji is unavailable.
lgtm, w/ some small fixes https://codereview.chromium.org/2513423002/diff/60001/native_client_sdk/src/t... File native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_pepper_interface_url_loader.cc (right): https://codereview.chromium.org/2513423002/diff/60001/native_client_sdk/src/t... native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_pepper_interface_url_loader.cc:490: request_resource->body.assign(static_cast<const char*>(data), len); This should append data, not replace it. https://codereview.chromium.org/2513423002/diff/60001/native_client_sdk/src/t... File native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_util.cc (right): https://codereview.chromium.org/2513423002/diff/60001/native_client_sdk/src/t... native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_util.cc:36: std::string out_value_helper; I think old_value is clearer here. https://codereview.chromium.org/2513423002/diff/60001/native_client_sdk/src/t... native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_util.cc:53: current_headers.substr(newline, current_headers.size() - newline); nit: can omit second param here, it will copy to the end of the string. See http://en.cppreference.com/w/cpp/string/basic_string/substr https://codereview.chromium.org/2513423002/diff/60001/native_client_sdk/src/t... File native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_util.h (right): https://codereview.chromium.org/2513423002/diff/60001/native_client_sdk/src/t... native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_util.h:68: void SetHeader(const std::string& current_headers, I think a nicer API would just modify the headers string directly.
The CQ bit was checked by chanpatorikku@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from binji@chromium.org Link to the patchset: https://codereview.chromium.org/2513423002/#ps80001 (title: "")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1480500945253830,
"parent_rev": "86bf03fa45a285853481af40dc8ce7afc4d3e65c", "commit_rev":
"c42b21e5e8c86bd10a218fae718a6a6cad11705b"}
Message was sent while issue was closed.
Description was changed from ========== [NaCl SDK] Implement FakeURLRequestInfoInterface::AppendDataToBody AppendDataToBody is going to be called by the future code, which includes a fake server. The fake server's going to respond requests sent from GoogleDriveFs, a file system exposed to nacl_io. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_nacl_sdk;master.tryserver.chromium.mac:mac_nacl_sdk;master.tryserver.chromium.win:win_nacl_sdk ========== to ========== [NaCl SDK] Implement FakeURLRequestInfoInterface::AppendDataToBody AppendDataToBody is going to be called by the future code, which includes a fake server. The fake server's going to respond requests sent from GoogleDriveFs, a file system exposed to nacl_io. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_nacl_sdk;master.tryserver.chromium.mac:mac_nacl_sdk;master.tryserver.chromium.win:win_nacl_sdk ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [NaCl SDK] Implement FakeURLRequestInfoInterface::AppendDataToBody AppendDataToBody is going to be called by the future code, which includes a fake server. The fake server's going to respond requests sent from GoogleDriveFs, a file system exposed to nacl_io. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_nacl_sdk;master.tryserver.chromium.mac:mac_nacl_sdk;master.tryserver.chromium.win:win_nacl_sdk ========== to ========== [NaCl SDK] Implement FakeURLRequestInfoInterface::AppendDataToBody AppendDataToBody is going to be called by the future code, which includes a fake server. The fake server's going to respond requests sent from GoogleDriveFs, a file system exposed to nacl_io. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_nacl_sdk;master.tryserver.chromium.mac:mac_nacl_sdk;master.tryserver.chromium.win:win_nacl_sdk Committed: https://crrev.com/bc3878414c047c5485b4403c2cf682bee9e737da Cr-Commit-Position: refs/heads/master@{#435228} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/bc3878414c047c5485b4403c2cf682bee9e737da Cr-Commit-Position: refs/heads/master@{#435228}
Message was sent while issue was closed.
Sorry. AppendDataToBody was written to replace data to body. The latest patch has been corrected to append data to body. I think I have been programming with the setter methods, which have very often (if not always) been to replace. I did not realize there can be morethan 1 type of setter methods, which can be replace, append, and so forth. To incrementally update, I can just realize that setter methods can be as append, not replace. https://codereview.chromium.org/2513423002/diff/60001/native_client_sdk/src/t... File native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_pepper_interface_url_loader.cc (right): https://codereview.chromium.org/2513423002/diff/60001/native_client_sdk/src/t... native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_pepper_interface_url_loader.cc:490: request_resource->body.assign(static_cast<const char*>(data), len); On 2016/11/29 20:53:57, binji wrote: > This should append data, not replace it. thanks. very good review comment from you. done to change from replace to append. https://codereview.chromium.org/2513423002/diff/60001/native_client_sdk/src/t... File native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_util.cc (right): https://codereview.chromium.org/2513423002/diff/60001/native_client_sdk/src/t... native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_util.cc:36: std::string out_value_helper; On 2016/11/29 20:53:57, binji wrote: > I think old_value is clearer here. done. https://codereview.chromium.org/2513423002/diff/60001/native_client_sdk/src/t... native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_util.cc:53: current_headers.substr(newline, current_headers.size() - newline); On 2016/11/29 20:53:57, binji wrote: > nit: can omit second param here, it will copy to the end of the string. See > http://en.cppreference.com/w/cpp/string/basic_string/substr done. https://codereview.chromium.org/2513423002/diff/60001/native_client_sdk/src/t... File native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_util.h (right): https://codereview.chromium.org/2513423002/diff/60001/native_client_sdk/src/t... native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_util.h:68: void SetHeader(const std::string& current_headers, On 2016/11/29 20:53:57, binji wrote: > I think a nicer API would just modify the headers string directly. thanks. very good comment from yours again. i didn't know about input/output parameter to be used for out_headers. done coding a nicer api.
Message was sent while issue was closed.
It's fine, but since this CL has already landed, please upload the new changes to a separate review. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
