Bug 119905

Summary: [iOS] Upstream Source/WTF
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: Web Template FrameworkAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: aestes, benjamin, cmarcelo, commit-queue, ddkilzer, joepeck
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
[PATCH] Proposed Fix benjamin: review+

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>.