Move FilePathWatcher to base/files.

Move FilePathWatcher to base/files.

FilePathWatcher is going to be used in future for monitoring
changes to /etc/resolv.conf so it needs live somewhere where
various consumers can get at it without incurring the wrath
of the layering gods.

TEST=existing tests
BUG=67734
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=81606

[Craig, 2011-04-05 15:30:38]

Hi Brett

Could you review this for me or suggest someone appropriate please?

Thank you.

(I see dmac has a pending change to filepathwatcher ... I suspect that will land
first so I'll rebase this when it does. I've renamed the _inotify file to _linux
and _browsertest to _unittest btw.)

[dmac, 2011-04-05 18:36:46]

On 2011/04/05 15:30:38, Craig wrote:
> Hi Brett
> 
> Could you review this for me or suggest someone appropriate please?
> 
> Thank you.
> 
> (I see dmac has a pending change to filepathwatcher ... I suspect that will
land
> first so I'll rebase this when it does. I've renamed the _inotify file to
_linux
> and _browsertest to _unittest btw.)

Craig, I think my file_path_watcher stuff is in. Note
http://src.chromium.org/viewvc/chrome?view=rev&revision=80494

[Craig, 2011-04-05 19:30:30]

On 2011/04/05 18:36:46, dmac wrote:
[snip]
> Craig, I think my file_path_watcher stuff is in. Note
> http://src.chromium.org/viewvc/chrome?view=rev&revision=80494

Super, thank you. I have moved the OWNERS file too and picked up the _mac.cc
fix. Please take a look.

[dmac, 2011-04-05 19:55:51]

+ mnissler

http://codereview.chromium.org/6793020/diff/5028/base/files/OWNERS
File base/files/OWNERS (right):

http://codereview.chromium.org/6793020/diff/5028/base/files/OWNERS#newcode1
base/files/OWNERS:1: mnissler@chromium.org
oi... I don't think we want to be OWNERS for all of files....

please mark us with a comment

# for file_path_watcher*

http://codereview.chromium.org/6793020/diff/5028/base/files/file_path_watcher...
File base/files/file_path_watcher_stub.cc (right):

http://codereview.chromium.org/6793020/diff/5028/base/files/file_path_watcher...
base/files/file_path_watcher_stub.cc:16: FileWatcher::Delegate* delegate,
do you need base::files::FileWatcher::Delegate

http://codereview.chromium.org/6793020/diff/5028/base/files/file_path_watcher...
File base/files/file_path_watcher_unittest.cc (right):

http://codereview.chromium.org/6793020/diff/5028/base/files/file_path_watcher...
base/files/file_path_watcher_unittest.cc:5: #include
"base/files/file_path_watcher.h"
this was a browsertest because it locks up in failure conditions. Browsertest
would kill off the test eventually. I have a work around I am working on right
now to fix this condition (awaiting review), but for right now I don't think we
can make this a unittest safely.

http://codereview.chromium.org/6793020/diff/5028/base/files/file_path_watcher...
base/files/file_path_watcher_unittest.cc:14: #endif
instead of prefixes everywhere, how about

using ::base::files;

Would it be better to use a using statement for all of the
file_path_watcher_*.cc files? seems more DRY than prefixing everything.

http://codereview.chromium.org/6793020/diff/5028/content/browser/plugin_servi...
File content/browser/plugin_service.cc (right):

http://codereview.chromium.org/6793020/diff/5028/content/browser/plugin_servi...
content/browser/plugin_service.cc:159: base::files::FilePathWatcher* watcher =
new base::files::FilePathWatcher();
just realized that this should go under the #if defined(OS_WIN) below because it
will leak in the case where plugin_dirs[i] is not absolute.

[Craig, 2011-04-06 15:27:30]

Thanks for the comments! I think I've fixed everything except the unit vs
browser test issue that I have a question about.

