Bug 73937 – [chromium] Guard access to WebKitPlatformSupport::currentThread with a null check

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Klein]

[chromium] Guard access to WebKitPlatformSupport::currentThread with a null check

[Adam Klein]

Created attachment 118076 [details]
Patch

[Adam Barth]

Comment on attachment 118076 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=118076&action=review

> Source/WebKit/chromium/ChangeLog:10
> +        Though |currentThread| is never null in production code, it is null in
> +        unit tests (such as Chromium's unit_tests) that call WebKit::initialize()
> +        without first constructing a MessageLoop.

Can you add this comment to the code as well?  We don't want folks cargo-cult copying this null check.

[David Levin]

Why is it a bad thing to construct a MessageLoop in the test code?

[Adam Klein]

Comment on attachment 118076 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=118076&action=review

>> Source/WebKit/chromium/ChangeLog:10
>> +        without first constructing a MessageLoop.
> 
> Can you add this comment to the code as well?  We don't want folks cargo-cult copying this null check.

Done.

[Adam Klein]

Created attachment 118081 [details]
Patch for landing

[David Levin]

Comment on attachment 118081 [details]
Patch for landing

Just blocking for the moment since there is an unanswered question.

[Adam Klein]

(In reply to comment #3)
> Why is it a bad thing to construct a MessageLoop in the test code?

It's not so much that tests don't construct MessageLoops, but that each test case inside, e.g., unit_tests wants to have control over its own MessageLoop. For example, some tests need an IO loop, others might need a UI loop.  And only one can exist per thread. So at initialization time (presumably during test startup), there's no loop, even if each test case creates a loop later (in its GTest test fixture).

[David Levin]

Comment on attachment 118081 [details]
Patch for landing

Thanks! Sorry for the hold up.

[WebKit Review Bot]

Comment on attachment 118081 [details]
Patch for landing

Clearing flags on attachment: 118081

Committed r102171: <http://trac.webkit.org/changeset/102171>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.