Move DnsApiTest.DnsResolveIPLiteral and DnsApiTest.DnsResolveHostname to app_shell_browsertests.

Move DnsApiTest.DnsResolveIPLiteral and DnsApiTest.DnsResolveHostname to app_shell_browsertests.

This results in a significant speedup; tests run in <500ms instead of >2s because the lengthy browser_tests startup is avoided.

This clones some aspects of extension_function_test_utils into extensions/browser/api_test_utils.cc. Later CLs will clean up the redundancy.

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

[Yoyo Zhou, 2014-07-16 02:30:26]

James, PTAL
Matt, please review the addition of "+net" to apps/shell/browser/DEPS

https://codereview.chromium.org/394103004/diff/1/apps/shell/browser/dns_apite...
File apps/shell/browser/dns_apitest.cc (right):

https://codereview.chromium.org/394103004/diff/1/apps/shell/browser/dns_apite...
apps/shell/browser/dns_apitest.cc:1: // Copyright 2014 The Chromium Authors. All
rights reserved.
I put this file in apps/shell/browser to avoid introducing an extensions->apps
dependency. The logical home for it is of course extensions/browser/api/dns.
Discuss.

https://codereview.chromium.org/394103004/diff/1/apps/shell/browser/dns_apite...
apps/shell/browser/dns_apitest.cc:70: TestFunctionDispatcherDelegate delegate;
Some of this setup (like here) is more difficult than before for writers of
tests. I'll have to look at some more tests where RunFunction.* functions are
used to determine whether this is a bad thing.

[mmenke, 2014-07-16 14:05:55]

https://codereview.chromium.org/394103004/diff/1/apps/shell/browser/dns_apite...
File apps/shell/browser/dns_apitest.cc (right):

https://codereview.chromium.org/394103004/diff/1/apps/shell/browser/dns_apite...
apps/shell/browser/dns_apitest.cc:8: #include "base/values.h"
You're missing the header for OVERRIDE.  Suggest just using macros.h for that
and DISALLOW_COPY_AND_ASSIGN.

https://codereview.chromium.org/394103004/diff/1/apps/shell/browser/dns_apite...
apps/shell/browser/dns_apitest.cc:17: #include "net/dns/mock_host_resolver.h"
Are you actual depending on this?  Looks like you just passing pointers to this
around, so could forward declare it instead (And since you're using functions
from other headers that return it, think you're fine not even forward declaring
it).

https://codereview.chromium.org/394103004/diff/1/apps/shell/browser/dns_apite...
apps/shell/browser/dns_apitest.cc:31: private:
DISALLOW_COPY_AND_ASSIGN (And include the requisite header)?

https://codereview.chromium.org/394103004/diff/1/apps/shell/browser/dns_apite...
apps/shell/browser/dns_apitest.cc:33: }
nit:  Linebreak before ending namespace.

https://codereview.chromium.org/394103004/diff/1/apps/shell/browser/dns_apite...
apps/shell/browser/dns_apitest.cc:33: }
nit:  "}  // namespace"

https://codereview.chromium.org/394103004/diff/1/apps/shell/browser/dns_apite...
apps/shell/browser/dns_apitest.cc:81:
static_cast<base::DictionaryValue*>(result.get());
No need for a cast, just use:

base::DictionaryValue* dict = NULL;
ASSERT_TRUE(result->GetAsDictionary(&dict));

https://codereview.chromium.org/394103004/diff/1/apps/shell/browser/dns_apite...
apps/shell/browser/dns_apitest.cc:83: int resultCode;
nit:  result_code.

https://codereview.chromium.org/394103004/diff/1/apps/shell/browser/dns_apite...
apps/shell/browser/dns_apitest.cc:85: EXPECT_EQ(net::OK, resultCode);
We're exposing Chrome network error codes to extensions?

[James Cook, 2014-07-16 17:15:32]

Awesome that this makes things so much faster.

https://codereview.chromium.org/394103004/diff/1/apps/shell/browser/dns_apite...
File apps/shell/browser/dns_apitest.cc (right):

