Opening hosted apps as windows doesn't work with chrome://flags/#create-app-windows-in-app |
||||
Issue descriptionChrome Version : 73.0.3635.0 OS Version: OS X 10.14.1 What steps will reproduce the problem? 1. Enable chrome://flags/#create-app-windows-in-app 2. Open chrome://apps 3. Choose a hosted app like the preinstalled Gmail, YouTube, or Drive 4. Right click on it, enable open as window 5. Click on the app to launch it What is the expected result? The app launches in a new window. What happens instead of that? The app doesn't launch and nothing happens. Chrome then refuses to quit properly when you try to - the dock icon keeps its little dot and you have to force quit to make it exit. I'm guessing that behaviour is because something in the shim to browser communication is hanging in this case.
,
Dec 10
Okay, I see how this happens. We speculatively create an AppShimHost (for issue 896917 ). This should be reasonably-easily-fix-able. We also probably want to tie this into requiring that apps be signed in issue 913362. Also, it would be AWESOME if we could actually get the "automatic" apps to use RemoteMacViews.
,
Dec 11
Digging into this a bit -- we end up trying to launch an app shim for these links in ExtensionAppShimHandler::Delegate::LaunchShim, but then we never find the shim over at: https://cs.chromium.org/chromium/src/chrome/browser/web_applications/components/web_app_shortcut_mac.mm?rcl=7d3def8575ecd2e5e2e7ab7f585961206007bd25&l=196 That's really no good -- we need to know a priori whether or not an app shim that can run exists. Or, we need to be much more aggressive about creating them. I think we should be much more aggressive about (re-)creating app shims. Check out the attached video for the behavior when I change [0] to bind UpdateAndLaunchShimOnFileThread, and no-op out the check at [1]. I think that's the scheme we should go for. Thoughts? [0] https://cs.chromium.org/chromium/src/chrome/browser/web_applications/components/web_app_shortcut_mac.mm?rcl=7d3def8575ecd2e5e2e7ab7f585961206007bd25&l=931 [1] https://cs.chromium.org/chromium/src/chrome/browser/web_applications/components/web_app_shortcut_mac.mm?rcl=7d3def8575ecd2e5e2e7ab7f585961206007bd25&l=728
,
Dec 11
So the core problem is that hosted apps don't have app shims, and so we try and launch an app shim that doesn't exist? And the video shows what happens if we actually create a physical shim on disk for hosted apps which don't have one?
,
Dec 11
I think this can be fixed with whichever is easiest of: - Creating an app shim for all hosted apps, or - Disabling the ability for hosted apps (that are not bookmark apps or PWAs) to be opened in a window. Note that since hosted apps are deprecated on macOS, and since we never launched the ability to open hosted apps in a window until now, this should be fine to do the latter. Keeping as a P1 now since the shim itself is hanging. But either of the above mitigations should be fine to close this one out. (The former probably needs to be done by ccameron or sdy; the latter can be done by alancutter@. Chris please let us know which you think is easier?)
,
Dec 11
> - Creating an app shim for all hosted apps, or I need to add some code in this direction to prevent the hang when a shim is unable to launch (if you check out task manager, we think that we have a window open for the zombie app). I'm going to try to talk to rsesek@ (security) and sdy@ about the process of updating and signing apps. It may be that we'll want to aggressively re-create app shims. My personal favorite option would be to - generate the app shim on the fly - add a manifest (icon and color) for gmail, youtube, etc, hosted apps - make those apps be created in a window by default But ... let's defer for the moment (we'll probably go with "disable opening those hosted apps in windows", unless we find ourselves with an abundance of time).
,
Dec 12
FWIW we should not change existing apps to open in window, they should keep opening in tab unless the user changes the option. Otherwise we are changing the behavior of existing things under people's noses, and some people will not like it.
,
Dec 13
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cd35b5d7f015f86ae1361323c3c1d9f5ec1e38f2 commit cd35b5d7f015f86ae1361323c3c1d9f5ec1e38f2 Author: Christopher Cameron <ccameron@chromium.org> Date: Thu Dec 13 18:48:41 2018 MacPWAs: Detect when app shim fails to launch Add a callback argument to web_app::MaybeLaunchShortcut which returns the base::Process that was spawned. Use this to close an AppShimHost if the shim process fails to launch. If we do not close the AppShimHost, then the corresponding browser windows will remain in a zombie state forever. Subsequent patches will be adding more arguments to web_app::MaybeLaunchShortcut to accommodate signing. Bug: 913394 Change-Id: I645ef1dfd620b67e4af89ced999912b1391f02ca Reviewed-on: https://chromium-review.googlesource.com/c/1373934 Reviewed-by: Dominick Ng <dominickn@chromium.org> Reviewed-by: ccameron <ccameron@chromium.org> Commit-Queue: ccameron <ccameron@chromium.org> Cr-Commit-Position: refs/heads/master@{#616372} [modify] https://crrev.com/cd35b5d7f015f86ae1361323c3c1d9f5ec1e38f2/chrome/browser/apps/app_shim/app_shim_host_mac.cc [modify] https://crrev.com/cd35b5d7f015f86ae1361323c3c1d9f5ec1e38f2/chrome/browser/apps/app_shim/app_shim_host_mac.h [modify] https://crrev.com/cd35b5d7f015f86ae1361323c3c1d9f5ec1e38f2/chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc [modify] https://crrev.com/cd35b5d7f015f86ae1361323c3c1d9f5ec1e38f2/chrome/browser/apps/app_shim/extension_app_shim_handler_mac.h [modify] https://crrev.com/cd35b5d7f015f86ae1361323c3c1d9f5ec1e38f2/chrome/browser/apps/app_shim/extension_app_shim_handler_mac_unittest.cc [modify] https://crrev.com/cd35b5d7f015f86ae1361323c3c1d9f5ec1e38f2/chrome/browser/web_applications/components/web_app_shortcut_mac.h [modify] https://crrev.com/cd35b5d7f015f86ae1361323c3c1d9f5ec1e38f2/chrome/browser/web_applications/components/web_app_shortcut_mac.mm
,
Dec 17
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/854f60331f12a91ec29c2d27a43e6a15a894b99c commit 854f60331f12a91ec29c2d27a43e6a15a894b99c Author: Christopher Cameron <ccameron@chromium.org> Date: Mon Dec 17 01:44:37 2018 Make ExtensionAppShimHandler's EnableExtension use a OnceCallback Copy the mock strategy for RunOnce callbacks from autofill_assistant. Bug: 913394 Change-Id: I8cf1aea196538efc32c6fc307f529fb356de8fcf Reviewed-on: https://chromium-review.googlesource.com/c/1379170 Commit-Queue: ccameron <ccameron@chromium.org> Reviewed-by: Dominick Ng <dominickn@chromium.org> Cr-Commit-Position: refs/heads/master@{#617033} [modify] https://crrev.com/854f60331f12a91ec29c2d27a43e6a15a894b99c/chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc [modify] https://crrev.com/854f60331f12a91ec29c2d27a43e6a15a894b99c/chrome/browser/apps/app_shim/extension_app_shim_handler_mac.h [modify] https://crrev.com/854f60331f12a91ec29c2d27a43e6a15a894b99c/chrome/browser/apps/app_shim/extension_app_shim_handler_mac_unittest.cc
,
Dec 18
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3901e198ba933f9b90cabb879f0f238dbd950d3a commit 3901e198ba933f9b90cabb879f0f238dbd950d3a Author: Christopher Cameron <ccameron@chromium.org> Date: Tue Dec 18 18:03:56 2018 MacPWAs: Don't create AppShimHost until profile and extension are loaded When an app shim attaches to the browser in ExtensionAppShimHandler:: OnShimProcessConnected - run OnProfileLoaded once the profile is loaded - run OnExtensionEnabled once the extension is enabled - create the AppShimHost only once the profile and extension are loaded and enabled - if we fail either of those steps, report the failure to the AppShimHostBootstrap, and create no AppShimHost. This is a cleanup from the way things were before in that we used to create the AppShimHost before the extension was loaded, and then communicate whether or not the extension was enabled successfully via the AppShimHost::OnAppLaunchComplete callback. This allows us to remove the AppShimHost::OnAppLaunchComplete method and its related state (has_sent_on_launch_complete_, etc), and makes reasoning about the state of a launch more straightforward. Bug: 913394 Change-Id: I60a96281ad34ddbeaa80939aea4e4aad2a3711bd Reviewed-on: https://chromium-review.googlesource.com/c/1378800 Reviewed-by: Dominick Ng <dominickn@chromium.org> Commit-Queue: ccameron <ccameron@chromium.org> Cr-Commit-Position: refs/heads/master@{#617558} [modify] https://crrev.com/3901e198ba933f9b90cabb879f0f238dbd950d3a/chrome/browser/apps/app_shim/app_shim_handler_mac.h [modify] https://crrev.com/3901e198ba933f9b90cabb879f0f238dbd950d3a/chrome/browser/apps/app_shim/app_shim_host_bootstrap_mac.cc [modify] https://crrev.com/3901e198ba933f9b90cabb879f0f238dbd950d3a/chrome/browser/apps/app_shim/app_shim_host_bootstrap_mac.h [modify] https://crrev.com/3901e198ba933f9b90cabb879f0f238dbd950d3a/chrome/browser/apps/app_shim/app_shim_host_mac.cc [modify] https://crrev.com/3901e198ba933f9b90cabb879f0f238dbd950d3a/chrome/browser/apps/app_shim/app_shim_host_mac.h [modify] https://crrev.com/3901e198ba933f9b90cabb879f0f238dbd950d3a/chrome/browser/apps/app_shim/app_shim_host_mac_unittest.cc [modify] https://crrev.com/3901e198ba933f9b90cabb879f0f238dbd950d3a/chrome/browser/apps/app_shim/app_shim_host_manager_browsertest_mac.mm [modify] https://crrev.com/3901e198ba933f9b90cabb879f0f238dbd950d3a/chrome/browser/apps/app_shim/apps_page_shim_handler.mm [modify] https://crrev.com/3901e198ba933f9b90cabb879f0f238dbd950d3a/chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc [modify] https://crrev.com/3901e198ba933f9b90cabb879f0f238dbd950d3a/chrome/browser/apps/app_shim/extension_app_shim_handler_mac.h [modify] https://crrev.com/3901e198ba933f9b90cabb879f0f238dbd950d3a/chrome/browser/apps/app_shim/extension_app_shim_handler_mac_unittest.cc
,
Dec 19
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a7e6f10e9210c8e79c40ce97ce0bfd0d8471ba6b commit a7e6f10e9210c8e79c40ce97ce0bfd0d8471ba6b Author: Christopher Cameron <ccameron@chromium.org> Date: Wed Dec 19 18:14:01 2018 MacPWAs: Use LaunchServices to find app bundle When launching an app, search for a matching app by bundle id, rather than using only the default install path. This makes us robust to the user moving or renaming our application. Update GetAppBundlesById to use the non-depreacted LaunchServices APIs when available, and return all found locations, preferring those in the ~/Applications/Chrome Apps/ path. Follow-on patches will incorporate these changes into UpdateShortcuts, allow creating of a meaningful .app name, and allow rebuilding of .apps when needed (e.g, when their signature is out of date). Bug: 913394 Change-Id: Ia30057a71f7aa42ae4260297cfe5bb4cf998b195 Reviewed-on: https://chromium-review.googlesource.com/c/1381235 Reviewed-by: Sidney San MartÃn <sdy@chromium.org> Reviewed-by: Dominick Ng <dominickn@chromium.org> Commit-Queue: ccameron <ccameron@chromium.org> Cr-Commit-Position: refs/heads/master@{#617880} [modify] https://crrev.com/a7e6f10e9210c8e79c40ce97ce0bfd0d8471ba6b/chrome/browser/web_applications/components/web_app_shortcut_mac.h [modify] https://crrev.com/a7e6f10e9210c8e79c40ce97ce0bfd0d8471ba6b/chrome/browser/web_applications/components/web_app_shortcut_mac.mm [modify] https://crrev.com/a7e6f10e9210c8e79c40ce97ce0bfd0d8471ba6b/chrome/browser/web_applications/components/web_app_shortcut_mac_unittest.mm
,
Dec 19
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d756b20e41082abfa4618d69f0404aa453c3511e commit d756b20e41082abfa4618d69f0404aa453c3511e Author: Christopher Cameron <ccameron@chromium.org> Date: Wed Dec 19 20:40:06 2018 MacPWAs: Update app shims where they are found Prior to this change, UpdateShims (and friends) would create a new AppShim in the path of the pre-existing AppShim. If the old AppShim had been renamed, this would result in two AppShims existing (one with the default name and one with the true name). This patch ensure that UpdateShims overwrites the AppShim that it finds. To do this, CreateShortcutsIn is renamed to CreateShortcutsAt, and it takes the actual AppShim.app paths, not the parent directories. Bug: 913394 Change-Id: Ifd3d7e6ac060fb142b6b122b4ee6b0f6b43bb8c5 Reviewed-on: https://chromium-review.googlesource.com/c/1381515 Commit-Queue: ccameron <ccameron@chromium.org> Reviewed-by: Dominick Ng <dominickn@chromium.org> Cr-Commit-Position: refs/heads/master@{#617939} [modify] https://crrev.com/d756b20e41082abfa4618d69f0404aa453c3511e/chrome/browser/web_applications/components/web_app_shortcut_mac.h [modify] https://crrev.com/d756b20e41082abfa4618d69f0404aa453c3511e/chrome/browser/web_applications/components/web_app_shortcut_mac.mm
,
Dec 20
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0c6ce182695fb785b9191fef5195eca9bec7be5f commit 0c6ce182695fb785b9191fef5195eca9bec7be5f Author: Christopher Cameron <ccameron@chromium.org> Date: Thu Dec 20 01:55:58 2018 MacPWAs: Merge shim launch and update paths Merge web_app::MaybeLaunchShortcut and web_app::UpdateAndLaunchShim to a single function that specifies the update behavior and takes a callback to be made on completion. In the merged path, make a few adjustments - only launch a shim that was updated - add a path to force recreation of shortcuts if needed Add additional tests for new behaviors. Bug: 913394 Change-Id: I1626747d57b98f0f501af30307f6664804fab9f6 Reviewed-on: https://chromium-review.googlesource.com/c/1383396 Commit-Queue: ccameron <ccameron@chromium.org> Reviewed-by: Dominick Ng <dominickn@chromium.org> Cr-Commit-Position: refs/heads/master@{#618074} [modify] https://crrev.com/0c6ce182695fb785b9191fef5195eca9bec7be5f/chrome/browser/apps/app_shim/app_shim_interactive_uitest_mac.mm [modify] https://crrev.com/0c6ce182695fb785b9191fef5195eca9bec7be5f/chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc [modify] https://crrev.com/0c6ce182695fb785b9191fef5195eca9bec7be5f/chrome/browser/web_applications/components/web_app_shortcut_mac.h [modify] https://crrev.com/0c6ce182695fb785b9191fef5195eca9bec7be5f/chrome/browser/web_applications/components/web_app_shortcut_mac.mm [modify] https://crrev.com/0c6ce182695fb785b9191fef5195eca9bec7be5f/chrome/browser/web_applications/components/web_app_shortcut_mac_unittest.mm [modify] https://crrev.com/0c6ce182695fb785b9191fef5195eca9bec7be5f/chrome/browser/web_applications/extensions/web_app_extension_shortcut_mac.mm
,
Dec 21
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2e3ac4f050e776b9ccc25fb8ad71481a50e22592 commit 2e3ac4f050e776b9ccc25fb8ad71481a50e22592 Author: Christopher Cameron <ccameron@chromium.org> Date: Fri Dec 21 06:56:50 2018 MacPWAs: Regenerate app shims as needed Move some functionality from ExtensionAppShimHandler to AppShimHost: - AppShimHost::LaunchShim is now called to initiate a launch. It calls out into the handler to actually do the launch (if needed) - AppShimHost::OnShimLaunchCompleted, which reports back the base::Process that was launched. To these functions, add an argument specifying if the app shim bundles should be recreated. Add the behavior that a Chrome-initiated launch follows the following pattern: - Launch the shim process - If the launch fails, then recreate the app shims and launch again - If that fails, then give up and close all pending windows. Bug: 913394 Change-Id: Ice28ea1af0642dbaa85c7aeeb833db0ad66b425d Reviewed-on: https://chromium-review.googlesource.com/c/1385447 Commit-Queue: ccameron <ccameron@chromium.org> Reviewed-by: Dominick Ng <dominickn@chromium.org> Cr-Commit-Position: refs/heads/master@{#618473} [modify] https://crrev.com/2e3ac4f050e776b9ccc25fb8ad71481a50e22592/chrome/browser/apps/app_shim/app_shim_handler_mac.cc [modify] https://crrev.com/2e3ac4f050e776b9ccc25fb8ad71481a50e22592/chrome/browser/apps/app_shim/app_shim_handler_mac.h [modify] https://crrev.com/2e3ac4f050e776b9ccc25fb8ad71481a50e22592/chrome/browser/apps/app_shim/app_shim_host_mac.cc [modify] https://crrev.com/2e3ac4f050e776b9ccc25fb8ad71481a50e22592/chrome/browser/apps/app_shim/app_shim_host_mac.h [modify] https://crrev.com/2e3ac4f050e776b9ccc25fb8ad71481a50e22592/chrome/browser/apps/app_shim/app_shim_host_mac_unittest.cc [modify] https://crrev.com/2e3ac4f050e776b9ccc25fb8ad71481a50e22592/chrome/browser/apps/app_shim/app_shim_host_manager_browsertest_mac.mm [modify] https://crrev.com/2e3ac4f050e776b9ccc25fb8ad71481a50e22592/chrome/browser/apps/app_shim/app_shim_interactive_uitest_mac.mm [modify] https://crrev.com/2e3ac4f050e776b9ccc25fb8ad71481a50e22592/chrome/browser/apps/app_shim/apps_page_shim_handler.h [modify] https://crrev.com/2e3ac4f050e776b9ccc25fb8ad71481a50e22592/chrome/browser/apps/app_shim/apps_page_shim_handler.mm [modify] https://crrev.com/2e3ac4f050e776b9ccc25fb8ad71481a50e22592/chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc [modify] https://crrev.com/2e3ac4f050e776b9ccc25fb8ad71481a50e22592/chrome/browser/apps/app_shim/extension_app_shim_handler_mac.h [modify] https://crrev.com/2e3ac4f050e776b9ccc25fb8ad71481a50e22592/chrome/browser/apps/app_shim/extension_app_shim_handler_mac_unittest.cc
,
Dec 21
This should be fixed now. Behavior matches the video in #3, except with the right icons. |
||||
►
Sign in to add a comment |
||||
Comment 1 by susan.boorgula@chromium.org
, Dec 10