Bug 108139 – Upstream iOS's AtomicString

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Benjamin Poulain]

Some more upstreaming.

[Benjamin Poulain]

Created attachment 185123 [details]
Patch

[Sam Weinig]

Comment on attachment 185123 [details]
Patch

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

> Source/WTF/wtf/text/AtomicString.cpp:34
> +#include <libkern/OSAtomic.h>

Can we use TCSpinLock instead?

> Source/WTF/wtf/text/AtomicString.cpp:44
> +#if PLATFORM(IOS)

I would like this to be a check for something more specific.  Maybe USE(WEB_THREAD)

> Source/WTF/wtf/text/AtomicString.cpp:121
> +#if PLATFORM(IOS)
> +    AtomicStringTableLocker locker;
> +#endif

Instead of having all these #ifdefs at the call site, could we just make those without a WebThread have an empty implementation of AtomicStringTableLocker?

[Benjamin Poulain]

Comment on attachment 185123 [details]
Patch

> > Source/WTF/wtf/text/AtomicString.cpp:44
> > +#if PLATFORM(IOS)
> 
> I would like this to be a check for something more specific.  Maybe USE(WEB_THREAD)

I like that idea.

> > Source/WTF/wtf/text/AtomicString.cpp:121
> > +#if PLATFORM(IOS)
> > +    AtomicStringTableLocker locker;
> > +#endif
> 
> Instead of having all these #ifdefs at the call site, could we just make those without a WebThread have an empty implementation of AtomicStringTableLocker?

I am afraid someone will misunderstand what it is and use AtomicStringTableLocker at a bad place. With the #ifdef, it is clear you should not touch that if not for iOS.

[Adam Barth]

Comment on attachment 185123 [details]
Patch

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

> Source/WTF/ChangeLog:9
> +        On iOS, WebCore can be executed from two different threads. To maintain consistency,
> +        a few changes had been made:

How many changes are required in WebCore to deal with this issue?

[Benjamin Poulain]

> > Source/WTF/ChangeLog:9
> > +        On iOS, WebCore can be executed from two different threads. To maintain consistency,
> > +        a few changes had been made:
> 
> How many changes are required in WebCore to deal with this issue?

Not much in common code. It requires quite a bit of code in the iOS platform code. The code does not become multithreaded in any way, the two threads are exclusive.

A lot of assertions (ASSERT(isMainThread())) need to be updated but that change is completely transparent if you don't have a WebThread.

[Benjamin Poulain]

Created attachment 185340 [details]
Patch

[David Kilzer (:ddkilzer)]

Comment on attachment 185340 [details]
Patch

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

> Source/WTF/wtf/Platform.h:620
> +#define WTF_USE_WEB_THREAD 1

What about USE(IOS_WEB_THREAD) instead?

[Benjamin Poulain]

> > Source/WTF/wtf/Platform.h:620
> > +#define WTF_USE_WEB_THREAD 1
> 
> What about USE(IOS_WEB_THREAD) instead?

I think having IOS in a USE() macro defeats the purpose a bit. But maybe Web_Thread is ambiguous?
Maybe?:
USE(WEBCORE_WITH_THREADS)
USE(THREADED_ACCESSS)
USE(MULTITHREAD_WEBKIT)

I don't mind in any way. I can update the macro before landing.

[David Kilzer (:ddkilzer)]

Comment on attachment 185340 [details]
Patch

r=me

[Benjamin Poulain]

Comment on attachment 185340 [details]
Patch

Clearing flags on attachment: 185340

Committed r141812: <http://trac.webkit.org/changeset/141812>

[Benjamin Poulain]

All reviewed patches have been landed.  Closing bug.