Remove bogus DCHECK for retail startup pages.

Remove bogus DCHECK for retail startup pages.

The DCHECK was failing without anybody noticing, presumably for a long
time, so just remove it.

BUG=None

[Mattias Nissler (ping if slow), 2013-09-27 15:19:03]

Please review.

[pastarmovj, 2013-09-27 15:47:59]

lgtm

[pastarmovj, 2013-09-27 15:48:25]

one nit.

https://chromiumcodereview.appspot.com/25073002/diff/1/chrome/browser/ui/star...
File chrome/browser/ui/startup/startup_browser_creator.cc (right):

https://chromiumcodereview.appspot.com/25073002/diff/1/chrome/browser/ui/star...
chrome/browser/ui/startup/startup_browser_creator.cc:369: if
(chromeos::KioskModeSettings::Get()->IsKioskModeEnabled()) {
No need for { }.

[Mattias Nissler (ping if slow), 2013-09-27 15:52:13]

+sky for chrome/browser/ui/OWNERS

https://chromiumcodereview.appspot.com/25073002/diff/1/chrome/browser/ui/star...
File chrome/browser/ui/startup/startup_browser_creator.cc (right):

https://chromiumcodereview.appspot.com/25073002/diff/1/chrome/browser/ui/star...
chrome/browser/ui/startup/startup_browser_creator.cc:369: if
(chromeos::KioskModeSettings::Get()->IsKioskModeEnabled()) {
On 2013/09/27 15:48:25, pastarmovj wrote:
> No need for { }.

Done.

[sky, 2013-09-27 17:01:15]

https://chromiumcodereview.appspot.com/25073002/diff/10001/chrome/browser/ui/...
File chrome/browser/ui/startup/startup_browser_creator.cc (right):

https://chromiumcodereview.appspot.com/25073002/diff/10001/chrome/browser/ui/...
chrome/browser/ui/startup/startup_browser_creator.cc:370: pref.type =
SessionStartupPref::DEFAULT;
Why is it a problem to restore the last session here? I see you comment, but
that reads as though the bug is session restore not handling additional urls,
which it should.

[Mattias Nissler (ping if slow), 2013-09-27 17:07:30]

https://chromiumcodereview.appspot.com/25073002/diff/10001/chrome/browser/ui/...
File chrome/browser/ui/startup/startup_browser_creator.cc (right):

https://chromiumcodereview.appspot.com/25073002/diff/10001/chrome/browser/ui/...
chrome/browser/ui/startup/startup_browser_creator.cc:370: pref.type =
SessionStartupPref::DEFAULT;
On 2013/09/27 17:01:15, sky wrote:
> Why is it a problem to restore the last session here? I see you comment, but
> that reads as though the bug is session restore not handling additional urls,
> which it should.

I don't know TBH. Julian, can you please comment?

[pastarmovj, 2013-09-30 08:53:33]

Marja do you mind chiming in on this as well? :)

Why does session restore ignore startup urls on ChromeOS (and maybe other
platforms too)?

https://chromiumcodereview.appspot.com/25073002/diff/10001/chrome/browser/ui/...
File chrome/browser/ui/startup/startup_browser_creator.cc (right):

https://chromiumcodereview.appspot.com/25073002/diff/10001/chrome/browser/ui/...
chrome/browser/ui/startup/startup_browser_creator.cc:370: pref.type =
SessionStartupPref::DEFAULT;
On 2013/09/27 17:07:30, Mattias Nissler wrote:
> On 2013/09/27 17:01:15, sky wrote:
> > Why is it a problem to restore the last session here? I see you comment, but
> > that reads as though the bug is session restore not handling additional
urls,
> > which it should.
> 
> I don't know TBH. Julian, can you please comment?

When the new smarter session restore was implemented it ignores the urls added
to the command line at least on ChromeOS. I can remember I discussed this with
Marja and I was convinced back then that it was not a bug but a feature and
therefore the check was put here instead. However I will add her to comment why
was this so because I don't remember myself anymore.

[marja, 2013-09-30 11:30:06]

https://chromiumcodereview.appspot.com/25073002/diff/10001/chrome/browser/ui/...
File chrome/browser/ui/startup/startup_browser_creator.cc (right):

https://chromiumcodereview.appspot.com/25073002/diff/10001/chrome/browser/ui/...
chrome/browser/ui/startup/startup_browser_creator.cc:370: pref.type =
SessionStartupPref::DEFAULT;
Hmm.

1) If your startup pref is "last", and you pass command line URLs, chrome
*should* open both the last open tabs and the command line URLs. This works on
Linux just fine. Does this not work on ChromeOS?

2) However, the comment in the deleted part of the code talks about
*startup_urls policy*. To me this sounds like the startup pref should be "open
these specified urls" in this case. Why is it not?

I find it suspicious that this CL changes the comment which explains why this
line of code is needed ;)

[Mattias Nissler (ping if slow), 2013-09-30 12:03:58]

https://chromiumcodereview.appspot.com/25073002/diff/10001/chrome/browser/ui/...
File chrome/browser/ui/startup/startup_browser_creator.cc (right):

https://chromiumcodereview.appspot.com/25073002/diff/10001/chrome/browser/ui/...
chrome/browser/ui/startup/startup_browser_creator.cc:370: pref.type =
SessionStartupPref::DEFAULT;
On 2013/09/30 11:30:06, marja wrote:
> Hmm.
> 
> 1) If your startup pref is "last", and you pass command line URLs, chrome
> *should* open both the last open tabs and the command line URLs. This works on
> Linux just fine. Does this not work on ChromeOS?
> 
> 2) However, the comment in the deleted part of the code talks about
> *startup_urls policy*. To me this sounds like the startup pref should be "open
> these specified urls" in this case. Why is it not?
> 
> I find it suspicious that this CL changes the comment which explains why this
> line of code is needed ;)

FWIW, I didn't change any code, just trying to get rid of a consistently failing
DCHECK and adjusting a comment to be actually helpful. The fact that nobody
seems to know what's going on here is a telltale sign for the previous comment
not being helpful. If the new comment is not accurate, then the entire logic
here probably doesn't make sense. If that's the conclusion, I can also just file
a bug against pastarmovj@ to find out what's really needed here.

[marja, 2013-09-30 12:34:26]

I read the new comment as "URLs passed as command line params only work if the
startup pref is "default"", and that sounds weird (at least, it's not true on
Linux, I don't have any data it would be true on ChromeOS - does somebody else)?

I think there are two things here:
- removing the DCHECK makes perfect sense here, since... why would the startup
pref be "last" here, I don't see any reason? :)
- the comment which explains why we need to change the startup pref to
"default". It also makes sense to enforce the startup pref is default (we don't
want to restore the last session on a kiosk) - is this the place where it is
enforced, or do you do it via policy or something else?

[marja, 2013-09-30 12:35:51]

> I think there are two things here:
> - removing the DCHECK makes perfect sense here, since... why would the startup
> pref be "last" here, I don't see any reason? :)
> - the comment which explains why we need to change the startup pref to
> "default". It also makes sense to enforce the startup pref is default (we
don't
> want to restore the last session on a kiosk) - is this the place where it is
> enforced, or do you do it via policy or something else?

Hmmh, what I wanted to say was: Changing it to default is needed either because
else something (what exactly?) doesn't work, or because it's the right thing to
do. Which one is this?

[Mattias Nissler (ping if slow), 2013-09-30 13:05:53]

On 2013/09/30 12:35:51, marja wrote:
> > I think there are two things here:
> > - removing the DCHECK makes perfect sense here, since... why would the
startup
> > pref be "last" here, I don't see any reason? :)
> > - the comment which explains why we need to change the startup pref to
> > "default". It also makes sense to enforce the startup pref is default (we
> don't
> > want to restore the last session on a kiosk) - is this the place where it is
> > enforced, or do you do it via policy or something else?
> 
> Hmmh, what I wanted to say was: Changing it to default is needed either
because
> else something (what exactly?) doesn't work, or because it's the right thing
to
> do. Which one is this?

Good point, I think this is the key piece of information that the comment is
lacking. Julian, which one is it?

[pastarmovj, 2013-09-30 14:31:53]

I just uploaded https://codereview.chromium.org/25076007/ which completely
removes this special case which turns out is not needed anymore.

[Mattias Nissler (ping if slow), 2013-09-30 14:40:17]

Thanks Julian. Closing this CL.

[marja, 2013-09-30 14:43:49]

Awesome!