Add the first app_shell_browsertest test.

Add the first app_shell_browsertests test.

This test simply launches an app window and waits for it to load.

Other changes in this CL:
- Clean up the way we exclude apps/ from our DEPS (applying it to all of apps/shell) and the common/browser/renderer DEPS separation.
- Move apps/test to apps/shell/test, for accuracy.

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

[Yoyo Zhou, 2014-04-08 01:23:12]

https://chromiumcodereview.appspot.com/224723022/diff/1/apps/shell/browser/DEPS
File apps/shell/browser/DEPS (right):

https://chromiumcodereview.appspot.com/224723022/diff/1/apps/shell/browser/DE...
apps/shell/browser/DEPS:1: include_rules = [
Note that we don't have to list ourself here.

[James Cook, 2014-04-08 16:03:49]

Overall looking good, mostly questions.

One meta-question: In most Chrome unit and browser tests I've been told not to
log things under normal conditions, as it makes the bots have too much output. 
But I see extension tests logging frequently.  Do we prefer to log in extensions
or not log?  (I prefer not logging under normal conditions.)

https://chromiumcodereview.appspot.com/224723022/diff/1/apps/shell/DEPS
File apps/shell/DEPS (right):

https://chromiumcodereview.appspot.com/224723022/diff/1/apps/shell/DEPS#newcode3
apps/shell/DEPS:3: "-apps",
Thank you for cleaning these up.

https://chromiumcodereview.appspot.com/224723022/diff/1/apps/shell/DEPS#newcode7
apps/shell/DEPS:7: "+apps/shell/common",
If we're forcing all the DEPS into subdirectories, would it be better to force
the individual subdirs to import /common and /test?

Alternately, should we reconsider just listing all DEPS in this file?  I'm not
sure we gain that much by differentiating browser vs. renderer vs. common DEPS.

https://chromiumcodereview.appspot.com/224723022/diff/1/apps/shell/app_shell.gyp
File apps/shell/app_shell.gyp (right):

https://chromiumcodereview.appspot.com/224723022/diff/1/apps/shell/app_shell....
apps/shell/app_shell.gyp:169: 'test/app_shell_test.h',
I'm OK with these names and I like that the app_shell_browsertests code is in a
file named app_shell. However, did you consider naming them all just shell_*? 
That's what I've done with most of the files in apps/shell. I was relying on the
//apps path and apps:: namespace to differentiate from other Shell* classes.

https://chromiumcodereview.appspot.com/224723022/diff/1/apps/shell/browser/sh...
File apps/shell/browser/shell_browser_main_parts.h (right):

https://chromiumcodereview.appspot.com/224723022/diff/1/apps/shell/browser/sh...
apps/shell/browser/shell_browser_main_parts.h:86: // If true, indicates the main
message loop should be run
nit: blank line above

And +1 for descriptive comments!

https://chromiumcodereview.appspot.com/224723022/diff/1/apps/shell/browser/sh...
File apps/shell/browser/shell_browsertest.cc (right):

https://chromiumcodereview.appspot.com/224723022/diff/1/apps/shell/browser/sh...
apps/shell/browser/shell_browsertest.cc:17: IN_PROC_BROWSER_TEST_F(AppShellTest,
Basic) {
Please comment what the test is testing.

https://chromiumcodereview.appspot.com/224723022/diff/1/apps/shell/browser/sh...
apps/shell/browser/shell_browsertest.cc:26: LOG(INFO) << "Loading path: " <<
test_data_dir.value();
Delete this line?  I don't think we like adding logging in tests.

https://chromiumcodereview.appspot.com/224723022/diff/1/apps/shell/test/app_s...
File apps/shell/test/app_shell_test.cc (right):

https://chromiumcodereview.appspot.com/224723022/diff/1/apps/shell/test/app_s...
apps/shell/test/app_shell_test.cc:25:
command_line->AppendSwitchASCII(switches::kTestType, "appshell");
Just curious, why do we need a testtype switch?

https://chromiumcodereview.appspot.com/224723022/diff/1/apps/shell/test/app_s...
apps/shell/test/app_shell_test.cc:29: void AppShellTest::TearDown() {
content::BrowserTestBase::TearDown(); }
Do you anticipate this function doing more than calling the base class? If not,
maybe don't override it?

https://chromiumcodereview.appspot.com/224723022/diff/1/extensions/test/data/...
File extensions/test/data/platform_app/background.js (right):

https://chromiumcodereview.appspot.com/224723022/diff/1/extensions/test/data/...
extensions/test/data/platform_app/background.js:5: console.info('runtime
onLaunched');
Does the test need to log?

https://chromiumcodereview.appspot.com/224723022/diff/1/extensions/test/data/...
File extensions/test/data/platform_app/world.js (right):

https://chromiumcodereview.appspot.com/224723022/diff/1/extensions/test/data/...
extensions/test/data/platform_app/world.js:4: window.onload = function() {
nit: blank line above

[Yoyo Zhou, 2014-04-08 21:55:20]

https://chromiumcodereview.appspot.com/224723022/diff/1/apps/shell/DEPS
File apps/shell/DEPS (right):

https://chromiumcodereview.appspot.com/224723022/diff/1/apps/shell/DEPS#newcode7
apps/shell/DEPS:7: "+apps/shell/common",
On 2014/04/08 16:03:49, James Cook wrote:
> If we're forcing all the DEPS into subdirectories, would it be better to force
> the individual subdirs to import /common and /test?

We could do that, but this is the pattern in chrome/ and content/ and it seems
reasonable enough.

> Alternately, should we reconsider just listing all DEPS in this file?  I'm not
> sure we gain that much by differentiating browser vs. renderer vs. common
DEPS.

App Shell doesn't have much going on outside of the browser process, but it's
probably still advisable to prevent stray includes of content/renderer, for
example.

https://chromiumcodereview.appspot.com/224723022/diff/1/apps/shell/app_shell.gyp
File apps/shell/app_shell.gyp (right):

https://chromiumcodereview.appspot.com/224723022/diff/1/apps/shell/app_shell....
apps/shell/app_shell.gyp:169: 'test/app_shell_test.h',
On 2014/04/08 16:03:49, James Cook wrote:
> I'm OK with these names and I like that the app_shell_browsertests code is in
a
> file named app_shell. However, did you consider naming them all just shell_*? 
> That's what I've done with most of the files in apps/shell. I was relying on
the
> //apps path and apps:: namespace to differentiate from other Shell* classes.

Ok, will just call them that.

https://chromiumcodereview.appspot.com/224723022/diff/1/apps/shell/browser/sh...
File apps/shell/browser/shell_browser_main_parts.h (right):

https://chromiumcodereview.appspot.com/224723022/diff/1/apps/shell/browser/sh...
apps/shell/browser/shell_browser_main_parts.h:86: // If true, indicates the main
message loop should be run
On 2014/04/08 16:03:49, James Cook wrote:
> nit: blank line above
> 
> And +1 for descriptive comments!

Done.

https://chromiumcodereview.appspot.com/224723022/diff/1/apps/shell/browser/sh...
File apps/shell/browser/shell_browsertest.cc (right):

https://chromiumcodereview.appspot.com/224723022/diff/1/apps/shell/browser/sh...
apps/shell/browser/shell_browsertest.cc:26: LOG(INFO) << "Loading path: " <<
test_data_dir.value();
On 2014/04/08 16:03:49, James Cook wrote:
> Delete this line?  I don't think we like adding logging in tests.

Done.

https://chromiumcodereview.appspot.com/224723022/diff/1/apps/shell/test/app_s...
File apps/shell/test/app_shell_test.cc (right):

https://chromiumcodereview.appspot.com/224723022/diff/1/apps/shell/test/app_s...
apps/shell/test/app_shell_test.cc:25:
command_line->AppendSwitchASCII(switches::kTestType, "appshell");
On 2014/04/08 16:03:49, James Cook wrote:
> Just curious, why do we need a testtype switch?

The chrome.test API checks that it's running with a --test switch.

https://chromiumcodereview.appspot.com/224723022/diff/1/apps/shell/test/app_s...
apps/shell/test/app_shell_test.cc:29: void AppShellTest::TearDown() {
content::BrowserTestBase::TearDown(); }
On 2014/04/08 16:03:49, James Cook wrote:
> Do you anticipate this function doing more than calling the base class? If
not,
> maybe don't override it?

Done.

https://chromiumcodereview.appspot.com/224723022/diff/1/extensions/test/data/...
File extensions/test/data/platform_app/background.js (right):

https://chromiumcodereview.appspot.com/224723022/diff/1/extensions/test/data/...
extensions/test/data/platform_app/background.js:5: console.info('runtime
onLaunched');
On 2014/04/08 16:03:49, James Cook wrote:
> Does the test need to log?

Not here.

https://chromiumcodereview.appspot.com/224723022/diff/1/extensions/test/data/...
File extensions/test/data/platform_app/world.js (right):

https://chromiumcodereview.appspot.com/224723022/diff/1/extensions/test/data/...
extensions/test/data/platform_app/world.js:4: window.onload = function() {
On 2014/04/08 16:03:49, James Cook wrote:
> nit: blank line above

Done.

P.S. I log here because it's how I know the test is actually working.

[James Cook, 2014-04-08 22:07:18]

LGTM.  Following the chrome/ and content/ convention for DEPS seems like the
right thing to do.

I'm really happy to see this patch!  It will be so good to get some test
coverage going on the trybots.

[Yoyo Zhou, 2014-04-08 22:20:27]

The CQ bit was checked by yoz@chromium.org

[commit-bot: I haz the power, 2014-04-08 22:20:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/224723022/40001

[commit-bot: I haz the power, 2014-04-08 23:20:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-08 23:20:34]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_rel

[Yoyo Zhou, 2014-04-08 23:27:37]

On 2014/04/08 23:20:34, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   tryserver.chromium on linux_chromium_rel

Looks like there is indeed a dodgy include from
apps/shell/common/shell_extensions_client.cc to apps/shell/browser. I'm going to
add a temporary dep.

[James Cook, 2014-04-08 23:29:34]

On 2014/04/08 23:27:37, Yoyo Zhou wrote:
> On 2014/04/08 23:20:34, I haz the power (commit-bot) wrote:
> > Try jobs failed on following builders:
> >   tryserver.chromium on linux_chromium_rel
> 
> Looks like there is indeed a dodgy include from
> apps/shell/common/shell_extensions_client.cc to apps/shell/browser. I'm going
to
> add a temporary dep.

That's my fault, sorry. Temporary dep sounds fine and I'll clean it up later.

[Yoyo Zhou, 2014-04-08 23:29:52]

The CQ bit was checked by yoz@chromium.org

[commit-bot: I haz the power, 2014-04-08 23:33:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/224723022/60001

[commit-bot: I haz the power, 2014-04-09 00:34:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/224723022/60001

[commit-bot: I haz the power, 2014-04-09 00:58:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/224723022/60001

[commit-bot: I haz the power, 2014-04-09 01:09:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/224723022/60001

[commit-bot: I haz the power, 2014-04-09 01:29:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/224723022/60001

[commit-bot: I haz the power, 2014-04-09 01:40:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/224723022/60001

[commit-bot: I haz the power, 2014-04-09 01:50:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/224723022/60001

[commit-bot: I haz the power, 2014-04-09 02:01:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/224723022/60001

[commit-bot: I haz the power, 2014-04-09 02:18:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/224723022/60001

[commit-bot: I haz the power, 2014-04-09 04:21:30]

Change committed as 262616