Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
403323004:
C++ readability CL. (Closed)
|
Created:
6 years, 5 months ago by vasilii Modified:
6 years, 3 months ago CC:
chromium-reviews, gcasto+watchlist_chromium.org, mkwst+watchlist_chromium.org, Garrett Casto, Ilya Sherman, Nicolas Zea Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionC++ readability CL.
My changes are mostly in these CLs:
https://codereview.chromium.org/131003006
https://codereview.chromium.org/139443004
https://codereview.chromium.org/183223003
https://codereview.chromium.org/246253014
https://codereview.chromium.org/226523003
Committed: https://crrev.com/9db0a1d04fb44ff57833f94d1c68e50d062c1dc9
Cr-Commit-Position: refs/heads/master@{#293107}
Patch Set 1 #
Total comments: 71
Patch Set 2 : Addressed comments in PasswordSyncableService #
Total comments: 62
Patch Set 3 : GMock #
Total comments: 63
Patch Set 4 : more comments #
Total comments: 14
Patch Set 5 : remove PasswordsEqual #
Total comments: 12
Patch Set 6 : remove NotifyLoginsChanged #Patch Set 7 : fix lint errors #Patch Set 8 : Rebase on trunk #Patch Set 9 : Work around stupid compilers #
Total comments: 69
Patch Set 10 : addressed Ilya's comments #
Total comments: 4
Patch Set 11 : addressed comments on comments #
Total comments: 2
Patch Set 12 : Final comment #
Messages
Total messages: 36 (1 generated)
Hi Dan, please review this readabilty CL. Thanks,
Thanks for participating in the readability process. It looks like you're already doing a good job of following the letter of the style guide, but you do still have some things to tighten up. Also, readability is not just about following mechanical rules, but making code as understandable as possible. I haven't gotten to your tests yet, but will on the next pass. There's a good chance my comments so far will also apply to the other places, so please make improvements wherever applicable. Please address each of my comments, but if I've misunderstood something that's obvious to those on your project or your group has strong local conventions I've made suggestions against, feel free to argue that I'm wrong. Like all code reviews, the goal is not adversarial but cooperative. In particular, this is only the second Chromium readability review I've seen, so feel free to point out where Chromium practices differ from what I've suggested. https://codereview.chromium.org/403323004/diff/1/components/password_manager/... File components/password_manager/core/browser/password_syncable_service.cc (right): https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.cc:19: namespace { For functions and variables (but not classes), you can use either anonymous namespaces or 'static' for things private to the .cc file. If the code in this area has a strong convention, you should probably follow that, but personally, I find the anon namespace worse that 'static' because it means that the reader has to look around the surrounding context of the function to see that it's private, rather than just looking at the declaration. 'static' also makes it easier to keep helper functions very close to the code that uses them, rather than being tempted to dump all the helpers in a single anon namespace at the top of the file. https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.cc:23: IDENTICAL, Has Chromium switched to the kIdentical/kSync/kLocal format for enums? http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Enumer... https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.cc:55: return IDENTICAL; for this return value, you don't assign to *new_password_form, unlike in the other two cases. that should be documented. But the fact that you've got three very different behaviors in this function depending on which of the three values return strongly suggests that this function shouldn't be doing that work. Instead, just define bool AreLocalAndSyncPasswordsEqual(...password_specificas, password_form); and then have your one call site of this function do the work of if (!AreLocalAndSyncPasswordsEqual(...)) { if (...date_created) { ...SyncDataFromPassword(*existing_local_entry_iter->second)... } else { scoped_ptr<autofill::PasswordForm> new_password(new autofill::PasswordForm); PasswordFromSpecifics(password_specifics, new_password.get()); ... } which actually not only makes this function simpler, but it makes the call site simpler, too! https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.cc:67: } move this function (or what's left of it) down near its only use. https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.cc:69: std::string MakePasswordSyncTag(const std::string& origin_url, This is moderately risky, having so many parameters of the same type. Would you notice if one of your two call sites swapped a couple of its arguments? Unless the parameters have some very strong natural ordering (such as an x,y,z triplet, for example), it's better to pass things like this as structured data with named fields. At the absolute bare minimum, move this function and the next one down to right near the public function that has to have the same order of args, and possibly add a warning comment about the risk. If the performance requirements of your class are gentle enough, it might even be worth having the version that takes a PasswordForm construct (stack local) PasswordSpecificData, and have the PasswordSpecificData version actually do the escaping and concatenation (i.e, have only two versions, both that use named fields). https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.cc:81: std::string MakePasswordSyncTag(const autofill::PasswordForm& password) { http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... Overloading is at least pretty uncommon. I does make sense in this case, it's a bit confusing for a particular name to be a mix of public and non-public overloads. I think that could use a comment. https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.cc:89: syncer::SyncChange::SyncChangeType GetSyncChangeType( it would probably be clearer to move this closer to its only call site. https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.cc:141: "Failed to get passwords from store.")); fits easily on previous line https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.cc:179: ++it) { you're not consistent about whether the "++" part of your for loops get their own line. in general, I personally think it's always better to use fewer lines when you can. https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.cc:276: return; http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Condit... I believe this is the first braceless conditional I've seen you use. Are these used anywhere in the surrounding codebase? Braces aren't required by the whole style guide, but are avoided in some parts of the codebase. Personally, I find it helpful to have braces around conditional flow control (like the "return" here) because it makes it much easier to see which indented lines need to be thought about rather than being some sort of unconditional expression that exceeds one line. You also have the option of if (blah) { return; } which is a nice mix of terseness and having braces as a clear marker of not being unconditional. https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.cc:317: passwords_entry_map->insert( if you're just ignoring the result of .insert(), then you might as well use the more intuitive (*passwords_entry_map)[MakePasswordSyncTag(*password_form)] = password_form; if you don't like the (*...) that needs, you can use a function-local non-const ref. (Non-const refs are only banned in function signatures.) https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.cc:329: for (std::vector<autofill::PasswordForm*>::const_iterator it = It's pretty unfortunate to have three nearly-identical copies of code here; it makes it very hard to see what's different between the three loops (and I initially wrote this comment differently, thinking that the three loops differed only in what vector was being added to 'changes'). Fortunately, it's easy to solve this, even in C++98. AppendChanges(&PasswordStoreSync::AddLoginImpl, new_entries, password_store_, &changes); AppendChanges(&PasswordStoreSync::UpdateLoginImpl, updated_entries, password_store_, &changes); ... All that requires is void AppendChanges( PasswordStoreChangeList (PasswordStoreSync::* modify)( const autofill::PasswordForm& form), vector<...>...) { for (...it...entries...) { PasswordStoreChangeList new_changes = (store->*modify)(**it); ... I personally generally avoid private methods, but making AppendChanges one would avoid having to pass password_store_ as an arg. https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.cc:414: password_specifics->set_scheme(password_form.scheme); while macros should be rare, there are times that they're a sufficient win, especially if they can be restricted to a small scope to avoid misuse. #define CopyField(field) \ password_specifics->set_ ## field(password_form.field) CopyField(scheme); CopyField(signon_realm); ...set_origin(...); // keep the parts that the macro just doesn't cover. CopyField(times_used); #undef CopyField https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.cc:414: password_specifics->set_scheme(password_form.scheme); whether or not you use a macro, you could make this function less hard to read by avoiding repeating the word "password" a bazillion times: specifics->set_scheme(form.scheme); It's not like the reader is going to lack enough context to tell that these things are about passwords, what with all the other things named that here. https://codereview.chromium.org/403323004/diff/1/components/password_manager/... File components/password_manager/core/browser/password_syncable_service.h (right): https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.h:25: namespace autofill { http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp... You have the option of namespace autofill { struct PasswordForm; } On the other hand, I see very few such usages in Chromium as yet. https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.h:37: class PasswordSyncableService : public syncer::SyncableService, http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Class_Comments Document the class. https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.h:40: // |PasswordSyncableService| is owned by |PasswordStore|. There is no PasswordStore here, only a PasswordStoreSync, which is assume what you mean. Normally, it doesn't make a lot of sense for a class to specify who owns instances of the class; if some client of the class creates and instance and then passes it to some other client, that's not your class's problem to worry about. Instead, you should be documenting the ownership of any pointers your class holds; so, document the fact that your constructor does not take ownership of 'password_store'. If you *also* want to document that the PasswordSyncableService is generally owned by the PasswordStoreSync that it uses, do so, but then you should probably say it's usually owned by 'password_store' to make clear that it's not just the same class, but the same instance. https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.h:44: // syncer::SyncableServiceImplementations s/Implementations/ implementation/, right? most importantly, you need a space. https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.h:57: // Notifies sync of changes to the password database. What does "sync" mean here? This password service? https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.h:61: // sync_start_util for more. make 'sync_start_util' more explicit, such as a full path relative to src/. https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.h:62: void InjectStartSyncFlare( sync_stat_util.h says "Typically this would be given to the SyncableService on construction.". Why is that not the case here? Since there's not constraint that the flare be specified at c'tor time, are there any constraints on when the flare must be injected by? is it ok to inject it after ActOnPasswordStoreChanges() has been called? https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.h:70: // Helper function to retrieve the entries from password db and fill both "Helper function" doesn't provide any information to the reader. https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.h:83: // Virtual so tests can override. Confusing. This allows a non-friend class to affect how your class works. That's okay, but should be make clearer, by making this method protected instead of private. Do keep the comment that the ability to override is for tests. https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.h:87: // Checks if |data|, the entry in sync db, needs to be created or updated should "the entry" here be "an entry"? surely the sync db has more than one entry. https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.h:88: // in the passwords db. Depending on what action needs to be performed, the This level of detail in a function comment can be excessive, especially for a function that is not a public contract with a client. You can avoid much of this by using a phrase such as "as appropriate". For example, it might be enough to say // Decides what action is needed for |data|, which must be an entry in the sync db, updating other parameters as needed. An element is removed from |unmatched_data_from_password_db| if its tag is identical to |data|'s. Having less detail makes it more likely that the comment remains true as the code evolves over time, as well as making it easier for the reader to understand the intended purpose of the function without understanding its implementation. https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.h:93: // an entry's tag in |umatched_data_from_password_db| then that entry will be spelling: s/umatched/unmatched/g https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.h:106: // |SyncProcessor| will mirror the |PasswordStore| changes in the sync db. Do you mean |sync_processor_| here? https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.h:109: // The password store that adds/updates/deletes password entries. Add "Not owned." https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.h:109: // The password store that adds/updates/deletes password entries. aren't adding and deleting kinds of updates? just say "updates". https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.h:112: // A signal to start sync as soon as possible. clarify that this is a signal that this class can activate, not one it listens to. https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.h:115: // True if processing sync changes is in progress. what is the effect of this? if your class is not thread-safe, why would it ever matter? https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.h:122: syncer::SyncData SyncDataFromPassword(const autofill::PasswordForm& password); this isn't used outside of your class and its test, is it? Move this inside your class as a static function, so that it's not as public. Since the function creates a new value and can't be used to break any of the internal constraints of your class, you can put the function in the public section. It's common to put such things at the bottom of the public section (still above the protected section) with a comment like // Only exposed for testing: All the same applies to the next two functions, too.
Perspectives of a Chromium reviewer; take with a grain of salt :) Thanks for the review, Dan! https://codereview.chromium.org/403323004/diff/1/components/password_manager/... File components/password_manager/core/browser/password_syncable_service.cc (right): https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.cc:19: namespace { On 2014/08/07 05:26:53, manthd wrote: > For functions and variables (but not classes), you can use either anonymous > namespaces or 'static' for things private to the .cc file. > > If the code in this area has a strong convention, you should probably follow > that, but personally, I find the anon namespace worse that 'static' because it > means that the reader has to look around the surrounding context of the function > to see that it's private, rather than just looking at the declaration. > > 'static' also makes it easier to keep helper functions very close to the code > that uses them, rather than being tempted to dump all the helpers in a single > anon namespace at the top of the file. FWIW, my experience has been that the Chromium codebase has a strong convention of preferring a single anonymous namespace at the top of the file, rather than sprinkling static bits throughout, possibly because helper classes must be declared in an anonymous namespace anyway. https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.cc:23: IDENTICAL, On 2014/08/07 05:26:53, manthd wrote: > Has Chromium switched to the kIdentical/kSync/kLocal format for enums? > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Enumer... Nope, sadly we haven't. https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.cc:276: return; On 2014/08/07 05:26:52, manthd wrote: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Condit... > > I believe this is the first braceless conditional I've seen you use. Are these > used anywhere in the surrounding codebase? Braces aren't required by the whole > style guide, but are avoided in some parts of the codebase. > > Personally, I find it helpful to have braces around conditional flow control > (like the "return" here) because it makes it much easier to see which indented > lines need to be thought about rather than being some sort of unconditional > expression that exceeds one line. > > You also have the option of > if (blah) { return; } > which is a nice mix of terseness and having braces as a clear marker of not > being unconditional. FWIW, this last version is very uncommon in the Chromium codebase, to the point where most editors of the code would change it back. https://codereview.chromium.org/403323004/diff/1/components/password_manager/... File components/password_manager/core/browser/password_syncable_service.h (right): https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.h:25: namespace autofill { On 2014/08/07 05:26:54, manthd wrote: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp... > You have the option of > > namespace autofill { struct PasswordForm; } > > On the other hand, I see very few such usages in Chromium as yet. Indeed, this would pretty uncommon style in the Chromium repository. https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.h:40: // |PasswordSyncableService| is owned by |PasswordStore|. On 2014/08/07 05:26:54, manthd wrote: > There is no PasswordStore here, only a PasswordStoreSync, which is assume what > you mean. > > Normally, it doesn't make a lot of sense for a class to specify who owns > instances of the class; if some client of the class creates and instance and > then passes it to some other client, that's not your class's problem to worry > about. > > Instead, you should be documenting the ownership of any pointers your class > holds; so, document the fact that your constructor does not take ownership of > 'password_store'. > > If you *also* want to document that the PasswordSyncableService is generally > owned by the PasswordStoreSync that it uses, do so, but then you should probably > say it's usually owned by 'password_store' to make clear that it's not just the > same class, but the same instance. FWIW, I requested adding a comment along these lines to clarify that |password_store| is guaranteed to outlive |this|. I agree that your suggestion is an improvement in clarity :) https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.h:109: // The password store that adds/updates/deletes password entries. On 2014/08/07 05:26:53, manthd wrote: > aren't adding and deleting kinds of updates? just say "updates". These are the technical terms that the sync code uses for different types of changes to sync data.
I addressed almost all of them in https://codereview.chromium.org/447333002. Except those which Ilya invalidated above. https://codereview.chromium.org/403323004/diff/1/components/password_manager/... File components/password_manager/core/browser/password_syncable_service.cc (right): https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.cc:67: } On 2014/08/07 05:26:52, manthd wrote: > move this function (or what's left of it) down near its only use. We prefer single anonymous namespace in Chrome. https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.cc:69: std::string MakePasswordSyncTag(const std::string& origin_url, On 2014/08/07 05:26:52, manthd wrote: > This is moderately risky, having so many parameters of the same type. Would you > notice if one of your two call sites swapped a couple of its arguments? > > Unless the parameters have some very strong natural ordering (such as an x,y,z > triplet, for example), it's better to pass things like this as structured data > with named fields. > > At the absolute bare minimum, move this function and the next one down to right > near the public function that has to have the same order of args, and possibly > add a warning comment about the risk. > > If the performance requirements of your class are gentle enough, it might even > be worth having the version that takes a PasswordForm construct (stack local) > PasswordSpecificData, and have the PasswordSpecificData version actually do the > escaping and concatenation (i.e, have only two versions, both that use named > fields). Done. https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.cc:81: std::string MakePasswordSyncTag(const autofill::PasswordForm& password) { On 2014/08/07 05:26:53, manthd wrote: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... > > Overloading is at least pretty uncommon. I does make sense in this case, it's a > bit confusing for a particular name to be a mix of public and non-public > overloads. I think that could use a comment. Done. https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.cc:89: syncer::SyncChange::SyncChangeType GetSyncChangeType( On 2014/08/07 05:26:53, manthd wrote: > it would probably be clearer to move this closer to its only call site. Impossible if we want to have it in the anonymous namespace. https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.cc:141: "Failed to get passwords from store.")); On 2014/08/07 05:26:53, manthd wrote: > fits easily on previous line From what I saw in the codebase, we always put FROM_HERE on the separate line. https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.cc:179: ++it) { On 2014/08/07 05:26:53, manthd wrote: > you're not consistent about whether the "++" part of your for loops get their > own line. > > in general, I personally think it's always better to use fewer lines when you > can. Here I agree. I usually put it on the same line. https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.cc:276: return; On 2014/08/07 07:36:14, Ilya Sherman wrote: > On 2014/08/07 05:26:52, manthd wrote: > > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Condit... > > > > I believe this is the first braceless conditional I've seen you use. Are > these > > used anywhere in the surrounding codebase? Braces aren't required by the > whole > > style guide, but are avoided in some parts of the codebase. > > > > Personally, I find it helpful to have braces around conditional flow control > > (like the "return" here) because it makes it much easier to see which indented > > lines need to be thought about rather than being some sort of unconditional > > expression that exceeds one line. > > > > You also have the option of > > if (blah) { return; } > > which is a nice mix of terseness and having braces as a clear marker of not > > being unconditional. > > FWIW, this last version is very uncommon in the Chromium codebase, to the point > where most editors of the code would change it back. I always use if (condition) return smth; In these files it's always like this. https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.cc:317: passwords_entry_map->insert( On 2014/08/07 05:26:53, manthd wrote: > if you're just ignoring the result of .insert(), then you might as well use the > more intuitive > (*passwords_entry_map)[MakePasswordSyncTag(*password_form)] = password_form; > > if you don't like the (*...) that needs, you can use a function-local non-const > ref. (Non-const refs are only banned in function signatures.) > Done. https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.cc:329: for (std::vector<autofill::PasswordForm*>::const_iterator it = On 2014/08/07 05:26:52, manthd wrote: > It's pretty unfortunate to have three nearly-identical copies of code here; it > makes it very hard to see what's different between the three loops (and I > initially wrote this comment differently, thinking that the three loops differed > only in what vector was being added to 'changes'). > > Fortunately, it's easy to solve this, even in C++98. > AppendChanges(&PasswordStoreSync::AddLoginImpl, new_entries, > password_store_, &changes); > AppendChanges(&PasswordStoreSync::UpdateLoginImpl, updated_entries, > password_store_, &changes); > ... > > All that requires is > void AppendChanges( > PasswordStoreChangeList (PasswordStoreSync::* modify)( > const autofill::PasswordForm& form), > vector<...>...) { > for (...it...entries...) { > PasswordStoreChangeList new_changes = (store->*modify)(**it); > ... > > I personally generally avoid private methods, but making AppendChanges one would > avoid having to pass password_store_ as an arg. Done. https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.cc:414: password_specifics->set_scheme(password_form.scheme); On 2014/08/07 05:26:53, manthd wrote: > whether or not you use a macro, you could make this function less hard to read > by avoiding repeating the word "password" a bazillion times: > specifics->set_scheme(form.scheme); > > It's not like the reader is going to lack enough context to tell that these > things are about passwords, what with all the other things named that here. I am not sure I like macros but Done. https://codereview.chromium.org/403323004/diff/1/components/password_manager/... File components/password_manager/core/browser/password_syncable_service.h (right): https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.h:37: class PasswordSyncableService : public syncer::SyncableService, On 2014/08/07 05:26:53, manthd wrote: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Class_Comments > > Document the class. Done. https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.h:40: // |PasswordSyncableService| is owned by |PasswordStore|. On 2014/08/07 07:36:14, Ilya Sherman wrote: > On 2014/08/07 05:26:54, manthd wrote: > > There is no PasswordStore here, only a PasswordStoreSync, which is assume what > > you mean. > > > > Normally, it doesn't make a lot of sense for a class to specify who owns > > instances of the class; if some client of the class creates and instance and > > then passes it to some other client, that's not your class's problem to worry > > about. > > > > Instead, you should be documenting the ownership of any pointers your class > > holds; so, document the fact that your constructor does not take ownership of > > 'password_store'. > > > > If you *also* want to document that the PasswordSyncableService is generally > > owned by the PasswordStoreSync that it uses, do so, but then you should > probably > > say it's usually owned by 'password_store' to make clear that it's not just > the > > same class, but the same instance. > > FWIW, I requested adding a comment along these lines to clarify that > |password_store| is guaranteed to outlive |this|. I agree that your suggestion > is an improvement in clarity :) Done. https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.h:44: // syncer::SyncableServiceImplementations On 2014/08/07 05:26:54, manthd wrote: > s/Implementations/ implementation/, right? most importantly, you need a space. Done. https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.h:57: // Notifies sync of changes to the password database. On 2014/08/07 05:26:53, manthd wrote: > What does "sync" mean here? This password service? We usually refer to "sync" as infrastructure that pushes/pulls the changes from the cloud. https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.h:61: // sync_start_util for more. On 2014/08/07 05:26:53, manthd wrote: > make 'sync_start_util' more explicit, such as a full path relative to src/. Done. https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.h:62: void InjectStartSyncFlare( On 2014/08/07 05:26:54, manthd wrote: > sync_stat_util.h says "Typically this would be given to the SyncableService on > construction.". Why is that not the case here? > > Since there's not constraint that the flare be specified at c'tor time, are > there any constraints on when the flare must be injected by? is it ok to inject > it after ActOnPasswordStoreChanges() has been called? We need this method anyway for unit tests. I decided not to put it in the constructor because it's not so important parameter. There are no constraints. It can be called after ActOnPasswordStoreChanges(). However, it makes sense to call it after construction. https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.h:70: // Helper function to retrieve the entries from password db and fill both On 2014/08/07 05:26:54, manthd wrote: > "Helper function" doesn't provide any information to the reader. Done. https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.h:83: // Virtual so tests can override. On 2014/08/07 05:26:53, manthd wrote: > Confusing. This allows a non-friend class to affect how your class works. > That's okay, but should be make clearer, by making this method protected instead > of private. Do keep the comment that the ability to override is for tests. Done. https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.h:87: // Checks if |data|, the entry in sync db, needs to be created or updated On 2014/08/07 05:26:54, manthd wrote: > should "the entry" here be "an entry"? surely the sync db has more than one > entry. Done. https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.h:88: // in the passwords db. Depending on what action needs to be performed, the On 2014/08/07 05:26:54, manthd wrote: > This level of detail in a function comment can be excessive, especially for a > function that is not a public contract with a client. You can avoid much of > this by using a phrase such as "as appropriate". > > For example, it might be enough to say > // Decides what action is needed for |data|, which must be an entry in the > sync db, updating other parameters as needed. An element is removed from > |unmatched_data_from_password_db| if its tag is identical to |data|'s. > > Having less detail makes it more likely that the comment remains true as the > code evolves over time, as well as making it easier for the reader to understand > the intended purpose of the function without understanding its implementation. Done. https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.h:106: // |SyncProcessor| will mirror the |PasswordStore| changes in the sync db. On 2014/08/07 05:26:54, manthd wrote: > Do you mean |sync_processor_| here? Done. https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.h:109: // The password store that adds/updates/deletes password entries. On 2014/08/07 05:26:54, manthd wrote: > Add "Not owned." Done. https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.h:112: // A signal to start sync as soon as possible. On 2014/08/07 05:26:54, manthd wrote: > clarify that this is a signal that this class can activate, not one it listens > to. Done. https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.h:115: // True if processing sync changes is in progress. On 2014/08/07 05:26:54, manthd wrote: > what is the effect of this? if your class is not thread-safe, why would it ever > matter? It's not thread-safe. However, this variable is used to distinguish updates from user and updates from this class. If we push something from the cloud to database we don't want to push the same changes back to Sync. https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.h:122: syncer::SyncData SyncDataFromPassword(const autofill::PasswordForm& password); On 2014/08/07 05:26:54, manthd wrote: > this isn't used outside of your class and its test, is it? > > Move this inside your class as a static function, so that it's not as public. > > Since the function creates a new value and can't be used to break any of the > internal constraints of your class, you can put the function in the public > section. It's common to put such things at the bottom of the public section > (still above the protected section) with a comment like > // Only exposed for testing: > > All the same applies to the next two functions, too. Done. Though there are disadvantages to this approach. We have 3 different MakePasswordSyncTag for convenience. Now they are from different scopes. I think it complicated the code.
I uploaded everything here.
I'm afraid that your tests are an enormous mess that I can't really review yet. Please start my making the changes I've asked for so far so that the tests themselves become simplified enough to be reviewable. https://codereview.chromium.org/403323004/diff/1/components/password_manager/... File components/password_manager/core/browser/password_syncable_service.cc (right): https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.cc:89: syncer::SyncChange::SyncChangeType GetSyncChangeType( On 2014/08/07 18:16:20, vasilii wrote: > On 2014/08/07 05:26:53, manthd wrote: > > it would probably be clearer to move this closer to its only call site. > > Impossible if we want to have it in the anonymous namespace. FYI, I see you've avoided this issue, but there are of course times that different style preferences come into conflict. For something as weak as the ordering of function definitions within a .cc file, it would have been better to move the non-anonymous version of the function up to right under the end of the anon namespace in order to get the two definitions close together. https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.cc:276: return; On 2014/08/07 18:16:21, vasilii wrote: > On 2014/08/07 07:36:14, Ilya Sherman wrote: > > On 2014/08/07 05:26:52, manthd wrote: > > > > > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Condit... > > > > > > I believe this is the first braceless conditional I've seen you use. Are > > these > > > used anywhere in the surrounding codebase? Braces aren't required by the > > whole > > > style guide, but are avoided in some parts of the codebase. > > > > > > Personally, I find it helpful to have braces around conditional flow control > > > (like the "return" here) because it makes it much easier to see which > indented > > > lines need to be thought about rather than being some sort of unconditional > > > expression that exceeds one line. > > > > > > You also have the option of > > > if (blah) { return; } > > > which is a nice mix of terseness and having braces as a clear marker of not > > > being unconditional. > > > > FWIW, this last version is very uncommon in the Chromium codebase, to the > point > > where most editors of the code would change it back. > > I always use > > if (condition) > return smth; > > In these files it's always like this. sgtm https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.cc:414: password_specifics->set_scheme(password_form.scheme); On 2014/08/07 18:16:20, vasilii wrote: > On 2014/08/07 05:26:53, manthd wrote: > > whether or not you use a macro, you could make this function less hard to read > > by avoiding repeating the word "password" a bazillion times: > > specifics->set_scheme(form.scheme); > > > > It's not like the reader is going to lack enough context to tell that these > > things are about passwords, what with all the other things named that here. > > I am not sure I like macros but Done. Yeah, I'm also sad about how that came out, because I hadn't noticed the need for the UTF wrappers in half the cases. You can go either way on this, since both choices are bad. Mainly, I want you to consider tightly scoped macros as one of the options to choose from. Sorry to have had you make a change and then be unhappy with it. https://codereview.chromium.org/403323004/diff/1/components/password_manager/... File components/password_manager/core/browser/password_syncable_service.h (right): https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.h:87: // Checks if |data|, the entry in sync db, needs to be created or updated On 2014/08/07 18:16:21, vasilii wrote: > On 2014/08/07 05:26:54, manthd wrote: > > should "the entry" here be "an entry"? surely the sync db has more than one > > entry. > > Done. Actually, now that I see a little better what the sync db is, I guess both "the" and "an" work. Whichever you like. https://codereview.chromium.org/403323004/diff/20001/components/password_mana... File components/password_manager/core/browser/password_syncable_service.cc (right): https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service.cc:17: namespace { Does the anon namespace normally go inside or outside of the main namespace in Chromium? My very quick survey suggests that it goes inside, and that's certainly true of internal code. For one thing, it avoids having to qualify many of the things in the file-local helper functions. https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service.cc:43: return true; You might as well just do return (password... && ... && ....used()); and avoid branching at all. https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service.cc:124: &new_sync_entries, given that you've got two objects with identical types and no really natural ordering (are new items necessarily "before" updated items?), it might be worth avoiding having these params adjacent, by putting them both in a struct and passing the struct (or its pointer as appropriate). Since you also need to pass both of them to WriteToPasswordStore, the annoyance of adding the struct also comes with a double pay-off. Note that you can reduce the invasiveness of the spare struct by just forward-declaring it in the header (probably in the private section in this case) and not defining it until the .cc file. Even with that, it's arguably a loss of clarity to use a struct rather than two separate fields, but that loss is well worth it if it avoid an easy-to-make-hard-to-detect problem. https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service.cc:193: PasswordFromSpecifics(specifics.password().client_only_encrypted_data(), You have many very similar code every time you call this function, and it's quite complex for no particular reason. You should instead define a small helper that lets you write AppendPasswordFromSpecifics(client_only_data, time_now, &new_sync_entries); The only spot call site that doesn't directly address is the one for deleted_entries below, and it's certainly worth the simplification to simply do AppendPasswordFromSpecifics(client_only_data, time_now, &deleted_entries); deleted_entries.back()->clear_data_synced(); https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service.cc:293: autofill::PasswordForm* new_password) { unless there is an absolutely critical performance reason to use an output parameter, just return the result of your function as a return value. because of RVO (which I believe even the dumbest compilers handler), there's no actual copy mode to create the return value, and it's often easy to avoid any copy to use the result. In particular, the call site just below becomes simply return ::MakePasswordSyncTag(PasswordFromSpecifics(password)); The only other call site, if you take the above suggestion about appending to scoped vectors will be in AppendPasswordFromSpecifics, with something like entries->push_back(new PasswordForm(PasswordFromSpecifics(...)); This copy from the result of PasswordFromSpecifics to the new heap-allocated PasswordForm is also allowed to be elided, but that's at least a tiny bit trickier, so i don't know for certain that every compiler Chromium uses will avoid the second copy. But even if there is one copy on some platforms (which I wouldn't bet on), that's usually an acceptable cost for having simpler code. (Note that there is another reason to use output params besides avoiding copies, but I don't think it applies here: the output parameter can hold allocated memory and reused that memory when the function assigns to it. For instance an append function can be passed a vector with a large capacity and thus not cause reallocation.) https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service.cc:348: (*passwords_entry_map)[::MakePasswordSyncTag(*password_form)] = this would be cleaner as PasswordEntryMap& entry_map = *password_entry_map; for (...) { entry_map[...] = ...; https://codereview.chromium.org/403323004/diff/20001/components/password_mana... File components/password_manager/core/browser/password_syncable_service.h (right): https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service.h:42: // constructor doesn't take the ownership of |password_store|. I think this is fine, but there are times that you should be even more specific: |password_store| must outlive the PasswordSyncableService. which differs from what you have now in that a constructor could use a pointer it was given, but then not keep a copy of the pointer. (Given the types in this case, I don't see any confusion, but it's worth keeping in mind for future.) https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service.h:46: // syncer::SyncableService: it is common to have the word "implementation" here, though I guess the baseclass's name including "Service" avoids most risk of confusion. https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service.h:67: // Only exposed for testing: blank line after this so that the comment doesn't look like it only applies to this one function. https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service.h:110: PasswordEntryMap* umatched_data_from_password_db, misspelt: s/um/unm/ https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service.h:130: // The password store that adds/updates/deletes password entries. Not owned by FYI, this is a common enough idea that you can just say "Not owned.", with "by thes class" implicit. https://codereview.chromium.org/403323004/diff/20001/components/password_mana... File components/password_manager/core/browser/password_syncable_service_unittest.cc (right): https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:39: const sync_pb::EntitySpecifics& specifics = sync_data.GetSpecifics(); this local variable doesn't give any better a name than the expression already has ("specifics" vs "GetSpecifics"), so the only thing you're adding is a repetition of the type. doesn't seem worth it here. https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:43: void PasswordsEqual(const sync_pb::PasswordSpecificsData& expected_password, You get **much** better error messages by using GMock's EqualsProto. ...Is that really not available in Chromium? Well, you can still vastly improve the error messages with something like MATCHER_P(EqualsPasswordSpecifics, password, "") { bool ok = true; if (arg.scheme() != password.scheme()) { *result_listener << "wrong scheme: '" << arg.scheme() << "' instead of '" << password.scheme() << ", "; ok = false; } if ...signon_realm... return ok; } except that the "..." in what I have is a lot of verbose code. this would actually be a good place for a macro (you don't have any mix of different cases here). or, you could easily do MATCHER... { testing::MatchResultListener os = result_listener; return EqualField(os, "scheme", arg.scheme(), password.scheme()) && EqualField(os, "signon_realm", arg.signon_realm(), ... ) && ... && ...; where EqualField is a simple template function. ...or you could go yell at somebody to public EqualsProto. I do not understand how that could possibly not have already been done. *sigh* More seriously, you can keep this as is, but it's got big downsides, which is that if this function ever finds a mismatch, then GUnit will report as the line that caused a problem, a line within this function body, not any line within a test body below. Unfortunately, you you call PasswordsEqual in a loop below, which base even getting a line number in the test body less valuable. GMock mostly works around this in the EqualsProto case in that when you write EXPECT_THAT(whatever, EqualsProto(some_expected_proto)); the EqualsProto object actually knows how to print the some_expected_proto object. There's also the option of using SCOPED_TRACE with a string that distinguishes different loop iterations. So, you have a lot of bad choices, which I want you to at least consider, but I guess the one you currently have is close to the best option. But seriously, I'd suggest that somebody file a Chromium bug to export EqualsProto (both GMock and protobuffers are already in use by Chromium!). https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:86: class FormFinder { "Pred" or "Predicate" would be a more common name that "Finder". https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:86: class FormFinder { http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Structs_vs._Cl... "For consistency with STL, you can use struct instead of class for functors and traits." In my experience, this is actually much stronger than "can"; all the functors I see are structs. That's especially nice here, since it means you can also omit the c'tor and instead use brace initialization (though in C++98 that requires a separate variable declaration, so you can opt against that). https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:89: ~FormFinder() {} This does nothing. It's a restatement of the implicit destructor. Within a single .cc file, there's no reason to have the definition in a different place (which is usually needed for a .h, and therefore a .h file should declare the non-implicit destructor). https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:97: bool FormFinder::operator()(const autofill::PasswordForm& form) const { it's very common for functors to have the operator function defined inline in the body of the struct, since that's basically the entire content of the functor. https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:111: virtual ~MockPasswordSyncableService() {} does nothing https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:120: class PasswordStoreDataVerifier { This is an astonishing amount of complexity for a test file. Some of this complexity is coming from having pulled apart the work you're doing into multiple classes doing encapsulation and data hiding on those classes as if they're production code. This is a poor choice in testing code; hiding is the exact opposite of what you want, the meaning of tests should be transparent. More of the complexity comes from avoiding copies. Short of many kilobytes of data, this is never the right trade-off for testing code; accept even several copies if it lets the tests be clearer. But the largest source of complexity here is a complete misuse of GMock. This entirely helper class inconveniently re-implementing MATCHER_P and a few other parts of GMock. *Everything* involved in the expected DB changes can be replaces with few individual calls to EXPECT_CALL. Of course, any place where you have an empty vector of changes, you can just use EXPECT_CALL(*password_store, BlahLoginImpl(_)).Times(0); and you can makes the .Times(0) case the default by using a password_store that is a testing::StrictMock<MockPasswordStore>. In the cases where you actually have a call to, for example AddLoginImpl, you just need EXPECT_CALL(*password_store, AddLoginImpl(PasswordIs(some_password))) .WillOnce(Return1PasswordOfType(PasswordStoreChange::ADD)); where you use MATCHER_P(PasswordIs, password, "") { /// compare 'arg' to 'password', like the FindForm functor does. } and ACTION_P(Return1PasswordOfType, type) { return blah; /// made the return value VerifyChange does, using 'type' and 'arg0' as the password that was passed to AddLoginImpl. } If the set of passwords has no duplicates, GMock will correctly match each call to the one matching password. If you actually have duplicate calls, you can use .Times(), or one of a few other tricks (ask me if needed). https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:203: void PasswordStoreDataVerifier::SetExpectedDBChanges( replace this whole function with GMock calls. https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:219: if (expected_db_add_changes_.empty()) { you don't need this 'if', you can just do EXPECT_CALL(...) .Times(blah.size()) // OK if the size is zero. .WillRepeatedly(...); // Still OK if size was zero; just not used. https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:254: void PasswordStoreDataVerifier::SetExpectedSyncChanges(SyncChangeList list) { this is a good example of something that has no place in test code. just move expected_sync_change_list_ directly into the test fixture class, as a protected member, and let you test bodies just call push_back on it as appropriate. (Well, actually don't do that; instead delete this an use GMock.) https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:260: for (SyncChangeList::const_iterator it = change_list.begin(); it looks like this can probably be entirely replaced by a use of testing::ElementsAre or ContainerEq and a simple matcher defined by MATCHER_P and storing an expected tag. https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:307: class PasswordSyncableServiceWrapper { see if you can just move all of this into the test fixture. (i see you currently use a second instance of it, but I'm hoping that goes away if you do other clean ups.) https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:319: MockPasswordStore* password_store() { it's very common for getters to be packed onto one line if they fit easily. it makes it easier to visually scan through the class and see all the getters with less interruption. https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:379: public PasswordSyncableServiceWrapper { strictly forbidden: "Multiple inheritance is allowed only when all superclasses, with the possible exception of the first one, are pure interfaces." http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Multip... And you have no reason at all to be using inheritance for this; just make a Wrapper member of the test fixture. https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:382: virtual ~PasswordSyncableServiceTest() {} nix
https://codereview.chromium.org/403323004/diff/1/components/password_manager/... File components/password_manager/core/browser/password_syncable_service.cc (right): https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.cc:89: syncer::SyncChange::SyncChangeType GetSyncChangeType( On 2014/08/14 06:55:46, manthd wrote: > On 2014/08/07 18:16:20, vasilii wrote: > > On 2014/08/07 05:26:53, manthd wrote: > > > it would probably be clearer to move this closer to its only call site. > > > > Impossible if we want to have it in the anonymous namespace. > > FYI, I see you've avoided this issue, but there are of course times that > different style preferences come into conflict. For something as weak as the > ordering of function definitions within a .cc file, it would have been better to > move the non-anonymous version of the function up to right under the end of the > anon namespace in order to get the two definitions close together. FWIW, in the Chromium repo, we typically try to keep the order of function definitions in the implementation file consistent with the order in the header file. https://codereview.chromium.org/403323004/diff/20001/components/password_mana... File components/password_manager/core/browser/password_syncable_service.h (right): https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service.h:46: // syncer::SyncableService: On 2014/08/14 06:55:46, manthd wrote: > it is common to have the word "implementation" here, though I guess the > baseclass's name including "Service" avoids most risk of confusion. FWIW, the style used here is very common in Chromium code. https://codereview.chromium.org/403323004/diff/20001/components/password_mana... File components/password_manager/core/browser/password_syncable_service_unittest.cc (right): https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:43: void PasswordsEqual(const sync_pb::PasswordSpecificsData& expected_password, On 2014/08/14 06:55:47, manthd wrote: > You get **much** better error messages by using GMock's EqualsProto. > > ...Is that really not available in Chromium? > > Well, you can still vastly improve the error messages with something like > MATCHER_P(EqualsPasswordSpecifics, password, "") { > bool ok = true; > if (arg.scheme() != password.scheme()) { > *result_listener << "wrong scheme: '" << arg.scheme() << "' instead of '" > << password.scheme() << ", "; > ok = false; > } > if ...signon_realm... > return ok; > } > > except that the "..." in what I have is a lot of verbose code. > > this would actually be a good place for a macro (you don't have any mix of > different cases here). > > or, you could easily do > MATCHER... { > testing::MatchResultListener os = result_listener; > return > EqualField(os, "scheme", arg.scheme(), password.scheme()) && > EqualField(os, "signon_realm", arg.signon_realm(), ... ) && > ... && > ...; > where EqualField is a simple template function. > > ...or you could go yell at somebody to public EqualsProto. I do not understand > how that could possibly not have already been done. *sigh* > > More seriously, you can keep this as is, but it's got big downsides, which is > that if this function ever finds a mismatch, then GUnit will report as the line > that caused a problem, a line within this function body, not any line within a > test body below. > > Unfortunately, you you call PasswordsEqual in a loop below, which base even > getting a line number in the test body less valuable. > > GMock mostly works around this in the EqualsProto case in that when you write > EXPECT_THAT(whatever, EqualsProto(some_expected_proto)); > the EqualsProto object actually knows how to print the some_expected_proto > object. > > There's also the option of using SCOPED_TRACE with a string that distinguishes > different loop iterations. > > So, you have a lot of bad choices, which I want you to at least consider, but I > guess the one you currently have is close to the best option. But seriously, > I'd suggest that somebody file a Chromium bug to export EqualsProto (both GMock > and protobuffers are already in use by Chromium!). EqualsProto would be handy to have. I don't know if it's available or not -- Chromium has only relatively recently started using protobuffers. Chromium does have a policy to limit the use of all but the most basic GMock statements. More precisely, many of the senior engineers on the team declared that GMock was like learning another language, and that it was too much overhead for test readability (that's, of course, very simplified, so please don't quote me on that). Thus, things that "are pretty clear" are allowed, but things "that are complex" are not. In practice, that probably means that EqualsProto would be smiled upon, but a MATCHER would be frowned upon.
https://codereview.chromium.org/403323004/diff/20001/components/password_mana... File components/password_manager/core/browser/password_syncable_service_unittest.cc (right): https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:43: void PasswordsEqual(const sync_pb::PasswordSpecificsData& expected_password, On 2014/08/14 07:19:50, Ilya Sherman wrote: > On 2014/08/14 06:55:47, manthd wrote: > > You get **much** better error messages by using GMock's EqualsProto. > > > > ...Is that really not available in Chromium? > > > > Well, you can still vastly improve the error messages with something like > > MATCHER_P(EqualsPasswordSpecifics, password, "") { > > bool ok = true; > > if (arg.scheme() != password.scheme()) { > > *result_listener << "wrong scheme: '" << arg.scheme() << "' instead of > '" > > << password.scheme() << ", "; > > ok = false; > > } > > if ...signon_realm... > > return ok; > > } > > > > except that the "..." in what I have is a lot of verbose code. > > > > this would actually be a good place for a macro (you don't have any mix of > > different cases here). > > > > or, you could easily do > > MATCHER... { > > testing::MatchResultListener os = result_listener; > > return > > EqualField(os, "scheme", arg.scheme(), password.scheme()) && > > EqualField(os, "signon_realm", arg.signon_realm(), ... ) && > > ... && > > ...; > > where EqualField is a simple template function. > > > > ...or you could go yell at somebody to public EqualsProto. I do not > understand > > how that could possibly not have already been done. *sigh* > > > > More seriously, you can keep this as is, but it's got big downsides, which is > > that if this function ever finds a mismatch, then GUnit will report as the > line > > that caused a problem, a line within this function body, not any line within a > > test body below. > > > > Unfortunately, you you call PasswordsEqual in a loop below, which base even > > getting a line number in the test body less valuable. > > > > GMock mostly works around this in the EqualsProto case in that when you write > > EXPECT_THAT(whatever, EqualsProto(some_expected_proto)); > > the EqualsProto object actually knows how to print the some_expected_proto > > object. > > > > There's also the option of using SCOPED_TRACE with a string that distinguishes > > different loop iterations. > > > > So, you have a lot of bad choices, which I want you to at least consider, but > I > > guess the one you currently have is close to the best option. But seriously, > > I'd suggest that somebody file a Chromium bug to export EqualsProto (both > GMock > > and protobuffers are already in use by Chromium!). > > EqualsProto would be handy to have. I don't know if it's available or not -- > Chromium has only relatively recently started using protobuffers. > > Chromium does have a policy to limit the use of all but the most basic GMock > statements. More precisely, many of the senior engineers on the team declared > that GMock was like learning another language, and that it was too much overhead > for test readability (that's, of course, very simplified, so please don't quote > me on that). Thus, things that "are pretty clear" are allowed, but things "that > are complex" are not. In practice, that probably means that EqualsProto would > be smiled upon, but a MATCHER would be frowned upon. I've read some of those discussions about using GMock; it's worth noting that Matchers are part of GMock because they're critical for mocking function calls, but they can also be used in GUnit with no mocking at all, via EXPECT_THAT, and the worries about GMock usage just don't apply to EXPECT_THAT. That means that EqualsProto has utility even if nobody ever mocks a single function. As such, making EqualsProto public would be an unmitigated win; it's a no-brainer once Chromium is using Protobuffers and has GMock in its source tree. Now, I understand some of the concerns about GMock; mocking out dependencies runs the risk of letting client code go out of sync with the behavior of its real dependencies and letting unit tests continue to pass despite that mismatch. That concern can be mitigated by good management of who defines mock subclasses. There's also the concern of GMock's domain-specific language. Again, Matchers fall into a rather different case than the rest of GMock: the matching step itself is really pretty transparent, and most of the complexity comes from understanding how mock functions respond to having matched a particular mock call. Even if there's a general concern about keeping GMock use simple, it doesn't seem to impinge much on things like MATCHER. (And it looks like there are already 50 uses of it in the Chromium codebase.) I admit that ACTION is a different matter, though I see uses of it, too. But most importantly, if simplicity is the goal, PasswordStoreDataVerifier absolutely must be removed. If I've understood the existing code correctly (which is fairly hard), about 230 lines of code can become about 25 by turning FormFinder into a MATCHER and defining one ACTION, and that's with zero change in the risk of dependency-behavior skew, since that risk comes for having a mock class, not from having the mock class's behavior in ACTION bodies.
I removed PasswordStoreDataVerifier. However, I don't see that the tests became more readable. https://codereview.chromium.org/403323004/diff/1/components/password_manager/... File components/password_manager/core/browser/password_syncable_service.cc (right): https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.cc:89: syncer::SyncChange::SyncChangeType GetSyncChangeType( On 2014/08/14 06:55:46, manthd wrote: > On 2014/08/07 18:16:20, vasilii wrote: > > On 2014/08/07 05:26:53, manthd wrote: > > > it would probably be clearer to move this closer to its only call site. > > > > Impossible if we want to have it in the anonymous namespace. > > FYI, I see you've avoided this issue, but there are of course times that > different style preferences come into conflict. For something as weak as the > ordering of function definitions within a .cc file, it would have been better to > move the non-anonymous version of the function up to right under the end of the > anon namespace in order to get the two definitions close together. Is it worth it here? The proposed solution doesn't scale and can't be applied to AreLocalAndSyncPasswordsEqual(). The method definition scope starting with something other than constructor looks strange to me here. https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.cc:414: password_specifics->set_scheme(password_form.scheme); On 2014/08/14 06:55:46, manthd wrote: > On 2014/08/07 18:16:20, vasilii wrote: > > On 2014/08/07 05:26:53, manthd wrote: > > > whether or not you use a macro, you could make this function less hard to > read > > > by avoiding repeating the word "password" a bazillion times: > > > specifics->set_scheme(form.scheme); > > > > > > It's not like the reader is going to lack enough context to tell that these > > > things are about passwords, what with all the other things named that here. > > > > I am not sure I like macros but Done. > > Yeah, I'm also sad about how that came out, because I hadn't noticed the need > for the UTF wrappers in half the cases. > > You can go either way on this, since both choices are bad. Mainly, I want you > to consider tightly scoped macros as one of the options to choose from. > > Sorry to have had you make a change and then be unhappy with it. I'll leave like this. It looks OK. https://codereview.chromium.org/403323004/diff/1/components/password_manager/... File components/password_manager/core/browser/password_syncable_service.h (right): https://codereview.chromium.org/403323004/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service.h:87: // Checks if |data|, the entry in sync db, needs to be created or updated On 2014/08/14 06:55:46, manthd wrote: > On 2014/08/07 18:16:21, vasilii wrote: > > On 2014/08/07 05:26:54, manthd wrote: > > > should "the entry" here be "an entry"? surely the sync db has more than one > > > entry. > > > > Done. > > Actually, now that I see a little better what the sync db is, I guess both "the" > and "an" work. Whichever you like. an entry. https://codereview.chromium.org/403323004/diff/20001/components/password_mana... File components/password_manager/core/browser/password_syncable_service.cc (right): https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service.cc:17: namespace { On 2014/08/14 06:55:46, manthd wrote: > Does the anon namespace normally go inside or outside of the main namespace in > Chromium? My very quick survey suggests that it goes inside, and that's > certainly true of internal code. For one thing, it avoids having to qualify > many of the things in the file-local helper functions. You are right. I was afraid of long calls like ::password_manager::MakePasswordSyncTag(*password_form); https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service.cc:43: return true; On 2014/08/14 06:55:46, manthd wrote: > You might as well just do > return (password... && > ... && > ....used()); > and avoid branching at all. Done. https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service.cc:124: &new_sync_entries, On 2014/08/14 06:55:46, manthd wrote: > given that you've got two objects with identical types and no really natural > ordering (are new items necessarily "before" updated items?), it might be worth > avoiding having these params adjacent, by putting them both in a struct and > passing the struct (or its pointer as appropriate). > > Since you also need to pass both of them to WriteToPasswordStore, the annoyance > of adding the struct also comes with a double pay-off. > > Note that you can reduce the invasiveness of the spare struct by just > forward-declaring it in the header (probably in the private section in this > case) and not defining it until the .cc file. > > Even with that, it's arguably a loss of clarity to use a struct rather than two > separate fields, but that loss is well worth it if it avoid an > easy-to-make-hard-to-detect problem. Done. Indeed, clarity suffered. https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service.cc:193: PasswordFromSpecifics(specifics.password().client_only_encrypted_data(), On 2014/08/14 06:55:46, manthd wrote: > You have many very similar code every time you call this function, and it's > quite complex for no particular reason. You should instead define a small > helper that lets you write > AppendPasswordFromSpecifics(client_only_data, time_now, &new_sync_entries); > > The only spot call site that doesn't directly address is the one for > deleted_entries below, and it's certainly worth the simplification to simply do > AppendPasswordFromSpecifics(client_only_data, time_now, &deleted_entries); > deleted_entries.back()->clear_data_synced(); Done. https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service.cc:293: autofill::PasswordForm* new_password) { On 2014/08/14 06:55:46, manthd wrote: > unless there is an absolutely critical performance reason to use an output > parameter, just return the result of your function as a return value. because > of RVO (which I believe even the dumbest compilers handler), there's no actual > copy mode to create the return value, and it's often easy to avoid any copy to > use the result. > > In particular, the call site just below becomes simply > return ::MakePasswordSyncTag(PasswordFromSpecifics(password)); > > The only other call site, if you take the above suggestion about appending to > scoped vectors will be in AppendPasswordFromSpecifics, with something like > entries->push_back(new PasswordForm(PasswordFromSpecifics(...)); > > This copy from the result of PasswordFromSpecifics to the new heap-allocated > PasswordForm is also allowed to be elided, but that's at least a tiny bit > trickier, so i don't know for certain that every compiler Chromium uses will > avoid the second copy. > > But even if there is one copy on some platforms (which I wouldn't bet on), > that's usually an acceptable cost for having simpler code. > > (Note that there is another reason to use output params besides avoiding copies, > but I don't think it applies here: the output parameter can hold allocated > memory and reused that memory when the function assigns to it. For instance an > append function can be passed a vector with a large capacity and thus not cause > reallocation.) Done. https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service.cc:348: (*passwords_entry_map)[::MakePasswordSyncTag(*password_form)] = On 2014/08/14 06:55:46, manthd wrote: > this would be cleaner as > PasswordEntryMap& entry_map = *password_entry_map; > for (...) { > entry_map[...] = ...; Done. https://codereview.chromium.org/403323004/diff/20001/components/password_mana... File components/password_manager/core/browser/password_syncable_service.h (right): https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service.h:42: // constructor doesn't take the ownership of |password_store|. On 2014/08/14 06:55:46, manthd wrote: > I think this is fine, but there are times that you should be even more specific: > |password_store| must outlive the PasswordSyncableService. > which differs from what you have now in that a constructor could use a pointer > it was given, but then not keep a copy of the pointer. (Given the types in this > case, I don't see any confusion, but it's worth keeping in mind for future.) Acknowledged. https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service.h:46: // syncer::SyncableService: On 2014/08/14 06:55:46, manthd wrote: > it is common to have the word "implementation" here, though I guess the > baseclass's name including "Service" avoids most risk of confusion. The most popular notation in Chrome is "// BaseClass:" https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service.h:67: // Only exposed for testing: On 2014/08/14 06:55:47, manthd wrote: > blank line after this so that the comment doesn't look like it only applies to > this one function. Done. https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service.h:110: PasswordEntryMap* umatched_data_from_password_db, On 2014/08/14 06:55:46, manthd wrote: > misspelt: s/um/unm/ Done. https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service.h:130: // The password store that adds/updates/deletes password entries. Not owned by On 2014/08/14 06:55:47, manthd wrote: > FYI, this is a common enough idea that you can just say "Not owned.", with "by > thes class" implicit. Done. https://codereview.chromium.org/403323004/diff/20001/components/password_mana... File components/password_manager/core/browser/password_syncable_service_unittest.cc (right): https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:39: const sync_pb::EntitySpecifics& specifics = sync_data.GetSpecifics(); On 2014/08/14 06:55:47, manthd wrote: > this local variable doesn't give any better a name than the expression already > has ("specifics" vs "GetSpecifics"), so the only thing you're adding is a > repetition of the type. doesn't seem worth it here. Done. https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:43: void PasswordsEqual(const sync_pb::PasswordSpecificsData& expected_password, On 2014/08/15 04:54:28, manthd wrote: > On 2014/08/14 07:19:50, Ilya Sherman wrote: > > On 2014/08/14 06:55:47, manthd wrote: > > > You get **much** better error messages by using GMock's EqualsProto. > > > > > > ...Is that really not available in Chromium? > > > > > > Well, you can still vastly improve the error messages with something like > > > MATCHER_P(EqualsPasswordSpecifics, password, "") { > > > bool ok = true; > > > if (arg.scheme() != password.scheme()) { > > > *result_listener << "wrong scheme: '" << arg.scheme() << "' instead of > > '" > > > << password.scheme() << ", "; > > > ok = false; > > > } > > > if ...signon_realm... > > > return ok; > > > } > > > > > > except that the "..." in what I have is a lot of verbose code. > > > > > > this would actually be a good place for a macro (you don't have any mix of > > > different cases here). > > > > > > or, you could easily do > > > MATCHER... { > > > testing::MatchResultListener os = result_listener; > > > return > > > EqualField(os, "scheme", arg.scheme(), password.scheme()) && > > > EqualField(os, "signon_realm", arg.signon_realm(), ... ) && > > > ... && > > > ...; > > > where EqualField is a simple template function. > > > > > > ...or you could go yell at somebody to public EqualsProto. I do not > > understand > > > how that could possibly not have already been done. *sigh* > > > > > > More seriously, you can keep this as is, but it's got big downsides, which > is > > > that if this function ever finds a mismatch, then GUnit will report as the > > line > > > that caused a problem, a line within this function body, not any line within > a > > > test body below. > > > > > > Unfortunately, you you call PasswordsEqual in a loop below, which base even > > > getting a line number in the test body less valuable. > > > > > > GMock mostly works around this in the EqualsProto case in that when you > write > > > EXPECT_THAT(whatever, EqualsProto(some_expected_proto)); > > > the EqualsProto object actually knows how to print the some_expected_proto > > > object. > > > > > > There's also the option of using SCOPED_TRACE with a string that > distinguishes > > > different loop iterations. > > > > > > So, you have a lot of bad choices, which I want you to at least consider, > but > > I > > > guess the one you currently have is close to the best option. But > seriously, > > > I'd suggest that somebody file a Chromium bug to export EqualsProto (both > > GMock > > > and protobuffers are already in use by Chromium!). > > > > EqualsProto would be handy to have. I don't know if it's available or not -- > > Chromium has only relatively recently started using protobuffers. > > > > Chromium does have a policy to limit the use of all but the most basic GMock > > statements. More precisely, many of the senior engineers on the team declared > > that GMock was like learning another language, and that it was too much > overhead > > for test readability (that's, of course, very simplified, so please don't > quote > > me on that). Thus, things that "are pretty clear" are allowed, but things > "that > > are complex" are not. In practice, that probably means that EqualsProto would > > be smiled upon, but a MATCHER would be frowned upon. > > I've read some of those discussions about using GMock; it's worth noting that > Matchers are part of GMock because they're critical for mocking function calls, > but they can also be used in GUnit with no mocking at all, via EXPECT_THAT, and > the worries about GMock usage just don't apply to EXPECT_THAT. > > That means that EqualsProto has utility even if nobody ever mocks a single > function. As such, making EqualsProto public would be an unmitigated win; it's > a no-brainer once Chromium is using Protobuffers and has GMock in its source > tree. > > Now, I understand some of the concerns about GMock; mocking out dependencies > runs the risk of letting client code go out of sync with the behavior of its > real dependencies and letting unit tests continue to pass despite that mismatch. > That concern can be mitigated by good management of who defines mock > subclasses. > > There's also the concern of GMock's domain-specific language. Again, Matchers > fall into a rather different case than the rest of GMock: the matching step > itself is really pretty transparent, and most of the complexity comes from > understanding how mock functions respond to having matched a particular mock > call. Even if there's a general concern about keeping GMock use simple, it > doesn't seem to impinge much on things like MATCHER. (And it looks like there > are already 50 uses of it in the Chromium codebase.) I admit that ACTION is a > different matter, though I see uses of it, too. > > But most importantly, if simplicity is the goal, PasswordStoreDataVerifier > absolutely must be removed. If I've understood the existing code correctly > (which is fairly hard), about 230 lines of code can become about 25 by turning > FormFinder into a MATCHER and defining one ACTION, and that's with zero change > in the risk of dependency-behavior skew, since that risk comes for having a mock > class, not from having the mock class's behavior in ACTION bodies. How should I proceed with this? https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:86: class FormFinder { On 2014/08/14 06:55:47, manthd wrote: > "Pred" or "Predicate" would be a more common name that "Finder". Done. https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:86: class FormFinder { On 2014/08/14 06:55:48, manthd wrote: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Structs_vs._Cl... > "For consistency with STL, you can use struct instead of class for functors and > traits." > > In my experience, this is actually much stronger than "can"; all the functors I > see are structs. > > That's especially nice here, since it means you can also omit the c'tor and > instead use brace initialization (though in C++98 that requires a separate > variable declaration, so you can opt against that). Done. https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:89: ~FormFinder() {} On 2014/08/14 06:55:47, manthd wrote: > This does nothing. It's a restatement of the implicit destructor. Within a > single .cc file, there's no reason to have the definition in a different place > (which is usually needed for a .h, and therefore a .h file should declare the > non-implicit destructor). Done. https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:97: bool FormFinder::operator()(const autofill::PasswordForm& form) const { On 2014/08/14 06:55:47, manthd wrote: > it's very common for functors to have the operator function defined inline in > the body of the struct, since that's basically the entire content of the > functor. Done. https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:111: virtual ~MockPasswordSyncableService() {} On 2014/08/14 06:55:47, manthd wrote: > does nothing Done. https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:120: class PasswordStoreDataVerifier { On 2014/08/14 06:55:47, manthd wrote: > This is an astonishing amount of complexity for a test file. > > Some of this complexity is coming from having pulled apart the work you're doing > into multiple classes doing encapsulation and data hiding on those classes as if > they're production code. This is a poor choice in testing code; hiding is the > exact opposite of what you want, the meaning of tests should be transparent. > > More of the complexity comes from avoiding copies. Short of many kilobytes of > data, this is never the right trade-off for testing code; accept even several > copies if it lets the tests be clearer. > > But the largest source of complexity here is a complete misuse of GMock. This > entirely helper class inconveniently re-implementing MATCHER_P and a few other > parts of GMock. > > *Everything* involved in the expected DB changes can be replaces with few > individual calls to EXPECT_CALL. > > Of course, any place where you have an empty vector of changes, you can just use > EXPECT_CALL(*password_store, BlahLoginImpl(_)).Times(0); > and you can makes the .Times(0) case the default by using a password_store that > is a testing::StrictMock<MockPasswordStore>. > > In the cases where you actually have a call to, for example AddLoginImpl, you > just need > EXPECT_CALL(*password_store, AddLoginImpl(PasswordIs(some_password))) > .WillOnce(Return1PasswordOfType(PasswordStoreChange::ADD)); > > where you use > MATCHER_P(PasswordIs, password, "") { > /// compare 'arg' to 'password', like the FindForm functor does. > } > and > ACTION_P(Return1PasswordOfType, type) { > return blah; /// made the return value VerifyChange does, using 'type' and > 'arg0' as the password that was passed to AddLoginImpl. > } > > If the set of passwords has no duplicates, GMock will correctly match each call > to the one matching password. If you actually have duplicate calls, you can use > .Times(), or one of a few other tricks (ask me if needed). The misuse of GMock was driven by the comment of my reviewer https://codereview.chromium.org/131003006/diff/20001/chrome/browser/password_..., https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:203: void PasswordStoreDataVerifier::SetExpectedDBChanges( On 2014/08/14 06:55:47, manthd wrote: > replace this whole function with GMock calls. Done. https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:219: if (expected_db_add_changes_.empty()) { On 2014/08/14 06:55:47, manthd wrote: > you don't need this 'if', you can just do > EXPECT_CALL(...) > .Times(blah.size()) // OK if the size is zero. > .WillRepeatedly(...); // Still OK if size was zero; just not used. Done. https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:254: void PasswordStoreDataVerifier::SetExpectedSyncChanges(SyncChangeList list) { On 2014/08/14 06:55:47, manthd wrote: > this is a good example of something that has no place in test code. just move > expected_sync_change_list_ directly into the test fixture class, as a protected > member, and let you test bodies just call push_back on it as appropriate. > (Well, actually don't do that; instead delete this an use GMock.) Done. https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:260: for (SyncChangeList::const_iterator it = change_list.begin(); On 2014/08/14 06:55:47, manthd wrote: > it looks like this can probably be entirely replaced by a use of > testing::ElementsAre or ContainerEq and a simple matcher defined by MATCHER_P > and storing an expected tag. I didn't find the nice way to do that. https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:307: class PasswordSyncableServiceWrapper { On 2014/08/14 06:55:48, manthd wrote: > see if you can just move all of this into the test fixture. (i see you > currently use a second instance of it, but I'm hoping that goes away if you do > other clean ups.) For one of the tests I use two PasswordSyncableService and MockPasswordStore. They are encapsulated in PasswordSyncableServiceWrapper. https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:319: MockPasswordStore* password_store() { On 2014/08/14 06:55:48, manthd wrote: > it's very common for getters to be packed onto one line if they fit easily. it > makes it easier to visually scan through the class and see all the getters with > less interruption. Done. https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:379: public PasswordSyncableServiceWrapper { On 2014/08/14 06:55:47, manthd wrote: > strictly forbidden: "Multiple inheritance is allowed only when all > superclasses, with the possible exception of the first one, are pure > interfaces." > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Multip... > > And you have no reason at all to be using inheritance for this; just make a > Wrapper member of the test fixture. Done. https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:382: virtual ~PasswordSyncableServiceTest() {} On 2014/08/14 06:55:47, manthd wrote: > nix Done.
Thanks for the changes so far. I understand the fact that my pushback on Chromium handling of mocking is putting more work on you. To some extend, this is the readability review working as intended, in that one of the goals is to provide best practices that can be used to make future overall improvements in code health, but I also realize that this is probably frustrating. If you feel overwhelmed or think that this constitutes an actual conflict with Chromium practices, please feel free to give me pushback and/or escalate to appropriate other Chromium engineers or to the C++ readability captain, Don Yang (omoikane@google.com). https://codereview.chromium.org/403323004/diff/20001/components/password_mana... File components/password_manager/core/browser/password_syncable_service_unittest.cc (right): https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:43: void PasswordsEqual(const sync_pb::PasswordSpecificsData& expected_password, On 2014/08/19 15:55:44, vasilii wrote: > On 2014/08/15 04:54:28, manthd wrote: > > On 2014/08/14 07:19:50, Ilya Sherman wrote: > > > On 2014/08/14 06:55:47, manthd wrote: > > > > You get **much** better error messages by using GMock's EqualsProto. > > > > > > > > ...Is that really not available in Chromium? > > > > > > > > Well, you can still vastly improve the error messages with something like > > > > MATCHER_P(EqualsPasswordSpecifics, password, "") { > > > > bool ok = true; > > > > if (arg.scheme() != password.scheme()) { > > > > *result_listener << "wrong scheme: '" << arg.scheme() << "' instead > of > > > '" > > > > << password.scheme() << ", "; > > > > ok = false; > > > > } > > > > if ...signon_realm... > > > > return ok; > > > > } > > > > > > > > except that the "..." in what I have is a lot of verbose code. > > > > > > > > this would actually be a good place for a macro (you don't have any mix of > > > > different cases here). > > > > > > > > or, you could easily do > > > > MATCHER... { > > > > testing::MatchResultListener os = result_listener; > > > > return > > > > EqualField(os, "scheme", arg.scheme(), password.scheme()) && > > > > EqualField(os, "signon_realm", arg.signon_realm(), ... ) && > > > > ... && > > > > ...; > > > > where EqualField is a simple template function. > > > > > > > > ...or you could go yell at somebody to public EqualsProto. I do not > > > understand > > > > how that could possibly not have already been done. *sigh* > > > > > > > > More seriously, you can keep this as is, but it's got big downsides, which > > is > > > > that if this function ever finds a mismatch, then GUnit will report as the > > > line > > > > that caused a problem, a line within this function body, not any line > within > > a > > > > test body below. > > > > > > > > Unfortunately, you you call PasswordsEqual in a loop below, which base > even > > > > getting a line number in the test body less valuable. > > > > > > > > GMock mostly works around this in the EqualsProto case in that when you > > write > > > > EXPECT_THAT(whatever, EqualsProto(some_expected_proto)); > > > > the EqualsProto object actually knows how to print the some_expected_proto > > > > object. > > > > > > > > There's also the option of using SCOPED_TRACE with a string that > > distinguishes > > > > different loop iterations. > > > > > > > > So, you have a lot of bad choices, which I want you to at least consider, > > but > > > I > > > > guess the one you currently have is close to the best option. But > > seriously, > > > > I'd suggest that somebody file a Chromium bug to export EqualsProto (both > > > GMock > > > > and protobuffers are already in use by Chromium!). > > > > > > EqualsProto would be handy to have. I don't know if it's available or not > -- > > > Chromium has only relatively recently started using protobuffers. > > > > > > Chromium does have a policy to limit the use of all but the most basic GMock > > > statements. More precisely, many of the senior engineers on the team > declared > > > that GMock was like learning another language, and that it was too much > > overhead > > > for test readability (that's, of course, very simplified, so please don't > > quote > > > me on that). Thus, things that "are pretty clear" are allowed, but things > > "that > > > are complex" are not. In practice, that probably means that EqualsProto > would > > > be smiled upon, but a MATCHER would be frowned upon. > > > > I've read some of those discussions about using GMock; it's worth noting that > > Matchers are part of GMock because they're critical for mocking function > calls, > > but they can also be used in GUnit with no mocking at all, via EXPECT_THAT, > and > > the worries about GMock usage just don't apply to EXPECT_THAT. > > > > That means that EqualsProto has utility even if nobody ever mocks a single > > function. As such, making EqualsProto public would be an unmitigated win; > it's > > a no-brainer once Chromium is using Protobuffers and has GMock in its source > > tree. > > > > Now, I understand some of the concerns about GMock; mocking out dependencies > > runs the risk of letting client code go out of sync with the behavior of its > > real dependencies and letting unit tests continue to pass despite that > mismatch. > > That concern can be mitigated by good management of who defines mock > > subclasses. > > > > There's also the concern of GMock's domain-specific language. Again, Matchers > > fall into a rather different case than the rest of GMock: the matching step > > itself is really pretty transparent, and most of the complexity comes from > > understanding how mock functions respond to having matched a particular mock > > call. Even if there's a general concern about keeping GMock use simple, it > > doesn't seem to impinge much on things like MATCHER. (And it looks like there > > are already 50 uses of it in the Chromium codebase.) I admit that ACTION is a > > different matter, though I see uses of it, too. > > > > But most importantly, if simplicity is the goal, PasswordStoreDataVerifier > > absolutely must be removed. If I've understood the existing code correctly > > (which is fairly hard), about 230 lines of code can become about 25 by turning > > FormFinder into a MATCHER and defining one ACTION, and that's with zero change > > in the risk of dependency-behavior skew, since that risk comes for having a > mock > > class, not from having the mock class's behavior in ACTION bodies. > > How should I proceed with this? I think that if you take the detailed suggestions I've given on this pass, you'll be able to remove a lot of support code, with only a few clearly-defined GMock helpers, and the test bodies themselves will become much more concise. I believe my suggestions respect the concerns that motivate limiting the use of GMock. If you're worried about that, ask Ilya if he agrees with my assessment before working to implement my suggestions. https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:120: class PasswordStoreDataVerifier { On 2014/08/19 15:55:44, vasilii wrote: > On 2014/08/14 06:55:47, manthd wrote: > > This is an astonishing amount of complexity for a test file. > > > > Some of this complexity is coming from having pulled apart the work you're > doing > > into multiple classes doing encapsulation and data hiding on those classes as > if > > they're production code. This is a poor choice in testing code; hiding is the > > exact opposite of what you want, the meaning of tests should be transparent. > > > > More of the complexity comes from avoiding copies. Short of many kilobytes of > > data, this is never the right trade-off for testing code; accept even several > > copies if it lets the tests be clearer. > > > > But the largest source of complexity here is a complete misuse of GMock. This > > entirely helper class inconveniently re-implementing MATCHER_P and a few other > > parts of GMock. > > > > *Everything* involved in the expected DB changes can be replaces with few > > individual calls to EXPECT_CALL. > > > > Of course, any place where you have an empty vector of changes, you can just > use > > EXPECT_CALL(*password_store, BlahLoginImpl(_)).Times(0); > > and you can makes the .Times(0) case the default by using a password_store > that > > is a testing::StrictMock<MockPasswordStore>. > > > > In the cases where you actually have a call to, for example AddLoginImpl, you > > just need > > EXPECT_CALL(*password_store, AddLoginImpl(PasswordIs(some_password))) > > .WillOnce(Return1PasswordOfType(PasswordStoreChange::ADD)); > > > > where you use > > MATCHER_P(PasswordIs, password, "") { > > /// compare 'arg' to 'password', like the FindForm functor does. > > } > > and > > ACTION_P(Return1PasswordOfType, type) { > > return blah; /// made the return value VerifyChange does, using 'type' and > > 'arg0' as the password that was passed to AddLoginImpl. > > } > > > > If the set of passwords has no duplicates, GMock will correctly match each > call > > to the one matching password. If you actually have duplicate calls, you can > use > > .Times(), or one of a few other tricks (ask me if needed). > > The misuse of GMock was driven by the comment of my reviewer > https://codereview.chromium.org/131003006/diff/20001/chrome/browser/password_..., Thanks for that pointer; it helps clarify what the motivation was. I've given a detail comment on the other snapshot. https://codereview.chromium.org/403323004/diff/40001/components/password_mana... File components/password_manager/core/browser/password_syncable_service.cc (right): https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service.cc:26: return password_form.scheme == password_specifics.scheme() && add parens; see comment below https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service.cc:50: return net::EscapePath(password.origin.spec()) + "|" + Very minor, but if you don't use parens here, the officially correct way of wrapping is to use the usual 4-space indent: return blah + "|" + blah + "|" + ...; which is obviously worse than what you do have here, but you can keep it pretty and conform to official style by just adding paretheses: return (blah + ... blah + ...); https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service.cc:60: case PasswordStoreChange::ADD: you have the option of packing these onto one line each, which would make clearer how similar each case is. https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service.cc:84: ScopedVector<autofill::PasswordForm> new_sync_entries; Part of the added ugliness of this solution is due to the long names. The elements of SyncEntries really don't need to include the word "sync" in their names; that's already included in the surrounding content. (The same holds for "entries", but you do still need a noun to make the field names field-like.) At minimum, you can make the expressions below into ...sync_entries.new_entries... instead of repeating "sync". https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service.cc:192: ScopedVector<autofill::PasswordForm> deleted_entries; Both calls to WriteToPasswordStore take the deleted_entries, too, so move this third vector into the SyncEntries struct. https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service.cc:198: switch (it->change_type()) { this is still a lot of repetion that makes less clear what's similar for all the cases. if you move the delete entries into the struct, then it becomes natural to write something like typedef ScopedVector<autofill::PasswordForm> EntryList; // put this in SyncEntries. SyncEntries::EntryList* entries = sync_entries.entries_for_change_type(it->change_type()); if (entries == NULL) { return ...CreateAndUploadError...; } AppendPasswordfFromSpecifics( blah...data(), time_time, entries); where SyncEntries::entries_for_change_type is what does this switch, but that's much cleaner because it's doesn't need braces or breaks, since each case just returns, such as case syncer::SyncChange::ACTION_ADD: return &new_entries; The only downside to this is that it puts times in the deleted_entries protos. It's not clear to me whether that's actually a problem, but if it is, then WriteToPasswordStore can strip them out. https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service.cc:252: ::password_manager::MakePasswordSyncTag(it->form()), you don't need to qualify uses of functions in your own namespace. https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service.cc:357: std::string tag = ::password_manager::MakePasswordSyncTag(*password_form); remove unneeded qualification. that also lets you avoid this temp variable. https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service.cc:399: time_now, I see that there are at least some function calls in Chromium that do pack more than one parameter onto a line. that can help the reader see more of what's going on in the large context by not fluffing the code so much. I think it would be a pretty big improvement in the case of these 'time_now' arguments. https://codereview.chromium.org/403323004/diff/40001/components/password_mana... File components/password_manager/core/browser/password_syncable_service_unittest.cc (right): https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:26: using testing::get; what uses this? https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:74: PasswordsEqual( Semantically, this doesn't really belong here. Deciding which form to operate on is the job of the matcher, deciding what to do with it is the Action's job. It would be better to remove the 'form' parameter from this action entirely and make the matcher tighter. At minimum, that would allow your EXPECT_CALLs below to not repeat the form both as an arg to the matcher and as an arg to the action. That does mean that this action should be renamed. Maybe to something like MakePasswordList. Making the matcher stricter is easy enough if you don't mind not having a field-by-field diff: MATCHER_P(PasswordIs, form, "") { sync_pb::PasswordSpecificsData expected_password = GetPasswordSpecifics(PasswordSyncableService::SyncDataFromPassword(form)); sync_pb::PasswordSpecificsData actual_password = ...(arg); if (expected_password.scheme() == actual_password.scheme() && ... && ... && ...) { return true; } *result_listener << "Password protobuffer for form does not match; expected:\n" << expected_password.DebugString() << "\nactual:\n" << actual_password.DebugString(); return false; } https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:83: } Notice that every EXPECT_CALL on password_store() uses the same action. I believe this was basically what motivated the criticism you pointed to by a previous reviewer. Basically, GMock mock methods actually mix two jobs of a mocking framework, verification and stub implementation. Verification consists of listing the set of method calls that are expected (and failing the test if something different happens), which stubbing consists of specifying what the results of method calls against mocked-out dependencies are. These two jobs mix together easily, because verification needs to be expressed in terms of what param values are passed to methods, while stub implementation behavior generally depends on the param values. But the two jobs can be done separately, and it can make sense to make this separation if the implementation behavior doesn't depend on the particular calls that are being stubbed. If *every* call to, say, AddLoginImpl() responds by returning a one-element list of its argument, then there's no need to associate that stub implementation with any particular call. I believe the suggestion by the previous reviewer amounted to requesting this separation. It can be hard to think about separating the jobs because of how GMock mixes them: it provides a single override of the method being mocked and only allows stub behaviors to be set for the method by associating a set of matchers for the parameters. It's common enough to want to combine the two jobs that this is usually the right choice, but there's an alternative. I'm going to pretend that PasswordStore has only one method, AddLoginImpl, and I'm going to be lazy about exact type signatures here; it should be obvious enough what I mean in the real code. Now, the current definition of MockPasswordStore is essentially class MockPasswordStore : public PasswordStore { MOCK_METHOD(AddLoginImpl, ChangeList(PasswordForm)); }; this means that you have to write EXPECT_CALL(store_, AddLoginImpl(some_expected_param)) .WillOnce(DoTheOneImplementation()); It would be possible to instead write class FakePasswordStore : ... { ChangeList AddLoginImpl(PasswordForm form) override { return ...// do the one implementation } }; which avoids the need to specify the implementation for every expected call, but also discards the ability to check which calls occur. It's possible to mix these: class SpyPasswordStore : ... { MOCK_METHOD(AddCalled, void(PasswordForm)); ChangeList AddLoginImpl(PasswordForm form) override { AddCalled(form); return ...// do the one implementation } }; and then use it with EXPECT_CALL(store_, AddCalled(some_expected_param)); without any specification of the behavior to have (since GMock happily assumes that a void method's default behavior is to do nothing, and there's no need to do anything in the call AddCalled().) This really comes down to a question of what the tests need to test. Given that the code under test doesn't actually *use* the PasswordStoreChangeList, except to pass it back to the password store (except that you've disable even that behavior in the tests), I don't think there's any benefit to having the behavior of {Add,Remove,Update}LoginIml be specified by Actions. In fact, you can further simply the code under test by removing the NotifyPasswordStoreOfLoginChanges wrapper function and instead just unconditionally calling password_store_->NotifyLoginsChanged. Since you're already providing a test-specific PasswordStore subclass, you can have *that* ignore the NotifyLoginsChanged if you need it ignored. On the other hand, I do think your tests should be confirming which forms are passed to AddLoginImpl and kin. So, that gives you two choices: You can implement something like the SpyPasswordStore I describe above, where you provide new mock methods with which to verify what password forms are passed, while having the behavior of the implementation unconditionally run after calling that new mock method. But I think you actually have an even simpler alternative here. Since the *only* effect of the return values of FooLooginImpl are to provide values to be passed to NotifyLoginsChanged, and the real implementation of that fuction does nothing if there are no element is the changes list, then you could just have all three of the FooLoginImpl methods return empty lists. That's a very simple thing to do using GMock, so you could avoid writing the separate spy functions: In the same spot as you currently create the MockPasswordStore, just add ON_CALL(*password_store_, AddLoginImpl(_)) .WillByDefault(Return(PasswordStoreChangeList())); ON_CALL(*password_store_, RemoveLoginImpl(_)) .WillByDefault(Return(PasswordStoreChangeList())); ON_CALL(*password_store_, UpdateLoginImpl(_)) .WillByDefault(Return(PasswordStoreChangeList())); or if something changes so that you're subclassing MockPasswordStore, the constructor of that subclass is another good place to do these ON_CALLs. https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:86: const SyncChangeList& change_list = arg1; Most of this function is re-implementing the exact logic of testing::UnorderedElementsAre(). And as above, this work belongs in a Matcher, not in an action. You do a per-element matcher that handles the appropriate types: MATCHER_P2(SyncChangeIs, change_type, password, "") { const SyncData& data = arg.sync_data(); return (arg.change_type == change_type && syncer::SyncDataLocal(data).GetTag() == PasswordSyncableService::MakePasswordSyncTag(password) && (change_type != Blah::DELETE || testing::ValueIs(data, PasswordIs(password))); } and then in the test bodies just do EXPECT_CALL(procesor_, ProcessSyncChanges(_, ElementsAre( SyncChangeIs(Blah::ADD, form)))); If I recall correctly, GMock will even happily default-construct the return type if you don't specify an action, which will do the same thing as return SyncError(); If I'm wrong about that, then ON_CALL().WillByDefault() would be a good choice. https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:99: expected_password)) { wrong indent. Indent stuff inside of the (...) of an 'if' as if it were a code block. You would write { actual_tag = Blah::MakePasswordSyncTag( expected_password); } ^^^^ indent of 4 from beginning of expression that spans lines so you should write if (actual_tag == PasswordSyncableService::MakePasswordSyncTag( expected_password)) { ^^^^ https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:192: .RetiresOnSaturation(); This is very surprising. RetiresOnSaturation is normally only needed when a set of expectations on a single method needs to be built incrementally. I don't actually see any spots in the code where this will have any effect. What changes if you remove this? In any case, if the only reason for this is to allow the password store data to be changes by calling SetPasswordStoreData more than once in a test, you could simply use EXPECT_CALL(...).WillRepeatedly(...); which avoids placing any requirements on how many times the method is called. The FillFooLogins functions actually are good example of how not to define mock methods. (I know that you don't own MockPasswordStore, so I'm not asking for additional changes here.) Basically, it's *very* unlikely that a test should ever really care how many times these fill functions are called; all it really needs to do is provide the values that should be returned when they are called. It would have been better for the owner of MockPasswordStore to provide "fake" implementations of the fill functions (not MOCK_METHOD versions of them), which simply copy from an internal vector, and add some setter functions for populating that internal vector. So, instead of writing EXPECT_CALL(password_store, FillFooLogins(_)) .WillRepeatedly(SetArgPointee<0>(logins)); the user could just write password_store.set_foo_logins(logins); In the unlikely event that the client of PasswordStore wants to test how many times the fill functions are called, they can write MOCK_METHOD(FillFooLogins, ...) themselves. https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:199: struct AppendVector { This is a verbose reimplementation of what GMock's ACTION macro does. Just do ACTION_P(AppendVector, elements) { arg0->insert(arg0->end(), elements.begin(), element.end()); } You then do not need Invoke: EXPECT_CALL(...).WillOnce(AppendVector(...)); https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:206: forms->insert(forms->end(), append_forms_.begin(), append_forms_.end()); The documentation of PasswordStoreSync is sadly ambiguous, but "fill" may mean "assign", not "append". And in any case, your actual code under test can't tell the difference because the it passes an empty vector. So, you can replace this entirely with EXPECT_CALL(...).WillOnce(SetArgPointee<0>(...)); waitaminute. It looks like the caller takes ownership. Sure would be nice if that were documented. That means that assignment is semantically wrong (even if it's working because you never try deleting the pointers from append_forms_). That means that if the action never runs, there's a memory leak, and if it runs more than once, there will be a double free. Unless I'm missing something, it looks like you never actually have more than one PasswordForm, in which case you can simply do ACTION_P(AppendLogin, form) { arg0->push_back(new PasswordForm(form)); return true; } ...WillOnce(AppendLogin(form)); ...WillOnce(Return(true)); Given that you don't actually have any reason to track how many calls to the fill functions happen, the easiest thing would be to just do ON_CALL(*password_store_, FillFooLogins(_)).WillByDefault(Return(true)); for the two fill functions and then in the few cases that you need to do PasswordForm form; form.blah = blah; ON_CALL(*wrapper_.password_store(), AppendForm(form)); https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:213: scoped_refptr<testing::StrictMock<MockPasswordStore> > password_store_; StrictMock defines a subclass of its template parameter. that means that you can avoid repeating the StrictMock anywhere that you could use a baseclass pointer: Foo* foo = new StrictMock<Foo>(); https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:230: static scoped_ptr<MockSyncChangeProcessor> CreateMockSyncProcessor() { are SyncChangeProcessors particularly heavyweight? Why not just have a protected field scoped_ptr<MockSyncChangeProcessor> processor_; that gets populated in the constructor? There's a few tests where it would be unused, but that should be fine. (Note that in GTest it's entirely normal to have protected fields that are used by all the test bodies; there's no need to provide getters for fields in the test fixture, and most fields should be protected, not private.) https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:246: SetPasswordStoreData(forms, std::vector<autofill::PasswordForm*>()); If you take my suggestions above, the code in this test up to this point becomes *much* simpler: autofill::PasswordForm form; form.signon_realm = "abc"; ON_CALL(*password_store(), FillAutofillableLogins(_)) .WillByDefault(AppendForm(form)); Or you could even provide different helper to wrapper_ so that it just becomes autofill::PasswordForm form; form.signon_realm = "abc"; wrapper_.set_autofillable_logins(form); https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:259: EXPECT_CALL(*processor, ProcessSyncChanges(_,_)) need a space in "_, _". https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:260: .WillOnce(VerifySyncChanges(sync_changes)); if you take the suggestions above, this simply becomes EXPECT_CALL(*processor_, ProcessSyncChanges(_, ElementsAre( SyncChangeIs(SyncChange::ACTION_ADD, form)))); Yes, that's weird indenting compared to most code. Tests are a bit more flexible in formatting where in provides a clear win, at long as the relaxed formatting doesn't "hide" anything. In this case, the two lines can be read as "Here's all the context of what I'm talking about, the exact list of elements passed to ProcessSyncChanges. And what those elements are is just a SyncChange that is an ADD of the given form.". https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:261: EXPECT_CALL(*service(), NotifyPasswordStoreOfLoginChanges(_)); I don't think this mock method is doing anything at all for the tests; it's just noise. https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:284: EXPECT_CALL(*processor, ProcessSyncChanges(_,_)) You should be able to use EXPECT_CALL(*processor_, ProcessSyncChanges(_, IsEmpty()));
https://codereview.chromium.org/403323004/diff/20001/components/password_mana... File components/password_manager/core/browser/password_syncable_service_unittest.cc (right): https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:43: void PasswordsEqual(const sync_pb::PasswordSpecificsData& expected_password, On 2014/08/20 03:30:46, manthd wrote: > On 2014/08/19 15:55:44, vasilii wrote: > > On 2014/08/15 04:54:28, manthd wrote: > > > On 2014/08/14 07:19:50, Ilya Sherman wrote: > > > > On 2014/08/14 06:55:47, manthd wrote: > > > > > You get **much** better error messages by using GMock's EqualsProto. > > > > > > > > > > ...Is that really not available in Chromium? > > > > > > > > > > Well, you can still vastly improve the error messages with something > like > > > > > MATCHER_P(EqualsPasswordSpecifics, password, "") { > > > > > bool ok = true; > > > > > if (arg.scheme() != password.scheme()) { > > > > > *result_listener << "wrong scheme: '" << arg.scheme() << "' > instead > > of > > > > '" > > > > > << password.scheme() << ", "; > > > > > ok = false; > > > > > } > > > > > if ...signon_realm... > > > > > return ok; > > > > > } > > > > > > > > > > except that the "..." in what I have is a lot of verbose code. > > > > > > > > > > this would actually be a good place for a macro (you don't have any mix > of > > > > > different cases here). > > > > > > > > > > or, you could easily do > > > > > MATCHER... { > > > > > testing::MatchResultListener os = result_listener; > > > > > return > > > > > EqualField(os, "scheme", arg.scheme(), password.scheme()) && > > > > > EqualField(os, "signon_realm", arg.signon_realm(), ... ) && > > > > > ... && > > > > > ...; > > > > > where EqualField is a simple template function. > > > > > > > > > > ...or you could go yell at somebody to public EqualsProto. I do not > > > > understand > > > > > how that could possibly not have already been done. *sigh* > > > > > > > > > > More seriously, you can keep this as is, but it's got big downsides, > which > > > is > > > > > that if this function ever finds a mismatch, then GUnit will report as > the > > > > line > > > > > that caused a problem, a line within this function body, not any line > > within > > > a > > > > > test body below. > > > > > > > > > > Unfortunately, you you call PasswordsEqual in a loop below, which base > > even > > > > > getting a line number in the test body less valuable. > > > > > > > > > > GMock mostly works around this in the EqualsProto case in that when you > > > write > > > > > EXPECT_THAT(whatever, EqualsProto(some_expected_proto)); > > > > > the EqualsProto object actually knows how to print the > some_expected_proto > > > > > object. > > > > > > > > > > There's also the option of using SCOPED_TRACE with a string that > > > distinguishes > > > > > different loop iterations. > > > > > > > > > > So, you have a lot of bad choices, which I want you to at least > consider, > > > but > > > > I > > > > > guess the one you currently have is close to the best option. But > > > seriously, > > > > > I'd suggest that somebody file a Chromium bug to export EqualsProto > (both > > > > GMock > > > > > and protobuffers are already in use by Chromium!). > > > > > > > > EqualsProto would be handy to have. I don't know if it's available or not > > -- > > > > Chromium has only relatively recently started using protobuffers. > > > > > > > > Chromium does have a policy to limit the use of all but the most basic > GMock > > > > statements. More precisely, many of the senior engineers on the team > > declared > > > > that GMock was like learning another language, and that it was too much > > > overhead > > > > for test readability (that's, of course, very simplified, so please don't > > > quote > > > > me on that). Thus, things that "are pretty clear" are allowed, but things > > > "that > > > > are complex" are not. In practice, that probably means that EqualsProto > > would > > > > be smiled upon, but a MATCHER would be frowned upon. > > > > > > I've read some of those discussions about using GMock; it's worth noting > that > > > Matchers are part of GMock because they're critical for mocking function > > calls, > > > but they can also be used in GUnit with no mocking at all, via EXPECT_THAT, > > and > > > the worries about GMock usage just don't apply to EXPECT_THAT. > > > > > > That means that EqualsProto has utility even if nobody ever mocks a single > > > function. As such, making EqualsProto public would be an unmitigated win; > > it's > > > a no-brainer once Chromium is using Protobuffers and has GMock in its source > > > tree. > > > > > > Now, I understand some of the concerns about GMock; mocking out dependencies > > > runs the risk of letting client code go out of sync with the behavior of its > > > real dependencies and letting unit tests continue to pass despite that > > mismatch. > > > That concern can be mitigated by good management of who defines mock > > > subclasses. > > > > > > There's also the concern of GMock's domain-specific language. Again, > Matchers > > > fall into a rather different case than the rest of GMock: the matching step > > > itself is really pretty transparent, and most of the complexity comes from > > > understanding how mock functions respond to having matched a particular mock > > > call. Even if there's a general concern about keeping GMock use simple, it > > > doesn't seem to impinge much on things like MATCHER. (And it looks like > there > > > are already 50 uses of it in the Chromium codebase.) I admit that ACTION is > a > > > different matter, though I see uses of it, too. > > > > > > But most importantly, if simplicity is the goal, PasswordStoreDataVerifier > > > absolutely must be removed. If I've understood the existing code correctly > > > (which is fairly hard), about 230 lines of code can become about 25 by > turning > > > FormFinder into a MATCHER and defining one ACTION, and that's with zero > change > > > in the risk of dependency-behavior skew, since that risk comes for having a > > mock > > > class, not from having the mock class's behavior in ACTION bodies. > > > > How should I proceed with this? > > I think that if you take the detailed suggestions I've given on this pass, > you'll be able to remove a lot of support code, with only a few clearly-defined > GMock helpers, and the test bodies themselves will become much more concise. > > I believe my suggestions respect the concerns that motivate limiting the use of > GMock. If you're worried about that, ask Ilya if he agrees with my assessment > before working to implement my suggestions. What is this EqualsProto? I can't find it in the gMock cook book. https://codereview.chromium.org/403323004/diff/40001/components/password_mana... File components/password_manager/core/browser/password_syncable_service.cc (right): https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service.cc:26: return password_form.scheme == password_specifics.scheme() && On 2014/08/20 03:30:46, manthd wrote: > add parens; see comment below Done. https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service.cc:50: return net::EscapePath(password.origin.spec()) + "|" + On 2014/08/20 03:30:46, manthd wrote: > Very minor, but if you don't use parens here, the officially correct way of > wrapping is to use the usual 4-space indent: > return blah + "|" + > blah + "|" + ...; > which is obviously worse than what you do have here, but you can keep it pretty > and conform to official style by just adding paretheses: > return (blah + ... > blah + ...); Done. https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service.cc:60: case PasswordStoreChange::ADD: On 2014/08/20 03:30:46, manthd wrote: > you have the option of packing these onto one line each, which would make > clearer how similar each case is. Done. https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service.cc:84: ScopedVector<autofill::PasswordForm> new_sync_entries; On 2014/08/20 03:30:47, manthd wrote: > Part of the added ugliness of this solution is due to the long names. The > elements of SyncEntries really don't need to include the word "sync" in their > names; that's already included in the surrounding content. (The same holds for > "entries", but you do still need a noun to make the field names field-like.) > > At minimum, you can make the expressions below into > ...sync_entries.new_entries... > instead of repeating "sync". Done. https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service.cc:192: ScopedVector<autofill::PasswordForm> deleted_entries; On 2014/08/20 03:30:47, manthd wrote: > Both calls to WriteToPasswordStore take the deleted_entries, too, so move this > third vector into the SyncEntries struct. Done. https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service.cc:198: switch (it->change_type()) { On 2014/08/20 03:30:46, manthd wrote: > this is still a lot of repetion that makes less clear what's similar for all the > cases. if you move the delete entries into the struct, then it becomes natural > to write something like > typedef ScopedVector<autofill::PasswordForm> EntryList; // put this in > SyncEntries. > SyncEntries::EntryList* entries = > sync_entries.entries_for_change_type(it->change_type()); > if (entries == NULL) { > return ...CreateAndUploadError...; > } > AppendPasswordfFromSpecifics( > blah...data(), time_time, entries); > where SyncEntries::entries_for_change_type is what does this switch, but that's > much cleaner because it's doesn't need braces or breaks, since each case just > returns, such as > case syncer::SyncChange::ACTION_ADD: return &new_entries; > > The only downside to this is that it puts times in the deleted_entries protos. > It's not clear to me whether that's actually a problem, but if it is, then > WriteToPasswordStore can strip them out. Done. https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service.cc:252: ::password_manager::MakePasswordSyncTag(it->form()), On 2014/08/20 03:30:46, manthd wrote: > you don't need to qualify uses of functions in your own namespace. Here I need. After I moved MakePasswordSyncTag(const sync_pb::PasswordSpecificsData&) to the class scope it takes precedence. https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service.cc:357: std::string tag = ::password_manager::MakePasswordSyncTag(*password_form); On 2014/08/20 03:30:47, manthd wrote: > remove unneeded qualification. > > that also lets you avoid this temp variable. See above. Without this variable the assignment doesn't fit into one line. https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service.cc:399: time_now, On 2014/08/20 03:30:47, manthd wrote: > I see that there are at least some function calls in Chromium that do pack more > than one parameter onto a line. that can help the reader see more of what's > going on in the large context by not fluffing the code so much. I think it > would be a pretty big improvement in the case of these 'time_now' arguments. Yeah, for me it's a mystery when we allow or disallow multiple parameters on one line. https://codereview.chromium.org/403323004/diff/40001/components/password_mana... File components/password_manager/core/browser/password_syncable_service_unittest.cc (right): https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:26: using testing::get; On 2014/08/20 03:30:48, manthd wrote: > what uses this? Now nothing. I was experimenting with Pointwise(m, container) to compare sync changes. https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:74: PasswordsEqual( On 2014/08/20 03:30:47, manthd wrote: > Semantically, this doesn't really belong here. Deciding which form to operate > on is the job of the matcher, deciding what to do with it is the Action's job. > > It would be better to remove the 'form' parameter from this action entirely and > make the matcher tighter. At minimum, that would allow your EXPECT_CALLs below > to not repeat the form both as an arg to the matcher and as an arg to the > action. > > That does mean that this action should be renamed. Maybe to something like > MakePasswordList. > > Making the matcher stricter is easy enough if you don't mind not having a > field-by-field diff: > MATCHER_P(PasswordIs, form, "") { > sync_pb::PasswordSpecificsData expected_password = > > GetPasswordSpecifics(PasswordSyncableService::SyncDataFromPassword(form)); > sync_pb::PasswordSpecificsData actual_password = > ...(arg); > if (expected_password.scheme() == actual_password.scheme() && > ... && ... && ...) { > return true; > } > *result_listener << "Password protobuffer for form does not match; > expected:\n" > << expected_password.DebugString() > << "\nactual:\n" > << actual_password.DebugString(); > return false; > } I wonder what the error message will be. If this unique key matches I can compare some other fields and print meaningful message if for example EXPECT_EQ(expected_password.scheme(), actual_password.scheme()); fails. If I put everything to the matcher then I'll get "Uninteresting mock call" and 100 fields of expected and actual passwords. https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:83: } On 2014/08/20 03:30:47, manthd wrote: > Notice that every EXPECT_CALL on password_store() uses the same action. I > believe this was basically what motivated the criticism you pointed to by a > previous reviewer. > > Basically, GMock mock methods actually mix two jobs of a mocking framework, > verification and stub implementation. Verification consists of listing the set > of method calls that are expected (and failing the test if something different > happens), which stubbing consists of specifying what the results of method calls > against mocked-out dependencies are. These two jobs mix together easily, > because verification needs to be expressed in terms of what param values are > passed to methods, while stub implementation behavior generally depends on the > param values. > > But the two jobs can be done separately, and it can make sense to make this > separation if the implementation behavior doesn't depend on the particular calls > that are being stubbed. If *every* call to, say, AddLoginImpl() responds by > returning a one-element list of its argument, then there's no need to associate > that stub implementation with any particular call. > > I believe the suggestion by the previous reviewer amounted to requesting this > separation. It can be hard to think about separating the jobs because of how > GMock mixes them: it provides a single override of the method being mocked and > only allows stub behaviors to be set for the method by associating a set of > matchers for the parameters. It's common enough to want to combine the two jobs > that this is usually the right choice, but there's an alternative. > > I'm going to pretend that PasswordStore has only one method, AddLoginImpl, and > I'm going to be lazy about exact type signatures here; it should be obvious > enough what I mean in the real code. Now, the current definition of > MockPasswordStore is essentially > class MockPasswordStore : public PasswordStore { > MOCK_METHOD(AddLoginImpl, ChangeList(PasswordForm)); > }; > this means that you have to write > EXPECT_CALL(store_, AddLoginImpl(some_expected_param)) > .WillOnce(DoTheOneImplementation()); > > It would be possible to instead write > class FakePasswordStore : ... { > ChangeList AddLoginImpl(PasswordForm form) override { > return ...// do the one implementation > } > }; > which avoids the need to specify the implementation for every expected call, but > also discards the ability to check which calls occur. > > It's possible to mix these: > class SpyPasswordStore : ... { > MOCK_METHOD(AddCalled, void(PasswordForm)); > ChangeList AddLoginImpl(PasswordForm form) override { > AddCalled(form); > return ...// do the one implementation > } > }; > and then use it with > EXPECT_CALL(store_, AddCalled(some_expected_param)); > without any specification of the behavior to have (since GMock happily assumes > that a void method's default behavior is to do nothing, and there's no need to > do anything in the call AddCalled().) > > This really comes down to a question of what the tests need to test. Given that > the code under test doesn't actually *use* the PasswordStoreChangeList, except > to pass it back to the password store (except that you've disable even that > behavior in the tests), I don't think there's any benefit to having the behavior > of {Add,Remove,Update}LoginIml be specified by Actions. > > In fact, you can further simply the code under test by removing the > NotifyPasswordStoreOfLoginChanges wrapper function and instead just > unconditionally calling password_store_->NotifyLoginsChanged. Since you're > already providing a test-specific PasswordStore subclass, you can have *that* > ignore the NotifyLoginsChanged if you need it ignored. > > On the other hand, I do think your tests should be confirming which forms are > passed to AddLoginImpl and kin. So, that gives you two choices: You can > implement something like the SpyPasswordStore I describe above, where you > provide new mock methods with which to verify what password forms are passed, > while having the behavior of the implementation unconditionally run after > calling that new mock method. But I think you actually have an even simpler > alternative here. Since the *only* effect of the return values of FooLooginImpl > are to provide values to be passed to NotifyLoginsChanged, and the real > implementation of that fuction does nothing if there are no element is the > changes list, then you could just have all three of the FooLoginImpl methods > return empty lists. That's a very simple thing to do using GMock, so you could > avoid writing the separate spy functions: In the same spot as you currently > create the MockPasswordStore, just add > ON_CALL(*password_store_, AddLoginImpl(_)) > .WillByDefault(Return(PasswordStoreChangeList())); > ON_CALL(*password_store_, RemoveLoginImpl(_)) > .WillByDefault(Return(PasswordStoreChangeList())); > ON_CALL(*password_store_, UpdateLoginImpl(_)) > .WillByDefault(Return(PasswordStoreChangeList())); > or if something changes so that you're subclassing MockPasswordStore, the > constructor of that subclass is another good place to do these ON_CALLs. I like this proposal! I envision the next step. We'll move StartSyncFlare somewhere and delete MockPasswordSyncableService. Where should I move it to? https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:86: const SyncChangeList& change_list = arg1; On 2014/08/20 03:30:48, manthd wrote: > Most of this function is re-implementing the exact logic of > testing::UnorderedElementsAre(). > > And as above, this work belongs in a Matcher, not in an action. > > You do a per-element matcher that handles the appropriate types: > MATCHER_P2(SyncChangeIs, change_type, password, "") { > const SyncData& data = arg.sync_data(); > return (arg.change_type == change_type && > syncer::SyncDataLocal(data).GetTag() == > PasswordSyncableService::MakePasswordSyncTag(password) && > (change_type != Blah::DELETE || > testing::ValueIs(data, PasswordIs(password))); > } > > and then in the test bodies just do > EXPECT_CALL(procesor_, > ProcessSyncChanges(_, ElementsAre( > SyncChangeIs(Blah::ADD, form)))); > > If I recall correctly, GMock will even happily default-construct the return type > if you don't specify an action, which will do the same thing as > return SyncError(); > If I'm wrong about that, then ON_CALL().WillByDefault() would be a good choice. Done. https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:99: expected_password)) { On 2014/08/20 03:30:47, manthd wrote: > wrong indent. Indent stuff inside of the (...) of an 'if' as if it were a code > block. You would write > { > actual_tag = Blah::MakePasswordSyncTag( > expected_password); > } > ^^^^ indent of 4 from beginning of expression that spans lines > so you should write > if (actual_tag == PasswordSyncableService::MakePasswordSyncTag( > expected_password)) { > ^^^^ Removed. https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:192: .RetiresOnSaturation(); On 2014/08/20 03:30:47, manthd wrote: > This is very surprising. RetiresOnSaturation is normally only needed when a set > of expectations on a single method needs to be built incrementally. > > I don't actually see any spots in the code where this will have any effect. > What changes if you remove this? > > In any case, if the only reason for this is to allow the password store data to > be changes by calling SetPasswordStoreData more than once in a test, you could > simply use > EXPECT_CALL(...).WillRepeatedly(...); > which avoids placing any requirements on how many times the method is called. > > The FillFooLogins functions actually are good example of how not to define mock > methods. (I know that you don't own MockPasswordStore, so I'm not asking for > additional changes here.) Basically, it's *very* unlikely that a test should > ever really care how many times these fill functions are called; all it really > needs to do is provide the values that should be returned when they are called. > It would have been better for the owner of MockPasswordStore to provide "fake" > implementations of the fill functions (not MOCK_METHOD versions of them), which > simply copy from an internal vector, and add some setter functions for > populating that internal vector. So, instead of writing > EXPECT_CALL(password_store, FillFooLogins(_)) > .WillRepeatedly(SetArgPointee<0>(logins)); > the user could just write > password_store.set_foo_logins(logins); > > In the unlikely event that the client of PasswordStore wants to test how many > times the fill functions are called, they can write MOCK_METHOD(FillFooLogins, > ...) themselves. The caller of FillAutofillableLogins owns PasswordForms and destroys them after the call. Here I didn't want to return the vector with dangling pointers the second time. https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:199: struct AppendVector { On 2014/08/20 03:30:48, manthd wrote: > This is a verbose reimplementation of what GMock's ACTION macro does. Just do > ACTION_P(AppendVector, elements) { > arg0->insert(arg0->end(), elements.begin(), element.end()); > } > > You then do not need Invoke: > EXPECT_CALL(...).WillOnce(AppendVector(...)); Done. https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:206: forms->insert(forms->end(), append_forms_.begin(), append_forms_.end()); On 2014/08/20 03:30:47, manthd wrote: > The documentation of PasswordStoreSync is sadly ambiguous, but "fill" may mean > "assign", not "append". And in any case, your actual code under test can't tell > the difference because the it passes an empty vector. So, you can replace this > entirely with > EXPECT_CALL(...).WillOnce(SetArgPointee<0>(...)); > > waitaminute. It looks like the caller takes ownership. Sure would be nice if > that were documented. That means that assignment is semantically wrong (even if > it's working because you never try deleting the pointers from append_forms_). > That means that if the action never runs, there's a memory leak, and if it runs > more than once, there will be a double free. > > Unless I'm missing something, it looks like you never actually have more than > one PasswordForm, in which case you can simply do > ACTION_P(AppendLogin, form) { > arg0->push_back(new PasswordForm(form)); > return true; > } > ...WillOnce(AppendLogin(form)); > ...WillOnce(Return(true)); > > Given that you don't actually have any reason to track how many calls to the > fill functions happen, the easiest thing would be to just do > ON_CALL(*password_store_, FillFooLogins(_)).WillByDefault(Return(true)); > for the two fill functions and then in the few cases that you need to do > PasswordForm form; > form.blah = blah; > ON_CALL(*wrapper_.password_store(), AppendForm(form)); > I had to use EXPECT_CALL because we use the strict mock. https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:213: scoped_refptr<testing::StrictMock<MockPasswordStore> > password_store_; On 2014/08/20 03:30:48, manthd wrote: > StrictMock defines a subclass of its template parameter. that means that you > can avoid repeating the StrictMock anywhere that you could use a baseclass > pointer: > Foo* foo = new StrictMock<Foo>(); Done. https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:230: static scoped_ptr<MockSyncChangeProcessor> CreateMockSyncProcessor() { On 2014/08/20 03:30:47, manthd wrote: > are SyncChangeProcessors particularly heavyweight? Why not just have a > protected field > scoped_ptr<MockSyncChangeProcessor> processor_; > that gets populated in the constructor? > > There's a few tests where it would be unused, but that should be fine. > > (Note that in GTest it's entirely normal to have protected fields that are used > by all the test bodies; there's no need to provide getters for fields in the > test fixture, and most fields should be protected, not private.) Whenever we use CreateMockSyncProcessor(), PasswordSyncableService takes ownership of this object. I thought that with the local variable the chance to use NULL pointer is lower than with processor_; https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:246: SetPasswordStoreData(forms, std::vector<autofill::PasswordForm*>()); On 2014/08/20 03:30:47, manthd wrote: > If you take my suggestions above, the code in this test up to this point becomes > *much* simpler: > autofill::PasswordForm form; > form.signon_realm = "abc"; > ON_CALL(*password_store(), FillAutofillableLogins(_)) > .WillByDefault(AppendForm(form)); > > Or you could even provide different helper to wrapper_ so that it just becomes > autofill::PasswordForm form; > form.signon_realm = "abc"; > wrapper_.set_autofillable_logins(form); Done. https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:259: EXPECT_CALL(*processor, ProcessSyncChanges(_,_)) On 2014/08/20 03:30:47, manthd wrote: > need a space in "_, _". Done. https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:260: .WillOnce(VerifySyncChanges(sync_changes)); On 2014/08/20 03:30:48, manthd wrote: > if you take the suggestions above, this simply becomes > EXPECT_CALL(*processor_, ProcessSyncChanges(_, ElementsAre( > SyncChangeIs(SyncChange::ACTION_ADD, form)))); > > Yes, that's weird indenting compared to most code. Tests are a bit more > flexible in formatting where in provides a clear win, at long as the relaxed > formatting doesn't "hide" anything. In this case, the two lines can be read as > "Here's all the context of what I'm talking about, the exact list of elements > passed to ProcessSyncChanges. And what those elements are is just a SyncChange > that is an ADD of the given form.". Done. https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:261: EXPECT_CALL(*service(), NotifyPasswordStoreOfLoginChanges(_)); On 2014/08/20 03:30:48, manthd wrote: > I don't think this mock method is doing anything at all for the tests; it's just > noise. But it's important that the PasswordStore is notified. Also we use strict mocks for everything, so the test will fail otherwise. https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:284: EXPECT_CALL(*processor, ProcessSyncChanges(_,_)) On 2014/08/20 03:30:48, manthd wrote: > You should be able to use > EXPECT_CALL(*processor_, ProcessSyncChanges(_, IsEmpty())); Done.
https://codereview.chromium.org/403323004/diff/40001/components/password_mana... File components/password_manager/core/browser/password_syncable_service_unittest.cc (right): https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:206: forms->insert(forms->end(), append_forms_.begin(), append_forms_.end()); On 2014/08/20 20:11:41, vasilii wrote: > On 2014/08/20 03:30:47, manthd wrote: > > The documentation of PasswordStoreSync is sadly ambiguous, but "fill" may mean > > "assign", not "append". And in any case, your actual code under test can't > tell > > the difference because the it passes an empty vector. So, you can replace > this > > entirely with > > EXPECT_CALL(...).WillOnce(SetArgPointee<0>(...)); > > > > waitaminute. It looks like the caller takes ownership. Sure would be nice if > > that were documented. That means that assignment is semantically wrong (even > if > > it's working because you never try deleting the pointers from append_forms_). > > That means that if the action never runs, there's a memory leak, and if it > runs > > more than once, there will be a double free. > > > > Unless I'm missing something, it looks like you never actually have more than > > one PasswordForm, in which case you can simply do > > ACTION_P(AppendLogin, form) { > > arg0->push_back(new PasswordForm(form)); > > return true; > > } > > ...WillOnce(AppendLogin(form)); > > ...WillOnce(Return(true)); > > > > Given that you don't actually have any reason to track how many calls to the > > fill functions happen, the easiest thing would be to just do > > ON_CALL(*password_store_, FillFooLogins(_)).WillByDefault(Return(true)); > > for the two fill functions and then in the few cases that you need to do > > PasswordForm form; > > form.blah = blah; > > ON_CALL(*wrapper_.password_store(), AppendForm(form)); > > > > I had to use EXPECT_CALL because we use the strict mock. Is it important that we use a strict mock rather than a nice mock?
https://codereview.chromium.org/403323004/diff/40001/components/password_mana... File components/password_manager/core/browser/password_syncable_service_unittest.cc (right): https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:206: forms->insert(forms->end(), append_forms_.begin(), append_forms_.end()); On 2014/08/20 20:26:34, Ilya Sherman wrote: > On 2014/08/20 20:11:41, vasilii wrote: > > On 2014/08/20 03:30:47, manthd wrote: > > > The documentation of PasswordStoreSync is sadly ambiguous, but "fill" may > mean > > > "assign", not "append". And in any case, your actual code under test can't > > tell > > > the difference because the it passes an empty vector. So, you can replace > > this > > > entirely with > > > EXPECT_CALL(...).WillOnce(SetArgPointee<0>(...)); > > > > > > waitaminute. It looks like the caller takes ownership. Sure would be nice > if > > > that were documented. That means that assignment is semantically wrong > (even > > if > > > it's working because you never try deleting the pointers from > append_forms_). > > > That means that if the action never runs, there's a memory leak, and if it > > runs > > > more than once, there will be a double free. > > > > > > Unless I'm missing something, it looks like you never actually have more > than > > > one PasswordForm, in which case you can simply do > > > ACTION_P(AppendLogin, form) { > > > arg0->push_back(new PasswordForm(form)); > > > return true; > > > } > > > ...WillOnce(AppendLogin(form)); > > > ...WillOnce(Return(true)); > > > > > > Given that you don't actually have any reason to track how many calls to the > > > fill functions happen, the easiest thing would be to just do > > > ON_CALL(*password_store_, FillFooLogins(_)).WillByDefault(Return(true)); > > > for the two fill functions and then in the few cases that you need to do > > > PasswordForm form; > > > form.blah = blah; > > > ON_CALL(*wrapper_.password_store(), AppendForm(form)); > > > > > > > I had to use EXPECT_CALL because we use the strict mock. > > Is it important that we use a strict mock rather than a nice mock? I can use a nice mock. But we don't want to allow and ignore calling AddLoginImpl 10 times.
https://codereview.chromium.org/403323004/diff/40001/components/password_mana... File components/password_manager/core/browser/password_syncable_service_unittest.cc (right): https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:206: forms->insert(forms->end(), append_forms_.begin(), append_forms_.end()); On 2014/08/21 08:06:21, vasilii wrote: > On 2014/08/20 20:26:34, Ilya Sherman wrote: > > On 2014/08/20 20:11:41, vasilii wrote: > > > On 2014/08/20 03:30:47, manthd wrote: > > > > The documentation of PasswordStoreSync is sadly ambiguous, but "fill" may > > mean > > > > "assign", not "append". And in any case, your actual code under test > can't > > > tell > > > > the difference because the it passes an empty vector. So, you can replace > > > this > > > > entirely with > > > > EXPECT_CALL(...).WillOnce(SetArgPointee<0>(...)); > > > > > > > > waitaminute. It looks like the caller takes ownership. Sure would be > nice > > if > > > > that were documented. That means that assignment is semantically wrong > > (even > > > if > > > > it's working because you never try deleting the pointers from > > append_forms_). > > > > That means that if the action never runs, there's a memory leak, and if it > > > runs > > > > more than once, there will be a double free. > > > > > > > > Unless I'm missing something, it looks like you never actually have more > > than > > > > one PasswordForm, in which case you can simply do > > > > ACTION_P(AppendLogin, form) { > > > > arg0->push_back(new PasswordForm(form)); > > > > return true; > > > > } > > > > ...WillOnce(AppendLogin(form)); > > > > ...WillOnce(Return(true)); > > > > > > > > Given that you don't actually have any reason to track how many calls to > the > > > > fill functions happen, the easiest thing would be to just do > > > > ON_CALL(*password_store_, FillFooLogins(_)).WillByDefault(Return(true)); > > > > for the two fill functions and then in the few cases that you need to do > > > > PasswordForm form; > > > > form.blah = blah; > > > > ON_CALL(*wrapper_.password_store(), AppendForm(form)); > > > > > > > > > > I had to use EXPECT_CALL because we use the strict mock. > > > > Is it important that we use a strict mock rather than a nice mock? > > I can use a nice mock. But we don't want to allow and ignore calling > AddLoginImpl 10 times. That's what test expectations are for ;) A NiceMock requires you, as the person writing the tests, to decide which expectations matter, and which ones do not. The ones that matter, you keep as test expectations. The ones that are just to satisfy a strict mock object, you drop. This leads to the same amount of meaningful test coverage, but with less code, and therefore typically clearer code.
I was going to give you another pass on the test file, but I got busy today. I do have more comments, but here's what I got to. I'll give you a hopefully last pass tomorrow. https://codereview.chromium.org/403323004/diff/40001/components/password_mana... File components/password_manager/core/browser/password_syncable_service.cc (right): https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service.cc:252: ::password_manager::MakePasswordSyncTag(it->form()), On 2014/08/20 20:11:41, vasilii wrote: > On 2014/08/20 03:30:46, manthd wrote: > > you don't need to qualify uses of functions in your own namespace. > > Here I need. After I moved MakePasswordSyncTag(const > sync_pb::PasswordSpecificsData&) to the class scope it takes precedence. I see; I had lost track of the fact that one version was free and the other a class member. Your solution of just making both members is good. https://codereview.chromium.org/403323004/diff/40001/components/password_mana... File components/password_manager/core/browser/password_syncable_service_unittest.cc (right): https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:206: forms->insert(forms->end(), append_forms_.begin(), append_forms_.end()); On 2014/08/20 20:26:34, Ilya Sherman wrote: > On 2014/08/20 20:11:41, vasilii wrote: > > On 2014/08/20 03:30:47, manthd wrote: > > > The documentation of PasswordStoreSync is sadly ambiguous, but "fill" may > mean > > > "assign", not "append". And in any case, your actual code under test can't > > tell > > > the difference because the it passes an empty vector. So, you can replace > > this > > > entirely with > > > EXPECT_CALL(...).WillOnce(SetArgPointee<0>(...)); > > > > > > waitaminute. It looks like the caller takes ownership. Sure would be nice > if > > > that were documented. That means that assignment is semantically wrong > (even > > if > > > it's working because you never try deleting the pointers from > append_forms_). > > > That means that if the action never runs, there's a memory leak, and if it > > runs > > > more than once, there will be a double free. > > > > > > Unless I'm missing something, it looks like you never actually have more > than > > > one PasswordForm, in which case you can simply do > > > ACTION_P(AppendLogin, form) { > > > arg0->push_back(new PasswordForm(form)); > > > return true; > > > } > > > ...WillOnce(AppendLogin(form)); > > > ...WillOnce(Return(true)); > > > > > > Given that you don't actually have any reason to track how many calls to the > > > fill functions happen, the easiest thing would be to just do > > > ON_CALL(*password_store_, FillFooLogins(_)).WillByDefault(Return(true)); > > > for the two fill functions and then in the few cases that you need to do > > > PasswordForm form; > > > form.blah = blah; > > > ON_CALL(*wrapper_.password_store(), AppendForm(form)); > > > > > > > I had to use EXPECT_CALL because we use the strict mock. > > Is it important that we use a strict mock rather than a nice mock? I don't think it is; that was just added in this CL to avoid a few .Times(0)s. If it's getting in the way, it's better to re-add the few explicit .Times(0) notations. https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:230: static scoped_ptr<MockSyncChangeProcessor> CreateMockSyncProcessor() { On 2014/08/20 20:11:41, vasilii wrote: > On 2014/08/20 03:30:47, manthd wrote: > > are SyncChangeProcessors particularly heavyweight? Why not just have a > > protected field > > scoped_ptr<MockSyncChangeProcessor> processor_; > > that gets populated in the constructor? > > > > There's a few tests where it would be unused, but that should be fine. > > > > (Note that in GTest it's entirely normal to have protected fields that are > used > > by all the test bodies; there's no need to provide getters for fields in the > > test fixture, and most fields should be protected, not private.) > > Whenever we use CreateMockSyncProcessor(), PasswordSyncableService takes > ownership of this object. I thought that with the local variable the chance to > use NULL pointer is lower than with processor_; Meh, you're in a unit test; if you use a null pointer, the test fails and you know what happened. The greater brevity in this case is worth it. https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:261: EXPECT_CALL(*service(), NotifyPasswordStoreOfLoginChanges(_)); On 2014/08/20 20:11:42, vasilii wrote: > On 2014/08/20 03:30:48, manthd wrote: > > I don't think this mock method is doing anything at all for the tests; it's > just > > noise. > > But it's important that the PasswordStore is notified. Also we use strict mocks > for everything, so the test will fail otherwise. The reason it doesn't seem important is that you're already examining all of the objects being passed to NotifyLoginsChanged. Yes, of course it's important that your class actually calls NotifyLoginsChanged(), but is it really important to use a mock function to confirm that it happens? If there's any sort of end-to-end testing that uses your class, then you don't really need to test it here. If you stop using a StrictMock, the easiest resolution is actually to remove all the EXPECT_CALLs for FooLoginImpl's and instead just have EXPECT_CALL(*processor, ProcessSyncChanges(_, ElementsAre( SyncChangeIs(SyncChange::ACTION_ADD, form)))); EXPECT_CALL(*password_store(), NotifyLoginsChanged(UnsortedElementsAre( PasswordChangeIs(PasswordStoreChange::ADD, list.back())))); https://codereview.chromium.org/403323004/diff/60001/components/password_mana... File components/password_manager/core/browser/password_syncable_service.cc (right): https://codereview.chromium.org/403323004/diff/60001/components/password_mana... components/password_manager/core/browser/password_syncable_service.cc:206: it->change_type() == syncer::SyncChange::ACTION_DELETE ? base::Time() what I meant was to just write AppendPasswordFromSpecifics( specifics.password().client_only_encrypted_data(), time_now, entries); and then in WriteToPasswordStore clearing out those time. (that has the tiny cost of requiring you to change the function to taking a non-const pointer instead of a const ref.) but i suspect you don't actually need to do that second part; why would the service care about timestamps on things it's in the process of deleting? https://codereview.chromium.org/403323004/diff/60001/components/password_mana... components/password_manager/core/browser/password_syncable_service.cc:235: MakePasswordSyncTag(it->form()), rewrap now https://codereview.chromium.org/403323004/diff/60001/components/password_mana... components/password_manager/core/browser/password_syncable_service.cc:323: void PasswordSyncableService::NotifyPasswordStoreOfLoginChanges( there's no longer any reason for this to be its own function, right? you can just do NotifyLoginsChanged in WriteToPasswordStore. https://codereview.chromium.org/403323004/diff/60001/components/password_mana... File components/password_manager/core/browser/password_syncable_service_unittest.cc (right): https://codereview.chromium.org/403323004/diff/60001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:181: .Times(AnyNumber()) The .Times(AnyNumber()) is implicit when you use .WillRepeatedly(); remove this. https://codereview.chromium.org/403323004/diff/60001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:346: NotifyLoginsChanged(PasswordStoreChangeList())); use IsEmpty, not a default-constructed list. (check for other instances of this.) https://codereview.chromium.org/403323004/diff/60001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:365: EXPECT_CALL(weak_processor, ProcessSyncChanges(_, ElementsAre( Do you care about the order of changes? Use UnorderedElementsAre if not. https://codereview.chromium.org/403323004/diff/60001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:434: for (SyncDataList::iterator i(actual_list.begin()), j(expected_list.begin()); If I'm not mistaken, this is now exactly the same as EXPECT_THAT(service()->GetAllSyncData(syncer::PASSWORDS), ElementsAre( PasswordIs(form1), PasswordIs(form2)));
https://codereview.chromium.org/403323004/diff/40001/components/password_mana... File components/password_manager/core/browser/password_syncable_service.cc (right): https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service.cc:252: ::password_manager::MakePasswordSyncTag(it->form()), On 2014/08/22 05:49:47, manthd wrote: > On 2014/08/20 20:11:41, vasilii wrote: > > On 2014/08/20 03:30:46, manthd wrote: > > > you don't need to qualify uses of functions in your own namespace. > > > > Here I need. After I moved MakePasswordSyncTag(const > > sync_pb::PasswordSpecificsData&) to the class scope it takes precedence. > > I see; I had lost track of the fact that one version was free and the other a > class member. Your solution of just making both members is good. I need both version in the test anyway. https://codereview.chromium.org/403323004/diff/40001/components/password_mana... File components/password_manager/core/browser/password_syncable_service_unittest.cc (right): https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:206: forms->insert(forms->end(), append_forms_.begin(), append_forms_.end()); On 2014/08/22 05:49:47, manthd wrote: > On 2014/08/20 20:26:34, Ilya Sherman wrote: > > On 2014/08/20 20:11:41, vasilii wrote: > > > On 2014/08/20 03:30:47, manthd wrote: > > > > The documentation of PasswordStoreSync is sadly ambiguous, but "fill" may > > mean > > > > "assign", not "append". And in any case, your actual code under test > can't > > > tell > > > > the difference because the it passes an empty vector. So, you can replace > > > this > > > > entirely with > > > > EXPECT_CALL(...).WillOnce(SetArgPointee<0>(...)); > > > > > > > > waitaminute. It looks like the caller takes ownership. Sure would be > nice > > if > > > > that were documented. That means that assignment is semantically wrong > > (even > > > if > > > > it's working because you never try deleting the pointers from > > append_forms_). > > > > That means that if the action never runs, there's a memory leak, and if it > > > runs > > > > more than once, there will be a double free. > > > > > > > > Unless I'm missing something, it looks like you never actually have more > > than > > > > one PasswordForm, in which case you can simply do > > > > ACTION_P(AppendLogin, form) { > > > > arg0->push_back(new PasswordForm(form)); > > > > return true; > > > > } > > > > ...WillOnce(AppendLogin(form)); > > > > ...WillOnce(Return(true)); > > > > > > > > Given that you don't actually have any reason to track how many calls to > the > > > > fill functions happen, the easiest thing would be to just do > > > > ON_CALL(*password_store_, FillFooLogins(_)).WillByDefault(Return(true)); > > > > for the two fill functions and then in the few cases that you need to do > > > > PasswordForm form; > > > > form.blah = blah; > > > > ON_CALL(*wrapper_.password_store(), AppendForm(form)); > > > > > > > > > > I had to use EXPECT_CALL because we use the strict mock. > > > > Is it important that we use a strict mock rather than a nice mock? > > I don't think it is; that was just added in this CL to avoid a few .Times(0)s. > If it's getting in the way, it's better to re-add the few explicit .Times(0) > notations. I removed this global EXPECT_CALL().AnyTime() instead. https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:230: static scoped_ptr<MockSyncChangeProcessor> CreateMockSyncProcessor() { On 2014/08/22 05:49:47, manthd wrote: > On 2014/08/20 20:11:41, vasilii wrote: > > On 2014/08/20 03:30:47, manthd wrote: > > > are SyncChangeProcessors particularly heavyweight? Why not just have a > > > protected field > > > scoped_ptr<MockSyncChangeProcessor> processor_; > > > that gets populated in the constructor? > > > > > > There's a few tests where it would be unused, but that should be fine. > > > > > > (Note that in GTest it's entirely normal to have protected fields that are > > used > > > by all the test bodies; there's no need to provide getters for fields in the > > > test fixture, and most fields should be protected, not private.) > > > > Whenever we use CreateMockSyncProcessor(), PasswordSyncableService takes > > ownership of this object. I thought that with the local variable the chance to > > use NULL pointer is lower than with processor_; > > Meh, you're in a unit test; if you use a null pointer, the test fails and you > know what happened. The greater brevity in this case is worth it. Done. https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:261: EXPECT_CALL(*service(), NotifyPasswordStoreOfLoginChanges(_)); On 2014/08/22 05:49:47, manthd wrote: > On 2014/08/20 20:11:42, vasilii wrote: > > On 2014/08/20 03:30:48, manthd wrote: > > > I don't think this mock method is doing anything at all for the tests; it's > > just > > > noise. > > > > But it's important that the PasswordStore is notified. Also we use strict > mocks > > for everything, so the test will fail otherwise. > > The reason it doesn't seem important is that you're already examining all of the > objects being passed to NotifyLoginsChanged. > > Yes, of course it's important that your class actually calls > NotifyLoginsChanged(), but is it really important to use a mock function to > confirm that it happens? If there's any sort of end-to-end testing that uses > your class, then you don't really need to test it here. > > If you stop using a StrictMock, the easiest resolution is actually to remove all > the EXPECT_CALLs for FooLoginImpl's and instead just have > EXPECT_CALL(*processor, ProcessSyncChanges(_, ElementsAre( > SyncChangeIs(SyncChange::ACTION_ADD, form)))); > EXPECT_CALL(*password_store(), NotifyLoginsChanged(UnsortedElementsAre( > PasswordChangeIs(PasswordStoreChange::ADD, list.back())))); FooLoginImpl are even more important than NotifyLoginsChanged. I don't think we test somewhere else that NotifyLoginsChanged is called. UI depends on this notification. https://codereview.chromium.org/403323004/diff/60001/components/password_mana... File components/password_manager/core/browser/password_syncable_service.cc (right): https://codereview.chromium.org/403323004/diff/60001/components/password_mana... components/password_manager/core/browser/password_syncable_service.cc:206: it->change_type() == syncer::SyncChange::ACTION_DELETE ? base::Time() On 2014/08/22 05:49:47, manthd wrote: > what I meant was to just write > AppendPasswordFromSpecifics( > specifics.password().client_only_encrypted_data(), time_now, entries); > > and then in WriteToPasswordStore clearing out those time. (that has the tiny > cost of requiring you to change the function to taking a non-const pointer > instead of a const ref.) > > but i suspect you don't actually need to do that second part; why would the > service care about timestamps on things it's in the process of deleting? Done. https://codereview.chromium.org/403323004/diff/60001/components/password_mana... components/password_manager/core/browser/password_syncable_service.cc:235: MakePasswordSyncTag(it->form()), On 2014/08/22 05:49:47, manthd wrote: > rewrap now Done. https://codereview.chromium.org/403323004/diff/60001/components/password_mana... components/password_manager/core/browser/password_syncable_service.cc:323: void PasswordSyncableService::NotifyPasswordStoreOfLoginChanges( On 2014/08/22 05:49:47, manthd wrote: > there's no longer any reason for this to be its own function, right? you can > just do NotifyLoginsChanged in WriteToPasswordStore. Done. https://codereview.chromium.org/403323004/diff/60001/components/password_mana... File components/password_manager/core/browser/password_syncable_service_unittest.cc (right): https://codereview.chromium.org/403323004/diff/60001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:181: .Times(AnyNumber()) On 2014/08/22 05:49:47, manthd wrote: > The .Times(AnyNumber()) is implicit when you use .WillRepeatedly(); remove this. Done. https://codereview.chromium.org/403323004/diff/60001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:346: NotifyLoginsChanged(PasswordStoreChangeList())); On 2014/08/22 05:49:47, manthd wrote: > use IsEmpty, not a default-constructed list. (check for other instances of > this.) Done. https://codereview.chromium.org/403323004/diff/60001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:365: EXPECT_CALL(weak_processor, ProcessSyncChanges(_, ElementsAre( On 2014/08/22 05:49:47, manthd wrote: > Do you care about the order of changes? Use UnorderedElementsAre if not. Yes, ordering is very important. https://codereview.chromium.org/403323004/diff/60001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:434: for (SyncDataList::iterator i(actual_list.begin()), j(expected_list.begin()); On 2014/08/22 05:49:47, manthd wrote: > If I'm not mistaken, this is now exactly the same as > EXPECT_THAT(service()->GetAllSyncData(syncer::PASSWORDS), ElementsAre( > PasswordIs(form1), > PasswordIs(form2))); Almost.
I have on last substantive suggestion, and one question. Other than that, this is looking very close to done to me. Once you have my +1, you need your own team to complete the review and normal (that should be easy given that Ilya has been paying attention) and then once you submit the CL, I'll mark you readability bug fixed and a bot will give you readability (I hope. Chromium reviews are unusual; feel free to kick me or omoikane@ if the bot gets confused.) https://codereview.chromium.org/403323004/diff/20001/components/password_mana... File components/password_manager/core/browser/password_syncable_service_unittest.cc (right): https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:43: void PasswordsEqual(const sync_pb::PasswordSpecificsData& expected_password, On 2014/08/20 20:11:41, vasilii wrote: > On 2014/08/20 03:30:46, manthd wrote: > > On 2014/08/19 15:55:44, vasilii wrote: > > > On 2014/08/15 04:54:28, manthd wrote: > > > > On 2014/08/14 07:19:50, Ilya Sherman wrote: > > > > > On 2014/08/14 06:55:47, manthd wrote: > > > > > > You get **much** better error messages by using GMock's EqualsProto. > > > > > > > > > > > > ...Is that really not available in Chromium? > > > > > > > > > > > > Well, you can still vastly improve the error messages with something > > like > > > > > > MATCHER_P(EqualsPasswordSpecifics, password, "") { > > > > > > bool ok = true; > > > > > > if (arg.scheme() != password.scheme()) { > > > > > > *result_listener << "wrong scheme: '" << arg.scheme() << "' > > instead > > > of > > > > > '" > > > > > > << password.scheme() << ", "; > > > > > > ok = false; > > > > > > } > > > > > > if ...signon_realm... > > > > > > return ok; > > > > > > } > > > > > > > > > > > > except that the "..." in what I have is a lot of verbose code. > > > > > > > > > > > > this would actually be a good place for a macro (you don't have any > mix > > of > > > > > > different cases here). > > > > > > > > > > > > or, you could easily do > > > > > > MATCHER... { > > > > > > testing::MatchResultListener os = result_listener; > > > > > > return > > > > > > EqualField(os, "scheme", arg.scheme(), password.scheme()) && > > > > > > EqualField(os, "signon_realm", arg.signon_realm(), ... ) && > > > > > > ... && > > > > > > ...; > > > > > > where EqualField is a simple template function. > > > > > > > > > > > > ...or you could go yell at somebody to public EqualsProto. I do not > > > > > understand > > > > > > how that could possibly not have already been done. *sigh* > > > > > > > > > > > > More seriously, you can keep this as is, but it's got big downsides, > > which > > > > is > > > > > > that if this function ever finds a mismatch, then GUnit will report as > > the > > > > > line > > > > > > that caused a problem, a line within this function body, not any line > > > within > > > > a > > > > > > test body below. > > > > > > > > > > > > Unfortunately, you you call PasswordsEqual in a loop below, which base > > > even > > > > > > getting a line number in the test body less valuable. > > > > > > > > > > > > GMock mostly works around this in the EqualsProto case in that when > you > > > > write > > > > > > EXPECT_THAT(whatever, EqualsProto(some_expected_proto)); > > > > > > the EqualsProto object actually knows how to print the > > some_expected_proto > > > > > > object. > > > > > > > > > > > > There's also the option of using SCOPED_TRACE with a string that > > > > distinguishes > > > > > > different loop iterations. > > > > > > > > > > > > So, you have a lot of bad choices, which I want you to at least > > consider, > > > > but > > > > > I > > > > > > guess the one you currently have is close to the best option. But > > > > seriously, > > > > > > I'd suggest that somebody file a Chromium bug to export EqualsProto > > (both > > > > > GMock > > > > > > and protobuffers are already in use by Chromium!). > > > > > > > > > > EqualsProto would be handy to have. I don't know if it's available or > not > > > -- > > > > > Chromium has only relatively recently started using protobuffers. > > > > > > > > > > Chromium does have a policy to limit the use of all but the most basic > > GMock > > > > > statements. More precisely, many of the senior engineers on the team > > > declared > > > > > that GMock was like learning another language, and that it was too much > > > > overhead > > > > > for test readability (that's, of course, very simplified, so please > don't > > > > quote > > > > > me on that). Thus, things that "are pretty clear" are allowed, but > things > > > > "that > > > > > are complex" are not. In practice, that probably means that EqualsProto > > > would > > > > > be smiled upon, but a MATCHER would be frowned upon. > > > > > > > > I've read some of those discussions about using GMock; it's worth noting > > that > > > > Matchers are part of GMock because they're critical for mocking function > > > calls, > > > > but they can also be used in GUnit with no mocking at all, via > EXPECT_THAT, > > > and > > > > the worries about GMock usage just don't apply to EXPECT_THAT. > > > > > > > > That means that EqualsProto has utility even if nobody ever mocks a single > > > > function. As such, making EqualsProto public would be an unmitigated win; > > > it's > > > > a no-brainer once Chromium is using Protobuffers and has GMock in its > source > > > > tree. > > > > > > > > Now, I understand some of the concerns about GMock; mocking out > dependencies > > > > runs the risk of letting client code go out of sync with the behavior of > its > > > > real dependencies and letting unit tests continue to pass despite that > > > mismatch. > > > > That concern can be mitigated by good management of who defines mock > > > > subclasses. > > > > > > > > There's also the concern of GMock's domain-specific language. Again, > > Matchers > > > > fall into a rather different case than the rest of GMock: the matching > step > > > > itself is really pretty transparent, and most of the complexity comes from > > > > understanding how mock functions respond to having matched a particular > mock > > > > call. Even if there's a general concern about keeping GMock use simple, > it > > > > doesn't seem to impinge much on things like MATCHER. (And it looks like > > there > > > > are already 50 uses of it in the Chromium codebase.) I admit that ACTION > is > > a > > > > different matter, though I see uses of it, too. > > > > > > > > But most importantly, if simplicity is the goal, PasswordStoreDataVerifier > > > > absolutely must be removed. If I've understood the existing code > correctly > > > > (which is fairly hard), about 230 lines of code can become about 25 by > > turning > > > > FormFinder into a MATCHER and defining one ACTION, and that's with zero > > change > > > > in the risk of dependency-behavior skew, since that risk comes for having > a > > > mock > > > > class, not from having the mock class's behavior in ACTION bodies. > > > > > > How should I proceed with this? > > > > I think that if you take the detailed suggestions I've given on this pass, > > you'll be able to remove a lot of support code, with only a few > clearly-defined > > GMock helpers, and the test bodies themselves will become much more concise. > > > > I believe my suggestions respect the concerns that motivate limiting the use > of > > GMock. If you're worried about that, ask Ilya if he agrees with my assessment > > before working to implement my suggestions. > > What is this EqualsProto? I can't find it in the gMock cook book. Arg, sorry, I'd been assuming perhaps unreasonably that you were familiar with coding in the internal codebase as well as Chromium. It's a GMock matcher that uses protobuffer reflection. It checks that the protobuffer it's matched against is the correct type and then compares each field in the protobuffer to the expected value, recursing into submessages and respecting field types (i.e. int32 fields get numerically compared, string fields get checked for string equality) and protobuffer default values. Finally, it's very smart about error reporting so that you can write EXPECT_THAT(some_proto, EqualsProto(" some_repeated_field: 1" " some_repeated_field: 2")); and get messages with things like "Missing element 1 of 'some_repeated_field'; found unexpected field 'some_optional_field' with value "foo"." So, sort of like ContainerEq, only better. https://codereview.chromium.org/403323004/diff/40001/components/password_mana... File components/password_manager/core/browser/password_syncable_service_unittest.cc (right): https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:74: PasswordsEqual( On 2014/08/20 20:11:42, vasilii wrote: > On 2014/08/20 03:30:47, manthd wrote: > > Semantically, this doesn't really belong here. Deciding which form to operate > > on is the job of the matcher, deciding what to do with it is the Action's job. > > > > It would be better to remove the 'form' parameter from this action entirely > and > > make the matcher tighter. At minimum, that would allow your EXPECT_CALLs > below > > to not repeat the form both as an arg to the matcher and as an arg to the > > action. > > > > That does mean that this action should be renamed. Maybe to something like > > MakePasswordList. > > > > Making the matcher stricter is easy enough if you don't mind not having a > > field-by-field diff: > > MATCHER_P(PasswordIs, form, "") { > > sync_pb::PasswordSpecificsData expected_password = > > > > GetPasswordSpecifics(PasswordSyncableService::SyncDataFromPassword(form)); > > sync_pb::PasswordSpecificsData actual_password = > > ...(arg); > > if (expected_password.scheme() == actual_password.scheme() && > > ... && ... && ...) { > > return true; > > } > > *result_listener << "Password protobuffer for form does not match; > > expected:\n" > > << expected_password.DebugString() > > << "\nactual:\n" > > << actual_password.DebugString(); > > return false; > > } > > I wonder what the error message will be. If this unique key matches I can > compare some other fields and print meaningful message if for example > EXPECT_EQ(expected_password.scheme(), actual_password.scheme()); fails. If I put > everything to the matcher then I'll get "Uninteresting mock call" and 100 fields > of expected and actual passwords. It will print something like "Unexpected call AddLoginImpl(blah blah)". The "blah blah" is the full argument, if GTest knows how to print its type, but it will also then print something for each possible EXPECT_CALL and explain why that EXPECT_CALL didn't match. The non-matching can be made more verbose by using MATCHER's 'result_listener' implicit arg. Whether this is worth providing for really depends on the expected failure modes. This depends on whether the types are printable by GTest. https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:261: EXPECT_CALL(*service(), NotifyPasswordStoreOfLoginChanges(_)); On 2014/08/22 12:27:15, vasilii wrote: > On 2014/08/22 05:49:47, manthd wrote: > > On 2014/08/20 20:11:42, vasilii wrote: > > > On 2014/08/20 03:30:48, manthd wrote: > > > > I don't think this mock method is doing anything at all for the tests; > it's > > > just > > > > noise. > > > > > > But it's important that the PasswordStore is notified. Also we use strict > > mocks > > > for everything, so the test will fail otherwise. > > > > The reason it doesn't seem important is that you're already examining all of > the > > objects being passed to NotifyLoginsChanged. > > > > Yes, of course it's important that your class actually calls > > NotifyLoginsChanged(), but is it really important to use a mock function to > > confirm that it happens? If there's any sort of end-to-end testing that uses > > your class, then you don't really need to test it here. > > > > If you stop using a StrictMock, the easiest resolution is actually to remove > all > > the EXPECT_CALLs for FooLoginImpl's and instead just have > > EXPECT_CALL(*processor, ProcessSyncChanges(_, ElementsAre( > > SyncChangeIs(SyncChange::ACTION_ADD, form)))); > > EXPECT_CALL(*password_store(), NotifyLoginsChanged(UnsortedElementsAre( > > PasswordChangeIs(PasswordStoreChange::ADD, list.back())))); > > FooLoginImpl are even more important than NotifyLoginsChanged. I don't think we > test somewhere else that NotifyLoginsChanged is called. UI depends on this > notification. Sure, both need to happen, but that doesn't mean that both need to have explicit expectations. If you only checked that the expected changes showed up in NotifyLoginsChanged(), then the only way of meeting that expectation would be to call FooLoginImpl with the correct values, at least assuming that there's an obvious mapping from the Impl calls to the notification, and you know that's a good assumption because you're using a password_store that you've specifically told to have that obvious mapping. This falls under the category of "don't repeat yourself"; one of the two kinds of expectations is strictly less interesting then the other, so just skip them. https://codereview.chromium.org/403323004/diff/80001/components/password_mana... File components/password_manager/core/browser/password_syncable_service_unittest.cc (right): https://codereview.chromium.org/403323004/diff/80001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:48: if (check_date_synced) { This is a bad idea for the simple fact that the call sites of the matcher now have bear "true" or "false" in their argument list. It's completely opaque what that means at the call site. "PasswordIs(form1)" pretty clearly means "matches a given password form", but "PasswordIs(form1, false)" doesn't say anything about what the "false" might mean. Remove this parameter entirely and find another way to do the same enforcement. Unless I've lost track, you want this to be enforced on every call to AddLoginImpl or UpdateLoginImpl and in no other places. You've already got a good place to deal with all calls to these, near line 153 in this snapshot. You can just do MATCHER(HasDateSynced, "") { ...arg.date_synced... } and then changed the default actions so that ON_CALL(*password_store_, AddLoginImpl(HasDateSynced())) .WillByDefault(Return(PasswordStoreChangeList())); and same for updates. Then, if one of these functions were called with a bad PasswordForm, GMock would report that the call was expected but that it didn't know what to do with the call and therefore (1) failed, and (2) continued the test by returning the default-constructed PasswordStoreChangeList, which will also break any expectation you have on the set of changes, making clear what went wrong. https://codereview.chromium.org/403323004/diff/80001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:150: password_store_ = new testing::StrictMock<MockPasswordStore>; i'm still not convinced that this needs to be strict. I really don't think there's any value in having particular expectations of {Add,Remove,Update}LoginImpl. But do note that if you make this not-strict the suggestion I give above about HasDataSynced needs to be tweaked by changing these to EXPECT_CALL(..., FooLoginImpl(HasLoginImpl)) .WillRepeatedly(...); so that GMock doesn't simply turn all calls to these into warning logs, regardless of whether the args have data_synced. https://codereview.chromium.org/403323004/diff/80001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:186: .WillByDefault(Return(SyncError())); Could you show me the error that results from just commenting out this ON_CALL? I'm curious why GMock would have trouble with just default-constructing the return value. https://codereview.chromium.org/403323004/diff/80001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:196: nix https://codereview.chromium.org/403323004/diff/80001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:408: std::vector<autofill::PasswordForm> actual_form_list; Sorry, this is one of the spots that I meant to comment further on. It's much better to keep the body of the tests nice and simple so that the EXPECT calls can be read as pretty much the whole description of what's being enforced. So, it would be really nice if you could write what I suggested in my last comment. GMock matchers are by default "polymorphic" (a term GMock has given a particular meaning to, though it does fit with the more general use of the term). For example the matcher Gt(4) is constructed from an int, but will happily match the double 4.1, even though 4.1 clearly narrows to 4, which would not match. You have the option of making PasswordIs polymorphic in this sense that it can match a SyncData or a PasswordForm. In fact, it already semantically really does operate on both; PasswordIs is constructed with a PasswordForm, and it matches a PasswordForm, but it then converts both of those to SyncData and then to PasswordSpecificData. What you really want to be able to say inside of PasswordIs is something like sync_pb::PasswordSpecificsData actual_password = GetPasswordSpecifics(somehow_get_a_SyncData(arg)); sync_pb::PasswordSpecificsData expected_password = GetPasswordSpecifics(somehow_get_a_SyncData(form)); ...use the two specific data objects... This is easy to do with an overloaded helper function const syncer::SyncData& GetSyncData(const syncer::SyncData& sync_data) { return sync_data; } syncer::SyncData GetSyncData(const autofill::PasswordForm& password) { return PasswordSyncableService::SyncDataFromPassword(password); } The way this works is that the object defined by the function that MATCHER_P produces is convertible to *any* kind of matcher (which uses the body attached to the MATCHER_P, and thus which is only valid for some possible matcher conversions). In particular the 'arg' implicit parameter is a different type depending on where you use the matcher. Thus, if you write just EXPECT_THAT(actual_list, ElementsAre(PasswordIs(form1))); then, EXPECT_THAT sees that 'actual_list' is SyncDataList, and asks ElementsAre to create a Matcher<SyncDataList>, which then asks what type the elements are and thus asks PasswordIs to become a matcher of SyncData. That's then the type of 'arg' (actually, it's going to be 'const SyncData&'), which means that it can be passed to the first overload of GetSyncData, and thus it does nothing except pass the SyncData back out.
https://codereview.chromium.org/403323004/diff/20001/components/password_mana... File components/password_manager/core/browser/password_syncable_service_unittest.cc (right): https://codereview.chromium.org/403323004/diff/20001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:43: void PasswordsEqual(const sync_pb::PasswordSpecificsData& expected_password, On 2014/08/26 01:46:03, manthd wrote: > On 2014/08/20 20:11:41, vasilii wrote: > > On 2014/08/20 03:30:46, manthd wrote: > > > On 2014/08/19 15:55:44, vasilii wrote: > > > > On 2014/08/15 04:54:28, manthd wrote: > > > > > On 2014/08/14 07:19:50, Ilya Sherman wrote: > > > > > > On 2014/08/14 06:55:47, manthd wrote: > > > > > > > You get **much** better error messages by using GMock's EqualsProto. > > > > > > > > > > > > > > ...Is that really not available in Chromium? > > > > > > > > > > > > > > Well, you can still vastly improve the error messages with something > > > like > > > > > > > MATCHER_P(EqualsPasswordSpecifics, password, "") { > > > > > > > bool ok = true; > > > > > > > if (arg.scheme() != password.scheme()) { > > > > > > > *result_listener << "wrong scheme: '" << arg.scheme() << "' > > > instead > > > > of > > > > > > '" > > > > > > > << password.scheme() << ", "; > > > > > > > ok = false; > > > > > > > } > > > > > > > if ...signon_realm... > > > > > > > return ok; > > > > > > > } > > > > > > > > > > > > > > except that the "..." in what I have is a lot of verbose code. > > > > > > > > > > > > > > this would actually be a good place for a macro (you don't have any > > mix > > > of > > > > > > > different cases here). > > > > > > > > > > > > > > or, you could easily do > > > > > > > MATCHER... { > > > > > > > testing::MatchResultListener os = result_listener; > > > > > > > return > > > > > > > EqualField(os, "scheme", arg.scheme(), password.scheme()) && > > > > > > > EqualField(os, "signon_realm", arg.signon_realm(), ... ) && > > > > > > > ... && > > > > > > > ...; > > > > > > > where EqualField is a simple template function. > > > > > > > > > > > > > > ...or you could go yell at somebody to public EqualsProto. I do not > > > > > > understand > > > > > > > how that could possibly not have already been done. *sigh* > > > > > > > > > > > > > > More seriously, you can keep this as is, but it's got big downsides, > > > which > > > > > is > > > > > > > that if this function ever finds a mismatch, then GUnit will report > as > > > the > > > > > > line > > > > > > > that caused a problem, a line within this function body, not any > line > > > > within > > > > > a > > > > > > > test body below. > > > > > > > > > > > > > > Unfortunately, you you call PasswordsEqual in a loop below, which > base > > > > even > > > > > > > getting a line number in the test body less valuable. > > > > > > > > > > > > > > GMock mostly works around this in the EqualsProto case in that when > > you > > > > > write > > > > > > > EXPECT_THAT(whatever, EqualsProto(some_expected_proto)); > > > > > > > the EqualsProto object actually knows how to print the > > > some_expected_proto > > > > > > > object. > > > > > > > > > > > > > > There's also the option of using SCOPED_TRACE with a string that > > > > > distinguishes > > > > > > > different loop iterations. > > > > > > > > > > > > > > So, you have a lot of bad choices, which I want you to at least > > > consider, > > > > > but > > > > > > I > > > > > > > guess the one you currently have is close to the best option. But > > > > > seriously, > > > > > > > I'd suggest that somebody file a Chromium bug to export EqualsProto > > > (both > > > > > > GMock > > > > > > > and protobuffers are already in use by Chromium!). > > > > > > > > > > > > EqualsProto would be handy to have. I don't know if it's available or > > not > > > > -- > > > > > > Chromium has only relatively recently started using protobuffers. > > > > > > > > > > > > Chromium does have a policy to limit the use of all but the most basic > > > GMock > > > > > > statements. More precisely, many of the senior engineers on the team > > > > declared > > > > > > that GMock was like learning another language, and that it was too > much > > > > > overhead > > > > > > for test readability (that's, of course, very simplified, so please > > don't > > > > > quote > > > > > > me on that). Thus, things that "are pretty clear" are allowed, but > > things > > > > > "that > > > > > > are complex" are not. In practice, that probably means that > EqualsProto > > > > would > > > > > > be smiled upon, but a MATCHER would be frowned upon. > > > > > > > > > > I've read some of those discussions about using GMock; it's worth noting > > > that > > > > > Matchers are part of GMock because they're critical for mocking function > > > > calls, > > > > > but they can also be used in GUnit with no mocking at all, via > > EXPECT_THAT, > > > > and > > > > > the worries about GMock usage just don't apply to EXPECT_THAT. > > > > > > > > > > That means that EqualsProto has utility even if nobody ever mocks a > single > > > > > function. As such, making EqualsProto public would be an unmitigated > win; > > > > it's > > > > > a no-brainer once Chromium is using Protobuffers and has GMock in its > > source > > > > > tree. > > > > > > > > > > Now, I understand some of the concerns about GMock; mocking out > > dependencies > > > > > runs the risk of letting client code go out of sync with the behavior of > > its > > > > > real dependencies and letting unit tests continue to pass despite that > > > > mismatch. > > > > > That concern can be mitigated by good management of who defines mock > > > > > subclasses. > > > > > > > > > > There's also the concern of GMock's domain-specific language. Again, > > > Matchers > > > > > fall into a rather different case than the rest of GMock: the matching > > step > > > > > itself is really pretty transparent, and most of the complexity comes > from > > > > > understanding how mock functions respond to having matched a particular > > mock > > > > > call. Even if there's a general concern about keeping GMock use simple, > > it > > > > > doesn't seem to impinge much on things like MATCHER. (And it looks like > > > there > > > > > are already 50 uses of it in the Chromium codebase.) I admit that > ACTION > > is > > > a > > > > > different matter, though I see uses of it, too. > > > > > > > > > > But most importantly, if simplicity is the goal, > PasswordStoreDataVerifier > > > > > absolutely must be removed. If I've understood the existing code > > correctly > > > > > (which is fairly hard), about 230 lines of code can become about 25 by > > > turning > > > > > FormFinder into a MATCHER and defining one ACTION, and that's with zero > > > change > > > > > in the risk of dependency-behavior skew, since that risk comes for > having > > a > > > > mock > > > > > class, not from having the mock class's behavior in ACTION bodies. > > > > > > > > How should I proceed with this? > > > > > > I think that if you take the detailed suggestions I've given on this pass, > > > you'll be able to remove a lot of support code, with only a few > > clearly-defined > > > GMock helpers, and the test bodies themselves will become much more concise. > > > > > > I believe my suggestions respect the concerns that motivate limiting the use > > of > > > GMock. If you're worried about that, ask Ilya if he agrees with my > assessment > > > before working to implement my suggestions. > > > > What is this EqualsProto? I can't find it in the gMock cook book. > > Arg, sorry, I'd been assuming perhaps unreasonably that you were familiar with > coding in the internal codebase as well as Chromium. > > It's a GMock matcher that uses protobuffer reflection. It checks that the > protobuffer it's matched against is the correct type and then compares each > field in the protobuffer to the expected value, recursing into submessages and > respecting field types (i.e. int32 fields get numerically compared, string > fields get checked for string equality) and protobuffer default values. > Finally, it's very smart about error reporting so that you can write > EXPECT_THAT(some_proto, EqualsProto(" some_repeated_field: 1" > " some_repeated_field: 2")); > and get messages with things like "Missing element 1 of 'some_repeated_field'; > found unexpected field 'some_optional_field' with value "foo"." > > So, sort of like ContainerEq, only better. FWIW, I think EqualsProto would be very nice to add to the Chromium repo. Do you know whether this code is appropriate to open-source? I see that there are a couple of uses of a very simplified EqualsProto already in use in Chromium, which just serialize the protocol buffers as strings and compare the strings. (FWIW, the serialization approach was also what I recommended on the original code review.)
On 2014/08/26 01:52:08, Ilya Sherman wrote: > https://codereview.chromium.org/403323004/diff/20001/components/password_mana... > File > components/password_manager/core/browser/password_syncable_service_unittest.cc > (right): > > https://codereview.chromium.org/403323004/diff/20001/components/password_mana... > components/password_manager/core/browser/password_syncable_service_unittest.cc:43: > void PasswordsEqual(const sync_pb::PasswordSpecificsData& expected_password, > > FWIW, I think EqualsProto would be very nice to add to the Chromium repo. Do > you know whether this code is appropriate to open-source? I see that there are > a couple of uses of a very simplified EqualsProto already in use in Chromium, > which just serialize the protocol buffers as strings and compare the strings. > (FWIW, the serialization approach was also what I recommended on the original > code review.) My guess would be that it's reasonable to open-source, but not immediately ready for such. Minimally, the implementation code will need to be swept for uses of google-specific features of proto (e.g. removal of its support for our reduced-copying string class). From 2009, there's CL/12019385, which implies portability issues that my be relevant to Chromium. Additionally, there's probably some complexity in the dependency graph: Chromium uses the public protobufs, and it uses the public GTest, but do either of those currently depend on the other? If not, it may be necessary to bundle EqualsProto as some third library. On the other hand, maybe that's a worthwhile thing to make; it makes sense to make EqualsProto available to anybody who is already using both libraries.
https://codereview.chromium.org/403323004/diff/40001/components/password_mana... File components/password_manager/core/browser/password_syncable_service_unittest.cc (right): https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:74: PasswordsEqual( On 2014/08/26 01:46:03, manthd wrote: > On 2014/08/20 20:11:42, vasilii wrote: > > On 2014/08/20 03:30:47, manthd wrote: > > > Semantically, this doesn't really belong here. Deciding which form to > operate > > > on is the job of the matcher, deciding what to do with it is the Action's > job. > > > > > > It would be better to remove the 'form' parameter from this action entirely > > and > > > make the matcher tighter. At minimum, that would allow your EXPECT_CALLs > > below > > > to not repeat the form both as an arg to the matcher and as an arg to the > > > action. > > > > > > That does mean that this action should be renamed. Maybe to something like > > > MakePasswordList. > > > > > > Making the matcher stricter is easy enough if you don't mind not having a > > > field-by-field diff: > > > MATCHER_P(PasswordIs, form, "") { > > > sync_pb::PasswordSpecificsData expected_password = > > > > > > GetPasswordSpecifics(PasswordSyncableService::SyncDataFromPassword(form)); > > > sync_pb::PasswordSpecificsData actual_password = > > > ...(arg); > > > if (expected_password.scheme() == actual_password.scheme() && > > > ... && ... && ...) { > > > return true; > > > } > > > *result_listener << "Password protobuffer for form does not match; > > > expected:\n" > > > << expected_password.DebugString() > > > << "\nactual:\n" > > > << actual_password.DebugString(); > > > return false; > > > } > > > > I wonder what the error message will be. If this unique key matches I can > > compare some other fields and print meaningful message if for example > > EXPECT_EQ(expected_password.scheme(), actual_password.scheme()); fails. If I > put > > everything to the matcher then I'll get "Uninteresting mock call" and 100 > fields > > of expected and actual passwords. > > It will print something like "Unexpected call AddLoginImpl(blah blah)". The > "blah blah" is the full argument, if GTest knows how to print its type, but it > will also then print something for each possible EXPECT_CALL and explain why > that EXPECT_CALL didn't match. The non-matching can be made more verbose by > using MATCHER's 'result_listener' implicit arg. Whether this is worth providing > for really depends on the expected failure modes. > > This depends on whether the types are printable by GTest. PasswordForm has operator<<. https://codereview.chromium.org/403323004/diff/40001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:261: EXPECT_CALL(*service(), NotifyPasswordStoreOfLoginChanges(_)); On 2014/08/26 01:46:03, manthd wrote: > On 2014/08/22 12:27:15, vasilii wrote: > > On 2014/08/22 05:49:47, manthd wrote: > > > On 2014/08/20 20:11:42, vasilii wrote: > > > > On 2014/08/20 03:30:48, manthd wrote: > > > > > I don't think this mock method is doing anything at all for the tests; > > it's > > > > just > > > > > noise. > > > > > > > > But it's important that the PasswordStore is notified. Also we use strict > > > mocks > > > > for everything, so the test will fail otherwise. > > > > > > The reason it doesn't seem important is that you're already examining all of > > the > > > objects being passed to NotifyLoginsChanged. > > > > > > Yes, of course it's important that your class actually calls > > > NotifyLoginsChanged(), but is it really important to use a mock function to > > > confirm that it happens? If there's any sort of end-to-end testing that > uses > > > your class, then you don't really need to test it here. > > > > > > If you stop using a StrictMock, the easiest resolution is actually to remove > > all > > > the EXPECT_CALLs for FooLoginImpl's and instead just have > > > EXPECT_CALL(*processor, ProcessSyncChanges(_, ElementsAre( > > > SyncChangeIs(SyncChange::ACTION_ADD, form)))); > > > EXPECT_CALL(*password_store(), NotifyLoginsChanged(UnsortedElementsAre( > > > PasswordChangeIs(PasswordStoreChange::ADD, list.back())))); > > > > FooLoginImpl are even more important than NotifyLoginsChanged. I don't think > we > > test somewhere else that NotifyLoginsChanged is called. UI depends on this > > notification. > > Sure, both need to happen, but that doesn't mean that both need to have explicit > expectations. > > If you only checked that the expected changes showed up in > NotifyLoginsChanged(), then the only way of meeting that expectation would be to > call FooLoginImpl with the correct values, at least assuming that there's an > obvious mapping from the Impl calls to the notification, and you know that's a > good assumption because you're using a password_store that you've specifically > told to have that obvious mapping. > > This falls under the category of "don't repeat yourself"; one of the two kinds > of expectations is strictly less interesting then the other, so just skip them. I removed NotifyLoginsChanged but left FooLoginImpl. I foresee that we may need more complex logic for merging in PasswordSyncableService. So we shouldn't avoid checking which particular FooLoginImpl methods are called. https://codereview.chromium.org/403323004/diff/80001/components/password_mana... File components/password_manager/core/browser/password_syncable_service_unittest.cc (right): https://codereview.chromium.org/403323004/diff/80001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:48: if (check_date_synced) { On 2014/08/26 01:46:03, manthd wrote: > This is a bad idea for the simple fact that the call sites of the matcher now > have bear "true" or "false" in their argument list. It's completely opaque what > that means at the call site. "PasswordIs(form1)" pretty clearly means "matches > a given password form", but "PasswordIs(form1, false)" doesn't say anything > about what the "false" might mean. > > Remove this parameter entirely and find another way to do the same enforcement. > > Unless I've lost track, you want this to be enforced on every call to > AddLoginImpl or UpdateLoginImpl and in no other places. You've already got a > good place to deal with all calls to these, near line 153 in this snapshot. > > You can just do > MATCHER(HasDateSynced, "") { > ...arg.date_synced... > } > and then changed the default actions so that > ON_CALL(*password_store_, AddLoginImpl(HasDateSynced())) > .WillByDefault(Return(PasswordStoreChangeList())); > and same for updates. > > Then, if one of these functions were called with a bad PasswordForm, GMock would > report that the call was expected but that it didn't know what to do with the > call and therefore (1) failed, and (2) continued the test by returning the > default-constructed PasswordStoreChangeList, which will also break any > expectation you have on the set of changes, making clear what went wrong. Done. https://codereview.chromium.org/403323004/diff/80001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:150: password_store_ = new testing::StrictMock<MockPasswordStore>; On 2014/08/26 01:46:03, manthd wrote: > i'm still not convinced that this needs to be strict. I really don't think > there's any value in having particular expectations of > {Add,Remove,Update}LoginImpl. > > But do note that if you make this not-strict the suggestion I give above about > HasDataSynced needs to be tweaked by changing these to > EXPECT_CALL(..., FooLoginImpl(HasLoginImpl)) > .WillRepeatedly(...); > so that GMock doesn't simply turn all calls to these into warning logs, > regardless of whether the args have data_synced. I think the code above is still valid for NiceMock. If HasDateSynced returns false then the error is "The mock function has no default action set, and its return type has no default value set." regardless of it's type. https://codereview.chromium.org/403323004/diff/80001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:186: .WillByDefault(Return(SyncError())); On 2014/08/26 01:46:03, manthd wrote: > Could you show me the error that results from just commenting out this ON_CALL? > I'm curious why GMock would have trouble with just default-constructing the > return value. "The mock function has no default action set, and its return type has no default value set." According to this https://code.google.com/p/googlemock/wiki/CookBook#Setting_the_Default_Value_... GMock default constructs only built-in types. https://codereview.chromium.org/403323004/diff/80001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:196: On 2014/08/26 01:46:03, manthd wrote: > nix Done. https://codereview.chromium.org/403323004/diff/80001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:408: std::vector<autofill::PasswordForm> actual_form_list; On 2014/08/26 01:46:03, manthd wrote: > Sorry, this is one of the spots that I meant to comment further on. > > It's much better to keep the body of the tests nice and simple so that the > EXPECT calls can be read as pretty much the whole description of what's being > enforced. So, it would be really nice if you could write what I suggested in my > last comment. > > GMock matchers are by default "polymorphic" (a term GMock has given a particular > meaning to, though it does fit with the more general use of the term). For > example the matcher Gt(4) is constructed from an int, but will happily match the > double 4.1, even though 4.1 clearly narrows to 4, which would not match. > > You have the option of making PasswordIs polymorphic in this sense that it can > match a SyncData or a PasswordForm. In fact, it already semantically really > does operate on both; PasswordIs is constructed with a PasswordForm, and it > matches a PasswordForm, but it then converts both of those to SyncData and then > to PasswordSpecificData. > > What you really want to be able to say inside of PasswordIs is something like > sync_pb::PasswordSpecificsData actual_password = > GetPasswordSpecifics(somehow_get_a_SyncData(arg)); > sync_pb::PasswordSpecificsData expected_password = > GetPasswordSpecifics(somehow_get_a_SyncData(form)); > ...use the two specific data objects... > This is easy to do with an overloaded helper function > const syncer::SyncData& GetSyncData(const syncer::SyncData& sync_data) { > return sync_data; > } > syncer::SyncData GetSyncData(const autofill::PasswordForm& password) { > return PasswordSyncableService::SyncDataFromPassword(password); > } > > The way this works is that the object defined by the function that MATCHER_P > produces is convertible to *any* kind of matcher (which uses the body attached > to the MATCHER_P, and thus which is only valid for some possible matcher > conversions). In particular the 'arg' implicit parameter is a different type > depending on where you use the matcher. > > Thus, if you write just > EXPECT_THAT(actual_list, ElementsAre(PasswordIs(form1))); > then, EXPECT_THAT sees that 'actual_list' is SyncDataList, and asks ElementsAre > to create a Matcher<SyncDataList>, which then asks what type the elements are > and thus asks PasswordIs to become a matcher of SyncData. That's then the type > of 'arg' (actually, it's going to be 'const SyncData&'), which means that it can > be passed to the first overload of GetSyncData, and thus it does nothing except > pass the SyncData back out. The problem is with syncer::SyncData used as a parameter to the matcher. The linkage error is 'error: undefined reference to 'syncer::PrintTo(syncer::SyncData const&, std::ostream*)'. SyncData has no operator<<.
lgtm LGTM. Feel free to submit when Ilya gives you the OK. I'll then mark your readability bug fixed, and assuming that it works with Chromium, a bot will give you readability shortly after. https://codereview.chromium.org/403323004/diff/80001/components/password_mana... File components/password_manager/core/browser/password_syncable_service_unittest.cc (right): https://codereview.chromium.org/403323004/diff/80001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:186: .WillByDefault(Return(SyncError())); On 2014/08/26 11:45:46, vasilii wrote: > On 2014/08/26 01:46:03, manthd wrote: > > Could you show me the error that results from just commenting out this > ON_CALL? > > I'm curious why GMock would have trouble with just default-constructing the > > return value. > > "The mock function has no default action set, and its return type has no default > value set." > According to this > https://code.google.com/p/googlemock/wiki/CookBook#Setting_the_Default_Value_... > GMock default constructs only built-in types. Thanks. I had thought that the default c'tor was the default default value, but it makes sense for that to be explicit. https://codereview.chromium.org/403323004/diff/80001/components/password_mana... components/password_manager/core/browser/password_syncable_service_unittest.cc:408: std::vector<autofill::PasswordForm> actual_form_list; On 2014/08/26 11:45:46, vasilii wrote: > On 2014/08/26 01:46:03, manthd wrote: > > Sorry, this is one of the spots that I meant to comment further on. > > > > It's much better to keep the body of the tests nice and simple so that the > > EXPECT calls can be read as pretty much the whole description of what's being > > enforced. So, it would be really nice if you could write what I suggested in > my > > last comment. > > > > GMock matchers are by default "polymorphic" (a term GMock has given a > particular > > meaning to, though it does fit with the more general use of the term). For > > example the matcher Gt(4) is constructed from an int, but will happily match > the > > double 4.1, even though 4.1 clearly narrows to 4, which would not match. > > > > You have the option of making PasswordIs polymorphic in this sense that it can > > match a SyncData or a PasswordForm. In fact, it already semantically really > > does operate on both; PasswordIs is constructed with a PasswordForm, and it > > matches a PasswordForm, but it then converts both of those to SyncData and > then > > to PasswordSpecificData. > > > > What you really want to be able to say inside of PasswordIs is something like > > sync_pb::PasswordSpecificsData actual_password = > > GetPasswordSpecifics(somehow_get_a_SyncData(arg)); > > sync_pb::PasswordSpecificsData expected_password = > > GetPasswordSpecifics(somehow_get_a_SyncData(form)); > > ...use the two specific data objects... > > This is easy to do with an overloaded helper function > > const syncer::SyncData& GetSyncData(const syncer::SyncData& sync_data) { > > return sync_data; > > } > > syncer::SyncData GetSyncData(const autofill::PasswordForm& password) { > > return PasswordSyncableService::SyncDataFromPassword(password); > > } > > > > The way this works is that the object defined by the function that MATCHER_P > > produces is convertible to *any* kind of matcher (which uses the body attached > > to the MATCHER_P, and thus which is only valid for some possible matcher > > conversions). In particular the 'arg' implicit parameter is a different type > > depending on where you use the matcher. > > > > Thus, if you write just > > EXPECT_THAT(actual_list, ElementsAre(PasswordIs(form1))); > > then, EXPECT_THAT sees that 'actual_list' is SyncDataList, and asks > ElementsAre > > to create a Matcher<SyncDataList>, which then asks what type the elements are > > and thus asks PasswordIs to become a matcher of SyncData. That's then the > type > > of 'arg' (actually, it's going to be 'const SyncData&'), which means that it > can > > be passed to the first overload of GetSyncData, and thus it does nothing > except > > pass the SyncData back out. > > The problem is with syncer::SyncData used as a parameter to the matcher. The > linkage error is 'error: undefined reference to > 'syncer::PrintTo(syncer::SyncData const&, std::ostream*)'. SyncData has no > operator<<. I see. The internal GTest happily prints something about the type not being printable. It's probably not worth the effort to work around this.
Thanks! Ilya, please review.
IMO most Chromium developers would balk at this heavy use of Gmock. However, I agree that this code is much cleaner than the previous code, so I think it's okay to land these changes. https://codereview.chromium.org/403323004/diff/160001/components/password_man... File components/password_manager/core/browser/password_syncable_service.cc (right): https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service.cc:22: // memberwise. nit: "Returns true iff |password_specifics| and |password_specifics| are equal memberwise." https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service.cc:53: case PasswordStoreChange::REMOVE: return syncer::SyncChange::ACTION_DELETE; Hmm, is this a "git cl format" change? I haven't seen this style used anywhere else in the Chromium repository. https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service.cc:59: void AppendPasswordFromSpecifics( Please document this function. https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service.cc:61: base::Time sync_time, nit: Please pass by const-reference. https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service.cc:71: typedef ScopedVector<autofill::PasswordForm> EntryList; I'm not thrilled about typedef'ing a ScopedVector in a way that looks the same as the typedef for an std::vector would be. Is it really painful to spell out the typename instead of using this typedef? https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service.cc:87: // List that contains the entries that are known to both sync and db but "db" -> "the local database" https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service.cc:88: // have updates in sync. They need to be updated in the passwords db. nit: "the passwords db" -> "the local database" https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service.cc:91: // The list of entries to be deleted from the local db. nit: "the local db" -> "the local database" https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service.cc:399: password_form.date_created) { nit: Please indent four more spaces. https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service.cc:416: void PasswordSyncableService::AppendChanges( nit: This method name is no longer quite complete, since it both modifies the local database and appends to |changes|. Perhaps "WriteEntriesToDatabase()" or something like that would be clearer? https://codereview.chromium.org/403323004/diff/160001/components/password_man... File components/password_manager/core/browser/password_syncable_service.h (right): https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service.h:37: // The implementation of Syncable Service API for the passwords. nit: "The implementation of the SyncableService API for passwords." https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service.h:42: // constructor doesn't take the ownership of |password_store|. nit: "Since the constructed |PasswordSyncableService| is typically owned by the |password_store|, the constructor doesn't take ownership of the |password_store|." https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service.h:73: // Extracts the |PasswordForm| data from sync's protobuffer format. nit: "protobuffer" -> "protobuf" or "protocol buffer" https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service.h:82: const autofill::PasswordForm& password); I would prefer that if these are only intended to be used in the test file, that they not be exposed in the header file. Instead, you can forward-declare them in the test file itself. For an example of this, see https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil... https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service.h:95: struct SyncEntries; nit: I believe that, per the style guide, this belongs above, with the typedefs. https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service.h:95: struct SyncEntries; Why does this need to be an inner struct? Can it be a free struct instead? https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service.h:100: // Checks |data|, an entry in sync db, and updates |sync_entries| or nit: Checks |data| for what? https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service.h:113: const autofill::PasswordForm& form), Optional nit: This is a case where I think a typedef could improve readability. https://codereview.chromium.org/403323004/diff/160001/components/password_man... File components/password_manager/core/browser/password_syncable_service_unittest.cc (right): https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service_unittest.cc:72: return true; nit: Please leave a blank line after this. I think given how long the condition is, it's probably a good idea to add curly braces as well. https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service_unittest.cc:73: *result_listener << "Password protobuffer does not match; expected:\n" nit: "protobuffer" -> "protocol buffer" or "protobuf" https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service_unittest.cc:75: << "\nactual:\n" nit: I'd prefer that you write these lines as << form << "\n" << "actual:\n" as that's a little easier to read, IMO. https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service_unittest.cc:92: arg0->push_back(new autofill::PasswordForm(form)); This looks like a memory leak. Please at least document this code to explain why it isn't (if it isn't). https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service_unittest.cc:125: }; Is this class intentionally copyable? https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service_unittest.cc:136: NOTREACHED(); Why do you have NOTREACHED() in test code? Can you just not override this method instead? https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service_unittest.cc:144: // The wrapper around a pair of PasswordSyncableService and PasswordStore. nit: "Convenience wrapper around a PasswordSyncableService and PasswordStore pair." https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service_unittest.cc:164: MockPasswordStore* password_store() { return password_store_; } nit: Please keep the .get() here. The code without .get() currently compiles, but soon will not. https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service_unittest.cc:205: .WillOnce(Return(true)); What code triggers these expectations? It seems to me like you're defining them too early. (Applies throughout.) https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service_unittest.cc:314: MockSyncChangeProcessor& weak_processor = *processor_; Please add a comment explaining why this is needed. https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service_unittest.cc:398: for (SyncDataList::iterator i = actual_list.begin(); nit: Please use "it" or "iter" for iterators.
https://codereview.chromium.org/403323004/diff/160001/components/password_man... File components/password_manager/core/browser/password_syncable_service.cc (right): https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service.cc:22: // memberwise. On 2014/08/28 06:59:37, Ilya Sherman wrote: > nit: "Returns true iff |password_specifics| and |password_specifics| are equal > memberwise." Done. https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service.cc:53: case PasswordStoreChange::REMOVE: return syncer::SyncChange::ACTION_DELETE; On 2014/08/28 06:59:38, Ilya Sherman wrote: > Hmm, is this a "git cl format" change? I haven't seen this style used anywhere > else in the Chromium repository. No, it's a reviewer's comment https://codereview.chromium.org/403323004/diff/40001/components/password_mana... https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service.cc:59: void AppendPasswordFromSpecifics( On 2014/08/28 06:59:38, Ilya Sherman wrote: > Please document this function. Done. https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service.cc:61: base::Time sync_time, On 2014/08/28 06:59:38, Ilya Sherman wrote: > nit: Please pass by const-reference. Why? It's just int64, simple types are passed by value. https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service.cc:71: typedef ScopedVector<autofill::PasswordForm> EntryList; On 2014/08/28 06:59:37, Ilya Sherman wrote: > I'm not thrilled about typedef'ing a ScopedVector in a way that looks the same > as the typedef for an std::vector would be. Is it really painful to spell out > the typename instead of using this typedef? Done. https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service.cc:87: // List that contains the entries that are known to both sync and db but On 2014/08/28 06:59:38, Ilya Sherman wrote: > "db" -> "the local database" Done. https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service.cc:88: // have updates in sync. They need to be updated in the passwords db. On 2014/08/28 06:59:37, Ilya Sherman wrote: > nit: "the passwords db" -> "the local database" Done. https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service.cc:91: // The list of entries to be deleted from the local db. On 2014/08/28 06:59:38, Ilya Sherman wrote: > nit: "the local db" -> "the local database" Done. https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service.cc:399: password_form.date_created) { On 2014/08/28 06:59:37, Ilya Sherman wrote: > nit: Please indent four more spaces. Done. https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service.cc:416: void PasswordSyncableService::AppendChanges( On 2014/08/28 06:59:38, Ilya Sherman wrote: > nit: This method name is no longer quite complete, since it both modifies the > local database and appends to |changes|. Perhaps "WriteEntriesToDatabase()" or > something like that would be clearer? Done. https://codereview.chromium.org/403323004/diff/160001/components/password_man... File components/password_manager/core/browser/password_syncable_service.h (right): https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service.h:37: // The implementation of Syncable Service API for the passwords. On 2014/08/28 06:59:38, Ilya Sherman wrote: > nit: "The implementation of the SyncableService API for passwords." Done. https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service.h:42: // constructor doesn't take the ownership of |password_store|. On 2014/08/28 06:59:38, Ilya Sherman wrote: > nit: "Since the constructed |PasswordSyncableService| is typically owned by the > |password_store|, the constructor doesn't take ownership of the > |password_store|." Done. https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service.h:73: // Extracts the |PasswordForm| data from sync's protobuffer format. On 2014/08/28 06:59:38, Ilya Sherman wrote: > nit: "protobuffer" -> "protobuf" or "protocol buffer" Done. https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service.h:82: const autofill::PasswordForm& password); On 2014/08/28 06:59:38, Ilya Sherman wrote: > I would prefer that if these are only intended to be used in the test file, that > they not be exposed in the header file. Instead, you can forward-declare them > in the test file itself. For an example of this, see > https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil... Done. https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service.h:95: struct SyncEntries; On 2014/08/28 06:59:38, Ilya Sherman wrote: > Why does this need to be an inner struct? Can it be a free struct instead? It's used as an argument to CreateOrUpdateEntry and WriteToPasswordStore. https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service.h:95: struct SyncEntries; On 2014/08/28 06:59:38, Ilya Sherman wrote: > nit: I believe that, per the style guide, this belongs above, with the typedefs. Done. https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service.h:100: // Checks |data|, an entry in sync db, and updates |sync_entries| or On 2014/08/28 06:59:38, Ilya Sherman wrote: > nit: Checks |data| for what? Done. https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service.h:113: const autofill::PasswordForm& form), On 2014/08/28 06:59:38, Ilya Sherman wrote: > Optional nit: This is a case where I think a typedef could improve readability. Done. https://codereview.chromium.org/403323004/diff/160001/components/password_man... File components/password_manager/core/browser/password_syncable_service_unittest.cc (right): https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service_unittest.cc:72: return true; On 2014/08/28 06:59:38, Ilya Sherman wrote: > nit: Please leave a blank line after this. I think given how long the condition > is, it's probably a good idea to add curly braces as well. Done. https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service_unittest.cc:73: *result_listener << "Password protobuffer does not match; expected:\n" On 2014/08/28 06:59:39, Ilya Sherman wrote: > nit: "protobuffer" -> "protocol buffer" or "protobuf" Done. https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service_unittest.cc:75: << "\nactual:\n" On 2014/08/28 06:59:39, Ilya Sherman wrote: > nit: I'd prefer that you write these lines as > > << form << "\n" > << "actual:\n" > > as that's a little easier to read, IMO. Done. https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service_unittest.cc:92: arg0->push_back(new autofill::PasswordForm(form)); On 2014/08/28 06:59:38, Ilya Sherman wrote: > This looks like a memory leak. Please at least document this code to explain > why it isn't (if it isn't). Done. https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service_unittest.cc:125: }; On 2014/08/28 06:59:38, Ilya Sherman wrote: > Is this class intentionally copyable? It's not copyable because the parent isn't copyable. https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service_unittest.cc:136: NOTREACHED(); On 2014/08/28 06:59:39, Ilya Sherman wrote: > Why do you have NOTREACHED() in test code? Can you just not override this > method instead? It's pure virtual. https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service_unittest.cc:144: // The wrapper around a pair of PasswordSyncableService and PasswordStore. On 2014/08/28 06:59:38, Ilya Sherman wrote: > nit: "Convenience wrapper around a PasswordSyncableService and PasswordStore > pair." Done. https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service_unittest.cc:164: MockPasswordStore* password_store() { return password_store_; } On 2014/08/28 06:59:39, Ilya Sherman wrote: > nit: Please keep the .get() here. The code without .get() currently compiles, > but soon will not. Done. https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service_unittest.cc:205: .WillOnce(Return(true)); On 2014/08/28 06:59:38, Ilya Sherman wrote: > What code triggers these expectations? It seems to me like you're defining them > too early. (Applies throughout.) MergeDataAndStartSyncing(). All the code before is just setting the expectations. This expectation just happens to be first. Actually it'll be the first one so if we want to use a sequence we can do that. https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service_unittest.cc:314: MockSyncChangeProcessor& weak_processor = *processor_; On 2014/08/28 06:59:38, Ilya Sherman wrote: > Please add a comment explaining why this is needed. Done. https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service_unittest.cc:398: for (SyncDataList::iterator i = actual_list.begin(); On 2014/08/28 06:59:39, Ilya Sherman wrote: > nit: Please use "it" or "iter" for iterators. Done.
https://codereview.chromium.org/403323004/diff/160001/components/password_man... File components/password_manager/core/browser/password_syncable_service.h (right): https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service.h:95: struct SyncEntries; On 2014/08/28 10:45:20, vasilii wrote: > On 2014/08/28 06:59:38, Ilya Sherman wrote: > > Why does this need to be an inner struct? Can it be a free struct instead? > > It's used as an argument to CreateOrUpdateEntry and WriteToPasswordStore. Why does that require it to be an inner struct, i.e. defined within the scope of the PasswordSyncableService class? https://codereview.chromium.org/403323004/diff/160001/components/password_man... File components/password_manager/core/browser/password_syncable_service_unittest.cc (right): https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service_unittest.cc:125: }; On 2014/08/28 10:45:21, vasilii wrote: > On 2014/08/28 06:59:38, Ilya Sherman wrote: > > Is this class intentionally copyable? > > It's not copyable because the parent isn't copyable. That can be dangerous to rely on, as parent classes can be updated without checking each of their children. Please add DISALLOW_COPY_AND_ASSIGN for this class. https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service_unittest.cc:205: .WillOnce(Return(true)); On 2014/08/28 10:45:21, vasilii wrote: > On 2014/08/28 06:59:38, Ilya Sherman wrote: > > What code triggers these expectations? It seems to me like you're defining > them > > too early. (Applies throughout.) > > MergeDataAndStartSyncing(). All the code before is just setting the > expectations. This expectation just happens to be first. Actually it'll be the > first one so if we want to use a sequence we can do that. I believe that all of the EXPECT_CALLs should be grouped together just before the function invocation that will trigger them. The only reason that EXPECT_CALLs might be separated from one another is if there is one set of expectations for one function invocation, and another set of expectations for a different function invocation. https://codereview.chromium.org/403323004/diff/180001/components/password_man... File components/password_manager/core/browser/password_syncable_service.h (right): https://codereview.chromium.org/403323004/diff/180001/components/password_man... components/password_manager/core/browser/password_syncable_service.h:101: SmthLoginImpl modify, Smth? How about "DatabaseOperation operation", or something like that? https://codereview.chromium.org/403323004/diff/180001/components/password_man... File components/password_manager/core/browser/password_syncable_service_unittest.cc (right): https://codereview.chromium.org/403323004/diff/180001/components/password_man... components/password_manager/core/browser/password_syncable_service_unittest.cc:48: // |password| entry. nit: You don't need these comments here, in addition to including them in the implementation file. We put comments in header files by forward-declarations because that's where the API for a class is defined. However, these functions are not part of the API of the class; they're just helper functions that you needed to expose for testing. Their API is really defined in the implementation file, and hence should be documented there.
lgtm https://codereview.chromium.org/403323004/diff/160001/components/password_man... File components/password_manager/core/browser/password_syncable_service.cc (right): https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service.cc:53: case PasswordStoreChange::REMOVE: return syncer::SyncChange::ACTION_DELETE; On 2014/08/28 10:45:20, vasilii wrote: > On 2014/08/28 06:59:38, Ilya Sherman wrote: > > Hmm, is this a "git cl format" change? I haven't seen this style used > anywhere > > else in the Chromium repository. > > No, it's a reviewer's comment > https://codereview.chromium.org/403323004/diff/40001/components/password_mana... If this is really not done elsewhere in Chromium, feel free to revert it; but in the internal codebase it would be allowed and I think it helps in this case. https://codereview.chromium.org/403323004/diff/160001/components/password_man... File components/password_manager/core/browser/password_syncable_service.h (right): https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service.h:82: const autofill::PasswordForm& password); On 2014/08/28 06:59:38, Ilya Sherman wrote: > I would prefer that if these are only intended to be used in the test file, that > they not be exposed in the header file. Instead, you can forward-declare them > in the test file itself. For an example of this, see > https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil... FYI, My own thinking has always been that if the testing-only functionality being exposed cannot be used to changes state, then it's sufficiently protected to be documented as testing-only. https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service.h:95: struct SyncEntries; On 2014/08/29 20:32:02, Ilya Sherman wrote: > On 2014/08/28 10:45:20, vasilii wrote: > > On 2014/08/28 06:59:38, Ilya Sherman wrote: > > > Why does this need to be an inner struct? Can it be a free struct instead? > > > > It's used as an argument to CreateOrUpdateEntry and WriteToPasswordStore. > > Why does that require it to be an inner struct, i.e. defined within the scope of > the PasswordSyncableService class? It could be, but why would that be better? It has no meaning outside this class, and it would need a more verbose name if moved outside the scope of the class.
https://codereview.chromium.org/403323004/diff/160001/components/password_man... File components/password_manager/core/browser/password_syncable_service.h (right): https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service.h:95: struct SyncEntries; On 2014/08/29 21:06:03, manthd wrote: > On 2014/08/29 20:32:02, Ilya Sherman wrote: > > On 2014/08/28 10:45:20, vasilii wrote: > > > On 2014/08/28 06:59:38, Ilya Sherman wrote: > > > > Why does this need to be an inner struct? Can it be a free struct > instead? > > > > > > It's used as an argument to CreateOrUpdateEntry and WriteToPasswordStore. > > > > Why does that require it to be an inner struct, i.e. defined within the scope > of > > the PasswordSyncableService class? > > It could be, but why would that be better? It has no meaning outside this > class, and it would need a more verbose name if moved outside the scope of the > class. Mostly I'm not a fan of inner classes because they have access to all of the private state of the class. I guess it doesn't matter a ton in this case, given that the inner class is very simple.
https://codereview.chromium.org/403323004/diff/160001/components/password_man... File components/password_manager/core/browser/password_syncable_service.h (right): https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service.h:82: const autofill::PasswordForm& password); On 2014/08/29 21:06:03, manthd wrote: > On 2014/08/28 06:59:38, Ilya Sherman wrote: > > I would prefer that if these are only intended to be used in the test file, > that > > they not be exposed in the header file. Instead, you can forward-declare them > > in the test file itself. For an example of this, see > > > https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil... > > FYI, My own thinking has always been that if the testing-only functionality > being exposed cannot be used to changes state, then it's sufficiently protected > to be documented as testing-only. I think the reasoning was that we don't want to pollute the header. https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service.h:95: struct SyncEntries; On 2014/08/29 21:26:07, Ilya Sherman wrote: > On 2014/08/29 21:06:03, manthd wrote: > > On 2014/08/29 20:32:02, Ilya Sherman wrote: > > > On 2014/08/28 10:45:20, vasilii wrote: > > > > On 2014/08/28 06:59:38, Ilya Sherman wrote: > > > > > Why does this need to be an inner struct? Can it be a free struct > > instead? > > > > > > > > It's used as an argument to CreateOrUpdateEntry and WriteToPasswordStore. > > > > > > Why does that require it to be an inner struct, i.e. defined within the > scope > > of > > > the PasswordSyncableService class? > > > > It could be, but why would that be better? It has no meaning outside this > > class, and it would need a more verbose name if moved outside the scope of the > > class. > > Mostly I'm not a fan of inner classes because they have access to all of the > private state of the class. I guess it doesn't matter a ton in this case, given > that the inner class is very simple. I agree with Dan. The struct is a private implementation of the class. https://codereview.chromium.org/403323004/diff/160001/components/password_man... File components/password_manager/core/browser/password_syncable_service_unittest.cc (right): https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service_unittest.cc:125: }; On 2014/08/29 20:32:02, Ilya Sherman wrote: > On 2014/08/28 10:45:21, vasilii wrote: > > On 2014/08/28 06:59:38, Ilya Sherman wrote: > > > Is this class intentionally copyable? > > > > It's not copyable because the parent isn't copyable. > > That can be dangerous to rely on, as parent classes can be updated without > checking each of their children. Please add DISALLOW_COPY_AND_ASSIGN for this > class. Done. https://codereview.chromium.org/403323004/diff/160001/components/password_man... components/password_manager/core/browser/password_syncable_service_unittest.cc:205: .WillOnce(Return(true)); On 2014/08/29 20:32:02, Ilya Sherman wrote: > On 2014/08/28 10:45:21, vasilii wrote: > > On 2014/08/28 06:59:38, Ilya Sherman wrote: > > > What code triggers these expectations? It seems to me like you're defining > > them > > > too early. (Applies throughout.) > > > > MergeDataAndStartSyncing(). All the code before is just setting the > > expectations. This expectation just happens to be first. Actually it'll be the > > first one so if we want to use a sequence we can do that. > > I believe that all of the EXPECT_CALLs should be grouped together just before > the function invocation that will trigger them. The only reason that > EXPECT_CALLs might be separated from one another is if there is one set of > expectations for one function invocation, and another set of expectations for a > different function invocation. Done. https://codereview.chromium.org/403323004/diff/180001/components/password_man... File components/password_manager/core/browser/password_syncable_service.h (right): https://codereview.chromium.org/403323004/diff/180001/components/password_man... components/password_manager/core/browser/password_syncable_service.h:101: SmthLoginImpl modify, On 2014/08/29 20:32:02, Ilya Sherman wrote: > Smth? How about "DatabaseOperation operation", or something like that? Done. https://codereview.chromium.org/403323004/diff/180001/components/password_man... File components/password_manager/core/browser/password_syncable_service_unittest.cc (right): https://codereview.chromium.org/403323004/diff/180001/components/password_man... components/password_manager/core/browser/password_syncable_service_unittest.cc:48: // |password| entry. On 2014/08/29 20:32:02, Ilya Sherman wrote: > nit: You don't need these comments here, in addition to including them in the > implementation file. We put comments in header files by forward-declarations > because that's where the API for a class is defined. However, these functions > are not part of the API of the class; they're just helper functions that you > needed to expose for testing. Their API is really defined in the implementation > file, and hence should be documented there. Done.
lgtm FYI, I'm going on vacation for a week. As far as I'm concerned this review is good, so it's up to Ilya to let you submit. I'd normally wait until you submit before I mark your bug fixed, but I don't want to delay you while I'm not online, so I'm marking in fixed now. If the bot understands Chromium as well as internal reviews, then it'll wait until you submit and then give you your readability bit. If that messes up, don't hesitate to poke omoikane@
LGTM % a final nit. Thanks, Vasilii! https://codereview.chromium.org/403323004/diff/200001/components/password_man... File components/password_manager/core/browser/password_syncable_service_unittest.cc (right): https://codereview.chromium.org/403323004/diff/200001/components/password_man... components/password_manager/core/browser/password_syncable_service_unittest.cc:39: nit: I'd omit this blank line, as well as the ones on lines 41 and 44, to make it clearer that the comment above applies to all four of these forward-declared methods.
https://codereview.chromium.org/403323004/diff/200001/components/password_man... File components/password_manager/core/browser/password_syncable_service_unittest.cc (right): https://codereview.chromium.org/403323004/diff/200001/components/password_man... components/password_manager/core/browser/password_syncable_service_unittest.cc:39: On 2014/09/02 20:34:18, Ilya Sherman wrote: > nit: I'd omit this blank line, as well as the ones on lines 41 and 44, to make > it clearer that the comment above applies to all four of these forward-declared > methods. Done.
The CQ bit was checked by vasilii@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vasilii@chromium.org/403323004/220001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as 539e57dcba9eef9368e84e31b879458c3bcacccc
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/9db0a1d04fb44ff57833f94d1c68e50d062c1dc9 Cr-Commit-Position: refs/heads/master@{#293107} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
