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

Issue 841971 link

Starred by 7 users

Issue metadata

Status: Fixed
Owner:
Last visit 15 days ago
Closed: May 2018
Cc:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug-Regression



Sign in to add a comment

Regression: Intent picker is unable to use 'Remember my choice' when the selected app is Chrome

Project Member Reported by djacobo@chromium.org, May 10 2018

Issue description

Starting 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.



 
Cc: -shihuis@chromium.org shihuis@google.com
Labels: ReleaseBlock-Stable M-67
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
Cc: djacobo@chromium.org
 Issue 840599  has been merged into this issue.
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. 
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.
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 :)
In buganizer, this is tracked by http://b/79616133

Comment 9 by willg...@gmail.com, May 14 2018

#CBC-RS/TC-watchlist

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/
Project Member

Comment 11 by bugdroid1@chromium.org, 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

Cc: kbleicher@chromium.org
Labels: Merge-Request-67
+ 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/ 

Cc: josa...@chromium.org
Given Kevin is ooo, could you please help us checking this out Josafat?
Project Member

Comment 14 by sheriffbot@chromium.org, May 18 2018

Labels: -Merge-Request-67 Merge-Review-67 Hotlist-Merge-Review
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
Labels: -Hotlist-Merge-Review -Merge-Review-67 Merge-Approved-67
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. 
yes, let's get this in, we are getting several feedback reports on it 
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.
The snippet above is taken from M67 chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle.cc

Project Member

Comment 19 by bugdroid1@chromium.org, May 18 2018

Labels: -merge-approved-67 merge-merged-3396
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

Status: Fixed (was: Started)
For the record 	Chrome 67.0.3396.52 has the change

Sign in to add a comment