Third patch in getting rid of caching MessageLoop pointers and always using C...

Third patch in getting rid of caching MessageLoop pointers and always using ChromeThread instead.

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

[jam, 2009-10-30 22:38:33]

Apologies this got much bigger than I thought it would.

[tim (not reviewing), 2009-10-30 22:57:11]

http://codereview.chromium.org/342068/diff/5086/6024
File chrome/browser/sync/profile_sync_service_unittest.cc (right):

http://codereview.chromium.org/342068/diff/5086/6024#newcode469
Line 469: ChromeThread file_thread_;
I'm curious what this is for; and why 'file'?

http://codereview.chromium.org/342068/diff/5086/6023
File chrome/browser/sync/sync_setup_wizard_unittest.cc (right):

http://codereview.chromium.org/342068/diff/5086/6023#newcode170
Line 170: ChromeThread file_thread_;
why 'file'?

[jam, 2009-10-30 23:17:33]

On Fri, Oct 30, 2009 at 3:57 PM, <tim@chromium.org> wrote:

>
> http://codereview.chromium.org/342068/diff/5086/6024
> File chrome/browser/sync/profile_sync_service_unittest.cc (right):
>
> http://codereview.chromium.org/342068/diff/5086/6024#newcode469
> Line 469: ChromeThread file_thread_;
> I'm curious what this is for; and why 'file'?
>

The bookmarkmodel code uses a file thread to write stuff to disk.
 Previously, the production code would branch depending on whether it's in a
unit test.  This is not optimal, since it can also lead to subtle bugs on
users' machines.  Better to keep the complexity for tests in tests.  In
these cases, it's not much complexity, it just fakes the current thread as a
file thread and calls RunAllPending to run all the posted tasks.

This is a model that we're moving to in order to improve the reliability of
the browser process.  I'm going to send an email to chromium-dev once I'm
done with these changes.


> http://codereview.chromium.org/342068/diff/5086/6023
> File chrome/browser/sync/sync_setup_wizard_unittest.cc (right):
>
> http://codereview.chromium.org/342068/diff/5086/6023#newcode170
> Line 170: ChromeThread file_thread_;
> why 'file'?
>
>
> http://codereview.chromium.org/342068
>

[Evan Stade, 2009-10-30 23:21:57]

mostly lg

http://codereview.chromium.org/342068/diff/1/13
File chrome/browser/autocomplete/history_url_provider_unittest.cc (right):

http://codereview.chromium.org/342068/diff/1/13#newcode99
Line 99: HistoryURLProviderTest() : file_thread_(ChromeThread::FILE,
&message_loop_) {}
nit: blank line after this

http://codereview.chromium.org/342068/diff/1/71
File chrome/browser/bookmarks/bookmark_storage.cc (right):

http://codereview.chromium.org/342068/diff/1/71#newcode138
Line 138: ChromeThread::FILE, FROM_HERE, new BackupTask(writer_.path()));
maybe in cases like this where you keep referencing ChromeThread::FILE you could
pull it out into a static const var? shrug

http://codereview.chromium.org/342068/diff/1/15
File chrome/browser/browsing_data_remover.cc (left):

http://codereview.chromium.org/342068/diff/1/15#oldcode139
Line 139: UserMetrics::RecordAction(L"ClearBrowsingData_Cache", profile_);
why are these two lines (138-139) deleted?

http://codereview.chromium.org/342068/diff/1/15
File chrome/browser/browsing_data_remover.cc (right):

http://codereview.chromium.org/342068/diff/1/15#newcode136
Line 136: ChromeThread::IO, FROM_HERE,
indentation

http://codereview.chromium.org/342068/diff/1/58
File chrome/browser/dom_ui/shown_sections_handler_unittest.cc (right):

http://codereview.chromium.org/342068/diff/1/58#newcode17
Line 17: PrefService pref((FilePath()));
nit: too many ()

http://codereview.chromium.org/342068/diff/1/44
File chrome/browser/gtk/dialogs_gtk.cc (right):

http://codereview.chromium.org/342068/diff/1/44#newcode152
Line 152: DCHECK(!ChromeThread::CurrentlyOn(ChromeThread::UI));
check should just be that we _are_ on the ui thread

http://codereview.chromium.org/342068/diff/1/74
File chrome/browser/memory_details.cc (right):

http://codereview.chromium.org/342068/diff/1/74#newcode39
Line 39: DCHECK(!ChromeThread::CurrentlyOn(ChromeThread::IO));
can we just check that we are on the ui thread?

