Add NaCl smoke test to browser_tests.

Add NaCl smoke test to browser_tests.

The smoke test is simple - it checks that Chrome can load nexes.  The more
interesting part of this CL is that it lays the groundwork for adding more NaCl
tests to browser_tests.  This will allow many nacl_integration tests to be
rewritten as browser_tests.  This CL adds an extensible jig for decoding
messages from Javascript, which makes it easier to add test logic in the web
page.  This CL also establishes locations in the build directory for nexes and
other test data.

There are two followup CLs planned: 1) simplify how nexes are built in GYP
and 2) share the code for running tests in Javascript with the PPAPI test
suite.

BUG= http://code.google.com/p/nativeclient/issues/detail?id=2959
TEST= browser_tests --gtest_filter=NaClBrowserTest*


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

[sky, 2012-08-10 22:34:15]

I did a quick look over parts of this. I'm not familiar enough with the nacl
side though. Can you get a reviewer from nacl too?

https://chromiumcodereview.appspot.com/10837207/diff/1/chrome/test/base/ui_te...
File chrome/test/base/ui_test_utils.cc (right):

https://chromiumcodereview.appspot.com/10837207/diff/1/chrome/test/base/ui_te...
chrome/test/base/ui_test_utils.cc:330: if (!exe_dir.IsAbsolute())
file_util::AbsolutePath(&exe_dir);
no single line if statements.

