Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2936003003:
MD Settings: Set all content setting values in Site Details Javascript. (Closed)
|
Created:
3 years, 6 months ago by Patti Lor Modified:
3 years, 5 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: Set all content setting values in the <site-details> object.
r476514 changed the "Site Details" page to show all content settings instead of
the non-default ones. However, the back-end code supporting this UI still only
retrieved non-default settings. This CL fixes this problem to retrieve the
correct values for all content settings displayed, consistent with those shown
in the Page Info bubble.
Note that the "Site Details" UI will now only show the correct content setting
value for content settings that are non-default (previously, they were all
incorrect). A follow-up CL will be required to enable the UI to show default
settings, such as "Ask (default)", "Allow (default)" or "Block (default)"
values. Insecure (http) origins will also show that certain permissions have
been blocked, unlike the Page Info bubble.
See https://codereview.chromium.org/2946393002/ for C++ tests.
BUG=656758
TEST=With #enable-site-details turned on, navigate to https://permission.site.
Allow and deny a few permissions and verify these changes show up in the Page
Info bubble. Then click "Site settings" in the Page Info bubble, and check the
non-default 'Allow'- / 'Block'- ed content settings are also shown in the
"Site Details" page for https://permission.site and match the non-default values
shown in the Page Info bubble.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2936003003
Cr-Commit-Position: refs/heads/master@{#483614}
Committed: https://chromium.googlesource.com/chromium/src/+/f3b6e00fe2826406218b15bd2f317cfebe835696
Patch Set 1 #Patch Set 2 : Remove dependency on Page Info UI code and introduce new logic to retrieve content settings. #
Total comments: 8
Patch Set 3 : Review comments, tests & cleanup. #
Total comments: 16
Patch Set 4 : More comments and fixes. #Patch Set 5 : Fix last reference to |site| in site_details.html. #
Total comments: 16
Patch Set 6 : Review comments. #
Total comments: 4
Patch Set 7 : Switch content setting source logic to if statements. #
Total comments: 6
Patch Set 8 : Review comments. #
Total comments: 9
Patch Set 9 : Convert getCategoryPermissionForOrigin to getOriginPermissions #
Total comments: 4
Patch Set 10 : Fix some closure type declarations. #Patch Set 11 : Remove embeddingDisplayName. #
Total comments: 9
Patch Set 12 : Review comments. #Dependent Patchsets: Messages
Total messages: 87 (63 generated)
Description was changed from ========== MD Settings: Set all content setting values in Site Details Javascript. r476514 changed the "Site Details" page to show all content settings instead of the non-default ones. However, the back-end code supporting this UI still only retrieved non-default settings. This CL fixes this problem to retrieve the correct values for all content settings displayed, consistent with those shown in the Page Info bubble. Note that the "Site Details" UI will now only show the correct content setting value for content settings that are non-default (previously, they were all incorrect). A follow-up CL will be required to enable the UI to show default settings, such as "Ask (default)", "Allow (default)" or "Block (default)" values. BUG=656758 TEST=With #enable-site-details turned on, navigate to https://permission.site. Allow and deny a few permissions and verify these show up in the Page Info bubble as non-default. Then click "Site settings" in the Page Info bubble, and check the non-default 'Allow'- / 'Block'- ed content settings are also shown in the "Site Details" page for https://permission.site and match the non-default values shown in the Page Info bubble. ========== to ========== MD Settings: Set all content setting values in Site Details Javascript. r476514 changed the "Site Details" page to show all content settings instead of the non-default ones. However, the back-end code supporting this UI still only retrieved non-default settings. This CL fixes this problem to retrieve the correct values for all content settings displayed, consistent with those shown in the Page Info bubble. Note that the "Site Details" UI will now only show the correct content setting value for content settings that are non-default (previously, they were all incorrect). A follow-up CL will be required to enable the UI to show default settings, such as "Ask (default)", "Allow (default)" or "Block (default)" values. BUG=656758 TEST=With #enable-site-details turned on, navigate to https://permission.site. Allow and deny a few permissions and verify these show up in the Page Info bubble as non-default. Then click "Site settings" in the Page Info bubble, and check the non-default 'Allow'- / 'Block'- ed content settings are also shown in the "Site Details" page for https://permission.site and match the non-default values shown in the Page Info bubble. 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: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
Description was changed from ========== MD Settings: Set all content setting values in Site Details Javascript. r476514 changed the "Site Details" page to show all content settings instead of the non-default ones. However, the back-end code supporting this UI still only retrieved non-default settings. This CL fixes this problem to retrieve the correct values for all content settings displayed, consistent with those shown in the Page Info bubble. Note that the "Site Details" UI will now only show the correct content setting value for content settings that are non-default (previously, they were all incorrect). A follow-up CL will be required to enable the UI to show default settings, such as "Ask (default)", "Allow (default)" or "Block (default)" values. BUG=656758 TEST=With #enable-site-details turned on, navigate to https://permission.site. Allow and deny a few permissions and verify these show up in the Page Info bubble as non-default. Then click "Site settings" in the Page Info bubble, and check the non-default 'Allow'- / 'Block'- ed content settings are also shown in the "Site Details" page for https://permission.site and match the non-default values shown in the Page Info bubble. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Set all content setting values in the <site-details> object. r476514 changed the "Site Details" page to show all content settings instead of the non-default ones. However, the back-end code supporting this UI still only retrieved non-default settings. This CL fixes this problem to retrieve the correct values for all content settings displayed, consistent with those shown in the Page Info bubble. Note that the "Site Details" UI will now only show the correct content setting value for content settings that are non-default (previously, they were all incorrect). A follow-up CL will be required to enable the UI to show default settings, such as "Ask (default)", "Allow (default)" or "Block (default)" values. Insecure (http) origins will also show that certain permissions have been blocked, unlike the Page Info bubble. BUG=656758 TEST=With #enable-site-details turned on, navigate to https://permission.site. Allow and deny a few permissions and verify these changes show up in the Page Info bubble. Then click "Site settings" in the Page Info bubble, and check the non-default 'Allow'- / 'Block'- ed content settings are also shown in the "Site Details" page for https://permission.site and match the non-default values shown in the Page Info bubble. 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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
patricialor@chromium.org changed reviewers: + raymes@chromium.org, tsergeant@chromium.org
Hi Raymes and Tim, PTAL? I still need to add/fix tests and probably do a little more cleanup, but want to get an OK for the overall approach and feedback on whether this change should be split up. A summary is: - Replace <site-details>'s SiteException |site| member with just an |origin| string (SiteException has a "category" on it, which doesn't make sense since <site-details> contains all the permission categories). - Changes in the URL will update |origin|, which will trigger an update of all the <site-details-permissions> objects. - This calls into a new method HandleGetCategoryPermissionForOrigin(), which replaces the old HandleGetSiteDetails(). This talks to the PermissionManager / HCSM and combines their responses into a PermissionResult, which gets converted into a RawSiteException. - <site-details> converts that into a SiteException and assigns it to the corresponding <site-details-permission>'s |site| member. Thanks both!
https://codereview.chromium.org/2936003003/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/site_details.js (right): https://codereview.chromium.org/2936003003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_details.js:71: if (this.origin === undefined) This check shouldn't be necessary anymore. Polymer won't fire the observer until origin is defined. https://codereview.chromium.org/2936003003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_details.js:78: this.root.querySelectorAll('site-details-permission'), Because it's the future (Chrome 51+), NodeList has a forEach method that you can use directly: this.root.querySelectorAll('site-details-permission').forEach(function(element) { ... }) Closure will only like this if you rebase past https://chromium.googlesource.com/chromium/src/+/9ede2cde5727eecdb377b57b32c5..., which added in the definition for that method. https://codereview.chromium.org/2936003003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_details.js:83: .then(function(permission_result) { JS Style uses camelCase rather than snake_case https://codereview.chromium.org/2936003003/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js (right): https://codereview.chromium.org/2936003003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js:145: getCategoryPermissionForOrigin: function( As discussed in person, Closure is failing because this method needs annotating.
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: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
Looking good https://codereview.chromium.org/2936003003/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/site_details.js (left): https://codereview.chromium.org/2936003003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_details.js:20: site: { site looks like it may be referred to in the .html file, does that need to change? https://codereview.chromium.org/2936003003/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js (right): https://codereview.chromium.org/2936003003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js:146: * Gets the category permission for a given origin. nit: It may be worth noting that this will be a different value to what will be in the exception list for the site since it combines different sources of data into a single permission value. https://codereview.chromium.org/2936003003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js:152: contentType, origin, embeddingOrigin) {}, nit: I think embeddingOrigin isn't needed right now. Maybe we should just leave it out? https://codereview.chromium.org/2936003003/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/site_settings_handler.cc (right): https://codereview.chromium.org/2936003003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/site_settings_handler.cc:125: if (value->GetType() == base::Value::Type::INTEGER) Could this just be a DCHECK_EQ instead of a NOTREACHED below? https://codereview.chromium.org/2936003003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/site_settings_handler.cc:146: case PermissionStatusSource::EXTENSION: I think having the PermissionStatusSource's that aren't currently used in the permission system adds some complexity that mightn't be needed right now. Could we instead convert directly from PermissionStatusSource or content settings source to the string that we need? Maybe We could have a single helper function in this file to do that? https://codereview.chromium.org/2936003003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/site_settings_handler.cc:598: auto raw_site_exception = base::MakeUnique<base::DictionaryValue>(); nit: what does raw mean here? https://codereview.chromium.org/2936003003/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/site_details_tests.js (right): https://codereview.chromium.org/2936003003/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/site_details_tests.js:158: browserProxy.whenCalled('getCategoryPermissionForOrigin') As we discussed I think the test might have to be changed to guarantee this will finish before it passes.
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: This issue passed the CQ dry run.
Thanks Raymes and Tim, PTAL. I added one new JS test and amended existing ones (C++) to cover the new code. https://codereview.chromium.org/2936003003/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/site_details.js (right): https://codereview.chromium.org/2936003003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_details.js:71: if (this.origin === undefined) On 2017/06/16 03:11:40, tsergeant wrote: > This check shouldn't be necessary anymore. Polymer won't fire the observer until > origin is defined. Done. https://codereview.chromium.org/2936003003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_details.js:78: this.root.querySelectorAll('site-details-permission'), On 2017/06/16 03:11:40, tsergeant wrote: > Because it's the future (Chrome 51+), NodeList has a forEach method that you can > use directly: > > this.root.querySelectorAll('site-details-permission').forEach(function(element) > { ... }) > > Closure will only like this if you rebase past > https://chromium.googlesource.com/chromium/src/+/9ede2cde5727eecdb377b57b32c5..., > which added in the definition for that method. Ohh, I was kinda confused about this. Thanks for the FYI :) Rebased & fixed. https://codereview.chromium.org/2936003003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_details.js:83: .then(function(permission_result) { On 2017/06/16 03:11:39, tsergeant wrote: > JS Style uses camelCase rather than snake_case Done. https://codereview.chromium.org/2936003003/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js (right): https://codereview.chromium.org/2936003003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js:145: getCategoryPermissionForOrigin: function( On 2017/06/16 03:11:40, tsergeant wrote: > As discussed in person, Closure is failing because this method needs annotating. Thanks for debugging this Tim! Fixed :D https://codereview.chromium.org/2936003003/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/site_details.js (left): https://codereview.chromium.org/2936003003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_details.js:20: site: { On 2017/06/19 04:13:12, raymes wrote: > site looks like it may be referred to in the .html file, does that need to > change? Oops, you're right, thank you. I just removed it, since it's not doing anything. https://codereview.chromium.org/2936003003/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js (right): https://codereview.chromium.org/2936003003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js:146: * Gets the category permission for a given origin. On 2017/06/19 04:13:12, raymes wrote: > nit: It may be worth noting that this will be a different value to what will be > in the exception list for the site since it combines different sources of data > into a single permission value. Done, kind of quoted you verbatim. https://codereview.chromium.org/2936003003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js:152: contentType, origin, embeddingOrigin) {}, On 2017/06/19 04:13:12, raymes wrote: > nit: I think embeddingOrigin isn't needed right now. Maybe we should just leave > it out? Done. https://codereview.chromium.org/2936003003/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/site_settings_handler.cc (right): https://codereview.chromium.org/2936003003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/site_settings_handler.cc:125: if (value->GetType() == base::Value::Type::INTEGER) On 2017/06/19 04:13:12, raymes wrote: > Could this just be a DCHECK_EQ instead of a NOTREACHED below? Done. https://codereview.chromium.org/2936003003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/site_settings_handler.cc:146: case PermissionStatusSource::EXTENSION: On 2017/06/19 04:13:12, raymes wrote: > I think having the PermissionStatusSource's that aren't currently used in the > permission system adds some complexity that mightn't be needed right now. Could > we instead convert directly from PermissionStatusSource or content settings > source to the string that we need? Maybe We could have a single helper function > in this file to do that? Done (with the helper function approach). I realised there was a bug currently where if there was an embargoed setting that was also controlled by policy or extension it would return that it was controlled by embargo, so I changed the order to fix that. https://codereview.chromium.org/2936003003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/site_settings_handler.cc:598: auto raw_site_exception = base::MakeUnique<base::DictionaryValue>(); On 2017/06/19 04:13:12, raymes wrote: > nit: what does raw mean here? This method is supposed to return a RawSiteException (see https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/site_s...). expandSiteException (called in <site-details>'s onOriginChanged() is meant to convert that to a SiteException, similar to the original code. https://codereview.chromium.org/2936003003/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/site_details_tests.js (right): https://codereview.chromium.org/2936003003/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/site_details_tests.js:158: browserProxy.whenCalled('getCategoryPermissionForOrigin') On 2017/06/19 04:13:12, raymes wrote: > As we discussed I think the test might have to be changed to guarantee this will > finish before it passes. Done, thank you for bringing this up because I wouldn't have noticed :)
lgtm with some small suggestions https://codereview.chromium.org/2936003003/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/site_settings_handler.cc (right): https://codereview.chromium.org/2936003003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/site_settings_handler.cc:598: auto raw_site_exception = base::MakeUnique<base::DictionaryValue>(); On 2017/06/20 08:25:54, Patti Lor wrote: > On 2017/06/19 04:13:12, raymes wrote: > > nit: what does raw mean here? > > This method is supposed to return a RawSiteException (see > https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/site_s...). > expandSiteException (called in <site-details>'s onOriginChanged() is meant to > convert that to a SiteException, similar to the original code. Thanks for explaining :) https://codereview.chromium.org/2936003003/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/site_settings_handler.cc (right): https://codereview.chromium.org/2936003003/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/site_settings_handler.cc:128: // These are not possible. Hmm, I think these may actually be possible. WHITELIST may not happen in these cases by chance, but supervised could. I think supervised should use the same string as the policy provider. Maybe we just return user-defined for whitelist for now, but if these values are ever returned, the setting shouldn't be modifiable. https://codereview.chromium.org/2936003003/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/site_settings_handler.cc:142: } You've done a good job of this logic considering that it's virtually impossible to get right! I might suggest that we reverse the order of checks though. For example, it's possible that KILL_SWITCH takes precedence over POLICY. So we might just want to move this switch statement first. If UNSPECIFIED is returned then fall back to the content settings source. This won't be completely correct because there are cases right now where we return UNSPECIFIED which aren't due to falling back to the HostContentSettingsMap. But I think it will be slightly more correct. We should definitely prioritize fixing up these decision reasons after v0 :) https://codereview.chromium.org/2936003003/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/site_settings_handler.cc:603: raw_site_exception->SetString(site_settings::kEmbeddingOrigin, ""); nit: I'm not sure if it matters, but we could just keep the embedding origin the same as the origin.
Generally looks good! Just a couple of test fixes https://codereview.chromium.org/2936003003/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/site_details.js (right): https://codereview.chromium.org/2936003003/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_details.js:74: .forEach(function(element) { Nit: you probably want to tell closure that |element| is a specific polymer element type. I think the way to do that here is .forEach(function(/** @type {SiteDetailsPermissionElement} */ element) { https://codereview.chromium.org/2936003003/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/settings/site_details_permission_tests.js (right): https://codereview.chromium.org/2936003003/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/site_details_permission_tests.js:102: // Flip the permission and validate that prefs stay in sync. As in old version of the test, you need promises here to ensure that the test waits for the asynchronous code in validatePermissionFlipWorks() to run. The validate function isn't re-entrant, either, so you'll need to keep the linear structure of the old version of the test: return validatePermissionFlipWorks(...).then(function() { return validatePermissionFlipWorks(...); }).then(function() { return validatePermissionFlipWorks(...); }) https://codereview.chromium.org/2936003003/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/site_details_permission_tests.js:125: // Flip the permission and validate that prefs stay in sync. As above, use promises here too.
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: This issue passed the CQ dry run.
Hey Raymes, can you take another look at the ConvertContentSettingSourceToString() logic in chrome/browser/ui/webui/settings/site_settings_handler.cc? Thanks both for the reviews! https://codereview.chromium.org/2936003003/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/site_settings_handler.cc (right): https://codereview.chromium.org/2936003003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/site_settings_handler.cc:598: auto raw_site_exception = base::MakeUnique<base::DictionaryValue>(); On 2017/06/20 23:42:35, raymes wrote: > On 2017/06/20 08:25:54, Patti Lor wrote: > > On 2017/06/19 04:13:12, raymes wrote: > > > nit: what does raw mean here? > > > > This method is supposed to return a RawSiteException (see > > > https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/site_s...). > > expandSiteException (called in <site-details>'s onOriginChanged() is meant to > > convert that to a SiteException, similar to the original code. > > Thanks for explaining :) Acknowledged :D https://codereview.chromium.org/2936003003/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/site_details.js (right): https://codereview.chromium.org/2936003003/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_details.js:74: .forEach(function(element) { On 2017/06/21 01:49:56, tsergeant wrote: > Nit: you probably want to tell closure that |element| is a specific polymer > element type. > > I think the way to do that here is > > .forEach(function(/** @type {SiteDetailsPermissionElement} */ element) { As discussed offline, we will skip this? Summary of why (please correct me if I'm wrong Tim): Making this change verbatim makes closure complain that "SiteDetailsPermissionElement" is not of type Element, number, or NodeList<Element>, even though it *should* be inheriting from Element. Trying to cast the entire list as a NodeList<SiteDetailsPermissionElement> is fine, but inspecting |element| afterwards still says it's of type "element" where it should be SiteDetailsPermissionElement. We aren't sure why calling element.expandSiteException() on line 78 is fine with closure if it doesn't know what type it is. https://codereview.chromium.org/2936003003/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/site_settings_handler.cc (right): https://codereview.chromium.org/2936003003/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/site_settings_handler.cc:128: // These are not possible. On 2017/06/20 23:42:36, raymes wrote: > Hmm, I think these may actually be possible. WHITELIST may not happen in these > cases by chance, but supervised could. I think supervised should use the same > string as the policy provider. Maybe we just return user-defined for whitelist > for now, but if these values are ever returned, the setting shouldn't be > modifiable. Ah, OK, thanks for clarifying. I did some investigating for supervised (made a supervised account and tried to use it) and could only find a way to whitelist/blacklist specific origins, not change the content settings of anything; but I'll defer to you on this. Fixed. https://codereview.chromium.org/2936003003/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/site_settings_handler.cc:142: } On 2017/06/20 23:42:36, raymes wrote: > You've done a good job of this logic considering that it's virtually impossible > to get right! > > I might suggest that we reverse the order of checks though. For example, it's > possible that KILL_SWITCH takes precedence over POLICY. > > So we might just want to move this switch statement first. If UNSPECIFIED is > returned then fall back to the content settings source. This won't be completely > correct because there are cases right now where we return UNSPECIFIED which > aren't due to falling back to the HostContentSettingsMap. But I think it will be > slightly more correct. > > We should definitely prioritize fixing up these decision reasons after v0 :) Ah, thanks for pointing that out. Fixed. I added a comment to document one of the incorrect cases (the latest patchset will make embargo look like it takes precedence over a setting the user has explicitly overridden embargo for). https://codereview.chromium.org/2936003003/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/site_settings_handler.cc:603: raw_site_exception->SetString(site_settings::kEmbeddingOrigin, ""); On 2017/06/20 23:42:36, raymes wrote: > nit: I'm not sure if it matters, but we could just keep the embedding origin the > same as the origin. Done. https://codereview.chromium.org/2936003003/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/settings/site_details_permission_tests.js (right): https://codereview.chromium.org/2936003003/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/site_details_permission_tests.js:102: // Flip the permission and validate that prefs stay in sync. On 2017/06/21 01:49:56, tsergeant wrote: > As in old version of the test, you need promises here to ensure that the test > waits for the asynchronous code in validatePermissionFlipWorks() to run. > > The validate function isn't re-entrant, either, so you'll need to keep the > linear structure of the old version of the test: > > return validatePermissionFlipWorks(...).then(function() { > return validatePermissionFlipWorks(...); > }).then(function() { > return validatePermissionFlipWorks(...); > }) Thanks Tim! Fixed. https://codereview.chromium.org/2936003003/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/site_details_permission_tests.js:125: // Flip the permission and validate that prefs stay in sync. On 2017/06/21 01:49:56, tsergeant wrote: > As above, use promises here too. Done.
lgtm! https://codereview.chromium.org/2936003003/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/site_details.js (right): https://codereview.chromium.org/2936003003/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_details.js:74: .forEach(function(element) { On 2017/06/21 06:36:37, Patti Lor wrote: > On 2017/06/21 01:49:56, tsergeant wrote: > > Nit: you probably want to tell closure that |element| is a specific polymer > > element type. > > > > I think the way to do that here is > > > > .forEach(function(/** @type {SiteDetailsPermissionElement} */ element) { > > As discussed offline, we will skip this? > > Summary of why (please correct me if I'm wrong Tim): Making this change verbatim > makes closure complain that "SiteDetailsPermissionElement" is not of type > Element, number, or NodeList<Element>, even though it *should* be inheriting > from Element. Trying to cast the entire list as a > NodeList<SiteDetailsPermissionElement> is fine, but inspecting |element| > afterwards still says it's of type "element" where it should be > SiteDetailsPermissionElement. We aren't sure why calling > element.expandSiteException() on line 78 is fine with closure if it doesn't know > what type it is. Slight correction on the last sentence: Our global Closure configuration means that Closure is fine with calling functions that don't exist on an object, so long as it has seen that function defined somewhere else previously.
https://codereview.chromium.org/2936003003/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/site_settings_handler.cc (right): https://codereview.chromium.org/2936003003/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/site_settings_handler.cc:128: // These are not possible. On 2017/06/21 06:36:37, Patti Lor wrote: > On 2017/06/20 23:42:36, raymes wrote: > > Hmm, I think these may actually be possible. WHITELIST may not happen in these > > cases by chance, but supervised could. I think supervised should use the same > > string as the policy provider. Maybe we just return user-defined for whitelist > > for now, but if these values are ever returned, the setting shouldn't be > > modifiable. > > Ah, OK, thanks for clarifying. I did some investigating for supervised (made a > supervised account and tried to use it) and could only find a way to > whitelist/blacklist specific origins, not change the content settings of > anything; but I'll defer to you on this. Fixed. I think it's possible for geolocation/camera based on content_settings_supervised_provider.cc. With that said, I haven't actually tested it. https://codereview.chromium.org/2936003003/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/site_settings_handler.cc (right): https://codereview.chromium.org/2936003003/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/site_settings_handler.cc:124: return site_settings::kPreferencesSource; We should probably have a separate string for this too. https://codereview.chromium.org/2936003003/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/site_settings_handler.cc:160: } Ah you're right about embargo! This is so confusing. Maybe it's best if we just have the checks in order of what we want returned. e.g. if (permission_status_source == KILL_SWITCH) return kill switch; if (whitelist) return user; if (content_setting_source == policy || supervised) return policy; if (content settings_source == extension) return extension; if (content_setting_source == user) if (embargo) return embargo else return user NOTREACHED() ? We lose the benefit of the switch statements but at least the we might be able to read the code later :P wdyt?
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_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2936003003/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/site_details.js (right): https://codereview.chromium.org/2936003003/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_details.js:74: .forEach(function(element) { On 2017/06/21 06:52:10, tsergeant wrote: > On 2017/06/21 06:36:37, Patti Lor wrote: > > On 2017/06/21 01:49:56, tsergeant wrote: > > > Nit: you probably want to tell closure that |element| is a specific polymer > > > element type. > > > > > > I think the way to do that here is > > > > > > .forEach(function(/** @type {SiteDetailsPermissionElement} */ element) { > > > > As discussed offline, we will skip this? > > > > Summary of why (please correct me if I'm wrong Tim): Making this change > verbatim > > makes closure complain that "SiteDetailsPermissionElement" is not of type > > Element, number, or NodeList<Element>, even though it *should* be inheriting > > from Element. Trying to cast the entire list as a > > NodeList<SiteDetailsPermissionElement> is fine, but inspecting |element| > > afterwards still says it's of type "element" where it should be > > SiteDetailsPermissionElement. We aren't sure why calling > > element.expandSiteException() on line 78 is fine with closure if it doesn't > know > > what type it is. > > Slight correction on the last sentence: Our global Closure configuration means > that Closure is fine with calling functions that don't exist on an object, so > long as it has seen that function defined somewhere else previously. Thanks Tim :) https://codereview.chromium.org/2936003003/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/site_settings_handler.cc (right): https://codereview.chromium.org/2936003003/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/site_settings_handler.cc:128: // These are not possible. On 2017/06/22 00:07:03, raymes wrote: > On 2017/06/21 06:36:37, Patti Lor wrote: > > On 2017/06/20 23:42:36, raymes wrote: > > > Hmm, I think these may actually be possible. WHITELIST may not happen in > these > > > cases by chance, but supervised could. I think supervised should use the > same > > > string as the policy provider. Maybe we just return user-defined for > whitelist > > > for now, but if these values are ever returned, the setting shouldn't be > > > modifiable. > > > > Ah, OK, thanks for clarifying. I did some investigating for supervised (made a > > supervised account and tried to use it) and could only find a way to > > whitelist/blacklist specific origins, not change the content settings of > > anything; but I'll defer to you on this. Fixed. > > I think it's possible for geolocation/camera based on > content_settings_supervised_provider.cc. With that said, I haven't actually > tested it. Oh ok, interesting. Thanks for the FYI. https://codereview.chromium.org/2936003003/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/site_settings_handler.cc (right): https://codereview.chromium.org/2936003003/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/site_settings_handler.cc:124: return site_settings::kPreferencesSource; On 2017/06/22 00:07:03, raymes wrote: > We should probably have a separate string for this too. Ah, thanks - I ended up just putting in a single TODO at the top to do all the plumbing since there are a bunch of new sources introduced here. https://codereview.chromium.org/2936003003/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/site_settings_handler.cc:160: } On 2017/06/22 00:07:03, raymes wrote: > Ah you're right about embargo! This is so confusing. > > Maybe it's best if we just have the checks in order of what we want returned. > > e.g. > > if (permission_status_source == KILL_SWITCH) > return kill switch; > > if (whitelist) > return user; > > if (content_setting_source == policy || supervised) > return policy; > > if (content settings_source == extension) > return extension; > > if (content_setting_source == user) > if (embargo) return embargo > else return user > > NOTREACHED() ? > > We lose the benefit of the switch statements but at least the we might be able > to read the code later :P wdyt? Agreed and done. Thanks Raymes! (The compiler complained about control reaching end of non-void function, so I had to replace the NOTREACHED() with the "return user" part of your suggested code.)
lgtm, thanks! https://codereview.chromium.org/2936003003/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/site_settings_handler.cc (right): https://codereview.chromium.org/2936003003/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/site_settings_handler.cc:124: content_settings_source == content_settings::SETTING_SOURCE_SUPERVISED) nit: add {} https://codereview.chromium.org/2936003003/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/site_settings_handler.cc:130: if (content_settings_source == content_settings::SETTING_SOURCE_NONE || is this one ever returned? Should we DCHECK it instead? https://codereview.chromium.org/2936003003/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/site_settings_handler.cc:141: return site_settings::kPreferencesSource; // Source #4, #6, #7, #8. I think it's better to keep the NOTREACHED because it means that if someone adds something, they will find out about it quickly. It's ok to have NOTREACHED and return a value as well though :)
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: This issue passed the CQ dry run.
patricialor@chromium.org changed reviewers: + dschuyler@chromium.org
Thanks Raymes! dschuyler@, PTAL? https://codereview.chromium.org/2936003003/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/site_settings_handler.cc (right): https://codereview.chromium.org/2936003003/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/site_settings_handler.cc:124: content_settings_source == content_settings::SETTING_SOURCE_SUPERVISED) On 2017/06/22 06:50:35, raymes wrote: > nit: add {} Done. https://codereview.chromium.org/2936003003/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/site_settings_handler.cc:130: if (content_settings_source == content_settings::SETTING_SOURCE_NONE || On 2017/06/22 06:50:35, raymes wrote: > is this one ever returned? Should we DCHECK it instead? I'm not sure - I saw it was set in HCSM::GetWebsiteSettingInternal(), but I think that code is hardly ever reached and I haven't really seen it happen, so I'll go ahead and DCHECK it. Thanks! https://codereview.chromium.org/2936003003/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/site_settings_handler.cc:141: return site_settings::kPreferencesSource; // Source #4, #6, #7, #8. On 2017/06/22 06:50:35, raymes wrote: > I think it's better to keep the NOTREACHED because it means that if someone adds > something, they will find out about it quickly. It's ok to have NOTREACHED and > return a value as well though :) Done.
On 2017/06/23 00:41:34, Patti Lor wrote: > Thanks Raymes! dschuyler@, PTAL? > > https://codereview.chromium.org/2936003003/diff/120001/chrome/browser/ui/webu... > File chrome/browser/ui/webui/settings/site_settings_handler.cc (right): > > https://codereview.chromium.org/2936003003/diff/120001/chrome/browser/ui/webu... > chrome/browser/ui/webui/settings/site_settings_handler.cc:124: > content_settings_source == content_settings::SETTING_SOURCE_SUPERVISED) > On 2017/06/22 06:50:35, raymes wrote: > > nit: add {} > > Done. > > https://codereview.chromium.org/2936003003/diff/120001/chrome/browser/ui/webu... > chrome/browser/ui/webui/settings/site_settings_handler.cc:130: if > (content_settings_source == content_settings::SETTING_SOURCE_NONE || > On 2017/06/22 06:50:35, raymes wrote: > > is this one ever returned? Should we DCHECK it instead? > > I'm not sure - I saw it was set in HCSM::GetWebsiteSettingInternal(), but I > think that code is hardly ever reached and I haven't really seen it happen, so > I'll go ahead and DCHECK it. Thanks! > > https://codereview.chromium.org/2936003003/diff/120001/chrome/browser/ui/webu... > chrome/browser/ui/webui/settings/site_settings_handler.cc:141: return > site_settings::kPreferencesSource; // Source #4, #6, #7, #8. > On 2017/06/22 06:50:35, raymes wrote: > > I think it's better to keep the NOTREACHED because it means that if someone > adds > > something, they will find out about it quickly. It's ok to have NOTREACHED and > > return a value as well though :) > > Done. I'm hoping to look this over on Monday (just wanted to let you know).
https://codereview.chromium.org/2936003003/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_details.html (right): https://codereview.chromium.org/2936003003/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_details.html:44: <!-- TODO(patricialor): This should show the origin's display name. --> The CL at https://codereview.chromium.org/2919853002/ explores changing how the displayName is generated. I'm mentioning it as an FYI and to invite you to make a new CL that does something similar (and I'll delete my CL) or maybe I can help follow-up on this TODO by porting that CL to this new code. https://codereview.chromium.org/2936003003/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_details.js (right): https://codereview.chromium.org/2936003003/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_details.js:80: }.bind(this)); IIUC, this will make a couple dozen (or so) queries. Site settings in general has made too many queries in the past - I'm not saying it was great. But, can this be condensed down to a single query? I.e. maybe request a list of (category, origin) pairs and get a list of results. https://codereview.chromium.org/2936003003/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_details.js:128: }); Again (or similarly), WDYT of making a single call to a new resetAllPermissionForOrigin rather than looping a call to resetPermission? https://codereview.chromium.org/2936003003/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/site_settings_handler.cc (right): https://codereview.chromium.org/2936003003/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/site_settings_handler.cc:138: return site_settings::kPreferencesSource; // Source #5. Thanks for the comments (lines 105 to 114), very helpful. I'm wondering (based on that comment) if this #5 return is being prioritized above #4 return at line 140? I'm not sure if I'm misunderstanding or if there's an error in the comments or an error in the code. Please let me know what you think :) (which could be pointing out my misunderstanding).
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #9 (id:160001) has been deleted
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...
Thanks for your review dschuyler, PTAL! https://codereview.chromium.org/2936003003/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_details.html (right): https://codereview.chromium.org/2936003003/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_details.html:44: <!-- TODO(patricialor): This should show the origin's display name. --> On 2017/06/26 22:07:05, dschuyler wrote: > The CL at https://codereview.chromium.org/2919853002/ > explores changing how the displayName is generated. I'm mentioning it as an FYI > and to invite you to make a new CL that does something similar (and I'll delete > my CL) or maybe I can help follow-up on this TODO by porting that CL to this new > code. Thank you for the FYI! I think your CL doesn't conflict with this one very much, since Site Details won't be supporting permissions set on embedded origins in the near future (it will depend on what "All Sites" ends up looking like), so I think whoever's CL lands last can just rebase. The |displayName| on the other hand will probably be needed, but I think we should explore that in a separate change. I'm happy to explore that in a follow up to this one, and leave this TODO here, but if you have other plans in mind, let me know. https://codereview.chromium.org/2936003003/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_details.js (right): https://codereview.chromium.org/2936003003/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_details.js:80: }.bind(this)); On 2017/06/26 22:07:05, dschuyler wrote: > IIUC, this will make a couple dozen (or so) queries. Site settings in general > has made too many queries in the past - I'm not saying it was great. But, can > this be condensed down to a single query? I.e. maybe request a list of > (category, origin) pairs and get a list of results. Done! getCategoryPermissionForOrigin -> getOriginPermissions. https://codereview.chromium.org/2936003003/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_details.js:128: }); On 2017/06/26 22:07:05, dschuyler wrote: > Again (or similarly), WDYT of making a single call to a new > resetAllPermissionForOrigin rather than looping a call to resetPermission? Good idea! I had a quick look today, but I'll send you something in a separate CL when it's ready? https://codereview.chromium.org/2936003003/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/site_settings_handler.cc (right): https://codereview.chromium.org/2936003003/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/site_settings_handler.cc:138: return site_settings::kPreferencesSource; // Source #5. On 2017/06/26 22:07:05, dschuyler wrote: > Thanks for the comments (lines 105 to 114), very helpful. I'm wondering (based > on that comment) if this #5 return is being prioritized above #4 return at line > 140? I'm not sure if I'm misunderstanding or if there's an error in the comments > or an error in the code. Please let me know what you think :) (which could be > pointing out my misunderstanding). Ah, yeah - I was also confused by this before, but I think it works as-is. It's an implementation detail you kinda have to know, though, so I've added a comment to explain, thanks for pointing it out. Let me know if the comment makes sense to you? Raymes and I have both agreed that it should be a priority to fix this after v0 of Site Details is launched, and I've got one half of the fix up at https://codereview.chromium.org/2945243002/, so hopefully this logic doesn't need to stay around too long.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
If it's really closure compiling, LGTM. https://codereview.chromium.org/2936003003/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_details.js (right): https://codereview.chromium.org/2936003003/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_details.js:128: }); On 2017/06/28 09:12:52, Patti Lor wrote: > On 2017/06/26 22:07:05, dschuyler wrote: > > Again (or similarly), WDYT of making a single call to a new > > resetAllPermissionForOrigin rather than looping a call to resetPermission? > > Good idea! I had a quick look today, but I'll send you something in a separate > CL when it's ready? SG https://codereview.chromium.org/2936003003/diff/180001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_details.js (right): https://codereview.chromium.org/2936003003/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_details.js:73: this.root.querySelectorAll('site-details-permission'); I thought a type case like: /** @type ... */ (foo) needed parenthesis around foo, i.e. (this.root.querySelectorAll('site-details-permission')); Is this file truly closure compiling (i.e. am I just wrong about the above)? https://codereview.chromium.org/2936003003/diff/180001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js (right): https://codereview.chromium.org/2936003003/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js:148: * @param {Array<string>} contentTypes A list of categories to retrieve Let's insist that Array is not null: @param {!Array<string>}
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...
patricialor@chromium.org changed reviewers: + calamity@chromium.org
Thanks dschuyler! calamity, PTAL for owners review of: chrome/browser/ui/webui/site_settings_helper.h chrome/browser/ui/webui/site_settings_helper.cc https://codereview.chromium.org/2936003003/diff/180001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_details.js (right): https://codereview.chromium.org/2936003003/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_details.js:73: this.root.querySelectorAll('site-details-permission'); On 2017/06/28 23:55:52, dschuyler wrote: > I thought a type case like: /** @type ... */ (foo) > needed parenthesis around foo, i.e. > (this.root.querySelectorAll('site-details-permission')); > > Is this file truly closure compiling (i.e. am I just wrong about the above)? Oops, yes, you're totally right. Parens added and SiteDetailsPermission -> SiteDetailsPermissionElement, plus the type for |categoryList| below fixed as well. Thanks! https://codereview.chromium.org/2936003003/diff/180001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js (right): https://codereview.chromium.org/2936003003/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js:148: * @param {Array<string>} contentTypes A list of categories to retrieve On 2017/06/28 23:55:52, dschuyler wrote: > Let's insist that Array is not null: > @param {!Array<string>} Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2936003003/diff/220001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/site_settings_handler.cc (right): https://codereview.chromium.org/2936003003/diff/220001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/site_settings_handler.cc:541: void SiteSettingsHandler::HandleGetOriginPermissions( The real version of this doesn't seem to ever get called in tests. It would be good to look into writing an integration test that has javascript calls that can change these values on the backend and ensure that this API call returns the correct data. This can be done in a follow-up. https://codereview.chromium.org/2936003003/diff/220001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/site_settings_handler.cc:560: static_cast<ContentSettingsType>(static_cast<int>( These casts are unnecessary. https://codereview.chromium.org/2936003003/diff/220001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/site_settings_handler.cc:563: std::string source_string = ""; nit: Is this initialization necessary? https://codereview.chromium.org/2936003003/diff/220001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/site_settings_handler.h (right): https://codereview.chromium.org/2936003003/diff/220001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/site_settings_handler.h:94: // the same origin. nit: the same origin? Maybe 'an origin'?
https://codereview.chromium.org/2936003003/diff/220001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/site_settings_handler.cc (right): https://codereview.chromium.org/2936003003/diff/220001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/site_settings_handler.cc:541: void SiteSettingsHandler::HandleGetOriginPermissions( On 2017/06/29 05:31:26, calamity wrote: > The real version of this doesn't seem to ever get called in tests. It would be > good to look into writing an integration test that has javascript calls that can > change these values on the backend and ensure that this API call returns the > correct data. > > This can be done in a follow-up. I mentioned this to calamity@ already, but I think there's very little precedent for C++ <-> JS integration tests in MD Settings -- AFAIK, the accepted way to test C++ handlers is with a unittest (eg, site_settings_handler_unittest.cc).
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: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
Description was changed from ========== MD Settings: Set all content setting values in the <site-details> object. r476514 changed the "Site Details" page to show all content settings instead of the non-default ones. However, the back-end code supporting this UI still only retrieved non-default settings. This CL fixes this problem to retrieve the correct values for all content settings displayed, consistent with those shown in the Page Info bubble. Note that the "Site Details" UI will now only show the correct content setting value for content settings that are non-default (previously, they were all incorrect). A follow-up CL will be required to enable the UI to show default settings, such as "Ask (default)", "Allow (default)" or "Block (default)" values. Insecure (http) origins will also show that certain permissions have been blocked, unlike the Page Info bubble. BUG=656758 TEST=With #enable-site-details turned on, navigate to https://permission.site. Allow and deny a few permissions and verify these changes show up in the Page Info bubble. Then click "Site settings" in the Page Info bubble, and check the non-default 'Allow'- / 'Block'- ed content settings are also shown in the "Site Details" page for https://permission.site and match the non-default values shown in the Page Info bubble. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Set all content setting values in the <site-details> object. r476514 changed the "Site Details" page to show all content settings instead of the non-default ones. However, the back-end code supporting this UI still only retrieved non-default settings. This CL fixes this problem to retrieve the correct values for all content settings displayed, consistent with those shown in the Page Info bubble. Note that the "Site Details" UI will now only show the correct content setting value for content settings that are non-default (previously, they were all incorrect). A follow-up CL will be required to enable the UI to show default settings, such as "Ask (default)", "Allow (default)" or "Block (default)" values. Insecure (http) origins will also show that certain permissions have been blocked, unlike the Page Info bubble. See https://codereview.chromium.org/2946393002/ for C++ tests. BUG=656758 TEST=With #enable-site-details turned on, navigate to https://permission.site. Allow and deny a few permissions and verify these changes show up in the Page Info bubble. Then click "Site settings" in the Page Info bubble, and check the non-default 'Allow'- / 'Block'- ed content settings are also shown in the "Site Details" page for https://permission.site and match the non-default values shown in the Page Info bubble. 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: This issue passed the CQ dry run.
https://codereview.chromium.org/2936003003/diff/220001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/site_settings_handler.cc (right): https://codereview.chromium.org/2936003003/diff/220001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/site_settings_handler.cc:541: void SiteSettingsHandler::HandleGetOriginPermissions( On 2017/06/29 07:14:48, tsergeant wrote: > On 2017/06/29 05:31:26, calamity wrote: > > The real version of this doesn't seem to ever get called in tests. It would be > > good to look into writing an integration test that has javascript calls that > can > > change these values on the backend and ensure that this API call returns the > > correct data. > > > > This can be done in a follow-up. > > I mentioned this to calamity@ already, but I think there's very little precedent > for C++ <-> JS integration tests in MD Settings -- AFAIK, the accepted way to > test C++ handlers is with a unittest (eg, site_settings_handler_unittest.cc). Thanks Tim! And yeah I have a follow-up test CL in https://codereview.chromium.org/2946393002/, updated this CL's description to link to it. https://codereview.chromium.org/2936003003/diff/220001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/site_settings_handler.cc:560: static_cast<ContentSettingsType>(static_cast<int>( On 2017/06/29 05:31:26, calamity wrote: > These casts are unnecessary. You're right, thanks! Fixed them everywhere (in this file). https://codereview.chromium.org/2936003003/diff/220001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/site_settings_handler.cc:563: std::string source_string = ""; On 2017/06/29 05:31:26, calamity wrote: > nit: Is this initialization necessary? Nope, fixed! Thanks. https://codereview.chromium.org/2936003003/diff/220001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/site_settings_handler.h (right): https://codereview.chromium.org/2936003003/diff/220001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/site_settings_handler.h:94: // the same origin. On 2017/06/29 05:31:26, calamity wrote: > nit: the same origin? Maybe 'an origin'? Done.
lgtm
The CQ bit was checked by patricialor@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tsergeant@chromium.org, raymes@chromium.org, dschuyler@chromium.org Link to the patchset: https://codereview.chromium.org/2936003003/#ps240001 (title: "Review comments.")
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": 240001, "attempt_start_ts": 1498792423989240,
"parent_rev": "c0f1967e5ded7b35fb1a065f97e94502ad370843", "commit_rev":
"f3b6e00fe2826406218b15bd2f317cfebe835696"}
Message was sent while issue was closed.
Description was changed from ========== MD Settings: Set all content setting values in the <site-details> object. r476514 changed the "Site Details" page to show all content settings instead of the non-default ones. However, the back-end code supporting this UI still only retrieved non-default settings. This CL fixes this problem to retrieve the correct values for all content settings displayed, consistent with those shown in the Page Info bubble. Note that the "Site Details" UI will now only show the correct content setting value for content settings that are non-default (previously, they were all incorrect). A follow-up CL will be required to enable the UI to show default settings, such as "Ask (default)", "Allow (default)" or "Block (default)" values. Insecure (http) origins will also show that certain permissions have been blocked, unlike the Page Info bubble. See https://codereview.chromium.org/2946393002/ for C++ tests. BUG=656758 TEST=With #enable-site-details turned on, navigate to https://permission.site. Allow and deny a few permissions and verify these changes show up in the Page Info bubble. Then click "Site settings" in the Page Info bubble, and check the non-default 'Allow'- / 'Block'- ed content settings are also shown in the "Site Details" page for https://permission.site and match the non-default values shown in the Page Info bubble. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Set all content setting values in the <site-details> object. r476514 changed the "Site Details" page to show all content settings instead of the non-default ones. However, the back-end code supporting this UI still only retrieved non-default settings. This CL fixes this problem to retrieve the correct values for all content settings displayed, consistent with those shown in the Page Info bubble. Note that the "Site Details" UI will now only show the correct content setting value for content settings that are non-default (previously, they were all incorrect). A follow-up CL will be required to enable the UI to show default settings, such as "Ask (default)", "Allow (default)" or "Block (default)" values. Insecure (http) origins will also show that certain permissions have been blocked, unlike the Page Info bubble. See https://codereview.chromium.org/2946393002/ for C++ tests. BUG=656758 TEST=With #enable-site-details turned on, navigate to https://permission.site. Allow and deny a few permissions and verify these changes show up in the Page Info bubble. Then click "Site settings" in the Page Info bubble, and check the non-default 'Allow'- / 'Block'- ed content settings are also shown in the "Site Details" page for https://permission.site and match the non-default values shown in the Page Info bubble. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2936003003 Cr-Commit-Position: refs/heads/master@{#483614} Committed: https://chromium.googlesource.com/chromium/src/+/f3b6e00fe2826406218b15bd2f31... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:240001) as https://chromium.googlesource.com/chromium/src/+/f3b6e00fe2826406218b15bd2f31... |
