Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2092763004:
[MD Settings] Implement search in material design passwords. (Closed)
|
Created:
4 years, 6 months ago by hcarmona Modified:
4 years, 5 months ago Reviewers:
dpapad CC:
arv+watch_chromium.org, chromium-reviews, dbeam+watch-settings_chromium.org, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, stevenjb+watch-md-settings_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[MD Settings] Implement search in material design passwords.
Includes test. Screenshot in bug.
BUG=626119
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/9dd148fc30594c576d61fac30e67b1dd8f4977a5
Cr-Commit-Position: refs/heads/master@{#406736}
Patch Set 1 : fix test #
Total comments: 8
Patch Set 2 : Feedback #Patch Set 3 : Update to better match mocks #
Total comments: 4
Patch Set 4 : dom-if and rebase #
Total comments: 4
Patch Set 5 : Feedback and Rebase #
Total comments: 12
Patch Set 6 : feedback #
Total comments: 4
Patch Set 7 : nits #
Total comments: 2
Patch Set 8 : indexOf -> includes #Messages
Total messages: 59 (40 generated)
Description was changed from ========== [MD Settings] Implement search in material design passwords. Includes test. BUG=595538 ========== to ========== [MD Settings] Implement search in material design passwords. Includes test. BUG=595538 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Patchset #1 (id:1) has been deleted
Description was changed from ========== [MD Settings] Implement search in material design passwords. Includes test. BUG=595538 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== [MD Settings] Implement search in material design passwords. Includes test. Screenshot in bug. BUG=595538 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
hcarmona@chromium.org changed reviewers: + dschuyler@chromium.org
PTAL, screenshots in bug. Thanks!
https://codereview.chromium.org/2092763004/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js (right): https://codereview.chromium.org/2092763004/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js:50: /** Filter on the saved passwords and exceptions */ nit: missing period. https://codereview.chromium.org/2092763004/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js:58: type: String, Should type be Array? https://codereview.chromium.org/2092763004/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js:62: /** A filtered list of password exceptions. */ If type is really Array, we'll want a @type. https://codereview.chromium.org/2092763004/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js:64: type: String, Should type be Array?
https://codereview.chromium.org/2092763004/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js (right): https://codereview.chromium.org/2092763004/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js:50: /** Filter on the saved passwords and exceptions */ On 2016/06/25 01:42:55, dschuyler wrote: > nit: missing period. Done. https://codereview.chromium.org/2092763004/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js:58: type: String, On 2016/06/25 01:42:55, dschuyler wrote: > Should type be Array? Done. https://codereview.chromium.org/2092763004/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js:62: /** A filtered list of password exceptions. */ On 2016/06/25 01:42:55, dschuyler wrote: > If type is really Array, we'll want a @type. Done. https://codereview.chromium.org/2092763004/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js:64: type: String, On 2016/06/25 01:42:55, dschuyler wrote: > Should type be Array? Done.
Let's have the search follow a similar pattern to the title bar search and dropdown menus where the controls (such as the clear text X button) match the text color. I think the text color here is paper grey 800.
Description was changed from ========== [MD Settings] Implement search in material design passwords. Includes test. Screenshot in bug. BUG=595538 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== [MD Settings] Implement search in material design passwords. Includes test. Screenshot in bug. BUG=595538 ==========
Description was changed from ========== [MD Settings] Implement search in material design passwords. Includes test. Screenshot in bug. BUG=595538 ========== to ========== [MD Settings] Implement search in material design passwords. Includes test. Screenshot in bug. BUG=595538 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Patchset #4 (id:80001) has been deleted
Patchset #3 (id:60001) has been deleted
Description was changed from ========== [MD Settings] Implement search in material design passwords. Includes test. Screenshot in bug. BUG=595538 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== [MD Settings] Implement search in material design passwords. Includes test. Screenshot in bug. BUG=626119 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
hcarmona@chromium.org changed reviewers: + dpapad@chromium.org - dschuyler@chromium.org
dschuyler's out and he was my reviewer, can you provide feedback for search in passwords? I've added updated screenshots. Alan is cc'd on the bug, so he can comment. Thanks!
Sending initial comment, have not gone through the entire CL yet. https://codereview.chromium.org/2092763004/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_page/settings_subpage.html (right): https://codereview.chromium.org/2092763004/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/settings_page/settings_subpage.html:71: <paper-input-container id="searchContainer" no-label-float hidden Can we re-use the existing cr-search-field instead of rolling our own search field here? See https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/cr_search... Also having this complex search field in the DOM for every subpage, when we only need it for the manage-passwords <settings-subpage> seems wasteful. Can we use dom-if instead of hidden?
The CQ bit was checked by hcarmona@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2092763004/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_page/settings_subpage.html (right): https://codereview.chromium.org/2092763004/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/settings_page/settings_subpage.html:71: <paper-input-container id="searchContainer" no-label-float hidden On 2016/07/13 18:11:17, dpapad wrote: > Can we re-use the existing cr-search-field instead of rolling our own search > field here? See > https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/cr_search... > > Also having this complex search field in the DOM for every subpage, when we only > need it for the manage-passwords <settings-subpage> seems wasteful. Can we use > dom-if instead of hidden? cr-search-field doesn't match the mocks. If we can get bettes@ to OK the changes, then it's not a big deal to change to use cr-search-field. See http://crrev.com/2147103002 for a rough idea. I started styling it to match our mocks but it felt like I was forcing cr-search-field to be two different things. The main differences are: 1. Search in passwords is always expanded 2. Clicking the clear button will clear the search instead toggling 3. Search field needs to be longer and isn't easily expanded w/ mixin As for the dom-if part: Done.
https://codereview.chromium.org/2092763004/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_page/settings_subpage.html (right): https://codereview.chromium.org/2092763004/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/settings_page/settings_subpage.html:71: <paper-input-container id="searchContainer" no-label-float hidden On 2016/07/13 at 23:19:40, Hector Carmona wrote: > On 2016/07/13 18:11:17, dpapad wrote: > > Can we re-use the existing cr-search-field instead of rolling our own search > > field here? See > > https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/cr_search... > > > > Also having this complex search field in the DOM for every subpage, when we only > > need it for the manage-passwords <settings-subpage> seems wasteful. Can we use > > dom-if instead of hidden? > > cr-search-field doesn't match the mocks. If we can get bettes@ to OK the > changes, then it's not a big deal to change to use cr-search-field. > See http://crrev.com/2147103002 for a rough idea. > > I started styling it to match our mocks but it felt like I was forcing > cr-search-field to be two different things. The main differences are: > 1. Search in passwords is always expanded > 2. Clicking the clear button will clear the search instead toggling > 3. Search field needs to be longer and isn't easily expanded w/ mixin > > As for the dom-if part: Done. cr-search-field logic is broken down to two parts 1) A "canonical" implementation with default styling (via the <cr-search-field> element). 2) A cr_search_field_behavior.js for cases where the default styling needs to be heavily modified. An example of a search field that uses the behavior, with completely different styling (and HTML) is https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/cr_toolba.... Even if styling is different, we should be able to use the behavior. https://codereview.chromium.org/2092763004/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_page/settings_subpage.html (right): https://codereview.chromium.org/2092763004/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/settings_page/settings_subpage.html:71: <template id="searchContainer" is="dom-if"> There is no need to assign an "id" to the template. This will actually interfere with the "search within settings" algorithm. The proper way to use dom-if is as follows <template is="dom-if" if="[[someBooleanVariableHere_]]"> In your code, all you have to do is flip the boolean, instead of finding the template by ID and setting if to true. You could even avoid having an observer on searchLabel completely, as follows <template is="dom-if" if="[[searchLabel]]"> https://codereview.chromium.org/2092763004/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/settings_page/settings_subpage.html:72: <paper-input-container no-label-float on-search="onSearchTermSearch_"> If we end up using our own HTML, paper-input-container also needs to be imported.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [MD Settings] Implement search in material design passwords. Includes test. Screenshot in bug. BUG=626119 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== [MD Settings] Implement search in material design passwords. Includes test. Screenshot in bug. BUG=626119 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation ==========
Patchset #5 (id:140001) has been deleted
The CQ bit was checked by hcarmona@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
For more details, see http://crbug.com/617627.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
Description was changed from ========== [MD Settings] Implement search in material design passwords. Includes test. Screenshot in bug. BUG=626119 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD Settings] Implement search in material design passwords. Includes test. Screenshot in bug. BUG=626119 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by hcarmona@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #5 (id:160001) has been deleted
PTAL, this is now using the CrSearchFieldBehavior https://codereview.chromium.org/2092763004/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_page/settings_subpage.html (right): https://codereview.chromium.org/2092763004/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/settings_page/settings_subpage.html:71: <paper-input-container id="searchContainer" no-label-float hidden On 2016/07/13 23:32:41, dpapad wrote: > On 2016/07/13 at 23:19:40, Hector Carmona wrote: > > On 2016/07/13 18:11:17, dpapad wrote: > > > Can we re-use the existing cr-search-field instead of rolling our own search > > > field here? See > > > > https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/cr_search... > > > > > > Also having this complex search field in the DOM for every subpage, when we > only > > > need it for the manage-passwords <settings-subpage> seems wasteful. Can we > use > > > dom-if instead of hidden? > > > > cr-search-field doesn't match the mocks. If we can get bettes@ to OK the > > changes, then it's not a big deal to change to use cr-search-field. > > See http://crrev.com/2147103002 for a rough idea. > > > > I started styling it to match our mocks but it felt like I was forcing > > cr-search-field to be two different things. The main differences are: > > 1. Search in passwords is always expanded > > 2. Clicking the clear button will clear the search instead toggling > > 3. Search field needs to be longer and isn't easily expanded w/ mixin > > > > As for the dom-if part: Done. > > cr-search-field logic is broken down to two parts > 1) A "canonical" implementation with default styling (via the <cr-search-field> > element). > 2) A cr_search_field_behavior.js for cases where the default styling needs to be > heavily modified. An example of a search field that uses the behavior, with > completely different styling (and HTML) is > https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/cr_toolba.... > > Even if styling is different, we should be able to use the behavior. Sounds good. https://codereview.chromium.org/2092763004/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_page/settings_subpage.html (right): https://codereview.chromium.org/2092763004/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/settings_page/settings_subpage.html:71: <template id="searchContainer" is="dom-if"> On 2016/07/13 23:32:41, dpapad wrote: > There is no need to assign an "id" to the template. This will actually interfere > with the "search within settings" algorithm. > > The proper way to use dom-if is as follows > > <template is="dom-if" if="[[someBooleanVariableHere_]]"> > > In your code, all you have to do is flip the boolean, instead of finding the > template by ID and setting if to true. You could even avoid having an observer > on searchLabel completely, as follows > > <template is="dom-if" if="[[searchLabel]]"> I knew of the data binding syntax, but didn't realize it would work if there's a type mismatch. Works great! Thanks for the heads up so I don't break search for the section. https://codereview.chromium.org/2092763004/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/settings_page/settings_subpage.html:72: <paper-input-container no-label-float on-search="onSearchTermSearch_"> On 2016/07/13 23:32:41, dpapad wrote: > If we end up using our own HTML, paper-input-container also needs to be > imported. Done.
https://codereview.chromium.org/2092763004/diff/180001/chrome/browser/resourc... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js (right): https://codereview.chromium.org/2092763004/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js:335: passwordFilter: String, Can we turn this member variable to private? https://codereview.chromium.org/2092763004/diff/180001/chrome/browser/resourc... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html (right): https://codereview.chromium.org/2092763004/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html:71: <iron-list id="passwordList" items="[[filteredPasswords]]" Nit (optional): You can avoid the need for computed properties by directly invoking a function here. <iron-list id="passwordList" items="[[getFilteredPasswords_(savedPasswords, filter)]]" and rename savedPasswords_() to getFilteredPasswords_(). Same holds for |filteredExceptions below|. https://codereview.chromium.org/2092763004/diff/180001/chrome/browser/resourc... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js (right): https://codereview.chromium.org/2092763004/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js:67: filteredPasswords: { If you end up keeping those (see previous comment on how to remove them), can filteredPasswords and filteredExceptions be private? https://codereview.chromium.org/2092763004/diff/180001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_page/settings_subpage.js (right): https://codereview.chromium.org/2092763004/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/settings_page/settings_subpage.js:43: this.notifyPath('searchTerm', this.searchTerm); Is the call to notifyPath necessary? Given that we use "notify: true" for |searchTerm|, and that it is bound with a 2-way binding to the parent's |passwordFilter| it seems that it is not. https://codereview.chromium.org/2092763004/diff/180001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_page/settings_subpage_search.js (right): https://codereview.chromium.org/2092763004/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/settings_page/settings_subpage_search.js:19: this.$.searchInput.value = ''; Can we avoid having to fire any events manually by calling CrSearchFieldBehavior#setValue() instead? IIUC this method is supposed to be the canonical way of programmatically modifying the textbox contents. https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/cr_search... https://codereview.chromium.org/2092763004/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/settings_page/settings_subpage_search.js:28: * @param {string} lastValue_ Nit: Parameter names don't usually have underscore suffix.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by hcarmona@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2092763004/diff/180001/chrome/browser/resourc... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js (right): https://codereview.chromium.org/2092763004/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js:335: passwordFilter: String, On 2016/07/20 19:34:51, dpapad wrote: > Can we turn this member variable to private? Done. https://codereview.chromium.org/2092763004/diff/180001/chrome/browser/resourc... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html (right): https://codereview.chromium.org/2092763004/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html:71: <iron-list id="passwordList" items="[[filteredPasswords]]" On 2016/07/20 19:34:51, dpapad wrote: > Nit (optional): You can avoid the need for computed properties by directly > invoking a function here. > > <iron-list id="passwordList" items="[[getFilteredPasswords_(savedPasswords, > filter)]]" > and rename savedPasswords_() to getFilteredPasswords_(). Same holds for > |filteredExceptions below|. Done. https://codereview.chromium.org/2092763004/diff/180001/chrome/browser/resourc... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js (right): https://codereview.chromium.org/2092763004/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js:67: filteredPasswords: { On 2016/07/20 19:34:51, dpapad wrote: > If you end up keeping those (see previous comment on how to remove them), can > filteredPasswords and filteredExceptions be private? Removed. https://codereview.chromium.org/2092763004/diff/180001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_page/settings_subpage.js (right): https://codereview.chromium.org/2092763004/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/settings_page/settings_subpage.js:43: this.notifyPath('searchTerm', this.searchTerm); On 2016/07/20 19:34:51, dpapad wrote: > Is the call to notifyPath necessary? > Given that we use "notify: true" for |searchTerm|, and that it is bound with a > 2-way binding to the parent's |passwordFilter| it seems that it is not. Not necessary. Removed. https://codereview.chromium.org/2092763004/diff/180001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_page/settings_subpage_search.js (right): https://codereview.chromium.org/2092763004/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/settings_page/settings_subpage_search.js:19: this.$.searchInput.value = ''; On 2016/07/20 19:34:51, dpapad wrote: > Can we avoid having to fire any events manually by calling > CrSearchFieldBehavior#setValue() instead? IIUC this method is supposed to be the > canonical way of programmatically modifying the textbox contents. > > https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/cr_search... Done. https://codereview.chromium.org/2092763004/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/settings_page/settings_subpage_search.js:28: * @param {string} lastValue_ On 2016/07/20 19:34:51, dpapad wrote: > Nit: Parameter names don't usually have underscore suffix. On second look, this entire method is unnecessary, bound directly to |lastValue_|.
LGTM! https://codereview.chromium.org/2092763004/diff/200001/chrome/browser/resourc... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js (right): https://codereview.chromium.org/2092763004/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js:334: /** Filter applied to passwords and password exceptions. */ @private https://codereview.chromium.org/2092763004/diff/200001/chrome/browser/resourc... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js (right): https://codereview.chromium.org/2092763004/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js:110: return password.loginPair.originUrl.indexOf(filter) >= 0 || Nit (optional): Could we use String#includes method instead? It has been in Chrome since M40, see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje.... I am not sure if the compiler externs have been updated though.
The CQ bit was checked by hcarmona@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for the feedback! https://codereview.chromium.org/2092763004/diff/200001/chrome/browser/resourc... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js (right): https://codereview.chromium.org/2092763004/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js:334: /** Filter applied to passwords and password exceptions. */ On 2016/07/20 21:20:47, dpapad wrote: > @private Done. https://codereview.chromium.org/2092763004/diff/200001/chrome/browser/resourc... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js (right): https://codereview.chromium.org/2092763004/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js:110: return password.loginPair.originUrl.indexOf(filter) >= 0 || On 2016/07/20 21:20:47, dpapad wrote: > Nit (optional): Could we use String#includes method instead? It has been in > Chrome since M40, see > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje.... > I am not sure if the compiler externs have been updated though. Closure compiler didn't complain. Done.
https://codereview.chromium.org/2092763004/diff/220001/chrome/browser/resourc... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js (right): https://codereview.chromium.org/2092763004/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js:126: return exception.exceptionUrl.indexOf(filter) >= 0; Nit: Use includes() here too?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by hcarmona@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2092763004/diff/220001/chrome/browser/resourc... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js (right): https://codereview.chromium.org/2092763004/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js:126: return exception.exceptionUrl.indexOf(filter) >= 0; On 2016/07/20 21:34:14, dpapad wrote: > Nit: Use includes() here too? Missed that. Good catch.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by hcarmona@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org Link to the patchset: https://codereview.chromium.org/2092763004/#ps240001 (title: "indexOf -> includes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [MD Settings] Implement search in material design passwords. Includes test. Screenshot in bug. BUG=626119 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD Settings] Implement search in material design passwords. Includes test. Screenshot in bug. BUG=626119 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #8 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== [MD Settings] Implement search in material design passwords. Includes test. Screenshot in bug. BUG=626119 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD Settings] Implement search in material design passwords. Includes test. Screenshot in bug. BUG=626119 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/9dd148fc30594c576d61fac30e67b1dd8f4977a5 Cr-Commit-Position: refs/heads/master@{#406736} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/9dd148fc30594c576d61fac30e67b1dd8f4977a5 Cr-Commit-Position: refs/heads/master@{#406736} |
