New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 824598 link

Starred by 2 users
Status: Started
Owner:
dominickn@chromium.org
Cc:
yusukes@chromium.org
elijahtaylor@chromium.org
█ djacobo@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug



Sign in to add a comment

Refactor ARC++ intent helper to be app-platform agnostic

Project Member Reported by dominickn@chromium.org, Mar 22 2018 
We want to allow other app platforms on Chrome OS to integrate into the system in the way ARC++ has, so this entails taking the existing ARC code and splitting it into app-platform-generic and app-platform-specific components.

The ARC++ intent helper allows users to launch apps from the omnibox. This bug is to take the intent helper code and split it into ARC++-specific and app-platform-generic components so that we can add other app platforms (first desktop PWAs) as launch options for users.
Project Member

Comment 1 by bugdroid1@chromium.org, Mar 27 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7c28994265ed2cabcd654ba38c62c6c1f3f0ec5d

commit 7c28994265ed2cabcd654ba38c62c6c1f3f0ec5d
Author: Dominick Ng <dominickn@chromium.org>
Date: Tue Mar 27 05:57:15 2018

Introduce AppsNavigationThrottle, and move ArcNavigationThrottle creation into it.

This CL creates the chrome/browser/chromeos/apps directory, which will
contain code to facilitate the different apps platforms on Chrome OS
interoperating with each other and the Chrome OS system.

Much of the initial work will be to take existing integrations for
ARC++, and make them sufficiently generic so that other app platforms
can use them. We first focus on the ARC++ intent helper, which allows
users to open an app from the omnibox.

This CL moves construction of the ArcNavigationThrottle, which controls
the appearance of the intent helper, from ChromeContentBrowserClient
into a new AppsNavigationThrottle. Follow-up CLs will begin to split
ArcNavigationThrottle so that the throttle-specific code is moved to
AppsNavigationThrottle. The change in this CL means that the follow-up
refactoring will not need to change ChromeContentBrowserClient again.

BUG=824598

Change-Id: I4062a198b631215709deeab80fe669e0aea53857
Reviewed-on: https://chromium-review.googlesource.com/974761
Reviewed-by: Satoru Takabayashi <satorux@chromium.org>
Reviewed-by: Elijah Taylor <elijahtaylor@chromium.org>
Reviewed-by: David Jacobo <djacobo@chromium.org>
Commit-Queue: Dominick Ng <dominickn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#546036}
[modify] https://crrev.com/7c28994265ed2cabcd654ba38c62c6c1f3f0ec5d/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/7c28994265ed2cabcd654ba38c62c6c1f3f0ec5d/chrome/browser/chromeos/BUILD.gn
[add] https://crrev.com/7c28994265ed2cabcd654ba38c62c6c1f3f0ec5d/chrome/browser/chromeos/apps/OWNERS
[add] https://crrev.com/7c28994265ed2cabcd654ba38c62c6c1f3f0ec5d/chrome/browser/chromeos/apps/README.md
[add] https://crrev.com/7c28994265ed2cabcd654ba38c62c6c1f3f0ec5d/chrome/browser/chromeos/apps/intent_helper/OWNERS
[add] https://crrev.com/7c28994265ed2cabcd654ba38c62c6c1f3f0ec5d/chrome/browser/chromeos/apps/intent_helper/apps_navigation_throttle.cc
[add] https://crrev.com/7c28994265ed2cabcd654ba38c62c6c1f3f0ec5d/chrome/browser/chromeos/apps/intent_helper/apps_navigation_throttle.h

Comment 2 by dominickn@chromium.org, Mar 27 2018

Cc: yusukes@chromium.org
One complicated case that's arisen from the design doc is what to do if a user has an ARC++ app and a desktop PWA installed that can handle a particular URL. We can't yet persist that the PWA should be opened all the time (we need to revamp the underlying preference model for apps and move it into the browser, with synchronisation into the app-platform-specific code like the ARC++ container).

We got guidance from Chrome OS UI review this morning: if a user has an ARC++ app and a desktop PWA installed that can handle a URL, we should show *both* of the apps in the intent picker, but *grey out* the persistence checkbox if the PWA is selected. This is the simplest option that should make M67, and should only appear in a very small number of cases (desktop PWA + ARC++ app installed). It also incentivises us working out the preference model in short order. :)

Comment 3 by yusukes@chromium.org, Mar 27 2018 

> but *grey out* the persistence checkbox if the PWA is selected

