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

8201538: Remove implementation support for applets from JavaFX #615

Closed

Conversation

kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Aug 31, 2021

This PR removes the obsolete applet implementation from JavaFX. It is an ongoing maintenance burden to carry around this legacy code. Also, cleaning this up could help in the implementation of GTK4, Wayland, and Metal, since we won't have to account for the way applet windows are created and managed.

Notes to reviewers:

The first part of the removal was to eliminate the methods and classes on the Java side that are associated with creating and managing an applet window, and which are no longer called. After these were removed, I then removed the corresponding methods and classes on the native side that are no longer called.

Shared Code

The following were removed from the shared code.

Removed Java classes

com.sun.javafx.tk.AppletWindow
com.sun.javafx.tk.quantum.GlassAppletWindow

Removed methods

The following methods were removed in the parent class and all subclasses.

com.sun.glass.ui.Application:
    public abstract Window createWindow(long parent)

com.sun.glass.ui.Window:
    public boolean getAppletMode()
    public void setAppletMode(boolean appletMode)
    public void dispatchNpapiEvent(Map eventInfo)
    protected abstract long _createChildWindow(long parent)
    protected Window(long parent)
    protected abstract int _getEmbeddedX(long ptr)
    protected abstract int _getEmbeddedY(long ptr)

com.sun.javafx.tk.Toolkit:
    public abstract AppletWindow createAppletWindow(...)
    public abstract void closeAppletWindow()

com.sun.javafx.tk.quantum.WindowStage:
    static void setAppletWindow(GlassAppletWindow aw)
    static GlassAppletWindow getAppletWindow()

Linux (Gtk) Java code

The following classes or methods were removed:

com.sun.glass.ui.gtk.GtkChildWindow (class removed)

com.sun.glass.ui.gtk.GtkWindow:
    protected GtkWindow(long parent)

Linux (Gtk) native glass:

The following native classes were removed:

WindowContextChild
WindowContextPlug

macOS Java code

The following classes or methods were removed:

com.sun.glass.events.mac.NpapiEvent (class removed)

com.sun.glass.ui.mac.MacApplication:
    native protected String _getRemoteLayerServerName()

com.sun.glass.ui.View:
    public int getNativeRemoteLayerId(String serverName)

com.sun.glass.ui.mac.MacView:
    native protected int _getNativeRemoteLayerId(long ptr, String serverName)
    native protected void _hostRemoteLayerId(long ptr, int nativeLayerId)

com.sun.glass.ui.mac.MacWindow:
    protected MacWindow(long parent)

macOS native code

The following native classes were removed:

GlassEmbeddedWindow*
GlassNSEvent
GlassView3D+Remote
RemoteLayerSupport

I also removed the jIsChild parameter from the window creation code which allowed for removing a lot of dead blocks of code. The main window creation method was:

- (id)_initWithContentRect:(NSRect)contentRect styleMask:(NSUInteger)windowStyle
        screen:(NSScreen *)screen jwindow:(jobject)jwindow jIsChild:(jboolean)jIsChild

This created a GlassEmbeddedWindow iff jIsChild == JNI_TRUE. Since jIsChild was only set to true by the (now removed) _createChildWindow(long) method, we can remove the parameter, the GlassEmbeddedWindow* classes, and all code blocks that are qualified by if (jIsChild).

Windows Java code

The following classes or methods were removed:

com.sun.glass.ui.win.WinChildWindow (class removed)

com.sun.glass.ui.win.WinWindow:
    protected WinWindow(long parent)

Windows native code

After removing all references to IsChild(), which was only ever true for _createChildWindow(), we can also remove the following:

GlassApplication::InstallMouseLLHook
GlassApplication::UninstallMouseLLHook

iOS Java code

com.sun.glass.ui.ios.IosWindow:
    protected IosWindow(long parent)

iOS native code

With the removal of the _createChildWindow method, the following JNI method in IosWindow can be removed:

Java_com_sun_glass_ui_ios_IosWindow__1createChildWindow(JNIEnv *, jobject, jlong)

As a note, I don't have a setup to build this. It is a simple, safe change, but should be double-checked by someone from Gluon.


Progress

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

Issue

  • JDK-8201538: Remove implementation support for applets from JavaFX

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 615

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Aug 31, 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.

@kevinrushforth
Copy link
Member Author

@kevinrushforth kevinrushforth commented Aug 31, 2021

/reviewers 2

@openjdk openjdk bot added the rfr label Aug 31, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Aug 31, 2021

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

