Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2552223002:
Remove explicit singletonness of ArcBridgeService part 2. (Closed)
|
Created:
4 years ago by hidehiko Modified:
4 years ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, yusukes+watch_chromium.org, tzik, hidehiko+watch_chromium.org, nhiroki, lhchavez+watch_chromium.org, oshima+watch_chromium.org, kinuko+fileapi, davemoore+watch_chromium.org, Luis Héctor Chávez Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove explicit singletonness of ArcBridgeService part 2.
This CL removes ArcBridgeService::Get() invocation in
arc_file_system_instance_util with small refactoring.
BUG=657687
BUG=b/31079732
TEST=Ran bots.
Committed: https://crrev.com/7de255ba5e28ef0db63be99ba4471ce4c1424ea7
Cr-Commit-Position: refs/heads/master@{#437220}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Address comment #
Total comments: 8
Patch Set 3 : rebase #Patch Set 4 : Address comments #
Messages
Total messages: 22 (12 generated)
The CQ bit was checked by hidehiko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
hidehiko@chromium.org changed reviewers: + hashimoto@chromium.org
PTAL: hashimoto@, FYI: lhchavez@.
Description was changed from ========== Remove explicit singletonness of ArcBridgeService part 2. This CL removes ArcBridgeService::Get() invocation in arc_file_system_instance_util with small refactoring. BUG=657687 BUG=b/31079732 TEST=Ran on test device. Ran bots. ========== to ========== Remove explicit singletonness of ArcBridgeService part 2. This CL removes ArcBridgeService::Get() invocation in arc_file_system_instance_util with small refactoring. BUG=657687 BUG=b/31079732 TEST=Ran bots. ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lhchavez@chromium.org changed reviewers: + lhchavez@chromium.org
https://codereview.chromium.org/2552223002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/fileapi/arc_file_system_instance_util.cc (left): https://codereview.chromium.org/2552223002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/fileapi/arc_file_system_instance_util.cc:39: LOG(ERROR) << "Failed to get FileSystemInstance."; Ah I see where the log came from. https://codereview.chromium.org/2552223002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/fileapi/arc_file_system_instance_util.cc (right): https://codereview.chromium.org/2552223002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/fileapi/arc_file_system_instance_util.cc:38: LOG_IF(ERROR, !file_system) << "Failed to get FileSystemInstance."; GetInstanceForMethod already logs on failure. Remove.
Thank you for review. PTAL. https://codereview.chromium.org/2552223002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/fileapi/arc_file_system_instance_util.cc (right): https://codereview.chromium.org/2552223002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/fileapi/arc_file_system_instance_util.cc:38: LOG_IF(ERROR, !file_system) << "Failed to get FileSystemInstance."; On 2016/12/06 18:44:49, Luis Héctor Chávez wrote: > GetInstanceForMethod already logs on failure. Remove. Oh, right! Done.
lgtm https://codereview.chromium.org/2552223002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader_unittest.cc (right): https://codereview.chromium.org/2552223002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader_unittest.cc:100: file_system_.reset(); nit: Try adding mojo::edk::test::Shutdown()[1] here. If it's not trivial to call it or it introduces test failures, please file a bug to call it in all our tests. 1: https://cs.chromium.org/chromium/src/mojo/edk/embedder/test_embedder.h?sq=pac...
https://codereview.chromium.org/2552223002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader_unittest.cc (right): https://codereview.chromium.org/2552223002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader_unittest.cc:96: arc_service_manager_->arc_bridge_service()->file_system()->SetInstance( Why do we need to do this? The same goes for the other test. https://codereview.chromium.org/2552223002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader_unittest.cc:98: arc_service_manager_.reset(); Why can't we let the dtor handle resetting these unique_ptrs? The same goes for the other test. https://codereview.chromium.org/2552223002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader_unittest.cc:107: std::unique_ptr<ArcServiceManager> arc_service_manager_; In the other test, arc_service_manager_ is placed before file_system_. Please stick to one rule to be consistent.
Thank you for review. PTAL. https://codereview.chromium.org/2552223002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader_unittest.cc (right): https://codereview.chromium.org/2552223002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader_unittest.cc:96: arc_service_manager_->arc_bridge_service()->file_system()->SetInstance( On 2016/12/07 04:15:50, hashimoto wrote: > Why do we need to do this? > The same goes for the other test. Hmm, I wanted to emulate the real tear down situation, but indeed it'd be ok to skip it as long as the test works well. Removed. https://codereview.chromium.org/2552223002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader_unittest.cc:98: arc_service_manager_.reset(); On 2016/12/07 04:15:50, hashimoto wrote: > Why can't we let the dtor handle resetting these unique_ptrs? > The same goes for the other test. Acknowledged. https://codereview.chromium.org/2552223002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader_unittest.cc:100: file_system_.reset(); On 2016/12/06 19:14:37, Luis Héctor Chávez wrote: > nit: Try adding mojo::edk::test::Shutdown()[1] here. If it's not trivial to call > it or it introduces test failures, please file a bug to call it in all our > tests. > > 1: > https://cs.chromium.org/chromium/src/mojo/edk/embedder/test_embedder.h?sq=pac... I haven't investigated yet, but causes a crash error. Filed a bug: crbug.com/672385 https://codereview.chromium.org/2552223002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader_unittest.cc:107: std::unique_ptr<ArcServiceManager> arc_service_manager_; On 2016/12/07 04:15:50, hashimoto wrote: > In the other test, arc_service_manager_ is placed before file_system_. > Please stick to one rule to be consistent. Done.
lgtm
The CQ bit was checked by hidehiko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lhchavez@chromium.org Link to the patchset: https://codereview.chromium.org/2552223002/#ps60001 (title: "Address comments")
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": 60001, "attempt_start_ts": 1481186820086170,
"parent_rev": "8e3bad307d8b852de643860e89b38d279fa9e8fd", "commit_rev":
"007bff870e7f5666337badd18c356febfcf6faa5"}
Message was sent while issue was closed.
Description was changed from ========== Remove explicit singletonness of ArcBridgeService part 2. This CL removes ArcBridgeService::Get() invocation in arc_file_system_instance_util with small refactoring. BUG=657687 BUG=b/31079732 TEST=Ran bots. ========== to ========== Remove explicit singletonness of ArcBridgeService part 2. This CL removes ArcBridgeService::Get() invocation in arc_file_system_instance_util with small refactoring. BUG=657687 BUG=b/31079732 TEST=Ran bots. ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Remove explicit singletonness of ArcBridgeService part 2. This CL removes ArcBridgeService::Get() invocation in arc_file_system_instance_util with small refactoring. BUG=657687 BUG=b/31079732 TEST=Ran bots. ========== to ========== Remove explicit singletonness of ArcBridgeService part 2. This CL removes ArcBridgeService::Get() invocation in arc_file_system_instance_util with small refactoring. BUG=657687 BUG=b/31079732 TEST=Ran bots. Committed: https://crrev.com/7de255ba5e28ef0db63be99ba4471ce4c1424ea7 Cr-Commit-Position: refs/heads/master@{#437220} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/7de255ba5e28ef0db63be99ba4471ce4c1424ea7 Cr-Commit-Position: refs/heads/master@{#437220} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