https://codereview.chromium.org/394103004/diff/1/apps/shell/browser/dns_apite...
apps/shell/browser/dns_apitest.cc:1: // Copyright 2014 The Chromium Authors. All
rights reserved.
On 2014/07/16 02:30:25, Yoyo Zhou wrote:
> I put this file in apps/shell/browser to avoid introducing an extensions->apps
> dependency. The logical home for it is of course extensions/browser/api/dns.
> Discuss.

I think the time has come to move app_shell out of src/apps/shell into
src/extensions/shell. It's just going to get increasingly awkward as more
browser_tests and unit_tests are extracted from Chrome.  We're fighting against
src/apps in CLs like this, namespaces, unit tests, etc.  How should we go about
doing that move?

https://codereview.chromium.org/394103004/diff/1/apps/shell/browser/dns_apite...
apps/shell/browser/dns_apitest.cc:68:
resolve_function->set_extension(empty_extension.get());
The old code has set_has_callback(true) after this line. Is the omission
intentional?

https://codereview.chromium.org/394103004/diff/1/apps/shell/browser/dns_apite...
apps/shell/browser/dns_apitest.cc:118:
static_cast<base::DictionaryValue*>(result.get());
ditto

https://codereview.chromium.org/394103004/diff/1/apps/shell/browser/dns_apite...
apps/shell/browser/dns_apitest.cc:120: int resultCode;
ditto

https://codereview.chromium.org/394103004/diff/1/extensions/browser/api/dns/m...
File extensions/browser/api/dns/mock_host_resolver_creator.cc (right):

https://codereview.chromium.org/394103004/diff/1/extensions/browser/api/dns/m...
extensions/browser/api/dns/mock_host_resolver_creator.cc:9: #include
"testing/gtest/include/gtest/gtest.h"
optional: Do you even need this? You could replace the ASSERT_TRUE with a CHECK
or DCHECK.

https://codereview.chromium.org/394103004/diff/1/extensions/browser/api_test_...
File extensions/browser/api_test_utils.cc (right):

https://codereview.chromium.org/394103004/diff/1/extensions/browser/api_test_...
extensions/browser/api_test_utils.cc:26: else
nit: no need for else

https://codereview.chromium.org/394103004/diff/1/extensions/browser/api_test_...
File extensions/browser/api_test_utils.h (right):

https://codereview.chromium.org/394103004/diff/1/extensions/browser/api_test_...
extensions/browser/api_test_utils.h:26: //
chrome/browser/extensions/extension_function_test_utils.h.
Could you file a tracking bug for this?

https://codereview.chromium.org/394103004/diff/1/extensions/browser/api_test_...
extensions/browser/api_test_utils.h:59: scoped_ptr<ExtensionFunctionDispatcher>
dispatcher,
Why do these need a dispatcher explicitly passed in, rather than doing what the
old code does and creating it locally inside RunFunction()'s implementation? 
Because the chrome code does something with Browser?

Also, could the code in
chrome/browser/extensions/extension_function_test_utils.cc call this
implementation?  I know you would like to clean this stuff up later, but if that
change is simple what do you think of doing it now?

[Yoyo Zhou, 2014-07-18 02:45:31]

https://codereview.chromium.org/394103004/diff/1/apps/shell/browser/dns_apite...
File apps/shell/browser/dns_apitest.cc (right):

https://codereview.chromium.org/394103004/diff/1/apps/shell/browser/dns_apite...
apps/shell/browser/dns_apitest.cc:1: // Copyright 2014 The Chromium Authors. All
rights reserved.
On 2014/07/16 17:15:31, James Cook wrote:
> On 2014/07/16 02:30:25, Yoyo Zhou wrote:
> > I put this file in apps/shell/browser to avoid introducing an
extensions->apps
> > dependency. The logical home for it is of course extensions/browser/api/dns.
> > Discuss.
> 
> I think the time has come to move app_shell out of src/apps/shell into
> src/extensions/shell. It's just going to get increasingly awkward as more
> browser_tests and unit_tests are extracted from Chrome.  We're fighting
against
> src/apps in CLs like this, namespaces, unit tests, etc.  How should we go
about
> doing that move?

