Create a secondary file thread, FILE_USER_BLOCKING...

Create a secondary file thread, FILE_USER_BLOCKING, for tasks requiring file IO which have user waiting for the results. Process appcache background tasks on that thread.

BUG=68894, 78359
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=113317

[michaeln, 2011-10-21 01:32:42]

also intend to make more use of this thread for filestream backed response
storage, will scyncly open filehandles on this thread for asycnc reading/writing
back on the IO thread

[michaeln, 2011-10-21 02:24:43]

i still have to fixup some unittests with mock APPCACHE threads

[michaeln, 2011-10-21 19:34:55]

new snapshot with some fixed up unittests

[rvargas (doing something else), 2011-10-21 19:46:17]

http://codereview.chromium.org/8366020/diff/3019/chrome/browser/browser_proce...
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/8366020/diff/3019/chrome/browser/browser_proce...
chrome/browser/browser_process_impl.cc:921: new
BrowserThread(BrowserThread::APPCACHE));
Just double checking that you don't intend to use async io on this thread.

http://codereview.chromium.org/8366020/diff/3019/chrome/browser/chrome_browse...
File chrome/browser/chrome_browser_main.cc (right):

http://codereview.chromium.org/8366020/diff/3019/chrome/browser/chrome_browse...
chrome/browser/chrome_browser_main.cc:318: process->appcache_thread();
Do you mind creating this thread before the cache thread. I'd prefer to keep the
cache and io thread together so that they are easier to identify on crash dumps.

http://codereview.chromium.org/8366020/diff/3019/content/browser/browser_thre...
File content/browser/browser_thread.cc (right):

http://codereview.chromium.org/8366020/diff/3019/content/browser/browser_thre...
content/browser/browser_thread.cc:20: "Chrome_AppCacheThread",  // APPCACHE
and here

http://codereview.chromium.org/8366020/diff/3019/content/browser/browser_thre...
File content/browser/browser_thread.h (right):

http://codereview.chromium.org/8366020/diff/3019/content/browser/browser_thre...
content/browser/browser_thread.h:69: APPCACHE,
ditto: before the cache thread?

http://codereview.chromium.org/8366020/diff/3019/content/shell/shell_browser_...
File content/shell/shell_browser_main.cc (right):

http://codereview.chromium.org/8366020/diff/3019/content/shell/shell_browser_...
content/shell/shell_browser_main.cc:65: appcache_thread_.reset(new
BrowserProcessSubThread(BrowserThread::APPCACHE));
and here

[michaeln, 2011-10-21 19:52:12]

http://codereview.chromium.org/8366020/diff/3019/chrome/browser/browser_proce...
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/8366020/diff/3019/chrome/browser/browser_proce...
chrome/browser/browser_process_impl.cc:921: new
BrowserThread(BrowserThread::APPCACHE));
On 2011/10/21 19:46:17, rvargas wrote:
> Just double checking that you don't intend to use async io on this thread.

definitely not right now, i don't anticipate doing so ever, if i eventually do
i'll change the options accordingly

http://codereview.chromium.org/8366020/diff/3019/chrome/browser/chrome_browse...
File chrome/browser/chrome_browser_main.cc (right):

http://codereview.chromium.org/8366020/diff/3019/chrome/browser/chrome_browse...
chrome/browser/chrome_browser_main.cc:318: process->appcache_thread();
On 2011/10/21 19:46:17, rvargas wrote:
> Do you mind creating this thread before the cache thread. I'd prefer to keep
the
> cache and io thread together so that they are easier to identify on crash
dumps.

will do (ditto other necessary places)

[michaeln, 2011-10-21 20:03:47]

changed the ordering of APPCACHE and CACHE threads

[michaeln, 2011-10-21 21:15:43]

tryjobs with the original ordering look good (patch set 8), starting some jobs
with the new ordering

[michaeln, 2011-10-21 22:23:17]

On 2011/10/21 21:15:43, michaeln wrote:
> tryjobs with the original ordering look good (patch set 8), starting some jobs
> with the new ordering

looks like i screwed something up with the new ordering :(

[rvargas (doing something else), 2011-10-21 23:11:16]

LGTM (not content owner)

[michaeln, 2011-10-21 23:33:50]

@darin, can you do the content review thing?

[michaeln, 2011-10-24 19:01:19]

@darin, ping

[michaeln, 2011-10-25 19:48:33]

jam, here's the CL i was talking to you about yesterday evening

[jam, 2011-10-25 23:55:49]

just commented on http://code.google.com/p/chromium/issues/detail?id=101602
about this, can we have this conversation in that bug and all agree on what the
new thread(s) would be called/used for? i'd like to hear back from darin as well

[jam, 2011-11-21 16:47:17]

can we make this more generic, i.e. FILE_USER_BLOCKING (please suggest better
names if you have some), that way we can move other tasks to it?

[michaeln, 2011-11-21 21:00:59]

On 2011/11/21 16:47:17, John Abd-El-Malek wrote:
> can we make this more generic, i.e. FILE_USER_BLOCKING (please suggest better
> names if you have some), that way we can move other tasks to it?

sure... i'll update the CL and ping you

[michaeln, 2011-11-23 00:10:20]

ping
> sure... i'll update the CL and ping you

[michaeln, 2011-11-23 18:37:26]

Looks like I have to poke at chromeframe to start the new thread there too?
[1122/200014:FATAL:appcache_storage_impl.cc(190)] Check failed: false. The
database thread is not running.

[jam, 2011-11-23 18:49:21]

On 2011/11/23 18:37:26, michaeln wrote:
> Looks like I have to poke at chromeframe to start the new thread there too?
> [1122/200014:FATAL:appcache_storage_impl.cc(190)] Check failed: false. The
> database thread is not running.

joi is soon landing http://codereview.chromium.org/8477004/, which will make the
thread creation happen in one place only (content), which should make this
easier

[michaeln, 2011-11-23 19:12:25]

i see the fake_external_tab.cc thing that i missed

[michaeln, 2011-11-29 02:43:11]

ptal, merged with joi's changes

[jam, 2011-11-29 17:31:09]

http://codereview.chromium.org/8366020/diff/38005/chrome/browser/browser_proc...
File chrome/browser/browser_process.h (right):

http://codereview.chromium.org/8366020/diff/38005/chrome/browser/browser_proc...
chrome/browser/browser_process.h:129: virtual base::Thread*
file_user_blocking_thread() = 0;
Joi is actually planning on removing the base::Thread getters for threads in
BrowserThread (since it interfers with having BrowserThread just be an interface
that content provides). It looks like this isn't needed, so please remove it

[michaeln, 2011-11-29 21:05:28]

http://codereview.chromium.org/8366020/diff/38005/chrome/browser/browser_proc...
File chrome/browser/browser_process.h (right):

http://codereview.chromium.org/8366020/diff/38005/chrome/browser/browser_proc...
chrome/browser/browser_process.h:129: virtual base::Thread*
file_user_blocking_thread() = 0;
On 2011/11/29 17:31:09, John Abd-El-Malek wrote:
> Joi is actually planning on removing the base::Thread getters for threads in
> BrowserThread (since it interfers with having BrowserThread just be an
interface
> that content provides). It looks like this isn't needed, so please remove it

Done.

[jam, 2011-11-30 21:52:06]

lgtm