Bug 119905 - [iOS] Upstream Source/WTF
Summary: [iOS] Upstream Source/WTF
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-08-16 12:27 PDT by Joseph Pecoraro
Modified: 2013-08-19 13:37 PDT (History)
6 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (15.36 KB, patch)
2013-08-16 12:30 PDT, Joseph Pecoraro
benjamin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2013-08-16 12:27:20 PDT
Upstream the remaining iOS WebKit diffs in Source/WTF.
Comment 1 Joseph Pecoraro 2013-08-16 12:30:50 PDT
Created attachment 208944 [details]
[PATCH] Proposed Fix
Comment 2 Joseph Pecoraro 2013-08-16 12:47:01 PDT
<rdar://problem/14750668>
Comment 3 Benjamin Poulain 2013-08-16 14:45:12 PDT
Comment on attachment 208944 [details]
[PATCH] Proposed Fix

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

> Source/WTF/wtf/Assertions.h:98
> +/* For project uses WTF but has no config.h, we need to explicitly set the export defines here. */

Wrong comment style.

> Source/WTF/wtf/Platform.h:1009
> +#if (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 70000) || ((PLATFORM(MAC) || (OS(WINDOWS) && USE(CG))) && !PLATFORM(IOS) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 1090)

Do we really need && __IPHONE_OS_VERSION_MIN_REQUIRED >= 70000?
It's not like we support anything before that.

You should get rid of the !PLATFORM(IOS) later in the list, it makes the #ifdef hard to follow.

> Source/WTF/wtf/text/StringStatics.cpp:86
> +        // On iOS WebKit, isMainThread() tests for the Web Thread, so use pthread_main_np() to ensure this is the real main thread.
> +        ASSERT(pthread_main_np());

Isn't isUIThread() solving this problem?
You could just replace ASSERT(isMainThread()); by ASSERT(isUIThread());
Comment 4 Joseph Pecoraro 2013-08-19 12:21:32 PDT
(In reply to comment #3)
> (From update of attachment 208944 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=208944&action=review
> 
> > Source/WTF/wtf/Assertions.h:98
> > +/* For project uses WTF but has no config.h, we need to explicitly set the export defines here. */
> 
> Wrong comment style.

All comments in Assertions.h use this style. Though I will update the grammar as estes had mentioned to me.


> > Source/WTF/wtf/Platform.h:1009
> > +#if (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 70000) || ((PLATFORM(MAC) || (OS(WINDOWS) && USE(CG))) && !PLATFORM(IOS) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 1090)
> 
> Do we really need && __IPHONE_OS_VERSION_MIN_REQUIRED >= 70000?
> It's not like we support anything before that.
> 
> You should get rid of the !PLATFORM(IOS) later in the list, it makes the #ifdef hard to follow.

I will remove this. I don't think anyone is compiling older iOS WebKits.

That said, the PLATFORM(IOS) later in the statement should only have been needed for iOS versions older then 70000, so I can now remove that.


> > Source/WTF/wtf/text/StringStatics.cpp:86
> > +        // On iOS WebKit, isMainThread() tests for the Web Thread, so use pthread_main_np() to ensure this is the real main thread.
> > +        ASSERT(pthread_main_np());
> 
> Isn't isUIThread() solving this problem?
> You could just replace ASSERT(isMainThread()); by ASSERT(isUIThread());

Sure, I will make this change.
Comment 5 Joseph Pecoraro 2013-08-19 12:25:31 PDT
> > > Source/WTF/wtf/Platform.h:1009
> > > +#if (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 70000) || ((PLATFORM(MAC) || (OS(WINDOWS) && USE(CG))) && !PLATFORM(IOS) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 1090)
> > 
> > Do we really need && __IPHONE_OS_VERSION_MIN_REQUIRED >= 70000?
> > It's not like we support anything before that.
> > 
> > You should get rid of the !PLATFORM(IOS) later in the list, it makes the #ifdef hard to follow.
> 
> I will remove this. I don't think anyone is compiling older iOS WebKits.
> 
> That said, the PLATFORM(IOS) later in the statement should only have been needed for iOS versions older then 70000, so I can now remove that.

On second thought, I want to keep them in. It is easy to remove them later if we wish. Mac does this with their features and it seems useful for documentation. For example this one as well in Platform.h:

   #if (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 60000) || (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 1080)
  #define WTF_USE_COREMEDIA 1
  #endif
Comment 6 Joseph Pecoraro 2013-08-19 13:37:48 PDT
Committed <http://trac.webkit.org/changeset/154294>.