481494 - Add optional splash screen on startup for our slow targets

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: initial splash screen patch for Windows CE

I'm sad to be creating this bug, but it's better than the alternative (which is no feedback at all during startup). We should get rid of this once we have startup nailed down to be "fast enough", or we should come up with some way to take advantage of this screen.

One idea is to take a screenshot of your browser when you close it, and use that as the startup splash, but grayed out with a progress bar.  Sort of a "waking up" type of display.  This has some problems, namely that you may have closed your browser for a reason. :)

The initial patch looks for a "splash.bmp" file in the same directory as firefox and uses it if present; if not, it creates an ugly little red window.  Ahem.

The somewhat convoluted gXRESplashScreen machinery is there to avoid having to touch xpcom or anything else, to make sure that the splash screen can come up as soon as the app is launched (and it does).  The thread is necessary to ensure that events are processed; without it, it often takes a second or two to get the WM_PAINT to the splash screen because the main thread is busy doing startup work.

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: suggested splash.bmp ;)

classic splash screen :)

Even includes space for a progress bar; my code doesn't currently draw one, though.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

* Can we use application.ini to enable/disable the splash screen?
* Can we put the image in chrome/splash? Icons are currently put in chrome/icons/default
* This is a common request from XULRunner app devs, so can we enable it for more than WinCE? Currently the code would work for XP_WIN, I think.
* We have talked about doing this for Maemo too, so a simple Linux based (png?) version would be nice.

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #2)
> * Can we use application.ini to enable/disable the splash screen?
> * Can we put the image in chrome/splash? Icons are currently put in
> chrome/icons/default

So the problem with both of these is that the splash screen is put up literally as soon as the app launches -- if we wait until we've read application.ini and know where to find chrome/splash, we're looking at a few seconds lost already.  I'm not really sure what to do about this.  An easy change is to only display a splash screen only if splash.bmp (or whatever) is present... that way at least people shipping a xulrunner can drop one in if necessary.

> * This is a common request from XULRunner app devs, so can we enable it for
> more than WinCE? Currently the code would work for XP_WIN, I think.

Sure, just the code needs to be written.

> * We have talked about doing this for Maemo too, so a simple Linux based (png?)
> version would be nice.

Yep, same as above.  The code is generic enough, just needs to be implemented.  I'm also not sure whether the actual impl lives in the right spot now, but it's sort of a core XRE piece, so it's probably not too bad.  I should probably add a NS_GetGlobalSplashscreen function rather than a magic symbol, though :)

Comment 4

[WADA:World Anti-bad-Duping Agency]

To Vladimir Vukicevic:
Because of innovation of CPU/OS and SQLite use by Fx 3 and successor, next Tb 3 & Sm 2, current issue is "termination time" rather than "startup time" on usual PC.
Can your implementation be easily ported to "termination splash screen"?
Will your "NS_GetGlobalSplashscreen function" support "termination splash screen"?

By the way, "suggested splash.bmp ;)" is rank:1 splash screen at following site.
> http://www.lotekk.net/index.php?page=moz&sub=splash
> LoTekk.net - mozilla / splash screens
> probably the largest collection of mozilla splashscreen, incl. ranking and voting.
For Firefox, I hope "Foxkeh" will be adopted as standard splash screen ;-)
> http://www.foxkeh.com/

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #4)
> To Vladimir Vukicevic:
> Because of innovation of CPU/OS and SQLite use by Fx 3 and successor, next Tb 3
> & Sm 2, current issue is "termination time" rather than "startup time" on usual
> PC.
> Can your implementation be easily ported to "termination splash screen"?
> Will your "NS_GetGlobalSplashscreen function" support "termination splash
> screen"?

No.  It's a startup splash screen.

Comment 7

[Doug Turner (:dougt)]

vlad: don't really like the the coupling between widget and toolkit, here.  could you listen to an event?   Or register a callback that, if set, widget will call when opening up the first window?

Comment 8

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

No, because there's no cross-platform way of delivering an event to a window and/or listening to one.  There's no coupling with toolkit; the xre startup code lives in toolkit out of convenience, but it's really "startup glue".  I don't see a problem having widget understand a piece of the startup process.  A better approach would be a function call to get the object as opposed to a global though, but it's essentially the same thing.

The problem is that we can't load/execute widget code before opening the splash screen, so there's no way to register the callback.. unless it's done out of band, ignoring xpcom entirely, and have widget provide a NS_RegisterFirstTopLevelWindowShowFunction or something.  That might work, without having to initialize xpcom to call it.

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: an updated patch, just a checkpoint

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: one more updated patch

Comment 11

[Brian Crowder]

What love does this bug need to be landable?

Comment 12

[Brian Crowder]

-#ifdef WINCE
-  ExtractEnvironmentFromCL(argc, argv);
-#endif

How'd this get here?  Is it necessary?

Comment 13

[Brian Crowder]

