Closed Bug 839058 Opened 11 years ago Closed 6 years ago

Boot message for apps

Categories

(Firefox OS Graveyard :: General, defect)

Product:
Firefox OS Graveyard
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED WONTFIX

People

(Reporter: kamituel, Unassigned)

Details

(Whiteboard: [games])

Attachments

(1 file, 3 obsolete files)

Changed accordingly to Fabrice's feedback.
7.49 KB, patch
fabrice
: review+
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (Windows NT 6.1) AppleWebKit/537.17 (KHTML, like Gecko) Chrome/24.0.1312.57 Safari/537.17

Steps to reproduce:

We need a way for apps to perform some actions on system boot (on shortly after).
This could be implemented as message broadcasted to Gaia apps (with some permission, I guess). 

This is similar to Android's ACTION_BOOT_COMPLETED broadcast.

This would allow apps to perform various initializations on boot time. In my case this would be calling mozPushNotification.register(), but could be other things as well.
Attached patch Proposed implementation. (obsolete) — — Splinter Review 
Draft of how it could be implemented.
Attachment #713374 - Flags: feedback+
I have several question regarding patch above:
1. Is using component a good idea to send a message to apps?
2. Is "profile-after-change" a good place to do so?
3. If so, where to put this component (for now it lives in /dom, but I suppose it's not the best place to store it)
This patch opens all the apps listening for "bootcomplete" at startup. I'm pretty sure that will lead to terrible performace and user experience on devices.
Do you have some idea how to avoid opening apps, yet still allowing those apps to execute some code?
We need proper background scripts for that. This is on the roadmap I think.
When I talked with Vivien about this, we came to the conslusion that background scripts (bckg services?) wouldn't fit here because they are to heavy. 

With Jonas and Doug we came up with using messages for that purpose.

Oh, and please note that this patch is a "draft" only. If we, by changing PermissionTable, will restrict receiving bootcomplete message to certified apps only, which would limit their number, would this make it acceptable from performance and user experience point of view?
Fabrice: It will only open the apps that have registered that they want to receive the "bootcomplete" message in their manifest, right?

That said, I don't think that what we want is a message that is broadcast on every boot. I think what we need is a message that is sent the first time the device is turned on. This is what we discussed over email.

Or really, to make the message work for 3rd party apps as well, the message should be broadcast to an application as soon as the application is installed and the device is running. For pre-installed app, that is the first time the device is started. For apps installed through a store, it is as soon as the app has been installed.

On subsequent startups there aren't currently a need to send additional system messages.

So I think we should do that and then call the message "installcomplete".
(In reply to Jonas Sicking (:sicking) from comment #7)
> Fabrice: It will only open the apps that have registered that they want to
> receive the "bootcomplete" message in their manifest, right?

yes
 
> So I think we should do that and then call the message "installcomplete".

I guess that could work, but I'm still not convinced that it's a good UX.
The idea is that there is no UI involved. Just the application initializing itself. So I don't think the user will notice anything.

We do have the problems that applications started through system messages, but never display UI, still end up in the card-deck. But I think that's a separate issue.
Well, from my point of view Jonas' solution with "installcomplete" message, ie. message which would be called once for an app, is fine, as long as push registration is indeed persistent across device reboots (and it's about to stay this way).

But if we can think of a use case where app should be able to run some code at every boot, we could do implement this right away. From the top of my head I can think of IM app - it would want to, at device boot, notify server that user changed status to "available". They do not necessarily need background script for that as there is no reason for it to run at all times.
Implementing a "device booted" message might be useful for other apps yes. But that's not what should be used for the remote installer since we shouldn't wake that up on every boot, but rather just the first one.

So either way we should add a "installcomplete" message here.
That's fine by me. Shall I proceed and fix this patch to implement "installcomplete", so we can have more concrete idea how it would look like?

Fabrice: is it, in your opinion, acceptable?
'install-complete' message implementation. 

Message is sent at first boot or after device update. App has to add 'install-complete' message handler in manifest, as well as 'install-complete' permission.

Only certified apps are supported.

Code lives in dom/apps/src/Webapps.jsm, because it seemed to be more natural place that creating separate module for that.

Jonas, Fabrice - does this code make sense?

If this code looks ok, we can discuss enhancing it with support for trusted apps - it looks to be straightforward, I'd like to do it.
Attachment #713374 - Attachment is obsolete: true
Attachment #716512 - Flags: review+
Attachment #716512 - Flags: feedback+
Attachment #716512 - Flags: review+ → review?(fabrice)
Comment on attachment 716512 [details] [diff] [review]
install-complete message for certified apps

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

::: dom/apps/src/Webapps.jsm
@@ +22,4 @@
>  Cu.import("resource://gre/modules/AppDownloadManager.jsm");
>  
>  function debug(aMsg) {
> +  dump("-*-*- Webapps.jsm : " + aMsg + "\n");

Nit: add the comment back in.

@@ +233,5 @@
> +        return;
> +      }
> +
> +      let msg = messages.filter(having('install-complete'))[0];
> +      if ( msg == undefined ) return;

Nit: if (msg == undefined) {
  return;
}

@@ +234,5 @@
> +      }
> +
> +      let msg = messages.filter(having('install-complete'))[0];
> +      if ( msg == undefined ) return;
> +      let page = app.origin + msg['install-complete'];

