webrevs.2 for macosx changes to jdk7u-osx
Phil Race
philip.race at oracle.com
Thu Dec 1 13:28:20 PST 2011
With the changes you outline below I think that'll be OK. I had noted
didn't expect
you to address or even investigate all of these which were more involved
etc but
I did want the observations captured in the review thread.
My comments about the process are concerned with making sure that
changes you make here in response to review comments don't get
overwritten in a subsequent import from the macosx port forest.
I don't know how we are going to ensure that.
-phil.
On 12/1/2011 1:15 PM, Michael McMahon wrote:
> 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