JDK-8232644: bugs in serialized-form.html #3817

hns · 2021-04-30T14:06:13Z

This implements most suggestions implemented in the JBS issue. The heading for class entries in the page is shortened to " ", followed by a simplified signature line like "class X extends Y implements Serializable".

Furthermore, package names in package headings now link to the package page, @param info is added to the methods, Externalizable is used for classes that implement it. I also added a check for content in @serial tags to avoid spurious empty   lines in the output.

I increased the font size for headings h4 to h6 in the stylesheets because their size was smaller than the default text size, which looked strange. Although the change in font size from h3 to h4 and beyond is now smaller I think it's still recognizable. Headings h4 to h5 are used very little in Javadoc (mostly static doc files and the serialized-form page). I looked at some of the static files and they looked good to me with the new larger heading fonts.

Although I didn't end up using the Signatures class to generate the class signatures (they're just too simple and too specific in what they list), I left in some cleanup of Signatures.TypeSignature, most significantly moving the processing of modifiers from ClassWriterImpl to Signatures.TypeSignature.

I only removed the obsolete resources from the English resource file out of habit of not touching the Chinese and Japanese ones. I guess I could remove it there too.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8232644: bugs in serialized-form.html

Reviewers

Jonathan Gibbons (@jonathan-gibbons - Reviewer) ⚠️ Review applies to 9a9e4a4

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3817/head:pull/3817
$ git checkout pull/3817

Update a local copy of the PR:
$ git checkout pull/3817
$ git pull https://git.openjdk.java.net/jdk pull/3817/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3817

View PR using the GUI difftool:
$ git pr show -t 3817

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3817.diff

bridgekeeper · 2021-04-30T14:06:45Z

👋 Welcome back hannesw! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2021-04-30T14:08:57Z

@hns The following label will be automatically applied to this pull request:

javadoc

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

hns · 2021-04-30T14:09:57Z

The new serialized-form.html page including the stylesheet changes can be viewed here:

http://cr.openjdk.java.net/~hannesw/8232644/api.00/serialized-form.html

mlbridge · 2021-04-30T14:13:07Z

Webrevs

01: Full - Incremental (092af734)
00: Full (9a9e4a49)

jonathan-gibbons

Generally: wow, very nice cleanup!

That being said, there is a potential conflict/race with a separate review to make sealed cases permanent (not preview.) So, this is approved, with a warning to coordinate with @vicente-romero-oracle about the sealed classes review #3613

jonathan-gibbons · 2021-05-06T14:37:11Z

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/Signatures.java

-            this.configuration = classWriter.configuration;
-        }
+        private static final Set<String> previewModifiers
+                = Set.of("sealed", "non-sealed");


This needs to be coordinated with the work to make sealed final.
In the changes for that, this set is now empty but the code to process the set (lines 110-126) remains, ready for any future preview modifiers.

Also, given this code exists elsewhere, we should probably try and pull it in to a shared method. That being said, Signatures is probably conceptually the right place for the shared method (not Utils.)

Also, whether or not we make a shared method, we should probably add an explanatory comment explaining why we keep an empty set around, just in case.

Generally: wow, very nice cleanup!

That being said, there is a potential conflict/race with a separate review to make sealed cases permanent (not preview.) So, this is approved, with a warning to coordinate with @vicente-romero-oracle about the sealed classes review #3613

Thanks for the review, Jon. @vicente-romero-oracle please go ahead with your PR, I'll update mine afterwards. (I should have been aware you were removing these sealed preview bits.)

jonathan-gibbons · 2021-05-06T14:42:25Z

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/Utils.java

@@ -493,7 +493,7 @@ public boolean definesSerializableFields(TypeElement aclass) {
    }

    @SuppressWarnings("preview")