http://codereview.chromium.org/342068/diff/1/40
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/342068/diff/1/40#newcode122
Line 122: // that code be moved to chrome\browser instead of being in net, so
that it
nit:/\\/\//

http://codereview.chromium.org/342068/diff/1/39
File chrome/browser/net/dns_master_unittest.cc (right):

http://codereview.chromium.org/342068/diff/1/39#newcode553
Line 553: 
so many extra lines oO

http://codereview.chromium.org/342068/diff/1/56
File chrome/browser/renderer_host/buffered_resource_handler.h (right):

http://codereview.chromium.org/342068/diff/1/56#newcode68
Line 68: static void OnPluginsLoaded(BufferedResourceHandler* handler);
A static method that takes an instance of the class?

http://codereview.chromium.org/342068/diff/1/26
File chrome/browser/sync/sync_setup_wizard.cc (right):

http://codereview.chromium.org/342068/diff/1/26#newcode148
Line 148: sync_source->AddRef();
is this because there's a floating ref or something?

http://codereview.chromium.org/342068/diff/1/85
File chrome/common/pref_service_unittest.cc (right):

http://codereview.chromium.org/342068/diff/1/85#newcode338
Line 338: PrefService prefs((FilePath()));
extra parens here and below

[jam, 2009-10-30 23:47:00]

http://codereview.chromium.org/342068/diff/1/13
File chrome/browser/autocomplete/history_url_provider_unittest.cc (right):

http://codereview.chromium.org/342068/diff/1/13#newcode99
Line 99: HistoryURLProviderTest() : file_thread_(ChromeThread::FILE,
&message_loop_) {}
On 2009/10/30 23:21:57, Evan Stade wrote:
> nit: blank line after this

Done.

http://codereview.chromium.org/342068/diff/1/71
File chrome/browser/bookmarks/bookmark_storage.cc (right):

http://codereview.chromium.org/342068/diff/1/71#newcode138
Line 138: ChromeThread::FILE, FROM_HERE, new BackupTask(writer_.path()));
On 2009/10/30 23:21:57, Evan Stade wrote:
> maybe in cases like this where you keep referencing ChromeThread::FILE you
could
> pull it out into a static const var? shrug

ChromeThread::FILE is just a a value from an enumeration..

http://codereview.chromium.org/342068/diff/1/15
File chrome/browser/browsing_data_remover.cc (left):

http://codereview.chromium.org/342068/diff/1/15#oldcode139
Line 139: UserMetrics::RecordAction(L"ClearBrowsingData_Cache", profile_);
On 2009/10/30 23:21:57, Evan Stade wrote:
> why are these two lines (138-139) deleted?

oops, that was a merge accident, fixed

http://codereview.chromium.org/342068/diff/1/15
File chrome/browser/browsing_data_remover.cc (right):

http://codereview.chromium.org/342068/diff/1/15#newcode136
Line 136: ChromeThread::IO, FROM_HERE,
On 2009/10/30 23:21:57, Evan Stade wrote:
> indentation

Done.

http://codereview.chromium.org/342068/diff/1/58
File chrome/browser/dom_ui/shown_sections_handler_unittest.cc (right):

http://codereview.chromium.org/342068/diff/1/58#newcode17
Line 17: PrefService pref((FilePath()));
On 2009/10/30 23:21:57, Evan Stade wrote:
> nit: too many ()

Visual Studio needs this for some strange reason.  I've seen others submit
similar changes to fix build breaks.  Darin and I couldn't figure out why this
is needed.

http://codereview.chromium.org/342068/diff/1/44
File chrome/browser/gtk/dialogs_gtk.cc (right):

http://codereview.chromium.org/342068/diff/1/44#newcode152
Line 152: DCHECK(!ChromeThread::CurrentlyOn(ChromeThread::UI));
On 2009/10/30 23:21:57, Evan Stade wrote:
> check should just be that we _are_ on the ui thread

I just kept the old behavior.  I believe it's doing this to succeed in
unit_tests.

http://codereview.chromium.org/342068/diff/1/74
File chrome/browser/memory_details.cc (right):

http://codereview.chromium.org/342068/diff/1/74#newcode39
Line 39: DCHECK(!ChromeThread::CurrentlyOn(ChromeThread::IO));
On 2009/10/30 23:21:57, Evan Stade wrote:
> can we just check that we are on the ui thread?

