Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
250833003:
Clean up the Linux FileWatcher implementation. (Closed)
|
Created:
6 years, 8 months ago by Lei Zhang Modified:
6 years, 7 months ago Reviewers:
Mattias Nissler (ping if slow) CC:
chromium-reviews, erikwright+watch_chromium.org Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionClean up the Linux FileWatcher implementation.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266387
Patch Set 1 #
Total comments: 7
Patch Set 2 : reflow comments #Patch Set 3 : fix style error for struct, misc nits #
Total comments: 11
Patch Set 4 : address comments #Messages
Total messages: 14 (0 generated)
https://codereview.chromium.org/250833003/diff/1/base/files/file_path_watcher... File base/files/file_path_watcher_linux.cc (left): https://codereview.chromium.org/250833003/diff/1/base/files/file_path_watcher... base/files/file_path_watcher_linux.cc:53: bool RemoveWatch(Watch watch, FilePathWatcherImpl* watcher); Nobody checks the return result. https://codereview.chromium.org/250833003/diff/1/base/files/file_path_watcher... base/files/file_path_watcher_linux.cc:126: WatchEntry(InotifyReader::Watch watch, const FilePath::StringType& subdir) Always called with |watch| = -1 https://codereview.chromium.org/250833003/diff/1/base/files/file_path_watcher... base/files/file_path_watcher_linux.cc:332: DCHECK(watch_entry->subdir_.empty() || No longer needed since we already validated |watches_| before the loop. https://codereview.chromium.org/250833003/diff/1/base/files/file_path_watcher... File base/files/file_path_watcher_linux.cc (right): https://codereview.chromium.org/250833003/diff/1/base/files/file_path_watcher... base/files/file_path_watcher_linux.cc:278: if (!valid_ || (watch == kInvalidWatch)) Slightly easier to check for invalid |watch| here than in all the callers. https://codereview.chromium.org/250833003/diff/1/base/files/file_path_watcher... base/files/file_path_watcher_linux.cc:324: if (watches_.empty()) { Only needed because we call DCHECK(HasValidWatchVector()); below. I added HasValidWatchVector() so when we operate on |watches_|, we can check and make sure the variants have been satisfied. https://codereview.chromium.org/250833003/diff/1/base/files/file_path_watcher... base/files/file_path_watcher_linux.cc:341: (child == watch_entry.linkname_) || I reordered the checks to avoid checking for linkname_.empty(). |child| has to be non-empty due to the prev line, so if we pass this check, on the next line, linkname_.empty() is implied. https://codereview.chromium.org/250833003/diff/1/base/files/file_path_watcher... base/files/file_path_watcher_linux.cc:452: watch_entry.linkname_.clear(); This is one actual change to the functionality. When we are updating, part of the path that used to be a symlink may no longer be a symlink.
See patch set 3
https://codereview.chromium.org/250833003/diff/40001/base/files/file_path_wat... File base/files/file_path_watcher_linux.cc (left): https://codereview.chromium.org/250833003/diff/40001/base/files/file_path_wat... base/files/file_path_watcher_linux.cc:291: FilePath::StringType child(event->len ? event->name : FILE_PATH_LITERAL("")); Dropping FILE_PATH_LITERAL adds inconsistency: Either we use FilePath::StringType with FILE_PATH_LITERAL, or we accept this code is only ever going to be useful on Linux, in which case we should use std::string instead of FilePath::StringType and drop FILE_PATH_LITERAL. I'm leaning towards the former, since that is more consistent with the rest of the code base. https://codereview.chromium.org/250833003/diff/40001/base/files/file_path_wat... File base/files/file_path_watcher_linux.cc (right): https://codereview.chromium.org/250833003/diff/40001/base/files/file_path_wat... base/files/file_path_watcher_linux.cc:323: // Happens when code flow reaches here from the PostTask() above. nit: s/Happens/May happen/ https://codereview.chromium.org/250833003/diff/40001/base/files/file_path_wat... base/files/file_path_watcher_linux.cc:333: for (size_t i = 0; i < watches_.size(); ++i) { So you just prefer indexing over iterators? Or is there a good reason for the conversion(s) that I'm missing? https://codereview.chromium.org/250833003/diff/40001/base/files/file_path_wat... base/files/file_path_watcher_linux.cc:345: bool is_direct_child = watch_entry.subdir.empty(); is_direct_child is a misleading name. If watch_entry.subdir.empty(), then that means there's a change in the last entry in the watch chain, which refers to the target. Hence, a better name here could be |is_watch_for_target|. https://codereview.chromium.org/250833003/diff/40001/base/files/file_path_wat... base/files/file_path_watcher_linux.cc:453: if (watch_entry.watch == InotifyReader::kInvalidWatch) { I would prefer to leave the base::IsLink() check here instead of moving it into AddWatchForBrokenSymlink. Otherwise, one may get the impression from the function name that the condition here indicates we're dealing with a broken symlink, which is not true.
https://codereview.chromium.org/250833003/diff/40001/base/files/file_path_wat... File base/files/file_path_watcher_linux.cc (left): https://codereview.chromium.org/250833003/diff/40001/base/files/file_path_wat... base/files/file_path_watcher_linux.cc:291: FilePath::StringType child(event->len ? event->name : FILE_PATH_LITERAL("")); On 2014/04/25 08:38:24, Mattias Nissler wrote: > Dropping FILE_PATH_LITERAL adds inconsistency: Either we use > FilePath::StringType with FILE_PATH_LITERAL, or we accept this code is only ever > going to be useful on Linux, in which case we should use std::string instead of > FilePath::StringType and drop FILE_PATH_LITERAL. I'm leaning towards the former, > since that is more consistent with the rest of the code base. Ok, done. https://codereview.chromium.org/250833003/diff/40001/base/files/file_path_wat... File base/files/file_path_watcher_linux.cc (right): https://codereview.chromium.org/250833003/diff/40001/base/files/file_path_wat... base/files/file_path_watcher_linux.cc:323: // Happens when code flow reaches here from the PostTask() above. On 2014/04/25 08:38:24, Mattias Nissler wrote: > nit: s/Happens/May happen/ Done. https://codereview.chromium.org/250833003/diff/40001/base/files/file_path_wat... base/files/file_path_watcher_linux.cc:333: for (size_t i = 0; i < watches_.size(); ++i) { On 2014/04/25 08:38:24, Mattias Nissler wrote: > So you just prefer indexing over iterators? Or is there a good reason for the > conversion(s) that I'm missing? I do, it makes the for-loop line easier to read in some cases. e.g. line 387 on the left. https://codereview.chromium.org/250833003/diff/40001/base/files/file_path_wat... base/files/file_path_watcher_linux.cc:345: bool is_direct_child = watch_entry.subdir.empty(); On 2014/04/25 08:38:24, Mattias Nissler wrote: > is_direct_child is a misleading name. If watch_entry.subdir.empty(), then that > means there's a change in the last entry in the watch chain, which refers to the > target. Hence, a better name here could be |is_watch_for_target|. Done. https://codereview.chromium.org/250833003/diff/40001/base/files/file_path_wat... base/files/file_path_watcher_linux.cc:453: if (watch_entry.watch == InotifyReader::kInvalidWatch) { On 2014/04/25 08:38:24, Mattias Nissler wrote: > I would prefer to leave the base::IsLink() check here instead of moving it into > AddWatchForBrokenSymlink. Otherwise, one may get the impression from the > function name that the condition here indicates we're dealing with a broken > symlink, which is not true. Done.
LGTM https://codereview.chromium.org/250833003/diff/40001/base/files/file_path_wat... File base/files/file_path_watcher_linux.cc (right): https://codereview.chromium.org/250833003/diff/40001/base/files/file_path_wat... base/files/file_path_watcher_linux.cc:333: for (size_t i = 0; i < watches_.size(); ++i) { On 2014/04/25 08:49:32, Lei Zhang wrote: > On 2014/04/25 08:38:24, Mattias Nissler wrote: > > So you just prefer indexing over iterators? Or is there a good reason for the > > conversion(s) that I'm missing? > > I do, it makes the for-loop line easier to read in some cases. e.g. line 387 on > the left. I don't see the advantage to be honest, and I personally do prefer iterators as these do work for all STL containers equally well. However, I don't think it's worth fighting over, and I don't convert code from on way to the other when I touch it. Do what you wish.
On 2014/04/25 09:06:57, Mattias Nissler wrote: > LGTM > > https://codereview.chromium.org/250833003/diff/40001/base/files/file_path_wat... > File base/files/file_path_watcher_linux.cc (right): > > https://codereview.chromium.org/250833003/diff/40001/base/files/file_path_wat... > base/files/file_path_watcher_linux.cc:333: for (size_t i = 0; i < > watches_.size(); ++i) { > On 2014/04/25 08:49:32, Lei Zhang wrote: > > On 2014/04/25 08:38:24, Mattias Nissler wrote: > > > So you just prefer indexing over iterators? Or is there a good reason for > the > > > conversion(s) that I'm missing? > > > > I do, it makes the for-loop line easier to read in some cases. e.g. line 387 > on > > the left. > > I don't see the advantage to be honest, and I personally do prefer iterators as > these do work for all STL containers equally well. However, I don't think it's > worth fighting over, and I don't convert code from on way to the other when I > touch it. Do what you wish. For vectors, IMO it's more compact to write: for (size_t i = 0 ...) than the iterator version. You can also access a vector of pointers more directly as foo[i]->bar instead of (*foo_it)->bar.
The CQ bit was checked by thestig@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/thestig@chromium.org/250833003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on win_chromium_rel
The CQ bit was checked by thestig@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/thestig@chromium.org/250833003/60001
Message was sent while issue was closed.
Change committed as 266387
Message was sent while issue was closed.
On 2014/04/25 17:30:16, Lei Zhang wrote: > On 2014/04/25 09:06:57, Mattias Nissler wrote: > > LGTM > > > > > https://codereview.chromium.org/250833003/diff/40001/base/files/file_path_wat... > > File base/files/file_path_watcher_linux.cc (right): > > > > > https://codereview.chromium.org/250833003/diff/40001/base/files/file_path_wat... > > base/files/file_path_watcher_linux.cc:333: for (size_t i = 0; i < > > watches_.size(); ++i) { > > On 2014/04/25 08:49:32, Lei Zhang wrote: > > > On 2014/04/25 08:38:24, Mattias Nissler wrote: > > > > So you just prefer indexing over iterators? Or is there a good reason for > > the > > > > conversion(s) that I'm missing? > > > > > > I do, it makes the for-loop line easier to read in some cases. e.g. line 387 > > on > > > the left. > > > > I don't see the advantage to be honest, and I personally do prefer iterators > as > > these do work for all STL containers equally well. However, I don't think it's > > worth fighting over, and I don't convert code from on way to the other when I > > touch it. Do what you wish. > > For vectors, IMO it's more compact to write: for (size_t i = 0 ...) than the > iterator version. You can also access a vector of pointers more directly as > foo[i]->bar instead of (*foo_it)->bar. I don't see the readability gain that would justify the churn caused by all the back-and-forth changes that people make :) Anyhow, fighting this is wasted time. |
