Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2573523004:
[NaCl SDK] Add more support and fix FakePepperInterfaceGoogleDriveFs (Closed)
|
Created:
4 years 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] Add more support and fix FakePepperInterfaceGoogleDriveFs
Add FakeDriveURLRequestInfoInterface, an FakeFileIoInterface instance, &
FakeDriveURLResponseInfoInterface::GetProperty() to
FakePepperInterfaceGoogleDriveFs, and fix a reference count cycle issue
in FakePepperInterfaceGoogleDriveFs. FakePepperInterfaceGoogleDriveFs
is going to be used by the unit tests of the future code.
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/7385e43d01140ac8de624cf348111257b7138b13
Cr-Commit-Position: refs/heads/master@{#440605}
Patch Set 1 #
Total comments: 3
Messages
Total messages: 16 (8 generated)
Description was changed from ========== [NaCl SDK] Add more support and fix FakePepperInterfaceGoogleDriveFs Add FakeDriveURLRequestInfoInterface & an FakeFileIoInterface instance to FakePepperInterfaceGoogleDriveFs, and fix a reference count cycle in FakePepperInterfaceGoogleDriveFs. FakePepperInterfaceGoogleDriveFs is going to be used by the unit tests of the future code. ========== to ========== [NaCl SDK] Add more support and fix FakePepperInterfaceGoogleDriveFs Add FakeDriveURLRequestInfoInterface & an FakeFileIoInterface instance to FakePepperInterfaceGoogleDriveFs, and fix a reference count cycle in FakePepperInterfaceGoogleDriveFs. FakePepperInterfaceGoogleDriveFs is going to be used by the unit tests of the future code. ==========
chanpatorikku@gmail.com changed reviewers: + binji@chromium.org, sbc@chromium.org
More support and a fix have to be in FakePepperInterfaceGoogleDriveFs after the future code which exposes GoogleDrive to nacl_io is run on it. The future code in nacl_io, even with minimal functionality, will increase a few hundred more lines of code, so to have a review with fewer number of lines of code, I looked at only the unit test codes in the previous review. ptal https://codereview.chromium.org/2573523004/diff/1/native_client_sdk/src/tests... File native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_pepper_interface_googledrivefs.cc (right): https://codereview.chromium.org/2573523004/diff/1/native_client_sdk/src/tests... native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_pepper_interface_googledrivefs.cc:62: PP_Resource file_ref; The previous code "PP_Resource loader" will make the destructor of FakeResourceManager showing a message because loader and response reference each other. In the previous review, I wrote that I read thoroughly on how reference counting in PP_Resource works. I read Create() from FakeResourceManager, AddRef() & Release() from FakeResourceManager & FakeCoreInterface, and Release() from ScopedResource. I didn't know about the destructor of FakeResourceManager showing a message when the reference counts of some resources are > 0. If you had a not very good experience on reviewing the previous code "PP_Resource loader", sorry. The fix changes "PP_Resource loader" to "FakeDriveURLLoaderResource* loader_resource". loader_resource may store a dangling pointer when the FakeDriveURLLoaderResource instance has been freed. The code is test code and it can be coded so a dangling pointer is not going to arise.
Description was changed from ========== [NaCl SDK] Add more support and fix FakePepperInterfaceGoogleDriveFs Add FakeDriveURLRequestInfoInterface & an FakeFileIoInterface instance to FakePepperInterfaceGoogleDriveFs, and fix a reference count cycle in FakePepperInterfaceGoogleDriveFs. FakePepperInterfaceGoogleDriveFs is going to be used by the unit tests of the future code. ========== to ========== [NaCl SDK] Add more support and fix FakePepperInterfaceGoogleDriveFs Add FakeDriveURLRequestInfoInterfac, an FakeFileIoInterface instance, & FakeDriveURLResponseInfoInterface::GetProperty() to FakePepperInterfaceGoogleDriveFs, and fix a reference count cycle issue in FakePepperInterfaceGoogleDriveFs. FakePepperInterfaceGoogleDriveFs is going to be used by the unit tests of the future code. ==========
I commented certain lines about the codes. When you are not busy, ptal https://codereview.chromium.org/2573523004/diff/1/native_client_sdk/src/tests... File native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_pepper_interface_googledrivefs.cc (right): https://codereview.chromium.org/2573523004/diff/1/native_client_sdk/src/tests... native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_pepper_interface_googledrivefs.cc:62: PP_Resource file_ref; On 2016/12/13 16:03:18, chanpatorikku wrote: > The previous code "PP_Resource loader" will make the destructor of > FakeResourceManager showing a message because loader and response reference each > other. > > In the previous review, I wrote that I read thoroughly on how reference counting > in PP_Resource works. I read Create() from FakeResourceManager, AddRef() & > Release() from FakeResourceManager & FakeCoreInterface, and Release() from > ScopedResource. I didn't know about the destructor of FakeResourceManager > showing a message when the reference counts of some resources are > 0. If you > had a not very good experience on reviewing the previous code "PP_Resource > loader", sorry. > > The fix changes "PP_Resource loader" to "FakeDriveURLLoaderResource* > loader_resource". loader_resource may store a dangling pointer when the > FakeDriveURLLoaderResource instance has been freed. The code is test code and > it can be coded so a dangling pointer is not going to arise. Other than "FakeDriveURLLoaderResource* loader_resource", there're 2 other ways of handling how to check to see if the corresponding loader_resource has been closed or destroyed. The 1st way is to convert back to "PP_Resource loader" but do not call AddRef() on the "loader" in Open(). The code may be less understandable when the response has "PP_Resource file_ref" that owns a reference but "PP_Resource loader" that does not own a reference. The 2nd way is to create 2 booleans "bool loader_destroyed; bool loader_closed" in the response. Both booleans are set as true in the constructor of the response. In Open(), both booleans are set to false after the response resource is created. In Close(), "loader_closed" is set to true. In the Destroy() of loader, the response is gotten from Get(), and "loader_destroyed" is set to true. In GetBodyAsFileRef(), either "loader_destroyed == true" or "loader_closed == true" is going to let 0 to be returned. https://codereview.chromium.org/2573523004/diff/1/native_client_sdk/src/tests... native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_pepper_interface_googledrivefs.cc:270: if (drive_response_resource->loader_resource == NULL) "drive_response_resource->loader_resource == NULL" doesn't have to be checked because drive_response_resource->loader_resource is set to a non-NULL in Open() and GetBodyAsFileRef() is called after Open(). It was weird about changing from FakeDriveURLLoaderResource* drive_loader_resource = core_interface_->resource_manager()->Get<FakeDriveURLLoaderResource>(drive_response_resource->loader); if (drive_loader_resource == NULL) return 0; to drive_response_resource->loader_resource == NULL; , and while changing the code, i did not do a verygood work on noticing the difference because i did not understand line by line how Get() worked and thought both codes just checked to see if loader_resource was able to be dereferenced. i just read line by line the related codes of all the related methods modified in this patch set so it won't happen again.
Contacting you again to see if I can catch you on the review this time. When you are not busy, ptal
Description was changed from ========== [NaCl SDK] Add more support and fix FakePepperInterfaceGoogleDriveFs Add FakeDriveURLRequestInfoInterfac, an FakeFileIoInterface instance, & FakeDriveURLResponseInfoInterface::GetProperty() to FakePepperInterfaceGoogleDriveFs, and fix a reference count cycle issue in FakePepperInterfaceGoogleDriveFs. FakePepperInterfaceGoogleDriveFs is going to be used by the unit tests of the future code. ========== to ========== [NaCl SDK] Add more support and fix FakePepperInterfaceGoogleDriveFs Add FakeDriveURLRequestInfoInterface, an FakeFileIoInterface instance, & FakeDriveURLResponseInfoInterface::GetProperty() to FakePepperInterfaceGoogleDriveFs, and fix a reference count cycle issue in FakePepperInterfaceGoogleDriveFs. FakePepperInterfaceGoogleDriveFs is going to be used by the unit tests of the future code. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_nacl_sdk;master.tryserver.chromium.mac:mac_nacl_sdk;master.tryserver.chromium.win:win_nacl_sdk ==========
My apology. There was a typo and a missing CQ_INCLUDE_TRYBOTS. Weird, but fixed. When you are not busy, ptal
lgtm
The CQ bit was checked by chanpatorikku@gmail.com
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": 1, "attempt_start_ts": 1482477500424260, "parent_rev":
"dbc50412b30b86806037d30ffc37c7185eb9fc1b", "commit_rev":
"022a493d04b84c3a465fd55b4e4c5b9e4c0b7833"}
Message was sent while issue was closed.
Description was changed from ========== [NaCl SDK] Add more support and fix FakePepperInterfaceGoogleDriveFs Add FakeDriveURLRequestInfoInterface, an FakeFileIoInterface instance, & FakeDriveURLResponseInfoInterface::GetProperty() to FakePepperInterfaceGoogleDriveFs, and fix a reference count cycle issue in FakePepperInterfaceGoogleDriveFs. FakePepperInterfaceGoogleDriveFs is going to be used by the unit tests of the future code. 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] Add more support and fix FakePepperInterfaceGoogleDriveFs Add FakeDriveURLRequestInfoInterface, an FakeFileIoInterface instance, & FakeDriveURLResponseInfoInterface::GetProperty() to FakePepperInterfaceGoogleDriveFs, and fix a reference count cycle issue in FakePepperInterfaceGoogleDriveFs. FakePepperInterfaceGoogleDriveFs is going to be used by the unit tests of the future code. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_nacl_sdk;master.tryserver.chromium.mac:mac_nacl_sdk;master.tryserver.chromium.win:win_nacl_sdk Review-Url: https://codereview.chromium.org/2573523004 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== [NaCl SDK] Add more support and fix FakePepperInterfaceGoogleDriveFs Add FakeDriveURLRequestInfoInterface, an FakeFileIoInterface instance, & FakeDriveURLResponseInfoInterface::GetProperty() to FakePepperInterfaceGoogleDriveFs, and fix a reference count cycle issue in FakePepperInterfaceGoogleDriveFs. FakePepperInterfaceGoogleDriveFs is going to be used by the unit tests of the future code. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_nacl_sdk;master.tryserver.chromium.mac:mac_nacl_sdk;master.tryserver.chromium.win:win_nacl_sdk Review-Url: https://codereview.chromium.org/2573523004 ========== to ========== [NaCl SDK] Add more support and fix FakePepperInterfaceGoogleDriveFs Add FakeDriveURLRequestInfoInterface, an FakeFileIoInterface instance, & FakeDriveURLResponseInfoInterface::GetProperty() to FakePepperInterfaceGoogleDriveFs, and fix a reference count cycle issue in FakePepperInterfaceGoogleDriveFs. FakePepperInterfaceGoogleDriveFs is going to be used by the unit tests of the future code. 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/7385e43d01140ac8de624cf348111257b7138b13 Cr-Commit-Position: refs/heads/master@{#440605} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/7385e43d01140ac8de624cf348111257b7138b13 Cr-Commit-Position: refs/heads/master@{#440605} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