I'm not really sure if this works. IIRC, app names in the picker do not always have "selected" state. The user doesn't really select it, but just click it.

Comment 4 by dominickn@chromium.org, Mar 28 2018 

#3: I just tested and it seems like there is selected state in the picker - you pick one and then press "Use app" or "Open in Chrome" to confirm.

Comment 5 by yusukes@chromium.org, Mar 28 2018 

Ah sorry you're right. I misremembered the UI.
Project Member

Comment 6 by bugdroid1@chromium.org, Apr 4 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/59497f7b74973e58e3a04067250f8a4a33e4bf02

commit 59497f7b74973e58e3a04067250f8a4a33e4bf02
Author: Dominick Ng <dominickn@chromium.org>
Date: Wed Apr 04 00:01:13 2018

Migrate generic throttle code from ArcNavigationThrottle to AppsNavigationThrottle.

This CL moves the generic Chrome OS intent picker navigation throttling
behaviour from ArcNavigationThrottle to AppsNavigationThrottle.
AppsNavigationThrottle::MaybeCreate now returns an instance of
AppsNavigationThrottle, and ArcNavigationThrottle now handles showing
the intent picker UI and querying ARC++ for apps. This CL should have no
functional changes.

Follow-up CLs will migrate the showing the intent picker UI to
AppsNavigationThrottle, and rename ArcNavigationThrottle. The renaming
was not done in this CL for ease of review. Strictly speaking,
ArcNavigationThrottle is still a semantically correct name; the main
reason for renaming is to avoid confusion with subclasses of
content::NavigationThrottle.

BUG=824598

Change-Id: I82c6e48cf7929701ea9f59cf811c4b7de70fd49a
Reviewed-on: https://chromium-review.googlesource.com/974622
Commit-Queue: Dominick Ng <dominickn@chromium.org>
Reviewed-by: Yusuke Sato <yusukes@chromium.org>
Reviewed-by: David Jacobo <djacobo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547875}
[modify] https://crrev.com/59497f7b74973e58e3a04067250f8a4a33e4bf02/chrome/browser/chromeos/BUILD.gn
[modify] https://crrev.com/59497f7b74973e58e3a04067250f8a4a33e4bf02/chrome/browser/chromeos/apps/intent_helper/apps_navigation_throttle.cc
[modify] https://crrev.com/59497f7b74973e58e3a04067250f8a4a33e4bf02/chrome/browser/chromeos/apps/intent_helper/apps_navigation_throttle.h
[add] https://crrev.com/59497f7b74973e58e3a04067250f8a4a33e4bf02/chrome/browser/chromeos/apps/intent_helper/apps_navigation_throttle_unittest.cc
[add] https://crrev.com/59497f7b74973e58e3a04067250f8a4a33e4bf02/chrome/browser/chromeos/apps/intent_helper/apps_navigation_types.h
[modify] https://crrev.com/59497f7b74973e58e3a04067250f8a4a33e4bf02/chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle.cc
[modify] https://crrev.com/59497f7b74973e58e3a04067250f8a4a33e4bf02/chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle.h
[modify] https://crrev.com/59497f7b74973e58e3a04067250f8a4a33e4bf02/chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle_unittest.cc
Project Member

Comment 7 by bugdroid1@chromium.org, Apr 4 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7fa990ed1273c8d597539081c02b0dbe8a568879

commit 7fa990ed1273c8d597539081c02b0dbe8a568879
Author: Dominick Ng <dominickn@chromium.org>
Date: Wed Apr 04 00:09:09 2018

Migrate showing the intent picker from ArcNavigationThrottle to AppsNavigationThrottle.

This CL moves the code responsible for displaying the intent picker UI
on Chrome OS from ArcNavigationThrottle to the app-platform-generic
AppsNavigationThrottle class. There should be no functional changes.

The supporting AppInfo type is migrated to apps_navigation_types.h to
allow other app platforms to make use of it.

Future CLs will migrate the
ArcNavigationThrottle::AsyncOnIntentPickerClosed method to
AppsNavigationThrottle, so that apps of different types can have UMA
recorded.

BUG=824598
TBR=sky@chromium.org

