RFR: 8328953 : JEditorPane.read throws ChangedCharSetException [v5]

Wed Mar 27 10:31:38 UTC 2024

On Wed, 27 Mar 2024 08:48:55 GMT, rjolly <duke at openjdk.org> wrote:

>> ChangedCharSetException is used to amend the charset during read according to html directives. Currently it causes immediate exit of the method which in turn causes failure to load html documents with charset directives (even if the latter must not change after all). This PR restores the catch operation as it was before the use of try with resources.
>
> rjolly has updated the pull request incrementally with one additional commit since the last revision:
> 
>   8328953 : JEditorPane.read throws ChangedCharSetException
>   
>   The fix is to add a nested `try`-block inside `try-with-resource`; all the exceptions are handled in the nested `try`; the outer `try`-with-resouces only closes the input stream.

Changes requested by aivanov (Reviewer).

test/jdk/javax/swing/JEditorPane/EditorPaneCharset.java line 35:

> 33: /*
> 34:  * @test
> 35:  * @key headless

Suggestion:

There's no *headless* key; only *headful* tests need to be marked specifically.

test/jdk/javax/swing/JEditorPane/EditorPaneCharset.java line 37:

> 35:  * @key headless
> 36:  * @bug 8328953
> 37:  * @summary JEditorPane.read throws ChangedCharSetException

Suggestion:

 * @summary Verifies JEditorPane.read doesn't throw ChangedCharSetException
            but handles it and reads HTML in the specified encoding

test/jdk/javax/swing/JEditorPane/EditorPaneCharset.java line 47:

> 45:             "<html lang=\"ru\">\n" +
> 46:             "<head>\n" +
> 47:            "    <meta http-equiv=\"Content-Type\" content=\"text/html; charset=windows-1251\">\n" +

Suggestion:

            "    <meta http-equiv="Content-Type" " + 
            "         content="text/html; charset=windows-1251">\n" +

test/jdk/javax/swing/JEditorPane/EditorPaneCharset.java line 52:

> 50:             "</body></html>\n";
> 51: 
> 52:     public static void main() throws IOException {

Suggestion:

    public static void main(String[] args)
            throws IOException, BadLocationException {

The `main` function should be like this. In [my comment](https://github.com/openjdk/jdk/pull/17567#discussion_r1539063737), I referred to it as `main()` because typing all these in a comment is tedious.

Yes, Java now allows `main` without arguments but this feature is not supported in previous versions of Java, and tests should not rely on this feature.

Alternatively, you may use `throws Exception` to allow all kinds of exceptions. Using `throws Exception` is more common. I don't have a strong preference in this case.

test/jdk/javax/swing/JEditorPane/EditorPaneCharset.java line 65:

> 63:         } catch (IOException e) {
> 64:             throw new RuntimeException(e);
> 65:         }

Now that you added `throws IOException` to `main` declaration, you can allow it to escape to fail the test. Please do. Remove the try-catch block.

test/jdk/javax/swing/JEditorPane/EditorPaneCharset.java line 78:

> 76:         } catch (BadLocationException e) {
> 77:             throw new RuntimeException(e);
> 78:         }

Similarly, remove this try-catch block and allow `BadLocationException` to escape.

test/jdk/javax/swing/JEditorPane/EditorPaneCharset.java line 82:

> 80: 
> 81:     private EditorPaneCharset() {
> 82:     }

The constructor is not needed. It used it in my test, but now it's empty.

-------------

PR Review: https://git.openjdk.org/jdk/pull/17567#pullrequestreview-1962874040
PR Review Comment: https://git.openjdk.org/jdk/pull/17567#discussion_r1540832221
PR Review Comment: https://git.openjdk.org/jdk/pull/17567#discussion_r1540834671
PR Review Comment: https://git.openjdk.org/jdk/pull/17567#discussion_r1540828514
PR Review Comment: https://git.openjdk.org/jdk/pull/17567#discussion_r1540814672
PR Review Comment: https://git.openjdk.org/jdk/pull/17567#discussion_r1540822881
PR Review Comment: https://git.openjdk.org/jdk/pull/17567#discussion_r1540824315
PR Review Comment: https://git.openjdk.org/jdk/pull/17567#discussion_r1540825793