Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
371743002:
Remove unnecessary scoped_ptr in password_form_manager_unittest.cc (Part 1) (Closed)
|
Created:
6 years, 5 months ago by lucinka.brozkova Modified:
6 years, 5 months ago CC:
chromium-reviews, gcasto+watchlist_chromium.org, mkwst+watchlist_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionRemove unnecessary scoped_ptr in password_form_manager_unittest.cc (Part 1)
Simple test code refactoring, no behaviour change.
Part 2 coming soon.
BUG=391487
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=281618
Patch Set 1 #
Total comments: 12
Patch Set 2 : Comments addressed #Patch Set 3 : Just rebased #Messages
Total messages: 10 (0 generated)
Hi Balázs and Václav, PTAL, this is big enough for one CL, but there will be more refactoring next week. Cheers, Lucie
LGTM, thanks! Looking forward to part 2, hoping you remove all the rest of unnecessary heap allocation. :) Cheers, Vaclav
LGTM with comments. Thanks for cleaning this up! https://codereview.chromium.org/371743002/diff/1/components/password_manager/... File components/password_manager/core/browser/password_form_manager_unittest.cc (right): https://codereview.chromium.org/371743002/diff/1/components/password_manager/... components/password_manager/core/browser/password_form_manager_unittest.cc:106: PasswordFormManagerTest() : client_(NULL) {} nit: client_(NULL /*password_store*/) https://codereview.chromium.org/371743002/diff/1/components/password_manager/... components/password_manager/core/browser/password_form_manager_unittest.cc:501: // A dumb password manager. Could you please remove this comment? I think it does not add much value. https://codereview.chromium.org/371743002/diff/1/components/password_manager/... components/password_manager/core/browser/password_form_manager_unittest.cc:652: MockPasswordManagerDriver driver; Could you please check if there is any reason to use a separate |driver| here, instead of the one built into the |client_with_store|? If not, we could simplify here as well. https://codereview.chromium.org/371743002/diff/1/components/password_manager/... components/password_manager/core/browser/password_form_manager_unittest.cc:709: MockPasswordManagerDriver driver; Same here. https://codereview.chromium.org/371743002/diff/1/components/password_manager/... components/password_manager/core/browser/password_form_manager_unittest.cc:750: NULL, client(), NULL, invalid_action_form, false); Although unlikely, could you please double-check here that having a NULL driver in both cases (instead of the same stub driver), does not lower the bar for passing this test?
https://codereview.chromium.org/371743002/diff/1/components/password_manager/... File components/password_manager/core/browser/password_form_manager_unittest.cc (right): https://codereview.chromium.org/371743002/diff/1/components/password_manager/... components/password_manager/core/browser/password_form_manager_unittest.cc:750: NULL, client(), NULL, invalid_action_form, false); On 2014/07/07 10:01:43, engedy wrote: > Although unlikely, could you please double-check here that having a NULL driver > in both cases (instead of the same stub driver), does not lower the bar for > passing this test? @engedy: Note, that it was NULL before. |driver| was just a scoped_ptr, which was never assigned an allocated object.
https://codereview.chromium.org/371743002/diff/1/components/password_manager/... File components/password_manager/core/browser/password_form_manager_unittest.cc (right): https://codereview.chromium.org/371743002/diff/1/components/password_manager/... components/password_manager/core/browser/password_form_manager_unittest.cc:750: NULL, client(), NULL, invalid_action_form, false); On 2014/07/07 12:18:03, vabr (Chromium) wrote: > On 2014/07/07 10:01:43, engedy wrote: > > Although unlikely, could you please double-check here that having a NULL > driver > > in both cases (instead of the same stub driver), does not lower the bar for > > passing this test? > > @engedy: Note, that it was NULL before. |driver| was just a scoped_ptr, which > was never assigned an allocated object. Oh, I am an idiot. Please ignore this comment.
Hi Balázs and Václav, Thank you for your reviews. I addressed all your comments. I will send this to commit queue. Cheers, Lucie https://codereview.chromium.org/371743002/diff/1/components/password_manager/... File components/password_manager/core/browser/password_form_manager_unittest.cc (right): https://codereview.chromium.org/371743002/diff/1/components/password_manager/... components/password_manager/core/browser/password_form_manager_unittest.cc:106: PasswordFormManagerTest() : client_(NULL) {} On 2014/07/07 10:01:43, engedy wrote: > nit: client_(NULL /*password_store*/) Done. https://codereview.chromium.org/371743002/diff/1/components/password_manager/... components/password_manager/core/browser/password_form_manager_unittest.cc:501: // A dumb password manager. On 2014/07/07 10:01:43, engedy wrote: > Could you please remove this comment? I think it does not add much value. Done. https://codereview.chromium.org/371743002/diff/1/components/password_manager/... components/password_manager/core/browser/password_form_manager_unittest.cc:652: MockPasswordManagerDriver driver; On 2014/07/07 10:01:43, engedy wrote: > Could you please check if there is any reason to use a separate |driver| here, > instead of the one built into the |client_with_store|? If not, we could simplify > here as well. I checked it is the same class and also no additional expectations were declared for the TestPasswordManagerClient instance. https://codereview.chromium.org/371743002/diff/1/components/password_manager/... components/password_manager/core/browser/password_form_manager_unittest.cc:709: MockPasswordManagerDriver driver; On 2014/07/07 10:01:43, engedy wrote: > Same here. Done. https://codereview.chromium.org/371743002/diff/1/components/password_manager/... components/password_manager/core/browser/password_form_manager_unittest.cc:750: NULL, client(), NULL, invalid_action_form, false); On 2014/07/07 12:36:16, engedy wrote: > On 2014/07/07 12:18:03, vabr (Chromium) wrote: > > On 2014/07/07 10:01:43, engedy wrote: > > > Although unlikely, could you please double-check here that having a NULL > > driver > > > in both cases (instead of the same stub driver), does not lower the bar for > > > passing this test? > > > > @engedy: Note, that it was NULL before. |driver| was just a scoped_ptr, which > > was never assigned an allocated object. > > Oh, I am an idiot. Please ignore this comment. Done.
The CQ bit was checked by lucinka.brozkova@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/lucinka.brozkova@gmail.com/371743002/4...
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...)
Message was sent while issue was closed.
Change committed as 281618 |
