webrevs.2 for macosx changes to jdk7u-osx
Michael McMahon
michael.x.mcmahon at oracle.com
Thu Dec 1 13:15:58 PST 2011
Phil,
Comments below.
On 29/11/11 23:54, Phil Race wrote:
> Comments from looking at :-
>
> Changes relative to jdk7u-osx
> http://cr.openjdk.java.net/~michaelm/7113349/2/jdk7u-osx/modified/
>
> But before that I want to raise that there are a few ways of handling issues
> that require to be done differently and ask what is going to be the case :
> 1. Fix them first in the MACOSX_PORT and then re-generate the webrev.
> 2. In the interests of time, push first to jdk7u-osx, *then* fix them in MACOSX_PORT,
> and create a new patch when they are all resolved.
> 3. Fix them in this patch only, and expect that they won't crop up again when
> next syncing from MACOSX_PORT to jdk7u-osx.
> This latter one is the least work, but implies that the MACOSX_PORT forest
> is more quickly heading for retirement before we forget all this.
>
I don't quite follow what the patch is you are referring to here.
I'm not sure either what the process will be once jdk is working in
jdk7u-osx.
In the short term, I'd expect to continue taking in fixes from macosx-port
for corelibs, and I think Artem will do the same for client.
> So which of these should be the norm ?
>
>
> Also some the comments aren't things I reasonably expect Mike to be able
> to resolve, as they are far too involved, or maybe just point to clean up
> we should consider.
>
>
> But in the great scheme of things the number of changes to shared [ client ] code
> as well as the changes themselves look much more reasonable now.
>
> On to comments :-
>
> FILES_export.gmk :
> 79 sun/awt/SunHints.java \
>
> This seems wrong .. or at least unnecessary as there's no native methods declared in this file.
>
ok
> More of an observation for investigation is that sun/awt/Makefile installs
> src/macosx/classes/sun/awt/fontconfigs/macosx.fontconfig.properties
>
right, though I'd prefer if the client team could investigate this later.
> I'm not sure if this is needed or used. and it looks a lot like a copy
> of the Windows file.
>
> mawt.gmk - and numerous other locations - reference X11.
> The client UI will be all cocoa based so the extent to which the X11
> code will
> continue to work is questionable.
>
> make/sun/cmm/lcms/Makefile expresses a depdency on libxawt.so
>
> This was apparently pre-existing but I can't think what there is in
> libxawt that
> would be needed by libcms.so which should run fine against headless ..
>
> Also the actual include change will get simplified in JDK 8 where for
> jigsaw
> needs the directory structure will be flattened like the OS X version
> here.
>
same for the above comments, if that's ok ...
> sun/font/Makefile ..
>
> the duplicate lines 203-212 seem unneeded .. why not just add
> CLASSHDRDIR to
> CPPFLAGS on all platforms .. can't do any harm, can it ?
>
right. that's already fixed. At least, the change is refactored to be
the minimal delta
>
> The same - more so - goes for lines 98-113 in sun/jawt/Makefile
>
fixed also.
>
> In sun/xawt/Makefile
>
> 141 ifeq ($(PLATFORM), macosx)
> 142 CPPFLAGS += -I$(CUPS_HEADERS_PATH)
> 143 endif
>
> This duplicates line 113
> 113 CPPFLAGS += -I$(CUPS_HEADERS_PATH)
>
fixed already.
>
>
> make/tools/freetypecheck/Makefile
>
>
> 53 ifeq ($(PLATFORM), macosx)
> 54 ifeq ($(OS_NAME), netbsd)
> 55 FT_LD_OPTIONS += -Wl,-R$(FREETYPE_LIB_PATH)
> 56 endif
> 57 FT_LD_OPTIONS += -lfreetype -lz
> 58 else # linux
>
>
> We don't need the inner netbsd case
>
fixed already
> Component.java
>
> 7913 if (clearOnFailure&& !res) {
> becomes
> 7913 if (!res) {
>
> This needs careful examination by someone who knows the focus manager ..
>
> sun/font/FontUtilities - and others - some casts to CompositeFont have
> been removed because logical fonts in the OSX port are handled as
> CFont which directly subclassees Font2D .. this is a bit messy in
> part because of the fact that CFont, which is platform specific
> relies on shared code a lot. We're getting away with it right now but
> some day this could force some substantial changes one way or another.
>
> SunFontManager.java
> line 3800 is very .. very long. Definitely> 80 chars
>
as before, I'd prefer if the client team could investigate this later.
>
>
> src/share/classes/sun/print/PrintJob2D.java
>
>
> 488 // MACOSX change
> 489 //<rdar://problem/2937917> REGR: Print Dialog does not appear
> 490 if (jobAttributes.getDialog() == DialogType.NATIVE) {
> 491 PageFormat oldPF = pageFormat;
> 492 PageFormat newPF = printerJob.pageDialog(oldPF);
> 493 if (oldPF == newPF) return false;
> 494 pageFormat = newPF;
> 495 }
> 496 // MACOSX
>
>
> I don't know what problem this was trying to fix but
> this can't be pushed. AWT printing doesn't provide for a PageDialog,
> and this would change behaviour on all platforms.
>
Ok. I'll take it out completely. In terms of keeping track of code that
may be important
on Mac, I guess the macos-port webrev would point to changes like this.
Maybe some of the Apple folks could shed some light on what this code does.
>
>
> *src/share/classes/sun/print/RasterPrinterJob.java*
>
>
> I see a lot of methods made protected. I am not
>
>
> sure how necessary this is (is there a better way), since Windows
> printing
>
> did not need to do this and its similarly all native based.
>
Again, am I right in assuming the change is harmless enough in the short
term
and can be investigated later?
>
>
>
> src/solaris/native/sun/awt/fontpath.c
>
>
> 64 // MMM: Is this still needed?
>
> I suspect only in the X11 Toolkit
> 66 // XXXDARWIN: Hard-code the path to Apple's freetype, as it is
>
> You mean fontconfig not freetype.
>
> 73 #define FONTCONFIG_DLL_VERSIONED X11_PATH "/lib/" VERSIONED_JNI_LIB_NAME("fontconfig", "1")
>
>
> I don't see where X11_PATH is defined but I think yuou can have /usr/X11R6 or /usr/X11R7
> in which case we will be baking in something that works only for some people. This is offset
> by being code that isn't actually for production ..
>
right. It's actually /usr/X11R6 defined in Defs-macosx.gmk
> 132 #elif MACOSX
> 133 static char *fullBSDFontPath[] = {
> 134 X11_PATH "/lib/X11/fonts/TrueType",
> ...
>
> I expect these directories exist if you have X11 installed but
> these aren't the standard OS X font folders which are (off the top of my head)
> /System/Library/Fonts and /Library/Fonts and /User/<foo>/Library/Fonts
>
> It seems a bit at odds with the fontconfig file I commented on above.
>
> Not sure what you should do here but I'd at least check the paths
> are valid and rename the var tofull_MACOSX_X11FontPath
>
Yes, this stuff seems to work fine, but I renamed the variable as suggested.
So, could you let me know if you're ok with me pushing subject to the
changes above being made? I will test out the changes and generate another
webrev for the exact change.
Thanks,
Michael.
> -phil.
>
>
>
> On 11/28/11 08:08 AM, Michael McMahon wrote:
>> Hi,
>>
>> Here is another version of the macosx webrev. This time it includes
>> all of the modifications and new files from macosx-port. Hence many
>> of the problems pointed out earlier with the inconsistencies relative
>> to the bsd code
>> are gone now. It builds and runs on all platforms and has been synced
>> with
>> jdk7u-dev (as of Friday Nov 25). I left the // MacOSX comments in
>> to highlight changes that people may want to look at more closely.
>>
>> Lastly, this time I have also included a webrev showing the changes
>> relative to macosx-port
>> for reference.
>>
>> Changes relative to jdk7u-osx
>> http://cr.openjdk.java.net/~michaelm/7113349/2/jdk7u-osx/modified/
>>
>> New files
>> http://cr.openjdk.java.net/~michaelm/7113349/2/jdk7u-osx/modified/
>>
>> Changes relative to macosx-port
>> http://cr.openjdk.java.net/~michaelm/7113349/2/macosx-port/modified/
>>
>> Thanks,
>> Michael.
>>
>
More information about the macosx-port-dev
mailing list