Add a BrowserThread::DCheckCurrentlyOn function.

Add a DCHECK_CURRENTLY_ON(BrowserThread::ID) macro.

I've often lost time when a DCHECK(BrowserThread::CurrentlyOn(id)) fails,
because I needed to re-run the program in gdb to figure out which thread was
actually running the function. This new wrapper macro should make that clearer.

I've converted one file in service_worker to make sure everything works.
It changes:

[...:FATAL:service_worker_context_core.cc(75)] Check failed: BrowserThread::CurrentlyOn(BrowserThread::IO).

into

[...:FATAL:service_worker_context_core.cc(75)] Check failed: ::content::BrowserThread::CurrentlyOn(BrowserThread::IO). Must be called on Chrome_IOThread; actually called on Chrome_UIThread.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=256913

[Jeffrey Yasskin, 2014-03-08 01:42:55]

Also note that I do still get the usual stack trace including the function
containing the call:

[...:FATAL:browser_thread.h(281)] Check failed: CurrentlyOn(identifier). Must be
called on IO thread; actually called on UI thread.
 [0x7f46ef8f319e] base::debug::StackTrace::StackTrace()
 [0x7f46ef975f55] logging::LogMessage::~LogMessage()
 [0x7f46e67336b6] content::BrowserThread::DCheckCurrentlyOn()
 [0x7f46e6732250] content::ServiceWorkerContextCore::RegisterServiceWorker()
 [0x000000af929f] content::ServiceWorkerContextTest_Register_Test::TestBody()
 [0x000001175673] testing::internal::HandleSehExceptionsInMethodIfSupported<>()
 [0x00000116297e] testing::internal::HandleExceptionsInMethodIfSupported<>()
 [0x000001158fb5] testing::Test::Run()
...

this might not be present for every time the DCHECK is hit, but it should be
present for most developers, afaik.

[Jói, 2014-03-10 13:54:09]

I'd suggest a macro to preserve the functionality that logs show the right file
and line number.

I'd also suggest following up with a change where you try to replace as many as
possible of the existing invocations of DCHECK(BrowserThread::CurrentlyOn(...))
with this.

Other than that, this looks like a good idea.

[jam, 2014-03-10 16:39:19]

On 2014/03/10 13:54:09, Jói wrote:
> I'd suggest a macro to preserve the functionality that logs show the right
file
> and line number.
> 
> I'd also suggest following up with a change where you try to replace as many
as
> possible of the existing invocations of
DCHECK(BrowserThread::CurrentlyOn(...))
> with this.

+1, if we add this, please convert all the existing instances so that there are
no other examples to use. of course, over many changes to make it easy for you
(also you can tbr all of them)

> 
> Other than that, this looks like a good idea.

[Jeffrey Yasskin, 2014-03-10 17:15:37]

'k. Would DCHECK_CURRENTLY_ON(BrowserThread::IO) be a good spelling?

On Mon, Mar 10, 2014 at 6:54 AM,  <joi@chromium.org> wrote:
> I'd suggest a macro to preserve the functionality that logs show the right
> file
> and line number.
>
> I'd also suggest following up with a change where you try to replace as many
> as
> possible of the existing invocations of
> DCHECK(BrowserThread::CurrentlyOn(...))
> with this.
>
> Other than that, this looks like a good idea.
>
> https://codereview.chromium.org/191763002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Jói, 2014-03-10 17:17:05]

Since it's a macro, and this will only ever deal with BrowserThread
IDs, I would suggest just DCHECK_CURRENTLY_ON(IO).

On Mon, Mar 10, 2014 at 5:15 PM, Jeffrey Yasskin <jyasskin@chromium.org> wrote:
> 'k. Would DCHECK_CURRENTLY_ON(BrowserThread::IO) be a good spelling?
>
> On Mon, Mar 10, 2014 at 6:54 AM,  <joi@chromium.org> wrote:
>> I'd suggest a macro to preserve the functionality that logs show the right
>> file
>> and line number.
>>
>> I'd also suggest following up with a change where you try to replace as many
>> as
>> possible of the existing invocations of
>> DCHECK(BrowserThread::CurrentlyOn(...))
>> with this.
>>
>> Other than that, this looks like a good idea.
>>
>> https://codereview.chromium.org/191763002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Jeffrey Yasskin, 2014-03-10 17:21:37]

That's certainly implementable, but I'm worried that it'll be harder
to read because it doesn't mention threads at all. I was actually
wondering if I should use DCHECK_CURRENTLY_ON_BROWSER_THREAD(IO) to
avoid colliding with any macros in codebases using content/. But now
that I've mentioned those concerns, I'm happy to update the CL to
whichever spelling you prefer.

On Mon, Mar 10, 2014 at 10:16 AM, Jói Sigurðsson <joi@chromium.org> wrote:
> Since it's a macro, and this will only ever deal with BrowserThread
> IDs, I would suggest just DCHECK_CURRENTLY_ON(IO).
>
> On Mon, Mar 10, 2014 at 5:15 PM, Jeffrey Yasskin <jyasskin@chromium.org>
wrote:
>> 'k. Would DCHECK_CURRENTLY_ON(BrowserThread::IO) be a good spelling?
>>
>> On Mon, Mar 10, 2014 at 6:54 AM,  <joi@chromium.org> wrote:
>>> I'd suggest a macro to preserve the functionality that logs show the right
>>> file
>>> and line number.
>>>
>>> I'd also suggest following up with a change where you try to replace as
many
>>> as
>>> possible of the existing invocations of
>>> DCHECK(BrowserThread::CurrentlyOn(...))
>>> with this.
>>>
>>> Other than that, this looks like a good idea.
>>>
>>> https://codereview.chromium.org/191763002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Jói, 2014-03-10 17:30:09]

