AppCache: Use a dedicated thread for the disk cache.

AppCache: Use a dedicated thread for the disk cache.

BUG=26730
TEST=current tests



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

[michaeln, 2010-05-27 01:59:57]

looks reasonable to me... but the unittests (webkit/test_shell_tests) and
test_shell (test_shell/simple_appcache_system.cc) need some poking

[rvargas (doing something else), 2010-05-29 01:53:19]

Please take another look.

[michaeln, 2010-05-31 17:59:13]

The use of the global g_cache_thread is kind of sketchy. The awkwardness of
having test_shell vs the all new and improved dump render tree in the works at
the same some is somewhat confusing... not to mention a separate target for
test_shell_tests. We end up defining g_cache_thread in multiple .cc files for
use depending on the target being built.

I wonder if a separate class which defines globals and is used in all targets
would help? Maybe...

class TestShellDiskCacheThread {
  // instanced in test_shell_webkit_init and support/test_webkit_client
  TestShellDiskCacheThread();

  // used by test_shell_request_context
  static MessageLoopProxy* message_loop_proxy();
};

http://codereview.chromium.org/2249005/diff/56001/57011
File webkit/support/test_webkit_client.cc (right):

http://codereview.chromium.org/2249005/diff/56001/57011#newcode55
webkit/support/test_webkit_client.cc:55: base::Thread* g_cache_thread = NULL;
and another

http://codereview.chromium.org/2249005/diff/56001/57013
File webkit/tools/test_shell/run_all_tests.cc (right):

http://codereview.chromium.org/2249005/diff/56001/57013#newcode44
webkit/tools/test_shell/run_all_tests.cc:44: base::Thread* g_cache_thread =
NULL;
here's another

http://codereview.chromium.org/2249005/diff/56001/57017
File webkit/tools/test_shell/test_shell_main.cc (right):

http://codereview.chromium.org/2249005/diff/56001/57017#newcode69
webkit/tools/test_shell/test_shell_main.cc:69: base::Thread* g_cache_thread =
NULL;
here's one definition of g_cache_thread

http://codereview.chromium.org/2249005/diff/56001/57018
File webkit/tools/test_shell/test_shell_request_context.cc (right):

http://codereview.chromium.org/2249005/diff/56001/57018#newcode27
webkit/tools/test_shell/test_shell_request_context.cc:27: extern base::Thread*
g_cache_thread;
ah... i was wondering where the global was used?

http://codereview.chromium.org/2249005/diff/56001/57020
File webkit/tools/test_shell/test_shell_webkit_init.cc (right):

http://codereview.chromium.org/2249005/diff/56001/57020#newcode5
webkit/tools/test_shell/test_shell_webkit_init.cc:5: #include
"webkit/tools/test_shell/test_shell_webkit_init.h"
Good call to split this into a .cc file seeing how big some of the method bodies
are. Would be really nice to create the new .cc file by svn copy'ing the .h file
so we retain the history of these method bodies.

http://codereview.chromium.org/2249005/diff/56001/57020#newcode23
webkit/tools/test_shell/test_shell_webkit_init.cc:23: extern base::Thread*
g_cache_thread;
and its used here too

[rvargas (doing something else), 2010-06-01 19:12:38]

On 2010/05/31 17:59:13, michaeln wrote:
> The use of the global g_cache_thread is kind of sketchy. The awkwardness of
> having test_shell vs the all new and improved dump render tree in the works at
> the same some is somewhat confusing... not to mention a separate target for
> test_shell_tests. We end up defining g_cache_thread in multiple .cc files for
> use depending on the target being built.

I'm not so happy about it either. I was trying to be very explicit about the
fact that now we have a global thread that should be managed as close to the
application main() as possible. The issue is that we have three test executables
that create an AppCache: test_shell, test_shell_tests (which we control) and RDT
(which we don't). I would prefer to have something like the code from patch set
#4, where the thread is encapsulated away, but that fails to connect DRT.

The real problem though (with the current approach, with our without
encapsulation) is that there is no connection between the thread and the request
context, so the request context can outlive the thread (destroyed with
TestWebkitClient), and even if there is no crash (the message loop is ref
counted), the destruction of the context will not work as expected, most likely
will hang.

I think it's better just to drop this and let these tests run with a memory-only
AppCache; we just have to makes sure that the differences in behavior are small
and covered by unit tests (the only thing should be that all responses are
synchronous but posted back again to make them look async, right?)

[rvargas (doing something else), 2010-06-01 20:08:40]

(I forgot to send this part)

http://codereview.chromium.org/2249005/diff/56001/57020
File webkit/tools/test_shell/test_shell_webkit_init.cc (right):

http://codereview.chromium.org/2249005/diff/56001/57020#newcode5
webkit/tools/test_shell/test_shell_webkit_init.cc:5: #include
"webkit/tools/test_shell/test_shell_webkit_init.h"
On 2010/05/31 17:59:13, michaeln wrote:
> Good call to split this into a .cc file seeing how big some of the method
bodies
> are. Would be really nice to create the new .cc file by svn copy'ing the .h
file
> so we retain the history of these method bodies.

yeah, I forgot about it. Thanks for pointing that out!.

[rvargas (doing something else), 2010-06-06 02:42:45]

Please take another look.

[michaeln, 2010-06-07 22:22:05]

LGTM