Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(454)

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:
chit chit, Dan Beam, 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, srahim+watch_chromium.org, stevenjb+watch-md-settings_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : adjust blur/enter behaviors #

Total comments: 3

Patch Set 3 : use a less cheaty way to change invalid #

Patch Set 4 : add tests for home button urls #

Total comments: 27

Patch Set 5 : fixes based on comments #

Patch Set 6 : remove dead code and unused properties from settings-input #

Patch Set 7 : remove more deadcode #

Patch Set 8 : Merge branch 'master' into home-button #

Total comments: 12

Patch Set 9 : fixes based on comments #

Total comments: 4

Patch Set 10 : remove dead code #

Patch Set 11 : fix formatting #

Total comments: 4

Messages

Total messages: 27 (14 generated)
scottchen
https://codereview.chromium.org/2766093002/diff/20001/chrome/browser/resources/settings/appearance_page/appearance_browser_proxy.js File chrome/browser/resources/settings/appearance_page/appearance_browser_proxy.js (right): https://codereview.chromium.org/2766093002/diff/20001/chrome/browser/resources/settings/appearance_page/appearance_browser_proxy.js#newcode61 chrome/browser/resources/settings/appearance_page/appearance_browser_proxy.js:61: // <if expr="chromeos"> clang-formatter did this. https://codereview.chromium.org/2766093002/diff/20001/chrome/browser/resources/settings/appearance_page/appearance_browser_proxy.js#newcode73 chrome/browser/resources/settings/appearance_page/appearance_browser_proxy.js:73: // ...
3 years, 9 months ago (2017-03-23 20:35:51 UTC) #3
dpapad
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
dpapad
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
scottchen
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
dpapad
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 at 20:48:54, scottchen wrote: > ...
3 years, 9 months ago (2017-03-24 21:47:43 UTC) #14
scottchen
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
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
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
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2766093002/220001
3 years, 9 months ago (2017-03-24 23:54:55 UTC) #21
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
Dan Beam
https://codereview.chromium.org/2766093002/diff/220001/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/220001/chrome/browser/resources/settings/appearance_page/appearance_page.js#newcode286 chrome/browser/resources/settings/appearance_page/appearance_page.js:286: if (inputElement.value == '') { why handle this special ...
3 years, 9 months ago (2017-03-27 11:43:07 UTC) #25
scottchen
https://codereview.chromium.org/2766093002/diff/220001/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/220001/chrome/browser/resources/settings/appearance_page/appearance_page.js#newcode286 chrome/browser/resources/settings/appearance_page/appearance_page.js:286: if (inputElement.value == '') { On 2017/03/27 11:43:07, Dan ...
3 years, 8 months ago (2017-03-28 21:02:20 UTC) #26
chit chit
3 years, 8 months ago (2017-03-28 21:11:42 UTC) #27
Message was sent while issue was closed.
On 2017/03/28 21:02:20, scottchen wrote:
>
https://codereview.chromium.org/2766093002/diff/220001/chrome/browser/resourc...
> File chrome/browser/resources/settings/appearance_page/appearance_page.js
> (right):
> 
>
https://codereview.chromium.org/2766093002/diff/220001/chrome/browser/resourc...
> chrome/browser/resources/settings/appearance_page/appearance_page.js:286: if
> (inputElement.value == '') {
> On 2017/03/27 11:43:07, Dan Beam (slow) wrote:
> > why handle this special case differently?
> 
> Same behavior as on-startup page setup currently - we don't want the input to
> say "Not Valid" when the user had just chosen this option and haven't typed in
> anything yet.
> 
>
https://codereview.chromium.org/2766093002/diff/220001/chrome/browser/resourc...
> File chrome/browser/resources/settings/controls/settings_input.js (right):
> 
>
https://codereview.chromium.org/2766093002/diff/220001/chrome/browser/resourc...
> chrome/browser/resources/settings/controls/settings_input.js:123:
> this.resetValue_();
> On 2017/03/27 11:43:07, Dan Beam (slow) wrote:
> > if (event.key == 'Enter' && this.invalid)
> >   event.preventDefault();
> > else if (event.key == 'Escape')
> >   this.resetValue_();
> 
> Oops, Demetrios gave me this feedback too and I made a patch locally, but
> apparently left it out before landing.
> Uploading as a separate CL: https://codereview.chromium.org/2777343005

Powered by Google App Engine
This is Rietveld 408576698