-    public String modifiersToString(Element e, boolean trailingSpace) {
+    public String modifiersToString(Element e) {


Thinking aloud, would this method be better in Signatures? Maybe later...

openjdk · 2021-05-06T14:48:26Z

@hns This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8232644: bugs in serialized-form.html

Reviewed-by: jjg

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 102 new commits pushed to the master branch:

3fcdc50: 8266646: Add more diagnostic to java/lang/System/LoggerFinder/modules
9a19a0c: 8264760: JVM crashes when two threads encounter the same resolution error
14f0afe: 8214237: Join parallel phases post evacuation
2798b0d: 8266349: Pass down requested page size to reserve_memory_special
e0c8688: 8262992: Improve @see output
d2b5350: 8263507: Improve structure of package summary pages
a65021e: 8266618: Remove broken -XX:-OptoRemoveUseless
94c6177: 8266536: Provide a variant of os::iso8601_time which works with arbitrary timestamps
71b8ad4: 8266609: AArch64: include FP/LR space in LIR_Assembler::initial_frame_size_in_bytes()
ebb68d2: 8049700: Deprecate obsolete classes and methods in javax/swing/plaf/basic
... and 92 more: https://git.openjdk.java.net/jdk/compare/eb8db12ce67b10957fa94fbf61d9a30db34ec01a...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

…asses changes

hns · 2021-05-07T13:51:29Z

I decided to back out of the signature/preview related cleanup changes in this PR, which are unrelated to the actual issue. I'll file a separate issue for the signature cleanup. The remaining changes are identical to the reviewed changes and should not conflict with any other pending PRs.

hns · 2021-05-07T15:32:49Z

/integrate

openjdk · 2021-05-07T15:35:19Z

@hns Since your change was applied there have been 107 commits pushed to the master branch:

b5b3119: 8266589: (fs) Improve performance of Files.copy() on macOS using copyfile(3)
947d69d: 8265042: javadoc HTML files not generated for types nested in records
946b0fe: 8266645: javac should not check for sealed supertypes in intersection types
74fecc0: 8266503: [UL] Make Decorations safely copy-able and reduce their size
86b8dc9: 8265426: Update java.security to use instanceof pattern variable
3fcdc50: 8266646: Add more diagnostic to java/lang/System/LoggerFinder/modules
9a19a0c: 8264760: JVM crashes when two threads encounter the same resolution error
14f0afe: 8214237: Join parallel phases post evacuation
2798b0d: 8266349: Pass down requested page size to reserve_memory_special
e0c8688: 8262992: Improve @see output
... and 97 more: https://git.openjdk.java.net/jdk/compare/eb8db12ce67b10957fa94fbf61d9a30db34ec01a...master

Your commit was automatically rebased without conflicts.

Pushed as commit c6aa8f1.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

JDK-8232644: bugs in serialized-form.html

9a9e4a4

openjdk bot added the rfr Pull request is ready for review label Apr 30, 2021

openjdk bot added the javadoc [email protected] label Apr 30, 2021

jonathan-gibbons approved these changes May 6, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 6, 2021

JDK-8232644: Back out of signature cleanup conflicting with sealed cl…

092af73

…asses changes

openjdk bot closed this May 7, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

JDK-8232644: bugs in serialized-form.html #3817

JDK-8232644: bugs in serialized-form.html #3817

Uh oh!

hns commented Apr 30, 2021 •

edited by openjdk bot

Loading

Uh oh!

bridgekeeper bot commented Apr 30, 2021

Uh oh!

openjdk bot commented Apr 30, 2021

Uh oh!

hns commented Apr 30, 2021

Uh oh!

mlbridge bot commented Apr 30, 2021 •

edited

Loading

Uh oh!

jonathan-gibbons left a comment

Uh oh!

jonathan-gibbons May 6, 2021

Uh oh!

hns May 6, 2021

Uh oh!

jonathan-gibbons May 6, 2021

Uh oh!

openjdk bot commented May 6, 2021 •

edited

Loading

Uh oh!

hns commented May 7, 2021

Uh oh!

hns commented May 7, 2021

Uh oh!

openjdk bot commented May 7, 2021

Uh oh!

Uh oh!

JDK-8232644: bugs in serialized-form.html #3817

JDK-8232644: bugs in serialized-form.html #3817

Uh oh!

Conversation

hns commented Apr 30, 2021 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Apr 30, 2021

Uh oh!

openjdk bot commented Apr 30, 2021

Uh oh!

hns commented Apr 30, 2021

Uh oh!

mlbridge bot commented Apr 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

jonathan-gibbons left a comment

Choose a reason for hiding this comment

Uh oh!

jonathan-gibbons May 6, 2021

Choose a reason for hiding this comment

Uh oh!

hns May 6, 2021

Choose a reason for hiding this comment

Uh oh!

jonathan-gibbons May 6, 2021

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented May 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hns commented May 7, 2021

Uh oh!

hns commented May 7, 2021

Uh oh!

openjdk bot commented May 7, 2021

Uh oh!

Uh oh!

hns commented Apr 30, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Apr 30, 2021 •

edited

Loading

openjdk bot commented May 6, 2021 •

edited

Loading