MD Settings: validate home button url input.
This CL adds validation to the home url input, and shows error message when the input is an invalid url. This CL also removes some of settings-input's logic, since it is no longer being used by anything outside of appearance-page.
BUG=681243
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2766093002
Cr-Commit-Position: refs/heads/master@{#459614}
Committed: https://chromium.googlesource.com/chromium/src/+/2f7628484099e467516f27c8a6e9f056461c1aab
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_compilation/builds/7198)
3 years, 9 months ago
(2017-03-23 23:08:37 UTC)
#7
https://codereview.chromium.org/2766093002/diff/60001/chrome/app/settings_strings.grdp File chrome/app/settings_strings.grdp (right): https://codereview.chromium.org/2766093002/diff/60001/chrome/app/settings_strings.grdp#newcode244 chrome/app/settings_strings.grdp:244: Not valid Can we reuse/repurpose the existing identical message ...
3 years, 9 months ago
(2017-03-24 01:39:32 UTC)
#8
https://codereview.chromium.org/2766093002/diff/60001/chrome/browser/resources/settings/appearance_page/appearance_page.html File chrome/browser/resources/settings/appearance_page/appearance_page.html (right): https://codereview.chromium.org/2766093002/diff/60001/chrome/browser/resources/settings/appearance_page/appearance_page.html#newcode131 chrome/browser/resources/settings/appearance_page/appearance_page.html:131: stop-keyboard-event-propagation no-extension-indicator On 2017/03/24 at 01:39:31, dpapad wrote: > ...
3 years, 9 months ago
(2017-03-24 18:07:16 UTC)
#9
https://codereview.chromium.org/2766093002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/appearance_page/appearance_page.html
(right):
https://codereview.chromium.org/2766093002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_page.html:131:
stop-keyboard-event-propagation no-extension-indicator
On 2017/03/24 at 01:39:31, dpapad wrote:
> Is settings-input used anywhere else besides the appearance-page? Asking
because I see |no-extension-indicator| here, which implies that we use
settings-input elsewhere and we do want the extension indicator. Or is it
obsolete code and we can simplify settings-input itself?
Looking in git history revealed that at some point it was used by the
downloads_page.html, but not anymore, see
https://codereview.chromium.org/1368733002. Can we clean up settings-input to
not have any dead code (probably better in a separate CL that lands before your
fix)?
scottchen
Patchset #7 (id:120001) has been deleted
3 years, 9 months ago
(2017-03-24 20:47:36 UTC)
#10
Patchset #7 (id:120001) has been deleted
scottchen
The CQ bit was checked by scottchen@chromium.org to run a CQ dry run
3 years, 9 months ago
(2017-03-24 20:48:26 UTC)
#11
https://codereview.chromium.org/2766093002/diff/60001/chrome/app/settings_strings.grdp File chrome/app/settings_strings.grdp (right): https://codereview.chromium.org/2766093002/diff/60001/chrome/app/settings_strings.grdp#newcode244 chrome/app/settings_strings.grdp:244: Not valid On 2017/03/24 01:39:31, dpapad wrote: > Can ...
3 years, 9 months ago
(2017-03-24 20:48:54 UTC)
#12
https://codereview.chromium.org/2766093002/diff/60001/chrome/app/settings_str...
File chrome/app/settings_strings.grdp (right):
https://codereview.chromium.org/2766093002/diff/60001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:244: Not valid
On 2017/03/24 01:39:31, dpapad wrote:
> Can we reuse/repurpose the existing identical message at
>
https://cs.chromium.org/chromium/src/chrome/app/settings_strings.grdp?l=1732-...
I feel like there's always a good chance strings used in different sections
could change, so I made a new one for easier maintainability. Do we save much by
reusing the same string?
https://codereview.chromium.org/2766093002/diff/60001/chrome/browser/resource...
File
chrome/browser/resources/settings/appearance_page/appearance_browser_proxy.js
(right):
https://codereview.chromium.org/2766093002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_browser_proxy.js:61:
// <if expr="chromeos">
On 2017/03/24 01:39:31, dpapad wrote:
> +dbeam:
> clang-format is causing those <if expr> statements to be indented.
> Is that intentional or should we revert those changes? They are causing
> unnecessarily larger diffs.
Yeah I'm wondering the same thing too. I thought we originally gave these 0
indentation so that they're easier to spot. Maybe we should modify clang-format
rule to not indent \^// <.*\
https://codereview.chromium.org/2766093002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_browser_proxy.js:81:
validateStartupPage: function(url) {
On 2017/03/24 01:39:31, dpapad wrote:
> This method needs to be declared on the AppearanceBrowserProxy interface at
the
> top of this file. Otherwise there is nothing to "override" here.
Done.
https://codereview.chromium.org/2766093002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/appearance_page/appearance_page.html
(right):
https://codereview.chromium.org/2766093002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_page.html:122:
pref="[[prefs.homepage_is_newtabpage]]" id="custom-input"
On 2017/03/24 01:39:31, dpapad wrote:
> Nit (optional): It is more common to have the id="..." be the first attribute
> (before "class=").
Done.
https://codereview.chromium.org/2766093002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_page.html:131:
stop-keyboard-event-propagation no-extension-indicator
On 2017/03/24 18:07:16, dpapad wrote:
> On 2017/03/24 at 01:39:31, dpapad wrote:
> > Is settings-input used anywhere else besides the appearance-page? Asking
> because I see |no-extension-indicator| here, which implies that we use
> settings-input elsewhere and we do want the extension indicator. Or is it
> obsolete code and we can simplify settings-input itself?
>
> Looking in git history revealed that at some point it was used by the
> downloads_page.html, but not anymore, see
> https://codereview.chromium.org/1368733002. Can we clean up settings-input to
> not have any dead code (probably better in a separate CL that lands before
your
> fix)?
I think since I already added stuff in settings-input, it'll be easier for me to
minify the code in settings-input first, land the bugfix, and then in a separate
CL migrate it to paper-input (and delete settings-input entirely, code-search
seems to show it's not used by anything anymore).
https://codereview.chromium.org/2766093002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/appearance_page/appearance_page.js
(right):
https://codereview.chromium.org/2766093002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_page.js:276:
validate_: function(event) {
On 2017/03/24 01:39:31, dpapad wrote:
> @param, @private missing.
Done.
https://codereview.chromium.org/2766093002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_page.js:280:
inputElement.invalid = false;
On 2017/03/24 01:39:31, dpapad wrote:
> Can we use data binding instead?
>
>
> // in appearance_page.html
> <settings-input invalid="[[isHomePageInvalid_]]" ...></settings-input>
>
> Then here just do
>
> if (inputElement.value == '')
> this.isHomepageInvalid_ = false;
>
> ...
Done.
https://codereview.chromium.org/2766093002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/controls/settings_input.js (right):
https://codereview.chromium.org/2766093002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/controls/settings_input.js:38: notify: true,
On 2017/03/24 01:39:31, dpapad wrote:
> Is this needed? From the docs, this is only applicable when a two-way binding
is
> used, but I did not see any binding for it here.
Based on your comment regarding using data-binding for invalid, it's actually
necessary to do a two-way binding here: <settings-input invalid="{{..}}">, since
settings-input programmatically change .invalid's value too, so if it doesn't
notify, they get de-synchronized and sometimes validity state isn't showing
correctly.
https://codereview.chromium.org/2766093002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/controls/settings_input.js:137: * Handler for
profile name keydowns.
On 2017/03/24 01:39:31, dpapad wrote:
> Is this comment obsolete?
Acknowledged.
https://codereview.chromium.org/2766093002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/controls/settings_input.js:142: /* If pressed
enter when input is invalid, do not trigger on-change */
On 2017/03/24 01:39:31, dpapad wrote:
> End full sentence comment with a period.
Done.
https://codereview.chromium.org/2766093002/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/appearance_page_test.js (right):
https://codereview.chromium.org/2766093002/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/settings/appearance_page_test.js:80: * @param {string}
value The string represnatation of boolean to return.
On 2017/03/24 01:39:32, dpapad wrote:
> No need for @param when you are using @override.
Done.
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2766093002/140001
3 years, 9 months ago
(2017-03-24 20:49:26 UTC)
#13
https://codereview.chromium.org/2766093002/diff/60001/chrome/app/settings_strings.grdp File chrome/app/settings_strings.grdp (right): https://codereview.chromium.org/2766093002/diff/60001/chrome/app/settings_strings.grdp#newcode244 chrome/app/settings_strings.grdp:244: Not valid On 2017/03/24 21:47:43, dpapad wrote: > On ...
3 years, 9 months ago
(2017-03-24 22:56:31 UTC)
#15
https://codereview.chromium.org/2766093002/diff/60001/chrome/app/settings_str...
File chrome/app/settings_strings.grdp (right):
https://codereview.chromium.org/2766093002/diff/60001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:244: Not valid
On 2017/03/24 21:47:43, dpapad wrote:
> On 2017/03/24 at 20:48:54, scottchen wrote:
> > On 2017/03/24 01:39:31, dpapad wrote:
> > > Can we reuse/repurpose the existing identical message at
> > >
>
https://cs.chromium.org/chromium/src/chrome/app/settings_strings.grdp?l=1732-...
> >
> > I feel like there's always a good chance strings used in different sections
> could change, so I made a new one for easier maintainability. Do we save much
by
> reusing the same string?
>
> I think is better to be frugal with our strings and only fork a string once
(if
> ever) the need arises, instead of preemptively having them as separate, just
in
> case.
>
> We have a large set of shared strings, meaning that they are used in multiple
UI
> surfaces in Settings, see
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/settings/md_sett....
>
> > Do we save much by reusing the same string?
> Every string gets translated in 40+ languages that Chrome supports. This
> generates additional work for the translation team, and also means that the
40+
> translated copies of the identical string are included in the final binary
> (AFAIK the translations are not downloaded on-demand).
Acknowledged.
https://codereview.chromium.org/2766093002/diff/60001/chrome/browser/resource...
File
chrome/browser/resources/settings/appearance_page/appearance_browser_proxy.js
(right):
https://codereview.chromium.org/2766093002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_browser_proxy.js:61:
// <if expr="chromeos">
On 2017/03/24 21:47:43, dpapad wrote:
> On 2017/03/24 at 20:48:54, scottchen wrote:
> > On 2017/03/24 01:39:31, dpapad wrote:
> > > +dbeam:
> > > clang-format is causing those <if expr> statements to be indented.
> > > Is that intentional or should we revert those changes? They are causing
> > > unnecessarily larger diffs.
> >
> > Yeah I'm wondering the same thing too. I thought we originally gave these 0
> indentation so that they're easier to spot. Maybe we should modify
clang-format
> rule to not indent \^// <.*\
>
> Since dbeam is OOO, can we revert those changes for now, until we establish
> whether the current output o the formatting tool is what we really want?
Done.
https://codereview.chromium.org/2766093002/diff/160001/chrome/browser/resourc...
File
chrome/browser/resources/settings/appearance_page/appearance_browser_proxy.js
(right):
https://codereview.chromium.org/2766093002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/appearance_page/appearance_browser_proxy.js:36:
validateStartupPage: assertNotReached
On 2017/03/24 21:47:43, dpapad wrote:
> Nit (optional): It's OK to add a trailing comma here.
Done.
https://codereview.chromium.org/2766093002/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/settings/appearance_page/appearance_page.html
(left):
https://codereview.chromium.org/2766093002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/appearance_page/appearance_page.html:125:
stop-keyboard-event-propagation no-extension-indicator>
On 2017/03/24 21:47:43, dpapad wrote:
> Is this not necessary anymore?
Moved it to inside settings-input.
https://codereview.chromium.org/2766093002/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/settings/controls/settings_input.js (right):
https://codereview.chromium.org/2766093002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/controls/settings_input.js:123: if (event.key
!= 'Escape')
On 2017/03/24 21:47:43, dpapad wrote:
> Nit(optional): Maybe shorten as follows.
>
> if (event.key == 'Escape')
> this.resetValue_();
Acknowledged. Will do in the refactor CL.
https://codereview.chromium.org/2766093002/diff/160001/chrome/test/data/webui...
File chrome/test/data/webui/settings/appearance_page_test.js (right):
https://codereview.chromium.org/2766093002/diff/160001/chrome/test/data/webui...
chrome/test/data/webui/settings/appearance_page_test.js:65: useSystemTheme:
function() {
On 2017/03/24 21:47:43, dpapad wrote:
> Accidental copy paste?
Done.
https://codereview.chromium.org/2766093002/diff/160001/chrome/test/data/webui...
chrome/test/data/webui/settings/appearance_page_test.js:83: var returnValue =
'false' ? false : true; // 'false'
On 2017/03/24 21:47:43, dpapad wrote:
> Instead of hardcoding an invalid value, how about you add a method as follows
>
> Set this.isValid_ to true in the constructor. Then do
>
> setValidateStartPageResponse(isValid) {
> this.isValid_ = isValid;
> }
>
> validateStartupPage(value) {
> this.methodCalled('validateStartupPage', value);
> return Promise.resolve(this.isValid_);
> }
>
> From your test, call browserProxy.setValidateStartupPageResponse(false), to
> force the validation to fail.
Done.
https://codereview.chromium.org/2766093002/diff/160001/chrome/test/data/webui...
chrome/test/data/webui/settings/appearance_page_test.js:274:
input.$$('paper-input').fire('change');
On 2017/03/24 21:47:43, dpapad wrote:
> Does the 'change' event have to be fired from paper-input? It seems that the
> following would also work', and would avoid accessing settings-input's
internal
> DOM structure.
>
> input.fire('change');
settingsInput.onChange_ was attached to <paper-input on-change="[[onChange_]]">,
so the change event would have to be triggered on the paper-input element.
dpapad
LGTM with nit. Also, for completeness, can we mention in the CL description that this ...
3 years, 9 months ago
(2017-03-24 23:48:54 UTC)
#16
Description was changed from ========== MD Settings: validate home button url input BUG=681243 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== ...
3 years, 9 months ago
(2017-03-24 23:53:28 UTC)
#17
Description was changed from
==========
MD Settings: validate home button url input
BUG=681243
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========
to
==========
MD Settings: validate home button url input.
This CL adds validation to the home url input, and shows error message when the
input is an invalid url. This CL also removes some of settings-input's logic,
since it is no longer being used by anything outside of appearance-page.
BUG=681243
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========
scottchen
https://codereview.chromium.org/2766093002/diff/180001/chrome/browser/resources/settings/appearance_page/appearance_page.js File chrome/browser/resources/settings/appearance_page/appearance_page.js (right): https://codereview.chromium.org/2766093002/diff/180001/chrome/browser/resources/settings/appearance_page/appearance_page.js#newcode106 chrome/browser/resources/settings/appearance_page/appearance_page.js:106: // <if expr="is_linux and not chromeos"> On 2017/03/24 23:48:53, ...
3 years, 9 months ago
(2017-03-24 23:54:04 UTC)
#18
CQ is committing da patch. Bot data: {"patchset_id": 220001, "attempt_start_ts": 1490399652098320, "parent_rev": "9de6f24edfb54f9afd3d2dee474a2602585f68dc", "commit_rev": "2f7628484099e467516f27c8a6e9f056461c1aab"}
3 years, 9 months ago
(2017-03-25 01:13:41 UTC)
#22
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1490399652098320,
"parent_rev": "9de6f24edfb54f9afd3d2dee474a2602585f68dc", "commit_rev":
"2f7628484099e467516f27c8a6e9f056461c1aab"}
commit-bot: I haz the power
Description was changed from ========== MD Settings: validate home button url input. This CL adds ...
3 years, 9 months ago
(2017-03-25 01:14:32 UTC)
#23
Message was sent while issue was closed.
Description was changed from
==========
MD Settings: validate home button url input.
This CL adds validation to the home url input, and shows error message when the
input is an invalid url. This CL also removes some of settings-input's logic,
since it is no longer being used by anything outside of appearance-page.
BUG=681243
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========
to
==========
MD Settings: validate home button url input.
This CL adds validation to the home url input, and shows error message when the
input is an invalid url. This CL also removes some of settings-input's logic,
since it is no longer being used by anything outside of appearance-page.
BUG=681243
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2766093002
Cr-Commit-Position: refs/heads/master@{#459614}
Committed:
https://chromium.googlesource.com/chromium/src/+/2f7628484099e467516f27c8a6e9...
==========
commit-bot: I haz the power
Committed patchset #11 (id:220001) as https://chromium.googlesource.com/chromium/src/+/2f7628484099e467516f27c8a6e9f056461c1aab
3 years, 9 months ago
(2017-03-25 01:14:33 UTC)
#24
Issue 2766093002: MD Settings: validate home button url input
(Closed)
Created 3 years, 9 months ago by scottchen
Modified 3 years, 8 months ago
Reviewers: dpapad, Dan Beam, chit chit
Base URL:
Comments: 50