Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2709613006:
mediaview: Implement FakeFileSystemInstance. (Closed)
|
Created:
3 years, 10 months ago by Shuhei Takahashi Modified:
3 years, 10 months ago Reviewers:
hidehiko CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, yusukes+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionmediaview: Implement FakeFileSystemInstance.
This patch adds fake implementations of new FileSystemInstance methods
introduced in https://codereview.chromium.org/2714433003.
BUG=chromium:684233
TEST=trybot
Review-Url: https://codereview.chromium.org/2709613006
Cr-Commit-Position: refs/heads/master@{#452776}
Committed: https://chromium.googlesource.com/chromium/src/+/3130418a5b9bb3e1bb389976617e3bd112ec37aa
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : Review ready. #
Total comments: 6
Patch Set 4 : Addressed hidehiko's comments. #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 24 (19 generated)
Description was changed from ========== wip BUG= ========== to ========== mediaview: Implement FakeFileSystemInstance. This patch adds fake implementations of new FileSystemInstance methods introduced in https://codereview.chromium.org/2714433003. BUG=chromium:684233 TEST=trybot ==========
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.
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...
nya@chromium.org changed reviewers: + hidehiko@chromium.org
hidehiko: PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with minor comment. https://codereview.chromium.org/2709613006/diff/40001/components/arc/test/fak... File components/arc/test/fake_file_system_instance.cc (right): https://codereview.chromium.org/2709613006/diff/40001/components/arc/test/fak... components/arc/test/fake_file_system_instance.cc:133: if (iter == document_to_watchers_.end()) { Could you elide the brace for one line if stmt? https://codereview.chromium.org/2709613006/diff/40001/components/arc/test/fak... components/arc/test/fake_file_system_instance.cc:257: document_to_watchers_[key].erase(watcher_id); Optional: you can document_to_watchers_[iter->second].erase(watcher_id); at L255 so that key is not needed. https://codereview.chromium.org/2709613006/diff/40001/components/arc/test/fak... File components/arc/test/fake_file_system_instance.h (right): https://codereview.chromium.org/2709613006/diff/40001/components/arc/test/fak... components/arc/test/fake_file_system_instance.h:165: int64_t next_watcher_id_ = 1; Just curious: any reasons this id starts from 1 rather than 0?
Patchset #4 (id:60001) has been deleted
Thanks for reviewing! https://codereview.chromium.org/2709613006/diff/40001/components/arc/test/fak... File components/arc/test/fake_file_system_instance.cc (right): https://codereview.chromium.org/2709613006/diff/40001/components/arc/test/fak... components/arc/test/fake_file_system_instance.cc:133: if (iter == document_to_watchers_.end()) { On 2017/02/23 11:30:45, hidehiko wrote: > Could you elide the brace for one line if stmt? Done. https://codereview.chromium.org/2709613006/diff/40001/components/arc/test/fak... components/arc/test/fake_file_system_instance.cc:257: document_to_watchers_[key].erase(watcher_id); On 2017/02/23 11:30:45, hidehiko wrote: > Optional: you can > document_to_watchers_[iter->second].erase(watcher_id); > at L255 so that key is not needed. OK, done. (I also changed the argument of watcher_to_document_.erase() to iter so that it is more obvious iter is invalidated) https://codereview.chromium.org/2709613006/diff/40001/components/arc/test/fak... File components/arc/test/fake_file_system_instance.h (right): https://codereview.chromium.org/2709613006/diff/40001/components/arc/test/fak... components/arc/test/fake_file_system_instance.h:165: int64_t next_watcher_id_ = 1; On 2017/02/23 11:30:45, hidehiko wrote: > Just curious: any reasons this id starts from 1 rather than 0? Because 0 is not obvious to me if it indicates failure or success. Using -1 for failures and positive integers for success sounds clearer to me.
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.
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 Link to the patchset: https://codereview.chromium.org/2709613006/#ps80001 (title: "Addressed hidehiko's 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": 80001, "attempt_start_ts": 1487918565313030,
"parent_rev": "533aed06060aba3689e57e04bb352f14cfc7ca25", "commit_rev":
"3130418a5b9bb3e1bb389976617e3bd112ec37aa"}
Message was sent while issue was closed.
Description was changed from ========== mediaview: Implement FakeFileSystemInstance. This patch adds fake implementations of new FileSystemInstance methods introduced in https://codereview.chromium.org/2714433003. BUG=chromium:684233 TEST=trybot ========== to ========== mediaview: Implement FakeFileSystemInstance. This patch adds fake implementations of new FileSystemInstance methods introduced in https://codereview.chromium.org/2714433003. BUG=chromium:684233 TEST=trybot Review-Url: https://codereview.chromium.org/2709613006 Cr-Commit-Position: refs/heads/master@{#452776} Committed: https://chromium.googlesource.com/chromium/src/+/3130418a5b9bb3e1bb389976617e... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as https://chromium.googlesource.com/chromium/src/+/3130418a5b9bb3e1bb389976617e... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
