Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2529543002:
MD Settings: indicate extension controlled search engines (Closed)
|
Created:
4 years, 1 month ago by Dan Beam Modified:
4 years 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. |
DescriptionMD Settings: indicate extension controlled search engines
R=dpapad@chromium.org
BUG=614265
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/bfe32efdc998006248c348e34678d5942c31fc3b
Cr-Commit-Position: refs/heads/master@{#436751}
Patch Set 1 : self-review #
Total comments: 6
Patch Set 2 : circumvent test failures #
Total comments: 4
Patch Set 3 : add test #Patch Set 4 : merge #
Messages
Total messages: 33 (22 generated)
Description was changed from ========== MD Settings: indicate extension controlled search engines R=dpapad@chromium.org BUG=614265 ========== to ========== MD Settings: indicate extension controlled search engines R=dpapad@chromium.org BUG=614265 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Patchset #1 (id:1) has been deleted
The CQ bit was checked by dbeam@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_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2529543002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/search_engines_page/search_engine_entry.html (right): https://codereview.chromium.org/2529543002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/search_engines_page/search_engine_entry.html:52: <template is="dom-if" if="[[showDots_]]"> Perhaps add a test for this? https://codereview.chromium.org/2529543002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/search_engines_page/search_engine_entry.js (right): https://codereview.chromium.org/2529543002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/search_engines_page/search_engine_entry.js:27: computed: 'computeShowDots_(engine.canBeDefault,' + Could this be computeShowDots_(engine.*) instead? If that is equivalent, it would also simplify the siganture of computeShowDots_(), by declaring no params, and using |this.engine| internally. https://codereview.chromium.org/2529543002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/search_engines_page/search_engine_entry.js:65: return canBeDefault || canBeEdited || canBeRemoved; Related to previous comment, you could simplify the signature as follows. computeShowDots_: function() { return this.engine.canBeDefault || ... || ...; }
Patchset #2 (id:40001) has been deleted
https://codereview.chromium.org/2529543002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/search_engines_page/search_engine_entry.html (right): https://codereview.chromium.org/2529543002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/search_engines_page/search_engine_entry.html:52: <template is="dom-if" if="[[showDots_]]"> On 2016/11/23 19:59:54, dpapad wrote: > Perhaps add a test for this? well, I removed the dom-if for a few reasons. a) an overwhelming majority of the time |showDots_| will be true so this is arguably worse for performance than better. b) it was breaking the tests because they rely this menu existing even when an engine can't be made default, edited, nor removed. this is also related to the type errors (missing arguments) we found. that said: do you want me to test that when an engine can't be edited, made default, or removed, the entry's <paper-icon-button> is visibility: hidden;? because that's starting to sound kinda brittle. https://codereview.chromium.org/2529543002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/search_engines_page/search_engine_entry.js (right): https://codereview.chromium.org/2529543002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/search_engines_page/search_engine_entry.js:27: computed: 'computeShowDots_(engine.canBeDefault,' + On 2016/11/23 19:59:54, dpapad wrote: > Could this be computeShowDots_(engine.*) instead? If that is equivalent, it > would also simplify the siganture of computeShowDots_(), by declaring no params, > and using |this.engine| internally. Acknowledged. https://codereview.chromium.org/2529543002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/search_engines_page/search_engine_entry.js:65: return canBeDefault || canBeEdited || canBeRemoved; On 2016/11/23 19:59:54, dpapad wrote: > Related to previous comment, you could simplify the signature as follows. > > computeShowDots_: function() { > return this.engine.canBeDefault || ... || ...; > } Acknowledged.
The CQ bit was checked by dbeam@chromium.org to run a CQ dry run
Patchset #2 (id:60001) has been deleted
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.
> that said: do you want me to test that when an engine can't be edited, made default, or removed, the entry's <paper-icon-button> is visibility: hidden;? because that's starting to sound kinda brittle. Couple of observations: 1) I don't think there is such a thing as a private attribute on host. show-dots_ It is visible from the outside, without violating any DOM boundaries, <search-engine-entry show-dots_>...</..> 2) Agreed that testing for visibility hidden is brittle. I think it would be more robust to disable that button, when it is hidden. This would ensure that it can't be activated (mouse-event or keyboard) and you could add a test where you check that the button is disabled (even though it is also hidden and you would not check that). https://codereview.chromium.org/2529543002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/search_engines_page/search_engine_entry.html (right): https://codereview.chromium.org/2529543002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/search_engines_page/search_engine_entry.html:33: visibility: hidden; Does that ensure that the button can't be reached via the keyboard (by tabbing)? https://codereview.chromium.org/2529543002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/search_engines_page/search_engine_entry.js (left): https://codereview.chromium.org/2529543002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/search_engines_page/search_engine_entry.js:72: closePopupMenu_: function() { Nit(optional): Why are you moving those 3 functions? This just makes the diff bigger.
On 2016/12/01 19:02:16, dpapad wrote:
> > that said: do you want me to test that when an engine can't be edited, made
> default, or removed, the entry's <paper-icon-button> is visibility: hidden;?
> because that's starting to sound kinda brittle.
>
> Couple of observations:
> 1) I don't think there is such a thing as a private attribute on host.
> show-dots_ It is visible from the outside, without violating any DOM
boundaries,
> <search-engine-entry show-dots_>...</..>
we talked about this and private by convention is about the best we're going to
do here.
>
> 2) Agreed that testing for visibility hidden is brittle. I think it would be
> more robust to disable that button, when it is hidden. This would ensure that
it
> can't be activated (mouse-event or keyboard) and you could add a test where
you
> check that the button is disabled (even though it is also hidden and you would
> not check that).
added a test using hasAttribute('show-dots_'). bettes@ wanted to hide the
button rather than disable (just like the logic for the individual items, i.e.
"Make default" is hidden rather than shown and disabled)
https://codereview.chromium.org/2529543002/diff/80001/chrome/browser/resource...
File
chrome/browser/resources/settings/search_engines_page/search_engine_entry.html
(right):
https://codereview.chromium.org/2529543002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/search_engines_page/search_engine_entry.html:33:
visibility: hidden;
On 2016/12/01 19:02:15, dpapad wrote:
> Does that ensure that the button can't be reached via the keyboard (by
tabbing)?
yes, AND fun fact: it also passes the a11y audit (sometimes the audit will
complain about things that are part of the tab order but are visually occluded
or something like that)
https://codereview.chromium.org/2529543002/diff/80001/chrome/browser/resource...
File
chrome/browser/resources/settings/search_engines_page/search_engine_entry.js
(left):
https://codereview.chromium.org/2529543002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/search_engines_page/search_engine_entry.js:72:
closePopupMenu_: function() {
On 2016/12/01 19:02:15, dpapad wrote:
> Nit(optional): Why are you moving those 3 functions? This just makes the diff
> bigger.
they weren't alphabetized
The CQ bit was checked by dbeam@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.
lgtm
The CQ bit was checked by dbeam@chromium.org
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
Failed to apply patch for
chrome/browser/resources/settings/search_engines_page/search_engine_entry.html:
While running git apply --index -p1;
error: patch failed:
chrome/browser/resources/settings/search_engines_page/search_engine_entry.html:26
error:
chrome/browser/resources/settings/search_engines_page/search_engine_entry.html:
patch does not apply
Patch:
chrome/browser/resources/settings/search_engines_page/search_engine_entry.html
Index:
chrome/browser/resources/settings/search_engines_page/search_engine_entry.html
diff --git
a/chrome/browser/resources/settings/search_engines_page/search_engine_entry.html
b/chrome/browser/resources/settings/search_engines_page/search_engine_entry.html
index
81130ec9cf383ca3ec5d125552bf3f969864d949..a721fe3a11d6157de7bb9baf83d9c673fe36075e
100644
---
a/chrome/browser/resources/settings/search_engines_page/search_engine_entry.html
+++
b/chrome/browser/resources/settings/search_engines_page/search_engine_entry.html
@@ -3,6 +3,7 @@
<link rel="import" href="chrome://resources/html/polymer.html">
<link rel="import" href="chrome://resources/html/icon.html">
<link rel="import"
href="chrome://resources/polymer/v1_0/paper-icon-button/paper-icon-button.html">
+<link rel="import" href="/controls/extension_controlled_indicator.html">
<link rel="import" href="/search_engines_page/search_engine_dialog.html">
<link rel="import" href="/search_engines_page/search_engine_entry_css.html">
<link rel="import"
href="/search_engines_page/search_engines_browser_proxy.html">
@@ -11,7 +12,7 @@
<dom-module id="settings-search-engine-entry">
<template>
<style include="settings-shared search-engine-entry">
- :host([is-default]) {
+ :host([is-default]) .list-item {
font-weight: 500;
}
@@ -26,6 +27,11 @@
text-overflow: ellipsis;
white-space: nowrap;
}
+
+ :host(:not([show-dots_])) paper-icon-button {
+ pointer-events: none;
+ visibility: hidden;
+ }
</style>
<template is="dom-if" if="[[showEditSearchEngineDialog_]]" restamp>
@@ -56,6 +62,13 @@
id="delete">$i18n{searchEnginesRemoveFromList}</button>
</dialog>
</div>
+ <template is="dom-if" if="[[engine.extension]]">
+ <extension-controlled-indicator
+ extension-id="[[engine.extension.id]]"
+ extension-name="[[engine.extension.name]]"
+ extension-can-be-disabled="[[engine.extension.canBeDisabled]]">
+ </extension-controlled-indicator>
+ </template>
</template>
<script src="search_engine_entry.js"></script>
</dom-module>
The CQ bit was checked by dbeam@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/2529543002/#ps120001 (title: "merge")
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": 120001, "attempt_start_ts": 1481057578516670,
"parent_rev": "0a97828b89666fb2904ef3c7b82bb0dedd49105c", "commit_rev":
"aa9759278a3b45adbf56d5d9732de2da34cb08c1"}
Message was sent while issue was closed.
Committed patchset #4 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== MD Settings: indicate extension controlled search engines R=dpapad@chromium.org BUG=614265 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: indicate extension controlled search engines R=dpapad@chromium.org BUG=614265 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/bfe32efdc998006248c348e34678d5942c31fc3b Cr-Commit-Position: refs/heads/master@{#436751} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/bfe32efdc998006248c348e34678d5942c31fc3b Cr-Commit-Position: refs/heads/master@{#436751} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