Change-Id: I875cd4d59f7ac562eaeba973fa67daf3a213f136
Reviewed-on: https://chromium-review.googlesource.com/981834
Commit-Queue: Dominick Ng <dominickn@chromium.org>
Reviewed-by: David Jacobo <djacobo@chromium.org>
Reviewed-by: Yusuke Sato <yusukes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547879}
[modify] https://crrev.com/7fa990ed1273c8d597539081c02b0dbe8a568879/chrome/browser/chromeos/BUILD.gn
[modify] https://crrev.com/7fa990ed1273c8d597539081c02b0dbe8a568879/chrome/browser/chromeos/apps/intent_helper/apps_navigation_throttle.cc
[modify] https://crrev.com/7fa990ed1273c8d597539081c02b0dbe8a568879/chrome/browser/chromeos/apps/intent_helper/apps_navigation_throttle.h
[add] https://crrev.com/7fa990ed1273c8d597539081c02b0dbe8a568879/chrome/browser/chromeos/apps/intent_helper/apps_navigation_types.cc
[modify] https://crrev.com/7fa990ed1273c8d597539081c02b0dbe8a568879/chrome/browser/chromeos/apps/intent_helper/apps_navigation_types.h
[modify] https://crrev.com/7fa990ed1273c8d597539081c02b0dbe8a568879/chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc
[modify] https://crrev.com/7fa990ed1273c8d597539081c02b0dbe8a568879/chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle.cc
[modify] https://crrev.com/7fa990ed1273c8d597539081c02b0dbe8a568879/chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle.h
[modify] https://crrev.com/7fa990ed1273c8d597539081c02b0dbe8a568879/chrome/browser/ui/browser_commands.cc
[modify] https://crrev.com/7fa990ed1273c8d597539081c02b0dbe8a568879/chrome/browser/ui/browser_commands.h
[modify] https://crrev.com/7fa990ed1273c8d597539081c02b0dbe8a568879/chrome/browser/ui/browser_dialogs.h
[modify] https://crrev.com/7fa990ed1273c8d597539081c02b0dbe8a568879/chrome/browser/ui/browser_window.h
[modify] https://crrev.com/7fa990ed1273c8d597539081c02b0dbe8a568879/chrome/browser/ui/views/frame/browser_view.cc
[modify] https://crrev.com/7fa990ed1273c8d597539081c02b0dbe8a568879/chrome/browser/ui/views/frame/browser_view.h
[modify] https://crrev.com/7fa990ed1273c8d597539081c02b0dbe8a568879/chrome/browser/ui/views/intent_picker_bubble_view.cc
[modify] https://crrev.com/7fa990ed1273c8d597539081c02b0dbe8a568879/chrome/browser/ui/views/intent_picker_bubble_view.h
[modify] https://crrev.com/7fa990ed1273c8d597539081c02b0dbe8a568879/chrome/browser/ui/views/intent_picker_bubble_view_unittest.cc
[modify] https://crrev.com/7fa990ed1273c8d597539081c02b0dbe8a568879/chrome/browser/ui/views/location_bar/intent_picker_view.cc
[modify] https://crrev.com/7fa990ed1273c8d597539081c02b0dbe8a568879/chrome/test/base/test_browser_window.h
Project Member

Comment 8 by bugdroid1@chromium.org, Apr 5 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6b17a19a6c95c70c333af563db1c17795e5146c1

commit 6b17a19a6c95c70c333af563db1c17795e5146c1
Author: Dominick Ng <dominickn@chromium.org>
Date: Thu Apr 05 03:23:20 2018

Move the IntentPickerResponse callback to AppsNavigationThrottle.

This CL completes the refactoring of the ArcNavigationThrottle to be
app-platform agnostic by moving the callback run when the user responds
to the intent picker from ArcNavigationThrottle to
AppsNavigationThrottle. This will enable a future CL where the
AppsNavigationThrottle can insert non-ARC++ apps into the intent
picker, and then launch those apps if the user selects them.

There should be no functional changes from this CL. Two implementation
changes include:

1) the intent picker UI now returns an app-platform-generic CloseReason,
   and a boolean indicating if the user wanted to persist their choice.
   The AppsNavigationThrottle passes these to the ARC++ code, which
   interpolates them to the existing histogram buckets for metrics
   (which were formerly returned directly by the UI). This simplifies
   code in the UI layer, and moves responsibility for computing
   histogram values into the app-platform-specific code.

2) IntentPickerResponse is changed to be a OnceCallback, which removes
   some book-keeping in the UI layer for enforcing that it is only run
   once.

BUG=824598
TBR=sky@chromium.org

Change-Id: I1de4fd85c30e9ed09622ead11b709b9d6cdcfa98
Reviewed-on: https://chromium-review.googlesource.com/983197
Commit-Queue: Dominick Ng <dominickn@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: David Jacobo <djacobo@chromium.org>
Reviewed-by: Yusuke Sato <yusukes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548306}
[modify] https://crrev.com/6b17a19a6c95c70c333af563db1c17795e5146c1/chrome/browser/chromeos/apps/intent_helper/apps_navigation_throttle.cc
[modify] https://crrev.com/6b17a19a6c95c70c333af563db1c17795e5146c1/chrome/browser/chromeos/apps/intent_helper/apps_navigation_throttle.h
[modify] https://crrev.com/6b17a19a6c95c70c333af563db1c17795e5146c1/chrome/browser/chromeos/apps/intent_helper/apps_navigation_types.h
[modify] https://crrev.com/6b17a19a6c95c70c333af563db1c17795e5146c1/chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc
[modify] https://crrev.com/6b17a19a6c95c70c333af563db1c17795e5146c1/chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle.cc
[modify] https://crrev.com/6b17a19a6c95c70c333af563db1c17795e5146c1/chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle.h
[modify] https://crrev.com/6b17a19a6c95c70c333af563db1c17795e5146c1/chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle_unittest.cc
[modify] https://crrev.com/6b17a19a6c95c70c333af563db1c17795e5146c1/chrome/browser/ui/browser_commands.cc
[modify] https://crrev.com/6b17a19a6c95c70c333af563db1c17795e5146c1/chrome/browser/ui/browser_dialogs.h
[modify] https://crrev.com/6b17a19a6c95c70c333af563db1c17795e5146c1/chrome/browser/ui/views/frame/browser_view.cc
[modify] https://crrev.com/6b17a19a6c95c70c333af563db1c17795e5146c1/chrome/browser/ui/views/intent_picker_bubble_view.cc
[modify] https://crrev.com/6b17a19a6c95c70c333af563db1c17795e5146c1/chrome/browser/ui/views/intent_picker_bubble_view.h
[modify] https://crrev.com/6b17a19a6c95c70c333af563db1c17795e5146c1/chrome/browser/ui/views/intent_picker_bubble_view_unittest.cc
[modify] https://crrev.com/6b17a19a6c95c70c333af563db1c17795e5146c1/chrome/browser/ui/views/toolbar/toolbar_view.cc
Project Member

Comment 9 by bugdroid1@chromium.org, Apr 5 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/309cd58c61de83793cb48ec8f43d84a885053128

commit 309cd58c61de83793cb48ec8f43d84a885053128
Author: Dominick Ng <dominickn@chromium.org>
Date: Thu Apr 05 05:39:34 2018

Make IntentPickerAppInfo move-only.

BUG=824598
TBR=sky@chromium.org

Change-Id: I09ac08a2042efffb8562b6a0d9764fdaa6231ff1
Reviewed-on: https://chromium-review.googlesource.com/991619
Commit-Queue: Dominick Ng <dominickn@chromium.org>
Reviewed-by: Yusuke Sato <yusukes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548343}
[modify] https://crrev.com/309cd58c61de83793cb48ec8f43d84a885053128/chrome/browser/chromeos/apps/intent_helper/apps_navigation_throttle.cc
[modify] https://crrev.com/309cd58c61de83793cb48ec8f43d84a885053128/chrome/browser/chromeos/apps/intent_helper/apps_navigation_throttle.h
[modify] https://crrev.com/309cd58c61de83793cb48ec8f43d84a885053128/chrome/browser/chromeos/apps/intent_helper/apps_navigation_types.cc
[modify] https://crrev.com/309cd58c61de83793cb48ec8f43d84a885053128/chrome/browser/chromeos/apps/intent_helper/apps_navigation_types.h
[modify] https://crrev.com/309cd58c61de83793cb48ec8f43d84a885053128/chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc
[modify] https://crrev.com/309cd58c61de83793cb48ec8f43d84a885053128/chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle.cc
[modify] https://crrev.com/309cd58c61de83793cb48ec8f43d84a885053128/chrome/browser/ui/browser_commands.cc
[modify] https://crrev.com/309cd58c61de83793cb48ec8f43d84a885053128/chrome/browser/ui/browser_commands.h
[modify] https://crrev.com/309cd58c61de83793cb48ec8f43d84a885053128/chrome/browser/ui/browser_dialogs.h
[modify] https://crrev.com/309cd58c61de83793cb48ec8f43d84a885053128/chrome/browser/ui/browser_window.h
[modify] https://crrev.com/309cd58c61de83793cb48ec8f43d84a885053128/chrome/browser/ui/views/frame/browser_view.cc
[modify] https://crrev.com/309cd58c61de83793cb48ec8f43d84a885053128/chrome/browser/ui/views/frame/browser_view.h
[modify] https://crrev.com/309cd58c61de83793cb48ec8f43d84a885053128/chrome/browser/ui/views/intent_picker_bubble_view.cc
[modify] https://crrev.com/309cd58c61de83793cb48ec8f43d84a885053128/chrome/browser/ui/views/intent_picker_bubble_view.h
[modify] https://crrev.com/309cd58c61de83793cb48ec8f43d84a885053128/chrome/browser/ui/views/intent_picker_bubble_view_unittest.cc
[modify] https://crrev.com/309cd58c61de83793cb48ec8f43d84a885053128/chrome/browser/ui/views/toolbar/toolbar_view.cc
[modify] https://crrev.com/309cd58c61de83793cb48ec8f43d84a885053128/chrome/browser/ui/views/toolbar/toolbar_view.h
[modify] https://crrev.com/309cd58c61de83793cb48ec8f43d84a885053128/chrome/test/base/test_browser_window.h
Project Member

