Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2702413007:
Cleaning up test only variables. (Closed)
DescriptionCleaning up test only variables.
BUG=693225
Review-Url: https://codereview.chromium.org/2702413007
Cr-Commit-Position: refs/heads/master@{#452798}
Committed: https://chromium.googlesource.com/chromium/src/+/3583570748a8d633e76a1c31e763f94fb1157eda
Patch Set 1 #
Total comments: 27
Patch Set 2 : Review, round 1. #
Total comments: 6
Patch Set 3 : Review, round 2. #Patch Set 4 : Compilation on Windows. #
Total comments: 1
Messages
Total messages: 23 (9 generated)
Description was changed from ========== Cleaning up test only variables. BUG=693225 ========== to ========== Cleaning up test only variables. BUG=693225 ==========
dyaroshev@yandex-team.ru changed reviewers: + mpearson@google.com, pkasting@chromium.org
On 2017/02/22 22:10:46, dyaroshev wrote: > mailto:dyaroshev@yandex-team.ru changed reviewers: > + mailto:mpearson@google.com, mailto:pkasting@chromium.org I propose to get rid of member variables that were obscuring code and did very little good in tests and instead cherrypicked extracting Trim function... from big refactoring before flat_sets here: https://codereview.chromium.org/2690303012/ and wrote couple of tests for it that cover almost everything from the original test and also some extra aspects of trimming more thoroughly.
https://codereview.chromium.org/2702413007/diff/1/components/omnibox/browser/... File components/omnibox/browser/in_memory_url_index_unittest.cc (right): https://codereview.chromium.org/2702413007/diff/1/components/omnibox/browser/... components/omnibox/browser/in_memory_url_index_unittest.cc:91: constexpr size_t kItemsToScoreLimit = 500; Make constants local. https://codereview.chromium.org/2702413007/diff/1/components/omnibox/browser/... components/omnibox/browser/in_memory_url_index_unittest.cc:757: } One extra addition is unnecessary. https://codereview.chromium.org/2702413007/diff/1/components/omnibox/browser/... components/omnibox/browser/in_memory_url_index_unittest.cc:759: auto history_id_set = FillHistoryIDSet(kMinRowId, row_id); Check that all ids are in https://codereview.chromium.org/2702413007/diff/1/components/omnibox/browser/... components/omnibox/browser/in_memory_url_index_unittest.cc:795: for (HistoryID id : history_id_set) { We could do the checks more efficiently for the set but we'll drop the sorting in the refactoring.
https://codereview.chromium.org/2702413007/diff/1/components/omnibox/browser/... File components/omnibox/browser/in_memory_url_index_unittest.cc (right): https://codereview.chromium.org/2702413007/diff/1/components/omnibox/browser/... components/omnibox/browser/in_memory_url_index_unittest.cc:99: base::Time OldVisitTime() { Nit: I would make these const temps in the caller side instead of helper functions, for a couple small reasons. One is that it's more readable to have the definition right there in code than to look up here for it. In the case of RecentVisitTime(), there's only one caller, so we wouldn't even duplicate code. Another is that it's not clear whether these values are arbitrary, and if they need to be the same across all callers, or are just generic "any old visit". Placing the definitions in the callers makes it more clear the tests are independent of each other. The third reason is that this will let you call Now() once and store the result in a temp instead of recomputing it repeatedly, which is an efficiency win. https://codereview.chromium.org/2702413007/diff/1/components/omnibox/browser/... components/omnibox/browser/in_memory_url_index_unittest.cc:752: TEST_F(InMemoryURLIndexTest, DontTrimMoreThanLimit) { Nit: Especially for the second test you add, but really for both of these, please give a descriptive comment sentence describing what the test is testing. https://codereview.chromium.org/2702413007/diff/1/components/omnibox/browser/... components/omnibox/browser/in_memory_url_index_unittest.cc:755: for (++row_id; size_t(row_id) < kItemsToScoreLimit - 1; ++row_id) { I don't think this works right? kMinRowId = 5000 and kItemsToScoreLimit = 500. I think you meant to compare against kMinRowId + kItemsToScoreLimit or something, or the loop won't execute. Nit: Don't cast to size_t with this syntax; use a static_cast (2 places). No {}. https://codereview.chromium.org/2702413007/diff/1/components/omnibox/browser/... components/omnibox/browser/in_memory_url_index_unittest.cc:757: } On 2017/02/22 22:21:51, dyaroshev wrote: > One extra addition is unnecessary. Not sure what this is saying. Is it explaining the "- 1" in the loop condition above? Because I don't really get that. https://codereview.chromium.org/2702413007/diff/1/components/omnibox/browser/... components/omnibox/browser/in_memory_url_index_unittest.cc:766: int expected_worst = 1; Nit: I can't instantly tell what this code is doing beyond the general "expect that the right number of things got trimmed". Add comments above blocks of code giving the high level summary of what a large block does. https://codereview.chromium.org/2702413007/diff/1/components/omnibox/browser/... File components/omnibox/browser/url_index_private_data.cc (right): https://codereview.chromium.org/2702413007/diff/1/components/omnibox/browser/... components/omnibox/browser/url_index_private_data.cc:198: history_ids_were_trimmed = Nit: Use |= https://codereview.chromium.org/2702413007/diff/1/components/omnibox/browser/... File components/omnibox/browser/url_index_private_data.h (right): https://codereview.chromium.org/2702413007/diff/1/components/omnibox/browser/... components/omnibox/browser/url_index_private_data.h:148: friend class InMemoryURLIndexTest; Not necessarily for this CL, but the fact that we have this friend declaration already makes me think we could plumb some accessor/mutator functions in the test base and kill off all the FRIEND_TEST declarations for this class below, which would be a nice win. https://codereview.chromium.org/2702413007/diff/1/components/omnibox/browser/... components/omnibox/browser/url_index_private_data.h:218: // returned from the HQP. Nit: I wouldn't linebreak here, but if you do, put a blank comment line below. https://codereview.chromium.org/2702413007/diff/1/components/omnibox/browser/... components/omnibox/browser/url_index_private_data.h:219: // Returns if anything was trimmed. Nit: if -> whether
I'll fix your comments tomorrow, just to clarify - you are ok with replacing test counters with tests for Trim... for the sake of clarity? They are not exactly one to one match. https://codereview.chromium.org/2702413007/diff/1/components/omnibox/browser/... File components/omnibox/browser/in_memory_url_index_unittest.cc (right): https://codereview.chromium.org/2702413007/diff/1/components/omnibox/browser/... components/omnibox/browser/in_memory_url_index_unittest.cc:99: base::Time OldVisitTime() { On 2017/02/23 01:04:45, Peter Kasting (backlogged) wrote: > The third reason is that this will let you call Now() once and store the result > in a temp instead of recomputing it repeatedly, which is an efficiency win. I think think that calling now is good because we get different timings for different results. https://codereview.chromium.org/2702413007/diff/1/components/omnibox/browser/... components/omnibox/browser/in_memory_url_index_unittest.cc:757: } On 2017/02/23 01:04:45, Peter Kasting (backlogged) wrote: > On 2017/02/22 22:21:51, dyaroshev wrote: > > One extra addition is unnecessary. > > Not sure what this is saying. Is it explaining the "- 1" in the loop condition > above? Because I don't really get that. It's more for me - to avoid extra patches, if I see smth I want fixed, I leave a comment and then fix with your issues. Here I'm calling AddHistory... 2 times: one before the loop, one after - this is unnecessary. https://codereview.chromium.org/2702413007/diff/1/components/omnibox/browser/... File components/omnibox/browser/url_index_private_data.cc (right): https://codereview.chromium.org/2702413007/diff/1/components/omnibox/browser/... components/omnibox/browser/url_index_private_data.cc:198: history_ids_were_trimmed = On 2017/02/23 01:04:45, Peter Kasting (backlogged) wrote: > Nit: Use |= This would be a bit operation - is it ok? I think that you are correct and compiler won't do lazy evaluation for bitwise or but to doublecheck, are you sure?
On 2017/02/23 01:41:26, dyaroshev wrote: > I'll fix your comments tomorrow, just to clarify - you are ok with replacing > test counters with tests for Trim... for the sake of clarity? They are not > exactly one to one match. As long as the coverage is equivalent or better, it seems OK to me. https://codereview.chromium.org/2702413007/diff/1/components/omnibox/browser/... File components/omnibox/browser/in_memory_url_index_unittest.cc (right): https://codereview.chromium.org/2702413007/diff/1/components/omnibox/browser/... components/omnibox/browser/in_memory_url_index_unittest.cc:757: } On 2017/02/23 01:41:26, dyaroshev wrote: > On 2017/02/23 01:04:45, Peter Kasting (backlogged) wrote: > > On 2017/02/22 22:21:51, dyaroshev wrote: > > > One extra addition is unnecessary. > > > > Not sure what this is saying. Is it explaining the "- 1" in the loop > condition > > above? Because I don't really get that. > > It's more for me - to avoid extra patches, if I see smth I want fixed, I leave a > comment and then fix with your issues. Here I'm calling AddHistory... 2 times: > one before the loop, one after - this is unnecessary. OK. I still don't really get the "- 1" in the loop conditional then. https://codereview.chromium.org/2702413007/diff/1/components/omnibox/browser/... File components/omnibox/browser/url_index_private_data.cc (right): https://codereview.chromium.org/2702413007/diff/1/components/omnibox/browser/... components/omnibox/browser/url_index_private_data.cc:198: history_ids_were_trimmed = On 2017/02/23 01:41:26, dyaroshev wrote: > On 2017/02/23 01:04:45, Peter Kasting (backlogged) wrote: > > Nit: Use |= > > This would be a bit operation - is it ok? > I think that you are correct and compiler won't do lazy evaluation for bitwise > or but to doublecheck, are you sure? As you note, A |= B is equivalent to A = A | B. Bitwise-or never short-circuits, no matter the types of its arguments, so this is safe.
On Peter Kasting wrote: > On dyaroshev wrote >> I'll fix your comments tomorrow, just to clarify - you are ok with replacing >> test counters with tests for Trim... for the sake of clarity? They are not >> exactly one to one match. > >As long as the coverage is equivalent or better, it seems OK to me. It's not precisely equivalent - we do not check number of matches before trimming but this doesn't seem to be a useful enough test to pollute a class for it. https://codereview.chromium.org/2702413007/diff/1/components/omnibox/browser/... File components/omnibox/browser/in_memory_url_index_unittest.cc (right): https://codereview.chromium.org/2702413007/diff/1/components/omnibox/browser/... components/omnibox/browser/in_memory_url_index_unittest.cc:91: constexpr size_t kItemsToScoreLimit = 500; On 2017/02/22 22:21:51, dyaroshev wrote: > Make constants local. Done. https://codereview.chromium.org/2702413007/diff/1/components/omnibox/browser/... components/omnibox/browser/in_memory_url_index_unittest.cc:99: base::Time OldVisitTime() { On 2017/02/23 01:41:26, dyaroshev wrote: > On 2017/02/23 01:04:45, Peter Kasting (backlogged) wrote: > > The third reason is that this will let you call Now() once and store the > result > > in a temp instead of recomputing it repeatedly, which is an efficiency win. > > I think think that calling now is good because we get different timings for > different results. I've done smth. Is it ok? https://codereview.chromium.org/2702413007/diff/1/components/omnibox/browser/... components/omnibox/browser/in_memory_url_index_unittest.cc:752: TEST_F(InMemoryURLIndexTest, DontTrimMoreThanLimit) { On 2017/02/23 01:04:45, Peter Kasting wrote: > Nit: Especially for the second test you add, but really for both of these, > please give a descriptive comment sentence describing what the test is testing. Done? https://codereview.chromium.org/2702413007/diff/1/components/omnibox/browser/... components/omnibox/browser/in_memory_url_index_unittest.cc:755: for (++row_id; size_t(row_id) < kItemsToScoreLimit - 1; ++row_id) { On 2017/02/23 01:04:45, Peter Kasting wrote: > I don't think this works right? kMinRowId = 5000 and kItemsToScoreLimit = 500. > I think you meant to compare against kMinRowId + kItemsToScoreLimit or > something, or the loop won't execute. > > Nit: Don't cast to size_t with this syntax; use a static_cast (2 places). No > {}. Not applicable. https://codereview.chromium.org/2702413007/diff/1/components/omnibox/browser/... components/omnibox/browser/in_memory_url_index_unittest.cc:757: } On 2017/02/23 01:46:06, Peter Kasting wrote: > On 2017/02/23 01:41:26, dyaroshev wrote: > > On 2017/02/23 01:04:45, Peter Kasting (backlogged) wrote: > > > On 2017/02/22 22:21:51, dyaroshev wrote: > > > > One extra addition is unnecessary. > > > > > > Not sure what this is saying. Is it explaining the "- 1" in the loop > > condition > > > above? Because I don't really get that. > > > > It's more for me - to avoid extra patches, if I see smth I want fixed, I leave > a > > comment and then fix with your issues. Here I'm calling AddHistory... 2 times: > > one before the loop, one after - this is unnecessary. > > OK. I still don't really get the "- 1" in the loop conditional then. Not applicable. https://codereview.chromium.org/2702413007/diff/1/components/omnibox/browser/... components/omnibox/browser/in_memory_url_index_unittest.cc:766: int expected_worst = 1; On 2017/02/23 01:04:45, Peter Kasting wrote: > Nit: I can't instantly tell what this code is doing beyond the general "expect > that the right number of things got trimmed". Add comments above blocks of code > giving the high level summary of what a large block does. Rewrote the test. https://codereview.chromium.org/2702413007/diff/1/components/omnibox/browser/... File components/omnibox/browser/url_index_private_data.cc (right): https://codereview.chromium.org/2702413007/diff/1/components/omnibox/browser/... components/omnibox/browser/url_index_private_data.cc:198: history_ids_were_trimmed = On 2017/02/23 01:46:06, Peter Kasting wrote: > On 2017/02/23 01:41:26, dyaroshev wrote: > > On 2017/02/23 01:04:45, Peter Kasting (backlogged) wrote: > > > Nit: Use |= > > > > This would be a bit operation - is it ok? > > I think that you are correct and compiler won't do lazy evaluation for bitwise > > or but to doublecheck, are you sure? > > As you note, A |= B is equivalent to A = A | B. Bitwise-or never > short-circuits, no matter the types of its arguments, so this is safe. Done. https://codereview.chromium.org/2702413007/diff/1/components/omnibox/browser/... File components/omnibox/browser/url_index_private_data.h (right): https://codereview.chromium.org/2702413007/diff/1/components/omnibox/browser/... components/omnibox/browser/url_index_private_data.h:218: // returned from the HQP. On 2017/02/23 01:04:45, Peter Kasting wrote: > Nit: I wouldn't linebreak here, but if you do, put a blank comment line below. Done. https://codereview.chromium.org/2702413007/diff/1/components/omnibox/browser/... components/omnibox/browser/url_index_private_data.h:219: // Returns if anything was trimmed. On 2017/02/23 01:04:45, Peter Kasting wrote: > Nit: if -> whether Done.
https://codereview.chromium.org/2702413007/diff/20001/components/omnibox/brow... File components/omnibox/browser/url_index_private_data.h (right): https://codereview.chromium.org/2702413007/diff/20001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.h:156: FRIEND_TEST_ALL_PREFIXES(InMemoryURLIndexTest, TrimHistortIds); typo
LGTM https://codereview.chromium.org/2702413007/diff/20001/components/omnibox/brow... File components/omnibox/browser/in_memory_url_index_unittest.cc (right): https://codereview.chromium.org/2702413007/diff/20001/components/omnibox/brow... components/omnibox/browser/in_memory_url_index_unittest.cc:770: { Nit: This outer brace not needed (the win of saying "|row_id| is only used here| seems less than the loss of implying "something happens at the exit of tis block that must occur before we continue") https://codereview.chromium.org/2702413007/diff/20001/components/omnibox/brow... components/omnibox/browser/in_memory_url_index_unittest.cc:804: EXPECT_EQ(current_count, kAlmostLimit); Nit: (expected, actual)
https://codereview.chromium.org/2702413007/diff/20001/components/omnibox/brow... File components/omnibox/browser/in_memory_url_index_unittest.cc (right): https://codereview.chromium.org/2702413007/diff/20001/components/omnibox/brow... components/omnibox/browser/in_memory_url_index_unittest.cc:770: { On 2017/02/24 01:37:41, Peter Kasting wrote: > Nit: This outer brace not needed (the win of saying "|row_id| is only used here| > seems less than the loss of implying "something happens at the exit of tis block > that must occur before we continue") Done. https://codereview.chromium.org/2702413007/diff/20001/components/omnibox/brow... components/omnibox/browser/in_memory_url_index_unittest.cc:804: EXPECT_EQ(current_count, kAlmostLimit); On 2017/02/24 01:37:41, Peter Kasting wrote: > Nit: (expected, actual) Done. https://codereview.chromium.org/2702413007/diff/20001/components/omnibox/brow... File components/omnibox/browser/url_index_private_data.h (right): https://codereview.chromium.org/2702413007/diff/20001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.h:156: FRIEND_TEST_ALL_PREFIXES(InMemoryURLIndexTest, TrimHistortIds); On 2017/02/24 01:29:27, dyaroshev wrote: > typo Done.
The CQ bit was checked by dyaroshev@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2702413007/#ps40001 (title: "Review, round 2.")
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: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
https://codereview.chromium.org/2702413007/diff/60001/components/omnibox/brow... File components/omnibox/browser/in_memory_url_index_unittest.cc (right): https://codereview.chromium.org/2702413007/diff/60001/components/omnibox/brow... components/omnibox/browser/in_memory_url_index_unittest.cc:740: auto GetHistoryIdsUpTo = [&](HistoryID max) { Visual studio does not implicitly capture constexpr. https://developercommunity.visualstudio.com/content/problem/1997/constexpr-no...
The CQ bit was checked by dyaroshev@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2702413007/#ps60001 (title: "Compilation on Windows.")
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": 60001, "attempt_start_ts": 1487935250043400,
"parent_rev": "86ecee8caf0ab7b5f4ac5f2a60ed19a341e384d4", "commit_rev":
"3583570748a8d633e76a1c31e763f94fb1157eda"}
Message was sent while issue was closed.
Description was changed from ========== Cleaning up test only variables. BUG=693225 ========== to ========== Cleaning up test only variables. BUG=693225 Review-Url: https://codereview.chromium.org/2702413007 Cr-Commit-Position: refs/heads/master@{#452798} Committed: https://chromium.googlesource.com/chromium/src/+/3583570748a8d633e76a1c31e763... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/3583570748a8d633e76a1c31e763... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
