webrevs.2 for macosx changes to jdk7u-osx

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

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