Issue metadata
Sign in to add a comment
|
Regression: Intent picker is unable to use 'Remember my choice' when the selected app is Chrome |
||||||||||||||||||||
Issue descriptionStarting from M67+ we are unable to save the user's preference when they decide to "stay in Chrome", which leads to the intent picker UI to be auto-displayed most of the times, this is even more evident if we have third-party browsers installed in ARC side.
,
May 10 2018
,
May 10 2018
After reading thru I think I know what it's going on. So we have 2 possible types of apps at the moment for the picker to show, ARC and PWA apps [1], of course we reserve an INVALID entry for errors/closing without picking an option. Since staying in Chrome is not an external app, but we still would like to persist the user's decision, we failed to correctly handle this scenario. We should have a way to persist this here [2] Ok that would solve persisting a new preference, but we also need to honor the previously set preference! That can be done here [3], by stop querying for app candidates as soon as I detect a preferred app, this is already done for ARC apps, but we forgot to do it for Chrome. Already working on this. [1] https://cs.chromium.org/chromium/src/chrome/browser/chromeos/apps/intent_helper/apps_navigation_types.h?type=cs&sq=package:chromium&l=16 [2] https://cs.chromium.org/chromium/src/chrome/browser/chromeos/apps/intent_helper/apps_navigation_throttle.cc?type=cs&q=OnIntentPicker&l=175 [3] https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle.cc?q=arc_navigation_th&sq=package:chromium&l=234
,
May 10 2018
,
May 10 2018
Thanks for catching this and deducing the cause. I think I introduced the bug during the refactoring work. :S I'll be back with laptop access from mid next week if that's of any help.
,
May 11 2018
I mean I reviewed the code so it is on me too :P, I was not expecting you to check this while ooo don't worry o/ I think we could introduce a new AppType, this WIP CL takes care of saving the preference crrev.com/c/1054584, now thinking about what to do for honoring such preference, keep in mind I Am trying to reduce this patch as much as possible so back porting it to M67 is not much of an issue, we could rework Apps|Arc-NavigationThrottle on ToT later.
,
May 11 2018
A couple of thoughts: 1. The PWA code landed after M67 branch, so this may be a nontrivial merge (conflict between trunk and branch) 2. Introducing a new apptype seems semantically wrong - strictly speaking, I think removing the apptype switch and calling MaybeLaunchOrPersistArcApp when apptype is AppType::ARC OR when the CloseReason is STAY_IN_CHROME might suffice (and be a smaller patch) This is an issue because we persist staying in Chrome within the ARC Container. With the app registry this should become less error prone soon :)
,
May 14 2018
In buganizer, this is tracked by http://b/79616133
,
May 14 2018
#CBC-RS/TC-watchlist
,
May 14 2018
That sounds reasonable to me Dominick, uploaded a new version, however I have had a hard time testing tot, let me know what you think. And yep, I Am excited about the app registry work o/
,
May 16 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6187d94092a3ae876cfd6a6a70be23328a0a6bfd commit 6187d94092a3ae876cfd6a6a70be23328a0a6bfd Author: David Jacobo <djacobo@chromium.org> Date: Wed May 16 19:13:56 2018 Fix regression for intent picker UI IntentPickerBubbleView is not saving the user's preference when they decide to "Stay in Chrome" + "Remember my choice". This is a regression that appeared in M67. This CL specials case AppType::INVALID to handle saving user's preference, previously proposed to add a new AppType::NATIVE_CHROME but that sounds wrong given this enum lists non-cros app sources. Besides that we need to make sure that the preference is communicated to ARC via AddPreferredPackage and honored within ArcNavigationThrottle, this can be achieved by adding a new enum PreferredPlatform that will help DidLaunchPreferredArcApp to be more explicit. Bug: 841971 Test: Build. Change-Id: I383a926ae613ffe48f4dcd3a478ac124aefe4947 Reviewed-on: https://chromium-review.googlesource.com/1054584 Commit-Queue: David Jacobo <djacobo@chromium.org> Reviewed-by: Yusuke Sato <yusukes@chromium.org> Cr-Commit-Position: refs/heads/master@{#559221} [modify] https://crrev.com/6187d94092a3ae876cfd6a6a70be23328a0a6bfd/chrome/browser/chromeos/apps/intent_helper/apps_navigation_throttle.cc [modify] https://crrev.com/6187d94092a3ae876cfd6a6a70be23328a0a6bfd/chrome/browser/chromeos/apps/intent_helper/apps_navigation_types.h [modify] https://crrev.com/6187d94092a3ae876cfd6a6a70be23328a0a6bfd/chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle.cc [modify] https://crrev.com/6187d94092a3ae876cfd6a6a70be23328a0a6bfd/chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle.h
,
May 18 2018
+ kbleicher 10688.0.0 has the change and it works as expected, Stay in chrome is persisted. steps I tried: 1) Enable ARC 2) Install chrome dev 3) search for dogs in Chrome 4) Click a link to en.wikipedia.org/wiki/dogs for example 5) since you have chrome dev it the UI will popout 6) Select 'Stay in Chrome' + 'Remember my choice' 7) Consecutive clicks to a similar links will remember the user decision, the omnibox's icon should be visible. We need this in M67 o/
,
May 18 2018
Given Kevin is ooo, could you please help us checking this out Josafat?
,
May 18 2018
This bug requires manual review: We are only 10 days from stable. Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
May 18 2018
This has made it through the PFQ and exists on ToT, and was verified, all signs show this is ok and something we want in 67.
,
May 18 2018
yes, let's get this in, we are getting several feedback reports on it
,
May 18 2018
crrev.com/c/1066597 is the port to M67, notice that the CL didn't apply cleanly. What remains the same for ToT and M67 chrome/browser/chromeos/apps/intent_helper/apps_navigation_types.h chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle.h chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle.cc However our fix in ToT modifies apps_navigation_throttle.cc, this is because we moved OnIntentPickerClosed from ArcNavigationThrottle to AppsNavigationThrottle, but not only that, we modified it to use AppType instead of IntentPickerCloseReason. The code for M67 that handles a response from the UI (i.e. IntentPickerBubbleView) is: 294 if (reason == chromeos::IntentPickerCloseReason::STAY_IN_CHROME || 295 (reason == chromeos::IntentPickerCloseReason::OPEN_APP && 296 app_type == chromeos::AppType::ARC)) { 297 if (should_persist) { 298 DCHECK(arc_service_manager); 299 if (ARC_GET_INSTANCE_FOR_METHOD( 300 arc_service_manager->arc_bridge_service()->intent_helper(), 301 AddPreferredPackage)) { 302 instance->AddPreferredPackage(pkg); 303 } 304 } 305 } This one persists the user's preference correctly even when we pick 'Stay in Chrome' + 'Remember my decision'so no need to change that.
,
May 18 2018
The snippet above is taken from M67 chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle.cc
,
May 18 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8e4baee3f1c0e88caea2f47a056711d05f243c64 commit 8e4baee3f1c0e88caea2f47a056711d05f243c64 Author: David Jacobo <djacobo@chromium.org> Date: Fri May 18 21:30:00 2018 [M67 merge] Fix regression for intent picker UI IntentPickerBubbleView is not saving the user's preference when they decide to "Stay in Chrome" + "Remember my choice". This is a regression that appeared in M67. This CL specials case AppType::INVALID to handle saving user's preference, previously proposed to add a new AppType::NATIVE_CHROME but that sounds wrong given this enum lists non-cros app sources. Besides that we need to make sure that the preference is communicated to ARC via AddPreferredPackage and honored within ArcNavigationThrottle, this can be achieved by adding a new enum PreferredPlatform that will help DidLaunchPreferredArcApp to be more explicit. (cherry picked from commit I383a926ae613ffe48f4dcd3a478ac124aefe4947) Bug: 841971 Test: Build. Change-Id: I405f2202b94e666768f9770e1e2edd0fbc9ec224 Reviewed-on: https://chromium-review.googlesource.com/1066597 Reviewed-by: Yusuke Sato <yusukes@chromium.org> Cr-Commit-Position: refs/branch-heads/3396@{#649} Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428} [modify] https://crrev.com/8e4baee3f1c0e88caea2f47a056711d05f243c64/chrome/browser/chromeos/apps/intent_helper/apps_navigation_types.h [modify] https://crrev.com/8e4baee3f1c0e88caea2f47a056711d05f243c64/chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle.cc [modify] https://crrev.com/8e4baee3f1c0e88caea2f47a056711d05f243c64/chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle.h
,
May 18 2018
,
May 21 2018
For the record Chrome 67.0.3396.52 has the change |
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by djacobo@chromium.org
, May 10 2018