Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2464323004:
Fix grouped history query range for months. (Closed)
DescriptionFix grouped history query range for months.
This CL fixes an issue where the end times for grouped history queries
were not being set and adds tests to prevent regressions.
BUG=661882
Committed: https://crrev.com/2e33a4d2c2d2add03be4ddfcb9647781d947b516
Cr-Commit-Position: refs/heads/master@{#432824}
Patch Set 1 : #
Total comments: 8
Patch Set 2 : address comments #
Total comments: 4
Patch Set 3 : rebase #Patch Set 4 : address comments #Patch Set 5 : give a valid time to all tests #
Messages
Total messages: 25 (14 generated)
Patchset #1 (id:1) has been deleted
calamity@chromium.org changed reviewers: + tsergeant@chromium.org
PTAL, thanks.
tsergeant@chromium.org changed reviewers: + dbeam@chromium.org
Sorry for the delay. Passing this review over to dbeam@ to review since I'm OOO today and Dan is probably more familiar with this code anyway. Also CC twellington@ as FYI, since this will conflict horribly with https://codereview.chromium.org/2450453002/.
i would not call myself familiar with any of this code, but here goes! https://codereview.chromium.org/2464323004/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/browsing_history_handler.cc (right): https://codereview.chromium.org/2464323004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/browsing_history_handler.cc:973: if (!base::Time::FromLocalExploded(exploded, &options->end_time)) { this is the functional fix, right? https://codereview.chromium.org/2464323004/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/browsing_history_handler.h (right): https://codereview.chromium.org/2464323004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/browsing_history_handler.h:123: base::Clock* clock; can you note that this is a weak ref? https://codereview.chromium.org/2464323004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/browsing_history_handler.h:159: can you mention that this is basically just for testing? https://codereview.chromium.org/2464323004/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/browsing_history_handler_unittest.cc (right): https://codereview.chromium.org/2464323004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/browsing_history_handler_unittest.cc:310: EXPECT_EQ(expected_time, options.begin_time); i don't really understand what all this is testing. can you put some comments? or better doc names? as in: base::Time last_week = expected_time - base::TimeDelta::FromDays(7); base::Time 3_weeks_ago = expected_time - base::TimeDelta::FromDays(21);
https://codereview.chromium.org/2464323004/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/browsing_history_handler.cc (right): https://codereview.chromium.org/2464323004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/browsing_history_handler.cc:973: if (!base::Time::FromLocalExploded(exploded, &options->end_time)) { On 2016/11/10 03:31:34, Dan Beam wrote: > this is the functional fix, right? Yep, nestled deep amongst all the clock nonsense. https://codereview.chromium.org/2464323004/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/browsing_history_handler.h (right): https://codereview.chromium.org/2464323004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/browsing_history_handler.h:123: base::Clock* clock; On 2016/11/10 03:31:34, Dan Beam wrote: > can you note that this is a weak ref? Done. https://codereview.chromium.org/2464323004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/browsing_history_handler.h:159: On 2016/11/10 03:31:34, Dan Beam wrote: > can you mention that this is basically just for testing? Done. https://codereview.chromium.org/2464323004/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/browsing_history_handler_unittest.cc (right): https://codereview.chromium.org/2464323004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/browsing_history_handler_unittest.cc:310: EXPECT_EQ(expected_time, options.begin_time); On 2016/11/10 03:31:34, Dan Beam wrote: > i don't really understand what all this is testing. can you put some comments? > or better doc names? as in: > > base::Time last_week = expected_time - base::TimeDelta::FromDays(7); > > base::Time 3_weeks_ago = expected_time - base::TimeDelta::FromDays(21); Done.
lgtm, tsergeant may also have thoughts https://codereview.chromium.org/2464323004/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/browsing_history_handler_unittest.cc (right): https://codereview.chromium.org/2464323004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/browsing_history_handler_unittest.cc:52: base::Time GetReferenceTime() { nit: PretendNow? https://codereview.chromium.org/2464323004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/browsing_history_handler_unittest.cc:298: base::Time expected_time = GetReferenceTime(); I think the reader loses a bit of information / has to attempt to follow more state when you re-use the same |expected_time| local while modifying it. What I meant was more along the lines of: base::Time pretend_now = GetPretendNow(); handler.test_clock()->SetNow(pretend_now); handler.SetQueryTimeInWeeks(0, &options); base::Time midnight_tonight = pretend_now.LocalMidnight(); base::Time midnight_tomorrow = midnight_tonight + base::TimeDelta::FromDays(1); EXPECT_EQ(midnight_tomorrow, options.end_time); base::Time midnight_a_week_ago = midnight_tomorrow - base::TimeDelta::FromDays(7); EXPECT_EQ(mignight_a_week_ago, options.begin_time); this reads more naturally because it's way more descriptive and i don't have to look through all the -= to attempt to mentally map from the start of the local to the end
nothing else to add, lgtm too
The CQ bit was checked by calamity@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/2464323004/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/browsing_history_handler_unittest.cc (right): https://codereview.chromium.org/2464323004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/browsing_history_handler_unittest.cc:52: base::Time GetReferenceTime() { On 2016/11/10 19:06:37, Dan Beam wrote: > nit: PretendNow? Done. https://codereview.chromium.org/2464323004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/browsing_history_handler_unittest.cc:298: base::Time expected_time = GetReferenceTime(); On 2016/11/10 19:06:36, Dan Beam wrote: > I think the reader loses a bit of information / has to attempt to follow more > state when you re-use the same |expected_time| local while modifying it. What I > meant was more along the lines of: > > base::Time pretend_now = GetPretendNow(); > handler.test_clock()->SetNow(pretend_now); > > handler.SetQueryTimeInWeeks(0, &options); > base::Time midnight_tonight = pretend_now.LocalMidnight(); > base::Time midnight_tomorrow = midnight_tonight + base::TimeDelta::FromDays(1); > EXPECT_EQ(midnight_tomorrow, options.end_time); > > base::Time midnight_a_week_ago = > midnight_tomorrow - base::TimeDelta::FromDays(7); > EXPECT_EQ(mignight_a_week_ago, options.begin_time); > > this reads more naturally because it's way more descriptive and i don't have to > look through all the -= to attempt to mentally map from the start of the local > to the end Done.
lgtm yay!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_TIMED_OUT, no build URL) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_TIMED_OUT, no build URL) android_compile_dbg on master.tryserver.chromium.android (JOB_TIMED_OUT, no build URL) android_cronet on master.tryserver.chromium.android (JOB_TIMED_OUT, no build URL) cast_shell_android on master.tryserver.chromium.android (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by calamity@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 calamity@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tsergeant@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2464323004/#ps100001 (title: "give a valid time to all tests")
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:100001)
Message was sent while issue was closed.
Description was changed from ========== Fix grouped history query range for months. This CL fixes an issue where the end times for grouped history queries were not being set and adds tests to prevent regressions. BUG=661882 ========== to ========== Fix grouped history query range for months. This CL fixes an issue where the end times for grouped history queries were not being set and adds tests to prevent regressions. BUG=661882 Committed: https://crrev.com/2e33a4d2c2d2add03be4ddfcb9647781d947b516 Cr-Commit-Position: refs/heads/master@{#432824} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/2e33a4d2c2d2add03be4ddfcb9647781d947b516 Cr-Commit-Position: refs/heads/master@{#432824} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
