Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1193143003:
Enable import/export of passwords into/from Password Manager (Closed)
|
Created:
5 years, 6 months ago by xunlu Modified:
4 years, 8 months ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, michaelpg+watch-options_chromium.org, tfarina, vabr+watchlist_chromium.org, arv+watch_chromium.org, asvitkine+watch_chromium.org, gcasto+watchlist_chromium.org, mkwst+watchlist-passwords_chromium.org, sabineb Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnable import/export of passwords into/from Password Manager
This change would allow users to bring their CSV-formatted passwords from major
third party login managers into chrome password manager, and vice versa.
BUG=341477
Committed: https://crrev.com/d0f2f0767cf45adc6c6f0d854a991faf841857a9
Cr-Commit-Position: refs/heads/master@{#387844}
Patch Set 1 : #
Total comments: 45
Patch Set 2 : Addressing comments #Patch Set 3 : fix problem revealed through git-cl-try #
Total comments: 14
Patch Set 4 : Add finch switch and address comments #
Total comments: 18
Patch Set 5 : Rebase and address comments #
Total comments: 4
Patch Set 6 : comments #
Total comments: 4
Patch Set 7 : nit #
Total comments: 20
Patch Set 8 : Rebase and comments #
Total comments: 2
Patch Set 9 : Addressing comments #
Total comments: 4
Patch Set 10 : nit #Patch Set 11 : rebase #Patch Set 12 : rebasing and refactoring to bring this cl up to date. #Patch Set 13 : rebase #Patch Set 14 : Removed import-complete dialog. #
Total comments: 94
Patch Set 15 : Address comments. #
Total comments: 12
Patch Set 16 : address comments. #
Total comments: 12
Patch Set 17 : comments #
Total comments: 5
Patch Set 18 : rebase #
Total comments: 2
Patch Set 19 : UMA comments #Patch Set 20 : fix trybot error #Patch Set 21 : rebase #
Total comments: 32
Patch Set 22 : comments #
Total comments: 4
Patch Set 23 : comment #
Total comments: 20
Patch Set 24 : comment #
Total comments: 12
Patch Set 25 : comment #
Total comments: 4
Messages
Total messages: 109 (37 generated)
Patchset #1 (id:1) has been deleted
xunlu@chromium.org changed reviewers: + gcasto@chromium.org, vabr@chromium.org
Please review this change
I didn't go through all the tests, but this is a first pass. https://codereview.chromium.org/1193143003/diff/20001/chrome/browser/resource... File chrome/browser/resources/options/password_manager.html (right): https://codereview.chromium.org/1193143003/diff/20001/chrome/browser/resource... chrome/browser/resources/options/password_manager.html:5: <div class="password-manager-default"> Wow, this is a terrible diff. https://codereview.chromium.org/1193143003/diff/20001/chrome/browser/resource... File chrome/browser/resources/options/password_manager.js (right): https://codereview.chromium.org/1193143003/diff/20001/chrome/browser/resource... chrome/browser/resources/options/password_manager.js:62: var thiz = this; It looks like this variable is named "self" in most of the other js files in this directory. https://codereview.chromium.org/1193143003/diff/20001/chrome/browser/resource... chrome/browser/resources/options/password_manager.js:258: var sections = document.querySelectorAll('.password-manager-default'); This seems like it might look a little strange, to hide the rest of the UI when we are showing the import dialog and then displaying it again afterwards. We should file a the launch but so you can attach screenshots to see how this looks in practice. https://codereview.chromium.org/1193143003/diff/20001/chrome/browser/ui/passw... File chrome/browser/ui/passwords/password_manager_presenter.cc (right): https://codereview.chromium.org/1193143003/diff/20001/chrome/browser/ui/passw... chrome/browser/ui/passwords/password_manager_presenter.cc:134: if (!password_manager_sync_metrics::IsSyncAccountCredential( I think that we decided for now that we were going to allow importing everything, since we don't disallow importing the sync credential via browser import. We should file a bug for this to make sure that it doesn't get lost though. https://codereview.chromium.org/1193143003/diff/20001/chrome/browser/ui/passw... chrome/browser/ui/passwords/password_manager_presenter.cc:191: ScopedVector<autofill::PasswordForm> retVal = password_list_.Pass(); Why aren't you just returning password_list_.Pass()? https://codereview.chromium.org/1193143003/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/password_manager_handler.cc (right): https://codereview.chromium.org/1193143003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/password_manager_handler.cc:48: IMPORT_FILE_SELECTED = 1, I'm assuming you are starting this at "1" so that if the file selector is called with nullptr it will be an error. You probably want to comment that since it's not obvious. https://codereview.chromium.org/1193143003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/password_manager_handler.cc:176: std::vector<std::string> tmp_supported_extensions = The fact that you need to specify the restrictions without the trailing dot, but FilePath::extension() contains the trailing dot is annoying. I think that I would make it so that this difference only needs to be handled in PasswordImporter, so GetSupportedFileExtensions() would be "csv" not ".csv". Then we remove the need to keep a copy here. I guess this would also need to return a vector<vector<string>> instead of vector<string> to match FileTypeInfo::extensions. https://codereview.chromium.org/1193143003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/password_manager_handler.cc:307: ui::SelectFileDialog::SELECT_OPEN_FILE, base::string16(), We may want to see if we should specify a title for this dialog. https://codereview.chromium.org/1193143003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/password_manager_handler.cc:308: base::FilePath(), &file_type_info, 1, "csv", I would DCHECK that supported extensions is not empty, and then use file_type_info.extensions[0][0]. The code otherwise tries to get the exact knowledge of the extensions supported in PasswordImporter, so it's strange that it would be hard coded here. https://codereview.chromium.org/1193143003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/password_manager_handler.cc:330: forms.size()); Especially since forms will always be empty here, I would think that it's more important to log what type of error this is. If we decide to do partial imports, a stat for number of passwords imported could be added. https://codereview.chromium.org/1193143003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/password_manager_handler.cc:330: forms.size()); Do we want to return here? I guess we haven't completely figured out how to handle failure but I think we decided that showing success in this case isn't great. https://codereview.chromium.org/1193143003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/password_manager_handler.cc:340: } Also logging failures due to this function returning false would be nice, though at the moment I don't think that it's possible for this to fail. https://codereview.chromium.org/1193143003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/password_manager_handler.cc:347: file_type_info.extensions.resize(supported_extentions_.size()); Not that this currently makes the assumption that import and export use the same file types. I would just use PasswordExporter::GetSupportedExtensions() here, but I suppose you could also codify this by having only one GetSupportedExtensions() function, though I'm not sure where it would go. https://codereview.chromium.org/1193143003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/password_manager_handler.cc:378: // TODO(xunlu): We do not plan to give any UI feedback for export I probably wouldn't label this as a TODO, since it's not really a call for action. It's just a comment. https://codereview.chromium.org/1193143003/diff/20001/components/password_man... File components/password_manager/core/browser/export/password_exporter.cc (right): https://codereview.chromium.org/1193143003/diff/20001/components/password_man... components/password_manager/core/browser/export/password_exporter.cc:93: // PasswordJSONWriter --------------------------------------------------------- Same as the importer, I wouldn't add this if we aren't going to support it right now. https://codereview.chromium.org/1193143003/diff/20001/components/password_man... File components/password_manager/core/browser/import/password_importer.cc (right): https://codereview.chromium.org/1193143003/diff/20001/components/password_man... components/password_manager/core/browser/import/password_importer.cc:25: typedef std::map<std::string, std::string> ColumnNameToValueMap; Seems like this typedef should really be in csv_reader.h, especially since it's already mentioned in the comments. https://codereview.chromium.org/1193143003/diff/20001/components/password_man... components/password_manager/core/browser/import/password_importer.cc:64: return PasswordImporter::SEMANTICAL_ERROR; This should be SEMANTIC_ERROR. Semantical isn't actually a word. https://codereview.chromium.org/1193143003/diff/20001/components/password_man... components/password_manager/core/browser/import/password_importer.cc:70: ConstRecordIterator; I wouldn't use a typedef just for a one time use. I would just write this inline. https://codereview.chromium.org/1193143003/diff/20001/components/password_man... components/password_manager/core/browser/import/password_importer.cc:91: !record.count(password_field_name_)) Generally we use braces when the "if" statement is more than one line long. https://codereview.chromium.org/1193143003/diff/20001/components/password_man... components/password_manager/core/browser/import/password_importer.cc:116: const base::FilePath::CharType PasswordCSVReader::kFileExtension[] = ".csv"; I'm pretty sure that this needs to be " = FILE_PATH_LITERAL(".csv")" to work correctly cross platform. https://codereview.chromium.org/1193143003/diff/20001/components/password_man... components/password_manager/core/browser/import/password_importer.cc:126: // PasswordJSONReader --------------------------------------------------------- Assuming we aren't going to support this functionality, we shouldn't add this code. I would remove it for now.
Patchset #2 (id:40001) has been deleted
https://codereview.chromium.org/1193143003/diff/20001/chrome/browser/resource... File chrome/browser/resources/options/password_manager.js (right): https://codereview.chromium.org/1193143003/diff/20001/chrome/browser/resource... chrome/browser/resources/options/password_manager.js:62: var thiz = this; On 2015/06/23 23:42:35, Garrett Casto wrote: > It looks like this variable is named "self" in most of the other js files in > this directory. Right. I actually named it "self" until I saw the second answer in http://stackoverflow.com/questions/962033/what-underlies-this-javascript-idio... It says that modern browser provides a global variable "self" for some other purpose https://codereview.chromium.org/1193143003/diff/20001/chrome/browser/resource... chrome/browser/resources/options/password_manager.js:258: var sections = document.querySelectorAll('.password-manager-default'); On 2015/06/23 23:42:35, Garrett Casto wrote: > This seems like it might look a little strange, to hide the rest of the UI when > we are showing the import dialog and then displaying it again afterwards. We > should file a the launch but so you can attach screenshots to see how this looks > in practice. Done. https://codereview.chromium.org/1193143003/diff/20001/chrome/browser/ui/passw... File chrome/browser/ui/passwords/password_manager_presenter.cc (right): https://codereview.chromium.org/1193143003/diff/20001/chrome/browser/ui/passw... chrome/browser/ui/passwords/password_manager_presenter.cc:134: if (!password_manager_sync_metrics::IsSyncAccountCredential( On 2015/06/23 23:42:35, Garrett Casto wrote: > I think that we decided for now that we were going to allow importing > everything, since we don't disallow importing the sync credential via browser > import. We should file a bug for this to make sure that it doesn't get lost > though. Done. https://codereview.chromium.org/1193143003/diff/20001/chrome/browser/ui/passw... chrome/browser/ui/passwords/password_manager_presenter.cc:191: ScopedVector<autofill::PasswordForm> retVal = password_list_.Pass(); On 2015/06/23 23:42:35, Garrett Casto wrote: > Why aren't you just returning password_list_.Pass()? If I just return password_list_.Pass(), I will need to call UpdatePasswordLists() in PasswordManagerHandler to repopulate password_list_. But I think it's better to just wrap this call in PasswordManagerPresenter. https://codereview.chromium.org/1193143003/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/password_manager_handler.cc (right): https://codereview.chromium.org/1193143003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/password_manager_handler.cc:48: IMPORT_FILE_SELECTED = 1, On 2015/06/23 23:42:36, Garrett Casto wrote: > I'm assuming you are starting this at "1" so that if the file selector is called > with nullptr it will be an error. You probably want to comment that since it's > not obvious. Done. https://codereview.chromium.org/1193143003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/password_manager_handler.cc:176: std::vector<std::string> tmp_supported_extensions = On 2015/06/23 23:42:36, Garrett Casto wrote: > The fact that you need to specify the restrictions without the trailing dot, but > FilePath::extension() contains the trailing dot is annoying. I think that I > would make it so that this difference only needs to be handled in > PasswordImporter, so GetSupportedFileExtensions() would be "csv" not ".csv". > Then we remove the need to keep a copy here. I guess this would also need to > return a vector<vector<string>> instead of vector<string> to match > FileTypeInfo::extensions. Done. https://codereview.chromium.org/1193143003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/password_manager_handler.cc:307: ui::SelectFileDialog::SELECT_OPEN_FILE, base::string16(), On 2015/06/23 23:42:36, Garrett Casto wrote: > We may want to see if we should specify a title for this dialog. Done. https://codereview.chromium.org/1193143003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/password_manager_handler.cc:308: base::FilePath(), &file_type_info, 1, "csv", On 2015/06/23 23:42:36, Garrett Casto wrote: > I would DCHECK that supported extensions is not empty, and then use > file_type_info.extensions[0][0]. The code otherwise tries to get the exact > knowledge of the extensions supported in PasswordImporter, so it's strange that > it would be hard coded here. Done. https://codereview.chromium.org/1193143003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/password_manager_handler.cc:330: forms.size()); On 2015/06/23 23:42:35, Garrett Casto wrote: > Especially since forms will always be empty here, I would think that it's more > important to log what type of error this is. If we decide to do partial imports, > a stat for number of passwords imported could be added. Done. https://codereview.chromium.org/1193143003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/password_manager_handler.cc:330: forms.size()); On 2015/06/23 23:42:35, Garrett Casto wrote: > Do we want to return here? I guess we haven't completely figured out how to > handle failure but I think we decided that showing success in this case isn't > great. Done. https://codereview.chromium.org/1193143003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/password_manager_handler.cc:340: } On 2015/06/23 23:42:36, Garrett Casto wrote: > Also logging failures due to this function returning false would be nice, though > at the moment I don't think that it's possible for this to fail. Done. https://codereview.chromium.org/1193143003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/password_manager_handler.cc:347: file_type_info.extensions.resize(supported_extentions_.size()); On 2015/06/23 23:42:36, Garrett Casto wrote: > Not that this currently makes the assumption that import and export use the same > file types. I would just use PasswordExporter::GetSupportedExtensions() here, > but I suppose you could also codify this by having only one > GetSupportedExtensions() function, though I'm not sure where it would go. Done. https://codereview.chromium.org/1193143003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/password_manager_handler.cc:378: // TODO(xunlu): We do not plan to give any UI feedback for export On 2015/06/23 23:42:36, Garrett Casto wrote: > I probably wouldn't label this as a TODO, since it's not really a call for > action. It's just a comment. Done. https://codereview.chromium.org/1193143003/diff/20001/components/password_man... File components/password_manager/core/browser/export/password_exporter.cc (right): https://codereview.chromium.org/1193143003/diff/20001/components/password_man... components/password_manager/core/browser/export/password_exporter.cc:93: // PasswordJSONWriter --------------------------------------------------------- On 2015/06/23 23:42:36, Garrett Casto wrote: > Same as the importer, I wouldn't add this if we aren't going to support it right > now. Done. https://codereview.chromium.org/1193143003/diff/20001/components/password_man... File components/password_manager/core/browser/import/password_importer.cc (right): https://codereview.chromium.org/1193143003/diff/20001/components/password_man... components/password_manager/core/browser/import/password_importer.cc:25: typedef std::map<std::string, std::string> ColumnNameToValueMap; On 2015/06/23 23:42:36, Garrett Casto wrote: > Seems like this typedef should really be in csv_reader.h, especially since it's > already mentioned in the comments. Done. https://codereview.chromium.org/1193143003/diff/20001/components/password_man... components/password_manager/core/browser/import/password_importer.cc:64: return PasswordImporter::SEMANTICAL_ERROR; On 2015/06/23 23:42:36, Garrett Casto wrote: > This should be SEMANTIC_ERROR. Semantical isn't actually a word. Done. https://codereview.chromium.org/1193143003/diff/20001/components/password_man... components/password_manager/core/browser/import/password_importer.cc:70: ConstRecordIterator; On 2015/06/23 23:42:36, Garrett Casto wrote: > I wouldn't use a typedef just for a one time use. I would just write this > inline. Done. https://codereview.chromium.org/1193143003/diff/20001/components/password_man... components/password_manager/core/browser/import/password_importer.cc:91: !record.count(password_field_name_)) On 2015/06/23 23:42:36, Garrett Casto wrote: > Generally we use braces when the "if" statement is more than one line long. Done. https://codereview.chromium.org/1193143003/diff/20001/components/password_man... components/password_manager/core/browser/import/password_importer.cc:116: const base::FilePath::CharType PasswordCSVReader::kFileExtension[] = ".csv"; On 2015/06/23 23:42:36, Garrett Casto wrote: > I'm pretty sure that this needs to be " = FILE_PATH_LITERAL(".csv")" to work > correctly cross platform. Done. https://codereview.chromium.org/1193143003/diff/20001/components/password_man... components/password_manager/core/browser/import/password_importer.cc:126: // PasswordJSONReader --------------------------------------------------------- On 2015/06/23 23:42:36, Garrett Casto wrote: > Assuming we aren't going to support this functionality, we shouldn't add this > code. I would remove it for now. Done.
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:100001) has been deleted
Patchset #3 (id:120001) has been deleted
Patchset #3 (id:140001) has been deleted
Patchset #3 (id:160001) has been deleted
We had talked about having a finch kill switch, did you decide to hold off on that for now? https://codereview.chromium.org/1193143003/diff/20001/chrome/browser/resource... File chrome/browser/resources/options/password_manager.js (right): https://codereview.chromium.org/1193143003/diff/20001/chrome/browser/resource... chrome/browser/resources/options/password_manager.js:62: var thiz = this; On 2015/06/25 17:12:15, xunlu wrote: > On 2015/06/23 23:42:35, Garrett Casto wrote: > > It looks like this variable is named "self" in most of the other js files in > > this directory. > > Right. I actually named it "self" until I saw the second answer in > http://stackoverflow.com/questions/962033/what-underlies-this-javascript-idio... > > It says that modern browser provides a global variable "self" for some other > purpose Perhaps, but given that it's common practice for other options pages, it shouldn't break in this context. My knowledge of JS isn't great, so I'm going to defer to common practice unless that doesn't work for whatever reason. https://codereview.chromium.org/1193143003/diff/20001/chrome/browser/ui/passw... File chrome/browser/ui/passwords/password_manager_presenter.cc (right): https://codereview.chromium.org/1193143003/diff/20001/chrome/browser/ui/passw... chrome/browser/ui/passwords/password_manager_presenter.cc:191: ScopedVector<autofill::PasswordForm> retVal = password_list_.Pass(); On 2015/06/25 17:12:15, xunlu wrote: > On 2015/06/23 23:42:35, Garrett Casto wrote: > > Why aren't you just returning password_list_.Pass()? > > If I just return password_list_.Pass(), I will need to call > UpdatePasswordLists() in PasswordManagerHandler to repopulate password_list_. > But I think it's better to just wrap this call in PasswordManagerPresenter. Got it. What I was actually thinking is that this should have been password_list_.get(), but that doesn't work either because you pass the data on to the FILE thread afterwards. Both for clarity and to save ourselves the work of actually reloading here, I would probably just do an explicit copy here. There is no trivial way to perform a deep copy of a ScopedVector, so I think that you loop through the vector and explicitly copy every element. https://codereview.chromium.org/1193143003/diff/180001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler.cc (right): https://codereview.chromium.org/1193143003/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:370: path.ReplaceExtension(FILE_PATH_LITERAL("csv")), password_list.Pass(), For encapsulation purposes, I don't think that I would override the path extension here and instead do it in PasswordExporter. Otherwise it will be easy to overlook this if we ever support other formats. https://codereview.chromium.org/1193143003/diff/180001/components/password_ma... File components/password_manager/core/browser/export/password_exporter.cc (right): https://codereview.chromium.org/1193143003/diff/180001/components/password_ma... components/password_manager/core/browser/export/password_exporter.cc:85: const base::FilePath::StringType PasswordCSVWriter::kFileExtension = The type was right before. We don't allow static variables of non-POD type. See https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Static_and_G... https://codereview.chromium.org/1193143003/diff/180001/components/password_ma... components/password_manager/core/browser/export/password_exporter.cc:123: extensions.resize(1); Don't resize here. If you care about performance and have a reasonable number of elements in the vector it can be worth doing, but here it is probably a net performance negative as well as being more verbose. https://codereview.chromium.org/1193143003/diff/180001/components/password_ma... File components/password_manager/core/browser/import/csv_reader.h (right): https://codereview.chromium.org/1193143003/diff/180001/components/password_ma... components/password_manager/core/browser/import/csv_reader.h:17: typedef std::map<std::string, std::string> ColumnNameToValueMap; For some reason I forgot that this would be used in csv_writer as well. Feels like this typedef should go in a common location. I guess somewhere in core/browser, maybe just a file called types.h with a comment about what this logically represents and where it's used. https://codereview.chromium.org/1193143003/diff/180001/components/password_ma... components/password_manager/core/browser/import/csv_reader.h:39: std::vector<std::map<std::string, std::string>>* records); |records| should be a vector of ColumnNameToValueMap https://codereview.chromium.org/1193143003/diff/180001/components/password_ma... File components/password_manager/core/browser/import/password_importer.cc (right): https://codereview.chromium.org/1193143003/diff/180001/components/password_ma... components/password_manager/core/browser/import/password_importer.cc:44: static const base::FilePath::StringType kFileExtension; Keep this as CharType[]. https://codereview.chromium.org/1193143003/diff/180001/components/password_ma... components/password_manager/core/browser/import/password_importer.cc:185: extensions.resize(1); No resize.
Hi Xunlu, If you explicitly want two reviews, let me know and I will have a look. Otherwise I'll assume Garrett is reviewing this and you don't need another review. Cheers, Vaclav
Patchset #4 (id:200001) has been deleted
@vabr: Please take a look at code changes in WebUI, thanks! https://codereview.chromium.org/1193143003/diff/20001/chrome/browser/resource... File chrome/browser/resources/options/password_manager.js (right): https://codereview.chromium.org/1193143003/diff/20001/chrome/browser/resource... chrome/browser/resources/options/password_manager.js:62: var thiz = this; On 2015/06/26 21:13:48, Garrett Casto wrote: > On 2015/06/25 17:12:15, xunlu wrote: > > On 2015/06/23 23:42:35, Garrett Casto wrote: > > > It looks like this variable is named "self" in most of the other js files in > > > this directory. > > > > Right. I actually named it "self" until I saw the second answer in > > > http://stackoverflow.com/questions/962033/what-underlies-this-javascript-idio... > > > > It says that modern browser provides a global variable "self" for some other > > purpose > > Perhaps, but given that it's common practice for other options pages, it > shouldn't break in this context. My knowledge of JS isn't great, so I'm going to > defer to common practice unless that doesn't work for whatever reason. Done. https://codereview.chromium.org/1193143003/diff/20001/chrome/browser/ui/passw... File chrome/browser/ui/passwords/password_manager_presenter.cc (right): https://codereview.chromium.org/1193143003/diff/20001/chrome/browser/ui/passw... chrome/browser/ui/passwords/password_manager_presenter.cc:191: ScopedVector<autofill::PasswordForm> retVal = password_list_.Pass(); On 2015/06/26 21:13:48, Garrett Casto wrote: > On 2015/06/25 17:12:15, xunlu wrote: > > On 2015/06/23 23:42:35, Garrett Casto wrote: > > > Why aren't you just returning password_list_.Pass()? > > > > If I just return password_list_.Pass(), I will need to call > > UpdatePasswordLists() in PasswordManagerHandler to repopulate password_list_. > > But I think it's better to just wrap this call in PasswordManagerPresenter. > > Got it. What I was actually thinking is that this should have been > password_list_.get(), but that doesn't work either because you pass the data on > to the FILE thread afterwards. > > Both for clarity and to save ourselves the work of actually reloading here, I > would probably just do an explicit copy here. There is no trivial way to perform > a deep copy of a ScopedVector, so I think that you loop through the vector and > explicitly copy every element. Done. https://codereview.chromium.org/1193143003/diff/180001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler.cc (right): https://codereview.chromium.org/1193143003/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:370: path.ReplaceExtension(FILE_PATH_LITERAL("csv")), password_list.Pass(), On 2015/06/26 21:13:48, Garrett Casto wrote: > For encapsulation purposes, I don't think that I would override the path > extension here and instead do it in PasswordExporter. Otherwise it will be easy > to overlook this if we ever support other formats. Done. https://codereview.chromium.org/1193143003/diff/180001/components/password_ma... File components/password_manager/core/browser/export/password_exporter.cc (right): https://codereview.chromium.org/1193143003/diff/180001/components/password_ma... components/password_manager/core/browser/export/password_exporter.cc:85: const base::FilePath::StringType PasswordCSVWriter::kFileExtension = On 2015/06/26 21:13:48, Garrett Casto wrote: > The type was right before. We don't allow static variables of non-POD type. See > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Static_and_G... Done. https://codereview.chromium.org/1193143003/diff/180001/components/password_ma... components/password_manager/core/browser/export/password_exporter.cc:123: extensions.resize(1); On 2015/06/26 21:13:48, Garrett Casto wrote: > Don't resize here. If you care about performance and have a reasonable number of > elements in the vector it can be worth doing, but here it is probably a net > performance negative as well as being more verbose. If the resize is removed, the extensions[0] on the next line would be illegal. Alternatively, I can replace the extension.resize with extension.push_back(*(new std::vector<base::FilePath::StringType>)). But it doesn't seem to make a difference. http://stackoverflow.com/questions/1902832/resize-versus-push-back-in-stdvect... https://codereview.chromium.org/1193143003/diff/180001/components/password_ma... File components/password_manager/core/browser/import/csv_reader.h (right): https://codereview.chromium.org/1193143003/diff/180001/components/password_ma... components/password_manager/core/browser/import/csv_reader.h:17: typedef std::map<std::string, std::string> ColumnNameToValueMap; On 2015/06/26 21:13:48, Garrett Casto wrote: > For some reason I forgot that this would be used in csv_writer as well. Feels > like this typedef should go in a common location. I guess somewhere in > core/browser, maybe just a file called types.h with a comment about what this > logically represents and where it's used. Done. https://codereview.chromium.org/1193143003/diff/180001/components/password_ma... components/password_manager/core/browser/import/csv_reader.h:39: std::vector<std::map<std::string, std::string>>* records); On 2015/06/26 21:13:48, Garrett Casto wrote: > |records| should be a vector of ColumnNameToValueMap Done. https://codereview.chromium.org/1193143003/diff/180001/components/password_ma... File components/password_manager/core/browser/import/password_importer.cc (right): https://codereview.chromium.org/1193143003/diff/180001/components/password_ma... components/password_manager/core/browser/import/password_importer.cc:44: static const base::FilePath::StringType kFileExtension; On 2015/06/26 21:13:48, Garrett Casto wrote: > Keep this as CharType[]. Done. https://codereview.chromium.org/1193143003/diff/180001/components/password_ma... components/password_manager/core/browser/import/password_importer.cc:185: extensions.resize(1); On 2015/06/26 21:13:48, Garrett Casto wrote: > No resize. See similar comments in password_exporter.cc
Hi Xunlu, On 2015/06/30 23:06:10, xunlu wrote: > @vabr: Please take a look at code changes in WebUI, thanks! You actually need an OWNER to have a look (I'm not an OWNER of WebUI). Consider the people listed in https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/.... Cheers, Vaclav
xunlu@chromium.org changed reviewers: + estade@chromium.org
@estade: Please review the WebUI changes.
On 2015/07/01 09:06:52, vabr (Chromium) wrote: > Hi Xunlu, > > On 2015/06/30 23:06:10, xunlu wrote: > > @vabr: Please take a look at code changes in WebUI, thanks! > > You actually need an OWNER to have a look (I'm not an OWNER of WebUI). Consider > the people listed in > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/.... > > Cheers, > Vaclav I think that I said the wrong directory to Xun. You are needed as a reviewer for chrome/browser/ui/passwords/
Hi Xun, I took a look at chrome/browser/ui/passwords/*. It generally looks good, but I still left some comments. Cheers, Vaclav https://codereview.chromium.org/1193143003/diff/220001/chrome/browser/ui/pass... File chrome/browser/ui/passwords/password_manager_presenter.cc (right): https://codereview.chromium.org/1193143003/diff/220001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.cc:132: for (std::vector<autofill::PasswordForm>::const_iterator it = forms.begin(); optional nit: for (const autofill::PasswordForm* form : forms) {...} https://codereview.chromium.org/1193143003/diff/220001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.cc:140: #if !defined(OS_ANDROID) // This is never called on Android. Instead of the comment, I suggest the self-documenting NOTREACHED(): #if defined(OS_ANDROID) NOTREACHED(); return false; #else ... https://codereview.chromium.org/1193143003/diff/220001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.cc:141: if (IsAuthenticationRequired()) { I suggest to pull out lines 141-147 as a separate private method and reuse it here and in RequestShowPassword() instead of duplicating the code. The main benefit is making sure that we are always consistent with how we ask for reauth. https://codereview.chromium.org/1193143003/diff/220001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.cc:149: return true; Please move the "return true;" to the !defined(OS_ANDROID) block. This should not be a "fall-back option", it is only meant as an answer to no authentication required. The behaviour for OS_ANDROID should be defined in a separate #if clause with NOTREACHED, as indicated in my comment above. https://codereview.chromium.org/1193143003/diff/220001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.cc:189: ScopedVector<autofill::PasswordForm> retVal; nit: ret_val, not retVal (See http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Variable_Names.) https://codereview.chromium.org/1193143003/diff/220001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.cc:191: for (ScopedVector<autofill::PasswordForm>::const_iterator it = optional nit: for (const autofill::PasswordForm* entry : password_list_) {...} https://codereview.chromium.org/1193143003/diff/220001/chrome/browser/ui/pass... File chrome/browser/ui/passwords/password_manager_presenter.h (right): https://codereview.chromium.org/1193143003/diff/220001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.h:56: // Add password forms in |forms| to PasswordStore, return true if succeed. nit: "if succeed" -> "on success" https://codereview.chromium.org/1193143003/diff/220001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.h:57: virtual bool AddPasswordsToStore( There is something fishy about this method -- it does not use anything from the presenter itself, only gets access to the password store through it. I don't think this should be part of the presenter logic. My suggestion is to transfer the for-loop with AddLogin() calls to the handler, and either use PasswordStoreFactory::GetForProfile() to get the store pointer there, or if that's not possible, just make the presenter's GetPasswordStore() public and use it in the handler. https://codereview.chromium.org/1193143003/diff/220001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.h:68: // Request passwords in |password_list_| to be exported. Return true if nit: Could you please make it clearer who/what decides to grant the request or not? Is it the user? Or some automated criterion? Also, is |password_list_| the same as "all password entries" referred with GetAllPasswords above? If so, then we should be consistent and only use "all password entries" in both cases. (Also note that referencing implementation details like data members in public API description may cause issues for readability and staying up to date.)
Patchset #5 (id:240001) has been deleted
Patchset #5 (id:260001) has been deleted
https://codereview.chromium.org/1193143003/diff/220001/chrome/browser/ui/pass... File chrome/browser/ui/passwords/password_manager_presenter.cc (right): https://codereview.chromium.org/1193143003/diff/220001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.cc:132: for (std::vector<autofill::PasswordForm>::const_iterator it = forms.begin(); On 2015/07/06 08:57:15, vabr (Chromium) wrote: > optional nit: > for (const autofill::PasswordForm* form : forms) {...} Done. https://codereview.chromium.org/1193143003/diff/220001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.cc:140: #if !defined(OS_ANDROID) // This is never called on Android. On 2015/07/06 08:57:15, vabr (Chromium) wrote: > Instead of the comment, I suggest the self-documenting NOTREACHED(): > > #if defined(OS_ANDROID) > NOTREACHED(); > return false; > #else > ... Done. https://codereview.chromium.org/1193143003/diff/220001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.cc:141: if (IsAuthenticationRequired()) { On 2015/07/06 08:57:15, vabr (Chromium) wrote: > I suggest to pull out lines 141-147 as a separate private method and reuse it > here and in RequestShowPassword() instead of duplicating the code. The main > benefit is making sure that we are always consistent with how we ask for reauth. Done. https://codereview.chromium.org/1193143003/diff/220001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.cc:149: return true; On 2015/07/06 08:57:16, vabr (Chromium) wrote: > Please move the "return true;" to the !defined(OS_ANDROID) block. This should > not be a "fall-back option", it is only meant as an answer to no authentication > required. The behaviour for OS_ANDROID should be defined in a separate #if > clause with NOTREACHED, as indicated in my comment above. Done. https://codereview.chromium.org/1193143003/diff/220001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.cc:189: ScopedVector<autofill::PasswordForm> retVal; On 2015/07/06 08:57:16, vabr (Chromium) wrote: > nit: ret_val, not retVal > (See > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Variable_Names.) Done. https://codereview.chromium.org/1193143003/diff/220001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.cc:191: for (ScopedVector<autofill::PasswordForm>::const_iterator it = On 2015/07/06 08:57:15, vabr (Chromium) wrote: > optional nit: > for (const autofill::PasswordForm* entry : password_list_) {...} Done. https://codereview.chromium.org/1193143003/diff/220001/chrome/browser/ui/pass... File chrome/browser/ui/passwords/password_manager_presenter.h (right): https://codereview.chromium.org/1193143003/diff/220001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.h:56: // Add password forms in |forms| to PasswordStore, return true if succeed. On 2015/07/06 08:57:16, vabr (Chromium) wrote: > nit: "if succeed" -> "on success" Done. https://codereview.chromium.org/1193143003/diff/220001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.h:57: virtual bool AddPasswordsToStore( On 2015/07/06 08:57:16, vabr (Chromium) wrote: > There is something fishy about this method -- it does not use anything from the > presenter itself, only gets access to the password store through it. I don't > think this should be part of the presenter logic. My suggestion is to transfer > the for-loop with AddLogin() calls to the handler, and either use > PasswordStoreFactory::GetForProfile() to get the store pointer there, or if > that's not possible, just make the presenter's GetPasswordStore() public and use > it in the handler. Done. https://codereview.chromium.org/1193143003/diff/220001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.h:68: // Request passwords in |password_list_| to be exported. Return true if On 2015/07/06 08:57:16, vabr (Chromium) wrote: > nit: Could you please make it clearer who/what decides to grant the request or > not? Is it the user? Or some automated criterion? > > Also, is |password_list_| the same as "all password entries" referred with > GetAllPasswords above? If so, then we should be consistent and only use "all > password entries" in both cases. (Also note that referencing implementation > details like data members in public API description may cause issues for > readability and staying up to date.) Done.
chrome/browser/ui/passwords/ will LGTM once the comments below are addressed. Thanks! Vaclav https://codereview.chromium.org/1193143003/diff/280001/chrome/browser/ui/pass... File chrome/browser/ui/passwords/password_manager_presenter.cc (right): https://codereview.chromium.org/1193143003/diff/280001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.cc:126: bool PasswordManagerPresenter::RequestToExportPassword() { This method now adds nothing to calling IsUserAuthenticated() directly (in particular, the #ifdefs are not necessary, given they also appear in IsUserAuthenticated). I would suggest making IsUserAuthenticated public and replacing RequestToExportPassword with IsUserAuthenticated. https://codereview.chromium.org/1193143003/diff/280001/chrome/browser/ui/pass... File chrome/browser/ui/passwords/password_manager_presenter.h (right): https://codereview.chromium.org/1193143003/diff/280001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.h:79: // Returns true if the user is authenticated nit: Missing a full-stop at the end.
https://codereview.chromium.org/1193143003/diff/280001/chrome/browser/ui/pass... File chrome/browser/ui/passwords/password_manager_presenter.cc (right): https://codereview.chromium.org/1193143003/diff/280001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.cc:126: bool PasswordManagerPresenter::RequestToExportPassword() { On 2015/07/07 12:58:05, vabr (Chromium) wrote: > This method now adds nothing to calling IsUserAuthenticated() directly (in > particular, the #ifdefs are not necessary, given they also appear in > IsUserAuthenticated). I would suggest making IsUserAuthenticated public and > replacing RequestToExportPassword with IsUserAuthenticated. Done. https://codereview.chromium.org/1193143003/diff/280001/chrome/browser/ui/pass... File chrome/browser/ui/passwords/password_manager_presenter.h (right): https://codereview.chromium.org/1193143003/diff/280001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.h:79: // Returns true if the user is authenticated On 2015/07/07 12:58:06, vabr (Chromium) wrote: > nit: Missing a full-stop at the end. Done.
lgtm https://codereview.chromium.org/1193143003/diff/300001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler.cc (right): https://codereview.chromium.org/1193143003/diff/300001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:388: // We do not plan to give any UI feedback for export Nit: This should wrap closer to 80 characters. https://codereview.chromium.org/1193143003/diff/300001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler.h (right): https://codereview.chromium.org/1193143003/diff/300001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.h:11: #include "base/gtest_prod_util.h" Looks like this isn't used. https://codereview.chromium.org/1193143003/diff/300001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.h:32: friend class ::PasswordManagerHandlerTest; It doesn't actually matter where this is, but generally we put this in the private section of the class declaration. https://codereview.chromium.org/1193143003/diff/300001/components/password_ma... File components/password_manager/core/browser/export/password_exporter_unittest.cc (right): https://codereview.chromium.org/1193143003/diff/300001/components/password_ma... components/password_manager/core/browser/export/password_exporter_unittest.cc:44: base::FilePath output_file = Nit: Could you verify that if you don't have ".csv" in output_file that it is appropriately added?
Patchset #7 (id:320001) has been deleted
@estade: Please take a look at the changes under chrome/browser/resources/options/ and /chrome/browser/ui/webui/options/
https://codereview.chromium.org/1193143003/diff/340001/chrome/browser/resourc... File chrome/browser/resources/options/password_manager.js (right): https://codereview.chromium.org/1193143003/diff/340001/chrome/browser/resourc... chrome/browser/resources/options/password_manager.js:254: * be visible final punctuation, also I think this indentation is off https://codereview.chromium.org/1193143003/diff/340001/chrome/browser/resourc... chrome/browser/resources/options/password_manager.js:257: setImportCompleteUIVisibility_: function(visible) { proper capitalization is Ui https://codereview.chromium.org/1193143003/diff/340001/chrome/browser/resourc... chrome/browser/resources/options/password_manager.js:258: var sections = document.querySelectorAll('.password-manager-default'); password-manager-default is a kind of non-descriptive class name (I couldn't figure out what it meant until I read the code). https://codereview.chromium.org/1193143003/diff/340001/chrome/browser/resourc... chrome/browser/resources/options/password_manager.js:260: if (sections[i].disabled != true) { if (!sections[i].disabled) ? or simply sections[i].hidden = visible && !sections[i].disabled; actually, I would just make a css rule .password-manager-default[disabled] { display: none; } and make this function simply set a class on the page. Then add another couple rules like so: #password-manager.importing .password-manager-default { display: none; } https://codereview.chromium.org/1193143003/diff/340001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler.cc (right): https://codereview.chromium.org/1193143003/diff/340001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:188: } no curlies https://codereview.chromium.org/1193143003/diff/340001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:290: default: the cast above is somewhat unsettling, but can you at least cast to the enum type (it will have to not be anonymous) and remove the default case? https://codereview.chromium.org/1193143003/diff/340001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:331: } no curlies https://codereview.chromium.org/1193143003/diff/340001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:355: if (password_manager_presenter_->IsUserAuthenticated()) { invert this check and return early https://codereview.chromium.org/1193143003/diff/340001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:384: base::Unretained(this))); I don't think base::Unretained is valid here. If you close the password manager tab while the export is happening then this could cause a crash. Except that ExportPasswordFileWritten() is empty. So can't you just pass base::Closure() or something?
@estade: I compiled this change on a windows machine and the layout of the [import] [export] [done] button is the exact reverse of the layout on linux. Is this some special OS-specific setting ? https://codereview.chromium.org/1193143003/diff/340001/chrome/browser/resourc... File chrome/browser/resources/options/password_manager.js (right): https://codereview.chromium.org/1193143003/diff/340001/chrome/browser/resourc... chrome/browser/resources/options/password_manager.js:254: * be visible On 2015/07/08 20:38:11, Evan Stade wrote: > final punctuation, also I think this indentation is off Done. https://codereview.chromium.org/1193143003/diff/340001/chrome/browser/resourc... chrome/browser/resources/options/password_manager.js:257: setImportCompleteUIVisibility_: function(visible) { On 2015/07/08 20:38:11, Evan Stade wrote: > proper capitalization is Ui Done. https://codereview.chromium.org/1193143003/diff/340001/chrome/browser/resourc... chrome/browser/resources/options/password_manager.js:258: var sections = document.querySelectorAll('.password-manager-default'); On 2015/07/08 20:38:11, Evan Stade wrote: > password-manager-default is a kind of non-descriptive class name (I couldn't > figure out what it meant until I read the code). Renamed to password-manager-home https://codereview.chromium.org/1193143003/diff/340001/chrome/browser/resourc... chrome/browser/resources/options/password_manager.js:260: if (sections[i].disabled != true) { On 2015/07/08 20:38:11, Evan Stade wrote: > if (!sections[i].disabled) ? or simply sections[i].hidden = visible && > !sections[i].disabled; > > actually, I would just make a css rule > > .password-manager-default[disabled] { > display: none; > } > > and make this function simply set a class on the page. Then add another couple > rules like so: > > #password-manager.importing .password-manager-default { > display: none; > } Done but not sure if this is exactly what you suggested. https://codereview.chromium.org/1193143003/diff/340001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler.cc (right): https://codereview.chromium.org/1193143003/diff/340001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:188: } On 2015/07/08 20:38:12, Evan Stade wrote: > no curlies Done. https://codereview.chromium.org/1193143003/diff/340001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:290: default: On 2015/07/08 20:38:12, Evan Stade wrote: > the cast above is somewhat unsettling, but can you at least cast to the enum > type (it will have to not be anonymous) and remove the default case? Done. https://codereview.chromium.org/1193143003/diff/340001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:331: } On 2015/07/08 20:38:12, Evan Stade wrote: > no curlies Done. https://codereview.chromium.org/1193143003/diff/340001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:355: if (password_manager_presenter_->IsUserAuthenticated()) { On 2015/07/08 20:38:12, Evan Stade wrote: > invert this check and return early Done. https://codereview.chromium.org/1193143003/diff/340001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:384: base::Unretained(this))); On 2015/07/08 20:38:12, Evan Stade wrote: > I don't think base::Unretained is valid here. If you close the password manager > tab while the export is happening then this could cause a crash. Except that > ExportPasswordFileWritten() is empty. So can't you just pass base::Closure() or > something? Done.
> @estade: I compiled this change on a windows machine and the layout of the > [import] [export] [done] button is the exact reverse of the layout on linux. Is > this some special OS-specific setting ? yea. The position of the positive button vs the cancel button is platform specific (cros/win one way, mac/linux the other). Usually there are only two buttons. I don't think the import/export buttons belong in the button strip. Do you have mocks? You could put them in the action area before the spacer div or you could put them elsewhere in the dialog. https://codereview.chromium.org/1193143003/diff/340001/chrome/browser/resourc... File chrome/browser/resources/options/password_manager.js (right): https://codereview.chromium.org/1193143003/diff/340001/chrome/browser/resourc... chrome/browser/resources/options/password_manager.js:260: if (sections[i].disabled != true) { On 2015/07/11 00:18:30, xunlu wrote: > On 2015/07/08 20:38:11, Evan Stade wrote: > > if (!sections[i].disabled) ? or simply sections[i].hidden = visible && > > !sections[i].disabled; > > > > actually, I would just make a css rule > > > > .password-manager-default[disabled] { > > display: none; > > } > > > > and make this function simply set a class on the page. Then add another couple > > rules like so: > > > > #password-manager.importing .password-manager-default { > > display: none; > > } > > Done but not sure if this is exactly what you suggested. this entire function should be something like: $('password-manager').classList.toggle('importing', !visible); the visibility of things can then be controlled in css https://codereview.chromium.org/1193143003/diff/360001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler.cc (right): https://codereview.chromium.org/1193143003/diff/360001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:383: void PasswordManagerHandler::ExportPasswordFileWritten() { is this used?
Patchset #9 (id:380001) has been deleted
https://codereview.chromium.org/1193143003/diff/340001/chrome/browser/resourc... File chrome/browser/resources/options/password_manager.js (right): https://codereview.chromium.org/1193143003/diff/340001/chrome/browser/resourc... chrome/browser/resources/options/password_manager.js:260: if (sections[i].disabled != true) { On 2015/07/11 00:28:41, Evan Stade wrote: > On 2015/07/11 00:18:30, xunlu wrote: > > On 2015/07/08 20:38:11, Evan Stade wrote: > > > if (!sections[i].disabled) ? or simply sections[i].hidden = visible && > > > !sections[i].disabled; > > > > > > actually, I would just make a css rule > > > > > > .password-manager-default[disabled] { > > > display: none; > > > } > > > > > > and make this function simply set a class on the page. Then add another > couple > > > rules like so: > > > > > > #password-manager.importing .password-manager-default { > > > display: none; > > > } > > > > Done but not sure if this is exactly what you suggested. > > this entire function should be something like: > > $('password-manager').classList.toggle('importing', !visible); > > the visibility of things can then be controlled in css Got it. Done https://codereview.chromium.org/1193143003/diff/360001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler.cc (right): https://codereview.chromium.org/1193143003/diff/360001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:383: void PasswordManagerHandler::ExportPasswordFileWritten() { On 2015/07/11 00:28:42, Evan Stade wrote: > is this used? Removed
On 2015/07/13 21:30:27, xunlu wrote: > https://codereview.chromium.org/1193143003/diff/340001/chrome/browser/resourc... > File chrome/browser/resources/options/password_manager.js (right): > > https://codereview.chromium.org/1193143003/diff/340001/chrome/browser/resourc... > chrome/browser/resources/options/password_manager.js:260: if > (sections[i].disabled != true) { > On 2015/07/11 00:28:41, Evan Stade wrote: > > On 2015/07/11 00:18:30, xunlu wrote: > > > On 2015/07/08 20:38:11, Evan Stade wrote: > > > > if (!sections[i].disabled) ? or simply sections[i].hidden = visible && > > > > !sections[i].disabled; > > > > > > > > actually, I would just make a css rule > > > > > > > > .password-manager-default[disabled] { > > > > display: none; > > > > } > > > > > > > > and make this function simply set a class on the page. Then add another > > couple > > > > rules like so: > > > > > > > > #password-manager.importing .password-manager-default { > > > > display: none; > > > > } > > > > > > Done but not sure if this is exactly what you suggested. > > > > this entire function should be something like: > > > > $('password-manager').classList.toggle('importing', !visible); > > > > the visibility of things can then be controlled in css > > Got it. Done > > https://codereview.chromium.org/1193143003/diff/360001/chrome/browser/ui/webu... > File chrome/browser/ui/webui/options/password_manager_handler.cc (right): > > https://codereview.chromium.org/1193143003/diff/360001/chrome/browser/ui/webu... > chrome/browser/ui/webui/options/password_manager_handler.cc:383: void > PasswordManagerHandler::ExportPasswordFileWritten() { > On 2015/07/11 00:28:42, Evan Stade wrote: > > is this used? > > Removed could you link me to the mocks you're working from?
https://codereview.chromium.org/1193143003/diff/400001/chrome/browser/resourc... File chrome/browser/resources/options/password_manager.css (right): https://codereview.chromium.org/1193143003/diff/400001/chrome/browser/resourc... chrome/browser/resources/options/password_manager.css:41: } whitespace in this file is off here and at the end https://codereview.chromium.org/1193143003/diff/400001/chrome/browser/resourc... chrome/browser/resources/options/password_manager.css:43: display: inline; slightly nicer, I think (one rule instead of two): #password-manager:not(.importing) .import-password-complete { display: none; }
On 2015/07/13 23:29:17, Evan Stade wrote: > On 2015/07/13 21:30:27, xunlu wrote: > > > https://codereview.chromium.org/1193143003/diff/340001/chrome/browser/resourc... > > File chrome/browser/resources/options/password_manager.js (right): > > > > > https://codereview.chromium.org/1193143003/diff/340001/chrome/browser/resourc... > > chrome/browser/resources/options/password_manager.js:260: if > > (sections[i].disabled != true) { > > On 2015/07/11 00:28:41, Evan Stade wrote: > > > On 2015/07/11 00:18:30, xunlu wrote: > > > > On 2015/07/08 20:38:11, Evan Stade wrote: > > > > > if (!sections[i].disabled) ? or simply sections[i].hidden = visible && > > > > > !sections[i].disabled; > > > > > > > > > > actually, I would just make a css rule > > > > > > > > > > .password-manager-default[disabled] { > > > > > display: none; > > > > > } > > > > > > > > > > and make this function simply set a class on the page. Then add another > > > couple > > > > > rules like so: > > > > > > > > > > #password-manager.importing .password-manager-default { > > > > > display: none; > > > > > } > > > > > > > > Done but not sure if this is exactly what you suggested. > > > > > > this entire function should be something like: > > > > > > $('password-manager').classList.toggle('importing', !visible); > > > > > > the visibility of things can then be controlled in css > > > > Got it. Done > > > > > https://codereview.chromium.org/1193143003/diff/360001/chrome/browser/ui/webu... > > File chrome/browser/ui/webui/options/password_manager_handler.cc (right): > > > > > https://codereview.chromium.org/1193143003/diff/360001/chrome/browser/ui/webu... > > chrome/browser/ui/webui/options/password_manager_handler.cc:383: void > > PasswordManagerHandler::ExportPasswordFileWritten() { > > On 2015/07/11 00:28:42, Evan Stade wrote: > > > is this used? > > > > Removed > > could you link me to the mocks you're working from? We don't have a mock yet. Here is the link to the launch bug https://code.google.com/p/chromium/issues/detail?id=504138
On 2015/07/13 23:53:49, xunlu wrote: > On 2015/07/13 23:29:17, Evan Stade wrote: > > On 2015/07/13 21:30:27, xunlu wrote: > > > > > > https://codereview.chromium.org/1193143003/diff/340001/chrome/browser/resourc... > > > File chrome/browser/resources/options/password_manager.js (right): > > > > > > > > > https://codereview.chromium.org/1193143003/diff/340001/chrome/browser/resourc... > > > chrome/browser/resources/options/password_manager.js:260: if > > > (sections[i].disabled != true) { > > > On 2015/07/11 00:28:41, Evan Stade wrote: > > > > On 2015/07/11 00:18:30, xunlu wrote: > > > > > On 2015/07/08 20:38:11, Evan Stade wrote: > > > > > > if (!sections[i].disabled) ? or simply sections[i].hidden = visible && > > > > > > !sections[i].disabled; > > > > > > > > > > > > actually, I would just make a css rule > > > > > > > > > > > > .password-manager-default[disabled] { > > > > > > display: none; > > > > > > } > > > > > > > > > > > > and make this function simply set a class on the page. Then add > another > > > > couple > > > > > > rules like so: > > > > > > > > > > > > #password-manager.importing .password-manager-default { > > > > > > display: none; > > > > > > } > > > > > > > > > > Done but not sure if this is exactly what you suggested. > > > > > > > > this entire function should be something like: > > > > > > > > $('password-manager').classList.toggle('importing', !visible); > > > > > > > > the visibility of things can then be controlled in css > > > > > > Got it. Done > > > > > > > > > https://codereview.chromium.org/1193143003/diff/360001/chrome/browser/ui/webu... > > > File chrome/browser/ui/webui/options/password_manager_handler.cc (right): > > > > > > > > > https://codereview.chromium.org/1193143003/diff/360001/chrome/browser/ui/webu... > > > chrome/browser/ui/webui/options/password_manager_handler.cc:383: void > > > PasswordManagerHandler::ExportPasswordFileWritten() { > > > On 2015/07/11 00:28:42, Evan Stade wrote: > > > > is this used? > > > > > > Removed > > > > could you link me to the mocks you're working from? > We don't have a mock yet. Here is the link to the launch bug > https://code.google.com/p/chromium/issues/detail?id=504138 buttons.png looks like a mock. Who created it? Why is the "Access your passwords from any device at passwords.google.com" text missing? Any changes to UI need designer approval (and ideally, mocks).
On 2015/07/13 23:55:42, Evan Stade wrote: > On 2015/07/13 23:53:49, xunlu wrote: > > On 2015/07/13 23:29:17, Evan Stade wrote: > > > On 2015/07/13 21:30:27, xunlu wrote: > > > > > > > > > > https://codereview.chromium.org/1193143003/diff/340001/chrome/browser/resourc... > > > > File chrome/browser/resources/options/password_manager.js (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1193143003/diff/340001/chrome/browser/resourc... > > > > chrome/browser/resources/options/password_manager.js:260: if > > > > (sections[i].disabled != true) { > > > > On 2015/07/11 00:28:41, Evan Stade wrote: > > > > > On 2015/07/11 00:18:30, xunlu wrote: > > > > > > On 2015/07/08 20:38:11, Evan Stade wrote: > > > > > > > if (!sections[i].disabled) ? or simply sections[i].hidden = visible > && > > > > > > > !sections[i].disabled; > > > > > > > > > > > > > > actually, I would just make a css rule > > > > > > > > > > > > > > .password-manager-default[disabled] { > > > > > > > display: none; > > > > > > > } > > > > > > > > > > > > > > and make this function simply set a class on the page. Then add > > another > > > > > couple > > > > > > > rules like so: > > > > > > > > > > > > > > #password-manager.importing .password-manager-default { > > > > > > > display: none; > > > > > > > } > > > > > > > > > > > > Done but not sure if this is exactly what you suggested. > > > > > > > > > > this entire function should be something like: > > > > > > > > > > $('password-manager').classList.toggle('importing', !visible); > > > > > > > > > > the visibility of things can then be controlled in css > > > > > > > > Got it. Done > > > > > > > > > > > > > > https://codereview.chromium.org/1193143003/diff/360001/chrome/browser/ui/webu... > > > > File chrome/browser/ui/webui/options/password_manager_handler.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1193143003/diff/360001/chrome/browser/ui/webu... > > > > chrome/browser/ui/webui/options/password_manager_handler.cc:383: void > > > > PasswordManagerHandler::ExportPasswordFileWritten() { > > > > On 2015/07/11 00:28:42, Evan Stade wrote: > > > > > is this used? > > > > > > > > Removed > > > > > > could you link me to the mocks you're working from? > > We don't have a mock yet. Here is the link to the launch bug > > https://code.google.com/p/chromium/issues/detail?id=504138 > > buttons.png looks like a mock. Who created it? Why is the "Access your passwords > from any device at passwords.google.com" text missing? > > Any changes to UI need designer approval (and ideally, mocks). buttons.png is just a screenshot of this implementation. The reason for ManageAccountLink missing is because when I run the Debug build on my machine I didn't set the corresponding finch commandline flag. @sabineb: Do we have any status update from the designer on this project?
https://codereview.chromium.org/1193143003/diff/400001/chrome/browser/resourc... File chrome/browser/resources/options/password_manager.css (right): https://codereview.chromium.org/1193143003/diff/400001/chrome/browser/resourc... chrome/browser/resources/options/password_manager.css:41: } On 2015/07/13 23:32:43, Evan Stade wrote: > whitespace in this file is off here and at the end Done. https://codereview.chromium.org/1193143003/diff/400001/chrome/browser/resourc... chrome/browser/resources/options/password_manager.css:43: display: inline; On 2015/07/13 23:32:43, Evan Stade wrote: > slightly nicer, I think (one rule instead of two): > > #password-manager:not(.importing) .import-password-complete { > display: none; > } Done.
On 2015/07/14 00:47:24, xunlu wrote: > https://codereview.chromium.org/1193143003/diff/400001/chrome/browser/resourc... > File chrome/browser/resources/options/password_manager.css (right): > > https://codereview.chromium.org/1193143003/diff/400001/chrome/browser/resourc... > chrome/browser/resources/options/password_manager.css:41: } > On 2015/07/13 23:32:43, Evan Stade wrote: > > whitespace in this file is off here and at the end > > Done. > > https://codereview.chromium.org/1193143003/diff/400001/chrome/browser/resourc... > chrome/browser/resources/options/password_manager.css:43: display: inline; > On 2015/07/13 23:32:43, Evan Stade wrote: > > slightly nicer, I think (one rule instead of two): > > > > #password-manager:not(.importing) .import-password-complete { > > display: none; > > } > > Done. (I'm waiting for some sort of designer confirmation before continuing this review.)
xunlu@chromium.org changed reviewers: + engedy@chromium.org
+engedy, since vabr@ is OOO this week can you help take a look? I basically rebased and refactored the code to make it update to date with the codebase. Also some minor changes to address comment#12 in crbug/504138. Thanks, Xun
On 2016/03/01 01:00:27, xunlu wrote: > +engedy, since vabr@ is OOO this week can you help take a look? > > I basically rebased and refactored the code to make it update to date with the > codebase. Also some minor changes to address comment#12 in crbug/504138. > > Thanks, > Xun Hi Xun, Thanks for your work on this! I'm not sure if there is a little misunderstanding, though. The version described in http://crbug.com/504138#c12 is very light on UI: just adds the two buttons, but no other UI. This CL, however, seems to be adding more. I'm not sure the approval to create a testing version for a UI review granted in http://crbug.com/504138#c13 would extend to that additional UI. Also, with the pending switch to the Material Design it might be better to hold on with adding UI until after the MD switch. Cheers, Vaclav
Removed the additional dialog, so the only UI change should be the two buttons.
Thank you, Xun, for your continuing work on this! I left some comments, please have a look. Also, it might be a good idea to make screenshots of how the settings and file dialogues look like at the moment, paste them on the bug and link here, for the convenience of the people reviewing the UI code. Thanks! Vaclav https://codereview.chromium.org/1193143003/diff/500001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1193143003/diff/500001/chrome/app/generated_r... chrome/app/generated_resources.grd:7298: + <message name="IDS_PASSWORD_MANAGER_MORE_BUTTON" desc="Text for the 'More' drop down button in the 'Save password' dialog"> Looks unused. https://codereview.chromium.org/1193143003/diff/500001/chrome/app/generated_r... chrome/app/generated_resources.grd:7301: + <message name="IDS_PASSWORD_MANAGER_SETTINGS_LINK" desc="Text for the link to chrome://settings/passwords in the 'Save password' dialog"> Looks unused. https://codereview.chromium.org/1193143003/diff/500001/chrome/app/generated_r... chrome/app/generated_resources.grd:7304: + <message name="IDS_PASSWORD_MANAGER_IMPORT_BUTTON" desc="Button Text for 'import password' functionality in Password Manger's WebUI option dialog"> I'm afraid this might be too technical a description for the translators. What about the following? desc="The label of the button for importing passwords. The button appears next to the list of saved passwords in Chrome's settings." (Similarly for export.) https://codereview.chromium.org/1193143003/diff/500001/chrome/app/generated_r... chrome/app/generated_resources.grd:7311: + Import Password To Chrome nit: Password -> Passwords (Also for export below). https://codereview.chromium.org/1193143003/diff/500001/chrome/browser/ui/pass... File chrome/browser/ui/passwords/password_manager_presenter.cc (right): https://codereview.chromium.org/1193143003/diff/500001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.cc:183: ret_val.push_back(std::move(copyOfPassword)); nit: A shorter way to write lines 181-183: ret_val.push_back(make_scoped_ptr(new autofill::PasswordForm(*form))); https://codereview.chromium.org/1193143003/diff/500001/chrome/browser/ui/pass... File chrome/browser/ui/passwords/password_ui_view.h (right): https://codereview.chromium.org/1193143003/diff/500001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_ui_view.h:56: To keep the CL smaller and git history cleaner, please revert this formatting change. Only files with significant changes should be kept. https://codereview.chromium.org/1193143003/diff/500001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler.cc (right): https://codereview.chromium.org/1193143003/diff/500001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:23: #include "chrome/browser/ui/ash/ash_util.h" nit: Please move this #ifdefed #include into the section below line 50. https://codereview.chromium.org/1193143003/diff/500001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:351: base::Bind(&PasswordManagerHandler::ImportPasswordFileRead, What happens when the user closes the settings page while the import is in progress? Is it really safe to have Unretained here? https://codereview.chromium.org/1193143003/diff/500001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:360: password_manager::PasswordImporter::SEMANTIC_ERROR); Please do not hard-code the fact that SEMANTIC_ERROR is last here. Also, the boundary value should be strictly greater than the highest possible value. To fix both, please define an additional enum value (NUM_IMPORT_RESULTS), make it the last enum value of Result and use NUM_IMPORT_RESULTS as the boundary value here. See components/password_manager/core/browser/password_manager_metrics_util.h for similar cases. https://codereview.chromium.org/1193143003/diff/500001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:368: password_manager_presenter_->GetPasswordStore(); Could you use PasswordStoreFactory::GetForProfile directly? That seems to make more sense that using the presenter for it (the presenter should take care of presenting the settings page, not providing the Chrome services). If you PasswordStoreFactory::GetForProfile, please also restore the presenter's GetPasswordStore as private again. https://codereview.chromium.org/1193143003/diff/500001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler.h (right): https://codereview.chromium.org/1193143003/diff/500001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.h:33: explicit PasswordManagerHandler(PasswordManagerPresenter* presenter); Please change this from taking a raw pointer to taking a scoped_ptr. That way it will be clear that the constructor takes the ownership. https://codereview.chromium.org/1193143003/diff/500001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.h:33: explicit PasswordManagerHandler(PasswordManagerPresenter* presenter); Also, please mark this constructor as "just for testing". It is just for testing, correct? https://codereview.chromium.org/1193143003/diff/500001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.h:64: friend class ::PasswordManagerHandlerTest; What private access does the test require? If it is just to access the presenter, that can be bypassed in the test (see my comment there). It is generally a bad practice to let the tests access private implementation details of the tests classes, so using a friend declaration should be the very last option to take. https://codereview.chromium.org/1193143003/diff/500001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.h:97: // 3. Writing completes -> completion callback(empty for now); nit: It does not make sense to speak about the unused completion callback here, it's an implementation detail not relevant to the high-level export flow. https://codereview.chromium.org/1193143003/diff/500001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler_unittest.cc (right): https://codereview.chromium.org/1193143003/diff/500001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler_unittest.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. 2016 https://codereview.chromium.org/1193143003/diff/500001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler_unittest.cc:57: DISALLOW_COPY_AND_ASSIGN(DummyPasswordManagerHandler); nit: Please add a blank line above this line. https://codereview.chromium.org/1193143003/diff/500001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler_unittest.cc:73: delete handler_; Why not use scoped_ptr instead of the manual deletions? https://codereview.chromium.org/1193143003/diff/500001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler_unittest.cc:78: UMA_HISTOGRAM_COUNTS("PasswordManager.ImportedPasswordsPerUserInCSV", 0); Could this all happen in the constructor instead of the SetUp? Constructor/desctructor is more enouraged than SetUp/TearDown: https://groups.google.com/a/chromium.org/d/msg/chromium-dev/J2K-2Dmupb8/f27GD... https://codereview.chromium.org/1193143003/diff/500001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler_unittest.cc:92: handler_->password_manager_presenter_.get()); Why do not cache this in the test instead of getting it back through the handler? https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... File components/password_manager/core/browser/export/password_exporter.cc (right): https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... components/password_manager/core/browser/export/password_exporter.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. 2016 https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... components/password_manager/core/browser/export/password_exporter.cc:116: is_extension_valid ? path : path.ReplaceExtension( Do I understand correctly that if the user selects "a.txt" as the target file, then Chrome will instead rewrite the content of "a.csv"? That sounds dangerous! Why do we force the extension on the user? https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... File components/password_manager/core/browser/export/password_exporter.h (right): https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... components/password_manager/core/browser/export/password_exporter.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. 2016 https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... components/password_manager/core/browser/export/password_exporter.h:37: const base::Closure& completion); Is |completion| used for anything right now? If not, I suggest removing it, we can add it once needed. If it is needed for testing, fair enough, let's keep it. https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... components/password_manager/core/browser/export/password_exporter.h:40: static std::vector<std::vector<base::FilePath::StringType>> Why is the type a vector of vectors? Could you either make it just a vector or explain the type in the comment? (Also for the same method of the importer.) https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... File components/password_manager/core/browser/export/password_exporter_unittest.cc (right): https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... components/password_manager/core/browser/export/password_exporter_unittest.cc:1: nit: remove the blank line https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... components/password_manager/core/browser/export/password_exporter_unittest.cc:2: // Copyright 2014 The Chromium Authors. All rights reserved. 2016 https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... components/password_manager/core/browser/export/password_exporter_unittest.cc:92: // Verify that exporter use ".csv" extension when no extension is provided nit: Add a full-stop (.) at the end of the sentences in comments (also below). https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... components/password_manager/core/browser/export/password_exporter_unittest.cc:100: ASSERT_NO_FATAL_FAILURE(StartExportAndWaitUntilCompleteThenReadOutput( (As noted above, this behaviour seems rather risky, we do not want to overwrite the user's files by accident.) https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... File components/password_manager/core/browser/import/password_importer.cc (right): https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... components/password_manager/core/browser/import/password_importer.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. 2016 https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... components/password_manager/core/browser/import/password_importer.cc:66: for (std::vector<ColumnNameToValueMap>::const_iterator it = records.begin(); nit: Consider a range-based for loop. As a bonus, with a range-based for loop you don't need to spell out the vector type here, which in turn might allow you to get rid of the convenience typedef ColumnNameToValueMap (which I would like to see gone, because it both adds a new file, and the need to look it up to understand the types). https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... components/password_manager/core/browser/import/password_importer.cc:120: std::vector<std::string>::const_iterator it = header.begin(); Please consider rewriting this with std::find_if and a lambda function instead of the custom for loop. https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... components/password_manager/core/browser/import/password_importer.cc:171: NOTREACHED() << "Unsupported extension: " << path.Extension(); The extension is user-specified, so it can happen that this else branch is executed. That's not a good place for NOTREACHED, which is only used for conditions which are impossible [1]. While not as risky as in the export case, I'm not sure we need to force the user into using the csv file extension. Could we just not care and import the file anyway? [1] http://dev.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED- https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... File components/password_manager/core/browser/import/password_importer.h (right): https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... components/password_manager/core/browser/import/password_importer.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. 2016 https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... components/password_manager/core/browser/import/password_importer.h:31: // TODO(engedy): Optimize this for performance? This TODO lacks context. Please either drop it or create a bug describing what is the potential issue and the suggested fix, and add a TODO(crbug.com/XXX) linking to that bug. https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... File components/password_manager/core/browser/import/password_importer_unittest.cc (right): https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... components/password_manager/core/browser/import/password_importer_unittest.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. 2016 https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... components/password_manager/core/browser/import/password_importer_unittest.cc:28: // Creates a test input file in a temporary directory with the specified Consider using a ScopedTempDir to keep your testing file(s) instead. That would spare you defining the clean-up yourself. https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... File components/password_manager/core/browser/types.h (right): https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... components/password_manager/core/browser/types.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. 2016 https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... File components/password_manager/core/common/experiments.cc (right): https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... components/password_manager/core/common/experiments.cc:21: bool ImportExportExperimentEnabled() { The call to base::FeatureList is so simple, that I would prefer to inline it instead of creating ImportExportExperimentEnabled. https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... File components/password_manager/core/common/password_manager_features.cc (right): https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... components/password_manager/core/common/password_manager_features.cc:31: "enable-password-import-export", base::FEATURE_DISABLED_BY_DEFAULT}; While the "enable" prefix matches the rest of the features here, it is a bad practice (and we are working on fixing it for the existing ones). The feature is just "password-import-export", enabling or disabling it is an action one can take on any feature. So please drop the "enable" both from the feature name and the constants name. https://codereview.chromium.org/1193143003/diff/500001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1193143003/diff/500001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:34081: + <owner>gcasto@chromium.org</owner> Because Garrett changed projects, please replace his e-mail with vabr@chromium.org in the added histograms.
Patchset #15 (id:520001) has been deleted
xunlu@chromium.org changed reviewers: - gcasto@chromium.org
Thanks for the detailed review, Vaclav. I addressed most of them but there are still a few I wasn't sure about. I replied with my questions or concerns for those, please take a look. https://codereview.chromium.org/1193143003/diff/500001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1193143003/diff/500001/chrome/app/generated_r... chrome/app/generated_resources.grd:7298: + <message name="IDS_PASSWORD_MANAGER_MORE_BUTTON" desc="Text for the 'More' drop down button in the 'Save password' dialog"> On 2016/03/10 10:53:31, vabr (Chromium) wrote: > Looks unused. Done. https://codereview.chromium.org/1193143003/diff/500001/chrome/app/generated_r... chrome/app/generated_resources.grd:7301: + <message name="IDS_PASSWORD_MANAGER_SETTINGS_LINK" desc="Text for the link to chrome://settings/passwords in the 'Save password' dialog"> On 2016/03/10 10:53:31, vabr (Chromium) wrote: > Looks unused. Done. https://codereview.chromium.org/1193143003/diff/500001/chrome/app/generated_r... chrome/app/generated_resources.grd:7304: + <message name="IDS_PASSWORD_MANAGER_IMPORT_BUTTON" desc="Button Text for 'import password' functionality in Password Manger's WebUI option dialog"> On 2016/03/10 10:53:31, vabr (Chromium) wrote: > I'm afraid this might be too technical a description for the translators. What > about the following? > > desc="The label of the button for importing passwords. The button appears next > to the list of saved passwords in Chrome's settings." > > (Similarly for export.) Done. https://codereview.chromium.org/1193143003/diff/500001/chrome/app/generated_r... chrome/app/generated_resources.grd:7311: + Import Password To Chrome On 2016/03/10 10:53:31, vabr (Chromium) wrote: > nit: Password -> Passwords > > (Also for export below). Done. https://codereview.chromium.org/1193143003/diff/500001/chrome/browser/ui/pass... File chrome/browser/ui/passwords/password_manager_presenter.cc (right): https://codereview.chromium.org/1193143003/diff/500001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.cc:183: ret_val.push_back(std::move(copyOfPassword)); On 2016/03/10 10:53:31, vabr (Chromium) wrote: > nit: A shorter way to write lines 181-183: > > ret_val.push_back(make_scoped_ptr(new autofill::PasswordForm(*form))); Done. https://codereview.chromium.org/1193143003/diff/500001/chrome/browser/ui/pass... File chrome/browser/ui/passwords/password_ui_view.h (right): https://codereview.chromium.org/1193143003/diff/500001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_ui_view.h:56: On 2016/03/10 10:53:31, vabr (Chromium) wrote: > To keep the CL smaller and git history cleaner, please revert this formatting > change. Only files with significant changes should be kept. Done. https://codereview.chromium.org/1193143003/diff/500001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler.cc (right): https://codereview.chromium.org/1193143003/diff/500001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:23: #include "chrome/browser/ui/ash/ash_util.h" On 2016/03/10 10:53:31, vabr (Chromium) wrote: > nit: Please move this #ifdefed #include into the section below line 50. Done. https://codereview.chromium.org/1193143003/diff/500001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:351: base::Bind(&PasswordManagerHandler::ImportPasswordFileRead, On 2016/03/10 10:53:31, vabr (Chromium) wrote: > What happens when the user closes the settings page while the import is in > progress? Is it really safe to have Unretained here? Maybe not, but the other options here(https://www.chromium.org/developers/coding-style/important-abstractions-and-data-structures#TOC-base::Callback-and-base::Bind-) don't seem to apply well either. Any suggestion? https://codereview.chromium.org/1193143003/diff/500001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:360: password_manager::PasswordImporter::SEMANTIC_ERROR); On 2016/03/10 10:53:31, vabr (Chromium) wrote: > Please do not hard-code the fact that SEMANTIC_ERROR is last here. Also, the > boundary value should be strictly greater than the highest possible value. > > To fix both, please define an additional enum value (NUM_IMPORT_RESULTS), make > it the last enum value of Result and use NUM_IMPORT_RESULTS as the boundary > value here. See > components/password_manager/core/browser/password_manager_metrics_util.h for > similar cases. Done. https://codereview.chromium.org/1193143003/diff/500001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:368: password_manager_presenter_->GetPasswordStore(); On 2016/03/10 10:53:31, vabr (Chromium) wrote: > Could you use PasswordStoreFactory::GetForProfile directly? That seems to make > more sense that using the presenter for it (the presenter should take care of > presenting the settings page, not providing the Chrome services). If you > PasswordStoreFactory::GetForProfile, please also restore the presenter's > GetPasswordStore as private again. Done. https://codereview.chromium.org/1193143003/diff/500001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler.h (right): https://codereview.chromium.org/1193143003/diff/500001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.h:33: explicit PasswordManagerHandler(PasswordManagerPresenter* presenter); On 2016/03/10 10:53:31, vabr (Chromium) wrote: > Also, please mark this constructor as "just for testing". It is just for > testing, correct? Done. https://codereview.chromium.org/1193143003/diff/500001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.h:33: explicit PasswordManagerHandler(PasswordManagerPresenter* presenter); On 2016/03/10 10:53:32, vabr (Chromium) wrote: > Please change this from taking a raw pointer to taking a scoped_ptr. That way it > will be clear that the constructor takes the ownership. Done. https://codereview.chromium.org/1193143003/diff/500001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.h:64: friend class ::PasswordManagerHandlerTest; On 2016/03/10 10:53:31, vabr (Chromium) wrote: > What private access does the test require? If it is just to access the > presenter, that can be bypassed in the test (see my comment there). > > It is generally a bad practice to let the tests access private implementation > details of the tests classes, so using a friend declaration should be the very > last option to take. not just the presenter. It also need to call the following method in handler which are not public: HandlePasswordExport(private), ImportPasswordFileRead(private), set_web_ui(protected) https://codereview.chromium.org/1193143003/diff/500001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.h:97: // 3. Writing completes -> completion callback(empty for now); On 2016/03/10 10:53:31, vabr (Chromium) wrote: > nit: It does not make sense to speak about the unused completion callback here, > it's an implementation detail not relevant to the high-level export flow. Done. https://codereview.chromium.org/1193143003/diff/500001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler_unittest.cc (right): https://codereview.chromium.org/1193143003/diff/500001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler_unittest.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/03/10 10:53:32, vabr (Chromium) wrote: > 2016 Done. https://codereview.chromium.org/1193143003/diff/500001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler_unittest.cc:57: DISALLOW_COPY_AND_ASSIGN(DummyPasswordManagerHandler); On 2016/03/10 10:53:32, vabr (Chromium) wrote: > nit: Please add a blank line above this line. Done. https://codereview.chromium.org/1193143003/diff/500001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler_unittest.cc:73: delete handler_; On 2016/03/10 10:53:32, vabr (Chromium) wrote: > Why not use scoped_ptr instead of the manual deletions? Doing that would crash the test during the destruction time. Debugged it for hours without success, so I changed it back. https://codereview.chromium.org/1193143003/diff/500001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler_unittest.cc:78: UMA_HISTOGRAM_COUNTS("PasswordManager.ImportedPasswordsPerUserInCSV", 0); On 2016/03/10 10:53:32, vabr (Chromium) wrote: > Could this all happen in the constructor instead of the SetUp? > Constructor/desctructor is more enouraged than SetUp/TearDown: > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/J2K-2Dmupb8/f27GD... Done. https://codereview.chromium.org/1193143003/diff/500001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler_unittest.cc:92: handler_->password_manager_presenter_.get()); On 2016/03/10 10:53:32, vabr (Chromium) wrote: > Why do not cache this in the test instead of getting it back through the > handler? Done. https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... File components/password_manager/core/browser/export/password_exporter.cc (right): https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... components/password_manager/core/browser/export/password_exporter.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2016/03/10 10:53:32, vabr (Chromium) wrote: > 2016 Done. https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... components/password_manager/core/browser/export/password_exporter.cc:116: is_extension_valid ? path : path.ReplaceExtension( On 2016/03/10 10:53:32, vabr (Chromium) wrote: > Do I understand correctly that if the user selects "a.txt" as the target file, > then Chrome will instead rewrite the content of "a.csv"? That sounds dangerous! > Why do we force the extension on the user? In the exporter dialog UI there is a drop down for output type and we currently only have CSV there. So the user really should only expect the output to be CSV. If we allow them to specify what ever extension they want and they specify something like "json" but it's actually CSV inside, there will be some confusions. I'll leave it for you to decide. https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... File components/password_manager/core/browser/export/password_exporter.h (right): https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... components/password_manager/core/browser/export/password_exporter.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2016/03/10 10:53:32, vabr (Chromium) wrote: > 2016 Done. https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... components/password_manager/core/browser/export/password_exporter.h:37: const base::Closure& completion); On 2016/03/10 10:53:32, vabr (Chromium) wrote: > Is |completion| used for anything right now? If not, I suggest removing it, we > can add it once needed. If it is needed for testing, fair enough, let's keep it. I removed it https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... components/password_manager/core/browser/export/password_exporter.h:40: static std::vector<std::vector<base::FilePath::StringType>> On 2016/03/10 10:53:32, vabr (Chromium) wrote: > Why is the type a vector of vectors? Could you either make it just a vector or > explain the type in the comment? > > (Also for the same method of the importer.) Comments updated. https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... File components/password_manager/core/browser/export/password_exporter_unittest.cc (right): https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... components/password_manager/core/browser/export/password_exporter_unittest.cc:1: On 2016/03/10 10:53:32, vabr (Chromium) wrote: > nit: remove the blank line Done. https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... components/password_manager/core/browser/export/password_exporter_unittest.cc:2: // Copyright 2014 The Chromium Authors. All rights reserved. On 2016/03/10 10:53:32, vabr (Chromium) wrote: > 2016 Done. https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... components/password_manager/core/browser/export/password_exporter_unittest.cc:92: // Verify that exporter use ".csv" extension when no extension is provided On 2016/03/10 10:53:32, vabr (Chromium) wrote: > nit: Add a full-stop (.) at the end of the sentences in comments (also below). Done. https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... components/password_manager/core/browser/export/password_exporter_unittest.cc:100: ASSERT_NO_FATAL_FAILURE(StartExportAndWaitUntilCompleteThenReadOutput( On 2016/03/10 10:53:32, vabr (Chromium) wrote: > (As noted above, this behaviour seems rather risky, we do not want to overwrite > the user's files by accident.) See my comments above. https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... File components/password_manager/core/browser/import/password_importer.cc (right): https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... components/password_manager/core/browser/import/password_importer.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2016/03/10 10:53:32, vabr (Chromium) wrote: > 2016 Done. https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... components/password_manager/core/browser/import/password_importer.cc:66: for (std::vector<ColumnNameToValueMap>::const_iterator it = records.begin(); On 2016/03/10 10:53:32, vabr (Chromium) wrote: > nit: Consider a range-based for loop. > > As a bonus, with a range-based for loop you don't need to spell out the vector > type here, which in turn might allow you to get rid of the convenience typedef > ColumnNameToValueMap (which I would like to see gone, because it both adds a new > file, and the need to look it up to understand the types). Done. https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... components/password_manager/core/browser/import/password_importer.cc:120: std::vector<std::string>::const_iterator it = header.begin(); On 2016/03/10 10:53:32, vabr (Chromium) wrote: > Please consider rewriting this with std::find_if and a lambda function instead > of the custom for loop. Done. https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... components/password_manager/core/browser/import/password_importer.cc:171: NOTREACHED() << "Unsupported extension: " << path.Extension(); On 2016/03/10 10:53:32, vabr (Chromium) wrote: > The extension is user-specified, so it can happen that this else branch is > executed. That's not a good place for NOTREACHED, which is only used for > conditions which are impossible [1]. > > While not as risky as in the export case, I'm not sure we need to force the user > into using the csv file extension. Could we just not care and import the file > anyway? > > [1] > http://dev.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED- Same concern as I explained in the Exporter comments. Up to you. https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... File components/password_manager/core/browser/import/password_importer.h (right): https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... components/password_manager/core/browser/import/password_importer.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2016/03/10 10:53:32, vabr (Chromium) wrote: > 2016 Done. https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... components/password_manager/core/browser/import/password_importer.h:31: // TODO(engedy): Optimize this for performance? On 2016/03/10 10:53:32, vabr (Chromium) wrote: > This TODO lacks context. Please either drop it or create a bug describing what > is the potential issue and the suggested fix, and add a TODO(crbug.com/XXX) > linking to that bug. Done. https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... File components/password_manager/core/browser/import/password_importer_unittest.cc (right): https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... components/password_manager/core/browser/import/password_importer_unittest.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2016/03/10 10:53:32, vabr (Chromium) wrote: > 2016 Done. https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... components/password_manager/core/browser/import/password_importer_unittest.cc:28: // Creates a test input file in a temporary directory with the specified On 2016/03/10 10:53:32, vabr (Chromium) wrote: > Consider using a ScopedTempDir to keep your testing file(s) instead. That would > spare you defining the clean-up yourself. Done. https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... File components/password_manager/core/browser/types.h (right): https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... components/password_manager/core/browser/types.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/03/10 10:53:33, vabr (Chromium) wrote: > 2016 Deleted this file. https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... File components/password_manager/core/common/experiments.cc (right): https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... components/password_manager/core/common/experiments.cc:21: bool ImportExportExperimentEnabled() { On 2016/03/10 10:53:33, vabr (Chromium) wrote: > The call to base::FeatureList is so simple, that I would prefer to inline it > instead of creating ImportExportExperimentEnabled. Done. https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... File components/password_manager/core/common/password_manager_features.cc (right): https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... components/password_manager/core/common/password_manager_features.cc:31: "enable-password-import-export", base::FEATURE_DISABLED_BY_DEFAULT}; On 2016/03/10 10:53:33, vabr (Chromium) wrote: > While the "enable" prefix matches the rest of the features here, it is a bad > practice (and we are working on fixing it for the existing ones). > > The feature is just "password-import-export", enabling or disabling it is an > action one can take on any feature. So please drop the "enable" both from the > feature name and the constants name. Done. https://codereview.chromium.org/1193143003/diff/500001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1193143003/diff/500001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:34081: + <owner>gcasto@chromium.org</owner> On 2016/03/10 10:53:33, vabr (Chromium) wrote: > Because Garrett changed projects, please replace his e-mail with > mailto:vabr@chromium.org in the added histograms. Done.
Thank you, Xun, for the changes and also for the replies. I tried to respond with some advice, please have a look. Cheers, Vaclav https://codereview.chromium.org/1193143003/diff/500001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler.cc (right): https://codereview.chromium.org/1193143003/diff/500001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:351: base::Bind(&PasswordManagerHandler::ImportPasswordFileRead, On 2016/03/16 07:23:58, xunlu wrote: > On 2016/03/10 10:53:31, vabr (Chromium) wrote: > > What happens when the user closes the settings page while the import is in > > progress? Is it really safe to have Unretained here? > > Maybe not, but the other options > here(https://www.chromium.org/developers/coding-style/important-abstractions-and-data-structures#TOC-base::Callback-and-base::Bind-) > don't seem to apply well either. Any suggestion? My suggestion is to split the functionality: Instead of PasswordManagerHandler taking care of executing ImportPasswordFileRead directly, this functionality should be transferred to a separate object (called, e.g., ImportPasswordResultConsumer, or some similar name). This object should be refcounted, created by PasswordManagerHandler in a local scoped_refptr within ImportPasswordFileSelected, and then just passed into the Bind call. The bind call will increase the refcount, making sure that the object survives the destruction of PasswordManagerHandler, and finishes the import. https://codereview.chromium.org/1193143003/diff/500001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler.h (right): https://codereview.chromium.org/1193143003/diff/500001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.h:64: friend class ::PasswordManagerHandlerTest; On 2016/03/16 07:23:58, xunlu wrote: > On 2016/03/10 10:53:31, vabr (Chromium) wrote: > > What private access does the test require? If it is just to access the > > presenter, that can be bypassed in the test (see my comment there). > > > > It is generally a bad practice to let the tests access private implementation > > details of the tests classes, so using a friend declaration should be the very > > last option to take. > > not just the presenter. It also need to call the following method in handler > which are not public: > HandlePasswordExport(private), > ImportPasswordFileRead(private), > set_web_ui(protected) > You should be able to call HandlePasswordExport through ProcessWebUIMessage with the message "updatePasswordLists". That will use the same path the production code clients would use. ImportPasswordFileRead should get solved if you split the functionality into the separate object, as I suggested in the other comment. As for set_web_ui, the comment in web_ui_message_handler.h says: "exposed to subclasses for testing purposes". So the right thing to do there would be to create a trivial child of the handler in the test, and expose the setter: class TestPasswordManagerHandler : public options::PasswordManagerHandler { public: using set_web_ui; }; In summary, it looks like the friend declaration is not needed. Please try to avoid it, even if the initial use were innocent, it tends to invite bad test design as the time goes. https://codereview.chromium.org/1193143003/diff/500001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler_unittest.cc (right): https://codereview.chromium.org/1193143003/diff/500001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler_unittest.cc:73: delete handler_; On 2016/03/16 07:23:58, xunlu wrote: > On 2016/03/10 10:53:32, vabr (Chromium) wrote: > > Why not use scoped_ptr instead of the manual deletions? > Doing that would crash the test during the destruction time. Debugged it for > hours without success, so I changed it back. Just looking at the code, I can tell that handler_ depends on presenter_, which depends on dummy_handler_. So the necessary order of destruction is in reverse of the dependencies, first the handler_ (which apparently owns the presenter_, so that's solved for us) and then the dummy_handler_. If you kept the member variable order as it is now, the desctruction order of the scoped_ptrs was wrong. Therefore, please define them in this order at the end of the member variable list: scoped_ptr<DummyPasswordManagerHandler> dummy_handler_; scoped_ptr<options::PasswordManagerHandler> handler_; That is equivalent to the current destructor, so should work OK. Please try doing that instead of the manual deletes -- does it still crash? I really don't think it is a good idea to have manual deletions here, especially just because the code crashes for an unknown reason otherwise (that sounds scary by itself). https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... File components/password_manager/core/browser/export/password_exporter.cc (right): https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... components/password_manager/core/browser/export/password_exporter.cc:116: is_extension_valid ? path : path.ReplaceExtension( On 2016/03/16 07:23:59, xunlu wrote: > On 2016/03/10 10:53:32, vabr (Chromium) wrote: > > Do I understand correctly that if the user selects "a.txt" as the target file, > > then Chrome will instead rewrite the content of "a.csv"? That sounds > dangerous! > > Why do we force the extension on the user? > > In the exporter dialog UI there is a drop down for output type and we currently > only have CSV there. So the user really should only expect the output to be CSV. > If we allow them to specify what ever extension they want and they specify > something like "json" but it's actually CSV inside, there will be some > confusions. > > I'll leave it for you to decide. Confusions are one thing, but an overwritten content sounds worse. While I do not understand completely, whether the drop-down with extensions only having CSV means that the user cannot return a file name with another extension, or that it just will be harder (need to type it or something like that), I still think not changing the name here is the safe option: in the former case there is no difference, and in the latter my original concern with overwriting files applies. https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... File components/password_manager/core/browser/import/password_importer.cc (right): https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... components/password_manager/core/browser/import/password_importer.cc:66: for (std::vector<ColumnNameToValueMap>::const_iterator it = records.begin(); On 2016/03/16 07:23:59, xunlu wrote: > On 2016/03/10 10:53:32, vabr (Chromium) wrote: > > nit: Consider a range-based for loop. > > > > As a bonus, with a range-based for loop you don't need to spell out the vector > > type here, which in turn might allow you to get rid of the convenience typedef > > ColumnNameToValueMap (which I would like to see gone, because it both adds a > new > > file, and the need to look it up to understand the types). > > Done. for (int i = 0...) is not a range-based for loop. What I meant is: for (const auto& record : records) { PasswordForm form; if (RecordToPasswordForm(record, &form)) ... } https://codereview.chromium.org/1193143003/diff/540001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler_unittest.cc (right): https://codereview.chromium.org/1193143003/diff/540001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler_unittest.cc:83: presenter_.reset(new MockPasswordManagerPresenter(dummy_handler_)); Looks like presenter_ is only used within the constructor, so perhaps it should be a local variable and not a data member? https://codereview.chromium.org/1193143003/diff/540001/components/password_ma... File components/password_manager/core/browser/import/password_importer.cc (right): https://codereview.chromium.org/1193143003/diff/540001/components/password_ma... components/password_manager/core/browser/import/password_importer.cc:119: std::vector<std::string>::const_iterator it = nit: Don't be afraid to use "auto it" instead of the full type. The type is clear from the surrounding lines, and less spam in the code makes the rest more readable. https://codereview.chromium.org/1193143003/diff/540001/components/password_ma... components/password_manager/core/browser/import/password_importer.cc:120: std::find_if(header.begin(), header.end(), [&options](std::string str) { std::string -> const std::string& https://codereview.chromium.org/1193143003/diff/540001/components/password_ma... File components/password_manager/core/browser/import/password_importer_unittest.cc (right): https://codereview.chromium.org/1193143003/diff/540001/components/password_ma... components/password_manager/core/browser/import/password_importer_unittest.cc:35: void SetUp() override { ASSERT_TRUE(temp_directory_.CreateUniqueTempDir()); } Please add this to the constructor instead. SetUp is discouraged, according to https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/Initia... https://codereview.chromium.org/1193143003/diff/540001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1193143003/diff/540001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:76104: +<enum name="PasswordGenerationSubmissionEvent" type="int"> Is this enum included by accident?
https://codereview.chromium.org/1193143003/diff/500001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler.cc (right): https://codereview.chromium.org/1193143003/diff/500001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:351: base::Bind(&PasswordManagerHandler::ImportPasswordFileRead, On 2016/03/16 17:48:28, vabr (Chromium) wrote: > On 2016/03/16 07:23:58, xunlu wrote: > > On 2016/03/10 10:53:31, vabr (Chromium) wrote: > > > What happens when the user closes the settings page while the import is in > > > progress? Is it really safe to have Unretained here? > > > > Maybe not, but the other options > > > here(https://www.chromium.org/developers/coding-style/important-abstractions-and-data-structures#TOC-base::Callback-and-base::Bind-) > > don't seem to apply well either. Any suggestion? > > My suggestion is to split the functionality: Instead of PasswordManagerHandler > taking care of executing ImportPasswordFileRead directly, this functionality > should be transferred to a separate object (called, e.g., > ImportPasswordResultConsumer, or some similar name). This object should be > refcounted, created by PasswordManagerHandler in a local scoped_refptr within > ImportPasswordFileSelected, and then just passed into the Bind call. The bind > call will increase the refcount, making sure that the object survives the > destruction of PasswordManagerHandler, and finishes the import. There is a problem: ImportPasswordFileRead needs to use PasswordStore. And getting the PasswordStore requires the Profile(e.g. https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...). So unless the Profile object can outlive the handler object, splitting the functionality is not going to help. I'm not sure about the life cycle of Profile, can you confirm? https://codereview.chromium.org/1193143003/diff/500001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler.h (right): https://codereview.chromium.org/1193143003/diff/500001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.h:64: friend class ::PasswordManagerHandlerTest; On 2016/03/16 17:48:28, vabr (Chromium) wrote: > On 2016/03/16 07:23:58, xunlu wrote: > > On 2016/03/10 10:53:31, vabr (Chromium) wrote: > > > What private access does the test require? If it is just to access the > > > presenter, that can be bypassed in the test (see my comment there). > > > > > > It is generally a bad practice to let the tests access private > implementation > > > details of the tests classes, so using a friend declaration should be the > very > > > last option to take. > > > > not just the presenter. It also need to call the following method in handler > > which are not public: > > HandlePasswordExport(private), > > ImportPasswordFileRead(private), > > set_web_ui(protected) > > > > You should be able to call HandlePasswordExport through ProcessWebUIMessage with > the message "updatePasswordLists". That will use the same path the production > code clients would use. > > ImportPasswordFileRead should get solved if you split the functionality into the > separate object, as I suggested in the other comment. > > As for set_web_ui, the comment in web_ui_message_handler.h says: "exposed to > subclasses for testing purposes". So the right thing to do there would be to > create a trivial child of the handler in the test, and expose the setter: > > class TestPasswordManagerHandler : public options::PasswordManagerHandler { > public: > using set_web_ui; > }; > > In summary, it looks like the friend declaration is not needed. Please try to > avoid it, even if the initial use were innocent, it tends to invite bad test > design as the time goes. * HandlePasswordExport() call removed. * ImportPasswordFileRead() still pending, see my comment in password_manager_handler.cc. * set_web_ui() removed. https://codereview.chromium.org/1193143003/diff/500001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler_unittest.cc (right): https://codereview.chromium.org/1193143003/diff/500001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler_unittest.cc:73: delete handler_; On 2016/03/16 17:48:28, vabr (Chromium) wrote: > On 2016/03/16 07:23:58, xunlu wrote: > > On 2016/03/10 10:53:32, vabr (Chromium) wrote: > > > Why not use scoped_ptr instead of the manual deletions? > > Doing that would crash the test during the destruction time. Debugged it for > > hours without success, so I changed it back. > > Just looking at the code, I can tell that handler_ depends on presenter_, which > depends on dummy_handler_. So the necessary order of destruction is in reverse > of the dependencies, first the handler_ (which apparently owns the presenter_, > so that's solved for us) and then the dummy_handler_. If you kept the member > variable order as it is now, the desctruction order of the scoped_ptrs was > wrong. > > Therefore, please define them in this order at the end of the member variable > list: > > scoped_ptr<DummyPasswordManagerHandler> dummy_handler_; > scoped_ptr<options::PasswordManagerHandler> handler_; > > That is equivalent to the current destructor, so should work OK. > > Please try doing that instead of the manual deletes -- does it still crash? I > really don't think it is a good idea to have manual deletions here, especially > just because the code crashes for an unknown reason otherwise (that sounds scary > by itself). It's working fine now. Thanks for the tip! https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... File components/password_manager/core/browser/export/password_exporter.cc (right): https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... components/password_manager/core/browser/export/password_exporter.cc:116: is_extension_valid ? path : path.ReplaceExtension( On 2016/03/16 17:48:28, vabr (Chromium) wrote: > On 2016/03/16 07:23:59, xunlu wrote: > > On 2016/03/10 10:53:32, vabr (Chromium) wrote: > > > Do I understand correctly that if the user selects "a.txt" as the target > file, > > > then Chrome will instead rewrite the content of "a.csv"? That sounds > > dangerous! > > > Why do we force the extension on the user? > > > > In the exporter dialog UI there is a drop down for output type and we > currently > > only have CSV there. So the user really should only expect the output to be > CSV. > > If we allow them to specify what ever extension they want and they specify > > something like "json" but it's actually CSV inside, there will be some > > confusions. > > > > I'll leave it for you to decide. > > Confusions are one thing, but an overwritten content sounds worse. > While I do not understand completely, whether the drop-down with extensions only > having CSV means that the user cannot return a file name with another extension, > or that it just will be harder (need to type it or something like that), I still > think not changing the name here is the safe option: in the former case there is > no difference, and in the latter my original concern with overwriting files > applies. I vaguely remember that we were going to use the extension to determine the proper writer. But since we only have CSVWriter now it's probably fine. So I'm going to remove the is_extension_valid logic as you suggested. Should you need to support more output format in the future, please update this part accordingly. https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... File components/password_manager/core/browser/import/password_importer.cc (right): https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... components/password_manager/core/browser/import/password_importer.cc:66: for (std::vector<ColumnNameToValueMap>::const_iterator it = records.begin(); On 2016/03/16 17:48:28, vabr (Chromium) wrote: > On 2016/03/16 07:23:59, xunlu wrote: > > On 2016/03/10 10:53:32, vabr (Chromium) wrote: > > > nit: Consider a range-based for loop. > > > > > > As a bonus, with a range-based for loop you don't need to spell out the > vector > > > type here, which in turn might allow you to get rid of the convenience > typedef > > > ColumnNameToValueMap (which I would like to see gone, because it both adds a > > new > > > file, and the need to look it up to understand the types). > > > > Done. > > for (int i = 0...) is not a range-based for loop. What I meant is: > > for (const auto& record : records) { > PasswordForm form; > if (RecordToPasswordForm(record, &form)) > ... > } Done. https://codereview.chromium.org/1193143003/diff/540001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler_unittest.cc (right): https://codereview.chromium.org/1193143003/diff/540001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler_unittest.cc:83: presenter_.reset(new MockPasswordManagerPresenter(dummy_handler_)); On 2016/03/16 17:48:28, vabr (Chromium) wrote: > Looks like presenter_ is only used within the constructor, so perhaps it should > be a local variable and not a data member? Done. https://codereview.chromium.org/1193143003/diff/540001/components/password_ma... File components/password_manager/core/browser/import/password_importer.cc (right): https://codereview.chromium.org/1193143003/diff/540001/components/password_ma... components/password_manager/core/browser/import/password_importer.cc:119: std::vector<std::string>::const_iterator it = On 2016/03/16 17:48:28, vabr (Chromium) wrote: > nit: Don't be afraid to use "auto it" instead of the full type. The type is > clear from the surrounding lines, and less spam in the code makes the rest more > readable. Done. https://codereview.chromium.org/1193143003/diff/540001/components/password_ma... components/password_manager/core/browser/import/password_importer.cc:120: std::find_if(header.begin(), header.end(), [&options](std::string str) { On 2016/03/16 17:48:28, vabr (Chromium) wrote: > std::string -> const std::string& Done. https://codereview.chromium.org/1193143003/diff/540001/components/password_ma... File components/password_manager/core/browser/import/password_importer_unittest.cc (right): https://codereview.chromium.org/1193143003/diff/540001/components/password_ma... components/password_manager/core/browser/import/password_importer_unittest.cc:35: void SetUp() override { ASSERT_TRUE(temp_directory_.CreateUniqueTempDir()); } On 2016/03/16 17:48:28, vabr (Chromium) wrote: > Please add this to the constructor instead. SetUp is discouraged, according to > https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/Initia... Seems that ASSERT_TRUE cannot be used in constructor as it can return void: ../../components/password_manager/core/browser/import/password_importer_unittest.cc:34:5: error: constructor 'PasswordImporterTest' must not return void expression ASSERT_TRUE(temp_directory_.CreateUniqueTempDir()); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ../../testing/gtest/include/gtest/gtest.h:1871:23: note: expanded from macro 'ASSERT_TRUE' GTEST_FATAL_FAILURE_) ^~~~~~~~~~~~~~~~~~~~~ ../../testing/gtest/include/gtest/internal/gtest-internal.h:1192:5: note: expanded from macro 'GTEST_TEST_BOOLEAN_' fail(::testing::internal::GetBoolAssertionFailureMessage(\ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ../../testing/gtest/include/gtest/internal/gtest-internal.h:1110:3: note: expanded from macro 'GTEST_FATAL_FAILURE_' return GTEST_MESSAGE_(message, ::testing::TestPartResult::kFatalFailure) ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ https://codereview.chromium.org/1193143003/diff/540001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1193143003/diff/540001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:76104: +<enum name="PasswordGenerationSubmissionEvent" type="int"> On 2016/03/16 17:48:28, vabr (Chromium) wrote: > Is this enum included by accident? no idea. Removed.
Thank you, Xun! Some responses and comments below. Cheers, Vaclav https://codereview.chromium.org/1193143003/diff/500001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler.cc (right): https://codereview.chromium.org/1193143003/diff/500001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:351: base::Bind(&PasswordManagerHandler::ImportPasswordFileRead, On 2016/03/18 21:15:30, xunlu wrote: > On 2016/03/16 17:48:28, vabr (Chromium) wrote: > > On 2016/03/16 07:23:58, xunlu wrote: > > > On 2016/03/10 10:53:31, vabr (Chromium) wrote: > > > > What happens when the user closes the settings page while the import is in > > > > progress? Is it really safe to have Unretained here? > > > > > > Maybe not, but the other options > > > > > > here(https://www.chromium.org/developers/coding-style/important-abstractions-and-data-structures#TOC-base::Callback-and-base::Bind-) > > > don't seem to apply well either. Any suggestion? > > > > My suggestion is to split the functionality: Instead of PasswordManagerHandler > > taking care of executing ImportPasswordFileRead directly, this functionality > > should be transferred to a separate object (called, e.g., > > ImportPasswordResultConsumer, or some similar name). This object should be > > refcounted, created by PasswordManagerHandler in a local scoped_refptr within > > ImportPasswordFileSelected, and then just passed into the Bind call. The bind > > call will increase the refcount, making sure that the object survives the > > destruction of PasswordManagerHandler, and finishes the import. > > There is a problem: > ImportPasswordFileRead needs to use PasswordStore. And getting the PasswordStore > requires the Profile(e.g. > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...). > So unless the Profile object can outlive the handler object, splitting the > functionality is not going to help. I'm not sure about the life cycle of > Profile, can you confirm? Yes, Profile definitely outlives the handler object. It represents the whole Chrome profile, and outlives all Chrome windows for that profile. The worst which can happen is that the helper object will ask for the password store and will get a null pointer, because the user just closed the very last tab, and Chrome is shutting down its services (including PasswordStore). In that case the import will fail, but the code should not crash. https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... File components/password_manager/core/browser/export/password_exporter.cc (right): https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... components/password_manager/core/browser/export/password_exporter.cc:116: is_extension_valid ? path : path.ReplaceExtension( On 2016/03/18 21:15:30, xunlu wrote: > On 2016/03/16 17:48:28, vabr (Chromium) wrote: > > On 2016/03/16 07:23:59, xunlu wrote: > > > On 2016/03/10 10:53:32, vabr (Chromium) wrote: > > > > Do I understand correctly that if the user selects "a.txt" as the target > > file, > > > > then Chrome will instead rewrite the content of "a.csv"? That sounds > > > dangerous! > > > > Why do we force the extension on the user? > > > > > > In the exporter dialog UI there is a drop down for output type and we > > currently > > > only have CSV there. So the user really should only expect the output to be > > CSV. > > > If we allow them to specify what ever extension they want and they specify > > > something like "json" but it's actually CSV inside, there will be some > > > confusions. > > > > > > I'll leave it for you to decide. > > > > Confusions are one thing, but an overwritten content sounds worse. > > While I do not understand completely, whether the drop-down with extensions > only > > having CSV means that the user cannot return a file name with another > extension, > > or that it just will be harder (need to type it or something like that), I > still > > think not changing the name here is the safe option: in the former case there > is > > no difference, and in the latter my original concern with overwriting files > > applies. > > I vaguely remember that we were going to use the extension to determine the > proper writer. But since we only have CSVWriter now it's probably fine. So I'm > going to remove the is_extension_valid logic as you suggested. Should you need > to support more output format in the future, please update this part > accordingly. I acknowledge that if support for other formats is added, then the code will need appropriate changes. Suggesting the .csv extension as the default choice to the user sounds fine, but the code should avoid too much generality and complexity for handling multiple extensions, until there is a real need to handle multiple extensions. https://codereview.chromium.org/1193143003/diff/540001/components/password_ma... File components/password_manager/core/browser/import/password_importer_unittest.cc (right): https://codereview.chromium.org/1193143003/diff/540001/components/password_ma... components/password_manager/core/browser/import/password_importer_unittest.cc:35: void SetUp() override { ASSERT_TRUE(temp_directory_.CreateUniqueTempDir()); } On 2016/03/18 21:15:30, xunlu wrote: > On 2016/03/16 17:48:28, vabr (Chromium) wrote: > > Please add this to the constructor instead. SetUp is discouraged, according to > > > https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/Initia... > > Seems that ASSERT_TRUE cannot be used in constructor as it can return void: > > ../../components/password_manager/core/browser/import/password_importer_unittest.cc:34:5: > error: constructor 'PasswordImporterTest' must not return void expression > ASSERT_TRUE(temp_directory_.CreateUniqueTempDir()); > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > ../../testing/gtest/include/gtest/gtest.h:1871:23: note: expanded from macro > 'ASSERT_TRUE' > GTEST_FATAL_FAILURE_) > ^~~~~~~~~~~~~~~~~~~~~ > ../../testing/gtest/include/gtest/internal/gtest-internal.h:1192:5: note: > expanded from macro 'GTEST_TEST_BOOLEAN_' > fail(::testing::internal::GetBoolAssertionFailureMessage(\ > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > ../../testing/gtest/include/gtest/internal/gtest-internal.h:1110:3: note: > expanded from macro 'GTEST_FATAL_FAILURE_' > return GTEST_MESSAGE_(message, ::testing::TestPartResult::kFatalFailure) > ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ I think it is fine to change ASSERT_TRUE to CHECK here. Avoiding the SetUp in favour of the constructor is more important. While we need to think twice about CHECKs in production code (use restricted by the style guide to only security reasons), here in the test it only matters whether the resulting error message and behaviour are comparable. Both ASSERT_TRUE and CHECK will indicate which assertion failed, and both will crash the test, so there is not much difference. https://codereview.chromium.org/1193143003/diff/560001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler_unittest.cc (right): https://codereview.chromium.org/1193143003/diff/560001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler_unittest.cc:34: }; nit: Please add one blank line to separate the class definition from the method definition (also between lines 39 and 40). https://codereview.chromium.org/1193143003/diff/560001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler_unittest.cc:128: ~PasswordManagerHandlerTest() override {} Nit: please add a blank line before the destructor. https://codereview.chromium.org/1193143003/diff/560001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler_unittest.cc:138: base::ListValue* tmp = new base::ListValue(); No need to create ListValue on the heap, just create it on the stack: base::ListValue tmp; web_ui_.ProcessWebUIMessage(GURL(), "exportPassword", tmp); (If you needed a heap-allocated one, then you should put it into a scoped_ptr.) https://codereview.chromium.org/1193143003/diff/560001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler_unittest.cc:139: web_ui_.ProcessWebUIMessage(GURL(""), "exportPassword", *tmp); Please replace GURL("") with just GURL(), that's faster than parsing the empty string. https://codereview.chromium.org/1193143003/diff/560001/components/password_ma... File components/password_manager/core/browser/export/password_exporter.cc (right): https://codereview.chromium.org/1193143003/diff/560001/components/password_ma... components/password_manager/core/browser/export/password_exporter.cc:104: path.Extension().substr(1) == PasswordCSVWriter::kFileExtension) { I suggest going even further and just creating the PasswordCSVWriter unconditionally, with a comment saying something like: "CSV is the only currently supported format." There is no point in checking the extension and ignoring the result just so that once we potentially add another supported format, we will spare about 3 lines of code in that CL. I suggest the same for the analogous part of handling the reader in the importer. https://codereview.chromium.org/1193143003/diff/560001/components/password_ma... File components/password_manager/core/browser/import/password_importer.cc (right): https://codereview.chromium.org/1193143003/diff/560001/components/password_ma... components/password_manager/core/browser/import/password_importer.cc:120: [&options](const std::string str) { Please make this a reference to a const string, not just value-passed const string. That will save the copy: const std::string& str
Patchset #17 (id:580001) has been deleted
xunlu@chromium.org changed reviewers: - engedy@chromium.org
https://codereview.chromium.org/1193143003/diff/500001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler.cc (right): https://codereview.chromium.org/1193143003/diff/500001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:351: base::Bind(&PasswordManagerHandler::ImportPasswordFileRead, On 2016/03/21 14:01:30, vabr (Chromium) wrote: > On 2016/03/18 21:15:30, xunlu wrote: > > On 2016/03/16 17:48:28, vabr (Chromium) wrote: > > > On 2016/03/16 07:23:58, xunlu wrote: > > > > On 2016/03/10 10:53:31, vabr (Chromium) wrote: > > > > > What happens when the user closes the settings page while the import is > in > > > > > progress? Is it really safe to have Unretained here? > > > > > > > > Maybe not, but the other options > > > > > > > > > > here(https://www.chromium.org/developers/coding-style/important-abstractions-and-data-structures#TOC-base::Callback-and-base::Bind-) > > > > don't seem to apply well either. Any suggestion? > > > > > > My suggestion is to split the functionality: Instead of > PasswordManagerHandler > > > taking care of executing ImportPasswordFileRead directly, this functionality > > > should be transferred to a separate object (called, e.g., > > > ImportPasswordResultConsumer, or some similar name). This object should be > > > refcounted, created by PasswordManagerHandler in a local scoped_refptr > within > > > ImportPasswordFileSelected, and then just passed into the Bind call. The > bind > > > call will increase the refcount, making sure that the object survives the > > > destruction of PasswordManagerHandler, and finishes the import. > > > > There is a problem: > > ImportPasswordFileRead needs to use PasswordStore. And getting the > PasswordStore > > requires the Profile(e.g. > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...). > > So unless the Profile object can outlive the handler object, splitting the > > functionality is not going to help. I'm not sure about the life cycle of > > Profile, can you confirm? > > Yes, Profile definitely outlives the handler object. It represents the whole > Chrome profile, and outlives all Chrome windows for that profile. > > The worst which can happen is that the helper object will ask for the password > store and will get a null pointer, because the user just closed the very last > tab, and Chrome is shutting down its services (including PasswordStore). In that > case the import will fail, but the code should not crash. Done. https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... File components/password_manager/core/browser/export/password_exporter.cc (right): https://codereview.chromium.org/1193143003/diff/500001/components/password_ma... components/password_manager/core/browser/export/password_exporter.cc:116: is_extension_valid ? path : path.ReplaceExtension( On 2016/03/21 14:01:30, vabr (Chromium) wrote: > On 2016/03/18 21:15:30, xunlu wrote: > > On 2016/03/16 17:48:28, vabr (Chromium) wrote: > > > On 2016/03/16 07:23:59, xunlu wrote: > > > > On 2016/03/10 10:53:32, vabr (Chromium) wrote: > > > > > Do I understand correctly that if the user selects "a.txt" as the target > > > file, > > > > > then Chrome will instead rewrite the content of "a.csv"? That sounds > > > > dangerous! > > > > > Why do we force the extension on the user? > > > > > > > > In the exporter dialog UI there is a drop down for output type and we > > > currently > > > > only have CSV there. So the user really should only expect the output to > be > > > CSV. > > > > If we allow them to specify what ever extension they want and they specify > > > > something like "json" but it's actually CSV inside, there will be some > > > > confusions. > > > > > > > > I'll leave it for you to decide. > > > > > > Confusions are one thing, but an overwritten content sounds worse. > > > While I do not understand completely, whether the drop-down with extensions > > only > > > having CSV means that the user cannot return a file name with another > > extension, > > > or that it just will be harder (need to type it or something like that), I > > still > > > think not changing the name here is the safe option: in the former case > there > > is > > > no difference, and in the latter my original concern with overwriting files > > > applies. > > > > I vaguely remember that we were going to use the extension to determine the > > proper writer. But since we only have CSVWriter now it's probably fine. So I'm > > going to remove the is_extension_valid logic as you suggested. Should you need > > to support more output format in the future, please update this part > > accordingly. > > I acknowledge that if support for other formats is added, then the code will > need appropriate changes. > Suggesting the .csv extension as the default choice to the user sounds fine, but > the code should avoid too much generality and complexity for handling multiple > extensions, until there is a real need to handle multiple extensions. Acknowledged. https://codereview.chromium.org/1193143003/diff/540001/components/password_ma... File components/password_manager/core/browser/import/password_importer_unittest.cc (right): https://codereview.chromium.org/1193143003/diff/540001/components/password_ma... components/password_manager/core/browser/import/password_importer_unittest.cc:35: void SetUp() override { ASSERT_TRUE(temp_directory_.CreateUniqueTempDir()); } On 2016/03/21 14:01:30, vabr (Chromium) wrote: > On 2016/03/18 21:15:30, xunlu wrote: > > On 2016/03/16 17:48:28, vabr (Chromium) wrote: > > > Please add this to the constructor instead. SetUp is discouraged, according > to > > > > > > https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/Initia... > > > > Seems that ASSERT_TRUE cannot be used in constructor as it can return void: > > > > > ../../components/password_manager/core/browser/import/password_importer_unittest.cc:34:5: > > error: constructor 'PasswordImporterTest' must not return void expression > > ASSERT_TRUE(temp_directory_.CreateUniqueTempDir()); > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > ../../testing/gtest/include/gtest/gtest.h:1871:23: note: expanded from macro > > 'ASSERT_TRUE' > > GTEST_FATAL_FAILURE_) > > ^~~~~~~~~~~~~~~~~~~~~ > > ../../testing/gtest/include/gtest/internal/gtest-internal.h:1192:5: note: > > expanded from macro 'GTEST_TEST_BOOLEAN_' > > fail(::testing::internal::GetBoolAssertionFailureMessage(\ > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > ../../testing/gtest/include/gtest/internal/gtest-internal.h:1110:3: note: > > expanded from macro 'GTEST_FATAL_FAILURE_' > > return GTEST_MESSAGE_(message, ::testing::TestPartResult::kFatalFailure) > > ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > I think it is fine to change ASSERT_TRUE to CHECK here. Avoiding the SetUp in > favour of the constructor is more important. > While we need to think twice about CHECKs in production code (use restricted by > the style guide to only security reasons), here in the test it only matters > whether the resulting error message and behaviour are comparable. Both > ASSERT_TRUE and CHECK will indicate which assertion failed, and both will crash > the test, so there is not much difference. Sounds good. Thanks! https://codereview.chromium.org/1193143003/diff/560001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler_unittest.cc (right): https://codereview.chromium.org/1193143003/diff/560001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler_unittest.cc:34: }; On 2016/03/21 14:01:30, vabr (Chromium) wrote: > nit: Please add one blank line to separate the class definition from the method > definition (also between lines 39 and 40). Done. https://codereview.chromium.org/1193143003/diff/560001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler_unittest.cc:128: ~PasswordManagerHandlerTest() override {} On 2016/03/21 14:01:30, vabr (Chromium) wrote: > Nit: please add a blank line before the destructor. Done. https://codereview.chromium.org/1193143003/diff/560001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler_unittest.cc:138: base::ListValue* tmp = new base::ListValue(); On 2016/03/21 14:01:30, vabr (Chromium) wrote: > No need to create ListValue on the heap, just create it on the stack: > > base::ListValue tmp; > web_ui_.ProcessWebUIMessage(GURL(), "exportPassword", tmp); > > (If you needed a heap-allocated one, then you should put it into a scoped_ptr.) Done. https://codereview.chromium.org/1193143003/diff/560001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler_unittest.cc:139: web_ui_.ProcessWebUIMessage(GURL(""), "exportPassword", *tmp); On 2016/03/21 14:01:30, vabr (Chromium) wrote: > Please replace GURL("") with just GURL(), that's faster than parsing the empty > string. Done. https://codereview.chromium.org/1193143003/diff/560001/components/password_ma... File components/password_manager/core/browser/export/password_exporter.cc (right): https://codereview.chromium.org/1193143003/diff/560001/components/password_ma... components/password_manager/core/browser/export/password_exporter.cc:104: path.Extension().substr(1) == PasswordCSVWriter::kFileExtension) { On 2016/03/21 14:01:30, vabr (Chromium) wrote: > I suggest going even further and just creating the PasswordCSVWriter > unconditionally, with a comment saying something like: "CSV is the only > currently supported format." > > There is no point in checking the extension and ignoring the result just so that > once we potentially add another supported format, we will spare about 3 lines of > code in that CL. > > I suggest the same for the analogous part of handling the reader in the > importer. Done. https://codereview.chromium.org/1193143003/diff/560001/components/password_ma... File components/password_manager/core/browser/import/password_importer.cc (right): https://codereview.chromium.org/1193143003/diff/560001/components/password_ma... components/password_manager/core/browser/import/password_importer.cc:120: [&options](const std::string str) { On 2016/03/21 14:01:30, vabr (Chromium) wrote: > Please make this a reference to a const string, not just value-passed const > string. That will save the copy: > > const std::string& str Done.
Thank you, Xun, this LGTM! Cheers, Vaclav https://codereview.chromium.org/1193143003/diff/600001/components/password_ma... File components/password_manager/core/browser/export/password_exporter.cc (right): https://codereview.chromium.org/1193143003/diff/600001/components/password_ma... components/password_manager/core/browser/export/password_exporter.cc:103: scoped_ptr<PasswordWriterBase> password_writer(new PasswordCSVWriter); nit: This even does not have to be a scoped_ptr, just creating it on the stack should work: PasswordCSVWriter password_writer; https://codereview.chromium.org/1193143003/diff/600001/components/password_ma... File components/password_manager/core/browser/import/password_importer.cc (right): https://codereview.chromium.org/1193143003/diff/600001/components/password_ma... components/password_manager/core/browser/import/password_importer.cc:171: scoped_ptr<PasswordReaderBase> password_reader(new PasswordCSVReader); nit: Also here, it might be enough to create the reader on stack: PasswordCSVReader password_reader;
xunlu@chromium.org changed reviewers: + holte@chromium.org
+holte, PTAL at the change in tools/metrics/histograms/histograms.xml https://codereview.chromium.org/1193143003/diff/600001/components/password_ma... File components/password_manager/core/browser/export/password_exporter.cc (right): https://codereview.chromium.org/1193143003/diff/600001/components/password_ma... components/password_manager/core/browser/export/password_exporter.cc:103: scoped_ptr<PasswordWriterBase> password_writer(new PasswordCSVWriter); On 2016/03/22 15:28:28, vabr (Chromium) wrote: > nit: This even does not have to be a scoped_ptr, just creating it on the stack > should work: > PasswordCSVWriter password_writer; Done. https://codereview.chromium.org/1193143003/diff/600001/components/password_ma... File components/password_manager/core/browser/import/password_importer.cc (right): https://codereview.chromium.org/1193143003/diff/600001/components/password_ma... components/password_manager/core/browser/import/password_importer.cc:171: scoped_ptr<PasswordReaderBase> password_reader(new PasswordCSVReader); On 2016/03/22 15:28:28, vabr (Chromium) wrote: > nit: Also here, it might be enough to create the reader on stack: > PasswordCSVReader password_reader; I'm not sure about this one because this variable needs to be bind. See line 175. If we create it on stack, how should we pass it in bind? I tried use base::Owned and base::Unretained, but they crashed the test.
LGTM, thank you! Vaclav https://codereview.chromium.org/1193143003/diff/600001/components/password_ma... File components/password_manager/core/browser/import/password_importer.cc (right): https://codereview.chromium.org/1193143003/diff/600001/components/password_ma... components/password_manager/core/browser/import/password_importer.cc:171: scoped_ptr<PasswordReaderBase> password_reader(new PasswordCSVReader); On 2016/03/23 04:50:52, xunlu wrote: > On 2016/03/22 15:28:28, vabr (Chromium) wrote: > > nit: Also here, it might be enough to create the reader on stack: > > PasswordCSVReader password_reader; > > I'm not sure about this one because this variable needs to be bind. See line > 175. > > If we create it on stack, how should we pass it in bind? I tried use base::Owned > and base::Unretained, but they crashed the test. Ah, sorry, my bad, this needs to be a scoped_ptr because the reason you state (passing it in the Bind call). Please ignore me, the current code is good.
https://codereview.chromium.org/1193143003/diff/620001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler.cc (right): https://codereview.chromium.org/1193143003/diff/620001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:382: UMA_HISTOGRAM_COUNTS("PasswordManager.FailToStorePasswordImportedFromCSV", You should probably use UMA_HISTOGRAM_BOOLEAN here, otherwise you are allocating 50 buckets for a single count. It would probably also be a good idea to store success in the same histogram, and make it a BooleanSuccess histogram.
https://codereview.chromium.org/1193143003/diff/620001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler.cc (right): https://codereview.chromium.org/1193143003/diff/620001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:382: UMA_HISTOGRAM_COUNTS("PasswordManager.FailToStorePasswordImportedFromCSV", On 2016/03/23 18:57:39, Steven Holte wrote: > You should probably use UMA_HISTOGRAM_BOOLEAN here, otherwise you are allocating > 50 buckets for a single count. It would probably also be a good idea to store > success in the same histogram, and make it a BooleanSuccess histogram. Done.
On 2016/03/24 06:19:30, xunlu wrote: > https://codereview.chromium.org/1193143003/diff/620001/chrome/browser/ui/webu... > File chrome/browser/ui/webui/options/password_manager_handler.cc (right): > > https://codereview.chromium.org/1193143003/diff/620001/chrome/browser/ui/webu... > chrome/browser/ui/webui/options/password_manager_handler.cc:382: > UMA_HISTOGRAM_COUNTS("PasswordManager.FailToStorePasswordImportedFromCSV", > On 2016/03/23 18:57:39, Steven Holte wrote: > > You should probably use UMA_HISTOGRAM_BOOLEAN here, otherwise you are > allocating > > 50 buckets for a single count. It would probably also be a good idea to store > > success in the same histogram, and make it a BooleanSuccess histogram. > > Done. lgtm
Patchset #21 (id:680001) has been deleted
Patchset #21 (id:700001) has been deleted
Patchset #21 (id:720001) has been deleted
Patchset #21 (id:730001) has been deleted
Patchset #21 (id:750001) has been deleted
The CQ bit was checked by xunlu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gcasto@chromium.org, vabr@chromium.org, holte@chromium.org Link to the patchset: https://codereview.chromium.org/1193143003/#ps770001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1193143003/770001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1193143003/770001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
+estade Hi Evan, can you take a look?
https://codereview.chromium.org/1193143003/diff/770001/chrome/browser/resourc... File chrome/browser/resources/options/password_manager.css (right): https://codereview.chromium.org/1193143003/diff/770001/chrome/browser/resourc... chrome/browser/resources/options/password_manager.css:35: display: none; why not put hidden in the html and .hidden = false in the js? https://codereview.chromium.org/1193143003/diff/770001/chrome/browser/resourc... chrome/browser/resources/options/password_manager.css:39: float: right; float is almost never the right choice (it should only be used for things that need text to wrap around them). What is the point of spacer-div if not to be part of a box model and expand to take up all extra space? https://codereview.chromium.org/1193143003/diff/770001/chrome/browser/resourc... File chrome/browser/resources/options/password_manager.js (right): https://codereview.chromium.org/1193143003/diff/770001/chrome/browser/resourc... chrome/browser/resources/options/password_manager.js:62: var self = this; is this used somewhere? https://codereview.chromium.org/1193143003/diff/770001/chrome/browser/resourc... chrome/browser/resources/options/password_manager.js:80: this.handleSearchQueryChange_.bind(this)); seems like the indentation was correct before https://codereview.chromium.org/1193143003/diff/770001/chrome/browser/resourc... chrome/browser/resources/options/password_manager.js:305: ['Options_PasswordManagerDeletePassword']); I think this indent needs to change too https://codereview.chromium.org/1193143003/diff/770001/chrome/browser/resourc... chrome/browser/resources/options/password_manager.js:325: 'showImportExportButton' nit: add a final comma nit 2: alphabetize https://codereview.chromium.org/1193143003/diff/770001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler.cc (right): https://codereview.chromium.org/1193143003/diff/770001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:90: // accidental call of SelectFile with params=NULL will error out. in what way does it error out? PasswordManagerHandler::FileSelected doesn't do anything special with values that aren't listed here. https://codereview.chromium.org/1193143003/diff/770001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:104: : password_manager_presenter_(std::move(presenter)) {} is this std::move necessary? https://codereview.chromium.org/1193143003/diff/770001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:221: password_manager::features::kPasswordImportExport)) nit: curlies https://codereview.chromium.org/1193143003/diff/770001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:316: switch (static_cast<FileSelectorCaller>(reinterpret_cast<intptr_t>(params))) { why are there two casts here https://codereview.chromium.org/1193143003/diff/770001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler.h (right): https://codereview.chromium.org/1193143003/diff/770001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.h:35: scoped_ptr<PasswordManagerPresenter> presenter); this need not be public. You can make it protected.
Patchset #22 (id:790001) has been deleted
Patchset #22 (id:810001) has been deleted
https://codereview.chromium.org/1193143003/diff/770001/chrome/browser/resourc... File chrome/browser/resources/options/password_manager.css (right): https://codereview.chromium.org/1193143003/diff/770001/chrome/browser/resourc... chrome/browser/resources/options/password_manager.css:35: display: none; On 2016/04/06 21:25:22, Evan Stade wrote: > why not put hidden in the html and .hidden = false in the js? Done. https://codereview.chromium.org/1193143003/diff/770001/chrome/browser/resourc... chrome/browser/resources/options/password_manager.css:39: float: right; On 2016/04/06 21:25:22, Evan Stade wrote: > float is almost never the right choice (it should only be used for things that > need text to wrap around them). What is the point of spacer-div if not to be > part of a box model and expand to take up all extra space? I changed it to align:right in html. https://codereview.chromium.org/1193143003/diff/770001/chrome/browser/resourc... File chrome/browser/resources/options/password_manager.js (right): https://codereview.chromium.org/1193143003/diff/770001/chrome/browser/resourc... chrome/browser/resources/options/password_manager.js:62: var self = this; On 2016/04/06 21:25:22, Evan Stade wrote: > is this used somewhere? Done. https://codereview.chromium.org/1193143003/diff/770001/chrome/browser/resourc... chrome/browser/resources/options/password_manager.js:80: this.handleSearchQueryChange_.bind(this)); On 2016/04/06 21:25:22, Evan Stade wrote: > seems like the indentation was correct before Done. https://codereview.chromium.org/1193143003/diff/770001/chrome/browser/resourc... chrome/browser/resources/options/password_manager.js:305: ['Options_PasswordManagerDeletePassword']); On 2016/04/06 21:25:22, Evan Stade wrote: > I think this indent needs to change too Done. https://codereview.chromium.org/1193143003/diff/770001/chrome/browser/resourc... chrome/browser/resources/options/password_manager.js:325: 'showImportExportButton' On 2016/04/06 21:25:22, Evan Stade wrote: > nit: add a final comma > nit 2: alphabetize Done. https://codereview.chromium.org/1193143003/diff/770001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler.cc (right): https://codereview.chromium.org/1193143003/diff/770001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:90: // accidental call of SelectFile with params=NULL will error out. On 2016/04/06 21:25:22, Evan Stade wrote: > in what way does it error out? PasswordManagerHandler::FileSelected doesn't do > anything special with values that aren't listed here. I'm not sure how such error will occur. I added "default: NOTREACHED()" in PasswordManagerHandler::FileSelected. https://codereview.chromium.org/1193143003/diff/770001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:104: : password_manager_presenter_(std::move(presenter)) {} On 2016/04/06 21:25:22, Evan Stade wrote: > is this std::move necessary? Just for or my education: why not? I thought there would be an ownership transfer of PasswordManagerPresenter from |presenter| to |password_manager_presenter_|. And if I remove "std::move", compiler complains: ../../chrome/browser/ui/webui/options/password_manager_handler.cc:104:7: error: call to deleted constructor of 'scoped_ptr<PasswordManagerPrese nter>' (aka 'std::unique_ptr<PasswordManagerPresenter, std::default_delete<PasswordManagerPresenter> >') : password_manager_presenter_(presenter) {} ^ ~~~~~~~~~ https://codereview.chromium.org/1193143003/diff/770001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:221: password_manager::features::kPasswordImportExport)) On 2016/04/06 21:25:22, Evan Stade wrote: > nit: curlies Done. https://codereview.chromium.org/1193143003/diff/770001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:316: switch (static_cast<FileSelectorCaller>(reinterpret_cast<intptr_t>(params))) { On 2016/04/06 21:25:22, Evan Stade wrote: > why are there two casts here Done. https://codereview.chromium.org/1193143003/diff/770001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler.h (right): https://codereview.chromium.org/1193143003/diff/770001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.h:35: scoped_ptr<PasswordManagerPresenter> presenter); On 2016/04/06 21:25:22, Evan Stade wrote: > this need not be public. You can make it protected. Done.
https://codereview.chromium.org/1193143003/diff/770001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler.cc (right): https://codereview.chromium.org/1193143003/diff/770001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:90: // accidental call of SelectFile with params=NULL will error out. On 2016/04/08 15:38:21, xunlu wrote: > On 2016/04/06 21:25:22, Evan Stade wrote: > > in what way does it error out? PasswordManagerHandler::FileSelected doesn't do > > anything special with values that aren't listed here. > > I'm not sure how such error will occur. I added "default: NOTREACHED()" in > PasswordManagerHandler::FileSelected. better to not have default because then if you add a new enum value the compiler will complain that it's unhandled (assuming you cast to FileSelectorCaller). And I don't think starting this enum at 1 because of a hypothetical error we can't explain makes sense. https://codereview.chromium.org/1193143003/diff/770001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:104: : password_manager_presenter_(std::move(presenter)) {} On 2016/04/08 15:38:21, xunlu wrote: > On 2016/04/06 21:25:22, Evan Stade wrote: > > is this std::move necessary? > > Just for or my education: why not? I thought there would be an ownership > transfer of PasswordManagerPresenter from |presenter| to > |password_manager_presenter_|. > > And if I remove "std::move", compiler complains: > ../../chrome/browser/ui/webui/options/password_manager_handler.cc:104:7: error: > call to deleted constructor of 'scoped_ptr<PasswordManagerPrese > nter>' (aka 'std::unique_ptr<PasswordManagerPresenter, > std::default_delete<PasswordManagerPresenter> >') > : password_manager_presenter_(presenter) {} > ^ ~~~~~~~~~ I posed it as a question because I didn't know the answer :) If the compiler is complaining then yes, it's required. https://codereview.chromium.org/1193143003/diff/770001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:316: switch (static_cast<FileSelectorCaller>(reinterpret_cast<intptr_t>(params))) { what I meant was for you to cast to FileSelectorCaller https://codereview.chromium.org/1193143003/diff/830001/chrome/browser/resourc... File chrome/browser/resources/options/password_manager.html (right): https://codereview.chromium.org/1193143003/diff/830001/chrome/browser/resourc... chrome/browser/resources/options/password_manager.html:31: <div class="import-export-button-strip" align="right"> I don't think this is correct either. Other button-strip elements in other webui don't use align. I don't get why you need to worry about this at all. Isn't it handled (with the spacer div) by ui/webui/resources/css/overlay.css? Just add the .button-strip class
https://codereview.chromium.org/1193143003/diff/770001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler.cc (right): https://codereview.chromium.org/1193143003/diff/770001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:90: // accidental call of SelectFile with params=NULL will error out. On 2016/04/08 17:28:28, Evan Stade wrote: > On 2016/04/08 15:38:21, xunlu wrote: > > On 2016/04/06 21:25:22, Evan Stade wrote: > > > in what way does it error out? PasswordManagerHandler::FileSelected doesn't > do > > > anything special with values that aren't listed here. > > > > I'm not sure how such error will occur. I added "default: NOTREACHED()" in > > PasswordManagerHandler::FileSelected. > > better to not have default because then if you add a new enum value the compiler > will complain that it's unhandled (assuming you cast to FileSelectorCaller). And > I don't think starting this enum at 1 because of a hypothetical error we can't > explain makes sense. CertificateManagerHandler was doing this, so I assume the error does exist? https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... https://codereview.chromium.org/1193143003/diff/770001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:316: switch (static_cast<FileSelectorCaller>(reinterpret_cast<intptr_t>(params))) { In that case, I need to use two casts otherwise there is a compiler error.
https://codereview.chromium.org/1193143003/diff/770001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler.cc (right): https://codereview.chromium.org/1193143003/diff/770001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:90: // accidental call of SelectFile with params=NULL will error out. On 2016/04/08 17:56:15, xunlu wrote: > On 2016/04/08 17:28:28, Evan Stade wrote: > > On 2016/04/08 15:38:21, xunlu wrote: > > > On 2016/04/06 21:25:22, Evan Stade wrote: > > > > in what way does it error out? PasswordManagerHandler::FileSelected > doesn't > > do > > > > anything special with values that aren't listed here. > > > > > > I'm not sure how such error will occur. I added "default: NOTREACHED()" in > > > PasswordManagerHandler::FileSelected. > > > > better to not have default because then if you add a new enum value the > compiler > > will complain that it's unhandled (assuming you cast to FileSelectorCaller). > And > > I don't think starting this enum at 1 because of a hypothetical error we can't > > explain makes sense. > > CertificateManagerHandler was doing this, so I assume the error does exist? > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... I don't think it's safe to copy-paste code unless you understand it. https://codereview.chromium.org/1193143003/diff/770001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:316: switch (static_cast<FileSelectorCaller>(reinterpret_cast<intptr_t>(params))) { On 2016/04/08 17:56:15, xunlu wrote: > In that case, I need to use two casts otherwise there is a compiler error. ok, then stick with two casts. Again, I asked the question because I wanted to know the answer.
https://codereview.chromium.org/1193143003/diff/770001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler.cc (right): https://codereview.chromium.org/1193143003/diff/770001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:90: // accidental call of SelectFile with params=NULL will error out. On 2016/04/08 18:05:09, Evan Stade wrote: > On 2016/04/08 17:56:15, xunlu wrote: > > On 2016/04/08 17:28:28, Evan Stade wrote: > > > On 2016/04/08 15:38:21, xunlu wrote: > > > > On 2016/04/06 21:25:22, Evan Stade wrote: > > > > > in what way does it error out? PasswordManagerHandler::FileSelected > > doesn't > > > do > > > > > anything special with values that aren't listed here. > > > > > > > > I'm not sure how such error will occur. I added "default: NOTREACHED()" in > > > > PasswordManagerHandler::FileSelected. > > > > > > better to not have default because then if you add a new enum value the > > compiler > > > will complain that it's unhandled (assuming you cast to FileSelectorCaller). > > And > > > I don't think starting this enum at 1 because of a hypothetical error we > can't > > > explain makes sense. > > > > CertificateManagerHandler was doing this, so I assume the error does exist? > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > > I don't think it's safe to copy-paste code unless you understand it. OK. I will remove "= 1" and the default logic in switch. https://codereview.chromium.org/1193143003/diff/770001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:104: : password_manager_presenter_(std::move(presenter)) {} On 2016/04/08 17:28:28, Evan Stade wrote: > On 2016/04/08 15:38:21, xunlu wrote: > > On 2016/04/06 21:25:22, Evan Stade wrote: > > > is this std::move necessary? > > > > Just for or my education: why not? I thought there would be an ownership > > transfer of PasswordManagerPresenter from |presenter| to > > |password_manager_presenter_|. > > > > And if I remove "std::move", compiler complains: > > ../../chrome/browser/ui/webui/options/password_manager_handler.cc:104:7: > error: > > call to deleted constructor of 'scoped_ptr<PasswordManagerPrese > > nter>' (aka 'std::unique_ptr<PasswordManagerPresenter, > > std::default_delete<PasswordManagerPresenter> >') > > : password_manager_presenter_(presenter) {} > > ^ ~~~~~~~~~ > > I posed it as a question because I didn't know the answer :) If the compiler is > complaining then yes, it's required. Acknowledged. https://codereview.chromium.org/1193143003/diff/770001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:316: switch (static_cast<FileSelectorCaller>(reinterpret_cast<intptr_t>(params))) { On 2016/04/08 18:05:09, Evan Stade wrote: > On 2016/04/08 17:56:15, xunlu wrote: > > In that case, I need to use two casts otherwise there is a compiler error. > > ok, then stick with two casts. Again, I asked the question because I wanted to > know the answer. Acknowledged. https://codereview.chromium.org/1193143003/diff/830001/chrome/browser/resourc... File chrome/browser/resources/options/password_manager.html (right): https://codereview.chromium.org/1193143003/diff/830001/chrome/browser/resourc... chrome/browser/resources/options/password_manager.html:31: <div class="import-export-button-strip" align="right"> On 2016/04/08 17:28:28, Evan Stade wrote: > I don't think this is correct either. Other button-strip elements in other webui > don't use align. I don't get why you need to worry about this at all. Isn't it > handled (with the spacer div) by ui/webui/resources/css/overlay.css? Just add > the .button-strip class But then how do I flush the button to the right of the button-strip div?
https://codereview.chromium.org/1193143003/diff/830001/chrome/browser/resourc... File chrome/browser/resources/options/password_manager.html (right): https://codereview.chromium.org/1193143003/diff/830001/chrome/browser/resourc... chrome/browser/resources/options/password_manager.html:31: <div class="import-export-button-strip" align="right"> On 2016/04/08 19:10:20, xunlu wrote: > On 2016/04/08 17:28:28, Evan Stade wrote: > > I don't think this is correct either. Other button-strip elements in other > webui > > don't use align. I don't get why you need to worry about this at all. Isn't it > > handled (with the spacer div) by ui/webui/resources/css/overlay.css? Just add > > the .button-strip class > > But then how do I flush the button to the right of the button-strip div? how do other dialogs do it?
https://codereview.chromium.org/1193143003/diff/830001/chrome/browser/resourc... File chrome/browser/resources/options/password_manager.html (right): https://codereview.chromium.org/1193143003/diff/830001/chrome/browser/resourc... chrome/browser/resources/options/password_manager.html:31: <div class="import-export-button-strip" align="right"> On 2016/04/08 19:23:18, Evan Stade wrote: > On 2016/04/08 19:10:20, xunlu wrote: > > On 2016/04/08 17:28:28, Evan Stade wrote: > > > I don't think this is correct either. Other button-strip elements in other > > webui > > > don't use align. I don't get why you need to worry about this at all. Isn't > it > > > handled (with the spacer div) by ui/webui/resources/css/overlay.css? Just > add > > > the .button-strip class > > > > But then how do I flush the button to the right of the button-strip div? > > how do other dialogs do it? Done.
https://codereview.chromium.org/1193143003/diff/850001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler.cc (right): https://codereview.chromium.org/1193143003/diff/850001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:395: } nit: no curlies https://codereview.chromium.org/1193143003/diff/850001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler.h (right): https://codereview.chromium.org/1193143003/diff/850001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.h:123: // The PasswordManagerPresenter object owned by the this view. nit: this comment seems a bit unnecessary https://codereview.chromium.org/1193143003/diff/850001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.h:126: // Handle for file picker this comment is not particularly useful either. Perhaps describe (briefly) why we have a file picker at all https://codereview.chromium.org/1193143003/diff/850001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.h:127: scoped_refptr<ui::SelectFileDialog> selected_file_dialog_; s/selected_/select_ https://codereview.chromium.org/1193143003/diff/850001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler_unittest.cc (right): https://codereview.chromium.org/1193143003/diff/850001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler_unittest.cc:25: class TestSelectFileDialogFactory final : public ui::SelectFileDialogFactory { anonymous namespace https://codereview.chromium.org/1193143003/diff/850001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler_unittest.cc:200: .Times(2) what is this attempting to test? https://codereview.chromium.org/1193143003/diff/850001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler_unittest.cc:209: ExportPassword(); I don't see what this is testing or what hypothetical regression it would catch
also I think you're supposed to be using std::unique_ptr instead of scoped_ptr now
https://codereview.chromium.org/1193143003/diff/850001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler.cc (right): https://codereview.chromium.org/1193143003/diff/850001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:395: } On 2016/04/08 21:00:15, Evan Stade wrote: > nit: no curlies Done. https://codereview.chromium.org/1193143003/diff/850001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler.h (right): https://codereview.chromium.org/1193143003/diff/850001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.h:123: // The PasswordManagerPresenter object owned by the this view. On 2016/04/08 21:00:15, Evan Stade wrote: > nit: this comment seems a bit unnecessary Done. https://codereview.chromium.org/1193143003/diff/850001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.h:126: // Handle for file picker On 2016/04/08 21:00:15, Evan Stade wrote: > this comment is not particularly useful either. Perhaps describe (briefly) why > we have a file picker at all Done. https://codereview.chromium.org/1193143003/diff/850001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.h:127: scoped_refptr<ui::SelectFileDialog> selected_file_dialog_; On 2016/04/08 21:00:15, Evan Stade wrote: > s/selected_/select_ Done. https://codereview.chromium.org/1193143003/diff/850001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler_unittest.cc (right): https://codereview.chromium.org/1193143003/diff/850001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler_unittest.cc:200: .Times(2) On 2016/04/08 21:00:15, Evan Stade wrote: > what is this attempting to test? This is to test PasswordManagerHandler::HandlePasswordImport() is called and part of its logic executed when importPassword in JS is called. https://codereview.chromium.org/1193143003/diff/850001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler_unittest.cc:209: ExportPassword(); On 2016/04/08 21:00:16, Evan Stade wrote: > I don't see what this is testing or what hypothetical regression it would catch Similar to previous one, this one verifies PasswordManagerHandler::HandlePasswordImport() is called and part of its logic executed when exportPassword in JS is called. I agree these tests are pretty limited in their coverage. But it seems the best we can do without having to friend this test class in PasswordManagerHandler. Any suggestions?
https://codereview.chromium.org/1193143003/diff/850001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler_unittest.cc (right): https://codereview.chromium.org/1193143003/diff/850001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler_unittest.cc:200: .Times(2) On 2016/04/08 21:42:55, xunlu wrote: > On 2016/04/08 21:00:15, Evan Stade wrote: > > what is this attempting to test? > > This is to test PasswordManagerHandler::HandlePasswordImport() is called and > part of its logic executed when importPassword in JS is called. far too tightly coupled to be useful https://codereview.chromium.org/1193143003/diff/850001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler_unittest.cc:209: ExportPassword(); On 2016/04/08 21:42:55, xunlu wrote: > On 2016/04/08 21:00:16, Evan Stade wrote: > > I don't see what this is testing or what hypothetical regression it would > catch > > Similar to previous one, this one verifies > PasswordManagerHandler::HandlePasswordImport() is called and part of its logic > executed when exportPassword in JS is called. > > I agree these tests are pretty limited in their coverage. But it seems the best > we can do without having to friend this test class in PasswordManagerHandler. > Any suggestions? can you work with the primary reviewers on this CL to come up with better tests? FRIEND_TEST is not the end of the world, and TestPasswordManagerHandler is a common pattern as well.
https://codereview.chromium.org/1193143003/diff/850001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler_unittest.cc (right): https://codereview.chromium.org/1193143003/diff/850001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler_unittest.cc:209: ExportPassword(); On 2016/04/08 21:46:39, Evan Stade wrote: > On 2016/04/08 21:42:55, xunlu wrote: > > On 2016/04/08 21:00:16, Evan Stade wrote: > > > I don't see what this is testing or what hypothetical regression it would > > catch > > > > Similar to previous one, this one verifies > > PasswordManagerHandler::HandlePasswordImport() is called and part of its logic > > executed when exportPassword in JS is called. > > > > I agree these tests are pretty limited in their coverage. But it seems the > best > > we can do without having to friend this test class in PasswordManagerHandler. > > Any suggestions? > > can you work with the primary reviewers on this CL to come up with better tests? > FRIEND_TEST is not the end of the world, and TestPasswordManagerHandler is a > common pattern as well. Considering that PasswordManagerHandler did not even have a unit_test before, I think this is already a step forward (baby one though :) ). We can explore other options if we find it necessary in the future. +vabr
On 2016/04/08 22:26:54, xunlu wrote: > https://codereview.chromium.org/1193143003/diff/850001/chrome/browser/ui/webu... > File chrome/browser/ui/webui/options/password_manager_handler_unittest.cc > (right): > > https://codereview.chromium.org/1193143003/diff/850001/chrome/browser/ui/webu... > chrome/browser/ui/webui/options/password_manager_handler_unittest.cc:209: > ExportPassword(); > On 2016/04/08 21:46:39, Evan Stade wrote: > > On 2016/04/08 21:42:55, xunlu wrote: > > > On 2016/04/08 21:00:16, Evan Stade wrote: > > > > I don't see what this is testing or what hypothetical regression it would > > > catch > > > > > > Similar to previous one, this one verifies > > > PasswordManagerHandler::HandlePasswordImport() is called and part of its > logic > > > executed when exportPassword in JS is called. > > > > > > I agree these tests are pretty limited in their coverage. But it seems the > > best > > > we can do without having to friend this test class in > PasswordManagerHandler. > > > Any suggestions? > > > > can you work with the primary reviewers on this CL to come up with better > tests? > > FRIEND_TEST is not the end of the world, and TestPasswordManagerHandler is a > > common pattern as well. > > Considering that PasswordManagerHandler did not even have a unit_test before, I > think this is already a step forward (baby one though :) ). We can explore other > options if we find it necessary in the future. > > +vabr I happen to think tightly coupled tests are a step backwards. If we for some reason want to call GetWebContents() three times instead of two (definitely not an "incorrect" behavior) we have to first realize this breaks a test then update said test. This test is providing no benefit. It's true that there's existing, untested functionality, but you're adding new functionality and it should be tested by good tests if possible, or by no tests if good ones are not possible.
Thanks, Evan, for raising good points. Also, thanks, Xunlu, for continuing to improve this CL. I left a suggestion for the unit test below. Cheers, Vaclav https://codereview.chromium.org/1193143003/diff/850001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler_unittest.cc (right): https://codereview.chromium.org/1193143003/diff/850001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler_unittest.cc:200: .Times(2) On 2016/04/08 21:46:39, Evan Stade wrote: > On 2016/04/08 21:42:55, xunlu wrote: > > On 2016/04/08 21:00:15, Evan Stade wrote: > > > what is this attempting to test? > > > > This is to test PasswordManagerHandler::HandlePasswordImport() is called and > > part of its logic executed when importPassword in JS is called. > > far too tightly coupled to be useful I share Evan's concerns here. My suggestion: Could you make TestSelectFileDialog call the listener as if the user chose some file, and then override FileSelected in TestPasswordManagerHandler to check that it was called with the expected |params|? On that note, it is unfortunate that we have TestPasswordManagerHandler in the test for PasswordManagerHandler, because it increases the danger that what we end up testing is not the production code. This is usually a hint that the tested class could be split into smaller functional blocks. In this case, my further suggestion would be to separate PasswordManagerHandler::HandlePasswordImport and PasswordManagerHandler::HandlePasswordExport from PasswordManagerHandler into a separate class to handle the dialogue creation (PasswordImportExportDialogHandler?), which would be owned by PasswordManagerHandler, but would not need to know about it at all. This class could remove some code duplication between the two existing Handle* methods. More importantly, this new class would also use another new class (PasswordImportExportDialogListener?) to fulfil the role of the dialogue listener (currently done by PasswordManagerHandler::FileSelected). Once you have such separation, it should be easier to write the tests: you won't have to set up the whole PasswordManagerHandler, in particular. But more importantly, when testing each of the newly added classes, you can mock out the rest, making it easier to verify that the important things are happening. For example, when testing the *DialogHandler, you would mock out the listener, and just check that it got the expected callback. Also, feel free to use the UMA signals to verify behaviour, through base::HistogramTester.
Patchset #24 (id:870001) has been deleted
https://codereview.chromium.org/1193143003/diff/850001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler_unittest.cc (right): https://codereview.chromium.org/1193143003/diff/850001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler_unittest.cc:25: class TestSelectFileDialogFactory final : public ui::SelectFileDialogFactory { On 2016/04/08 21:00:16, Evan Stade wrote: > anonymous namespace Done. https://codereview.chromium.org/1193143003/diff/850001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler_unittest.cc:200: .Times(2) On 2016/04/11 08:42:59, vabr (Chromium) wrote: > On 2016/04/08 21:46:39, Evan Stade wrote: > > On 2016/04/08 21:42:55, xunlu wrote: > > > On 2016/04/08 21:00:15, Evan Stade wrote: > > > > what is this attempting to test? > > > > > > This is to test PasswordManagerHandler::HandlePasswordImport() is called and > > > part of its logic executed when importPassword in JS is called. > > > > far too tightly coupled to be useful > > I share Evan's concerns here. > > My suggestion: > Could you make TestSelectFileDialog call the listener as if the user chose some > file, and then override FileSelected in TestPasswordManagerHandler to check that > it was called with the expected |params|? > > On that note, it is unfortunate that we have TestPasswordManagerHandler in the > test for PasswordManagerHandler, because it increases the danger that what we > end up testing is not the production code. This is usually a hint that the > tested class could be split into smaller functional blocks. In this case, my > further suggestion would be to separate > PasswordManagerHandler::HandlePasswordImport and > PasswordManagerHandler::HandlePasswordExport from PasswordManagerHandler into a > separate class to handle the dialogue creation > (PasswordImportExportDialogHandler?), which would be owned by > PasswordManagerHandler, but would not need to know about it at all. This class > could remove some code duplication between the two existing Handle* methods. > More importantly, this new class would also use another new class > (PasswordImportExportDialogListener?) to fulfil the role of the dialogue > listener (currently done by PasswordManagerHandler::FileSelected). > > Once you have such separation, it should be easier to write the tests: you won't > have to set up the whole PasswordManagerHandler, in particular. But more > importantly, when testing each of the newly added classes, you can mock out the > rest, making it easier to verify that the important things are happening. For > example, when testing the *DialogHandler, you would mock out the listener, and > just check that it got the expected callback. Also, feel free to use the UMA > signals to verify behaviour, through base::HistogramTester. Thanks Vaclav. Test updated.
Thanks, Xun Lu. I believe the unittest is acceptable now (with a comment about using the FileSelectorCaller values addressed), LGTM. Cheers, Vaclav https://codereview.chromium.org/1193143003/diff/850001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler_unittest.cc (right): https://codereview.chromium.org/1193143003/diff/850001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler_unittest.cc:200: .Times(2) On 2016/04/12 05:31:03, xunlu wrote: > On 2016/04/11 08:42:59, vabr (Chromium) wrote: > > On 2016/04/08 21:46:39, Evan Stade wrote: > > > On 2016/04/08 21:42:55, xunlu wrote: > > > > On 2016/04/08 21:00:15, Evan Stade wrote: > > > > > what is this attempting to test? > > > > > > > > This is to test PasswordManagerHandler::HandlePasswordImport() is called > and > > > > part of its logic executed when importPassword in JS is called. > > > > > > far too tightly coupled to be useful > > > > I share Evan's concerns here. > > > > My suggestion: > > Could you make TestSelectFileDialog call the listener as if the user chose > some > > file, and then override FileSelected in TestPasswordManagerHandler to check > that > > it was called with the expected |params|? > > > > On that note, it is unfortunate that we have TestPasswordManagerHandler in the > > test for PasswordManagerHandler, because it increases the danger that what we > > end up testing is not the production code. This is usually a hint that the > > tested class could be split into smaller functional blocks. In this case, my > > further suggestion would be to separate > > PasswordManagerHandler::HandlePasswordImport and > > PasswordManagerHandler::HandlePasswordExport from PasswordManagerHandler into > a > > separate class to handle the dialogue creation > > (PasswordImportExportDialogHandler?), which would be owned by > > PasswordManagerHandler, but would not need to know about it at all. This class > > could remove some code duplication between the two existing Handle* methods. > > More importantly, this new class would also use another new class > > (PasswordImportExportDialogListener?) to fulfil the role of the dialogue > > listener (currently done by PasswordManagerHandler::FileSelected). > > > > Once you have such separation, it should be easier to write the tests: you > won't > > have to set up the whole PasswordManagerHandler, in particular. But more > > importantly, when testing each of the newly added classes, you can mock out > the > > rest, making it easier to verify that the important things are happening. For > > example, when testing the *DialogHandler, you would mock out the listener, and > > just check that it got the expected callback. Also, feel free to use the UMA > > signals to verify behaviour, through base::HistogramTester. > > Thanks Vaclav. Test updated. Thanks. I wish we had the above suggested separation of the functionality to have proper testing, but I'm not going to torture you with that. It will remain a technical debt which I'll have to pay later, though. https://codereview.chromium.org/1193143003/diff/890001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler.h (right): https://codereview.chromium.org/1193143003/diff/890001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.h:41: // ui::SelectFileDialog::Listener implementation Please comment on |params| being in fact of type FileSelectorCaller. (In the unittest I suggested moving the enum to the header, so it will make sense to speak about it here.) https://codereview.chromium.org/1193143003/diff/890001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler_unittest.cc (right): https://codereview.chromium.org/1193143003/diff/890001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler_unittest.cc:218: FileSelected(IsEmptyPath(), 1, reinterpret_cast<void*>(0))); Here and on line 228 you are referencing FileSelectorCaller values, so let's move the enum definition to the header file and reference it by the value names instead of relying on 0 and 1 mapping to the right values. https://codereview.chromium.org/1193143003/diff/890001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler_unittest.cc:226: .WillRepeatedly(testing::Return(true)); Please also use Times(AtLeast(1)), to ensure that the code checks the authentication at least once. Using just WillRepeatedly() makes the default to be Times(AtLeast(0)).
lgtm https://codereview.chromium.org/1193143003/diff/890001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler_unittest.cc (right): https://codereview.chromium.org/1193143003/diff/890001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler_unittest.cc:217: EXPECT_CALL(*(handler_.get()), do you need this many parens? In fact I think *handler_ should work. https://codereview.chromium.org/1193143003/diff/890001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler_unittest.cc:224: EXPECT_CALL(*(static_cast<MockPasswordManagerPresenter*>(presenter_raw_)), do you need this many parens? https://codereview.chromium.org/1193143003/diff/890001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler_unittest.cc:227: EXPECT_CALL(*(handler_.get()), ditto
https://codereview.chromium.org/1193143003/diff/890001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler.h (right): https://codereview.chromium.org/1193143003/diff/890001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.h:41: // ui::SelectFileDialog::Listener implementation On 2016/04/12 08:55:39, vabr (Chromium) wrote: > Please comment on |params| being in fact of type FileSelectorCaller. > (In the unittest I suggested moving the enum to the header, so it will make > sense to speak about it here.) Done. https://codereview.chromium.org/1193143003/diff/890001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler_unittest.cc (right): https://codereview.chromium.org/1193143003/diff/890001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler_unittest.cc:217: EXPECT_CALL(*(handler_.get()), On 2016/04/15 20:52:42, Evan Stade wrote: > do you need this many parens? In fact I think *handler_ should work. Done. https://codereview.chromium.org/1193143003/diff/890001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler_unittest.cc:218: FileSelected(IsEmptyPath(), 1, reinterpret_cast<void*>(0))); On 2016/04/12 08:55:39, vabr (Chromium) wrote: > Here and on line 228 you are referencing FileSelectorCaller values, so let's > move the enum definition to the header file and reference it by the value names > instead of relying on 0 and 1 mapping to the right values. Done. https://codereview.chromium.org/1193143003/diff/890001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler_unittest.cc:224: EXPECT_CALL(*(static_cast<MockPasswordManagerPresenter*>(presenter_raw_)), On 2016/04/15 20:52:42, Evan Stade wrote: > do you need this many parens? Casting is needed here, otherwise the code won't compile. https://codereview.chromium.org/1193143003/diff/890001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler_unittest.cc:226: .WillRepeatedly(testing::Return(true)); On 2016/04/12 08:55:39, vabr (Chromium) wrote: > Please also use Times(AtLeast(1)), to ensure that the code checks the > authentication at least once. Using just WillRepeatedly() makes the default to > be Times(AtLeast(0)). Done. https://codereview.chromium.org/1193143003/diff/890001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler_unittest.cc:227: EXPECT_CALL(*(handler_.get()), On 2016/04/15 20:52:42, Evan Stade wrote: > ditto Done.
The CQ bit was checked by xunlu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gcasto@chromium.org, holte@chromium.org, vabr@chromium.org, estade@chromium.org Link to the patchset: https://codereview.chromium.org/1193143003/#ps910001 (title: "comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1193143003/910001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1193143003/910001
Message was sent while issue was closed.
Description was changed from ========== Enable import/export of passwords into/from Password Manager This change would allow users to bring their CSV-formatted passwords from major third party login managers into chrome password manager, and vice versa. BUG=341477 ========== to ========== Enable import/export of passwords into/from Password Manager This change would allow users to bring their CSV-formatted passwords from major third party login managers into chrome password manager, and vice versa. BUG=341477 ==========
Message was sent while issue was closed.
Committed patchset #25 (id:910001)
Message was sent while issue was closed.
Description was changed from ========== Enable import/export of passwords into/from Password Manager This change would allow users to bring their CSV-formatted passwords from major third party login managers into chrome password manager, and vice versa. BUG=341477 ========== to ========== Enable import/export of passwords into/from Password Manager This change would allow users to bring their CSV-formatted passwords from major third party login managers into chrome password manager, and vice versa. BUG=341477 Committed: https://crrev.com/d0f2f0767cf45adc6c6f0d854a991faf841857a9 Cr-Commit-Position: refs/heads/master@{#387844} ==========
Message was sent while issue was closed.
Patchset 25 (id:??) landed as https://crrev.com/d0f2f0767cf45adc6c6f0d854a991faf841857a9 Cr-Commit-Position: refs/heads/master@{#387844}
Message was sent while issue was closed.
finnur@chromium.org changed reviewers: + finnur@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1193143003/diff/910001/chrome/browser/resourc... File chrome/browser/resources/options/password_manager.js (right): https://codereview.chromium.org/1193143003/diff/910001/chrome/browser/resourc... chrome/browser/resources/options/password_manager.js:290: showImportExportButton_: function() { This fails the Closure compile: ## /media/largedrive/code/chromium/src/chrome/browser/resources/options/password_manager.js:290: ## ERROR - Private method exported by cr.makePublic() has no JSDoc. ## showImportExportButton_: function() { ## ^
Message was sent while issue was closed.
jyasskin@chromium.org changed reviewers: + jyasskin@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1193143003/diff/910001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler.cc (right): https://codereview.chromium.org/1193143003/diff/910001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:331: ANNOTATE_LEAKING_OBJECT_PTR(select_file_policy); Please have the test's SelectFileDialogFactory take ownership of the policy instead of leaking it.
Message was sent while issue was closed.
In addition to the 2 CLs mentioned below, a 3rd CL was uploaded to make the import/export available through about:flags (https://codereview.chromium.org/1900063002/). https://codereview.chromium.org/1193143003/diff/910001/chrome/browser/resourc... File chrome/browser/resources/options/password_manager.js (right): https://codereview.chromium.org/1193143003/diff/910001/chrome/browser/resourc... chrome/browser/resources/options/password_manager.js:290: showImportExportButton_: function() { On 2016/04/18 14:25:46, Finnur wrote: > This fails the Closure compile: > > ## > /media/largedrive/code/chromium/src/chrome/browser/resources/options/password_manager.js:290: > > ## ERROR - Private method exported by cr.makePublic() has no JSDoc. > ## showImportExportButton_: function() { > ## ^ This was fixed by dbeam@ in https://codereview.chromium.org/1895723003. https://codereview.chromium.org/1193143003/diff/910001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler.cc (right): https://codereview.chromium.org/1193143003/diff/910001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:331: ANNOTATE_LEAKING_OBJECT_PTR(select_file_policy); On 2016/04/19 00:57:04, Jeffrey Yasskin wrote: > Please have the test's SelectFileDialogFactory take ownership of the policy > instead of leaking it. Fix at https://codereview.chromium.org/1898143002. |
