[RFC][Icedtea-Web]: more refactoring + dead code removal
Dr Andrew John Hughes
ahughes at redhat.com
Wed Mar 2 09:09:40 PST 2011
On 10:50 Wed 02 Mar , Denis Lila wrote:
> Hi.
>
> I've been looking at the java side of the plugin more
> and I think we should make the attached changes. Mostly
> they're removing unused code (in one case (a String.split call)
> the code is executed, but it's still useless).
>
> Other than that, I've made some declarations private/static/final,
> and in a few cases moved code around so it was easier to read
> (like grouping static variables together, and moving a constructor
> closer to the top of the class, where the variable declarations are).
>
> I also fixed a typo (I think it was a typo).
>
> I'm a bit uneasy about some of these changes. In particular, removing
> the SecurityManager argument to PluginMessageHandlerWorker and removing
> the creation of pav in PluginStreamHandler, so some feedback would be nice.
>
> Thank you,
> Denis.
I'll leave the code review to Deepak, but has anyone tried running FindBugs
on the plugin and NetX codebases?
> diff -r 738c9c852dda ChangeLog
> --- a/ChangeLog Tue Mar 01 11:55:31 2011 -0500
> +++ b/ChangeLog Wed Mar 02 10:28:47 2011 -0500
> @@ -1,3 +1,29 @@
> +2011-03-02 Denis Lila <dlila at redhat.com>
> +
> + * plugin/icedteanp/java/sun/applet/PluginAppletSecurityContext.java:
> + (prepopulateMethod) removed unused object o.
> + * plugin/icedteanp/java/sun/applet/PluginCallRequest.java:
> + Made all the members private.
> + * plugin/icedteanp/java/sun/applet/PluginMessageConsumer.java:
> + Removed unused imports.
> + (MAX_PARALLEL_INITS, MAX_WORKERS, PRIORITY_WORKERS, readQueue,
> + workers, streamHandler, consumerThread,
> + registerPriorityWait(String), unRegisterPriorityWait(String)):
> + made private.
> + (initWorkers, as, processedIds, unRegisterPriorityWait(Long),
> + addToInitWorkers): removed - unused.
> + (getPriorityStrIfPriority): made static; replaced while with for-each.
> + (notifyWorkerIsFree): removed synchronized section - useless.
> + (ConsumerThread.run): removed call to addToInitWorkers.
> + * plugin/icedteanp/java/sun/applet/PluginMessageHandlerWorker.java:
> + Removed explicit member initializations to the default values; fixed typo.
> + (PluginMessageHandlerWorker): Removed SecurityManager argument - unused.
> + * plugin/icedteanp/java/sun/applet/PluginStreamHandler.java:
> + Removed unused imports.
> + (consumer, shuttingDown): made private.
> + (pav, writeQueue, getMessage, messageAvailable): removed - unused.
> + (PluginStreamHandler): removed pav initialization.
> +
> 2011-03-01 Andrew Su <asu at redhat.com>
>
> * netx/net/sourceforge/jnlp/controlpanel/ControlPanel.java
> diff -r 738c9c852dda plugin/icedteanp/java/sun/applet/AppletSecurityContextManager.java
> --- a/plugin/icedteanp/java/sun/applet/AppletSecurityContextManager.java Tue Mar 01 11:55:31 2011 -0500
> +++ b/plugin/icedteanp/java/sun/applet/AppletSecurityContextManager.java Wed Mar 02 10:28:47 2011 -0500
> @@ -43,7 +43,6 @@
> public class AppletSecurityContextManager {
>
> // Context identifier -> PluginAppletSecurityContext object.
> - // FIXME: make private
> private static HashMap<Integer, PluginAppletSecurityContext> contexts =
> new HashMap<Integer, PluginAppletSecurityContext>();
>
> diff -r 738c9c852dda plugin/icedteanp/java/sun/applet/PluginAppletSecurityContext.java
> --- a/plugin/icedteanp/java/sun/applet/PluginAppletSecurityContext.java Tue Mar 01 11:55:31 2011 -0500
> +++ b/plugin/icedteanp/java/sun/applet/PluginAppletSecurityContext.java Wed Mar 02 10:28:47 2011 -0500
> @@ -1228,15 +1228,14 @@
> Class<?> c = (Class<?>) store.getObject(classID);
> Method m = null;
> Constructor cs = null;
> - Object o = null;
>
> try {
> if (methodName.equals("<init>")
> || methodName.equals("<clinit>")) {
> - o = cs = c.getConstructor(signature.getClassArray());
> + cs = c.getConstructor(signature.getClassArray());
> store.reference(cs);
> } else {
> - o = m = c.getMethod(methodName, signature.getClassArray());
> + m = c.getMethod(methodName, signature.getClassArray());
> store.reference(m);
> }
> } catch (NoSuchMethodException e) {
> diff -r 738c9c852dda plugin/icedteanp/java/sun/applet/PluginCallRequest.java
> --- a/plugin/icedteanp/java/sun/applet/PluginCallRequest.java Tue Mar 01 11:55:31 2011 -0500
> +++ b/plugin/icedteanp/java/sun/applet/PluginCallRequest.java Wed Mar 02 10:28:47 2011 -0500
> @@ -40,10 +40,10 @@
> // FIXME: for each type of request extend a new (anonymous?)
> // PluginCallRequest.
> public abstract class PluginCallRequest {
> - String message;
> - Long reference;
> - PluginCallRequest next;
> - boolean done = false;
> + private String message;
> + private Long reference;
> + private PluginCallRequest next;
> + private boolean done = false;
>
> public PluginCallRequest(String message, Long reference) {
> this.message = message;
> diff -r 738c9c852dda plugin/icedteanp/java/sun/applet/PluginMessageConsumer.java
> --- a/plugin/icedteanp/java/sun/applet/PluginMessageConsumer.java Tue Mar 01 11:55:31 2011 -0500
> +++ b/plugin/icedteanp/java/sun/applet/PluginMessageConsumer.java Wed Mar 02 10:28:47 2011 -0500
> @@ -38,29 +38,29 @@
> package sun.applet;
>
> import java.util.ArrayList;
> -import java.util.Hashtable;
> -import java.util.Iterator;
> import java.util.LinkedList;
>
> class PluginMessageConsumer {
>
> - private static int MAX_PARALLEL_INITS = 1;
> + private static final int MAX_PARALLEL_INITS = 1;
>
> // Each initialization requires 5 responses (tag, handle, width, proxy, cookie)
> // before the message stack unlocks/collapses. This works out well because we
> // want to allow upto 5 parallel tasks anyway
> - private static int MAX_WORKERS = MAX_PARALLEL_INITS * 4;
> - private static int PRIORITY_WORKERS = MAX_PARALLEL_INITS * 2;
> + private static final int MAX_WORKERS = MAX_PARALLEL_INITS * 4;
> + private static final int PRIORITY_WORKERS = MAX_PARALLEL_INITS * 2;
>
> - private static Hashtable<Integer, PluginMessageHandlerWorker> initWorkers = new Hashtable<Integer, PluginMessageHandlerWorker>(2);
> + private static LinkedList<String> priorityWaitQueue = new LinkedList<String>();
>
> - LinkedList<String> readQueue = new LinkedList<String>();
> - private static LinkedList<String> priorityWaitQueue = new LinkedList<String>();
> - ArrayList<PluginMessageHandlerWorker> workers = new ArrayList<PluginMessageHandlerWorker>();
> - PluginStreamHandler streamHandler = null;
> - AppletSecurity as;
> - ConsumerThread consumerThread = new ConsumerThread();
> - private static ArrayList<Integer> processedIds = new ArrayList<Integer>();
> + private LinkedList<String> readQueue = new LinkedList<String>();
> + private ArrayList<PluginMessageHandlerWorker> workers = new ArrayList<PluginMessageHandlerWorker>();
> + private PluginStreamHandler streamHandler;
> + private ConsumerThread consumerThread = new ConsumerThread();
> +
> + public PluginMessageConsumer(PluginStreamHandler streamHandler) {
> + this.streamHandler = streamHandler;
> + this.consumerThread.start();
> + }
>
> /**
> * Registers a reference to wait for. Responses to registered priority
> @@ -78,78 +78,45 @@
> *
> * @param searchString the string to look for in a response
> */
> - public static void registerPriorityWait(String searchString) {
> + private static void registerPriorityWait(String searchString) {
> PluginDebug.debug("Registering priority for string " + searchString);
> synchronized (priorityWaitQueue) {
> - if (!priorityWaitQueue.contains(searchString))
> + if (!priorityWaitQueue.contains(searchString)) {
> priorityWaitQueue.add(searchString);
> + }
> }
> }
>
> /**
> - * Unregisters a priority reference to wait for.
> - *
> - * @param reference The reference to remove
> - */
> - public static void unRegisterPriorityWait(Long reference) {
> - unRegisterPriorityWait(reference.toString());
> - }
> -
> - /**
> * Unregisters a priority string to wait for.
> *
> * @param searchString The string to unregister from the priority list
> */
> - public static void unRegisterPriorityWait(String searchString) {
> + private static void unRegisterPriorityWait(String searchString) {
> synchronized (priorityWaitQueue) {
> priorityWaitQueue.remove(searchString);
> }
> }
>
> - public PluginMessageConsumer(PluginStreamHandler streamHandler) {
> -
> - as = new AppletSecurity();
> - this.streamHandler = streamHandler;
> - this.consumerThread.start();
> - }
> -
> - private String getPriorityStrIfPriority(String message) {
> + private static String getPriorityStrIfPriority(String message) {
>
> // Destroy messages are permanently high priority
> - if (message.endsWith("destroy"))
> + if (message.endsWith("destroy")) {
> return "destroy";
> + }
>
> synchronized (priorityWaitQueue) {
> - Iterator<String> it = priorityWaitQueue.iterator();
> -
> - while (it.hasNext()) {
> - String priorityStr = it.next();
> - if (message.indexOf(priorityStr) > 0)
> - return priorityStr;
> + for (String priorityStr : priorityWaitQueue) {
> + if (message.indexOf(priorityStr) > 0) {
> + return priorityStr;
> + }
> }
> }
>
> return null;
> }
>
> - private void addToInitWorkers(Integer instanceNum, PluginMessageHandlerWorker worker) {
> - synchronized (initWorkers) {
> - initWorkers.put(instanceNum, worker);
> - }
> - }
> -
> public void notifyWorkerIsFree(PluginMessageHandlerWorker worker) {
> - synchronized (initWorkers) {
> - Iterator<Integer> i = initWorkers.keySet().iterator();
> - while (i.hasNext()) {
> - Integer key = i.next();
> - if (initWorkers.get(key).equals(worker)) {
> - processedIds.add(key);
> - initWorkers.remove(key);
> - }
> - }
> - }
> -
> consumerThread.interrupt();
> }
>
> @@ -201,8 +168,6 @@
>
> if (message != null) {
>
> - String[] msgParts = message.split(" ");
> -
> String priorityStr = getPriorityStrIfPriority(message);
> boolean isPriorityResponse = (priorityStr != null);
>
> @@ -220,9 +185,6 @@
> continue; // re-loop to try next msg
> }
>
> - if (msgParts[2].equals("tag"))
> - addToInitWorkers((new Integer(msgParts[1])), worker);
> -
> if (isPriorityResponse) {
> unRegisterPriorityWait(priorityStr);
> }
> @@ -257,10 +219,10 @@
>
> if (workers.size() < (MAX_WORKERS - PRIORITY_WORKERS)) {
> PluginDebug.debug("Cannot find free worker, creating worker " + workers.size());
> - worker = new PluginMessageHandlerWorker(this, streamHandler, workers.size(), as, false);
> + worker = new PluginMessageHandlerWorker(this, streamHandler, workers.size(), false);
> } else if (prioritized) {
> PluginDebug.debug("Cannot find free worker, creating priority worker " + workers.size());
> - worker = new PluginMessageHandlerWorker(this, streamHandler, workers.size(), as, true);
> + worker = new PluginMessageHandlerWorker(this, streamHandler, workers.size(), true);
> } else {
> return null;
> }
> diff -r 738c9c852dda plugin/icedteanp/java/sun/applet/PluginMessageHandlerWorker.java
> --- a/plugin/icedteanp/java/sun/applet/PluginMessageHandlerWorker.java Tue Mar 01 11:55:31 2011 -0500
> +++ b/plugin/icedteanp/java/sun/applet/PluginMessageHandlerWorker.java Wed Mar 02 10:28:47 2011 -0500
> @@ -40,16 +40,16 @@
> class PluginMessageHandlerWorker extends Thread {
>
> private boolean free = true;
> - private boolean isPriorityWorker = false;
> - private int id;
> - private String message = null;
> - PluginStreamHandler streamHandler = null;
> - PluginMessageConsumer consumer = null;
> + private final boolean isPriorityWorker;
> + private final int id;
> + private String message;
> + private PluginStreamHandler streamHandler;
> + private PluginMessageConsumer consumer;
>
> public PluginMessageHandlerWorker(
> PluginMessageConsumer consumer,
> PluginStreamHandler streamHandler, int id,
> - SecurityManager sm, boolean isPriorityWorker) {
> + boolean isPriorityWorker) {
>
> this.id = id;
> this.streamHandler = streamHandler;
> @@ -70,7 +70,7 @@
>
> PluginDebug.debug("Consumer (priority=" + isPriorityWorker + ") thread " + id + " consuming " + message);
>
> - // ideally, whoever returns things object should mark it
> + // ideally, whoever returns this object should mark it
> // busy first, but just in case..
> busy();
>
> diff -r 738c9c852dda plugin/icedteanp/java/sun/applet/PluginStreamHandler.java
> --- a/plugin/icedteanp/java/sun/applet/PluginStreamHandler.java Tue Mar 01 11:55:31 2011 -0500
> +++ b/plugin/icedteanp/java/sun/applet/PluginStreamHandler.java Wed Mar 02 10:28:47 2011 -0500
> @@ -46,8 +46,6 @@
> import java.io.OutputStreamWriter;
> import java.net.MalformedURLException;
> import java.nio.charset.Charset;
> -import java.util.Date;
> -import java.util.LinkedList;
>
> import javax.swing.SwingUtilities;
>
> @@ -60,27 +58,14 @@
>
> private JavaConsole console = new JavaConsole();
>
> - LinkedList<String> writeQueue = new LinkedList<String>();
> + private PluginMessageConsumer consumer;
> + private Boolean shuttingDown = false;
>
> - PluginMessageConsumer consumer;
> - Boolean shuttingDown = false;
> -
> - PluginAppletViewer pav;
>
> public PluginStreamHandler(InputStream inputstream, OutputStream outputstream)
> throws MalformedURLException, IOException {
>
> PluginDebug.debug("Current context CL=" + Thread.currentThread().getContextClassLoader());
> - try {
> - pav = (PluginAppletViewer) ClassLoader.getSystemClassLoader().loadClass("sun.applet.PluginAppletViewer").newInstance();
> - PluginDebug.debug("Loaded: " + pav + " CL=" + pav.getClass().getClassLoader());
> - } catch (InstantiationException e) {
> - e.printStackTrace();
> - } catch (IllegalAccessException e) {
> - e.printStackTrace();
> - } catch (ClassNotFoundException e) {
> - e.printStackTrace();
> - }
>
> PluginDebug.debug("Creating consumer...");
> consumer = new PluginMessageConsumer(this);
> @@ -353,17 +338,6 @@
> return;
> }
>
> - public boolean messageAvailable() {
> - return writeQueue.size() != 0;
> - }
> -
> - public String getMessage() {
> - synchronized (writeQueue) {
> - String ret = writeQueue.size() > 0 ? writeQueue.poll() : "";
> - return ret;
> - }
> - }
> -
> private void showConsole() {
> SwingUtilities.invokeLater(new Runnable() {
> public void run() {
--
Andrew :)
Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)
Support Free Java!
Contribute to GNU Classpath and IcedTea
http://www.gnu.org/software/classpath
http://icedtea.classpath.org
PGP Key: F5862A37 (https://keys.indymedia.org/)
Fingerprint = EA30 D855 D50F 90CD F54D 0698 0713 C3ED F586 2A37
More information about the distro-pkg-dev
mailing list