Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1855393006:
[Chrome Settings UI] If User Exceptions are not allowed, prevent editing / viewing. (Closed)
|
Created:
4 years, 8 months ago by huangs Modified:
4 years, 7 months ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, michaelpg+watch-options_chromium.org, arv+watch_chromium.org, grt (UTC plus 2), Georges Khalil Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Chrome Settings UI] If User Exceptions are not allowed, prevent editing / viewing.
If user exceptions are rendered useless by policy defaults (or other default
providers with higher precedence), disabled them in exceptions dialog by:
- Hiding all editable rows.
- Removing the "Add New Exceptions" row.
BUG=568031
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/ff30d12673cee058e00d6d8da59ba971f24172bb
Cr-Commit-Position: refs/heads/master@{#391885}
Patch Set 1 #Patch Set 2 : Fix refactoring bug. #
Total comments: 3
Patch Set 3 : Sync. #Patch Set 4 : Connect with backend. #
Total comments: 2
Patch Set 5 : Fix incorrect ArrayDataModel.splice() usage. #
Total comments: 6
Patch Set 6 : Adding and using ArrayDataModel.pop(). #
Total comments: 12
Patch Set 7 : Fix JS styles. #
Total comments: 7
Patch Set 8 : Sync. #Patch Set 9 : Making ExceptionsList.allowEdit_ a private member. #
Messages
Total messages: 38 (10 generated)
huangs@chromium.org changed reviewers: + estade@chromium.org
This is sliced off from https://codereview.chromium.org/1865803002/ PTAL. Thanks!
The CQ bit was checked by huangs@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1855393006/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1855393006/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I don't think it makes sense to separate this from the other CL because on its own, this is dead code. https://codereview.chromium.org/1855393006/diff/20001/chrome/browser/resource... File chrome/browser/resources/options/content_settings.js (right): https://codereview.chromium.org/1855393006/diff/20001/chrome/browser/resource... chrome/browser/resources/options/content_settings.js:137: * @param {boolean} maybeEnableEdit Whether user may view/edit exceptions. maybe is a confusing word and should generally be avoided https://codereview.chromium.org/1855393006/diff/20001/chrome/browser/resource... File chrome/browser/resources/options/content_settings_exceptions_area.js (right): https://codereview.chromium.org/1855393006/diff/20001/chrome/browser/resource... chrome/browser/resources/options/content_settings_exceptions_area.js:531: if (item.dataItem.source == 'preference') { nit: no curlies
Thanks! Will update the other CL. Closing current CL.
https://codereview.chromium.org/1855393006/diff/20001/chrome/browser/resource... File chrome/browser/resources/options/content_settings.js (right): https://codereview.chromium.org/1855393006/diff/20001/chrome/browser/resource... chrome/browser/resources/options/content_settings.js:137: * @param {boolean} maybeEnableEdit Whether user may view/edit exceptions. Replaced with "allow".
Description was changed from ========== [Chrome Settings UI] Add ContentSettings.setMaybeEnablePrefExceptions(). This allows ContentSettingsHandler to dynamically enable/disable editing of user preference exceptions for specific content types. Specifically, we can: - Show/hide all editable rows. - Add/remove the "Add New Exceptions" row. BUG=601057 ========== to ========== [Chrome Settings UI] Add ContentSettings.setMaybeEnablePrefExceptions(). This allows ContentSettingsHandler to dynamically enable/disable editing of user preference exceptions for specific content types. Specifically, we can: - Show/hide all editable rows. - Add/remove the "Add New Exceptions" row. BUG=601057 ==========
Message was sent while issue was closed.
Description was changed from ========== [Chrome Settings UI] Add ContentSettings.setMaybeEnablePrefExceptions(). This allows ContentSettingsHandler to dynamically enable/disable editing of user preference exceptions for specific content types. Specifically, we can: - Show/hide all editable rows. - Add/remove the "Add New Exceptions" row. BUG=601057 ========== to ========== [Chrome Settings UI] If User Exceptions are not allowed, prevent editing / viewing. 12345678901234567890123456789012345678901234567890123456789012345678901234567890 If user exceptions are rendered useless by policy defaults (or other default providers with higher precedence), disabled them in exceptions dialog by: - Hiding all editable rows. - Removing the "Add New Exceptions" row. BUG=568031 ==========
Description was changed from ========== [Chrome Settings UI] If User Exceptions are not allowed, prevent editing / viewing. 12345678901234567890123456789012345678901234567890123456789012345678901234567890 If user exceptions are rendered useless by policy defaults (or other default providers with higher precedence), disabled them in exceptions dialog by: - Hiding all editable rows. - Removing the "Add New Exceptions" row. BUG=568031 ========== to ========== [Chrome Settings UI] If User Exceptions are not allowed, prevent editing / viewing. If user exceptions are rendered useless by policy defaults (or other default providers with higher precedence), disabled them in exceptions dialog by: - Hiding all editable rows. - Removing the "Add New Exceptions" row. BUG=568031 ==========
The required data is piped through (http://crrev.com/1873343002), so resurrecting this CL now that its effects manifest. PTAL.
https://codereview.chromium.org/1855393006/diff/60001/chrome/browser/resource... File chrome/browser/resources/options/content_settings_exceptions_area.js (right): https://codereview.chromium.org/1855393006/diff/60001/chrome/browser/resource... chrome/browser/resources/options/content_settings_exceptions_area.js:551: this.dataModel.splice(oldSize, oldSize, null); I tried to understand but can't figure out what this block of code is doing. Is this just a push? Below is just a pop?
Updated, PTAL. https://codereview.chromium.org/1855393006/diff/60001/chrome/browser/resource... File chrome/browser/resources/options/content_settings_exceptions_area.js (right): https://codereview.chromium.org/1855393006/diff/60001/chrome/browser/resource... chrome/browser/resources/options/content_settings_exceptions_area.js:551: this.dataModel.splice(oldSize, oldSize, null); I thought the params were (start, end), but it was (start, remove count). Updated. Also using push() instead, since it's supported by ArrayDataModel.
https://codereview.chromium.org/1855393006/diff/80001/chrome/browser/resource... File chrome/browser/resources/options/content_settings_exceptions_area.js (right): https://codereview.chromium.org/1855393006/diff/80001/chrome/browser/resource... chrome/browser/resources/options/content_settings_exceptions_area.js:553: this.dataModel.splice(oldSize - 1, 1); // pop. I would be in favor of adding pop to ArrayDataModel
https://chromiumcodereview.appspot.com/1855393006/diff/80001/chrome/browser/r... File chrome/browser/resources/options/content_settings_exceptions_area.js (right): https://chromiumcodereview.appspot.com/1855393006/diff/80001/chrome/browser/r... chrome/browser/resources/options/content_settings_exceptions_area.js:553: this.dataModel.splice(oldSize - 1, 1); // pop. ArrayDataModel stores raw list of data in |this.array_|, and the elements are ordered by |this.indexes_|, sorting is applied to this order. If we add pop() then it will remove the "logical last element", i.e., this.array_[this.index_[this.index_.length - 1]]? Meanwhile, it looks like ExceptionList is skipping all this sorting business because |this.dataModel.sortStatus_.field| == null. Rather than adding a more general pop() that only we use, and without exercising its generality, I think it's cleaner to just call splice() explicitly?
https://chromiumcodereview.appspot.com/1855393006/diff/80001/chrome/browser/r... File chrome/browser/resources/options/content_settings_exceptions_area.js (right): https://chromiumcodereview.appspot.com/1855393006/diff/80001/chrome/browser/r... chrome/browser/resources/options/content_settings_exceptions_area.js:553: this.dataModel.splice(oldSize - 1, 1); // pop. On 2016/04/26 17:37:03, huangs wrote: > ArrayDataModel stores raw list of data in |this.array_|, and the elements are > ordered by |this.indexes_|, sorting is applied to this order. If we add pop() > then it will remove the "logical last element", i.e., > this.array_[this.index_[this.index_.length - 1]]? > > Meanwhile, it looks like ExceptionList is skipping all this sorting business > because |this.dataModel.sortStatus_.field| == null. Rather than adding a more > general pop() that only we use, and without exercising its generality, I think > it's cleaner to just call splice() explicitly? Don't you think pop() would be useful in other places? It's likely that the only reason it's not used anywhere else currently is because it doesn't exist and everyone just keeps using splice.
huangs@chromium.org changed reviewers: + dbeam@chromium.org
+dbeam@ for discussion: We're contemplating adding a ArrayDataModel.pop() function, rather than calling splice() directly. PTAL. Thanks! https://codereview.chromium.org/1855393006/diff/80001/chrome/browser/resource... File chrome/browser/resources/options/content_settings_exceptions_area.js (right): https://codereview.chromium.org/1855393006/diff/80001/chrome/browser/resource... chrome/browser/resources/options/content_settings_exceptions_area.js:553: this.dataModel.splice(oldSize - 1, 1); // pop. My preference is to keep the CL small. It seems adding pop() and refactoring users for it should be an independent cleanup? +dbeam@ for discussion.
https://codereview.chromium.org/1855393006/diff/80001/chrome/browser/resource... File chrome/browser/resources/options/content_settings_exceptions_area.js (right): https://codereview.chromium.org/1855393006/diff/80001/chrome/browser/resource... chrome/browser/resources/options/content_settings_exceptions_area.js:553: this.dataModel.splice(oldSize - 1, 1); // pop. On 2016/04/26 23:18:20, huangs wrote: > My preference is to keep the CL small. It seems adding pop() and refactoring > users for it should be an independent cleanup? +dbeam@ for discussion. i don't really know much about ArrayDataModel, but could we just add pop() as an alias for splice(this.length - 1, 1) basically with a TODO?
Updated, PTAL. https://codereview.chromium.org/1855393006/diff/80001/chrome/browser/resource... File chrome/browser/resources/options/content_settings_exceptions_area.js (right): https://codereview.chromium.org/1855393006/diff/80001/chrome/browser/resource... chrome/browser/resources/options/content_settings_exceptions_area.js:553: this.dataModel.splice(oldSize - 1, 1); // pop. Added pop(). It returns the last item as well, for least surprise.
https://codereview.chromium.org/1855393006/diff/100001/chrome/browser/resourc... File chrome/browser/resources/options/content_settings_exceptions_area.js (right): https://codereview.chromium.org/1855393006/diff/100001/chrome/browser/resourc... chrome/browser/resources/options/content_settings_exceptions_area.js:549: var oldSize = this.dataModel.length; unused https://codereview.chromium.org/1855393006/diff/100001/chrome/browser/resourc... chrome/browser/resources/options/content_settings_exceptions_area.js:552: } else { no curlies https://codereview.chromium.org/1855393006/diff/100001/chrome/browser/resourc... chrome/browser/resources/options/content_settings_exceptions_area.js:553: console.log(this.dataModel.pop()); remove log https://codereview.chromium.org/1855393006/diff/100001/chrome/browser/resourc... chrome/browser/resources/options/content_settings_exceptions_area.js:556: this.updateRowVisibility(); does this need to be called if oldIsEditable == newIsEditable? https://codereview.chromium.org/1855393006/diff/100001/ui/webui/resources/js/... File ui/webui/resources/js/cr/ui/array_data_model.js (right): https://codereview.chromium.org/1855393006/diff/100001/ui/webui/resources/js/... ui/webui/resources/js/cr/ui/array_data_model.js:229: * @return {*} The last item, or undefined if model has no more items. we should @template-ize this class some day https://codereview.chromium.org/1855393006/diff/100001/ui/webui/resources/js/... ui/webui/resources/js/cr/ui/array_data_model.js:233: return deletedItems.length > 0 ? deletedItems[0] : undefined; return this.splice(this.array_.length - 1, 1)[0];
Thanks! Updated. https://chromiumcodereview.appspot.com/1855393006/diff/100001/chrome/browser/... File chrome/browser/resources/options/content_settings_exceptions_area.js (right): https://chromiumcodereview.appspot.com/1855393006/diff/100001/chrome/browser/... chrome/browser/resources/options/content_settings_exceptions_area.js:549: var oldSize = this.dataModel.length; On 2016/04/27 21:47:27, Dan Beam wrote: > unused Done. https://chromiumcodereview.appspot.com/1855393006/diff/100001/chrome/browser/... chrome/browser/resources/options/content_settings_exceptions_area.js:552: } else { On 2016/04/27 21:47:27, Dan Beam wrote: > no curlies Done. https://chromiumcodereview.appspot.com/1855393006/diff/100001/chrome/browser/... chrome/browser/resources/options/content_settings_exceptions_area.js:553: console.log(this.dataModel.pop()); On 2016/04/27 21:47:27, Dan Beam wrote: > remove log Done. https://chromiumcodereview.appspot.com/1855393006/diff/100001/chrome/browser/... chrome/browser/resources/options/content_settings_exceptions_area.js:556: this.updateRowVisibility(); On 2016/04/27 21:47:27, Dan Beam wrote: > does this need to be called if oldIsEditable == newIsEditable? Done. https://chromiumcodereview.appspot.com/1855393006/diff/100001/ui/webui/resour... File ui/webui/resources/js/cr/ui/array_data_model.js (right): https://chromiumcodereview.appspot.com/1855393006/diff/100001/ui/webui/resour... ui/webui/resources/js/cr/ui/array_data_model.js:229: * @return {*} The last item, or undefined if model has no more items. Don't understand what you mean? https://chromiumcodereview.appspot.com/1855393006/diff/100001/ui/webui/resour... ui/webui/resources/js/cr/ui/array_data_model.js:233: return deletedItems.length > 0 ? deletedItems[0] : undefined; On 2016/04/27 21:47:27, Dan Beam wrote: > return this.splice(this.array_.length - 1, 1)[0]; Done.
Ping for review, PTAL.
Changing plans: Instead of prevent editing, I'll just gray out the items and display the little building icon to show that user exceptions are ineffective. This is incremental change from current behavior. We can continue debate whether to hide and allow edit, etc., later.
https://codereview.chromium.org/1855393006/diff/120001/chrome/browser/resourc... File chrome/browser/resources/options/content_settings.js (right): https://codereview.chromium.org/1855393006/diff/120001/chrome/browser/resourc... chrome/browser/resources/options/content_settings.js:137: * @param {boolean} allowUserExceptions Whether user may set exceptions. please closure compile your code, this name doesn't match (allowUserExceptions vs userExceptionsAllowed) and is a boolean here but a string in the other class https://codereview.chromium.org/1855393006/diff/120001/chrome/browser/resourc... File chrome/browser/resources/options/content_settings_exceptions_area.js (right): https://codereview.chromium.org/1855393006/diff/120001/chrome/browser/resourc... chrome/browser/resources/options/content_settings_exceptions_area.js:539: * @param {string} allowEdit Whether or not user may set preference should this be a boolean? https://codereview.chromium.org/1855393006/diff/120001/chrome/browser/resourc... chrome/browser/resources/options/content_settings_exceptions_area.js:544: this.allowEdit = allowEdit; can this be private?
Description was changed from ========== [Chrome Settings UI] If User Exceptions are not allowed, prevent editing / viewing. If user exceptions are rendered useless by policy defaults (or other default providers with higher precedence), disabled them in exceptions dialog by: - Hiding all editable rows. - Removing the "Add New Exceptions" row. BUG=568031 ========== to ========== [Chrome Settings UI] If User Exceptions are not allowed, prevent editing / viewing. If user exceptions are rendered useless by policy defaults (or other default providers with higher precedence), disabled them in exceptions dialog by: - Hiding all editable rows. - Removing the "Add New Exceptions" row. BUG=568031 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Addressed the bug, have not done the graying part yet. https://codereview.chromium.org/1855393006/diff/120001/chrome/browser/resourc... File chrome/browser/resources/options/content_settings.js (right): https://codereview.chromium.org/1855393006/diff/120001/chrome/browser/resourc... chrome/browser/resources/options/content_settings.js:137: * @param {boolean} allowUserExceptions Whether user may set exceptions. Fixed. How do you "properly" run Closure on this? I thought the code are just Closure-like, without using closure? Anyway, ran java -jar C:\closure\compiler.jar %* --checks_only --warning_level VERBOSE --jscomp_warning "*" to content_settings.js and content_settings_exceptions_area.js . There are bunch of existing errors. Ensured that I didn't add new genuine error. https://codereview.chromium.org/1855393006/diff/120001/chrome/browser/resourc... File chrome/browser/resources/options/content_settings_exceptions_area.js (right): https://codereview.chromium.org/1855393006/diff/120001/chrome/browser/resourc... chrome/browser/resources/options/content_settings_exceptions_area.js:539: * @param {string} allowEdit Whether or not user may set preference On 2016/05/03 04:20:26, Dan Beam wrote: > should this be a boolean? Done. https://codereview.chromium.org/1855393006/diff/120001/chrome/browser/resourc... chrome/browser/resources/options/content_settings_exceptions_area.js:544: this.allowEdit = allowEdit; On 2016/05/03 04:20:26, Dan Beam wrote: > can this be private? Done.
lgtm https://codereview.chromium.org/1855393006/diff/120001/chrome/browser/resourc... File chrome/browser/resources/options/content_settings.js (right): https://codereview.chromium.org/1855393006/diff/120001/chrome/browser/resourc... chrome/browser/resources/options/content_settings.js:137: * @param {boolean} allowUserExceptions Whether user may set exceptions. On 2016/05/03 18:34:18, huangs wrote: > Fixed. > > How do you "properly" run Closure on this? I thought the code are just > Closure-like, without using closure? Anyway, ran > > java -jar C:\closure\compiler.jar %* --checks_only --warning_level VERBOSE > --jscomp_warning "*" > > to content_settings.js and content_settings_exceptions_area.js . There are > bunch of existing errors. Ensured that I didn't add new genuine error. https://groups.google.com/a/chromium.org/d/msg/chromium-dev/15zvJ-TjMQY/bJq1r...
Ah okay, thanks for the tip re. closure_compilation! I'm planning to abandon the current approach of hiding "useless" user exceptions, and just gray them out (maybe with little building icon). What are your thoughts on this? Turns out it's more work than I thought, but it's a more conservative choice.
Decided: Going with current approach of hiding the user exception list. Showing user's list in some crippled form, even more "complete", is more confusing for the user, and more complex to implement. Committing!
The CQ bit was checked by huangs@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1855393006/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1855393006/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== [Chrome Settings UI] If User Exceptions are not allowed, prevent editing / viewing. If user exceptions are rendered useless by policy defaults (or other default providers with higher precedence), disabled them in exceptions dialog by: - Hiding all editable rows. - Removing the "Add New Exceptions" row. BUG=568031 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== [Chrome Settings UI] If User Exceptions are not allowed, prevent editing / viewing. If user exceptions are rendered useless by policy defaults (or other default providers with higher precedence), disabled them in exceptions dialog by: - Hiding all editable rows. - Removing the "Add New Exceptions" row. BUG=568031 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/ff30d12673cee058e00d6d8da59ba971f24172bb Cr-Commit-Position: refs/heads/master@{#391885} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/ff30d12673cee058e00d6d8da59ba971f24172bb Cr-Commit-Position: refs/heads/master@{#391885}
Message was sent while issue was closed.
A revert of this CL (patchset #9 id:160001) has been created in https://chromiumcodereview.appspot.com/1962163002/ by huangs@chromium.org. The reason for reverting is: Reverting since this is causing http://crbug.com/609725 . Problem is that we're hiding all "non-editable" entries in exception lists. However, the list is sometimes used for read-only data, e.g., Zoom Level. Will need to modify the logic in a follow-up.. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
