Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1425013002:
Removing bad dependencies from autocomplete_provider_unittest (Closed)
DescriptionRemoving 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 #
Messages
Total messages: 60 (11 generated)
Description was changed from ========== [WIP] Removing bad dependencies from autocomplete_provider_unittest Remove chrome_autocomplete_scheme_classifier dependency and use TestingSchemeClassifier. Add MockAutocompleteProviderClient client to get TemplateURlService. BUG=544906 ========== to ========== [WIP] Removing bad dependencies from autocomplete_provider_unittest Remove chrome_autocomplete_scheme_classifier dependency and use TestingSchemeClassifier. Add MockAutocompleteProviderClient client to get TemplateURlService. BUG=544906 ==========
abhishek.a21@samsung.com changed reviewers: + droger@chromium.org
PTAL!
https://codereview.chromium.org/1425013002/diff/1/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete_provider_unittest.cc (right): https://codereview.chromium.org/1425013002/diff/1/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_provider_unittest.cc:275: if (TemplateURLServiceFactory::GetForProfile(&profile_) == NULL) { You have to remove this call to TemplateURLServiceFactory. https://codereview.chromium.org/1425013002/diff/1/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_provider_unittest.cc:285: TemplateURLServiceFactory::GetForProfile(&profile_); Here you need to use the TemplateURLService you created. I am not sure if you can get it from client_, because it is a mock. Maybe you need to have the TemplateURLService as a member of AutocompleteProviderTest.
PTAL! https://codereview.chromium.org/1425013002/diff/1/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete_provider_unittest.cc (right): https://codereview.chromium.org/1425013002/diff/1/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_provider_unittest.cc:275: if (TemplateURLServiceFactory::GetForProfile(&profile_) == NULL) { On 2015/10/28 16:56:47, droger wrote: > You have to remove this call to TemplateURLServiceFactory. Done. https://codereview.chromium.org/1425013002/diff/1/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_provider_unittest.cc:285: TemplateURLServiceFactory::GetForProfile(&profile_); On 2015/10/28 16:56:47, droger wrote: > Here you need to use the TemplateURLService you created. > I am not sure if you can get it from client_, because it is a mock. > Maybe you need to have the TemplateURLService as a member of > AutocompleteProviderTest. Actually I'm able to get the TemplateURLService from client_ if I'm making in NiceMock. I'm not sure, but I think its require here for one more reason, as it's required to replace ChromeAutocompleteProviderClient uses. WDYT?
https://codereview.chromium.org/1425013002/diff/20001/chrome/browser/autocomp... File chrome/browser/autocomplete/autocomplete_provider_unittest.cc (right): https://codereview.chromium.org/1425013002/diff/20001/chrome/browser/autocomp... chrome/browser/autocomplete/autocomplete_provider_unittest.cc:256: }; add blank line here https://codereview.chromium.org/1425013002/diff/20001/chrome/browser/autocomp... chrome/browser/autocomplete/autocomplete_provider_unittest.cc:257: void AutocompleteProviderTest::SetUp() { Nit: in general, using the constructor is preferred over using the SetUp method. See https://code.google.com/p/googletest/wiki/FAQ#Should_I_use_the_constructor/de... https://codereview.chromium.org/1425013002/diff/20001/chrome/browser/autocomp... chrome/browser/autocomplete/autocomplete_provider_unittest.cc:359: make_scoped_ptr(client_.get()), NULL, Here and everywhere you call make_scoped_ptr(client_.get()): this is a problem, because make_scoped_ptr() takes ownership of the client, but the client is already owned by AutocompleteProviderTest. This will cause a double free. I'm not sure what is the best way to fix that. One way would be to do new AutocompleteController(client_.Pass(), ...); This will reset |client_| to null though, so you may need to keep a weak reference to it (or to the template service).
https://codereview.chromium.org/1425013002/diff/1/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete_provider_unittest.cc (right): https://codereview.chromium.org/1425013002/diff/1/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_provider_unittest.cc:285: TemplateURLServiceFactory::GetForProfile(&profile_); On 2015/10/30 14:15:55, Abhishek wrote: > On 2015/10/28 16:56:47, droger wrote: > > Here you need to use the TemplateURLService you created. > > I am not sure if you can get it from client_, because it is a mock. > > Maybe you need to have the TemplateURLService as a member of > > AutocompleteProviderTest. > > Actually I'm able to get the TemplateURLService from client_ if I'm making in > NiceMock. > > > I'm not sure, but I think its require here for one more reason, as it's required > to replace ChromeAutocompleteProviderClient uses. > > WDYT? I don't understand what you are asking here. If you can get the client from the mock, then it should be fine, no?
Patchset #3 (id:40001) has been deleted
PTAL #3! https://codereview.chromium.org/1425013002/diff/20001/chrome/browser/autocomp... File chrome/browser/autocomplete/autocomplete_provider_unittest.cc (right): https://codereview.chromium.org/1425013002/diff/20001/chrome/browser/autocomp... chrome/browser/autocomplete/autocomplete_provider_unittest.cc:256: }; On 2015/11/02 10:20:33, droger wrote: > add blank line here Done. https://codereview.chromium.org/1425013002/diff/20001/chrome/browser/autocomp... chrome/browser/autocomplete/autocomplete_provider_unittest.cc:257: void AutocompleteProviderTest::SetUp() { On 2015/11/02 10:20:33, droger wrote: > Nit: in general, using the constructor is preferred over using the SetUp method. > > See > https://code.google.com/p/googletest/wiki/FAQ#Should_I_use_the_constructor/de... Done. https://codereview.chromium.org/1425013002/diff/20001/chrome/browser/autocomp... chrome/browser/autocomplete/autocomplete_provider_unittest.cc:359: make_scoped_ptr(client_.get()), NULL, On 2015/11/02 10:20:33, droger wrote: > Here and everywhere you call make_scoped_ptr(client_.get()): > this is a problem, because make_scoped_ptr() takes ownership of the client, but > the client is already owned by AutocompleteProviderTest. This will cause a > double free. Thanks for letting me know about it! > > I'm not sure what is the best way to fix that. > One way would be to do > new AutocompleteController(client_.Pass(), ...); > This will reset |client_| to null though, so you may need to keep a weak > reference to it (or to the template service). I'm checking how to create weak reference to it! Actually if I try making client_ as weekPtr then I'm not able pass here. And if I try making template_url_service as weekPtr then I'm not able to set it into client_ :/
On 2015/11/02 10:21:46, droger wrote: > https://codereview.chromium.org/1425013002/diff/1/chrome/browser/autocomplete... > File chrome/browser/autocomplete/autocomplete_provider_unittest.cc (right): > > https://codereview.chromium.org/1425013002/diff/1/chrome/browser/autocomplete... > chrome/browser/autocomplete/autocomplete_provider_unittest.cc:285: > TemplateURLServiceFactory::GetForProfile(&profile_); > On 2015/10/30 14:15:55, Abhishek wrote: > > On 2015/10/28 16:56:47, droger wrote: > > > Here you need to use the TemplateURLService you created. > > > I am not sure if you can get it from client_, because it is a mock. > > > Maybe you need to have the TemplateURLService as a member of > > > AutocompleteProviderTest. > > > > Actually I'm able to get the TemplateURLService from client_ if I'm making in > > NiceMock. > > > > > > I'm not sure, but I think its require here for one more reason, as it's > required > > to replace ChromeAutocompleteProviderClient uses. > > > > WDYT? > > I don't understand what you are asking here. If you can get the client from the > mock, then it should be fine, no? Sorry for my confusing comment! Yes, I'm able to get the client from NiceMock.
Description was changed from ========== [WIP] Removing bad dependencies from autocomplete_provider_unittest Remove chrome_autocomplete_scheme_classifier dependency and use TestingSchemeClassifier. Add MockAutocompleteProviderClient client to get TemplateURlService. BUG=544906 ========== to ========== [WIP] 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. BUG=544906 ==========
https://codereview.chromium.org/1425013002/diff/20001/chrome/browser/autocomp... File chrome/browser/autocomplete/autocomplete_provider_unittest.cc (right): https://codereview.chromium.org/1425013002/diff/20001/chrome/browser/autocomp... chrome/browser/autocomplete/autocomplete_provider_unittest.cc:359: make_scoped_ptr(client_.get()), NULL, On 2015/11/03 13:34:33, Abhishek wrote: > On 2015/11/02 10:20:33, droger wrote: > > Here and everywhere you call make_scoped_ptr(client_.get()): > > this is a problem, because make_scoped_ptr() takes ownership of the client, > but > > the client is already owned by AutocompleteProviderTest. This will cause a > > double free. > Thanks for letting me know about it! > > > > > I'm not sure what is the best way to fix that. > > One way would be to do > > new AutocompleteController(client_.Pass(), ...); > > This will reset |client_| to null though, so you may need to keep a weak > > reference to it (or to the template service). > > I'm checking how to create weak reference to it! > Actually if I try making client_ as weekPtr then I'm not able pass here. And if > I try making template_url_service as weekPtr then I'm not able to set it into > client_ :/ Just to make sure there is no misunderstanding: You cannot use base::WeakPtr here because the client does not support it (it would need to inherit from base::SupportsWeakPtr). What I meant here was to keep a raw pointer, in addition to the scoped pointer. I'm not sure of the details though, and maybe we don't even need it. One thing is sure though: you cannot put the same object in two different scoped_ptrs, because they will both delete the object and it will crash.
PTAL #4! https://codereview.chromium.org/1425013002/diff/20001/chrome/browser/autocomp... File chrome/browser/autocomplete/autocomplete_provider_unittest.cc (right): https://codereview.chromium.org/1425013002/diff/20001/chrome/browser/autocomp... chrome/browser/autocomplete/autocomplete_provider_unittest.cc:359: make_scoped_ptr(client_.get()), NULL, On 2015/11/03 14:27:48, droger wrote: > On 2015/11/03 13:34:33, Abhishek wrote: > > On 2015/11/02 10:20:33, droger wrote: > > > Here and everywhere you call make_scoped_ptr(client_.get()): > > > this is a problem, because make_scoped_ptr() takes ownership of the client, > > but > > > the client is already owned by AutocompleteProviderTest. This will cause a > > > double free. > > Thanks for letting me know about it! > > > > > > > > I'm not sure what is the best way to fix that. > > > One way would be to do > > > new AutocompleteController(client_.Pass(), ...); > > > This will reset |client_| to null though, so you may need to keep a weak > > > reference to it (or to the template service). > > > > I'm checking how to create weak reference to it! > > Actually if I try making client_ as weekPtr then I'm not able pass here. And > if > > I try making template_url_service as weekPtr then I'm not able to set it into > > client_ :/ > > Just to make sure there is no misunderstanding: You cannot use base::WeakPtr > here because the client does not support it (it would need to inherit from > base::SupportsWeakPtr). > > What I meant here was to keep a raw pointer, in addition to the scoped pointer. > I'm not sure of the details though, and maybe we don't even need it. > Thanks! I've updated it to raw pointer. > One thing is sure though: you cannot put the same object in two different > scoped_ptrs, because they will both delete the object and it will crash.
This looks good. Did you check if the tests now pass? Could you do try jobs (i.e. git cl try). The only concern I have now is around the memory management of the client (and in particular how to prevent bugs from happening in the future when people touch this file). For example, if someone forgets to call the Register...() methods and the ownership of |client_| is never passed to |controller_|. In this case |client_| leaks (never deleted). Similarly, if someone calls Register...() twice, then the make_scopedptr() is called twice and a double free will happen. Maybe the answer is just to add more debug assertions. One way for example would be to have a boolean client_owned_ that would be false at the beginning and is set to true when the client is put in a scoped_ptr. Then we can DCHECK(!client_owned_) before calling make_scoped_ptr to ensure it is not called twice, and DCHECK(client_owned_) in the destructor of the test, to ensure that the client is not leaking. This is only a suggestion, and not necessarily a good one, I'll let you think about what is the best here. https://codereview.chromium.org/1425013002/diff/80001/chrome/browser/autocomp... File chrome/browser/autocomplete/autocomplete_provider_unittest.cc (right): https://codereview.chromium.org/1425013002/diff/80001/chrome/browser/autocomp... chrome/browser/autocomplete/autocomplete_provider_unittest.cc:248: scoped_ptr<AutocompleteController> controller_; Add a comment here: MockAutocompleteProviderClient* client_; // Owned by |controller_|. https://codereview.chromium.org/1425013002/diff/80001/chrome/browser/autocomp... chrome/browser/autocomplete/autocomplete_provider_unittest.cc:328: // notifications. Remove this comment, it's no longer accurate.
PTAL! https://codereview.chromium.org/1425013002/diff/80001/chrome/browser/autocomp... File chrome/browser/autocomplete/autocomplete_provider_unittest.cc (right): https://codereview.chromium.org/1425013002/diff/80001/chrome/browser/autocomp... chrome/browser/autocomplete/autocomplete_provider_unittest.cc:248: scoped_ptr<AutocompleteController> controller_; On 2015/11/04 14:29:59, droger wrote: > Add a comment here: > MockAutocompleteProviderClient* client_; // Owned by |controller_|. Done. https://codereview.chromium.org/1425013002/diff/80001/chrome/browser/autocomp... chrome/browser/autocomplete/autocomplete_provider_unittest.cc:328: // notifications. On 2015/11/04 14:29:59, droger wrote: > Remove this comment, it's no longer accurate. Done.
On 2015/11/04 14:29:59, droger wrote:
> This looks good.
> Did you check if the tests now pass? Could you do try jobs (i.e. git cl try).
>
Actually 3 test cases are getting timed out:
AutocompleteProviderTest.AssistedQueryStats
(../../chrome/browser/autocomplete/autocomplete_provider_unittest.cc:601)
AutocompleteProviderTest.Query
(../../chrome/browser/autocomplete/autocomplete_provider_unittest.cc:582)
AutocompleteProviderTest.RemoveDuplicates
(../../chrome/browser/autocomplete/autocomplete_provider_unittest.cc:621)
Need to check them further.
> The only concern I have now is around the memory management of the client (and
> in particular how to prevent bugs from happening in the future when people
touch
> this file).
>
> For example, if someone forgets to call the Register...() methods and the
> ownership of |client_| is never passed to |controller_|.
> In this case |client_| leaks (never deleted).
>
> Similarly, if someone calls Register...() twice, then the make_scopedptr() is
> called twice and a double free will happen.
>
> Maybe the answer is just to add more debug assertions.
> One way for example would be to have a boolean client_owned_ that would be
false
> at the beginning and is set to true when the client is put in a scoped_ptr.
> Then we can DCHECK(!client_owned_) before calling make_scoped_ptr to ensure it
> is not called twice, and DCHECK(client_owned_) in the destructor of the test,
to
> ensure that the client is not leaking.
>
I've updated the patch as per the suggestion.
> This is only a suggestion, and not necessarily a good one, I'll let you think
> about what is the best here.
>
>
https://codereview.chromium.org/1425013002/diff/80001/chrome/browser/autocomp...
> File chrome/browser/autocomplete/autocomplete_provider_unittest.cc (right):
>
>
https://codereview.chromium.org/1425013002/diff/80001/chrome/browser/autocomp...
> chrome/browser/autocomplete/autocomplete_provider_unittest.cc:248:
> scoped_ptr<AutocompleteController> controller_;
> Add a comment here:
> MockAutocompleteProviderClient* client_; // Owned by |controller_|.
>
>
https://codereview.chromium.org/1425013002/diff/80001/chrome/browser/autocomp...
> chrome/browser/autocomplete/autocomplete_provider_unittest.cc:328: //
> notifications.
> Remove this comment, it's no longer accurate.
Ok. Let me know if you need help for the tests, or ping me when they are green. https://codereview.chromium.org/1425013002/diff/100001/chrome/browser/autocom... File chrome/browser/autocomplete/autocomplete_provider_unittest.cc (right): https://codereview.chromium.org/1425013002/diff/100001/chrome/browser/autocom... chrome/browser/autocomplete/autocomplete_provider_unittest.cc:257: client_owned_ = false; Nit: Move to the initializer list. https://codereview.chromium.org/1425013002/diff/100001/chrome/browser/autocom... chrome/browser/autocomplete/autocomplete_provider_unittest.cc:261: if (client_owned_) { Can you replace lines 261-263 by // Check that the client is not leaked. DCHECK(client_owned_);
PTAL #6! https://codereview.chromium.org/1425013002/diff/100001/chrome/browser/autocom... File chrome/browser/autocomplete/autocomplete_provider_unittest.cc (right): https://codereview.chromium.org/1425013002/diff/100001/chrome/browser/autocom... chrome/browser/autocomplete/autocomplete_provider_unittest.cc:257: client_owned_ = false; On 2015/11/09 15:23:33, droger wrote: > Nit: Move to the initializer list. Done. https://codereview.chromium.org/1425013002/diff/100001/chrome/browser/autocom... chrome/browser/autocomplete/autocomplete_provider_unittest.cc:261: if (client_owned_) { On 2015/11/09 15:23:33, droger wrote: > Can you replace lines 261-263 by > > // Check that the client is not leaked. > DCHECK(client_owned_); Actually we are making client_owned_ = true at #327, #370. But we never made it false. I think due to it, crashing in 2 more test cases. AutocompleteProviderTest.ExactMatchKeywords AutocompleteProviderTest.RedundantKeywordsIgnoredInResult Please suggest!
On 2015/11/09 15:23:34, droger wrote: > Ok. Let me know if you need help for the tests, or ping me when they are green. > I need your help to solve those 3 timed out UT. Please guide me how to proceed further!
On 2015/11/18 10:44:24, Abhishek wrote: > On 2015/11/09 15:23:34, droger wrote: > > Ok. Let me know if you need help for the tests, or ping me when they are > green. > > > I need your help to solve those 3 timed out UT. Please guide me how to proceed > further! Sorry, I was pretty busy these last days, I'll take some time to look at this today.
On 2015/11/18 11:02:20, droger wrote: > On 2015/11/18 10:44:24, Abhishek wrote: > > On 2015/11/09 15:23:34, droger wrote: > > > Ok. Let me know if you need help for the tests, or ping me when they are > > green. > > > > > I need your help to solve those 3 timed out UT. Please guide me how to proceed > > further! > > Sorry, I was pretty busy these last days, I'll take some time to look at this > today. Thanks for your support!
I've patched your CL, played a bit with it and came up with this version that seems to pass the tests: https://codereview.chromium.org/1460683002/ You can take what you want from it.
On 2015/11/18 14:30:55, droger wrote: > I've patched your CL, played a bit with it and came up with this version that > seems to pass the tests: > https://codereview.chromium.org/1460683002/ > > You can take what you want from it. Thanks for your kind help! I'll check and update this patch.
Patchset #6 (id:120001) has been deleted
On 2015/11/18 14:55:25, Abhishek wrote: > On 2015/11/18 14:30:55, droger wrote: > > I've patched your CL, played a bit with it and came up with this version that > > seems to pass the tests: > > https://codereview.chromium.org/1460683002/ > > > > You can take what you want from it. > > Thanks for your kind help! > > I'll check and update this patch. Thanks! Now all the test's are getting passed on bots. Please take a look! Tomorrow I'll move this UT to components/omnibox/browser/
Description was changed from ========== [WIP] 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. BUG=544906 ========== to ========== 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 ==========
Componentized autocomplete_provider_unittest. PTAL!
droger@chromium.org changed reviewers: + pkasting@chromium.org
LGTM Adding pkasting for review. https://codereview.chromium.org/1425013002/diff/160001/components/omnibox/bro... File components/omnibox/browser/autocomplete_provider_unittest.cc (right): https://codereview.chromium.org/1425013002/diff/160001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:262: bool client_owned_; Add a comment for |client_owned_|: // Sanity check to ensure that |client_| ownership has been passed to |controller_| exactly once.
I've updated the patch. PTAL! https://codereview.chromium.org/1425013002/diff/160001/components/omnibox/bro... File components/omnibox/browser/autocomplete_provider_unittest.cc (right): https://codereview.chromium.org/1425013002/diff/160001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:262: bool client_owned_; On 2015/11/19 09:02:44, droger wrote: > Add a comment for |client_owned_|: > // Sanity check to ensure that |client_| ownership has been passed to > |controller_| exactly once. Done.
https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/bro... File components/omnibox/browser/autocomplete_provider_unittest.cc (left): https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:638: Nit: Don't remove this https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/bro... File components/omnibox/browser/autocomplete_provider_unittest.cc (right): https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:37: namespace { Nit: Blank line below https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:47: return metrics::OmniboxInputType::INVALID; Nit: Simpler: return net::URLRequest::IsHandledProtocol(scheme) ? metrics::OmniboxInputType::URL : metrics::OmniboxInputType::INVALID; https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:49: }; Nit: DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:62: return; Don't we still need to call QuitWhenIdle() if the closure is null? https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:71: }; Nit: DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:113: }; Nit: DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:190: AutocompleteProviderTest(); Nit: Constructor before destructor https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:204: protected: Nit: Remove this duplicate line https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:262: // Sanity check to ensure that |client_| ownership has been passed to Nit: Sanity check -> Used https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:265: }; Nit: DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:275: DCHECK(client_owned_); Avoid using DCHECK in tests; failed DCHECKs crash the test and leave the bot in a bad state. Find ways to test this using ASSERT_ macros instead. (Several places) https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:323: base::string16(), client_); Nit: Shorter: TestProvider* provider2 = new TestProvider( kResultsPerProvider * 2, base::ASCIIToUTF16(same_destinations ? "http://a" : "http://b"), base::string16(), client_); https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:330: client_owned_ = true; Nit: Three times you write a block of code to do: DCHECK(!client_owned_); controller_.reset(new AutocompleteController(make_scoped_ptr(client_), nullptr, type)); client_owned_ = true; Where the only difference is |type|. Factor this out into a helper function. (Note use of nullptr for NULL as well)
I've updated all comments. PTAL! https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/bro... File components/omnibox/browser/autocomplete_provider_unittest.cc (left): https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:638: On 2015/11/19 19:04:41, Peter Kasting wrote: > Nit: Don't remove this Done. https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/bro... File components/omnibox/browser/autocomplete_provider_unittest.cc (right): https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:37: namespace { On 2015/11/19 19:04:41, Peter Kasting wrote: > Nit: Blank line below Done. https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:47: return metrics::OmniboxInputType::INVALID; On 2015/11/19 19:04:41, Peter Kasting wrote: > Nit: Simpler: > > return net::URLRequest::IsHandledProtocol(scheme) ? > metrics::OmniboxInputType::URL : metrics::OmniboxInputType::INVALID; Done. https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:49: }; On 2015/11/19 19:04:41, Peter Kasting wrote: > Nit: DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:62: return; On 2015/11/19 19:04:41, Peter Kasting wrote: > Don't we still need to call QuitWhenIdle() if the closure is null? I think we can't do that, because tests are getting crashed if I add it. https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:71: }; On 2015/11/19 19:04:41, Peter Kasting wrote: > Nit: DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:113: }; On 2015/11/19 19:04:41, Peter Kasting wrote: > Nit: DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:190: AutocompleteProviderTest(); On 2015/11/19 19:04:41, Peter Kasting wrote: > Nit: Constructor before destructor Done. https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:204: protected: On 2015/11/19 19:04:41, Peter Kasting wrote: > Nit: Remove this duplicate line Done. https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:262: // Sanity check to ensure that |client_| ownership has been passed to On 2015/11/19 19:04:40, Peter Kasting wrote: > Nit: Sanity check -> Used Done. https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:265: }; On 2015/11/19 19:04:41, Peter Kasting wrote: > Nit: DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:275: DCHECK(client_owned_); On 2015/11/19 19:04:41, Peter Kasting wrote: > Avoid using DCHECK in tests; failed DCHECKs crash the test and leave the bot in > a bad state. Find ways to test this using ASSERT_ macros instead. (Several > places) As per the gtest.h we can't use ASSERT_* directly from destructor. So I'm doing this inside TearDown(). Please correct me if I'm wrong here! https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:323: base::string16(), client_); On 2015/11/19 19:04:41, Peter Kasting wrote: > Nit: Shorter: > > TestProvider* provider2 = new TestProvider( > kResultsPerProvider * 2, > base::ASCIIToUTF16(same_destinations ? "http://a" : "http://b"), > base::string16(), client_); Done. https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:330: client_owned_ = true; On 2015/11/19 19:04:41, Peter Kasting wrote: > Nit: Three times you write a block of code to do: > > DCHECK(!client_owned_); > controller_.reset(new AutocompleteController(make_scoped_ptr(client_), > nullptr, type)); > client_owned_ = true; > > Where the only difference is |type|. Factor this out into a helper function. > > (Note use of nullptr for NULL as well) Done.
LGTM. Note that some comments below are replies on the previous patch set. https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/bro... File components/omnibox/browser/autocomplete_provider_unittest.cc (right): https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:62: return; On 2015/11/20 09:37:20, Abhishek wrote: > On 2015/11/19 19:04:41, Peter Kasting wrote: > > Don't we still need to call QuitWhenIdle() if the closure is null? > > I think we can't do that, because tests are getting crashed if I add it. Can you investigate to the point of understanding why? If it turns out there's a good reason that's happening, we should put a comment here explaining why we shouldn't do this; or if it turns out the crashes are due to a bug, we should fix that. https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:275: DCHECK(client_owned_); On 2015/11/20 09:37:20, Abhishek wrote: > On 2015/11/19 19:04:41, Peter Kasting wrote: > > Avoid using DCHECK in tests; failed DCHECKs crash the test and leave the bot > in > > a bad state. Find ways to test this using ASSERT_ macros instead. (Several > > places) > > As per the gtest.h we can't use ASSERT_* directly from destructor. So I'm doing > this inside TearDown(). > Please correct me if I'm wrong here! Avoiding ASSERT in destructors is a good move. I believe you can safely use EXPECT in a destructor, though, which is probably the right thing to do anyway here, since failing this check shouldn't instantly halt the test. https://codereview.chromium.org/1425013002/diff/200001/components/omnibox/bro... File components/omnibox/browser/autocomplete_provider_unittest.cc (right): https://codereview.chromium.org/1425013002/diff/200001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:69: if (closure_.is_null()) { Nit: Can combine these conditionals: if (controller->done() && !closure_.is_null()) { ... https://codereview.chromium.org/1425013002/diff/200001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:157: ASSERT_GT(kResultsPerProvider, 0U); Don't bother checking this; this is a hardcoded constant in this file. If you're worried that someone would change this later you can simply add a comment on the constant definition noting that it has to be greater than zero and why. https://codereview.chromium.org/1425013002/diff/200001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:193: ASSERT_TRUE(match.GetTemplateURL(service, false) != nullptr); Nit: I believe with nullptr you can now do ASSERT_NE(nullptr, ...), but if not, consider dropping the "!= nullptr" part. https://codereview.chromium.org/1425013002/diff/200001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:206: void TearDown() override; Nit: Functions go below member type definitions. https://codereview.chromium.org/1425013002/diff/200001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:272: // Reset controller with provided AutocompleteProvider::Type |type| Nit: Function comments should be declarative (Resets), not imperative (Reset). Trailing period. Missing articles (the controller, the provided). |type| may be more than one type; consider using language more like what AutocompleteController() uses to describe why it takes an int for this. https://codereview.chromium.org/1425013002/diff/200001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:423: ASSERT_FALSE(client_owned_); If the test won't otherwise crash if this fails, then change to EXPECT_FALSE.
I've updated the patch. PTAL! https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/bro... File components/omnibox/browser/autocomplete_provider_unittest.cc (right): https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:62: return; On 2015/11/20 19:52:18, Peter Kasting wrote: > On 2015/11/20 09:37:20, Abhishek wrote: > > On 2015/11/19 19:04:41, Peter Kasting wrote: > > > Don't we still need to call QuitWhenIdle() if the closure is null? > > > > I think we can't do that, because tests are getting crashed if I add it. > > Can you investigate to the point of understanding why? If it turns out there's > a good reason that's happening, we should put a comment here explaining why we > shouldn't do this; or if it turns out the crashes are due to a bug, we should > fix that. AutocompleteProviderTest.AllowExactKeywordMatch AutocompleteProviderTest.ExtraQueryParams These are the cases when closure_ is null. If we call QuitWhenIdle when closure_ is null then in release build it's not crashing, but in debug build it crashes. I think I'll add TODO for me and create a bug for it. WDYT? https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:275: DCHECK(client_owned_); On 2015/11/20 19:52:18, Peter Kasting wrote: > On 2015/11/20 09:37:20, Abhishek wrote: > > On 2015/11/19 19:04:41, Peter Kasting wrote: > > > Avoid using DCHECK in tests; failed DCHECKs crash the test and leave the bot > > in > > > a bad state. Find ways to test this using ASSERT_ macros instead. (Several > > > places) > > > > As per the gtest.h we can't use ASSERT_* directly from destructor. So I'm > doing > > this inside TearDown(). > > Please correct me if I'm wrong here! > > Avoiding ASSERT in destructors is a good move. I believe you can safely use > EXPECT in a destructor, though, which is probably the right thing to do anyway > here, since failing this check shouldn't instantly halt the test. Done. https://codereview.chromium.org/1425013002/diff/200001/components/omnibox/bro... File components/omnibox/browser/autocomplete_provider_unittest.cc (right): https://codereview.chromium.org/1425013002/diff/200001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:69: if (closure_.is_null()) { On 2015/11/20 19:52:18, Peter Kasting wrote: > Nit: Can combine these conditionals: > > if (controller->done() && !closure_.is_null()) { > ... Done. https://codereview.chromium.org/1425013002/diff/200001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:157: ASSERT_GT(kResultsPerProvider, 0U); On 2015/11/20 19:52:18, Peter Kasting wrote: > Don't bother checking this; this is a hardcoded constant in this file. I believe original author intention was just to double check this before using it. If > you're worried that someone would change this later you can simply add a comment > on the constant definition noting that it has to be greater than zero and why. Anyways its a const, so its value can't be changed directly. WDYT? https://codereview.chromium.org/1425013002/diff/200001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:193: ASSERT_TRUE(match.GetTemplateURL(service, false) != nullptr); On 2015/11/20 19:52:18, Peter Kasting wrote: > Nit: I believe with nullptr you can now do ASSERT_NE(nullptr, ...), but if not, > consider dropping the "!= nullptr" part. Done. https://codereview.chromium.org/1425013002/diff/200001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:206: void TearDown() override; On 2015/11/20 19:52:18, Peter Kasting wrote: > Nit: Functions go below member type definitions. I've removed it. Because we can use destructor with EXPECT_TRUE. https://codereview.chromium.org/1425013002/diff/200001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:272: // Reset controller with provided AutocompleteProvider::Type |type| On 2015/11/20 19:52:18, Peter Kasting wrote: > Nit: Function comments should be declarative (Resets), not imperative (Reset). > Trailing period. Missing articles (the controller, the provided). |type| may > be more than one type; consider using language more like what > AutocompleteController() uses to describe why it takes an int for this. Done. https://codereview.chromium.org/1425013002/diff/200001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:423: ASSERT_FALSE(client_owned_); On 2015/11/20 19:52:18, Peter Kasting wrote: > If the test won't otherwise crash if this fails, then change to EXPECT_FALSE. Done.
https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/bro... File components/omnibox/browser/autocomplete_provider_unittest.cc (right): https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:62: return; On 2015/11/21 12:11:20, Abhishek wrote: > On 2015/11/20 19:52:18, Peter Kasting wrote: > > On 2015/11/20 09:37:20, Abhishek wrote: > > > On 2015/11/19 19:04:41, Peter Kasting wrote: > > > > Don't we still need to call QuitWhenIdle() if the closure is null? > > > > > > I think we can't do that, because tests are getting crashed if I add it. > > > > Can you investigate to the point of understanding why? If it turns out > there's > > a good reason that's happening, we should put a comment here explaining why we > > shouldn't do this; or if it turns out the crashes are due to a bug, we should > > fix that. > > AutocompleteProviderTest.AllowExactKeywordMatch > AutocompleteProviderTest.ExtraQueryParams > > These are the cases when closure_ is null. > > If we call QuitWhenIdle when closure_ is null then in release build it's not > crashing, but in debug build it crashes. Crashes how? DCHECK failure? I think you should continue investigating this at least until you completely understand the crash before checking this patch in. https://codereview.chromium.org/1425013002/diff/200001/components/omnibox/bro... File components/omnibox/browser/autocomplete_provider_unittest.cc (right): https://codereview.chromium.org/1425013002/diff/200001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:157: ASSERT_GT(kResultsPerProvider, 0U); On 2015/11/21 12:11:20, Abhishek wrote: > On 2015/11/20 19:52:18, Peter Kasting wrote: > > Don't bother checking this; this is a hardcoded constant in this file. > I believe original author intention was just to double check this before using > it. I'm saying, the double-check isn't necessary. Just remove the line entirely. It's an anti-pattern for tests to assert things about the test construction itself; they should only be checking things about the code under test.
PTAL! https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/bro... File components/omnibox/browser/autocomplete_provider_unittest.cc (right): https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:62: return; On 2015/11/21 21:08:42, Peter Kasting wrote: > On 2015/11/21 12:11:20, Abhishek wrote: > > On 2015/11/20 19:52:18, Peter Kasting wrote: > > > On 2015/11/20 09:37:20, Abhishek wrote: > > > > On 2015/11/19 19:04:41, Peter Kasting wrote: > > > > > Don't we still need to call QuitWhenIdle() if the closure is null? > > > > > > > > I think we can't do that, because tests are getting crashed if I add it. > > > > > > Can you investigate to the point of understanding why? If it turns out > > there's > > > a good reason that's happening, we should put a comment here explaining why > we > > > shouldn't do this; or if it turns out the crashes are due to a bug, we > should > > > fix that. > > > > AutocompleteProviderTest.AllowExactKeywordMatch > > AutocompleteProviderTest.ExtraQueryParams > > > > These are the cases when closure_ is null. > > > > If we call QuitWhenIdle when closure_ is null then in release build it's not > > crashing, but in debug build it crashes. > > Crashes how? Actually its crashing in both the cases(either closure_ is null or not null) into debug build. Basically its a CHECK failed terminate inside the MessageLoop while calling QuitWhenIdle(). Here are the crash logs from my local: -- [32077:32077:1123/200225:11232244126621:FATAL:message_loop.cc(302)] Check failed: false. Must be inside Run to call Quit #0 0x00000182c70e base::debug::StackTrace::StackTrace() #1 0x0000017473bf logging::LogMessage::~LogMessage() #2 0x00000175024c base::MessageLoop::QuitWhenIdle() #3 0x000000cd5b69 (anonymous namespace)::AutocompleteProviderClientWithClosure::OnAutocompleteControllerResultReady() #4 0x00000223d24f AutocompleteController::NotifyChanged() #5 0x00000223b9e6 AutocompleteController::UpdateResult() #6 0x00000223afd9 AutocompleteController::Start() #7 0x000000cd2686 AutocompleteProviderTest::RunExactKeymatchTest() -- Other function BT is also same. > DCHECK failure? No. > I think you should continue investigating this at > least until you completely understand the crash before checking this patch in. When I google it I found similar callstack related to MessageLoop at https://crbug.com/541737. But I'm not sure how much its applicable here. Could you please help me to understand this debug build terminate issue? https://codereview.chromium.org/1425013002/diff/200001/components/omnibox/bro... File components/omnibox/browser/autocomplete_provider_unittest.cc (right): https://codereview.chromium.org/1425013002/diff/200001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:157: ASSERT_GT(kResultsPerProvider, 0U); On 2015/11/21 21:08:42, Peter Kasting wrote: > On 2015/11/21 12:11:20, Abhishek wrote: > > On 2015/11/20 19:52:18, Peter Kasting wrote: > > > Don't bother checking this; this is a hardcoded constant in this file. > > I believe original author intention was just to double check this before using > > it. > > I'm saying, the double-check isn't necessary. Just remove the line entirely. > It's an anti-pattern for tests to assert things about the test construction > itself; they should only be checking things about the code under test. Sorry for the confusion here! Done.
https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/bro... File components/omnibox/browser/autocomplete_provider_unittest.cc (right): https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:62: return; On 2015/11/23 15:22:07, Abhishek wrote: > On 2015/11/21 21:08:42, Peter Kasting wrote: > > On 2015/11/21 12:11:20, Abhishek wrote: > > > On 2015/11/20 19:52:18, Peter Kasting wrote: > > > > On 2015/11/20 09:37:20, Abhishek wrote: > > > > > On 2015/11/19 19:04:41, Peter Kasting wrote: > > > > > > Don't we still need to call QuitWhenIdle() if the closure is null? > > > > > > > > > > I think we can't do that, because tests are getting crashed if I add it. > > > > > > > > Can you investigate to the point of understanding why? If it turns out > > > there's > > > > a good reason that's happening, we should put a comment here explaining > why > > we > > > > shouldn't do this; or if it turns out the crashes are due to a bug, we > > should > > > > fix that. > > > > > > AutocompleteProviderTest.AllowExactKeywordMatch > > > AutocompleteProviderTest.ExtraQueryParams > > > > > > These are the cases when closure_ is null. > > > > > > If we call QuitWhenIdle when closure_ is null then in release build it's not > > > crashing, but in debug build it crashes. > > > > Crashes how? > Actually its crashing in both the cases(either closure_ is null or not null) > into debug build. > Basically its a CHECK failed terminate inside the MessageLoop while calling > QuitWhenIdle(). > > Here are the crash logs from my local: > -- > [32077:32077:1123/200225:11232244126621:FATAL:message_loop.cc(302)] Check > failed: false. Must be inside Run to call Quit > #0 0x00000182c70e base::debug::StackTrace::StackTrace() > #1 0x0000017473bf logging::LogMessage::~LogMessage() > #2 0x00000175024c base::MessageLoop::QuitWhenIdle() > #3 0x000000cd5b69 (anonymous > namespace)::AutocompleteProviderClientWithClosure::OnAutocompleteControllerResultReady() > #4 0x00000223d24f AutocompleteController::NotifyChanged() > #5 0x00000223b9e6 AutocompleteController::UpdateResult() > #6 0x00000223afd9 AutocompleteController::Start() > #7 0x000000cd2686 AutocompleteProviderTest::RunExactKeymatchTest() > -- > Other function BT is also same. > > > > DCHECK failure? > No. > > > I think you should continue investigating this at > > least until you completely understand the crash before checking this patch in. > > When I google it I found similar callstack related to MessageLoop at > https://crbug.com/541737. But I'm not sure how much its applicable here. > > Could you please help me to understand this debug build terminate issue? I don't know much about the specifics of this test, but the change that is done in this CL is preserving the existing behavior. Before this CL, this code was run once the test registered to the NOTIFICATION_AUTOCOMPLETE_CONTROLLER_RESULT_READY notification, and was not run at all when the notification was not observed. The new code calls set_closure() instead of registering for the notification, but the behavior is otherwise similar. You can see that set_closure() is called at the same place where the notification was registered (line 338 below). Removing the early return causes the code to be called at some time when it was not run before.
On 2015/11/23 15:22:08, Abhishek wrote: > PTAL! > > https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/bro... > File components/omnibox/browser/autocomplete_provider_unittest.cc (right): > > https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/bro... > components/omnibox/browser/autocomplete_provider_unittest.cc:62: return; > On 2015/11/21 21:08:42, Peter Kasting wrote: > > On 2015/11/21 12:11:20, Abhishek wrote: > > > On 2015/11/20 19:52:18, Peter Kasting wrote: > > > > On 2015/11/20 09:37:20, Abhishek wrote: > > > > > On 2015/11/19 19:04:41, Peter Kasting wrote: > > > > > > Don't we still need to call QuitWhenIdle() if the closure is null? > > > > > > > > > > I think we can't do that, because tests are getting crashed if I add it. > > > > > > > > Can you investigate to the point of understanding why? If it turns out > > > there's > > > > a good reason that's happening, we should put a comment here explaining > why > > we > > > > shouldn't do this; or if it turns out the crashes are due to a bug, we > > should > > > > fix that. > > > > > > AutocompleteProviderTest.AllowExactKeywordMatch > > > AutocompleteProviderTest.ExtraQueryParams > > > > > > These are the cases when closure_ is null. > > > > > > If we call QuitWhenIdle when closure_ is null then in release build it's not > > > crashing, but in debug build it crashes. > > > > Crashes how? > Actually its crashing in both the cases(either closure_ is null or not null) > into debug build. I'm confused. You're saying the patch as it is on here, even without removing the closure null-check, is still crashing? That seems strange since with the null-check you should be running QuitWhenIdle() in the same cases as before. If it only crashes with the null-check removed then I think I can understand why and we just need a comment I did notice that the controller should only be calling OnAutocompleteControllerResultReady() when it's already done, so some of the done() checks in this file look skippable, but that shouldn't be affecting correctness.
On 2015/11/23 19:46:35, Peter Kasting wrote: > On 2015/11/23 15:22:08, Abhishek wrote: > > PTAL! > > > > > https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/bro... > > File components/omnibox/browser/autocomplete_provider_unittest.cc (right): > > > > > https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/bro... > > components/omnibox/browser/autocomplete_provider_unittest.cc:62: return; > > On 2015/11/21 21:08:42, Peter Kasting wrote: > > > On 2015/11/21 12:11:20, Abhishek wrote: > > > > On 2015/11/20 19:52:18, Peter Kasting wrote: > > > > > On 2015/11/20 09:37:20, Abhishek wrote: > > > > > > On 2015/11/19 19:04:41, Peter Kasting wrote: > > > > > > > Don't we still need to call QuitWhenIdle() if the closure is null? > > > > > > > > > > > > I think we can't do that, because tests are getting crashed if I add > it. > > > > > > > > > > Can you investigate to the point of understanding why? If it turns out > > > > there's > > > > > a good reason that's happening, we should put a comment here explaining > > why > > > we > > > > > shouldn't do this; or if it turns out the crashes are due to a bug, we > > > should > > > > > fix that. > > > > > > > > AutocompleteProviderTest.AllowExactKeywordMatch > > > > AutocompleteProviderTest.ExtraQueryParams > > > > > > > > These are the cases when closure_ is null. > > > > > > > > If we call QuitWhenIdle when closure_ is null then in release build it's > not > > > > crashing, but in debug build it crashes. > > > > > > Crashes how? > > Actually its crashing in both the cases(either closure_ is null or not null) > > into debug build. > > I'm confused. You're saying the patch as it is on here, even without removing > the closure null-check, is still crashing? That seems strange since with the > null-check you should be running QuitWhenIdle() in the same cases as before. If > it only crashes with the null-check removed then I think I can understand why > and we just need a comment Oops, hit send without finishing this. Actually I still *don't* fully understand why things break in this case unless they also break with the test as-is.
On 2015/11/23 19:47:23, Peter Kasting wrote: > On 2015/11/23 19:46:35, Peter Kasting wrote: > > On 2015/11/23 15:22:08, Abhishek wrote: > > > PTAL! > > > > > > > > > https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/bro... > > > File components/omnibox/browser/autocomplete_provider_unittest.cc (right): > > > > > > > > > https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/bro... > > > components/omnibox/browser/autocomplete_provider_unittest.cc:62: return; > > > On 2015/11/21 21:08:42, Peter Kasting wrote: > > > > On 2015/11/21 12:11:20, Abhishek wrote: > > > > > On 2015/11/20 19:52:18, Peter Kasting wrote: > > > > > > On 2015/11/20 09:37:20, Abhishek wrote: > > > > > > > On 2015/11/19 19:04:41, Peter Kasting wrote: > > > > > > > > Don't we still need to call QuitWhenIdle() if the closure is null? > > > > > > > > > > > > > > I think we can't do that, because tests are getting crashed if I add > > it. > > > > > > > > > > > > Can you investigate to the point of understanding why? If it turns > out > > > > > there's > > > > > > a good reason that's happening, we should put a comment here > explaining > > > why > > > > we > > > > > > shouldn't do this; or if it turns out the crashes are due to a bug, we > > > > should > > > > > > fix that. > > > > > > > > > > AutocompleteProviderTest.AllowExactKeywordMatch > > > > > AutocompleteProviderTest.ExtraQueryParams > > > > > > > > > > These are the cases when closure_ is null. > > > > > > > > > > If we call QuitWhenIdle when closure_ is null then in release build it's > > not > > > > > crashing, but in debug build it crashes. > > > > > > > > Crashes how? > > > Actually its crashing in both the cases(either closure_ is null or not null) > > > into debug build. > > > > I'm confused. You're saying the patch as it is on here, even without removing > > the closure null-check, is still crashing? That seems strange since with the > > null-check you should be running QuitWhenIdle() in the same cases as before. > If > > it only crashes with the null-check removed then I think I can understand why > > and we just need a comment > > Oops, hit send without finishing this. Actually I still *don't* fully > understand why things break in this case unless they also break with the test > as-is. Argh, that's not what I wanted to say either! Where is my brain. Take 3: if things crash without the closure null-check but not with it it means that in some scenarios before the controller was running and the message loop terminating when we hadn't actually set up the controller to notify us. I don't think that was supposed to have been happening before, so I'm still mystified how the control flow there works. OK, I think I got it that time.
On 2015/11/23 19:49:00, Peter Kasting wrote: > On 2015/11/23 19:47:23, Peter Kasting wrote: > > On 2015/11/23 19:46:35, Peter Kasting wrote: > > > On 2015/11/23 15:22:08, Abhishek wrote: > > > > PTAL! > > > > > > > > > > > > > > https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/bro... > > > > File components/omnibox/browser/autocomplete_provider_unittest.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/bro... > > > > components/omnibox/browser/autocomplete_provider_unittest.cc:62: return; > > > > On 2015/11/21 21:08:42, Peter Kasting wrote: > > > > > On 2015/11/21 12:11:20, Abhishek wrote: > > > > > > On 2015/11/20 19:52:18, Peter Kasting wrote: > > > > > > > On 2015/11/20 09:37:20, Abhishek wrote: > > > > > > > > On 2015/11/19 19:04:41, Peter Kasting wrote: > > > > > > > > > Don't we still need to call QuitWhenIdle() if the closure is > null? > > > > > > > > > > > > > > > > I think we can't do that, because tests are getting crashed if I > add > > > it. > > > > > > > > > > > > > > Can you investigate to the point of understanding why? If it turns > > out > > > > > > there's > > > > > > > a good reason that's happening, we should put a comment here > > explaining > > > > why > > > > > we > > > > > > > shouldn't do this; or if it turns out the crashes are due to a bug, > we > > > > > should > > > > > > > fix that. > > > > > > > > > > > > AutocompleteProviderTest.AllowExactKeywordMatch > > > > > > AutocompleteProviderTest.ExtraQueryParams > > > > > > > > > > > > These are the cases when closure_ is null. > > > > > > > > > > > > If we call QuitWhenIdle when closure_ is null then in release build > it's > > > not > > > > > > crashing, but in debug build it crashes. > > > > > > > > > > Crashes how? > > > > Actually its crashing in both the cases(either closure_ is null or not > null) > > > > into debug build. > > > > > > I'm confused. You're saying the patch as it is on here, even without > removing > > > the closure null-check, is still crashing? That seems strange since with > the > > > null-check you should be running QuitWhenIdle() in the same cases as before. > > > If > > > it only crashes with the null-check removed then I think I can understand > why > > > and we just need a comment > > > > Oops, hit send without finishing this. Actually I still *don't* fully > > understand why things break in this case unless they also break with the test > > as-is. > > Argh, that's not what I wanted to say either! Where is my brain. > > Take 3: if things crash without the closure null-check but not with it it means > that in some scenarios before the controller was running and the message loop > terminating when we hadn't actually set up the controller to notify us. I don't > think that was supposed to have been happening before, so I'm still mystified > how the control flow there works. > > OK, I think I got it that time. I did not try to actually run the test to check, but looking at the code I think the following is happening: The notification was registered in ResetControllerWithTestProviders(). There are several tests that are not calling ResetControllerWithTestProviders(), and thus were not registering for the notification. One of them is for example GetDestinationURL(). Presumably OnAutocompleteControllerResultReady() was called during these tests, but did nothing because the notification was not registered. In the new architecture using the closure, we need some kind of early return in these cases to match the behavior.
https://codereview.chromium.org/1425013002/diff/260001/components/omnibox/bro... File components/omnibox/browser/autocomplete_provider_unittest.cc (right): https://codereview.chromium.org/1425013002/diff/260001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:70: // closure_ is null. Remove this TODO, we need to resolve this before landing.
On 2015/11/24 15:50:02, droger wrote: > On 2015/11/23 19:49:00, Peter Kasting wrote: > > On 2015/11/23 19:47:23, Peter Kasting wrote: > > > On 2015/11/23 19:46:35, Peter Kasting wrote: > > > > On 2015/11/23 15:22:08, Abhishek wrote: > > > > > PTAL! > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/bro... > > > > > File components/omnibox/browser/autocomplete_provider_unittest.cc > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1425013002/diff/180001/components/omnibox/bro... > > > > > components/omnibox/browser/autocomplete_provider_unittest.cc:62: return; > > > > > On 2015/11/21 21:08:42, Peter Kasting wrote: > > > > > > On 2015/11/21 12:11:20, Abhishek wrote: > > > > > > > On 2015/11/20 19:52:18, Peter Kasting wrote: > > > > > > > > On 2015/11/20 09:37:20, Abhishek wrote: > > > > > > > > > On 2015/11/19 19:04:41, Peter Kasting wrote: > > > > > > > > > > Don't we still need to call QuitWhenIdle() if the closure is > > null? > > > > > > > > > > > > > > > > > > I think we can't do that, because tests are getting crashed if I > > add > > > > it. > > > > > > > > > > > > > > > > Can you investigate to the point of understanding why? If it > turns > > > out > > > > > > > there's > > > > > > > > a good reason that's happening, we should put a comment here > > > explaining > > > > > why > > > > > > we > > > > > > > > shouldn't do this; or if it turns out the crashes are due to a > bug, > > we > > > > > > should > > > > > > > > fix that. > > > > > > > > > > > > > > AutocompleteProviderTest.AllowExactKeywordMatch > > > > > > > AutocompleteProviderTest.ExtraQueryParams > > > > > > > > > > > > > > These are the cases when closure_ is null. > > > > > > > > > > > > > > If we call QuitWhenIdle when closure_ is null then in release build > > it's > > > > not > > > > > > > crashing, but in debug build it crashes. > > > > > > > > > > > > Crashes how? > > > > > Actually its crashing in both the cases(either closure_ is null or not > > null) > > > > > into debug build. > > > > > > > > I'm confused. You're saying the patch as it is on here, even without > > removing > > > > the closure null-check, is still crashing? That seems strange since with > > the > > > > null-check you should be running QuitWhenIdle() in the same cases as > before. > > > > > If > > > > it only crashes with the null-check removed then I think I can understand > > why > > > > and we just need a comment > > > > > > Oops, hit send without finishing this. Actually I still *don't* fully > > > understand why things break in this case unless they also break with the > test > > > as-is. > > > > Argh, that's not what I wanted to say either! Where is my brain. > > > > Take 3: if things crash without the closure null-check but not with it it > means > > that in some scenarios before the controller was running and the message loop > > terminating when we hadn't actually set up the controller to notify us. I > don't > > think that was supposed to have been happening before, so I'm still mystified > > how the control flow there works. > > > > OK, I think I got it that time. > > I did not try to actually run the test to check, but looking at the code I think > the following is happening: > > The notification was registered in ResetControllerWithTestProviders(). > There are several tests that are not calling ResetControllerWithTestProviders(), > and thus were not registering for the notification. > One of them is for example GetDestinationURL(). > > Presumably OnAutocompleteControllerResultReady() was called during these tests, > but did nothing because the notification was not registered. > In the new architecture using the closure, we need some kind of early return in > these cases to match the behavior. I'm trying to debug it and understand the behavior difference between old and the new version of code. I'll update it here, once I get anything.
https://codereview.chromium.org/1425013002/diff/260001/components/omnibox/bro... File components/omnibox/browser/autocomplete_provider_unittest.cc (right): https://codereview.chromium.org/1425013002/diff/260001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:70: // closure_ is null. On 2015/11/24 15:50:17, droger wrote: > Remove this TODO, we need to resolve this before landing. Done.
pkasting: can we commit this? or is there still something you need to
understand?
I patched this CL and ran it in the debugger, and I can confirm that the
test !closure_.is_null() is necessary.
Let me try to explain it again:
base::MessageLoop::current()->QuitWhenIdle()
requires a message loop to be running.
Thus, it can only be called in tests that call RunTest() (which is the only code
that runs a message loop).
For the tests that don't call RunTest(), QuitWhenIdle() must not be called. And
the closure null-check is a way to do that.
The old code behaves the same: the notification is only registered for tests
that call RunTest(), and if you register it for another test it crashes in the
same way.
Here is such a backtrace:
* frame #0: 0x000000010df95366 libbase.dylib`base::debug::BreakDebugger() + 38
at debugger_posix.cc:247
frame #1: 0x000000010e02771c
libbase.dylib`logging::LogMessage::~LogMessage(this=0x00007fff5fbfd028) + 3388
at logging.cc:721
frame #2: 0x000000010e024403
libbase.dylib`logging::LogMessage::~LogMessage(this=0x00007fff5fbfd028) + 35 at
logging.cc:501
frame #3: 0x000000010e0697ed
libbase.dylib`base::MessageLoop::QuitWhenIdle(this=0x0000000133d7f3d0) + 365 at
message_loop.cc:303
frame #4: 0x0000000101127f45 components_unittests`(anonymous
namespace)::AutocompleteProviderClientWithClosure::OnAutocompleteControllerResultReady(this=0x0000000134020e00,
controller=0x0000000133d82680) + 117 at autocomplete_provider_unittest.cc:71
frame #5: 0x000000010359eb4d
components_unittests`AutocompleteController::NotifyChanged(this=0x0000000133d82680,
notify_default_match=true) + 141 at autocomplete_controller.cc:608
frame #6: 0x0000000103599fa9
components_unittests`AutocompleteController::UpdateResult(this=0x0000000133d82680,
regenerate_result=false, force_notify_default_match_changed=true) + 6745 at
autocomplete_controller.cc:468
frame #7: 0x0000000103598162
components_unittests`AutocompleteController::Start(this=0x0000000133d82680,
input=0x00007fff5fbfe848) + 4018 at autocomplete_controller.cc:293
frame #8: 0x00000001011200c1
components_unittests`AutocompleteProviderTest::RunExactKeymatchTest(this=0x0000000133d7f330,
allow_exact_keyword_match=true) + 529 at autocomplete_provider_unittest.cc:506
frame #9: 0x000000010112189b
components_unittests`AutocompleteProviderTest_AllowExactKeywordMatch_Test::TestBody(this=0x0000000133d7f330)
+ 59 at autocomplete_provider_unittest.cc:581
The changes below should result in a test that still doesn't crash (please verify this) but is a bit safer and more robust. https://codereview.chromium.org/1425013002/diff/280001/components/omnibox/bro... File components/omnibox/browser/autocomplete_provider_unittest.cc (right): https://codereview.chromium.org/1425013002/diff/280001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:69: if (controller->done() && !closure_.is_null()) { Change this block to: if (!closure_.is_null()) closure_.Run(); if (base::MessageLoop::current()->is_running()) base::MessageLoop::current()->QuitWhenIdle(); https://codereview.chromium.org/1425013002/diff/280001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:425: RunQuery(base::ASCIIToUTF16("a")); Change this to: RunQuery("a", true); https://codereview.chromium.org/1425013002/diff/280001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:485: void AutocompleteProviderTest::RunQuery(const base::string16 query) { Change |query| to be a const std::string& and call base::ASCIIToUTF16() on it when passing to AutocompleteInput(). Add a second param to this function, "bool allow_exact_keyword_match", and pass it as the fourth-to-last argument to AutocompleteInput() (see current RunExactKeymatchTest() code for reference). https://codereview.chromium.org/1425013002/diff/280001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:509: allow_exact_keyword_match, false, false, TestingSchemeClassifier())); Change this to just do: RunQuery("k test", allow_exact_keyword_match); https://codereview.chromium.org/1425013002/diff/280001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:510: EXPECT_TRUE(controller_->done()); Remove this statement.
PTAL! https://codereview.chromium.org/1425013002/diff/280001/components/omnibox/bro... File components/omnibox/browser/autocomplete_provider_unittest.cc (right): https://codereview.chromium.org/1425013002/diff/280001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:69: if (controller->done() && !closure_.is_null()) { On 2015/12/04 20:43:48, Peter Kasting wrote: > Change this block to: > > if (!closure_.is_null()) > closure_.Run(); > if (base::MessageLoop::current()->is_running()) > base::MessageLoop::current()->QuitWhenIdle(); Done. https://codereview.chromium.org/1425013002/diff/280001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:425: RunQuery(base::ASCIIToUTF16("a")); On 2015/12/04 20:43:48, Peter Kasting wrote: > Change this to: > > RunQuery("a", true); Done. https://codereview.chromium.org/1425013002/diff/280001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:485: void AutocompleteProviderTest::RunQuery(const base::string16 query) { On 2015/12/04 20:43:48, Peter Kasting wrote: > Change |query| to be a const std::string& and call base::ASCIIToUTF16() on it > when passing to AutocompleteInput(). > > Add a second param to this function, "bool allow_exact_keyword_match", and pass > it as the fourth-to-last argument to AutocompleteInput() (see current > RunExactKeymatchTest() code for reference). Done. https://codereview.chromium.org/1425013002/diff/280001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:509: allow_exact_keyword_match, false, false, TestingSchemeClassifier())); On 2015/12/04 20:43:47, Peter Kasting wrote: > Change this to just do: > > RunQuery("k test", allow_exact_keyword_match); Done. https://codereview.chromium.org/1425013002/diff/280001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:510: EXPECT_TRUE(controller_->done()); On 2015/12/04 20:43:48, Peter Kasting wrote: > Remove this statement. Done.
I've checked both debug and release build. All tests are getting passed in my local with latest patchset. Thanks! PTAL!
@pkasting: PTAL #14!
@pkasting: PTAL #14!
LGTM https://codereview.chromium.org/1425013002/diff/300001/components/omnibox/bro... File components/omnibox/browser/autocomplete_provider_unittest.cc (right): https://codereview.chromium.org/1425013002/diff/300001/components/omnibox/bro... 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(), false)); https://codereview.chromium.org/1425013002/diff/300001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:286: client_->set_template_url_service(template_url_service.Pass()); Nit: Can this just be this?: client_->set_template_url_service( make_scoped_ptr(new TemplateURLService(nullptr, 0))); If not, I think we're converting .Pass() to std::move()? (Not sure)
Updated in patchset #15! https://codereview.chromium.org/1425013002/diff/300001/components/omnibox/bro... File components/omnibox/browser/autocomplete_provider_unittest.cc (right): https://codereview.chromium.org/1425013002/diff/300001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:188: ASSERT_NE(nullptr, match.GetTemplateURL(service, false)); On 2015/12/08 20:51:34, Peter Kasting wrote: > Nit: Or just: > > ASSERT_NE(nullptr, > match.GetTemplateURL(client_->GetTemplateURLService(), false)); Done. https://codereview.chromium.org/1425013002/diff/300001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:286: client_->set_template_url_service(template_url_service.Pass()); On 2015/12/08 20:51:34, Peter Kasting wrote: > Nit: Can this just be this?: > > client_->set_template_url_service( > make_scoped_ptr(new TemplateURLService(nullptr, 0))); > > If not, I think we're converting .Pass() to std::move()? (Not sure) Done.
Updated in patchset #15! https://codereview.chromium.org/1425013002/diff/300001/components/omnibox/bro... File components/omnibox/browser/autocomplete_provider_unittest.cc (right): https://codereview.chromium.org/1425013002/diff/300001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:188: ASSERT_NE(nullptr, match.GetTemplateURL(service, false)); On 2015/12/08 20:51:34, Peter Kasting wrote: > Nit: Or just: > > ASSERT_NE(nullptr, > match.GetTemplateURL(client_->GetTemplateURLService(), false)); Done. https://codereview.chromium.org/1425013002/diff/300001/components/omnibox/bro... components/omnibox/browser/autocomplete_provider_unittest.cc:286: client_->set_template_url_service(template_url_service.Pass()); On 2015/12/08 20:51:34, Peter Kasting wrote: > Nit: Can this just be this?: > > client_->set_template_url_service( > make_scoped_ptr(new TemplateURLService(nullptr, 0))); > > If not, I think we're converting .Pass() to std::move()? (Not sure) Done.
The CQ bit was checked by abhishek.a21@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from droger@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/1425013002/#ps320001 (title: "updated")
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
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 ==========
Message was sent while issue was closed.
Committed patchset #15 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/877c36928f02350da8e1e38a0e549ddd6f946e0c Cr-Commit-Position: refs/heads/master@{#363992} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
