Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(14)

Issue 1425013002: Removing bad dependencies from autocomplete_provider_unittest (Closed)

Created:
5 years, 1 month ago by Abhishek
Modified:
5 years ago
Reviewers:
droger, Peter Kasting
CC:
chromium-reviews, James Su, Jitu( very slow this week)
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Removing bad dependencies from autocomplete_provider_unittest Remove chrome_autocomplete_scheme_classifier dependency and use TestingSchemeClassifier. Add MockAutocompleteProviderClient client to get TemplateURlService. Remove NOTIFICATION_AUTOCOMPLETE_CONTROLLER_RESULT_READY and making a direct call to AutocompleteProviderTest. Using message_loop over BrowserThreadBundle. Move autocomplete_provider_unittest BUG=544906 Committed: https://crrev.com/877c36928f02350da8e1e38a0e549ddd6f946e0c Cr-Commit-Position: refs/heads/master@{#363992}

Patch Set 1 #

Total comments: 5

Patch Set 2 : Remove more dependency #

Total comments: 8

Patch Set 3 : Removed Notification dependency #

Patch Set 4 : updated #

Total comments: 4

Patch Set 5 : Add client_owned_ #

Total comments: 4

Patch Set 6 : Update with timed out tests #

Patch Set 7 : move unittest file to components #

Total comments: 2

Patch Set 8 : add comment for client_owned_ #

Total comments: 35

Patch Set 9 : updated #

Total comments: 14

Patch Set 10 : rebase and updated #

Patch Set 11 : updated typo mistake #

Patch Set 12 : updated #

Total comments: 2

Patch Set 13 : removed TODO #

Total comments: 10

Patch Set 14 : updated #

Total comments: 4

Patch Set 15 : updated #

Unified diffs Side-by-side diffs Delta from patch set Stats (+138 lines, -880 lines) Patch
M chrome/browser/autocomplete/autocomplete_provider_unittest.cc View 1 2 3 4 5 6 1 chunk +0 lines, -756 lines 0 comments Download
M chrome/chrome_tests_unit.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +0 lines, -1 line 0 comments Download
M components/components_tests.gyp View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -0 lines 0 comments Download
M components/omnibox/browser/BUILD.gn View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
A + components/omnibox/browser/autocomplete_provider_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 23 chunks +136 lines, -123 lines 0 comments Download

Messages

