692479 - Implement screenshoot based fast startup on Maemo Harmattan

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P5)

Description

[Oleg Romashin (:romaxa)]

For Qt port we are using QGraphicsView as main widget, and that widget appears with some content only after
1) lubxul symbols preloaded (3sec hot, ~10sec cold)
2) XRE started 
3) Widget created and shown

as result for cold startup we can see browser window in ~15 sec, hot startup 6 seconds

Idea here is to create faststartup component which will do next:
1) in nsBrowserApp create faststartup instance which will create QApplication and QGraphicsView
2) start native loop
3) on first paint render awesomebar (from screenshots), and exit from native event loop
4) Load symbols, start Gecko
5) reuse QApplication and QGRaphicsView created in faststartup component for nsAppRunner and nsWindow, drop stuff created in fast startup and replace it with nsIWindow widgets

Result is:
We can see first fennec window with awesomebar in 1-2 seconds
Main XUL content painted in 6-15 seconds depends on cold or hot startup.
wip is available here:
http://hg.mozilla.org/users/romaxa_gmail.com/cross-video-layer/file/default/faststartup.diff

Comment 1

[Oleg Romashin (:romaxa)]

reminder for code which does painting of static UI
http://mxr.mozilla.org/mozilla-central/source/embedding/android/GeckoSurfaceView.java#109

Comment 2

[Oleg Romashin (:romaxa)]

Attachment: Make possible to create MozM/QGraphicsView outside XPCOM

Ok, here is the thing which allow us to create MozQ/MGraphicsView before XPCom started and use it later in nsWidget code.

Comment 3

[Oleg Romashin (:romaxa)]

Attachment: A bit hacky way to create faststartup view

Comment 4

[Oleg Romashin (:romaxa)]

Comment on attachment 565853 [details] [diff] [review]
A bit hacky way to create faststartup view

Not sure what is the right place for startup component, and how good is the current way to implement it... need feedback

Comment 5

[Oleg Romashin (:romaxa)]

Comment on attachment 565853 [details] [diff] [review]
A bit hacky way to create faststartup view

Found that mark was making same thing for android, adding for feedback here too.
without this startup if user opening some link in email, facebook et.c., then fennec starting and starting and starting... 5-10 seconds... 
with these patches fennec static UI shown in < 1 second, and user at least can see that something is going on...
probably make sense to add simple URL text painting...

Comment 6

[John Hammink]

Maemo is strictly low priority.

Comment 7

[Oleg Romashin (:romaxa)]

Attachment: faststartup static UI, simple version

Ok, here is version which using single screenshoot file, showing url text.

Comment 8

[Oleg Romashin (:romaxa)]

Attachment: faststartup static UI for Qt fennec N9

Added simple text ellipsizing...

Comment 9

[Oleg Romashin (:romaxa)]

Attachment: faststartup static UI for Qt fennec N9

Removed bad "/" url suffix.

Comment 10

[Oleg Romashin (:romaxa)]

Comment on attachment 568087 [details] [diff] [review]
faststartup static UI for Qt fennec N9

Dougt could you look at this patch?

Comment 11

[Oleg Romashin (:romaxa)]

Attachment: faststartup static UI for Qt fennec N9

Fixed crash with too early removal of fastWidget...
Disabled double QGLWidget setup

Comment 12

[Doug Turner (:dougt)]

Comment on attachment 568248 [details] [diff] [review]
faststartup static UI for Qt fennec N9

bsmedberg - the new directory, is that okay?

Comment 13

[Oleg Romashin (:romaxa)]

Attachment: faststartup static UI for Qt fennec N9

Here is the version which host new component under widget/qt path

Comment 14

[Doug Turner (:dougt)]

Comment on attachment 569254 [details] [diff] [review]
faststartup static UI for Qt fennec N9

Review of attachment 569254 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/app/nsBrowserApp.cpp
@@ +70,5 @@
>  
>  #include "mozilla/Telemetry.h"
> +#if MOZ_PLATFORM_MAEMO == 6
> +#include "nsFastStartupQt.h"
> +int    gArgc;

doesn't qApp have these?

@@ +204,5 @@
>    XRE_FreeAppData(appData);
>    return result;
>  }
>  
> +bool GeckoPreLoader(const char* execPath, bool doPreload)

why the need to move this stuff here?  I would rather leave this where it was, and just #ifdef your doPreload bit for MAEMO6

