[ Including Alex; there is a question/confirmation related to a change he pushed, that needs his input ]
Hi Erik, thanks for your feedback, comments inline… Updated webrev: http://cr.openjdk.java.net/~chegar/8039362/01/webrev/ On 7 Apr 2014, at 16:00, Erik Joelsson <erik.joels...@oracle.com> wrote: > Hello Chris, > > The changes seem to do what you intend, but I have a couple of questions. > > If this is a standard properties file, wouldn't it make sense to either > "clean" or "compile" it like other properties files? We just want to copy this file, as is, into the exploded classes dir and resources.jar, so that it can be accessed through getResouceAsStream(). > I note that there is a macosx version of this file but from what I can see of > both the old and new makefile logic, it's not being used. Should it be? If > not it should be removed. Ah, I see that now. I just checked a build on Mac and it copies src/solaris/lib/content-types.properties into the build. The Mac version of this file was added by 7153735: "[macosx] Text with diacritics is pasted with broken encoding" [1], but I don’t believe anyone in the networking team was aware of it. The only difference between the Mac and Solaris versions [2] is a missing change that I pushed a while back for 7025938 ”Add bitmap mime type to content-types.properties” [3] ( this change should be in all versions of the file! ). The Mac version needs to be removed, since it is never used and just confusing. Thanks for catching this Erik. (Note: there are genuine differences between the Unix and Windows versions) Alex, You ok with me removing this file, or was there a specific reason it was added? I cannot find one. > In CopyIntoClasses.gmk, there is no need to declare explicit rules for files > that follow standard patterns. For this file, you could add it to the list of > CLEAN_FILES, if cleaning is ok in this case. Or if straight copying is > preferred, adding a pattern to COPY_PATTERNS that includes this file is also > possible. In the updated webrev I added it to COPY_PATTERNS so that it is copied as is. > On the other hand, since there is the complication of the unused macosx > version of the file, standard patterns may not apply. It would be good to > resolve this and make content-types.properties fit better with an existing > pattern. The Mac version has been removed in the latest webrev, pending confirmation from Alex. -Chris. [1] http://hg.openjdk.java.net/jdk9/dev/jdk/rev/e0bf70361777 [2] $ diff -u src/macosx/lib/content-types.properties src/solaris/lib/content-types.properties --- src/macosx/lib/content-types.properties 2014-03-27 10:28:14.807133317 +0000 +++ src/solaris/lib/content-types.properties 2014-03-27 10:28:22.135133049 +0000 @@ -225,6 +225,10 @@ icon=png;\ action=browser +image/bmp: \ + description=Bitmap Image;\ + file_extensions=.bmp; + text/html: \ description=HTML Document;\ file_extensions=.htm,.html;\ [3] http://hg.openjdk.java.net/jdk8/tl/jdk/rev/46b53f80ab0a > > /Erik > > On 2014-04-07 16:33, Chris Hegarty wrote: >> Adding build-dev ( for the makefile changes ). >> >> -Chris. >> >> -------- Original Message -------- >> Subject: RFR [9] 8039362: Read content-types.properties as a resource >> Date: Mon, 07 Apr 2014 15:27:43 +0100 >> From: Chris Hegarty <chris.hega...@oracle.com> >> To: OpenJDK Network Dev list <net-dev@openjdk.java.net> >> >> Following JDK-8004963: "URLConnection, downgrade normative reference to >> ${java.home}/lib/content-types.properties", this bug [1] moves >> content-types.properties out of the image lib directory and into >> resources.jar ( to be loaded as a resources file ). This approach is >> acceptable, since the file is not expected to be user editable. >> >> Webrev: >> http://cr.openjdk.java.net/~chegar/8039362/00/webrev/ >> >> MimeTable.save(String) can be simply removed since it is never called, >> and editing the default table is not supported. >> >> The motive for this bug is the modular JDK where we need the flexibility >> to put anything that is module-private into a module-private location. >> In this case it would appear that the above files are not a supported >> interface and so should move to a location that should be read as >> resources. >> >> -Chris. >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8039362 >> >> >