I'll move to src/extensions/shell after this CL. Filed crbug.com/394987 for it.

https://codereview.chromium.org/394103004/diff/1/apps/shell/browser/dns_apite...
apps/shell/browser/dns_apitest.cc:8: #include "base/values.h"
On 2014/07/16 14:05:54, mmenke wrote:
> You're missing the header for OVERRIDE.  Suggest just using macros.h for that
> and DISALLOW_COPY_AND_ASSIGN.

Done.

https://codereview.chromium.org/394103004/diff/1/apps/shell/browser/dns_apite...
apps/shell/browser/dns_apitest.cc:17: #include "net/dns/mock_host_resolver.h"
On 2014/07/16 14:05:54, mmenke wrote:
> Are you actual depending on this?  Looks like you just passing pointers to
this
> around, so could forward declare it instead (And since you're using functions
> from other headers that return it, think you're fine not even forward
declaring
> it).

It's used in line 42.

https://codereview.chromium.org/394103004/diff/1/apps/shell/browser/dns_apite...
apps/shell/browser/dns_apitest.cc:31: private:
On 2014/07/16 14:05:54, mmenke wrote:
> DISALLOW_COPY_AND_ASSIGN (And include the requisite header)?

Done.

https://codereview.chromium.org/394103004/diff/1/apps/shell/browser/dns_apite...
apps/shell/browser/dns_apitest.cc:33: }
On 2014/07/16 14:05:54, mmenke wrote:
> nit:  "}  // namespace"

Done.

https://codereview.chromium.org/394103004/diff/1/apps/shell/browser/dns_apite...
apps/shell/browser/dns_apitest.cc:68:
resolve_function->set_extension(empty_extension.get());
On 2014/07/16 17:15:31, James Cook wrote:
> The old code has set_has_callback(true) after this line. Is the omission
> intentional?

No. Don't know why it works without it, though. I'll add it back.

https://codereview.chromium.org/394103004/diff/1/apps/shell/browser/dns_apite...
apps/shell/browser/dns_apitest.cc:81:
static_cast<base::DictionaryValue*>(result.get());
On 2014/07/16 14:05:54, mmenke wrote:
> No need for a cast, just use:
> 
> base::DictionaryValue* dict = NULL;
> ASSERT_TRUE(result->GetAsDictionary(&dict));

Done.

https://codereview.chromium.org/394103004/diff/1/apps/shell/browser/dns_apite...
apps/shell/browser/dns_apitest.cc:83: int resultCode;
On 2014/07/16 14:05:54, mmenke wrote:
> nit:  result_code.

Done.

https://codereview.chromium.org/394103004/diff/1/apps/shell/browser/dns_apite...
apps/shell/browser/dns_apitest.cc:85: EXPECT_EQ(net::OK, resultCode);
On 2014/07/16 14:05:54, mmenke wrote:
> We're exposing Chrome network error codes to extensions?

Yes, in lots of places already; see
https://code.google.com/p/chromium/codesearch#search/&q=net::ok%20file:extens...

(In case you're concerned about adding this DEP on net/ to apps/shell, we're
actually going to move the apps/shell code to extensions/ soon, as James
mentioned above, so it won't be a new DEP after all since "+net" is already in
extensions/DEPS.)

https://codereview.chromium.org/394103004/diff/1/apps/shell/browser/dns_apite...
apps/shell/browser/dns_apitest.cc:118:
static_cast<base::DictionaryValue*>(result.get());
On 2014/07/16 17:15:31, James Cook wrote:
> ditto

Done.

https://codereview.chromium.org/394103004/diff/1/apps/shell/browser/dns_apite...
apps/shell/browser/dns_apitest.cc:120: int resultCode;
On 2014/07/16 17:15:31, James Cook wrote:
> ditto

Done.

https://codereview.chromium.org/394103004/diff/1/extensions/browser/api/dns/m...
File extensions/browser/api/dns/mock_host_resolver_creator.cc (right):