http://codereview.chromium.org/6793020/diff/5028/base/files/OWNERS
File base/files/OWNERS (right):

http://codereview.chromium.org/6793020/diff/5028/base/files/OWNERS#newcode1
base/files/OWNERS:1: mnissler@chromium.org
On 2011/04/05 19:55:51, dmac wrote:
> oi... I don't think we want to be OWNERS for all of files....
> 
> please mark us with a comment
> 
> # for file_path_watcher*

Done.

http://codereview.chromium.org/6793020/diff/5028/base/files/file_path_watcher...
File base/files/file_path_watcher_stub.cc (right):

http://codereview.chromium.org/6793020/diff/5028/base/files/file_path_watcher...
base/files/file_path_watcher_stub.cc:16: FileWatcher::Delegate* delegate,
On 2011/04/05 19:55:51, dmac wrote:
> do you need base::files::FileWatcher::Delegate

Fixed with 'using' to match the other changes.

http://codereview.chromium.org/6793020/diff/5028/base/files/file_path_watcher...
File base/files/file_path_watcher_unittest.cc (right):

http://codereview.chromium.org/6793020/diff/5028/base/files/file_path_watcher...
base/files/file_path_watcher_unittest.cc:5: #include
"base/files/file_path_watcher.h"
On 2011/04/05 19:55:51, dmac wrote:
> this was a browsertest because it locks up in failure conditions. Browsertest
> would kill off the test eventually. I have a work around I am working on right
> now to fix this condition (awaiting review), but for right now I don't think
we
> can make this a unittest safely.

OK. Should I wait for your fix or just turn this back into a browsertest? (eg.
leave the file in the new location with the new name and just hook it into the
original gyp file temporarily with a TODO or put the file back in the old
location etc. etc. ... ugly either way).

http://codereview.chromium.org/6793020/diff/5028/base/files/file_path_watcher...
base/files/file_path_watcher_unittest.cc:14: #endif
On 2011/04/05 19:55:51, dmac wrote:
> instead of prefixes everywhere, how about
> 
> using ::base::files;
> 
> Would it be better to use a using statement for all of the
> file_path_watcher_*.cc files? seems more DRY than prefixing everything.

Yes, sorry about that. I've used "using ::base::files::FilePathWatcher;" in the
.cc files now and it's much cleaner.

http://codereview.chromium.org/6793020/diff/5028/content/browser/plugin_servi...
File content/browser/plugin_service.cc (right):

http://codereview.chromium.org/6793020/diff/5028/content/browser/plugin_servi...
content/browser/plugin_service.cc:159: base::files::FilePathWatcher* watcher =
new base::files::FilePathWatcher();
On 2011/04/05 19:55:51, dmac wrote:
> just realized that this should go under the #if defined(OS_WIN) below because
it
> will leak in the case where plugin_dirs[i] is not absolute.

Done.

[brettw, 2011-04-06 16:16:54]

http://codereview.chromium.org/6793020/diff/11001/base/files/file_path_watcher.h
File base/files/file_path_watcher.h (right):

http://codereview.chromium.org/6793020/diff/11001/base/files/file_path_watche...
base/files/file_path_watcher.h:127: #endif  // BASE_FILES_FILE_PATH_WATCHER_H_
Blank line before this.

http://codereview.chromium.org/6793020/diff/11001/base/files/file_path_watche...
File base/files/file_path_watcher_linux.cc (right):

