Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2651883003:
Clean up ARC file system unit tests. (Closed)
|
Created:
3 years, 11 months ago by Shuhei Takahashi Modified:
3 years, 10 months 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 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionClean up ARC file system unit tests.
1. Introduce a generic FakeFileSystemInstance.
Tests that want to test ARC file system operations can use this fake without
implementing its own FileSystemInstance.
2. Removed ArcFileSystemOperationRunner interface.
Former ArcDeferredFileSystemOperationRunner is now the only implementation
of the interface, so the interface is removed and ArcDeferredFSOpRunner is
renamed to ArcFSOpRunner.
3. Inject FileSystemInstance rather than ArcFSOpRunner in unit tests.
In unit tests, ArcFSOpRunner without deferring is created with
ArcFSOpRunner::CreateForTesting() so that operations are passed through
to FileSystemInstance.
TEST=unit_tests
BUG=chromium:683049
Review-Url: https://codereview.chromium.org/2651883003
Cr-Commit-Position: refs/heads/master@{#447480}
Committed: https://chromium.googlesource.com/chromium/src/+/91afa75f31ca8fdfc901de32098ef729218de847
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : Fake FileSystemInstance, not ArcFileSystemOperationRunner. #
Total comments: 29
Patch Set 4 : Addressed comments. #
Total comments: 4
Patch Set 5 : Give up constexpr as per hashimoto's request. #
Total comments: 10
Patch Set 6 : Addressed hidehiko's comments. #
Total comments: 4
Patch Set 7 : Addressed hashimoto's comments. #Patch Set 8 : Rebased. #Messages
Total messages: 60 (35 generated)
The CQ bit was checked by nya@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by nya@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
nya@chromium.org changed reviewers: + hashimoto@chromium.org, hidehiko@chromium.org
hidehiko, hashimoto: PTAL
Sorry for not catching this in earlier reviews, but why do we need to have a fake of ArcFileSystemOperationRunner, not that of FileSystemInstance? An internal resource recommends to use real objects if possible (in this case, real operation runner + fake FileSystemInstance should result in using more real code) and I think it makes a lot of sense.
On 2017/01/24 08:26:11, hashimoto wrote: > Sorry for not catching this in earlier reviews, but why do we need to have a > fake of ArcFileSystemOperationRunner, not that of FileSystemInstance? > An internal resource recommends to use real objects if possible (in this case, > real operation runner + fake FileSystemInstance should result in using more real > code) and I think it makes a lot of sense. Could you point me to that internal resource?
On 2017/01/24 08:32:21, Shuhei Takahashi wrote: > On 2017/01/24 08:26:11, hashimoto wrote: > > Sorry for not catching this in earlier reviews, but why do we need to have a > > fake of ArcFileSystemOperationRunner, not that of FileSystemInstance? > > An internal resource recommends to use real objects if possible (in this case, > > real operation runner + fake FileSystemInstance should result in using more > real > > code) and I think it makes a lot of sense. > > Could you point me to that internal resource? I got the pointer offline. Actually I don't fully agree with you because we can't use "real" (= "production" IMO) ArcFileSystemOperationRunner instance in unit tests, but I agree with the suggestion in the document you pointed that fakes should be created at the lowest level possible to minimize the cost of inventing fakes. So I'm fine with making FakeFileSystemInstance, but I want to hear the opinion of OWNER before proceeding. hidehiko, WDYT?
On 2017/01/24 09:08:11, Shuhei Takahashi wrote: > On 2017/01/24 08:32:21, Shuhei Takahashi wrote: > > On 2017/01/24 08:26:11, hashimoto wrote: > > > Sorry for not catching this in earlier reviews, but why do we need to have a > > > fake of ArcFileSystemOperationRunner, not that of FileSystemInstance? > > > An internal resource recommends to use real objects if possible (in this > case, > > > real operation runner + fake FileSystemInstance should result in using more > > real > > > code) and I think it makes a lot of sense. > > > > Could you point me to that internal resource? > > I got the pointer offline. > > Actually I don't fully agree with you because we can't use "real" (= > "production" IMO) ArcFileSystemOperationRunner instance in unit tests, but I > agree with the suggestion in the document you pointed that fakes should be > created at the lowest level possible to minimize the cost of inventing fakes. > > So I'm fine with making FakeFileSystemInstance, but I want to hear the opinion > of OWNER before proceeding. hidehiko, WDYT? One perspective about mocking FileSystemInstance: If we choose to mock FileSystemInstance, I'm planning to introduce ArcImmediateFileSystemOperationRunner which just directly calls FileSystemInstance, instead of using ArcDeferredFileSystemOperationRunner in unit tests. One reason for it is because I don't come up with a good way to set up ArcDeferredFileSystemOperationRunner properly in unit tests without knowledge of in what conditions ArcDeferredFileSystemOperationRunner defers file system operations. Currently ArcDeferredFileSystemOperationRunner defers file system operations when ARC is enabled and FileSystemInstance is not available, but it might change in the future; for example, we may want to wait 1 second after FileSystemInstance gets ready in order to avoid file system operation flakiness (this is just a crazy example). When we were to make such changes in ArcDeferredFileSystemOperationRunner, I think unit tests using ArcFileSystemOperationRunner should not be affected. Do you agree? Introducing ArcImmediateFileSystemOperationRunner is an obvious way to avoid embedding knowledge of the condition to defer in unit tests. On the other hand, if we were to use ArcDeferredFileSystemOperationRunner, I only come up with a naive way to forcibly turn off deferring when it learns it's running under unit tests, but it does not make sense compared to ArcImmediateFileSystemOperationRunner. If you have any alternative suggestion please let me know.
tl;dr; How about - Use FakeFileSystemInstance mojom impl. - Merge ArcDeferredFileSystemOperationRunner impl into ArcFileSystemOperationRunner. (i.e. get rid of interface). - Add a very simple mechanism to disable "defer". - E.g. adding a flag. There could be alternative options, here. I think it reduces the total size of testing-only code we need to maintain. Commented detailed discussion below. On 2017/01/24 11:34:59, Shuhei Takahashi wrote: > On 2017/01/24 09:08:11, Shuhei Takahashi wrote: > > On 2017/01/24 08:32:21, Shuhei Takahashi wrote: > > > On 2017/01/24 08:26:11, hashimoto wrote: > > > > Sorry for not catching this in earlier reviews, but why do we need to have > a > > > > fake of ArcFileSystemOperationRunner, not that of FileSystemInstance? > > > > An internal resource recommends to use real objects if possible (in this > > case, > > > > real operation runner + fake FileSystemInstance should result in using > more > > > real > > > > code) and I think it makes a lot of sense. > > > > > > Could you point me to that internal resource? > > > > I got the pointer offline. > > > > Actually I don't fully agree with you because we can't use "real" (= > > "production" IMO) ArcFileSystemOperationRunner instance in unit tests, but I > > agree with the suggestion in the document you pointed that fakes should be > > created at the lowest level possible to minimize the cost of inventing fakes. > > > > So I'm fine with making FakeFileSystemInstance, but I want to hear the opinion > > of OWNER before proceeding. hidehiko, WDYT? > I think which layer we should fake is a good point for maintenance point of view. Thank you hashimoto@ for putting this topic on the table, and sorry, nya@, that I couldn't catch this earlier, neither. In general, I agree to make a fake in lowest layer as written in the doc. So, +1 to use FakeFileSystemInstance, assuming there is no huge blocker. > One perspective about mocking FileSystemInstance: > > If we choose to mock FileSystemInstance, I'm planning to introduce > ArcImmediateFileSystemOperationRunner which just directly calls > FileSystemInstance, instead of using ArcDeferredFileSystemOperationRunner in > unit tests. > > One reason for it is because I don't come up with a good way to set up > ArcDeferredFileSystemOperationRunner properly in unit tests without knowledge of > in what conditions ArcDeferredFileSystemOperationRunner defers file system > operations. Currently ArcDeferredFileSystemOperationRunner defers file system > operations when ARC is enabled and FileSystemInstance is not available, but it > might change in the future; for example, we may want to wait 1 second after > FileSystemInstance gets ready in order to avoid file system operation flakiness > (this is just a crazy example). When we were to make such changes in > ArcDeferredFileSystemOperationRunner, I think unit tests using > ArcFileSystemOperationRunner should not be affected. Do you agree? > That's a good point. I agree that client tests shouldn't be affected, ... if ArcFSOperationRunner is a fixed interface. But, can we step back a bit? We can design the class / interface / semantics as we like. If the current interface is not feasible for our goal (reducing the test-only code for better maintenance), I think it's still an option to change it. WDYT? > Introducing ArcImmediateFileSystemOperationRunner is an obvious way to avoid > embedding knowledge of the condition to defer in unit tests. On the other hand, > if we were to use ArcDeferredFileSystemOperationRunner, I only come up with a > naive way to forcibly turn off deferring when it learns it's running under unit > tests, but it does not make sense compared to > ArcImmediateFileSystemOperationRunner. If you have any alternative suggestion > please let me know. Hmm... ArcImmediateFileSystemOperationRunner sounds overkill to me. It is yet another test-only class with name, instead of fake impl. Actually, we would need to maintain it separately from the production class, similar to fake. Let's keep focusing on the goal "minimizing the test-only code to be maintained." IIUC, we would like to avoid "defer" somehow, because - From implementation point of view, it depends on ArcSessionManager, which is not easily usable in unittests. - From concept point of view, it's nice if we can avoid touching unittest even when we change the deferring policy. (I.e., otherwise we can directly use the current implementation, right?) Then, I think one of the lightest ways is adding a flag to enable/disable "defer". pros: - The change will be small (I guess about 10- lines). - Thanks to the current implementation, I do not think it needs huge maintenance cost. - E.g., adding/removing FSInstance method would equal to a case we do not have the flag. - We can get rid of interface class. (ArcDeferredFSOperationRunner can be ArcFSOperationRunner. No test-only class is needed.) - It uses production code actually (but it just avoids "defer" code path). cons: - Having test-only code in production class is not that elegant, unfortunately. (*1) IMHO, in this specific case, comparing to the loss of elegance, maintaining small code looks to have more benefit practically. WDYT? Note: (*1) Although this is only for testing, we also can think that this is a feature of ArcFSOperationRunner, IMHO. I meant; "ArcFSOperationRunner has a feature to disable/enable deferring" (... but used only for testing). Actually, although we should have ForTesting() suffix to avoid accidental invocation, other parts should/can be simply implemented as if it is just a feature. Also, the way (with ForTesting()) is common in chromium AFAIK. In the both sense, I'm not much worried about it, in this specific case.
I'm fine with hidehiko's plan. hashimoto, are you also okay with it? On 2017/01/24 14:05:16, hidehiko wrote: > tl;dr; How about > > - Use FakeFileSystemInstance mojom impl. > - Merge ArcDeferredFileSystemOperationRunner impl into > ArcFileSystemOperationRunner. (i.e. get rid of interface). > - Add a very simple mechanism to disable "defer". > - E.g. adding a flag. There could be alternative options, here. > > I think it reduces the total size of testing-only code we need to maintain. > > Commented detailed discussion below. > > > On 2017/01/24 11:34:59, Shuhei Takahashi wrote: > > On 2017/01/24 09:08:11, Shuhei Takahashi wrote: > > > On 2017/01/24 08:32:21, Shuhei Takahashi wrote: > > > > On 2017/01/24 08:26:11, hashimoto wrote: > > > > > Sorry for not catching this in earlier reviews, but why do we need to > have > > a > > > > > fake of ArcFileSystemOperationRunner, not that of FileSystemInstance? > > > > > An internal resource recommends to use real objects if possible (in this > > > case, > > > > > real operation runner + fake FileSystemInstance should result in using > > more > > > > real > > > > > code) and I think it makes a lot of sense. > > > > > > > > Could you point me to that internal resource? > > > > > > I got the pointer offline. > > > > > > Actually I don't fully agree with you because we can't use "real" (= > > > "production" IMO) ArcFileSystemOperationRunner instance in unit tests, but I > > > agree with the suggestion in the document you pointed that fakes should be > > > created at the lowest level possible to minimize the cost of inventing > fakes. > > > > > > So I'm fine with making FakeFileSystemInstance, but I want to hear the > opinion > > > of OWNER before proceeding. hidehiko, WDYT? > > > > I think which layer we should fake is a good point for maintenance point of > view. > Thank you hashimoto@ for putting this topic on the table, and sorry, nya@, that > I couldn't catch this earlier, neither. > In general, I agree to make a fake in lowest layer as written in the doc. > So, +1 to use FakeFileSystemInstance, assuming there is no huge blocker. > > > One perspective about mocking FileSystemInstance: > > > > If we choose to mock FileSystemInstance, I'm planning to introduce > > ArcImmediateFileSystemOperationRunner which just directly calls > > FileSystemInstance, instead of using ArcDeferredFileSystemOperationRunner in > > unit tests. > > > > One reason for it is because I don't come up with a good way to set up > > ArcDeferredFileSystemOperationRunner properly in unit tests without knowledge > of > > in what conditions ArcDeferredFileSystemOperationRunner defers file system > > operations. Currently ArcDeferredFileSystemOperationRunner defers file system > > operations when ARC is enabled and FileSystemInstance is not available, but it > > might change in the future; for example, we may want to wait 1 second after > > FileSystemInstance gets ready in order to avoid file system operation > flakiness > > (this is just a crazy example). When we were to make such changes in > > ArcDeferredFileSystemOperationRunner, I think unit tests using > > ArcFileSystemOperationRunner should not be affected. Do you agree? > > > > That's a good point. > I agree that client tests shouldn't be affected, > ... if ArcFSOperationRunner is a fixed interface. > But, can we step back a bit? > We can design the class / interface / semantics as we like. > If the current interface is not feasible for our goal (reducing the test-only > code for better maintenance), > I think it's still an option to change it. WDYT? > > > Introducing ArcImmediateFileSystemOperationRunner is an obvious way to avoid > > embedding knowledge of the condition to defer in unit tests. On the other > hand, > > if we were to use ArcDeferredFileSystemOperationRunner, I only come up with a > > naive way to forcibly turn off deferring when it learns it's running under > unit > > tests, but it does not make sense compared to > > ArcImmediateFileSystemOperationRunner. If you have any alternative suggestion > > please let me know. > > Hmm... ArcImmediateFileSystemOperationRunner sounds overkill to me. > It is yet another test-only class with name, instead of fake impl. > Actually, we would need to maintain it separately from the production class, > similar to fake. > Let's keep focusing on the goal "minimizing the test-only code to be > maintained." > > IIUC, we would like to avoid "defer" somehow, because > - From implementation point of view, it depends on ArcSessionManager, which is > not easily usable in unittests. > - From concept point of view, it's nice if we can avoid touching unittest even > when we change the deferring policy. > (I.e., otherwise we can directly use the current implementation, right?) > > Then, I think one of the lightest ways is adding a flag to enable/disable > "defer". > pros: > - The change will be small (I guess about 10- lines). > - Thanks to the current implementation, I do not think it needs huge maintenance > cost. > - E.g., adding/removing FSInstance method would equal to a case we do not have > the flag. > - We can get rid of interface class. (ArcDeferredFSOperationRunner can be > ArcFSOperationRunner. No test-only class is needed.) > - It uses production code actually (but it just avoids "defer" code path). > > cons: > - Having test-only code in production class is not that elegant, unfortunately. > (*1) > > IMHO, in this specific case, comparing to the loss of elegance, maintaining > small code looks to have more benefit practically. WDYT? > > > Note: > (*1) > Although this is only for testing, we also can think that this is a feature of > ArcFSOperationRunner, IMHO. > I meant; "ArcFSOperationRunner has a feature to disable/enable deferring" (... > but used only for testing). > Actually, although we should have ForTesting() suffix to avoid accidental > invocation, > other parts should/can be simply implemented as if it is just a feature. > Also, the way (with ForTesting()) is common in chromium AFAIK. > In the both sense, I'm not much worried about it, in this specific case.
On 2017/01/25 03:27:56, Shuhei Takahashi wrote: > I'm fine with hidehiko's plan. hashimoto, are you also okay with it? sg ArcDeferredFileSystemOperationRunner already supports disabling defer behavior for testing, so just making it public with a suffix ForTesting (presubmit will warn if it's used in production code) should be easy. > > > On 2017/01/24 14:05:16, hidehiko wrote: > > tl;dr; How about > > > > - Use FakeFileSystemInstance mojom impl. > > - Merge ArcDeferredFileSystemOperationRunner impl into > > ArcFileSystemOperationRunner. (i.e. get rid of interface). > > - Add a very simple mechanism to disable "defer". > > - E.g. adding a flag. There could be alternative options, here. > > > > I think it reduces the total size of testing-only code we need to maintain. > > > > Commented detailed discussion below. > > > > > > On 2017/01/24 11:34:59, Shuhei Takahashi wrote: > > > On 2017/01/24 09:08:11, Shuhei Takahashi wrote: > > > > On 2017/01/24 08:32:21, Shuhei Takahashi wrote: > > > > > On 2017/01/24 08:26:11, hashimoto wrote: > > > > > > Sorry for not catching this in earlier reviews, but why do we need to > > have > > > a > > > > > > fake of ArcFileSystemOperationRunner, not that of FileSystemInstance? > > > > > > An internal resource recommends to use real objects if possible (in > this > > > > case, > > > > > > real operation runner + fake FileSystemInstance should result in using > > > more > > > > > real > > > > > > code) and I think it makes a lot of sense. > > > > > > > > > > Could you point me to that internal resource? > > > > > > > > I got the pointer offline. > > > > > > > > Actually I don't fully agree with you because we can't use "real" (= > > > > "production" IMO) ArcFileSystemOperationRunner instance in unit tests, but > I > > > > agree with the suggestion in the document you pointed that fakes should be > > > > created at the lowest level possible to minimize the cost of inventing > > fakes. > > > > > > > > So I'm fine with making FakeFileSystemInstance, but I want to hear the > > opinion > > > > of OWNER before proceeding. hidehiko, WDYT? > > > > > > > I think which layer we should fake is a good point for maintenance point of > > view. > > Thank you hashimoto@ for putting this topic on the table, and sorry, nya@, > that > > I couldn't catch this earlier, neither. > > In general, I agree to make a fake in lowest layer as written in the doc. > > So, +1 to use FakeFileSystemInstance, assuming there is no huge blocker. > > > > > One perspective about mocking FileSystemInstance: > > > > > > If we choose to mock FileSystemInstance, I'm planning to introduce > > > ArcImmediateFileSystemOperationRunner which just directly calls > > > FileSystemInstance, instead of using ArcDeferredFileSystemOperationRunner in > > > unit tests. > > > > > > One reason for it is because I don't come up with a good way to set up > > > ArcDeferredFileSystemOperationRunner properly in unit tests without > knowledge > > of > > > in what conditions ArcDeferredFileSystemOperationRunner defers file system > > > operations. Currently ArcDeferredFileSystemOperationRunner defers file > system > > > operations when ARC is enabled and FileSystemInstance is not available, but > it > > > might change in the future; for example, we may want to wait 1 second after > > > FileSystemInstance gets ready in order to avoid file system operation > > flakiness > > > (this is just a crazy example). When we were to make such changes in > > > ArcDeferredFileSystemOperationRunner, I think unit tests using > > > ArcFileSystemOperationRunner should not be affected. Do you agree? > > > > > > > That's a good point. > > I agree that client tests shouldn't be affected, > > ... if ArcFSOperationRunner is a fixed interface. > > But, can we step back a bit? > > We can design the class / interface / semantics as we like. > > If the current interface is not feasible for our goal (reducing the test-only > > code for better maintenance), > > I think it's still an option to change it. WDYT? > > > > > Introducing ArcImmediateFileSystemOperationRunner is an obvious way to avoid > > > embedding knowledge of the condition to defer in unit tests. On the other > > hand, > > > if we were to use ArcDeferredFileSystemOperationRunner, I only come up with > a > > > naive way to forcibly turn off deferring when it learns it's running under > > unit > > > tests, but it does not make sense compared to > > > ArcImmediateFileSystemOperationRunner. If you have any alternative > suggestion > > > please let me know. > > > > Hmm... ArcImmediateFileSystemOperationRunner sounds overkill to me. > > It is yet another test-only class with name, instead of fake impl. > > Actually, we would need to maintain it separately from the production class, > > similar to fake. > > Let's keep focusing on the goal "minimizing the test-only code to be > > maintained." > > > > IIUC, we would like to avoid "defer" somehow, because > > - From implementation point of view, it depends on ArcSessionManager, which is > > not easily usable in unittests. > > - From concept point of view, it's nice if we can avoid touching unittest even > > when we change the deferring policy. > > (I.e., otherwise we can directly use the current implementation, right?) > > > > Then, I think one of the lightest ways is adding a flag to enable/disable > > "defer". > > pros: > > - The change will be small (I guess about 10- lines). > > - Thanks to the current implementation, I do not think it needs huge > maintenance > > cost. > > - E.g., adding/removing FSInstance method would equal to a case we do not > have > > the flag. > > - We can get rid of interface class. (ArcDeferredFSOperationRunner can be > > ArcFSOperationRunner. No test-only class is needed.) > > - It uses production code actually (but it just avoids "defer" code path). > > > > cons: > > - Having test-only code in production class is not that elegant, > unfortunately. > > (*1) > > > > IMHO, in this specific case, comparing to the loss of elegance, maintaining > > small code looks to have more benefit practically. WDYT? > > > > > > Note: > > (*1) > > Although this is only for testing, we also can think that this is a feature of > > ArcFSOperationRunner, IMHO. > > I meant; "ArcFSOperationRunner has a feature to disable/enable deferring" (... > > but used only for testing). > > Actually, although we should have ForTesting() suffix to avoid accidental > > invocation, > > other parts should/can be simply implemented as if it is just a feature. > > Also, the way (with ForTesting()) is common in chromium AFAIK. > > In the both sense, I'm not much worried about it, in this specific case.
Description was changed from ========== Provide generic FakeArcFileSystemOperationRunner. All tests that want to fake ArcFileSystemOperationRunner should use this new generic FakeArcFileSystemOperationRunner rather than implementing ArcFileSystemOperationRunner by themselves. TEST=unit_tests BUG=chromium:683049 ========== to ========== Clean up ARC file system unit tests. 1. Generic FakeFileSystemInstance is introduced. Tests that want to test ARC file system operations can use this fake without implementing its own FileSystemInstance. 2. TEST=unit_tests BUG=chromium:683049 ==========
The CQ bit was checked by nya@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...
Description was changed from ========== Clean up ARC file system unit tests. 1. Generic FakeFileSystemInstance is introduced. Tests that want to test ARC file system operations can use this fake without implementing its own FileSystemInstance. 2. TEST=unit_tests BUG=chromium:683049 ========== to ========== Clean up ARC file system unit tests. 1. Introduce a generic FakeFileSystemInstance. Tests that want to test ARC file system operations can use this fake without implementing its own FileSystemInstance. 2. Removed ArcFileSystemOperationRunner interface. Former ArcDeferredFileSystemOperationRunner is now the only implementation of the interface, so the interface is removed and ArcDeferredFSOpRunner is renamed to ArcFSOpRunner. 3. Inject FileSystemInstance rather than ArcFSOpRunner in unit tests. In unit tests, ArcFSOpRunner without deferring is created with ArcFSOpRunner::CreateForTesting() so that operations are passed through to FileSystemInstance. TEST=unit_tests BUG=chromium:683049 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Updated the change according to the discussion above. PTAL.
Overall design looks good. It looks much simpler to me! https://codereview.chromium.org/2651883003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_unittest.cc (right): https://codereview.chromium.org/2651883003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_unittest.cc:114: kMusicSpec, kDupsSpec, kDup2Spec, Could you sort, at least kDupNSpec where N is number? https://codereview.chromium.org/2651883003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc (right): https://codereview.chromium.org/2651883003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc:26: return new ArcFileSystemOperationRunner(bridge_service, false); So, corresponding to the header's comment, this can be base::MakeUnique<ArcFileSystemOperationRunner>( ... ). https://codereview.chromium.org/2651883003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.h (right): https://codereview.chromium.org/2651883003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.h:23: // This is an abstraction layer on top of mojom::FileSystemInstance. All ARC I like this is explicitly documented :D. Thank you! https://codereview.chromium.org/2651883003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.h:24: // file system operations should go through this interface, rather than Optional: maybe s/interface/class/ is clearer here? In Chrome/C++ context, "interface" is often used for a class with pure-virtual methods, but yours is concrete one? https://codereview.chromium.org/2651883003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.h:63: static ArcFileSystemOperationRunner* CreateForTesting( Could you return std::unique_ptr<ArcFileSystemOperationRunner> instead, to let client manage the instance? https://codereview.chromium.org/2651883003/diff/40001/components/arc/BUILD.gn File components/arc/BUILD.gn (right): https://codereview.chromium.org/2651883003/diff/40001/components/arc/BUILD.gn... components/arc/BUILD.gn:191: "//url", Looks unused. Could you remove? https://codereview.chromium.org/2651883003/diff/40001/components/arc/test/fak... File components/arc/test/fake_file_system_instance.cc (right): https://codereview.chromium.org/2651883003/diff/40001/components/arc/test/fak... components/arc/test/fake_file_system_instance.cc:28: CHECK(base::CreateTemporaryFileInDir(temp_dir, &path)); Could you return an error, instead of crash? You can ASSERT_TRUE() in the set up. cf) the last item of https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... Ditto for below. https://codereview.chromium.org/2651883003/diff/40001/components/arc/test/fak... File components/arc/test/fake_file_system_instance.h (right): https://codereview.chromium.org/2651883003/diff/40001/components/arc/test/fak... components/arc/test/fake_file_system_instance.h:44: const char* content; Could you comment this only accept c-style string? https://codereview.chromium.org/2651883003/diff/40001/components/arc/test/fak... components/arc/test/fake_file_system_instance.h:48: bool stream; Optional: "seekable" may be consistent with your comment...? https://codereview.chromium.org/2651883003/diff/40001/components/arc/test/fak... components/arc/test/fake_file_system_instance.h:50: constexpr FileSpec() : FileSpec(nullptr, nullptr, false) {} This is POD, so I do not think you need ctors? https://codereview.chromium.org/2651883003/diff/40001/components/arc/test/fak... components/arc/test/fake_file_system_instance.h:79: constexpr DocumentSpec() Same here.
https://codereview.chromium.org/2651883003/diff/40001/components/arc/test/fak... File components/arc/test/fake_file_system_instance.h (right): https://codereview.chromium.org/2651883003/diff/40001/components/arc/test/fak... components/arc/test/fake_file_system_instance.h:50: constexpr FileSpec() : FileSpec(nullptr, nullptr, false) {} On 2017/01/26 07:24:20, hidehiko wrote: > This is POD, so I do not think you need ctors? Or, how about replacing const char* with std::string to allow new tests to construct test data dynamically? https://codereview.chromium.org/2651883003/diff/40001/components/arc/test/fak... components/arc/test/fake_file_system_instance.h:77: uint64_t last_modified; base::Time() is Chromium's standard type for time values. https://codereview.chromium.org/2651883003/diff/40001/components/arc/test/fak... components/arc/test/fake_file_system_instance.h:126: std::map<std::string, FileSpec> files_; Please make it clear that this map is from URL to file. (e.g. renaming this to urls_to_files_, or adding a comment).
The CQ bit was checked by nya@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
PTAL https://codereview.chromium.org/2651883003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_unittest.cc (right): https://codereview.chromium.org/2651883003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_unittest.cc:114: kMusicSpec, kDupsSpec, kDup2Spec, On 2017/01/26 07:24:20, hidehiko wrote: > Could you sort, at least kDupNSpec where N is number? It's intentional as commented. I'll further add comments... https://codereview.chromium.org/2651883003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc (right): https://codereview.chromium.org/2651883003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc:26: return new ArcFileSystemOperationRunner(bridge_service, false); On 2017/01/26 07:24:20, hidehiko wrote: > So, corresponding to the header's comment, this can be > base::MakeUnique<ArcFileSystemOperationRunner>( ... ). Done. https://codereview.chromium.org/2651883003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.h (right): https://codereview.chromium.org/2651883003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.h:24: // file system operations should go through this interface, rather than On 2017/01/26 07:24:20, hidehiko wrote: > Optional: maybe s/interface/class/ is clearer here? > In Chrome/C++ context, "interface" is often used for a class with pure-virtual > methods, > but yours is concrete one? Done. https://codereview.chromium.org/2651883003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.h:63: static ArcFileSystemOperationRunner* CreateForTesting( On 2017/01/26 07:24:20, hidehiko wrote: > Could you return std::unique_ptr<ArcFileSystemOperationRunner> instead, to let > client manage the instance? Done. https://codereview.chromium.org/2651883003/diff/40001/components/arc/BUILD.gn File components/arc/BUILD.gn (right): https://codereview.chromium.org/2651883003/diff/40001/components/arc/BUILD.gn... components/arc/BUILD.gn:191: "//url", On 2017/01/26 07:24:20, hidehiko wrote: > Looks unused. Could you remove? Oops, done. https://codereview.chromium.org/2651883003/diff/40001/components/arc/test/fak... File components/arc/test/fake_file_system_instance.cc (right): https://codereview.chromium.org/2651883003/diff/40001/components/arc/test/fak... components/arc/test/fake_file_system_instance.cc:28: CHECK(base::CreateTemporaryFileInDir(temp_dir, &path)); On 2017/01/26 07:24:20, hidehiko wrote: > Could you return an error, instead of crash? You can ASSERT_TRUE() in the set > up. > > cf) the last item of > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > > Ditto for below. As we discussed offline, please let me use DCHECK() for the cases where it makes sense to crash in unit tests (e.g. obvious errors in setting test expectations, I/O errors and thread checker). I'll keep in mind to use EXPECT/ASSERT wherever it makes sense to catch such errors in unit tests. https://codereview.chromium.org/2651883003/diff/40001/components/arc/test/fak... File components/arc/test/fake_file_system_instance.h (right): https://codereview.chromium.org/2651883003/diff/40001/components/arc/test/fak... components/arc/test/fake_file_system_instance.h:44: const char* content; On 2017/01/26 07:24:20, hidehiko wrote: > Could you comment this only accept c-style string? Good point, done. https://codereview.chromium.org/2651883003/diff/40001/components/arc/test/fak... components/arc/test/fake_file_system_instance.h:48: bool stream; On 2017/01/26 07:24:20, hidehiko wrote: > Optional: "seekable" may be consistent with your comment...? Done. https://codereview.chromium.org/2651883003/diff/40001/components/arc/test/fak... components/arc/test/fake_file_system_instance.h:50: constexpr FileSpec() : FileSpec(nullptr, nullptr, false) {} On 2017/01/26 08:05:54, hashimoto wrote: > On 2017/01/26 07:24:20, hidehiko wrote: > > This is POD, so I do not think you need ctors? > > Or, how about replacing const char* with std::string to allow new tests to > construct test data dynamically? std::string makes this class non-constexpr, and then we can't define FileSpec constants in global scope, reducing convenience in unit tests a bit. Or if you have any suggestion to write such kind of unit test constants without depending on constexpr please let me know. https://codereview.chromium.org/2651883003/diff/40001/components/arc/test/fak... components/arc/test/fake_file_system_instance.h:77: uint64_t last_modified; On 2017/01/26 08:05:54, hashimoto wrote: > base::Time() is Chromium's standard type for time values. I know, but in this case it makes more sense to use the most similar type as one used in file_system.mojom because the purpose of this class is to fake FileSystemInstance. It was also suggested to use a time type in file_system.mojom but it was technically impossible due to an existing bug. I added a TODO comment. https://codereview.chromium.org/2651883003/diff/40001/components/arc/test/fak... components/arc/test/fake_file_system_instance.h:79: constexpr DocumentSpec() On 2017/01/26 07:24:20, hidehiko wrote: > Same here. Done. https://codereview.chromium.org/2651883003/diff/40001/components/arc/test/fak... components/arc/test/fake_file_system_instance.h:126: std::map<std::string, FileSpec> files_; On 2017/01/26 08:05:54, hashimoto wrote: > Please make it clear that this map is from URL to file. > (e.g. renaming this to urls_to_files_, or adding a comment). Added comments.
https://codereview.chromium.org/2651883003/diff/40001/components/arc/test/fak... File components/arc/test/fake_file_system_instance.h (right): https://codereview.chromium.org/2651883003/diff/40001/components/arc/test/fak... components/arc/test/fake_file_system_instance.h:50: constexpr FileSpec() : FileSpec(nullptr, nullptr, false) {} On 2017/01/27 09:55:10, Shuhei Takahashi wrote: > On 2017/01/26 08:05:54, hashimoto wrote: > > On 2017/01/26 07:24:20, hidehiko wrote: > > > This is POD, so I do not think you need ctors? > > > > Or, how about replacing const char* with std::string to allow new tests to > > construct test data dynamically? > > std::string makes this class non-constexpr, and then we can't define FileSpec > constants in global scope, reducing convenience in unit tests a bit. Or if you > have any suggestion to write such kind of unit test constants without depending > on constexpr please let me know. I'm concerned about possible use-after-free of strings, as it's easy to fill this struct with std::string::c_str() and mistakenly mutate or destroy the string object during this class's lifetime. Could you at least stop storing const char* in files_ and documents_?
The CQ bit was checked by nya@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...
PTAL On 2017/01/27 10:46:50, hashimoto wrote: > https://codereview.chromium.org/2651883003/diff/40001/components/arc/test/fak... > File components/arc/test/fake_file_system_instance.h (right): > > https://codereview.chromium.org/2651883003/diff/40001/components/arc/test/fak... > components/arc/test/fake_file_system_instance.h:50: constexpr FileSpec() : > FileSpec(nullptr, nullptr, false) {} > On 2017/01/27 09:55:10, Shuhei Takahashi wrote: > > On 2017/01/26 08:05:54, hashimoto wrote: > > > On 2017/01/26 07:24:20, hidehiko wrote: > > > > This is POD, so I do not think you need ctors? > > > > > > Or, how about replacing const char* with std::string to allow new tests to > > > construct test data dynamically? > > > > std::string makes this class non-constexpr, and then we can't define FileSpec > > constants in global scope, reducing convenience in unit tests a bit. Or if you > > have any suggestion to write such kind of unit test constants without > depending > > on constexpr please let me know. > > I'm concerned about possible use-after-free of strings, as it's easy to fill > this struct with std::string::c_str() and mistakenly mutate or destroy the > string object during this class's lifetime. > Could you at least stop storing const char* in files_ and documents_? OK, I updated the patch to use std::string in FakeFileSystemInstance and define another constexpr struct to be used only in ArcDocumentsProviderRootTest Note that now we need bunch of ctor/dtor/copy-ctor for POD types in FakeFileSystemInstance as complained by chromium-style warnings.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
For constexpr v.s. std::string, I'm fine with either way, as long as it works. So, I defer the decision to you and hashimoto@. If constexpr is chosen, PS4 l-g-t-m with a few minor comments. If std::string is chosen, PS5 almost lg, but I have several more minor comments in addition to PS4's. https://codereview.chromium.org/2651883003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_unittest.cc (right): https://codereview.chromium.org/2651883003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_unittest.cc:114: kMusicSpec, kDupsSpec, kDup2Spec, On 2017/01/27 09:55:10, Shuhei Takahashi wrote: > On 2017/01/26 07:24:20, hidehiko wrote: > > Could you sort, at least kDupNSpec where N is number? > > It's intentional as commented. I'll further add comments... Oh, got it. Thank you for explanation and adding the comment. https://codereview.chromium.org/2651883003/diff/40001/components/arc/test/fak... File components/arc/test/fake_file_system_instance.cc (right): https://codereview.chromium.org/2651883003/diff/40001/components/arc/test/fak... components/arc/test/fake_file_system_instance.cc:28: CHECK(base::CreateTemporaryFileInDir(temp_dir, &path)); On 2017/01/27 09:55:10, Shuhei Takahashi wrote: > On 2017/01/26 07:24:20, hidehiko wrote: > > Could you return an error, instead of crash? You can ASSERT_TRUE() in the set > > up. > > > > cf) the last item of > > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > > > > Ditto for below. > > As we discussed offline, please let me use DCHECK() for the cases where it makes > sense to crash in unit tests (e.g. obvious errors in setting test expectations, > I/O errors and thread checker). I'll keep in mind to use EXPECT/ASSERT wherever > it makes sense to catch such errors in unit tests. Sounds reasonable to me. https://codereview.chromium.org/2651883003/diff/60001/components/arc/test/fak... File components/arc/test/fake_file_system_instance.cc (right): https://codereview.chromium.org/2651883003/diff/60001/components/arc/test/fak... components/arc/test/fake_file_system_instance.cc:63: DCHECK(thread_checker_.CalledOnValidThread()); This looks redundant, because |thread_checker_| is initialized in the ctor, here, so it always succeeds. https://codereview.chromium.org/2651883003/diff/60001/components/arc/test/fak... File components/arc/test/fake_file_system_instance.h (right): https://codereview.chromium.org/2651883003/diff/60001/components/arc/test/fak... components/arc/test/fake_file_system_instance.h:46: enum Seekable { Could you use "enum class"? https://codereview.chromium.org/2651883003/diff/80001/components/arc/test/fak... File components/arc/test/fake_file_system_instance.cc (right): https://codereview.chromium.org/2651883003/diff/80001/components/arc/test/fak... components/arc/test/fake_file_system_instance.cc:30: int written_size = base::WriteFile(path, content.c_str(), content.size()); s/content.c_str()/constent.data()/ Here, std::string is used as a byte-buffer. So, data() is more appropriate, I think, because c_str() means c-style string (\0 terminate str) conceptually. (Note, AFAIK, these are not much different in C++11, but for readability). https://codereview.chromium.org/2651883003/diff/80001/components/arc/test/fak... components/arc/test/fake_file_system_instance.cc:44: fd_write.get(), content.c_str(), content.size()); Ditto. https://codereview.chromium.org/2651883003/diff/80001/components/arc/test/fak... File components/arc/test/fake_file_system_instance.h (right): https://codereview.chromium.org/2651883003/diff/80001/components/arc/test/fak... components/arc/test/fake_file_system_instance.h:60: File(); Could you get rid of map::operator[], and replace it by map::insert()? Then, IIUC, you do not need default ctor any more. Ditto for Document. https://codereview.chromium.org/2651883003/diff/80001/components/arc/test/fak... components/arc/test/fake_file_system_instance.h:62: File(const File& rhs); IIUC, because this is not DISALLOW_COPY_AND_ASSIGN() struct, you can simply drop declaration and definition of ctor, so that compiler will generate default copy-ctor automatically, and this gets movable. https://codereview.chromium.org/2651883003/diff/80001/components/arc/test/fak... components/arc/test/fake_file_system_instance.h:75: // ID of the parent document. Can be nullptr string if this is a root. If you use std::string, |nullptr| is out-dated comment. Could you update it?
The CQ bit was checked by nya@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...
PTAL https://codereview.chromium.org/2651883003/diff/60001/components/arc/test/fak... File components/arc/test/fake_file_system_instance.cc (right): https://codereview.chromium.org/2651883003/diff/60001/components/arc/test/fak... components/arc/test/fake_file_system_instance.cc:63: DCHECK(thread_checker_.CalledOnValidThread()); On 2017/01/27 15:50:00, hidehiko wrote: > This looks redundant, because |thread_checker_| is initialized in the ctor, > here, so it always succeeds. Done. https://codereview.chromium.org/2651883003/diff/60001/components/arc/test/fak... File components/arc/test/fake_file_system_instance.h (right): https://codereview.chromium.org/2651883003/diff/60001/components/arc/test/fak... components/arc/test/fake_file_system_instance.h:46: enum Seekable { On 2017/01/27 15:50:00, hidehiko wrote: > Could you use "enum class"? Done. https://codereview.chromium.org/2651883003/diff/80001/components/arc/test/fak... File components/arc/test/fake_file_system_instance.cc (right): https://codereview.chromium.org/2651883003/diff/80001/components/arc/test/fak... components/arc/test/fake_file_system_instance.cc:30: int written_size = base::WriteFile(path, content.c_str(), content.size()); On 2017/01/27 15:50:00, hidehiko wrote: > s/content.c_str()/constent.data()/ > > Here, std::string is used as a byte-buffer. > So, data() is more appropriate, I think, because c_str() means c-style string > (\0 terminate str) conceptually. > (Note, AFAIK, these are not much different in C++11, but for readability). I don't have any preference on this. https://codereview.chromium.org/2651883003/diff/80001/components/arc/test/fak... components/arc/test/fake_file_system_instance.cc:44: fd_write.get(), content.c_str(), content.size()); On 2017/01/27 15:50:00, hidehiko wrote: > Ditto. Done. https://codereview.chromium.org/2651883003/diff/80001/components/arc/test/fak... File components/arc/test/fake_file_system_instance.h (right): https://codereview.chromium.org/2651883003/diff/80001/components/arc/test/fak... components/arc/test/fake_file_system_instance.h:60: File(); On 2017/01/27 15:50:00, hidehiko wrote: > Could you get rid of map::operator[], and replace it by map::insert()? > Then, IIUC, you do not need default ctor any more. > > Ditto for Document. Well, of course I know I can get rid of the default constructor, but I did not bother to do so because implementing the default constructor is not a big deal and having it will allow us to use this type intuitively like map::operator[]. But anyway, if you want me to do so, I can. https://codereview.chromium.org/2651883003/diff/80001/components/arc/test/fak... components/arc/test/fake_file_system_instance.h:62: File(const File& rhs); On 2017/01/27 15:50:00, hidehiko wrote: > IIUC, because this is not DISALLOW_COPY_AND_ASSIGN() struct, you can simply drop > declaration and definition of ctor, so that compiler will generate default > copy-ctor automatically, and this gets movable. As I wrote in earlier comment, we can't drop copy-ctor due to chromium-style requirement. ../../components/arc/test/fake_file_system_instance.h:67:3: error: [chromium-style] Complex class/struct needs an explicit out-of-line copy constructor. struct Document { ^ https://codereview.chromium.org/2651883003/diff/80001/components/arc/test/fak... components/arc/test/fake_file_system_instance.h:75: // ID of the parent document. Can be nullptr string if this is a root. On 2017/01/27 15:50:00, hidehiko wrote: > If you use std::string, |nullptr| is out-dated comment. Could you update it? Good catch.
https://codereview.chromium.org/2651883003/diff/100001/components/arc/test/fa... File components/arc/test/fake_file_system_instance.h (right): https://codereview.chromium.org/2651883003/diff/100001/components/arc/test/fa... components/arc/test/fake_file_system_instance.h:47: YES, How about swapping the order (i.e. NO, YES) to make NO equal to 0? It should result in less confusing code. https://codereview.chromium.org/2651883003/diff/100001/components/arc/test/fa... components/arc/test/fake_file_system_instance.h:61: File(const File& rhs); "rhs" doesn't make sense in a ctor.
PTAL https://codereview.chromium.org/2651883003/diff/100001/components/arc/test/fa... File components/arc/test/fake_file_system_instance.h (right): https://codereview.chromium.org/2651883003/diff/100001/components/arc/test/fa... components/arc/test/fake_file_system_instance.h:61: File(const File& rhs); On 2017/01/30 03:29:10, hashimoto wrote: > "rhs" doesn't make sense in a ctor. Pro tip: Just "does not make sense" in review comments does not make sense. Please suggest an alternative.
The CQ bit was checked by nya@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2651883003/diff/100001/components/arc/test/fa... File components/arc/test/fake_file_system_instance.h (right): https://codereview.chromium.org/2651883003/diff/100001/components/arc/test/fa... components/arc/test/fake_file_system_instance.h:61: File(const File& rhs); On 2017/01/30 03:39:16, Shuhei Takahashi wrote: > On 2017/01/30 03:29:10, hashimoto wrote: > > "rhs" doesn't make sense in a ctor. > > Pro tip: Just "does not make sense" in review comments does not make sense. > Please suggest an alternative. FYI: I'm fine with either "rhs" or "that" as it is already used in Chromium. According to git grep, "other" looks more popular in Chromium? Or, another pattern is repeating the class name (e.g. File(const File& file), Document(const Document& document)). cf) "git grep -e ' \([a-zA-Z0-9]\+)(const \1&'"
hashimoto: Friendly ping just in case you missed this CL. But I'm not in hurry at all.
lgtm
The CQ bit was checked by nya@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for chrome/browser/chromeos/BUILD.gn:
While running git apply --index -p1;
error: patch failed: chrome/browser/chromeos/BUILD.gn:1458
error: chrome/browser/chromeos/BUILD.gn: patch does not apply
Patch: chrome/browser/chromeos/BUILD.gn
Index: chrome/browser/chromeos/BUILD.gn
diff --git a/chrome/browser/chromeos/BUILD.gn b/chrome/browser/chromeos/BUILD.gn
index
0148c7decad26c636d6679f3523e07930576b38e..b1af366f17f2ca887ef1b2301c1c9bc307ce1082
100644
--- a/chrome/browser/chromeos/BUILD.gn
+++ b/chrome/browser/chromeos/BUILD.gn
@@ -247,8 +247,6 @@ source_set("chromeos") {
"arc/fileapi/arc_content_file_system_file_stream_reader.h",
"arc/fileapi/arc_content_file_system_url_util.cc",
"arc/fileapi/arc_content_file_system_url_util.h",
- "arc/fileapi/arc_deferred_file_system_operation_runner.cc",
- "arc/fileapi/arc_deferred_file_system_operation_runner.h",
"arc/fileapi/arc_documents_provider_async_file_util.cc",
"arc/fileapi/arc_documents_provider_async_file_util.h",
"arc/fileapi/arc_documents_provider_backend_delegate.cc",
@@ -261,6 +259,8 @@ source_set("chromeos") {
"arc/fileapi/arc_documents_provider_root_map.h",
"arc/fileapi/arc_documents_provider_util.cc",
"arc/fileapi/arc_documents_provider_util.h",
+ "arc/fileapi/arc_file_system_operation_runner.cc",
+ "arc/fileapi/arc_file_system_operation_runner.h",
"arc/fileapi/arc_file_system_operation_runner_util.cc",
"arc/fileapi/arc_file_system_operation_runner_util.h",
"arc/fileapi/arc_file_system_service.cc",
@@ -1458,9 +1458,9 @@ source_set("unit_tests") {
"arc/fileapi/arc_content_file_system_async_file_util_unittest.cc",
"arc/fileapi/arc_content_file_system_file_stream_reader_unittest.cc",
"arc/fileapi/arc_content_file_system_url_util_unittest.cc",
- "arc/fileapi/arc_deferred_file_system_operation_runner_unittest.cc",
"arc/fileapi/arc_documents_provider_root_unittest.cc",
"arc/fileapi/arc_documents_provider_util_unittest.cc",
+ "arc/fileapi/arc_file_system_operation_runner_unittest.cc",
"arc/intent_helper/arc_external_protocol_dialog_unittest.cc",
"arc/intent_helper/arc_navigation_throttle_unittest.cc",
"arc/optin/arc_terms_of_service_negotiator_unittest.cc",
The CQ bit was checked by nya@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hidehiko@chromium.org, hashimoto@chromium.org Link to the patchset: https://codereview.chromium.org/2651883003/#ps140001 (title: "Rebased.")
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": 140001, "attempt_start_ts": 1485939399944140,
"parent_rev": "3348391b82de537a2cba63e5c60d0dcbafe40f22", "commit_rev":
"91afa75f31ca8fdfc901de32098ef729218de847"}
Message was sent while issue was closed.
Description was changed from ========== Clean up ARC file system unit tests. 1. Introduce a generic FakeFileSystemInstance. Tests that want to test ARC file system operations can use this fake without implementing its own FileSystemInstance. 2. Removed ArcFileSystemOperationRunner interface. Former ArcDeferredFileSystemOperationRunner is now the only implementation of the interface, so the interface is removed and ArcDeferredFSOpRunner is renamed to ArcFSOpRunner. 3. Inject FileSystemInstance rather than ArcFSOpRunner in unit tests. In unit tests, ArcFSOpRunner without deferring is created with ArcFSOpRunner::CreateForTesting() so that operations are passed through to FileSystemInstance. TEST=unit_tests BUG=chromium:683049 ========== to ========== Clean up ARC file system unit tests. 1. Introduce a generic FakeFileSystemInstance. Tests that want to test ARC file system operations can use this fake without implementing its own FileSystemInstance. 2. Removed ArcFileSystemOperationRunner interface. Former ArcDeferredFileSystemOperationRunner is now the only implementation of the interface, so the interface is removed and ArcDeferredFSOpRunner is renamed to ArcFSOpRunner. 3. Inject FileSystemInstance rather than ArcFSOpRunner in unit tests. In unit tests, ArcFSOpRunner without deferring is created with ArcFSOpRunner::CreateForTesting() so that operations are passed through to FileSystemInstance. TEST=unit_tests BUG=chromium:683049 Review-Url: https://codereview.chromium.org/2651883003 Cr-Commit-Position: refs/heads/master@{#447480} Committed: https://chromium.googlesource.com/chromium/src/+/91afa75f31ca8fdfc901de32098e... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/91afa75f31ca8fdfc901de32098e... |
