Intent picker ommibox's entry shows up on false positive scenarios |
|||||||||
Issue descriptionFrom M72+ on Cros devices intent picker's omnibox icon is over-triggering, potentially introduced due to IntentPickerAutoDisplayService. Besides the bad UI, this is registering way too many error entries on the feature's UMA metrics (ie. ChromeOS.Apps.IntentPickerAction) The root cause seems to be the package name of our Arc's side intent handling app to be taken as an app candidate, thus for every single navigation there could be app false app candidates. Marking as M72 since this is a regression as stated before, and we are proactively working on a fix.
,
Dec 19
This CL is available (and tested) in Cros 11425.0.0 I would like to cherry-pick it to M72, since it add tests and remove an ugly regression (introduced on say milestone). I think this is a low risk change since it only affects intent picker UI which is only available for ChromeOS and changes are minimal.
,
Dec 19
This bug requires manual review: M72 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: govind@(Android), kariahda@(iOS), djmm@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Dec 19
,
Dec 24
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible! If all merges have been completed, please remove any remaining Merge-Approved labels from this issue. Thanks for your time! To disable nags, add the Disable-Nags label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Dec 26
I will merge on Jan 2nd, thanks for the approval o/
,
Dec 28
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible! If all merges have been completed, please remove any remaining Merge-Approved labels from this issue. Thanks for your time! To disable nags, add the Disable-Nags label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jan 2
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/56d7acae23b50aebce3557b319ed895f086b71eb commit 56d7acae23b50aebce3557b319ed895f086b71eb Author: David Jacobo <djacobo@chromium.org> Date: Wed Jan 02 19:24:48 2019 Filter intent_helper apk in ArcIntentHelperBridge ArcIntentHelperBridge offers ShouldChromeHandleUrl(const GURL& url) as a way to early tell whether or not ARC has an installed app candidate that could deal with a passed |url|. Currently AppsNavigationThrottle uses this method as a pre-filter, done in Chrome-side whenever we wonder whether or not query ARC, given the async nature of this query, we need to double check that there are actual app candidates installed before showing saying so to the user via the intent picker's omnibox icon. This CL filters out intent_helper's apk package name as a candidate on ArcIntentHelperBridge, it also verifies that the async query actually return a valid installed app candidate before making changes to the omnibox. Bug: 914496 Test: Adding unit tests for this behavior, previous tests still pass. Change-Id: I2f3da1c6b6bdc67938b8413a7c06e7a51e9e8474 Reviewed-on: https://chromium-review.googlesource.com/c/1372921 Reviewed-by: Yusuke Sato <yusukes@chromium.org> Reviewed-by: Dominick Ng <dominickn@chromium.org> Commit-Queue: David Jacobo <djacobo@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#616206}(cherry picked from commit f11a0251c25afd6d8bf229425bedf0605bdd16d8) Reviewed-on: https://chromium-review.googlesource.com/c/1393428 Reviewed-by: David Jacobo <djacobo@chromium.org> Cr-Commit-Position: refs/branch-heads/3626@{#539} Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437} [modify] https://crrev.com/56d7acae23b50aebce3557b319ed895f086b71eb/chrome/browser/chromeos/apps/intent_helper/apps_navigation_throttle.cc [modify] https://crrev.com/56d7acae23b50aebce3557b319ed895f086b71eb/components/arc/intent_helper/arc_intent_helper_bridge.cc [modify] https://crrev.com/56d7acae23b50aebce3557b319ed895f086b71eb/components/arc/intent_helper/arc_intent_helper_bridge_unittest.cc
,
Jan 2
Here's a summary of the rules that were executed: - OnlyMergeApprovedChange: Rule Failed -- Revision 56d7acae23b50aebce3557b319ed895f086b71eb was merged to refs/branch-heads/3626 branch with no merge approval from a TPM! Please explain why this change was merged to the branch! - AcknowledgeMerge: Notification Required --
,
Jan 2
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/56d7acae23b50aebce3557b319ed895f086b71eb Commit: 56d7acae23b50aebce3557b319ed895f086b71eb Author: djacobo@chromium.org Commiter: djacobo@chromium.org Date: 2019-01-02 19:24:48 +0000 UTC Filter intent_helper apk in ArcIntentHelperBridge ArcIntentHelperBridge offers ShouldChromeHandleUrl(const GURL& url) as a way to early tell whether or not ARC has an installed app candidate that could deal with a passed |url|. Currently AppsNavigationThrottle uses this method as a pre-filter, done in Chrome-side whenever we wonder whether or not query ARC, given the async nature of this query, we need to double check that there are actual app candidates installed before showing saying so to the user via the intent picker's omnibox icon. This CL filters out intent_helper's apk package name as a candidate on ArcIntentHelperBridge, it also verifies that the async query actually return a valid installed app candidate before making changes to the omnibox. Bug: 914496 Test: Adding unit tests for this behavior, previous tests still pass. Change-Id: I2f3da1c6b6bdc67938b8413a7c06e7a51e9e8474 Reviewed-on: https://chromium-review.googlesource.com/c/1372921 Reviewed-by: Yusuke Sato <yusukes@chromium.org> Reviewed-by: Dominick Ng <dominickn@chromium.org> Commit-Queue: David Jacobo <djacobo@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#616206}(cherry picked from commit f11a0251c25afd6d8bf229425bedf0605bdd16d8) Reviewed-on: https://chromium-review.googlesource.com/c/1393428 Reviewed-by: David Jacobo <djacobo@chromium.org> Cr-Commit-Position: refs/branch-heads/3626@{#539} Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
,
Jan 2
dgagnon@ approved in comment#5, so TPM actually took a look at this.
,
Jan 4
Removing the tags for merging without approval as is not the case (see comment#5 above), thanks.
,
Jan 4
build 11316.60.0+ has the change, that's where I checked anyways o/ |
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by bugdroid1@chromium.org
, Dec 13