Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2113123002:
Filters out URL clicks based on host matching only. (Closed)
|
Created:
4 years, 5 months ago by malaykeshav Modified:
4 years, 5 months ago Reviewers:
Yusuke Sato CC:
chromium-reviews, davemoore+watch_chromium.org, elijahtaylor+arcwatch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, oshima+watch_chromium.org, yusukes+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFilters out URL clicks based on host matching only.
This is a very basic implementation of clank's
ExternalNavigationHandler.ShouldOverrideUrlLoadingInternal().
Currently it only matches the referrer's host name to the host name of
the target URL.
COMPONENT=ChromeOS, Arc
BUG=624950
Committed: https://crrev.com/37970403bf95114d1c6659e618be8e43f4e343cd
Cr-Commit-Position: refs/heads/master@{#403586}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Filters out URL clicks based on host matching only. #
Messages
Total messages: 20 (7 generated)
The CQ bit was checked by malaykeshav@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.
malaykeshav@chromium.org changed reviewers: + yusukes@chromium.org
Thanks for working on this! https://codereview.chromium.org/2113123002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_navigation_throttle.cc (right): https://codereview.chromium.org/2113123002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_navigation_throttle.cc:213: bool ArcNavigationThrottle::ShouldOverrideUrlLoading() { Is it possible to add unit tests for this? https://codereview.chromium.org/2113123002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_navigation_throttle.cc:216: if (current_url == previous_url) I think this function should also return false when e.g. |previous_url| is http://a.foo.com/ and |current_url| is http://b.foo.com/, but the current logic does not catch such cases. I'm not really familiar with net/ but you could probably use SameDomainOrHost() in net/base/registry_controlled_domains/registry_controlled_domain.h to support that. net/base/registry_controlled_domains/registry_controlled_domain_unittest.cc shows how the utility function works. https://codereview.chromium.org/2113123002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_navigation_throttle.h (right): https://codereview.chromium.org/2113123002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_navigation_throttle.h:63: bool ShouldOverrideUrlLoading(); Can you document the function?
cc: djacobo@ FYI
https://codereview.chromium.org/2113123002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_navigation_throttle.cc (right): https://codereview.chromium.org/2113123002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_navigation_throttle.cc:213: bool ArcNavigationThrottle::ShouldOverrideUrlLoading() { On 2016/07/01 at 14:05:24, Yusuke Sato wrote: > Is it possible to add unit tests for this? Can be made unit testable. Just need to pass navigation_handle as argument. https://codereview.chromium.org/2113123002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_navigation_throttle.cc:216: if (current_url == previous_url) On 2016/07/01 at 14:05:24, Yusuke Sato wrote: > I think this function should also return false when e.g. |previous_url| is http://a.foo.com/ and |current_url| is http://b.foo.com/, but the current logic does not catch such cases. > If we are going from mail.google.com to maps.google.com, we are not showing the intent picker? > I'm not really familiar with net/ but you could probably use SameDomainOrHost() in net/base/registry_controlled_domains/registry_controlled_domain.h to support that. net/base/registry_controlled_domains/registry_controlled_domain_unittest.cc shows how the utility function works. Done https://codereview.chromium.org/2113123002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_navigation_throttle.h (right): https://codereview.chromium.org/2113123002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_navigation_throttle.h:63: bool ShouldOverrideUrlLoading(); On 2016/07/01 at 14:05:24, Yusuke Sato wrote: > Can you document the function? Done
lgtm https://codereview.chromium.org/2113123002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_navigation_throttle.cc (right): https://codereview.chromium.org/2113123002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_navigation_throttle.cc:216: if (current_url == previous_url) On 2016/07/01 23:20:41, malaykeshav wrote: > On 2016/07/01 at 14:05:24, Yusuke Sato wrote: > > I think this function should also return false when e.g. |previous_url| is > http://a.foo.com/ and |current_url| is http://b.foo.com/, but the current logic > does not catch such cases. > > > If we are going from http://mail.google.com to http://maps.google.com, we are not showing the > intent picker? The original Clank code seems to properly handle such a case by comparing handlers for mail.google.com and ones for maps.google.com (they usually differ, which triggers the picker UI). But in our case, getting handlers for a URL requires a slow mojo call which we don't want to use in this function... I asked to use the net:: function because showing the UI too often is worse than not showing it IMO. > > > I'm not really familiar with net/ but you could probably use > SameDomainOrHost() in > net/base/registry_controlled_domains/registry_controlled_domain.h to support > that. > net/base/registry_controlled_domains/registry_controlled_domain_unittest.cc > shows how the utility function works. > Done
The CQ bit was checked by malaykeshav@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 malaykeshav@chromium.org
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 #2 (id:20001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Filters out URL clicks based on host matching only. This is a very basic implementation of clank's ExternalNavigationHandler.ShouldOverrideUrlLoadingInternal(). Currently it only matches the referrer's host name to the host name of the target URL. COMPONENT=ChromeOS, Arc BUG=624950 ========== to ========== Filters out URL clicks based on host matching only. This is a very basic implementation of clank's ExternalNavigationHandler.ShouldOverrideUrlLoadingInternal(). Currently it only matches the referrer's host name to the host name of the target URL. COMPONENT=ChromeOS, Arc BUG=624950 Committed: https://crrev.com/37970403bf95114d1c6659e618be8e43f4e343cd Cr-Commit-Position: refs/heads/master@{#403586} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/37970403bf95114d1c6659e618be8e43f4e343cd Cr-Commit-Position: refs/heads/master@{#403586} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
