Re: RFR: 8272808: Update constant collections to use the new immutable collections - leftovers [v3]
On Thu, 4 Nov 2021 17:15:45 GMT, Nir Lisker wrote: >> Replacements of more immutable collections. >> >> One thing I found was that the field `idMap` in >> `com.sun.java.scene.web.WebViewHelper.WebView` seems unused. I can remove it >> if that's indeed the case. > > Nir Lisker has updated the pull request incrementally with one additional > commit since the last revision: > > Removed unused field Marked as reviewed by jvos (Reviewer). - PR: https://git.openjdk.java.net/jfx/pull/627
Re: RFR: 8272808: Update constant collections to use the new immutable collections - leftovers [v3]
On Thu, 4 Nov 2021 17:15:45 GMT, Nir Lisker wrote: >> Replacements of more immutable collections. >> >> One thing I found was that the field `idMap` in >> `com.sun.java.scene.web.WebViewHelper.WebView` seems unused. I can remove it >> if that's indeed the case. > > Nir Lisker has updated the pull request incrementally with one additional > commit since the last revision: > > Removed unused field Marked as reviewed by kcr (Lead). - PR: https://git.openjdk.java.net/jfx/pull/627
Re: RFR: 8272808: Update constant collections to use the new immutable collections - leftovers [v2]
On Fri, 15 Oct 2021 14:24:15 GMT, Nir Lisker wrote: >> Replacements of more immutable collections. >> >> One thing I found was that the field `idMap` in >> `com.sun.java.scene.web.WebViewHelper.WebView` seems unused. I can remove it >> if that's indeed the case. > > Nir Lisker has updated the pull request incrementally with one additional > commit since the last revision: > > Removed unused imports I removed the field. Needs a quick re-review. - PR: https://git.openjdk.java.net/jfx/pull/627
Re: RFR: 8272808: Update constant collections to use the new immutable collections - leftovers [v3]
> Replacements of more immutable collections. > > One thing I found was that the field `idMap` in > `com.sun.java.scene.web.WebViewHelper.WebView` seems unused. I can remove it > if that's indeed the case. Nir Lisker has updated the pull request incrementally with one additional commit since the last revision: Removed unused field - Changes: - all: https://git.openjdk.java.net/jfx/pull/627/files - new: https://git.openjdk.java.net/jfx/pull/627/files/79606579..fc72e5dd Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=627=02 - incr: https://webrevs.openjdk.java.net/?repo=jfx=627=01-02 Stats: 2 lines in 1 file changed: 0 ins; 2 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/627.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/627/head:pull/627 PR: https://git.openjdk.java.net/jfx/pull/627
Re: RFR: 8272808: Update constant collections to use the new immutable collections - leftovers [v2]
On Fri, 15 Oct 2021 14:24:15 GMT, Nir Lisker wrote: >> Replacements of more immutable collections. >> >> One thing I found was that the field `idMap` in >> `com.sun.java.scene.web.WebViewHelper.WebView` seems unused. I can remove it >> if that's indeed the case. > > Nir Lisker has updated the pull request incrementally with one additional > commit since the last revision: > > Removed unused imports If you want to remove the idMap field in the android/.../WebView.java, that's fine. If not, that's fine too. All other changes look reasonable to me. - Marked as reviewed by jvos (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/627
Re: RFR: 8272808: Update constant collections to use the new immutable collections - leftovers [v2]
On Wed, 27 Oct 2021 15:52:38 GMT, Nir Lisker wrote: > @johanvos Can you comment on `idMap` in > `com.sun.java.scene.web.WebViewHelper.WebView`? That is unused and can safely be deleted. - PR: https://git.openjdk.java.net/jfx/pull/627
Re: RFR: 8272808: Update constant collections to use the new immutable collections - leftovers [v2]
On Fri, 15 Oct 2021 14:24:15 GMT, Nir Lisker wrote: >> Replacements of more immutable collections. >> >> One thing I found was that the field `idMap` in >> `com.sun.java.scene.web.WebViewHelper.WebView` seems unused. I can remove it >> if that's indeed the case. > > Nir Lisker has updated the pull request incrementally with one additional > commit since the last revision: > > Removed unused imports @johanvos Can you comment on `idMap` in `com.sun.java.scene.web.WebViewHelper.WebView`? - PR: https://git.openjdk.java.net/jfx/pull/627
Re: RFR: 8272808: Update constant collections to use the new immutable collections - leftovers [v2]
On Fri, 15 Oct 2021 14:24:15 GMT, Nir Lisker wrote: >> Replacements of more immutable collections. >> >> One thing I found was that the field `idMap` in >> `com.sun.java.scene.web.WebViewHelper.WebView` seems unused. I can remove it >> if that's indeed the case. > > Nir Lisker has updated the pull request incrementally with one additional > commit since the last revision: > > Removed unused imports Looks good to me. - Marked as reviewed by arapte (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/627
Re: RFR: 8272808: Update constant collections to use the new immutable collections - leftovers [v2]
On Fri, 15 Oct 2021 14:24:15 GMT, Nir Lisker wrote: >> Replacements of more immutable collections. >> >> One thing I found was that the field `idMap` in >> `com.sun.java.scene.web.WebViewHelper.WebView` seems unused. I can remove it >> if that's indeed the case. > > Nir Lisker has updated the pull request incrementally with one additional > commit since the last revision: > > Removed unused imports Marked as reviewed by kcr (Lead). - PR: https://git.openjdk.java.net/jfx/pull/627
Re: RFR: 8272808: Update constant collections to use the new immutable collections - leftovers [v2]
On Fri, 15 Oct 2021 16:23:10 GMT, Kevin Rushforth wrote: > > Do you want to do something about `idMap` in > > `com.sun.java.scene.web.WebViewHelper.WebView` that I pointed out in the PR? > > I missed that comment. Where do you see this field? I can't find it anywhere. Oh, I see it now. It's in the android sources, which aren't part of the regular build. @johanvos will need to comment. - PR: https://git.openjdk.java.net/jfx/pull/627
Re: RFR: 8272808: Update constant collections to use the new immutable collections - leftovers [v2]
On Fri, 15 Oct 2021 15:04:00 GMT, Nir Lisker wrote: > Do you want to do something about `idMap` in > `com.sun.java.scene.web.WebViewHelper.WebView` that I pointed out in the PR? I missed that comment. Where do you see this field? I can't find it anywhere. - PR: https://git.openjdk.java.net/jfx/pull/627
Re: RFR: 8272808: Update constant collections to use the new immutable collections - leftovers [v2]
On Fri, 15 Oct 2021 14:24:15 GMT, Nir Lisker wrote: >> Replacements of more immutable collections. >> >> One thing I found was that the field `idMap` in >> `com.sun.java.scene.web.WebViewHelper.WebView` seems unused. I can remove it >> if that's indeed the case. > > Nir Lisker has updated the pull request incrementally with one additional > commit since the last revision: > > Removed unused imports Do you want to do something about `idMap` in `com.sun.java.scene.web.WebViewHelper.WebView` that I pointed out in the PR? - PR: https://git.openjdk.java.net/jfx/pull/627
Re: RFR: 8272808: Update constant collections to use the new immutable collections - leftovers [v2]
> Replacements of more immutable collections. > > One thing I found was that the field `idMap` in > `com.sun.java.scene.web.WebViewHelper.WebView` seems unused. I can remove it > if that's indeed the case. Nir Lisker has updated the pull request incrementally with one additional commit since the last revision: Removed unused imports - Changes: - all: https://git.openjdk.java.net/jfx/pull/627/files - new: https://git.openjdk.java.net/jfx/pull/627/files/255a314d..79606579 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=627=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx=627=00-01 Stats: 4 lines in 1 file changed: 0 ins; 4 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/627.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/627/head:pull/627 PR: https://git.openjdk.java.net/jfx/pull/627
Re: RFR: 8272808: Update constant collections to use the new immutable collections - leftovers
On Thu, 14 Oct 2021 23:50:10 GMT, Kevin Rushforth wrote: >> Replacements of more immutable collections. >> >> One thing I found was that the field `idMap` in >> `com.sun.java.scene.web.WebViewHelper.WebView` seems unused. I can remove it >> if that's indeed the case. > > modules/javafx.web/src/main/java/com/sun/webkit/Utilities.java line 91: > >> 89: >> 90: // List of packages to reject >> 91: private static final List PACKAGES_REJECT_LIST = List.of( > > After this change, I think the import of `java.util.Arrays` is unused. Even more than that one :) - PR: https://git.openjdk.java.net/jfx/pull/627
Re: RFR: 8272808: Update constant collections to use the new immutable collections - leftovers
On Tue, 21 Sep 2021 11:13:06 GMT, Nir Lisker wrote: > Replacements of more immutable collections. > > One thing I found was that the field `idMap` in > `com.sun.java.scene.web.WebViewHelper.WebView` seems unused. I can remove it > if that's indeed the case. Marked as reviewed by kcr (Lead). modules/javafx.web/src/main/java/com/sun/webkit/Utilities.java line 91: > 89: > 90: // List of packages to reject > 91: private static final List PACKAGES_REJECT_LIST = List.of( After this change, I think the import of `java.util.Arrays` is unused. - PR: https://git.openjdk.java.net/jfx/pull/627
Re: RFR: 8272808: Update constant collections to use the new immutable collections - leftovers
On Thu, 23 Sep 2021 18:03:37 GMT, Marius Hanl wrote: >> Replacements of more immutable collections. >> >> One thing I found was that the field `idMap` in >> `com.sun.java.scene.web.WebViewHelper.WebView` seems unused. I can remove it >> if that's indeed the case. > > modules/javafx.graphics/src/main/java/com/sun/javafx/font/WindowsFontMap.java > line 44: > >> 42: } >> 43: >> 44: private static Map PLATFORM_FONT_MAP; > > Shouldn't this variable name stay with the normal camel case because it is > not final (only static)? Yes, but it's created only once via lazy initialization and then it's effectively final. A matter of taste I would say. There is also a JEP that deals with these cases https://openjdk.java.net/jeps/8209964. - PR: https://git.openjdk.java.net/jfx/pull/627
Re: RFR: 8272808: Update constant collections to use the new immutable collections - leftovers
On Thu, 23 Sep 2021 21:55:57 GMT, delvh wrote: >> We still build JavaFX with `--release 11` so applications using JavaFX can >> run on JDK 11 or later. At some point we will bump the minimum version of >> Java that is required, but until then we cannot use records or any other >> feature that isn't in Java 11. > > That is indeed a valid argument as to why this isn't a record. > Obviously, I did not think about that while reviewing. @delvh If you are interested, I already have a branch with records replacements for when we are able to use them https://github.com/nlisker/jfx/tree/Records. It hasn't been updated in a while so it doesn't include everything on my list. - PR: https://git.openjdk.java.net/jfx/pull/627
Re: RFR: 8272808: Update constant collections to use the new immutable collections - leftovers
On Thu, 23 Sep 2021 12:43:20 GMT, Kevin Rushforth wrote:
>> modules/javafx.graphics/src/main/java/com/sun/javafx/font/WindowsFontMap.java
>> line 32:
>>
>>> 30: class WindowsFontMap {
>>> 31:
>>> 32: private static class FamilyDescription {
>>
>> Isn't that basically a `record` in disguise?
>> Letting this class be a record would remove a lot of lines, especially down
>> below.
>> Also, it would come with the benefit of not having to worry about whether
>> these values have been changed or not as they would then always be final.
>> To me at least it does not look as if variables of this class should be
>> mutable after having been created.
>
> We still build JavaFX with `--release 11` so applications using JavaFX can
> run on JDK 11 or later. At some point we will bump the minimum version of
> Java that is required, but until then we cannot use records or any other
> feature that isn't in Java 11.
That is indeed a valid argument as to why this isn't a record.
Obviously, I did not think about that while reviewing.
-
PR: https://git.openjdk.java.net/jfx/pull/627
Re: RFR: 8272808: Update constant collections to use the new immutable collections - leftovers
On Tue, 21 Sep 2021 11:13:06 GMT, Nir Lisker wrote: > Replacements of more immutable collections. > > One thing I found was that the field `idMap` in > `com.sun.java.scene.web.WebViewHelper.WebView` seems unused. I can remove it > if that's indeed the case. modules/javafx.graphics/src/main/java/com/sun/javafx/font/WindowsFontMap.java line 44: > 42: } > 43: > 44: private static Map PLATFORM_FONT_MAP; Shouldn't this variable name stay with the normal camel case because it is not final (only static)? - PR: https://git.openjdk.java.net/jfx/pull/627
Re: RFR: 8272808: Update constant collections to use the new immutable collections - leftovers
On Tue, 21 Sep 2021 11:13:06 GMT, Nir Lisker wrote: > Replacements of more immutable collections. > > One thing I found was that the field `idMap` in > `com.sun.java.scene.web.WebViewHelper.WebView` seems unused. I can remove it > if that's indeed the case. This is a pretty simple change, but a second pair of eyes would be helpful. - PR: https://git.openjdk.java.net/jfx/pull/627
Re: RFR: 8272808: Update constant collections to use the new immutable collections - leftovers
On Wed, 22 Sep 2021 21:36:26 GMT, delvh
wrote:
>> Replacements of more immutable collections.
>>
>> One thing I found was that the field `idMap` in
>> `com.sun.java.scene.web.WebViewHelper.WebView` seems unused. I can remove it
>> if that's indeed the case.
>
> modules/javafx.graphics/src/main/java/com/sun/javafx/font/WindowsFontMap.java
> line 32:
>
>> 30: class WindowsFontMap {
>> 31:
>> 32: private static class FamilyDescription {
>
> Isn't that basically a `record` in disguise?
> Letting this class be a record would remove a lot of lines, especially down
> below.
> Also, it would come with the benefit of not having to worry about whether
> these values have been changed or not as they would then always be final.
> To me at least it does not look as if variables of this class should be
> mutable after having been created.
We still build JavaFX with `--release 11` so applications using JavaFX can run
on JDK 11 or later. At some point we will bump the minimum version of Java that
is required, but until then we cannot use records or any other feature that
isn't in Java 11.
-
PR: https://git.openjdk.java.net/jfx/pull/627
Re: RFR: 8272808: Update constant collections to use the new immutable collections - leftovers
On Tue, 21 Sep 2021 11:13:06 GMT, Nir Lisker wrote:
> Replacements of more immutable collections.
>
> One thing I found was that the field `idMap` in
> `com.sun.java.scene.web.WebViewHelper.WebView` seems unused. I can remove it
> if that's indeed the case.
modules/javafx.graphics/src/main/java/com/sun/javafx/font/WindowsFontMap.java
line 32:
> 30: class WindowsFontMap {
> 31:
> 32: private static class FamilyDescription {
Isn't that basically a `record` in disguise?
Letting this class be a record would remove a lot of lines, especially down
below.
Also, it would come with the benefit of not having to worry about whether these
values have been changed or not as they would then always be final.
To me at least it does not look as if variables of this class should be mutable
after having been created.
-
PR: https://git.openjdk.java.net/jfx/pull/627
