Invalid URLs should not be added to startup page list.

Invalid URLs should not be added to startup page list.

BUG=100658
TEST=Try to add an invalid URL in start pages list in chrome://setttings. It should have no effect.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=106947

[rosen.dash, 2011-10-19 07:58:19]

We have to check validity before adding any URL to start up page list. This
prevents creating blank entries in chrome://settings page. This also prevents in
hitting DCHECK in debug build in GURL::spec() called from
BrowserOptionsHandler::OnModelChanged.

[jam, 2011-10-19 18:19:52]

I'm not familiar with this code, so I'm not a good reviewer

[csilv, 2011-10-19 18:49:48]

http://codereview.chromium.org/8349030/diff/1/chrome/browser/ui/webui/options...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

http://codereview.chromium.org/8349030/diff/1/chrome/browser/ui/webui/options...
chrome/browser/ui/webui/options/browser_options_handler.cc:387: if
(url.spec().empty())
I think url.is_valid() would be a better choice here.  I also think the comment
is unnecessary, adding the is_valid() check will be self documenting.

[rosen.dash, 2011-10-20 06:31:24]

Updated according to review comment.

http://codereview.chromium.org/8349030/diff/1/chrome/browser/ui/webui/options...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

http://codereview.chromium.org/8349030/diff/1/chrome/browser/ui/webui/options...
chrome/browser/ui/webui/options/browser_options_handler.cc:387: if
(url.spec().empty())
On 2011/10/19 18:49:48, csilv wrote:
> I think url.is_valid() would be a better choice here.  I also think the
comment
> is unnecessary, adding the is_valid() check will be self documenting.

Done.

[csilv, 2011-10-20 16:53:54]

lgtm

[commit-bot: I haz the power, 2011-10-20 19:40:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nqk836@motorola.com/8349030/6001

[commit-bot: I haz the power, 2011-10-20 19:40:23]

Presubmit check for 8349030-6001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
nqk836@motorola.com is not in AUTHORS file. If you are a new contributor, please
visit
http://www.chromium.org/developers/contributing-code and read the "Legal"
section
If you are a chromite, verify the contributor signed the CLA.

[csilv, 2011-10-20 19:53:32]

On 2011/10/20 19:40:23, I haz the power (commit-bot) wrote:
> Presubmit check for 8349030-6001 failed and returned exit status 1.
> 
> Running presubmit commit checks ...
> 
> ** Presubmit Warnings **
> mailto:nqk836@motorola.com is not in AUTHORS file. If you are a new
contributor, please
> visit
> http://www.chromium.org/developers/contributing-code and read the "Legal"
> section
> If you are a chromite, verify the contributor signed the CLA.

I'll can put the AUTHORS change in a separate CL.  Is it possible for you to
upload this CL from an account tied to rosen.dash@motorola.com?

[csilv, 2011-10-20 19:54:52]

(Alternatively, we can put you in the AUTHORS file as nqk836@motorola.com.)

[csilv, 2011-10-20 20:00:56]

On 2011/10/20 19:54:52, csilv wrote:
> (Alternatively, we can put you in the AUTHORS file as
mailto:nqk836@motorola.com.)

Rosen,

I recommend that you change your email address in the AUTHORS file to
nqk836@motorola.com.  That will allow this patch to successfully commit.  I have
already verified that you have signed the CLA.

[rosen.dash, 2011-10-21 06:19:32]

On 2011/10/20 20:00:56, csilv wrote:
> On 2011/10/20 19:54:52, csilv wrote:
> > (Alternatively, we can put you in the AUTHORS file as
> mailto:nqk836@motorola.com.)
> 
> Rosen,
> 
> I recommend that you change your email address in the AUTHORS file to
> mailto:nqk836@motorola.com.  That will allow this patch to successfully
commit.  I have
> already verified that you have signed the CLA.

Done.

[commit-bot: I haz the power, 2011-10-21 06:23:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nqk836@motorola.com/8349030/13001

[commit-bot: I haz the power, 2011-10-21 09:36:45]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is mac_rel, revision is 106695, job name
was 8349030-13001 (retry) (retry) (retry) (retry).

[commit-bot: I haz the power, 2011-10-21 17:03:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nqk836@motorola.com/8349030/13001

[commit-bot: I haz the power, 2011-10-21 17:03:14]

Can't apply patch for file AUTHORS.
While running patch -p1 --forward --force;
patching file AUTHORS
Hunk #1 FAILED at 138.
1 out of 1 hunk FAILED -- saving rejects to file AUTHORS.rej

[commit-bot: I haz the power, 2011-10-24 05:40:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nqk836@motorola.com/8349030/15002

[commit-bot: I haz the power, 2011-10-24 06:42:14]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is linux_rel, revision is 106890, job name
was 8349030-15002 (previous was lost) (previous was lost) (previous was lost)
(previous was lost).

[commit-bot: I haz the power, 2011-10-24 16:56:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nqk836@motorola.com/8349030/15002

[commit-bot: I haz the power, 2011-10-24 18:13:17]

Change committed as 106947