Create TestNavigationThrottle |
||||
Issue descriptionAt [1], nasko@ asked me to modify CancellingNavigationThrottle to include the functionality I needed (listening to see if WillFailRequest was called). That functionality already exists in NavigationThrottleCallbackRunner, a private class in navigation_simulator.cc [2] which observes Will* calls in order to count them. In addition, there's a TestNavigationThrottle navigation_handle_impl_unittest.cc that counts them directly. I propose to unify these by renaming CancellingNavigationThrottle to TestNavigationThrottle and: - Exposing functions that return how often each Will* method was called. - Adding a constructor parameter to specify a full NavigationThrottleResult to be used instead of the default CANCEL. (This would make one of my tests a bit more robust.) nasko@, does that sounds reasonable to you? [1] https://chromium-review.googlesource.com/c/chromium/src/+/621873/38/content/test/navigation_simulator_unittest.cc#30 [2] https://cs.chromium.org/chromium/src/content/public/test/navigation_simulator.cc?l=30&rcl=3a4f36179baee9b37d8132b7700b0f1699b40841 [3] https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_handle_impl_unittest.cc?l=42&rcl=c52283dc37346ca5febb569f2f52b0628e45d77e
,
Sep 29 2017
This definitely sounds good and will eliminate multiple subclasses of NavigationThrottle to unify them. Moving the code to a common location inside content/ and converting current test to use those sgtm!
,
Oct 4 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/79e1a976180ae82598448329db910238d31a7635 commit 79e1a976180ae82598448329db910238d31a7635 Author: Lucas Garron <lgarron@chromium.org> Date: Wed Oct 04 22:25:06 2017 Generalize CancellingNavigationThrottle to a new TestNavigationThrottle class. Bug: 770292 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation Change-Id: I1f3cb8d0d38503326b37adcebf3dee17856c82d4 Reviewed-on: https://chromium-review.googlesource.com/693376 Commit-Queue: Lucas Garron <lgarron@chromium.org> Reviewed-by: Charlie Harrison <csharrison@chromium.org> Reviewed-by: Nasko Oskov <nasko@chromium.org> Cr-Commit-Position: refs/heads/master@{#506542} [modify] https://crrev.com/79e1a976180ae82598448329db910238d31a7635/chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_unittest.cc [modify] https://crrev.com/79e1a976180ae82598448329db910238d31a7635/components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc [modify] https://crrev.com/79e1a976180ae82598448329db910238d31a7635/content/browser/frame_host/navigation_handle_impl_unittest.cc [delete] https://crrev.com/52b7477e019a2113213c942ab6dfbd8647668a6d/content/public/test/cancelling_navigation_throttle.cc [delete] https://crrev.com/52b7477e019a2113213c942ab6dfbd8647668a6d/content/public/test/cancelling_navigation_throttle.h [add] https://crrev.com/79e1a976180ae82598448329db910238d31a7635/content/public/test/test_navigation_throttle.cc [add] https://crrev.com/79e1a976180ae82598448329db910238d31a7635/content/public/test/test_navigation_throttle.h [modify] https://crrev.com/79e1a976180ae82598448329db910238d31a7635/content/test/BUILD.gn [modify] https://crrev.com/79e1a976180ae82598448329db910238d31a7635/content/test/navigation_simulator_unittest.cc
,
Oct 4 2017
,
Oct 4 2017
,
Oct 5 2017
Design doc that I didn't end up following (although the observer approach may become useful in the future): https://docs.google.com/document/d/1-phvRzNPxYO2MXD25elnOg38q2z1ph0FVoA9tq9pqrU/edit#
,
Oct 5 2017
|
||||
►
Sign in to add a comment |
||||
Comment 1 by csharrison@chromium.org
, Sep 29 2017