http://codereview.chromium.org/6793020/diff/11001/base/files/file_path_watche...
base/files/file_path_watcher_linux.cc:456: FilePathWatcher::FilePathWatcher() {
Doesn't this need to be in the right namespace to compile? I would expect the
entire file to be wrapped in the base::files namespace and for the using to be
unnecessary. Ditto for the other files.

http://codereview.chromium.org/6793020/diff/11001/base/files/file_path_watche...
File base/files/file_path_watcher_unittest.cc (right):

http://codereview.chromium.org/6793020/diff/11001/base/files/file_path_watche...
base/files/file_path_watcher_unittest.cc:31: using
::base::files::FilePathWatcher;
I would also wrap this in base::files and delete this using.

[dmac, 2011-04-06 16:19:53]

http://codereview.chromium.org/6793020/diff/5028/base/files/file_path_watcher...
File base/files/file_path_watcher_unittest.cc (right):

http://codereview.chromium.org/6793020/diff/5028/base/files/file_path_watcher...
base/files/file_path_watcher_unittest.cc:5: #include
"base/files/file_path_watcher.h"
On 2011/04/06 15:27:30, Craig wrote:
> On 2011/04/05 19:55:51, dmac wrote:
> > this was a browsertest because it locks up in failure conditions.
Browsertest
> > would kill off the test eventually. I have a work around I am working on
right
> > now to fix this condition (awaiting review), but for right now I don't think
> we
> > can make this a unittest safely.
> 
> OK. Should I wait for your fix or just turn this back into a browsertest? (eg.
> leave the file in the new location with the new name and just hook it into the
> original gyp file temporarily with a TODO or put the file back in the old
> location etc. etc. ... ugly either way).

For right now I would turn it back into a browsertest, and hook it from the
original gyp file with a TODO (please log a bug and refer to it in the TODO).
Put my name on the bug, and then when I hopefully get my change in somebody can
fix it up.

[Craig, 2011-04-06 18:14:25]

On 2011/04/06 16:16:54, brettw wrote:
http://codereview.chromium.org/6793020/diff/11001/base/files/file_path_watche...
> base/files/file_path_watcher_linux.cc:456: FilePathWatcher::FilePathWatcher()
{
> Doesn't this need to be in the right namespace to compile? I would expect the
> entire file to be wrapped in the base::files namespace and for the using to be
> unnecessary. Ditto for the other files.

I think the reason why this works (and I confirmed that the constructor is
genuinely in base::files with objdump as I wasn't 100% sure myself now) is that
I've used a 'using declaration' rather than a 'using directive' and so the
compiler has essentially replaced FilePathWatcher with
::base::files::FilePathWatcher everywhere in the file.

I'll wrap it up in a more normal fashion though to avoid confusion and do the
same for _mac, _win and _stub.cc

>
http://codereview.chromium.org/6793020/diff/11001/base/files/file_path_watche...
> File base/files/file_path_watcher_unittest.cc (right):
> 
>
http://codereview.chromium.org/6793020/diff/11001/base/files/file_path_watche...
> base/files/file_path_watcher_unittest.cc:31: using
> ::base::files::FilePathWatcher;
> I would also wrap this in base::files and delete this using.

Will do.

I'll sort this out together with dmac's browesertest change tomorrow as I'm
running a bit late here.

Thank you.

[Ryan Sleevi, 2011-04-07 22:15:00]

Craig: You hopped off IRC by the time I got around; the issue with operator<<
and wstring is because of ADL as you suspected.

http://stackoverflow.com/questions/493985/c-compiler-unable-to-find-function-...

See also http://echelog.matzon.dk/logs/browse/chromium/1295910000 , the
conversation between akalin and thakis @ 22:44

In this case, namespace base { operator<<(std::ostream&, const StringPiece&) }
is what is doing the hiding. 

You can get around it by adding "using ::operator<<;" in the function to pull in
the global operator<<(std::ostream&, const std::wstring&) /
operator<<(std::ostream&, const wchar_t*)

I encountered this same problem in namespace net {}, but luckily I was able to
remove the namespace net { operator<< } as it turned out to be unused.

[Craig, 2011-04-08 17:37:09]

On 2011/04/07 22:15:00, Ryan Sleevi wrote:
> Craig: You hopped off IRC by the time I got around; the issue with operator<<
> and wstring is because of ADL as you suspected.
> 
>
http://stackoverflow.com/questions/493985/c-compiler-unable-to-find-function-...
> 
> See also http://echelog.matzon.dk/logs/browse/chromium/1295910000 , the
> conversation between akalin and thakis @ 22:44
> 
> In this case, namespace base { operator<<(std::ostream&, const StringPiece&) }
> is what is doing the hiding. 
> 
> You can get around it by adding "using ::operator<<;" in the function to pull
in
> the global operator<<(std::ostream&, const std::wstring&) /
> operator<<(std::ostream&, const wchar_t*)
> 
> I encountered this same problem in namespace net {}, but luckily I was able to
> remove the namespace net { operator<< } as it turned out to be unused.

This is awesome. Thanks for the help Ryan! I've added the using trick now.

I've also done the other requested changes:
- namespace base and files stuff in selected places rather than "using
...FilePathWatcher"
- the test is called browsertest again to avoid confusion and hooked into
chrome_tests.gypi with a TODO. It's still in the new location.
- bug has been filed to move the test

Please take another look. Thank you.

[Craig, 2011-04-13 15:22:07]

> Please take another look. Thank you.

Any comments?

[ And just so I don't lose this if the try results disappear, the error message
that resulted in the operator<< fix was this:

Compiling...
file_path_watcher_win.cc
e:\b\build\slave\win\build\src\base\files\file_path_watcher_win.cc(208) : error
C2679: binary '<<' : no operator found which takes a right-hand operand of type
'const FilePath::StringType' (or there is no acceptable conversion) ]

[brettw, 2011-04-13 15:38:02]

LGTM

[dmac, 2011-04-13 15:41:16]

On 2011/04/13 15:38:02, brettw wrote:
> LGTM

LGTM

[jam, 2011-04-18 22:47:58]

btw the content/common/file_path_watcher directory still exists

[Craig Schlenter, 2011-04-19 03:59:52]

On 2011/04/18 22:47:58, John Abd-El-Malek wrote:
> btw the content/common/file_path_watcher directory still exists

Yes, I did see that. Sorry :( I'm planning on doing a svn rm on the dir directly
when I have some time to watch the tree and get sheriff approval to directly
nuke it as neither git nor gcl are keen on killing the dir for me (well it's
already gone in my git checkout but that didn't propagate through with git-cl)

[Craig, 2011-04-21 17:50:40]

On 2011/04/19 03:59:52, Craig Schlenter wrote:
> On 2011/04/18 22:47:58, John Abd-El-Malek wrote:
> > btw the content/common/file_path_watcher directory still exists
> 
> Yes, I did see that. Sorry :( I'm planning on doing a svn rm on the dir
directly
> when I have some time to watch the tree and get sheriff approval to directly
> nuke it as neither git nor gcl are keen on killing the dir for me (well it's
> already gone in my git checkout but that didn't propagate through with git-cl)

FYI, I have nuked the empty subdir now in r82508 and filed crbug.com/80178 for
improving the gcl/git cl tools.

[dmaclach1, 2011-04-21 18:13:46]

Thanks Craig.

Cheers,
Dave

On Thu, Apr 21, 2011 at 10:50 AM,  <craig.schlenter@chromium.org> wrote:
> On 2011/04/19 03:59:52, Craig Schlenter wrote:
>>
>> On 2011/04/18 22:47:58, John Abd-El-Malek wrote:
>> > btw the content/common/file_path_watcher directory still exists
>
>> Yes, I did see that. Sorry :( I'm planning on doing a svn rm on the dir
>
> directly
>>
>> when I have some time to watch the tree and get sheriff approval to
>> directly
>> nuke it as neither git nor gcl are keen on killing the dir for me (well
>> it's
>> already gone in my git checkout but that didn't propagate through with
>> git-cl)
>
> FYI, I have nuked the empty subdir now in r82508 and filed crbug.com/80178
> for
> improving the gcl/git cl tools.
>
> http://codereview.chromium.org/6793020/
>