Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
216183008:
Password manager internals page: Introduce logger in browser (Closed)
|
Created:
6 years, 9 months ago by vabr (Chromium) Modified:
6 years, 8 months ago CC:
chromium-reviews, benquan, browser-components-watch_chromium.org, Dane Wallinga, dyu1, estade+watch_chromium.org, Ilya Sherman, rouslan+autofillwatch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionPassword manager internals page: Introduce logger in browser
Password manager internals page serves as a debugging output from the process of observing submitted password forms and offering the user to save the password. The user will be able to grab the debugging info and pass it onto the Chrome developers to help investigate issues.
This CL introduces:
1) SavePasswordProgressLogger, a collection of methods to easily format logs and scrub privacy sensitive information out of them.
2) BrowserSavePasswordProgressLogger, a specialization of the Logger for the browser part of the password management code.
More context in the design doc: https://docs.google.com/document/d/1ArDhTo0w-8tOPiTwqM1gG6ZGqODo8UTpXlJjjnCNK4s/edit?usp=sharing
Follow-up CLs will introduce the renderer specialization of the Logger, and actual logging calls.
BUG=347927
TBR=blundell@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=262671
Patch Set 1 : #
Total comments: 18
Patch Set 2 : Rebased #Patch Set 3 : Comments addressed #
Total comments: 53
Patch Set 4 : Further comments addressed #
Total comments: 40
Patch Set 5 : Rebased #Patch Set 6 : Further comments addressed #
Total comments: 12
Patch Set 7 : Rebased #Patch Set 8 : Comments addressed, Monitor renamed #
Total comments: 4
Patch Set 9 : Rebased #Patch Set 10 : Final comments addressed #Messages
Total messages: 26 (0 generated)
Hi Ilya, Could you please take a look? This is the other end of the internals page logging. What is missing yet, is using the new classes in the production code, but that would make this CL too big. (If you have a glimpse at how I plan to use them, you can check that in CL https://codereview.chromium.org/216233011/, which is not meant for landing. But you should not need to look there for reviewing this CL.) Thanks, Vaclav
> (If you have a glimpse at how I plan to use them, you can check that in CL I meant "If you want to have a glimpse...".
IMO something much simpler would serve our needs better. Let's not over-engineer this logging code, which will be used relatively infrequently, and will not be on a performance hot path. Let's instead make it very clear and easy to maintain. https://codereview.chromium.org/216183008/diff/20001/components/autofill/core... File components/autofill/core/common/save_password_progress_monitor_helper.h (right): https://codereview.chromium.org/216183008/diff/20001/components/autofill/core... components/autofill/core/common/save_password_progress_monitor_helper.h:31: // the appropriate implementation. This seems really needlessly complicated. There are many simpler options: (1) Don't run the logging code when it's not needed. (2) Use a NULL pointer when logging isn't needed, then do (1). (3) Pass a boolean flag to the monitor class to control whether logging is on or off. etc. https://codereview.chromium.org/216183008/diff/20001/components/autofill/core... components/autofill/core/common/save_password_progress_monitor_helper.h:42: // the logs get diplayed in a wrong order. This seems a little silly. Why not just send the messages as they come in, rather than waiting until the class is destroyed? https://codereview.chromium.org/216183008/diff/20001/components/autofill/core... components/autofill/core/common/save_password_progress_monitor_helper.h:59: typedef std::string SerializableLogType; What's the advantage of obfuscating/renaming std::string? https://codereview.chromium.org/216183008/diff/20001/components/autofill/core... components/autofill/core/common/save_password_progress_monitor_helper.h:65: typedef base::Callback<void(const SerializableLogType&)> LogSender; Rather than using a callback, how about having subclasses of the class that know how to send their own messages? Are there more than two types of callbacks that you anticipate being used? https://codereview.chromium.org/216183008/diff/20001/components/autofill/core... File components/autofill/core/common/save_password_progress_monitor_helper_impl.cc (right): https://codereview.chromium.org/216183008/diff/20001/components/autofill/core... components/autofill/core/common/save_password_progress_monitor_helper_impl.cc:79: } while (false) Why are you using macros for this? https://codereview.chromium.org/216183008/diff/20001/components/autofill/core... components/autofill/core/common/save_password_progress_monitor_helper_impl.cc:215: // swap() and RVO to avoid copying the string. This sort of optimization is not necessary for this code. Please prefer simplicity until you see evidence that complexity is worth the cost. https://codereview.chromium.org/216183008/diff/20001/components/autofill/core... File components/autofill/core/common/save_password_progress_monitor_helper_impl.h (right): https://codereview.chromium.org/216183008/diff/20001/components/autofill/core... components/autofill/core/common/save_password_progress_monitor_helper_impl.h:41: std::string ScrubURL(const GURL& url); nit: Methods belong above member variables. https://codereview.chromium.org/216183008/diff/20001/components/password_mana... File components/password_manager/core/common/save_password_progress_monitor.cc (right): https://codereview.chromium.org/216183008/diff/20001/components/password_mana... components/password_manager/core/common/save_password_progress_monitor.cc:8: #include "components/password_manager/core/browser/password_manager_client.h" It's not appropriate for common/ to depend on browser/. Is there any particular reason that this class does not itself live under browser/? https://codereview.chromium.org/216183008/diff/20001/components/password_mana... File components/password_manager/core/common/save_password_progress_monitor.h (right): https://codereview.chromium.org/216183008/diff/20001/components/password_mana... components/password_manager/core/common/save_password_progress_monitor.h:24: void set_client(PasswordManagerClient* client) { client_ = client; } What is the memory ownership model here? How do you guarantee that the client outlives the monitor?
Hi Ilya, Thanks for your patience with me, and the great ideas how to simplify this. PTAL. Vaclav https://codereview.chromium.org/216183008/diff/20001/components/autofill/core... File components/autofill/core/common/save_password_progress_monitor_helper.h (right): https://codereview.chromium.org/216183008/diff/20001/components/autofill/core... components/autofill/core/common/save_password_progress_monitor_helper.h:31: // the appropriate implementation. On 2014/03/29 00:50:52, Ilya Sherman wrote: > This seems really needlessly complicated. There are many simpler options: > (1) Don't run the logging code when it's not needed. > (2) Use a NULL pointer when logging isn't needed, then do (1). > (3) Pass a boolean flag to the monitor class to control whether logging is on > or off. > etc. Fair enough. I think I'll go with (2). To the extent relevant to this CL, this allowed me to get rid of *Impl. https://codereview.chromium.org/216183008/diff/20001/components/autofill/core... components/autofill/core/common/save_password_progress_monitor_helper.h:42: // the logs get diplayed in a wrong order. On 2014/03/29 00:50:52, Ilya Sherman wrote: > This seems a little silly. Why not just send the messages as they come in, > rather than waiting until the class is destroyed? Indeed. I was initially scared of too many IPC calls, but because such debugging will be off most of the time, that's not a concern. Changed. https://codereview.chromium.org/216183008/diff/20001/components/autofill/core... components/autofill/core/common/save_password_progress_monitor_helper.h:59: typedef std::string SerializableLogType; On 2014/03/29 00:50:52, Ilya Sherman wrote: > What's the advantage of obfuscating/renaming std::string? None any more. I used it when I was also passing the group IDs and stuff. Removed, also with hints about serializing. https://codereview.chromium.org/216183008/diff/20001/components/autofill/core... components/autofill/core/common/save_password_progress_monitor_helper.h:65: typedef base::Callback<void(const SerializableLogType&)> LogSender; On 2014/03/29 00:50:52, Ilya Sherman wrote: > Rather than using a callback, how about having subclasses of the class that know > how to send their own messages? Good idea. Done. > Are there more than two types of callbacks that > you anticipate being used? Not in production code. In tests, there are some others. https://codereview.chromium.org/216183008/diff/20001/components/autofill/core... File components/autofill/core/common/save_password_progress_monitor_helper_impl.cc (right): https://codereview.chromium.org/216183008/diff/20001/components/autofill/core... components/autofill/core/common/save_password_progress_monitor_helper_impl.cc:79: } while (false) On 2014/03/29 00:50:52, Ilya Sherman wrote: > Why are you using macros for this? I just got rid of them. (Originally introduced to save lines, but they probably added more.) https://codereview.chromium.org/216183008/diff/20001/components/autofill/core... components/autofill/core/common/save_password_progress_monitor_helper_impl.cc:215: // swap() and RVO to avoid copying the string. On 2014/03/29 00:50:52, Ilya Sherman wrote: > This sort of optimization is not necessary for this code. Please prefer > simplicity until you see evidence that complexity is worth the cost. Done. https://codereview.chromium.org/216183008/diff/20001/components/autofill/core... File components/autofill/core/common/save_password_progress_monitor_helper_impl.h (right): https://codereview.chromium.org/216183008/diff/20001/components/autofill/core... components/autofill/core/common/save_password_progress_monitor_helper_impl.h:41: std::string ScrubURL(const GURL& url); On 2014/03/29 00:50:52, Ilya Sherman wrote: > nit: Methods belong above member variables. Oops, done. https://codereview.chromium.org/216183008/diff/20001/components/password_mana... File components/password_manager/core/common/save_password_progress_monitor.cc (right): https://codereview.chromium.org/216183008/diff/20001/components/password_mana... components/password_manager/core/common/save_password_progress_monitor.cc:8: #include "components/password_manager/core/browser/password_manager_client.h" On 2014/03/29 00:50:52, Ilya Sherman wrote: > It's not appropriate for common/ to depend on browser/. Is there any particular > reason that this class does not itself live under browser/? Good point. I don't see a need for staying in common/, so moving this to browser/. https://codereview.chromium.org/216183008/diff/20001/components/password_mana... File components/password_manager/core/common/save_password_progress_monitor.h (right): https://codereview.chromium.org/216183008/diff/20001/components/password_mana... components/password_manager/core/common/save_password_progress_monitor.h:24: void set_client(PasswordManagerClient* client) { client_ = client; } On 2014/03/29 00:50:52, Ilya Sherman wrote: > What is the memory ownership model here? How do you guarantee that the client > outlives the monitor? The PasswordManagerClient owns the PasswordManagerDriver, which owns the PasswordManager, which owns the SavePasswordProgressMonitor. Thinking about this more, using |set_client| to turn the manager on and off is not the most readable interface. So I'll make the client a construction-time argument, and provide an explicit on/off switch instead.
Thanks, this is much easier to follow! https://codereview.chromium.org/216183008/diff/80001/components/autofill/core... File components/autofill/core/common/save_password_progress_monitor_helper.cc (right): https://codereview.chromium.org/216183008/diff/80001/components/autofill/core... components/autofill/core/common/save_password_progress_monitor_helper.cc:74: log.append(" }"); Are you sure you want all of these statements to appear on a single line? That's going to be a very long line of text... Maybe it would be simpler to build up a base::DictionaryValue, and then print that out in some standard way? https://codereview.chromium.org/216183008/diff/80001/components/autofill/core... components/autofill/core/common/save_password_progress_monitor_helper.cc:102: LogMessage(log); Optional nit: Up to you, but I think condensing this all down to a single line would be simpler: LogMessage(message + ": " + ScrubURL(url)); But, if you think the parallelism with the other methods is worth keeping this method in its current state, that's fine too. https://codereview.chromium.org/216183008/diff/80001/components/autofill/core... File components/autofill/core/common/save_password_progress_monitor_helper.h (right): https://codereview.chromium.org/216183008/diff/80001/components/autofill/core... components/autofill/core/common/save_password_progress_monitor_helper.h:28: class SavePasswordProgressMonitorHelper { nit: This code should be in the autofill namespace, since it's defined within the autofill component. https://codereview.chromium.org/216183008/diff/80001/components/autofill/core... File components/autofill/core/common/save_password_progress_monitor_helper_unittest.cc (right): https://codereview.chromium.org/216183008/diff/80001/components/autofill/core... components/autofill/core/common/save_password_progress_monitor_helper_unittest.cc:27: bool CheckForSubstringInLogs(const std::string& substring); nit: Docs. https://codereview.chromium.org/216183008/diff/80001/components/autofill/core... components/autofill/core/common/save_password_progress_monitor_helper_unittest.cc:27: bool CheckForSubstringInLogs(const std::string& substring); Optional nit: Perhaps name this something like "ContainsSubstring()"? https://codereview.chromium.org/216183008/diff/80001/components/autofill/core... components/autofill/core/common/save_password_progress_monitor_helper_unittest.cc:42: } Optional nit: I'd recommend inlining methods that are defined only in a test implementation file, but up to you. https://codereview.chromium.org/216183008/diff/80001/components/autofill/core... components/autofill/core/common/save_password_progress_monitor_helper_unittest.cc:56: EXPECT_TRUE(helper.CheckForSubstringInLogs("example.org")); nit: "http://example.org"? https://codereview.chromium.org/216183008/diff/80001/components/autofill/core... components/autofill/core/common/save_password_progress_monitor_helper_unittest.cc:63: "f_name", nit: What is "f_"? https://codereview.chromium.org/216183008/diff/80001/components/autofill/core... components/autofill/core/common/save_password_progress_monitor_helper_unittest.cc:93: } Please write separate test cases for the two subtests here. Ditto everywhere below where you have multiple subtests in a single TEST() macro. https://codereview.chromium.org/216183008/diff/80001/components/autofill/core... components/autofill/core/common/save_password_progress_monitor_helper_unittest.cc:135: helper.CheckForSubstringInLogs(base::StringPrintf("%d", signed_number))); Please skip the StringPrintf call and just test for "-12345" directly. That's easier to verify as correct, as I don't have to remember what "%d" means. I'd recommend the same above, but I guess std::numeric_limits<size_t> can vary by platform, so that's harder to do there. https://codereview.chromium.org/216183008/diff/80001/components/password_mana... File components/password_manager/core/browser/save_password_progress_monitor.cc (right): https://codereview.chromium.org/216183008/diff/80001/components/password_mana... components/password_manager/core/browser/save_password_progress_monitor.cc:17: virtual void SendLog(const std::string& log) OVERRIDE; nit: Please include the comment "// SavePasswordProgressMonitorHelper:" above this line. https://codereview.chromium.org/216183008/diff/80001/components/password_mana... components/password_manager/core/browser/save_password_progress_monitor.cc:36: : client_(client), logging_activated_(false) { nit: One variable per line, please. https://codereview.chromium.org/216183008/diff/80001/components/password_mana... components/password_manager/core/browser/save_password_progress_monitor.cc:43: SavePasswordProgressMonitor::CreateHelper() { nit: Indent this line four spaces. https://codereview.chromium.org/216183008/diff/80001/components/password_mana... File components/password_manager/core/browser/save_password_progress_monitor.h (right): https://codereview.chromium.org/216183008/diff/80001/components/password_mana... components/password_manager/core/browser/save_password_progress_monitor.h:18: class SavePasswordProgressMonitor { nit: This code should probably be in a namespace -- is that not common practice for the password_manager component? https://codereview.chromium.org/216183008/diff/80001/components/password_mana... components/password_manager/core/browser/save_password_progress_monitor.h:22: class LocalHelper; Can this be tucked into an anonymous namespace in the implementation file? Or possibly even better, can the SavePasswordProgressMonitor class be entirely replaced by a subclass of the SavePasswordProgressMonitorHelper class (which could then possibly drop the "Helper" from its name)? https://codereview.chromium.org/216183008/diff/80001/components/password_mana... components/password_manager/core/browser/save_password_progress_monitor.h:25: // to activate it. nit: "construction, use" -> "construction. Use" https://codereview.chromium.org/216183008/diff/80001/components/password_mana... File components/password_manager/core/browser/save_password_progress_monitor_unittest.cc (right): https://codereview.chromium.org/216183008/diff/80001/components/password_mana... components/password_manager/core/browser/save_password_progress_monitor_unittest.cc:7: #include "base/callback.h" nit: Doesn't seem to be used. https://codereview.chromium.org/216183008/diff/80001/components/password_mana... components/password_manager/core/browser/save_password_progress_monitor_unittest.cc:24: base::FieldTrial::Probability(const std::string&)); Rather than mocking these methods, please just define empty bodies for them. In fact, it would be fantastic if you could define a new TestPasswordManagerClient class that could be shared among tests, rather than having each test re-invent the wheel. https://codereview.chromium.org/216183008/diff/80001/components/password_mana... components/password_manager/core/browser/save_password_progress_monitor_unittest.cc:28: void(autofill::PasswordFormFillData* fill_data)); Why do you need to mock this at all? https://codereview.chromium.org/216183008/diff/80001/components/password_mana... components/password_manager/core/browser/save_password_progress_monitor_unittest.cc:31: return AuthenticateAutofillAndFillFormPtr(fill_data.release()); This looks like a memory leak. Can you just leave this method body empty instead? https://codereview.chromium.org/216183008/diff/80001/components/password_mana... components/password_manager/core/browser/save_password_progress_monitor_unittest.cc:42: EXPECT_CALL(client, LogSavePasswordProgress(_)).Times(1); nit: Please replace "_" with "test" for a tighter test. https://codereview.chromium.org/216183008/diff/80001/components/password_mana... components/password_manager/core/browser/save_password_progress_monitor_unittest.cc:46: // No more calls to client.LogSavePasswordProgress expected. I think you can include a EXPECT_CALL(client, LogSavePasswordProgress(_)).Times(0); stmt here to verify this. https://codereview.chromium.org/216183008/diff/80001/components/password_mana... components/password_manager/core/browser/save_password_progress_monitor_unittest.cc:48: } Please split this up into several test cases, one per behavior. I'd recommend at least three: (1) Logging is deactivated for all messages. (2) Logging is activated for all messages. (3) Logging starts out deactivated, and then is activated midway through.
Hi Ilya, PTAL. I addressed all your comments. Most of them I accepted fully, but in some cases I pushed back and tried to make sure first that I understood the proposal or you my reasons for the current code. Please review those in particular. Thanks a lot. Vaclav https://codereview.chromium.org/216183008/diff/80001/components/autofill/core... File components/autofill/core/common/save_password_progress_monitor_helper.cc (right): https://codereview.chromium.org/216183008/diff/80001/components/autofill/core... components/autofill/core/common/save_password_progress_monitor_helper.cc:74: log.append(" }"); On 2014/04/03 23:22:52, Ilya Sherman wrote: > Are you sure you want all of these statements to appear on a single line? > That's going to be a very long line of text... > > Maybe it would be simpler to build up a base::DictionaryValue, and then print > that out in some standard way? I like the approach with DictionaryValue -- it allows me to keep formatting in one place, while creating the structure in all the Log* messages. I tried to find a standard way of printing a Value, but I only found the operator<< in values.h, which is only meant for testing. So I implemented a printer myself. https://codereview.chromium.org/216183008/diff/80001/components/autofill/core... components/autofill/core/common/save_password_progress_monitor_helper.cc:102: LogMessage(log); On 2014/04/03 23:22:52, Ilya Sherman wrote: > Optional nit: Up to you, but I think condensing this all down to a single line > would be simpler: > > LogMessage(message + ": " + ScrubURL(url)); > > But, if you think the parallelism with the other methods is worth keeping this > method in its current state, that's fine too. I agree. But anyway, during addressing the previous comment, the code was changed, and this does no longer apply. https://codereview.chromium.org/216183008/diff/80001/components/autofill/core... File components/autofill/core/common/save_password_progress_monitor_helper.h (right): https://codereview.chromium.org/216183008/diff/80001/components/autofill/core... components/autofill/core/common/save_password_progress_monitor_helper.h:28: class SavePasswordProgressMonitorHelper { On 2014/04/03 23:22:52, Ilya Sherman wrote: > nit: This code should be in the autofill namespace, since it's defined within > the autofill component. Done. https://codereview.chromium.org/216183008/diff/80001/components/autofill/core... File components/autofill/core/common/save_password_progress_monitor_helper_unittest.cc (right): https://codereview.chromium.org/216183008/diff/80001/components/autofill/core... components/autofill/core/common/save_password_progress_monitor_helper_unittest.cc:27: bool CheckForSubstringInLogs(const std::string& substring); On 2014/04/03 23:22:52, Ilya Sherman wrote: > Optional nit: Perhaps name this something like "ContainsSubstring()"? Done. Used LogsContainSubstring, because it might not be clear how the helper contains strings. https://codereview.chromium.org/216183008/diff/80001/components/autofill/core... components/autofill/core/common/save_password_progress_monitor_helper_unittest.cc:27: bool CheckForSubstringInLogs(const std::string& substring); On 2014/04/03 23:22:52, Ilya Sherman wrote: > nit: Docs. Did you mean I should document this? If so, did renaming and inlining of the method help? In the current state I'm not sure adding a comment is helping much, but I'll add one if you insist. https://codereview.chromium.org/216183008/diff/80001/components/autofill/core... components/autofill/core/common/save_password_progress_monitor_helper_unittest.cc:42: } On 2014/04/03 23:22:52, Ilya Sherman wrote: > Optional nit: I'd recommend inlining methods that are defined only in a test > implementation file, but up to you. Done. These are self-documenting one-liners, so it actually makes it easier to understand what they do just by reading the class definition. https://codereview.chromium.org/216183008/diff/80001/components/autofill/core... components/autofill/core/common/save_password_progress_monitor_helper_unittest.cc:56: EXPECT_TRUE(helper.CheckForSubstringInLogs("example.org")); On 2014/04/03 23:22:52, Ilya Sherman wrote: > nit: "http://example.org"? Done. https://codereview.chromium.org/216183008/diff/80001/components/autofill/core... components/autofill/core/common/save_password_progress_monitor_helper_unittest.cc:63: "f_name", On 2014/04/03 23:22:52, Ilya Sherman wrote: > nit: What is "f_"? form_, corrected in the code. https://codereview.chromium.org/216183008/diff/80001/components/autofill/core... components/autofill/core/common/save_password_progress_monitor_helper_unittest.cc:93: } On 2014/04/03 23:22:52, Ilya Sherman wrote: > Please write separate test cases for the two subtests here. Ditto everywhere > below where you have multiple subtests in a single TEST() macro. Done. (And the ObjectExist case disappeared completely, because I decided to use the LogBoolean method to log object existence anyway.) https://codereview.chromium.org/216183008/diff/80001/components/autofill/core... components/autofill/core/common/save_password_progress_monitor_helper_unittest.cc:135: helper.CheckForSubstringInLogs(base::StringPrintf("%d", signed_number))); On 2014/04/03 23:22:52, Ilya Sherman wrote: > Please skip the StringPrintf call and just test for "-12345" directly. That's > easier to verify as correct, as I don't have to remember what "%d" means. I'd > recommend the same above, but I guess std::numeric_limits<size_t> can vary by > platform, so that's harder to do there. Done. In the meantime I dropped the support for unsigned values not convertible to int, so that solved the numeric_limits case. https://codereview.chromium.org/216183008/diff/80001/components/password_mana... File components/password_manager/core/browser/save_password_progress_monitor.cc (right): https://codereview.chromium.org/216183008/diff/80001/components/password_mana... components/password_manager/core/browser/save_password_progress_monitor.cc:17: virtual void SendLog(const std::string& log) OVERRIDE; On 2014/04/03 23:22:52, Ilya Sherman wrote: > nit: Please include the comment "// SavePasswordProgressMonitorHelper:" above > this line. Done. https://codereview.chromium.org/216183008/diff/80001/components/password_mana... components/password_manager/core/browser/save_password_progress_monitor.cc:36: : client_(client), logging_activated_(false) { On 2014/04/03 23:22:52, Ilya Sherman wrote: > nit: One variable per line, please. I will do that, if you want to Ilya, but please consider the following: 1) The style guide actually lists this as allowed: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr... 2) This is how "git cl format" formatted it. While "git cl format" is not canonical, it is style guide-conforming, and making exceptions puts more maintenance burden on future coders and reviewers. https://codereview.chromium.org/216183008/diff/80001/components/password_mana... components/password_manager/core/browser/save_password_progress_monitor.cc:43: SavePasswordProgressMonitor::CreateHelper() { On 2014/04/03 23:22:52, Ilya Sherman wrote: > nit: Indent this line four spaces. That would actually be against the code style, look for "If you break after the return type of a function definition, do not indent." in http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi.... Also, the argument with "git cl format" from above applies here. https://codereview.chromium.org/216183008/diff/80001/components/password_mana... File components/password_manager/core/browser/save_password_progress_monitor.h (right): https://codereview.chromium.org/216183008/diff/80001/components/password_mana... components/password_manager/core/browser/save_password_progress_monitor.h:18: class SavePasswordProgressMonitor { On 2014/04/03 23:22:52, Ilya Sherman wrote: > nit: This code should probably be in a namespace -- is that not common practice > for the password_manager component? It will be a common practice: http://crbug.com/348523. I added the namespace here already, so that I don't forget it this CL lands after the one introducing the namespace. https://codereview.chromium.org/216183008/diff/80001/components/password_mana... components/password_manager/core/browser/save_password_progress_monitor.h:22: class LocalHelper; On 2014/04/03 23:22:52, Ilya Sherman wrote: > Can this be tucked into an anonymous namespace in the implementation file? Or > possibly even better, can the SavePasswordProgressMonitor class be entirely > replaced by a subclass of the SavePasswordProgressMonitorHelper class (which > could then possibly drop the "Helper" from its name)? I'm all for an anonymous namespace. Not so sure about having the monitor a subclass of the helper. My view was that the monitor is responsible for knowing when and how to pass string logs for display, whereas the helper will provide formatting, privacy protections (dropping personal info), and other tasks between logging and having the string logs. https://codereview.chromium.org/216183008/diff/80001/components/password_mana... components/password_manager/core/browser/save_password_progress_monitor.h:25: // to activate it. On 2014/04/03 23:22:52, Ilya Sherman wrote: > nit: "construction, use" -> "construction. Use" Done. https://codereview.chromium.org/216183008/diff/80001/components/password_mana... File components/password_manager/core/browser/save_password_progress_monitor_unittest.cc (right): https://codereview.chromium.org/216183008/diff/80001/components/password_mana... components/password_manager/core/browser/save_password_progress_monitor_unittest.cc:7: #include "base/callback.h" On 2014/04/03 23:22:52, Ilya Sherman wrote: > nit: Doesn't seem to be used. Done. https://codereview.chromium.org/216183008/diff/80001/components/password_mana... components/password_manager/core/browser/save_password_progress_monitor_unittest.cc:24: base::FieldTrial::Probability(const std::string&)); On 2014/04/03 23:22:52, Ilya Sherman wrote: > Rather than mocking these methods, please just define empty bodies for them. In > fact, it would be fantastic if you could define a new TestPasswordManagerClient > class that could be shared among tests, rather than having each test re-invent > the wheel. There are two kinds of PasswordManagerClient-derived classes used in tests: TestPasswordManagerClients and MockPasswordManagerClients. The former usually perform some simple function related to the test, so they are neither good candidates for mocking, nor for pulling out into a single TestPasswordManagerClient used by all the tests. The latter (MockPasswordManagerClient) has currently just a single instance, in components/password_manager/core/browser/password_manager_unittest.cc, where I stole it from. I could pull out the MockPasswordManagerClient into a separate file and share it between the password_manager_unittest.cc and this unittest. I'm not sure how much mocked classes (as opposed to test classes) are used this way, however. My impression was that mocks are always local. Please let me know if you still prefer me to pull out the mock client. https://codereview.chromium.org/216183008/diff/80001/components/password_mana... components/password_manager/core/browser/save_password_progress_monitor_unittest.cc:28: void(autofill::PasswordFormFillData* fill_data)); On 2014/04/03 23:22:52, Ilya Sherman wrote: > Why do you need to mock this at all? I don't, good point. For the time being I'll make it a {} implementation, unless you tell me to pull it out of this file (see comment above). https://codereview.chromium.org/216183008/diff/80001/components/password_mana... components/password_manager/core/browser/save_password_progress_monitor_unittest.cc:31: return AuthenticateAutofillAndFillFormPtr(fill_data.release()); On 2014/04/03 23:22:52, Ilya Sherman wrote: > This looks like a memory leak. Can you just leave this method body empty > instead? Done. https://codereview.chromium.org/216183008/diff/80001/components/password_mana... components/password_manager/core/browser/save_password_progress_monitor_unittest.cc:42: EXPECT_CALL(client, LogSavePasswordProgress(_)).Times(1); On 2014/04/03 23:22:52, Ilya Sherman wrote: > nit: Please replace "_" with "test" for a tighter test. Done. https://codereview.chromium.org/216183008/diff/80001/components/password_mana... components/password_manager/core/browser/save_password_progress_monitor_unittest.cc:46: // No more calls to client.LogSavePasswordProgress expected. On 2014/04/03 23:22:52, Ilya Sherman wrote: > I think you can include a EXPECT_CALL(client, > LogSavePasswordProgress(_)).Times(0); stmt here to verify this. Done. https://codereview.chromium.org/216183008/diff/80001/components/password_mana... components/password_manager/core/browser/save_password_progress_monitor_unittest.cc:48: } On 2014/04/03 23:22:52, Ilya Sherman wrote: > Please split this up into several test cases, one per behavior. I'd recommend > at least three: > > (1) Logging is deactivated for all messages. > (2) Logging is activated for all messages. > (3) Logging starts out deactivated, and then is activated midway through. Done.
https://codereview.chromium.org/216183008/diff/80001/components/password_mana... File components/password_manager/core/browser/save_password_progress_monitor.cc (right): https://codereview.chromium.org/216183008/diff/80001/components/password_mana... components/password_manager/core/browser/save_password_progress_monitor.cc:36: : client_(client), logging_activated_(false) { On 2014/04/04 17:58:13, vabr (Chromium) wrote: > On 2014/04/03 23:22:52, Ilya Sherman wrote: > > nit: One variable per line, please. > > I will do that, if you want to Ilya, but please consider the following: > 1) The style guide actually lists this as allowed: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr... The style guide only allows this when the entire method signature fits on a single line. At least, that's how I've always seen the rule interpreted. > 2) This is how "git cl format" formatted it. While "git cl format" is not > canonical, it is style guide-conforming, and making exceptions puts more > maintenance burden on future coders and reviewers. This sounds like a bug with git cl format. If you disagree, how about we move the discussion to a mailing list, and not block this CL on getting it resolved? https://codereview.chromium.org/216183008/diff/80001/components/password_mana... components/password_manager/core/browser/save_password_progress_monitor.cc:43: SavePasswordProgressMonitor::CreateHelper() { On 2014/04/04 17:58:13, vabr (Chromium) wrote: > On 2014/04/03 23:22:52, Ilya Sherman wrote: > > nit: Indent this line four spaces. > > That would actually be against the code style, look for "If you break after the > return type of a function definition, do not indent." in > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi.... > Also, the argument with "git cl format" from above applies here. Good to know -- thanks for correcting me on this. https://codereview.chromium.org/216183008/diff/80001/components/password_mana... File components/password_manager/core/browser/save_password_progress_monitor_unittest.cc (right): https://codereview.chromium.org/216183008/diff/80001/components/password_mana... components/password_manager/core/browser/save_password_progress_monitor_unittest.cc:24: base::FieldTrial::Probability(const std::string&)); On 2014/04/04 17:58:13, vabr (Chromium) wrote: > On 2014/04/03 23:22:52, Ilya Sherman wrote: > > Rather than mocking these methods, please just define empty bodies for them. > In > > fact, it would be fantastic if you could define a new > TestPasswordManagerClient > > class that could be shared among tests, rather than having each test re-invent > > the wheel. > > There are two kinds of PasswordManagerClient-derived classes used in tests: > TestPasswordManagerClients and MockPasswordManagerClients. The former usually > perform some simple function related to the test, so they are neither good > candidates for mocking, nor for pulling out into a single > TestPasswordManagerClient used by all the tests. > The latter (MockPasswordManagerClient) has currently just a single instance, in > components/password_manager/core/browser/password_manager_unittest.cc, where I > stole it from. I could pull out the MockPasswordManagerClient into a separate > file and share it between the password_manager_unittest.cc and this unittest. > I'm not sure how much mocked classes (as opposed to test classes) are used this > way, however. My impression was that mocks are always local. > > Please let me know if you still prefer me to pull out the mock client. I'm asking you to write something similar to this: https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil... That is, create a TestPasswordManagerClient class that just provides empty implementations for all of the pure virtual methods. Individual tests can then provide their own subclasses off of TestPasswordManagerClient, without having to define all of the pure-virtual methods that they don't actually care about. https://codereview.chromium.org/216183008/diff/100001/components/autofill/cor... File components/autofill/core/common/save_password_progress_monitor_helper.cc (right): https://codereview.chromium.org/216183008/diff/100001/components/autofill/cor... components/autofill/core/common/save_password_progress_monitor_helper.cc:29: static const char kEndOfLine[] = "<BR>"; Hmm, I think elsewhere in this CL you were using "\n". I think that's probably better -- I'd hold off on converting to HTML until you're ready to display the text in a web UI. https://codereview.chromium.org/216183008/diff/100001/components/autofill/cor... components/autofill/core/common/save_password_progress_monitor_helper.cc:71: } Can you use base/json/json_writer.h to replace most of this code? https://codereview.chromium.org/216183008/diff/100001/components/autofill/cor... components/autofill/core/common/save_password_progress_monitor_helper.cc:83: // We do not use the "<<" operator for PasswordForms, because it also prints nit: Please omit the "We" -- this pronoun is generally frowned upon in Chromium code, as it's typically just a filler word, and the referent can be unclear. https://codereview.chromium.org/216183008/diff/100001/components/autofill/cor... components/autofill/core/common/save_password_progress_monitor_helper.cc:87: log.SetString("Scheme", "HTML"); nit: Why is "Scheme" capitalized, whereas most everything else is not? https://codereview.chromium.org/216183008/diff/100001/components/autofill/cor... components/autofill/core/common/save_password_progress_monitor_helper.cc:96: log.SetString("Scheme", "OTHER"); nit: Rather than calling log.SetString("Scheme", ...); several times, it might be nice to write this as log.SetString("Scheme", SchemeToString(form.scheme)); (which would require decomposing out the helper SchemeToString function). https://codereview.chromium.org/216183008/diff/100001/components/autofill/cor... components/autofill/core/common/save_password_progress_monitor_helper.cc:100: log.SetString("signon realm", ScrubURL(form_signon_realm_url)); nit: IMO this reads more naturally if you combine the two lines as log.SetString("signon realm", ScrubURL(GURL(form.signon_realm))); https://codereview.chromium.org/216183008/diff/100001/components/autofill/cor... components/autofill/core/common/save_password_progress_monitor_helper.cc:103: ScrubURL(form_original_signon_realm_url)); Ditto https://codereview.chromium.org/216183008/diff/100001/components/autofill/cor... components/autofill/core/common/save_password_progress_monitor_helper.cc:135: LogValue(message, url_log); nit: IMO combining these into a single line actually improves readability. https://codereview.chromium.org/216183008/diff/100001/components/autofill/cor... components/autofill/core/common/save_password_progress_monitor_helper.cc:141: LogValue(message, boolean_log); Ditto here and below. https://codereview.chromium.org/216183008/diff/100001/components/autofill/cor... components/autofill/core/common/save_password_progress_monitor_helper.cc:166: LogValue("Final decision taken", StringValue("DROP the password")); nit: As above, it would be nice not to duplicate the majority of this line three times. https://codereview.chromium.org/216183008/diff/100001/components/autofill/cor... components/autofill/core/common/save_password_progress_monitor_helper.cc:180: std::string SavePasswordProgressMonitorHelper::ScrubURL(const GURL& url) { nit: You might want to return an empty string if the |url| is invalid. Otherwise, a compromised renderer could theoretically crash Chrome... which isn't probably not a great attack vector, but still. https://codereview.chromium.org/216183008/diff/100001/components/autofill/cor... File components/autofill/core/common/save_password_progress_monitor_helper.h (right): https://codereview.chromium.org/216183008/diff/100001/components/autofill/cor... components/autofill/core/common/save_password_progress_monitor_helper.h:15: } // namespace base Optional nit: No need for the "// namespace base" comment when forward-declaring classes. This is an exception to the general rule, because these namespace blocks are so short. https://codereview.chromium.org/216183008/diff/100001/components/autofill/cor... components/autofill/core/common/save_password_progress_monitor_helper.h:70: std::string ScrubURL(const GURL& url); nit: Can this be tucked into the anonymous namespace in the implementation file? https://codereview.chromium.org/216183008/diff/100001/components/autofill/cor... File components/autofill/core/common/save_password_progress_monitor_helper_unittest.cc (right): https://codereview.chromium.org/216183008/diff/100001/components/autofill/cor... components/autofill/core/common/save_password_progress_monitor_helper_unittest.cc:111: EXPECT_TRUE(helper.LogsContainSubstring("654321")); Please split this up into two separate test cases. https://codereview.chromium.org/216183008/diff/100001/components/autofill/cor... components/autofill/core/common/save_password_progress_monitor_helper_unittest.cc:135: } Please split this up into three separate test cases. https://codereview.chromium.org/216183008/diff/100001/components/password_man... File components/password_manager/core/browser/save_password_progress_monitor.cc (right): https://codereview.chromium.org/216183008/diff/100001/components/password_man... components/password_manager/core/browser/save_password_progress_monitor.cc:51: else nit: I've seen "else" after a "return" strongly discouraged in Chromium code, so please omit it. https://codereview.chromium.org/216183008/diff/100001/components/password_man... File components/password_manager/core/browser/save_password_progress_monitor.h (right): https://codereview.chromium.org/216183008/diff/100001/components/password_man... components/password_manager/core/browser/save_password_progress_monitor.h:36: // them. Please document that the returned value *must* not outlive the monitor. (In fact, it might be easier not to transfer ownership at all, so that clients don't need to think about this. I'll again encourage you to think about whether you can just have the SavePasswordProgressMonitor class be a subclass of the "helper" class [which you'd then probably want to rename to something more like "logger" or "scrubber" or something].) https://codereview.chromium.org/216183008/diff/100001/components/password_man... components/password_manager/core/browser/save_password_progress_monitor.h:48: // causes logs being handed to |client_| for displaying. nit: "causes logs being handed" -> "causes logs to be sent" https://codereview.chromium.org/216183008/diff/100001/components/password_man... File components/password_manager/core/browser/save_password_progress_monitor_unittest.cc (right): https://codereview.chromium.org/216183008/diff/100001/components/password_man... components/password_manager/core/browser/save_password_progress_monitor_unittest.cc:11: using password_manager::SavePasswordProgressMonitor; nit: The test code should just live in the password_manager namespace as well. https://codereview.chromium.org/216183008/diff/100001/components/password_man... components/password_manager/core/browser/save_password_progress_monitor_unittest.cc:52: monitor.AcceptLog(kTestText2); nit: It's a little weird that you log kTestText2 before kTestText. Not a big deal, but slightly distracting when grokking the test.
Hi Ilya, Thanks again for your very helpful comments. This time I addressed all of them without objections, please take a look again. Note: due to renaming of the helper class, it's not easy to see the difference between patchsets for it. Sorry about that. :( Good thing that you helped me to squash the size of it first. :) Cheers, Vaclav https://codereview.chromium.org/216183008/diff/80001/components/password_mana... File components/password_manager/core/browser/save_password_progress_monitor.cc (right): https://codereview.chromium.org/216183008/diff/80001/components/password_mana... components/password_manager/core/browser/save_password_progress_monitor.cc:36: : client_(client), logging_activated_(false) { On 2014/04/04 23:13:47, Ilya Sherman wrote: > On 2014/04/04 17:58:13, vabr (Chromium) wrote: > > On 2014/04/03 23:22:52, Ilya Sherman wrote: > > > nit: One variable per line, please. > > > > I will do that, if you want to Ilya, but please consider the following: > > 1) The style guide actually lists this as allowed: > > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr... > > The style guide only allows this when the entire method signature fits on a > single line. At least, that's how I've always seen the rule interpreted. > > > 2) This is how "git cl format" formatted it. While "git cl format" is not > > canonical, it is style guide-conforming, and making exceptions puts more > > maintenance burden on future coders and reviewers. > > This sounds like a bug with git cl format. If you disagree, how about we move > the discussion to a mailing list, and not block this CL on getting it resolved? Apparently, this issue has already been discussed on the mailing list: https://groups.google.com/a/chromium.org/d/msg/chromium-dev/tAHqqLOgDAQ/KtK5X... There the conclusion is that both a) the style guide is ambiguous, b) the clang-format does a legal thing and is not going to be changed, and c) the code OWNERS have the final word on following clang-format suggestions or not. So, while I'm not happy about this state, I'm now implementing your suggestion. https://codereview.chromium.org/216183008/diff/80001/components/password_mana... File components/password_manager/core/browser/save_password_progress_monitor_unittest.cc (right): https://codereview.chromium.org/216183008/diff/80001/components/password_mana... components/password_manager/core/browser/save_password_progress_monitor_unittest.cc:24: base::FieldTrial::Probability(const std::string&)); On 2014/04/04 23:13:47, Ilya Sherman wrote: > On 2014/04/04 17:58:13, vabr (Chromium) wrote: > > On 2014/04/03 23:22:52, Ilya Sherman wrote: > > > Rather than mocking these methods, please just define empty bodies for them. > > > In > > > fact, it would be fantastic if you could define a new > > TestPasswordManagerClient > > > class that could be shared among tests, rather than having each test > re-invent > > > the wheel. > > > > There are two kinds of PasswordManagerClient-derived classes used in tests: > > TestPasswordManagerClients and MockPasswordManagerClients. The former usually > > perform some simple function related to the test, so they are neither good > > candidates for mocking, nor for pulling out into a single > > TestPasswordManagerClient used by all the tests. > > The latter (MockPasswordManagerClient) has currently just a single instance, > in > > components/password_manager/core/browser/password_manager_unittest.cc, where I > > stole it from. I could pull out the MockPasswordManagerClient into a separate > > file and share it between the password_manager_unittest.cc and this unittest. > > I'm not sure how much mocked classes (as opposed to test classes) are used > this > > way, however. My impression was that mocks are always local. > > > > Please let me know if you still prefer me to pull out the mock client. > > I'm asking you to write something similar to this: > https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil... > > That is, create a TestPasswordManagerClient class that just provides empty > implementations for all of the pure virtual methods. Individual tests can then > provide their own subclasses off of TestPasswordManagerClient, without having to > define all of the pure-virtual methods that they don't actually care about. Ah, thanks, that makes sense. I did it, but used the word Dummy to prefix the test client class name, to avoid clash with the existing TestPasswordManagerClient class. https://codereview.chromium.org/216183008/diff/100001/components/autofill/cor... File components/autofill/core/common/save_password_progress_monitor_helper.cc (right): https://codereview.chromium.org/216183008/diff/100001/components/autofill/cor... components/autofill/core/common/save_password_progress_monitor_helper.cc:29: static const char kEndOfLine[] = "<BR>"; On 2014/04/04 23:13:47, Ilya Sherman wrote: > Hmm, I think elsewhere in this CL you were using "\n". I think that's probably > better -- I'd hold off on converting to HTML until you're ready to display the > text in a web UI. Done. (Actually, this part of code is now replaced by the JSONWriter call.) https://codereview.chromium.org/216183008/diff/100001/components/autofill/cor... components/autofill/core/common/save_password_progress_monitor_helper.cc:71: } On 2014/04/04 23:13:47, Ilya Sherman wrote: > Can you use base/json/json_writer.h to replace most of this code? Great idea! Done. https://codereview.chromium.org/216183008/diff/100001/components/autofill/cor... components/autofill/core/common/save_password_progress_monitor_helper.cc:83: // We do not use the "<<" operator for PasswordForms, because it also prints On 2014/04/04 23:13:47, Ilya Sherman wrote: > nit: Please omit the "We" -- this pronoun is generally frowned upon in Chromium > code, as it's typically just a filler word, and the referent can be unclear. Done. https://codereview.chromium.org/216183008/diff/100001/components/autofill/cor... components/autofill/core/common/save_password_progress_monitor_helper.cc:87: log.SetString("Scheme", "HTML"); On 2014/04/04 23:13:47, Ilya Sherman wrote: > nit: Why is "Scheme" capitalized, whereas most everything else is not? My mistake, thanks for catching that. Done. https://codereview.chromium.org/216183008/diff/100001/components/autofill/cor... components/autofill/core/common/save_password_progress_monitor_helper.cc:96: log.SetString("Scheme", "OTHER"); On 2014/04/04 23:13:47, Ilya Sherman wrote: > nit: Rather than calling log.SetString("Scheme", ...); several times, it might > be nice to write this as log.SetString("Scheme", SchemeToString(form.scheme)); > (which would require decomposing out the helper SchemeToString function). Done. https://codereview.chromium.org/216183008/diff/100001/components/autofill/cor... components/autofill/core/common/save_password_progress_monitor_helper.cc:100: log.SetString("signon realm", ScrubURL(form_signon_realm_url)); On 2014/04/04 23:13:47, Ilya Sherman wrote: > nit: IMO this reads more naturally if you combine the two lines as > log.SetString("signon realm", ScrubURL(GURL(form.signon_realm))); Done. https://codereview.chromium.org/216183008/diff/100001/components/autofill/cor... components/autofill/core/common/save_password_progress_monitor_helper.cc:103: ScrubURL(form_original_signon_realm_url)); On 2014/04/04 23:13:47, Ilya Sherman wrote: > Ditto Done. https://codereview.chromium.org/216183008/diff/100001/components/autofill/cor... components/autofill/core/common/save_password_progress_monitor_helper.cc:135: LogValue(message, url_log); On 2014/04/04 23:13:47, Ilya Sherman wrote: > nit: IMO combining these into a single line actually improves readability. Done. https://codereview.chromium.org/216183008/diff/100001/components/autofill/cor... components/autofill/core/common/save_password_progress_monitor_helper.cc:141: LogValue(message, boolean_log); On 2014/04/04 23:13:47, Ilya Sherman wrote: > Ditto here and below. Done. https://codereview.chromium.org/216183008/diff/100001/components/autofill/cor... components/autofill/core/common/save_password_progress_monitor_helper.cc:166: LogValue("Final decision taken", StringValue("DROP the password")); On 2014/04/04 23:13:47, Ilya Sherman wrote: > nit: As above, it would be nice not to duplicate the majority of this line three > times. Done. https://codereview.chromium.org/216183008/diff/100001/components/autofill/cor... components/autofill/core/common/save_password_progress_monitor_helper.cc:180: std::string SavePasswordProgressMonitorHelper::ScrubURL(const GURL& url) { On 2014/04/04 23:13:47, Ilya Sherman wrote: > nit: You might want to return an empty string if the |url| is invalid. > Otherwise, a compromised renderer could theoretically crash Chrome... which > isn't probably not a great attack vector, but still. Done. https://codereview.chromium.org/216183008/diff/100001/components/autofill/cor... File components/autofill/core/common/save_password_progress_monitor_helper.h (right): https://codereview.chromium.org/216183008/diff/100001/components/autofill/cor... components/autofill/core/common/save_password_progress_monitor_helper.h:15: } // namespace base On 2014/04/04 23:13:47, Ilya Sherman wrote: > Optional nit: No need for the "// namespace base" comment when forward-declaring > classes. This is an exception to the general rule, because these namespace > blocks are so short. Done. https://codereview.chromium.org/216183008/diff/100001/components/autofill/cor... components/autofill/core/common/save_password_progress_monitor_helper.h:70: std::string ScrubURL(const GURL& url); On 2014/04/04 23:13:47, Ilya Sherman wrote: > nit: Can this be tucked into the anonymous namespace in the implementation file? Done. https://codereview.chromium.org/216183008/diff/100001/components/autofill/cor... File components/autofill/core/common/save_password_progress_monitor_helper_unittest.cc (right): https://codereview.chromium.org/216183008/diff/100001/components/autofill/cor... components/autofill/core/common/save_password_progress_monitor_helper_unittest.cc:111: EXPECT_TRUE(helper.LogsContainSubstring("654321")); On 2014/04/04 23:13:47, Ilya Sherman wrote: > Please split this up into two separate test cases. Done. https://codereview.chromium.org/216183008/diff/100001/components/autofill/cor... components/autofill/core/common/save_password_progress_monitor_helper_unittest.cc:135: } On 2014/04/04 23:13:47, Ilya Sherman wrote: > Please split this up into three separate test cases. Done. Sorry for missing that last time. https://codereview.chromium.org/216183008/diff/100001/components/password_man... File components/password_manager/core/browser/save_password_progress_monitor.cc (right): https://codereview.chromium.org/216183008/diff/100001/components/password_man... components/password_manager/core/browser/save_password_progress_monitor.cc:51: else On 2014/04/04 23:13:47, Ilya Sherman wrote: > nit: I've seen "else" after a "return" strongly discouraged in Chromium code, so > please omit it. Oops, thanks for catching this. Done. https://codereview.chromium.org/216183008/diff/100001/components/password_man... File components/password_manager/core/browser/save_password_progress_monitor.h (right): https://codereview.chromium.org/216183008/diff/100001/components/password_man... components/password_manager/core/browser/save_password_progress_monitor.h:36: // them. On 2014/04/04 23:13:47, Ilya Sherman wrote: > Please document that the returned value *must* not outlive the monitor. (In > fact, it might be easier not to transfer ownership at all, so that clients don't > need to think about this. I'll again encourage you to think about whether you > can just have the SavePasswordProgressMonitor class be a subclass of the > "helper" class [which you'd then probably want to rename to something more like > "logger" or "scrubber" or something].) OK, I reconsidered, and decided to change *MonitorHelper to *Logger, and let *Monitor inherit it. I just realised that the (now) *Logger is stateless, just a bunch of convenience methods, and these methods are always needed when the monitor is used, so the logic of the code does not seem to suffer. (If the *Logger were not stateless, it would be weirder, because there used to be multiple instances of it, which would all become one monitor now). Once I start using the other kind of *Logger in the renderer code, it might seem strange, that that *Logger would be sending something over IPC to the *Monitor, which is also a *Logger. But that looks like a minor concern to me. Thanks for your persistence here :), I believe this change you steered me into particularly improved and simplified the code. https://codereview.chromium.org/216183008/diff/100001/components/password_man... components/password_manager/core/browser/save_password_progress_monitor.h:48: // causes logs being handed to |client_| for displaying. On 2014/04/04 23:13:47, Ilya Sherman wrote: > nit: "causes logs being handed" -> "causes logs to be sent" Done. https://codereview.chromium.org/216183008/diff/100001/components/password_man... File components/password_manager/core/browser/save_password_progress_monitor_unittest.cc (right): https://codereview.chromium.org/216183008/diff/100001/components/password_man... components/password_manager/core/browser/save_password_progress_monitor_unittest.cc:11: using password_manager::SavePasswordProgressMonitor; On 2014/04/04 23:13:47, Ilya Sherman wrote: > nit: The test code should just live in the password_manager namespace as well. Done, also for the other unittest in the autofill namespace. Please let me know if that's different. https://codereview.chromium.org/216183008/diff/100001/components/password_man... components/password_manager/core/browser/save_password_progress_monitor_unittest.cc:52: monitor.AcceptLog(kTestText2); On 2014/04/04 23:13:47, Ilya Sherman wrote: > nit: It's a little weird that you log kTestText2 before kTestText. Not a big > deal, but slightly distracting when grokking the test. Done, sorry about that. :)
Thanks! Just a few comments left: https://codereview.chromium.org/216183008/diff/80001/components/password_mana... File components/password_manager/core/browser/save_password_progress_monitor.cc (right): https://codereview.chromium.org/216183008/diff/80001/components/password_mana... components/password_manager/core/browser/save_password_progress_monitor.cc:36: : client_(client), logging_activated_(false) { On 2014/04/07 14:36:23, vabr (Chromium) wrote: > On 2014/04/04 23:13:47, Ilya Sherman wrote: > > On 2014/04/04 17:58:13, vabr (Chromium) wrote: > > > On 2014/04/03 23:22:52, Ilya Sherman wrote: > > > > nit: One variable per line, please. > > > > > > I will do that, if you want to Ilya, but please consider the following: > > > 1) The style guide actually lists this as allowed: > > > > > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr... > > > > The style guide only allows this when the entire method signature fits on a > > single line. At least, that's how I've always seen the rule interpreted. > > > > > 2) This is how "git cl format" formatted it. While "git cl format" is not > > > canonical, it is style guide-conforming, and making exceptions puts more > > > maintenance burden on future coders and reviewers. > > > > This sounds like a bug with git cl format. If you disagree, how about we move > > the discussion to a mailing list, and not block this CL on getting it > resolved? > > Apparently, this issue has already been discussed on the mailing list: > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/tAHqqLOgDAQ/KtK5X... > > There the conclusion is that both a) the style guide is ambiguous, b) the > clang-format does a legal thing and is not going to be changed, and c) the code > OWNERS have the final word on following clang-format suggestions or not. > > So, while I'm not happy about this state, I'm now implementing your suggestion. Thanks for pointing me to that discussion. If either formatting is ok with the style guide, then either is ok with me as well. I'll try to update my internalized copy of the style guide for future reviews :) https://codereview.chromium.org/216183008/diff/160001/components/autofill/cor... File components/autofill/core/common/save_password_progress_logger.cc (right): https://codereview.chromium.org/216183008/diff/160001/components/autofill/cor... components/autofill/core/common/save_password_progress_logger.cc:58: return StringValue(""); nit: "return StringValue(std::string());" https://codereview.chromium.org/216183008/diff/160001/components/autofill/cor... components/autofill/core/common/save_password_progress_logger.cc:136: base::JSONWriter::OPTIONS_OMIT_BINARY_VALUES | Hmm, why do you set OMIT_BINARY_VALUES? This class doesn't log any binary values, so if one is encountered, that's a bug -- right? https://codereview.chromium.org/216183008/diff/160001/components/password_man... File components/password_manager/core/browser/dummy_test_password_manager_client.h (right): https://codereview.chromium.org/216183008/diff/160001/components/password_man... components/password_manager/core/browser/dummy_test_password_manager_client.h:12: // Use this class as a base for mock clients to avoid stubbing uninteresting nit: "mock" -> "test" (or, "mock or test", if you prefer) https://codereview.chromium.org/216183008/diff/160001/components/password_man... components/password_manager/core/browser/dummy_test_password_manager_client.h:15: class DummyTestPasswordManagerClient : public PasswordManagerClient { optional nit: Perhaps "Stub" or "Stubbed" rather than "DummyTest"? Up to you, whichever you think is clearest. https://codereview.chromium.org/216183008/diff/160001/components/password_man... File components/password_manager/core/browser/save_password_progress_monitor.h (right): https://codereview.chromium.org/216183008/diff/160001/components/password_man... components/password_manager/core/browser/save_password_progress_monitor.h:38: SendLogActiveInactive); Please don't friend tests. This gives test code too much leeway to muck with private state of a class, which can lead to very wonky test code in the long run. Instead, please either (a) Upcast the pointer in the test code, since SendLog() is public on the parent class; or (b) Declare the SendLog() method with protected visibility, and increase the visibility to public in a subclass for testing. https://codereview.chromium.org/216183008/diff/160001/components/password_man... components/password_manager/core/browser/save_password_progress_monitor.h:44: // logging is activated or not. Why not have other classes just send log strings directly to the PasswordManagerClient? To enforce the logging_activated_ check? Regardless, there aren't any other loggers yet. Let's keep the comments restricted to just what this method is used for in this CL. We can then expand the comment in the CL that adds logging from the renderer process.
Hi Ilya, I addressed all your comments, fixed some minor things, and updated the CL description. I also renamed the Monitor class, and changed it's expected usage lifetime (local object as opposed to a PasswordManager member variable), that's described in one of my comments. Let me know if you object to that change. Finally, I started https://codereview.chromium.org/228263004/ to add the renderer version of the logger. You don't have to look at it now, but it's very likely that I'll send it to you once we finish this. Thanks! Vaclav https://codereview.chromium.org/216183008/diff/80001/components/password_mana... File components/password_manager/core/browser/save_password_progress_monitor.cc (right): https://codereview.chromium.org/216183008/diff/80001/components/password_mana... components/password_manager/core/browser/save_password_progress_monitor.cc:36: : client_(client), logging_activated_(false) { On 2014/04/08 00:18:03, Ilya Sherman wrote: > On 2014/04/07 14:36:23, vabr (Chromium) wrote: > > On 2014/04/04 23:13:47, Ilya Sherman wrote: > > > On 2014/04/04 17:58:13, vabr (Chromium) wrote: > > > > On 2014/04/03 23:22:52, Ilya Sherman wrote: > > > > > nit: One variable per line, please. > > > > > > > > I will do that, if you want to Ilya, but please consider the following: > > > > 1) The style guide actually lists this as allowed: > > > > > > > > > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr... > > > > > > The style guide only allows this when the entire method signature fits on a > > > single line. At least, that's how I've always seen the rule interpreted. > > > > > > > 2) This is how "git cl format" formatted it. While "git cl format" is not > > > > canonical, it is style guide-conforming, and making exceptions puts more > > > > maintenance burden on future coders and reviewers. > > > > > > This sounds like a bug with git cl format. If you disagree, how about we > move > > > the discussion to a mailing list, and not block this CL on getting it > > resolved? > > > > Apparently, this issue has already been discussed on the mailing list: > > > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/tAHqqLOgDAQ/KtK5X... > > > > There the conclusion is that both a) the style guide is ambiguous, b) the > > clang-format does a legal thing and is not going to be changed, and c) the > code > > OWNERS have the final word on following clang-format suggestions or not. > > > > So, while I'm not happy about this state, I'm now implementing your > suggestion. > > Thanks for pointing me to that discussion. If either formatting is ok with the > style guide, then either is ok with me as well. I'll try to update my > internalized copy of the style guide for future reviews :) Thanks, Ilya. The current instance actually solved itself by reducing the number of member variables to one here. :) https://codereview.chromium.org/216183008/diff/160001/components/autofill/cor... File components/autofill/core/common/save_password_progress_logger.cc (right): https://codereview.chromium.org/216183008/diff/160001/components/autofill/cor... components/autofill/core/common/save_password_progress_logger.cc:58: return StringValue(""); On 2014/04/08 00:18:03, Ilya Sherman wrote: > nit: "return StringValue(std::string());" Done. https://codereview.chromium.org/216183008/diff/160001/components/autofill/cor... components/autofill/core/common/save_password_progress_logger.cc:136: base::JSONWriter::OPTIONS_OMIT_BINARY_VALUES | On 2014/04/08 00:18:03, Ilya Sherman wrote: > Hmm, why do you set OMIT_BINARY_VALUES? This class doesn't log any binary > values, so if one is encountered, that's a bug -- right? That's a very good point, thanks for spotting that! Done. https://codereview.chromium.org/216183008/diff/160001/components/password_man... File components/password_manager/core/browser/dummy_test_password_manager_client.h (right): https://codereview.chromium.org/216183008/diff/160001/components/password_man... components/password_manager/core/browser/dummy_test_password_manager_client.h:12: // Use this class as a base for mock clients to avoid stubbing uninteresting On 2014/04/08 00:18:03, Ilya Sherman wrote: > nit: "mock" -> "test" (or, "mock or test", if you prefer) Done. https://codereview.chromium.org/216183008/diff/160001/components/password_man... components/password_manager/core/browser/dummy_test_password_manager_client.h:15: class DummyTestPasswordManagerClient : public PasswordManagerClient { On 2014/04/08 00:18:03, Ilya Sherman wrote: > optional nit: Perhaps "Stub" or "Stubbed" rather than "DummyTest"? Up to you, > whichever you think is clearest. That's much better name. Thanks! (I chose "Stub" because it's shorter.) https://codereview.chromium.org/216183008/diff/160001/components/password_man... File components/password_manager/core/browser/save_password_progress_monitor.h (right): https://codereview.chromium.org/216183008/diff/160001/components/password_man... components/password_manager/core/browser/save_password_progress_monitor.h:38: SendLogActiveInactive); On 2014/04/08 00:18:03, Ilya Sherman wrote: > Please don't friend tests. This gives test code too much leeway to muck with > private state of a class, which can lead to very wonky test code in the long > run. > > Instead, please either > (a) Upcast the pointer in the test code, since SendLog() is public on the > parent class; or > (b) Declare the SendLog() method with protected visibility, and increase the > visibility to public in a subclass for testing. Fair enough. SendLog is actually not public even in the *Logger class, so I'll go with (b). https://codereview.chromium.org/216183008/diff/160001/components/password_man... components/password_manager/core/browser/save_password_progress_monitor.h:44: // logging is activated or not. On 2014/04/08 00:18:03, Ilya Sherman wrote: > Why not have other classes just send log strings directly to the > PasswordManagerClient? To enforce the logging_activated_ check? You made me think about this a bit more (thanks!), and I believe the concept of a "monitor" owned by the PasswordManager is no longer needed. The monitor was there to make sure preprocessing such as group IDs etc. was done. I understand the monitor now really just as a browser-side specialisation of the *Logger. So I renamed the class to reflect that. (Also the inheritance now makes 100% sense to me.) I'll also make the future loggers always talk directly to the PasswordManagerClient. The important change is in the lifetime: the (past) *Monitor should be used only as a local variable, and only when logging is required. I should have changed this at the moment I introduced the inheritance, because running all the Log* preprocessing methods all the time, and throwing out only the result was wasteful. That means I need to get rid of the |logging_activated_| flag. The (past) *Monitor is now only constructed on demand, when logging should be active. So that will simplify the whole thing as well. > > Regardless, there aren't any other loggers yet. Let's keep the comments > restricted to just what this method is used for in this CL. We can then expand > the comment in the CL that adds logging from the renderer process. Agreed, done.
LGTM with a final couple of nits addressed. Thanks very much for being open to iterating on the design for this, Václav :) https://codereview.chromium.org/216183008/diff/240001/components/password_man... File components/password_manager/core/browser/browser_save_password_progress_logger.h (right): https://codereview.chromium.org/216183008/diff/240001/components/password_man... components/password_manager/core/browser/browser_save_password_progress_logger.h:29: // The PasswordManagerClient to which logs can be sent for display. nit: Please document that the |client_| is expected to (aka "must") outlive |this|. https://codereview.chromium.org/216183008/diff/240001/components/password_man... File components/password_manager/core/browser/browser_save_password_progress_logger_unittest.cc (right): https://codereview.chromium.org/216183008/diff/240001/components/password_man... components/password_manager/core/browser/browser_save_password_progress_logger_unittest.cc:25: } nit: You can abbreviate these three lines to just "using BrowserSavePasswordProgressLogger::SendLog;"
Thanks, Ilya, For making the design much better. I will update the design doc shortly. Colin, I'm TBR-ing you for components/components_tests.gyp, which just adds two new tests. Sending to CQ now. Cheers, Vaclav https://codereview.chromium.org/216183008/diff/240001/components/password_man... File components/password_manager/core/browser/browser_save_password_progress_logger.h (right): https://codereview.chromium.org/216183008/diff/240001/components/password_man... components/password_manager/core/browser/browser_save_password_progress_logger.h:29: // The PasswordManagerClient to which logs can be sent for display. On 2014/04/08 20:50:30, Ilya Sherman wrote: > nit: Please document that the |client_| is expected to (aka "must") outlive > |this|. Done. https://codereview.chromium.org/216183008/diff/240001/components/password_man... File components/password_manager/core/browser/browser_save_password_progress_logger_unittest.cc (right): https://codereview.chromium.org/216183008/diff/240001/components/password_man... components/password_manager/core/browser/browser_save_password_progress_logger_unittest.cc:25: } On 2014/04/08 20:50:30, Ilya Sherman wrote: > nit: You can abbreviate these three lines to just "using > BrowserSavePasswordProgressLogger::SendLog;" TIL, thanks! :)
The CQ bit was checked by vabr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vabr@chromium.org/216183008/300001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vabr@chromium.org/216183008/300001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vabr@chromium.org/216183008/300001
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vabr@chromium.org/216183008/300001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vabr@chromium.org/216183008/300001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vabr@chromium.org/216183008/300001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vabr@chromium.org/216183008/300001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vabr@chromium.org/216183008/300001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vabr@chromium.org/216183008/300001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vabr@chromium.org/216183008/300001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vabr@chromium.org/216183008/300001
Message was sent while issue was closed.
Change committed as 262671 |
