Icedtea-web splashscreen implementation

[Omair Majid]

(my mailer has been behaving really weird, apologies if some comments
are duplicated)

On 03/02/2012 07:35 AM, Jiri Vanek wrote:
> Hi All!

Hi Jiri,

Thanks for doing this. The patch is quite long and complicated, so I am
going to do this in parts. One request for the future: could you split
out patches and send one-per email? It avoids mixing a bunch of
not-exactly-related patches with each other. Or at least avoid sending a
combined patch along with all the parts.

Also, can you please add _unit_ tests? If nothing else, they will force
you to decouple the code and clean up the design.

Something I am a little curious about is the interaction between the
LaunchHandler and the splash screen. I got the feeling that NetX had
intended LaunchHandler to be used to indicate progress with the
launching of JNLP programs. And that's what I used to implement the
splash screens. Perhaps it might be good if we could somehow combine
it/split responsibilities. If the launch handler was per
application/applet (currently it is shared by the entire runtime), it
could be the one to always trigger the (right) splash screen.

That would mean that applets and jnlps could always rely on informing
the launch handler (without knowing what it is) and the launch handler
could do the right thing (show dialog, hide splash, whatever).

> This patch is initial version of three and half connected things. Applet
> + javaws splashcreen and testing of applets in firefox (and testing of
> splascreen).

Perhaps you could post these separately? Or at least split into multiple
patches (one patch that adds the hooks and interfaces for the splash
screen and another that adds implementation for the splash screen?) Or
did you do that already in the version you posted? (it's hard to review
stuff if you include 3 patches one of which is the other two combined ;) )

> I'm sorry for two days delay, which was caused by more testing and
> creating this "document" and its attending animation. I'm also really
> sorry for its length.

No worries about the delay. I can see it's quite complicated. And I can
understand it would be hard to test.

> People cc-ed (as patch is to big to pass to distro without approve)
> Omair - just to kept him in loop ;)

Thanks :)

> Splashscreen(s) are designed to be easily replaceable,

/me looks at the rest of the patch below and has some doubts

See my comments in-line.

> and are providing
> set of utilities  for lunching and disposing it. When ratio of WxH is to
> big or applet is to small then alternate - plain IcedTea web (plugin)
> [with little bit animated web (plugin)] is shown.

Nice!

> Test framework was enriched for slow downloading. If the file have
> prefix "XslowX" then its downlaoding (even on localhost) will take
> 10seconds.

I see Pavel has already pointed this out, but please no magic behaviour
like this. It will make life hell for the whoever ends up maintaining
these in the future.

> *Splashscreens*
> _integration_
> The gate for applet's splashscreen is PluginAppletViewer. It is
> responsible for show it when applet is to be initialized and for
> removing it when applet starts. Little bit more complicated is showing
> of error to user when some exception during initialization occurs. Those
> exceptions are mostly thrown from netx which do not have dependence on
> plugin.

I think the "splashscreen" (it's more of a loading screen for the
applets, isn't it?) is doing too much. It is drawing the logo, animating
it, showing error messages (and probably a lot more). Can you please
split this out?

I am thinking of something like this (this is only the "view" (swing/awt
component) hierarchy:

AppletViewerPanel
  - Main-Container
    - Splash Screen Container
    - Error Indicator Container

Basically, each class/object should have one task. And you can swap out
the splash screen with the error indicator when an error happens. If you
can, please split out the gui-related classes from the non-gui related
(the logic) classes.

> To forward error to applet is used SplashUtils.showError set of
> overloaded methods.

I just looked at the implementation of this method. Please avoid using
reflection. It just pushes compile time errors into runtime errors, and
pollutes method signatures (you know, NoSuchMethodException,
IllegalAccessException, IllegalArgumentException,
InvocationTargetException). Is
there an issue with using a normal interface?

> SplashUtils have also factory method which handles
> which splash (type and purpose (javaws x applet)) is about to be shown
> (if any - see bonuses)

Not many places in icedtea-web make an explicit distinction between
javaws and applet. Instead of spreading this knowledge around, can you
put this in one place? JNLPRuntime seems appropriate to me, but there
may be a better place.

> There are actually three and half types of splashscreen. Applet one
> (which have error variation also), javaws one (forced by specification,
> which also forces title,vendor and homepage with description to be
> shown),

This is not quite true. I know what the "specification" says, but other
implementations do not follow this particular requirement.

> and java's -spalsh which is showing image until control is
> forwarded to netx.

Let's not worry about this one ;)

> All those four  splashscreens are cooperating quite well and support of
> custom splashses (by -J-splash or by <icon kind="splash"...>) are not lost.

I wasn't aware you could actually mess with what happens if -splash is used.

> _implementation_
> The splashscreen can be any class which implements interface
> SplashScreen. it must be able to paint itself, to add itself as
> component to show error, to be in composed of vectors.....and to be nice:)

As mentioned above, this is violating the Single Responsibility
Principle and doing too much in one interface. Please split the interface.

> The factory method have parameter whether to provide splash for javaws
> (it is showing also information from information element (vendor,
> description,homepage,title..) - specification commanding)

Out of curiosity, why did you decide to implement InformationElement? Is
InformationDesc not sufficient?

> The showing of error screen is very successfull, but one case I was
> unable to catch - when connection with server dies in middle of resource
> transfer and TODO - I have forgot to test signatures and stuff around:-/
> - but this should be handled n netx-way too.

Unit tests to check this, please!

> I'm showing also version on splash (and forwarding it to error mesage)
> (ok?) - Looks to me like good idea....

Not sure if showing version on the splash itself is a great idea, but I
agree it is good to show it in the error.

> I have noticed that when more then one applets are presented in page,
> then splashes sometimes shows, sometimes no.... I have tried to
> synchronize (I can be wrong!) in Launcher and in PluginAppletViewer, and
> it looks like it helped but still this occurs (quait randomly :-/)

Ugh...Please do look into this. I am not a fan of adding synchronization
(generally, it makes things more convoluted) unless absolutely necessary
and only where it really is needed.

> Sometimes(less then 10% of lunches, randomly spread) freeing of applet
> takes quite long time, and it looks like C-error (Can not fetch applets
> id from java side), But it is rare and always exit at the end.

Is this a regression? Or do you get the same behaviour without the splash?

> _bonuses_
> I have added recognition of two environment variables -
> ICEDTEA_WEB_PLUGIN_SPLASH and ICEDTEA_WEB_SPLASH. Those variables alow
> user to change splahs screen before lunchtime and - what is real reason
> for those two variables - to disable splashscreen at all.

The only environment variable that IcedTea-Web currently uses is
ICEDTEAPLUGIN_DEBUG. It would be great if all the names were (somehow)
consistent.

> I really can
> imagine some developer really tired of looking to same splash during his
> appelt/javaws_app testing phase.

I don't think this is terribly important. Still, it's nice to have it.

> When "custom" (or "none") splash is defined, then javaws "java's"
> -splash is ignored too.
> 

So we mess around with java's -splash? Why?

> This throws BrokenPipe Exception, but is working fine.

Eh? Throws an exception and works fine? What's throwing the exception?

Cheers,
Omair