Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11348263:
Change FilePathWatcher tests to use the new callback style FilePathWatcher::Watch(). (Closed)
|
Created:
8 years ago by Lei Zhang Modified:
8 years ago CC:
chromium-reviews, erikwright+watch_chromium.org Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionChange FilePathWatcher tests to use the new callback style FilePathWatcher::Watch().
BUG=130980
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170665
Patch Set 1 #Patch Set 2 : weak_ptr #Patch Set 3 : remove debug statement #
Total comments: 15
Patch Set 4 : address comments #
Total comments: 2
Patch Set 5 : more templates #Patch Set 6 : move deleter back where it was #
Total comments: 4
Patch Set 7 : delete callback test, add test delegate base class #Patch Set 8 : add overrides #Messages
Total messages: 18 (0 generated)
See patch set 3.
https://codereview.chromium.org/11348263/diff/5001/base/files/file_path_watch... File base/files/file_path_watcher_browsertest.cc (right): https://codereview.chromium.org/11348263/diff/5001/base/files/file_path_watch... base/files/file_path_watcher_browsertest.cc:100: class TestDelegate : public SupportsWeakPtr<TestDelegate> { Do we need to update the comment? Can we rename TestDelegate => TestFileChangeEvents or something similar? https://codereview.chromium.org/11348263/diff/5001/base/files/file_path_watch... base/files/file_path_watcher_browsertest.cc:104: // guarantee |loop| outlives the file thread on which OnFileChanged runs. Do we need to update this comment? https://codereview.chromium.org/11348263/diff/5001/base/files/file_path_watch... base/files/file_path_watcher_browsertest.cc:163: base::WaitableEvent* completion) { nit: DCHECK(watcher) DCHECK(deleter) DCHECK(result) DCHECK(completion) https://codereview.chromium.org/11348263/diff/5001/base/files/file_path_watch... base/files/file_path_watcher_browsertest.cc:230: TestDelegate* delegate) WARN_UNUSED_RESULT { WARN_UNUSED_RESULT is not required any more. https://codereview.chromium.org/11348263/diff/5001/base/files/file_path_watch... base/files/file_path_watcher_browsertest.cc:243: Deleter* deleter) WARN_UNUSED_RESULT { WARN_UNUSED_RESULT is not required any more. https://codereview.chromium.org/11348263/diff/5001/base/files/file_path_watch... base/files/file_path_watcher_browsertest.cc:375: ASSERT_TRUE(deleter->watcher() == NULL); Do we need to Delete the |deleter| on file thread? https://codereview.chromium.org/11348263/diff/5001/base/files/file_path_watch... base/files/file_path_watcher_browsertest.cc:427: ASSERT_TRUE(WriteFile(file, "content v2")); nit: Can we use kNewContent instead of "content v2" or declare a new const char[] for "content v2"?
https://codereview.chromium.org/11348263/diff/5001/base/files/file_path_watch... File base/files/file_path_watcher_browsertest.cc (right): https://codereview.chromium.org/11348263/diff/5001/base/files/file_path_watch... base/files/file_path_watcher_browsertest.cc:148: void SetupWatchCallbackForTestDelegate(const FilePath& target, SetupWatchCallbackForTestDelegate and SetupWatchCallbackForDeleter looks very similar expect for the third input param. Can we use function templates? Thereby, we can avoid SetupWatchForTestDelegate and SetupWatchForTestDeleter.
https://codereview.chromium.org/11348263/diff/5001/base/files/file_path_watch... File base/files/file_path_watcher_browsertest.cc (right): https://codereview.chromium.org/11348263/diff/5001/base/files/file_path_watch... base/files/file_path_watcher_browsertest.cc:100: class TestDelegate : public SupportsWeakPtr<TestDelegate> { On 2012/11/28 20:13:06, kmadhusu wrote: > Do we need to update the comment? Done. > Can we rename TestDelegate => TestFileChangeEvents or something similar? I don't think anything has really changed in this CL. TestDelegate is still the class that gets delegated the job of handling the callbacks from FilePathWatcher. https://codereview.chromium.org/11348263/diff/5001/base/files/file_path_watch... base/files/file_path_watcher_browsertest.cc:104: // guarantee |loop| outlives the file thread on which OnFileChanged runs. On 2012/11/28 20:13:06, kmadhusu wrote: > Do we need to update this comment? The comment is obsolete. https://codereview.chromium.org/11348263/diff/5001/base/files/file_path_watch... base/files/file_path_watcher_browsertest.cc:148: void SetupWatchCallbackForTestDelegate(const FilePath& target, On 2012/11/28 20:19:44, kmadhusu wrote: > SetupWatchCallbackForTestDelegate and SetupWatchCallbackForDeleter looks very > similar expect for the third input param. Can we use function templates? > Thereby, we can avoid SetupWatchForTestDelegate and SetupWatchForTestDeleter. Done. https://codereview.chromium.org/11348263/diff/5001/base/files/file_path_watch... base/files/file_path_watcher_browsertest.cc:163: base::WaitableEvent* completion) { On 2012/11/28 20:13:06, kmadhusu wrote: > nit: > DCHECK(watcher) > DCHECK(deleter) > DCHECK(result) > DCHECK(completion) Given how straight-forward this 4 line function is, we really don't need these DCHECKs to document the fact that these pointers should be non-NULL. If it crashes, the reason should be obvious. We also shouldn't use them in tests. See the last bullet point in http://dev.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED- https://codereview.chromium.org/11348263/diff/5001/base/files/file_path_watch... base/files/file_path_watcher_browsertest.cc:230: TestDelegate* delegate) WARN_UNUSED_RESULT { On 2012/11/28 20:13:06, kmadhusu wrote: > WARN_UNUSED_RESULT is not required any more. Why not? The caller should check and make sure SetupWatch succeeded. https://codereview.chromium.org/11348263/diff/5001/base/files/file_path_watch... base/files/file_path_watcher_browsertest.cc:375: ASSERT_TRUE(deleter->watcher() == NULL); On 2012/11/28 20:13:06, kmadhusu wrote: > Do we need to Delete the |deleter| on file thread? No, once the watcher is gone, the callback that contains the weak pointer is gone too. Thus it is safe to delete the Deleter on any thread. https://codereview.chromium.org/11348263/diff/5001/base/files/file_path_watch... base/files/file_path_watcher_browsertest.cc:427: ASSERT_TRUE(WriteFile(file, "content v2")); On 2012/11/28 20:13:06, kmadhusu wrote: > nit: Can we use kNewContent instead of "content v2" or declare a new const > char[] for "content v2"? I reverted this cleanup, since it's also distracting from the main review.
https://codereview.chromium.org/11348263/diff/5003/base/files/file_path_watch... File base/files/file_path_watcher_browsertest.cc (right): https://codereview.chromium.org/11348263/diff/5003/base/files/file_path_watch... base/files/file_path_watcher_browsertest.cc:227: bool SetupWatchForDeleter(const FilePath& target, Can you write a common function template to replace SetupWatchForTestDelegate and SetupWatchForDeleter?
https://codereview.chromium.org/11348263/diff/5003/base/files/file_path_watch... File base/files/file_path_watcher_browsertest.cc (right): https://codereview.chromium.org/11348263/diff/5003/base/files/file_path_watch... base/files/file_path_watcher_browsertest.cc:227: bool SetupWatchForDeleter(const FilePath& target, On 2012/11/28 23:42:38, kmadhusu wrote: > Can you write a common function template to replace SetupWatchForTestDelegate > and SetupWatchForDeleter? Done, but it cannot be inlined.
lgtm
+mnissler
Mostly good, the one issue I'd like to settle is whether we can consolidate the separate two mechanisms for installing callbacks into one. https://codereview.chromium.org/11348263/diff/10001/base/files/file_path_watc... File base/files/file_path_watcher_browsertest.cc (right): https://codereview.chromium.org/11348263/diff/10001/base/files/file_path_watc... base/files/file_path_watcher_browsertest.cc:119: template <class T> Note: Instead of templates, you could use a Delegate base class with a virtual void OnFileChanged(). I guess either is fine with me, but I seem to remember the style guide advises against templates in general. https://codereview.chromium.org/11348263/diff/10001/base/files/file_path_watc... base/files/file_path_watcher_browsertest.cc:296: loop_.Run(); There are now essentially two mechanisms for setting the callback and synchronizing threads: The one in SetupWatchCallback uses a WaitableEvent for that, the one here uses PostTaskAndReply. Can we do with only one version? For the record, I'd prefer the WaitableEvent-based version, since that doesn't require loop_.Run().
https://codereview.chromium.org/11348263/diff/10001/base/files/file_path_watc... File base/files/file_path_watcher_browsertest.cc (right): https://codereview.chromium.org/11348263/diff/10001/base/files/file_path_watc... base/files/file_path_watcher_browsertest.cc:119: template <class T> On 2012/11/29 10:45:34, Mattias Nissler wrote: > Note: Instead of templates, you could use a Delegate base class with a virtual > void OnFileChanged(). I guess either is fine with me, but I seem to remember the > style guide advises against templates in general. Done. https://codereview.chromium.org/11348263/diff/10001/base/files/file_path_watc... base/files/file_path_watcher_browsertest.cc:296: loop_.Run(); On 2012/11/29 10:45:34, Mattias Nissler wrote: > There are now essentially two mechanisms for setting the callback and > synchronizing threads: The one in SetupWatchCallback uses a WaitableEvent for > that, the one here uses PostTaskAndReply. Can we do with only one version? For > the record, I'd prefer the WaitableEvent-based version, since that doesn't > require loop_.Run(). I deleted this test. All the tests have been switched over to the new callback system, so having an additional test that tests the new callback system is unnecessary.
LGTM, thanks for doing this!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/thestig@chromium.org/11348263/5005
Retried try job too often on win_aura for step(s) aura_unittests, content_browsertests, content_unittests, views_unittests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/thestig@chromium.org/11348263/5005
Sorry for I got bad news for ya. Compile failed with a clobber build on win_rel. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/thestig@chromium.org/11348263/5005
Message was sent while issue was closed.
Change committed as 170665 |
