Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8251123: doclint warnings about missing javadoc tags and comments #369

Closed
wants to merge 3 commits into from

Conversation

@mrserb
Copy link
Member

@mrserb mrserb commented Sep 25, 2020

We have a number of missing javadoc tags and comments in the desktop module.
Most of the missing comments are related to the serialized form.

The fix:

  • Adds missing comments to the non-static/non-transient fields(even private) of the "serializable" classes
  • Adds comments to the "serializable" classes even if those classes are non-public
  • Fixes references/adds missing tags to the special methods(like readObject/writeObject)
  • Delete the java.awt.PeerFixer class.

I need advice about what exact change should be reviewed in the CSR(except PeerFixer removal)

Note that in some cases I added the comments to the "implementation details", so I did not specify it fully.

The old review request:
https://mail.openjdk.java.net/pipermail/beans-dev/2020-August/000423.html


Progress

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

Issue

  • JDK-8251123: doclint warnings about missing javadoc tags and comments

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/369/head:pull/369
$ git checkout pull/369

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 25, 2020

👋 Welcome back serb! 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
Copy link

@openjdk openjdk bot commented Sep 25, 2020

@mrserb The following labels will be automatically applied to this pull request: 2d awt beans i18n swing.

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

@mrserb
Copy link
Member Author

@mrserb mrserb commented Sep 25, 2020

/csr

@openjdk openjdk bot added the csr label Sep 25, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Sep 25, 2020

@mrserb this pull request will not be integrated until the CSR request JDK-8252544 for issue JDK-8251123 has been approved.

@mrserb mrserb marked this pull request as ready for review Sep 25, 2020
@openjdk openjdk bot added the rfr label Sep 25, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 25, 2020

Webrevs

@jayathirthrao
Copy link
Member

@jayathirthrao jayathirthrao commented Sep 28, 2020

Inputs from https://mail.openjdk.java.net/pipermail/beans-dev/2020-August/000424.html are incorporated or is this fresh git review?

@mrserb
Copy link
Member Author

@mrserb mrserb commented Sep 28, 2020

Inputs from https://mail.openjdk.java.net/pipermail/beans-dev/2020-August/000424.html are incorporated or is this fresh git review?

It is a fresh update, it changes from the old review request. I made it less "dangerous".

/**
* Focus transfers for the lightweight components request should be made
* synchronously.
*/

This comment has been minimized.

@jayathirthrao

jayathirthrao Sep 30, 2020
Member

More simpler way "Focus transfers should be synchronous for lightweight component requests"


/**
* A {@code IIODOMException} is thrown by the {@code IIOMetadataNode} in
* "exceptional" circumstances.

This comment has been minimized.

@prrace

prrace Oct 2, 2020
Contributor

"A" -> "An"

@@ -810,52 +827,3 @@ public AccessibleRole getAccessibleRole() {
} // class AccessibleAWTScrollPane

}

/*
* In JDK 1.1.1, the pkg private class java.awt.PeerFixer was moved to

This comment has been minimized.

@prrace

prrace Oct 2, 2020
Contributor

As I mentioned in the OLD review thread for hg, this definitely needs a CSR
and it needs to be called out.
I think it should be separated out from this fix which is about fixing doclint warnings but here you are making an incompatible change. Let's not mix the two.

/*
* Serial Data Version
/**
* Serialized data version.
* @serial
*/
private int checkboxMenuItemSerializedDataVersion = 1;

This comment has been minimized.

@prrace

prrace Oct 2, 2020
Contributor

OK. Good this was being discussed in the old review and it should stay.


/**
* This constant is used when the backward focus traversal order is active.
*/
private final int BACKWARD_TRAVERSAL = 1;

This comment has been minimized.

@prrace

prrace Oct 2, 2020
Contributor

I see you also reverted the change of these two to static so that is also good.

Copy link
Contributor

@prrace prrace left a comment

I think the main thing here is I would separate out removing the duplicate PeerFixer into a new bug.

I also see that the CSR is still just a pure template.

@prrace
Copy link
Contributor

@prrace prrace commented Oct 2, 2020

I need advice about what exact change should be reviewed in the CSR(except PeerFixer removal)

Probably almost all of it.

mrserb added 2 commits Oct 3, 2020
@mrserb
Copy link
Member Author

@mrserb mrserb commented Oct 3, 2020

The PeerFixer is extracted to the separate CR:
https://bugs.openjdk.java.net/browse/JDK-8253965
#493

Other review comments fixed as well, CSR is in progress.

@mrserb
Copy link
Member Author

@mrserb mrserb commented Oct 3, 2020

CSR is updated.

@prrace
prrace approved these changes Oct 3, 2020
@openjdk openjdk bot removed the csr label Oct 6, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Oct 6, 2020

@mrserb 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 more details.

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

8251123: doclint warnings about missing javadoc tags and comments

Reviewed-by: jdv, prr

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 135 new commits pushed to the master branch:

  • 5d84e95: 8204256: improve jlink error message to report unsupported class file format
  • 4fe68f5: 8253426: jpackage is unable to generate working EXE for add-launcher configurations
  • c9d0407: 8253794: TestAbortVMOnSafepointTimeout never timeouts
  • f2f77f7: 8253761: Wrong URI syntax printed by jar --describe-module
  • b29e108: 8253944: Certain method references to VarHandle methods should fail
  • 88d75c9: 8156071: List.of: reduce array copying during creation
  • ea27a54: 8224509: Incorrect alignment in CDS related allocation code on 32-bit platforms
  • 4d29116: 8253433: Remove -XX:+Debugging product option
  • 81dae70: 8253948: Memory leak in ImageFileReader
  • 65cab55: 8253971: ZGC: Flush mark stacks after processing concurrent roots
  • ... and 125 more: https://git.openjdk.java.net/jdk/compare/cfa3f7493149170f2b23a516bc95110dab43fd06...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.

@openjdk openjdk bot added the ready label Oct 6, 2020
@mrserb
Copy link
Member Author

@mrserb mrserb commented Oct 6, 2020

/integrate

@openjdk openjdk bot closed this Oct 6, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Oct 6, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Oct 6, 2020

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

  • c9d1dcc: 8253902: G1: Starting a new marking cycle before the conc mark thread fully completed causes assertion failure
  • 9199783: 8253565: PPC64: Fix duplicate if condition in vm_version_ppc.cpp
  • 1728547: 8254010: GrowableArrayView::print fails to compile
  • 6e61861: 8254046: Remove double semicolon introduced by JDK-8235521
  • 5d84e95: 8204256: improve jlink error message to report unsupported class file format
  • 4fe68f5: 8253426: jpackage is unable to generate working EXE for add-launcher configurations
  • c9d0407: 8253794: TestAbortVMOnSafepointTimeout never timeouts
  • f2f77f7: 8253761: Wrong URI syntax printed by jar --describe-module
  • b29e108: 8253944: Certain method references to VarHandle methods should fail
  • 88d75c9: 8156071: List.of: reduce array copying during creation
  • ... and 129 more: https://git.openjdk.java.net/jdk/compare/cfa3f7493149170f2b23a516bc95110dab43fd06...master

Your commit was automatically rebased without conflicts.

Pushed as commit f397b60.

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

@mrserb mrserb deleted the mrserb:JDK-8251123 branch Oct 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants