It's confusing to have a class called EventLoop which spins a run loop.
Created attachment 380492 [details] Patch
Created attachment 380494 [details] Fix GTK+
Created attachment 380495 [details] Patch
Comment on attachment 380495 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=380495&action=review r=me, this looks fine to me, but I'd suggest asking Joe to take a look as he's more familiar with this code > Source/JavaScriptCore/inspector/JSGlobalObjectScriptDebugServer.cpp:79 > + return "com.apple.JavaScriptCore.remote-inspector-runloop-mode"; `_s`? > Source/JavaScriptCore/inspector/JSGlobalObjectScriptDebugServer.h:41 > + static String runLoopMode(); NIT: I think we should move this to `RemoteInspectionTarget` since it's more "generic" than `JSGlobalObjectScriptDebugServer` (`RemoteInspectionTarget` is also used when inspecting webpages, which doesn't use `JSGlobalObjectScriptDebugServer`). > Source/WTF/wtf/win/RunLoopWin.cpp:125 > + MSG msg; NIT: we should call this `message` to match the rest of the file > Source/WTF/wtf/win/RunLoopWin.cpp:127 > + m_ended = true; This no longer exists. > Source/WTF/wtf/win/RunLoopWin.cpp:132 > + TranslateMessage(&msg); > + DispatchMessage(&msg); Should these be `::TranslateMessage` and `::DispatchMessage`?
Comment on attachment 380495 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=380495&action=review >> Source/JavaScriptCore/inspector/JSGlobalObjectScriptDebugServer.h:41 >> + static String runLoopMode(); > > NIT: I think we should move this to `RemoteInspectionTarget` since it's more "generic" than `JSGlobalObjectScriptDebugServer` (`RemoteInspectionTarget` is also used when inspecting webpages, which doesn't use `JSGlobalObjectScriptDebugServer`). WebCore uses a slightly different EventLoop with different modes. Only JSContext debugging used the special mode which pauses the world. > Source/WTF/wtf/cf/RunLoopCF.cpp:70 > +RunLoop::CycleResult RunLoop::cycle(String mode) > +{ > + CFTimeInterval timeInterval = 0.05; > + CFRunLoopRunInMode(mode.isNull() ? kCFRunLoopDefaultMode : mode.createCFString().get(), timeInterval, true); > + return CycleResult::Continue; > +} According to r151562 (bug 117596) https://trac.webkit.org/changeset/151562/webkit This would produce hangs while paused in Web Inspector in other apps. Might want to do the following for PLATFORM(MAC): [NSApp setWindowsNeedUpdate:YES]; [NSApp sendEvent:[NSApp nextEventMatchingMask:NSAnyEventMask untilDate:[NSDate dateWithTimeIntervalSinceNow:0.05] inMode:NSDefaultRunLoopMode dequeue:YES]];
> [NSApp setWindowsNeedUpdate:YES]; > [NSApp sendEvent:[NSApp nextEventMatchingMask:NSAnyEventMask > untilDate:[NSDate dateWithTimeIntervalSinceNow:0.05] > inMode:NSDefaultRunLoopMode dequeue:YES]]; Err, obviously respecting the `mode` passed in.
(In reply to Devin Rousso from comment #4) > Comment on attachment 380495 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=380495&action=review > > r=me, this looks fine to me, but I'd suggest asking Joe to take a look as > he's more familiar with this code > > > Source/JavaScriptCore/inspector/JSGlobalObjectScriptDebugServer.cpp:79 > > + return "com.apple.JavaScriptCore.remote-inspector-runloop-mode"; > > `_s`? Fixed. > > Source/JavaScriptCore/inspector/JSGlobalObjectScriptDebugServer.h:41 > > + static String runLoopMode(); > > NIT: I think we should move this to `RemoteInspectionTarget` since it's more > "generic" than `JSGlobalObjectScriptDebugServer` (`RemoteInspectionTarget` > is also used when inspecting webpages, which doesn't use > `JSGlobalObjectScriptDebugServer`). We can't doe that because RemoteInspectionTarget isn't always enabled but JSGlobalObjectScriptDebugServer is. > > Source/WTF/wtf/win/RunLoopWin.cpp:125 > > + MSG msg; > > NIT: we should call this `message` to match the rest of the file Fixed. > > Source/WTF/wtf/win/RunLoopWin.cpp:127 > > + m_ended = true; > > This no longer exists. Removed. > > Source/WTF/wtf/win/RunLoopWin.cpp:132 > > + TranslateMessage(&msg); > > + DispatchMessage(&msg); > > Should these be `::TranslateMessage` and `::DispatchMessage`? I'll do that although it shouldn't matter.
(In reply to Joseph Pecoraro from comment #5) > Comment on attachment 380495 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=380495&action=review > > > Source/WTF/wtf/cf/RunLoopCF.cpp:70 > > +RunLoop::CycleResult RunLoop::cycle(String mode) > > +{ > > + CFTimeInterval timeInterval = 0.05; > > + CFRunLoopRunInMode(mode.isNull() ? kCFRunLoopDefaultMode : mode.createCFString().get(), timeInterval, true); > > + return CycleResult::Continue; > > +} > > According to r151562 (bug 117596) > https://trac.webkit.org/changeset/151562/webkit > > This would produce hangs while paused in Web Inspector in other apps. > > Might want to do the following for PLATFORM(MAC): > > [NSApp setWindowsNeedUpdate:YES]; > [NSApp sendEvent:[NSApp nextEventMatchingMask:NSAnyEventMask > untilDate:[NSDate dateWithTimeIntervalSinceNow:0.05] > inMode:NSDefaultRunLoopMode dequeue:YES]]; Ugh... we can't do that because JavaScriptCore can't link against AppKit :(
(In reply to Joseph Pecoraro from comment #5) > Comment on attachment 380495 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=380495&action=review > > > Source/WTF/wtf/cf/RunLoopCF.cpp:70 > > +RunLoop::CycleResult RunLoop::cycle(String mode) > > +{ > > + CFTimeInterval timeInterval = 0.05; > > + CFRunLoopRunInMode(mode.isNull() ? kCFRunLoopDefaultMode : mode.createCFString().get(), timeInterval, true); > > + return CycleResult::Continue; > > +} > > According to r151562 (bug 117596) > https://trac.webkit.org/changeset/151562/webkit > > This would produce hangs while paused in Web Inspector in other apps. Looks like this fix is no longer needed. I've tested both Mini Browser WK1 as well as the app attached on <rdar://problem/14133001> and they both seem to work fine.
Created attachment 380716 [details] Updated
Comment on attachment 380716 [details] Updated View in context: https://bugs.webkit.org/attachment.cgi?id=380716&action=review r=me > Source/WTF/wtf/RunLoop.h:65 > + WTF_EXPORT_PRIVATE CycleResult static cycle(String = { }); Can this be a `const String&` or does it need to be a `String` for a particular reason?
Comment on attachment 380716 [details] Updated View in context: https://bugs.webkit.org/attachment.cgi?id=380716&action=review > ChangeLog:8 > + * WebKit.xcworkspace/xcshareddata/WorkspaceSettings.xcsettings: This doesn't look like it was modified. Might be an accident, so this file can be unmodified.
Committed r251036: <https://trac.webkit.org/changeset/251036>
<rdar://problem/56312647>
Comment on attachment 380495 [details] Patch Good cleanup. I'll keep my eyes open for regressions that come in. In particular, auto-inspect and remote debugging 3rd party apps seem most affected by RunLoop modes.