Add test that verifies web-page to chrome communication.

Introduces cros:// schema for web-page to chrome communication in ChromeOS.
Add test that verifies web-page to chrome communication.

BUG=cros/3177
TEST=out/Debug/browser_tests --gtest_filter=AccountScreenTest.TestSchemeInspector


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

[whywhat, 2010-05-27 16:32:45]

LGTM with minor comments.

http://codereview.chromium.org/2291002/diff/4001/5001
File chrome/browser/chromeos/login/account_screen_browsertest.cc (right):

http://codereview.chromium.org/2291002/diff/4001/5001#newcode65
chrome/browser/chromeos/login/account_screen_browsertest.cc:65: // Do not
navigate to the given URL. Navigate to about::blank instead.
about::blank -> about:blank?

http://codereview.chromium.org/2291002/diff/4001/5002
File chrome/common/url_constants.cc (right):

http://codereview.chromium.org/2291002/diff/4001/5002#newcode15
chrome/common/url_constants.cc:15: const char kCrosScheme[] = "cros";
Maybe guard this identificator with #if defined(OS_CHROMEOS) too?

[Paweł Hajdan Jr., 2010-05-27 16:41:54]

Drive-by with a minor test comment.

http://codereview.chromium.org/2291002/diff/4001/5001
File chrome/browser/chromeos/login/account_screen_browsertest.cc (right):

http://codereview.chromium.org/2291002/diff/4001/5001#newcode76
chrome/browser/chromeos/login/account_screen_browsertest.cc:76:
MessageLoopForUI::current()->Run(NULL);
Shouldn't this be ui_test_utils::RunMessageLoop(), or are there some reasons why
you need to do it this way? If the latter, this would deserve a comment.

[glotov, 2010-05-27 16:58:08]

http://codereview.chromium.org/2291002/diff/4001/5001
File chrome/browser/chromeos/login/account_screen_browsertest.cc (right):

http://codereview.chromium.org/2291002/diff/4001/5001#newcode65
chrome/browser/chromeos/login/account_screen_browsertest.cc:65: // Do not
navigate to the given URL. Navigate to about::blank instead.
On 2010/05/27 16:32:45, whywhat wrote:
> about::blank -> about:blank?

Done.

http://codereview.chromium.org/2291002/diff/4001/5001#newcode76
chrome/browser/chromeos/login/account_screen_browsertest.cc:76:
MessageLoopForUI::current()->Run(NULL);
On 2010/05/27 16:41:54, Paweł Hajdan Jr. wrote:
> Shouldn't this be ui_test_utils::RunMessageLoop(), or are there some reasons
why
> you need to do it this way? If the latter, this would deserve a comment.

Done.

http://codereview.chromium.org/2291002/diff/4001/5002
File chrome/common/url_constants.cc (right):

http://codereview.chromium.org/2291002/diff/4001/5002#newcode15
chrome/common/url_constants.cc:15: const char kCrosScheme[] = "cros";
Agree, although not pretty.

On 2010/05/27 16:32:45, whywhat wrote:
> Maybe guard this identificator with #if defined(OS_CHROMEOS) too?

[Paweł Hajdan Jr., 2010-05-27 17:07:19]

Code I commented in the drive-by LGTM.

[Nikita (slow), 2010-05-27 17:40:53]

http://codereview.chromium.org/2291002/diff/13001/14001
File chrome/browser/chromeos/login/account_screen_browsertest.cc (right):

http://codereview.chromium.org/2291002/diff/13001/14001#newcode52
chrome/browser/chromeos/login/account_screen_browsertest.cc:52: static bool
inspector_called = false;  // had to use global flag as
Is it possible to move this flag and InspectorHook to AccountScreenTest and
avoid static?

http://codereview.chromium.org/2291002/diff/13001/14001#newcode63
chrome/browser/chromeos/login/account_screen_browsertest.cc:63:
ChromeThread::PostTask(ChromeThread::UI, FROM_HERE,
Is this the only solution to quit the test?

I'd prefer something like

ui_test_utils::RunMessageLoop();
EXPECT_TRUE(inspector_called);
ui_test_utils::RunAllPendingInMessageLoop();

If it works.

http://codereview.chromium.org/2291002/diff/13001/14002
File chrome/common/url_constants.cc (right):

http://codereview.chromium.org/2291002/diff/13001/14002#newcode14
chrome/common/url_constants.cc:14: const char kCrosScheme[] = "cros";
Should be unchanged?

http://codereview.chromium.org/2291002/diff/13001/14002#newcode30
chrome/common/url_constants.cc:30: const char kChromeUIScheme[] = "chrome";
You should leave kChromeUIScheme unchanged and
introduce kCrosScheme only for OS_CHROMEOS

[glotov, 2010-05-27 17:55:04]

http://codereview.chromium.org/2291002/diff/13001/14001
File chrome/browser/chromeos/login/account_screen_browsertest.cc (right):

http://codereview.chromium.org/2291002/diff/13001/14001#newcode52
chrome/browser/chromeos/login/account_screen_browsertest.cc:52: static bool
inspector_called = false;  // had to use global flag as
It will be static there.
InspectorHook() doesn't have context so I cannot easily use local flags here.
This is what I tried to say in comments :)

On 2010/05/27 17:40:54, Nikita Kostylev wrote:
> Is it possible to move this flag and InspectorHook to AccountScreenTest and
> avoid static?

http://codereview.chromium.org/2291002/diff/13001/14001#newcode63
chrome/browser/chromeos/login/account_screen_browsertest.cc:63:
ChromeThread::PostTask(ChromeThread::UI, FROM_HERE,
Not sure what you mean.
ui_test_utils::RunMessageLoop(); must be stopped by some mean, so we stop it
here. 
 MessageLoopForUI::current()->Quit(); quits the message loop after all pending
tasks are done.

On 2010/05/27 17:40:54, Nikita Kostylev wrote:
> Is this the only solution to quit the test?
> 
> I'd prefer something like
> 
> ui_test_utils::RunMessageLoop();
> EXPECT_TRUE(inspector_called);
> ui_test_utils::RunAllPendingInMessageLoop();
> 
> If it works.

http://codereview.chromium.org/2291002/diff/13001/14002
File chrome/common/url_constants.cc (right):

http://codereview.chromium.org/2291002/diff/13001/14002#newcode14
chrome/common/url_constants.cc:14: const char kCrosScheme[] = "cros";
On 2010/05/27 17:40:54, Nikita Kostylev wrote:
> Should be unchanged?

Done.

http://codereview.chromium.org/2291002/diff/13001/14002#newcode30
chrome/common/url_constants.cc:30: const char kChromeUIScheme[] = "chrome";
On 2010/05/27 17:40:54, Nikita Kostylev wrote:
> You should leave kChromeUIScheme unchanged and
> introduce kCrosScheme only for OS_CHROMEOS

Done. Just didn't uploaded the final patch with the fix.

[Nikita (slow), 2010-05-28 14:06:39]

LGTM