Total messages: 60 (11 generated)
Abhishek
PTAL!
5 years, 1 month ago (2015-10-28 16:18:09 UTC) #3
droger
https://codereview.chromium.org/1425013002/diff/1/chrome/browser/autocomplete/autocomplete_provider_unittest.cc File chrome/browser/autocomplete/autocomplete_provider_unittest.cc (right): https://codereview.chromium.org/1425013002/diff/1/chrome/browser/autocomplete/autocomplete_provider_unittest.cc#newcode275 chrome/browser/autocomplete/autocomplete_provider_unittest.cc:275: if (TemplateURLServiceFactory::GetForProfile(&profile_) == NULL) { You have to remove ...
5 years, 1 month ago (2015-10-28 16:56:47 UTC) #4
Abhishek
PTAL! https://codereview.chromium.org/1425013002/diff/1/chrome/browser/autocomplete/autocomplete_provider_unittest.cc File chrome/browser/autocomplete/autocomplete_provider_unittest.cc (right): https://codereview.chromium.org/1425013002/diff/1/chrome/browser/autocomplete/autocomplete_provider_unittest.cc#newcode275 chrome/browser/autocomplete/autocomplete_provider_unittest.cc:275: if (TemplateURLServiceFactory::GetForProfile(&profile_) == NULL) { On 2015/10/28 16:56:47, ...
5 years, 1 month ago (2015-10-30 14:15:55 UTC) #5
droger
https://codereview.chromium.org/1425013002/diff/20001/chrome/browser/autocomplete/autocomplete_provider_unittest.cc File chrome/browser/autocomplete/autocomplete_provider_unittest.cc (right): https://codereview.chromium.org/1425013002/diff/20001/chrome/browser/autocomplete/autocomplete_provider_unittest.cc#newcode256 chrome/browser/autocomplete/autocomplete_provider_unittest.cc:256: }; add blank line here https://codereview.chromium.org/1425013002/diff/20001/chrome/browser/autocomplete/autocomplete_provider_unittest.cc#newcode257 chrome/browser/autocomplete/autocomplete_provider_unittest.cc:257: void AutocompleteProviderTest::SetUp() ...
5 years, 1 month ago (2015-11-02 10:20:34 UTC) #6
droger
https://codereview.chromium.org/1425013002/diff/1/chrome/browser/autocomplete/autocomplete_provider_unittest.cc File chrome/browser/autocomplete/autocomplete_provider_unittest.cc (right): https://codereview.chromium.org/1425013002/diff/1/chrome/browser/autocomplete/autocomplete_provider_unittest.cc#newcode285 chrome/browser/autocomplete/autocomplete_provider_unittest.cc:285: TemplateURLServiceFactory::GetForProfile(&profile_); On 2015/10/30 14:15:55, Abhishek wrote: > On 2015/10/28 ...
5 years, 1 month ago (2015-11-02 10:21:46 UTC) #7
Abhishek
PTAL #3! https://codereview.chromium.org/1425013002/diff/20001/chrome/browser/autocomplete/autocomplete_provider_unittest.cc File chrome/browser/autocomplete/autocomplete_provider_unittest.cc (right): https://codereview.chromium.org/1425013002/diff/20001/chrome/browser/autocomplete/autocomplete_provider_unittest.cc#newcode256 chrome/browser/autocomplete/autocomplete_provider_unittest.cc:256: }; On 2015/11/02 10:20:33, droger wrote: > ...
5 years, 1 month ago (2015-11-03 13:34:33 UTC) #9
Abhishek
On 2015/11/02 10:21:46, droger wrote: > https://codereview.chromium.org/1425013002/diff/1/chrome/browser/autocomplete/autocomplete_provider_unittest.cc > File chrome/browser/autocomplete/autocomplete_provider_unittest.cc (right): > > https://codereview.chromium.org/1425013002/diff/1/chrome/browser/autocomplete/autocomplete_provider_unittest.cc#newcode285 > ...
5 years, 1 month ago (2015-11-03 13:36:07 UTC) #10
droger
https://codereview.chromium.org/1425013002/diff/20001/chrome/browser/autocomplete/autocomplete_provider_unittest.cc File chrome/browser/autocomplete/autocomplete_provider_unittest.cc (right): https://codereview.chromium.org/1425013002/diff/20001/chrome/browser/autocomplete/autocomplete_provider_unittest.cc#newcode359 chrome/browser/autocomplete/autocomplete_provider_unittest.cc:359: make_scoped_ptr(client_.get()), NULL, On 2015/11/03 13:34:33, Abhishek wrote: > On ...
5 years, 1 month ago (2015-11-03 14:27:48 UTC) #12
Abhishek
PTAL #4! https://codereview.chromium.org/1425013002/diff/20001/chrome/browser/autocomplete/autocomplete_provider_unittest.cc File chrome/browser/autocomplete/autocomplete_provider_unittest.cc (right): https://codereview.chromium.org/1425013002/diff/20001/chrome/browser/autocomplete/autocomplete_provider_unittest.cc#newcode359 chrome/browser/autocomplete/autocomplete_provider_unittest.cc:359: make_scoped_ptr(client_.get()), NULL, On 2015/11/03 14:27:48, droger wrote: ...
5 years, 1 month ago (2015-11-04 11:57:11 UTC) #13
droger
This looks good. Did you check if the tests now pass? Could you do try ...
5 years, 1 month ago (2015-11-04 14:29:59 UTC) #14
Abhishek
PTAL! https://codereview.chromium.org/1425013002/diff/80001/chrome/browser/autocomplete/autocomplete_provider_unittest.cc File chrome/browser/autocomplete/autocomplete_provider_unittest.cc (right): https://codereview.chromium.org/1425013002/diff/80001/chrome/browser/autocomplete/autocomplete_provider_unittest.cc#newcode248 chrome/browser/autocomplete/autocomplete_provider_unittest.cc:248: scoped_ptr<AutocompleteController> controller_; On 2015/11/04 14:29:59, droger wrote: > ...
5 years, 1 month ago (2015-11-09 14:28:27 UTC) #15
Abhishek
On 2015/11/04 14:29:59, droger wrote: > This looks good. > Did you check if the ...
5 years, 1 month ago (2015-11-09 14:30:01 UTC) #16
droger
Ok. Let me know if you need help for the tests, or ping me when ...
5 years, 1 month ago (2015-11-09 15:23:34 UTC) #17
Abhishek
PTAL #6! https://codereview.chromium.org/1425013002/diff/100001/chrome/browser/autocomplete/autocomplete_provider_unittest.cc File chrome/browser/autocomplete/autocomplete_provider_unittest.cc (right): https://codereview.chromium.org/1425013002/diff/100001/chrome/browser/autocomplete/autocomplete_provider_unittest.cc#newcode257 chrome/browser/autocomplete/autocomplete_provider_unittest.cc:257: client_owned_ = false; On 2015/11/09 15:23:33, droger ...
5 years, 1 month ago (2015-11-18 10:43:28 UTC) #18
Abhishek
On 2015/11/09 15:23:34, droger wrote: > Ok. Let me know if you need help for ...
5 years, 1 month ago (2015-11-18 10:44:24 UTC) #19
droger
On 2015/11/18 10:44:24, Abhishek wrote: > On 2015/11/09 15:23:34, droger wrote: > > Ok. Let ...
5 years, 1 month ago (2015-11-18 11:02:20 UTC) #20
Abhishek
On 2015/11/18 11:02:20, droger wrote: > On 2015/11/18 10:44:24, Abhishek wrote: > > On 2015/11/09 ...
5 years, 1 month ago (2015-11-18 11:30:26 UTC) #21
droger
I've patched your CL, played a bit with it and came up with this version ...
5 years, 1 month ago (2015-11-18 14:30:55 UTC) #22
Abhishek
On 2015/11/18 14:30:55, droger wrote: > I've patched your CL, played a bit with it ...
5 years, 1 month ago (2015-11-18 14:55:25 UTC) #23
Abhishek
On 2015/11/18 14:55:25, Abhishek wrote: > On 2015/11/18 14:30:55, droger wrote: > > I've patched ...
5 years, 1 month ago (2015-11-18 21:24:52 UTC) #25
Abhishek
Componentized autocomplete_provider_unittest. PTAL!
5 years, 1 month ago (2015-11-19 04:20:37 UTC) #27
droger
LGTM Adding pkasting for review. https://codereview.chromium.org/1425013002/diff/160001/components/omnibox/browser/autocomplete_provider_unittest.cc File components/omnibox/browser/autocomplete_provider_unittest.cc (right): https://codereview.chromium.org/1425013002/diff/160001/components/omnibox/browser/autocomplete_provider_unittest.cc#newcode262 components/omnibox/browser/autocomplete_provider_unittest.cc:262: bool client_owned_; Add a ...
5 years, 1 month ago (2015-11-19 09:02:44 UTC) #29
Abhishek
I've updated the patch. PTAL! https://codereview.chromium.org/1425013002/diff/160001/components/omnibox/browser/autocomplete_provider_unittest.cc File components/omnibox/browser/autocomplete_provider_unittest.cc (right): https://codereview.chromium.org/1425013002/diff/160001/components/omnibox/browser/autocomplete_provider_unittest.cc#newcode262 components/omnibox/browser/autocomplete_provider_unittest.cc:262: bool client_owned_; On 2015/11/19 ...
5 years, 1 month ago (2015-11-19 14:12:08 UTC) #30
Peter Kasting
https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/browser/autocomplete_provider_unittest.cc File components/omnibox/browser/autocomplete_provider_unittest.cc (left): https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/browser/autocomplete_provider_unittest.cc#oldcode638 components/omnibox/browser/autocomplete_provider_unittest.cc:638: Nit: Don't remove this https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/browser/autocomplete_provider_unittest.cc File components/omnibox/browser/autocomplete_provider_unittest.cc (right): https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/browser/autocomplete_provider_unittest.cc#newcode37 ...
5 years, 1 month ago (2015-11-19 19:04:41 UTC) #31
Abhishek
I've updated all comments. PTAL! https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/browser/autocomplete_provider_unittest.cc File components/omnibox/browser/autocomplete_provider_unittest.cc (left): https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/browser/autocomplete_provider_unittest.cc#oldcode638 components/omnibox/browser/autocomplete_provider_unittest.cc:638: On 2015/11/19 19:04:41, Peter ...
5 years, 1 month ago (2015-11-20 09:37:21 UTC) #32
Peter Kasting
LGTM. Note that some comments below are replies on the previous patch set. https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/browser/autocomplete_provider_unittest.cc File ...
5 years, 1 month ago (2015-11-20 19:52:18 UTC) #33
Abhishek
I've updated the patch. PTAL! https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/browser/autocomplete_provider_unittest.cc File components/omnibox/browser/autocomplete_provider_unittest.cc (right): https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/browser/autocomplete_provider_unittest.cc#newcode62 components/omnibox/browser/autocomplete_provider_unittest.cc:62: return; On 2015/11/20 19:52:18, ...
5 years, 1 month ago (2015-11-21 12:11:21 UTC) #34
Peter Kasting
https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/browser/autocomplete_provider_unittest.cc File components/omnibox/browser/autocomplete_provider_unittest.cc (right): https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/browser/autocomplete_provider_unittest.cc#newcode62 components/omnibox/browser/autocomplete_provider_unittest.cc:62: return; On 2015/11/21 12:11:20, Abhishek wrote: > On 2015/11/20 ...
5 years, 1 month ago (2015-11-21 21:08:43 UTC) #35
Abhishek
PTAL! https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/browser/autocomplete_provider_unittest.cc File components/omnibox/browser/autocomplete_provider_unittest.cc (right): https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/browser/autocomplete_provider_unittest.cc#newcode62 components/omnibox/browser/autocomplete_provider_unittest.cc:62: return; On 2015/11/21 21:08:42, Peter Kasting wrote: > ...
5 years, 1 month ago (2015-11-23 15:22:08 UTC) #36
droger
https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/browser/autocomplete_provider_unittest.cc File components/omnibox/browser/autocomplete_provider_unittest.cc (right): https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/browser/autocomplete_provider_unittest.cc#newcode62 components/omnibox/browser/autocomplete_provider_unittest.cc:62: return; On 2015/11/23 15:22:07, Abhishek wrote: > On 2015/11/21 ...
5 years, 1 month ago (2015-11-23 16:11:16 UTC) #37
Peter Kasting
On 2015/11/23 15:22:08, Abhishek wrote: > PTAL! > > https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/browser/autocomplete_provider_unittest.cc > File components/omnibox/browser/autocomplete_provider_unittest.cc (right): > ...
5 years, 1 month ago (2015-11-23 19:46:35 UTC) #38
Peter Kasting
On 2015/11/23 19:46:35, Peter Kasting wrote: > On 2015/11/23 15:22:08, Abhishek wrote: > > PTAL! ...
5 years, 1 month ago (2015-11-23 19:47:23 UTC) #39
Peter Kasting
On 2015/11/23 19:47:23, Peter Kasting wrote: > On 2015/11/23 19:46:35, Peter Kasting wrote: > > ...
5 years, 1 month ago (2015-11-23 19:49:00 UTC) #40
droger
On 2015/11/23 19:49:00, Peter Kasting wrote: > On 2015/11/23 19:47:23, Peter Kasting wrote: > > ...
5 years ago (2015-11-24 15:50:02 UTC) #41
droger
https://codereview.chromium.org/1425013002/diff/260001/components/omnibox/browser/autocomplete_provider_unittest.cc File components/omnibox/browser/autocomplete_provider_unittest.cc (right): https://codereview.chromium.org/1425013002/diff/260001/components/omnibox/browser/autocomplete_provider_unittest.cc#newcode70 components/omnibox/browser/autocomplete_provider_unittest.cc:70: // closure_ is null. Remove this TODO, we need ...
5 years ago (2015-11-24 15:50:17 UTC) #42
Abhishek
On 2015/11/24 15:50:02, droger wrote: > On 2015/11/23 19:49:00, Peter Kasting wrote: > > On ...
5 years ago (2015-11-24 17:48:25 UTC) #43
Abhishek
https://codereview.chromium.org/1425013002/diff/260001/components/omnibox/browser/autocomplete_provider_unittest.cc File components/omnibox/browser/autocomplete_provider_unittest.cc (right): https://codereview.chromium.org/1425013002/diff/260001/components/omnibox/browser/autocomplete_provider_unittest.cc#newcode70 components/omnibox/browser/autocomplete_provider_unittest.cc:70: // closure_ is null. On 2015/11/24 15:50:17, droger wrote: ...
5 years ago (2015-11-24 17:48:50 UTC) #44
droger
pkasting: can we commit this? or is there still something you need to understand? I ...
5 years ago (2015-12-04 12:47:25 UTC) #45
Peter Kasting
The changes below should result in a test that still doesn't crash (please verify this) ...
5 years ago (2015-12-04 20:43:48 UTC) #46
Abhishek
PTAL! https://codereview.chromium.org/1425013002/diff/280001/components/omnibox/browser/autocomplete_provider_unittest.cc File components/omnibox/browser/autocomplete_provider_unittest.cc (right): https://codereview.chromium.org/1425013002/diff/280001/components/omnibox/browser/autocomplete_provider_unittest.cc#newcode69 components/omnibox/browser/autocomplete_provider_unittest.cc:69: if (controller->done() && !closure_.is_null()) { On 2015/12/04 20:43:48, ...
5 years ago (2015-12-05 02:39:39 UTC) #47
Abhishek
I've checked both debug and release build. All tests are getting passed in my local ...
5 years ago (2015-12-05 04:11:57 UTC) #48
Abhishek
@pkasting: PTAL #14!
5 years ago (2015-12-08 17:47:51 UTC) #49
Abhishek
@pkasting: PTAL #14!
5 years ago (2015-12-08 17:47:51 UTC) #50
Peter Kasting
LGTM https://codereview.chromium.org/1425013002/diff/300001/components/omnibox/browser/autocomplete_provider_unittest.cc File components/omnibox/browser/autocomplete_provider_unittest.cc (right): https://codereview.chromium.org/1425013002/diff/300001/components/omnibox/browser/autocomplete_provider_unittest.cc#newcode188 components/omnibox/browser/autocomplete_provider_unittest.cc:188: ASSERT_NE(nullptr, match.GetTemplateURL(service, false)); Nit: Or just: ASSERT_NE(nullptr, match.GetTemplateURL(client_->GetTemplateURLService(), ...
5 years ago (2015-12-08 20:51:34 UTC) #51
Abhishek
Updated in patchset #15! https://codereview.chromium.org/1425013002/diff/300001/components/omnibox/browser/autocomplete_provider_unittest.cc File components/omnibox/browser/autocomplete_provider_unittest.cc (right): https://codereview.chromium.org/1425013002/diff/300001/components/omnibox/browser/autocomplete_provider_unittest.cc#newcode188 components/omnibox/browser/autocomplete_provider_unittest.cc:188: ASSERT_NE(nullptr, match.GetTemplateURL(service, false)); On 2015/12/08 ...
5 years ago (2015-12-09 04:56:30 UTC) #52
Abhishek
Updated in patchset #15! https://codereview.chromium.org/1425013002/diff/300001/components/omnibox/browser/autocomplete_provider_unittest.cc File components/omnibox/browser/autocomplete_provider_unittest.cc (right): https://codereview.chromium.org/1425013002/diff/300001/components/omnibox/browser/autocomplete_provider_unittest.cc#newcode188 components/omnibox/browser/autocomplete_provider_unittest.cc:188: ASSERT_NE(nullptr, match.GetTemplateURL(service, false)); On 2015/12/08 ...
5 years ago (2015-12-09 04:56:31 UTC) #53
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1425013002/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1425013002/320001
5 years ago (2015-12-09 04:57:28 UTC) #56
commit-bot: I haz the power
Committed patchset #15 (id:320001)
5 years ago (2015-12-09 05:54:30 UTC) #58
commit-bot: I haz the power
5 years ago (2015-12-09 05:55:10 UTC) #60
Message was sent while issue was closed.
Patchset 15 (id:??) landed as
https://crrev.com/877c36928f02350da8e1e38a0e549ddd6f946e0c
Cr-Commit-Position: refs/heads/master@{#363992}

Powered by Google App Engine
This is Rietveld 408576698