ditto

http://codereview.chromium.org/342068/diff/1/40
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/342068/diff/1/40#newcode122
Line 122: // that code be moved to chrome\browser instead of being in net, so
that it
On 2009/10/30 23:21:57, Evan Stade wrote:
> nit:/\\/\//

Done.

http://codereview.chromium.org/342068/diff/1/39
File chrome/browser/net/dns_master_unittest.cc (right):

http://codereview.chromium.org/342068/diff/1/39#newcode553
Line 553: 
On 2009/10/30 23:21:57, Evan Stade wrote:
> so many extra lines oO

Done.

http://codereview.chromium.org/342068/diff/1/56
File chrome/browser/renderer_host/buffered_resource_handler.h (right):

http://codereview.chromium.org/342068/diff/1/56#newcode68
Line 68: static void OnPluginsLoaded(BufferedResourceHandler* handler);
On 2009/10/30 23:21:57, Evan Stade wrote:
> A static method that takes an instance of the class?

This was done to ensure that the object could never be deleted on the file
thread (say if the task on the IO thread executed before the file thread's stack
unwound).  This is cleaned up in my soon-to-be-committed change:
http://codereview.chromium.org/338065

http://codereview.chromium.org/342068/diff/1/26
File chrome/browser/sync/sync_setup_wizard.cc (right):

http://codereview.chromium.org/342068/diff/1/26#newcode148
Line 148: sync_source->AddRef();
On 2009/10/30 23:21:57, Evan Stade wrote:
> is this because there's a floating ref or something?

we can't just delete it since RefCountedThredSafe's destructor checks to make
sure that the object was deleted through a call to Release.

http://codereview.chromium.org/342068/diff/1/85
File chrome/common/pref_service_unittest.cc (right):

http://codereview.chromium.org/342068/diff/1/85#newcode338
Line 338: PrefService prefs((FilePath()));
On 2009/10/30 23:21:57, Evan Stade wrote:
> extra parens here and below

ditto

[Evan Stade, 2009-10-31 00:05:18]

