Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8273946: Move clearQuad method to BaseShaderGraphics superclass #628

Closed

Conversation

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Sep 21, 2021

As mentioned in JBS, the clearQuad methods in D3DGraphics and ES2Graphics are now identical, which should have been the case all along. The fact that they weren't identical was the source of a bug that was only discovered during the testing of JDK-8090547 on the es2 pipeline.

This PR moves the clearQuad method to the BaseShaderGraphics superclass to get rid of the unnecessary code duplication. This will be helpful when implementing the metal pipeline as well. As a note, I made the context field in the D3DGraphics final, which it should have been. This is necessary to ensure that moving the method to the super-class is equivalent.

No new tests are needed, since this is just refactoring.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8273946: Move clearQuad method to BaseShaderGraphics superclass

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jfx pull/628/head:pull/628
$ git checkout pull/628

Update a local copy of the PR:
$ git checkout pull/628
$ git pull https://git.openjdk.java.net/jfx pull/628/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 628

View PR using the GUI difftool:
$ git pr show -t 628

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jfx/pull/628.diff

@kevinrushforth
Copy link
Member Author

@kevinrushforth kevinrushforth commented Sep 21, 2021

/reviewers 2

Loading

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 21, 2021

👋 Welcome back kcr! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

Loading

@openjdk openjdk bot added the rfr label Sep 21, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Sep 21, 2021

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 21, 2021

Webrevs

Loading

Copy link
Collaborator

@jperedadnr jperedadnr left a comment

Looks good to me

Loading

Copy link
Collaborator

@nlisker nlisker left a comment

Looks good. The tests pass.

Two unrelated comments that I have observed during the review:

  1. java.security.AccessController is deprecated for removal since 17. We need to do something about this. It's used in BaseShaderGraphics, which is where I saw the warning, but it's used in other places too.
  2. Eclipse graciously informs me that "The method D3DGraphics.getContext() does not override the inherited method from BaseShaderGraphics since it is private to a different package". It detected the @Override annotation there and noted that the superclass method is not visible to this class since they are in different packages, so it is not actually overriding. This could lead to another subtle bug. Maybe that method should be protected. I didn't investigate much further.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Sep 21, 2021

@kevinrushforth This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8273946: Move clearQuad method to BaseShaderGraphics superclass

Reviewed-by: jpereda, nlisker

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

Loading

@openjdk openjdk bot added the ready label Sep 21, 2021
@kevinrushforth
Copy link
Member Author

@kevinrushforth kevinrushforth commented Sep 22, 2021

Thanks for the review.

Two unrelated comments that I have observed during the review:

  1. java.security.AccessController is deprecated for removal since 17. We need to do something about this. It's used in BaseShaderGraphics, which is where I saw the warning, but it's used in other places too.

We added @SuppressWarnings("removal") annotations for JDK-8264139, so I'm surprised the IDE is still flagging it. FWIW, it will be several years before we can remove any of these calls, since running with a security manager is still supported in JDK 17 LTS.

  1. Eclipse graciously informs me that "The method D3DGraphics.getContext() does not override the inherited method from BaseShaderGraphics since it is private to a different package". It detected the @Override annotation there and noted that the superclass method is not visible to this class since they are in different packages, so it is not actually overriding. This could lead to another subtle bug. Maybe that method should be protected. I didn't investigate much further.

Interesting. I guess the only reason the IDE is warning in this case is that it notices the @Override annotation, which is there because it implements the method in D3DGraphicsContextSource, and warns that it doesn't override the method of the same name in the superclass. We could consider a follow-up issue to clean this up, but I'm not really inclined to change anything.

Loading

@kevinrushforth
Copy link
Member Author

@kevinrushforth kevinrushforth commented Sep 22, 2021

/integrate

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Sep 22, 2021

Going to push as commit 338b999.
Since your change was applied there has been 1 commit pushed to the master branch:

  • 5c355fa: 8090158: Wrong implementation of adjustValue in scrollBars

Your commit was automatically rebased without conflicts.

Loading

@openjdk openjdk bot closed this Sep 22, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Sep 22, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Sep 22, 2021

@kevinrushforth Pushed as commit 338b999.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Loading

@nlisker
Copy link
Collaborator

@nlisker nlisker commented Sep 22, 2021

We added @SuppressWarnings("removal") annotations for JDK-8264139, so I'm surprised the IDE is still flagging it.

The warning is on the import of the class, which can be suppressed by adding the annotation to the class declaration. Maybe it's only this way in the Eclipse compiler.

We could consider a follow-up issue to clean this up, but I'm not really inclined to change anything.

The question is if this can cause any bugs? Is the method supposed to override? If yes and it isn't, it sounds like something could go wrong.

Loading

@kevinrushforth
Copy link
Member Author

@kevinrushforth kevinrushforth commented Sep 22, 2021

We added @SuppressWarnings("removal") annotations for JDK-8264139, so I'm surprised the IDE is still flagging it.

The warning is on the import of the class, which can be suppressed by adding the annotation to the class declaration. Maybe it's only this way in the Eclipse compiler.

This sounds like an Eclipse problem then. In any case, I would not consider adding annotations to the classes. We went to some effort to make the scope of the annotations as local as possible (as did the JDK).

We could consider a follow-up issue to clean this up, but I'm not really inclined to change anything.

The question is if this can cause any bugs? Is the method supposed to override? If yes and it isn't, it sounds like something could go wrong.

It isn't intended to be an override. The context parameter is passed up to the superclass and stored there as well as in the subclass. We could explore a different design pattern, but I don't think it is necessary.

Loading

@kevinrushforth kevinrushforth deleted the 8273946-clearQuad branch Sep 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants