Re: RFR: 8283664: Remove jtreg tag manual=yesno for java/awt/print/PrinterJob/PrintTextTest.java [v2]
On Thu, 31 Oct 2024 19:16:30 GMT, Harshitha Onkar wrote: >> Daniel Gredler has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Update copyright year, imports, array style, frame title > > Is the manual test verified on all platforms? @honkar-jdk I've made the requested changes, is there anything else that should be addressed? @prrace Would you be able to provide the second review for this PR? - PR Comment: https://git.openjdk.org/jdk/pull/21716#issuecomment-2457554542
RFR: 8343601: javax/swing/JMenuItem/8158566/CloseOnMouseClickPropertyTest.java fails in ubuntu 22.04
It is seen that javax/swing/JMenuItem/8158566/CloseOnMouseClickPropertyTest.java fails in ubuntu 22.04 OCI instance. It is seen that test frame is being rendered at top-left of screen instead of at the center, and robot mouseMove is being used to point and click on radiobutton, checkbox etc and linux left dock placement can be a problem with this top-left rendering. Fix is made to render the frame at the center. Repeated iteration of the test run in 22.04 OCI instance is ok. - Commit messages: - Test fix Changes: https://git.openjdk.org/jdk/pull/21896/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=21896&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8343601 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/21896.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/21896/head:pull/21896 PR: https://git.openjdk.org/jdk/pull/21896
Re: RFR: 8339783: Implement JEP 479: Remove the Windows 32-bit x86 Port [v30]
On Mon, 4 Nov 2024 20:42:59 GMT, Magnus Ihse Bursie wrote: >> This is the implementation of [JEP 479: _Remove the Windows 32-bit x86 >> Port_](https://openjdk.org/jeps/479). >> >> This is the summary of JEP 479: >>> Remove the source code and build support for the Windows 32-bit x86 port. >>> This port was [deprecated for removal in JDK >>> 21](https://openjdk.org/jeps/449) with the express intent to remove it in a >>> future release. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > fix: jvm_md.h was included, but not jvm.h... This has now passed internal CI testing tier1-5 (except for one test that also fails in mainline). - PR Comment: https://git.openjdk.org/jdk/pull/21744#issuecomment-2457376495
Re: RFR: 8283664: Remove jtreg tag manual=yesno for java/awt/print/PrinterJob/PrintTextTest.java [v2]
On Thu, 31 Oct 2024 21:36:01 GMT, Daniel Gredler wrote: >> There are multiple issue with this test case >> 1) Parser error due to yesno in @run main/manual=yesno >> 2) User can only compare the UI rendering and compare with the print out. >> User can't mark the test as pass or fail due to pass or fail buttons are >> missing. >> 3) When the test is executed using jtreg after user click on the print >> button on the print dialog the whole test UIs ( frames) gets dispose and >> user cannot compare the printout with the UI. But this works as expected in >> test is running individually using java PrintTextTest > > Daniel Gredler has updated the pull request incrementally with one additional > commit since the last revision: > > Update copyright year, imports, array style, frame title Changes requested by aivanov (Reviewer). test/jdk/java/awt/print/PrinterJob/PrintTextTest.java line 1: > 1: /* Could you make the `PrintJAText` class a nested static class inside `PrintTextTest`? Yes, it requires more refactoring but this ensures no duplicate class names. Since `PrintJAText` extends `PrintTextTest`, I suggest moving all the painting logic into `PrintText` class which will be a nested static class. Then `PrintJAText` (renamed to `PrintJapaneseText` if I understand the intent correctly) will extend `PrintText`. All the other logic will remain unchanged. Separating the painting logic from the test class itself, I believe, would also make thing clearer. In addition to the above, let's resolve IDE warnings at lines 349 and 411 (in the current revision): -byte data[] = new byte[s.length()]; +byte[] data = new byte[s.length()]; -TextLayout tl = new TextLayout(s, new HashMap(), frc); +TextLayout tl = new TextLayout(s, new HashMap<>(), frc); Overall, the changes look good to me. Thank you for your contribution. - PR Review: https://git.openjdk.org/jdk/pull/21716#pullrequestreview-2416174421 PR Review Comment: https://git.openjdk.org/jdk/pull/21716#discussion_r1829683897
Re: RFR: 8283664: Remove jtreg tag manual=yesno for java/awt/print/PrinterJob/PrintTextTest.java [v2]
On Thu, 31 Oct 2024 21:38:48 GMT, Daniel Gredler wrote: > > Is the manual test verified on all platforms? > > This bug (JDK-8283664) only covers getting the test back to a runnable state. > There is a separate entry (JDK-8148334) for fixing bugs exposed by this test > which have not yet been fixed. I did verify that the bugs documented in > JDK-8148334 do still exhibit -- so JDK-8148334 should remain open for now. The test may need to be problem-listed against [8148334](https://bugs.openjdk.org/browse/JDK-8148334) as it's known to fail. - PR Comment: https://git.openjdk.org/jdk/pull/21716#issuecomment-2457689426
Re: RFR: 8339783: Implement JEP 479: Remove the Windows 32-bit x86 Port [v30]
On Mon, 4 Nov 2024 20:42:59 GMT, Magnus Ihse Bursie wrote:
>> This is the implementation of [JEP 479: _Remove the Windows 32-bit x86
>> Port_](https://openjdk.org/jeps/479).
>>
>> This is the summary of JEP 479:
>>> Remove the source code and build support for the Windows 32-bit x86 port.
>>> This port was [deprecated for removal in JDK
>>> 21](https://openjdk.org/jeps/449) with the express intent to remove it in a
>>> future release.
>
> Magnus Ihse Bursie has updated the pull request incrementally with one
> additional commit since the last revision:
>
> fix: jvm_md.h was included, but not jvm.h...
I kind of wish the __cdecl / __stdcall changes had been done separately. Oh
well.
src/hotspot/os/windows/os_windows.cpp line 5820:
> 5818: }
> 5819:
> 5820: // FIXME
???
src/hotspot/os/windows/os_windows.cpp line 5863:
> 5861: return nullptr;
> 5862: }
> 5863:
[pre-existing, and can't comment on line 5858 because it's not sufficiently
near a change.]
The calculation of `len` is wasting a byte when `lib_name` is null. The `+2`
accounts for the
terminating `NUL` and the underscore separator between the sym_name part and
the lib_name
part. That underscore isn't added when there isn't a lib_name part. I think
the simplest fix would
be to change `name_len` to `(name_len +1)` and `+2` to `+1` in that
calculation. And add some
commentary.
This might be deemed not worth fixing as there is likely often no actual
wastage, due to alignment
padding, and it slightly further complicates the calculation. But additional
commentary would still
be desirable, to guide the next careful reader. In which case it might be
simpler to describe the
fixed version.
Since this is pre-existing and relatively harmless in execution, it can be
addressed in a followup
change.
src/hotspot/share/include/jvm.h line 1165:
> 1163: #define AGENT_ONLOAD_SYMBOLS{"Agent_OnLoad"}
> 1164: #define AGENT_ONUNLOAD_SYMBOLS {"Agent_OnUnload"}
> 1165: #define AGENT_ONATTACH_SYMBOLS {"Agent_OnAttach"}
There is more cleanup that can be done here. These definitions are used as
array initializers (hence the surrounding curly braces). They are now always
singleton, rather than sometimes having 2 elements. The uses iterate over the
now always singleton arrays. Those iterations are no longer needed and could
be eliminated. And these macros could be eliminated, using the corresponding
string directly in each use. This can all be done as a followup change.
src/java.base/share/native/libjava/NativeLibraries.c line 67:
> 65: strcat(jniEntryName, "_");
> 66: strcat(jniEntryName, cname);
> 67: }
I would prefer this be directly inlined at the sole call (in findJniFunction),
to make it easier to verify there aren't any buffer overrun problems. (I don't
think there are, but looking at this in isolation triggered warnings in my
head.)
Also, it looks like all callers of findJniFunction ensure the cname argument
is non-null. So there should be no need to handle the null case in
findJniFunction (other than perhaps an assert or something). That could be
addressed in a followup. (I've already implicitly suggested elsewhere in this
PR revising this function in a followup because of the JNI_ON[UN]LOAD_SYMBOLS
thing.)
-
Changes requested by kbarrett (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/21744#pullrequestreview-2415002837
PR Review Comment: https://git.openjdk.org/jdk/pull/21744#discussion_r1829659373
PR Review Comment: https://git.openjdk.org/jdk/pull/21744#discussion_r1828969105
PR Review Comment: https://git.openjdk.org/jdk/pull/21744#discussion_r1829478432
PR Review Comment: https://git.openjdk.org/jdk/pull/21744#discussion_r1829684759
Re: RFR: 8283664: Remove jtreg tag manual=yesno for java/awt/print/PrinterJob/PrintTextTest.java [v2]
On Tue, 5 Nov 2024 16:44:14 GMT, Alexey Ivanov wrote: >> Daniel Gredler has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Update copyright year, imports, array style, frame title > > test/jdk/java/awt/print/PrinterJob/PrintTextTest.java line 1: > >> 1: /* > > Could you make the `PrintJAText` class a nested static class inside > `PrintTextTest`? > > Yes, it requires more refactoring but this ensures no duplicate class names. > > Since `PrintJAText` extends `PrintTextTest`, I suggest moving all the > painting logic into `PrintText` class which will be a nested static class. > Then `PrintJAText` (renamed to `PrintJapaneseText` if I understand the intent > correctly) will extend `PrintText`. > > All the other logic will remain unchanged. > > Separating the painting logic from the test class itself, I believe, would > also make thing clearer. > > > In addition to the above, let's resolve IDE warnings at lines 349 and 411 (in > the current revision): > > > -byte data[] = new byte[s.length()]; > +byte[] data = new byte[s.length()]; > > -TextLayout tl = new TextLayout(s, new HashMap(), frc); > +TextLayout tl = new TextLayout(s, new HashMap<>(), frc); > > > Overall, the changes look good to me. Thank you for your contribution. Thanks! Let me know if this last commit is what you had in mind. It might be easier to review with whitespace ignored, since the class additions required some indentation changes. - PR Review Comment: https://git.openjdk.org/jdk/pull/21716#discussion_r1829763803
Re: RFR: 8343418: Use HashMap instead of Hashtable for CSS.htmlAttrToCssAttrMap
On Sat, 2 Nov 2024 09:08:41 GMT, Andrey Turbanov wrote: > > I am not seeing a lot of value in this churn in completely OK code. > > Can't say I see that code as "OK". > Hashtable usage in 2024 brings eye bleeding for most of developers. > Specifying incorrect initial size adds even more _fun_. @prrace I tend to agree with @turbanoff, doing such clean-ups makes the code base cleaner. It's possible that such a change may introduce regressions… yet I think this change is rather safe. Using `null` as the key into `htmlAttrToCssAttrMap` looks like a bug, and Andrey provided his analysis which proves `null` is never used. I looked at the code too, and I didn't find any other usages. `htmlAttrToCssAttrMap` may be accessed concurrently, the text model in Swing says the document supports mutations from other threads, there's also asynchronous view implementations. However, the map is read-only after it's initialised. @ExE-Boss's suggestion of using `Map.of` to initialise the map looks reasonable. - PR Comment: https://git.openjdk.org/jdk/pull/21785#issuecomment-2457814390
Re: RFR: 8283664: Remove jtreg tag manual=yesno for java/awt/print/PrinterJob/PrintTextTest.java [v3]
On Tue, 5 Nov 2024 18:27:22 GMT, Alexey Ivanov wrote: >> Daniel Gredler has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Separate paint logic from test class > > test/jdk/java/awt/print/PrinterJob/PrintTextTest.java line 232: > >> 230: } >> 231: >> 232: // The test needs a physical font that supports Latin. > > The `getPhysicalFont` could be updated so that its `switch` statement uses > `Font.` constants, I'm pretty sure that was the intention. It looks like it's trying to be extra resilient by switching on `n.toLowerCase()` (i.e. case-insensitive). Is it OK to make it case-sensitive and use the constants? - PR Review Comment: https://git.openjdk.org/jdk/pull/21716#discussion_r1829855797
Re: RFR: 8338411: Implement JEP 486: Permanently Disable the Security Manager [v8]
> This is the implementation of JEP 486: Permanently Disable the Security > Manager. See [JEP 486](https://openjdk.org/jeps/486) for more details. The > [CSR](https://bugs.openjdk.org/browse/JDK-8338412) describes in detail the > main changes in the JEP and also includes an apidiff of the specification > changes. > > NOTE: the majority (~95%) of the changes in this PR are test updates > (removal/modifications) and API specification changes, the latter mostly to > remove `@throws SecurityException`. The remaining changes are primarily the > removal of the `SecurityManager`, `Policy`, `AccessController` and other > Security Manager API implementations. There is very little new code. > > The code changes can be broken down into roughly the following categories: > > 1. Degrading the behavior of Security Manager APIs to either throw Exceptions > by default or provide an execution environment that disallows access to all > resources by default. > 2. Changing hundreds of methods and constructors to no longer throw a > `SecurityException` if a Security Manager was enabled. They will operate as > they did in JDK 23 with no Security Manager enabled. > 3. Changing the `java` command to exit with a fatal error if a Security > Manager is enabled. > 4. Removing the hotspot native code for the privileged stack walk and the > inherited access control context. The remaining hotspot code and tests > related to the Security Manager will be removed immediately after integration > - see [JDK-8341916](https://bugs.openjdk.org/browse/JDK-8341916). > 5. Removing or modifying hundreds of tests. Many tests that tested Security > Manager behavior are no longer relevant and thus have been removed or > modified. > > There are a handful of Security Manager related tests that are failing and > are at the end of the `test/jdk/ProblemList.txt`, > `test/langtools/ProblemList.txt` and `test/hotspot/jtreg/ProblemList.txt` > files - these will be removed or separate bugs will be filed before > integrating this PR. > > Inside the JDK, we have retained calls to > `SecurityManager::getSecurityManager` and `AccessController::doPrivileged` > for now, as these methods have been degraded to behave the same as they did > in JDK 23 with no Security Manager enabled. After we integrate this JEP, > those calls will be removed in each area (client-libs, core-libs, security, > etc). > > I don't expect each reviewer to review all the code changes in this JEP. > Rather, I advise that you only focus on the changes for the area > (client-libs, core-libs, net, security, etc) that you are most f... Sean Mullan has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 217 commits: - Merge remote-tracking branch 'jdk-sandbox/jep486' into JDK-8338411 - Merge branch 'master' into jep486 - Merge branch 'master' into jep486 - Merge branch 'master' into jep486 - Merge remote-tracking branch 'jdk-sandbox/jep486' into JDK-8338411 - Remove "access" and "policy" options from debug help. - Merge branch 'master' into jep486 - Merge branch 'jep486' of https://github.com/openjdk/jdk-sandbox into jep486 - remove MainClassCantBeLoadedTest from problemlisting - remove LauncherErrors test from problemlisting - ... and 207 more: https://git.openjdk.org/jdk/compare/3fab8e37...e9e7f0c9 - Changes: https://git.openjdk.org/jdk/pull/21498/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=21498&range=07 Stats: 68934 lines in 1889 files changed: 2490 ins; 62600 del; 3844 mod Patch: https://git.openjdk.org/jdk/pull/21498.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/21498/head:pull/21498 PR: https://git.openjdk.org/jdk/pull/21498
Re: RFR: 8283664: Remove jtreg tag manual=yesno for java/awt/print/PrinterJob/PrintTextTest.java [v3]
On Tue, 5 Nov 2024 16:50:18 GMT, Alexey Ivanov wrote: > The test may need to be problem-listed against > [8148334](https://bugs.openjdk.org/browse/JDK-8148334) as it's known to fail. Should that be part of this PR? If so, do I just add this line to `test/jdk/ProblemList.txt`? `java/awt/print/PrinterJob/PrintTextTest.java 8148334 generic-all` - PR Comment: https://git.openjdk.org/jdk/pull/21716#issuecomment-2457814505
Re: RFR: 8337287: Update image in javax.swing.text.Document.insert [v2]
On Sun, 22 Sep 2024 14:47:14 GMT, Alexey Ivanov wrote:
>> This changeset updates the image in the documentation for the
>> `Document.insert` method. The image in `Document.remove` was updated by
>> [JDK-4622866](https://bugs.openjdk.org/browse/JDK-4622866) in PR #15701.
>>
>> Now the illustration of inserting looks similar to removing. The new image
>> is in SVG format.
>>
>> For reference:
>>
>> - [`Document.insert` in JDK
>> 22](https://docs.oracle.com/en/java/javase/22/docs/api/java.desktop/javax/swing/text/Document.html#insertString(int,java.lang.String,javax.swing.text.AttributeSet));
>> - [Updated docs for JDK
>> 24](https://cr.openjdk.org/~aivanov/8337287/api/java.desktop/javax/swing/text/Document.html#insertString(int,java.lang.String,javax.swing.text.AttributeSet)).
>>
>> As in the case with `remove`, I marked up to classes and members with
>> `{@code}`.
>
> Alexey Ivanov has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Describe inserting ‘quick’ into text
> > > Does it make sense to add a sentence which describes the image? I added
> > > one for `remove` to explain all the `Position`s in the removed area are
> > > collapsed.
> >
> >
> > I missed this question. Yes, sure.
>
> I updated the PR with an example, look for “For example, if the document
> contains the text…”.
@prrace Did you have a chance to look at the updated text? The newly added
paragraph starts at [line
482](https://github.com/openjdk/jdk/pull/20376/files#diff-db4af1927b99dc7cdea095238dce3e6060258058feaaf587d3b6848562fc1453R482).
-
PR Comment: https://git.openjdk.org/jdk/pull/20376#issuecomment-2457811841
Re: RFR: 8283664: Remove jtreg tag manual=yesno for java/awt/print/PrinterJob/PrintTextTest.java [v3]
> There are multiple issue with this test case > 1) Parser error due to yesno in @run main/manual=yesno > 2) User can only compare the UI rendering and compare with the print out. > User can't mark the test as pass or fail due to pass or fail buttons are > missing. > 3) When the test is executed using jtreg after user click on the print button > on the print dialog the whole test UIs ( frames) gets dispose and user cannot > compare the printout with the UI. But this works as expected in test is > running individually using java PrintTextTest Daniel Gredler has updated the pull request incrementally with one additional commit since the last revision: Separate paint logic from test class - Changes: - all: https://git.openjdk.org/jdk/pull/21716/files - new: https://git.openjdk.org/jdk/pull/21716/files/204f78b6..c181680b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=21716&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=21716&range=01-02 Stats: 250 lines in 1 file changed: 20 ins; 21 del; 209 mod Patch: https://git.openjdk.org/jdk/pull/21716.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/21716/head:pull/21716 PR: https://git.openjdk.org/jdk/pull/21716
Re: RFR: 8338411: Implement JEP 486: Permanently Disable the Security Manager [v7]
> This is the implementation of JEP 486: Permanently Disable the Security > Manager. See [JEP 486](https://openjdk.org/jeps/486) for more details. The > [CSR](https://bugs.openjdk.org/browse/JDK-8338412) describes in detail the > main changes in the JEP and also includes an apidiff of the specification > changes. > > NOTE: the majority (~95%) of the changes in this PR are test updates > (removal/modifications) and API specification changes, the latter mostly to > remove `@throws SecurityException`. The remaining changes are primarily the > removal of the `SecurityManager`, `Policy`, `AccessController` and other > Security Manager API implementations. There is very little new code. > > The code changes can be broken down into roughly the following categories: > > 1. Degrading the behavior of Security Manager APIs to either throw Exceptions > by default or provide an execution environment that disallows access to all > resources by default. > 2. Changing hundreds of methods and constructors to no longer throw a > `SecurityException` if a Security Manager was enabled. They will operate as > they did in JDK 23 with no Security Manager enabled. > 3. Changing the `java` command to exit with a fatal error if a Security > Manager is enabled. > 4. Removing the hotspot native code for the privileged stack walk and the > inherited access control context. The remaining hotspot code and tests > related to the Security Manager will be removed immediately after integration > - see [JDK-8341916](https://bugs.openjdk.org/browse/JDK-8341916). > 5. Removing or modifying hundreds of tests. Many tests that tested Security > Manager behavior are no longer relevant and thus have been removed or > modified. > > There are a handful of Security Manager related tests that are failing and > are at the end of the `test/jdk/ProblemList.txt`, > `test/langtools/ProblemList.txt` and `test/hotspot/jtreg/ProblemList.txt` > files - these will be removed or separate bugs will be filed before > integrating this PR. > > Inside the JDK, we have retained calls to > `SecurityManager::getSecurityManager` and `AccessController::doPrivileged` > for now, as these methods have been degraded to behave the same as they did > in JDK 23 with no Security Manager enabled. After we integrate this JEP, > those calls will be removed in each area (client-libs, core-libs, security, > etc). > > I don't expect each reviewer to review all the code changes in this JEP. > Rather, I advise that you only focus on the changes for the area > (client-libs, core-libs, net, security, etc) that you are most f... Sean Mullan has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 213 commits: - Merge remote-tracking branch 'jdk-sandbox/jep486' into JDK-8338411 - Remove "access" and "policy" options from debug help. - Merge branch 'master' into jep486 - Merge branch 'jep486' of https://github.com/openjdk/jdk-sandbox into jep486 - remove MainClassCantBeLoadedTest from problemlisting - remove LauncherErrors test from problemlisting - Merge branch 'master' into jep486 - Remove left-over paragraph about SM use from LoggerFinder - Merge branch 'master' into jep486 - Merge branch 'master' into jep486 - ... and 203 more: https://git.openjdk.org/jdk/compare/f69b6016...51d2a2a3 - Changes: https://git.openjdk.org/jdk/pull/21498/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=21498&range=06 Stats: 68938 lines in 1889 files changed: 2490 ins; 62600 del; 3848 mod Patch: https://git.openjdk.org/jdk/pull/21498.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/21498/head:pull/21498 PR: https://git.openjdk.org/jdk/pull/21498
Re: RFR: 8283664: Remove jtreg tag manual=yesno for java/awt/print/PrinterJob/PrintTextTest.java [v3]
On Tue, 5 Nov 2024 17:41:07 GMT, Daniel Gredler wrote:
>> There are multiple issue with this test case
>> 1) Parser error due to yesno in @run main/manual=yesno
>> 2) User can only compare the UI rendering and compare with the print out.
>> User can't mark the test as pass or fail due to pass or fail buttons are
>> missing.
>> 3) When the test is executed using jtreg after user click on the print
>> button on the print dialog the whole test UIs ( frames) gets dispose and
>> user cannot compare the printout with the UI. But this works as expected in
>> test is running individually using java PrintTextTest
>
> Daniel Gredler has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Separate paint logic from test class
Changes requested by aivanov (Reviewer).
test/jdk/java/awt/print/PrinterJob/PrintTextTest.java line 84:
> 82:
> 83: public static void main(String[] args) throws Exception {
> 84:
I think blank lines at the start of methods are redundant. I don't insist on
removing them though.
test/jdk/java/awt/print/PrinterJob/PrintTextTest.java line 88:
> 86: PageFormat portrait = pjob.defaultPage();
> 87: portrait.setOrientation(PageFormat.PORTRAIT);
> 88: int preferredSize = (int) portrait.getImageableWidth();
The `preferredSize` could've remained static… to save some typing. Yet passing
it explicitly to objects is even better.
test/jdk/java/awt/print/PrinterJob/PrintTextTest.java line 101:
> 99: String name = "Page " + page++;
> 100: PrintText pt = new PrintText(name, font, null, false,
> preferredSize);
> 101: pane.add(name, pt);
Suggestion:
pane.addTab(name, pt);
I think
[`JTabbedPane.addTab`](https://docs.oracle.com/en/java/javase/21/docs/api/java.desktop/javax/swing/JTabbedPane.html#addTab(java.lang.String,java.awt.Component))
should be used to add components rather than inherited `add`.
test/jdk/java/awt/print/PrinterJob/PrintTextTest.java line 128:
> 126: book.append(pt, landscape);
> 127:
> 128: font = new Font("Dialog", Font.PLAIN, 18);
Suggestion:
font = new Font(Font.DIALOG, Font.PLAIN, 18);
test/jdk/java/awt/print/PrinterJob/PrintTextTest.java line 136:
> 134: book.append(pt, landscape);
> 135:
> 136: font = new Font("Dialog", Font.PLAIN, 18);
Suggestion:
font = new Font(Font.DIALOG, Font.PLAIN, 18);
test/jdk/java/awt/print/PrinterJob/PrintTextTest.java line 145:
> 143: book.append(pt, landscape);
> 144:
> 145: font = new Font("Dialog", Font.PLAIN, 18);
Suggestion:
font = new Font(Font.DIALOG, Font.PLAIN, 18);
test/jdk/java/awt/print/PrinterJob/PrintTextTest.java line 158:
> 156: pt = new PrintText(name, font, null, false, preferredSize);
> 157: pane.add(pt, BorderLayout.CENTER);
> 158: pane.add(name, pt);
Suggestion:
pane.addTab(name, pt);
test/jdk/java/awt/print/PrinterJob/PrintTextTest.java line 162:
> 160: book.append(pt, landscape);
> 161:
> 162: font = new Font("Monospaced", Font.PLAIN, 12);
Suggestion:
font = new Font(Font.MONOSPACED, Font.PLAIN, 12);
test/jdk/java/awt/print/PrinterJob/PrintTextTest.java line 166:
> 164: pt = new PrintText(name, font, null, false, preferredSize);
> 165: pane.add(pt, BorderLayout.CENTER);
> 166: pane.add(name, pt);
Suggestion:
pane.addTab(name, pt);
One shouldn't add the same component twice. (Yes, I understand it's existing
code, but it's worth fixing.)
test/jdk/java/awt/print/PrinterJob/PrintTextTest.java line 174:
> 172: pt = new PrintText(name, xfont, null, false, preferredSize);
> 173: pane.add(pt, BorderLayout.CENTER);
> 174: pane.add(name, pt);
Suggestion:
pane.addTab(name, pt);
And the following cases.
test/jdk/java/awt/print/PrinterJob/PrintTextTest.java line 214:
> 212: }
> 213: } catch (PrinterException e) {
> 214: throw new RuntimeException(e.getMessage());
Suggestion:
throw new RuntimeException(e.getMessage(), e);
The original exception could be very useful for debugging a failure.
test/jdk/java/awt/print/PrinterJob/PrintTextTest.java line 232:
> 230: }
> 231:
> 232: // The test needs a physical font that supports Latin.
The `getPhysicalFont` could be updated so that its `switch` statement uses
`Font.` constants, I'm pretty sure that was the intention.
test/jdk/java/awt/print/PrinterJob/PrintTextTest.java line 268:
> 266: protected String page;
> 267: protected boolean useFM;
> 268: protected int preferredSize;
All the fields could be `final`, the values aren't modified.
test/jdk/java/awt/print/PrinterJob/PrintTextTest.java line 279:
> 277: }
> 278:
> 279: public static AttributedCharacterIterator getIterator(String s) {
Suggestion:
private static Attributed
Re: RFR: 8338411: Implement JEP 486: Permanently Disable the Security Manager [v8]
On Tue, 5 Nov 2024 18:58:22 GMT, Sean Mullan wrote: >> This is the implementation of JEP 486: Permanently Disable the Security >> Manager. See [JEP 486](https://openjdk.org/jeps/486) for more details. The >> [CSR](https://bugs.openjdk.org/browse/JDK-8338412) describes in detail the >> main changes in the JEP and also includes an apidiff of the specification >> changes. >> >> NOTE: the majority (~95%) of the changes in this PR are test updates >> (removal/modifications) and API specification changes, the latter mostly to >> remove `@throws SecurityException`. The remaining changes are primarily the >> removal of the `SecurityManager`, `Policy`, `AccessController` and other >> Security Manager API implementations. There is very little new code. >> >> The code changes can be broken down into roughly the following categories: >> >> 1. Degrading the behavior of Security Manager APIs to either throw >> Exceptions by default or provide an execution environment that disallows >> access to all resources by default. >> 2. Changing hundreds of methods and constructors to no longer throw a >> `SecurityException` if a Security Manager was enabled. They will operate as >> they did in JDK 23 with no Security Manager enabled. >> 3. Changing the `java` command to exit with a fatal error if a Security >> Manager is enabled. >> 4. Removing the hotspot native code for the privileged stack walk and the >> inherited access control context. The remaining hotspot code and tests >> related to the Security Manager will be removed immediately after >> integration - see [JDK-8341916](https://bugs.openjdk.org/browse/JDK-8341916). >> 5. Removing or modifying hundreds of tests. Many tests that tested Security >> Manager behavior are no longer relevant and thus have been removed or >> modified. >> >> There are a handful of Security Manager related tests that are failing and >> are at the end of the `test/jdk/ProblemList.txt`, >> `test/langtools/ProblemList.txt` and `test/hotspot/jtreg/ProblemList.txt` >> files - these will be removed or separate bugs will be filed before >> integrating this PR. >> >> Inside the JDK, we have retained calls to >> `SecurityManager::getSecurityManager` and `AccessController::doPrivileged` >> for now, as these methods have been degraded to behave the same as they did >> in JDK 23 with no Security Manager enabled. After we integrate this JEP, >> those calls will be removed in each area (client-libs, core-libs, security, >> etc). >> >> I don't expect each reviewer to review all the code changes in this JEP. >> Rather, I advise that you only focus on the changes for the area >> (client-libs, core-libs, net, ... > > Sean Mullan has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 217 commits: > > - Merge remote-tracking branch 'jdk-sandbox/jep486' into JDK-8338411 > - Merge branch 'master' into jep486 > - Merge branch 'master' into jep486 > - Merge branch 'master' into jep486 > - Merge remote-tracking branch 'jdk-sandbox/jep486' into JDK-8338411 > - Remove "access" and "policy" options from debug help. > - Merge branch 'master' into jep486 > - Merge branch 'jep486' of https://github.com/openjdk/jdk-sandbox into jep486 > - remove MainClassCantBeLoadedTest from problemlisting > - remove LauncherErrors test from problemlisting > - ... and 207 more: https://git.openjdk.org/jdk/compare/3fab8e37...e9e7f0c9 The latest update includes: - merging with the mainline sources - a couple of test removals from the ProblemList for tests that no longer apply - removal of the "policy" and "access" options from the `-Djava.security.debug` help option. - a small update to remove some SM specific text from the `System.LoggerFinder` class description. - copyright header updates for several tests that were previously modified - addition of a few client manual tests to the ProblemList that have dependencies on the Security Manager and will be resolved later - PR Comment: https://git.openjdk.org/jdk/pull/21498#issuecomment-2458058335
Re: RFR: 8343601: javax/swing/JMenuItem/8158566/CloseOnMouseClickPropertyTest.java fails in ubuntu 22.04
On Tue, 5 Nov 2024 09:40:02 GMT, Prasanta Sadhukhan wrote: > It is seen that > javax/swing/JMenuItem/8158566/CloseOnMouseClickPropertyTest.java fails in > ubuntu 22.04 OCI instance. It is seen that test frame is being rendered at > top-left of screen instead of at the center, and robot mouseMove is being > used to point and click on radiobutton, checkbox etc and linux left dock > placement can be a problem with this top-left rendering. > > Fix is made to render the frame at the center. > Repeated iteration of the test run in 22.04 OCI instance is ok. "linux left dock placement can be a problem with this top-left rendering." are you saying that the dock is the actual problem here ? Why has it never been a problem on other 22.04 systems ? - PR Comment: https://git.openjdk.org/jdk/pull/21896#issuecomment-2458080665
Re: RFR: 8283664: Remove jtreg tag manual=yesno for java/awt/print/PrinterJob/PrintTextTest.java [v2]
On Tue, 5 Nov 2024 17:37:40 GMT, Daniel Gredler wrote: >> test/jdk/java/awt/print/PrinterJob/PrintTextTest.java line 1: >> >>> 1: /* >> >> Could you make the `PrintJAText` class a nested static class inside >> `PrintTextTest`? >> >> Yes, it requires more refactoring but this ensures no duplicate class names. >> >> Since `PrintJAText` extends `PrintTextTest`, I suggest moving all the >> painting logic into `PrintText` class which will be a nested static class. >> Then `PrintJAText` (renamed to `PrintJapaneseText` if I understand the >> intent correctly) will extend `PrintText`. >> >> All the other logic will remain unchanged. >> >> Separating the painting logic from the test class itself, I believe, would >> also make thing clearer. >> >> >> In addition to the above, let's resolve IDE warnings at lines 349 and 411 >> (in the current revision): >> >> >> -byte data[] = new byte[s.length()]; >> +byte[] data = new byte[s.length()]; >> >> -TextLayout tl = new TextLayout(s, new HashMap(), frc); >> +TextLayout tl = new TextLayout(s, new HashMap<>(), frc); >> >> >> Overall, the changes look good to me. Thank you for your contribution. > > Thanks! Let me know if this last commit is what you had in mind. It might be > easier to review with whitespace ignored, since the class additions required > some indentation changes. Yes, that's what I had in mind. Thank you. - PR Review Comment: https://git.openjdk.org/jdk/pull/21716#discussion_r1829825695
Re: RFR: 8338411: Implement JEP 486: Permanently Disable the Security Manager [v6]
On Sun, 3 Nov 2024 12:33:05 GMT, Alan Bateman wrote: >> Right - this paragraph - lines 1620-1625 (old file) / 1362-1367 (new file) >> is no longer relevant and should be removed too. Thanks for spotting that. > > Removed in jep486 branch in sandbox so will get picked up when PR is > refreshed. Fixed in https://github.com/openjdk/jdk/pull/21498/commits/ab586f6619ca5f7e213876219abf61a36155735c - PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1829886748
Re: RFR: 8343118: [TESTBUG] java/awt/PrintJob/PrintCheckboxTest/PrintCheckboxManualTest.java fails with rror. Can't find HTML file PrintCheckboxManualTest.html [v2]
On Mon, 4 Nov 2024 06:41:05 GMT, Prasanta Sadhukhan wrote: >> Test was pointing to missing html file, probably due to old Applet usage. >> Test is updated to use PFJ.. > > Prasanta Sadhukhan has updated the pull request incrementally with one > additional commit since the last revision: > > Restrict to linux I think maintaining the restriction to linux for the test is a good choice. This way you can leave the test summary as is. Although, we're excluding Windows from the test even though the test seems to work for it, I think this is a fine solution, at least for now. - Marked as reviewed by dnguyen (Committer). PR Review: https://git.openjdk.org/jdk/pull/21786#pullrequestreview-2416493066
Re: RFR: 8283664: Remove jtreg tag manual=yesno for java/awt/print/PrinterJob/PrintTextTest.java [v4]
> There are multiple issue with this test case > 1) Parser error due to yesno in @run main/manual=yesno > 2) User can only compare the UI rendering and compare with the print out. > User can't mark the test as pass or fail due to pass or fail buttons are > missing. > 3) When the test is executed using jtreg after user click on the print button > on the print dialog the whole test UIs ( frames) gets dispose and user cannot > compare the printout with the UI. But this works as expected in test is > running individually using java PrintTextTest Daniel Gredler has updated the pull request incrementally with one additional commit since the last revision: Use Font constants, addTab(), @Override, final and private modifiers - Changes: - all: https://git.openjdk.org/jdk/pull/21716/files - new: https://git.openjdk.org/jdk/pull/21716/files/c181680b..43fda0e0 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=21716&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=21716&range=02-03 Stats: 38 lines in 1 file changed: 5 ins; 6 del; 27 mod Patch: https://git.openjdk.org/jdk/pull/21716.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/21716/head:pull/21716 PR: https://git.openjdk.org/jdk/pull/21716
Re: RFR: 8283664: Remove jtreg tag manual=yesno for java/awt/print/PrinterJob/PrintTextTest.java [v4]
On Tue, 5 Nov 2024 17:48:08 GMT, Daniel Gredler wrote: > > The test may need to be problem-listed against > > [8148334](https://bugs.openjdk.org/browse/JDK-8148334) as it's known to > > fail. > > Should that be part of this PR? If so, do I just add this line to > `test/jdk/ProblemList.txt`? > > `java/awt/print/PrinterJob/PrintTextTest.java 8148334 generic-all` I think it should. We know the test fails because the printed version differs from the displayed one, there's a bug to track it. Thus, the test should be in the problem list. Perhaps, the only reason it isn't PL'ed is because the test couldn't be run. The PL entry line looks good to me. - PR Comment: https://git.openjdk.org/jdk/pull/21716#issuecomment-2458157880
Re: RFR: 8283664: Remove jtreg tag manual=yesno for java/awt/print/PrinterJob/PrintTextTest.java [v3]
On Tue, 5 Nov 2024 21:07:57 GMT, Alexey Ivanov wrote: >> It looks like it's trying to be extra resilient by switching on >> `n.toLowerCase()` (i.e. case-insensitive). Is it OK to make it >> case-sensitive and use the constants? > > I thought about it too. Using constants and case-sensitive switch should be > enough. > > Also, I think the `switch` statement should have `continue` rather than > `break` for these logical fonts… unless the logical fonts are guaranteed to > be in the end of the array returned by > `GraphicsEnvironment.getAvailableFontFamilyNames()`. Done! - PR Review Comment: https://git.openjdk.org/jdk/pull/21716#discussion_r1830196351
Re: RFR: 8283664: Remove jtreg tag manual=yesno for java/awt/print/PrinterJob/PrintTextTest.java [v4]
On Tue, 5 Nov 2024 21:12:29 GMT, Alexey Ivanov wrote: > The only things left are problem-listing and updating the `switch` statement. Thanks again for the review, these last points should now be resolved. - PR Comment: https://git.openjdk.org/jdk/pull/21716#issuecomment-2458387963
Re: RFR: 8283664: Remove jtreg tag manual=yesno for java/awt/print/PrinterJob/PrintTextTest.java [v5]
> There are multiple issue with this test case > 1) Parser error due to yesno in @run main/manual=yesno > 2) User can only compare the UI rendering and compare with the print out. > User can't mark the test as pass or fail due to pass or fail buttons are > missing. > 3) When the test is executed using jtreg after user click on the print button > on the print dialog the whole test UIs ( frames) gets dispose and user cannot > compare the printout with the UI. But this works as expected in test is > running individually using java PrintTextTest Daniel Gredler has updated the pull request incrementally with one additional commit since the last revision: Use constants in switch, update problem list - Changes: - all: https://git.openjdk.org/jdk/pull/21716/files - new: https://git.openjdk.org/jdk/pull/21716/files/43fda0e0..1e8f91f9 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=21716&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=21716&range=03-04 Stats: 8 lines in 2 files changed: 1 ins; 0 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/21716.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/21716/head:pull/21716 PR: https://git.openjdk.org/jdk/pull/21716
Re: RFR: 8283664: Remove jtreg tag manual=yesno for java/awt/print/PrinterJob/PrintTextTest.java [v3]
On Tue, 5 Nov 2024 18:52:30 GMT, Daniel Gredler wrote: >> test/jdk/java/awt/print/PrinterJob/PrintTextTest.java line 232: >> >>> 230: } >>> 231: >>> 232: // The test needs a physical font that supports Latin. >> >> The `getPhysicalFont` could be updated so that its `switch` statement uses >> `Font.` constants, I'm pretty sure that was the intention. > > It looks like it's trying to be extra resilient by switching on > `n.toLowerCase()` (i.e. case-insensitive). Is it OK to make it case-sensitive > and use the constants? I thought about it too. Using constants and case-sensitive switch should be enough. Also, I think the `switch` statement should have `continue` rather than `break` for these logical fonts… unless the logical fonts are guaranteed to be in the end of the array returned by `GraphicsEnvironment.getAvailableFontFamilyNames()`. - PR Review Comment: https://git.openjdk.org/jdk/pull/21716#discussion_r1830004402
Re: RFR: 8283664: Remove jtreg tag manual=yesno for java/awt/print/PrinterJob/PrintTextTest.java [v4]
On Tue, 5 Nov 2024 19:13:44 GMT, Daniel Gredler wrote: >> There are multiple issue with this test case >> 1) Parser error due to yesno in @run main/manual=yesno >> 2) User can only compare the UI rendering and compare with the print out. >> User can't mark the test as pass or fail due to pass or fail buttons are >> missing. >> 3) When the test is executed using jtreg after user click on the print >> button on the print dialog the whole test UIs ( frames) gets dispose and >> user cannot compare the printout with the UI. But this works as expected in >> test is running individually using java PrintTextTest > > Daniel Gredler has updated the pull request incrementally with one additional > commit since the last revision: > > Use Font constants, addTab(), @Override, final and private modifiers Looks good to me. The only things left are problem-listing and updating the `switch` statement. - Changes requested by aivanov (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/21716#pullrequestreview-2416704791
Re: RFR: 6672644: JComboBox still scrolling if switch to another window and return back
On Wed, 4 Sep 2024 07:52:08 GMT, Abhishek Kumar wrote: >> In a JComboBox, if the user opens the dropdown list and clicks and holds the >> down-button, then ALT-TABs to switch focus, when the user re-focuses the >> frame with the JComboBox and opens the dropdown list again, the list will >> still be scrolling even though the down-button isn't pressed. >> >> This isn't OS or L&F specific, although Aqua L&F does not have any >> directional arrows in the dropdown list (and is thus exempt). This led me to >> believe it could be handled in BasicComboBoxUI where focusLost and focusGain >> are used or isPopupVisible but the scroll behavior cannot be altered here. >> Likewise for BasicComboPopup where `autoscroll` is used. However, this >> behavior isn't related to autoscroll and is actually found in the JScrollbar >> of the JScrollpane inside of the JComboBox. The timer for the scroll action >> starts but is never stopped if focus is lost, so a new listener is created >> and used. The proposed solution uses `KeyboardFocusManager` to track the >> focus owner. The listener stops the `scrollTimer` when the `focusOwner` >> property is changed. With this change, the list no longer automatically >> scrolls when re-focused and instead opens normally. >> >> The included test is manual due to the need to confirm that the list still >> scrolls after ALT-TABing. The L&F is set to Metal since it is the >> cross-platform lookandfeel and has directional buttons for the JScrollPane >> list. > > I can verify on linux machine that it is suto-scrolling but it is > intermittent on windows as well as on linux. Sometimes the list is scrolling > and sometimes it's not. Are you facing the same? > To regain focus, if click on frame title bar then the intermittent issue can > be observed. @kumarabhi006 Tested with the additional changes I made. Looks like it's working now. No other tests are failing in CI with the changes either. Can you check it out again? - PR Comment: https://git.openjdk.org/jdk/pull/20845#issuecomment-2458263614
Re: RFR: 6672644: JComboBox still scrolling if switch to another window and return back [v3]
> In a JComboBox, if the user opens the dropdown list and clicks and holds the > down-button, then ALT-TABs to switch focus, when the user re-focuses the > frame with the JComboBox and opens the dropdown list again, the list will > still be scrolling even though the down-button isn't pressed. > > This isn't OS or L&F specific, although Aqua L&F does not have any > directional arrows in the dropdown list (and is thus exempt). This led me to > believe it could be handled in BasicComboBoxUI where focusLost and focusGain > are used or isPopupVisible but the scroll behavior cannot be altered here. > Likewise for BasicComboPopup where `autoscroll` is used. However, this > behavior isn't related to autoscroll and is actually found in the JScrollbar > of the JScrollpane inside of the JComboBox. The timer for the scroll action > starts but is never stopped if focus is lost, so a new listener is created > and used. The proposed solution uses `KeyboardFocusManager` to track the > focus owner. The listener stops the `scrollTimer` when the `focusOwner` > property is changed. With this change, the list no longer automatically > scrolls when re-focused and instead opens normally. > > The included test is manual due to the need to confirm that the list still > scrolls after ALT-TABing. The L&F is set to Metal since it is the > cross-platform lookandfeel and has directional buttons for the JScrollPane > list. Damon Nguyen has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision: - Update fix to resolve button pressed appearance. Handle review comments - Merge branch 'master' into 6672644/focusScroll - Initial commit - Review comments - Initial commit - Changes: - all: https://git.openjdk.org/jdk/pull/20845/files - new: https://git.openjdk.org/jdk/pull/20845/files/312c484d..2dbba1ae Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20845&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20845&range=01-02 Stats: 269780 lines in 2960 files changed: 230777 ins; 22759 del; 16244 mod Patch: https://git.openjdk.org/jdk/pull/20845.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20845/head:pull/20845 PR: https://git.openjdk.org/jdk/pull/20845
Re: RFR: 8339783: Implement JEP 479: Remove the Windows 32-bit x86 Port [v30]
On Mon, 4 Nov 2024 20:42:59 GMT, Magnus Ihse Bursie wrote:
>> This is the implementation of [JEP 479: _Remove the Windows 32-bit x86
>> Port_](https://openjdk.org/jeps/479).
>>
>> This is the summary of JEP 479:
>>> Remove the source code and build support for the Windows 32-bit x86 port.
>>> This port was [deprecated for removal in JDK
>>> 21](https://openjdk.org/jeps/449) with the express intent to remove it in a
>>> future release.
>
> Magnus Ihse Bursie has updated the pull request incrementally with one
> additional commit since the last revision:
>
> fix: jvm_md.h was included, but not jvm.h...
I think you may be throwing the baby out with the bath water when it comes to
`__stdcall`. It may be that 32-bit requires `__stdcall` but I don't see
anything that states `__stdcall` is ONLY for 32-bit!
src/hotspot/os/windows/os_windows.cpp line 510:
> 508: // Thread start routine for all newly created threads.
> 509: // Called with the associated Thread* as the argument.
> 510: static unsigned thread_native_entry(void* t) {
Whoa! Hold on there. The `_stdcall` is required here and nothing to do with
32-bit. We use `begindthreadex` to start threads and the entry function is
required to be `_stdcall`.
https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/beginthread-beginthreadex?view=msvc-170
-
Changes requested by dholmes (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/21744#pullrequestreview-2417056020
PR Review Comment: https://git.openjdk.org/jdk/pull/21744#discussion_r1830259353
Re: RFR: 8339783: Implement JEP 479: Remove the Windows 32-bit x86 Port [v30]
On Wed, 6 Nov 2024 00:58:10 GMT, David Holmes wrote: > I think you may be throwing the baby out with the bath water when it comes to > `__stdcall`. It may be that 32-bit requires `__stdcall` but I don't see > anything that states `__stdcall` is ONLY for 32-bit! https://learn.microsoft.com/en-us/cpp/cpp/stdcall?view=msvc-170 `On ARM and x64 processors, __stdcall is accepted and ignored by the compiler; on ARM and x64 architectures, by convention, arguments are passed in registers when possible, and subsequent arguments are passed on the stack.` - PR Comment: https://git.openjdk.org/jdk/pull/21744#issuecomment-2458534929
Re: RFR: 8343418: Unnecessary Hashtable usage in CSS.htmlAttrToCssAttrMap [v3]
> If a thread-safe implementation is not needed, it is recommended to use
> HashMap instead of legacy synchronized Hashtable.
> Map `CSS.htmlAttrToCssAttrMap` is modified only within static initialization
> block and then only `get` method is called.
>
> The only subtle difference is that Hashtable doesn't allow `null` keys and
> throws NPE from `get` method.
> I've checked all possible keys which are passed to `htmlAttrToCssAttrMap.get`.
> Currently 3 different execution paths are possible:
>
> **1,2**
> When `HTML.Attribute.BORDER` is passed as a key. It's definitely non-null.
>
> javax.swing.text.html.CSS#translateHTMLToCSS
> translateAttribute(HTML.Attribute.BORDER, "1", cssAttrSet);
> CSS.Attribute[] cssAttrList = getCssAttribute(key);
>
>
> javax.swing.text.html.CSS#translateAttributes
>translateAttribute(HTML.Attribute.BORDER, Integer.toString(borderWidth),
> cssAttrSet);
> CSS.Attribute[] cssAttrList = getCssAttribute(key);`
>
>
> **3**
> When non-null `key` is passed as a key.
>
> javax.swing.text.html.CSS#translateAttributes
>
> if (name instanceof HTML.Attribute) { // from this `instanceof` we know
> that it's non-null
> HTML.Attribute key = (HTML.Attribute)name;
>
> translateAttribute(key, (String) htmlAttrSet.getAttribute(key),
> cssAttrSet);
> CSS.Attribute[] cssAttrList = getCssAttribute(key);
>
>
> 
>
>
> I've used HashMap.newHashMap method instead of constructor to avoid resizing
> of internal HashMap array.
Andrey Turbanov has updated the pull request incrementally with one additional
commit since the last revision:
Use immutable map instead of HashMap.
It fits even better: has the same null handling as Hashtable
-
Changes:
- all: https://git.openjdk.org/jdk/pull/21785/files
- new: https://git.openjdk.org/jdk/pull/21785/files/b653278f..a0860021
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=21785&range=02
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=21785&range=01-02
Stats: 54 lines in 1 file changed: 5 ins; 1 del; 48 mod
Patch: https://git.openjdk.org/jdk/pull/21785.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/21785/head:pull/21785
PR: https://git.openjdk.org/jdk/pull/21785
Re: RFR: 8339783: Implement JEP 479: Remove the Windows 32-bit x86 Port [v30]
On Wed, 6 Nov 2024 01:44:48 GMT, Alex Menkov wrote: > I think you may be throwing the baby out with the bath water when it comes to > `__stdcall`. It may be that 32-bit requires `__stdcall` but I don't see > anything that states `__stdcall` is ONLY for 32-bit! To my knowledge the only thing __cdecl and __stdcall do is affect the argument passing on the stack since 32 bit uses the stack to pass arguments. Since 64 bit passes arguments inside registers and then only later uses the stack if there are too many parameters to fit in the parameter registers (Basically permanent __fastcall), these specifiers are probably ignored in all 64 bit platforms - PR Comment: https://git.openjdk.org/jdk/pull/21744#issuecomment-2458712195
Re: RFR: 8339783: Implement JEP 479: Remove the Windows 32-bit x86 Port [v30]
On Wed, 6 Nov 2024 01:44:48 GMT, Alex Menkov wrote: > On ARM and x64 processors, __stdcall is accepted and ignored by the compiler; @alexmenkov and @TheShermanTanker , I stand corrected and my apologies to @magicus . - PR Comment: https://git.openjdk.org/jdk/pull/21744#issuecomment-2458778303
Re: RFR: 6672644: JComboBox still scrolling if switch to another window and return back [v3]
On Tue, 5 Nov 2024 22:29:08 GMT, Damon Nguyen wrote:
>> In a JComboBox, if the user opens the dropdown list and clicks and holds the
>> down-button, then ALT-TABs to switch focus, when the user re-focuses the
>> frame with the JComboBox and opens the dropdown list again, the list will
>> still be scrolling even though the down-button isn't pressed.
>>
>> This isn't OS or L&F specific, although Aqua L&F does not have any
>> directional arrows in the dropdown list (and is thus exempt). This led me to
>> believe it could be handled in BasicComboBoxUI where focusLost and focusGain
>> are used or isPopupVisible but the scroll behavior cannot be altered here.
>> Likewise for BasicComboPopup where `autoscroll` is used. However, this
>> behavior isn't related to autoscroll and is actually found in the JScrollbar
>> of the JScrollpane inside of the JComboBox. The timer for the scroll action
>> starts but is never stopped if focus is lost, so a new listener is created
>> and used. The proposed solution uses `KeyboardFocusManager` to track the
>> focus owner. The listener stops the `scrollTimer` when the `focusOwner`
>> property is changed. With this change, the list no longer automatically
>> scrolls when re-focused and instead opens normally.
>>
>> The included test is manual due to the need to confirm that the list still
>> scrolls after ALT-TABing. The L&F is set to Metal since it is the
>> cross-platform lookandfeel and has directional buttons for the JScrollPane
>> list.
>
> Damon Nguyen has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains five additional
> commits since the last revision:
>
> - Update fix to resolve button pressed appearance. Handle review comments
> - Merge branch 'master' into 6672644/focusScroll
> - Initial commit
> - Review comments
> - Initial commit
src/java.desktop/share/classes/javax/swing/plaf/basic/BasicScrollBarUI.java
line 524:
> 522: * @return a keyboard focus listener
> 523: */
> 524: protected KeyboardFocusListener createKeyboardFocusListener(){
Suggestion:
protected KeyboardFocusListener createKeyboardFocusListener() {
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20845#discussion_r1830429465