https://chromiumcodereview.appspot.com/10837207/diff/1/chrome/test/base/ui_te...
chrome/test/base/ui_test_utils.cc:352: for (size_t tmp_itr = match; tmp_itr <
src_size; ++tmp_itr) {
no {} here and 355.

https://chromiumcodereview.appspot.com/10837207/diff/1/chrome/test/nacl/nacl_...
File chrome/test/nacl/nacl_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10837207/diff/1/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest.cc:34: Continue,
enums should have app caps -> CONTINUE

https://chromiumcodereview.appspot.com/10837207/diff/1/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest.cc:43: void SetError(const std::string
&message) {
string& message
That style every where.

https://chromiumcodereview.appspot.com/10837207/diff/1/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest.cc:48: bool ok() {
const

https://chromiumcodereview.appspot.com/10837207/diff/1/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest.cc:52: const std::string& error_message() {
const

https://chromiumcodereview.appspot.com/10837207/diff/1/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest.cc:64: public:
indent one space.

https://chromiumcodereview.appspot.com/10837207/diff/1/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest.cc:65: 
no newline.

https://chromiumcodereview.appspot.com/10837207/diff/1/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest.cc:87: if(!value->GetAsDictionary(&msg))
space after if (here and 91)

https://chromiumcodereview.appspot.com/10837207/diff/1/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest.cc:101: const std::string &field) 
WARN_UNUSED_RESULT {
extra space after ) here and 105

https://chromiumcodereview.appspot.com/10837207/diff/1/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest.cc:121: MessageResponse
HandleStructuredMessage(const std::string& type,
virtual and OVERRIDE

https://chromiumcodereview.appspot.com/10837207/diff/1/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest.cc:122: DictionaryValue *msg) {
Value&

https://chromiumcodereview.appspot.com/10837207/diff/1/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest.cc:142: bool test_passed() {
const

https://chromiumcodereview.appspot.com/10837207/diff/1/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest.cc:148: };
DISALLOW...

https://chromiumcodereview.appspot.com/10837207/diff/1/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest.cc:163: timer_.Start(FROM_HERE, timeout, this,
&JavascriptTestObserver::TimeOut);
We do you need a timer here?

https://chromiumcodereview.appspot.com/10837207/diff/1/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest.cc:166: bool Run() {
Description?

https://chromiumcodereview.appspot.com/10837207/diff/1/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest.cc:173: void Observe(
virtual OVERRIDE

https://chromiumcodereview.appspot.com/10837207/diff/1/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest.cc:234: if
(!ui_test_utils::GetRelativeBuildDirectory(document_root)) {
no {}

https://chromiumcodereview.appspot.com/10837207/diff/1/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest.cc:246: void SetUpCommandLine(CommandLine*
command_line) {
virtual and OVERRIDE

https://chromiumcodereview.appspot.com/10837207/diff/1/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest.cc:260: virtual FilePath::StringType variant()
= 0;
virtual methods use CamelCase

https://chromiumcodereview.appspot.com/10837207/diff/1/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest.cc:303: return FILE_PATH_LITERAL("newlib");
Isn't there a better way to pass this in? gtest supports parameterized tests.

[Nick Bray, 2012-08-10 23:16:28]

Fixed the style issues.
Could you give me more background on test timeouts?
I'll look into the gtest parameterization.
Added mseaborn and bradnelson as reviewers.  They are, after all, going to be
OWNERS...

mseaborn: test jig design.
bradnelson: GYP.

https://chromiumcodereview.appspot.com/10837207/diff/1/chrome/test/base/ui_te...
File chrome/test/base/ui_test_utils.cc (right):

https://chromiumcodereview.appspot.com/10837207/diff/1/chrome/test/base/ui_te...
chrome/test/base/ui_test_utils.cc:330: if (!exe_dir.IsAbsolute())
file_util::AbsolutePath(&exe_dir);
On 2012/08/10 22:34:15, sky wrote:
> no single line if statements.

Done.

https://chromiumcodereview.appspot.com/10837207/diff/1/chrome/test/base/ui_te...
chrome/test/base/ui_test_utils.cc:352: for (size_t tmp_itr = match; tmp_itr <
src_size; ++tmp_itr) {
On 2012/08/10 22:34:15, sky wrote:
> no {} here and 355.

Done.

https://chromiumcodereview.appspot.com/10837207/diff/1/chrome/test/nacl/nacl_...
File chrome/test/nacl/nacl_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10837207/diff/1/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest.cc:34: Continue,
On 2012/08/10 22:34:15, sky wrote:
> enums should have app caps -> CONTINUE

Done.

https://chromiumcodereview.appspot.com/10837207/diff/1/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest.cc:43: void SetError(const std::string
&message) {
On 2012/08/10 22:34:15, sky wrote:
> string& message
> That style every where.

Done.

https://chromiumcodereview.appspot.com/10837207/diff/1/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest.cc:52: const std::string& error_message() {
On 2012/08/10 22:34:15, sky wrote:
> const

Done.

https://chromiumcodereview.appspot.com/10837207/diff/1/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest.cc:64: public:
On 2012/08/10 22:34:15, sky wrote:
> indent one space.

Done.

https://chromiumcodereview.appspot.com/10837207/diff/1/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest.cc:65: 
On 2012/08/10 22:34:15, sky wrote:
> no newline.

Done.

https://chromiumcodereview.appspot.com/10837207/diff/1/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest.cc:87: if(!value->GetAsDictionary(&msg))
On 2012/08/10 22:34:15, sky wrote:
> space after if (here and 91)

Done.

https://chromiumcodereview.appspot.com/10837207/diff/1/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest.cc:101: const std::string &field) 
WARN_UNUSED_RESULT {
On 2012/08/10 22:34:15, sky wrote:
> extra space after ) here and 105

Done.

https://chromiumcodereview.appspot.com/10837207/diff/1/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest.cc:122: DictionaryValue *msg) {
On 2012/08/10 22:34:15, sky wrote:
> Value&

Done.

https://chromiumcodereview.appspot.com/10837207/diff/1/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest.cc:142: bool test_passed() {
On 2012/08/10 22:34:15, sky wrote:
> const

Done.

https://chromiumcodereview.appspot.com/10837207/diff/1/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest.cc:148: };
On 2012/08/10 22:34:15, sky wrote:
> DISALLOW...

Done.

https://chromiumcodereview.appspot.com/10837207/diff/1/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest.cc:163: timer_.Start(FROM_HERE, timeout, this,
&JavascriptTestObserver::TimeOut);
On 2012/08/10 22:34:15, sky wrote:
> We do you need a timer here?

I had two reasons:

1) This is adapted from the PPAPI test jig.  They had a timer and I plan to
share this class in a future CL.  I did notice browser_tests time out on their
own, however, in 45 seconds?  This made me wonder about the timer, myself.

2) In the future I want to diagnose why the test timed out - renderer hung, etc.
 This requires having an internal timeout.

