Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2915743002:
MD Settings: Include settings for displaying Images in Site Details. (Closed)
|
Created:
3 years, 6 months ago by Patti Lor Modified:
3 years, 6 months ago 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, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: Include settings for displaying Images in Site Details.
The "Images" content setting is missing from "Site Details". Add it.
chrome: //settings/content/siteDetails?site=http%3A%2F%2Fpermission.site%3A80
Review-Url: https://codereview.chromium.org/2915743002
Cr-Commit-Position: refs/heads/master@{#476493}
Committed: https://chromium.googlesource.com/chromium/src/+/3cc2beebbda59f2cdd99f2f1b309999ee3a78c54
Patch Set 1 #Patch Set 2 : Add entries for images in TestSiteSettingsPrefsBrowserProxy. #Patch Set 3 : Revert accidental include removal. #Patch Set 4 : Fixup #
Total comments: 3
Messages
Total messages: 29 (20 generated)
Description was changed from ========== MD Settings: Include settings for displaying Images in Site Details. The "Images" content setting is missing from "Site Details". Add it. BUG=656758 TEST=With #enable-site-settings and/or #enable-site-details turned on, navigate to http://permission.site. From the Page Info bubble, change the "Images" setting away from "Allow (default)". Then go to chrome://settings/content/siteDetails?site=http%3A%2F%2Fpermission.site%3A80 and verify "Images" shows up with the setting it was changed to. ========== to ========== MD Settings: Include settings for displaying Images in Site Details. The "Images" content setting is missing from "Site Details". Add it. BUG=656758 TEST=With #enable-site-settings and/or #enable-site-details turned on, navigate to http://permission.site. From the Page Info bubble, change the "Images" setting away from "Allow (default)". Then go to chrome://settings/content/siteDetails?site=http%3A%2F%2Fpermission.site%3A80 and verify "Images" shows up with the setting it was changed to. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by patricialor@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_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 patricialor@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...
The CQ bit was checked by patricialor@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_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 patricialor@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...
patricialor@chromium.org changed reviewers: + tsergeant@chromium.org
Hi Tim, PTAL!
Generally looks good 👍 https://codereview.chromium.org/2915743002/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/settings/site_details_tests.js (right): https://codereview.chromium.org/2915743002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/site_details_tests.js:170: assertFalse(testElement.$.mic.hidden, msg); Should images be added to these test cases?
https://codereview.chromium.org/2915743002/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/settings/site_details_tests.js (right): https://codereview.chromium.org/2915743002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/site_details_tests.js:170: assertFalse(testElement.$.mic.hidden, msg); On 2017/06/01 04:02:24, tsergeant wrote: > Should images be added to these test cases? Oops, yes. calamity@ said in a separate CL (that shows all the permissions all the time) to delete this test though so I am leaving it as-is (see https://codereview.chromium.org/2912253003/diff/60001/chrome/test/data/webui/...).
Cool, lgtm https://codereview.chromium.org/2915743002/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/settings/site_details_tests.js (right): https://codereview.chromium.org/2915743002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/site_details_tests.js:170: assertFalse(testElement.$.mic.hidden, msg); On 2017/06/01 04:22:24, Patti Lor wrote: > On 2017/06/01 04:02:24, tsergeant wrote: > > Should images be added to these test cases? > > Oops, yes. calamity@ said in a separate CL (that shows all the permissions all > the time) to delete this test though so I am leaving it as-is (see > https://codereview.chromium.org/2912253003/diff/60001/chrome/test/data/webui/...). Fair enough
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
patricialor@chromium.org changed reviewers: + dbeam@chromium.org
Hi dbeam, PTAL at chrome/browser/resources/settings/site_settings/site_details_permission.js Thanks!
i don't see that file in your most recent patchset, but the current stuff lgtm
Oops, copy-paste error I guess (I didn't mean to reference that file in this CL). Thanks both for the reviews!
The CQ bit was checked by patricialor@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1496362084833560,
"parent_rev": "93932dc8c8355fd719083079ee153f0f953fe767", "commit_rev":
"3cc2beebbda59f2cdd99f2f1b309999ee3a78c54"}
Message was sent while issue was closed.
Description was changed from ========== MD Settings: Include settings for displaying Images in Site Details. The "Images" content setting is missing from "Site Details". Add it. BUG=656758 TEST=With #enable-site-settings and/or #enable-site-details turned on, navigate to http://permission.site. From the Page Info bubble, change the "Images" setting away from "Allow (default)". Then go to chrome://settings/content/siteDetails?site=http%3A%2F%2Fpermission.site%3A80 and verify "Images" shows up with the setting it was changed to. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Include settings for displaying Images in Site Details. The "Images" content setting is missing from "Site Details". Add it. chrome: //settings/content/siteDetails?site=http%3A%2F%2Fpermission.site%3A80 Review-Url: https://codereview.chromium.org/2915743002 Cr-Commit-Position: refs/heads/master@{#476493} Committed: https://chromium.googlesource.com/chromium/src/+/3cc2beebbda59f2cdd99f2f1b309... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/3cc2beebbda59f2cdd99f2f1b309... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
