RFR: 8328977 : JEditorPane.setPage not thread-safe, pageLoader not cancelled [v2]

[Alexey Ivanov]

On Fri, 19 Apr 2024 12:30:26 GMT, Renjith Kannath Pariyangad <rkannathpari at openjdk.org> wrote:

>> Hi Reviewers,
>> 
>> Added pageloader cancel before new page creation along with code restructuring. Moved all page loading calls inside synchronize to make it thread safe.
>> 
>> Regards,
>> Renjith.
>
> Renjith Kannath Pariyangad has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Suggesions updated

Changes requested by aivanov (Reviewer).

src/java.desktop/share/classes/javax/swing/JEditorPane.java line 481:

> 479:         if (!page.equals(loaded) && page.getRef() == null) {
> 480:             scrollRectToVisible(new Rectangle(0, 0, 1, 1));
> 481:         }

This should go below the `if`-block:


if ((postData == null) && (page.sameFile(loaded))) {


And the condition becomes redundant. After that `if`-block, we know for sure the page is going to be reloaded. That is the code should look like this:


        if ((postData == null) && (page.sameFile(loaded))) {
            // The same page with different reference
            /* <the code you have> */
            return;
        }

        // different url or POST method, load the new content

        // reset scrollbar
        scrollRectToVisible(new Rectangle(0, 0, 1, 1));

src/java.desktop/share/classes/javax/swing/JEditorPane.java line 488:

> 486:             }
> 487:             return;
> 488:         }

Suggestion:

        if ((postData == null) && (page.sameFile(loaded))) {
            // The same page with different reference
            if (reference != null) {
                scrollToReference(reference);
            } else {
                // Scroll to the top of the page
                scrollRectToVisible(new Rectangle(0, 0, 1, 1));
            }
            return;
        }

Here, `reference` contains the value of `page.getReference()`. I suggest moving that variable to the top too (after `postData`) and re-use it.

I also suggest marking `loaded`, `postData`, `reference` as `final`, it explicitly indicates the values are not changed in the method.

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

PR Review: https://git.openjdk.org/jdk/pull/18670#pullrequestreview-2011492799
PR Review Comment: https://git.openjdk.org/jdk/pull/18670#discussion_r1572416371
PR Review Comment: https://git.openjdk.org/jdk/pull/18670#discussion_r1572407882