Bug 20890 – Need to decouple building for the Mac from using NSURL/CFURL

• Component: Platform
• Status: RESOLVED WONTFIX

[Avi Drissman]

Throughout the code (especially in resource loading) it is assumed that building on the Mac means we're using the NSURL machinery. We're not going that road in Chromium, and we need to decouple those assumptions.

Attached is a patch. It's just a proposal, and I suspect that introducing a new #define for NSURL networking might be a better plan.

[Avi Drissman]

Created attachment 23482 [details]
Proposed patch

[Mark Rowe (bdash)]

I think a separate #define would be a lot nicer than chaining #if's like that.

[Eric Seidel (no email)]

Comment on attachment 23482 [details]
Proposed patch

This will need a ChangeLog, and to be marked for review to be a real patch considered for landing.  See:
http://webkit.org/coding/contributing.html

Mark suggests that we add a define USE(NSURLRESPONSE) to wrap these, and make the MAC build define WTF_USE_NSURLRESPONSE and the CHROMIUM build make sure that define is off.

I'm not even sure why these methods exist, since somehow the non-mac builds don't need these on the clients.

The file you would need to edit to add such a USE would be:
http://trac.webkit.org/browser/trunk/WebCore/config.h

[Mark Rowe (bdash)]

The methods exist to support API on Mac.  The non-Mac ports don't expose an equivalent API, so this code is currently #if'd for Mac only.  

[Avi Drissman]

Created attachment 23502 [details]
Revised patch

[Eric Seidel (no email)]

Comment on attachment 23502 [details]
Revised patch

Looks fine.  Eventually we'll have to make additional edits to config.h to match our config.h.in.  Long term all of config.h.in should be in WebKit's repository, not chromium's.

[Mark Rowe (bdash)]

Given the purpose of the method, I think we could do better for a macro name than USE(NSURLRESPONSE) for this.  I also think that refactoring this method to be cross-platform would be preferable, though this code was obviously already #if'd for Mac only.

[Alexey Proskuryakov]

I once again question the idea of Chromium having PLATFORM(MAC) defined - the idea of MAC has always been "Apple's Mac port", and it shouldn't be defined when building Chromium, or Qt/Gtk ports on Mac OS X (the latter ports do adhere to this principle).

This patch is of course formally correct, but all it does is making it possible for non-Apple ports to use NSURL, which no port has been interested in yet. More granularity seems pointless if no one is going to use it.

[Alexey Proskuryakov]

If the idea is to redefine MAC to an OS-level macro similar to WIN_OS, then it should probably be discussed on webkit-dev, as this affects other ports.

[Mark Rowe (bdash)]

(In reply to comment #9)
> If the idea is to redefine MAC to an OS-level macro similar to WIN_OS, then it
> should probably be discussed on webkit-dev, as this affects other ports.

PLATFORM(MAC) is already a platform-level macro.  It indicates that we are building against the Mac-level (CoreFoundation and above) frameworks on Mac OS X.  PLATFORM(DARWIN) indicates that we're building against the BSD-level and below APIs of Mac OS X.  

(In reply to comment #8)
> I once again question the idea of Chromium having PLATFORM(MAC) defined - the
> idea of MAC has always been "Apple's Mac port", and it shouldn't be defined
> when building Chromium, or Qt/Gtk ports on Mac OS X (the latter ports do adhere
> to this principle).

PLATFORM(MAC) doesn't indicate "Apple's Mac port".  We don't really have a macro to indicate such a thing at this time.  Perhaps we need to introduce one?  On Windows, for example, we indicate "Apple's Windows port" by the combination of PLATFORM(WIN) and PLATFORM(CG).

The Qt and Gtk ports do not set PLATFORM(MAC) when building on Mac OS X as they do not wish to use any of the Mac-level APIs even if they happen to be building on Mac OS X.  The situation may be the same for Chromium, in which case they may opt to leave PLATFORM(MAC) unset even if building on Mac OS X.  Otherwise, if they wish to use Mac-level APIs, they should define PLATFORM(MAC) and we should work out how to address any issues that arise from the code conflating PLATFORM(MAC) with "Apple's Mac port".

[Alexey Proskuryakov]

(In reply to comment #10)
> PLATFORM(MAC) doesn't indicate "Apple's Mac port".  We don't really have a
> macro to indicate such a thing at this time.

There seem to be different opinions about this (and I think I am not the only one who has an opinion different from yours :) ). That's not good.

I don't really care about the historical meaning of these macros - but we should ensure that everyone is on the same page. I do care about us staying practical, and not creating platform macros that no existing port needs, just for the sake of theoretical granularity.

[Amanda Walker]

(In reply to comment #10)
> PLATFORM(MAC) doesn't indicate "Apple's Mac port".  We don't really have a
> macro to indicate such a thing at this time.  Perhaps we need to introduce one?
>  On Windows, for example, we indicate "Apple's Windows port" by the combination
> of PLATFORM(WIN) and PLATFORM(CG).

This is what we used as a pattern, in fact--originally we were going to go exactly parallel with "PLATFORM(MAC) + PLATFORM(SKIA)", but PLATFORM(CHROMIUM) made more sense, since it controls code having to do with the application architecture, not just the graphics API.

> The Qt and Gtk ports do not set PLATFORM(MAC) when building on Mac OS X as they
> do not wish to use any of the Mac-level APIs even if they happen to be building
> on Mac OS X.  The situation may be the same for Chromium, in which case they
> may opt to leave PLATFORM(MAC) unset even if building on Mac OS X.  Otherwise,
> if they wish to use Mac-level APIs, they should define PLATFORM(MAC) and we
> should work out how to address any issues that arise from the code conflating
> PLATFORM(MAC) with "Apple's Mac port".

Indeed.  The Mac version of Chromium uses plenty of Mac APIs (so it's not analogous to Qt), but we're not using Apple's WebView--so all of the code that does things like crawl around the view hierarchy at runtime to invoke methods on superviews, or has dependencies on Apple's resource loaders, etc. will fail.  That's the sort of thing we intended to note with PLATFORM(CHROMIUM).  We *do* want to reuse as much Apple code in WebCore as possible, which is why we didn't fork off an entirely separate platform.

All that said, I'm happy to move the discussion to webkit-dev.  We won't be the last project to run into this, so a more general solution would be great.

[Adam Barth]

Comment on attachment 23502 [details]
Revised patch

Clearing review flag.  There has been a lot of discussion in this bug after the r+.  Please renominate if this patch still matches our current thinking on these #ifdefs.

[Avi Drissman]

Withdrawn. We're going to eventually move off of PLATFORM(MAC) anyway and there are quite a few efforts going on to split off from PLATFORM(MAC) stand-alone pieces.