On Fri, Oct 30, 2009 at 4:47 PM,  <jam@chromium.org> wrote:
>
> http://codereview.chromium.org/342068/diff/1/13
> File chrome/browser/autocomplete/history_url_provider_unittest.cc
> (right):
>
> http://codereview.chromium.org/342068/diff/1/13#newcode99
> Line 99: HistoryURLProviderTest() : file_thread_(ChromeThread::FILE,
> &message_loop_) {}
> On 2009/10/30 23:21:57, Evan Stade wrote:
>>
>> nit: blank line after this
>
> Done.
>
> http://codereview.chromium.org/342068/diff/1/71
> File chrome/browser/bookmarks/bookmark_storage.cc (right):
>
> http://codereview.chromium.org/342068/diff/1/71#newcode138
> Line 138: ChromeThread::FILE, FROM_HERE, new
> BackupTask(writer_.path()));
> On 2009/10/30 23:21:57, Evan Stade wrote:
>>
>> maybe in cases like this where you keep referencing ChromeThread::FILE
>
> you could
>>
>> pull it out into a static const var? shrug
>
> ChromeThread::FILE is just a a value from an enumeration..
>
> http://codereview.chromium.org/342068/diff/1/15
> File chrome/browser/browsing_data_remover.cc (left):
>
> http://codereview.chromium.org/342068/diff/1/15#oldcode139
> Line 139: UserMetrics::RecordAction(L"ClearBrowsingData_Cache",
> profile_);
> On 2009/10/30 23:21:57, Evan Stade wrote:
>>
>> why are these two lines (138-139) deleted?
>
> oops, that was a merge accident, fixed
>
> http://codereview.chromium.org/342068/diff/1/15
> File chrome/browser/browsing_data_remover.cc (right):
>
> http://codereview.chromium.org/342068/diff/1/15#newcode136
> Line 136: ChromeThread::IO, FROM_HERE,
> On 2009/10/30 23:21:57, Evan Stade wrote:
>>
>> indentation
>
> Done.
>
> http://codereview.chromium.org/342068/diff/1/58
> File chrome/browser/dom_ui/shown_sections_handler_unittest.cc (right):
>
> http://codereview.chromium.org/342068/diff/1/58#newcode17
> Line 17: PrefService pref((FilePath()));
> On 2009/10/30 23:21:57, Evan Stade wrote:
>>
>> nit: too many ()
>
> Visual Studio needs this for some strange reason. =A0I've seen others
> submit similar changes to fix build breaks. =A0Darin and I couldn't figur=
e
> out why this is needed.
>
> http://codereview.chromium.org/342068/diff/1/44
> File chrome/browser/gtk/dialogs_gtk.cc (right):
>
> http://codereview.chromium.org/342068/diff/1/44#newcode152
> Line 152: DCHECK(!ChromeThread::CurrentlyOn(ChromeThread::UI));
> On 2009/10/30 23:21:57, Evan Stade wrote:
>>
>> check should just be that we _are_ on the ui thread
>
> I just kept the old behavior. =A0I believe it's doing this to succeed in
> unit_tests.
>
> http://codereview.chromium.org/342068/diff/1/74
> File chrome/browser/memory_details.cc (right):
>
> http://codereview.chromium.org/342068/diff/1/74#newcode39
> Line 39: DCHECK(!ChromeThread::CurrentlyOn(ChromeThread::IO));
> On 2009/10/30 23:21:57, Evan Stade wrote:
>>
>> can we just check that we are on the ui thread?
>
> ditto
>
> http://codereview.chromium.org/342068/diff/1/40
> File chrome/browser/net/chrome_url_request_context.cc (right):
>
> http://codereview.chromium.org/342068/diff/1/40#newcode122
> Line 122: // that code be moved to chrome\browser instead of being in
> net, so that it
> On 2009/10/30 23:21:57, Evan Stade wrote:
>>
>> nit:/\\/\//
>
> Done.
>
> http://codereview.chromium.org/342068/diff/1/39
> File chrome/browser/net/dns_master_unittest.cc (right):
>
> http://codereview.chromium.org/342068/diff/1/39#newcode553
> Line 553:
> On 2009/10/30 23:21:57, Evan Stade wrote:
>>
>> so many extra lines oO
>
> Done.
>
> http://codereview.chromium.org/342068/diff/1/56
> File chrome/browser/renderer_host/buffered_resource_handler.h (right):
>
> http://codereview.chromium.org/342068/diff/1/56#newcode68
> Line 68: static void OnPluginsLoaded(BufferedResourceHandler* handler);
> On 2009/10/30 23:21:57, Evan Stade wrote:
>>
>> A static method that takes an instance of the class?
>
> This was done to ensure that the object could never be deleted on the
> file thread (say if the task on the IO thread executed before the file
> thread's stack unwound). =A0This is cleaned up in my soon-to-be-committed
> change: http://codereview.chromium.org/338065

my concern was that this static function is semantically equivalent to a me=
thod

>
> http://codereview.chromium.org/342068/diff/1/26
> File chrome/browser/sync/sync_setup_wizard.cc (right):
>
> http://codereview.chromium.org/342068/diff/1/26#newcode148
> Line 148: sync_source->AddRef();
> On 2009/10/30 23:21:57, Evan Stade wrote:
>>
>> is this because there's a floating ref or something?
>
> we can't just delete it since RefCountedThredSafe's destructor checks to
> make sure that the object was deleted through a call to Release.

right, but why the AddRef()?

>
> http://codereview.chromium.org/342068/diff/1/85
> File chrome/common/pref_service_unittest.cc (right):
>
> http://codereview.chromium.org/342068/diff/1/85#newcode338
> Line 338: PrefService prefs((FilePath()));
> On 2009/10/30 23:21:57, Evan Stade wrote:
>>
>> extra parens here and below
>
> ditto
>
> http://codereview.chromium.org/342068
>

[Evan Stade, 2009-10-31 00:05:59]

http://codereview.chromium.org/342068/diff/1/44
File chrome/browser/gtk/dialogs_gtk.cc (right):

http://codereview.chromium.org/342068/diff/1/44#newcode152
Line 152: DCHECK(!ChromeThread::CurrentlyOn(ChromeThread::UI));
On 2009/10/30 23:47:00, John Abd-El-Malek wrote:
> On 2009/10/30 23:21:57, Evan Stade wrote:
> > check should just be that we _are_ on the ui thread
> 
> I just kept the old behavior.  I believe it's doing this to succeed in
> unit_tests.

a) aren't unit tests run on the ui thread?
b) you actually changed the check from not io and not file to not ui and not io

[jam, 2009-10-31 00:42:45]

On Fri, Oct 30, 2009 at 5:05 PM, Evan Stade <estade@chromium.org> wrote:

> On Fri, Oct 30, 2009 at 4:47 PM,  <jam@chromium.org> wrote:
> >
> > http://codereview.chromium.org/342068/diff/1/13
> > File chrome/browser/autocomplete/history_url_provider_unittest.cc
> > (right):
> >
> > http://codereview.chromium.org/342068/diff/1/13#newcode99
> > Line 99: HistoryURLProviderTest() : file_thread_(ChromeThread::FILE,
> > &message_loop_) {}
> > On 2009/10/30 23:21:57, Evan Stade wrote:
> >>
> >> nit: blank line after this
> >
> > Done.
> >
> > http://codereview.chromium.org/342068/diff/1/71
> > File chrome/browser/bookmarks/bookmark_storage.cc (right):
> >
> > http://codereview.chromium.org/342068/diff/1/71#newcode138
> > Line 138: ChromeThread::FILE, FROM_HERE, new
> > BackupTask(writer_.path()));
> > On 2009/10/30 23:21:57, Evan Stade wrote:
> >>
> >> maybe in cases like this where you keep referencing ChromeThread::FILE
> >
> > you could
> >>
> >> pull it out into a static const var? shrug
> >
> > ChromeThread::FILE is just a a value from an enumeration..
> >
> > http://codereview.chromium.org/342068/diff/1/15
> > File chrome/browser/browsing_data_remover.cc (left):
> >
> > http://codereview.chromium.org/342068/diff/1/15#oldcode139
> > Line 139: UserMetrics::RecordAction(L"ClearBrowsingData_Cache",
> > profile_);
> > On 2009/10/30 23:21:57, Evan Stade wrote:
> >>
> >> why are these two lines (138-139) deleted?
> >
> > oops, that was a merge accident, fixed
> >
> > http://codereview.chromium.org/342068/diff/1/15
> > File chrome/browser/browsing_data_remover.cc (right):
> >
> > http://codereview.chromium.org/342068/diff/1/15#newcode136
> > Line 136: ChromeThread::IO, FROM_HERE,
> > On 2009/10/30 23:21:57, Evan Stade wrote:
> >>
> >> indentation
> >
> > Done.
> >
> > http://codereview.chromium.org/342068/diff/1/58
> > File chrome/browser/dom_ui/shown_sections_handler_unittest.cc (right):
> >
> > http://codereview.chromium.org/342068/diff/1/58#newcode17
> > Line 17: PrefService pref((FilePath()));
> > On 2009/10/30 23:21:57, Evan Stade wrote:
> >>
> >> nit: too many ()
> >
> > Visual Studio needs this for some strange reason.  I've seen others
> > submit similar changes to fix build breaks.  Darin and I couldn't figure
> > out why this is needed.
> >
> > http://codereview.chromium.org/342068/diff/1/44
> > File chrome/browser/gtk/dialogs_gtk.cc (right):
> >
> > http://codereview.chromium.org/342068/diff/1/44#newcode152
> > Line 152: DCHECK(!ChromeThread::CurrentlyOn(ChromeThread::UI));
> > On 2009/10/30 23:21:57, Evan Stade wrote:
> >>
> >> check should just be that we _are_ on the ui thread
> >
> > I just kept the old behavior.  I believe it's doing this to succeed in
> > unit_tests.
> >
> > http://codereview.chromium.org/342068/diff/1/74
> > File chrome/browser/memory_details.cc (right):
> >
> > http://codereview.chromium.org/342068/diff/1/74#newcode39
> > Line 39: DCHECK(!ChromeThread::CurrentlyOn(ChromeThread::IO));
> > On 2009/10/30 23:21:57, Evan Stade wrote:
> >>
> >> can we just check that we are on the ui thread?
> >
> > ditto
> >
> > http://codereview.chromium.org/342068/diff/1/40
> > File chrome/browser/net/chrome_url_request_context.cc (right):
> >
> > http://codereview.chromium.org/342068/diff/1/40#newcode122
> > Line 122: // that code be moved to chrome\browser instead of being in
> > net, so that it
> > On 2009/10/30 23:21:57, Evan Stade wrote:
> >>
> >> nit:/\\/\//
> >
> > Done.
> >
> > http://codereview.chromium.org/342068/diff/1/39
> > File chrome/browser/net/dns_master_unittest.cc (right):
> >
> > http://codereview.chromium.org/342068/diff/1/39#newcode553
> > Line 553:
> > On 2009/10/30 23:21:57, Evan Stade wrote:
> >>
> >> so many extra lines oO
> >
> > Done.
> >
> > http://codereview.chromium.org/342068/diff/1/56
> > File chrome/browser/renderer_host/buffered_resource_handler.h (right):
> >
> > http://codereview.chromium.org/342068/diff/1/56#newcode68
> > Line 68: static void OnPluginsLoaded(BufferedResourceHandler* handler);
> > On 2009/10/30 23:21:57, Evan Stade wrote:
> >>
> >> A static method that takes an instance of the class?
> >
> > This was done to ensure that the object could never be deleted on the
> > file thread (say if the task on the IO thread executed before the file
> > thread's stack unwound).  This is cleaned up in my soon-to-be-committed
> > change: http://codereview.chromium.org/338065
>
> my concern was that this static function is semantically equivalent to a
> method
>

right.  this is not good, which is why i'm fixing it.  it has to be done
that way now (i.e. before my other change lands), since if a posttask was
done on a method, then we get into issues with the object being deleted on
the wrong thread.


> >
> > http://codereview.chromium.org/342068/diff/1/26
> > File chrome/browser/sync/sync_setup_wizard.cc (right):
> >
> > http://codereview.chromium.org/342068/diff/1/26#newcode148
> > Line 148: sync_source->AddRef();
> > On 2009/10/30 23:21:57, Evan Stade wrote:
> >>
> >> is this because there's a floating ref or something?
> >
> > we can't just delete it since RefCountedThredSafe's destructor checks to
> > make sure that the object was deleted through a call to Release.
>
> right, but why the AddRef()?
>

because it starts with a refcount of 0.  so if we release it without an
addref, there are dchecks that the refcount drops below 0.

>
> >
> > http://codereview.chromium.org/342068/diff/1/85
> > File chrome/common/pref_service_unittest.cc (right):
> >
> > http://codereview.chromium.org/342068/diff/1/85#newcode338
> > Line 338: PrefService prefs((FilePath()));
> > On 2009/10/30 23:21:57, Evan Stade wrote:
> >>
> >> extra parens here and below
> >
> > ditto
> >
> > http://codereview.chromium.org/342068
> >
>

[jam, 2009-10-31 00:43:27]

http://codereview.chromium.org/342068/diff/1/44
File chrome/browser/gtk/dialogs_gtk.cc (right):

http://codereview.chromium.org/342068/diff/1/44#newcode152
Line 152: DCHECK(!ChromeThread::CurrentlyOn(ChromeThread::UI));
On 2009/10/31 00:05:59, Evan Stade wrote:
> On 2009/10/30 23:47:00, John Abd-El-Malek wrote:
> > On 2009/10/30 23:21:57, Evan Stade wrote:
> > > check should just be that we _are_ on the ui thread
> > 
> > I just kept the old behavior.  I believe it's doing this to succeed in
> > unit_tests.
> 
> a) aren't unit tests run on the ui thread?

