Hi Lance, > If you look at MultiReleaseJarTest.java, it explicitly references JAR FS in a > comment. Same for JFSTester.java in the @Summary. These should > definitely be updated. There are comments > in ModulesInCustomFileSystem.java as well.
Ok, agreed for MultiReleaseJarTest and JFSTester. I fixed the comments in there. In ModulesInCustomFileSystem I can't see comments that explicitly refer to JarFileSystem. > I would suggesting moving the code added to the constructor for checking > the releaseVersion/multi-release properties to a method and grouping it > with the other methods added for supporting MR jars around line 1396. > (keeps it easier to follow and maintain) > > Done that. > > Thank you. I do think however we need to change the name of the method > to perhaps “checkReleaseVersionProperty” as the current name > “initializeMultiReleaseJar” does not seem quite right to me. Ok, I changed it to " initializeReleaseVersion". I didn't follow your suggestion because the method not only checks the release version property but also triggers some initialization if a version property is found and it's an mr-jar. > I would also update the comment for the method to something like: > > ---------------- > Checks to see if the Zip File System property “releaseVersion” has been > specified. If it has not been specified then check for the existence of the > “multi-release” property. If set and the file represents a multi-release > JAR, > determine the runtime version to use. > ---------------- I updated the comment, trying to incorporate your suggestion. How do you like it? > Could you also add a comment above the isMultiReleaseJar method to for > clarity going forward (I know there was not one there before) > > Done, too. > > Thank you. > > I think we can tweak the comment slightly to something like > > ——————— > Returns true if the Manifest main attribute “Multi-Release” is set to true; > false otherwise > -------------------- Done. > I might change the name of the lookup field in the method makeParentDirs > with the addition of the Function lookup on line 126. > > I chose to change the name of the global field in line 126 to be named > mrlookup. Also updated the comment. > > I am not sure “mrlookup” is the best name as this field is used with or > without a multi-release jar. Perhaps “IndexNodeNameLookup” or > “entryLookup" Ok, entryLookup seems the best fit to me. Changed that. http://cr.openjdk.java.net/~clanger/webrevs/8234089.3/ Thanks again for reviewing. Cheers Christoph