Comment 10 by bugdroid1@chromium.org, Apr 10 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c7867ebdfb9d6a4d9233dd4be363cd291a6bc6aa

commit c7867ebdfb9d6a4d9233dd4be363cd291a6bc6aa
Author: Dominick Ng <dominickn@chromium.org>
Date: Tue Apr 10 21:20:54 2018

Fix a race condition with redirects in the Chrome OS intent picker.

This CL fixes a bug introduced during the refactoring of
ArcNavigationThrottle to AppsNavigationThrottle where the intent picker
could be shown multiple times during a redirect chain. If the user has
ARC apps installed which handle all URLs (e.g. Chrome Beta/Chrome Dev),
we could get into a situation where we show the intent picker but
reset our internal state to be not showing.

The fix is to only set the internal |ui_displayed_| state to false if
the apps list sent back to AppsNavigationThrottle is empty.

BUG=824598

Change-Id: I990d00d90665d362ee9e894ad71a71454e78ebea
Reviewed-on: https://chromium-review.googlesource.com/1003812
Reviewed-by: David Jacobo <djacobo@chromium.org>
Commit-Queue: Dominick Ng <dominickn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#549649}
[modify] https://crrev.com/c7867ebdfb9d6a4d9233dd4be363cd291a6bc6aa/chrome/browser/chromeos/apps/intent_helper/apps_navigation_throttle.cc
Project Member

Comment 11 by bugdroid1@chromium.org, Apr 16 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4c34fe8d5be3146eccf14f8f47e7798d9d59d930

commit 4c34fe8d5be3146eccf14f8f47e7798d9d59d930
Author: Dominick Ng <dominickn@chromium.org>
Date: Mon Apr 16 02:30:14 2018

Add desktop PWA pop-out as an option in the Chrome OS intent picker.

This CL allows installed desktop PWAs on Chrome OS to appear as an
option in the intent picker. If the user chooses to open the current URL
in the PWA, the current web contents will be reparented to the app
container.

Following UI review guidance, the persistence toggle is disabled for
desktop PWAs as the underlying system does not yet support persisting
the user's choice. The checkbox is re-enabled if an ARC app is selected.

The Arc.IntentHelper* UMA histograms are deprecated in favour of new,
app-platform generic histograms, and will be removed in an upcoming
milestone.

BUG=824598

