RFR: 8247957: remove doclint support for HTML 4

Jonathan Gibbons jjg at openjdk.java.net
Fri Dec 11 07:23:15 UTC 2020


On Wed, 28 Oct 2020 03:26:16 GMT, Yoshiki Sato <ysatowse at 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


More information about the compiler-dev mailing list