MD Settings: validate startup URLs

MD Settings: validate startup URLs

R=dschuyler@chromium.org
BUG=531750
Committed: https://crrev.com/bd8f69639d3bc5150f557fb5ed9a13c8408dbd6f
Cr-Commit-Position: refs/heads/master@{#384786}

[Dan Beam, 2016-03-31 18:17:18]

Description was changed from

==========
MD Settings: validate startup URLs

R=dschuyler@chromium.org
BUG=399866,531750
==========

to

==========
MD Settings: validate startup URLs

R=dpapad@chromium.org
BUG=399866,531750
==========

[Dan Beam, 2016-03-31 18:17:18]

dbeam@chromium.org changed reviewers:
+ dpapad@chromium.org
- dschuyler@chromium.org

[dpapad, 2016-03-31 18:39:57]

https://codereview.chromium.org/1849543006/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/on_startup_page/startup_urls_page.js
(right):

https://codereview.chromium.org/1849543006/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/on_startup_page/startup_urls_page.js:50:
isNewUrlValid_: {
Nit(optional): When the default value is false you can shorten more

isNewUrlValid_ : Boolean,

https://codereview.chromium.org/1849543006/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/on_startup_page/startup_urls_page.js:105:
this.isNewUrlValid_ = valid;
Nit (optional): s/valid/isValid

https://codereview.chromium.org/1849543006/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/settings/on_startup_page/startup_urls_page_browser_proxy.js
(right):

https://codereview.chromium.org/1849543006/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/on_startup_page/startup_urls_page_browser_proxy.js:16:
* @return {Promise<boolean>} Whether the URL is valid.
!Promise<boolean>

https://codereview.chromium.org/1849543006/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/on_startup_page/startup_urls_page_browser_proxy.js:48:
if (url.trim().length == 0)
Nit (optional):

return url.trim().length == 0 ?
    Promise.resolve(false) :
    cr.sendWithPromise('validateStartupPage', url);

https://codereview.chromium.org/1849543006/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/settings_startup_pages_handler.cc (right):

https://codereview.chromium.org/1849543006/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/settings_startup_pages_handler.cc:152: const
base::ListValue* args) {
CHECK_EQ(2U, args->GetSize()); (and possibly in other places where this is
missing in this file?)

https://codereview.chromium.org/1849543006/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/settings_startup_pages_handler.cc:157:
args->GetString(1, &url_string);
CHECK(args->GetString(1, &url_string));

^This is the pattern I've encountered in most other handlers.

[Dan Beam, 2016-03-31 18:42:50]

wasn't published yet ;)

[Dan Beam, 2016-03-31 18:51:14]

Description was changed from

==========
MD Settings: validate startup URLs

R=dpapad@chromium.org
BUG=399866,531750
==========

to

==========
MD Settings: validate startup URLs

R=dschuyler@chromium.org
BUG=399866,531750
==========

[Dan Beam, 2016-03-31 18:51:14]

dbeam@chromium.org changed reviewers:
+ dschuyler@chromium.org
- dpapad@chromium.org

[Dan Beam, 2016-03-31 19:06:31]

Patchset #4 (id:60001) has been deleted

[Dan Beam, 2016-03-31 19:06:40]

Patchset #3 (id:40001) has been deleted

[Dan Beam, 2016-03-31 19:06:50]

Patchset #1 (id:1) has been deleted

[Dan Beam, 2016-03-31 19:06:59]

Description was changed from

==========
MD Settings: validate startup URLs

R=dschuyler@chromium.org
BUG=399866,531750
==========

to

==========
MD Settings: validate startup URLs

R=dschuyler@chromium.org
BUG=531750
==========

[dschuyler, 2016-03-31 20:29:26]

https://codereview.chromium.org/1849543006/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/on_startup_page/startup_urls_page.js
(right):

https://codereview.chromium.org/1849543006/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/on_startup_page/startup_urls_page.js:106:
newUrlChanged_: function(newUrl) {
There looks to be a small race condition here.
I think it can be avoided by setting isNewUrlValid_
to false here, before doing the validation.
The race I'm thinking of is having a valid URL
and then making it invalid and adding it before
the promise comes back.

https://codereview.chromium.org/1849543006/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/settings_startup_pages_handler.cc (right):

https://codereview.chromium.org/1849543006/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/settings_startup_pages_handler.cc:92:
NOTREACHED();
Would these be ok as a DLOG instead of NOTREACHED()?

[Dan Beam, 2016-03-31 22:06:13]

Patchset #4 (id:120001) has been deleted

[Dan Beam, 2016-03-31 22:06:21]

Patchset #3 (id:100001) has been deleted

[Dan Beam, 2016-03-31 22:06:55]

https://codereview.chromium.org/1849543006/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/on_startup_page/startup_urls_page.js
(right):

https://codereview.chromium.org/1849543006/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/on_startup_page/startup_urls_page.js:50:
isNewUrlValid_: {
On 2016/03/31 18:39:56, dpapad wrote:
> Nit(optional): When the default value is false you can shorten more
> 
> isNewUrlValid_ : Boolean,

i wouldn't encourage this, as one might assume the default is false, but it's
not crystal clear from looking at this site

https://codereview.chromium.org/1849543006/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/on_startup_page/startup_urls_page.js:105:
this.isNewUrlValid_ = valid;
On 2016/03/31 18:39:56, dpapad wrote:
> Nit (optional): s/valid/isValid

Done.

https://codereview.chromium.org/1849543006/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/settings/on_startup_page/startup_urls_page_browser_proxy.js
(right):

https://codereview.chromium.org/1849543006/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/on_startup_page/startup_urls_page_browser_proxy.js:16:
* @return {Promise<boolean>} Whether the URL is valid.
On 2016/03/31 18:39:56, dpapad wrote:
> !Promise<boolean>

Done.

https://codereview.chromium.org/1849543006/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/on_startup_page/startup_urls_page_browser_proxy.js:48:
if (url.trim().length == 0)
On 2016/03/31 18:39:56, dpapad wrote:
> Nit (optional):
> 
> return url.trim().length == 0 ?
>     Promise.resolve(false) :
>     cr.sendWithPromise('validateStartupPage', url);

Done. (but not clearer, imo, and doesn't shave lines)

https://codereview.chromium.org/1849543006/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/settings_startup_pages_handler.cc (right):

https://codereview.chromium.org/1849543006/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/settings_startup_pages_handler.cc:152: const
base::ListValue* args) {
On 2016/03/31 18:39:56, dpapad wrote:
> CHECK_EQ(2U, args->GetSize()); (and possibly in other places where this is
> missing in this file?)

ehhhhhhhhhhhhhhhhh, i'll add it here

https://codereview.chromium.org/1849543006/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/settings_startup_pages_handler.cc:157:
args->GetString(1, &url_string);
On 2016/03/31 18:39:56, dpapad wrote:
> CHECK(args->GetString(1, &url_string));
> 
> ^This is the pattern I've encountered in most other handlers.

ehhhhhhhhhh, ok

https://codereview.chromium.org/1849543006/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/on_startup_page/startup_urls_page.js
(right):

https://codereview.chromium.org/1849543006/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/on_startup_page/startup_urls_page.js:106:
newUrlChanged_: function(newUrl) {
On 2016/03/31 20:29:25, dschuyler wrote:
> There looks to be a small race condition here.
> I think it can be avoided by setting isNewUrlValid_
> to false here, before doing the validation.
> The race I'm thinking of is having a valid URL
> and then making it invalid and adding it before
> the promise comes back.

Done.

https://codereview.chromium.org/1849543006/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/settings_startup_pages_handler.cc (right):

https://codereview.chromium.org/1849543006/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/settings_startup_pages_handler.cc:92:
NOTREACHED();
On 2016/03/31 20:29:26, dschuyler wrote:
> Would these be ok as a DLOG instead of NOTREACHED()?

this should never be reached as the user shouldn't be able to add an invalid URL
in the new UI (the [ ADD ] button should be disabled)

[Dan Beam, 2016-04-01 05:48:13]

Patchset #4 (id:160001) has been deleted

[Dan Beam, 2016-04-01 05:48:35]

Patchset #3 (id:140001) has been deleted

[Dan Beam, 2016-04-01 23:24:33]

ping dschuyler

[dschuyler, 2016-04-02 00:36:07]

lgtm

[Dan Beam, 2016-04-02 01:32:24]

The CQ bit was checked by dbeam@chromium.org

[commit-bot: I haz the power, 2016-04-02 01:32:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1849543006/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1849543006/180001

[commit-bot: I haz the power, 2016-04-02 02:48:14]

Description was changed from

==========
MD Settings: validate startup URLs

R=dschuyler@chromium.org
BUG=531750
==========

to

==========
MD Settings: validate startup URLs

R=dschuyler@chromium.org
BUG=531750
==========

[commit-bot: I haz the power, 2016-04-02 02:48:15]

Committed patchset #3 (id:180001)

[commit-bot: I haz the power, 2016-04-02 02:49:50]

Description was changed from

==========
MD Settings: validate startup URLs

R=dschuyler@chromium.org
BUG=531750
==========

to

==========
MD Settings: validate startup URLs

R=dschuyler@chromium.org
BUG=531750

Committed: https://crrev.com/bd8f69639d3bc5150f557fb5ed9a13c8408dbd6f
Cr-Commit-Position: refs/heads/master@{#384786}
==========

[commit-bot: I haz the power, 2016-04-02 02:49:52]

Patchset 3 (id:??) landed as
https://crrev.com/bd8f69639d3bc5150f557fb5ed9a13c8408dbd6f
Cr-Commit-Position: refs/heads/master@{#384786}