::: widget/src/qt/faststartupqt/mozqwidgetfast.cpp
@@ +55,5 @@
> +    return;
> +  }
> +  char *lastSlash = strrchr(exePath, XPCOM_FILE_PATH_SEPARATOR[0]);
> +  if (!lastSlash ||
> +      (lastSlash - exePath > int(MAXPATHLEN - sizeof(XPCOM_DLL) - 1))) {

is this cast needed?

@@ +61,5 @@
> +  }
> +  strcpy(++lastSlash, "/");
> +  QString resourcePath(QString((const char*)&exePath) + QString("res/drawable/"));
> +  mToolbar.load(resourcePath + "toolbar_splash.png");
> +  mIcon.load(resourcePath + "favicon32.png");

nit: These probably should be #defines...

@@ +79,5 @@
> +  int toolbarHeight = 80;
> +  int faviconOffset = 25;
> +  int faviconSize = 32;
> +  float toolbarPartWidth = 77;
> +  int tileWidth = 2;

Where did these numbers come from.  is that what DENSITY_HIGH is all about?

@@ +99,5 @@
> +                       mIcon);
> +  if (!mUrl.isEmpty()) {
> +    float urlHeight = 24.0f;
> +    int urlOffsetX = 80;
> +    int urlOffsetY = 48;

same.  magic numbers need comments.

Comment 15

[Oleg Romashin (:romaxa)]

(In reply to Doug Turner (:dougt) from comment #14)
> Comment on attachment 569254 [details] [diff] [review] [diff] [details] [review]
> faststartup static UI for Qt fennec N9
> 
> Review of attachment 569254 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/app/nsBrowserApp.cpp
> @@ +70,5 @@
> >  
> >  #include "mozilla/Telemetry.h"
> > +#if MOZ_PLATFORM_MAEMO == 6
> > +#include "nsFastStartupQt.h"
> > +int    gArgc;
> 
> doesn't qApp have these?
> 
> @@ +204,5 @@
> >    XRE_FreeAppData(appData);
> >    return result;
> >  }
> >  
> > +bool GeckoPreLoader(const char* execPath, bool doPreload)
> 
> why the need to move this stuff here?  I would rather leave this where it
> was, and just #ifdef your doPreload bit for MAEMO6

I did not want to add XPCOMGlueLoadXULFunctions into faststartup code, otherwise I need add -DXPCOM_GLUE and link libxul with $(XPCOM_STANDALONE_GLUE_LDOPTS) (Glue staf is not linking inside libxul...)

So in order to avoid that, I created callback function which I pass into faststartup code, and that returns back into GLUE browserApp code...


> 
> ::: widget/src/qt/faststartupqt/mozqwidgetfast.cpp
> @@ +55,5 @@
> > +    return;
> > +  }
> > +  char *lastSlash = strrchr(exePath, XPCOM_FILE_PATH_SEPARATOR[0]);
> > +  if (!lastSlash ||
> > +      (lastSlash - exePath > int(MAXPATHLEN - sizeof(XPCOM_DLL) - 1))) {
> 
> is this cast needed?
> 
> @@ +61,5 @@
> > +  }
> > +  strcpy(++lastSlash, "/");
> > +  QString resourcePath(QString((const char*)&exePath) + QString("res/drawable/"));
> > +  mToolbar.load(resourcePath + "toolbar_splash.png");
> > +  mIcon.load(resourcePath + "favicon32.png");
> 
> nit: These probably should be #defines...
> 
> @@ +79,5 @@
> > +  int toolbarHeight = 80;
> > +  int faviconOffset = 25;
> > +  int faviconSize = 32;
> > +  float toolbarPartWidth = 77;
> > +  int tileWidth = 2;
> 
> Where did these numbers come from.  is that what DENSITY_HIGH is all about?
> 
> @@ +99,5 @@
> > +                       mIcon);
> > +  if (!mUrl.isEmpty()) {
> > +    float urlHeight = 24.0f;
> > +    int urlOffsetX = 80;
> > +    int urlOffsetY = 48;
> 
> same.  magic numbers need comments.

Comment 16

[Oleg Romashin (:romaxa)]

Attachment: faststartup static UI for Qt fennec N9

Added some comments, and a bit changed pre-load functions handling... (ifdefed with less changes).
Cast is needed, because there are  unsigned/signed int warning.

Comment 17

[Oleg Romashin (:romaxa)]

Is last version ok?

Comment 18

[Oleg Romashin (:romaxa)]

Try build
https://tbpl.mozilla.org/?tree=Try&rev=d2930fda0287

Comment 19

[Oleg Romashin (:romaxa)]

https://hg.mozilla.org/mozilla-central/rev/a9c6bf5f17df