Looking at the facts, I don't have a complete understanding of the timer
situation, I was cargoculting this in from the other test runner.  Advice?

https://chromiumcodereview.appspot.com/10837207/diff/1/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest.cc:166: bool Run() {
On 2012/08/10 22:34:15, sky wrote:
> Description?

Done.

https://chromiumcodereview.appspot.com/10837207/diff/1/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest.cc:173: void Observe(
On 2012/08/10 22:34:15, sky wrote:
> virtual OVERRIDE

Done.

https://chromiumcodereview.appspot.com/10837207/diff/1/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest.cc:234: if
(!ui_test_utils::GetRelativeBuildDirectory(document_root)) {
On 2012/08/10 22:34:15, sky wrote:
> no {}

Done.

https://chromiumcodereview.appspot.com/10837207/diff/1/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest.cc:246: void SetUpCommandLine(CommandLine*
command_line) {
On 2012/08/10 22:34:15, sky wrote:
> virtual and OVERRIDE

Done.

https://chromiumcodereview.appspot.com/10837207/diff/1/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest.cc:260: virtual FilePath::StringType variant()
= 0;
On 2012/08/10 22:34:15, sky wrote:
> virtual methods use CamelCase

Done.

https://chromiumcodereview.appspot.com/10837207/diff/1/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest.cc:303: return FILE_PATH_LITERAL("newlib");
On 2012/08/10 22:34:15, sky wrote:
> Isn't there a better way to pass this in? gtest supports parameterized tests.

I'll look into this.

[Nick Bray, 2012-08-10 23:42:30]

re: parameterized tests - the deal breaker for me is that the resulting test
names are hard to filter.  For example, how would I run tests only where variant
== "newlib"?  With the current approach (plus some macros to remove redundant
declarations - see the PPAPI tests) it's easier to filter because the variant is
implicit in the test suite name.

[bradn, 2012-08-13 21:10:59]

LGTM on the gyp

http://codereview.chromium.org/10837207/diff/1013/chrome/test/data/nacl/nacl_...
File chrome/test/data/nacl/nacl_test_data.gyp (right):

http://codereview.chromium.org/10837207/diff/1013/chrome/test/data/nacl/nacl_...
chrome/test/data/nacl/nacl_test_data.gyp:9: 'targets': [
This is pretty much what I recalled.
A comment to note its in need of being refactored to a common file?

http://codereview.chromium.org/10837207/diff/1013/chrome/test/data/nacl/simpl...
File chrome/test/data/nacl/simple.nmf (right):

http://codereview.chromium.org/10837207/diff/1013/chrome/test/data/nacl/simpl...
chrome/test/data/nacl/simple.nmf:8: 
Drop extra line

[sky, 2012-08-13 21:29:56]

https://chromiumcodereview.appspot.com/10837207/diff/1/chrome/test/nacl/nacl_...
File chrome/test/nacl/nacl_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10837207/diff/1/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest.cc:163: timer_.Start(FROM_HERE, timeout, this,
&JavascriptTestObserver::TimeOut);
On 2012/08/10 23:16:28, Nick Bray wrote:
> On 2012/08/10 22:34:15, sky wrote:
> > We do you need a timer here?
> 
> I had two reasons:
> 
> 1) This is adapted from the PPAPI test jig.  They had a timer and I plan to
> share this class in a future CL.  I did notice browser_tests time out on their
> own, however, in 45 seconds?  This made me wonder about the timer, myself.
> 
> 2) In the future I want to diagnose why the test timed out - renderer hung,
etc.
>  This requires having an internal timeout.
> 
> Looking at the facts, I don't have a complete understanding of the timer
> situation, I was cargoculting this in from the other test runner.  Advice?

