Extract ResultCatcher from ExtensionApiTest. Use it in ShellApiTest.

Extract ResultCatcher from ExtensionApiTest. Use it in ShellApiTest, which is separated out from AppShellTest.

Move the remaining DnsApiTest to app_shell_browsertests.

BUG=388893
Committed: https://crrev.com/d6f2fa209bab91490944004b5189cd4191e09024
Cr-Commit-Position: refs/heads/master@{#293517}

[Yoyo Zhou, 2014-09-03 02:08:49]

yoz@chromium.org changed reviewers:
+ jam@chromium.org, jamescook@chromium.org

[Yoyo Zhou, 2014-09-03 02:09:15]

Patchset #1 (id:1) has been deleted

[Yoyo Zhou, 2014-09-03 02:09:31]

Patchset #2 (id:40001) has been deleted

[Yoyo Zhou, 2014-09-03 02:10:16]

jamescook: please review
jam: please review extensions/test/DEPS addition

https://chromiumcodereview.appspot.com/529293003/diff/20001/extensions/test/DEPS
File extensions/test/DEPS (right):

https://chromiumcodereview.appspot.com/529293003/diff/20001/extensions/test/D...
extensions/test/DEPS:2: "+content/public/browser",
I'm not sure whether it's better to put things like result_catcher in
extensions/browser or extensions/test. I think I would prefer extensions/test,
since among other things it's not immediately obvious from the name that it's
test-related. On the other hand, it's easier to keep the dependencies in line if
it's in extensions/browser. Thoughts?

[James Cook, 2014-09-03 18:08:42]

Looking good, mostly curious about ExtensionApiTest::ResultCatcher

https://chromiumcodereview.appspot.com/529293003/diff/20001/extensions/shell/...
File extensions/shell/test/shell_apitest.cc (right):

https://chromiumcodereview.appspot.com/529293003/diff/20001/extensions/shell/...
extensions/shell/test/shell_apitest.cc:31: return false;
nit: maybe clearer as:

if (!loaded)
  return false;

https://chromiumcodereview.appspot.com/529293003/diff/20001/extensions/shell/...
File extensions/shell/test/shell_apitest.h (right):

https://chromiumcodereview.appspot.com/529293003/diff/20001/extensions/shell/...
extensions/shell/test/shell_apitest.h:21: // Loads an unpacked platform app from
a directory using |extension_system_|
maybe AppShellTest::extension_system_ since it isn't a member here? Or just say
something about the current ExtensionSystem?

https://chromiumcodereview.appspot.com/529293003/diff/20001/extensions/shell/...
extensions/shell/test/shell_apitest.h:23: // |app_dir| is a subpath under
extensions/test/data.
nit: wrap with line above

https://chromiumcodereview.appspot.com/529293003/diff/20001/extensions/shell/...
extensions/shell/test/shell_apitest.h:29: 
DISALLOW_COPY_AND_ASSIGN

https://chromiumcodereview.appspot.com/529293003/diff/20001/extensions/test/DEPS
File extensions/test/DEPS (right):

https://chromiumcodereview.appspot.com/529293003/diff/20001/extensions/test/D...
extensions/test/DEPS:2: "+content/public/browser",
On 2014/09/03 02:10:16, Yoyo Zhou wrote:
> I'm not sure whether it's better to put things like result_catcher in
> extensions/browser or extensions/test. I think I would prefer extensions/test,
> since among other things it's not immediately obvious from the name that it's
> test-related. On the other hand, it's easier to keep the dependencies in line
if
> it's in extensions/browser. Thoughts?

I think it's better to keep these in extensions/test. I would probably just do
"+content/public", which is what is done in chrome/test/DEPS.

https://chromiumcodereview.appspot.com/529293003/diff/20001/extensions/test/d...
File extensions/test/data/api_test/dns/api/background.js (right):

https://chromiumcodereview.appspot.com/529293003/diff/20001/extensions/test/d...
extensions/test/data/api_test/dns/api/background.js:5: console.log('background
loaded test test DEBUGYOZ');
do you still need this line?

https://chromiumcodereview.appspot.com/529293003/diff/20001/extensions/test/r...
File extensions/test/result_catcher.h (right):

https://chromiumcodereview.appspot.com/529293003/diff/20001/extensions/test/r...
extensions/test/result_catcher.h:21: // Helper class that observes tests failing
or passing. Observation starts
What's the plan for ExtensionApiTest::ResultCatcher?

https://chromiumcodereview.appspot.com/529293003/diff/20001/extensions/test/r...
extensions/test/result_catcher.h:42: virtual void Observe(int type,
Comment about which interface you are overriding

https://chromiumcodereview.appspot.com/529293003/diff/20001/extensions/test/r...
extensions/test/result_catcher.h:56: content::BrowserContext*
context_restriction_;
it's horribly long but I would call this browser_context_restriction_

[jam, 2014-09-04 16:14:31]

lgtm

[Yoyo Zhou, 2014-09-04 21:57:42]

https://chromiumcodereview.appspot.com/529293003/diff/20001/extensions/shell/...
File extensions/shell/test/shell_apitest.cc (right):

https://chromiumcodereview.appspot.com/529293003/diff/20001/extensions/shell/...
extensions/shell/test/shell_apitest.cc:31: return false;
On 2014/09/03 18:08:41, James Cook wrote:
> nit: maybe clearer as:
> 
> if (!loaded)
>   return false;

Done.

https://chromiumcodereview.appspot.com/529293003/diff/20001/extensions/shell/...
File extensions/shell/test/shell_apitest.h (right):

https://chromiumcodereview.appspot.com/529293003/diff/20001/extensions/shell/...
extensions/shell/test/shell_apitest.h:21: // Loads an unpacked platform app from
a directory using |extension_system_|
On 2014/09/03 18:08:41, James Cook wrote:
> maybe AppShellTest::extension_system_ since it isn't a member here? Or just
say
> something about the current ExtensionSystem?

Updated this.

https://chromiumcodereview.appspot.com/529293003/diff/20001/extensions/shell/...
extensions/shell/test/shell_apitest.h:23: // |app_dir| is a subpath under
extensions/test/data.
On 2014/09/03 18:08:41, James Cook wrote:
> nit: wrap with line above

Done.

https://chromiumcodereview.appspot.com/529293003/diff/20001/extensions/shell/...
extensions/shell/test/shell_apitest.h:29: 
On 2014/09/03 18:08:41, James Cook wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://chromiumcodereview.appspot.com/529293003/diff/20001/extensions/test/DEPS
File extensions/test/DEPS (right):

https://chromiumcodereview.appspot.com/529293003/diff/20001/extensions/test/D...
extensions/test/DEPS:2: "+content/public/browser",
On 2014/09/03 18:08:41, James Cook wrote:
> On 2014/09/03 02:10:16, Yoyo Zhou wrote:
> > I'm not sure whether it's better to put things like result_catcher in
> > extensions/browser or extensions/test. I think I would prefer
extensions/test,
> > since among other things it's not immediately obvious from the name that
it's
> > test-related. On the other hand, it's easier to keep the dependencies in
line
> if
> > it's in extensions/browser. Thoughts?
> 
> I think it's better to keep these in extensions/test. I would probably just do
> "+content/public", which is what is done in chrome/test/DEPS.

Done.

https://chromiumcodereview.appspot.com/529293003/diff/20001/extensions/test/d...
File extensions/test/data/api_test/dns/api/background.js (right):

https://chromiumcodereview.appspot.com/529293003/diff/20001/extensions/test/d...
extensions/test/data/api_test/dns/api/background.js:5: console.log('background
loaded test test DEBUGYOZ');
On 2014/09/03 18:08:41, James Cook wrote:
> do you still need this line?

No.

https://chromiumcodereview.appspot.com/529293003/diff/20001/extensions/test/r...
File extensions/test/result_catcher.h (right):

https://chromiumcodereview.appspot.com/529293003/diff/20001/extensions/test/r...
extensions/test/result_catcher.h:21: // Helper class that observes tests failing
or passing. Observation starts
On 2014/09/03 18:08:42, James Cook wrote:
> What's the plan for ExtensionApiTest::ResultCatcher?

It will be replaced with this one. Added a TODO.

https://chromiumcodereview.appspot.com/529293003/diff/20001/extensions/test/r...
extensions/test/result_catcher.h:42: virtual void Observe(int type,
On 2014/09/03 18:08:42, James Cook wrote:
> Comment about which interface you are overriding

Done.

https://chromiumcodereview.appspot.com/529293003/diff/20001/extensions/test/r...
extensions/test/result_catcher.h:56: content::BrowserContext*
context_restriction_;
On 2014/09/03 18:08:42, James Cook wrote:
> it's horribly long but I would call this browser_context_restriction_

Done.

[James Cook, 2014-09-04 22:08:59]

LGTM

https://chromiumcodereview.appspot.com/529293003/diff/60001/extensions/test/r...
File extensions/test/result_catcher.h (right):

https://chromiumcodereview.appspot.com/529293003/diff/60001/extensions/test/r...
extensions/test/result_catcher.h:26: // TODO(yoz): Replace the version in
ExtensionApiTest with this.
Can you put a comment like this in ExtensionApiTest also?

[Yoyo Zhou, 2014-09-04 22:42:55]

https://chromiumcodereview.appspot.com/529293003/diff/60001/extensions/test/r...
File extensions/test/result_catcher.h (right):

https://chromiumcodereview.appspot.com/529293003/diff/60001/extensions/test/r...
extensions/test/result_catcher.h:26: // TODO(yoz): Replace the version in
ExtensionApiTest with this.
On 2014/09/04 22:08:59, James Cook wrote:
> Can you put a comment like this in ExtensionApiTest also?

Done.

[Yoyo Zhou, 2014-09-04 22:43:08]

The CQ bit was checked by yoz@chromium.org

[commit-bot: I haz the power, 2014-09-04 22:47:02]

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

[commit-bot: I haz the power, 2014-09-05 04:20:22]

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

[commit-bot: I haz the power, 2014-09-05 04:20:23]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[Yoyo Zhou, 2014-09-05 13:11:45]

The CQ bit was checked by yoz@chromium.org

[commit-bot: I haz the power, 2014-09-05 13:12:54]

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

[commit-bot: I haz the power, 2014-09-05 14:19:00]

Committed patchset #3 (id:80001) as 3844dea19678bf76437ede844d1c7194c7b9fa3b

[commit-bot: I haz the power, 2014-09-10 03:39:28]

Patchset 3 (id:??) landed as
https://crrev.com/d6f2fa209bab91490944004b5189cd4191e09024
Cr-Commit-Position: refs/heads/master@{#293517}