Move all run-loop calls into the run-loop thread, and use correct memory handling (retain/release).

Move all run-loop calls into the run-loop thread, and use correct memory handling (retain/release).

BUG=
R=sgjesse@google.com

Committed: https://code.google.com/p/dart/source/detail?r=32421

[Søren Gjesse, 2014-02-07 10:20:02]

lgtm, with comments

https://codereview.chromium.org/140183004/diff/1/runtime/bin/file_system_watc...
File runtime/bin/file_system_watcher_macos.cc (right):

https://codereview.chromium.org/140183004/diff/1/runtime/bin/file_system_watc...
runtime/bin/file_system_watcher_macos.cc:68: ref_(NULL) {
Please keep this code in the Start method and call that after creating the
object.

https://codereview.chromium.org/140183004/diff/1/runtime/bin/file_system_watc...
runtime/bin/file_system_watcher_macos.cc:71: context.info = this;
Please add a comment that this is scheduling StartCallback in the run loop
thread.

https://codereview.chromium.org/140183004/diff/1/runtime/bin/file_system_watc...
runtime/bin/file_system_watcher_macos.cc:83: ~Node() {
I am not a great fan of placing this code in the destructor. I think it is much
better to have it in Stop, and then delete the object after calling Stop.

https://codereview.chromium.org/140183004/diff/1/runtime/bin/file_system_watc...
runtime/bin/file_system_watcher_macos.cc:87: context.info = this;
Please add a comment that this is scheduling StopCallback in the run loop
thread.

https://codereview.chromium.org/140183004/diff/1/runtime/bin/file_system_watc...
runtime/bin/file_system_watcher_macos.cc:169: FSEventsWatcher() : run_loop_(0) {
Please add a Start method for starting the run-loop thread and call that after
constructing the object.

https://codereview.chromium.org/140183004/diff/1/runtime/bin/file_system_watc...
runtime/bin/file_system_watcher_macos.cc:183: ~FSEventsWatcher() {
Please move this to a Stop method and call that before calling the destructor.

https://codereview.chromium.org/140183004/diff/1/runtime/bin/file_system_watc...
runtime/bin/file_system_watcher_macos.cc:206: static void Run(uword arg) {
We should consider in debug mode to get the thread id here, and then assert that
the different methods run on the expected thread. That would also serve as
documentation of how this works, as it is pretty subtle.

[Anders Johnsen, 2014-02-07 12:49:24]

https://codereview.chromium.org/140183004/diff/1/runtime/bin/file_system_watc...
File runtime/bin/file_system_watcher_macos.cc (right):

https://codereview.chromium.org/140183004/diff/1/runtime/bin/file_system_watc...
runtime/bin/file_system_watcher_macos.cc:68: ref_(NULL) {
On 2014/02/07 10:20:02, Søren Gjesse wrote:
> Please keep this code in the Start method and call that after creating the
> object.

Done.

https://codereview.chromium.org/140183004/diff/1/runtime/bin/file_system_watc...
runtime/bin/file_system_watcher_macos.cc:71: context.info = this;
On 2014/02/07 10:20:02, Søren Gjesse wrote:
> Please add a comment that this is scheduling StartCallback in the run loop
> thread.

Done.

https://codereview.chromium.org/140183004/diff/1/runtime/bin/file_system_watc...
runtime/bin/file_system_watcher_macos.cc:83: ~Node() {
On 2014/02/07 10:20:02, Søren Gjesse wrote:
> I am not a great fan of placing this code in the destructor. I think it is
much
> better to have it in Stop, and then delete the object after calling Stop.

Done.

https://codereview.chromium.org/140183004/diff/1/runtime/bin/file_system_watc...
runtime/bin/file_system_watcher_macos.cc:87: context.info = this;
On 2014/02/07 10:20:02, Søren Gjesse wrote:
> Please add a comment that this is scheduling StopCallback in the run loop
> thread.

Done.

https://codereview.chromium.org/140183004/diff/1/runtime/bin/file_system_watc...
runtime/bin/file_system_watcher_macos.cc:169: FSEventsWatcher() : run_loop_(0) {
On 2014/02/07 10:20:02, Søren Gjesse wrote:
> Please add a Start method for starting the run-loop thread and call that after
> constructing the object.

Done.

https://codereview.chromium.org/140183004/diff/1/runtime/bin/file_system_watc...
runtime/bin/file_system_watcher_macos.cc:183: ~FSEventsWatcher() {
On 2014/02/07 10:20:02, Søren Gjesse wrote:
> Please move this to a Stop method and call that before calling the destructor.

Done.

https://codereview.chromium.org/140183004/diff/1/runtime/bin/file_system_watc...
runtime/bin/file_system_watcher_macos.cc:206: static void Run(uword arg) {
On 2014/02/07 10:20:02, Søren Gjesse wrote:
> We should consider in debug mode to get the thread id here, and then assert
that
> the different methods run on the expected thread. That would also serve as
> documentation of how this works, as it is pretty subtle.

Done.

[Anders Johnsen, 2014-02-07 12:49:45]

Committed patchset #2 manually as r32421 (presubmit successful).

[Søren Gjesse, 2014-02-07 13:03:25]

https://codereview.chromium.org/140183004/diff/60001/runtime/bin/file_system_...
File runtime/bin/file_system_watcher_macos.cc (right):

https://codereview.chromium.org/140183004/diff/60001/runtime/bin/file_system_...
runtime/bin/file_system_watcher_macos.cc:308: #ifdef DEBUG
Please get rid of all the #ifdef/#endif, and add a comment saying that threadId_
is only used in debug mode.

[Anders Johnsen, 2014-02-07 13:16:11]

https://codereview.chromium.org/140183004/diff/60001/runtime/bin/file_system_...
File runtime/bin/file_system_watcher_macos.cc (right):

https://codereview.chromium.org/140183004/diff/60001/runtime/bin/file_system_...
runtime/bin/file_system_watcher_macos.cc:308: #ifdef DEBUG
On 2014/02/07 13:03:25, Søren Gjesse wrote:
> Please get rid of all the #ifdef/#endif, and add a comment saying that
threadId_
> is only used in debug mode.

Done.