Bug 180308 – WTF shouldn't have both Thread and ThreadIdentifier

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Filip Pizlo]

I find it strange that we often compare threads by doing:

Thread& a = ...
Thread& b = ...
a==b // loads id() from a, loads id() from b, compares id's

In reality, == comparisons for Thread* reveal thread equality.

I think we should get rid of ThreadIdentifier.  Moreover, to encourage == comparison of Thread, we should encourage using Thread by pointer rather than by reference.

[Yusuke Suzuki]

I have several thoughts about ThreadIdentifier.
But I *like* this change: we should use Thread instead of ThreadIdentifier. This is apparently nice.

I remember that some places use RefPtr<Thread> explicitly since it may be nullptr.
Personally I think Ref<Thread> is good to describe Thread is not nullptr.

And I believe Thread::create should return Ref<Thread> instead of RefPtr<>.
When we failed to spawn a thread, we would not have a good way to keep WebKit working.
It would be good to add Thread::tryCreate and use it for some specific places.

1. Some WebCore code just holds ThreadIdentifier.
Such program assumes that ThreadIdentifier is monotonocally increasing...
So when refactoring, we need to keep this thing in our mind.

Note that the above assumption is already broken in Windows. And replacing it with Ref<Thread> would work. And this use is mainly used for debugging.

2. In Windows, some internal blobs (That’s used for AppleWin and WinCairo) uses WTF’s old threading.
I’m not sure about it much. But when removing createThread API, it causes build error. So I kept old createThread API over Thread::create.

I think it returns ThreadIdentifier. So at least in Windows, we need to keep it even if it is not used for the other WebKit code.

But I think it’s not a problem. Since Windows’ ThreadIdentifier is system provided TID, we can just use it.

[Yusuke Suzuki]

Created attachment 328290 [details]
Patch

[EWS Watchlist]

Attachment 328290 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:194:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:194:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:195:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:195:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:279:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:298:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/Supplementable.h:120:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 7 in 66 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 328291 [details]
Patch

[EWS Watchlist]

Attachment 328291 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:194:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:194:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:195:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:195:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:279:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:298:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/Supplementable.h:120:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 7 in 67 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 328292 [details]
Patch

[EWS Watchlist]

Attachment 328292 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:194:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:194:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:195:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:195:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:279:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:298:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/Supplementable.h:120:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 7 in 67 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 328292 [details]
Patch

Attachment 328292 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/5476426

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 328293 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Yusuke Suzuki]

Created attachment 328295 [details]
Patch

[EWS Watchlist]

Attachment 328295 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:194:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:194:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:195:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:195:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:279:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:298:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/Supplementable.h:120:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 7 in 67 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 328295 [details]
Patch

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

Good idea to clean this up; messy.

> Source/WTF/wtf/RecursiveLockAdapter.h:86
> +    Thread* m_owner { nullptr }; // Use Thread* instead of RefPtr<Thread> since owner thread is always alive while m_onwer is et.

Typo: m_onwer

> Source/WTF/wtf/Threading.h:146
>      bool operator==(const Thread& thread)

Keeping these operator== is a little bit peculiar. It means that we can compare a thread with another without using .get(), .ptr() or &, which I suppose is handy, but we could have done that for almost any other object that represents something unique. Like even, say, DOM::Document.

It was certainly strange before that we guaranteed uniqueness of both the ID an of the reference counted thread object.

I would be tempted to remove these entirely; we would need to change more code then, but I think it would be logical to compare RefPtr<Thread> or Thread* rather than doing == on the Thread objects themselves since they are all unique and no two are equal unless they are the same one. Or if we are keeping these operators, I would be tempted to add even more overloads so we can compare a Thread* with a Thread& without doing anything.

> Source/WTF/wtf/ThreadingPthreads.cpp:303
> +        LOG_ERROR("Thread %p was unable to be detached\n", this);

Also need to remove the reference to ThreadIdentifier in the comment in Thread::initializeCurrentTLS.

