[rfc][icedtea-web] make console aware of plugin messages
Jiri Vanek
jvanek at redhat.com
Wed Dec 11 10:11:19 PST 2013
On 12/11/2013 04:57 PM, Pavel Tisnovsky wrote:
> Hi Jiri,
>
> this patch looks good in overall (mainly the synchronization during pushing/popping strings to a pipe!).
Thanx!
> I have just minor objections:
>
> netx/net/sourceforge/jnlp/util/logging/JavaConsole.java:
>
> the code:
> while (true) {
> try{
> String s = br.readLine();
> if (s == null) {
> break;
> }
> processPluginMessage(s);
>
> might be rewritten as:
> while ((String s = br.readLine()) != null) {
> processPluginMessage(s);
> }
Oh no, I hate this style :( //not fixed
>
> OutputController.getLogger().log("Ended processing of plugin-debug-to-conosle " + file.getAbsolutePath());
> ^^^^^^
> OutputController.getLogger().log("Started processing of plugin-debug-to-conosle " + file.getAbsolutePath());\
sure, thanx! Also I made Thread named.
> ^^^^^^
> netx/net/sourceforge/jnlp/util/logging/LogConfig.java Fri Nov 29 17:30:50 2013 +0100
> + * logging stuff fmay be interested in used config
> ^^^
fixed
>
> That code smells a bit :-) but now I know why U did it - to use the same method in a console with
> selectable output format:
> + public String toString() {
> + return toString(true, true, true, true, true, true, true);
> + }
>
>
yah :)
> plugin/icedteanp/IcedTeaNPPlugin.cc:
>
> IMHO it's better to explicitly initialize those variables to NULL, even if it's not scrictly necessary:
> +// Applet viewer debug pipe name.
> +gchar* debug_pipe_name;
> +
done
>
> I'm not sure if it's better to flush the pipe before reconecting the C and Java part:
> + if (plugin_debug_to_console){
> + //jvm_up is now false
> + if (g_io_channel_shutdown (debug_to_appletviewer,
>
nope - in this time console is dead anyway. So I would rather not risk flush from different thread.
>
> plugin/icedteanp/IcedTeaNPPlugin.h
> + nanosleep(&ts ,0); it might be time consuming, do you have any profiler
> output showing if there is/is not a perf. problem here?
Welll, I dont know. But it is worthy to work on as another changeset.
>
>
> + snprintf(ldebug_message, MESSAGE_SIZE, "%s%s", ldebug_header, ldebug_body); \
> + char ldebug_channel_message[MESSAGE_SIZE+50]; \
> + struct timeval current_time; \
> + gettimeofday (¤t_time, NULL);\
> + if (jvm_up) { \
> + sprintf(ldebug_channel_message,"%s %ld %s", "plugindebug", current_time.tv_sec*1000000L+current_time.tv_usec, ldebug_message); \
> ^^ should be snprintf(ldebug_message, MESSAGE_SIZE+50 or you migth create new constant (which is better)
> + push_pre_init_messages(ldebug_channel_message); \
> + } else { \
> + sprintf(ldebug_channel_message,"%s %ld %s", "preinit_plugindebug", current_time.tv_sec*1000000L+current_time.tv_usec, ldebug_message); \
> ^^ should be snprintf(ldebug_message, MESSAGE_SIZE+50 or you migth create new constant (which is better)
all four fixed, and constan introduced.
> + push_pre_init_messages(ldebug_channel_message); \
>
> + struct timeval current_time; \
> + gettimeofday (¤t_time, NULL);\
> + if (jvm_up) { \
> + sprintf(ldebug_channel_message,"%s %ld %s", "pluginerror", current_time.tv_sec*1000000L+current_time.tv_usec, ldebug_message); \
> + push_pre_init_messages(ldebug_channel_message); \
> + } else { \
> + sprintf(ldebug_channel_message,"%s %ld %s", "preinit_pluginerror", current_time.tv_sec*1000000L+current_time.tv_usec, ldebug_message); \
> + push_pre_init_messages(ldebug_channel_message); \
> dtto, a good candidate for refactoring :)
true. Voulenteer O:) ?
>
> + OutputController.getLogger().log(OutputController.Level.ERROR_ALL, "Warning, althoug console is on, plugin debug conenctin do not exists. No plugn information will be dysplayed in console (only java ones).");
> althoug <- typo
> dysplayed <- typo
fixed
>
>
However, bunch of tests started to fail with one those two exceptions:
eg java.lang.NullPointerException
at LessVerboseTextListener.getAnnotation(LessVerboseTextListener.java:126)
at LessVerboseTextListener.getRemote(LessVerboseTextListener.java:147)
at LessVerboseTextListener.printRemote(LessVerboseTextListener.java:153)
at LessVerboseTextListener.testFailure(LessVerboseTextListener.java:52)
at org.junit.runner.notification.RunNotifier$4.notifyListener(RunNotifier.java:139)
at org.junit.runner.notification.RunNotifier$SafeNotifier.run(RunNotifier.java:61)
at org.junit.runner.notification.RunNotifier.fireTestFailures(RunNotifier.java:134)
at org.junit.runner.notification.RunNotifier.fireTestFailure(RunNotifier.java:128)
at org.junit.internal.runners.model.EachTestNotifier.addFailure(EachTestNotifier.java:23)
at
org.junit.internal.runners.model.EachTestNotifier.addMultipleFailureException(EachTestNotifier.java:30)
at org.junit.internal.runners.model.EachTestNotifier.addFailure(EachTestNotifier.java:21)
at org.junit.runners.ParentRunner.run(ParentRunner.java:315)
at org.junit.runners.Suite.runChild(Suite.java:127)
at org.junit.runners.Suite.runChild(Suite.java:26)
at org.junit.runners.ParentRunner$3.run(ParentRunner.java:238)
at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:63)
at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:236)
at org.junit.runners.ParentRunner.access$000(ParentRunner.java:53)
at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:229)
at org.junit.runners.ParentRunner.run(ParentRunner.java:309)
at org.junit.runner.JUnitCore.run(JUnitCore.java:160)
at org.junit.runner.JUnitCore.run(JUnitCore.java:138)
at org.junit.runner.JUnitCore.run(JUnitCore.java:117)
at CommandLine.runMain(CommandLine.java:48)
at CommandLine.runMainAndExit(CommandLine.java:28)
at CommandLine.main(CommandLine.java:24)
and
Exception in thread "Thread-112" java.lang.NoClassDefFoundError: Could not initialize class
sun.awt.X11.XToolkit
at java.lang.Class.forName0(Native Method)
at java.lang.Class.forName(Class.java:190)
at java.awt.Toolkit$2.run(Toolkit.java:868)
at java.security.AccessController.doPrivileged(Native Method)
at java.awt.Toolkit.getDefaultToolkit(Toolkit.java:860)
at javax.swing.UIManager.getSystemLookAndFeelClassName(UIManager.java:608)
at net.sourceforge.jnlp.util.logging.JavaConsole.initialize(JavaConsole.java:164)
at net.sourceforge.jnlp.util.logging.JavaConsole.<init>(JavaConsole.java:126)
at net.sourceforge.jnlp.util.logging.JavaConsole.getConsole(JavaConsole.java:91)
at net.sourceforge.jnlp.util.logging.OutputController.consume(OutputController.java:157)
at net.sourceforge.jnlp.util.logging.OutputController.access$200(OutputController.java:46)
at net.sourceforge.jnlp.util.logging.OutputController$1.run(OutputController.java:229)
at java.lang.Thread.run(Thread.java:744)
and one memory leek was detected
...
Passed: get_scriptable_object
NOTE: Next error expected
Unknown plugin value requested.
*** WARNING: NP_GetValue has a memory leak! 1 more operator 'new' allocations than 'delete's!
Passed: NP_GetValue
...
And none of above issue is fixed when used together with rewritten console.
I swear They were not presented when I was splitting the patch, so I'm going to rerun them again
tomorow, and fix them as another chnageset if needed.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: consolePluginAware3.diff
Type: text/x-patch
Size: 52485 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20131211/683459f6/consolePluginAware3-0001.diff
More information about the distro-pkg-dev
mailing list