[RFC][Icedtea-Web]: more refactoring + dead code removal
Deepak Bhole
dbhole at redhat.com
Fri Mar 4 15:27:21 PST 2011
* Denis Lila <dlila at redhat.com> [2011-03-02 10:51]:
> 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.
I assume you used Eclipse to do most of this refactoring? In terms of
removed arguments like SecurityManager, they are not used (they probably
were at some point in the past).
Based on the glance I've taken at the change, they fine to me. Okay for
HEAD.
Cheers,
Deepak
>
> Thank you,
> Denis.
> 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() {
More information about the distro-pkg-dev
mailing list