Change-Id: I292ffc8cca9212483f0c0e1359610d3d0c40c633
Reviewed-on: https://chromium-review.googlesource.com/991495
Commit-Queue: Dominick Ng <dominickn@chromium.org>
Reviewed-by: David Jacobo <djacobo@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Yusuke Sato <yusukes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550926}
[modify] https://crrev.com/4c34fe8d5be3146eccf14f8f47e7798d9d59d930/chrome/browser/chromeos/apps/intent_helper/apps_navigation_throttle.cc
[modify] https://crrev.com/4c34fe8d5be3146eccf14f8f47e7798d9d59d930/chrome/browser/chromeos/apps/intent_helper/apps_navigation_throttle.h
[modify] https://crrev.com/4c34fe8d5be3146eccf14f8f47e7798d9d59d930/chrome/browser/chromeos/apps/intent_helper/apps_navigation_throttle_unittest.cc
[modify] https://crrev.com/4c34fe8d5be3146eccf14f8f47e7798d9d59d930/chrome/browser/chromeos/apps/intent_helper/apps_navigation_types.h
[modify] https://crrev.com/4c34fe8d5be3146eccf14f8f47e7798d9d59d930/chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc
[modify] https://crrev.com/4c34fe8d5be3146eccf14f8f47e7798d9d59d930/chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle.cc
[modify] https://crrev.com/4c34fe8d5be3146eccf14f8f47e7798d9d59d930/chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle.h
[modify] https://crrev.com/4c34fe8d5be3146eccf14f8f47e7798d9d59d930/chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle_unittest.cc
[modify] https://crrev.com/4c34fe8d5be3146eccf14f8f47e7798d9d59d930/chrome/browser/ui/browser_commands.cc
[modify] https://crrev.com/4c34fe8d5be3146eccf14f8f47e7798d9d59d930/chrome/browser/ui/browser_commands.h
[modify] https://crrev.com/4c34fe8d5be3146eccf14f8f47e7798d9d59d930/chrome/browser/ui/views/intent_picker_bubble_view.cc
[modify] https://crrev.com/4c34fe8d5be3146eccf14f8f47e7798d9d59d930/chrome/browser/ui/views/intent_picker_bubble_view.h
[modify] https://crrev.com/4c34fe8d5be3146eccf14f8f47e7798d9d59d930/chrome/browser/ui/views/location_bar/intent_picker_view.cc
[modify] https://crrev.com/4c34fe8d5be3146eccf14f8f47e7798d9d59d930/tools/metrics/histograms/enums.xml
[modify] https://crrev.com/4c34fe8d5be3146eccf14f8f47e7798d9d59d930/tools/metrics/histograms/histograms.xml
Project Member

Comment 12 by bugdroid1@chromium.org, Apr 17 2018

Labels: merge-merged-testbranch
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4c34fe8d5be3146eccf14f8f47e7798d9d59d930

commit 4c34fe8d5be3146eccf14f8f47e7798d9d59d930
Author: Dominick Ng <dominickn@chromium.org>
Date: Mon Apr 16 02:30:14 2018

Add desktop PWA pop-out as an option in the Chrome OS intent picker.

This CL allows installed desktop PWAs on Chrome OS to appear as an
option in the intent picker. If the user chooses to open the current URL
in the PWA, the current web contents will be reparented to the app
container.

Following UI review guidance, the persistence toggle is disabled for
desktop PWAs as the underlying system does not yet support persisting
the user's choice. The checkbox is re-enabled if an ARC app is selected.

The Arc.IntentHelper* UMA histograms are deprecated in favour of new,
app-platform generic histograms, and will be removed in an upcoming
milestone.

BUG=824598

Change-Id: I292ffc8cca9212483f0c0e1359610d3d0c40c633
Reviewed-on: https://chromium-review.googlesource.com/991495
Commit-Queue: Dominick Ng <dominickn@chromium.org>
Reviewed-by: David Jacobo <djacobo@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Yusuke Sato <yusukes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550926}
[modify] https://crrev.com/4c34fe8d5be3146eccf14f8f47e7798d9d59d930/chrome/browser/chromeos/apps/intent_helper/apps_navigation_throttle.cc
[modify] https://crrev.com/4c34fe8d5be3146eccf14f8f47e7798d9d59d930/chrome/browser/chromeos/apps/intent_helper/apps_navigation_throttle.h
[modify] https://crrev.com/4c34fe8d5be3146eccf14f8f47e7798d9d59d930/chrome/browser/chromeos/apps/intent_helper/apps_navigation_throttle_unittest.cc
[modify] https://crrev.com/4c34fe8d5be3146eccf14f8f47e7798d9d59d930/chrome/browser/chromeos/apps/intent_helper/apps_navigation_types.h
[modify] https://crrev.com/4c34fe8d5be3146eccf14f8f47e7798d9d59d930/chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc
[modify] https://crrev.com/4c34fe8d5be3146eccf14f8f47e7798d9d59d930/chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle.cc
[modify] https://crrev.com/4c34fe8d5be3146eccf14f8f47e7798d9d59d930/chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle.h
[modify] https://crrev.com/4c34fe8d5be3146eccf14f8f47e7798d9d59d930/chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle_unittest.cc
[modify] https://crrev.com/4c34fe8d5be3146eccf14f8f47e7798d9d59d930/chrome/browser/ui/browser_commands.cc
[modify] https://crrev.com/4c34fe8d5be3146eccf14f8f47e7798d9d59d930/chrome/browser/ui/browser_commands.h
[modify] https://crrev.com/4c34fe8d5be3146eccf14f8f47e7798d9d59d930/chrome/browser/ui/views/intent_picker_bubble_view.cc
[modify] https://crrev.com/4c34fe8d5be3146eccf14f8f47e7798d9d59d930/chrome/browser/ui/views/intent_picker_bubble_view.h
[modify] https://crrev.com/4c34fe8d5be3146eccf14f8f47e7798d9d59d930/chrome/browser/ui/views/location_bar/intent_picker_view.cc
[modify] https://crrev.com/4c34fe8d5be3146eccf14f8f47e7798d9d59d930/tools/metrics/histograms/enums.xml
[modify] https://crrev.com/4c34fe8d5be3146eccf14f8f47e7798d9d59d930/tools/metrics/histograms/histograms.xml
Project Member

