Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2458073003:
Modifies how Arc's throttle handles redirections (Closed)
|
Created:
4 years, 1 month ago by djacobo_ Modified:
4 years, 1 month ago Reviewers:
Yusuke Sato 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 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionModifies how Arc's throttle handles redirections
Since we want to handle navigations going from https to http we need to
enforce having a Referrer(), we call it |starting_gurl_|. This will
allow us to know where the navigation started and fire up the intent
picker.
This also modifies the way we treat redirections within the same
navigation throttle object. Since we now enforce having a starting GURL
we are not discarding as many throttles via ShouldOverrideUrlLoading(),
which previously discarded a throttle just because it didn't contain a
referrer.
BUG=654941
TEST=Check the repro info on the bug, also manually tested with
youtube links.
Committed: https://crrev.com/04a713e72e35274ba7e3f219dbc60c670208b120
Cr-Commit-Position: refs/heads/master@{#428873}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Removing redirected_to_arc_ flag #
Total comments: 5
Patch Set 3 : Improving the WillRedirectRequest() code comments #
Total comments: 4
Patch Set 4 : Fixing unit tests #Patch Set 5 : Rebasing #
Messages
Total messages: 37 (25 generated)
The CQ bit was checked by djacobo@google.com 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...
Description was changed from ========== Modifies how Arc's throttle handles redirections Since we want to handle navigations going from https to http we need to enforce having a Referrer(), we call it |starting_gurl_|. This will allow us to know where the navigation started and fire up the intent picker. This also modifies the way we treat redirections within the same navigation throttle object. Since we now enforce having a starting GURL we are not discarding as many throttles via ShouldOverrideUrlLoading(), which previously discarded a throttle just because it didn't contain a referrer. BUG=654941 TEST=Check the repro info on the bug, also manually tested with youtube links. ========== to ========== Modifies how Arc's throttle handles redirections Since we want to handle navigations going from https to http we need to enforce having a Referrer(), we call it |starting_gurl_|. This will allow us to know where the navigation started and fire up the intent picker. This also modifies the way we treat redirections within the same navigation throttle object. Since we now enforce having a starting GURL we are not discarding as many throttles via ShouldOverrideUrlLoading(), which previously discarded a throttle just because it didn't contain a referrer. BUG=654941 TEST=Check the repro info on the bug, also manually tested with youtube links. ==========
djacobo@google.com changed reviewers: + yusukes@chromium.org
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2458073003/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle.cc (right): https://codereview.chromium.org/2458073003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle.cc:56: // Check the scheme for both |previous_url| and |current_url| since an Please update the unit test. https://codereview.chromium.org/2458073003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle.cc:129: starting_gurl_(GURL()), redundant initialization, please remove the line. https://codereview.chromium.org/2458073003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle.cc:164: return redirected_to_arc_ ? content::NavigationThrottle::CANCEL_AND_IGNORE Is this path really taken with redirected_to_arc_ == true? If it is, how? I'm asking because 'redirected_to_arc_ == true' means that CANCEL_AND_IGNORE should have already been called at line 349-350.
The CQ bit was checked by djacobo@google.com 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/2458073003/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle.cc (right): https://codereview.chromium.org/2458073003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle.cc:56: // Check the scheme for both |previous_url| and |current_url| since an On 2016/10/29 00:39:57, Yusuke Sato wrote: > Please update the unit test. Done. https://codereview.chromium.org/2458073003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle.cc:129: starting_gurl_(GURL()), On 2016/10/29 00:39:57, Yusuke Sato wrote: > redundant initialization, please remove the line. Done. https://codereview.chromium.org/2458073003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle.cc:164: return redirected_to_arc_ ? content::NavigationThrottle::CANCEL_AND_IGNORE On 2016/10/29 00:39:57, Yusuke Sato wrote: > Is this path really taken with redirected_to_arc_ == true? If it is, how? > > I'm asking because 'redirected_to_arc_ == true' means that CANCEL_AND_IGNORE > should have already been called at line 349-350. Thanks for catching this, I misunderstood what PROCEED does, the path redirect_to_arc_ == true will never be taken as CANCEL_AND_IGNORE already took place after selecting an app from the UI, so it's safe to always return PROCEED here. Done.
https://codereview.chromium.org/2458073003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle.cc (right): https://codereview.chromium.org/2458073003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle.cc:162: return content::NavigationThrottle::PROCEED; Thanks for removing the member variable. However, I guess I'm still a bit confused... When do we have to return PROCEED? What's wrong with NOTREACHED? Can you provide an example or two as a code comment (or as a CL description, whichever you prefer)? https://codereview.chromium.org/2458073003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle.h (right): https://codereview.chromium.org/2458073003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. Add BUG=b:31968481? This CL also fixes the bug, correct? https://codereview.chromium.org/2458073003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle_unittest.cc (right): https://codereview.chromium.org/2458073003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle_unittest.cc:67: // A navigation with neither an http nor https scheme cannot be overriden. Thanks!
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_...)
The CQ bit was checked by djacobo@google.com 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/2458073003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle.cc (right): https://codereview.chromium.org/2458073003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle.cc:162: return content::NavigationThrottle::PROCEED; On 2016/10/31 20:26:51, Yusuke Sato wrote: > Thanks for removing the member variable. However, I guess I'm still a bit > confused... When do we have to return PROCEED? What's wrong with NOTREACHED? Can > you provide an example or two as a code comment (or as a CL description, > whichever you prefer)? Hopefully this is better :) https://codereview.chromium.org/2458073003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle.h (right): https://codereview.chromium.org/2458073003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/10/31 20:26:51, Yusuke Sato wrote: > Add BUG=b:31968481? This CL also fixes the bug, correct? The solution to the bug you mention doesn't depend on this, I would prefer to keep them independent from each other.
thanks, lgtm with unittest fixes. https://codereview.chromium.org/2458073003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle.cc (right): https://codereview.chromium.org/2458073003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle.cc:170: // 1) Was caught via WillStartRequest() and 2) and 3) are caught via was (lower case) https://codereview.chromium.org/2458073003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle_unittest.cc (right): https://codereview.chromium.org/2458073003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle_unittest.cc:68: EXPECT_FALSE(GURL("chrome-extension://fake_document"), ArcNavigationThrottle::ShouldOverrideUrlLoadingForTesting( is missing (here and elsewhere).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_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 djacobo@google.com 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...
Built and executed unit tests locally (courage--) https://codereview.chromium.org/2458073003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle.cc (right): https://codereview.chromium.org/2458073003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle.cc:170: // 1) Was caught via WillStartRequest() and 2) and 3) are caught via On 2016/10/31 21:27:09, Yusuke Sato wrote: > was (lower case) Done. https://codereview.chromium.org/2458073003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle_unittest.cc (right): https://codereview.chromium.org/2458073003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle_unittest.cc:68: EXPECT_FALSE(GURL("chrome-extension://fake_document"), On 2016/10/31 21:27:09, Yusuke Sato wrote: > ArcNavigationThrottle::ShouldOverrideUrlLoadingForTesting( > > is missing (here and elsewhere). Done.
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 djacobo@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from yusukes@chromium.org Link to the patchset: https://codereview.chromium.org/2458073003/#ps60001 (title: "Fixing unit tests")
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...)
The CQ bit was checked by djacobo@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from yusukes@chromium.org Link to the patchset: https://codereview.chromium.org/2458073003/#ps80001 (title: "Rebasing")
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.
Description was changed from ========== Modifies how Arc's throttle handles redirections Since we want to handle navigations going from https to http we need to enforce having a Referrer(), we call it |starting_gurl_|. This will allow us to know where the navigation started and fire up the intent picker. This also modifies the way we treat redirections within the same navigation throttle object. Since we now enforce having a starting GURL we are not discarding as many throttles via ShouldOverrideUrlLoading(), which previously discarded a throttle just because it didn't contain a referrer. BUG=654941 TEST=Check the repro info on the bug, also manually tested with youtube links. ========== to ========== Modifies how Arc's throttle handles redirections Since we want to handle navigations going from https to http we need to enforce having a Referrer(), we call it |starting_gurl_|. This will allow us to know where the navigation started and fire up the intent picker. This also modifies the way we treat redirections within the same navigation throttle object. Since we now enforce having a starting GURL we are not discarding as many throttles via ShouldOverrideUrlLoading(), which previously discarded a throttle just because it didn't contain a referrer. BUG=654941 TEST=Check the repro info on the bug, also manually tested with youtube links. ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Modifies how Arc's throttle handles redirections Since we want to handle navigations going from https to http we need to enforce having a Referrer(), we call it |starting_gurl_|. This will allow us to know where the navigation started and fire up the intent picker. This also modifies the way we treat redirections within the same navigation throttle object. Since we now enforce having a starting GURL we are not discarding as many throttles via ShouldOverrideUrlLoading(), which previously discarded a throttle just because it didn't contain a referrer. BUG=654941 TEST=Check the repro info on the bug, also manually tested with youtube links. ========== to ========== Modifies how Arc's throttle handles redirections Since we want to handle navigations going from https to http we need to enforce having a Referrer(), we call it |starting_gurl_|. This will allow us to know where the navigation started and fire up the intent picker. This also modifies the way we treat redirections within the same navigation throttle object. Since we now enforce having a starting GURL we are not discarding as many throttles via ShouldOverrideUrlLoading(), which previously discarded a throttle just because it didn't contain a referrer. BUG=654941 TEST=Check the repro info on the bug, also manually tested with youtube links. Committed: https://crrev.com/04a713e72e35274ba7e3f219dbc60c670208b120 Cr-Commit-Position: refs/heads/master@{#428873} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/04a713e72e35274ba7e3f219dbc60c670208b120 Cr-Commit-Position: refs/heads/master@{#428873} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