Tried this on my HTC Touch Pro (with the hunk removal from comment #12 removed).  It's nice, at least it's obvious that something's happening.  The screen is quit a bit too small, though.

Comment 14

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

The screen resizes based on the size of splash.bmp -- there's just no splash.bmp included so it just uses some random default size I picked.  I guess we can make the default size bigger.

The ExtractEnvironment thing can go away, no idea how that made it into this patch.

But basically, that last checkpoint is good to go, I think; perhaps the only change needed is to add an --enable-splashscreen to configure, to define MOZ_SPLASHSCREEN instead of defining it for wince like I do here.

Comment 15

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: updated patch

And updated again.  This adds --enable-splashscreen to configure and makes everything conditional on MOZ_SPLASHSCREEN.  Since it touches xre startup, asking bsmedberg for review, but I think he's out for a few weeks; not sure who another reviewer could be.

Comment 16

[Benjamin Smedberg]

Comment on attachment 373683 [details] [diff] [review]
updated patch

>diff --git a/toolkit/xre/nsSplashScreen.h b/toolkit/xre/nsSplashScreen.h

>+#ifndef _NS_SPLASHSCREEN_H_
>+#define _NS_SPLASHSCREEN_H_

nit, identifiers that begin with an underscore are reserved for compilers and system headers, please use nsSplashScreen_h

>+class nsSplashScreen {
>+public:
>+    // set the dimensions, background image, and other
>+    // bits for this splash screen
>+    virtual void SetSize(int width, int height) { }
>+    virtual void SetBackground(const char *fname) { }
>+    virtual void SetHasProgressBar(PRBool hasProgress) { }
>+    virtual void SetHasMessageLine(PRBool hasMessage) { }

There don't appear to be any callers or implementers of these methods... please remove them.

Making IsOpen() virtual negates any benefit of having it be inline. Can this be a non-virtual method?

>+    /* Update the splash screen to the given progress value (0..100) and
>+     * the given message. */
>+    virtual void Update(PRInt32 progress, const char *message) = 0;

I think this should be UTF8 (which is easy to implement by using CP_UTF8 instead of CP_ACP).

>+extern "C" {
>+    nsSplashScreen *NS_GetSplashScreen(PRBool create);
>+    typedef nsSplashScreen* (*NS_GetSplashScreenPtr) (PRBool);
>+}

No need for this to be an externally-available function: instead make them a pair of static functions within nsSplashScreen:

static nsSplashScreen* Create();
static nsSplashScreen* Get();

>diff --git a/configure.in b/configure.in

> dnl ========================================================
>+dnl Splashscreen
>+dnl ========================================================
>+AC_ARG_ENABLE(splashscreen,
>+              [  --enable-splashscreen   display splashscreen while loading (default=no)],
>+              [enable_splash="yes"],)

nit, Go ahead and add a second enable_splash="" third argument so that mozconfigs can override eachother if necessary.

>diff --git a/toolkit/xre/Makefile.in b/toolkit/xre/Makefile.in

>+ifdef MOZ_SPLASHSCREEN
>+EXPORTS += nsSplashScreen.h

Do we really need to export this? I'd prefer to keep it local and use LOCAL_INCLUDES to get to it.

>diff --git a/toolkit/xre/nsAppRunner.cpp b/toolkit/xre/nsAppRunner.cpp

>+  MOZ_SPLASHSCREEN_UPDATE(10, "Starting Up...");

The progress strings are not localizable. Is it possible to have a splashscreen that has a progress bar but no text, or avoids localizable strings some other way?

>+  MOZ_SPLASHSCREEN_UPDATE(20, "Initializing networking...");

That's a really odd way of saying it.

>+          MOZ_SPLASHSCREEN_UPDATE(70, "Sending startup notification...");

Sending it where? This would make me as a user nervous that you were sending data over the network or something.

Comment 17

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: updated & simplified [Checkin: Comment 19]

Made all those changes... only untranslatable string is "Starting up", the window title; not sure what to do about that.  I can't even get the product name all that easily, since we haven't read app.ini.  Maybe that window title should just be "Mozilla"?

Comment 18

[Brad Lassey [:blassey] (use needinfo?)]

Maybe have "Mozilla" as place holder and provide a callback that nsINIParser can call.

Comment 19

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Checked in; I like the idea of having a callback for nsINIParser to call, but will do that as a followup bug.

Comment 20

[[e-mail]]

Comment on attachment 383307 [details] [diff] [review]
updated & simplified
[Checkin: Comment 19]

>+ifdef MOZ_SPLASHSCREEN
>+ifeq ($(OS_ARCH),WINCE)
Why limit this to CE?

>+#ifdef MOZ_SPLASHSCREEN
>+#define MOZ_SPLASHSCREEN_UPDATE(_i)  do { if (splashScreen) splashScreen->Update(_i); } while(0)
>+#else
>+#define MOZ_SPLASHSCREEN_UPDATE(_i)  do { } while(0)
>+#endif
Could use PR_BEGIN/END_MACRO for readability

>+  // we're about to show the first toplevel window,
>+  // so kill off any splash screen if we had one
We used to have bugs on the splash screen becoming the foreground thread and not releasing activation to the real window. Has that been independently resolved?

Comment 21

[Serge Gautherie (:sgautherie)]

(In reply to comment #19)
> Checked in

http://hg.mozilla.org/mozilla-central/rev/31fd342897c7