Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2428733002:
MD Settings: Migrating autofill cr-shared-menu to settings-action-menu. (Closed)
|
Created:
4 years, 2 months ago by dpapad Modified:
4 years, 2 months ago Reviewers:
hcarmona CC:
arv+watch_chromium.org, chromium-reviews, dbeam+watch-settings_chromium.org, Dan Beam, hcarmona, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, stevenjb+watch-md-settings_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: Migrating autofill cr-shared-menu to settings-action-menu.
- Stop interpreting |activeAddress|, |activeCreditCard| as a signal to open
dialogs.
- Use |activeAddress|, |activeCreditCard| as the model for open dialogs and
shared action menus.
- Use data binding to hide/show action menu entries, instead of setting
'hidden' programmatically.
BUG=639718
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/2a040cae1c0b74ab0cd585d40f45d4d266fdc3bc
Cr-Commit-Position: refs/heads/master@{#426039}
Patch Set 1 #Patch Set 2 : Test #
Total comments: 2
Patch Set 3 : Fix compilation. #Patch Set 4 : Fix address tests. #
Total comments: 6
Patch Set 5 : Fixtypo #Patch Set 6 : Re-sort #
Messages
Total messages: 28 (19 generated)
Description was changed from ========== MD Settings: Migrating autofill cr-shared-menu to settings-action-menu. BUG=639718 ========== to ========== MD Settings: Migrating autofill cr-shared-menu to settings-action-menu. BUG=639718 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== MD Settings: Migrating autofill cr-shared-menu to settings-action-menu. BUG=639718 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Migrating autofill cr-shared-menu to settings-action-menu. - Stop interpreting activeAddress, activeCreditCard as a signal to open dialogs. - Use activeAddress, activeCreditCard as the model for open dialogs and shared action menus. - Use data binding to hide/show action menu entries (instead of setting 'hidden' programmatically). BUG=639718 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by dpapad@chromium.org to run a CQ dry run
Description was changed from ========== MD Settings: Migrating autofill cr-shared-menu to settings-action-menu. - Stop interpreting activeAddress, activeCreditCard as a signal to open dialogs. - Use activeAddress, activeCreditCard as the model for open dialogs and shared action menus. - Use data binding to hide/show action menu entries (instead of setting 'hidden' programmatically). BUG=639718 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Migrating autofill cr-shared-menu to settings-action-menu. - Stop interpreting activeAddress, activeCreditCard as a signal to open dialogs. - Use activeAddress, activeCreditCard as the model for open dialogs and shared action menus. - Use data binding to hide/show action menu entries (instead of setting 'hidden' programmatically). BUG=639718 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== MD Settings: Migrating autofill cr-shared-menu to settings-action-menu. - Stop interpreting activeAddress, activeCreditCard as a signal to open dialogs. - Use activeAddress, activeCreditCard as the model for open dialogs and shared action menus. - Use data binding to hide/show action menu entries (instead of setting 'hidden' programmatically). BUG=639718 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Migrating autofill cr-shared-menu to settings-action-menu. - Stop interpreting |activeAddress|, |activeCreditCard| as a signal to open dialogs. - Use |activeAddress|, |activeCreditCard| as the model for open dialogs and shared action menus. - Use data binding to hide/show action menu entries (instead of setting 'hidden' programmatically). BUG=639718 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== MD Settings: Migrating autofill cr-shared-menu to settings-action-menu. - Stop interpreting |activeAddress|, |activeCreditCard| as a signal to open dialogs. - Use |activeAddress|, |activeCreditCard| as the model for open dialogs and shared action menus. - Use data binding to hide/show action menu entries (instead of setting 'hidden' programmatically). BUG=639718 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Migrating autofill cr-shared-menu to settings-action-menu. - Stop interpreting |activeAddress|, |activeCreditCard| as a signal to open dialogs. - Use |activeAddress|, |activeCreditCard| as the model for open dialogs and shared action menus. - Use data binding to hide/show action menu entries, instead of setting 'hidden' programmatically). BUG=639718 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
dpapad@chromium.org changed reviewers: + dbeam@chromium.org
https://codereview.chromium.org/2428733002/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_autofill_section_browsertest.js (left): https://codereview.chromium.org/2428733002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:13: ROOT_PATH + 'ui/webui/resources/js/load_time_data.js', settings_action_menu.html depends on direction_delagate.html, which depends on i18n_setup.html, which loads loadTimeData. So I had to remove this line otherwise I get the following error Unexpected condition on chrome://md-settings/passwords_and_forms_page/autofill_section.html: should only include this file once. https://codereview.chromium.org/2428733002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:126: loadTimeData.data = this.i18nStrings; This line started causing me problems, because loadTimeData was already populated at the time this was called, triggering the assertion at https://cs.chromium.org/chromium/src/ui/webui/resources/js/load_time_data.js?.... I don't think this is needed at all though (tests pass even without it, since those i18n strings are already in loadTimeData.
Description was changed from ========== MD Settings: Migrating autofill cr-shared-menu to settings-action-menu. - Stop interpreting |activeAddress|, |activeCreditCard| as a signal to open dialogs. - Use |activeAddress|, |activeCreditCard| as the model for open dialogs and shared action menus. - Use data binding to hide/show action menu entries, instead of setting 'hidden' programmatically). BUG=639718 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Migrating autofill cr-shared-menu to settings-action-menu. - Stop interpreting |activeAddress|, |activeCreditCard| as a signal to open dialogs. - Use |activeAddress|, |activeCreditCard| as the model for open dialogs and shared action menus. - Use data binding to hide/show action menu entries, instead of setting 'hidden' programmatically. BUG=639718 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by dpapad@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...
Description was changed from ========== MD Settings: Migrating autofill cr-shared-menu to settings-action-menu. - Stop interpreting |activeAddress|, |activeCreditCard| as a signal to open dialogs. - Use |activeAddress|, |activeCreditCard| as the model for open dialogs and shared action menus. - Use data binding to hide/show action menu entries, instead of setting 'hidden' programmatically. BUG=639718 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Migrating autofill cr-shared-menu to settings-action-menu. - Stop interpreting |activeAddress|, |activeCreditCard| as a signal to open dialogs. - Use |activeAddress|, |activeCreditCard| as the model for open dialogs and shared action menus. - Use data binding to hide/show action menu entries, instead of setting 'hidden' programmatically. BUG=639718 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
dbeam@chromium.org changed reviewers: + hcarmona@chromium.org - dbeam@chromium.org
+hcarmona@ as he's got more experience with this code
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM, just 2 nits. https://codereview.chromium.org/2428733002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.html (right): https://codereview.chromium.org/2428733002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.html:66: hidden$="[[!activeAddress.metadata.isLocal"] typo here, should be: hidden$="[[!activeAddress.metadata.isLocal]]" https://codereview.chromium.org/2428733002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/compiled_resources2.gyp (right): https://codereview.chromium.org/2428733002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/compiled_resources2.gyp:24: 'address_edit_dialog', Can these be sorted?
https://codereview.chromium.org/2428733002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.html (right): https://codereview.chromium.org/2428733002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.html:66: hidden$="[[!activeAddress.metadata.isLocal"] On 2016/10/18 at 14:24:42, Hector Carmona wrote: > typo here, should be: > hidden$="[[!activeAddress.metadata.isLocal]]" Good catch. https://codereview.chromium.org/2428733002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/compiled_resources2.gyp (right): https://codereview.chromium.org/2428733002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/compiled_resources2.gyp:24: 'address_edit_dialog', On 2016/10/18 at 14:24:42, Hector Carmona wrote: > Can these be sorted? This is the order after sorting with Unix 'sort' utility.
https://codereview.chromium.org/2428733002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/compiled_resources2.gyp (right): https://codereview.chromium.org/2428733002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/compiled_resources2.gyp:24: 'address_edit_dialog', On 2016/10/18 19:02:35, dpapad wrote: > On 2016/10/18 at 14:24:42, Hector Carmona wrote: > > Can these be sorted? > > This is the order after sorting with Unix 'sort' utility. hmmmm, have you set LC_ALL=C before running !sort?
https://codereview.chromium.org/2428733002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/compiled_resources2.gyp (right): https://codereview.chromium.org/2428733002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/compiled_resources2.gyp:24: 'address_edit_dialog', > > This is the order after sorting with Unix 'sort' utility. > > hmmmm, have you set LC_ALL=C before running !sort? Done. Had not tried this until now.
The CQ bit was checked by dpapad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hcarmona@chromium.org Link to the patchset: https://codereview.chromium.org/2428733002/#ps100001 (title: "Re-sort")
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: Migrating autofill cr-shared-menu to settings-action-menu. - Stop interpreting |activeAddress|, |activeCreditCard| as a signal to open dialogs. - Use |activeAddress|, |activeCreditCard| as the model for open dialogs and shared action menus. - Use data binding to hide/show action menu entries, instead of setting 'hidden' programmatically. BUG=639718 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Migrating autofill cr-shared-menu to settings-action-menu. - Stop interpreting |activeAddress|, |activeCreditCard| as a signal to open dialogs. - Use |activeAddress|, |activeCreditCard| as the model for open dialogs and shared action menus. - Use data binding to hide/show action menu entries, instead of setting 'hidden' programmatically. BUG=639718 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== MD Settings: Migrating autofill cr-shared-menu to settings-action-menu. - Stop interpreting |activeAddress|, |activeCreditCard| as a signal to open dialogs. - Use |activeAddress|, |activeCreditCard| as the model for open dialogs and shared action menus. - Use data binding to hide/show action menu entries, instead of setting 'hidden' programmatically. BUG=639718 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Migrating autofill cr-shared-menu to settings-action-menu. - Stop interpreting |activeAddress|, |activeCreditCard| as a signal to open dialogs. - Use |activeAddress|, |activeCreditCard| as the model for open dialogs and shared action menus. - Use data binding to hide/show action menu entries, instead of setting 'hidden' programmatically. BUG=639718 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/2a040cae1c0b74ab0cd585d40f45d4d266fdc3bc Cr-Commit-Position: refs/heads/master@{#426039} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/2a040cae1c0b74ab0cd585d40f45d4d266fdc3bc Cr-Commit-Position: refs/heads/master@{#426039} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
