webrevs.2 for macosx changes to jdk7u-osx (original) (raw)

Phil Race philip.race at oracle.com
Tue Nov 29 15:54:00 PST 2011

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.

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.

More of an observation for investigation is that sun/awt/Makefile installs

src/macosx/classes/sun/awt/fontconfigs/macosx.fontconfig.properties

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.

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 ?

The same - more so - goes for lines 98-113 in sun/jawt/Makefile

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)

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

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

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.

*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.

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 ..

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//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

-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