> Source/WebCore/Modules/webaudio/AudioContext.cpp:136
> -    , m_graphOwnerThread(UndefinedThreadIdentifier)
> +    , m_graphOwnerThread(nullptr)

Initialize in class definition instead?

> Source/WebCore/Modules/webaudio/AudioContext.cpp:152
> -    , m_graphOwnerThread(UndefinedThreadIdentifier)
> +    , m_graphOwnerThread(nullptr)

Ditto.

> Source/WebCore/Modules/webaudio/AudioContext.h:387
> -    volatile ThreadIdentifier m_audioThread { 0 };
> -    volatile ThreadIdentifier m_graphOwnerThread; // if the lock is held then this is the thread which owns it, otherwise == UndefinedThreadIdentifier
> +    // FIXME: Using volatile seems incorrect.
> +    // https://bugs.webkit.org/show_bug.cgi?id=180332
> +    volatile Thread* m_audioThread { nullptr };
> +    volatile Thread* m_graphOwnerThread; // if the lock is held then this is the thread which owns it, otherwise == nullptr.

If volatile needs to be kept at all, then it needs to be after the "*":

    Thread* volatile m_audioThread { nullptr };
    Thread* volatile m_graphOwnerThread { nullptr }; // Thread which owns lock currently held.

It’s the storage of the data member that was marked volatile before, previously an integer, now a pointer, not the storage of the contents of a Thread object, which is what volatile Thread* means. I am not sure the volatile is needed, but the volatile before the * is definitely not helpful.

> Source/WebCore/Modules/webdatabase/DatabaseDetails.h:41
>          , m_creationTime(0)

This seems way more like a struct than a class, not to be changed in this patch, I guess.

> Source/WebCore/Modules/webdatabase/DatabaseDetails.h:80
> +    RefPtr<Thread> m_thread;

Why not Ref<Thread> m_thread { Thread::current() } as we have been doing other places?

> Source/WebCore/platform/Supplementable.h:127
> +    Ref<Thread> m_thread;

Why not initialize here as you have in other places?

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxy.h:94
> +    RefPtr<Thread> m_compositorThread { nullptr };

No need for { nullptr } for a RefPtr. They all do that automatically.

> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:67
> +    ASSERT(m_creationThread.get() == Thread::current());

I am really surprised that these .get() are needed. What happens if we don’t do get()? I think maybe the reason they are needed is that we overload the operator== as a class member of the Thread class instead of writing it as a free function that takes two Thread& or maybe const Thread&.

If we are going so far as to actually keep the overload of ==, then I think we should try to do it in a way where we don’t need to do all this get().

Without the overload of ==, I think it would be:

    ASSERT(m_creationThread.ptr() == &Thread::current());

> Source/WebCore/workers/service/ServiceWorkerJob.cpp:50
> +    ASSERT(Thread::current() == m_creationThread.get());

Annoying to have the same idiom here as ServicesWorkerContainer, but with all the assertions in the opposite order! We should fix them to match.

> Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:500
> +    static std::once_flag flag;

Why move from a simple boolean to a std::call_once? I don’t think this function needs to be thread-safe; but maybe I am mistaken.

> Tools/DumpRenderTree/mac/DumpRenderTree.mm:1069
> +static Thread* Thread1 { nullptr };
> +static Thread* Thread2 { nullptr };

There is no reason these need to be outside the function. They can be static, but inside the function. Also no need for the { nullptr } since all globals get initialized.

> Tools/DumpRenderTree/mac/DumpRenderTree.mm:1071
>  static void testThreadIdentifierMap()

The name of this function no longer makes sense. Could be ThreadIdentityMap or ThreadObjectMap or just ThreadMap.

> Tools/DumpRenderTree/mac/DumpRenderTree.mm:1089
>      // Now create another thread using WTF. On OSX, it will have the same pthread handle

The comment here confidently states that on macOS this will end up getting the same pthread handle. But I am not sure that will always happen, the code doesn’t assert it, and if it doesn’t happen then the test isn’t really testing what it claims to test.