Comment 13 by bugdroid1@chromium.org, Apr 18 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0e2cbe016572567774b8c3b884ba579b0aa4f882

commit 0e2cbe016572567774b8c3b884ba579b0aa4f882
Author: Dominick Ng <dominickn@chromium.org>
Date: Wed Apr 18 00:38:08 2018

Make ArcNavigationThrottle a content::WebContentsObserver.

This CL ensures that querying for ARC apps is gracefully aborted if the
owning WebContents is destroyed during the asynchronous process.

ArcNavigationThrottle is converted to be a self-owning object with a
private constructor and destructor. When the static app query methods
are invoked, an instance of the object is instantiated to handle the
querying. By overriding WebContentsDestroyed(), it can cancel all
in-flight callbacks upon WebContents destruction and safely bail out of
querying.

BUG=824598
TBR=sky@chromium.org

Change-Id: I2ba6d391b9babec46b277cbcebe567786215bf5a
Reviewed-on: https://chromium-review.googlesource.com/1003432
Commit-Queue: Dominick Ng <dominickn@chromium.org>
Reviewed-by: Yusuke Sato <yusukes@chromium.org>
Reviewed-by: David Jacobo <djacobo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551537}
[modify] https://crrev.com/0e2cbe016572567774b8c3b884ba579b0aa4f882/chrome/browser/chromeos/apps/intent_helper/apps_navigation_throttle.cc
[modify] https://crrev.com/0e2cbe016572567774b8c3b884ba579b0aa4f882/chrome/browser/chromeos/apps/intent_helper/apps_navigation_throttle.h
[modify] https://crrev.com/0e2cbe016572567774b8c3b884ba579b0aa4f882/chrome/browser/chromeos/apps/intent_helper/apps_navigation_types.h
[modify] https://crrev.com/0e2cbe016572567774b8c3b884ba579b0aa4f882/chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle.cc
[modify] https://crrev.com/0e2cbe016572567774b8c3b884ba579b0aa4f882/chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle.h
[modify] https://crrev.com/0e2cbe016572567774b8c3b884ba579b0aa4f882/chrome/browser/ui/views/location_bar/intent_picker_view.cc
Project Member

Comment 14 by bugdroid1@chromium.org, Apr 19 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c4a60b3331f69e171a94d4a5c3c129d0213e710f

commit c4a60b3331f69e171a94d4a5c3c129d0213e710f
Author: Dominick Ng <dominickn@chromium.org>
Date: Thu Apr 19 05:18:49 2018

Split out BookmarkAppNavigationBrowserTest into a separate .h/.cc file.

This will allow other tests to reuse the common bookmark app navigation
test set up code. This CL has no functional changes.

BUG=824598

Change-Id: Ib6fa3551d6839592e4caefd7e9948e99aa2fa67e
Reviewed-on: https://chromium-review.googlesource.com/1015582
Commit-Queue: Dominick Ng <dominickn@chromium.org>
Reviewed-by: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551947}
[add] https://crrev.com/c4a60b3331f69e171a94d4a5c3c129d0213e710f/chrome/browser/extensions/bookmark_app_navigation_browsertest.cc
[add] https://crrev.com/c4a60b3331f69e171a94d4a5c3c129d0213e710f/chrome/browser/extensions/bookmark_app_navigation_browsertest.h
[modify] https://crrev.com/c4a60b3331f69e171a94d4a5c3c129d0213e710f/chrome/browser/extensions/bookmark_app_navigation_throttle_browsertest.cc
[modify] https://crrev.com/c4a60b3331f69e171a94d4a5c3c129d0213e710f/chrome/test/BUILD.gn
Project Member