@@ -152,73 +139,70 @@ public final WindowStage init(GlassSystemMenu sysmenu) {
private void initPlatformWindow() {
if (platformWindow == null) {
Application app = Application.GetApplication();
if (isPrimaryStage && (null != appletWindow)) {
platformWindow = app.createWindow(appletWindow.getGlassWindow().getNativeWindow());
} else {
Copy link
Member Author

@kevinrushforth kevinrushforth Aug 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The diffs in this method below this point are mostly caused by indentation. I recommend reviewers select the "Hide whitespace changes" option.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Aug 31, 2021

Webrevs

mstr2
mstr2 approved these changes Oct 31, 2021
Copy link
Collaborator

@mstr2 mstr2 left a comment

This PR does what I would expect it to do. I tested it by building an application on all three desktop platforms and it works as usual.

@kevinrushforth kevinrushforth removed the request for review from pankaj-bansal Nov 30, 2021
@kevinrushforth kevinrushforth requested a review from aghaisas Dec 8, 2021
Copy link
Collaborator

@aghaisas aghaisas left a comment

Changes look good.

@openjdk
Copy link

@openjdk openjdk bot commented Dec 10, 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:

8201538: Remove implementation support for applets from JavaFX

Reviewed-by: mstrauss, aghaisas, jvos

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 65 new commits pushed to the master branch:

  • 85b8e96: 8278494: Remove .hgtags
  • 7915193: 8278425: TreeTableCellStartEditTest uses deprecated TreeTableCell methods
  • 6fd4ab6: 8191995: Regression: DatePicker must commit on focusLost
  • 5805bf8: 8276313: ScrollPane scroll delta incorrectly depends on content height
  • d3fbb51: 8276553: ListView scrollTo() is broken after fix for JDK-8089589
  • aa045c5: 8272118: ListViewSkin et al: must not cancel edit on scrolling
  • 5bd72a7: 8277457: AccessControlException: access denied ("java.net.NetPermission" "getCookieHandler")
  • 3d57213: 8277475: Update JDK_DOCS property to point to JDK 17 docs
  • b257647: 8276847: JSException: ReferenceError: Can't find variable: IntersectionObserver
  • fc3792d: 8276206: Rename TextBinding class to better express its purpose
  • ... and 55 more: https://git.openjdk.java.net/jfx/compare/91f017091786654a4564a5a054dbd22e6a01c120...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this 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.

@openjdk openjdk bot added the ready label Dec 10, 2021
@kevinrushforth
Copy link
Member Author

@kevinrushforth kevinrushforth commented Dec 10, 2021

@johanvos or @jperedadnr -- before I integrate this do you want to take a look at the iOS changes here? The changes look trivially correct to me, but since I don't have the ability to build the iOS code, I wanted to give you a change to comment.

Copy link
Collaborator

@johanvos johanvos left a comment

I did a build and sanity check for iOS, and it looks good.
The removed code should indeed not be referenced.

@kevinrushforth
Copy link
Member Author

@kevinrushforth kevinrushforth commented Dec 10, 2021

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Dec 10, 2021

Going to push as commit 4f9b047.
Since your change was applied there have been 65 commits pushed to the master branch:

  • 85b8e96: 8278494: Remove .hgtags
  • 7915193: 8278425: TreeTableCellStartEditTest uses deprecated TreeTableCell methods
  • 6fd4ab6: 8191995: Regression: DatePicker must commit on focusLost
  • 5805bf8: 8276313: ScrollPane scroll delta incorrectly depends on content height
  • d3fbb51: 8276553: ListView scrollTo() is broken after fix for JDK-8089589
  • aa045c5: 8272118: ListViewSkin et al: must not cancel edit on scrolling
  • 5bd72a7: 8277457: AccessControlException: access denied ("java.net.NetPermission" "getCookieHandler")
  • 3d57213: 8277475: Update JDK_DOCS property to point to JDK 17 docs
  • b257647: 8276847: JSException: ReferenceError: Can't find variable: IntersectionObserver
  • fc3792d: 8276206: Rename TextBinding class to better express its purpose
  • ... and 55 more: https://git.openjdk.java.net/jfx/compare/91f017091786654a4564a5a054dbd22e6a01c120...master

Your commit was automatically rebased without conflicts.

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

@openjdk openjdk bot commented Dec 10, 2021

@kevinrushforth Pushed as commit 4f9b047.

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

@kevinrushforth kevinrushforth deleted the 8201538-nuke-applet-impl branch Dec 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated
4 participants