no, only if you explicitly create a MessageLoop of type UI.  normally there's no
MessageLoop::current() on unit tests.

> b) you actually changed the check from not io and not file to not ui and not
io

oops, I copied and pasted wrong.  fixed.

[Evan Stade, 2009-10-31 00:46:45]

On Fri, Oct 30, 2009 at 5:43 PM,  <jam@chromium.org> wrote:
>
> http://codereview.chromium.org/342068/diff/1/44
> File chrome/browser/gtk/dialogs_gtk.cc (right):
>
> http://codereview.chromium.org/342068/diff/1/44#newcode152
> Line 152: DCHECK(!ChromeThread::CurrentlyOn(ChromeThread::UI));
> On 2009/10/31 00:05:59, Evan Stade wrote:
>>
>> On 2009/10/30 23:47:00, John Abd-El-Malek wrote:
>> > On 2009/10/30 23:21:57, Evan Stade wrote:
>> > > check should just be that we _are_ on the ui thread
>> >
>> > I just kept the old behavior. =A0I believe it's doing this to succeed
>
> in
>>
>> > unit_tests.
>
>> a) aren't unit tests run on the ui thread?
>
> no, only if you explicitly create a MessageLoop of type UI. =A0normally
> there's no MessageLoop::current() on unit tests.
>
>> b) you actually changed the check from not io and not file to not ui
>
> and not io
>
> oops, I copied and pasted wrong. =A0fixed.
>
> http://codereview.chromium.org/342068
>

LGTM