Bug 95977 - webkit build for 64-bit Mac chromium
Summary: webkit build for 64-bit Mac chromium
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac (Intel) OS X 10.7
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-09-06 05:42 PDT by Catalin badea
Modified: 2012-09-10 11:48 PDT (History)
11 users (show)

See Also:

Attachments
Patch fix. (5.83 KB, patch)
2012-09-06 06:56 PDT, Catalin badea 		no flags Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Catalin badea 2012-09-06 05:42:50 PDT 
Webkit yields compilation errors when trying to build chromium for the x86_64 architecture (Mac OS X 10.7).
Comment 1 Catalin badea 2012-09-06 06:56:04 PDT 
Created attachment 162501 [details]
Patch fix.
Comment 2 WebKit Review Bot 2012-09-10 02:30:07 PDT 
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 3 Eric Seidel (no email) 2012-09-10 09:35:43 PDT 
Comment on attachment 162501 [details]
Patch fix.

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

> Source/WebKit/chromium/public/mac/WebSubstringUtil.h:34
> +#include "../platform/WebCommon.h"

This seems like an odd way to include a header?  Are you sure this is proper form?  Maybe <platform/WebCommon.h> ?

> Source/WebKit/chromium/src/mac/WebSubstringUtil.mm:35
> +#import <Cocoa/Cocoa.h>

Why the re-org?
Comment 4 Eric Seidel (no email) 2012-09-10 09:36:01 PDT 
Comment on attachment 162501 [details]
Patch fix.

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

> Source/WebCore/platform/graphics/IntRect.h:236
> +#if (PLATFORM(MAC) || (PLATFORM(CHROMIUM) && OS(DARWIN))) \
> +        && !defined(NSGEOMETRY_TYPES_SAME_AS_CGGEOMETRY_TYPES) \
> +        || (PLATFORM(QT) && USE(QTKIT))

Is Qt wrong here too?
Comment 5 Catalin badea 2012-09-10 10:07:47 PDT 
Thank you for the review.

(In reply to comment #3)
> (From update of attachment 162501 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=162501&action=review
> 
> > Source/WebKit/chromium/public/mac/WebSubstringUtil.h:34
> > +#include "../platform/WebCommon.h"
> 
> This seems like an odd way to include a header?  Are you sure this is proper form?  Maybe <platform/WebCommon.h> ?

I've followed the headers in WebKit/chromium/public/mac/ as examples.

> > Source/WebKit/chromium/src/mac/WebSubstringUtil.mm:35
> > +#import <Cocoa/Cocoa.h>
> 
> Why the re-org?

Cocoa/Cocoa.h needs to be included first in order for the SGEOMETRY_TYPES_SAME_AS_CGGEOMETRY_TYPES macro to be defined
before the contents of FloatSize.h & similar headers are being preprocessed ensuring that no extra constructors will be declared.
This also complies with header include ordering.
Comment 6 Eric Seidel (no email) 2012-09-10 11:22:17 PDT 
Comment on attachment 162501 [details]
Patch fix.

Seems reasonable.
Comment 7 WebKit Review Bot 2012-09-10 11:48:45 PDT 
Comment on attachment 162501 [details]
Patch fix.

Clearing flags on attachment: 162501

Committed r128085: <http://trac.webkit.org/changeset/128085>
Comment 8 WebKit Review Bot 2012-09-10 11:48:49 PDT 
All reviewed patches have been landed.  Closing bug.