When NSRunLoop is used in the WebContent process, accessibility must be initialized for VoiceOver to work.
Created attachment 330558 [details] Patch
Comment on attachment 330558 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=330558&action=review This looks good, but I'm sad we have to instantiate an NSApp instance. We should see if the accessibility team can live without that change (but that's not relevant to this patch). > Source/WebKit/Configurations/WebContentService.xcconfig:56 > +RUNLOOP_TYPE[sdk=macosx10.11*] = _NSApplicationMain; Oh, nice! > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:302 > + NSApp = [NSApplication alloc]; We need to see if allocating an NSApplication causes us to attach to stuff we don't want. So the accessibility code needs an NSApp instantiated to work? Or is this just something that they could potentially fix on their side?
(In reply to Brent Fulgham from comment #2) > Comment on attachment 330558 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=330558&action=review > > This looks good, but I'm sad we have to instantiate an NSApp instance. We > should see if the accessibility team can live without that change (but > that's not relevant to this patch). > > > Source/WebKit/Configurations/WebContentService.xcconfig:56 > > +RUNLOOP_TYPE[sdk=macosx10.11*] = _NSApplicationMain; > > Oh, nice! > > > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:302 > > + NSApp = [NSApplication alloc]; > > We need to see if allocating an NSApplication causes us to attach to stuff > we don't want. So the accessibility code needs an NSApp instantiated to > work? Or is this just something that they could potentially fix on their > side? Currently, allocating an NSApplication object is needed for VoiceOver to work, but I believe this could be fixed on their side. Thanks for reviewing!
Comment on attachment 330558 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=330558&action=review >>> Source/WebKit/Configurations/WebContentService.xcconfig:56 >>> +RUNLOOP_TYPE[sdk=macosx10.11*] = _NSApplicationMain; >> >> Oh, nice! > > Currently, allocating an NSApplication object is needed for VoiceOver to work, but I believe this could be fixed on their side. > > Thanks for reviewing! Can you file a Radar about that and add it as a comment to the place where we make the NSApp allocation?
(In reply to Brent Fulgham from comment #4) > Comment on attachment 330558 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=330558&action=review > > >>> Source/WebKit/Configurations/WebContentService.xcconfig:56 > >>> +RUNLOOP_TYPE[sdk=macosx10.11*] = _NSApplicationMain; > >> > >> Oh, nice! > > > > Currently, allocating an NSApplication object is needed for VoiceOver to work, but I believe this could be fixed on their side. > > > > Thanks for reviewing! > > Can you file a Radar about that and add it as a comment to the place where > we make the NSApp allocation? Yes, will do :)
Created attachment 330567 [details] Patch
Created attachment 330717 [details] Patch
Comment on attachment 330717 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=330717&action=review > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:304 > + NSApp = [NSApplication alloc]; > + [NSApplication _accessibilityInitialize]; Calling +alloc without -init is bad.
(In reply to Anders Carlsson from comment #8) > Comment on attachment 330717 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=330717&action=review > > > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:304 > > + NSApp = [NSApplication alloc]; > > + [NSApplication _accessibilityInitialize]; > > Calling +alloc without -init is bad. That makes sense. However, we are trying to avoid all initialization of NSApplication, since it should not be needed in the WebContent process. This allocation will be unnecessary when NSApp is no longer needed for VoiceOver to work. Thanks for reviewing!
Created attachment 330914 [details] Patch
Comment on attachment 330914 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=330914&action=review > Source/WebCore/ChangeLog:4 > + https://bugs.webkit.org/show_bug.cgi?id=181331 Please put the <rdar> here. > Source/WebCore/platform/mac/ThemeMac.mm:71 > + // Creating a NSWindow will crash if the NSApplication event loop is not running. Creating AN (not Creating A) > Source/WebKit/ChangeLog:4 > + https://bugs.webkit.org/show_bug.cgi?id=181331 <radar> > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:92 > ++ (void)_accessibilityInitialize; We should do this in a header file (Source/WebCore/pal/spi/cocoa/NSApplicationSPI.h), then include that header file here. > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:304 > + NSApp = [NSApplication alloc]; While Anders is correct, I prefer that we not initialize NSApp, since we are trying to get rid of it, and it does bad things (sandbox-wise) when it starts up.
This patch looks good, but r- until the SPI is moved to our "spi" folder.
(In reply to Brent Fulgham from comment #12) > This patch looks good, but r- until the SPI is moved to our "spi" folder. Thanks for reviewing! I will update the patch.
<rdar://problem/36408004>
Created attachment 330926 [details] Patch
Comment on attachment 330926 [details] Patch Excellent! r=me.
Comment on attachment 330926 [details] Patch Attachment 330926 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/6023079 New failing tests: http/tests/misc/slow-loading-animated-image.html
Created attachment 330937 [details] Archive of layout-test-results from ews116 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 330926 [details] Patch Attachment 330926 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/6024538 New failing tests: http/tests/workers/service/postmessage-after-sw-process-crash.https.html
Created attachment 330956 [details] Archive of layout-test-results from ews107 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Created attachment 331100 [details] Patch
Comment on attachment 331100 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=331100&action=review Assuming the statement about [NSApplication sharedApplication] I made is true, r=me. > Source/WebCore/PAL/ChangeLog:5 > + rdar://problem/36408004 <rdar://problem/3640004> (I.e., angle brackets around the radar URL please!) > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:177 > + [NSApplication sharedApplication]; So I believe that this call causes an alloc and init of NSApplication to occur. Is that correct?
(In reply to Brent Fulgham from comment #22) > Comment on attachment 331100 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=331100&action=review > > Assuming the statement about [NSApplication sharedApplication] I made is > true, r=me. > > > Source/WebCore/PAL/ChangeLog:5 > > + rdar://problem/36408004 > > <rdar://problem/3640004> > > (I.e., angle brackets around the radar URL please!) > > > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:177 > > + [NSApplication sharedApplication]; > > So I believe that this call causes an alloc and init of NSApplication to > occur. Is that correct? Yes, that is correct. Thanks for reviewing!
rdar://problem/36408004 (because comment 22 had a typo, targeting the wrong radar number)
Committed r226807: <https://trac.webkit.org/changeset/226807/webkit>.