Invalid URLs are no longer mangled when reopening the Options window

Invalid URLs are no longer mangled when reopening the Options window.

If the homepage URL is changed to an invalid URL, the homepage preference is swapped to a blank URL and NTP is shown.

BUG=40996
TEST=Set homepage to http://www.google.com, close Chromium, restart Chromium and see homepage set to http://www.google.com. Set homepage to http://, close Chromium, restart Chromium and see homepage set to New Tab Page.

Patch by Jared Wein <weinjared@gmail.com>;

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

[weinjared, 2010-05-23 07:16:54]

On 2010/05/23 07:14:56, weinjared wrote:
> 

The first two patch sets are due to my misunderstanding of how the gcl upload
worked. Apologies in advance.

[Mohamed Mansour, 2010-05-23 08:02:04]

http://codereview.chromium.org/2102019/diff/6001/7002
File chrome/browser/views/options/general_page_view.cc (right):

http://codereview.chromium.org/2102019/diff/6001/7002#newcode291
chrome/browser/views/options/general_page_view.cc:291: if
(GURL(url_string).is_valid()) {
nit: remove the curly brackets since its just one line conditional:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Conditionals

http://codereview.chromium.org/2102019/diff/6001/7002#newcode294
chrome/browser/views/options/general_page_view.cc:294: homepage_.SetValue(L"");
I guess this is ok, but the functionality has been changed now. Before, if the
URL isn't valid, it will rollback to the previous valid URL. But in this case,
it will always be an empty URL.

The reason why it is showing a http://http/ is because "http" is a valid URL so
when you type "http", the URLFixerUpper should convert that to http://http/ and
thats the last valid URL. When the URL is invalid, I assume it will rollback to
that.

I don't know what we would need here, should we from now on assume all bad URL's
will be empty, or treat it as it was before and rollback to the last valid entry
(which seems more user friendly).

[weinjared, 2010-05-23 13:22:54]

On 2010/05/23 08:02:04, Mohamed Mansour wrote:
> http://codereview.chromium.org/2102019/diff/6001/7002
> File chrome/browser/views/options/general_page_view.cc (right):
> 
> http://codereview.chromium.org/2102019/diff/6001/7002#newcode291
> chrome/browser/views/options/general_page_view.cc:291: if
> (GURL(url_string).is_valid()) {
> nit: remove the curly brackets since its just one line conditional:
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Conditionals
> 
> http://codereview.chromium.org/2102019/diff/6001/7002#newcode294
> chrome/browser/views/options/general_page_view.cc:294:
homepage_.SetValue(L"");
> I guess this is ok, but the functionality has been changed now. Before, if the
> URL isn't valid, it will rollback to the previous valid URL. But in this case,
> it will always be an empty URL.
> 
> The reason why it is showing a http://http/ is because "http" is a valid URL
so
> when you type "http", the URLFixerUpper should convert that to http://http/
and
> thats the last valid URL. When the URL is invalid, I assume it will rollback
to
> that.
> 
> I don't know what we would need here, should we from now on assume all bad
URL's
> will be empty, or treat it as it was before and rollback to the last valid
entry
> (which seems more user friendly).

Yes, you are right. The fix almost seems worse than the problem it is trying to
solve. I should mention that I covered this bug in more detail within the actual
issue page. Here is what I had submitted:

This bug appears to happen because each keystroke is committed to the
preferences. When 
the user enters in the colon and slashes, they URL is now an invalid URL and it
is not committed.

http://ftp/ and http://https/ show up because URLFixerUpper::FixupURL has
interpreted 
the raw 'ftp' and 'https' as hostnames and assigned them a generic scheme of
http.

This bug will _not_ be reproduced by pasting "http:// in the textbox over a
valid URL 
like "http://www.google.com". There must be an event where the URL *might* be
valid 
(such as entering in the scheme as separate keystrokes).

I am open to any and all suggestions about how this functionality should be
handled.

[Finnur, 2010-05-23 15:45:13]

http://codereview.chromium.org/2102019/diff/6001/7002
File chrome/browser/views/options/general_page_view.cc (right):

http://codereview.chromium.org/2102019/diff/6001/7002#newcode294
chrome/browser/views/options/general_page_view.cc:294: homepage_.SetValue(L"");
I wonder if we should in the else clause (when we know the url is invalid) just
do:

if (url_string equals one the major protocols, such as http)
  SetHomepage(L"");

... with a comment explaining why we do this.

[Peter Kasting, 2010-05-23 16:04:45]

This patch is the wrong fix.

The right fix is to not update the pref until the user closes the options
dialog.  At the time the user closes the dialog, an invalid URL (that can't be
fixed up) should be discarded and not result in a change to the underlying pref.

[Finnur, 2010-05-23 16:10:58]

Yeah, that's probably the best way forward...

[Nico, 2010-05-23 17:05:37]

On OS x, the preferred needs to be updated immediately, so this might still
be necessary (haven't actually looked at the patch. Sent from my phone etc.)

On May 23, 2010 9:04 AM, <pkasting@chromium.org> wrote:

This patch is the wrong fix.

The right fix is to not update the pref until the user closes the options
dialog.  At the time the user closes the dialog, an invalid URL (that can't
be
fixed up) should be discarded and not result in a change to the underlying
pref.



http://codereview.chromium.org/2102019/show

[Mohamed Mansour, 2010-05-23 17:14:10]

Yea, in Windows, everything in options is updated immediately, so I don't know
how the prefs should be persisted when closing, unless you are overriding
"WindowClosing". But no one does that and the feeling wont be instantaneous like
the rest.

Perhaps we shouldn't mark it as a bug since it makes sense for it to append
http.

[Peter Kasting, 2010-05-23 19:32:04]

On 2010/05/23 16:04:45, Peter Kasting wrote:
> This patch is the wrong fix.
> 
> The right fix is to not update the pref until the user closes the options
> dialog.  At the time the user closes the dialog, an invalid URL (that can't be
> fixed up) should be discarded and not result in a change to the underlying
pref.

After more thought, I was wrong.  The current patch is more consistent with how
we immediately update all the other prefs.  So stick with that method.

Two nits: Remove {}, and change L"" to std::wstring().

[tfarina, 2010-05-23 20:21:04]

Please, could you fix the linux side as well?

[Peter Kasting, 2010-05-23 23:27:51]

On 2010/05/23 20:21:04, tfarina wrote:
> Please, could you fix the linux side as well?

Oh yeah, I forgot to ask if Mac and Linux needed fixes.  Either those should be
in this patch or we should update the bug to be clear about what's not done once
we check this in.

[weinjared, 2010-05-24 01:48:12]

On 2010/05/23 23:27:51, Peter Kasting wrote:
> On 2010/05/23 20:21:04, tfarina wrote:
> > Please, could you fix the linux side as well?
> 
> Oh yeah, I forgot to ask if Mac and Linux needed fixes.  Either those should
be
> in this patch or we should update the bug to be clear about what's not done
once
> we check this in.

I've made the style changes and ported the change over to the Linux and Mac
builds as well (although I was not able to test those changes). The changes can
be found in Patch 4.

[tfarina, 2010-05-24 02:27:53]

http://codereview.chromium.org/2102019/diff/19001/13004
File chrome/browser/gtk/options/general_page_gtk.cc (right):

http://codereview.chromium.org/2102019/diff/19001/13004#newcode643
chrome/browser/gtk/options/general_page_gtk.cc:643:
homepage_.SetValue(std::wstring());
no, this should be in SetHomepage, which already verify if the url is valid.

[weinjared, 2010-05-24 02:37:40]

On 2010/05/24 02:27:53, tfarina wrote:
> File chrome/browser/gtk/options/general_page_gtk.cc (right):
> no, this should be in SetHomepage, which already verify if the url is valid.

Thanks for catching that, I've updated it and the new changes are in Patch Set
5.

[Mohamed Mansour, 2010-05-24 02:51:02]

Thanks Jared! Everyone, we met with Jared at I/O and was really interested in
contributing, so it is his first patch.

BTW Jared, if you don't have Linux or Mac, and cannot test this on those
machines, maybe someone else could do those areas. If your familiar with those
platforms, I can run them by the try bot.

Basically, you can follow the same approach as whats been done in Linux, they
use setHomepage(GURL) directly since it already checks for an invalid URL, but
missing the empty string for homepage_.

http://codereview.chromium.org/2102019/diff/7003/23002
File chrome/browser/cocoa/preferences_window_controller.mm (right):

http://codereview.chromium.org/2102019/diff/7003/23002#newcode1017
chrome/browser/cocoa/preferences_window_controller.mm:1017: if
(GURL(fixedURL).is_valid())
Follow same approach as Linux/Windows (where we remove this conditional) and
call setHomepage directly.

http://codereview.chromium.org/2102019/diff/7003/23004
File chrome/browser/views/options/general_page_view.cc (right):

http://codereview.chromium.org/2102019/diff/7003/23004#newcode235
chrome/browser/views/options/general_page_view.cc:235:
SetHomepage(GetNewTabUIURLString());
Change to: SetHomepage(GURL());

http://codereview.chromium.org/2102019/diff/7003/23004#newcode291
chrome/browser/views/options/general_page_view.cc:291: if
(GURL(url_string).is_valid())
You can remove all this and add the empty string inside the SetHomepage and just
do:
SetHomepage(GURL(url_string));

http://codereview.chromium.org/2102019/diff/7003/23004#newcode752
chrome/browser/views/options/general_page_view.cc:752:
new_tab_page_is_home_page_.SetValue(true);
Add after this:
homepage_.SetValue(std::wstring());

[weinjared, 2010-05-24 02:54:48]

I've vaguely familiar with Mac (participate in obj-c code reviews at work) and
the linux code seemed similar enough, just a different factoring of the code. 

You can feel free to run it through the try bot. I wish there were some unit
tests around this change, but I didn't see where I could put the tests. Any and
all ideas will be greatly appreciated.

[weinjared, 2010-05-24 03:05:29]

On 2010/05/24 02:51:02, Mohamed Mansour wrote:
> Follow same approach as Linux/Windows (where we remove this conditional) and
> call setHomepage directly.

Making this change would mean that if the homepage is set to NewTab page, the
old value will not be preserved. This is in contrary to the comment in
general_page_view.h:109

[Mohamed Mansour, 2010-05-24 03:14:38]

On 2010/05/24 03:05:29, weinjared wrote:
> Making this change would mean that if the homepage is set to NewTab page, the
> old value will not be preserved. This is in contrary to the comment in
> general_page_view.h:109

Okay, sounds good then, Peter / Finnur can give the last LGTM.

[tfarina, 2010-05-24 03:18:54]

On 2010/05/24 03:14:38, Mohamed Mansour wrote:
> On 2010/05/24 03:05:29, weinjared wrote:
> > Making this change would mean that if the homepage is set to NewTab page,
the
> > old value will not be preserved. This is in contrary to the comment in
> > general_page_view.h:109
> 
> Okay, sounds good then, Peter / Finnur can give the last LGTM.
The linux change, LGTM.

[Finnur, 2010-05-24 04:53:26]

I´m still worried about what conditions can trigger an invalid URL, causing the
homepage to be silently set to the new tab page. Given we don't want to delay
the save (as Peter originally suggested), I think my comment still stands. Have
you considered what I mentioned in my first reply?

[weinjared, 2010-05-24 11:11:12]

On 2010/05/24 04:53:26, Finnur wrote:
> Have you considered what I mentioned in my first reply?

Yeah I have. I think that might be a good compromise and something that will be
less extreme of a change. I can imagine somebody typing in a URL and
fat-fingering a couple characters causing an invalid URL in some way, then
testing it by using the home page button, then returning and finding that what
they typed is gone. That could be a poor user experience.

Would we only want to support standard schemes then for this change? Those seem
the safest to me.

[Finnur, 2010-05-24 14:55:50]

> Would we only want to support standard schemes then

Yes, http, ftp, and https. I don't think we care about this in the context of
non-standard schemes.

[Peter Kasting, 2010-05-24 17:43:32]

I am strongly opposed to Finnur's suggestion.  This should be checked in as-is. 
LGTM on the code as it stands.

[Finnur, 2010-05-24 17:46:03]

I'll defer to Peter. I don't have a strong opinion on this.

[Peter Kasting, 2010-05-24 18:39:37]

My main concern is consistency.  If we check for "http://", then if the user
just types "http:", we'll leave the homepage set to "http".  But if we check
for "http:", then if the user deletes from an existing URL down to just
"http://", he'll be left with a homepage set to a one-character hostname.
And in either case, typing one kind of invalid URL will reset things, but
another won't.

Am I missing something?  I haven't thought through every possible case,
maybe this is actually a good idea?  Help me out here...

PK

On May 24, 2010 10:46 AM, <finnur@chromium.org> wrote:
> I'll defer to Peter. I don't have a strong opinion on this.
>
> http://codereview.chromium.org/2102019/show

[Finnur, 2010-05-24 19:28:29]

How about this: 

Do what you currently do, but only set it to blank if the url invalid _and_ the
host is non-null?

That should blank our the homepage if you write:

http:
http:/
http://

... but as soon as you start typing the host or anything else that comes after
it any mistake is simply ignored (and not blanked out)?

[Finnur, 2010-05-24 19:29:00]

That was supposed to read: if the url _is_ invalid.

On 2010/05/24 19:28:29, Finnur wrote:
> How about this: 
> 
> Do what you currently do, but only set it to blank if the url invalid _and_
the
> host is non-null?
> 
> That should blank our the homepage if you write:
> 
> http:
> http:/
> http://
> 
> ... but as soon as you start typing the host or anything else that comes after
> it any mistake is simply ignored (and not blanked out)?

[Peter Kasting, 2010-05-24 19:34:43]

Did you mean "host _is_ null"?

I still don't know whether this is a great idea... it just seems strange
that sometimes invalid URLs clear the field and sometimes they don't.

PK

On May 24, 2010 12:28 PM, <finnur@chromium.org> wrote:
> How about this:
>
> Do what you currently do, but only set it to blank if the url invalid
_and_
> the
> host is non-null?
>
> That should blank our the homepage if you write:
>
> http:
> http:/
> http://
>
> ... but as soon as you start typing the host or anything else that comes
> after
> it any mistake is simply ignored (and not blanked out)?
>
>
> http://codereview.chromium.org/2102019/show

[Finnur, 2010-05-24 19:55:15]

> Did you mean "host _is_ null"?

Yes. Jeez, I can't type today. :/

if the url is invalid {
  if the host is null 
    blank the homepage value out
}
 
> I still don't know whether this is a great idea... it just
> seems strange that sometimes invalid URLs clear the field
> and sometimes they don't.

We have the weirdness problem today, we have it with the changelist as-is and my
proposed change doesn't affect that. 

I think silently ignoring changes that are invalid is always going to lead to
these weird scenarios. There is always going to be some guesswork for the user,
unless we come up with some more guidance while typing, like the exclamation
point that we have in the Edit Search Engine dialog.

I think "http:", "http:/", "http://" are all obvious to the user where the
homepage value is missing and I don't think they'll be surprised that we blank
it out.

On the other hand, they would probably be surprised if we blank out their
homepage value because they added some weird character at the end of the url, or
something.

(shrug)

[Peter Kasting, 2010-05-25 00:05:51]

On Mon, May 24, 2010 at 12:55 PM, <finnur@chromium.org> wrote:

> I think "http:", "http:/", "http://" are all obvious to the user where the
> homepage value is missing and I don't think they'll be surprised that we
> blank
> it out.
>
> On the other hand, they would probably be surprised if we blank out their
> homepage value because they added some weird character at the end of the
> url, or
> something.
>

Hmm... OK, well, let's try it then.

http://codereview.chromium.org/2102019/show
>

[Peter Kasting, 2010-05-25 03:19:45]

LGTM.  I'm sorry we've run you around so much on this patch.  Normally we're a
lot more sane than that, but in this case I think we just hadn't thought a lot
about the issue.

http://codereview.chromium.org/2102019/diff/34001/35002
File chrome/browser/cocoa/preferences_window_controller.mm (right):

http://codereview.chromium.org/2102019/diff/34001/35002#newcode1018
chrome/browser/cocoa/preferences_window_controller.mm:1018: GURL
fixedURL(URLFixerUpper::FixupURL(unfixedURL, std::string()));
Nit: Could probably just be
[self setHomepage:GURL(URLFixerUpper::...

http://codereview.chromium.org/2102019/diff/34001/35004
File chrome/browser/views/options/general_page_view.cc (right):

http://codereview.chromium.org/2102019/diff/34001/35004#newcode290
chrome/browser/views/options/general_page_view.cc:290: std::string url_string =
URLFixerUpper::FixupURL(
Nit: Could just be
SetHomepage(GURL(URLFixerUpper::...

http://codereview.chromium.org/2102019/diff/34001/35005
File chrome/browser/views/options/general_page_view.h (right):

http://codereview.chromium.org/2102019/diff/34001/35005#newcode1
chrome/browser/views/options/general_page_view.h:1: // Copyright (c) 2009 The
Chromium Authors. All rights reserved.
Nit: 2010

[weinjared, 2010-05-25 03:29:44]

Thanks pkasting. I've made the changes you recommended. I also updated some
comments that I had changed earlier but didn't update when the decision was made
to check for null-hostnames.

[Finnur, 2010-05-25 03:48:52]

LGTM

[Mohamed Mansour, 2010-05-25 04:14:01]

LGTM, thanks for submitting your first patch :) Looking forward for more! I
submitted this to the Try job and if it passes, I can/or a Googler can commit
this for you tomorrow. Did you sign the CLA (Committer license agreement)?

[weinjared, 2010-05-25 04:17:31]

On 2010/05/25 04:14:01, Mohamed Mansour wrote:
> Did you sign the CLA (Committer license agreement)?

Yeah I signed the CLA. I'm just glad to contribute to such a great product!

[weinjared, 2010-05-25 04:37:54]

Sorry about the try-failure. Just did a gclient sync and also noticed that the
general_page_gtk.h file wasn't being uploaded with the CL. @mhm, can you submit
it for the try-server again?

[weinjared, 2010-05-25 13:04:02]

The try build for the mac failed. I've noted the line that caused the failure
and will fix it tonight.

http://codereview.chromium.org/2102019/diff/47001/48002
File chrome/browser/cocoa/preferences_window_controller.mm (right):

http://codereview.chromium.org/2102019/diff/47001/48002#newcode806
chrome/browser/cocoa/preferences_window_controller.mm:806:
UTF8ToWide(homepage.spec()) == GetNewTabUIURLString()) {
Remove the UTF8ToWide because GetNewTabUIURLString returns a std::string and not
a std::wstring

[weinjared, 2010-05-25 22:24:53]

I have fixed the error with the mac build.

[weinjared, 2010-05-26 01:25:21]

The try builds failed on Windows and Linux, but they look to be independent of
my changes.

Can this CL be committed or should I continue to send it to the try server until
they are all green?

[Peter Kasting, 2010-05-26 04:28:53]

On 2010/05/26 01:25:21, weinjared wrote:
> The try builds failed on Windows and Linux, but they look to be independent of
> my changes.

Linux is definitely fine. Windows isn't your fault, it's sad the bot is totally
hosed though.  However, since you had a clean try run on Windows with the
previous patch and the only change is on Mac, this should be fine.

If no one lands this before tomorrow I will do it.  And I hope your next
patching experience goes more easily :)

[Mohamed Mansour, 2010-05-26 13:24:01]

Patch committed! Thanks Jared! As Peter stated, we hope your next patching
experience goes more easily :) I am impressed you got your hands in the Chromium
source base after a couple days of I/O. Good job! Welcome to Chromium :)