[RFC][Icedtea-Web]: more refactoring + dead code removal
Dr Andrew John Hughes
ahughes at redhat.com
Mon Mar 7 08:00:20 PST 2011
On 18:27 Fri 04 Mar , Deepak Bhole wrote:
> * 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?
What makes you think that?
> 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() {
>
--
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