On 4/18/19 5:51 AM, Hannes Wallnöfer wrote:
Hi Jon,
The webrev URL was missing in your RFR, but I found it at the expected location:
http://cr.openjdk.java.net/~jjg/8222669/webrev.00/
Now this is quite a bit nicer than my „solution“ for RawHtml length
calculation. Obviously, I still have a bit to learn in order to see potential
for code cleanup and not assume things are the way they are supposed to be.
Generally, I consider RawHtml to be a "class of last resort" to be used
when there is no better alternative. For example, given that we don't
parse HTML text given on the command line (e.g. for the legal footer
text), using RawHtml is a reasonable alternative. However, the reality
of the implementation is somewhat worse, because for somewhat pragmatic
reasons, the translations from DocTree nodes to HtmlTree nodes uses
somewhat more use to DocTree::toString and RawHtml than I think is
reasonable. So, I consider that part of the world to be "OK, but could
be better".
The only suggestion I have is that there are three almost identical pieces of
code that escape a String to a StringBuilder. It would be nice to factor that
out into a shared method somewhere.
Yes, I was aware of that when I was modifying the code. The code is not
quite identical, but now that you make me look at it again, I can see
how to make it happen. That will be a nice addition to this cleanup.
Thanks for pointing this out.
-- Jon
Hannes
Am 17.04.2019 um 22:02 schrieb Jonathan Gibbons <[email protected]>:
Please review a relatively simple changeset to introduce and use a new class to
explicitly handle HTML entities within an HtmlTree. This allows for some
customized behavior (specifically a character count of 0 for a zero-width
space) and avoids an otherwise unnecessary use of the RawHtml class.
In addition to introducing the new class, the use of Contents.SPACE and Contents.ZERO_WIDTH_SPACE is updated to use the
new entity constants. FInally, explicit uses of strings "<", ">" and
"&" are updated to use appropriate new entities.
-- Jon
JBS: https://bugs.openjdk.java.net/browse/JDK-8222669
Webrev: https://bugs.openjdk.java.net/browse/JDK-8222669
