Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2468363005:
[MD settings] show blocked sites even when category is blocked (Closed)
|
Created:
4 years, 1 month ago by dschuyler Modified:
4 years, 1 month ago Reviewers:
dpapad CC:
arv+watch_chromium.org, chromium-reviews, dbeam+watch-settings_chromium.org, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, stevenjb+watch-md-settings_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[MD settings] show blocked sites even when category is blocked
This CL removes the logic that sometimes hides the blocked or session
only lists, so that they are now always shown. This is a desired UI
change from Alan.
BUG=662491, 662493
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/dbefa1097f65603c4832c44b1a12cb419016d1e5
Cr-Commit-Position: refs/heads/master@{#431449}
Patch Set 1 #Patch Set 2 : updated test #Patch Set 3 : fixes #
Total comments: 13
Patch Set 4 : review changes #
Total comments: 2
Patch Set 5 : closure enum types on content settings #Patch Set 6 : unit test changes #
Total comments: 2
Messages
Total messages: 47 (34 generated)
Description was changed from ========== [MD settings] show blocked sites even when category is blocked This CL removes the logic that sometimes hides the blocked or session only lists, so that they are now always shown. This is a desired UI change from Alan. BUG=662491 ========== to ========== [MD settings] show blocked sites even when category is blocked This CL removes the logic that sometimes hides the blocked or session only lists, so that they are now always shown. This is a desired UI change from Alan. BUG=662491 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...
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 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
dschuyler@chromium.org changed reviewers: + dpapad@chromium.org
https://codereview.chromium.org/2468363005/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/add_site_dialog.js (right): https://codereview.chromium.org/2468363005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/add_site_dialog.js:19: allowException: Boolean, This was used when calling browserProxy.setCategoryPermissionForOrigin before. I don't see any uses of it now, is it still necessary? https://codereview.chromium.org/2468363005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/add_site_dialog.js:21: contentSetting: String, Can you add a comment explaining what does this member var hold? It is not clear without any context. https://codereview.chromium.org/2468363005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/add_site_dialog.js:81: pattern, pattern, this.category, this.contentSetting, Are we assuming that |contentSettings| has been populated by a parent? Can we add an assertion before calling setCategoryPermissionForOrigin? https://codereview.chromium.org/2468363005/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/site_list.js (right): https://codereview.chromium.org/2468363005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_list.js:182: this.$.category.hidden = Could this be done with a data binding that depends on |categorySubtype| and |category| instead? https://codereview.chromium.org/2468363005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_list.js:214: var dialog = document.createElement('add-site-dialog'); Nit (optional): This function could be simplified if the dialog was declared in the HTML. We have several such examples, that go as follows: onTap_: function() { this.dialogModel_ = contentSetting; // The model feeding the dialog. this.showDialog_ = true; // this is bound do a dom-if with restamp. } // This listener is registered via HTML on-close="..." onDialogClosed_: function() { this.showDialog_ = false; this.dialogModel_ = null; } https://codereview.chromium.org/2468363005/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/site_list_tests.js (right): https://codereview.chromium.org/2468363005/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/site_list_tests.js:468: // default is enabled. Seems that there is an extra "the", not sure.
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/2468363005/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/add_site_dialog.js (right): https://codereview.chromium.org/2468363005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/add_site_dialog.js:19: allowException: Boolean, On 2016/11/07 21:59:35, dpapad wrote: > This was used when calling browserProxy.setCategoryPermissionForOrigin before. I > don't see any uses of it now, is it still necessary? Done. https://codereview.chromium.org/2468363005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/add_site_dialog.js:21: contentSetting: String, On 2016/11/07 21:59:35, dpapad wrote: > Can you add a comment explaining what does this member var hold? It is not clear > without any context. Done. https://codereview.chromium.org/2468363005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/add_site_dialog.js:81: pattern, pattern, this.category, this.contentSetting, On 2016/11/07 21:59:35, dpapad wrote: > Are we assuming that |contentSettings| has been populated by a parent? Can we > add an assertion before calling setCategoryPermissionForOrigin? I added asserts in attached so that they would be checked early and have lower overhead (vs checking in member functions other than attached). https://codereview.chromium.org/2468363005/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/site_list.js (right): https://codereview.chromium.org/2468363005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_list.js:182: this.$.category.hidden = On 2016/11/07 21:59:35, dpapad wrote: > Could this be done with a data binding that depends on |categorySubtype| and > |category| instead? There is a binding style handler setup on line 130. Though that's now observing more bindings than necessary, I'll trim that down. https://codereview.chromium.org/2468363005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_list.js:214: var dialog = document.createElement('add-site-dialog'); On 2016/11/07 21:59:35, dpapad wrote: > Nit (optional): This function could be simplified if the dialog was declared in > the HTML. We have several such examples, that go as follows: > > onTap_: function() { > this.dialogModel_ = contentSetting; // The model feeding the dialog. > this.showDialog_ = true; // this is bound do a dom-if with restamp. > } > > // This listener is registered via HTML on-close="..." > onDialogClosed_: function() { > this.showDialog_ = false; > this.dialogModel_ = null; > } Going by advice I got while visiting the Polymer folks: there's less overhead to create an element than adding template dom-if. I think we should work out whether there is an impact with creating nodes vs. dom-if and convert them all one way or the other so that they are consistent. Creating an element in JS also has a small advantage that the closure compiler can type check the properties (which doesn't currently happen afaik in html). https://codereview.chromium.org/2468363005/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/site_list_tests.js (right): https://codereview.chromium.org/2468363005/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/site_list_tests.js:468: // default is enabled. On 2016/11/07 21:59:35, dpapad wrote: > Seems that there is an extra "the", not sure. Done.
LGTM https://codereview.chromium.org/2468363005/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/site_list.js (right): https://codereview.chromium.org/2468363005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_list.js:214: var dialog = document.createElement('add-site-dialog'); On 2016/11/07 at 23:54:04, dschuyler wrote: > On 2016/11/07 21:59:35, dpapad wrote: > > Nit (optional): This function could be simplified if the dialog was declared in > > the HTML. We have several such examples, that go as follows: > > > > onTap_: function() { > > this.dialogModel_ = contentSetting; // The model feeding the dialog. > > this.showDialog_ = true; // this is bound do a dom-if with restamp. > > } > > > > // This listener is registered via HTML on-close="..." > > onDialogClosed_: function() { > > this.showDialog_ = false; > > this.dialogModel_ = null; > > } > > Going by advice I got while visiting the Polymer folks: there's less > overhead to create an element than adding template dom-if. I think > we should work out whether there is an impact with creating nodes vs. > dom-if and convert them all one way or the other so that they are > consistent. Creating an element in JS also has a small advantage that > the closure compiler can type check the properties (which doesn't > currently happen afaik in html). SG. I would be positively surprised if the compiler's Polymer pass understands createElement() return type without casting it to anything. As it stands now, I just think it is using its lenient policy when a type is not available. Regarding converting all <dialog> declarations to either programmatic or declarative, I think it might be too disruptive. Declarative has a lot of advantages (data binding, listener registration), so it would be far from trivial to accommodate all cases with only programmatic DOM manipulation. https://codereview.chromium.org/2468363005/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/add_site_dialog.js (right): https://codereview.chromium.org/2468363005/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/add_site_dialog.js:20: contentSetting: String, As discussed, could this be typed as an enum instead of string? If that is too much of a change, seperate CL is fine.
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/2468363005/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/add_site_dialog.js (right): https://codereview.chromium.org/2468363005/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/add_site_dialog.js:20: contentSetting: String, On 2016/11/08 00:12:32, dpapad wrote: > As discussed, could this be typed as an enum instead of string? If that is too > much of a change, seperate CL is fine. Done.
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_...)
Description was changed from ========== [MD settings] show blocked sites even when category is blocked This CL removes the logic that sometimes hides the blocked or session only lists, so that they are now always shown. This is a desired UI change from Alan. BUG=662491 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD settings] show blocked sites even when category is blocked This CL removes the logic that sometimes hides the blocked or session only lists, so that they are now always shown. This is a desired UI change from Alan. BUG=662491, 662493 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...
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_...)
Patchset #6 (id:100001) has been deleted
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...
Changes in the unit tests because changing the enabled status no longer triggers a proxy call.
https://codereview.chromium.org/2468363005/diff/120001/chrome/test/data/webui... File chrome/test/data/webui/settings/site_list_tests.js (left): https://codereview.chromium.org/2468363005/diff/120001/chrome/test/data/webui... chrome/test/data/webui/settings/site_list_tests.js:440: assertFalse(testElement.$.category.hidden); Does it still makes sense to assert() for both categoryEnabled true/false values? If so, could we only remove the whenCalled() calls but preserve the assertions that the still is showing for both true/false?
https://codereview.chromium.org/2468363005/diff/120001/chrome/test/data/webui... File chrome/test/data/webui/settings/site_list_tests.js (left): https://codereview.chromium.org/2468363005/diff/120001/chrome/test/data/webui... chrome/test/data/webui/settings/site_list_tests.js:440: assertFalse(testElement.$.category.hidden); On 2016/11/10 22:22:45, dpapad wrote: > Does it still makes sense to assert() for both categoryEnabled true/false > values? If so, could we only remove the whenCalled() calls but preserve the > assertions that the still is showing for both true/false? I think it made sense to have the prior testing when those two unrelated things were linked together. It's something that would be easy to break because the linking wouldn't be obvious to a newcomer. When there is no connection between them I think it's less helpful to test the (non-) relation. Creating a new relation between those pieces is as unlikely as creating a relation between any other boolean and showing the category. iiuc the question is about doing something like this: assertFalse(testElement.$.category.hidden); testElement.categoryEnabled = false; Polymer.dom.flush(); assertFalse(testElement.$.category.hidden); I can add that. If that's desired, please let me know. My opinion is that it's not needed (doesn't hurt, just not needed).
> I think it made sense to have the prior testing when those two unrelated > things were linked together. It's something that would be easy to break > because the linking wouldn't be obvious to a newcomer. > > When there is no connection between them I think it's less helpful to > test the (non-) relation. Creating a new relation between those pieces is > as unlikely as creating a relation between any other boolean and showing > the category. > > iiuc the question is about doing something like this: > > assertFalse(testElement.$.category.hidden); > testElement.categoryEnabled = false; > Polymer.dom.flush(); > assertFalse(testElement.$.category.hidden); > > I can add that. If that's desired, please let me know. My opinion is > that it's not needed (doesn't hurt, just not needed). Thanks for the explanation. Yes I was asking for what you depict at the snippet example, but if it does not make sense anymore (non-related states), then it does not give us coverage of anything useful. The analogy I can think of is testing that an event is not fired when X happens.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by dschuyler@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org Link to the patchset: https://codereview.chromium.org/2468363005/#ps120001 (title: "unit test changes")
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] show blocked sites even when category is blocked This CL removes the logic that sometimes hides the blocked or session only lists, so that they are now always shown. This is a desired UI change from Alan. BUG=662491, 662493 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD settings] show blocked sites even when category is blocked This CL removes the logic that sometimes hides the blocked or session only lists, so that they are now always shown. This is a desired UI change from Alan. BUG=662491, 662493 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== [MD settings] show blocked sites even when category is blocked This CL removes the logic that sometimes hides the blocked or session only lists, so that they are now always shown. This is a desired UI change from Alan. BUG=662491, 662493 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD settings] show blocked sites even when category is blocked This CL removes the logic that sometimes hides the blocked or session only lists, so that they are now always shown. This is a desired UI change from Alan. BUG=662491, 662493 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/dbefa1097f65603c4832c44b1a12cb419016d1e5 Cr-Commit-Position: refs/heads/master@{#431449} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/dbefa1097f65603c4832c44b1a12cb419016d1e5 Cr-Commit-Position: refs/heads/master@{#431449} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
