Closed Bug 692479 Opened 8 years ago Closed 8 years ago

Implement screenshoot based fast startup on Maemo Harmattan

Categories

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

ARM
Maemo
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: romaxa, Assigned: romaxa)

References

Details

(Whiteboard: [QA-])

Attachments

(1 file, 7 obsolete files)

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
Ok, here is the thing which allow us to create MozQ/MGraphicsView before XPCom started and use it later in nsWidget code.
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
Attachment #565853 - Flags: feedback?(doug.turner)
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...
Attachment #565853 - Flags: feedback?(mark.finkle)
Maemo is strictly low priority.
Priority: -- → P5
Depends on: 693422
Whiteboard: [QA-]
Ok, here is version which using single screenshoot file, showing url text.
Assignee: nobody → romaxa
Attachment #565852 - Attachment is obsolete: true
Attachment #565853 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #565853 - Flags: feedback?(mark.finkle)
Attachment #565853 - Flags: feedback?(doug.turner)
Attachment #566357 - Flags: review?(mark.finkle)
Added simple text ellipsizing...
Attachment #566357 - Attachment is obsolete: true
Attachment #566357 - Flags: review?(mark.finkle)
Attachment #566373 - Flags: review?(mark.finkle)
Removed bad "/" url suffix.
Attachment #566373 - Attachment is obsolete: true
Attachment #566373 - Flags: review?(mark.finkle)
Attachment #568087 - Flags: review?(mbrubeck)
Comment on attachment 568087 [details] [diff] [review]
faststartup static UI for Qt fennec N9

Dougt could you look at this patch?
Attachment #568087 - Flags: review?(doug.turner)
Fixed crash with too early removal of fastWidget...
Disabled double QGLWidget setup
Attachment #568087 - Attachment is obsolete: true
Attachment #568087 - Flags: review?(mbrubeck)
Attachment #568087 - Flags: review?(doug.turner)
Attachment #568248 - Flags: review?(doug.turner)
Comment on attachment 568248 [details] [diff] [review]
faststartup static UI for Qt fennec N9

bsmedberg - the new directory, is that okay?
Attachment #568248 - Flags: review?(benjamin)
Here is the version which host new component under widget/qt path
Attachment #568248 - Attachment is obsolete: true
Attachment #568248 - Flags: review?(doug.turner)
Attachment #568248 - Flags: review?(benjamin)
Attachment #569254 - Flags: review?(doug.turner)
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.
Attachment #569254 - Flags: review?(doug.turner) → review-
(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.
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.
Attachment #569254 - Attachment is obsolete: true
Attachment #569279 - Flags: review?(doug.turner)
Is last version ok?
Attachment #569279 - Flags: review?(doug.turner) → review+
https://hg.mozilla.org/mozilla-central/rev/a9c6bf5f17df
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.