Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2230513003:
[MD settings] setup site settings menus (Closed)
|
Created:
4 years, 4 months ago by dschuyler Modified:
4 years, 4 months ago Reviewers:
tommycli CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, arv+watch_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] setup site settings menus
This CL gets the site settings dropdown menus working again and adds a
unit test to help detect the issue in the future.
BUG=636045
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/4e44460d32499881c1c18830202f5b6f1ccebe96
Cr-Commit-Position: refs/heads/master@{#411522}
Patch Set 1 #
Total comments: 2
Patch Set 2 : review changes #
Total comments: 4
Patch Set 3 : review changes #
Total comments: 4
Patch Set 4 : review changes #
Messages
Total messages: 30 (18 generated)
Description was changed from ========== [MD settings] setup site settings menus This CL gets the site settings dropdown menus working again and adds a unit test to help detect the issue in the future. BUG=636045 ========== to ========== [MD settings] setup site settings menus This CL gets the site settings dropdown menus working again and adds a unit test to help detect the issue in the future. BUG=636045 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by dschuyler@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...
dschuyler@chromium.org changed reviewers: + tommycli@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
thanks see below https://codereview.chromium.org/2230513003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/site_settings/site_list.html (right): https://codereview.chromium.org/2230513003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/site_settings/site_list.html:69: <paper-menu-button hidden="[[isPolicyControlled_(item.source)]]"> shouldShowMenu still exists in the js file. Also this changes the meaning somewhat, since shouldShowMenu is true if this.allSites is true. Does that still work?
The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run
Thanks! https://codereview.chromium.org/2230513003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/site_settings/site_list.html (right): https://codereview.chromium.org/2230513003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/site_settings/site_list.html:69: <paper-menu-button hidden="[[isPolicyControlled_(item.source)]]"> On 2016/08/10 17:31:23, tommycli wrote: > shouldShowMenu still exists in the js file. > > Also this changes the meaning somewhat, since shouldShowMenu is true if > this.allSites is true. Does that still work? Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2230513003/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/settings/site_list_tests.js (right): https://codereview.chromium.org/2230513003/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/site_list_tests.js:656: // menu item. I think the comment needs an update. https://codereview.chromium.org/2230513003/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/site_list_tests.js:664: MockInteractions.tap(menuItems[0]); After the tap, return browserProxy.whenCalled('resetCategoryPermission') or browserProxy.whenCalled('setCategoryPermission'), to ensure that we 'wait' for the test to go through the problematic section of code.
The CQ bit was checked by dschuyler@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/2230513003/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/settings/site_list_tests.js (right): https://codereview.chromium.org/2230513003/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/site_list_tests.js:656: // menu item. On 2016/08/11 21:53:10, tommycli wrote: > I think the comment needs an update. Done. https://codereview.chromium.org/2230513003/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/site_list_tests.js:664: MockInteractions.tap(menuItems[0]); On 2016/08/11 21:53:10, tommycli wrote: > After the tap, return browserProxy.whenCalled('resetCategoryPermission') or > browserProxy.whenCalled('setCategoryPermission'), to ensure that we 'wait' for > the test to go through the problematic section of code. Done.
https://codereview.chromium.org/2230513003/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/site_list_tests.js (right): https://codereview.chromium.org/2230513003/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/site_list_tests.js:662: return Promise.race([ Since we control the test data, we know which of these two should be called right? And then we can only listen to one of them? https://codereview.chromium.org/2230513003/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/site_list_tests.js:665: ]).then(function(contentType) { no need for 'then'
The CQ bit was checked by dschuyler@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/2230513003/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/site_list_tests.js (right): https://codereview.chromium.org/2230513003/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/site_list_tests.js:662: return Promise.race([ On 2016/08/11 22:46:24, tommycli wrote: > Since we control the test data, we know which of these two should be called > right? And then we can only listen to one of them? Done. https://codereview.chromium.org/2230513003/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/site_list_tests.js:665: ]).then(function(contentType) { On 2016/08/11 22:46:24, tommycli wrote: > no need for 'then' Done.
lgtm thank you!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dschuyler@chromium.org
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.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [MD settings] setup site settings menus This CL gets the site settings dropdown menus working again and adds a unit test to help detect the issue in the future. BUG=636045 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD settings] setup site settings menus This CL gets the site settings dropdown menus working again and adds a unit test to help detect the issue in the future. BUG=636045 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/4e44460d32499881c1c18830202f5b6f1ccebe96 Cr-Commit-Position: refs/heads/master@{#411522} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/4e44460d32499881c1c18830202f5b6f1ccebe96 Cr-Commit-Position: refs/heads/master@{#411522} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
