Fix flakiness in RemoteFileSystemExtensionApiTests

Fix flakiness in RemoteFileSystemExtensionApiTests

We get in trouble because FileBrowserEventRouter creates gdata system service
early on (before we setup test service factory). When we create test system
service, the old one gets shutdown and destroyed, but they both use the same
gdata cache root path (which is created and destroyed async).

This patch avoid use of GDataSystemServiceFactory::SetTestingFactoryAndUse,
which cannot be called before an system service instance is already alive
(created early on by FileBroserEventRouter).

the flakiness was introduced by https://src.chromium.org/viewvc/chrome?view=rev&revision=147208

TEST=RemoteFileSystemExtensionApiTest.*
BUG=none

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

[tbarzic, 2012-07-20 15:41:30]

Hey, can you take a look at this?

I'm not too thrilled by this solution (I'm open to suggestions; I hope I'm
missing something obvious :) ), but it should work..

One alternative would be to keep GetWithCustomDocumentsServiceForTesting and set
the cache root to some custom path (and thus avoid race in initialization and
deinitialization of the cache).
I don't like that solution because file browser event router would not observe
live file system for dir changed events (and I plan to add some test that
verifies the extension receives appropriate events on file creation/deletion).

[satorux1, 2012-07-20 17:01:25]

http://codereview.chromium.org/10805034/diff/2001/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_system_service.h (right):

http://codereview.chromium.org/10805034/diff/2001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_system_service.h:102: static void
set_cache_root_for_test(const char* cache_root);
let's use a std::string.

const std::string&

http://codereview.chromium.org/10805034/diff/2001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_system_service.h:115: static const char*
test_cache_root_;
static std::string* test_cache_root_

can we just have them in anonymous namespace, rather than static class members?
would be nice to have less things in .h file.

[tbarzic, 2012-07-20 18:34:05]

https://chromiumcodereview.appspot.com/10805034/diff/2001/chrome/browser/chro...
File chrome/browser/chromeos/gdata/gdata_system_service.h (right):

https://chromiumcodereview.appspot.com/10805034/diff/2001/chrome/browser/chro...
chrome/browser/chromeos/gdata/gdata_system_service.h:102: static void
set_cache_root_for_test(const char* cache_root);
On 2012/07/20 17:01:25, satorux1 wrote:
> let's use a std::string.
> 
> const std::string&

Done.

https://chromiumcodereview.appspot.com/10805034/diff/2001/chrome/browser/chro...
chrome/browser/chromeos/gdata/gdata_system_service.h:115: static const char*
test_cache_root_;
On 2012/07/20 17:01:25, satorux1 wrote:
> static std::string* test_cache_root_
> 
> can we just have them in anonymous namespace, rather than static class
members?
> would be nice to have less things in .h file.

Done.

[satorux1, 2012-07-21 06:49:12]

http://codereview.chromium.org/10805034/diff/9/chrome/browser/chromeos/gdata/...
File chrome/browser/chromeos/gdata/gdata_system_service.cc (right):

http://codereview.chromium.org/10805034/diff/9/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_system_service.cc:171: g_test_cache_root =
&cache_root;
this looks unsafe... It assumes that the caller holds the string.

Maybe:

  if (g_test_cache_root)
     delete g_test_cache_root;
  if (!cache_root.empty())
    g_test_cache_root = new std::string(cache_root);

[tbarzic, 2012-07-23 19:38:20]

http://codereview.chromium.org/10805034/diff/9/chrome/browser/chromeos/gdata/...
File chrome/browser/chromeos/gdata/gdata_system_service.cc (right):

http://codereview.chromium.org/10805034/diff/9/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_system_service.cc:171: g_test_cache_root =
&cache_root;
On 2012/07/21 06:49:13, satorux1 wrote:
> this looks unsafe... It assumes that the caller holds the string.
> 
> Maybe:
> 
>   if (g_test_cache_root)
>      delete g_test_cache_root;
>   if (!cache_root.empty())
>     g_test_cache_root = new std::string(cache_root);
> 

yeah, Friday is the day for unsafe code :)

Done.

[satorux1, 2012-07-23 20:01:42]

LGTM

[commit-bot: I haz the power, 2012-07-23 21:38:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tbarzic@chromium.org/10805034/7003

[commit-bot: I haz the power, 2012-07-23 23:06:30]

Change committed as 147980