Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2194523002:
[arc-intents] Handle redirects. (Closed)
|
Created:
4 years, 4 months ago by Ben Kwa Modified:
4 years, 4 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, yusukes+watch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[arc-intents] Handle redirects.
- Move intent filtering (via LocalActivityResolver) into
ArcNavigationThrottle. Attach an ArcNavigationThrottle to all
navigations, don't pre-filter them. This ensures that the intent
filtering code sees all the URLs in the redirect chain (not just the
first URL).
- Modify the ArcNavigationThrottle so that it memoizes the action taken
by a user on an intent disambig. This avoids repeatedly popping up
the intent disambig on each link in a redirect
chain (e.g. www.some-site.com -> www.twitter.com -> twitter.com).
BUG=632428
Committed: https://crrev.com/5a9a30409316fd19af685b1a88ae21b3e51e71c4
Cr-Commit-Position: refs/heads/master@{#410155}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Apply review feedback. #
Total comments: 16
Patch Set 3 : Apply review feedback. #
Total comments: 3
Patch Set 4 : Move DCHECK thread checks. #Patch Set 5 : Sync/rebase. #
Messages
Total messages: 53 (30 generated)
kenobi@chromium.org changed reviewers: + djacobo@google.com, nasko@chromium.org
The CQ bit was checked by kenobi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2194523002/diff/1/chrome/browser/chrome_conte... File chrome/browser/chrome_content_browser_client.cc (left): https://codereview.chromium.org/2194523002/diff/1/chrome/browser/chrome_conte... chrome/browser/chrome_content_browser_client.cc:2944: scoped_refptr<arc::LocalActivityResolver> local_resolver = If we are not using the local_resolver in here, remove the local_activity_resolver.h from the includes https://codereview.chromium.org/2194523002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_navigation_throttle.cc (right): https://codereview.chromium.org/2194523002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_navigation_throttle.cc:62: previous_user_action_(CloseReason::SIZE) {} move before weak_ptr_factory pls. https://codereview.chromium.org/2194523002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_navigation_throttle.h (right): https://codereview.chromium.org/2194523002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_navigation_throttle.h:60: NavigationThrottle::ThrottleCheckResult HandleRequest(); nit: As HandleRequest() is not an override of content::Navigation, we should group this function with OnAppCandidatesReceived. https://codereview.chromium.org/2194523002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_navigation_throttle.h:80: CloseReason previous_user_action_; Put this before weak_ptr_factory_, builders are complaining because of this.
lhchavez@chromium.org changed reviewers: + lhchavez@chromium.org
https://codereview.chromium.org/2194523002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_navigation_throttle.h (right): https://codereview.chromium.org/2194523002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_navigation_throttle.h:80: CloseReason previous_user_action_; On 2016/07/28 20:08:01, djacobo wrote: > Put this before weak_ptr_factory_, builders are complaining because of this. While we're on it, please add a comment to |weak_ptr_factory_| mentioning that it must be the last member so that its destruction can invalidate any in-flight weak pointers.
https://codereview.chromium.org/2194523002/diff/1/chrome/browser/chrome_conte... File chrome/browser/chrome_content_browser_client.cc (left): https://codereview.chromium.org/2194523002/diff/1/chrome/browser/chrome_conte... chrome/browser/chrome_content_browser_client.cc:2944: scoped_refptr<arc::LocalActivityResolver> local_resolver = On 2016/07/28 20:08:01, djacobo wrote: > If we are not using the local_resolver in here, remove the > local_activity_resolver.h from the includes Done. https://codereview.chromium.org/2194523002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_navigation_throttle.h (right): https://codereview.chromium.org/2194523002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_navigation_throttle.h:60: NavigationThrottle::ThrottleCheckResult HandleRequest(); On 2016/07/28 20:08:01, djacobo wrote: > nit: As HandleRequest() is not an override of content::Navigation, we should > group this function with OnAppCandidatesReceived. Done. https://codereview.chromium.org/2194523002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_navigation_throttle.h:80: CloseReason previous_user_action_; On 2016/07/28 20:08:01, djacobo wrote: > Put this before weak_ptr_factory_, builders are complaining because of this. Done. https://codereview.chromium.org/2194523002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_navigation_throttle.h:80: CloseReason previous_user_action_; On 2016/07/28 20:12:14, Luis Héctor Chávez wrote: > On 2016/07/28 20:08:01, djacobo wrote: > > Put this before weak_ptr_factory_, builders are complaining because of this. > > While we're on it, please add a comment to |weak_ptr_factory_| mentioning that > it must be the last member so that its destruction can invalidate any in-flight > weak pointers. Done.
The CQ bit was checked by kenobi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2194523002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_navigation_throttle.cc (right): https://codereview.chromium.org/2194523002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_navigation_throttle.cc:61: previous_user_action_(CloseReason::SIZE), Orthogonal to this CL in general, but I find this confusing to read. What does size have to do with closing reason? Maybe rename in the future to some more meaningful value - Invalid/InitialState/NotClosed/etc. https://codereview.chromium.org/2194523002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_navigation_throttle.cc:84: // chrome should not see a redirect. nit: Chrome, as lowercase can mean other things. https://codereview.chromium.org/2194523002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_navigation_throttle.cc:88: // No disambig has previously been popped up for this - continue. nit: I find "disambig" to be a very strange abbreviation and not very readable, but it seems it is already present in a bunch of places in this file :(. https://codereview.chromium.org/2194523002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_navigation_throttle.cc:99: // Mask out any redirect qualifiers - this method handles navigation from nit: Empty line before the comment. https://codereview.chromium.org/2194523002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_navigation_throttle.cc:126: // If this navigation event doesn't hit an android app. nit: The sentence doesn't read as a complete one. Maybe something like "Proceed with the navigation, if it is not supposed to be handled by an external app." https://codereview.chromium.org/2194523002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_navigation_throttle.h (right): https://codereview.chromium.org/2194523002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_navigation_throttle.h:78: CloseReason previous_user_action_; nit: Please put a comment on what this variable is used for.
https://codereview.chromium.org/2194523002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_navigation_throttle.cc (right): https://codereview.chromium.org/2194523002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_navigation_throttle.cc:61: previous_user_action_(CloseReason::SIZE), On 2016/07/29 16:42:55, nasko wrote: > Orthogonal to this CL in general, but I find this confusing to read. What does > size have to do with closing reason? Maybe rename in the future to some more > meaningful value - Invalid/InitialState/NotClosed/etc. I aliased SIZE to INVALID and I'm using INVALID here instead. https://codereview.chromium.org/2194523002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_navigation_throttle.cc:84: // chrome should not see a redirect. On 2016/07/29 16:42:55, nasko wrote: > nit: Chrome, as lowercase can mean other things. Done. https://codereview.chromium.org/2194523002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_navigation_throttle.cc:88: // No disambig has previously been popped up for this - continue. On 2016/07/29 16:42:55, nasko wrote: > nit: I find "disambig" to be a very strange abbreviation and not very readable, > but it seems it is already present in a bunch of places in this file :(. Changed "DisambigDialog" to "IntentPicker" throughout. https://codereview.chromium.org/2194523002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_navigation_throttle.cc:99: // Mask out any redirect qualifiers - this method handles navigation from On 2016/07/29 16:42:55, nasko wrote: > nit: Empty line before the comment. Done. https://codereview.chromium.org/2194523002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_navigation_throttle.cc:126: // If this navigation event doesn't hit an android app. On 2016/07/29 16:42:55, nasko wrote: > nit: The sentence doesn't read as a complete one. Maybe something like "Proceed > with the navigation, if it is not supposed to be handled by an external app." Done. https://codereview.chromium.org/2194523002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_navigation_throttle.h (right): https://codereview.chromium.org/2194523002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_navigation_throttle.h:78: CloseReason previous_user_action_; On 2016/07/29 16:42:55, nasko wrote: > nit: Please put a comment on what this variable is used for. Done.
The CQ bit was checked by kenobi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
1 question, PTAL https://codereview.chromium.org/2194523002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_navigation_throttle.cc (right): https://codereview.chromium.org/2194523002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_navigation_throttle.cc:68: return HandleRequest(); question: Do we need to add DCHECK_CURRENTLY_ON(content::BrowserThread::UI); here and in WillRedirectRequest() ? We have it on HandleRequest() but I don't know if it suffices. https://codereview.chromium.org/2194523002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_navigation_throttle.h (right): https://codereview.chromium.org/2194523002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_navigation_throttle.h:48: using ShowIntentPickerCallback = Thanks for renaming here and all the repetitions, it's clearer now.
The CQ bit was checked by kenobi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2194523002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_navigation_throttle.cc (right): https://codereview.chromium.org/2194523002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_navigation_throttle.cc:68: return HandleRequest(); On 2016/07/29 20:23:17, djacobo wrote: > question: Do we need to add DCHECK_CURRENTLY_ON(content::BrowserThread::UI); > > here and in WillRedirectRequest() ? We have it on HandleRequest() but I don't > know if it suffices. I moved the DCHECK thread checks back into WillHandleRequest and WillRedirectRequest.
lgtm lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2194523002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_navigation_throttle.cc (right): https://codereview.chromium.org/2194523002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_navigation_throttle.cc:61: previous_user_action_(CloseReason::SIZE), On 2016/07/29 18:29:12, Ben Kwa wrote: > On 2016/07/29 16:42:55, nasko wrote: > > Orthogonal to this CL in general, but I find this confusing to read. What does > > size have to do with closing reason? Maybe rename in the future to some more > > meaningful value - Invalid/InitialState/NotClosed/etc. > > I aliased SIZE to INVALID and I'm using INVALID here instead. Why not just say INVALID as the last one? Why do we need to have a SIZE? https://codereview.chromium.org/2194523002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_navigation_throttle.cc:88: // No disambig has previously been popped up for this - continue. On 2016/07/29 18:29:12, Ben Kwa wrote: > On 2016/07/29 16:42:55, nasko wrote: > > nit: I find "disambig" to be a very strange abbreviation and not very > readable, > > but it seems it is already present in a bunch of places in this file :(. > > Changed "DisambigDialog" to "IntentPicker" throughout. Awesome! A lot more clear and readable this way!
https://codereview.chromium.org/2194523002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_navigation_throttle.cc (right): https://codereview.chromium.org/2194523002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_navigation_throttle.cc:61: previous_user_action_(CloseReason::SIZE), On 2016/08/01 23:13:53, nasko wrote: > On 2016/07/29 18:29:12, Ben Kwa wrote: > > On 2016/07/29 16:42:55, nasko wrote: > > > Orthogonal to this CL in general, but I find this confusing to read. What > does > > > size have to do with closing reason? Maybe rename in the future to some more > > > meaningful value - Invalid/InitialState/NotClosed/etc. > > > > I aliased SIZE to INVALID and I'm using INVALID here instead. > > Why not just say INVALID as the last one? Why do we need to have a SIZE? There is a UMA call below that takes the max value of the enum as one of its arguments, I'm assuming that SIZE is more readable in that context, so I kept it around.
Code looks good. However, let's resolve the design question first - should we be firing intents on redirects or not. https://codereview.chromium.org/2194523002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_navigation_throttle.cc (right): https://codereview.chromium.org/2194523002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_navigation_throttle.cc:61: previous_user_action_(CloseReason::SIZE), On 2016/08/01 23:55:08, Ben Kwa wrote: > On 2016/08/01 23:13:53, nasko wrote: > > On 2016/07/29 18:29:12, Ben Kwa wrote: > > > On 2016/07/29 16:42:55, nasko wrote: > > > > Orthogonal to this CL in general, but I find this confusing to read. What > > does > > > > size have to do with closing reason? Maybe rename in the future to some > more > > > > meaningful value - Invalid/InitialState/NotClosed/etc. > > > > > > I aliased SIZE to INVALID and I'm using INVALID here instead. > > > > Why not just say INVALID as the last one? Why do we need to have a SIZE? > > There is a UMA call below that takes the max value of the enum as one of its > arguments, I'm assuming that SIZE is more readable in that context, so I kept it > around. Acknowledged.
On 2016/08/03 16:36:50, nasko (slow) wrote: > Code looks good. However, let's resolve the design question first - should we be > firing intents on redirects or not. Ok. I was wrong originally - clank actually *does* fire intents on redirects. Apologies for the misinformation. The confusion arose because clank appears to have its own internal logic governing whether or not to even fire an intent, and that logic seems to do a bit of probing and weighting of the match responses. TLDR: I had to write a custom app and play with intent filters for a while to get it to work, but in the end I was able to verify that navigating to http://www.twitter.com will redirect to http://mobile.twitter.com, and that redirection does, in fact, get sent out as a VIEW intent if the right app exists on the device to handle it.
LGTM
The CQ bit was checked by kenobi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
kenobi@chromium.org changed reviewers: + yusukes@chromium.org
Moving yusukes@ to +R because lhchavez is out.
The CQ bit was checked by yusukes@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by kenobi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by kenobi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yusukes@chromium.org, nasko@chromium.org, djacobo@google.com Link to the patchset: https://codereview.chromium.org/2194523002/#ps80001 (title: "Sync/rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [arc-intents] Handle redirects. - Move intent filtering (via LocalActivityResolver) into ArcNavigationThrottle. Attach an ArcNavigationThrottle to all navigations, don't pre-filter them. This ensures that the intent filtering code sees all the URLs in the redirect chain (not just the first URL). - Modify the ArcNavigationThrottle so that it memoizes the action taken by a user on an intent disambig. This avoids repeatedly popping up the intent disambig on each link in a redirect chain (e.g. www.some-site.com -> www.twitter.com -> twitter.com). BUG=632428 ========== to ========== [arc-intents] Handle redirects. - Move intent filtering (via LocalActivityResolver) into ArcNavigationThrottle. Attach an ArcNavigationThrottle to all navigations, don't pre-filter them. This ensures that the intent filtering code sees all the URLs in the redirect chain (not just the first URL). - Modify the ArcNavigationThrottle so that it memoizes the action taken by a user on an intent disambig. This avoids repeatedly popping up the intent disambig on each link in a redirect chain (e.g. www.some-site.com -> www.twitter.com -> twitter.com). BUG=632428 Committed: https://crrev.com/5a9a30409316fd19af685b1a88ae21b3e51e71c4 Cr-Commit-Position: refs/heads/master@{#410155} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/5a9a30409316fd19af685b1a88ae21b3e51e71c4 Cr-Commit-Position: refs/heads/master@{#410155} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
