Bug 170391

Created attachment 306055 [details] Patch

Comment on attachment 306055 [details] Patch It looks OK to me, but I wonder if the separate implementation for CF is really necessary. If it really is, then I'd expect RunLoop to be implemented in terms of CFRunLoop....

Comment on attachment 306055 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=306055&action=review > Source/JavaScriptCore/runtime/PromiseDeferredTimer.cpp:-85 > +#if USE(CF) > CFRunLoopRun(); > -} It's unnecessary since we already have the assertion that CFRunLoopGetCurrent() == m_runLoop.get(). So, RunLoop::run() drives current thread RunLoop and it is OK for all the ports.

Comment on attachment 306055 [details] Patch OK then, please rewrite to get rid of CFRunLoop.

(In reply to Michael Catanzaro from comment #4) > Comment on attachment 306055 [details] > Patch > > OK then, please rewrite to get rid of CFRunLoop. A bit worried thing is that, current RunLoop & RunLoop::Timer do not have the full features of CFRunLoop. For example, RunLoop is thread-safe, functions can be called from the other thread. But RunLoop::Timer is only available from the created thread. And current Heap's timers are touched from the other thread. So, I think we still need to use CFRunLoop right now. But at least, in runRunLoop, we can just use RunLoop::run(). But for doWork function, we still need CFRunLoop thing.

(In reply to Yusuke Suzuki from comment #5) > (In reply to Michael Catanzaro from comment #4) > > Comment on attachment 306055 [details] > > Patch > > > > OK then, please rewrite to get rid of CFRunLoop. > > A bit worried thing is that, current RunLoop & RunLoop::Timer do not have > the full features of CFRunLoop. For example, RunLoop is thread-safe, > functions can be called from the other thread. But RunLoop::Timer is only > available from the created thread. And current Heap's timers are touched > from the other thread. > > So, I think we still need to use CFRunLoop right now. But at least, in > runRunLoop, we can just use RunLoop::run(). But for doWork function, we > still need CFRunLoop thing. Also note that iOS calls setRunLoop. Since CF timers already have a m_runLoop member I thought it was safer to use the member. I agree we could use RunLoop::run(), but since we need to keep the CF ifdefs for the stop, I think it's less confusing to have ifdefs for the run too.

(In reply to Carlos Garcia Campos from comment #6) > (In reply to Yusuke Suzuki from comment #5) > > (In reply to Michael Catanzaro from comment #4) > > > Comment on attachment 306055 [details] > > > Patch > > > > > > OK then, please rewrite to get rid of CFRunLoop. > > > > A bit worried thing is that, current RunLoop & RunLoop::Timer do not have > > the full features of CFRunLoop. For example, RunLoop is thread-safe, > > functions can be called from the other thread. But RunLoop::Timer is only > > available from the created thread. And current Heap's timers are touched > > from the other thread. > > > > So, I think we still need to use CFRunLoop right now. But at least, in > > runRunLoop, we can just use RunLoop::run(). But for doWork function, we > > still need CFRunLoop thing. > > Also note that iOS calls setRunLoop. Since CF timers already have a > m_runLoop member I thought it was safer to use the member. I agree we could > use RunLoop::run(), but since we need to keep the CF ifdefs for the stop, I > think it's less confusing to have ifdefs for the run too. OK, we should change & drop all the CF things when we use RunLoop & RunLoop::Timer thing exclusively. Eventually, we should change current JSRunLoopTimer to accept RunLoop instead of CFRunLoopRef thing.

Committed r214936: <http://trac.webkit.org/changeset/214936>