> Tools/DumpRenderTree/mac/DumpRenderTree.mm:1090
> +    // but should get a different RefPtr<Thread> if the previous RefPtr<thread> is held.

Type here, "thread" instead of "Thread".

But also, this old test is in the wrong place. If we were adding it today it would be in TestWebKitAPI, not inside DumpRenderTree. We should consider moving it there.

[Yusuke Suzuki]

Comment on attachment 328295 [details]
Patch

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

Thank you

>> Source/WTF/wtf/RecursiveLockAdapter.h:86
>> +    Thread* m_owner { nullptr }; // Use Thread* instead of RefPtr<Thread> since owner thread is always alive while m_onwer is et.
> 
> Typo: m_onwer

Fixed.

>> Source/WTF/wtf/Threading.h:146
>>      bool operator==(const Thread& thread)
> 
> Keeping these operator== is a little bit peculiar. It means that we can compare a thread with another without using .get(), .ptr() or &, which I suppose is handy, but we could have done that for almost any other object that represents something unique. Like even, say, DOM::Document.
> 
> It was certainly strange before that we guaranteed uniqueness of both the ID an of the reference counted thread object.
> 
> I would be tempted to remove these entirely; we would need to change more code then, but I think it would be logical to compare RefPtr<Thread> or Thread* rather than doing == on the Thread objects themselves since they are all unique and no two are equal unless they are the same one. Or if we are keeping these operators, I would be tempted to add even more overloads so we can compare a Thread* with a Thread& without doing anything.

OK, I removed this from Thread. And use pointer comparison in all the places.

>> Source/WTF/wtf/ThreadingPthreads.cpp:303
>> +        LOG_ERROR("Thread %p was unable to be detached\n", this);
> 
> Also need to remove the reference to ThreadIdentifier in the comment in Thread::initializeCurrentTLS.

Dropped.

>> Source/WebCore/Modules/webaudio/AudioContext.cpp:136
>> +    , m_graphOwnerThread(nullptr)
> 
> Initialize in class definition instead?

Fixed.

>> Source/WebCore/Modules/webaudio/AudioContext.cpp:152
>> +    , m_graphOwnerThread(nullptr)
> 
> Ditto.

Fixed.

>> Source/WebCore/Modules/webaudio/AudioContext.h:387
>> +    volatile Thread* m_graphOwnerThread; // if the lock is held then this is the thread which owns it, otherwise == nullptr.
> 
> If volatile needs to be kept at all, then it needs to be after the "*":
> 
>     Thread* volatile m_audioThread { nullptr };
>     Thread* volatile m_graphOwnerThread { nullptr }; // Thread which owns lock currently held.
> 
> It’s the storage of the data member that was marked volatile before, previously an integer, now a pointer, not the storage of the contents of a Thread object, which is what volatile Thread* means. I am not sure the volatile is needed, but the volatile before the * is definitely not helpful.

Oops, nice catch. Fixed.

>> Source/WebCore/Modules/webdatabase/DatabaseDetails.h:80
>> +    RefPtr<Thread> m_thread;
> 
> Why not Ref<Thread> m_thread { Thread::current() } as we have been doing other places?

This is because Ref<Thread> is not copyable. We need to add bunch of duplicate code for copy constructor and assignment operator.
In the meantime, I added them. But I think we should have some helper class to allow copying for Ref. Like, CopyableRef<>, which inherits Ref<> but copyable.

>> Source/WebCore/platform/Supplementable.h:127
>> +    Ref<Thread> m_thread;
> 
> Why not initialize here as you have in other places?

Fixed. And change the constructor to `Supplementable() = default`.

>> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxy.h:94
>> +    RefPtr<Thread> m_compositorThread { nullptr };
> 
> No need for { nullptr } for a RefPtr. They all do that automatically.

Dropped nullptr.

>> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:67
>> +    ASSERT(m_creationThread.get() == Thread::current());
> 
> I am really surprised that these .get() are needed. What happens if we don’t do get()? I think maybe the reason they are needed is that we overload the operator== as a class member of the Thread class instead of writing it as a free function that takes two Thread& or maybe const Thread&.
> 
> If we are going so far as to actually keep the overload of ==, then I think we should try to do it in a way where we don’t need to do all this get().
> 
> Without the overload of ==, I think it would be:
> 
>     ASSERT(m_creationThread.ptr() == &Thread::current());

OK, changed to use pointer comparisons.

>> Source/WebCore/workers/service/ServiceWorkerJob.cpp:50
>> +    ASSERT(Thread::current() == m_creationThread.get());
> 
> Annoying to have the same idiom here as ServicesWorkerContainer, but with all the assertions in the opposite order! We should fix them to match.

Aligned to the same style, xxx == &Thread::current().

>> Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:500
>> +    static std::once_flag flag;
> 
> Why move from a simple boolean to a std::call_once? I don’t think this function needs to be thread-safe; but maybe I am mistaken.

Yes, this function does not need to be thread-safe. But I think std::call_once is more readable if it is only called once.

>> Tools/DumpRenderTree/mac/DumpRenderTree.mm:1069
>> +static Thread* Thread2 { nullptr };
> 
> There is no reason these need to be outside the function. They can be static, but inside the function. Also no need for the { nullptr } since all globals get initialized.

OK, placed them in function.

>> Tools/DumpRenderTree/mac/DumpRenderTree.mm:1071
>>  static void testThreadIdentifierMap()
> 
> The name of this function no longer makes sense. Could be ThreadIdentityMap or ThreadObjectMap or just ThreadMap.

Moved it to TestWebKitAPI.

>> Tools/DumpRenderTree/mac/DumpRenderTree.mm:1089
>>      // Now create another thread using WTF. On OSX, it will have the same pthread handle
> 
> The comment here confidently states that on macOS this will end up getting the same pthread handle. But I am not sure that will always happen, the code doesn’t assert it, and if it doesn’t happen then the test isn’t really testing what it claims to test.

Yeah, it sounds that this is really implementation dependent of the system library.
I changed this to `may`.

>> Tools/DumpRenderTree/mac/DumpRenderTree.mm:1090
>> +    // but should get a different RefPtr<Thread> if the previous RefPtr<thread> is held.
> 
> Type here, "thread" instead of "Thread".
> 
> But also, this old test is in the wrong place. If we were adding it today it would be in TestWebKitAPI, not inside DumpRenderTree. We should consider moving it there.

Moved this to TestWebKitAPI.

[Yusuke Suzuki]

Created attachment 328327 [details]
Patch for landing

Patch for landing

[EWS Watchlist]

Attachment 328327 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:194:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:194:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:195:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:195:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:279:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:298:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 6 in 69 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 328328 [details]
Patch for landing

Patch for landing

[EWS Watchlist]

Attachment 328328 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:194:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:194:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:195:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:195:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:279:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:298:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 6 in 69 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 328329 [details]
Patch for landing

Patch for landing

[EWS Watchlist]

Attachment 328329 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:194:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:194:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:195:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:195:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:279:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:298:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 6 in 69 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Committed r225470: <https://trac.webkit.org/changeset/225470>

[Radar WebKit Bug Importer]

<rdar://problem/35823384>

[Yusuke Suzuki]

Comment on attachment 328329 [details]
Patch for landing

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

> Source/WebCore/Modules/webdatabase/SQLTransaction.cpp:187
> +    if (&Thread::current() != m_database->databaseThread().getThread())

I'll exchange operands in a follow-up patch.

> Source/WebCore/bindings/js/JSCallbackData.h:55
> -        , m_thread(currentThread())
> +        , m_thread(Thread::current())

I'll move to the class field in a follow-up patch.

> Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:435
> +    ASSERT(&Thread::current() == libxmlLoaderThread);

I'll fix this in a follow-up patch.

[Yusuke Suzuki]

Committed r225731: <https://trac.webkit.org/changeset/225731>