On Wed, 28 Oct 2020 03:26:16 GMT, Yoshiki Sato <ysato...@openjdk.org> wrote:
> HTML4 is no longer supported in javadoc.
>
> doclint needs to drop HTML4 support as well.
> The changes consist of:
> * Removing jdk.javadoc.internal.doclint.HtmlVersion and its references.
> * Sorting out supported tags and attributes in HTML5 (including fix incorrect
> permission of valign in TH, TR, TD, THEAD and TBODY)
> * Modifying test code and expected outputs to be checked in HTML5
Generally nice; congratulations on figuring this all out.
Some comments in files need response and/or action.
Please make the review non-draft as well
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclint/resources/doclint_zh_CN.properties
line 30:
> 28: dc.attr.lacks.value = \u5C5E\u6027\u7F3A\u5C11\u503C
> 29: dc.attr.not.number = \u5C5E\u6027\u503C\u4E0D\u662F\u6570\u5B57
> 30: dc.attr.not.supported.html4 = \u5C5E\u6027\u5728 HTML4
> \u4E2D\u4E0D\u53D7\u652F\u6301: {0}
In general, we (Dev) do not edit any localized resource files. That happens
"automatically" by the localization team.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclint/HtmlTag.java line
410:
> 408: OBSOLETE,
> 409: UNSUPPORTED
> 410: }
On one hand, I don't think we need this level of detail, but on the other, I
see it closely matches `AttrKind`, so OK.
Is there are useful distinction between INVALID / OBSOLETE / UNSUPPORTED ?
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclint/Checker.java line
692:
> 690: }
> 691: } catch (NumberFormatException ex) {
> 692: env.messages.error(HTML, tree,
> "dc.attr.img.border", attr);
It's generally better practice to use unique message keys at each call site.
This is so that if there is a downstream problem when someone reports a problem
with an error message, we know the exact single place in the code where the
message comes from.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclint/DocLint.java line
229:
> 227: String argVersion = arg.substring(arg.indexOf(":") + 1);
> 228: if (argVersion == null || !argVersion.equals("html5")) {
> 229: throw new BadArgs("dc.bad.value.for.option", arg,
> argVersion);
It might be friendly to give a warning when this option is used, saying that
`html5` is the default and only supported version, and that this option may be
removed in a future release.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclint/DocLint.java line
348:
> 346: String argVersion = arg.substring(arg.indexOf(":") + 1);
> 347: if (argVersion == null || !argVersion.equals("html5")) {
> 348: throw new IllegalArgumentException(argVersion);
These lines are only used when invoked from javac/javadoc etc, so it would be
reasonable to delete them entirely, provided those tools never try and use this
option.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclint/HtmlTag.java line 51:
> 49: * public.
> 50: *
> 51: * @see <a href="http://www.w3.org/TR/REC-html40/";>HTML 4.01
> Specification</a>
Given the presence of UNSUPPORTED in the ensuing code, I'd recommend leaving
this link, for historical purposes.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclint/HtmlTag.java line
360:
> 358: attrs(AttrKind.OK, COLSPAN, ROWSPAN, HEADERS, SCOPE,
> Attr.ABBR),
> 359: attrs(AttrKind.UNSUPPORTED, WIDTH, BGCOLOR, HEIGHT, NOWRAP,
> AXIS, ALIGN, CHAR, CHAROFF),
> 360: attrs(AttrKind.OK, VALIGN)), // Removed after JDK-8255214
> fixed.
Does this need attention? JDK-8255215 has been fixed
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclint/HtmlTag.java line
541:
> 539: OBSOLETE,
> 540: UNSUPPORTED
> 541: }
Is there a useful distinction between INVALID / OBSOLETE / UNSUPPORTED ?
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclint/HtmlVersion.java
line 1:
> 1: /*
Yay!
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclint/resources/doclint.properties
line 32:
> 30: dc.attr.not.supported.html5 = attribute not supported in HTML5: {0}
> 31: dc.attr.obsolete = attribute obsolete: {0}
> 32: dc.attr.obsolete.use.css = attribute obsolete, use CSS instead: {0}
Is this still required?
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclint/resources/doclint.properties
line 35:
> 33: dc.attr.repeated = repeated attribute: {0}
> 34: dc.attr.img.border = attribute border for img only accepts "0", use CSS
> instead: {0}
> 35: dc.attr.table.border = attribute border for table only accepts "" or "1",
> use CSS instead: {0}
I suggest quoting the attribute name "border"
test/langtools/tools/doclint/AccessibilityTest.out line 28:
> 26: * <table><tr><th>head<tr><td>data</table>
> 27: ^
> 28: AccessibilityTest.java:66: error: no summary or caption for table
summary is not a valid attribute in HTML5, so I would omit it from the message.
test/langtools/tools/doclint/AnchorTest.java line 3:
> 1: /*
> 2: * @test /nodynamiccopyright/
> 3: * @bug 8004832 8247957
The bug number should only be added when the test can be considered to be a
significant test of the work.
In general, don't bother to add the bug number when a test is just incidentally
affected by the work.
In this case, there appears to be no other change to the file, but I guess OK,
you've changed to golden output file.
test/langtools/tools/doclint/html/HtmlVersionTagsAttrsTest.java line 11:
> 9: * @run main DocLintTester -badargs -XhtmlVersion:
> HtmlVersionTagsAttrsTest.java
> 10: *
> 11: *
I'm guessing these blank lines are to maintain line numbers, right?
test/langtools/tools/doclint/html/TableTagTest.out line 22:
> 20: * <table summary="abc" width="50%"> <tr> <td> <tfoot> <tr>
> </tfoot></table>
> 21: ^
> 22: 7 errors
Does it work to add the final newline: various tools give warnings if files do
not end with newline.
test/langtools/tools/doclint/html/TextNotAllowed.out line 30:
> 28: TextNotAllowed.java:16: error: attribute not supported in HTML5: summary
> 29: * <table summary=description> abc </table>
> 30: ^
The test is 'Text Not Allowed', so the test file should be valid except for
text where it is not allowed. Don't add spurious other errors. In this case,
provide a `<caption>` to keep DocLint happy.
This applies throughout this test.
test/langtools/tools/doclint/tidy/InvalidName.out line 3:
> 1: InvalidName.java:17: error: invalid name for anchor: "foo()"
> 2: * <a id="foo()">invalid</a>
> 3: ^
This seems incorrect. In HTML5 all names are valid unless they contain
whitespace.
test/langtools/tools/doclint/tidy/TextNotAllowed.out line 2:
> 1: TextNotAllowed.java:14: error: attribute not supported in HTML5: summary
> 2: * <table summary=description> abc </table>
See previous comments about spurious error messages
test/langtools/tools/doclint/tidy/TrimmingEmptyTag.out line 5:
> 3: ^
> 4: TrimmingEmptyTag.java:15: error: attribute not supported in HTML5: summary
> 5: * <table summary=description></table>
ditto
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclint/DocLint.java line
148:
> 146: return;
> 147: } else if (useXhtmlVersion) {
> 148: System.err.println(localize("dc.main.use.xhtmlversion"));
use `out.println`
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclint/HtmlTag.java line
360:
> 358: EnumSet.of(Flag.ACCEPTS_BLOCK, Flag.ACCEPTS_INLINE),
> 359: attrs(AttrKind.OK, COLSPAN, ROWSPAN, HEADERS, SCOPE,
> Attr.ABBR),
> 360: attrs(AttrKind.UNSUPPORTED, WIDTH, BGCOLOR, HEIGHT, NOWRAP,
> AXIS, ALIGN, CHAR, CHAROFF, VALIGN)), // Removed after JDK-8255214 fixed.
remove comment
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclint/HtmlTag.java line
375:
> 373:
> 374: TR(BlockType.TABLE_ITEM, EndKind.OPTIONAL,
> 375: attrs(AttrKind.UNSUPPORTED, ALIGN, CHAR, CHAROFF, BGCOLOR,
> VALIGN)) { // Removed after JDK-8255215 fixed
remove comment
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclint/resources/doclint.properties
line 91:
> 89: dc.main.ioerror=IO error: {0}
> 90: dc.main.no.files.given=No files given
> 91: dc.main.use.xhtmlversion=html5 is the default and only supported version
> for -XhtmlVersion, and this option may be removed in a future release.
change `version` to `value`
change `, and` to `;`
`
html5 is the default and only supported value for -XhtmlVersion; this option
may be removed in a future release.
`
-------------
Changes requested by jjg (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/893
