Add browser test for new user CrOS login flow

Add browser test for new user CrOS login flow

BUG=178009
TEST=this test

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

[glotov, 2013-04-05 17:43:43]

Jeffrey, need you owner review of
chrome/browser/extensions/user_script_master.cc.

Other files are still being polished.

[Nikita (slow), 2013-04-05 17:49:07]

cc: Zel

[Nikita (slow), 2013-04-05 17:50:36]

https://codereview.chromium.org/13542003/diff/12001/base/test/test_timeouts.cc
File base/test/test_timeouts.cc (right):

https://codereview.chromium.org/13542003/diff/12001/base/test/test_timeouts.c...
base/test/test_timeouts.cc:58: int TestTimeouts::action_max_timeout_ms_ =
4500000;
test change

[Nikita (slow), 2013-04-05 17:58:30]

https://codereview.chromium.org/13542003/diff/19002/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/13542003/diff/19002/chrome/chrome_tests.gypi#...
chrome/chrome_tests.gypi:375: 'browser/chromeos/login/oobe_browsertest.cc',
I think that you also need to add "chrome/test/data/chromeos/service_login.html"
here to fix the error that you've got on bots.

[Nikita (slow), 2013-04-05 18:00:51]

https://codereview.chromium.org/13542003/diff/19002/google_apis/gaia/gaia_url...
File google_apis/gaia/gaia_urls.cc (right):

https://codereview.chromium.org/13542003/diff/19002/google_apis/gaia/gaia_url...
google_apis/gaia/gaia_urls.cc:14: const char kDefaultGaiaBaseUrl[] =
"localhost:8040";
Test change, you need to support configuring this URL in your test.

https://codereview.chromium.org/13542003/diff/19002/google_apis/gaia/gaia_url...
google_apis/gaia/gaia_urls.cc:77: gaia_origin_url_ = "http://" + host_base;
Same here.

[glotov, 2013-04-05 18:58:05]

https://codereview.chromium.org/13542003/diff/19002/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/13542003/diff/19002/chrome/chrome_tests.gypi#...
chrome/chrome_tests.gypi:375: 'browser/chromeos/login/oobe_browsertest.cc',
On 2013/04/05 17:58:31, Nikita Kostylev wrote:
> I think that you also need to add
"chrome/test/data/chromeos/service_login.html"
> here to fix the error that you've got on bots.

Done.

[Nikita (slow), 2013-04-08 17:43:44]

https://codereview.chromium.org/13542003/diff/61001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/login_browsertest.cc (right):

https://codereview.chromium.org/13542003/diff/61001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_browsertest.cc:215: FROM_HERE,
chromeos::BaseLoginDisplayHost::default_host());
nit: One parameter per line.

https://codereview.chromium.org/13542003/diff/61001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/oobe_browsertest.cc (right):

https://codereview.chromium.org/13542003/diff/61001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oobe_browsertest.cc:70:
webui_login_display->ShowSigninScreenForCreds("username", "password");
nit: Please add TODO that mock GAIA server should support username/password
configuration.

https://codereview.chromium.org/13542003/diff/61001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oobe_browsertest.cc:135: 
Please add TODO to support parsing request parameters.

https://codereview.chromium.org/13542003/diff/61001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oobe_browsertest.cc:144: const std::string
continue_url = request.content.substr(
You should add attemptToken parameter with some stub value.

[Nikita (slow), 2013-04-08 17:44:06]

lgtm

[glotov, 2013-04-08 19:30:24]

https://codereview.chromium.org/13542003/diff/61001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/login_browsertest.cc (right):

https://codereview.chromium.org/13542003/diff/61001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_browsertest.cc:215: FROM_HERE,
chromeos::BaseLoginDisplayHost::default_host());
This is possible too, if all params fits a line. No?

https://codereview.chromium.org/13542003/diff/61001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/oobe_browsertest.cc (right):

https://codereview.chromium.org/13542003/diff/61001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oobe_browsertest.cc:70:
webui_login_display->ShowSigninScreenForCreds("username", "password");
On 2013/04/08 17:43:44, Nikita Kostylev wrote:
> nit: Please add TODO that mock GAIA server should support username/password
> configuration.

Done.

[Nikita (slow), 2013-04-08 19:56:41]

lgtm

https://codereview.chromium.org/13542003/diff/37012/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/oobe_browsertest.cc (right):

https://codereview.chromium.org/13542003/diff/37012/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oobe_browsertest.cc:140: "http://localhost:" +
test_server_->base_url().port();
This won't work as there's a hardcoded localhost:8040 in test manifest.

https://codereview.chromium.org/13542003/diff/37012/chrome/browser/ui/webui/c...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h (right):

https://codereview.chromium.org/13542003/diff/37012/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h:442: // Testing
flag, true when we want to use http:// for gaia
nit: Update comment.

[Nikita (slow), 2013-04-10 04:46:44]

+Matt for chrome/browser/extensions/* OWNERS review

[glotov, 2013-04-10 13:03:24]

https://codereview.chromium.org/13542003/diff/61001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/oobe_browsertest.cc (right):

https://codereview.chromium.org/13542003/diff/61001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oobe_browsertest.cc:144: const std::string
continue_url = request.content.substr(
On 2013/04/08 17:43:44, Nikita Kostylev wrote:
> You should add attemptToken parameter with some stub value.

Done.

https://codereview.chromium.org/13542003/diff/37012/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/oobe_browsertest.cc (right):

https://codereview.chromium.org/13542003/diff/37012/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oobe_browsertest.cc:140: "http://localhost:" +
test_server_->base_url().port();
On 2013/04/08 19:56:41, Nikita Kostylev wrote:
> This won't work as there's a hardcoded localhost:8040 in test manifest.

Done.

https://codereview.chromium.org/13542003/diff/37012/chrome/browser/ui/webui/c...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h (right):

https://codereview.chromium.org/13542003/diff/37012/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h:442: // Testing
flag, true when we want to use http:// for gaia
On 2013/04/08 19:56:41, Nikita Kostylev wrote:
> nit: Update comment.

Done.

[Matt Perry, 2013-04-10 18:52:09]

https://codereview.chromium.org/13542003/diff/90001/chrome/browser/extensions...
File chrome/browser/extensions/user_script_master.cc (right):

https://codereview.chromium.org/13542003/diff/90001/chrome/browser/extensions...
chrome/browser/extensions/user_script_master.cc:194: if
(extensions::ImageLoader::IsComponentExtensionResource(
I don't understand this change. IsComponentExtensionResource is used for image
files. Why would a script be there?

Also, component extensions with content scripts already work. Why is this
necessary?

[glotov, 2013-04-11 15:31:19]

gaia_auth extension with manifest_test.json did not work. Unlike normal
manifest.json, it included test/content.js that could not be found without the
change.

[Nikita (slow), 2013-04-15 21:06:15]

Matt, friendly ping

[Matt Perry, 2013-04-15 21:26:25]

On 2013/04/11 15:31:19, glotov wrote:
> gaia_auth extension with manifest_test.json did not work. Unlike normal
> manifest.json, it included test/content.js that could not be found without the
> change.

(Sorry, didn't see this response.)

What do you mean by "unlike normal manifest.json"? There are other component
extensions that use content scripts, aren't there? Looking at the code, I've
found chrome/browser/resources/help_app and
chrome/browser/resources/chromeos/access_chromevox.

[Nikita (slow), 2013-04-16 03:46:32]

On 2013/04/15 21:26:25, Matt Perry wrote:
> On 2013/04/11 15:31:19, glotov wrote:
> > gaia_auth extension with manifest_test.json did not work. Unlike normal
> > manifest.json, it included test/content.js that could not be found without
the
> > change.
> 
> (Sorry, didn't see this response.)
> 
> What do you mean by "unlike normal manifest.json"? There are other component
> extensions that use content scripts, aren't there? Looking at the code, I've
> found chrome/browser/resources/help_app and
> chrome/browser/resources/chromeos/access_chromevox.

The difference is that help_app and access_chromevox are loaded from file
system:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...


In case of gaia_auth it is loaded completely from browser resources. And in case
of using test version of gaia_auth extension we've found that content script
loading from resources is not supported. Even though extension is loaded from
resources itself just fine content script is loading is performed from file
system and fails. This small patch addresses that.

[zel, 2013-04-16 15:20:36]

why don't you move test/content.js to test_content.js?


On Mon, Apr 15, 2013 at 8:46 PM, <nkostylev@chromium.org> wrote:

> On 2013/04/15 21:26:25, Matt Perry wrote:
>
>> On 2013/04/11 15:31:19, glotov wrote:
>> > gaia_auth extension with manifest_test.json did not work. Unlike normal
>> > manifest.json, it included test/content.js that could not be found
>> without
>>
> the
>
>> > change.
>>
>
>  (Sorry, didn't see this response.)
>>
>
>  What do you mean by "unlike normal manifest.json"? There are other
>> component
>> extensions that use content scripts, aren't there? Looking at the code,
>> I've
>> found chrome/browser/resources/help_**app and
>> chrome/browser/resources/**chromeos/access_chromevox.
>>
>
> The difference is that help_app and access_chromevox are loaded from file
> system:
> https://code.google.com/p/**chromium/codesearch#chromium/**
> src/chrome/browser/extensions/**component_loader.cc&q=help_**
>
app&sq=package:chromium&type=**cs&l=351<https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/extensions/component_loader.cc&q=help_app&sq=package:chromium&type=cs&l=351>
>
> https://code.google.com/p/**chromium/codesearch#chromium/**
> src/chrome/common/extensions/**extension_constants.cc&q=**
>
kChromeVoxExtensionPath&sq=**package:chromium&type=cs&l=146<https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/extensions/extension_constants.cc&q=kChromeVoxExtensionPath&sq=package:chromium&type=cs&l=146>
> https://code.google.com/p/**chromium/codesearch#chromium/**
> src/chrome/browser/extensions/**component_loader.cc&q=**
>
kChromeVoxExtensionPath&sq=**package:chromium&type=cs&l=448<https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/extensions/component_loader.cc&q=kChromeVoxExtensionPath&sq=package:chromium&type=cs&l=448>
>
>
> In case of gaia_auth it is loaded completely from browser resources. And
> in case
> of using test version of gaia_auth extension we've found that content
> script
> loading from resources is not supported. Even though extension is loaded
> from
> resources itself just fine content script is loading is performed from file
> system and fails. This small patch addresses that.
>
>
https://codereview.chromium.**org/13542003/<https://codereview.chromium.org/1...
>

[glotov, 2013-04-16 15:34:40]

I tried that, but the file in not found in the file system, relative path
does not matter here.


On Tue, Apr 16, 2013 at 7:20 PM, Zelidrag Hornung <zelidrag@chromium.org>wrote:

> why don't you move test/content.js to test_content.js?
>
>
> On Mon, Apr 15, 2013 at 8:46 PM, <nkostylev@chromium.org> wrote:
>
>> On 2013/04/15 21:26:25, Matt Perry wrote:
>>
>>> On 2013/04/11 15:31:19, glotov wrote:
>>> > gaia_auth extension with manifest_test.json did not work. Unlike normal
>>> > manifest.json, it included test/content.js that could not be found
>>> without
>>>
>> the
>>
>>> > change.
>>>
>>
>>  (Sorry, didn't see this response.)
>>>
>>
>>  What do you mean by "unlike normal manifest.json"? There are other
>>> component
>>> extensions that use content scripts, aren't there? Looking at the code,
>>> I've
>>> found chrome/browser/resources/help_**app and
>>> chrome/browser/resources/**chromeos/access_chromevox.
>>>
>>
>> The difference is that help_app and access_chromevox are loaded from file
>> system:
>> https://code.google.com/p/**chromium/codesearch#chromium/**
>> src/chrome/browser/extensions/**component_loader.cc&q=help_**
>>
app&sq=package:chromium&type=**cs&l=351<https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/extensions/component_loader.cc&q=help_app&sq=package:chromium&type=cs&l=351>
>>
>> https://code.google.com/p/**chromium/codesearch#chromium/**
>> src/chrome/common/extensions/**extension_constants.cc&q=**
>>
kChromeVoxExtensionPath&sq=**package:chromium&type=cs&l=146<https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/extensions/extension_constants.cc&q=kChromeVoxExtensionPath&sq=package:chromium&type=cs&l=146>
>> https://code.google.com/p/**chromium/codesearch#chromium/**
>> src/chrome/browser/extensions/**component_loader.cc&q=**
>>
kChromeVoxExtensionPath&sq=**package:chromium&type=cs&l=448<https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/extensions/component_loader.cc&q=kChromeVoxExtensionPath&sq=package:chromium&type=cs&l=448>
>>
>>
>> In case of gaia_auth it is loaded completely from browser resources. And
>> in case
>> of using test version of gaia_auth extension we've found that content
>> script
>> loading from resources is not supported. Even though extension is loaded
>> from
>> resources itself just fine content script is loading is performed from
>> file
>> system and fails. This small patch addresses that.
>>
>>
https://codereview.chromium.**org/13542003/<https://codereview.chromium.org/1...
>>
>
>


-- 
Thank you,
Denis

[zel, 2013-04-16 15:37:34]

Ah, ok - I thought that's the issue since I ran into similar problem with
resource loading from sub-dirs before. So, the content scripts can't be now
loaded from chrome resources period.


On Tue, Apr 16, 2013 at 8:34 AM, Denis Glotov <glotov@chromium.org> wrote:

> I tried that, but the file in not found in the file system, relative path
> does not matter here.
>
>
> On Tue, Apr 16, 2013 at 7:20 PM, Zelidrag Hornung
<zelidrag@chromium.org>wrote:
>
>> why don't you move test/content.js to test_content.js?
>>
>>
>> On Mon, Apr 15, 2013 at 8:46 PM, <nkostylev@chromium.org> wrote:
>>
>>> On 2013/04/15 21:26:25, Matt Perry wrote:
>>>
>>>> On 2013/04/11 15:31:19, glotov wrote:
>>>> > gaia_auth extension with manifest_test.json did not work. Unlike
>>>> normal
>>>> > manifest.json, it included test/content.js that could not be found
>>>> without
>>>>
>>> the
>>>
>>>> > change.
>>>>
>>>
>>>  (Sorry, didn't see this response.)
>>>>
>>>
>>>  What do you mean by "unlike normal manifest.json"? There are other
>>>> component
>>>> extensions that use content scripts, aren't there? Looking at the code,
>>>> I've
>>>> found chrome/browser/resources/help_**app and
>>>> chrome/browser/resources/**chromeos/access_chromevox.
>>>>
>>>
>>> The difference is that help_app and access_chromevox are loaded from file
>>> system:
>>> https://code.google.com/p/**chromium/codesearch#chromium/**
>>> src/chrome/browser/extensions/**component_loader.cc&q=help_**
>>>
app&sq=package:chromium&type=**cs&l=351<https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/extensions/component_loader.cc&q=help_app&sq=package:chromium&type=cs&l=351>
>>>
>>> https://code.google.com/p/**chromium/codesearch#chromium/**
>>> src/chrome/common/extensions/**extension_constants.cc&q=**
>>>
kChromeVoxExtensionPath&sq=**package:chromium&type=cs&l=146<https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/extensions/extension_constants.cc&q=kChromeVoxExtensionPath&sq=package:chromium&type=cs&l=146>
>>> https://code.google.com/p/**chromium/codesearch#chromium/**
>>> src/chrome/browser/extensions/**component_loader.cc&q=**
>>>
kChromeVoxExtensionPath&sq=**package:chromium&type=cs&l=448<https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/extensions/component_loader.cc&q=kChromeVoxExtensionPath&sq=package:chromium&type=cs&l=448>
>>>
>>>
>>> In case of gaia_auth it is loaded completely from browser resources. And
>>> in case
>>> of using test version of gaia_auth extension we've found that content
>>> script
>>> loading from resources is not supported. Even though extension is loaded
>>> from
>>> resources itself just fine content script is loading is performed from
>>> file
>>> system and fails. This small patch addresses that.
>>>
>>>
https://codereview.chromium.**org/13542003/<https://codereview.chromium.org/1...
>>>
>>
>>
>
>
> --
> Thank you,
> Denis
>

[Matt Perry, 2013-04-16 17:07:25]

OK, thanks for the explanation. I didn't realize content script loading from
resources was just broken before.

LGTM

[glotov, 2013-04-17 11:44:50]

Committed patchset #8 manually as r194572 (presubmit successful).

[Nikita (slow), 2013-04-17 13:18:42]

Why cros_* trybots are missing?