Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
489103004:
Allow editing passwords in settings/passwords (Closed)
|
Created:
6 years, 4 months ago by jaekyeom Modified:
4 years, 3 months ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, gcasto+watchlist_chromium.org, arv+watch_chromium.org, mkwst+watchlist_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionAllow editing passwords in settings/passwords
BUG=377410
R=vabr@chromium.org,dbeam@chromium.org
TEST=In chrome://settings/passwords, revealed passwords should be editable.
Committed: https://crrev.com/41c3d488906019eb4cec3b43ad39e67e9ee9b92c
Cr-Commit-Position: refs/heads/master@{#295912}
Patch Set 1 #
Total comments: 4
Patch Set 2 : It supports overwriting and adding. Added the unit tests. #
Total comments: 10
Patch Set 3 : Added one more test. Modified the comments and the string. #
Total comments: 42
Patch Set 4 : comments addressed #
Total comments: 39
Patch Set 5 : comments addressed again #Patch Set 6 : modified a bit #Patch Set 7 : Exclude 2 tests on Android #
Total comments: 28
Patch Set 8 : comments from engedy #Patch Set 9 : Fixed a typo #Patch Set 10 : Removed a blank line #Patch Set 11 : Rebased the branch #Patch Set 12 : Fixed a type mismatch in password_manager.js #Messages
Total messages: 37 (6 generated)
PTAL. I didn’t add UMA “PasswordManager_UpdatePassword” since I guessed that I’m not allowed to do so.
Hi jaekyeom, As noted on the bug (http://crbug.com/377410#c4), this work is likely duplicating the work of engedy@, who I now add as a reviewer. I'm sorry for the inconvenience. Cheers, Vaclav
On 2014/08/21 12:14:44, vabr (Chromium) wrote: > Hi jaekyeom, > > As noted on the bug (http://crbug.com/377410#c4), this work is likely > duplicating the work of engedy@, who I now add as a reviewer. > > I'm sorry for the inconvenience. > > Cheers, > Vaclav On second thought, I only have a prototype so far which I would need to clean up anyway, so I think there is, in fact, not all that much duplication of work involved here. @jaekyeom, from my side, please feel free to proceed with this work. Just a few notes: * In the long run, we will probably want to support editing usernames as well. * We will probably want to support adding new credentials -- it is quite all right to have only the 3 fields, the others are optional anyway. * The optimal relation between "editing" and "showing" is still somewhat unclear to me. I agree that allowing to edit a hidden password is somewhat pointless, but allowing to completely overwrite an existing password without showing it might have some merit.
On 2014/08/21 16:57:17, engedy (OOO) wrote: > On 2014/08/21 12:14:44, vabr (Chromium) wrote: > > Hi jaekyeom, > > > > As noted on the bug (http://crbug.com/377410#c4), this work is likely > > duplicating the work of engedy@, who I now add as a reviewer. > > > > I'm sorry for the inconvenience. > > > > Cheers, > > Vaclav > > On second thought, I only have a prototype so far which I would need to clean up > anyway, so I think there is, in fact, not all that much duplication of work > involved here. @jaekyeom, from my side, please feel free to proceed with this > work. Hi, vabr and engedy. Thank you for your consideration. Then I will refer the following notes and proceed with this if you don’t mind. > > Just a few notes: > * In the long run, we will probably want to support editing usernames as well. > * We will probably want to support adding new credentials -- it is quite all > right to have only the 3 fields, the others are optional anyway. > * The optimal relation between "editing" and "showing" is still somewhat > unclear to me. I agree that allowing to edit a hidden password is somewhat > pointless, but allowing to completely overwrite an existing password without > showing it might have some merit. It seems nice to have that feature. I’m planning to add overwrite button on the left of the show button. Please let me know if you have a better solution/suggestion. Thanks, Jaekyeom.
Hi jaekyeom, Your changes to chrome/browser/ui/passwords/password_manager_presenter.*, based on what engedy@ said, sound reasonable. However, please add unit tests. Also, please do not forget you need dbeam@'s review for the other 6 files. Cheers, Vaclav https://codereview.chromium.org/489103004/diff/1/chrome/browser/ui/passwords/... File chrome/browser/ui/passwords/password_manager_presenter.h (right): https://codereview.chromium.org/489103004/diff/1/chrome/browser/ui/passwords/... chrome/browser/ui/passwords/password_manager_presenter.h:49: // Updates the password value of the entry at |index|. nit: This whole comment could be shortened to: "Updates the entry at |index| with |password_value|." https://codereview.chromium.org/489103004/diff/1/chrome/browser/ui/webui/opti... File chrome/browser/ui/webui/options/password_manager_handler.h (right): https://codereview.chromium.org/489103004/diff/1/chrome/browser/ui/webui/opti... chrome/browser/ui/webui/options/password_manager_handler.h:49: // Updates the password value of an entry. nit: Similarly, consider shortening the whole comment to: "Updates the entry at |index| with |password_value|."
Hi vabr and dbeam, Please take a look. It now supports overwriting hidden passwords and adding a new entry. Also I added the unit tests for the changes to PasswordManagerPresenter. Thanks, Jaekyeom. https://codereview.chromium.org/489103004/diff/1/chrome/browser/ui/passwords/... File chrome/browser/ui/passwords/password_manager_presenter.h (right): https://codereview.chromium.org/489103004/diff/1/chrome/browser/ui/passwords/... chrome/browser/ui/passwords/password_manager_presenter.h:49: // Updates the password value of the entry at |index|. On 2014/08/22 10:11:44, vabr (Chromium) wrote: > nit: This whole comment could be shortened to: > "Updates the entry at |index| with |password_value|." Done. https://codereview.chromium.org/489103004/diff/1/chrome/browser/ui/webui/opti... File chrome/browser/ui/webui/options/password_manager_handler.h (right): https://codereview.chromium.org/489103004/diff/1/chrome/browser/ui/webui/opti... chrome/browser/ui/webui/options/password_manager_handler.h:49: // Updates the password value of an entry. On 2014/08/22 10:11:44, vabr (Chromium) wrote: > nit: Similarly, consider shortening the whole comment to: > "Updates the entry at |index| with |password_value|." Done.
Hi jaekyeom, Your changes to chrome/browser/ui/passwords/password_manager_presenter.*, LGTM, with a couple of comments. Thanks for doing this, thus helping the password manager users! Cheers, Vaclav https://codereview.chromium.org/489103004/diff/20001/chrome/app/generated_res... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/489103004/diff/20001/chrome/app/generated_res... chrome/app/generated_resources.grd:7322: + URL Maybe "origin URL"? There are more URLs associated with a password (notably the action URL of the password form), and although the origin URL seems to make the most sense here anyway, it might help to clarify that. https://codereview.chromium.org/489103004/diff/20001/chrome/browser/ui/passwo... File chrome/browser/ui/passwords/password_manager_presenter.cc (right): https://codereview.chromium.org/489103004/diff/20001/chrome/browser/ui/passwo... chrome/browser/ui/passwords/password_manager_presenter.cc:102: // renderer, which shouldn't be saved to the password store. Is it "might come" or "can only come"? If I understood the code correctly, as long as the renderer is not compromised, it should not trigger calling this with an invalid origin or an empty password, no matter what the user types, correct? grammar nit: "which" in the second sentence seems to refer to the renderer, and saving a renderer in a password store does not make sense. I suggest not including the "which"-sentence at all -- it's apparent from the code that such input is not saved to the store. https://codereview.chromium.org/489103004/diff/20001/chrome/browser/ui/passwo... File chrome/browser/ui/passwords/password_manager_presenter.h (right): https://codereview.chromium.org/489103004/diff/20001/chrome/browser/ui/passwo... chrome/browser/ui/passwords/password_manager_presenter.h:54: void AddPassword(const GURL& origin, Please document restrictions on |origin| and |password_value| (to match the NOTREACHED() checks in the .cc file). https://codereview.chromium.org/489103004/diff/20001/chrome/browser/ui/passwo... chrome/browser/ui/passwords/password_manager_presenter.h:59: void UpdatePassword(size_t index, const base::string16& password_value); Please document restrictions on |password_value| (to match the NOTREACHED() check in the .cc file). https://codereview.chromium.org/489103004/diff/20001/chrome/browser/ui/passwo... File chrome/browser/ui/passwords/password_manager_presenter_unittest.cc (right): https://codereview.chromium.org/489103004/diff/20001/chrome/browser/ui/passwo... chrome/browser/ui/passwords/password_manager_presenter_unittest.cc:229: Could you please also add a simple test for CheckOriginValidityForAdding, with a couple of valid and invalid origins?
Thank you for your review! dbeam@ Could you please give me a review for the other files? Thanks, Jaekyeom. https://codereview.chromium.org/489103004/diff/20001/chrome/app/generated_res... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/489103004/diff/20001/chrome/app/generated_res... chrome/app/generated_resources.grd:7322: + URL On 2014/08/25 07:35:07, vabr (Chromium) wrote: > Maybe "origin URL"? > There are more URLs associated with a password (notably the action URL of the > password form), and although the origin URL seems to make the most sense here > anyway, it might help to clarify that. Done. https://codereview.chromium.org/489103004/diff/20001/chrome/browser/ui/passwo... File chrome/browser/ui/passwords/password_manager_presenter.cc (right): https://codereview.chromium.org/489103004/diff/20001/chrome/browser/ui/passwo... chrome/browser/ui/passwords/password_manager_presenter.cc:102: // renderer, which shouldn't be saved to the password store. On 2014/08/25 07:35:07, vabr (Chromium) wrote: > Is it "might come" or "can only come"? If I understood the code correctly, as > long as the renderer is not compromised, it should not trigger calling this with > an invalid origin or an empty password, no matter what the user types, correct? > > grammar nit: "which" in the second sentence seems to refer to the renderer, and > saving a renderer in a password store does not make sense. I suggest not > including the "which"-sentence at all -- it's apparent from the code that such > input is not saved to the store. Done. https://codereview.chromium.org/489103004/diff/20001/chrome/browser/ui/passwo... File chrome/browser/ui/passwords/password_manager_presenter.h (right): https://codereview.chromium.org/489103004/diff/20001/chrome/browser/ui/passwo... chrome/browser/ui/passwords/password_manager_presenter.h:54: void AddPassword(const GURL& origin, On 2014/08/25 07:35:07, vabr (Chromium) wrote: > Please document restrictions on |origin| and |password_value| (to match the > NOTREACHED() checks in the .cc file). Done. https://codereview.chromium.org/489103004/diff/20001/chrome/browser/ui/passwo... chrome/browser/ui/passwords/password_manager_presenter.h:59: void UpdatePassword(size_t index, const base::string16& password_value); On 2014/08/25 07:35:07, vabr (Chromium) wrote: > Please document restrictions on |password_value| (to match the NOTREACHED() > check in the .cc file). Done. https://codereview.chromium.org/489103004/diff/20001/chrome/browser/ui/passwo... File chrome/browser/ui/passwords/password_manager_presenter_unittest.cc (right): https://codereview.chromium.org/489103004/diff/20001/chrome/browser/ui/passwo... chrome/browser/ui/passwords/password_manager_presenter_unittest.cc:229: On 2014/08/25 07:35:07, vabr (Chromium) wrote: > Could you please also add a simple test for CheckOriginValidityForAdding, with a > couple of valid and invalid origins? Done.
Thanks for addressing the comments, chrome/browser/ui/passwords/password_manager_presenter.* in patch set 3 still LGTM. Cheers, Vaclav
https://codereview.chromium.org/489103004/diff/40001/chrome/browser/resources... File chrome/browser/resources/options/password_manager.js (right): https://codereview.chromium.org/489103004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/password_manager.js:218: * @param {boolean} valid The validity of the origin for adding. @private https://codereview.chromium.org/489103004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/password_manager.js:224: var model = this.savedPasswordsList_.dataModel; what if model.length == 0? https://codereview.chromium.org/489103004/diff/40001/chrome/browser/resources... File chrome/browser/resources/options/password_manager_list.css (right): https://codereview.chromium.org/489103004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.css:19: #saved-passwords-list .list-inline-button { -webkit-margin-end: 2px; https://codereview.chromium.org/489103004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.css:33: } ^ and remove these 2 blocks https://codereview.chromium.org/489103004/diff/40001/chrome/browser/resources... File chrome/browser/resources/options/password_manager_list.js (right): https://codereview.chromium.org/489103004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.js:48: // The password. ^ i don't think this style of comments is very useful https://codereview.chromium.org/489103004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.js:70: }); passwordInput.addEventListener('input', function(event) { this.updatePasswordValidity(); }.bind(this)); https://codereview.chromium.org/489103004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.js:118: this.addEventListener('canceledit', this.onEditCancelled_); s/onEditCancelled_/resetInputs/ https://codereview.chromium.org/489103004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.js:119: this.addEventListener('commitedit', this.onEditCommitted_); s/onEditCommitted_/finishEdit/ https://codereview.chromium.org/489103004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.js:124: * @return {HTMLElement} The URL element. @protected https://codereview.chromium.org/489103004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.js:146: * @return {HTMLElement} The username element. @protected on all the methods you @override later (or @private if they're not overridden) https://codereview.chromium.org/489103004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.js:167: input.classList.remove('inactive-password'); remove curlies https://codereview.chromium.org/489103004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.js:248: // Does nothing here. ^ probably OK to remove this comment https://codereview.chromium.org/489103004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.js:346: return url ? url : ''; return this.dataItem[0] || ''; https://codereview.chromium.org/489103004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.js:358: return username ? username : ''; return this.dataItem[1] || ''; https://codereview.chromium.org/489103004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.js:370: return password ? password : ''; return this.dataItem[2] || ''; https://codereview.chromium.org/489103004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.js:380: * @param {boolean} showPasswords Kept for the consistency. what does "Kept for the consistency" mean? https://codereview.chromium.org/489103004/diff/40001/chrome/browser/ui/passwo... File chrome/browser/ui/passwords/password_manager_presenter.cc (right): https://codereview.chromium.org/489103004/diff/40001/chrome/browser/ui/passwo... chrome/browser/ui/passwords/password_manager_presenter.cc:98: const base::string16& password_value) { #if defined(OS_ANDROID) NOTREACHED(); #else ... #endif https://codereview.chromium.org/489103004/diff/40001/chrome/browser/ui/passwo... chrome/browser/ui/passwords/password_manager_presenter.cc:106: PasswordStore* store = GetPasswordStore(); when is |store| NULL? https://codereview.chromium.org/489103004/diff/40001/chrome/browser/ui/passwo... chrome/browser/ui/passwords/password_manager_presenter.cc:130: #if !defined(OS_ANDROID) // This is never called on Android. same re: NOTREACHED() https://codereview.chromium.org/489103004/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/password_manager_handler.cc (right): https://codereview.chromium.org/489103004/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/password_manager_handler.cc:147: !args->GetString(2, &password_value)) curlies https://codereview.chromium.org/489103004/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/password_manager_handler.h (right): https://codereview.chromium.org/489103004/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/password_manager_handler.h:49: // Checks if |origin| is valid for adding a new entry. Called while the user nit: 1 \s between sentences
Thanks for the review. Please take another look. Thanks, Jaekyeom. https://codereview.chromium.org/489103004/diff/40001/chrome/browser/resources... File chrome/browser/resources/options/password_manager.js (right): https://codereview.chromium.org/489103004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/password_manager.js:218: * @param {boolean} valid The validity of the origin for adding. On 2014/08/26 17:39:05, Dan Beam wrote: > @private Done. https://codereview.chromium.org/489103004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/password_manager.js:224: var model = this.savedPasswordsList_.dataModel; On 2014/08/26 17:39:05, Dan Beam wrote: > what if model.length == 0? Done. I modified the condition a bit and added a comment. https://codereview.chromium.org/489103004/diff/40001/chrome/browser/resources... File chrome/browser/resources/options/password_manager_list.css (right): https://codereview.chromium.org/489103004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.css:19: #saved-passwords-list .list-inline-button { On 2014/08/26 17:39:06, Dan Beam wrote: > -webkit-margin-end: 2px; Done. https://codereview.chromium.org/489103004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.css:33: } On 2014/08/26 17:39:06, Dan Beam wrote: > ^ and remove these 2 blocks Done. https://codereview.chromium.org/489103004/diff/40001/chrome/browser/resources... File chrome/browser/resources/options/password_manager_list.js (right): https://codereview.chromium.org/489103004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.js:48: // The password. On 2014/08/26 17:39:06, Dan Beam wrote: > ^ i don't think this style of comments is very useful Done. https://codereview.chromium.org/489103004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.js:70: }); On 2014/08/26 17:39:06, Dan Beam wrote: > passwordInput.addEventListener('input', function(event) { > this.updatePasswordValidity(); > }.bind(this)); Done. https://codereview.chromium.org/489103004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.js:118: this.addEventListener('canceledit', this.onEditCancelled_); On 2014/08/26 17:39:06, Dan Beam wrote: > s/onEditCancelled_/resetInputs/ Done. https://codereview.chromium.org/489103004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.js:119: this.addEventListener('commitedit', this.onEditCommitted_); On 2014/08/26 17:39:06, Dan Beam wrote: > s/onEditCommitted_/finishEdit/ Done. https://codereview.chromium.org/489103004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.js:124: * @return {HTMLElement} The URL element. On 2014/08/26 17:39:06, Dan Beam wrote: > @protected Done. https://codereview.chromium.org/489103004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.js:146: * @return {HTMLElement} The username element. On 2014/08/26 17:39:06, Dan Beam wrote: > @protected on all the methods you @override later (or @private if they're not > overridden) Done. https://codereview.chromium.org/489103004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.js:167: input.classList.remove('inactive-password'); On 2014/08/26 17:39:06, Dan Beam wrote: > remove curlies Done. https://codereview.chromium.org/489103004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.js:248: // Does nothing here. On 2014/08/26 17:39:06, Dan Beam wrote: > ^ probably OK to remove this comment Done. https://codereview.chromium.org/489103004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.js:346: return url ? url : ''; On 2014/08/26 17:39:06, Dan Beam wrote: > return this.dataItem[0] || ''; Done. https://codereview.chromium.org/489103004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.js:358: return username ? username : ''; On 2014/08/26 17:39:06, Dan Beam wrote: > return this.dataItem[1] || ''; Done. https://codereview.chromium.org/489103004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.js:370: return password ? password : ''; On 2014/08/26 17:39:06, Dan Beam wrote: > return this.dataItem[2] || ''; Done. https://codereview.chromium.org/489103004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.js:380: * @param {boolean} showPasswords Kept for the consistency. On 2014/08/26 17:39:06, Dan Beam wrote: > what does "Kept for the consistency" mean? Sorry, it was unclear. |showPasswords| was meaningless to PasswordAddRowListItem, but it was passed to PasswordListItem.prototype.decorate in order to make all PasswordListItems get the same |showPasswords| value. But that also seemed meaningless so I've removed it. https://codereview.chromium.org/489103004/diff/40001/chrome/browser/ui/passwo... File chrome/browser/ui/passwords/password_manager_presenter.cc (right): https://codereview.chromium.org/489103004/diff/40001/chrome/browser/ui/passwo... chrome/browser/ui/passwords/password_manager_presenter.cc:98: const base::string16& password_value) { On 2014/08/26 17:39:06, Dan Beam wrote: > #if defined(OS_ANDROID) > NOTREACHED(); > #else > ... > #endif Done. https://codereview.chromium.org/489103004/diff/40001/chrome/browser/ui/passwo... chrome/browser/ui/passwords/password_manager_presenter.cc:106: PasswordStore* store = GetPasswordStore(); On 2014/08/26 17:39:07, Dan Beam wrote: > when is |store| NULL? I believe that it will be NULL during testing without calling SetTestingFactory(). PasswordStoreFactory::ServiceIsNULLWhileTesting() returns true. https://codereview.chromium.org/489103004/diff/40001/chrome/browser/ui/passwo... chrome/browser/ui/passwords/password_manager_presenter.cc:130: #if !defined(OS_ANDROID) // This is never called on Android. On 2014/08/26 17:39:06, Dan Beam wrote: > same re: NOTREACHED() Done. https://codereview.chromium.org/489103004/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/password_manager_handler.cc (right): https://codereview.chromium.org/489103004/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/password_manager_handler.cc:147: !args->GetString(2, &password_value)) On 2014/08/26 17:39:07, Dan Beam wrote: > curlies Done. https://codereview.chromium.org/489103004/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/password_manager_handler.h (right): https://codereview.chromium.org/489103004/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/password_manager_handler.h:49: // Checks if |origin| is valid for adding a new entry. Called while the user On 2014/08/26 17:39:07, Dan Beam wrote: > nit: 1 \s between sentences Done. However I've wanted to know if there is a guide/decision about this style, because AFAIK 2 spaces after a period are common in Chromium C++ codebase. (In CS): //.*[.]\s{2}\S file:.+[.](cc|h) And they also appear in the examples in Google C++ Style Guide.
Ping?? On 2014/08/27 12:28:54, jaekyeom wrote: > Thanks for the review. > Please take another look. > > Thanks, > Jaekyeom. > > https://codereview.chromium.org/489103004/diff/40001/chrome/browser/resources... > File chrome/browser/resources/options/password_manager.js (right): > > https://codereview.chromium.org/489103004/diff/40001/chrome/browser/resources... > chrome/browser/resources/options/password_manager.js:218: * @param {boolean} > valid The validity of the origin for adding. > On 2014/08/26 17:39:05, Dan Beam wrote: > > @private > > Done. > > https://codereview.chromium.org/489103004/diff/40001/chrome/browser/resources... > chrome/browser/resources/options/password_manager.js:224: var model = > this.savedPasswordsList_.dataModel; > On 2014/08/26 17:39:05, Dan Beam wrote: > > what if model.length == 0? > > Done. > I modified the condition a bit and added a comment. > > https://codereview.chromium.org/489103004/diff/40001/chrome/browser/resources... > File chrome/browser/resources/options/password_manager_list.css (right): > > https://codereview.chromium.org/489103004/diff/40001/chrome/browser/resources... > chrome/browser/resources/options/password_manager_list.css:19: > #saved-passwords-list .list-inline-button { > On 2014/08/26 17:39:06, Dan Beam wrote: > > -webkit-margin-end: 2px; > > Done. > > https://codereview.chromium.org/489103004/diff/40001/chrome/browser/resources... > chrome/browser/resources/options/password_manager_list.css:33: } > On 2014/08/26 17:39:06, Dan Beam wrote: > > ^ and remove these 2 blocks > > Done. > > https://codereview.chromium.org/489103004/diff/40001/chrome/browser/resources... > File chrome/browser/resources/options/password_manager_list.js (right): > > https://codereview.chromium.org/489103004/diff/40001/chrome/browser/resources... > chrome/browser/resources/options/password_manager_list.js:48: // The password. > On 2014/08/26 17:39:06, Dan Beam wrote: > > ^ i don't think this style of comments is very useful > > Done. > > https://codereview.chromium.org/489103004/diff/40001/chrome/browser/resources... > chrome/browser/resources/options/password_manager_list.js:70: }); > On 2014/08/26 17:39:06, Dan Beam wrote: > > passwordInput.addEventListener('input', function(event) { > > this.updatePasswordValidity(); > > }.bind(this)); > > Done. > > https://codereview.chromium.org/489103004/diff/40001/chrome/browser/resources... > chrome/browser/resources/options/password_manager_list.js:118: > this.addEventListener('canceledit', this.onEditCancelled_); > On 2014/08/26 17:39:06, Dan Beam wrote: > > s/onEditCancelled_/resetInputs/ > > Done. > > https://codereview.chromium.org/489103004/diff/40001/chrome/browser/resources... > chrome/browser/resources/options/password_manager_list.js:119: > this.addEventListener('commitedit', this.onEditCommitted_); > On 2014/08/26 17:39:06, Dan Beam wrote: > > s/onEditCommitted_/finishEdit/ > > Done. > > https://codereview.chromium.org/489103004/diff/40001/chrome/browser/resources... > chrome/browser/resources/options/password_manager_list.js:124: * @return > {HTMLElement} The URL element. > On 2014/08/26 17:39:06, Dan Beam wrote: > > @protected > > Done. > > https://codereview.chromium.org/489103004/diff/40001/chrome/browser/resources... > chrome/browser/resources/options/password_manager_list.js:146: * @return > {HTMLElement} The username element. > On 2014/08/26 17:39:06, Dan Beam wrote: > > @protected on all the methods you @override later (or @private if they're not > > overridden) > > Done. > > https://codereview.chromium.org/489103004/diff/40001/chrome/browser/resources... > chrome/browser/resources/options/password_manager_list.js:167: > input.classList.remove('inactive-password'); > On 2014/08/26 17:39:06, Dan Beam wrote: > > remove curlies > > Done. > > https://codereview.chromium.org/489103004/diff/40001/chrome/browser/resources... > chrome/browser/resources/options/password_manager_list.js:248: // Does nothing > here. > On 2014/08/26 17:39:06, Dan Beam wrote: > > ^ probably OK to remove this comment > > Done. > > https://codereview.chromium.org/489103004/diff/40001/chrome/browser/resources... > chrome/browser/resources/options/password_manager_list.js:346: return url ? url > : ''; > On 2014/08/26 17:39:06, Dan Beam wrote: > > return this.dataItem[0] || ''; > > Done. > > https://codereview.chromium.org/489103004/diff/40001/chrome/browser/resources... > chrome/browser/resources/options/password_manager_list.js:358: return username ? > username : ''; > On 2014/08/26 17:39:06, Dan Beam wrote: > > return this.dataItem[1] || ''; > > Done. > > https://codereview.chromium.org/489103004/diff/40001/chrome/browser/resources... > chrome/browser/resources/options/password_manager_list.js:370: return password ? > password : ''; > On 2014/08/26 17:39:06, Dan Beam wrote: > > return this.dataItem[2] || ''; > > Done. > > https://codereview.chromium.org/489103004/diff/40001/chrome/browser/resources... > chrome/browser/resources/options/password_manager_list.js:380: * @param > {boolean} showPasswords Kept for the consistency. > On 2014/08/26 17:39:06, Dan Beam wrote: > > what does "Kept for the consistency" mean? > > Sorry, it was unclear. > |showPasswords| was meaningless to PasswordAddRowListItem, but it was passed to > PasswordListItem.prototype.decorate in order to make all PasswordListItems get > the same |showPasswords| value. > But that also seemed meaningless so I've removed it. > > https://codereview.chromium.org/489103004/diff/40001/chrome/browser/ui/passwo... > File chrome/browser/ui/passwords/password_manager_presenter.cc (right): > > https://codereview.chromium.org/489103004/diff/40001/chrome/browser/ui/passwo... > chrome/browser/ui/passwords/password_manager_presenter.cc:98: const > base::string16& password_value) { > On 2014/08/26 17:39:06, Dan Beam wrote: > > #if defined(OS_ANDROID) > > NOTREACHED(); > > #else > > ... > > #endif > > Done. > > https://codereview.chromium.org/489103004/diff/40001/chrome/browser/ui/passwo... > chrome/browser/ui/passwords/password_manager_presenter.cc:106: PasswordStore* > store = GetPasswordStore(); > On 2014/08/26 17:39:07, Dan Beam wrote: > > when is |store| NULL? > > I believe that it will be NULL during testing without calling > SetTestingFactory(). > PasswordStoreFactory::ServiceIsNULLWhileTesting() returns true. > > https://codereview.chromium.org/489103004/diff/40001/chrome/browser/ui/passwo... > chrome/browser/ui/passwords/password_manager_presenter.cc:130: #if > !defined(OS_ANDROID) // This is never called on Android. > On 2014/08/26 17:39:06, Dan Beam wrote: > > same re: NOTREACHED() > > Done. > > https://codereview.chromium.org/489103004/diff/40001/chrome/browser/ui/webui/... > File chrome/browser/ui/webui/options/password_manager_handler.cc (right): > > https://codereview.chromium.org/489103004/diff/40001/chrome/browser/ui/webui/... > chrome/browser/ui/webui/options/password_manager_handler.cc:147: > !args->GetString(2, &password_value)) > On 2014/08/26 17:39:07, Dan Beam wrote: > > curlies > > Done. > > https://codereview.chromium.org/489103004/diff/40001/chrome/browser/ui/webui/... > File chrome/browser/ui/webui/options/password_manager_handler.h (right): > > https://codereview.chromium.org/489103004/diff/40001/chrome/browser/ui/webui/... > chrome/browser/ui/webui/options/password_manager_handler.h:49: // Checks if > |origin| is valid for adding a new entry. Called while the user > On 2014/08/26 17:39:07, Dan Beam wrote: > > nit: 1 \s between sentences > > Done. > However I've wanted to know if there is a guide/decision about this style, > because AFAIK 2 spaces after a period are common in Chromium C++ codebase. (In > CS): //.*[.]\s{2}\S file:.+[.](cc|h) > And they also appear in the examples in Google C++ Style Guide.
sorry for the lag https://codereview.chromium.org/489103004/diff/60001/chrome/browser/resources... File chrome/browser/resources/options/password_manager.js (right): https://codereview.chromium.org/489103004/diff/60001/chrome/browser/resources... chrome/browser/resources/options/password_manager.js:233: var model = this.savedPasswordsList_.dataModel; assert(model.length > 0); https://codereview.chromium.org/489103004/diff/60001/chrome/browser/resources... File chrome/browser/resources/options/password_manager_list.css (right): https://codereview.chromium.org/489103004/diff/60001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.css:12: right: 0; nit: just put this in the default rule, e.g. #saved-passwords-list .list-inline-buttons-container { display: flex; position: absolute; right: 0; top: 3px; } html[dir='rtl'] #saved-passwords-list .list-inline-buttons-container { left: 0; right: auto; } https://codereview.chromium.org/489103004/diff/60001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.css:38: -webkit-padding-end: 1ex; why ex over em? https://codereview.chromium.org/489103004/diff/60001/chrome/browser/resources... File chrome/browser/resources/options/password_manager_list.js (right): https://codereview.chromium.org/489103004/diff/60001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.js:126: urlEl.classList.add('url'); urlEl.className += ' all the classes' https://codereview.chromium.org/489103004/diff/60001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.js:145: setupUsernameElement: function() { nit: why not createUsernameElement? https://codereview.chromium.org/489103004/diff/60001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.js:166: input.classList.add('inactive-password'); nit: input.classList.toggle('inactive-password', !this.selected); https://codereview.chromium.org/489103004/diff/60001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.js:245: * @param {boolean} valid The validity of the URL. can this be @protected? https://codereview.chromium.org/489103004/diff/60001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.js:443: PasswordManager.addPassword(newUrl, newUsername, newPassword); it'd arguably be better for this list element not to know about the password manager, but i don't care that much https://codereview.chromium.org/489103004/diff/60001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.js:461: this.urlField.setCustomValidity(valid ? '' : ' '); what does the ' ' do? https://codereview.chromium.org/489103004/diff/60001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.js:565: if (!entry) why not just create the item directly instead of adding this kind of funky code...? https://codereview.chromium.org/489103004/diff/60001/chrome/browser/ui/passwo... File chrome/browser/ui/passwords/password_manager_presenter.cc (right): https://codereview.chromium.org/489103004/diff/60001/chrome/browser/ui/passwo... chrome/browser/ui/passwords/password_manager_presenter.cc:122: form.ssl_valid = origin.SchemeIsSecure(); are all secure schemes SSL encrypted? does an HTTPS origin mean the SSL is actually valid? https://codereview.chromium.org/489103004/diff/60001/chrome/browser/ui/passwo... File chrome/browser/ui/passwords/password_manager_presenter.h (right): https://codereview.chromium.org/489103004/diff/60001/chrome/browser/ui/passwo... chrome/browser/ui/passwords/password_manager_presenter.h:10: #include "base/basictypes.h" // For size_t. https://codereview.chromium.org/489103004/diff/60001/chrome/browser/ui/passwo... chrome/browser/ui/passwords/password_manager_presenter.h:18: } forward declare or include headers for: - base::string16 - GURL https://codereview.chromium.org/489103004/diff/60001/chrome/browser/ui/passwo... chrome/browser/ui/passwords/password_manager_presenter.h:33: ^ static would probably go here https://codereview.chromium.org/489103004/diff/60001/chrome/browser/ui/passwo... chrome/browser/ui/passwords/password_manager_presenter.h:50: static bool CheckOriginValidityForAdding(const GURL& origin); nit: move static method right under ctor https://codereview.chromium.org/489103004/diff/60001/chrome/browser/ui/passwo... chrome/browser/ui/passwords/password_manager_presenter.h:53: // |password_value|. |origin| should have been validated by nit: 1 \s between sentences https://codereview.chromium.org/489103004/diff/60001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/password_manager_handler.cc (right): https://codereview.chromium.org/489103004/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/password_manager_handler.cc:147: !args->GetString(2, &password_value)) { NOTREACHED(); https://codereview.chromium.org/489103004/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/password_manager_handler.cc:156: if (!ExtractIntegerValue(args, &index) || index < 0) NOTREACHED(); https://codereview.chromium.org/489103004/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/password_manager_handler.cc:163: } } else { NOTREACHED(); }
Thank you, PTAL. And please read my replies to some of your comments. Thanks, Jaekyeom. https://codereview.chromium.org/489103004/diff/60001/chrome/browser/resources... File chrome/browser/resources/options/password_manager.js (right): https://codereview.chromium.org/489103004/diff/60001/chrome/browser/resources... chrome/browser/resources/options/password_manager.js:233: var model = this.savedPasswordsList_.dataModel; On 2014/09/11 05:17:57, Dan Beam wrote: > assert(model.length > 0); Done. https://codereview.chromium.org/489103004/diff/60001/chrome/browser/resources... File chrome/browser/resources/options/password_manager_list.css (right): https://codereview.chromium.org/489103004/diff/60001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.css:12: right: 0; On 2014/09/11 05:17:57, Dan Beam wrote: > nit: just put this in the default rule, e.g. > > #saved-passwords-list .list-inline-buttons-container { > display: flex; > position: absolute; > right: 0; > top: 3px; > } > > html[dir='rtl'] #saved-passwords-list .list-inline-buttons-container { > left: 0; > right: auto; > } Done. https://codereview.chromium.org/489103004/diff/60001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.css:38: -webkit-padding-end: 1ex; On 2014/09/11 05:17:57, Dan Beam wrote: > why ex over em? Done. https://codereview.chromium.org/489103004/diff/60001/chrome/browser/resources... File chrome/browser/resources/options/password_manager_list.js (right): https://codereview.chromium.org/489103004/diff/60001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.js:126: urlEl.classList.add('url'); On 2014/09/11 05:17:57, Dan Beam wrote: > urlEl.className += ' all the classes' Done. https://codereview.chromium.org/489103004/diff/60001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.js:145: setupUsernameElement: function() { On 2014/09/11 05:17:57, Dan Beam wrote: > nit: why not createUsernameElement? Done. https://codereview.chromium.org/489103004/diff/60001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.js:166: input.classList.add('inactive-password'); On 2014/09/11 05:17:57, Dan Beam wrote: > nit: input.classList.toggle('inactive-password', !this.selected); Done. https://codereview.chromium.org/489103004/diff/60001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.js:245: * @param {boolean} valid The validity of the URL. On 2014/09/11 05:17:57, Dan Beam wrote: > can this be @protected? No, it can't. It is called by PasswordManager. https://codereview.chromium.org/489103004/diff/60001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.js:443: PasswordManager.addPassword(newUrl, newUsername, newPassword); On 2014/09/11 05:17:57, Dan Beam wrote: > it'd arguably be better for this list element not to know about the password > manager, but i don't care that much Actually, I did this way since the original codes had used the password manager as the interface to the browser. e.g. Call requestShowPassword -> get the result via showPassword https://codereview.chromium.org/489103004/diff/60001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.js:461: this.urlField.setCustomValidity(valid ? '' : ' '); On 2014/09/11 05:17:57, Dan Beam wrote: > what does the ' ' do? Done. I added the texts. https://codereview.chromium.org/489103004/diff/60001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.js:565: if (!entry) On 2014/09/11 05:17:57, Dan Beam wrote: > why not just create the item directly instead of adding this kind of funky > code...? What does "create the item directly" mean? Using just appendChild..? I believe it won't work correctly unless |dataModel| has an entry corresponding to the placeholder item. https://codereview.chromium.org/489103004/diff/60001/chrome/browser/ui/passwo... File chrome/browser/ui/passwords/password_manager_presenter.cc (right): https://codereview.chromium.org/489103004/diff/60001/chrome/browser/ui/passwo... chrome/browser/ui/passwords/password_manager_presenter.cc:122: form.ssl_valid = origin.SchemeIsSecure(); On 2014/09/11 05:17:58, Dan Beam wrote: > are all secure schemes SSL encrypted? does an HTTPS origin mean the SSL is > actually valid? Done. I added the comment. https://codereview.chromium.org/489103004/diff/60001/chrome/browser/ui/passwo... File chrome/browser/ui/passwords/password_manager_presenter.h (right): https://codereview.chromium.org/489103004/diff/60001/chrome/browser/ui/passwo... chrome/browser/ui/passwords/password_manager_presenter.h:10: On 2014/09/11 05:17:58, Dan Beam wrote: > #include "base/basictypes.h" // For size_t. Done. https://codereview.chromium.org/489103004/diff/60001/chrome/browser/ui/passwo... chrome/browser/ui/passwords/password_manager_presenter.h:18: } On 2014/09/11 05:17:58, Dan Beam wrote: > forward declare or include headers for: > - base::string16 > - GURL Done. https://codereview.chromium.org/489103004/diff/60001/chrome/browser/ui/passwo... chrome/browser/ui/passwords/password_manager_presenter.h:33: On 2014/09/11 05:17:58, Dan Beam wrote: > ^ static would probably go here Done. https://codereview.chromium.org/489103004/diff/60001/chrome/browser/ui/passwo... chrome/browser/ui/passwords/password_manager_presenter.h:50: static bool CheckOriginValidityForAdding(const GURL& origin); On 2014/09/11 05:17:58, Dan Beam wrote: > nit: move static method right under ctor Done. https://codereview.chromium.org/489103004/diff/60001/chrome/browser/ui/passwo... chrome/browser/ui/passwords/password_manager_presenter.h:53: // |password_value|. |origin| should have been validated by On 2014/09/11 05:17:58, Dan Beam wrote: > nit: 1 \s between sentences Done. https://codereview.chromium.org/489103004/diff/60001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/password_manager_handler.cc (right): https://codereview.chromium.org/489103004/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/password_manager_handler.cc:147: !args->GetString(2, &password_value)) { On 2014/09/11 05:17:58, Dan Beam wrote: > NOTREACHED(); Done. https://codereview.chromium.org/489103004/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/password_manager_handler.cc:156: if (!ExtractIntegerValue(args, &index) || index < 0) On 2014/09/11 05:17:58, Dan Beam wrote: > NOTREACHED(); Done. https://codereview.chromium.org/489103004/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/password_manager_handler.cc:163: } On 2014/09/11 05:17:58, Dan Beam wrote: > } else { > NOTREACHED(); > } Done.
lgtm https://codereview.chromium.org/489103004/diff/60001/chrome/browser/resources... File chrome/browser/resources/options/password_manager_list.js (right): https://codereview.chromium.org/489103004/diff/60001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.js:565: if (!entry) On 2014/09/12 10:35:57, jaekyeom wrote: > On 2014/09/11 05:17:57, Dan Beam wrote: > > why not just create the item directly instead of adding this kind of funky > > code...? > > What does "create the item directly" mean? Using just appendChild..? > I believe it won't work correctly unless |dataModel| has an entry corresponding > to the placeholder item. ah, i see
The CQ bit was checked by btapiz@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/489103004/100001
The CQ bit was unchecked by btapiz@gmail.com
Thank you for the approval. By the way, I made a little change on password_manager_presenter_unittest.cc, since AddPassword and UpdatePassword are not reachable on Android. So this need to be approved; vabr@ could you take a look again? Thanks, Jaekyeom.
Back from OOO. I have added a few minor comments, but all-an-all, very nice work, thank you! https://codereview.chromium.org/489103004/diff/120001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/489103004/diff/120001/chrome/app/generated_re... chrome/app/generated_resources.grd:7321: + <message name="IDS_PASSWORDS_PAGE_URL_INSTRUCTION" desc="Placeholder text shown in the URL field of the Add New Entry row"> Note that the translators will not receive the resource codenames or any context other than the "desc" attribute. Therefore it is very important to include all relevant information there. With this in mind, could you please elaborate more here and in the other new strings, and make sure to mention that it is for the password page? E.g., here we could write: desc="Placeholder text shown in the URL field when manually adding new credentials on the passwords page in settings." https://codereview.chromium.org/489103004/diff/120001/chrome/app/generated_re... chrome/app/generated_resources.grd:7322: + Origin URL We have discussed this with Vaclav in depth, and after some consideration, we think this might be too technical and impossible to translate. It would be better to say "Login page URL". What do you think? https://codereview.chromium.org/489103004/diff/120001/chrome/app/generated_re... chrome/app/generated_resources.grd:7331: + Must be a valid HTTP or HTTPS URL nit: Login page URL must be... https://codereview.chromium.org/489103004/diff/120001/chrome/browser/resource... File chrome/browser/resources/options/password_manager_list.js (right): https://codereview.chromium.org/489103004/diff/120001/chrome/browser/resource... chrome/browser/resources/options/password_manager_list.js:85: 'list-inline-button custom-appearance overwrite-button'; nit: Do we use the 'overwrite-button' class anywhere? If not, we should remove it. https://codereview.chromium.org/489103004/diff/120001/chrome/browser/ui/passw... File chrome/browser/ui/passwords/password_manager_presenter.h (right): https://codereview.chromium.org/489103004/diff/120001/chrome/browser/ui/passw... chrome/browser/ui/passwords/password_manager_presenter.h:11: #include "base/basictypes.h" nit: basictypes.h is deprecated. Please include <stddef.h> directly. https://codereview.chromium.org/489103004/diff/120001/chrome/browser/ui/passw... File chrome/browser/ui/passwords/password_manager_presenter_unittest.cc (right): https://codereview.chromium.org/489103004/diff/120001/chrome/browser/ui/passw... chrome/browser/ui/passwords/password_manager_presenter_unittest.cc:5: #include "base/macros.h" Do we need this? https://codereview.chromium.org/489103004/diff/120001/chrome/browser/ui/passw... chrome/browser/ui/passwords/password_manager_presenter_unittest.cc:6: #include "base/memory/ref_counted.h" I think we can drop this, as this must already be included from mock_password_store_service.h, otherwise it would not compile -- and we only need it for calling this. https://codereview.chromium.org/489103004/diff/120001/chrome/browser/ui/passw... chrome/browser/ui/passwords/password_manager_presenter_unittest.cc:18: using testing::AllOf; nit: Conventially, we only introduce a using directive if it can save lines in total. https://codereview.chromium.org/489103004/diff/120001/chrome/browser/ui/passw... chrome/browser/ui/passwords/password_manager_presenter_unittest.cc:176: GURL complex_origin("https://foo:bar@host.com/path?query=v#ref"); Please add a custom port as well (and update the expectations on |signon_realm| and |origin|). https://codereview.chromium.org/489103004/diff/120001/chrome/browser/ui/passw... chrome/browser/ui/passwords/password_manager_presenter_unittest.cc:193: std::string username1 = "username"; nit: Conventionally, we use const char kUsername1[] = "..."; in these cases. https://codereview.chromium.org/489103004/diff/120001/chrome/browser/ui/passw... chrome/browser/ui/passwords/password_manager_presenter_unittest.cc:239: "https://foo:bar@host.com/path?query=v#ref" Please also add an URL with a custom a port specified. https://codereview.chromium.org/489103004/diff/120001/chrome/browser/ui/passw... chrome/browser/ui/passwords/password_manager_presenter_unittest.cc:242: EXPECT_TRUE(PasswordManagerPresenter::CheckOriginValidityForAdding( Could you please add: SCOPED_TRACE(kValidOrigins[i].spec()); https://codereview.chromium.org/489103004/diff/120001/chrome/browser/ui/passw... chrome/browser/ui/passwords/password_manager_presenter_unittest.cc:253: EXPECT_FALSE(PasswordManagerPresenter::CheckOriginValidityForAdding( Same here. https://codereview.chromium.org/489103004/diff/120001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/password_manager_handler.cc (right): https://codereview.chromium.org/489103004/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/password_manager_handler.cc:161: if (!ExtractIntegerValue(args, &index) || index < 0) { To reduce total number of lines, could you please also combine this check with the next one?
Thanks, engedy. I've addressed your comments. PTAL. Thanks, Jaekyeom. https://codereview.chromium.org/489103004/diff/120001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/489103004/diff/120001/chrome/app/generated_re... chrome/app/generated_resources.grd:7321: + <message name="IDS_PASSWORDS_PAGE_URL_INSTRUCTION" desc="Placeholder text shown in the URL field of the Add New Entry row"> On 2014/09/15 14:38:58, engedy wrote: > Note that the translators will not receive the resource codenames or any context > other than the "desc" attribute. Therefore it is very important to include all > relevant information there. With this in mind, could you please elaborate more > here and in the other new strings, and make sure to mention that it is for the > password page? > > E.g., here we could write: desc="Placeholder text shown in the URL field when > manually adding new credentials on the passwords page in settings." Done. https://codereview.chromium.org/489103004/diff/120001/chrome/app/generated_re... chrome/app/generated_resources.grd:7322: + Origin URL On 2014/09/15 14:38:58, engedy wrote: > We have discussed this with Vaclav in depth, and after some consideration, we > think this might be too technical and impossible to translate. It would be > better to say "Login page URL". What do you think? I agree with you. Done. https://codereview.chromium.org/489103004/diff/120001/chrome/app/generated_re... chrome/app/generated_resources.grd:7331: + Must be a valid HTTP or HTTPS URL On 2014/09/15 14:38:58, engedy wrote: > nit: Login page URL must be... Done. https://codereview.chromium.org/489103004/diff/120001/chrome/browser/resource... File chrome/browser/resources/options/password_manager_list.js (right): https://codereview.chromium.org/489103004/diff/120001/chrome/browser/resource... chrome/browser/resources/options/password_manager_list.js:85: 'list-inline-button custom-appearance overwrite-button'; On 2014/09/15 14:38:58, engedy wrote: > nit: Do we use the 'overwrite-button' class anywhere? If not, we should remove > it. Done. https://codereview.chromium.org/489103004/diff/120001/chrome/browser/ui/passw... File chrome/browser/ui/passwords/password_manager_presenter.h (right): https://codereview.chromium.org/489103004/diff/120001/chrome/browser/ui/passw... chrome/browser/ui/passwords/password_manager_presenter.h:11: #include "base/basictypes.h" On 2014/09/15 14:38:59, engedy wrote: > nit: basictypes.h is deprecated. Please include <stddef.h> directly. Done. https://codereview.chromium.org/489103004/diff/120001/chrome/browser/ui/passw... File chrome/browser/ui/passwords/password_manager_presenter_unittest.cc (right): https://codereview.chromium.org/489103004/diff/120001/chrome/browser/ui/passw... chrome/browser/ui/passwords/password_manager_presenter_unittest.cc:5: #include "base/macros.h" On 2014/09/15 14:38:59, engedy wrote: > Do we need this? Yes, for arraysize(). https://codereview.chromium.org/489103004/diff/120001/chrome/browser/ui/passw... chrome/browser/ui/passwords/password_manager_presenter_unittest.cc:6: #include "base/memory/ref_counted.h" On 2014/09/15 14:38:59, engedy wrote: > I think we can drop this, as this must already be included from > mock_password_store_service.h, otherwise it would not compile -- and we only > need it for calling this. Done. https://codereview.chromium.org/489103004/diff/120001/chrome/browser/ui/passw... chrome/browser/ui/passwords/password_manager_presenter_unittest.cc:18: using testing::AllOf; On 2014/09/15 14:38:59, engedy wrote: > nit: Conventially, we only introduce a using directive if it can save lines in > total. Done. https://codereview.chromium.org/489103004/diff/120001/chrome/browser/ui/passw... chrome/browser/ui/passwords/password_manager_presenter_unittest.cc:176: GURL complex_origin("https://foo:bar@host.com/path?query=v#ref"); On 2014/09/15 14:38:59, engedy wrote: > Please add a custom port as well (and update the expectations on |signon_realm| > and |origin|). Done. https://codereview.chromium.org/489103004/diff/120001/chrome/browser/ui/passw... chrome/browser/ui/passwords/password_manager_presenter_unittest.cc:193: std::string username1 = "username"; On 2014/09/15 14:38:59, engedy wrote: > nit: Conventionally, we use const char kUsername1[] = "..."; in these cases. Done. https://codereview.chromium.org/489103004/diff/120001/chrome/browser/ui/passw... chrome/browser/ui/passwords/password_manager_presenter_unittest.cc:239: "https://foo:bar@host.com/path?query=v#ref" On 2014/09/15 14:38:59, engedy wrote: > Please also add an URL with a custom a port specified. Done. https://codereview.chromium.org/489103004/diff/120001/chrome/browser/ui/passw... chrome/browser/ui/passwords/password_manager_presenter_unittest.cc:242: EXPECT_TRUE(PasswordManagerPresenter::CheckOriginValidityForAdding( On 2014/09/15 14:38:59, engedy wrote: > Could you please add: > > SCOPED_TRACE(kValidOrigins[i].spec()); Done. https://codereview.chromium.org/489103004/diff/120001/chrome/browser/ui/passw... chrome/browser/ui/passwords/password_manager_presenter_unittest.cc:253: EXPECT_FALSE(PasswordManagerPresenter::CheckOriginValidityForAdding( On 2014/09/15 14:38:59, engedy wrote: > Same here. Done. https://codereview.chromium.org/489103004/diff/120001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/password_manager_handler.cc (right): https://codereview.chromium.org/489103004/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/password_manager_handler.cc:161: if (!ExtractIntegerValue(args, &index) || index < 0) { On 2014/09/15 14:38:59, engedy wrote: > To reduce total number of lines, could you please also combine this check with > the next one? Done.
LGTM, thank you! I have no more concerns. (I will want to play around with details of the UX, but that can wait.)
Further changes to chrome/browser/ui/passwords/password_manager_presenter.* look even better than before, so LGTM, thanks! Vaclav
The CQ bit was checked by btapiz@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/489103004/180001
The CQ bit was unchecked by commit-bot@chromium.org
The CQ bit was unchecked by btapiz@gmail.com
Try jobs failed on following builders: mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/58134) ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) win_chromium_x64_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Thank you, reviewers! I will rebase my branch because of the patch failure.
I've rebased the branch (patch set 11). And I had found a mistake, a type mismatch in password_manager.js; uploaded another one for fixing it (patch set 12). Could you please take a look again? Thanks, Jaekyeom.
On 2014/09/20 02:00:15, jaekyeom wrote: > I've rebased the branch (patch set 11). > And I had found a mistake, a type mismatch in password_manager.js; uploaded > another one for fixing it (patch set 12). > > Could you please take a look again? > > Thanks, > Jaekyeom. (Additional comment, sorry for another mail) The type mismatch meant that the getter length() is PasswordsList's one but |savedPasswordsList_| is only required to be DeletableItemList.
On 2014/09/20 02:13:41, jaekyeom wrote: > On 2014/09/20 02:00:15, jaekyeom wrote: > > I've rebased the branch (patch set 11). > > And I had found a mistake, a type mismatch in password_manager.js; uploaded > > another one for fixing it (patch set 12). > > > > Could you please take a look again? > > > > Thanks, > > Jaekyeom. > > (Additional comment, sorry for another mail) > > The type mismatch meant that the getter length() is PasswordsList's one but > |savedPasswordsList_| is only required to be DeletableItemList. Still LGTM, thanks for spotting that. Given that this is a minor change, feel free to go ahead and push it to the CQ.
The CQ bit was checked by btapiz@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/489103004/220001
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as 7fb909e7e6034de942c4f52e98b5c872fd9a6d4a
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/41c3d488906019eb4cec3b43ad39e67e9ee9b92c Cr-Commit-Position: refs/heads/master@{#295912}
Message was sent while issue was closed.
what do i do if my ex used this to hack into my saved passwords and took
log files from my computer
> #manage-profile-overlay {
> width: 616px;
> }
>
> .profile-icon-grid-item {
> height: 31px;
> margin: 2px 4px;
> padding: 4px;
> width: 38px;
> }
>
> .profile-icon {
> height: 31px;
> width: 38px;
> }
>
> #create-profile-name-input-container,
> #manage-profile-name-input-container {
> margin-top: 5px;
> }
>
> #create-profile-name-input-container {
> margin-bottom: 5px;
> }
>
> #create-profile-name,
> #manage-profile-name {
> margin-left: 10px;
> }
>
> #create-profile-name:invalid,
> #manage-profile-name:invalid {
> background-color: pink;
> }
>
> #disconnect-managed-profile-domain-name {
> font-weight: bold;
> }
>
> #disconnect-managed-profile-domain-information {
> background-color: #f0f0f0;
> padding: 14px 0 14px 17px;
> }
>
> #disconnect-managed-profile-message {
> padding-top: 9px;
> }
>
> #create-profile-error-bubble,
> #manage-profile-error-bubble {
> -webkit-transition: max-height 200ms, padding 200ms;
> background-color: rgb(238, 185, 57);
> border-radius: 4px;
> font-weight: bold;
> margin-left: auto;
> margin-right: auto;
> max-height: 50px;
> overflow: hidden;
> padding: 10px 10px;
> text-align: center;
> width: 80%;
> }
>
> html[dir='ltr'] #create-profile-error-bubble {
> margin-left: 20px;
> width: 90%;
> }
>
> html[dir='rtl'] #create-profile-error-bubble {
> margin-right: 20px;
> width: 90%;
> }
>
> #create-profile-error-bubble[hidden],
> #manage-profile-error-bubble[hidden] {
> display: block !important;
> max-height: 0;
> padding: 0 10px;
> }
>
> #create-profile-icon-grid,
> #manage-profile-icon-grid {
> background-color: rgba(255, 255, 255, 0.75);
> padding: 2px;
> }
>
> :-webkit-any(#create-profile-content, #manage-profile-content) >
> :not(:last-child) {
> margin-bottom: 10px;
> }
>
> :-webkit-any(#create-profile-content, #manage-profile-content) >
> :not(:first-child) {
> margin-top: 10px;
> }
>
> :-webkit-any(#create-profile-content, #manage-profile-content) >
> .name-input-container {
> margin-top: 5px;
> }
>
> :-webkit-any(#create-profile-content, #manage-profile-content) >
> .name-label-container {
> margin-bottom: 5px;
> }
>
> #create-profile-content {
> padding-bottom: 0;
> }
>
> .action-area-shortcut-container {
> -webkit-box-flex: 1;
> }
>
> /* Proper spacing for the buttons. */
> #remove-shortcut-button,
> #add-shortcut-button {
> -webkit-margin-end: 10px;
> }
>
> #delete-supervised-profile-addendum {
> -webkit-padding-start: 48px;
> margin-top: 10px;
> }
>
> html[dir='ltr'] #delete-profile-icon {
> float: left;
> margin-right: 10px;
> }
>
> html[dir='rtl'] #delete-profile-icon {
> float: right;
> margin-left: 10px;
> }
>
> #create-profile-supervised-not-signed-in {
> color: #999;
> }
>
> #create-profile-supervised-not-signed-in-label,
> #create-profile-supervised-account-details-out-of-date-label {
> white-space: pre-wrap;
> word-wrap: break-word;
> }
>
> #create-profile-supervised-content-area {
> padding-top: 5px;
> }
>
> #import-existing-supervised-user-link {
> bottom: 25px;
> left: 17px;
> position: absolute;
> }
>
> #supervised-user-import-existing {
> margin: 0;
> padding: 0;
> }
> </style>
> <style>/* Copyright (c) 2012 The Chromium Authors. All rights reserved.
> * Use of this source code is governed by a BSD-style license that can be
> * found in the LICENSE file.
> */
>
> #password-manager > div.content-area {
> width: 600px;
> }
>
> #password-search-column {
> bottom: 10px;
> position: absolute;
> right: 0;
> }
>
> html[dir=rtl] #password-search-column {
> left: 0;
> right: auto;
> }
>
> #password-list-headers {
> position: relative;
> width: 100%;
> }
>
> #passwords-title {
> display: inline-block;
> }</style>
> <style>/* Copyright (c) 2012 The Chromium Authors. All rights reserved.
> * Use of this source code is governed by a BSD-style license that can be
> * found in the LICENSE file. */
>
> #saved-passwords-list .list-inline-button {
> -webkit-transition: opacity 150ms;
> background: rgb(138, 170, 237);
> font-size: 0.9em;
> height: 18px;
> margin-left: 2px;
> margin-right: 2px;
> padding: 0 2px;
> position: absolute;
> top: 3px;
> }
>
> html[dir='ltr'] #saved-passwords-list .list-inline-button {
> right: 2px;
> }
>
> html[dir='rtl'] #saved-passwords-list .list-inline-button {
> left: 2px;
> }
>
> input.inactive-item {
> background: transparent;
> border: none;
> }
>
> #saved-passwords-list .url {
> box-sizing: border-box;
> width: 40%;
> }
>
> #saved-passwords-list .deletable-item:not(:hover) a:not(:focus) {
> color: black;
> text-decoration: none;
> }
>
> #saved-passwords-list .name {
> -webkit-box-flex: 1;
> width: 30%;
> }
>
> #saved-passwords-list .url,
> #saved-passwords-list .name {
> -webkit-user-select: text;
> }
>
> #saved-passwords-list .password,
> #saved-passwords-list .federation {
> -webkit-box-flex: 1;
> position: relative;
> width: 30%;
> }
>
> #saved-passwords-list .password input[type='password'],
> #saved-passwords-list .password input[type='text'] {
> box-sizing: border-box;
> width: 100%;
> }
>
> #password-exceptions-list .url {
> -webkit-box-flex: 1;
> }
>
> #saved-passwords-list .url,
> #saved-passwords-list .name,
> #saved-passwords-list .federation,
> #password-exceptions-list .url {
> overflow: hidden;
> text-overflow: ellipsis;
> }
--
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.
|
