On Thu, 12 Nov 2020 02:28:51 GMT, Jonathan Gibbons <j...@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. @jonathan-gibbons The new commit includes the changes for your review comments and "8256312: Valid anchor 'id' value not allowed". Please take a look over at your convenience. > Please make the review non-draft as well Thanks for reviewing @jonathan-gibbons This request should have got changed to "non-draft". FYI. all tests in jdk-tier1 are still green with the latest changes. https://mach5.us.oracle.com/mdash/jobs/yoshiki-jdk-20201211-0555-16616611 > 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. I see. Will discard all changes done in the localized resource files. > 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 ? OK: valid OBSOLETE: obsolete, deprecated, but still supported (valid) UNSUPPORTED: ever supported but no longer supported (invalid) INVALID: the rest of others (invalid) UNSUPPORTED can be used if we would like to choose a friendly message instead of saying "unknown tag" only. OBSOLETE is not used anywhere in this commit. Although HTML5 has some obsolete features, [JDK-8215577](https://bugs.openjdk.java.net/browse/JDK-8215577) didn't define them as valid features if my understanding is correct. So I chose not to allow obsolete features in order to avoid inconsistency. > 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. Agreed. I would like to change 675-695 as below case BORDER: if (currTag == HtmlTag.TABLE) { String v = getAttrValue(tree); try { if (v == null || (!v.isEmpty() && Integer.parseInt(v) != 1)) { env.messages.error(HTML, tree, "dc.attr.table.border.not.valid", attr); } } catch (NumberFormatException ex) { env.messages.error(HTML, tree, "dc.attr.table.border.not.number", attr); } } else if (currTag == HtmlTag.IMG) { String v = getAttrValue(tree); try { if (v == null || (!v.isEmpty() && Integer.parseInt(v) != 0)) { env.messages.error(HTML, tree, "dc.attr.img.border.not.valid", attr); } } catch (NumberFormatException ex) { env.messages.error(HTML, tree, "dc.attr.img.border.not.number", attr); } } break; > 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. Ok > 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. It works though. But I will add a blank line at the end for consistency. > 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. Thanks. I was lacking of such perspective. > 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 Ok. > 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 Ok. > 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. Ok. > 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 Yes, now that this can be removed. I will do. > 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. These lines are likely to be used as long as the "--doclint-format html5" option is permitted. For example, this option is still used in the make/common/JavaCompilation.gmk. > 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 ? Same distinction as mentioned for ElemKind > 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? Do you mean ".html5" would be better to be removed? If so, yes. I just left it because the message would be friendly if it still say "attribute not supported in HTML5". > 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" Ok. > 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. You are right. I will remove "summary" from the message, and then remove two of the tests that still use "summary". > 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. Understood. Thanks for your instruction. > 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? Right > 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. Understood. Need to file it as another DocLint bug. ------------- PR: https://git.openjdk.java.net/jdk/pull/893
