Startup window on chromeos has been disabled.

Startup window on chromeos has been disabled.

BUG=235754
TEST=manual
NOTRY=True

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203736

[achuithb, 2013-05-29 00:20:32]

Please take a look Tim.

[nduca, 2013-05-29 00:51:49]

How is this backward compatible with old revs of cros where it wasn't disabled?
You need to start coercing this login process to work with all chromes, not just
the latest, at some point.

[achuithb, 2013-05-29 01:14:37]

On 2013/05/29 00:51:49, nduca wrote:
> How is this backward compatible with old revs of cros where it wasn't
disabled?
> You need to start coercing this login process to work with all chromes, not
just
> the latest, at some point.

My hope is that this change ought to work with older versions of chromeos. I
don't think the existence of the startup window affects any tests? If it does,
those tests should probably be fixed, don't you think?

This is the revision that removed the startup window:
https://src.chromium.org/viewvc/chrome?revision=197900&view=revision

There is likely to soon be another change that will re-introduce an apps v2
startup window on ChromeOS.

There are also recent fixes that this CL depends on:
https://codereview.chromium.org/15356002/
https://codereview.chromium.org/15305014/

It seems to me that it's going to be cumbersome to come up with code that
successfully launches a single non-incognito window for this entire range of
CLs. 

Maybe we can have a minimum supported version of cros in telemetry? That way we
don't have to deal with legacy code that goes too far into the past. What do you
think?

[achuithb, 2013-05-29 01:23:38]

To give some more background, we used to close the startup window on cros, which
automatically launched a regular browser window.

Then crrev.com/197900 landed, and the startup window was gone. Telemetry was now
timing out waiting for the startup window.

This CL asks to open a new tab, but it turned out that there were two bugs in
chrome, where a single 'new' would launch 2 incognito tabs: crbug.com/241925

Tim fixed the bug with incognito windows (crrev.com/202240) and I fixed the bug
with 2 tabs (crrev.com/202028), but between 197900 and 202240, there's no good
way to open a regular browser tab on chromeos.

[nduca, 2013-05-29 13:29:37]

Please have hartmanng review.

You've given me a lot of data without answering my question though: will a test
that used to work on old versions of cros now fail? And if it does, then why is
that the tests fault?

[hartmanng, 2013-05-29 18:53:23]

I'm inclined to agree with Nat. IIRC, on older revs, the startup window prevents
us from opening a new tab properly, so if we don't close the startup window I
think tests (and benchmarks themselves) may start failing on older reference
builds.

Could you try running the test suite and some benchmarks on a build before
crrev.com/197900? I'm not convinced it will work.

If the tests fail, but benchmarks succeed, you may be right that the tests need
updating, but I have a feeling those tests were probably there for a reason.


> It seems to me that it's going to be cumbersome to come up with code that
> successfully launches a single non-incognito window for this entire range of
> CLs.

You may be right about that. It's unfortunate, but I'm not sure the right
solution is to just deprecate relatively recent revisions so quickly.


> Maybe we can have a minimum supported version of cros in telemetry? That way
we
> don't have to deal with legacy code that goes too far into the past. What do
you
> think?

I get why this would be nice for us, but at the same time, we do want to be able
to run reference builds. And if there is a new patch coming soon to re-enable
the startup window like you say, I have a feeling we'll end up just pushing the
"last supported" revision forward further and further.

[Tim Song, 2013-05-30 17:11:33]

On 2013/05/29 18:53:23, hartmanng wrote:
> I'm inclined to agree with Nat. IIRC, on older revs, the startup window
prevents
> us from opening a new tab properly, so if we don't close the startup window I
> think tests (and benchmarks themselves) may start failing on older reference
> builds.
> 
> Could you try running the test suite and some benchmarks on a build before
> crrev.com/197900? I'm not convinced it will work.
> 
> If the tests fail, but benchmarks succeed, you may be right that the tests
need
> updating, but I have a feeling those tests were probably there for a reason.
> 
> 
> > It seems to me that it's going to be cumbersome to come up with code that
> > successfully launches a single non-incognito window for this entire range of
> > CLs.
> 
> You may be right about that. It's unfortunate, but I'm not sure the right
> solution is to just deprecate relatively recent revisions so quickly.
> 
> 
> > Maybe we can have a minimum supported version of cros in telemetry? That way
> we
> > don't have to deal with legacy code that goes too far into the past. What do
> you
> > think?
> 
> I get why this would be nice for us, but at the same time, we do want to be
able
> to run reference builds. And if there is a new patch coming soon to re-enable
> the startup window like you say, I have a feeling we'll end up just pushing
the
> "last supported" revision forward further and further.

Since the startup window is just an extension, can't we just check if it exists,
and only invoke the waiting if it does? Is there a timing issue that would
prevent this approach from working?

[achuithb, 2013-05-31 19:48:37]

Glenn: PTAL.

[hartmanng, 2013-05-31 19:55:37]

Great, that's much better I think. Assuming you've tested on current and legacy
builds, LGTM.

[Tim Song, 2013-05-31 21:50:19]

lgtm

[commit-bot: I haz the power, 2013-06-03 18:21:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/achuith@chromium.org/15492002/25001

[commit-bot: I haz the power, 2013-06-03 18:21:43]

Change committed as 203736