Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2763613002:
Add DelayNavigationThrottle (Closed)
|
Created:
3 years, 9 months ago by Bryan McQuade Modified:
3 years, 9 months ago CC:
chromium-reviews, loading-reviews_chromium.org, Randy Smith (Not in Mondays) Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd DelayNavigationThrottle
DelayNavigationThrottle delays all HTTP/HTTPS main frame navigations
by an amount specified in a field trial parameter.
BUG=704117
Review-Url: https://codereview.chromium.org/2763613002
Cr-Commit-Position: refs/heads/master@{#459575}
Committed: https://chromium.googlesource.com/chromium/src/+/b455655aabeb57f21c947ed8cdaf4761d5d20fd8
Patch Set 1 #Patch Set 2 : cleanup #Patch Set 3 : clean up comments #Patch Set 4 : attempt linux fix #Patch Set 5 : add tests #Patch Set 6 : cleanup #Patch Set 7 : cleanup #Patch Set 8 : add about:flags support #Patch Set 9 : fix about flags test #Patch Set 10 : rebase #Patch Set 11 : rebase #
Total comments: 12
Patch Set 12 : address some comments #
Total comments: 1
Patch Set 13 : port to parameterized test #Patch Set 14 : cleanup #Patch Set 15 : move to page_load_metrics #Patch Set 16 : fix tests #
Total comments: 18
Patch Set 17 : address comments #Patch Set 18 : reorder tests #Patch Set 19 : add support for randomized delay #
Total comments: 2
Patch Set 20 : add dchecks #Messages
Total messages: 103 (74 generated)
The CQ bit was checked by bmcquade@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 checked by bmcquade@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 checked by bmcquade@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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by bmcquade@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 checked by bmcquade@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...
Description was changed from ========== Add DelayNavigationThrottle, which delays all navigations. BUG= ========== to ========== Add DelayNavigationThrottle DelayNavigationThrottle delays all HTTP/HTTPS main frame navigations by an amount specified in a field trial parameter. BUG= ==========
The CQ bit was checked by bmcquade@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_...)
The CQ bit was checked by bmcquade@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 checked by bmcquade@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_...) linux_chromium_tsan_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 bmcquade@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: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by bmcquade@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_...)
Description was changed from ========== Add DelayNavigationThrottle DelayNavigationThrottle delays all HTTP/HTTPS main frame navigations by an amount specified in a field trial parameter. BUG= ========== to ========== Add DelayNavigationThrottle DelayNavigationThrottle delays all HTTP/HTTPS main frame navigations by an amount specified in a field trial parameter. BUG=704117 ==========
The CQ bit was checked by bmcquade@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...
bmcquade@chromium.org changed reviewers: + csharrison@chromium.org
PTAL, thanks!
On 2017/03/22 14:43:06, Bryan McQuade wrote: > PTAL, thanks! Driveby comment: Is chrome/b/l really the right place for NavigationThrottles? Seems like when/if we mojo-ify net, most of the corresponding content/browser/loader code will either be in a separate process, or refactored completely and in some other location.
On 2017/03/22 at 15:08:36, mmenke wrote: > On 2017/03/22 14:43:06, Bryan McQuade wrote: > > PTAL, thanks! > > Driveby comment: Is chrome/b/l really the right place for NavigationThrottles? Seems like when/if we mojo-ify net, most of the corresponding content/browser/loader code will either be in a separate process, or refactored completely and in some other location. I'm open to moving it, though I don't see a better location in chrome/browser/. csharrison was also supportive of putting this in loader. Do you have a suggestion for a better location for this class?
A few comments. mmenke: Do you have a suggestion where this throttle should live? Renderer_host? https://codereview.chromium.org/2763613002/diff/200001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2763613002/diff/200001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:3438: if (handle->IsInMainFrame()) { optional: Since you already have a MaybeCreate* method, can move the IsInMainFrame check there, so the throttle decides the policy. https://codereview.chromium.org/2763613002/diff/200001/chrome/browser/loader/... File chrome/browser/loader/delay_navigation_throttle.cc (right): https://codereview.chromium.org/2763613002/diff/200001/chrome/browser/loader/... chrome/browser/loader/delay_navigation_throttle.cc:12: #include "content/public/browser/browser_thread.h" Is it used? https://codereview.chromium.org/2763613002/diff/200001/chrome/browser/loader/... chrome/browser/loader/delay_navigation_throttle.cc:32: search::IsInstantNTP(handle->GetWebContents())) { Is this correct? IsInstantNTP looks at the visible navigation entry. Is this the proper NTP URL before navigation commit? https://codereview.chromium.org/2763613002/diff/200001/chrome/browser/loader/... chrome/browser/loader/delay_navigation_throttle.cc:36: if (!base::FeatureList::IsEnabled(kDelayNavigationFeature)) Let's move this to be the first check. IsInstantNTP looks like a pretty reasonably expensive function, with GURL allocations and multiple map lookups. https://codereview.chromium.org/2763613002/diff/200001/chrome/browser/loader/... chrome/browser/loader/delay_navigation_throttle.cc:53: return base::WrapUnique(new DelayNavigationThrottle( nit: base::MakeUnique is preferred to avoid bare "new". https://codereview.chromium.org/2763613002/diff/200001/chrome/browser/loader/... File chrome/browser/loader/delay_navigation_throttle_unittest.cc (right): https://codereview.chromium.org/2763613002/diff/200001/chrome/browser/loader/... chrome/browser/loader/delay_navigation_throttle_unittest.cc:17: class ScopedDelayNavigationFeatureParams { optional: Since this is a fairly simple feature, a simpler alternative would be to use a parameterized test harness, and override SetUpCommandLine to set the feature flags.
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 bmcquade@chromium.org to run a CQ dry run
thx! will switch to a parameterized test harness shortly, but have addressed the other comments. https://codereview.chromium.org/2763613002/diff/200001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2763613002/diff/200001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:3438: if (handle->IsInMainFrame()) { On 2017/03/22 at 15:30:12, Charlie Harrison wrote: > optional: Since you already have a MaybeCreate* method, can move the IsInMainFrame check there, so the throttle decides the policy. sure, good idea, done. https://codereview.chromium.org/2763613002/diff/200001/chrome/browser/loader/... File chrome/browser/loader/delay_navigation_throttle.cc (right): https://codereview.chromium.org/2763613002/diff/200001/chrome/browser/loader/... chrome/browser/loader/delay_navigation_throttle.cc:12: #include "content/public/browser/browser_thread.h" On 2017/03/22 at 15:30:12, Charlie Harrison wrote: > Is it used? nope - removed, thanks! https://codereview.chromium.org/2763613002/diff/200001/chrome/browser/loader/... chrome/browser/loader/delay_navigation_throttle.cc:32: search::IsInstantNTP(handle->GetWebContents())) { On 2017/03/22 at 15:30:12, Charlie Harrison wrote: > Is this correct? IsInstantNTP looks at the visible navigation entry. Is this the proper NTP URL before navigation commit? interesting, thanks for digging in. it does seem to have the intended behavior, but given the comment, i probably should not rely on it. instead, i've changed it to compare on the URL. https://codereview.chromium.org/2763613002/diff/200001/chrome/browser/loader/... chrome/browser/loader/delay_navigation_throttle.cc:36: if (!base::FeatureList::IsEnabled(kDelayNavigationFeature)) On 2017/03/22 at 15:30:12, Charlie Harrison wrote: > Let's move this to be the first check. IsInstantNTP looks like a pretty reasonably expensive function, with GURL allocations and multiple map lookups. done https://codereview.chromium.org/2763613002/diff/200001/chrome/browser/loader/... chrome/browser/loader/delay_navigation_throttle.cc:53: return base::WrapUnique(new DelayNavigationThrottle( On 2017/03/22 at 15:30:12, Charlie Harrison wrote: > nit: base::MakeUnique is preferred to avoid bare "new". done
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
One more thing https://codereview.chromium.org/2763613002/diff/220001/chrome/browser/about_f... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2763613002/diff/220001/chrome/browser/about_f... chrome/browser/about_flags.cc:768: {DelayNavigationThrottle::kParamDelayNavigationDurationMillis, "5000"}, 5s seems extreme. Is that going to be an experiment group? I think a 1s delay is much approachable if we want people to try this out.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/03/22 at 18:06:45, csharrison wrote: > One more thing > > https://codereview.chromium.org/2763613002/diff/220001/chrome/browser/about_f... > File chrome/browser/about_flags.cc (right): > > https://codereview.chromium.org/2763613002/diff/220001/chrome/browser/about_f... > chrome/browser/about_flags.cc:768: {DelayNavigationThrottle::kParamDelayNavigationDurationMillis, "5000"}, > 5s seems extreme. Is that going to be an experiment group? I think a 1s delay is much approachable if we want people to try this out. Yes, absolutely, no plans to ever add a 5 second delay in a real experiment. This was just intended to allow people to try it and be really certain that it was working. I started with a 1s delay here, but found that it was sometimes difficult to tell if it was working.
The CQ bit was checked by bmcquade@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 checked by bmcquade@chromium.org to run a CQ dry run
PTAL, thanks! https://codereview.chromium.org/2763613002/diff/200001/chrome/browser/loader/... File chrome/browser/loader/delay_navigation_throttle_unittest.cc (right): https://codereview.chromium.org/2763613002/diff/200001/chrome/browser/loader/... chrome/browser/loader/delay_navigation_throttle_unittest.cc:17: class ScopedDelayNavigationFeatureParams { On 2017/03/22 at 15:30:12, Charlie Harrison wrote: > optional: Since this is a fairly simple feature, a simpler alternative would be to use a parameterized test harness, and override SetUpCommandLine to set the feature flags. Sure, I ported to a parameterized harness. I kept the ScopedFeatureList, but was able to make it a member of the test harness class.
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_...)
I will review the tests tomorrow, but just looking through the code I think we ought to move this to chrome/browser/renderer_host. This is the closest thing in chrome/browser to frame_host, where NavigationHandle lives. Additionally, it is also where ChromeNavigationUIData lives.
On 2017/03/22 at 23:54:35, csharrison wrote: > I will review the tests tomorrow, but just looking through the code I think we ought to move this to chrome/browser/renderer_host. > > This is the closest thing in chrome/browser to frame_host, where NavigationHandle lives. Additionally, it is also where ChromeNavigationUIData lives. Ok. Can you give a little more reason why this is better homed in renderer_host? Looking at what's in that directory, I see: chrome_extension_message_filter chrome_navigation_ui_data chrome_render_message_filter chrome_render_widget_host_view_mac_delegate chrome_render_widget_host_view_mac_history_swiper I'm struggling to see how these are related to DelayNavigationThrottle. How is ChromeNavigationUIData related? I'm happy to move it, but my concern is that we move it, I find a new reviewer who's an owner there, and they push back on homing it in renderer_host because it's not well related to anything else in that directory. I'd like to have some confidence that this belongs in renderer_host before we move it. I'm currently a bit skeptical.
On 2017/03/23 00:57:51, Bryan McQuade wrote: > On 2017/03/22 at 23:54:35, csharrison wrote: > > I will review the tests tomorrow, but just looking through the code I think we > ought to move this to chrome/browser/renderer_host. > > > > This is the closest thing in chrome/browser to frame_host, where > NavigationHandle lives. Additionally, it is also where ChromeNavigationUIData > lives. > > Ok. Can you give a little more reason why this is better homed in renderer_host? > > Looking at what's in that directory, I see: > chrome_extension_message_filter > chrome_navigation_ui_data > chrome_render_message_filter > chrome_render_widget_host_view_mac_delegate > chrome_render_widget_host_view_mac_history_swiper > > I'm struggling to see how these are related to DelayNavigationThrottle. How is > ChromeNavigationUIData related? It is a Chrome level navigation piece of code, like this throttle. It's worth noting that chrome/browser/loader is a new directory. It all came from renderer_host, which is why I considered it. Let's just punt on this decision for now, because I'm not positive renderer_host is right :/ > > I'm happy to move it, but my concern is that we move it, I find a new reviewer > who's an owner there, and they push back on homing it in renderer_host because > it's not well related to anything else in that directory. I'd like to have some > confidence that this belongs in renderer_host before we move it. I'm currently a > bit skeptical.
On 2017/03/23 at 01:04:31, csharrison wrote: > On 2017/03/23 00:57:51, Bryan McQuade wrote: > > On 2017/03/22 at 23:54:35, csharrison wrote: > > > I will review the tests tomorrow, but just looking through the code I think we > > ought to move this to chrome/browser/renderer_host. > > > > > > This is the closest thing in chrome/browser to frame_host, where > > NavigationHandle lives. Additionally, it is also where ChromeNavigationUIData > > lives. > > > > Ok. Can you give a little more reason why this is better homed in renderer_host? > > > > Looking at what's in that directory, I see: > > chrome_extension_message_filter > > chrome_navigation_ui_data > > chrome_render_message_filter > > chrome_render_widget_host_view_mac_delegate > > chrome_render_widget_host_view_mac_history_swiper > > > > I'm struggling to see how these are related to DelayNavigationThrottle. How is > > ChromeNavigationUIData related? > It is a Chrome level navigation piece of code, like this throttle. It's worth noting that chrome/browser/loader is a new directory. It all came from renderer_host, which is why I considered it. > > Let's just punt on this decision for now, because I'm not positive renderer_host is right :/ Sounds good. I'll start a thread with jochen to drive clarity on where this belongs. > > > > I'm happy to move it, but my concern is that we move it, I find a new reviewer > > who's an owner there, and they push back on homing it in renderer_host because > > it's not well related to anything else in that directory. I'd like to have some > > confidence that this belongs in renderer_host before we move it. I'm currently a > > bit skeptical.
The CQ bit was checked by bmcquade@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...
On 2017/03/23 at 01:08:40, Bryan McQuade wrote: > On 2017/03/23 at 01:04:31, csharrison wrote: > > On 2017/03/23 00:57:51, Bryan McQuade wrote: > > > On 2017/03/22 at 23:54:35, csharrison wrote: > > > > I will review the tests tomorrow, but just looking through the code I think we > > > ought to move this to chrome/browser/renderer_host. > > > > > > > > This is the closest thing in chrome/browser to frame_host, where > > > NavigationHandle lives. Additionally, it is also where ChromeNavigationUIData > > > lives. > > > > > > Ok. Can you give a little more reason why this is better homed in renderer_host? > > > > > > Looking at what's in that directory, I see: > > > chrome_extension_message_filter > > > chrome_navigation_ui_data > > > chrome_render_message_filter > > > chrome_render_widget_host_view_mac_delegate > > > chrome_render_widget_host_view_mac_history_swiper > > > > > > I'm struggling to see how these are related to DelayNavigationThrottle. How is > > > ChromeNavigationUIData related? > > It is a Chrome level navigation piece of code, like this throttle. It's worth noting that chrome/browser/loader is a new directory. It all came from renderer_host, which is why I considered it. > > > > Let's just punt on this decision for now, because I'm not positive renderer_host is right :/ > > Sounds good. I'll start a thread with jochen to drive clarity on where this belongs. > > > > > > > I'm happy to move it, but my concern is that we move it, I find a new reviewer > > > who's an owner there, and they push back on homing it in renderer_host because > > > it's not well related to anything else in that directory. I'd like to have some > > > confidence that this belongs in renderer_host before we move it. I'm currently a > > > bit skeptical. I've moved the code to plm, per our discussion. PTAL, thanks!
The CQ bit was checked by bmcquade@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: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
LGTM % nits https://codereview.chromium.org/2763613002/diff/300001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/experiments/delay_navigation_throttle_unittest.cc (right): https://codereview.chromium.org/2763613002/diff/300001/chrome/browser/page_lo... chrome/browser/page_load_metrics/experiments/delay_navigation_throttle_unittest.cc:26: base::TimeDelta navigation_delay = base::TimeDelta::FromSeconds(1); include base/time/time.h https://codereview.chromium.org/2763613002/diff/300001/chrome/browser/page_lo... chrome/browser/page_load_metrics/experiments/delay_navigation_throttle_unittest.cc:27: GURL url("http://www.example.com/"); include url/gurl.h https://codereview.chromium.org/2763613002/diff/300001/chrome/browser/page_lo... chrome/browser/page_load_metrics/experiments/delay_navigation_throttle_unittest.cc:29: scoped_refptr<base::TestMockTimeTaskRunner> mock_time_task_runner( include base/memory/ref_counted.h https://codereview.chromium.org/2763613002/diff/300001/chrome/browser/page_lo... chrome/browser/page_load_metrics/experiments/delay_navigation_throttle_unittest.cc:31: std::unique_ptr<content::NavigationHandle> test_handle = include <memory> https://codereview.chromium.org/2763613002/diff/300001/chrome/browser/page_lo... chrome/browser/page_load_metrics/experiments/delay_navigation_throttle_unittest.cc:38: EXPECT_FALSE(mock_time_task_runner->HasPendingTask()); include gtest https://codereview.chromium.org/2763613002/diff/300001/chrome/browser/page_lo... chrome/browser/page_load_metrics/experiments/delay_navigation_throttle_unittest.cc:39: EXPECT_EQ(content::NavigationThrottle::DEFER, include navigation_throttle https://codereview.chromium.org/2763613002/diff/300001/chrome/browser/page_lo... chrome/browser/page_load_metrics/experiments/delay_navigation_throttle_unittest.cc:84: std::string>> { nit: Can you make the double a TimeDelta and the string a GURL? https://codereview.chromium.org/2763613002/diff/300001/chrome/browser/page_lo... chrome/browser/page_load_metrics/experiments/delay_navigation_throttle_unittest.cc:148: }; DISALLOW_COPY_AND_ASSIGN and include base/macros
https://codereview.chromium.org/2763613002/diff/300001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/experiments/delay_navigation_throttle_unittest.cc (right): https://codereview.chromium.org/2763613002/diff/300001/chrome/browser/page_lo... chrome/browser/page_load_metrics/experiments/delay_navigation_throttle_unittest.cc:84: std::string>> { On 2017/03/23 at 17:28:39, Charlie Harrison wrote: > nit: Can you make the double a TimeDelta and the string a GURL? i can address the other comments, but the double is for a probability between 0 and 1, so i think double is the right choice here (timedelta seems wrong). lmk if i am missing something though.
https://codereview.chromium.org/2763613002/diff/300001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/experiments/delay_navigation_throttle_unittest.cc (right): https://codereview.chromium.org/2763613002/diff/300001/chrome/browser/page_lo... chrome/browser/page_load_metrics/experiments/delay_navigation_throttle_unittest.cc:84: std::string>> { On 2017/03/23 17:32:47, Bryan McQuade wrote: > On 2017/03/23 at 17:28:39, Charlie Harrison wrote: > > nit: Can you make the double a TimeDelta and the string a GURL? > > i can address the other comments, but the double is for a probability between 0 > and 1, so i think double is the right choice here (timedelta seems wrong). lmk > if i am missing something though. Ha, sorry. Misread it. double is fine. Can you make one of the tests use a probability less than 1 and greater than 0?
On 2017/03/23 at 17:36:56, csharrison wrote: > https://codereview.chromium.org/2763613002/diff/300001/chrome/browser/page_lo... > File chrome/browser/page_load_metrics/experiments/delay_navigation_throttle_unittest.cc (right): > > https://codereview.chromium.org/2763613002/diff/300001/chrome/browser/page_lo... > chrome/browser/page_load_metrics/experiments/delay_navigation_throttle_unittest.cc:84: std::string>> { > On 2017/03/23 17:32:47, Bryan McQuade wrote: > > On 2017/03/23 at 17:28:39, Charlie Harrison wrote: > > > nit: Can you make the double a TimeDelta and the string a GURL? > > > > i can address the other comments, but the double is for a probability between 0 > > and 1, so i think double is the right choice here (timedelta seems wrong). lmk > > if i am missing something though. > > Ha, sorry. Misread it. double is fine. > > Can you make one of the tests use a probability less than 1 and greater than 0? I don't think so - then the test would not be deterministic. LMK if you see a way to write a deterministic test w/ a value in between 0 and 1.
On 2017/03/23 17:46:23, Bryan McQuade wrote: > On 2017/03/23 at 17:36:56, csharrison wrote: > > > https://codereview.chromium.org/2763613002/diff/300001/chrome/browser/page_lo... > > File > chrome/browser/page_load_metrics/experiments/delay_navigation_throttle_unittest.cc > (right): > > > > > https://codereview.chromium.org/2763613002/diff/300001/chrome/browser/page_lo... > > > chrome/browser/page_load_metrics/experiments/delay_navigation_throttle_unittest.cc:84: > std::string>> { > > On 2017/03/23 17:32:47, Bryan McQuade wrote: > > > On 2017/03/23 at 17:28:39, Charlie Harrison wrote: > > > > nit: Can you make the double a TimeDelta and the string a GURL? > > > > > > i can address the other comments, but the double is for a probability > between 0 > > > and 1, so i think double is the right choice here (timedelta seems wrong). > lmk > > > if i am missing something though. > > > > Ha, sorry. Misread it. double is fine. > > > > Can you make one of the tests use a probability less than 1 and greater than > 0? > > I don't think so - then the test would not be deterministic. LMK if you see a > way to write a deterministic test w/ a value in between 0 and 1. Sorry, I'm a little out of it today. My suggestion doesn't really make sense.
On 2017/03/23 17:46:23, Bryan McQuade wrote: > On 2017/03/23 at 17:36:56, csharrison wrote: > > > https://codereview.chromium.org/2763613002/diff/300001/chrome/browser/page_lo... > > File > chrome/browser/page_load_metrics/experiments/delay_navigation_throttle_unittest.cc > (right): > > > > > https://codereview.chromium.org/2763613002/diff/300001/chrome/browser/page_lo... > > > chrome/browser/page_load_metrics/experiments/delay_navigation_throttle_unittest.cc:84: > std::string>> { > > On 2017/03/23 17:32:47, Bryan McQuade wrote: > > > On 2017/03/23 at 17:28:39, Charlie Harrison wrote: > > > > nit: Can you make the double a TimeDelta and the string a GURL? > > > > > > i can address the other comments, but the double is for a probability > between 0 > > > and 1, so i think double is the right choice here (timedelta seems wrong). > lmk > > > if i am missing something though. > > > > Ha, sorry. Misread it. double is fine. > > > > Can you make one of the tests use a probability less than 1 and greater than > 0? > > I don't think so - then the test would not be deterministic. LMK if you see a > way to write a deterministic test w/ a value in between 0 and 1. Sorry, I'm a little out of it today. My suggestion doesn't really make sense.
On 2017/03/23 at 17:46:23, Bryan McQuade wrote: > On 2017/03/23 at 17:36:56, csharrison wrote: > > https://codereview.chromium.org/2763613002/diff/300001/chrome/browser/page_lo... > > File chrome/browser/page_load_metrics/experiments/delay_navigation_throttle_unittest.cc (right): > > > > https://codereview.chromium.org/2763613002/diff/300001/chrome/browser/page_lo... > > chrome/browser/page_load_metrics/experiments/delay_navigation_throttle_unittest.cc:84: std::string>> { > > On 2017/03/23 17:32:47, Bryan McQuade wrote: > > > On 2017/03/23 at 17:28:39, Charlie Harrison wrote: > > > > nit: Can you make the double a TimeDelta and the string a GURL? > > > > > > i can address the other comments, but the double is for a probability between 0 > > > and 1, so i think double is the right choice here (timedelta seems wrong). lmk > > > if i am missing something though. > > > > Ha, sorry. Misread it. double is fine. > > > > Can you make one of the tests use a probability less than 1 and greater than 0? > > I don't think so - then the test would not be deterministic. LMK if you see a way to write a deterministic test w/ a value in between 0 and 1. (i could provide a mock rng if you want, which would enable this - happy to do it if you think it's important)
On 2017/03/23 17:51:46, Bryan McQuade wrote: > On 2017/03/23 at 17:46:23, Bryan McQuade wrote: > > On 2017/03/23 at 17:36:56, csharrison wrote: > > > > https://codereview.chromium.org/2763613002/diff/300001/chrome/browser/page_lo... > > > File > chrome/browser/page_load_metrics/experiments/delay_navigation_throttle_unittest.cc > (right): > > > > > > > https://codereview.chromium.org/2763613002/diff/300001/chrome/browser/page_lo... > > > > chrome/browser/page_load_metrics/experiments/delay_navigation_throttle_unittest.cc:84: > std::string>> { > > > On 2017/03/23 17:32:47, Bryan McQuade wrote: > > > > On 2017/03/23 at 17:28:39, Charlie Harrison wrote: > > > > > nit: Can you make the double a TimeDelta and the string a GURL? > > > > > > > > i can address the other comments, but the double is for a probability > between 0 > > > > and 1, so i think double is the right choice here (timedelta seems wrong). > lmk > > > > if i am missing something though. > > > > > > Ha, sorry. Misread it. double is fine. > > > > > > Can you make one of the tests use a probability less than 1 and greater than > 0? > > > > I don't think so - then the test would not be deterministic. LMK if you see a > way to write a deterministic test w/ a value in between 0 and 1. > > (i could provide a mock rng if you want, which would enable this - happy to do > it if you think it's important) It isnt necessary, imo
addressed comments, thx! https://codereview.chromium.org/2763613002/diff/300001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/experiments/delay_navigation_throttle_unittest.cc (right): https://codereview.chromium.org/2763613002/diff/300001/chrome/browser/page_lo... chrome/browser/page_load_metrics/experiments/delay_navigation_throttle_unittest.cc:26: base::TimeDelta navigation_delay = base::TimeDelta::FromSeconds(1); On 2017/03/23 at 17:28:39, Charlie Harrison wrote: > include base/time/time.h done https://codereview.chromium.org/2763613002/diff/300001/chrome/browser/page_lo... chrome/browser/page_load_metrics/experiments/delay_navigation_throttle_unittest.cc:27: GURL url("http://www.example.com/"); On 2017/03/23 at 17:28:40, Charlie Harrison wrote: > include url/gurl.h done https://codereview.chromium.org/2763613002/diff/300001/chrome/browser/page_lo... chrome/browser/page_load_metrics/experiments/delay_navigation_throttle_unittest.cc:29: scoped_refptr<base::TestMockTimeTaskRunner> mock_time_task_runner( On 2017/03/23 at 17:28:39, Charlie Harrison wrote: > include base/memory/ref_counted.h done https://codereview.chromium.org/2763613002/diff/300001/chrome/browser/page_lo... chrome/browser/page_load_metrics/experiments/delay_navigation_throttle_unittest.cc:31: std::unique_ptr<content::NavigationHandle> test_handle = On 2017/03/23 at 17:28:39, Charlie Harrison wrote: > include <memory> done https://codereview.chromium.org/2763613002/diff/300001/chrome/browser/page_lo... chrome/browser/page_load_metrics/experiments/delay_navigation_throttle_unittest.cc:38: EXPECT_FALSE(mock_time_task_runner->HasPendingTask()); On 2017/03/23 at 17:28:39, Charlie Harrison wrote: > include gtest done https://codereview.chromium.org/2763613002/diff/300001/chrome/browser/page_lo... chrome/browser/page_load_metrics/experiments/delay_navigation_throttle_unittest.cc:39: EXPECT_EQ(content::NavigationThrottle::DEFER, On 2017/03/23 at 17:28:40, Charlie Harrison wrote: > include navigation_throttle done https://codereview.chromium.org/2763613002/diff/300001/chrome/browser/page_lo... chrome/browser/page_load_metrics/experiments/delay_navigation_throttle_unittest.cc:84: std::string>> { On 2017/03/23 at 17:36:55, Charlie Harrison wrote: > On 2017/03/23 17:32:47, Bryan McQuade wrote: > > On 2017/03/23 at 17:28:39, Charlie Harrison wrote: > > > nit: Can you make the double a TimeDelta and the string a GURL? > > > > i can address the other comments, but the double is for a probability between 0 > > and 1, so i think double is the right choice here (timedelta seems wrong). lmk > > if i am missing something though. > > Ha, sorry. Misread it. double is fine. > > Can you make one of the tests use a probability less than 1 and greater than 0? switched to a gurl. https://codereview.chromium.org/2763613002/diff/300001/chrome/browser/page_lo... chrome/browser/page_load_metrics/experiments/delay_navigation_throttle_unittest.cc:148: }; On 2017/03/23 at 17:28:39, Charlie Harrison wrote: > DISALLOW_COPY_AND_ASSIGN and include base/macros done
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
bmcquade@chromium.org changed reviewers: + isherman@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
bmcquade@chromium.org changed reviewers: - isherman@chromium.org
isherman, PTAL for histograms.xml, thanks!
The CQ bit was checked by bmcquade@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...
bmcquade@chromium.org changed reviewers: + isherman@chromium.org
isherman, PTAL for histograms.xml, thanks!
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 bmcquade@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.
still lgtm https://codereview.chromium.org/2763613002/diff/360001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/experiments/delay_navigation_throttle.cc (right): https://codereview.chromium.org/2763613002/diff/360001/chrome/browser/page_lo... chrome/browser/page_load_metrics/experiments/delay_navigation_throttle.cc:24: if (delay_probability == 0 || delay_probability < base::RandDouble()) Can we DCHECK that delay_probability is <= 1?
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
https://codereview.chromium.org/2763613002/diff/360001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/experiments/delay_navigation_throttle.cc (right): https://codereview.chromium.org/2763613002/diff/360001/chrome/browser/page_lo... chrome/browser/page_load_metrics/experiments/delay_navigation_throttle.cc:24: if (delay_probability == 0 || delay_probability < base::RandDouble()) On 2017/03/24 at 17:06:58, Charlie Harrison wrote: > Can we DCHECK that delay_probability is <= 1? sure, good idea. done, thanks!
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.
histograms.xml lgtm
The CQ bit was checked by bmcquade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from csharrison@chromium.org Link to the patchset: https://codereview.chromium.org/2763613002/#ps380001 (title: "add dchecks")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 380001, "attempt_start_ts": 1490393856797900,
"parent_rev": "105dc7df0557f8dc718a6f2914e66a1af13bfcba", "commit_rev":
"b455655aabeb57f21c947ed8cdaf4761d5d20fd8"}
Message was sent while issue was closed.
Description was changed from ========== Add DelayNavigationThrottle DelayNavigationThrottle delays all HTTP/HTTPS main frame navigations by an amount specified in a field trial parameter. BUG=704117 ========== to ========== Add DelayNavigationThrottle DelayNavigationThrottle delays all HTTP/HTTPS main frame navigations by an amount specified in a field trial parameter. BUG=704117 Review-Url: https://codereview.chromium.org/2763613002 Cr-Commit-Position: refs/heads/master@{#459575} Committed: https://chromium.googlesource.com/chromium/src/+/b455655aabeb57f21c947ed8cdaf... ==========
Message was sent while issue was closed.
Committed patchset #20 (id:380001) as https://chromium.googlesource.com/chromium/src/+/b455655aabeb57f21c947ed8cdaf... |