I'm fine with your original proposal DCHECK_CURRENTLY_ON(BrowserThread::IO)

On Mon, Mar 10, 2014 at 5:21 PM, Jeffrey Yasskin <jyasskin@chromium.org> wrote:
> That's certainly implementable, but I'm worried that it'll be harder
> to read because it doesn't mention threads at all. I was actually
> wondering if I should use DCHECK_CURRENTLY_ON_BROWSER_THREAD(IO) to
> avoid colliding with any macros in codebases using content/. But now
> that I've mentioned those concerns, I'm happy to update the CL to
> whichever spelling you prefer.
>
> On Mon, Mar 10, 2014 at 10:16 AM, Jói Sigurðsson <joi@chromium.org> wrote:
>> Since it's a macro, and this will only ever deal with BrowserThread
>> IDs, I would suggest just DCHECK_CURRENTLY_ON(IO).
>>
>> On Mon, Mar 10, 2014 at 5:15 PM, Jeffrey Yasskin <jyasskin@chromium.org>
wrote:
>>> 'k. Would DCHECK_CURRENTLY_ON(BrowserThread::IO) be a good spelling?
>>>
>>> On Mon, Mar 10, 2014 at 6:54 AM,  <joi@chromium.org> wrote:
>>>> I'd suggest a macro to preserve the functionality that logs show the right
>>>> file
>>>> and line number.
>>>>
>>>> I'd also suggest following up with a change where you try to replace as
many
>>>> as
>>>> possible of the existing invocations of
>>>> DCHECK(BrowserThread::CurrentlyOn(...))
>>>> with this.
>>>>
>>>> Other than that, this looks like a good idea.
>>>>
>>>> https://codereview.chromium.org/191763002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Jeffrey Yasskin, 2014-03-10 18:03:03]

Ok, switched to a macro. PTAL. I'll send a bunch of update CLs once this looks
stable, as requested.

[Jói, 2014-03-10 21:43:10]

LGTM

[Jeffrey Yasskin, 2014-03-10 22:07:20]

Sorry for the extra round-trip: Vince noticed that I could use
base::MessageLoop::current()->thread_name() to get better messages for some
non-browser threads. That doesn't work in the test I tried where the thread is
named "", but it also led me to notice g_browser_thread_names which has names
for the non-UI threads, which let me shorten GetThreadName considerably.

[Jói, 2014-03-11 08:41:24]

LGTM w/a nit.

https://codereview.chromium.org/191763002/diff/40001/content/public/browser/b...
File content/public/browser/browser_thread.h (right):

https://codereview.chromium.org/191763002/diff/40001/content/public/browser/b...
content/public/browser/browser_thread.h:276: #define
DCHECK_CURRENTLY_ON(thread_identifier)                      \
nit: This would be more discoverable if placed at the top of this header. A
sentence of documentation would also help.

[Jeffrey Yasskin, 2014-03-11 19:11:22]

https://codereview.chromium.org/191763002/diff/40001/content/public/browser/b...
File content/public/browser/browser_thread.h (right):

https://codereview.chromium.org/191763002/diff/40001/content/public/browser/b...
content/public/browser/browser_thread.h:276: #define
DCHECK_CURRENTLY_ON(thread_identifier)                      \
On 2014/03/11 08:41:25, Jói wrote:
> nit: This would be more discoverable if placed at the top of this header. A
> sentence of documentation would also help.

SG. Done, thanks.

[alecflett, 2014-03-11 19:26:54]

lgtm for service_worker

[Jeffrey Yasskin, 2014-03-11 19:33:57]

The CQ bit was checked by jyasskin@chromium.org

[commit-bot: I haz the power, 2014-03-11 19:53:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jyasskin@chromium.org/191763002/50001

[commit-bot: I haz the power, 2014-03-11 22:30:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-11 22:30:44]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Jeffrey Yasskin, 2014-03-12 00:36:02]

+sky for content/browser: sorry for leaving you off before

[sky, 2014-03-12 02:32:12]

You have John on this review, and he knows the code better than I, so I'm
removing myself.

[Jeffrey Yasskin, 2014-03-12 15:48:11]

OK, then John, can I get an lgtm from you?
On Mar 11, 2014 7:32 PM, <sky@chromium.org> wrote:

> You have John on this review, and he knows the code better than I, so I'm
> removing myself.
>
> https://codereview.chromium.org/191763002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jam, 2014-03-13 18:15:57]

lgtm

[Jeffrey Yasskin, 2014-03-13 18:18:38]

The CQ bit was checked by jyasskin@chromium.org

[commit-bot: I haz the power, 2014-03-13 18:18:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jyasskin@chromium.org/191763002/50001

[commit-bot: I haz the power, 2014-03-13 20:24:55]

Change committed as 256913