https://codereview.chromium.org/394103004/diff/1/extensions/browser/api/dns/m...
extensions/browser/api/dns/mock_host_resolver_creator.cc:9: #include
"testing/gtest/include/gtest/gtest.h"
On 2014/07/16 17:15:31, James Cook wrote:
> optional: Do you even need this? You could replace the ASSERT_TRUE with a
CHECK
> or DCHECK.

Yeah, CHECK seems better.

https://codereview.chromium.org/394103004/diff/1/extensions/browser/api_test_...
File extensions/browser/api_test_utils.cc (right):

https://codereview.chromium.org/394103004/diff/1/extensions/browser/api_test_...
extensions/browser/api_test_utils.cc:26: else
On 2014/07/16 17:15:31, James Cook wrote:
> nit: no need for else

I rewrote this since I had just copied it over and it was dumb.

https://codereview.chromium.org/394103004/diff/1/extensions/browser/api_test_...
File extensions/browser/api_test_utils.h (right):

https://codereview.chromium.org/394103004/diff/1/extensions/browser/api_test_...
extensions/browser/api_test_utils.h:26: //
chrome/browser/extensions/extension_function_test_utils.h.
On 2014/07/16 17:15:31, James Cook wrote:
> Could you file a tracking bug for this?

crbug.com/394840

https://codereview.chromium.org/394103004/diff/1/extensions/browser/api_test_...
extensions/browser/api_test_utils.h:59: scoped_ptr<ExtensionFunctionDispatcher>
dispatcher,
On 2014/07/16 17:15:31, James Cook wrote:
> Why do these need a dispatcher explicitly passed in, rather than doing what
the
> old code does and creating it locally inside RunFunction()'s implementation? 
> Because the chrome code does something with Browser?
> 
> Also, could the code in
> chrome/browser/extensions/extension_function_test_utils.cc call this
> implementation?  I know you would like to clean this stuff up later, but if
that
> change is simple what do you think of doing it now?

The old code uses a default TestFunctionDispatcherDelegate which gets the
browser's ExtensionWindowController. I'm hoping that a lot of tests aren't going
to need an ExtensionWindowController at all (like these DNS ones).

I changed the old RunFunction to call the new one. Let's see if it works...

[tfarina, 2014-07-18 02:47:37]

https://codereview.chromium.org/394103004/diff/1/apps/shell/browser/dns_apite...
File apps/shell/browser/dns_apitest.cc (right):

https://codereview.chromium.org/394103004/diff/1/apps/shell/browser/dns_apite...
apps/shell/browser/dns_apitest.cc:1: // Copyright 2014 The Chromium Authors. All
rights reserved.
On 2014/07/18 02:45:30, Yoyo Zhou wrote:
> On 2014/07/16 17:15:31, James Cook wrote:
> > On 2014/07/16 02:30:25, Yoyo Zhou wrote:
> > > I put this file in apps/shell/browser to avoid introducing an
> extensions->apps
> > > dependency. The logical home for it is of course
extensions/browser/api/dns.
> > > Discuss.
> > 
> > I think the time has come to move app_shell out of src/apps/shell into
> > src/extensions/shell. It's just going to get increasingly awkward as more
> > browser_tests and unit_tests are extracted from Chrome.  We're fighting
> against
> > src/apps in CLs like this, namespaces, unit tests, etc.  How should we go
> about
> > doing that move?
> 
> I'll move to src/extensions/shell after this CL. Filed crbug.com/394987 for
it.
extensions/shell :) Cool!

[James Cook, 2014-07-18 20:32:49]

LGTM with nits

https://codereview.chromium.org/394103004/diff/60001/apps/shell/browser/dns_a...
File apps/shell/browser/dns_apitest.cc (right):

https://codereview.chromium.org/394103004/diff/60001/apps/shell/browser/dns_a...
apps/shell/browser/dns_apitest.cc:86: int result_code;
nit: Init to 0 just so you don't read uninitialized memory if the GetInteger
fails

https://codereview.chromium.org/394103004/diff/60001/apps/shell/browser/dns_a...
apps/shell/browser/dns_apitest.cc:122: int result_code;
ditto

https://codereview.chromium.org/394103004/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/extension_function_test_utils.cc (right):

https://codereview.chromium.org/394103004/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_function_test_utils.cc:59: base::Value*
result(ParseJSON(data));
nit: I find "result = ParseJSON(data)" slightly easier to read.

https://codereview.chromium.org/394103004/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_function_test_utils.cc:250: // The cast is a
hack; these flags should be defined in only one place.
TODO(yoz) here

https://codereview.chromium.org/394103004/diff/60001/extensions/browser/api_t...
File extensions/browser/api_test_utils.cc (right):

https://codereview.chromium.org/394103004/diff/60001/extensions/browser/api_t...
extensions/browser/api_test_utils.cc:22: base::ListValue* ParseList(const
std::string& data) {
Should this use the cleaned-up implementation you wrote in the other file?

[mmenke, 2014-07-18 21:32:30]

LGTM

https://codereview.chromium.org/394103004/diff/1/apps/shell/browser/dns_apite...
File apps/shell/browser/dns_apitest.cc (right):

https://codereview.chromium.org/394103004/diff/1/apps/shell/browser/dns_apite...
apps/shell/browser/dns_apitest.cc:17: #include "net/dns/mock_host_resolver.h"
On 2014/07/18 02:45:30, Yoyo Zhou wrote:
> On 2014/07/16 14:05:54, mmenke wrote:
> > Are you actual depending on this?  Looks like you just passing pointers to
> this
> > around, so could forward declare it instead (And since you're using
functions
> > from other headers that return it, think you're fine not even forward
> declaring
> > it).
> 
> It's used in line 42.

Are you sure?  resolver_creator_->CreateMockHostResolver() calls a function that
returns a net::MockHostResolver*, but you never dereference it, or call any
method defined in MockHostResolver.  Looks like all you need is a forward
declaration.

https://codereview.chromium.org/394103004/diff/1/apps/shell/browser/dns_apite...
apps/shell/browser/dns_apitest.cc:85: EXPECT_EQ(net::OK, resultCode);
On 2014/07/18 02:45:30, Yoyo Zhou wrote:
> On 2014/07/16 14:05:54, mmenke wrote:
> > We're exposing Chrome network error codes to extensions?
> 
> Yes, in lots of places already; see
>
https://code.google.com/p/chromium/codesearch#search/&q=net::ok%20file:extens...
> 
> (In case you're concerned about adding this DEP on net/ to apps/shell, we're
> actually going to move the apps/shell code to extensions/ soon, as James
> mentioned above, so it won't be a new DEP after all since "+net" is already in
> extensions/DEPS.)

I'm not really concerned about more stuff depending on net/, it just seems weird
that the raw network error codes (Which don't really have any stability of
meaning guarantees - they don't change that often, but we occasionally split old
codes into new ones for debugging purposes) are exposed to extensions.

[Yoyo Zhou, 2014-07-22 00:10:31]

https://codereview.chromium.org/394103004/diff/1/apps/shell/browser/dns_apite...
File apps/shell/browser/dns_apitest.cc (right):

https://codereview.chromium.org/394103004/diff/1/apps/shell/browser/dns_apite...
apps/shell/browser/dns_apitest.cc:17: #include "net/dns/mock_host_resolver.h"
On 2014/07/18 21:32:30, mmenke wrote:
> On 2014/07/18 02:45:30, Yoyo Zhou wrote:
> > On 2014/07/16 14:05:54, mmenke wrote:
> > > Are you actual depending on this?  Looks like you just passing pointers to
> > this
> > > around, so could forward declare it instead (And since you're using
> functions
> > > from other headers that return it, think you're fine not even forward
> > declaring
> > > it).
> > 
> > It's used in line 42.
> 
> Are you sure?  resolver_creator_->CreateMockHostResolver() calls a function
that
> returns a net::MockHostResolver*, but you never dereference it, or call any
> method defined in MockHostResolver.  Looks like all you need is a forward
> declaration.

It turns out this is because the header was needed to know that a
MockHostResolver is a HostResolver. I've changed MockHostResolverCreator's
interface to return a HostResolver* (it seems logical for its consumers not to
care what kind of HostResolver it is).

https://codereview.chromium.org/394103004/diff/1/apps/shell/browser/dns_apite...
apps/shell/browser/dns_apitest.cc:85: EXPECT_EQ(net::OK, resultCode);
On 2014/07/18 21:32:30, mmenke wrote:
> On 2014/07/18 02:45:30, Yoyo Zhou wrote:
> > On 2014/07/16 14:05:54, mmenke wrote:
> > > We're exposing Chrome network error codes to extensions?
> > 
> > Yes, in lots of places already; see
> >
>
https://code.google.com/p/chromium/codesearch#search/&q=net::ok%20file:extens...
> > 
> > (In case you're concerned about adding this DEP on net/ to apps/shell, we're
> > actually going to move the apps/shell code to extensions/ soon, as James
> > mentioned above, so it won't be a new DEP after all since "+net" is already
in
> > extensions/DEPS.)
> 
> I'm not really concerned about more stuff depending on net/, it just seems
weird
> that the raw network error codes (Which don't really have any stability of
> meaning guarantees - they don't change that often, but we occasionally split
old
> codes into new ones for debugging purposes) are exposed to extensions.

This happens in other APIs too, I think, e.g.
https://developer.chrome.com/apps/sockets_tcp

"The result code returned from the underlying network call. A negative value
indicates an error."

This is probably not ideal, but it's what we have right now...

https://codereview.chromium.org/394103004/diff/60001/apps/shell/browser/dns_a...
File apps/shell/browser/dns_apitest.cc (right):

https://codereview.chromium.org/394103004/diff/60001/apps/shell/browser/dns_a...
apps/shell/browser/dns_apitest.cc:86: int result_code;
On 2014/07/18 20:32:48, James Cook wrote:
> nit: Init to 0 just so you don't read uninitialized memory if the GetInteger
> fails

Done.

https://codereview.chromium.org/394103004/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/extension_function_test_utils.cc (right):

https://codereview.chromium.org/394103004/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_function_test_utils.cc:59: base::Value*
result(ParseJSON(data));
On 2014/07/18 20:32:48, James Cook wrote:
> nit: I find "result = ParseJSON(data)" slightly easier to read.

Me too.

https://codereview.chromium.org/394103004/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_function_test_utils.cc:250: // The cast is a
hack; these flags should be defined in only one place.
On 2014/07/18 20:32:48, James Cook wrote:
> TODO(yoz) here

added

https://codereview.chromium.org/394103004/diff/60001/extensions/browser/api_t...
File extensions/browser/api_test_utils.cc (right):

https://codereview.chromium.org/394103004/diff/60001/extensions/browser/api_t...
extensions/browser/api_test_utils.cc:22: base::ListValue* ParseList(const
std::string& data) {
On 2014/07/18 20:32:48, James Cook wrote:
> Should this use the cleaned-up implementation you wrote in the other file?

Woops. I meant to do that here.

[Yoyo Zhou, 2014-07-22 00:13:01]

rebase

[Yoyo Zhou, 2014-07-22 21:26:16]

rebase2

[Yoyo Zhou, 2014-07-22 21:26:51]

The CQ bit was checked by yoz@chromium.org

[commit-bot: I haz the power, 2014-07-22 21:28:09]

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

[Yoyo Zhou, 2014-07-22 22:02:23]

The CQ bit was unchecked by yoz@chromium.org

[Yoyo Zhou, 2014-07-22 22:10:10]

The CQ bit was checked by yoz@chromium.org

[commit-bot: I haz the power, 2014-07-22 22:11:56]

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

[commit-bot: I haz the power, 2014-07-23 01:22:02]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)

[commit-bot: I haz the power, 2014-07-23 02:11:43]

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

[commit-bot: I haz the power, 2014-07-23 02:11:44]

Try jobs failed on following builders:
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)

[Yoyo Zhou, 2014-07-23 02:20:15]

The CQ bit was checked by yoz@chromium.org

[commit-bot: I haz the power, 2014-07-23 02:22:31]

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

[commit-bot: I haz the power, 2014-07-23 06:52:44]

Change committed as 284872