Comment 15 by bugdroid1@chromium.org, Apr 20 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3fd5afc124ed18e8fd9d8ab6777f0a1674ef1cb0

commit 3fd5afc124ed18e8fd9d8ab6777f0a1674ef1cb0
Author: Dominick Ng <dominickn@chromium.org>
Date: Fri Apr 20 13:21:34 2018

Add tests for desktop PWAs appearing in the Chrome OS intent picker.

This CL adds tests for triggering the Chrome OS intent picker with
installed desktop PWAs. Specifically:
 * link navigations from a tabbed browser to a URL within the scope of
   an installed PWA trigger the intent picker with the installed app
   details
 * all other link navigations from an app browser or from a tabbed
   browser outside the scope of an installed PWA do not trigger the
   intent picker.

BUG=824598

Change-Id: I6aa5d067ef999bb428451572c97dcb459725c84e
Reviewed-on: https://chromium-review.googlesource.com/1013883
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Reviewed-by: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Reviewed-by: David Jacobo <djacobo@chromium.org>
Commit-Queue: Dominick Ng <dominickn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#552314}
[modify] https://crrev.com/3fd5afc124ed18e8fd9d8ab6777f0a1674ef1cb0/chrome/browser/ui/views/intent_picker_bubble_view.h
[add] https://crrev.com/3fd5afc124ed18e8fd9d8ab6777f0a1674ef1cb0/chrome/browser/ui/views/intent_picker_bubble_view_browsertest.cc
[modify] https://crrev.com/3fd5afc124ed18e8fd9d8ab6777f0a1674ef1cb0/chrome/browser/ui/views/intent_picker_bubble_view_unittest.cc
[modify] https://crrev.com/3fd5afc124ed18e8fd9d8ab6777f0a1674ef1cb0/chrome/test/BUILD.gn
Project Member

Comment 16 by bugdroid1@chromium.org, May 23 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1240aee06a0164dfe38756214a44ecb02b5d8912

commit 1240aee06a0164dfe38756214a44ecb02b5d8912
Author: Dominick Ng <dominickn@chromium.org>
Date: Wed May 23 06:35:54 2018

Do not pop out the intent picker if only desktop PWAs are in the app list.

Until the Chrome OS app registry is implemented, we cannot persist
"Remember my choice" for desktop PWAs in the intent picker.

On the advice of UI review, this CL suppresses popping out the intent
picker and instead shows the intent picker omnibox icon if only
desktop PWAs are available for the picker. This reduces user annoyance
until the persistence mechanism is available, at which point we will
pop out the picker for desktop PWAs again.

The picker will pop out if any ARC app is present in the list of apps.

BUG=824598

Change-Id: I027f231bae533f077a8a85acca41b64ae7a81444
Reviewed-on: https://chromium-review.googlesource.com/1068579
Commit-Queue: Dominick Ng <dominickn@chromium.org>
Reviewed-by: David Jacobo <djacobo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#560970}
[modify] https://crrev.com/1240aee06a0164dfe38756214a44ecb02b5d8912/chrome/browser/chromeos/apps/intent_helper/apps_navigation_throttle.cc
Project Member

Comment 17 by bugdroid1@chromium.org, Sep 20 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/985aba67fc2054a089de1b90ca02a35219f2232d

commit 985aba67fc2054a089de1b90ca02a35219f2232d
Author: yusukes <yusukes@chromium.org>
Date: Thu Sep 20 01:06:02 2018

arc: Stop recording obsolete UMA metrics

The Arc.IntentHandler* metrics have been replaced with
ChromeOS.Apps.IntentPicker* months ago.

BUG=824598
TEST=try

Change-Id: I4d86f17c4a1f5529e6bf063837b75a48564bc378
Reviewed-on: https://chromium-review.googlesource.com/1234860
Commit-Queue: Yusuke Sato <yusukes@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#592644}
[modify] https://crrev.com/985aba67fc2054a089de1b90ca02a35219f2232d/chrome/browser/chromeos/apps/intent_helper/apps_navigation_throttle.cc
[modify] https://crrev.com/985aba67fc2054a089de1b90ca02a35219f2232d/tools/metrics/histograms/enums.xml
►

Sign in to add a comment