Nuke the timer. The test suite handles it for you. I agree that error reporting
isn't as nice, but I would rather see a general solution added to browser_tests
to address that need rather than something specific here.

https://chromiumcodereview.appspot.com/10837207/diff/1013/chrome/test/nacl/na...
File chrome/test/nacl/nacl_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10837207/diff/1013/chrome/test/nacl/na...
chrome/test/nacl/nacl_browsertest.cc:39: TestMessageHandler() : ok_(true) {}
Add a virtual destructor.

https://chromiumcodereview.appspot.com/10837207/diff/1013/chrome/test/nacl/na...
chrome/test/nacl/nacl_browsertest.cc:65: MessageResponse HandleMessage(const
std::string& json) {
virtual OVERRIDE

https://chromiumcodereview.appspot.com/10837207/diff/1013/chrome/test/nacl/na...
chrome/test/nacl/nacl_browsertest.cc:85: DictionaryValue *msg;
* on the other side, eg DictionaryValue* msg. Search through this file for ' *',
I think there are a handful of places that have it wrong still.

https://chromiumcodereview.appspot.com/10837207/diff/1013/chrome/test/nacl/na...
chrome/test/nacl/nacl_browsertest.cc:108: };
DISALLOW_...

[Mark Seaborn, 2012-08-13 22:16:59]

https://chromiumcodereview.appspot.com/10837207/diff/1013/chrome/test/base/ui...
File chrome/test/base/ui_test_utils.cc (right):

https://chromiumcodereview.appspot.com/10837207/diff/1013/chrome/test/base/ui...
chrome/test/base/ui_test_utils.cc:339: size_t match, exe_size, src_size;
Nit: The normal style is one variable decl per line, I think.  Same for next
line.  (I can't find it in the style guide, but Chromium's "*" and "&" spacing
don't make much sense without a one-per-line policy.)

https://chromiumcodereview.appspot.com/10837207/diff/1013/chrome/test/base/ui...
chrome/test/base/ui_test_utils.cc:347: for (match = 0; match < exe_size && match
< src_size; ++match) {
Why write src_size rather than src_parts.size()?

https://chromiumcodereview.appspot.com/10837207/diff/1013/chrome/test/data/na...
File chrome/test/data/nacl/DEPS (right):

https://chromiumcodereview.appspot.com/10837207/diff/1013/chrome/test/data/na...
chrome/test/data/nacl/DEPS:5: 
Drop extra line (as Brad says for another file :-) )

https://chromiumcodereview.appspot.com/10837207/diff/1013/chrome/test/data/na...
File chrome/test/data/nacl/nacl_load_test.html (right):

https://chromiumcodereview.appspot.com/10837207/diff/1013/chrome/test/data/na...
chrome/test/data/nacl/nacl_load_test.html:1: <html>
Shouldn't this file have a copyright message?  The codebase doesn't seem to be
consistent here, but better to have one.

https://chromiumcodereview.appspot.com/10837207/diff/1013/chrome/test/data/na...
chrome/test/data/nacl/nacl_load_test.html:18: var e =
document.createElement("embed");
Nit: use single quotes for consistency?  Also, rename 'e' -> 'embed' to avoid
cryptic single-char variable names?

https://chromiumcodereview.appspot.com/10837207/diff/1013/chrome/test/data/na...
File chrome/test/data/nacl/nacl_test_data.gyp (right):

https://chromiumcodereview.appspot.com/10837207/diff/1013/chrome/test/data/na...
chrome/test/data/nacl/nacl_test_data.gyp:36: 'out_newlib32':
'<(PRODUCT_DIR)/nacl_test_data/newlib/>(nexe_target)_newlib_x32.nexe',
Stupid question: is it really necessary to list all output architectures?

https://chromiumcodereview.appspot.com/10837207/diff/1013/chrome/test/data/na...
chrome/test/data/nacl/nacl_test_data.gyp:48: '-lplatform',
AFAICT, you don't use libplatform or libgio in the test you're adding.  (You
don't use libpthread directly but it's probably still needed by "-lppapi".)

https://chromiumcodereview.appspot.com/10837207/diff/1013/chrome/test/data/na...
chrome/test/data/nacl/nacl_test_data.gyp:60: '-mno-tls-use-call',
Why is this here?  It looks like it has been cargo-culted.  This is only for use
inside the IRT.

https://chromiumcodereview.appspot.com/10837207/diff/1013/chrome/test/data/na...
File chrome/test/data/nacl/simple.nmf (right):

https://chromiumcodereview.appspot.com/10837207/diff/1013/chrome/test/data/na...
chrome/test/data/nacl/simple.nmf:3: "x86-32": {"url": "simple_newlib_x32.nexe"},
Can't this manifest file be auto-generated for newlib too?  We managed to mostly
get away from having to check NMF files for the Scons-based tests.

[Nick Bray (chromium), 2012-08-13 23:16:25]

PTAL

https://chromiumcodereview.appspot.com/10837207/diff/1013/chrome/test/data/na...
File chrome/test/data/nacl/DEPS (right):

https://chromiumcodereview.appspot.com/10837207/diff/1013/chrome/test/data/na...
chrome/test/data/nacl/DEPS:5: 
On 2012/08/13 22:16:59, Mark Seaborn wrote:
> Drop extra line (as Brad says for another file :-) )

