Bug 109459

Summary: [iOS] Upstream changes to Platform.h
Product: WebKit Reporter: David Kilzer (:ddkilzer) <ddkilzer>
Component: Web Template FrameworkAssignee: David Kilzer (:ddkilzer) <ddkilzer>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cmarcelo, joepeck, laszlo.gombos, ojan.autocc, sam, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch v1 benjamin: review+, benjamin: commit-queue-

Description David Kilzer (:ddkilzer) 2013-02-11 10:08:27 PST 
Upstream iOS changes to Platform.h.
Comment 1 David Kilzer (:ddkilzer) 2013-02-11 10:15:37 PST 
Created attachment 187610 [details]
Patch v1
Comment 2 Laszlo Gombos 2013-02-11 10:33:57 PST 
Comment on attachment 187610 [details]
Patch v1

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

> Source/WTF/wtf/Platform.h:-621
> -#if PLATFORM(IOS_SIMULATOR)
> -    #define ENABLE_JIT 0
> -    #define ENABLE_YARR_JIT 0

Is the intention to turn on JIT for the simulator as well ? If it is, the patch looks good to me.
Comment 3 David Kilzer (:ddkilzer) 2013-02-11 10:49:06 PST 
(In reply to comment #2)
> (From update of attachment 187610 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=187610&action=review
> 
> > Source/WTF/wtf/Platform.h:-621
> > -#if PLATFORM(IOS_SIMULATOR)
> > -    #define ENABLE_JIT 0
> > -    #define ENABLE_YARR_JIT 0
> 
> Is the intention to turn on JIT for the simulator as well ? If it is, the patch looks good to me.

Yes.
Comment 4 Benjamin Poulain 2013-02-11 13:19:15 PST 
Comment on attachment 187610 [details]
Patch v1

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

> Source/WTF/wtf/Platform.h:624
>  

Remove the blank line.

> Source/WTF/wtf/Platform.h:627
> +#define ENABLE_JIT 1
> +#define ENABLE_LLINT 1
> +#define ENABLE_YARR_JIT 1

Move those 3 ENABLE with the ones above?

> Source/WTF/wtf/Platform.h:1131
> +#if PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 60000 || PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 1080

I'd just add parenthesis here for readability.
Comment 5 David Kilzer (:ddkilzer) 2013-02-11 15:38:37 PST 
(In reply to comment #4)
> (From update of attachment 187610 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=187610&action=review
> 
> > Source/WTF/wtf/Platform.h:624
> >  
> 
> Remove the blank line.

Okay.

> > Source/WTF/wtf/Platform.h:627
> > +#define ENABLE_JIT 1
> > +#define ENABLE_LLINT 1
> > +#define ENABLE_YARR_JIT 1
> 
> Move those 3 ENABLE with the ones above?

Actually, all of these are defined below, so they should probably just be removed for "clarity".  (I don't find the definitions below very easy to follow, but better to match other platforms.)

> > Source/WTF/wtf/Platform.h:1131
> > +#if PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 60000 || PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 1080
> 
> I'd just add parenthesis here for readability.

Will do.

Thanks!
Comment 6 David Kilzer (:ddkilzer) 2013-02-11 15:56:46 PST 
Committed r142537: <http://trac.webkit.org/changeset/142537>