Show an infobar when the user is using an unsupported flag.

Show an infobar when the user is using an unsupported flag.

[viettrungluu, 2009-12-11 19:15:51]

Just drive-by comments, since I'm probably not the person to lgtm this.

http://codereview.chromium.org/490019/diff/2001/2002
File chrome/browser/browser_init.cc (right):

http://codereview.chromium.org/490019/diff/2001/2002#newcode259
chrome/browser/browser_init.cc:259: // Since our flag names are in English
anyway, I don't think we should
Surely people can use flags even if they don't understand English, and they need
the warning even more if they don't understand the flag names.

http://codereview.chromium.org/490019/diff/2001/2002#newcode262
chrome/browser/browser_init.cc:262: StringPrintf("You are using an unsupported
command-line flag: --%s.  "
I'm sceptical that there should be two spaces after the period.

http://codereview.chromium.org/490019/diff/2001/2002#newcode624
chrome/browser/browser_init.cc:624: if (i == 0 && process_startup) {
I would slightly prefer if it were "process_startup && i == 0" for reasons one
may be able to work out (there are two).

http://codereview.chromium.org/490019/diff/2001/2002#newcode663
chrome/browser/browser_init.cc:663: bad_flag = *flag;
I'm a little disappointed that you only warn of at most one flag (which is
evidently fine for the current list of bad flags), but that's okay.

[Evan Martin, 2009-12-11 19:18:45]

Thanks for looking!

http://codereview.chromium.org/490019/diff/2001/2002
File chrome/browser/browser_init.cc (right):

http://codereview.chromium.org/490019/diff/2001/2002#newcode262
chrome/browser/browser_init.cc:262: StringPrintf("You are using an unsupported
command-line flag: --%s.  "
On 2009/12/11 19:15:51, viettrungluu wrote:
> I'm sceptical that there should be two spaces after the period.

Religious debate!  :P
I will change it.  Habit.

http://codereview.chromium.org/490019/diff/2001/2002#newcode624
chrome/browser/browser_init.cc:624: if (i == 0 && process_startup) {
On 2009/12/11 19:15:51, viettrungluu wrote:
> I would slightly prefer if it were "process_startup && i == 0" for reasons one
> may be able to work out (there are two).

I was matching the existing code, which I think dominates any other reasoning
here (it's not performance-critical).  But I'll change it if you insist.

http://codereview.chromium.org/490019/diff/2001/2002#newcode663
chrome/browser/browser_init.cc:663: bad_flag = *flag;
On 2009/12/11 19:15:51, viettrungluu wrote:
> I'm a little disappointed that you only warn of at most one flag (which is
> evidently fine for the current list of bad flags), but that's okay.

You run out of space in the infobar otherwise.

[Mohamed Mansour, 2009-12-11 19:42:34]

http://codereview.chromium.org/490019/diff/2001/2002
File chrome/browser/browser_init.cc (right):

http://codereview.chromium.org/490019/diff/2001/2002#newcode663
chrome/browser/browser_init.cc:663: bad_flag = *flag;
On 2009/12/11 19:18:45, Evan Martin wrote:
> On 2009/12/11 19:15:51, viettrungluu wrote:
> > I'm a little disappointed that you only warn of at most one flag (which is
> > evidently fine for the current list of bad flags), but that's okay.
> 
> You run out of space in the infobar otherwise.

This is good that way, because we can put many BadFlagsInfoBar if necessary. One
for each flag. That will surely annoy the user :) which is of course full of
awesomeness.

[viettrungluu, 2009-12-11 19:45:39]

Another thing which occurred to me is that users may end up with an unsupported
flag and not know about it (or forget about it) ... and then be confused by this
infobar. We could provide a "link" to a support page or something.

In any case, this would be much better than what we have now, except in the
ignorance-is-bliss department.

http://codereview.chromium.org/490019/diff/2001/2002
File chrome/browser/browser_init.cc (right):

http://codereview.chromium.org/490019/diff/2001/2002#newcode624
chrome/browser/browser_init.cc:624: if (i == 0 && process_startup) {
On 2009/12/11 19:18:45, Evan Martin wrote:
> On 2009/12/11 19:15:51, viettrungluu wrote:
> > I would slightly prefer if it were "process_startup && i == 0" for reasons
one
> > may be able to work out (there are two).
> 
> I was matching the existing code, which I think dominates any other reasoning
> here (it's not performance-critical).  But I'll change it if you insist.

Fair enough. I definitely don't insist.

http://codereview.chromium.org/490019/diff/2001/2002#newcode663
chrome/browser/browser_init.cc:663: bad_flag = *flag;
On 2009/12/11 19:18:45, Evan Martin wrote:
> On 2009/12/11 19:15:51, viettrungluu wrote:
> > I'm a little disappointed that you only warn of at most one flag (which is
> > evidently fine for the current list of bad flags), but that's okay.
> 
> You run out of space in the infobar otherwise.

Good point.

[jorlow, 2010-03-01 18:03:11]

LGTM

Lets get this in.  I'm sorry we ended up wasting so much time trying to design
this thing by committee instead of just doing something and iterating.

[Evan Martin, 2010-03-01 18:03:52]

I muted all the threads.  Do I have some buy-in on this?

[jorlow, 2010-03-01 18:05:49]

All the other solutions have been shot down.  A while ago I sent a "unless
someone has a better idea, lets do Evan's patch" and no one said no.

On Mon, Mar 1, 2010 at 6:03 PM, <evan@chromium.org> wrote:

> I muted all the threads.  Do I have some buy-in on this?
>
>
> http://codereview.chromium.org/490019
>

[viettrungluu, 2010-03-01 18:08:08]

+1000.

(I'd still prefer the warning be localized, but as I said anything is better
than nothing.)

On 2010/03/01 18:03:11, Jeremy Orlow wrote:
> LGTM
> 
> Lets get this in.  I'm sorry we ended up wasting so much time trying to design
> this thing by committee instead of just doing something and iterating.

[tony, 2010-03-02 02:23:57]

(drive by nits, feel free to ignore)
I also support localizing the string.  Also, it would be nice if all the info
bars in the file were moved to a separate .h file.  browser_init.cc has so much
random stuff in it already.