On Wed, 19 May 2021 12:31:20 GMT, liach <github.com+7806504+li...@openjdk.org> wrote:
>> Something is wrong, but I cannot quite put my finger on it. I think this PR >> merely highlights a pre-existing issue that should be investigated >> separately. >> >> I'm surprised by the concept of validity for the **generated** content. I'm >> also surprised by the fact that a part of the content can be silently >> omitted depending on whether or not that part is valid. This suggests that >> javadoc uses instances of HtmlTree as its data model which it shouldn't do. > > This special path for `ContentBuilder` was added in > [JDK-8026370](https://bugs.openjdk.java.net/browse/JDK-8026370), commit > 99e02c21cdb25bd113dd751f63f0c1fd27f07b57, which, surprisingly, is like the > opposite of this missing empty tag issue; the `isValid` was not touched since > its addition from long ago in > [JDK-6851834](https://bugs.openjdk.java.net/browse/JDK-6851834). > ContentBuilder itself was introduced in > [JDK-8011642](https://bugs.openjdk.java.net/browse/JDK-8011642) or commit > f961eafe351c24536e539819e9efb14272701195. > > It is not quite clear to me the original intentions of `isValid` from over > one decade ago. One of the clue may be that when `ContentBuilder` was added, > jonathan maybe did not have `isValid` handling in mind? And we are now in a > somewhat weird shape. My general recollection is that this dates all the way back to the introduction of `HtmlTree`. Prior to that, HTML was generated directly with `PrintWriter.print` calls (!) and that code gave rise to some notable errors; specifically, empty lists. A list was started, beginning with `<ul>` or `<dl>`, items were added, and the list was closed. Except that sometimes no items were added. Which was generally tolerated by browsers, but not by checkers like `HTML Tidy` when we started using that. Roll the clock forward to the introduction of `HtmlTree`. For the most part, this was done at that time by a mostly direct translation of `print` calls into creating and adding `HtmlTree` nodes. The "empty list" problem was solved by checking _ex post facto_ that nodes were valid before adding them into an enclosing container. The conversion was a big enough job in itself that it was not reasonable to restructure the code at that time to decide whether we should even be creating the list in the first place. It would be better to check the collection of items to be added up front, and not attempt to create an invalid HTML list with no contents in the first place. I've not (yet) followed this thread in detail, but IMO the `isValid` mechanism may be a wart of Technical Debt that it is time to remove. If nothing else, the general improvements to the API over the past few years should mean that we should always and only create valid nodes in the first place, and that we should never create or need to check for invalid nodes. That being said, such a change may be a Big Deal and should probably be handled separately. ------------- PR: https://git.openjdk.java.net/jdk/pull/4066
