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: Macintosh 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

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.