Bug 202716 – Make WebInspector's remote debug EventLoop code into RunLoop

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Ryosuke Niwa]

It's confusing to have a class called EventLoop which spins a run loop.

[Ryosuke Niwa]

Created attachment 380492 [details]
Patch

[Ryosuke Niwa]

Created attachment 380494 [details]
Fix GTK+

[Ryosuke Niwa]

Created attachment 380495 [details]
Patch

[Devin Rousso]

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`?

[Joseph Pecoraro]

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]];

[Joseph Pecoraro]

>     [NSApp setWindowsNeedUpdate:YES];
>     [NSApp sendEvent:[NSApp nextEventMatchingMask:NSAnyEventMask
> untilDate:[NSDate dateWithTimeIntervalSinceNow:0.05]
> inMode:NSDefaultRunLoopMode dequeue:YES]];

Err, obviously respecting the `mode` passed in.

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

(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 :(

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

Created attachment 380716 [details]
Updated

[Joseph Pecoraro]

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?

[Joseph Pecoraro]

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.

[Ryosuke Niwa]

Committed r251036: <https://trac.webkit.org/changeset/251036>

[Radar WebKit Bug Importer]

<rdar://problem/56312647>

[Blaze Burg]

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.