ChromeShellDelegate::OpenUrlFromArc needs to be replaced with a mojo API call |
||||
Issue descriptionCurrently we call ChromeShellDelegate::OpenUrlFromArc from two places: * arc_intent_helper_bridge.cc * AssistantController::OnOpenUrlResponse() The latter is presumably done for expediency, but we currently set Arc context data for the tab, which is probably harmless? We also open chrome URLs via SystemTrayClient::Show* via mojo, which does much the same thing for a handful of chrome URLs. OpenUrlFromArc has special case handling for chrome://settings (which seems a little fragile). We should consider implementing a general purpose mojo handler for opening URLs in a Chrome tab or window, and handle these requests in a constant manner.
,
May 23 2018
If you go the generic URL opening route be sure to have someone from security look at it. I could imagine us getting into trouble if we allow javascript:<foo> URLs, maybe some internal urls, etc.
,
May 25 2018
,
Jun 7 2018
I'll have a look. I see we have ash::mojom::NewWindowClient::NewTabWithUrl already, so perhaps I can extend NewWindowClient.
,
Jun 11 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/84611ddaa77666930a0c24d94d312488a5abef47 commit 84611ddaa77666930a0c24d94d312488a5abef47 Author: James Cook <jamescook@chromium.org> Date: Mon Jun 11 19:01:40 2018 chromeos: Support OpenUrlFromArc in out-of-process ash When ash runs outside the browser process we can't use in-process delegates like ash::ShellDelegate / ChromeShellDelegate. We have to use mojo IPC, like ChromeNewWindowClient. Migrate ChromeShellDelegate::OpenUrlFromArc to ChromeNewWindowClient and consolidate it with the similar function NewTabWithUrl. This means we use similar URL opening code for ARC, assistant and crostini. Use NewTabWithUrl for assistant, because it doesn't need the special ARC handling. Fix two potential issues with NewTabWithUrl: * Opening a chrome://settings URL should open in a new window * In multi-profile, if the sole browser window for the current user is "teleported" to another user's desktop the newly opened tab might not be visible, so move the window to the current desktop. For ARC, rather than calling from //components/arc to //ash to //chrome, just directly delegate from //components/arc to //chrome, since the ARC bridge runs in the browser process. Clean up an unused NewTabWithUrl declaration in new_window.mojom Bug: 846128 Change-Id: I994bff37c9e7552cd016f6e1f55444d80da32644 Reviewed-on: https://chromium-review.googlesource.com/1091879 Commit-Queue: James Cook <jamescook@chromium.org> Reviewed-by: Luis Hector Chavez <lhchavez@chromium.org> Reviewed-by: Tom Sepez <tsepez@chromium.org> Reviewed-by: David Jacobo <djacobo@chromium.org> Cr-Commit-Position: refs/heads/master@{#566089} [modify] https://crrev.com/84611ddaa77666930a0c24d94d312488a5abef47/ash/assistant/assistant_controller.cc [modify] https://crrev.com/84611ddaa77666930a0c24d94d312488a5abef47/ash/new_window_controller.h [modify] https://crrev.com/84611ddaa77666930a0c24d94d312488a5abef47/ash/public/interfaces/new_window.mojom [modify] https://crrev.com/84611ddaa77666930a0c24d94d312488a5abef47/ash/shell/shell_delegate_impl.cc [modify] https://crrev.com/84611ddaa77666930a0c24d94d312488a5abef47/ash/shell/shell_delegate_impl.h [modify] https://crrev.com/84611ddaa77666930a0c24d94d312488a5abef47/ash/shell_delegate.h [modify] https://crrev.com/84611ddaa77666930a0c24d94d312488a5abef47/ash/shell_delegate_mash.cc [modify] https://crrev.com/84611ddaa77666930a0c24d94d312488a5abef47/ash/shell_delegate_mash.h [modify] https://crrev.com/84611ddaa77666930a0c24d94d312488a5abef47/ash/test_shell_delegate.cc [modify] https://crrev.com/84611ddaa77666930a0c24d94d312488a5abef47/ash/test_shell_delegate.h [modify] https://crrev.com/84611ddaa77666930a0c24d94d312488a5abef47/chrome/browser/ui/ash/chrome_new_window_client.cc [modify] https://crrev.com/84611ddaa77666930a0c24d94d312488a5abef47/chrome/browser/ui/ash/chrome_new_window_client.h [modify] https://crrev.com/84611ddaa77666930a0c24d94d312488a5abef47/chrome/browser/ui/ash/chrome_shell_delegate.cc [modify] https://crrev.com/84611ddaa77666930a0c24d94d312488a5abef47/chrome/browser/ui/ash/chrome_shell_delegate.h [modify] https://crrev.com/84611ddaa77666930a0c24d94d312488a5abef47/components/arc/BUILD.gn [modify] https://crrev.com/84611ddaa77666930a0c24d94d312488a5abef47/components/arc/intent_helper/arc_intent_helper_bridge.cc [modify] https://crrev.com/84611ddaa77666930a0c24d94d312488a5abef47/components/arc/intent_helper/arc_intent_helper_bridge.h [modify] https://crrev.com/84611ddaa77666930a0c24d94d312488a5abef47/components/arc/intent_helper/arc_intent_helper_bridge_unittest.cc [add] https://crrev.com/84611ddaa77666930a0c24d94d312488a5abef47/components/arc/intent_helper/open_url_delegate.h
,
Jun 11 2018
|
||||
►
Sign in to add a comment |
||||
Comment 1 by steve...@chromium.org
, May 23 2018