Commit 61c84ff3 authored by ccameron's avatar ccameron Committed by Commit Bot

Reland "RemoteMacViews: Fix hang at command-Q"

This reverts commit 2e1e507d.

Reason for revert: Flakey test has been disabled. The behavior
that the test is testing is being deprecated in Chrome 73.

Original change's description:
> Revert "RemoteMacViews: Fix hang at command-Q"
> 
> This reverts commit 464fae69.
> 
> Reason for revert: Causes heavy flakiness (14/17 on Mac 10.11) and attempted fix showed no effect: https://crrev.com/c/1369136
> 
> TBR=ccameron@chromium.org
> Bug: 902583
> 
> Original change's description:
> > RemoteMacViews: Fix hang at command-Q
> > 
> > When we call -[NSApp terminate] in the app shim process, this does
> > not immediately terminate. Rather, it
> >  0. Sends the message appShimController_->host()->QuitApp() and waits
> >     for a reply before actually terminating.
> >  1. In the browser, this call hits ExtensionAppShimHandler::
> >     CloseBrowsersForApp
> >  2. Which will end up calling NativeWidgetMac::Close
> >  3. Which will then politely request that the app shim process close
> >     the corresponding window via
> >     views_bridge_mac::mojom::BridgedNativeWidget::CloseWindow
> >  4. Which will send an ack back to the browser via
> >     views_bridge_mac::mojom::BridgedNativeWidgetHost::OnWindowHasClosed
> >  5. Whereupon the browser window will be registered as closed, and a
> >     close message will be sent to the app shim, and the wait for
> >     termination will break
> > 
> > This ends up not working at step 3. Our suspended terminate ends up
> > running a nested message loop inside a mojo message handler, and so
> > it will never execute the CloseWindow message, and therefore never
> > close.
> > 
> > Change the logic at [0] to send the QuitApp message, and then
> > immediately terminate the application.
> > 
> > Bug: 902583
> > Change-Id: I60fd7b5fdf9f145369d06219e8ebef36cdcd0f44
> > Reviewed-on: https://chromium-review.googlesource.com/c/1364220
> > Reviewed-by: Robert Sesek <rsesek@chromium.org>
> > Commit-Queue: ccameron <ccameron@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#614922}
> 
> TBR=ccameron@chromium.org,rsesek@chromium.org
> 
> # Not skipping CQ checks because original CL landed > 1 day ago.
> 
> Bug: 902583
> Change-Id: I34a3300dc188cdd0de58692042ce131a8639117f
> Reviewed-on: https://chromium-review.googlesource.com/c/1371475
> Reviewed-by: Friedrich Horschig [CET] <fhorschig@chromium.org>
> Commit-Queue: Friedrich Horschig [CET] <fhorschig@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#615485}

TBR=ccameron@chromium.org,rsesek@chromium.org,fhorschig@chromium.org

Change-Id: Iceda65f61ac8a63bfd53803f713f0472f90e0d47
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 902583
Reviewed-on: https://chromium-review.googlesource.com/c/1371939Reviewed-by: default avatarccameron <ccameron@chromium.org>
Commit-Queue: ccameron <ccameron@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615605}
parent 640f80fd
......@@ -201,7 +201,7 @@ void AppShimController::SetUserAttention(
}
void AppShimController::Close() {
[delegate_ terminateNow];
[NSApp terminate:nil];
}
bool AppShimController::SendFocusApp(apps::AppShimFocusType focus_type,
......
......@@ -18,8 +18,6 @@ class AppShimController;
: NSObject<NSApplicationDelegate, NSUserInterfaceValidations> {
@private
AppShimController* appShimController_; // Weak, initially NULL.
BOOL terminateNow_;
BOOL terminateRequested_;
std::vector<base::FilePath> filesToOpenAtStartup_;
}
......@@ -36,10 +34,6 @@ class AppShimController;
// Takes an array of NSString*.
- (void)openFiles:(NSArray*)filename;
// Terminate immediately. This is necessary as we override terminate: to send
// a QuitApp message.
- (void)terminateNow;
@end
#endif // CHROME_APP_SHIM_APP_SHIM_DELEGATE_H_
......@@ -67,13 +67,11 @@
- (NSApplicationTerminateReply)applicationShouldTerminate:
(NSApplication*)sender {
if (terminateNow_ || !appShimController_)
return NSTerminateNow;
appShimController_->host()->QuitApp();
// Wait for the channel to close before terminating.
terminateRequested_ = YES;
return NSTerminateLater;
// Send a last message to the host indicating that the host should close all
// associated browser windows.
if (appShimController_)
appShimController_->host()->QuitApp();
return NSTerminateNow;
}
- (void)applicationWillHide:(NSNotification*)notification {
......@@ -86,16 +84,6 @@
appShimController_->host()->SetAppHidden(false);
}
- (void)terminateNow {
if (terminateRequested_) {
[NSApp replyToApplicationShouldTerminate:NSTerminateNow];
return;
}
terminateNow_ = YES;
[NSApp terminate:nil];
}
- (BOOL)validateUserInterfaceItem:(id<NSValidatedUserInterfaceItem>)item {
return NO;
}
......
......@@ -1039,12 +1039,7 @@ bool BridgedNativeWidgetImpl::ShouldRunCustomAnimationFor(
}
bool BridgedNativeWidgetImpl::RedispatchKeyEvent(NSEvent* event) {
NSWindow* window = ns_window();
DCHECK([window.class conformsToProtocol:@protocol(CommandDispatchingWindow)]);
NSObject<CommandDispatchingWindow>* command_dispatching_window =
base::mac::ObjCCastStrict<NSObject<CommandDispatchingWindow>>(window);
return
[[command_dispatching_window commandDispatcher] redispatchKeyEvent:event];
return [[window_ commandDispatcher] redispatchKeyEvent:event];
}
NSWindow* BridgedNativeWidgetImpl::ns_window() {
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment