RemoteMacViews: App shim not used when launching apps from chrome://apps or "Open in ...", |
||||||
Issue description1. Install Killer Marmot (https://killer-marmot.appspot.com/web/) 2. Ensure that Killer Marmot isn't running 3 (version A). Go to chrome://apps and click on Killer Marmot 3 (verison B). Go to https://killer-marmot.appspot.com/web/, click on the 3-dot menu, and click "open in Killer Marmot" In these cases, launching the PWA app shim will race with creating the PWA app window. When the window is created and checks for its host at [0], it will find that the host app doesn't exist, and will just use the browser for its windows. We should either A: ensure that the app shim process has completed launching before creating windows for it. B: force migrating of browser windows when the app shim process launches I strongly favor [A], but I'm not familiar with all of the details.
,
Oct 19
~1 second or so, so it's not a long wait.
,
Oct 19
OK then, I think that's fine. Is it faster when warm? (If you've already started the app once for this Chrome session?)
,
Oct 23
This may be solve-able via some appropriate mojo-ification. In particular, you can send mojo messages to a not-yet-established connection. I'll see if creating a "AppShimHostPrecursor" or something like that works.
,
Oct 24
The fix for this is a bit involved, but I believe it's a good change for us to get ahead of.
At present the init sequence can be
A1. the user tries to launch an app by clicking in chrome://apps
A2. we launch the app in ExtensionAppShimHandler::Delegate::LaunchShim
A3. we exist in limbo where the AppShimHost "will hopefully exist soon", but may not
A4. when the app launches, it causes an AppShimHost to be created
The state of [3] is a bit strange -- an AppShimHost may be created eventually ... or it may fail.
I'd like it to be the case that
B1. the user tries to launch an app by clicking in chrome://apps
B2. we create an AppShimHost for the (profile,app_id) combo
- this AppShimHost has all of the relevant mojo interfaces already existing
- you can send (enqueue) messages to them already (this is a mojo feature)
B3. when the app launches, it specifies its (profile,app_id) combo to chrome
- this attaches to the existing AppShimHost for that combo
- all of the relevant interfaces are then bound
- and then enqueued messages are automatically sent
In this scheme, the AppShimHost can also be notified if its app shim has failed to launch (either we failed to launch the .app, or we did launch the .app but it timed out somehow). In that case, it can shut down the AppShimHost and put up some UI saying "hey, launching this app failed".
,
Oct 24
The proposal #5 seems like a fairly sensible change to me, especially since Mojo is designed that you can create one end of a Mojo pipe and start sending messages prior to the other end being connected.
,
Oct 24
Thanks! For some future reference ... The top of the stack is Browser::Browser CreateApplicationWindow Then when we create the app shim we're in apps::ExtensionAppShimHandler::Delegate::LaunchShim apps::ExtensionAppShimHandler::Observe content::NotificationServiceImpl::Notify BrowserList::AddBrowser But that happens *after* we have created the browser window ... which had the stack apps::ExtensionAppShimHandler::FindHostForBrowser BrowserFrameMac::GetBridgeFactoryHost views::NativeWidgetMac::InitNativeWidget BrowserFrameMac::InitNativeWidget views::Widget::Init BrowserFrame::InitBrowserFrame BrowserWindow::CreateBrowserWindow So I'm going to have to change ExtensionAppShimHandler::FindHostForBrowser to be ExtensionAppShimHandler::FindOrCreateHostForBrowser, which will create the host interface as needed.
,
Oct 25
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4643b7ab07dfe224d9e9d10890a9c3b86f70d880 commit 4643b7ab07dfe224d9e9d10890a9c3b86f70d880 Author: Christopher Cameron <ccameron@chromium.org> Date: Thu Oct 25 20:30:32 2018 AppShim: Separate out bootstrap interface from main interface This is an incremental step towards allowing an AppShimHost to be created before the app shim process has finished launching (to fix the bug where we can't create windows in the right process when launching from chrome://apps). Move the initialization methods from AppShim and AppShimHost into a separate "bootstrap" interface which is created from a mojo::PlatformChannelEndpoint. This "bootstrap" interface is created only once the app shim process has launched and connected to chrome (and vice-verse in the app shim process). Leave the remaining interfaces in the AppShim and AppShimHost methods. Immediately create a mojo::InterfaceRequest and mojo::InterfacePtr for these interfaces. Methods may immediately be enqueued against the mojo::InterfacePtr. Once the "bootstrap" connection is established, the mojo::InterfaceRequest is passed to the other process, and all queued messages are executed. Split the interfaces here, but do not separate out AppShimHost into two structures yet. That will come in a follow-on patch. Bug: 896917 Change-Id: I3272d72b58b9311a845ae281a91b23e87041cb63 Reviewed-on: https://chromium-review.googlesource.com/c/1297541 Commit-Queue: ccameron <ccameron@chromium.org> Reviewed-by: Dominick Ng <dominickn@chromium.org> Cr-Commit-Position: refs/heads/master@{#602836} [modify] https://crrev.com/4643b7ab07dfe224d9e9d10890a9c3b86f70d880/chrome/app_shim/app_shim_controller.h [modify] https://crrev.com/4643b7ab07dfe224d9e9d10890a9c3b86f70d880/chrome/app_shim/app_shim_controller.mm [modify] https://crrev.com/4643b7ab07dfe224d9e9d10890a9c3b86f70d880/chrome/app_shim/chrome_main_app_mode_mac.mm [modify] https://crrev.com/4643b7ab07dfe224d9e9d10890a9c3b86f70d880/chrome/browser/apps/app_shim/app_shim_host_mac.cc [modify] https://crrev.com/4643b7ab07dfe224d9e9d10890a9c3b86f70d880/chrome/browser/apps/app_shim/app_shim_host_mac.h [modify] https://crrev.com/4643b7ab07dfe224d9e9d10890a9c3b86f70d880/chrome/browser/apps/app_shim/app_shim_host_mac_unittest.cc [modify] https://crrev.com/4643b7ab07dfe224d9e9d10890a9c3b86f70d880/chrome/browser/apps/app_shim/app_shim_host_manager_browsertest_mac.mm [modify] https://crrev.com/4643b7ab07dfe224d9e9d10890a9c3b86f70d880/chrome/common/mac/app_shim.mojom
,
Oct 26
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/73c1fd6e4743ebd12b02fdc9d71bf3bd26b37e1b commit 73c1fd6e4743ebd12b02fdc9d71bf3bd26b37e1b Author: Christopher Cameron <ccameron@chromium.org> Date: Fri Oct 26 22:55:38 2018 Mac PWA launch: Separate AppShimHostBootstrap form AppShimHost This splits AppShimHost into two separate structures, along its mojo interface lines. The AppShimHostBootstrap class is created as soon as a connection is created with the app shim process, and awaits the LaunchApp mojo call, through which the AppShimHostBootstrap will indicate its profile and app id. The AppShimHost class is created only after the LaunchApp call is made, and takes ownership of the AppShimHostBootstrap. The next patch in this sequence will allow the AppShimHost to be created independently AppShimHostBootstrap, to support the situation where Chrome launches the app shim. Bug: 896917 Change-Id: I0d240a9ded78fcea3a0a6e26a2bad466402d08fb Reviewed-on: https://chromium-review.googlesource.com/c/1299668 Reviewed-by: Dominick Ng <dominickn@chromium.org> Commit-Queue: ccameron <ccameron@chromium.org> Cr-Commit-Position: refs/heads/master@{#603238} [modify] https://crrev.com/73c1fd6e4743ebd12b02fdc9d71bf3bd26b37e1b/chrome/browser/apps/app_shim/BUILD.gn [add] https://crrev.com/73c1fd6e4743ebd12b02fdc9d71bf3bd26b37e1b/chrome/browser/apps/app_shim/app_shim_host_bootstrap_mac.cc [add] https://crrev.com/73c1fd6e4743ebd12b02fdc9d71bf3bd26b37e1b/chrome/browser/apps/app_shim/app_shim_host_bootstrap_mac.h [modify] https://crrev.com/73c1fd6e4743ebd12b02fdc9d71bf3bd26b37e1b/chrome/browser/apps/app_shim/app_shim_host_mac.cc [modify] https://crrev.com/73c1fd6e4743ebd12b02fdc9d71bf3bd26b37e1b/chrome/browser/apps/app_shim/app_shim_host_mac.h [modify] https://crrev.com/73c1fd6e4743ebd12b02fdc9d71bf3bd26b37e1b/chrome/browser/apps/app_shim/app_shim_host_mac_unittest.cc [modify] https://crrev.com/73c1fd6e4743ebd12b02fdc9d71bf3bd26b37e1b/chrome/browser/apps/app_shim/app_shim_host_manager_mac.mm [modify] https://crrev.com/73c1fd6e4743ebd12b02fdc9d71bf3bd26b37e1b/chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc
,
Oct 27
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/03a84df88a57d6b836ec35f46710e86f7c8a42ef commit 03a84df88a57d6b836ec35f46710e86f7c8a42ef Author: Christopher Cameron <ccameron@chromium.org> Date: Sat Oct 27 09:04:48 2018 RemoteMacViews: Fix host id generation This was supposed to be a global, not a local. TBR=dominickn Bug: 896917 Change-Id: I4b242f3db5700a9845690a45c1722b643649e741 Reviewed-on: https://chromium-review.googlesource.com/c/1303678 Commit-Queue: ccameron <ccameron@chromium.org> Reviewed-by: ccameron <ccameron@chromium.org> Cr-Commit-Position: refs/heads/master@{#603324} [modify] https://crrev.com/03a84df88a57d6b836ec35f46710e86f7c8a42ef/chrome/browser/apps/app_shim/app_shim_host_mac.cc
,
Oct 30
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/199a60445abaae76f0ba7ae3bc659674031643ba commit 199a60445abaae76f0ba7ae3bc659674031643ba Author: Christopher Cameron <ccameron@chromium.org> Date: Tue Oct 30 07:29:23 2018 MacPWAs: Decouple AppShimHost and AppShimHostBootstrap Update AppShimHandler::OnShimLaunch to take a AppShimHostBootstrap, and use that to create an AppShimHost, rather than having the caller create the AppShimHost. * Have the method for AppShimHost creation be in the AppShimHandler::Delegate class for overriding in tests. * This sets us up to have the AppShimHost be created before the app shim process has completed launching (and thereby be able to queue up creating windows in the app shim process). Add to AppShimHandler::Host's public interface an OnBootstrapConnected function, where the AppShimHandler::Host takes ownership of the app shim process. * This is currently always called immediately after the AppShimHost's creation, but that will change in the next patch. Make the tests rely more heavily on the real classes instead of fake stubs. In particular, I plan to remove AppShimHandler::Host as an abstract interface because the tests cover much more ground when run on the real test (and the one real implementation, AppShimHost, has no other public methods). Make one subtle change in the tests. In the event of a duplicate app launch, the app's windows will be focused by the existing AppShimHost, because we don't create an AppShimHost in the event of any failure. This has the same user-visible behavior (because the windows being focused are the same). Bug: 896917 Change-Id: Ideea05d00edc1bf4c11825e70ad44229b1174724 Reviewed-on: https://chromium-review.googlesource.com/c/1305889 Commit-Queue: ccameron <ccameron@chromium.org> Reviewed-by: Dominick Ng <dominickn@chromium.org> Cr-Commit-Position: refs/heads/master@{#603821} [modify] https://crrev.com/199a60445abaae76f0ba7ae3bc659674031643ba/chrome/browser/apps/app_shim/app_shim_handler_mac.h [modify] https://crrev.com/199a60445abaae76f0ba7ae3bc659674031643ba/chrome/browser/apps/app_shim/app_shim_host_bootstrap_mac.cc [modify] https://crrev.com/199a60445abaae76f0ba7ae3bc659674031643ba/chrome/browser/apps/app_shim/app_shim_host_bootstrap_mac.h [modify] https://crrev.com/199a60445abaae76f0ba7ae3bc659674031643ba/chrome/browser/apps/app_shim/app_shim_host_mac.cc [modify] https://crrev.com/199a60445abaae76f0ba7ae3bc659674031643ba/chrome/browser/apps/app_shim/app_shim_host_mac.h [modify] https://crrev.com/199a60445abaae76f0ba7ae3bc659674031643ba/chrome/browser/apps/app_shim/app_shim_host_mac_unittest.cc [modify] https://crrev.com/199a60445abaae76f0ba7ae3bc659674031643ba/chrome/browser/apps/app_shim/app_shim_host_manager_browsertest_mac.mm [modify] https://crrev.com/199a60445abaae76f0ba7ae3bc659674031643ba/chrome/browser/apps/app_shim/app_shim_interactive_uitest_mac.mm [modify] https://crrev.com/199a60445abaae76f0ba7ae3bc659674031643ba/chrome/browser/apps/app_shim/app_shim_quit_interactive_uitest_mac.mm [modify] https://crrev.com/199a60445abaae76f0ba7ae3bc659674031643ba/chrome/browser/apps/app_shim/apps_page_shim_handler.h [modify] https://crrev.com/199a60445abaae76f0ba7ae3bc659674031643ba/chrome/browser/apps/app_shim/apps_page_shim_handler.mm [modify] https://crrev.com/199a60445abaae76f0ba7ae3bc659674031643ba/chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc [modify] https://crrev.com/199a60445abaae76f0ba7ae3bc659674031643ba/chrome/browser/apps/app_shim/extension_app_shim_handler_mac.h [modify] https://crrev.com/199a60445abaae76f0ba7ae3bc659674031643ba/chrome/browser/apps/app_shim/extension_app_shim_handler_mac_unittest.cc [modify] https://crrev.com/199a60445abaae76f0ba7ae3bc659674031643ba/chrome/browser/ui/cocoa/apps/native_app_window_cocoa_browsertest.mm
,
Oct 30
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9b566135967a479c2f54e20278714199f2b4e677 commit 9b566135967a479c2f54e20278714199f2b4e677 Author: Owen Min <zmin@chromium.org> Date: Tue Oct 30 19:44:07 2018 Revert "MacPWAs: Decouple AppShimHost and AppShimHostBootstrap" This reverts commit 199a60445abaae76f0ba7ae3bc659674031643ba. Reason for revert: NativeAppWindowCocoaBrowserTest.HideShowWithAppWithShim failed on Mac10.12 https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Mac10.12%20Tests/16328 Original change's description: > MacPWAs: Decouple AppShimHost and AppShimHostBootstrap > > Update AppShimHandler::OnShimLaunch to take a AppShimHostBootstrap, > and use that to create an AppShimHost, rather than having the > caller create the AppShimHost. > * Have the method for AppShimHost creation be in the > AppShimHandler::Delegate class for overriding in tests. > * This sets us up to have the AppShimHost be created before the app shim > process has completed launching (and thereby be able to queue up > creating windows in the app shim process). > > Add to AppShimHandler::Host's public interface an OnBootstrapConnected > function, where the AppShimHandler::Host takes ownership of the app > shim process. > * This is currently always called immediately after the AppShimHost's > creation, but that will change in the next patch. > > Make the tests rely more heavily on the real classes instead of fake > stubs. In particular, I plan to remove AppShimHandler::Host as an > abstract interface because the tests cover much more ground when run on > the real test (and the one real implementation, AppShimHost, has no > other public methods). > > Make one subtle change in the tests. In the event of a duplicate app > launch, the app's windows will be focused by the existing AppShimHost, > because we don't create an AppShimHost in the event of any failure. This > has the same user-visible behavior (because the windows being focused > are the same). > > Bug: 896917 > Change-Id: Ideea05d00edc1bf4c11825e70ad44229b1174724 > Reviewed-on: https://chromium-review.googlesource.com/c/1305889 > Commit-Queue: ccameron <ccameron@chromium.org> > Reviewed-by: Dominick Ng <dominickn@chromium.org> > Cr-Commit-Position: refs/heads/master@{#603821} TBR=ccameron@chromium.org,dominickn@chromium.org Change-Id: I1e60835172d2eac0e402b4de142d16e1edf54dc9 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 896917 Reviewed-on: https://chromium-review.googlesource.com/c/1308664 Reviewed-by: Owen Min <zmin@chromium.org> Commit-Queue: Owen Min <zmin@chromium.org> Cr-Commit-Position: refs/heads/master@{#603988} [modify] https://crrev.com/9b566135967a479c2f54e20278714199f2b4e677/chrome/browser/apps/app_shim/app_shim_handler_mac.h [modify] https://crrev.com/9b566135967a479c2f54e20278714199f2b4e677/chrome/browser/apps/app_shim/app_shim_host_bootstrap_mac.cc [modify] https://crrev.com/9b566135967a479c2f54e20278714199f2b4e677/chrome/browser/apps/app_shim/app_shim_host_bootstrap_mac.h [modify] https://crrev.com/9b566135967a479c2f54e20278714199f2b4e677/chrome/browser/apps/app_shim/app_shim_host_mac.cc [modify] https://crrev.com/9b566135967a479c2f54e20278714199f2b4e677/chrome/browser/apps/app_shim/app_shim_host_mac.h [modify] https://crrev.com/9b566135967a479c2f54e20278714199f2b4e677/chrome/browser/apps/app_shim/app_shim_host_mac_unittest.cc [modify] https://crrev.com/9b566135967a479c2f54e20278714199f2b4e677/chrome/browser/apps/app_shim/app_shim_host_manager_browsertest_mac.mm [modify] https://crrev.com/9b566135967a479c2f54e20278714199f2b4e677/chrome/browser/apps/app_shim/app_shim_interactive_uitest_mac.mm [modify] https://crrev.com/9b566135967a479c2f54e20278714199f2b4e677/chrome/browser/apps/app_shim/app_shim_quit_interactive_uitest_mac.mm [modify] https://crrev.com/9b566135967a479c2f54e20278714199f2b4e677/chrome/browser/apps/app_shim/apps_page_shim_handler.h [modify] https://crrev.com/9b566135967a479c2f54e20278714199f2b4e677/chrome/browser/apps/app_shim/apps_page_shim_handler.mm [modify] https://crrev.com/9b566135967a479c2f54e20278714199f2b4e677/chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc [modify] https://crrev.com/9b566135967a479c2f54e20278714199f2b4e677/chrome/browser/apps/app_shim/extension_app_shim_handler_mac.h [modify] https://crrev.com/9b566135967a479c2f54e20278714199f2b4e677/chrome/browser/apps/app_shim/extension_app_shim_handler_mac_unittest.cc [modify] https://crrev.com/9b566135967a479c2f54e20278714199f2b4e677/chrome/browser/ui/cocoa/apps/native_app_window_cocoa_browsertest.mm
,
Oct 31
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f23e09f8c9cbec20c1ba139cea08aa98cb738e05 commit f23e09f8c9cbec20c1ba139cea08aa98cb738e05 Author: Christopher Cameron <ccameron@chromium.org> Date: Wed Oct 31 04:33:23 2018 MacPWAs: Decouple AppShimHost and AppShimHostBootstrap Update AppShimHandler::OnShimLaunch to take a AppShimHostBootstrap, and use that to create an AppShimHost, rather than having the caller create the AppShimHost. * Have the method for AppShimHost creation be in the AppShimHandler::Delegate class for overriding in tests. * This sets us up to have the AppShimHost be created before the app shim process has completed launching (and thereby be able to queue up creating windows in the app shim process). Add to AppShimHandler::Host's public interface an OnBootstrapConnected function, where the AppShimHandler::Host takes ownership of the app shim process. * This is currently always called immediately after the AppShimHost's creation, but that will change in the next patch. Make the tests rely more heavily on the real classes instead of fake stubs. In particular, I plan to remove AppShimHandler::Host as an abstract interface because the tests cover much more ground when run on the real test (and the one real implementation, AppShimHost, has no other public methods). Make one subtle change in the tests. In the event of a duplicate app launch, the app's windows will be focused by the existing AppShimHost, because we don't create an AppShimHost in the event of any failure. This has the same user-visible behavior (because the windows being focused are the same). Re-land of crrev.com/603821 TBR=dominickn Bug: 896917 Change-Id: I4efc493d048e898d86aca736244a8e5a5672ccef Reviewed-on: https://chromium-review.googlesource.com/c/1309222 Reviewed-by: ccameron <ccameron@chromium.org> Commit-Queue: ccameron <ccameron@chromium.org> Cr-Commit-Position: refs/heads/master@{#604153} [modify] https://crrev.com/f23e09f8c9cbec20c1ba139cea08aa98cb738e05/chrome/browser/apps/app_shim/app_shim_handler_mac.h [modify] https://crrev.com/f23e09f8c9cbec20c1ba139cea08aa98cb738e05/chrome/browser/apps/app_shim/app_shim_host_bootstrap_mac.cc [modify] https://crrev.com/f23e09f8c9cbec20c1ba139cea08aa98cb738e05/chrome/browser/apps/app_shim/app_shim_host_bootstrap_mac.h [modify] https://crrev.com/f23e09f8c9cbec20c1ba139cea08aa98cb738e05/chrome/browser/apps/app_shim/app_shim_host_mac.cc [modify] https://crrev.com/f23e09f8c9cbec20c1ba139cea08aa98cb738e05/chrome/browser/apps/app_shim/app_shim_host_mac.h [modify] https://crrev.com/f23e09f8c9cbec20c1ba139cea08aa98cb738e05/chrome/browser/apps/app_shim/app_shim_host_mac_unittest.cc [modify] https://crrev.com/f23e09f8c9cbec20c1ba139cea08aa98cb738e05/chrome/browser/apps/app_shim/app_shim_host_manager_browsertest_mac.mm [modify] https://crrev.com/f23e09f8c9cbec20c1ba139cea08aa98cb738e05/chrome/browser/apps/app_shim/app_shim_interactive_uitest_mac.mm [modify] https://crrev.com/f23e09f8c9cbec20c1ba139cea08aa98cb738e05/chrome/browser/apps/app_shim/app_shim_quit_interactive_uitest_mac.mm [modify] https://crrev.com/f23e09f8c9cbec20c1ba139cea08aa98cb738e05/chrome/browser/apps/app_shim/apps_page_shim_handler.h [modify] https://crrev.com/f23e09f8c9cbec20c1ba139cea08aa98cb738e05/chrome/browser/apps/app_shim/apps_page_shim_handler.mm [modify] https://crrev.com/f23e09f8c9cbec20c1ba139cea08aa98cb738e05/chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc [modify] https://crrev.com/f23e09f8c9cbec20c1ba139cea08aa98cb738e05/chrome/browser/apps/app_shim/extension_app_shim_handler_mac.h [modify] https://crrev.com/f23e09f8c9cbec20c1ba139cea08aa98cb738e05/chrome/browser/apps/app_shim/extension_app_shim_handler_mac_unittest.cc [modify] https://crrev.com/f23e09f8c9cbec20c1ba139cea08aa98cb738e05/chrome/browser/ui/cocoa/apps/native_app_window_cocoa_browsertest.mm
,
Oct 31
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ae0e8508a5031107bfeb8102b1c8a843516feaf6 commit ae0e8508a5031107bfeb8102b1c8a843516feaf6 Author: Christopher Cameron <ccameron@chromium.org> Date: Wed Oct 31 23:31:15 2018 MacPWAs: Allow creating of AppShimHost before process exists Allow for an AppShimHost to be created before the corresponding AppShimHostBootstrap. Update OnAppLaunchComplete to save the launch result and send it only once an AppShimHostBootstrap is attached. Change ExtensionAppShimHandler profile paths to use the full path (as is reported by the Profile object, as opposed to the shim's message). Change the method GetHostForBrowser to instead be GetViewsBridgeFactoryHostForBrowser, because the ViewsBridgeFactory is what all callers are after. Make this call trigger creation of an AppShimHost if needed to provide a ViewsBridgeFactory. Keep this guarded behind a flag for now. Add tests for this functionality. In the tests, further decouple the AppShimHostBootstrap from its AppShimHost. Bug: 896917 Change-Id: Ib9e4ad6535ca87cd0e01303fbfc772b8892aec12 Reviewed-on: https://chromium-review.googlesource.com/c/1309307 Reviewed-by: Dominick Ng <dominickn@chromium.org> Reviewed-by: ccameron <ccameron@chromium.org> Reviewed-by: Avi Drissman <avi@chromium.org> Commit-Queue: ccameron <ccameron@chromium.org> Cr-Commit-Position: refs/heads/master@{#604440} [modify] https://crrev.com/ae0e8508a5031107bfeb8102b1c8a843516feaf6/chrome/browser/apps/app_shim/app_shim_handler_mac.h [modify] https://crrev.com/ae0e8508a5031107bfeb8102b1c8a843516feaf6/chrome/browser/apps/app_shim/app_shim_host_mac.cc [modify] https://crrev.com/ae0e8508a5031107bfeb8102b1c8a843516feaf6/chrome/browser/apps/app_shim/app_shim_host_mac.h [modify] https://crrev.com/ae0e8508a5031107bfeb8102b1c8a843516feaf6/chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc [modify] https://crrev.com/ae0e8508a5031107bfeb8102b1c8a843516feaf6/chrome/browser/apps/app_shim/extension_app_shim_handler_mac.h [modify] https://crrev.com/ae0e8508a5031107bfeb8102b1c8a843516feaf6/chrome/browser/apps/app_shim/extension_app_shim_handler_mac_unittest.cc [modify] https://crrev.com/ae0e8508a5031107bfeb8102b1c8a843516feaf6/chrome/browser/ui/views/frame/browser_frame_mac.mm
,
Nov 2
This is working now. I unearthed a bunch of TODOs about ensuring that we don't wait forever for an app that fails to launch, which I should take care of as well.
,
Nov 6
,
Nov 7
,
Dec 1
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/07670151f372877db007bad2f15f2a3c59f00b04 commit 07670151f372877db007bad2f15f2a3c59f00b04 Author: Christopher Cameron <ccameron@chromium.org> Date: Sat Dec 01 18:41:24 2018 Remove AppShimHandler::Host interface and access AppShimHost directly This has been a longstanding TODO. Pending work will require adding more methods to route to AppShimHost, so just delete this rather than adding more methods to it. TBR=mgiuca Bug: 896917 Change-Id: Idd8f55c7b6f549354441e28832e292ce693b9363 Reviewed-on: https://chromium-review.googlesource.com/c/1357574 Reviewed-by: ccameron <ccameron@chromium.org> Commit-Queue: ccameron <ccameron@chromium.org> Cr-Commit-Position: refs/heads/master@{#612938} [modify] https://crrev.com/07670151f372877db007bad2f15f2a3c59f00b04/chrome/browser/apps/app_shim/app_shim_handler_mac.h [modify] https://crrev.com/07670151f372877db007bad2f15f2a3c59f00b04/chrome/browser/apps/app_shim/app_shim_host_mac.cc [modify] https://crrev.com/07670151f372877db007bad2f15f2a3c59f00b04/chrome/browser/apps/app_shim/app_shim_host_mac.h [modify] https://crrev.com/07670151f372877db007bad2f15f2a3c59f00b04/chrome/browser/apps/app_shim/app_shim_host_mac_unittest.cc [modify] https://crrev.com/07670151f372877db007bad2f15f2a3c59f00b04/chrome/browser/apps/app_shim/app_shim_host_manager_browsertest_mac.mm [modify] https://crrev.com/07670151f372877db007bad2f15f2a3c59f00b04/chrome/browser/apps/app_shim/app_shim_interactive_uitest_mac.mm [modify] https://crrev.com/07670151f372877db007bad2f15f2a3c59f00b04/chrome/browser/apps/app_shim/apps_page_shim_handler.h [modify] https://crrev.com/07670151f372877db007bad2f15f2a3c59f00b04/chrome/browser/apps/app_shim/apps_page_shim_handler.mm [modify] https://crrev.com/07670151f372877db007bad2f15f2a3c59f00b04/chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc [modify] https://crrev.com/07670151f372877db007bad2f15f2a3c59f00b04/chrome/browser/apps/app_shim/extension_app_shim_handler_mac.h [modify] https://crrev.com/07670151f372877db007bad2f15f2a3c59f00b04/chrome/browser/apps/app_shim/extension_app_shim_handler_mac_unittest.cc [modify] https://crrev.com/07670151f372877db007bad2f15f2a3c59f00b04/chrome/browser/ui/cocoa/apps/native_app_window_cocoa_browsertest.mm
,
Dec 3
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7ae9c3659c6a84cc44d26b68813df2dc54f55c16 commit 7ae9c3659c6a84cc44d26b68813df2dc54f55c16 Author: Guido Urdaneta <guidou@chromium.org> Date: Mon Dec 03 13:04:24 2018 Revert "Remove AppShimHandler::Host interface and access AppShimHost directly" This reverts commit 07670151f372877db007bad2f15f2a3c59f00b04. Reason for revert: Suspect of introducing consistent failure on Mac12.12 Tests bot. https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Mac10.12%20Tests First failure: https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Mac10.12%20Tests/17211 Sample logs [ RUN ] NativeAppWindowCocoaBrowserTest.HideShowWithAppWithShim [12598:61443:1201/114417.410054:WARNING:notification_platform_bridge_mac.mm(521)] AlertNotificationService: XPC connection invalidated. ../../chrome/browser/ui/cocoa/apps/native_app_window_cocoa_browsertest.mm:198: Failure Actual function call count doesn't match EXPECT_CALL(*mock_host, OnAppUnhideWithoutActivation())... Expected: to be called once Actual: never called - unsatisfied and active Stack trace: 0 browser_tests 0x000000010ca5edbb testing::internal::UnitTestImpl::CurrentOsStackTraceExceptTop(int) + 91 1 browser_tests 0x000000010ca5e779 testing::internal::AssertHelper::operator=(testing::Message const&) const + 89 2 browser_tests 0x000000010ca4dd61 testing::internal::GoogleTestFailureReporter::ReportFailure(testing::internal::FailureReporterInterface::FailureType, char const*, int, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) + 97 3 browser_tests 0x000000010ca52a28 testing::internal::UntypedFunctionMockerBase::VerifyAndClearExpectationsLocked() + 584 4 browser_tests 0x000000010ca53399 testing::Mock::VerifyAndClearExpectationsLocked(void*) + 393 5 browser_tests 0x000000010ca531f1 testing::Mock::VerifyAndClearExpectations(void*) + 33 6 browser_tests 0x000000010c75e4cd NativeAppWindowCocoaBrowserTest_HideShowWithAppWithShim_Test::RunTestOnMainThread() + 3117 7 browser_tests 0x000000010ff002bb content::BrowserTestBase::ProxyRunTestOnMainThreadLoop() + 491 8 browser_tests 0x000000010f944014 ChromeBrowserMainParts::PreMainMessageLoopRunImpl() + 4372 9 browser_tests 0x000000010f942e3d ChromeBrowserMainParts::PreMainMessageLoopRun() + 45 10 browser_tests 0x000000010daaedc2 content::BrowserMainLoop::PreMainMessageLoopRun() + 50 11 browser_tests 0x000000010df0f397 content::StartupTaskRunner::RunAllTasksNow() + 39 12 browser_tests 0x000000010daad9fb content::BrowserMainLoop::CreateStartupTasks() + 683 13 browser_tests 0x000000010dab1065 content::BrowserMainRunnerImpl::Initialize(content::MainFunctionParams const&) + 85 14 browser_tests 0x000000010daabde2 content::BrowserMain(content::MainFunctionParams const&) + 178 15 browser_tests 0x000000010f3cc601 content::ContentMainRunnerImpl::RunServiceManager(content::MainFunctionParams&, bool) + 241 16 browser_tests 0x000000010f3cc4e4 content::ContentMainRunnerImpl::Run(bool) + 292 17 browser_tests 0x00000001127318cb service_manager::Main(service_manager::MainParams const&) + 3051 18 browser_tests 0x000000010f3cb684 content::ContentMain(content::ContentMainParams const&) + 68 19 browser_tests 0x000000010fefff0d content::BrowserTestBase::SetUp() + 2829 20 browser_tests 0x000000010f8b75f1 InProcessBrowserTest::SetUp() + 529 Original change's description: > Remove AppShimHandler::Host interface and access AppShimHost directly > > This has been a longstanding TODO. Pending work will require adding > more methods to route to AppShimHost, so just delete this rather than > adding more methods to it. > > TBR=mgiuca > > Bug: 896917 > Change-Id: Idd8f55c7b6f549354441e28832e292ce693b9363 > Reviewed-on: https://chromium-review.googlesource.com/c/1357574 > Reviewed-by: ccameron <ccameron@chromium.org> > Commit-Queue: ccameron <ccameron@chromium.org> > Cr-Commit-Position: refs/heads/master@{#612938} TBR=ccameron@chromium.org,mgiuca@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 896917 Change-Id: I0e4314f3a0d9b1241ecdde333a4d02b327a06917 Reviewed-on: https://chromium-review.googlesource.com/c/1358431 Reviewed-by: Guido Urdaneta <guidou@chromium.org> Commit-Queue: Guido Urdaneta <guidou@chromium.org> Cr-Commit-Position: refs/heads/master@{#613064} [modify] https://crrev.com/7ae9c3659c6a84cc44d26b68813df2dc54f55c16/chrome/browser/apps/app_shim/app_shim_handler_mac.h [modify] https://crrev.com/7ae9c3659c6a84cc44d26b68813df2dc54f55c16/chrome/browser/apps/app_shim/app_shim_host_mac.cc [modify] https://crrev.com/7ae9c3659c6a84cc44d26b68813df2dc54f55c16/chrome/browser/apps/app_shim/app_shim_host_mac.h [modify] https://crrev.com/7ae9c3659c6a84cc44d26b68813df2dc54f55c16/chrome/browser/apps/app_shim/app_shim_host_mac_unittest.cc [modify] https://crrev.com/7ae9c3659c6a84cc44d26b68813df2dc54f55c16/chrome/browser/apps/app_shim/app_shim_host_manager_browsertest_mac.mm [modify] https://crrev.com/7ae9c3659c6a84cc44d26b68813df2dc54f55c16/chrome/browser/apps/app_shim/app_shim_interactive_uitest_mac.mm [modify] https://crrev.com/7ae9c3659c6a84cc44d26b68813df2dc54f55c16/chrome/browser/apps/app_shim/apps_page_shim_handler.h [modify] https://crrev.com/7ae9c3659c6a84cc44d26b68813df2dc54f55c16/chrome/browser/apps/app_shim/apps_page_shim_handler.mm [modify] https://crrev.com/7ae9c3659c6a84cc44d26b68813df2dc54f55c16/chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc [modify] https://crrev.com/7ae9c3659c6a84cc44d26b68813df2dc54f55c16/chrome/browser/apps/app_shim/extension_app_shim_handler_mac.h [modify] https://crrev.com/7ae9c3659c6a84cc44d26b68813df2dc54f55c16/chrome/browser/apps/app_shim/extension_app_shim_handler_mac_unittest.cc [modify] https://crrev.com/7ae9c3659c6a84cc44d26b68813df2dc54f55c16/chrome/browser/ui/cocoa/apps/native_app_window_cocoa_browsertest.mm
,
Dec 4
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/eb1c4bfd50b2b4d076be1b4c8a84e9d4eb781dd6 commit eb1c4bfd50b2b4d076be1b4c8a84e9d4eb781dd6 Author: Christopher Cameron <ccameron@chromium.org> Date: Tue Dec 04 18:10:20 2018 Remove AppShimHandler::Host interface and access AppShimHost directly This has been a longstanding TODO. Pending work will require adding more methods to route to AppShimHost, so just delete this rather than adding more methods to it. TBR=mgiuca (reland of crrev.com/612938) Bug: 896917 Change-Id: Ia04efcc0e9b9d6adbcd1fecb63db6ef4f3723721 Reviewed-on: https://chromium-review.googlesource.com/c/1359724 Reviewed-by: ccameron <ccameron@chromium.org> Commit-Queue: ccameron <ccameron@chromium.org> Cr-Commit-Position: refs/heads/master@{#613599} [modify] https://crrev.com/eb1c4bfd50b2b4d076be1b4c8a84e9d4eb781dd6/chrome/browser/apps/app_shim/app_shim_handler_mac.h [modify] https://crrev.com/eb1c4bfd50b2b4d076be1b4c8a84e9d4eb781dd6/chrome/browser/apps/app_shim/app_shim_host_mac.cc [modify] https://crrev.com/eb1c4bfd50b2b4d076be1b4c8a84e9d4eb781dd6/chrome/browser/apps/app_shim/app_shim_host_mac.h [modify] https://crrev.com/eb1c4bfd50b2b4d076be1b4c8a84e9d4eb781dd6/chrome/browser/apps/app_shim/app_shim_host_mac_unittest.cc [modify] https://crrev.com/eb1c4bfd50b2b4d076be1b4c8a84e9d4eb781dd6/chrome/browser/apps/app_shim/app_shim_host_manager_browsertest_mac.mm [modify] https://crrev.com/eb1c4bfd50b2b4d076be1b4c8a84e9d4eb781dd6/chrome/browser/apps/app_shim/app_shim_interactive_uitest_mac.mm [modify] https://crrev.com/eb1c4bfd50b2b4d076be1b4c8a84e9d4eb781dd6/chrome/browser/apps/app_shim/apps_page_shim_handler.h [modify] https://crrev.com/eb1c4bfd50b2b4d076be1b4c8a84e9d4eb781dd6/chrome/browser/apps/app_shim/apps_page_shim_handler.mm [modify] https://crrev.com/eb1c4bfd50b2b4d076be1b4c8a84e9d4eb781dd6/chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc [modify] https://crrev.com/eb1c4bfd50b2b4d076be1b4c8a84e9d4eb781dd6/chrome/browser/apps/app_shim/extension_app_shim_handler_mac.h [modify] https://crrev.com/eb1c4bfd50b2b4d076be1b4c8a84e9d4eb781dd6/chrome/browser/apps/app_shim/extension_app_shim_handler_mac_unittest.cc [modify] https://crrev.com/eb1c4bfd50b2b4d076be1b4c8a84e9d4eb781dd6/chrome/browser/ui/cocoa/apps/native_app_window_cocoa_browsertest.mm
,
Dec 5
|
||||||
►
Sign in to add a comment |
||||||
Comment 1 by mgiuca@chromium.org
, Oct 18