Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1591053002:
Add a password handler to get the list of passwords in md-settings. (Closed)
|
Created:
4 years, 11 months ago by hcarmona Modified:
4 years, 10 months ago CC:
dpapad, arv+watch_chromium.org, chromium-reviews, dbeam+watch-settings_chromium.org, michaelpg+watch-md-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, stevenjb Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a password handler to get the list of passwords in md-settings.
Also adds the PasswordsPrivateApi so that chrome.send is encapsulated.
Updates passwords section to get correct field (was previously using
|origin| should be using |shownUrl|).
BUG=546836
Committed: https://crrev.com/84175a076ef3a326d3db0e8396c69a851ad16d79
Cr-Commit-Position: refs/heads/master@{#372534}
Patch Set 1 #
Total comments: 10
Patch Set 2 : feedback + closure compile #
Total comments: 27
Patch Set 3 : Feedback #
Total comments: 8
Patch Set 4 : Feedback #
Total comments: 19
Patch Set 5 : chrome.passwordsPrivate #
Total comments: 20
Patch Set 6 : Feedback #
Total comments: 20
Patch Set 7 : Feedback #
Total comments: 2
Patch Set 8 : Nit: inline function #Messages
Total messages: 45 (14 generated)
hcarmona@chromium.org changed reviewers: + dbeam@chromium.org, dschuyler@chromium.org
PTAL and give feedback. Thanks!
dpapad@chromium.org changed reviewers: + dpapad@chromium.org
https://codereview.chromium.org/1591053002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js (right): https://codereview.chromium.org/1591053002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js:55: self.savedPasswords = passwordList; Nit: No need for the "var self = this" trick. You can do the following. settings.PasswordsPrivateApi.updatePasswordLists(function(passwordList) { ... }.bind(this)); https://codereview.chromium.org/1591053002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_private_api.js (right): https://codereview.chromium.org/1591053002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/passwords_and_forms_page/passwords_private_api.js:18: * @param {function(Array<object>)} exceptionListCallback Called with the list Array indicates that null can be passed. Array<Object> indicates that the array can hold nulls. I think the correct annotation here (and elsewhere in this file) is as follows !Array<!Object> https://codereview.chromium.org/1591053002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/passwords_and_forms_page/passwords_private_api.js:21: PasswordsPrivateApi.updatePasswordLists = function(passwordListCallback, Could we follow the same pattern that Tommy followed at https://codereview.chromium.org/1575543003, where there is no need to pass callbacks to the private API wrapper? Instead see change_picture.js#attached() method. https://codereview.chromium.org/1591053002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/passwords_and_forms_page/passwords_private_api.js:30: * @param {int} index @param {number} Compiler only understands "number", there is no distinction between int and float in JS. https://codereview.chromium.org/1591053002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/passwords_and_forms_page/passwords_private_api.js:38: * retrieved by calling the private APIs. Nit: 4 spaces indent
Addressed dpapad's feedback and added compiled_resources.gyp to make sure the js is closure compiled. https://codereview.chromium.org/1591053002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js (right): https://codereview.chromium.org/1591053002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js:55: self.savedPasswords = passwordList; On 2016/01/15 22:02:28, dpapad wrote: > Nit: No need for the "var self = this" trick. You can do the following. > > settings.PasswordsPrivateApi.updatePasswordLists(function(passwordList) { > ... > }.bind(this)); Done. https://codereview.chromium.org/1591053002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_private_api.js (right): https://codereview.chromium.org/1591053002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/passwords_and_forms_page/passwords_private_api.js:18: * @param {function(Array<object>)} exceptionListCallback Called with the list On 2016/01/15 22:02:28, dpapad wrote: > Array indicates that null can be passed. Array<Object> indicates that the array > can hold nulls. > > I think the correct annotation here (and elsewhere in this file) is as follows > !Array<!Object> Done. https://codereview.chromium.org/1591053002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/passwords_and_forms_page/passwords_private_api.js:21: PasswordsPrivateApi.updatePasswordLists = function(passwordListCallback, On 2016/01/15 22:02:28, dpapad wrote: > Could we follow the same pattern that Tommy followed at > https://codereview.chromium.org/1575543003, where there is no need to pass > callbacks to the private API wrapper? Instead see change_picture.js#attached() > method. We had an off-line discussion about this and we decided we want to talk with Tommy to see if we can come up with a good approach that will keep the code small and maybe improve the code's readability by somehow tying the bound function to the chrome.send (at least in the private API). I'll leave this as-is until we have consensus. https://codereview.chromium.org/1591053002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/passwords_and_forms_page/passwords_private_api.js:30: * @param {int} index On 2016/01/15 22:02:28, dpapad wrote: > @param {number} > > Compiler only understands "number", there is no distinction between int and > float in JS. Done. https://codereview.chromium.org/1591053002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/passwords_and_forms_page/passwords_private_api.js:38: * retrieved by calling the private APIs. On 2016/01/15 22:02:28, dpapad wrote: > Nit: 4 spaces indent Done.
michaelpg@chromium.org changed reviewers: + michaelpg@chromium.org
https://codereview.chromium.org/1591053002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js (right): https://codereview.chromium.org/1591053002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js:53: settings.PasswordsPrivateApi.updatePasswordLists(function(passwordList) { nit: newline before "function" to make indentation more readable https://codereview.chromium.org/1591053002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js:54: this.savedPasswords = passwordList; 2-space indent (but see above) https://codereview.chromium.org/1591053002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js:56: function(exceptionList) { these should be 4 space indent because they continue the argument list https://codereview.chromium.org/1591053002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_private_api.js (right): https://codereview.chromium.org/1591053002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_private_api.js:5: cr.exportPath('settings'); exportPath is redundant with cr.define https://codereview.chromium.org/1591053002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_private_api.js:12: function PasswordsPrivateApi() {} why is this a function/constructor? if you just want the namespace: cr.define('settings.PasswordsPrivateApi', function() { function foo() {} function bar() {} return {foo: foo, bar: bar}; }); https://codereview.chromium.org/1591053002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_private_api.js:15: * Encapsulate the calls between JS and C++ to update the list of passwords. use indicative comments for functions (e.g., Encapsulates the calls...) https://codereview.chromium.org/1591053002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_private_api.js:23: PasswordsPrivateApi.setSavedPasswordsList = passwordListCallback; this is really weird. can you save the callback as a local variable or private member, and call it from setSavedPasswordList? same for below https://codereview.chromium.org/1591053002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_private_api.js:33: PasswordsPrivateApi.showPassword = function(index, password) {}; do you need a TODO here? https://codereview.chromium.org/1591053002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.css (right): https://codereview.chromium.org/1591053002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.css:31: --paper-input-container-disabled: { when alphabetizing, --foo should come first; move width to the bottom https://codereview.chromium.org/1591053002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html (right): https://codereview.chromium.org/1591053002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html:25: <paper-input id="password" type="password" disabled="true" just disabled, no need for ="true" https://codereview.chromium.org/1591053002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/passwords_handler.cc (right): https://codereview.chromium.org/1591053002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/passwords_handler.cc:18: #include "chrome/browser/sync/profile_sync_service_factory.h" get rid of the includes you don't use
stevenjb@chromium.org changed reviewers: + stevenjb@chromium.org
https://codereview.chromium.org/1591053002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_private_api.js (right): https://codereview.chromium.org/1591053002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_private_api.js:22: exceptionListCallback) { API methods with multiple callback arguments are unusual. It is unclear what the expected behavior should be if one or the other fails. Can this API be defined to either make this two separate methods, or to provide all of the data in a single callback?
https://codereview.chromium.org/1591053002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js (right): https://codereview.chromium.org/1591053002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js:53: settings.PasswordsPrivateApi.updatePasswordLists(function(passwordList) { On 2016/01/16 07:12:48, michaelpg wrote: > nit: newline before "function" to make indentation more readable Done. https://codereview.chromium.org/1591053002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js:54: this.savedPasswords = passwordList; On 2016/01/16 07:12:48, michaelpg wrote: > 2-space indent (but see above) Done. https://codereview.chromium.org/1591053002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js:56: function(exceptionList) { On 2016/01/16 07:12:48, michaelpg wrote: > these should be 4 space indent because they continue the argument list Done. https://codereview.chromium.org/1591053002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_private_api.js (right): https://codereview.chromium.org/1591053002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_private_api.js:5: cr.exportPath('settings'); On 2016/01/16 07:12:48, michaelpg wrote: > exportPath is redundant with cr.define Done. https://codereview.chromium.org/1591053002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_private_api.js:12: function PasswordsPrivateApi() {} On 2016/01/16 07:12:48, michaelpg wrote: > why is this a function/constructor? > > if you just want the namespace: > > cr.define('settings.PasswordsPrivateApi', function() { > function foo() {} > function bar() {} > return {foo: foo, bar: bar}; > }); Good to know. Updated. https://codereview.chromium.org/1591053002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_private_api.js:15: * Encapsulate the calls between JS and C++ to update the list of passwords. On 2016/01/16 07:12:48, michaelpg wrote: > use indicative comments for functions (e.g., Encapsulates the calls...) Done. https://codereview.chromium.org/1591053002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_private_api.js:22: exceptionListCallback) { On 2016/01/19 21:34:28, stevenjb wrote: > API methods with multiple callback arguments are unusual. It is unclear what the > expected behavior should be if one or the other fails. > > Can this API be defined to either make this two separate methods, or to provide > all of the data in a single callback? Yes, we can do something else with this API. Not 100% sure what the right approach is. I like the idea of decoupling the update of these 2 lists and having 2 functions one to update each list. That change doesn't look too complex, but would touch shared code between the new and old settings. I can make a separate CL for that. Should that be a prerequisite for this CL? https://codereview.chromium.org/1591053002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_private_api.js:23: PasswordsPrivateApi.setSavedPasswordsList = passwordListCallback; On 2016/01/16 07:12:48, michaelpg wrote: > this is really weird. can you save the callback as a local variable or private > member, and call it from setSavedPasswordList? same for below I'm not sure what you mean about this being "weird". I'm trying to see how adding a local or private variable will improve this. This just assigns the callback to the passed in functions, and adding another variable to hold the callback seems odd to me because we can just call the callback directly. https://codereview.chromium.org/1591053002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_private_api.js:33: PasswordsPrivateApi.showPassword = function(index, password) {}; On 2016/01/16 07:12:48, michaelpg wrote: > do you need a TODO here? Done. https://codereview.chromium.org/1591053002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.css (right): https://codereview.chromium.org/1591053002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.css:31: --paper-input-container-disabled: { On 2016/01/16 07:12:48, michaelpg wrote: > when alphabetizing, --foo should come first; move width to the bottom Done. https://codereview.chromium.org/1591053002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html (right): https://codereview.chromium.org/1591053002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html:25: <paper-input id="password" type="password" disabled="true" On 2016/01/16 07:12:48, michaelpg wrote: > just disabled, no need for ="true" Done. https://codereview.chromium.org/1591053002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/passwords_handler.cc (right): https://codereview.chromium.org/1591053002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/passwords_handler.cc:18: #include "chrome/browser/sync/profile_sync_service_factory.h" On 2016/01/16 07:12:48, michaelpg wrote: > get rid of the includes you don't use I've removed a bunch. Compiles on my machine, trybots will tell me if I removed too much for other platforms and I'll update if necessary.
https://codereview.chromium.org/1591053002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_private_api.js (right): https://codereview.chromium.org/1591053002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_private_api.js:23: PasswordsPrivateApi.setSavedPasswordsList = passwordListCallback; On 2016/01/20 at 02:32:50, Hector Carmona wrote: > On 2016/01/16 07:12:48, michaelpg wrote: > > this is really weird. can you save the callback as a local variable or private > > member, and call it from setSavedPasswordList? same for below > > I'm not sure what you mean about this being "weird". I'm trying to see > how adding a local or private variable will improve this. This just > assigns the callback to the passed in functions, and adding another > variable to hold the callback seems odd to me because we can just call > the callback directly. Agree with Hector. What is the benefit of adding this extra indirection level here? https://codereview.chromium.org/1591053002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.css (right): https://codereview.chromium.org/1591053002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.css:30: --paper-input-container-disabled: { I have never seen this pattern before (nested CSS rules). Is this documented somewhere I can take a look? https://codereview.chromium.org/1591053002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_passwords_section_browsertest.js (right): https://codereview.chromium.org/1591053002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_passwords_section_browsertest.js:69: var validate = function(nodes, passwordList, index) { Nit: Can you add some type annotation to this function? https://codereview.chromium.org/1591053002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_passwords_section_browsertest.js:70: assertTrue(!!nodes[index], 'Failed to get nodes[' + index + ']'); Nit (optional): How about var node = nodes[index]; var passwordData = passwordList[index]; ... // avoids repeating nodes[index] and passwordList[index]
https://codereview.chromium.org/1591053002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_private_api.js (right): https://codereview.chromium.org/1591053002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_private_api.js:23: PasswordsPrivateApi.setSavedPasswordsList = passwordListCallback; On 2016/01/20 17:56:07, dpapad wrote: > On 2016/01/20 at 02:32:50, Hector Carmona wrote: > > On 2016/01/16 07:12:48, michaelpg wrote: > > > this is really weird. can you save the callback as a local variable or > private > > > member, and call it from setSavedPasswordList? same for below > > > > I'm not sure what you mean about this being "weird". I'm trying to see > > how adding a local or private variable will improve this. This just > > assigns the callback to the passed in functions, and adding another > > variable to hold the callback seems odd to me because we can just call > > the callback directly. > > Agree with Hector. What is the benefit of adding this extra indirection level > here? You're overriding a method defined at line 40. So calling the function (eg from C++) is a no-op until updatePasswordLists has been called by the javascript. At least document that on that line. Furthermore the C++ needs to know not to call the method multiple times in case the callback registered by updatePasswordLists isn't intended to be used more than once. What if updatePasswordLists is called twice, from two different places, before the C++ round-trip completes? Only the 2nd callback will be called, and it will be called twice? https://codereview.chromium.org/1591053002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_private_api.js (right): https://codereview.chromium.org/1591053002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_private_api.js:13: updatePasswordLists = function(passwordListCallback, missing "var" here and elsewhere or, just "function updatePasswordLists(..." https://codereview.chromium.org/1591053002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.css (right): https://codereview.chromium.org/1591053002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.css:30: --paper-input-container-disabled: { On 2016/01/20 17:56:07, dpapad wrote: > I have never seen this pattern before (nested CSS rules). Is this documented > somewhere I can take a look? https://www.polymer-project.org/1.0/docs/devguide/styling.html#custom-css-mixins
why does this CL have 5 reviewers?
Description was changed from ========== Add a password handler to get the list of passwords in md-settings. Also adds the PasswordsPrivateApi so that chrome.send is encapsulated. Updates passwords section to get correct field (was previously using |origin| should be using |shownUrl|). BUG=546836 ========== to ========== Add a password handler to get the list of passwords in md-settings. Also adds the PasswordsPrivateApi so that chrome.send is encapsulated. Updates passwords section to get correct field (was previously using |origin| should be using |shownUrl|). BUG=546836 ==========
dschuyler@chromium.org changed reviewers: - dschuyler@chromium.org
hcarmona@chromium.org changed reviewers: - dpapad@chromium.org, stevenjb@chromium.org
hcarmona@chromium.org changed required reviewers: + dbeam@chromium.org, michaelpg@chromium.org
On 2016/01/20 22:41:19, Dan Beam wrote: > why does this CL have 5 reviewers? Probably b/c this was mentioned in the email thread about C++/JS interaction. I've trimmed my reviewers to dbeam and michaelpg. https://codereview.chromium.org/1591053002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_private_api.js (right): https://codereview.chromium.org/1591053002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_private_api.js:23: PasswordsPrivateApi.setSavedPasswordsList = passwordListCallback; On 2016/01/20 21:20:56, michaelpg wrote: > On 2016/01/20 17:56:07, dpapad wrote: > > On 2016/01/20 at 02:32:50, Hector Carmona wrote: > > > On 2016/01/16 07:12:48, michaelpg wrote: > > > > this is really weird. can you save the callback as a local variable or > > private > > > > member, and call it from setSavedPasswordList? same for below > > > > > > I'm not sure what you mean about this being "weird". I'm trying to see > > > how adding a local or private variable will improve this. This just > > > assigns the callback to the passed in functions, and adding another > > > variable to hold the callback seems odd to me because we can just call > > > the callback directly. > > > > Agree with Hector. What is the benefit of adding this extra indirection level > > here? > > You're overriding a method defined at line 40. So calling the function (eg from > C++) is a no-op until updatePasswordLists has been called by the javascript. At > least document that on that line. Furthermore the C++ needs to know not to call > the method multiple times in case the callback registered by updatePasswordLists > isn't intended to be used more than once. > > What if updatePasswordLists is called twice, from two different places, before > the C++ round-trip completes? Only the 2nd callback will be called, and it will > be called twice? I've added a comment that the callbacks will be no-op until they've been set and that they may be called multiple times. Right now, this code makes the assumptions that you mentioned. Would the approach at http://crrev.com/1609923003 be better for this? I had that as a POC for the discussion on C++/JS, but I think it makes sense to use it here instead of the current approach. https://codereview.chromium.org/1591053002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_private_api.js (right): https://codereview.chromium.org/1591053002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_private_api.js:13: updatePasswordLists = function(passwordListCallback, On 2016/01/20 21:20:56, michaelpg wrote: > missing "var" here and elsewhere > or, just "function updatePasswordLists(..." Done. https://codereview.chromium.org/1591053002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_passwords_section_browsertest.js (right): https://codereview.chromium.org/1591053002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_passwords_section_browsertest.js:69: var validate = function(nodes, passwordList, index) { On 2016/01/20 17:56:07, dpapad wrote: > Nit: Can you add some type annotation to this function? Done. https://codereview.chromium.org/1591053002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_passwords_section_browsertest.js:70: assertTrue(!!nodes[index], 'Failed to get nodes[' + index + ']'); On 2016/01/20 17:56:07, dpapad wrote: > Nit (optional): How about > > var node = nodes[index]; > var passwordData = passwordList[index]; > ... > // avoids repeating nodes[index] and passwordList[index] Done.
Friendly ping on this CL.
On 2016/01/22 19:09:47, Hector Carmona wrote: > Friendly ping on this CL. If this is the example CL for the chrome.send discussion, let's resolve that first.
TL;DR: Why not use chrome.passwordsPrivate? Seems to have most of the functionality you need, and you could add the rest instead of creating a new handler. https://codereview.chromium.org/1591053002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js (right): https://codereview.chromium.org/1591053002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js:29: * Lazy loaded when the password section is expanded. after making the typedef: @type {!Array<!settings.PaswordsPrivateApi.Password>} https://codereview.chromium.org/1591053002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_private_api.js (right): https://codereview.chromium.org/1591053002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_private_api.js:8: * @param {function(!Array<!Object>)} passwordListCallback Called with the can you add a typedef, e.g. "settings.PasswordsPrivateApi.Password", defining what the objects look like, and reference that here instead of Object? optionally, you can then typedef "Array<!settings.PPA.Password>" as PasswordList since it's used a lot https://codereview.chromium.org/1591053002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_private_api.js:16: exceptionListCallback) { fix spacing https://codereview.chromium.org/1591053002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_private_api.js:51: return { updatePasswordLists: updatePasswordLists, nit: no spacing inside object literal {} https://codereview.chromium.org/1591053002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/passwords_handler.cc (right): https://codereview.chromium.org/1591053002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/passwords_handler.cc:64: #if !defined(OS_ANDROID) GYP includes this in "chrome_browser_ui_non_mobile_sources", so OS_ANDROID should never be defined. https://codereview.chromium.org/1591053002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/passwords_handler.cc:77: "removeSavedPassword", why reimplement these instead of using chrome.passwordsPrivate? https://codereview.chromium.org/1591053002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/passwords_handler.cc:90: void PasswordsHandler::HandleRemoveSavedPassword(const base::ListValue* args) { nit: order according to header file https://codereview.chromium.org/1591053002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/passwords_handler.cc:91: std::string string_value = base::UTF16ToUTF8(ExtractStringValue(args)); here and below: why not pass as an integer? https://codereview.chromium.org/1591053002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/passwords_handler.cc:93: if (base::StringToInt(string_value, &index) && index >= 0) { here and below: StringToUint? DCHECK if false? https://codereview.chromium.org/1591053002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/passwords_handler.h (right): https://codereview.chromium.org/1591053002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/passwords_handler.h:40: #if !defined(OS_ANDROID) ditto
tommycli@chromium.org changed reviewers: + tommycli@chromium.org
michealpg@: I think he's not using chrome.passwordsPrivate, because he wants to preserve the existing C++ Handler code. hcarmona@: Maybe git cl upload --similarity 30 or 40 or something so the passwords_handler.cc file looks like a copy instead of a whole new file. It appears that the callback registration within private_api.js is consistent with the discussion we just had. It might be good to deregister the callbacks on detach, but overall this looks like it would work to me.
So, we decided that it was not important to create a new extension API if one doe snot exist, but we should still *prefer* to use extension APIs if they already exist and only require relatively minor changes, since an extension API gives us type checking, callbacks, and a clearly defined interface between JS and C++. The key here is the definition of "minor changes"; I haven't looked at the CL closely enough to suggest whether or not passwordsPrivate would be a good fit here, but I would definitely suggest considering it, and would be happy to help out with any changes that would be needed. On Fri, Jan 22, 2016 at 4:28 PM, <tommycli@chromium.org> wrote: > michealpg@: I think he's not using chrome.passwordsPrivate, because he > wants to > preserve the existing C++ Handler code. > > hcarmona@: Maybe git cl upload --similarity 30 or 40 or something so the > passwords_handler.cc file looks like a copy instead of a whole new file. > > It appears that the callback registration within private_api.js is > consistent > with the discussion we just had. It might be good to deregister the > callbacks on > detach, but overall this looks like it would work to me. > > https://codereview.chromium.org/1591053002/ > -- 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.
It looks like passwordsPrivate <https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...> was actually written for the Settings UI but never wired up, so we should either use it or remove it. The pattern is a little unusual (to me anyway) in that it requires adding an event listener to receive the passwords and exceptions lists, but since the UI is going to want to receive updates anyway, this seems fine to me. On Fri, Jan 22, 2016 at 4:33 PM, Steven Bennetts <stevenjb@chromium.org> wrote: > So, we decided that it was not important to create a new extension API if > one doe snot exist, but we should still *prefer* to use extension APIs if > they already exist and only require relatively minor changes, since an > extension API gives us type checking, callbacks, and a clearly defined > interface between JS and C++. The key here is the definition of "minor > changes"; I haven't looked at the CL closely enough to suggest whether or > not passwordsPrivate would be a good fit here, but I would definitely > suggest considering it, and would be happy to help out with any changes > that would be needed. > > > On Fri, Jan 22, 2016 at 4:28 PM, <tommycli@chromium.org> wrote: > >> michealpg@: I think he's not using chrome.passwordsPrivate, because he >> wants to >> preserve the existing C++ Handler code. >> >> hcarmona@: Maybe git cl upload --similarity 30 or 40 or something so the >> passwords_handler.cc file looks like a copy instead of a whole new file. >> >> It appears that the callback registration within private_api.js is >> consistent >> with the discussion we just had. It might be good to deregister the >> callbacks on >> detach, but overall this looks like it would work to me. >> >> https://codereview.chromium.org/1591053002/ >> > > -- 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.
On 2016/01/23 00:45:01, stevenjb wrote: > It looks like passwordsPrivate > <https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...> > was actually written for the Settings UI but never wired up, so we should > either use it or remove it. > > The pattern is a little unusual (to me anyway) in that it requires adding > an event listener to receive the passwords and exceptions lists, but since > the UI is going to want to receive updates anyway, this seems fine to me. yes, I think that is precisely the purpose
Changed to use chrome.passwordsPrivate https://codereview.chromium.org/1591053002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js (right): https://codereview.chromium.org/1591053002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js:29: * Lazy loaded when the password section is expanded. On 2016/01/22 20:54:17, michaelpg wrote: > after making the typedef: > @type {!Array<!settings.PaswordsPrivateApi.Password>} Objects were already typedef'd. Using them and updated code. https://codereview.chromium.org/1591053002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_private_api.js (right): https://codereview.chromium.org/1591053002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_private_api.js:8: * @param {function(!Array<!Object>)} passwordListCallback Called with the On 2016/01/22 20:54:17, michaelpg wrote: > can you add a typedef, e.g. "settings.PasswordsPrivateApi.Password", defining > what the objects look like, and reference that here instead of Object? > > optionally, you can then typedef "Array<!settings.PPA.Password>" as PasswordList > since it's used a lot Acknowledged. https://codereview.chromium.org/1591053002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_private_api.js:16: exceptionListCallback) { On 2016/01/22 20:54:17, michaelpg wrote: > fix spacing Acknowledged. https://codereview.chromium.org/1591053002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_private_api.js:51: return { updatePasswordLists: updatePasswordLists, On 2016/01/22 20:54:17, michaelpg wrote: > nit: no spacing inside object literal {} Acknowledged. https://codereview.chromium.org/1591053002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/passwords_handler.cc (right): https://codereview.chromium.org/1591053002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/passwords_handler.cc:64: #if !defined(OS_ANDROID) On 2016/01/22 20:54:17, michaelpg wrote: > GYP includes this in "chrome_browser_ui_non_mobile_sources", so OS_ANDROID > should never be defined. Good to know, but this file is gone. https://codereview.chromium.org/1591053002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/passwords_handler.cc:77: "removeSavedPassword", On 2016/01/22 20:54:17, michaelpg wrote: > why reimplement these instead of using chrome.passwordsPrivate? Done. Implemented using chrome.passwordsPrivate. https://codereview.chromium.org/1591053002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/passwords_handler.cc:90: void PasswordsHandler::HandleRemoveSavedPassword(const base::ListValue* args) { On 2016/01/22 20:54:17, michaelpg wrote: > nit: order according to header file Acknowledged. https://codereview.chromium.org/1591053002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/passwords_handler.cc:91: std::string string_value = base::UTF16ToUTF8(ExtractStringValue(args)); On 2016/01/22 20:54:17, michaelpg wrote: > here and below: why not pass as an integer? Acknowledged. https://codereview.chromium.org/1591053002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/passwords_handler.cc:93: if (base::StringToInt(string_value, &index) && index >= 0) { On 2016/01/22 20:54:17, michaelpg wrote: > here and below: StringToUint? > DCHECK if false? Acknowledged.
lgtm https://codereview.chromium.org/1591053002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html (right): https://codereview.chromium.org/1591053002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html:18: <div id="shownUrl">[[item.loginPair.originUrl]]</div> why is "shownUrl" better? https://codereview.chromium.org/1591053002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html:24: --> <!-- TODO(hcarmona): ... --> IMO https://codereview.chromium.org/1591053002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html:27: value="[[getEmptyPassword(item.numCharactersInPassword)]]"> nit: getEmptyPassword_ (private if possible) https://codereview.chromium.org/1591053002/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_passwords_section_browsertest.js (right): https://codereview.chromium.org/1591053002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_passwords_section_browsertest.js:42: * @param {number} passwordLength needs a @return, might also want to @typedef this https://codereview.chromium.org/1591053002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_passwords_section_browsertest.js:44: createPasswordItem(url, username, passwordLength) { createPasswordItem: function(url, username, passwordLength) { https://codereview.chromium.org/1591053002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_passwords_section_browsertest.js:44: createPasswordItem(url, username, passwordLength) { can this be private? https://codereview.chromium.org/1591053002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_passwords_section_browsertest.js:53: * @param {Array<Element>} nodes The nodes that will be checked. nit: this should probably be !Array<!Element> https://codereview.chromium.org/1591053002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_passwords_section_browsertest.js:54: * @param {Array<Object>} passwordList The expected data. and !Array<!Object>
https://codereview.chromium.org/1591053002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js (right): https://codereview.chromium.org/1591053002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js:30: * @type {!Array<!PasswordUiEntry>} Please re-generate the passwords_private.js externs to use the new chrome.passwordsPrivate.PasswordUiEntry style syntax, a la https://code.google.com/p/chromium/codesearch#chromium/src/third_party/closur...
stevenjb@chromium.org changed reviewers: + stevenjb@chromium.org
Could you do the following as part of this CL: 1. From chrome/src, run: python ./tools/json_schema_compiler/compiler.py -g externs chrome/common/extensions/api/passwords_private.idl > third_party/closure_compiler/externs/passwords_private.js 2. Replace PasswordUiEntry with chrome.passwordsPrivate.PasswordUiEntry 3. Make sure closure compiles (A few months back we decided to namespace types associated with extension. We updated the tool, but didn't every generated extern file, instead we've been updating them as we go.) Thanks! https://codereview.chromium.org/1591053002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.html (right): https://codereview.chromium.org/1591053002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.html:7: <link rel="import" href="chrome://md-settings/passwords_and_forms_page/passwords_private_api.html"> Left over?
On 2016/01/27 00:56:56, stevenjb wrote: > Could you do the following as part of this CL: > > 1. From chrome/src, run: > python ./tools/json_schema_compiler/compiler.py -g externs > chrome/common/extensions/api/passwords_private.idl > > third_party/closure_compiler/externs/passwords_private.js > > 2. Replace PasswordUiEntry with chrome.passwordsPrivate.PasswordUiEntry > > 3. Make sure closure compiles > > (A few months back we decided to namespace types associated with extension. We > updated the tool, but didn't every generated extern file, instead we've been > updating them as we go.) > > Thanks! Thanks for the quick tutorial on how to auto-gen the file! It's now up-to-date :-) https://codereview.chromium.org/1591053002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.html (right): https://codereview.chromium.org/1591053002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.html:7: <link rel="import" href="chrome://md-settings/passwords_and_forms_page/passwords_private_api.html"> On 2016/01/27 00:56:56, stevenjb wrote: > Left over? Yes, removed. https://codereview.chromium.org/1591053002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js (right): https://codereview.chromium.org/1591053002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js:30: * @type {!Array<!PasswordUiEntry>} On 2016/01/27 00:49:11, michaelpg wrote: > Please re-generate the passwords_private.js externs to use > the new chrome.passwordsPrivate.PasswordUiEntry style syntax, a la > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/closur... Done. https://codereview.chromium.org/1591053002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html (right): https://codereview.chromium.org/1591053002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html:18: <div id="shownUrl">[[item.loginPair.originUrl]]</div> On 2016/01/27 00:17:45, Dan Beam wrote: > why is "shownUrl" better? It's arbitrary, I've changed it to "originUrl" to match the property. https://codereview.chromium.org/1591053002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html:24: --> On 2016/01/27 00:17:45, Dan Beam wrote: > <!-- TODO(hcarmona): > ... --> > > IMO Done. https://codereview.chromium.org/1591053002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html:27: value="[[getEmptyPassword(item.numCharactersInPassword)]]"> On 2016/01/27 00:17:45, Dan Beam wrote: > nit: getEmptyPassword_ (private if possible) Done. https://codereview.chromium.org/1591053002/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_passwords_section_browsertest.js (right): https://codereview.chromium.org/1591053002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_passwords_section_browsertest.js:42: * @param {number} passwordLength On 2016/01/27 00:17:45, Dan Beam wrote: > needs a @return, might also want to @typedef this Done. https://codereview.chromium.org/1591053002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_passwords_section_browsertest.js:44: createPasswordItem(url, username, passwordLength) { On 2016/01/27 00:17:45, Dan Beam wrote: > createPasswordItem: function(url, username, passwordLength) { Done. https://codereview.chromium.org/1591053002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_passwords_section_browsertest.js:44: createPasswordItem(url, username, passwordLength) { On 2016/01/27 00:17:45, Dan Beam wrote: > can this be private? Done. https://codereview.chromium.org/1591053002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_passwords_section_browsertest.js:53: * @param {Array<Element>} nodes The nodes that will be checked. On 2016/01/27 00:17:45, Dan Beam wrote: > nit: this should probably be !Array<!Element> Done. https://codereview.chromium.org/1591053002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_passwords_section_browsertest.js:54: * @param {Array<Object>} passwordList The expected data. On 2016/01/27 00:17:45, Dan Beam wrote: > and !Array<!Object> Done. https://codereview.chromium.org/1591053002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js (right): https://codereview.chromium.org/1591053002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js:51: loadPasswords_: function(passwordSectionOpened) { Had to change this b/c I realized that I was adding a listener every time the passwords section was expanded/collapsed.
https://codereview.chromium.org/1591053002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js (right): https://codereview.chromium.org/1591053002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js:51: loadPasswords_: function(passwordSectionOpened) { On 2016/01/27 02:46:18, Hector Carmona wrote: > Had to change this b/c I realized that I was adding a listener every > time the passwords section was expanded/collapsed. ok, but why is passwordSectionOpened necessary? https://codereview.chromium.org/1591053002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js:57: this.onSavedPasswordsListChanged_ = function(passwordList) { Why create this dynamically rather than have this on the prototype? https://codereview.chromium.org/1591053002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.css (right): https://codereview.chromium.org/1591053002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.css:29: paper-input.readonly-password { or just paper-input, or paper-input[disabled] if the vars should only be defined when it's disabled https://codereview.chromium.org/1591053002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.css:36: width: 150px; how is the width related to it being readonly https://codereview.chromium.org/1591053002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html (right): https://codereview.chromium.org/1591053002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html:19: <div id="username"secondary>[[item.loginPair.username]]</div> fix id="username"secondary https://codereview.chromium.org/1591053002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js (left): https://codereview.chromium.org/1591053002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js:24: type: Array, still a required parameter for polymer (run-time) regardless of closure (static analysis) https://codereview.chromium.org/1591053002/diff/100001/chrome/test/data/webui... File chrome/test/data/webui/settings/settings_passwords_section_browsertest.js (right): https://codereview.chromium.org/1591053002/diff/100001/chrome/test/data/webui... chrome/test/data/webui/settings/settings_passwords_section_browsertest.js:81: * This test will validate_ that the section is loaded with data. validate https://codereview.chromium.org/1591053002/diff/100001/chrome/test/data/webui... chrome/test/data/webui/settings/settings_passwords_section_browsertest.js:94: self.createPasswordItem_('website.com', 'mario', 7) nit: vary passwordLength to catch moar bugs
Thanks for updating the externs! https://codereview.chromium.org/1591053002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js (right): https://codereview.chromium.org/1591053002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js:63: this.onSavedPasswordsListChanged_.bind(this)); Looking at this again, I think it would be more clear to do this in ready() and just inline it: ready() { chrome.settingsPrivate.onPrefsChanged.addListener(function(passwordList) { this.savedPasswords = passwordList; }.bind(this); } Existing example: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...
I've tracked down the issue I mentioned during our standup about the iron-list not expanding. It was fixed in polymer here: https://github.com/PolymerElements/iron-collapse/commit/be51f9ce885680d064bc4... Can this CL land w/o that fix? I've applied those changes to the iron-collapse locally and my code works as expected (the list expands properly). So, no changes required to make it work. But this won't work properly w/o updating polymer. https://codereview.chromium.org/1591053002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js (right): https://codereview.chromium.org/1591053002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js:51: loadPasswords_: function(passwordSectionOpened) { On 2016/01/27 04:23:43, michaelpg wrote: > On 2016/01/27 02:46:18, Hector Carmona wrote: > > Had to change this b/c I realized that I was adding a listener every > > time the passwords section was expanded/collapsed. > > ok, but why is passwordSectionOpened necessary? Not used any more :-) https://codereview.chromium.org/1591053002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js:57: this.onSavedPasswordsListChanged_ = function(passwordList) { On 2016/01/27 04:23:43, michaelpg wrote: > Why create this dynamically rather than have this on the prototype? Updated: now part of the prototype and bound on |ready|. https://codereview.chromium.org/1591053002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js:63: this.onSavedPasswordsListChanged_.bind(this)); On 2016/01/27 19:28:45, stevenjb wrote: > Looking at this again, I think it would be more clear to do this in ready() and > just inline it: > > ready() { > chrome.settingsPrivate.onPrefsChanged.addListener(function(passwordList) { > this.savedPasswords = passwordList; > }.bind(this); > } > > Existing example: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... Awesome! Done. My only concern is that this API triggers a callback when the listener is added. This means we'll retrieve the data whether we use it or not. Maybe not a big deal? https://codereview.chromium.org/1591053002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.css (right): https://codereview.chromium.org/1591053002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.css:29: paper-input.readonly-password { On 2016/01/27 04:23:43, michaelpg wrote: > or just paper-input, or paper-input[disabled] if the vars should only be defined > when it's disabled Done. https://codereview.chromium.org/1591053002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.css:36: width: 150px; On 2016/01/27 04:23:43, michaelpg wrote: > how is the width related to it being readonly Not related to being read-only. I've removed the class, is there something I should change here? https://codereview.chromium.org/1591053002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html (right): https://codereview.chromium.org/1591053002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html:19: <div id="username"secondary>[[item.loginPair.username]]</div> On 2016/01/27 04:23:43, michaelpg wrote: > fix id="username"secondary Done. https://codereview.chromium.org/1591053002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js (left): https://codereview.chromium.org/1591053002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js:24: type: Array, On 2016/01/27 04:23:43, michaelpg wrote: > still a required parameter for polymer (run-time) regardless of closure (static > analysis) Done, here and elsewhere. https://codereview.chromium.org/1591053002/diff/100001/chrome/test/data/webui... File chrome/test/data/webui/settings/settings_passwords_section_browsertest.js (right): https://codereview.chromium.org/1591053002/diff/100001/chrome/test/data/webui... chrome/test/data/webui/settings/settings_passwords_section_browsertest.js:81: * This test will validate_ that the section is loaded with data. On 2016/01/27 04:23:43, michaelpg wrote: > validate Good catch. Fixed. https://codereview.chromium.org/1591053002/diff/100001/chrome/test/data/webui... chrome/test/data/webui/settings/settings_passwords_section_browsertest.js:94: self.createPasswordItem_('website.com', 'mario', 7) On 2016/01/27 04:23:43, michaelpg wrote: > nit: vary passwordLength to catch moar bugs Done.
On 2016/01/29 23:52:12, Hector Carmona wrote: > I've tracked down the issue I mentioned during our standup about the > iron-list not expanding. It was fixed in polymer here: > https://github.com/PolymerElements/iron-collapse/commit/be51f9ce885680d064bc4... > > Can this CL land w/o that fix? I've applied those changes to the > iron-collapse locally and my code works as expected (the list expands > properly). So, no changes required to make it work. But this won't > work properly w/o updating polymer. I think it's fine. iron-list has been in various stages of broken throughout this project. And it's not like we're near a branch point, let alone a "done" product. > > https://codereview.chromium.org/1591053002/diff/100001/chrome/browser/resourc... > File > chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js > (right): > > https://codereview.chromium.org/1591053002/diff/100001/chrome/browser/resourc... > chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js:51: > loadPasswords_: function(passwordSectionOpened) { > On 2016/01/27 04:23:43, michaelpg wrote: > > On 2016/01/27 02:46:18, Hector Carmona wrote: > > > Had to change this b/c I realized that I was adding a listener every > > > time the passwords section was expanded/collapsed. > > > > ok, but why is passwordSectionOpened necessary? > > Not used any more :-) > > https://codereview.chromium.org/1591053002/diff/100001/chrome/browser/resourc... > chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js:57: > this.onSavedPasswordsListChanged_ = function(passwordList) { > On 2016/01/27 04:23:43, michaelpg wrote: > > Why create this dynamically rather than have this on the prototype? > > Updated: now part of the prototype and bound on |ready|. > > https://codereview.chromium.org/1591053002/diff/100001/chrome/browser/resourc... > chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js:63: > this.onSavedPasswordsListChanged_.bind(this)); > On 2016/01/27 19:28:45, stevenjb wrote: > > Looking at this again, I think it would be more clear to do this in ready() > and > > just inline it: > > > > ready() { > > chrome.settingsPrivate.onPrefsChanged.addListener(function(passwordList) { > > this.savedPasswords = passwordList; > > }.bind(this); > > } > > > > Existing example: > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... > > Awesome! Done. > > My only concern is that this API triggers a callback when the listener > is added. This means we'll retrieve the data whether we use it or not. > Maybe not a big deal? > > https://codereview.chromium.org/1591053002/diff/100001/chrome/browser/resourc... > File > chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.css > (right): > > https://codereview.chromium.org/1591053002/diff/100001/chrome/browser/resourc... > chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.css:29: > paper-input.readonly-password { > On 2016/01/27 04:23:43, michaelpg wrote: > > or just paper-input, or paper-input[disabled] if the vars should only be > defined > > when it's disabled > > Done. > > https://codereview.chromium.org/1591053002/diff/100001/chrome/browser/resourc... > chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.css:36: > width: 150px; > On 2016/01/27 04:23:43, michaelpg wrote: > > how is the width related to it being readonly > > Not related to being read-only. I've removed the class, is there > something I should change here? > > https://codereview.chromium.org/1591053002/diff/100001/chrome/browser/resourc... > File > chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html > (right): > > https://codereview.chromium.org/1591053002/diff/100001/chrome/browser/resourc... > chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html:19: > <div id="username"secondary>[[item.loginPair.username]]</div> > On 2016/01/27 04:23:43, michaelpg wrote: > > fix id="username"secondary > > Done. > > https://codereview.chromium.org/1591053002/diff/100001/chrome/browser/resourc... > File > chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js > (left): > > https://codereview.chromium.org/1591053002/diff/100001/chrome/browser/resourc... > chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js:24: > type: Array, > On 2016/01/27 04:23:43, michaelpg wrote: > > still a required parameter for polymer (run-time) regardless of closure > (static > > analysis) > > Done, here and elsewhere. > > https://codereview.chromium.org/1591053002/diff/100001/chrome/test/data/webui... > File chrome/test/data/webui/settings/settings_passwords_section_browsertest.js > (right): > > https://codereview.chromium.org/1591053002/diff/100001/chrome/test/data/webui... > chrome/test/data/webui/settings/settings_passwords_section_browsertest.js:81: * > This test will validate_ that the section is loaded with data. > On 2016/01/27 04:23:43, michaelpg wrote: > > validate > > Good catch. Fixed. > > https://codereview.chromium.org/1591053002/diff/100001/chrome/test/data/webui... > chrome/test/data/webui/settings/settings_passwords_section_browsertest.js:94: > self.createPasswordItem_('website.com', 'mario', 7) > On 2016/01/27 04:23:43, michaelpg wrote: > > nit: vary passwordLength to catch moar bugs > > Done.
lgtm https://codereview.chromium.org/1591053002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.css (right): https://codereview.chromium.org/1591053002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.css:36: width: 150px; On 2016/01/29 23:52:11, Hector Carmona wrote: > On 2016/01/27 04:23:43, michaelpg wrote: > > how is the width related to it being readonly > > Not related to being read-only. I've removed the class, is there > something I should change here? Nope, the latest change makes the intent easier to understand, thx. https://codereview.chromium.org/1591053002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js (right): https://codereview.chromium.org/1591053002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js:50: this.onSavedPasswordsListChanged_.bind(this)); Like Steven said, I would just inline the function itself here, but it's a matter of style.
Committing this. https://codereview.chromium.org/1591053002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js (right): https://codereview.chromium.org/1591053002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js:50: this.onSavedPasswordsListChanged_.bind(this)); On 2016/01/30 01:30:15, michaelpg wrote: > Like Steven said, I would just inline the function itself here, but it's a > matter of style. Inlined, I missed that part in Steven's comment.
The CQ bit was checked by hcarmona@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org, michaelpg@chromium.org Link to the patchset: https://codereview.chromium.org/1591053002/#ps140001 (title: "Nit: inline function")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1591053002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1591053002/140001
Message was sent while issue was closed.
Description was changed from ========== Add a password handler to get the list of passwords in md-settings. Also adds the PasswordsPrivateApi so that chrome.send is encapsulated. Updates passwords section to get correct field (was previously using |origin| should be using |shownUrl|). BUG=546836 ========== to ========== Add a password handler to get the list of passwords in md-settings. Also adds the PasswordsPrivateApi so that chrome.send is encapsulated. Updates passwords section to get correct field (was previously using |origin| should be using |shownUrl|). BUG=546836 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Add a password handler to get the list of passwords in md-settings. Also adds the PasswordsPrivateApi so that chrome.send is encapsulated. Updates passwords section to get correct field (was previously using |origin| should be using |shownUrl|). BUG=546836 ========== to ========== Add a password handler to get the list of passwords in md-settings. Also adds the PasswordsPrivateApi so that chrome.send is encapsulated. Updates passwords section to get correct field (was previously using |origin| should be using |shownUrl|). BUG=546836 Committed: https://crrev.com/84175a076ef3a326d3db0e8396c69a851ad16d79 Cr-Commit-Position: refs/heads/master@{#372534} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/84175a076ef3a326d3db0e8396c69a851ad16d79 Cr-Commit-Position: refs/heads/master@{#372534} |