Done.

https://chromiumcodereview.appspot.com/10837207/diff/1013/chrome/test/data/na...
File chrome/test/data/nacl/nacl_load_test.html (right):

https://chromiumcodereview.appspot.com/10837207/diff/1013/chrome/test/data/na...
chrome/test/data/nacl/nacl_load_test.html:1: <html>
On 2012/08/13 22:16:59, Mark Seaborn wrote:
> Shouldn't this file have a copyright message?  The codebase doesn't seem to be
> consistent here, but better to have one.

The cases where we have copyright notices in html files are extremely rare.  All
the same, I added it.

https://chromiumcodereview.appspot.com/10837207/diff/1013/chrome/test/data/na...
chrome/test/data/nacl/nacl_load_test.html:18: var e =
document.createElement("embed");
On 2012/08/13 22:16:59, Mark Seaborn wrote:
> Nit: use single quotes for consistency?  Also, rename 'e' -> 'embed' to avoid
> cryptic single-char variable names?

Done.

https://chromiumcodereview.appspot.com/10837207/diff/1013/chrome/test/data/na...
File chrome/test/data/nacl/nacl_test_data.gyp (right):

https://chromiumcodereview.appspot.com/10837207/diff/1013/chrome/test/data/na...
chrome/test/data/nacl/nacl_test_data.gyp:9: 'targets': [
On 2012/08/13 21:10:59, bradn wrote:
> This is pretty much what I recalled.
> A comment to note its in need of being refactored to a common file?

Done.

https://chromiumcodereview.appspot.com/10837207/diff/1013/chrome/test/data/na...
chrome/test/data/nacl/nacl_test_data.gyp:36: 'out_newlib32':
'<(PRODUCT_DIR)/nacl_test_data/newlib/>(nexe_target)_newlib_x32.nexe',
On 2012/08/13 22:16:59, Mark Seaborn wrote:
> Stupid question: is it really necessary to list all output architectures?

Long term, no.  Short term, yes.  This is how we change the target directories.
:/

https://chromiumcodereview.appspot.com/10837207/diff/1013/chrome/test/data/na...
chrome/test/data/nacl/nacl_test_data.gyp:48: '-lplatform',
On 2012/08/13 22:16:59, Mark Seaborn wrote:
> AFAICT, you don't use libplatform or libgio in the test you're adding.  (You
> don't use libpthread directly but it's probably still needed by "-lppapi".)

Done.

https://chromiumcodereview.appspot.com/10837207/diff/1013/chrome/test/data/na...
chrome/test/data/nacl/nacl_test_data.gyp:60: '-mno-tls-use-call',
On 2012/08/13 22:16:59, Mark Seaborn wrote:
> Why is this here?  It looks like it has been cargo-culted.  This is only for
use
> inside the IRT.

Done.

https://chromiumcodereview.appspot.com/10837207/diff/1013/chrome/test/data/na...
File chrome/test/data/nacl/simple.nmf (right):

https://chromiumcodereview.appspot.com/10837207/diff/1013/chrome/test/data/na...
chrome/test/data/nacl/simple.nmf:3: "x86-32": {"url": "simple_newlib_x32.nexe"},
On 2012/08/13 22:16:59, Mark Seaborn wrote:
> Can't this manifest file be auto-generated for newlib too?  We managed to
mostly
> get away from having to check NMF files for the Scons-based tests.

This is the plan, but I was trying to keep this CL somewhat reasonably sized.

https://chromiumcodereview.appspot.com/10837207/diff/1013/chrome/test/data/na...
chrome/test/data/nacl/simple.nmf:8: 
On 2012/08/13 21:10:59, bradn wrote:
> Drop extra line

Done.

https://chromiumcodereview.appspot.com/10837207/diff/1013/chrome/test/nacl/na...
File chrome/test/nacl/nacl_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10837207/diff/1013/chrome/test/nacl/na...
chrome/test/nacl/nacl_browsertest.cc:39: TestMessageHandler() : ok_(true) {}
On 2012/08/13 21:29:57, sky wrote:
> Add a virtual destructor.

Done.

https://chromiumcodereview.appspot.com/10837207/diff/1013/chrome/test/nacl/na...
chrome/test/nacl/nacl_browsertest.cc:65: MessageResponse HandleMessage(const
std::string& json) {
On 2012/08/13 21:29:57, sky wrote:
> virtual OVERRIDE

Done.

https://chromiumcodereview.appspot.com/10837207/diff/1013/chrome/test/nacl/na...
chrome/test/nacl/nacl_browsertest.cc:85: DictionaryValue *msg;
On 2012/08/13 21:29:57, sky wrote:
> * on the other side, eg DictionaryValue* msg. Search through this file for '
*',
> I think there are a handful of places that have it wrong still.

I fixed the "&" issues, but forgot about the "*" issues.
Done.

https://chromiumcodereview.appspot.com/10837207/diff/1013/chrome/test/nacl/na...
chrome/test/nacl/nacl_browsertest.cc:108: };
On 2012/08/13 21:29:57, sky wrote:
> DISALLOW_...

Abstract class.

[Nick Bray (chromium), 2012-08-13 23:16:59]

Forgot to remove the timer, sorry.  Doing it...

[Nick Bray (chromium), 2012-08-13 23:45:31]

Good to go, PTAL.

[sky, 2012-08-14 00:11:09]

LGTM

[commit-bot: I haz the power, 2012-08-14 23:13:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ncbray@chromium.org/10837207/12004

[commit-bot: I haz the power, 2012-08-15 02:10:31]

Try job failure for 10837207-12004 (retry) on win_rel for step
"interactive_ui_tests".
It's a second try, previously, step "interactive_ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-08-15 16:58:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ncbray@chromium.org/10837207/12004

[commit-bot: I haz the power, 2012-08-15 18:23:39]

Change committed as 151716