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

Wed Mar 27 10:53:25 UTC 2024

On Wed, 27 Mar 2024 10:43: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.

Other than a couple of nits, it looks good to me.

I'll submit a test job to run all the tests with this fix and this test.

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

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

Suggestion:

            "          content="text/html; charset=windows-1251">\n" +

I suggest moving it by one space to align to the start of the `http-equiv` attribute.

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

> 55:         editorPane.setContentType("text/html");
> 56:         Document document = editorPane.getDocument();
> 57:         // Shouldn't throw ChangedCharSetException

Suggestion:

        Document document = editorPane.getDocument();

        // Shouldn't throw ChangedCharSetException

I'd add a blank line here too. It starts a new block which is the point of the test.

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

> 66:         Element p = body.getElement(0);
> 67:         String pText = document.getText(p.getStartOffset(),
> 68:                                     p.getEndOffset() - p.getStartOffset());

Suggestion:

        String pText = document.getText(p.getStartOffset(),
                                        p.getEndOffset() - p.getStartOffset());

Let's align the second argument to the opening parenthesis.

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

PR Review: https://git.openjdk.org/jdk/pull/17567#pullrequestreview-1962948785
PR Review Comment: https://git.openjdk.org/jdk/pull/17567#discussion_r1540870025
PR Review Comment: https://git.openjdk.org/jdk/pull/17567#discussion_r1540872420
PR Review Comment: https://git.openjdk.org/jdk/pull/17567#discussion_r1540872209