Implement screenshoot based fast startup on Maemo Harmattan

RESOLVED FIXED

Status

P5
normal
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: romaxa, Assigned: romaxa)

Tracking

Trunk
ARM
Maemo

Details

(Whiteboard: [QA-])

Attachments

(1 attachment, 7 obsolete attachments)

(Assignee)

Description

7 years ago
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
(Assignee)

Comment 2

7 years ago
Created attachment 565852 [details] [diff] [review]
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.
(Assignee)

Comment 3

7 years ago
Created attachment 565853 [details] [diff] [review]
A bit hacky way to create faststartup view
(Assignee)

Comment 4

7 years ago
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)
(Assignee)

Comment 5

7 years ago
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)

Comment 6

7 years ago
Maemo is strictly low priority.
Priority: -- → P5
(Assignee)

Updated

7 years ago
Depends on: 693422
Whiteboard: [QA-]
(Assignee)

Comment 7

7 years ago
Created attachment 566357 [details] [diff] [review]
faststartup static UI, simple version

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)
(Assignee)

Comment 8

7 years ago
Created attachment 566373 [details] [diff] [review]
faststartup static UI for Qt fennec N9

Added simple text ellipsizing...
Attachment #566357 - Attachment is obsolete: true
Attachment #566373 - Flags: review?(mark.finkle)
Attachment #566357 - Flags: review?(mark.finkle)
(Assignee)

Comment 9

7 years ago
Created attachment 568087 [details] [diff] [review]
faststartup static UI for Qt fennec N9

Removed bad "/" url suffix.
Attachment #566373 - Attachment is obsolete: true
Attachment #566373 - Flags: review?(mark.finkle)
Attachment #568087 - Flags: review?(mbrubeck)
(Assignee)

Comment 10

7 years ago
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)
(Assignee)

Comment 11

7 years ago
Created attachment 568248 [details] [diff] [review]
faststartup static UI for Qt fennec N9

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)
(Assignee)

Comment 13

7 years ago
Created attachment 569254 [details] [diff] [review]
faststartup static UI for Qt fennec N9

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-
(Assignee)

Comment 15

7 years ago
(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.
(Assignee)

Comment 16

7 years ago
Created attachment 569279 [details] [diff] [review]
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.
Attachment #569254 - Attachment is obsolete: true
Attachment #569279 - Flags: review?(doug.turner)
(Assignee)

Comment 17

7 years ago
Is last version ok?
Attachment #569279 - Flags: review?(doug.turner) → review+
(Assignee)

Comment 19

7 years ago
https://hg.mozilla.org/mozilla-central/rev/a9c6bf5f17df
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.