You can't build the URI like that, you need to do a proper relative uri resolution. See how this is done at https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#446

@@ +458,5 @@
> +      if (runUpdate) {
> +        for (let id in this.webapps) {
> +          this.sendInstallCompleteMessageForApp(id);
> +        }
> +      }

Hm, didn't we decide to do that only after app install, and not at each update?
Attachment #716512 - Flags: review?(fabrice) → review-
Fabrice, isn't 'runUpdate' a flag which is set when the handset is first booted? (I think 'install-complete' won't be called after each boot).
(In reply to Kamil Leszczuk from comment #15)
> Fabrice, isn't 'runUpdate' a flag which is set when the handset is first
> booted? (I think 'install-complete' won't be called after each boot).

Yes, that's correct. But we can't have a message name "install-complete" and make it fire at first boot and after a gecko update. My understanding of comment #11 is that we want that to fire a single time, after the app is installed.
> My understanding of
> comment #11 is that we want that to fire a single time, after the app is
> installed.

Yes, true. This patch does this for certified apps only, correct?

Should I go and do this for non-certified apps as well?
(In reply to Kamil Leszczuk from comment #17)
> > My understanding of
> > comment #11 is that we want that to fire a single time, after the app is
> > installed.
> 
> Yes, true. This patch does this for certified apps only, correct?

This patch does not do that. It fires the system message for all certified apps that listen for this message every time we update gecko+gaia, not just once. If you are on the nightly update channel, it will start the app every day.

> Should I go and do this for non-certified apps as well?

We may want that for privileged apps, but we that can be changed later.
Fabrice, I'm looking into your remarks.

In order for 'installcomplete' to be sent once per app, and not after gecko update, I can't see any simple way of putting this together.

My first approach (using 'runUpdate' flag to decide wheter to send message or not) cannot be used here. Is there another easy way of determining whether a message has been sent to an app?

If not, I can see two ways of dealing with it:

1. Use system preferences (Services.prefs) to persist a flag after sending this message to certifed apps. This could look like: 
  
  if ( runUpdate && flagInPrefsSet() ) {
    for (let id in this.webapps) {
      this.sendInstallCompleteMessageForApp(id);
    }
  }

This approach would not fit in a scenario where 'installcomplete' is available for non-certified apps.

2. Implement some simple DB ('InstallCompleteDB') and persist there information which app received 'installcomplete' message already. 

This is more complex than approach 1, but it would be fine when 'installcomplete' is enabled for non-certified apps.

What do you think?
This seems like information that should be stored in the application registry.
Second interation of the patch. 'install-complete' is sent once per each interested, certified app. 

This adds 'installCompleteSent' to the application registry (Jonas, thanks for the hint!).

I have also tested this for non-certified apps. It works as expected. To enable, uncomment "this.sendInstallCompleteMessageForApp(id);" in 'confirmInstall()' (and allow permission in PermissionsTable.jsm). 
I am not sure about the 'aFromSync' flag though - is my code in the right place in 'confirmInstall()'?.

App would require in the manifest:

  "permissions": {
    "install-complete":{}
  },

and:

  "messages": [
     { "install-complete": "/receiver.html" }
  ]
Attachment #716512 - Attachment is obsolete: true
Attachment #728179 - Flags: review?(fabrice)
I don't think that we need a permission to enable this feature.
Jonas, how about a scenario when 'install-complete' is enabled for non-certified apps - wouldn't we want for it to be permission-protected in order to avoid automatic launching of an app without user consent?
Comment on attachment 728179 [details] [diff] [review]
'install-complete' for certified apps.

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

There are still a few issues with this patch, sorry if I was not clear in my previous review.
- You are reloading the manifest to check for the new system message, but you don't do it fully correctly. Look at what _registerSystemMessagesForEntryPoint() is doing for details. Since we don't want to replicate all this code, I think the cleanest solution is to add a method to nsISystemMessagesInternal that would return the pageURL(s) for an app if the app has a given system message registered. The IDL is at https://mxr.mozilla.org/mozilla-central/source/dom/messages/interfaces/nsISystemMessagesInternal.idl

- We need to send this message (if it has never been delivered) in the following cases:
 * after an update, in _processManifestForIds().
 * when we finish the installation of a hosted app, in confirmInstall alongside |this.updateAppHandlers(null, app.manifest, app);|
 * when we finish the installation of a packaged app, also in confirmInstall, after |this.updateAppHandlers(null, aManifest, appObject);|

I don't have a strong opinion on whether we want a permission that or not. Since Jonas is ok with not having a permission, let's remove it. We can still only let privileged and certified applications use that if we want.
Attachment #728179 - Flags: review?(fabrice) → review-
we're fine without a permission still
Hi, this is a code based on Fabrice's remarks. It implements 'install-complete' for certified, packaged and web apps.
Attachment #728179 - Attachment is obsolete: true
Attachment #739458 - Flags: review?(fabrice)
Comment on attachment 739458 [details] [diff] [review]
Changed accordingly to Fabrice's feedback.

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

r=me with nits addressed.

::: dom/apps/src/Webapps.jsm
@@ +445,5 @@
> +      return;
> +    }
> +
> +    let manifestURL = Services.io.newURI(aApp.manifestURL, null, null);
> +    let pages = msgmgr.getPagesForMessage('install-complete', manifestURL);

nit: we use double quotes for strings in this file.

@@ +448,5 @@
> +    let manifestURL = Services.io.newURI(aApp.manifestURL, null, null);
> +    let pages = msgmgr.getPagesForMessage('install-complete', manifestURL);
> +    pages.forEach(function (aPageURISpec) {
> +      let pageURL = Services.io.newURI(aPageURISpec, null, null);
> +      msgmgr.sendMessage('install-complete', {}, pageURL, manifestURL);

idem

::: dom/messages/SystemMessageInternal.js
@@ +75,5 @@
> +      return [];
> +    }
> +
> +    let matching = function (aPage) {
> +      return (aPage.type === aType && aPage.manifest === aManifestURI.spec);    

nit: trailing whitespace

@@ +76,5 @@
> +    }
> +
> +    let matching = function (aPage) {
> +      return (aPage.type === aType && aPage.manifest === aManifestURI.spec);    
> +    };    

idem

::: dom/messages/interfaces/nsISystemMessagesInternal.idl
@@ +8,5 @@
>  interface nsIDOMWindow;
>  
>  // Implemented by the contract id @mozilla.org/system-message-internal;1
>  
>  [scriptable, uuid(d8de761a-94fe-44d5-80eb-3c8bd8cd7d0b)]

You need to change the uuid since you modify the interface.

@@ +11,5 @@
>  
>  [scriptable, uuid(d8de761a-94fe-44d5-80eb-3c8bd8cd7d0b)]
>  interface nsISystemMessagesInternal : nsISupports
>  {
> +  /* 

nit: trailing whitespace

@@ +12,5 @@
>  [scriptable, uuid(d8de761a-94fe-44d5-80eb-3c8bd8cd7d0b)]
>  interface nsISystemMessagesInternal : nsISupports
>  {
> +  /* 
> +   * Return URI's for all pages registered for a given message type, 

idem
Attachment #739458 - Flags: review?(fabrice) → review+
Note: this feature would be highly valuable for the games project in conjunction with bug 929236.  An app will be able to use the 'install-complete' message to trigger compilation/caching of asm.js so that the first-run experience gets a cache hit (compilation can take 20s on mobile, so this is a big perception improvement).

Question: does the installation UI wait for for the handling of 'install-complete' to finish before visually indicating that installation is complete?  This would be really nice, but judging from the comments, I suspect not.  Assuming 'no': what will happen if the user runs the app while the 'install-complete' message is still being handled?
Whiteboard: [games]
(In reply to Luke Wagner [:luke] from comment #28)
> 
> Question: does the installation UI wait for for the handling of
> 'install-complete' to finish before visually indicating that installation is
> complete?  This would be really nice, but judging from the comments, I
> suspect not.  Assuming 'no': what will happen if the user runs the app while
> the 'install-complete' message is still being handled?

No, the installation UI has no idea that we are sending this 'install-complete' message, and even less when the process has ended. If the user starts the app while going through the install-complete process, I think that currently gaia will only focus the app iframe, considering that the app is already running. That's obviously not ideal. I really wonder if you should not rather use a manifest whitelist instead for your asm.js caching.
(In reply to Fabrice Desré [:fabrice] from comment #29)
That would be a fallback option, but it seems more ideal to provide this extra functionality to apps since there are any number of first-run-only things that an app might want to move to installation to improve the first-run performance.
Firefox OS is not being worked on
Status: UNCONFIRMED → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: