Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(536)

Issue 8477004: Have content/ create and destroy its own threads. (Closed)

Created:
9 years, 1 month ago by Jói
Modified:
9 years ago
CC:
chromium-reviews, cbentzel+watch_chromium.org, jennb, prasadt, Erik does not do reviews, Dmitry Titov, dcheng, mihaip+watch_chromium.org, joi+watch-content_chromium.org, Aaron Boodman, darin-cc_chromium.org, jianli, brettw-cc_chromium.org, robertshield, dpranke-watch+content_chromium.org, amit
Visibility:
Public.

Description

Have content/ create and destroy its own threads. (Re-land) Change embedding API and embedders to allow for this. Push inheritance of base::Thread down to content::BrowserThreadImpl so that content::BrowserThread is just a namespace for API functions. This change temporarily disables chrome_frame_net_tests as agreed by the CF lead, see bug 105435. TBR=ben@chromium.org (IWYU change only) BUG=98716, 104578, 105435 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=111695 Reverted (problems on official bot): r111698 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=111705

Patch Set 1 #

Patch Set 2 : Merge to lkgr. More IWYU. #

Patch Set 3 : Fix a couple of issues, more IWYU. #

Patch Set 4 : Fix placement of thread shutdown. #

Patch Set 5 : More IWYU (other platforms). #

Patch Set 6 : Further include fixes. #

Patch Set 7 : More IWYU, missing link-time dependency for Chrome Frame. #

Total comments: 6

Patch Set 8 : Thread restrictions need to be lifted for all embedders. #

Patch Set 9 : Fix shutdown sequence. Merge to lkgr. #

Patch Set 10 : Merge to head. #

Patch Set 11 : With this patchset, Chrome runs and exits normally on Linux. #

Total comments: 6

Patch Set 12 : Should pass most tests. Removed pure IWYU changes, will do in follow-up. #

Patch Set 13 : Merge to LKGR 11/22 #

Patch Set 14 : Merge to LKGR 11/23 #

Patch Set 15 : Temporary - look at browser_list, browser_main_loop, content/p/b/browser_shutdown #

Total comments: 2

Patch Set 16 : Introduce and use BrowserThreadDelegate, BMP::Pre/PostStartThread, to make this a safe refactoring. #

Total comments: 2

Patch Set 17 : Merge only, to LKGR 11/24. #

Patch Set 18 : Merge only, to LKGR 11/25. #

Patch Set 19 : Temporarily disable CF net tests, add probable fix for them, update suppression. #

Total comments: 2

Patch Set 20 : Fix official build, avoid DCHECK in official Linux/ChromeOS builds. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+966 lines, -519 lines) Patch
M chrome/browser/browser_process_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 6 chunks +23 lines, -28 lines 0 comments Download
M chrome/browser/browser_process_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 10 chunks +85 lines, -154 lines 0 comments Download
M chrome/browser/browser_shutdown.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 18 1 chunk +12 lines, -7 lines 0 comments Download
M chrome/browser/browser_shutdown.cc View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +5 lines, -1 line 0 comments Download
M chrome/browser/chrome_browser_main.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 18 4 chunks +20 lines, -0 lines 0 comments Download
M chrome/browser/chrome_browser_main.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 21 chunks +100 lines, -73 lines 0 comments Download
M chrome/browser/chromeos/input_method/xkeyboard.cc View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +5 lines, -3 lines 0 comments Download
M chrome/browser/chromeos/login/login_utils_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 7 chunks +21 lines, -14 lines 0 comments Download
M chrome/browser/chromeos/login/webui_screen_locker.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +6 lines, -9 lines 0 comments Download
M chrome/browser/io_thread.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 3 chunks +37 lines, -10 lines 0 comments Download
M chrome/browser/io_thread.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 8 chunks +37 lines, -16 lines 0 comments Download
M chrome/browser/sync/tools/DEPS View 1 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/sync/tools/sync_listen_notifications.cc View 1 3 chunks +4 lines, -3 lines 0 comments Download
M chrome/browser/ui/browser_list.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 18 3 chunks +9 lines, -18 lines 0 comments Download
M chrome/browser/ui/gtk/gtk_util.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +5 lines, -2 lines 0 comments Download
M chrome/browser/ui/gtk/select_file_dialog_impl_kde.cc View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +5 lines, -2 lines 0 comments Download
M chrome/tools/profiles/generate_profile.cc View 1 2 3 4 5 2 chunks +3 lines, -2 lines 0 comments Download
M chrome_frame/chrome_frame.gyp View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -0 lines 0 comments Download
M chrome_frame/test/net/fake_external_tab.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 3 chunks +9 lines, -2 lines 0 comments Download
M chrome_frame/test/net/fake_external_tab.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 6 chunks +34 lines, -10 lines 0 comments Download
M content/browser/browser_main_loop.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 18 4 chunks +17 lines, -0 lines 0 comments Download
M content/browser/browser_main_loop.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 18 5 chunks +199 lines, -0 lines 0 comments Download
M content/browser/browser_process_sub_thread.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 18 2 chunks +4 lines, -2 lines 0 comments Download
M content/browser/browser_thread_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 18 2 chunks +23 lines, -5 lines 0 comments Download
M content/browser/browser_thread_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 7 chunks +90 lines, -70 lines 0 comments Download
M content/content_browser.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +2 lines, -0 lines 0 comments Download
M content/public/browser/browser_main_parts.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 18 3 chunks +33 lines, -0 lines 0 comments Download
A content/public/browser/browser_shutdown.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 18 1 chunk +32 lines, -0 lines 0 comments Download
M content/public/browser/browser_thread.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 5 chunks +44 lines, -37 lines 0 comments Download
A content/public/browser/browser_thread_delegate.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +32 lines, -0 lines 0 comments Download
M content/shell/shell_browser_context.cc View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +5 lines, -2 lines 0 comments Download
M content/shell/shell_browser_main.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 18 1 chunk +8 lines, -11 lines 0 comments Download
M content/shell/shell_browser_main.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 18 2 chunks +16 lines, -33 lines 0 comments Download
M content/test/test_browser_thread.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 2 chunks +6 lines, -2 lines 0 comments Download
M content/test/test_browser_thread.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 2 chunks +30 lines, -2 lines 0 comments Download
M tools/valgrind/memcheck/suppressions.txt View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +1 line, -1 line 0 comments Download
M views/accessible_pane_view.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 21 (0 generated)
Jói
jam: Main reviewer, content/ owner. Other owners FYI, will TBR if I don't hear back ...
9 years, 1 month ago (2011-11-07 15:56:34 UTC) #1
jam
lgtm http://codereview.chromium.org/8477004/diff/6085/chrome/browser/ui/gtk/dialogs_kde.cc File chrome/browser/ui/gtk/dialogs_kde.cc (right): http://codereview.chromium.org/8477004/diff/6085/chrome/browser/ui/gtk/dialogs_kde.cc#newcode5 chrome/browser/ui/gtk/dialogs_kde.cc:5: #include "content/public/browser/browser_thread.h" can we sort this now that ...
9 years, 1 month ago (2011-11-07 21:43:23 UTC) #2
eroman
LGTM for browser/net/*
9 years, 1 month ago (2011-11-07 21:58:21 UTC) #3
cpu_(ooo_6.6-7.5)
lgtm for chrome/browser/component_updater*
9 years, 1 month ago (2011-11-08 02:15:13 UTC) #4
Mattias Nissler (ping if slow)
chrome/browser/policy, chrome/browser/prefs LGTM. Trybots are pretty red though :)
9 years, 1 month ago (2011-11-08 09:28:13 UTC) #5
tim (not reviewing)
LGTM for browser/sync/* On Tue, Nov 8, 2011 at 1:28 AM, <mnissler@chromium.org> wrote: > chrome/browser/policy, ...
9 years, 1 month ago (2011-11-08 14:39:00 UTC) #6
Jói
John, please take another look. This isn't quite ready yet as you'll see from the ...
9 years, 1 month ago (2011-11-11 16:52:42 UTC) #7
robertshield
Drive by comments on this most awesome of CLs :-) http://codereview.chromium.org/8477004/diff/21001/chrome/browser/browser_process_impl.cc File chrome/browser/browser_process_impl.cc (right): http://codereview.chromium.org/8477004/diff/21001/chrome/browser/browser_process_impl.cc#newcode254 ...
9 years, 1 month ago (2011-11-14 04:15:55 UTC) #8
jam
1) sorry for the late reply, I just saw this 2) it's getting really hard ...
9 years, 1 month ago (2011-11-17 21:42:26 UTC) #9
jam
http://codereview.chromium.org/8477004/diff/21001/content/public/browser/browser_main_parts.h File content/public/browser/browser_main_parts.h (right): http://codereview.chromium.org/8477004/diff/21001/content/public/browser/browser_main_parts.h#newcode88 content/public/browser/browser_main_parts.h:88: virtual IOThreadDelegate* PreMainMessageLoopRun() = 0; it seems a little ...
9 years, 1 month ago (2011-11-17 21:51:20 UTC) #10
Jói
Hi John, As requested, I reduced the number of files in the change quite a ...
9 years, 1 month ago (2011-11-18 23:21:43 UTC) #11
jam
lgtm, thanks for reducing the number of files, this makes it much easier to review ...
9 years, 1 month ago (2011-11-19 02:17:09 UTC) #12
Jói
I've tried using MessageLoop::AddDestructionObserver and the problem is that when the message loop is destructed ...
9 years, 1 month ago (2011-11-23 16:49:17 UTC) #13
jam
patchset 15 looks good, just one comment about location of function http://codereview.chromium.org/8477004/diff/41007/content/public/browser/browser_shutdown.h File content/public/browser/browser_shutdown.h (right): ...
9 years, 1 month ago (2011-11-23 18:30:51 UTC) #14
Jói
Hi John, I believe at this point, given our discussions today, this is close enough ...
9 years, 1 month ago (2011-11-23 22:13:39 UTC) #15
jam
lgtm http://codereview.chromium.org/8477004/diff/43013/content/public/browser/browser_thread_delegate.h File content/public/browser/browser_thread_delegate.h (right): http://codereview.chromium.org/8477004/diff/43013/content/public/browser/browser_thread_delegate.h#newcode21 content/public/browser/browser_thread_delegate.h:21: BrowserThreadDelegate() {} nit: we've been avoiding these in ...
9 years, 1 month ago (2011-11-23 22:39:35 UTC) #16
Jói
Hi Greg, Since Robert's away today, could you take a quick look at the updated ...
9 years ago (2011-11-25 16:28:24 UTC) #17
grt (UTC plus 2)
LGTM with one nit you're welcome to ignore since it's a temporary thing. http://codereview.chromium.org/8477004/diff/53005/chrome_frame/test/net/fake_external_tab.cc File ...
9 years ago (2011-11-25 16:35:55 UTC) #18
Jói
http://codereview.chromium.org/8477004/diff/53005/chrome_frame/test/net/fake_external_tab.cc File chrome_frame/test/net/fake_external_tab.cc (right): http://codereview.chromium.org/8477004/diff/53005/chrome_frame/test/net/fake_external_tab.cc#newcode619 chrome_frame/test/net/fake_external_tab.cc:619: LOG(INFO) << "Temporarily not running any ChromeFrame " << ...
9 years ago (2011-11-25 16:38:10 UTC) #19
Jói
http://codereview.chromium.org/8477004/diff/43013/content/public/browser/browser_thread_delegate.h File content/public/browser/browser_thread_delegate.h (right): http://codereview.chromium.org/8477004/diff/43013/content/public/browser/browser_thread_delegate.h#newcode21 content/public/browser/browser_thread_delegate.h:21: BrowserThreadDelegate() {} On 2011/11/23 22:39:35, John Abd-El-Malek wrote: > ...
9 years ago (2011-11-28 12:51:14 UTC) #20
Jói
9 years ago (2011-11-28 13:07:59 UTC) #21
TBR ben@ for IWYU-only change in views/ (added #include "base/message_loop.h")

Committed as r111695.

Cheers,
Jói


On Mon, Nov 28, 2011 at 12:51 PM,  <joi@chromium.org> wrote:
>
>
http://codereview.chromium.org/8477004/diff/43013/content/public/browser/brow...
> File content/public/browser/browser_thread_delegate.h (right):
>
>
http://codereview.chromium.org/8477004/diff/43013/content/public/browser/brow...
> content/public/browser/browser_thread_delegate.h:21:
> BrowserThreadDelegate() {}
> On 2011/11/23 22:39:35, John Abd-El-Malek wrote:
>>
>> nit: we've been avoiding these in the content API interfaces
>
>
> Done.
>
> http://codereview.chromium.org/8477004/

Powered by Google App Engine
This is Rietveld 408576698