Icedtea-web splashscreen implementation
Omair Majid
omajid at redhat.com
Mon Oct 29 11:42:57 PDT 2012
Hi Jiri,
Comments in-line below.
On 10/24/2012 05:53 AM, Jiri Vanek wrote:
> diff -r c52fcd37dbd8 netx/net/sourceforge/jnlp/GuiLaunchHandler.java
> --- a/netx/net/sourceforge/jnlp/GuiLaunchHandler.java Tue Oct 23 09:56:26 2012 +0200
> +++ b/netx/net/sourceforge/jnlp/GuiLaunchHandler.java Wed Oct 24 11:46:08 2012 +0200
> @@ -80,10 +80,13 @@
> }
>
> private void closeSplashScreen() {
> - synchronized(mutex) {
> + synchronized (mutex) {
> if (splashScreen != null) {
> if (splashScreen.isSplashScreenValid()) {
> splashScreen.setVisible(false);
> + if (splashScreen.isCustomSplashscreen()) {
> + splashScreen.stopAnimation();
> + }
Perhaps splashScreen.stopAnimation() can be called without the
splashScreen.isCustomSplashscreen check? stopAnimation() already does
this check.
> diff -r c52fcd37dbd8 netx/net/sourceforge/jnlp/JNLPSplashScreen.java
> --- a/netx/net/sourceforge/jnlp/JNLPSplashScreen.java Tue Oct 23 09:56:26 2012 +0200
> +++ b/netx/net/sourceforge/jnlp/JNLPSplashScreen.java Wed Oct 24 11:46:08 2012 +0200
> + //Toolkit.getDefaultToolkit().getScreenSize(); works always, but center
> + //only to middle of all monitors.
> + //Below code works on 1.4 and higher, and center to middle of primary monmtor
This comment, just by itself, does not make too much sense (I guess it
requires the reader to know about the previous implementation). Perhaps
you can rephrase it?
// Centering to middle of Toolkit.getDefaultToolkit().getScreenSize()
// centers to the middle of all monitors. Let's center to the middle
// of the primary monitor instead.
// TODO center on the 'current' monitor to meet user expectation
> + setLocationRelativeTo(null);
> }
>
> @Override
> public void paint(Graphics g) {
> if (splashImage == null) {
> + super.paint(g);
Nice catch!
> +
> + boolean isCustomSplashscreen() {
> + return (componetSplash!=null);
> + }
> +
> + void stopAnimation() {
> + if (isCustomSplashscreen()) componetSplash.stopAnimation();
> + }
Please consider making these methods public or private.
> }
> diff -r c52fcd37dbd8 netx/net/sourceforge/jnlp/Launcher.java
> --- a/netx/net/sourceforge/jnlp/Launcher.java Tue Oct 23 09:56:26 2012 +0200
> +++ b/netx/net/sourceforge/jnlp/Launcher.java Wed Oct 24 11:46:08 2012 +0200
> protected ApplicationInstance launchApplet(JNLPFile file, boolean enableCodeBase, Container cont) throws LaunchException {
> + if (JNLPRuntime.getForksAllowed() && file.needsNewVM()) {
> + if (!JNLPRuntime.isHeadless()) {
> + SplashScreen sp = SplashScreen.getSplashScreen();
> + if (sp != null) {
> + sp.close();
> + }
> + }
This is really puzzling. Forks are never allowed for applets, so the
first if statement should never evaluate to true. But if we could start
new VMs for applets, we would need to prevent applets from running in
this VM. We need to return from this method to avoid executing further
code, but we also need to notify the plugin that another VM now handles
the plugin.
I think it's just simpler to remove this code. Maybe add this instead?
if (JNLPRuntime.getForksAllowed()) {
throw new AssertionError("applets are not allowed to fork");
}
> + }
> + if (handler != null) {
> + handler.launchInitialized(file);
> + }
Please don't add null checks. Why is the handler null? Maybe it should
be set to a no-op handler instance (in the constructor of this class) if
it's not otherwise set?
> - protected AppletInstance createApplet(JNLPFile file, boolean enableCodeBase, Container cont) throws LaunchException {
> + protected synchronized AppletInstance createApplet(JNLPFile file, boolean enableCodeBase, Container cont) throws LaunchException {
Okay, this "synchronized" keyword here looks rather out of place. If
synchronization is really needed, I don't think we should be adding it
to this class.
> + // appletInstance is needed by ServiceManager when looking up
> + // services. This could potentially be done in applet constructor
> + // so initialize appletInstance before creating applet.
Could you leave this change out of the patch? Let's not mix in multiple
fixes/changes.
Also, I am not sure if applets are allowed to use the ServiceManager
methods until init() is invoked. They are not allowed to call applet
methods, for example:
http://docs.oracle.com/javase/7/docs/api/java/applet/Applet.html#Applet()
> + AppletInstance appletInstance = null;
> + ThreadGroup group = Thread.currentThread().getThreadGroup();
> try {
> JNLPClassLoader loader = JNLPClassLoader.getInstance(file, updatePolicy);
>
> if (enableCodeBase) {
> loader.enableCodeBase();
> } else if (file.getResources().getJARs().length == 0) {
> - throw new ClassNotFoundException("Can't do a codebase look up and there are no jars. Failing sooner rather than later");
> + Exception ex = new ClassNotFoundException("Can't do a codebase look up and there are no jars. Failing sooner rather than later");
> + try {
> + SplashUtils.showError(ex, new AppletEnvironment(file, new AppletInstance(file, group, new ClassLoader() {
> + }, null)));
> + } catch (Exception exx) {
> + if (JNLPRuntime.isDebug()) {
> + exx.printStackTrace();
> + }
> + }
Would it be possible to use launchError to handle this message rather
than using SplashUtils directly?
> private LaunchException launchError(LaunchException ex) {
> + return launchError(ex, null);
> + }
> +
> + private LaunchException launchError(LaunchException ex, AppletInstance applet) {
> + if (applet != null) {
> + SplashUtils.showErrorCaught(ex, applet);
> + }
RFE: I would like to see (in some future patch) a applet-specific
handler that can be used to avoid needing a explicit call to SplashUtils.
> + public void setAppletViewerFrame(SplashController framePanel) {
> + appletViewerFrame=framePanel;
> + }
> +
> + @Override
> + public void removeSplash() {
> + appletViewerFrame.removeSplash();
> + }
> +
> + @Override
> + public void replaceSplash(SplashPanel r) {
> + appletViewerFrame.replaceSplash(r);
> + }
> +
> + @Override
> + public int getSplashWidth() {
> + return appletViewerFrame.getSplashWidth();
> + }
> +
> + @Override
> + public int getSplashHeigth() {
> + return appletViewerFrame.getSplashHeigth();
> + }
Interesting that all these methods are manipulating the
appletViewerFrame. Looks to me like they belong in PluginAppletViewer
class. Or maybe the instance variable appletViewerFrame should be
renamed to splashController. But it still seems odd that these methods
are in this class if all they are doing is passing the calls along to
the SplashController.
> diff -r c52fcd37dbd8 plugin/icedteanp/java/sun/applet/PluginAppletViewer.java
> --- a/plugin/icedteanp/java/sun/applet/PluginAppletViewer.java Tue Oct 23 09:56:26 2012 +0200
> +++ b/plugin/icedteanp/java/sun/applet/PluginAppletViewer.java Wed Oct 24 11:46:08 2012 +0200
I would like someone more knowledgeable than me to review this piece of
code too. There are lots of changes with implications that I am unaware of.
> @@ -145,7 +149,12 @@
> @Override public void run() {
> panel.createNewAppContext();
> // create the frame.
> - PluginAppletViewer.framePanel(identifier, handle, panel);
> + int width = Integer.parseInt(atts.get("width"));
> + int height = Integer.parseInt(atts.get("height"));
> +
> + PluginDebug.debug("X and Y are: " + width + " " + height);
The width/height are being obtained twice now. Is the second invocation
needed?
> - public static void framePanel(int identifier, long handle, NetxPanel panel) {
> + public static synchronized PluginAppletViewer framePanel(int identifier,long handle, int width, int height, NetxPanel panel) {
Maybe this method should be renamed to createPanel now? It's not framing
the panel anymore.
Also, why the 'synchronized' keyword. What requires synchronization?
> + final AppletPanel fPanel = panel;
> + try {
> + SwingUtilities.invokeAndWait(new Runnable() {
> +
> + public void run() {
> + add("Center", fPanel);
> + fPanel.setVisible(false);
> +
> + splashPanel = SplashUtils.getSplashScreen(fPanel.getWidth(), fPanel.getHeight());
> + if (splashPanel != null) {
> + splashPanel.startAnimation();
> + PluginDebug.debug("Added splash " + splashPanel);
> + add("Center", splashPanel.getSplashComponent());
> + }
> + pack();
> + }
> + });
> + } catch (Exception e) {
> + e.printStackTrace(); // Not much we can do other than print
> + }
This new code looks like it really should be in its own method.
> + @Override
> + public void removeSplash() {
> +
> + // Loading done. Remove splash screen.
> + if (splashPanel == null) {
> + return;
> + }
Is the comment relevant? At first I thought it was saying that
splashPanel == null implies that loading is done. But going through the
code, it looks like it just makes this method a no-op after first
invocation.
> + try {
> + SwingUtilities.invokeAndWait(new Runnable() {
> +
> + public void run() {
> + splashPanel.getSplashComponent().setVisible(false);
> + splashPanel.stopAnimation();
> + remove(splashPanel.getSplashComponent());
> + splashPanel = null;
> + // Re-add the applet
> + remove(panel);
> + add(panel);
I am guessing the remove()/add() is need to notify the container of
changes? Maybe the comment can say why that's being done instead of
saying what's being done.
> + case AppletPanel.APPLET_START: {
> + String s="Error1 detected";
> + PluginDebug.debug(s);
> + if (src.status != AppletPanel.APPLET_INIT && src.status != AppletPanel.APPLET_STOP) {
> + SplashPanel sp=SplashUtils.getErrorSplashScreen(appletViewer.panel.getWidth(), appletViewer.panel.getHeight(), new Exception(s));
> + appletViewer.replaceSplash(sp);
> + }
> +
> + break;
> + }
Could you add a comment explaining how APPLET_START is an error?
> + case AppletPanel.APPLET_ERROR: {
> + String s="Error2 detected";
> + PluginDebug.debug(s);
The debug message doesn't explain anything. Could you add more information?
> + System.err.println("Waiting for applet init");
Sounds like this should be a PluginDebug.debug() message.
Cheers,
Omair
--
PGP Key: 66484681 (http://pgp.mit.edu/)
Fingerprint = F072 555B 0A17 3957 4E95 0056 F286 F14F 6648 4681
More information about the distro-pkg-dev
mailing list