1037329 - [b2g] Implement SystemUpdate WebAPI

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

We need an API to fetch and apply system update package and allow partners to implement their own update protocol.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I think fabrice has more thinking than me here, but I agree that this is definitely something that we need. The two goals I'd like to solve are:

1. Enable OEMs to land their own update code in Gecko.
2. Remove the need for the Settings API hacks we currently have for signaling things like
    * Is an update available to be downloaded
    * Allow gaia to start the download
    * Progress during download
    * Allow gaia to start installation

I'd really like to land the various OEM update code directly in Gecko. Possibly in different file which we can use build-time selection to select which files to build.

Comment 2

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: [WIP] system-update-api.patch

register update provider via category while start-up.

Comment 3

[(no longer active)]

Thanks Shih-Chiang, for working on this!

Comment 4

[[:fabrice] Fabrice Desré]

Comment on attachment 8462327 [details] [diff] [review]
[WIP] system-update-api.patch

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

I didn't look at everything in detail yet, but overall this looks good! A few things:
- do you really hate comments? :P
- I'm not sure how to protect setActiveProvider(). If two apps have the permission to use this api, nothing prevents app1 to set a provider1, and app2 to change to provider2 while provider1 is actually doing something. I think we should reject the promise in this case.
- it's fine to do that in a followup, but we'll need an implementation exposing our update system.

::: dom/system/SystemUpdateManager.js
@@ +146,5 @@
> +      uuid: self._provider.uuid 
> +    });
> +  },
> +  setParameter: function(name, value) {
> +    debug('setParameter: ' + name + ' = ' + value);

nit: double quotes in this file.

::: dom/system/SystemUpdateService.jsm
@@ +167,5 @@
> +      }
> +    }
> +
> +    if (!providerInfo) {
> +      aData.error = "no active provider";

I would rather use CamelCase for error code, like NoActiveProvider, and use them when rejecting the promise.

::: dom/system/nsISystemUpdateProvider.idl
@@ +30,5 @@
> +  void applyUpdate();
> +  bool setParameter(in DOMString name, in DOMString value);
> +  DOMString getParameter(in DOMString name);
> +
> +  void hook(in nsISystemUpdateListener listener);

s/hook/setListener (or addListener if we need to allow multiple listeners)

@@ +31,5 @@
> +  bool setParameter(in DOMString name, in DOMString value);
> +  DOMString getParameter(in DOMString name);
> +
> +  void hook(in nsISystemUpdateListener listener);
> +  void unhook();

unsetListener()

::: dom/system/nsISystemUpdateRegistry.idl
@@ +8,5 @@
> +interface nsISystemUpdateRegistry : nsISupports
> +{
> +  readonly attribute unsigned long providerCount;
> +  jsval itemAt(in unsigned long index);
> +};

I don't think we need that interface. Make the service implement nsIVariant and that will be your array.

::: dom/webidl/SystemUpdate.webidl
@@ +16,5 @@
> +};
> +
> +[JSImplementation="@mozilla.org/system-update-provider;1",
> + CheckPermissions="settings",
> + Pref="dom.system-update.enabled"]

hm, do we need that? we will have N provider implementations with their own contract ID, right?

Comment 5

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Fabrice Desré [:fabrice] from comment #4)
> Comment on attachment 8462327 [details] [diff] [review]
> [WIP] system-update-api.patch
> 
> Review of attachment 8462327 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I didn't look at everything in detail yet, but overall this looks good! A
> few things:
> - do you really hate comments? :P
Sorry I was rushing to provide a reference design to unblock our partner. I'll add more necessary comments before submitting for review. :)
> - I'm not sure how to protect setActiveProvider(). If two apps have the
> permission to use this api, nothing prevents app1 to set a provider1, and
> app2 to change to provider2 while provider1 is actually doing something. I
> think we should reject the promise in this case.
That's a good question, I'll think about when to lock/unlock the active provider.
> - it's fine to do that in a followup, but we'll need an implementation
> exposing our update system.
Exposing our update system is quit simple, we only need to implement nsISystemUpdateProvider in UpdatePrompt.js. I'll include this part on this bug, but we'll definitely need follow-up bugs for gaia modification and removal of mozChromeEvent/mozContentEvent/mozSettings in UpdatePrompt.js.
> 
> ::: dom/system/nsISystemUpdateProvider.idl
> @@ +30,5 @@
> > +  void applyUpdate();
> > +  bool setParameter(in DOMString name, in DOMString value);
> > +  DOMString getParameter(in DOMString name);
> > +
> > +  void hook(in nsISystemUpdateListener listener);
> 
> s/hook/setListener (or addListener if we need to allow multiple listeners)
My design only allow single listener so I'll use |setListener/unsetListener|.
> 
> ::: dom/system/nsISystemUpdateRegistry.idl
> @@ +8,5 @@
> > +interface nsISystemUpdateRegistry : nsISupports
> > +{
> > +  readonly attribute unsigned long providerCount;
> > +  jsval itemAt(in unsigned long index);
> > +};
> 
> I don't think we need that interface. Make the service implement nsIVariant
> and that will be your array.
Will try, thanks for the advice!
> 
> ::: dom/webidl/SystemUpdate.webidl
> @@ +16,5 @@
> > +};
> > +
> > +[JSImplementation="@mozilla.org/system-update-provider;1",
> > + CheckPermissions="settings",
> > + Pref="dom.system-update.enabled"]
> 
> hm, do we need that? we will have N provider implementations with their own
> contract ID, right?
This contract ID doesn't map to any component, I was trying to make WebIDL codegen not look for C++ source files. BTW, SystemUpdateProvider is act as a proxy and it has only one implementation in SystemUpdateManager.js. 3rd party will hook their implementation via nsISystemUpdateProvider XPIDL interface.

Comment 6

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: System Update architecture.png

Here is the architecture I have in current WIP.

Comment 7

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 1 - system-update-api.patch

update according to @fabrice's feedback.

Comment 8

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: [WIP] Par 2 - enable-system-update-on-b2g.patch

Comment 9

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 3 - system-update-testcase.patch

Comment 10

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

UpdatePrompt.js uses two different methods to interact with System app and Settings app. Definitely need a Gaia developer who is familiar with System Update to make sure the WebIDL exposes enough information for our UI design.

Comment 11

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Thanks to @arthur, the corresponding UX spec is in bug 1034735 attachment 8453638 [details]. To accommodate the current UX spec we'll need to address the following usages in the WebIDL:

1. Notify system app while switching update provider, in order to prompt for the following events.
2. Error handling for each stage (idle/checking/downloading/applying).

Comment 12

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 1 - system-update-api.patch, v2

rebase my patch with following modification:
1. merge with system-update-testcase.patch
2. remove SystemUpdateRegistry for simplicity.
3. add system-update permission
4. fix SystemUpdateManager uninit procedure
5. nit addressed.

Comment 13

[kang hee don]

I adapted two attached patches part1 and part2 into B2G 2.2.0.0-prelease. but I got an error "navigator.updateManager is undefined"
I thought SystemUpdate.wdbidl was build correctly, because I checked "SystemUpdateBinding.cpp" and "SystemUpdateBinding.h" files was created in build out folder.

I didn't use test_system_update_enabled.html but call navigator.updateManager.getActiveProvider() in gaia app.      

How can i adapt your patches?

Comment 14

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to kang hee don from comment #13)
> I adapted two attached patches part1 and part2 into B2G 2.2.0.0-prelease.
> but I got an error "navigator.updateManager is undefined"
> I thought SystemUpdate.wdbidl was build correctly, because I checked
> "SystemUpdateBinding.cpp" and "SystemUpdateBinding.h" files was created in
> build out folder.
> 
> I didn't use test_system_update_enabled.html but call
> navigator.updateManager.getActiveProvider() in gaia app.      
> 
> How can i adapt your patches?

You'll also need to add "system-update" permission in app manifest.

Comment 15

[kang hee don]

Thanks for shih-Chiang Chien's comment.
As shih-Chiang Chien's comment, I added a permission to communication app.
After that I got a new error like as "NS_ERROR_FACTORY_NOT_REGISTERED".

Below is a code I added
  var activeProvider = navigator.updateManager.getActiveProvider();

Which code should I check?

first of all I'd like to analyze patches, and then I'll make a oem update provider.

Comment 16

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: [WIP] Part 2 - enable-system-update-on-b2g.patch

fix the issue mentioned in comment #15.

Hi Kang Hee Don, thanks for your information. I'm glad to hear 3rd-party update provider adapting our update architecture at this early stage. Feel free to raise any questions on the API usage/design because it's not finalized yet.

Comment 17

[kang hee don]

As I know, in FFOS V2.0, SystemUpdate procedure had a interface between Gaia setting app and Gaia system app (update_manager.js, updatable.js).

Will you maintain above interface in feature?

I'd like to know webapi usage/design in aspect of gaia and gecko layer.

Will system update webapi be called in system app as like FFOS v2.0?

Comment 18

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to kang hee don from comment #17)
> As I know, in FFOS V2.0, SystemUpdate procedure had a interface between Gaia
> setting app and Gaia system app (update_manager.js, updatable.js).
> 
> Will you maintain above interface in feature?
> 
> I'd like to know webapi usage/design in aspect of gaia and gecko layer.
> 
> Will system update webapi be called in system app as like FFOS v2.0?
Yes, System App and Settings App are the target user of SystemUpdate API. We are going to get rid of those mozChromeEvent/mozContentEvent/mozSettings hacks and to allow OEM partners to plugin their own protocol between device and update server.

Comment 19

[kang hee don]

If you don't mind, Can I get any document that explain a SystemUpdate procedure between Settings App and System app?
The attached image didn't explain any procedure between Settings App and System.

That's a very important point, for my working, I will need to add some web api.

Comment 20

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Here is the document I have in hand.
https://docs.google.com/presentation/d/11Bco460JwmUUDg3itwOI0QsnkvSDicT1Bk5bi2gZ2IU/edit?usp=sharing

Comment 21

[kang hee don]

I thought these patches were based on b2g 2.2-prerelease.

When will these patches be landed?
I'd like to determine the version of b2g for starting my work.

Comment 22

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

No, these are based on mozilla-central. Currently there is no ETA to land these patch because we haven't arrange the resource to migrate Mozilla updater.

Comment 23

[James Cheng[:JamesCheng]]

Attachment: Part 1 - system-update-api.patch

Rebase the previous patch and assign myself to follow-up.
Fix some issue about the error cases handling. Notify Gaia by onerror event.

Comment 24

[James Cheng[:JamesCheng]]

Attachment: Part 2 - enable-system-update-on-b2g.patch

Rebase the previous patch.

Comment 25

[James Cheng[:JamesCheng]]

Comment on attachment 8603207 [details] [diff] [review]
Part 1 - system-update-api.patch

Hello Ehsan,
Please help to review this patch for implementing SystemUpdate API.
Thanks.

Comment 26

[James Cheng[:JamesCheng]]

Comment on attachment 8603208 [details] [diff] [review]
Part 2 - enable-system-update-on-b2g.patch

Hello Fabrice,

Please help to review this patch for enable system update API on b2g.

Thanks.

Comment 27

[James Cheng[:JamesCheng]]

Attachment: Part 1 - system-update-api.patch

Specify the test case which only runs on b2g.

Comment 28

[James Cheng[:JamesCheng]]

Attachment: Part 1 - system-update-api.patch

Dynamically add the mock provider when running the test case on b2g.

and attach the try result for reference:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c190cfd62fe5

Hi Ehsan, please help to review this patch.

Thank you very much.

Comment 29

[(no longer active)]

I'll try to get to this later this week...

Comment 30

[James Cheng[:JamesCheng]]

Hi Ehsan, 
For Bug 1161927, the patch is ready and migrate to our proposed APIs.

It seems the APIs are sufficient for update procedure.

Please help us to review the patch if you have available time.

Thanks for your help.

Comment 31

[(no longer active)]

Sorry, will try to look at it this week.

Comment 32

[[:fabrice] Fabrice Desré]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #31)
> Sorry, will try to look at it this week.

Same for me...

Comment 33

[James Cheng[:JamesCheng]]

Hello Ehsan and Fabrice, 

We planned to finish the review process before the end of June.

Please help us for the first review of these patches.

Thank you.

Comment 34

[James Cheng[:JamesCheng]]

Hi Ehsan and Fabrice,

We hope the review process can be completed before end of June,

Could you please do a first review for the patches or is there any suggested reviewer if you are very busy right now.

Thanks.

Comment 35

[[:fabrice] Fabrice Desré]

Comment on attachment 8603208 [details] [diff] [review]
Part 2 - enable-system-update-on-b2g.patch

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

I'd like to take another look once my comments are addressed.

Also, with this patch we end up with the old mozChromeEvent/mozContentEvent way of managing updates *and* this api. Can you file bugs to:
1) Update gaia to use the new api.
2) Remove the old mozChromeEvent/mozContentEvent mechanism from gecko.

::: b2g/app/b2g.js
@@ +1102,5 @@
>  // BroadcastChannel API
>  pref("dom.broadcastChannel.enabled", true);
>  
> +// SystemUpdate API
> +pref("dom.system-update.enabled", true);

we usually use snake case for prefs: dom.system_update.enabled

@@ +1104,5 @@
>  
> +// SystemUpdate API
> +pref("dom.system-update.enabled", true);
> +#ifdef MOZ_UPDATER
> +pref("dom.system-update.active", "@mozilla.org/updates/update-prompt;1");

Please move that in the already existing #ifdef MOZ_UPDATER block.

::: b2g/components/UpdatePrompt.js
@@ +84,5 @@
>  
>      if (updateCount == 0) {
>        this._updatePrompt.setUpdateStatus("no-updates");
> +
> +      if(this._updatePrompt._updateListener) {

nit: |if (| but you can also write:
this._updatePrompt._updateListener && this._updatePrompt._updateListener.onError(...)

@@ +95,5 @@
>      let update = Services.aus.selectUpdate(updates, updateCount);
>      if (!update) {
>        this._updatePrompt.setUpdateStatus("already-latest-version");
> +
> +      if(this._updatePrompt._updateListener) {

same here

@@ +154,5 @@
>    _update: null,
>    _applyPromptTimer: null,
>    _waitingForIdle: false,
>    _updateCheckListner: null,
> +  _updateListener: null,

Please rename to _systemUpdateListener

@@ +166,5 @@
> +
> +  // nsISystemUpdateProvider
> +  checkForUpdate: function() {
> +    this.forceUpdateCheck();
> +  },

Add a blank line between functions

@@ +176,5 @@
> +  },
> +  applyUpdate: function() {
> +    this.handleApplyPromptResult({result: "restart"});
> +  },
> +  setParameter: function(name, value) {

nit : (aName, aValue)

@@ +195,5 @@
> +    }
> +
> +    return true;
> +  },
> +  getParameter: function(name) {

nit: aName

@@ +196,5 @@
> +
> +    return true;
> +  },
> +  getParameter: function(name) {
> +    return this._availableParameters[name];

do we want to return `undefined` or `null` for incorrect parameters? I tend to prefer `null`

@@ +198,5 @@
> +  },
> +  getParameter: function(name) {
> +    return this._availableParameters[name];
> +  },
> +  setListener: function(listener) {

nit: aListener

@@ +199,5 @@
> +  getParameter: function(name) {
> +    return this._availableParameters[name];
> +  },
> +  setListener: function(listener) {
> +    this._updateListener = listener;

If an update is available or ready, we should trigger the event right away at this point.

@@ +201,5 @@
> +  },
> +  setListener: function(listener) {
> +    this._updateListener = listener;
> +  },
> +  unsetListener: function(listener) {

nit: aListener

@@ +226,5 @@
>    checkForUpdates: function UP_checkForUpdates() { },
>  
>    showUpdateAvailable: function UP_showUpdateAvailable(aUpdate) {
> +    if (this._updateListener) {
> +      //XXX need the list of required package information with Gaia dev

please remove this comment.

Comment 36

[James Cheng[:JamesCheng]]

Attachment: Part 2 - enable-system-update-on-b2g.patch

Hi Fabrice,

I fired Bug 1172244 for removing mozChrome and Content Event,

and the Bug 1161927 "Migrate settings app to use new system update API." has finished and will start to review once this bug is landed.

I have revised the patch as your comment mentioned. Please help to continue the review process.

Thank you very much.

Comment 37

[James Cheng[:JamesCheng]]

Attachment: Part 2 - enable-system-update-on-b2g.patch

Sorry for fixing a typo, please review this patch, thanks

Comment 38

[James Cheng[:JamesCheng]]

Attachment: Part 1 - system-update-api.patch

Hi Ehsan,

As Fabrice commented : use snake case for prefs: dom.system_update.enabled.

I also need to modify this patch.

Please review this new one.

Thank you very much.

Comment 39

[(no longer active)]

Comment on attachment 8616404 [details] [diff] [review]
Part 1 - system-update-api.patch

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

Apologies for the long delay here.

I reviewed the test and the API for now, and I would like to ask for some changes to the API.  I haven't reviewed the implementation yet since that will change when you address my comments on the API...

::: dom/system/tests/test_system_update_enabled.html
@@ +46,5 @@
> +        targetProvider = provider;
> +        break;
> +      }
> +    }
> +    info('provider uuid: ' + targetProvider.uuid);

This doesn't actually test that you have received the provider you expect.

@@ +75,5 @@
> +      ok(true, 'receive updateavailable event');
> +      info('event: ' + JSON.stringify(event.detail));
> +      resolve(provider);
> +    });
> +    provider.checkForUpdate();

I think it would be much nicer if checkForUpdate() returned a promise that would resolve when the check was finished.  That way we wouldn't need the updateavailable event.

@@ +88,5 @@
> +      is(event.loaded, 10, 'expected loaded');
> +      is(event.total, 100, 'expected total');
> +      resolve(provider);
> +    });
> +    provider.startDownload();

Similarly, it would be nicer if startDownload() would return a promise that would resolve when the download was successfully finished, and rejected otherwise.  We should keep the progress event though, since promises cannot signal their progress right now.

@@ +94,5 @@
> +}
> +function testStopDownload(provider) {
> +  info('test StopDownload');
> +  return new Promise(function(resolve, reject) {
> +    provider.stopDownload();

Does stopDownload actually stop the download immediately?  Or is it asynchronous?  If it's async then stopDownload() should return a promise that gets resolved when the download is actually stopped.

@@ +101,5 @@
> +}
> +function testApplyUpdate(provider) {
> +  info('test ApplyUpdate');
> +  return new Promise(function(resolve, reject) {
> +    provider.applyUpdate();

Same as above.  I don't think applyUpdate() can possibly be synchronous.

::: dom/webidl/SystemUpdate.webidl
@@ +10,5 @@
> +dictionary SystemUpdatePackageInfo {
> +  DOMString type;
> +  DOMString version;
> +  DOMString description;
> +  Date buildDate;

Please use DOMTimeStamp instead of Date.

@@ +19,5 @@
> + CheckPermissions="system-update",
> + Pref="dom.system_update.enabled"]
> +interface SystemUpdateProvider : EventTarget {
> +  //SystemUpdateProviderInfo
> +  readonly attribute any info;

Instead of this, can we just add name and uuid as properties of SystemUpdateProvider?  It seems better than this.

@@ +31,5 @@
> +  void startDownload();
> +  void stopDownload();
> +  void applyUpdate();
> +  boolean setParameter(DOMString name, DOMString value);
> +  DOMString getParameter(DOMString name);

What are these parameters supposed to be?  What is their use case?  Can we add APIs for what these are expected to do?  Or document them?  Right now, it's impossible to tell what these are used for.

::: modules/libpref/init/all.js
@@ +4775,5 @@
>  // platforms; and set to 0 to disable the low-memory check altogether.
>  pref("camera.control.low_memory_thresholdMB", 404);
>  #endif
>  
> +// SysteUpdate API

Typo here.

Comment 40

[James Cheng[:JamesCheng]]

Attachment: Part 2 - enable-system-update-on-b2g.patch

Find treeherder error and fixed as

https://treeherder.mozilla.org/#/jobs?repo=try&revision=23f3579452d3

Attach the new patch.

Thanks.

Comment 41

[James Cheng[:JamesCheng]]

Hi Ehsan,

After discussing with Gaia developer,

We would like to use event based API instead of Promise for some reasons.

1. In our API use case, there are more than one point that needs to receive the result.

For example, setting app invokes checkForUpdate API and system app is waiting for the event back.(different process).

2. The updater will spontaneously fire the event not only by polling.

For startDownload,stopDownload and applyUpdate,

We expect that Gaia triggers the APIs and notify from onError event handler if there is any error occurred.

It works fine at current Gaia app migration as Bug 1161927.

If we invoke applyUpdate, the behavior is to reboot and start the FOTA procedure directly, we have no chance to get the result back.

Can we use this design?


For |Date| to |DOMTimestamp|,

We receive |Date| in gecko implementation and directly pass to gaia.

In my concept, DOMTimestamp can represent the relative time.

But in our use case, we need the absolute time representing the package build time.

Can I use |Date| in this condition?


For setParameter/getParameter, the use case in mozilla's updater is to get/set the setting such as |last update time|, |update status|...etc.

It is hard to define the API since we cannot define the API only for mozilla's update provider.

It is necessary to supply the API to allow OEM/ODM to get/set their own essential parameters.

So we use the general name to name the API. Is it acceptable to you?

The remaining suggestions will be addressed in next patch.

Please kindly feedback us.

Thank you very much.

Comment 42

[(no longer active)]

(In reply to James Cheng[:JamesCheng] from comment #41)
> Hi Ehsan,
> 
> After discussing with Gaia developer,
> 
> We would like to use event based API instead of Promise for some reasons.
> 
> 1. In our API use case, there are more than one point that needs to receive
> the result.
> 
> For example, setting app invokes checkForUpdate API and system app is
> waiting for the event back.(different process).

Is the intention for these two bits of code to use the same API?

Do we want to worry about the case where the event fires before the system app has set up a listener for it?

> 2. The updater will spontaneously fire the event not only by polling.

Same question as above: what if the system app has not set up an event handler yet?  Should we worry about that?

> For startDownload,stopDownload and applyUpdate,
> 
> We expect that Gaia triggers the APIs and notify from onError event handler
> if there is any error occurred.
> 
> It works fine at current Gaia app migration as Bug 1161927.
> 
> If we invoke applyUpdate, the behavior is to reboot and start the FOTA
> procedure directly, we have no chance to get the result back.
> 
> Can we use this design?

OK, if applyUpdate reboots, returning a promise from it doesn't make sense.  I'm not sure if I understand what you mean about the other two functions here though.

> For |Date| to |DOMTimestamp|,
> 
> We receive |Date| in gecko implementation and directly pass to gaia.
> 
> In my concept, DOMTimestamp can represent the relative time.
> 
> But in our use case, we need the absolute time representing the package
> build time.
> 
> Can I use |Date| in this condition?

DOMTimeStamp can mean milliseconds since epoch, so it's not necessarily relative time.  It's just an integer so doesn't have the issues associated with the Date object.  Since you can convert it to a Date object whenever you need one, this shouldn't be an issue.

> For setParameter/getParameter, the use case in mozilla's updater is to
> get/set the setting such as |last update time|, |update status|...etc.
> 
> It is hard to define the API since we cannot define the API only for
> mozilla's update provider.
> 
> It is necessary to supply the API to allow OEM/ODM to get/set their own
> essential parameters.
> 
> So we use the general name to name the API. Is it acceptable to you?

Well, but we still need to have a list of parameters that the updater service needs to be aware of and handle, if the OEM/ODM is supposed to be able to provide their own updater.  Do we have such a list?  I'm not objecting to this way of passing arguments, just want to make sure there is a good reason for doing things this way.

Comment 43

[James Cheng[:JamesCheng]]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #42)
> (In reply to James Cheng[:JamesCheng] from comment #41)
> > Hi Ehsan,
> > 
> > After discussing with Gaia developer,
> > 
> > We would like to use event based API instead of Promise for some reasons.
> > 
> > 1. In our API use case, there are more than one point that needs to receive
> > the result.
> > 
> > For example, setting app invokes checkForUpdate API and system app is
> > waiting for the event back.(different process).
> 
> Is the intention for these two bits of code to use the same API?
> 
Yes.

> Do we want to worry about the case where the event fires before the system
> app has set up a listener for it?
>
Yes, as Fabrice mentioned, if there is any pending event before listener registering, it will fire after the event setting up. I've implement it in another patch.
 
> > 2. The updater will spontaneously fire the event not only by polling.
> 
> Same question as above: what if the system app has not set up an event
> handler yet?  Should we worry about that?
> 
> > For startDownload,stopDownload and applyUpdate,
> > 
> > We expect that Gaia triggers the APIs and notify from onError event handler
> > if there is any error occurred.
> > 
> > It works fine at current Gaia app migration as Bug 1161927.
> > 
> > If we invoke applyUpdate, the behavior is to reboot and start the FOTA
> > procedure directly, we have no chance to get the result back.
> > 
> > Can we use this design?
> 
> OK, if applyUpdate reboots, returning a promise from it doesn't make sense. 
> I'm not sure if I understand what you mean about the other two functions
> here though.
> 
Sorry for my poor expression. I mean we expect Gaia invokes start/stopDownload and they don't need to wait.
If there is any error occurred, it will fire onError event with reasons. 
If no error occurred, it will fire onProgress.
This pattern is similar to mozChrome/Content Event in current Gaia design and it can work well using this patch on Bug 1161927.


> > For |Date| to |DOMTimestamp|,
> > 
> > We receive |Date| in gecko implementation and directly pass to gaia.
> > 
> > In my concept, DOMTimestamp can represent the relative time.
> > 
> > But in our use case, we need the absolute time representing the package
> > build time.
> > 
> > Can I use |Date| in this condition?
> 
> DOMTimeStamp can mean milliseconds since epoch, so it's not necessarily
> relative time.  It's just an integer so doesn't have the issues associated
> with the Date object.  Since you can convert it to a Date object whenever
> you need one, this shouldn't be an issue.
> 
Ok, I will change to DomTimestamp by DOMTimestamp = Date.getTime(). 
Could you please let me know or give me some hint about the issues using Date object?
I cannot understand the disadvantage using Date.

> > For setParameter/getParameter, the use case in mozilla's updater is to
> > get/set the setting such as |last update time|, |update status|...etc.
> > 
> > It is hard to define the API since we cannot define the API only for
> > mozilla's update provider.
> > 
> > It is necessary to supply the API to allow OEM/ODM to get/set their own
> > essential parameters.
> > 
> > So we use the general name to name the API. Is it acceptable to you?
> 
> Well, but we still need to have a list of parameters that the updater
> service needs to be aware of and handle, if the OEM/ODM is supposed to be
> able to provide their own updater.  Do we have such a list?  I'm not
> objecting to this way of passing arguments, just want to make sure there is
> a good reason for doing things this way.

The white list is embedded in the implementation,
_availableParameters: {
	    "deviceinfo.last_updated": null,
	    "gecko.updateStatus": null,
	    "app.update.channel": null,
	    "app.update.interval": null,
	    "app.update.url": null,
},
If Gaia app set the wrong parameter, we will return false.
The white list depends on the partner implementation so it is hard to regulate what the available parameters are. Currently, we decide to design such a general API for that.

Comment 44

[(no longer active)]

(In reply to James Cheng[:JamesCheng] from comment #43)
> > Do we want to worry about the case where the event fires before the system
> > app has set up a listener for it?
> >
> Yes, as Fabrice mentioned, if there is any pending event before listener
> registering, it will fire after the event setting up. I've implement it in
> another patch.

That is not really how events are supposed to work normally.  I guess since this API will be used only by people who should know exactly how this stuff works, it's fine, but you need to carefully document this in the IDL.

Also, please double check the way that you dispatch the events with Olli Pettay.  Usually, when dispatching DOM events, you do so without caring whether there is an event handler registered, and I guess in this case if you need to know whether someone handled your event, you should do something entirely different than how normal events are dispatched (I'm not really sure what the best way to do that would be.)

> > > 2. The updater will spontaneously fire the event not only by polling.
> > 
> > Same question as above: what if the system app has not set up an event
> > handler yet?  Should we worry about that?
> > 
> > > For startDownload,stopDownload and applyUpdate,
> > > 
> > > We expect that Gaia triggers the APIs and notify from onError event handler
> > > if there is any error occurred.
> > > 
> > > It works fine at current Gaia app migration as Bug 1161927.
> > > 
> > > If we invoke applyUpdate, the behavior is to reboot and start the FOTA
> > > procedure directly, we have no chance to get the result back.
> > > 
> > > Can we use this design?
> > 
> > OK, if applyUpdate reboots, returning a promise from it doesn't make sense. 
> > I'm not sure if I understand what you mean about the other two functions
> > here though.
> > 
> Sorry for my poor expression. I mean we expect Gaia invokes
> start/stopDownload and they don't need to wait.
> If there is any error occurred, it will fire onError event with reasons. 
> If no error occurred, it will fire onProgress.
> This pattern is similar to mozChrome/Content Event in current Gaia design
> and it can work well using this patch on Bug 1161927.

OK.  I don't think this is ideal, but again this API is not meant for general usage, so I guess that's fine.

> > > For |Date| to |DOMTimestamp|,
> > > 
> > > We receive |Date| in gecko implementation and directly pass to gaia.
> > > 
> > > In my concept, DOMTimestamp can represent the relative time.
> > > 
> > > But in our use case, we need the absolute time representing the package
> > > build time.
> > > 
> > > Can I use |Date| in this condition?
> > 
> > DOMTimeStamp can mean milliseconds since epoch, so it's not necessarily
> > relative time.  It's just an integer so doesn't have the issues associated
> > with the Date object.  Since you can convert it to a Date object whenever
> > you need one, this shouldn't be an issue.
> > 
> Ok, I will change to DomTimestamp by DOMTimestamp = Date.getTime(). 
> Could you please let me know or give me some hint about the issues using
> Date object?
> I cannot understand the disadvantage using Date.

Issues such as an attribute that returns a Date type creating a new JS Date object every time that it is invoked instead of returning the same object, it being impossible to serialize JS Date objects in JSON, etc...

> > > For setParameter/getParameter, the use case in mozilla's updater is to
> > > get/set the setting such as |last update time|, |update status|...etc.
> > > 
> > > It is hard to define the API since we cannot define the API only for
> > > mozilla's update provider.
> > > 
> > > It is necessary to supply the API to allow OEM/ODM to get/set their own
> > > essential parameters.
> > > 
> > > So we use the general name to name the API. Is it acceptable to you?
> > 
> > Well, but we still need to have a list of parameters that the updater
> > service needs to be aware of and handle, if the OEM/ODM is supposed to be
> > able to provide their own updater.  Do we have such a list?  I'm not
> > objecting to this way of passing arguments, just want to make sure there is
> > a good reason for doing things this way.
> 
> The white list is embedded in the implementation,
> _availableParameters: {
> 	    "deviceinfo.last_updated": null,
> 	    "gecko.updateStatus": null,
> 	    "app.update.channel": null,
> 	    "app.update.interval": null,
> 	    "app.update.url": null,
> },
> If Gaia app set the wrong parameter, we will return false.
> The white list depends on the partner implementation so it is hard to
> regulate what the available parameters are. Currently, we decide to design
> such a general API for that.

OK, please document these values in the IDL, and also state that partner implementations may use other parameters as well.  Also, I think it would be nice if you gave these better names, such as "update-url", "last-update-date", "update-status", etc.  You don't need to keep using the names we use internally for these!

Comment 45

[James Cheng[:JamesCheng]]

Attachment: Part 1 - system-update-api.patch

Hi Ehsan, 
Please help me to review the revised patch.
Thank you very much.

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #44)
> (In reply to James Cheng[:JamesCheng] from comment #43)
> > > Do we want to worry about the case where the event fires before the system
> > > app has set up a listener for it?
> > >
> > Yes, as Fabrice mentioned, if there is any pending event before listener
> > registering, it will fire after the event setting up. I've implement it in
> > another patch.
> 
> That is not really how events are supposed to work normally.  I guess since
> this API will be used only by people who should know exactly how this stuff
> works, it's fine, but you need to carefully document this in the IDL.
> 
> Also, please double check the way that you dispatch the events with Olli
> Pettay.  Usually, when dispatching DOM events, you do so without caring
> whether there is an event handler registered, and I guess in this case if
> you need to know whether someone handled your event, you should do something
> entirely different than how normal events are dispatched (I'm not really
> sure what the best way to do that would be.)
> 
I've documented in IDL. 
In our webidl implementation, we don't fire pending event when the event handler is registered(not against normal events dispatching). We handle the case that user invoke setActiveProvider, if there is a pending update event, we will fire the event from |UpdatePrompt.js| (the mozilla's update provider). It is implementation-dependent so I documented it in IDL. 

> > > > 2. The updater will spontaneously fire the event not only by polling.
> > > 
> > > Same question as above: what if the system app has not set up an event
> > > handler yet?  Should we worry about that?
> > > 
> > > > For startDownload,stopDownload and applyUpdate,
> > > > 
> > > > We expect that Gaia triggers the APIs and notify from onError event handler
> > > > if there is any error occurred.
> > > > 
> > > > It works fine at current Gaia app migration as Bug 1161927.
> > > > 
> > > > If we invoke applyUpdate, the behavior is to reboot and start the FOTA
> > > > procedure directly, we have no chance to get the result back.
> > > > 
> > > > Can we use this design?
> > > 
> > > OK, if applyUpdate reboots, returning a promise from it doesn't make sense. 
> > > I'm not sure if I understand what you mean about the other two functions
> > > here though.
> > > 
> > Sorry for my poor expression. I mean we expect Gaia invokes
> > start/stopDownload and they don't need to wait.
> > If there is any error occurred, it will fire onError event with reasons. 
> > If no error occurred, it will fire onProgress.
> > This pattern is similar to mozChrome/Content Event in current Gaia design
> > and it can work well using this patch on Bug 1161927.
> 
> OK.  I don't think this is ideal, but again this API is not meant for
> general usage, so I guess that's fine.
> 
> > > > For |Date| to |DOMTimestamp|,
> > > > 
> > > > We receive |Date| in gecko implementation and directly pass to gaia.
> > > > 
> > > > In my concept, DOMTimestamp can represent the relative time.
> > > > 
> > > > But in our use case, we need the absolute time representing the package
> > > > build time.
> > > > 
> > > > Can I use |Date| in this condition?
> > > 
> > > DOMTimeStamp can mean milliseconds since epoch, so it's not necessarily
> > > relative time.  It's just an integer so doesn't have the issues associated
> > > with the Date object.  Since you can convert it to a Date object whenever
> > > you need one, this shouldn't be an issue.
> > > 
> > Ok, I will change to DomTimestamp by DOMTimestamp = Date.getTime(). 
> > Could you please let me know or give me some hint about the issues using
> > Date object?
> > I cannot understand the disadvantage using Date.
> 
> Issues such as an attribute that returns a Date type creating a new JS Date
> object every time that it is invoked instead of returning the same object,
> it being impossible to serialize JS Date objects in JSON, etc...
> 
Thanks. Could you tell me the search keyword or bug number that discuss |Date| issue? I want to learn more deeper from it.

> > > > For setParameter/getParameter, the use case in mozilla's updater is to
> > > > get/set the setting such as |last update time|, |update status|...etc.
> > > > 
> > > > It is hard to define the API since we cannot define the API only for
> > > > mozilla's update provider.
> > > > 
> > > > It is necessary to supply the API to allow OEM/ODM to get/set their own
> > > > essential parameters.
> > > > 
> > > > So we use the general name to name the API. Is it acceptable to you?
> > > 
> > > Well, but we still need to have a list of parameters that the updater
> > > service needs to be aware of and handle, if the OEM/ODM is supposed to be
> > > able to provide their own updater.  Do we have such a list?  I'm not
> > > objecting to this way of passing arguments, just want to make sure there is
> > > a good reason for doing things this way.
> > 
> > The white list is embedded in the implementation,
> > _availableParameters: {
> > 	    "deviceinfo.last_updated": null,
> > 	    "gecko.updateStatus": null,
> > 	    "app.update.channel": null,
> > 	    "app.update.interval": null,
> > 	    "app.update.url": null,
> > },
> > If Gaia app set the wrong parameter, we will return false.
> > The white list depends on the partner implementation so it is hard to
> > regulate what the available parameters are. Currently, we decide to design
> > such a general API for that.
> 
> OK, please document these values in the IDL, and also state that partner
> implementations may use other parameters as well.  Also, I think it would be
> nice if you gave these better names, such as "update-url",
> "last-update-date", "update-status", etc.  You don't need to keep using the
> names we use internally for these!
Addressed.

Comment 46

[(no longer active)]

Comment on attachment 8622281 [details] [diff] [review]
Part 1 - system-update-api.patch

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

Andrea, do you mind reviewing this, please?  Please see my prior discussion on the API side of things with James.  Thanks!

Comment 47

[Andrea Marchesini [:baku]]

Comment on attachment 8622281 [details] [diff] [review]
Part 1 - system-update-api.patch

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

I'm happy to review this patch again.

::: dom/system/SystemUpdateService.jsm
@@ +40,5 @@
> +    if (this._instance) {
> +      this._instance.unsetListener();
> +    }
> +
> +    this._instance = null;

move this into the if()

@@ +50,5 @@
> +    if (this._isInActivity(PROVIDER_ACTIVITY_CHECKING)) {
> +      return;
> +    }
> +
> +    this._setActivity(PROVIDER_ACTIVITY_CHECKING);

Instead calling _setActibity and _isInActivity in all these methods, you can do:

_runIfNotInActivity: /* please choose a better name */ function(aActivity, aCb) {
  if (!this._isInActivity(aActivity)) {
    this._setActivity(aActivity);
    aCb();
  }
}

then use this method in checkForUpdate, startDownload, stopDownload and applyUpdate.

@@ +124,5 @@
> +      total: aTotal,
> +    });
> +  },
> +
> +  onUpdateReady: function() {

can we say that "onUpdateReady" should be received only when _isInActivity(PROVIDER_ACTIVITY_DOWNLOADING) returns true?
If this is true, can we do something similar to what I suggested before?

_runIfActiveAndInAction: function(aAction, aCb) { /* a better name please :) */ ...

and then convert onUpdateReady, onUpdateAvailable, onProgress and so on in this way:

  this._runIfActiveAndInAction(function() {
    ppm.broadcastAsyncMessage("SystemUpdate:OnUpdateReady", {
      uuid: this.id
    });
    this._unsetActivity(PROVIDER_ACTIVITY_DOWNLOADING);
  }.bind(this));

@@ +133,5 @@
> +    ppmm.broadcastAsyncMessage("SystemUpdate:OnUpdateReady", {
> +      uuid: this.id,
> +    });
> +
> +    this._unsetActivity(PROVIDER_ACTIVITY_DOWNLOADING);

I really think we need this check I proposed before because otherwise you can have some race conditions. Think about this (and tell me if I'm write):

1. startDownload()
2. here we start the download
3. stopDownload()
4. for some race conditions you receive the onUpdateReady <!-- we should ignore this because we are not in DOWNLOADING state.

@@ +137,5 @@
> +    this._unsetActivity(PROVIDER_ACTIVITY_DOWNLOADING);
> +  },
> +
> +  onError: function(aErrMsg) {
> +    if (!SystemUpdateService._isActiveProviderId(this.id)) {

Also onError should be managed only if we are in some kind of state. Correct?

@@ +185,5 @@
> +
> +  init: function() {
> +    debug("init");
> +
> +    this.messages = ["SystemUpdate:GetProviders",

you don't need to do 'this.messages' right? I mean, you don't use this.messages anywhere else.
Just do: let messages = [ ...

@@ +229,5 @@
> +      }
> +    }
> +  },
> +
> +  addProvider: function(aClassId, aContractId, aName) {

can you add some kind of check about aClassId, aContractId and aName?

@@ +245,5 @@
> +    //dynamically add the provider info to list.
> +    this._providers.push({
> +        id: aClassId,
> +        name: aName,
> +        contractId: aContractId

So... I don't know if I like this. I propose a different approach and you tell me if this makes sense or not.
Instead having an array of {id, name, contractId}, can we have an array of objects. Anytime you receive a addProvider you do a QI to nsISystemUpdateProvider in order to check if the incoming object is actually a provider. Nothing more.

In particular, what I don't like of your approach is that, if I call addProvider('foobar', 'foobar', foobar') you will realize that this is not a valid provider only when _updateActiveProvide is called. And we don't have any error handling there. And this makes your code super fragile.

::: dom/system/nsISystemUpdateProvider.idl
@@ +26,5 @@
> +   */
> +  void onUpdateReady();
> +
> +  /**
> +   * callback for notifying any error while checking/downloading/applying an update package.

can you have 80chars max per line? Here and in the rest of the file.

::: dom/system/tests/preload-SystemUpdateManager-jsm.js
@@ +6,5 @@
> +
> +const Cc = Components.classes;
> +const Ci = Components.interfaces;
> +const Cm = Components.manager;
> +const Cu = Components.utils;

const {classes: Cc, interfaces: Ci, utils: Cu} = Components;

::: dom/webidl/SystemUpdate.webidl
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +dictionary SystemUpdateProviderInfo {
> +  DOMString name;

some default values for these name and uuid?

@@ +11,5 @@
> +  DOMString type;
> +  DOMString version;
> +  DOMString description;
> +  DOMTimeStamp buildDate;
> +  unsigned long long size;

same here.

Comment 48

[[:fabrice] Fabrice Desré]

Comment on attachment 8617896 [details] [diff] [review]
Part 2 - enable-system-update-on-b2g.patch

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

::: b2g/components/UpdatePrompt.js
@@ +213,5 @@
> +
> +  setListener: function(aListener) {
> +    this._systemUpdateListener = aListener;
> +
> +    //If an update is available or ready, trigger the event right away at this point.

nit: // If...

Comment 49

[James Cheng[:JamesCheng]]

Attachment: [master]Part-2-enable-SystemUpdate-API-on-b2g.-r.patch

Thanks for your reviewing.
Carry r+ from Fabrice and fix the nit.

Comment 50

[James Cheng[:JamesCheng]]

Attachment: Part-1-implement-SystemUpdate-API.-r-bak.patch

(In reply to Andrea Marchesini (:baku) from comment #47)
> Comment on attachment 8622281 [details] [diff] [review]
> Part 1 - system-update-api.patch
> 
> Review of attachment 8622281 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm happy to review this patch again.
> 
> ::: dom/system/SystemUpdateService.jsm
> @@ +40,5 @@
> > +    if (this._instance) {
> > +      this._instance.unsetListener();
> > +    }
> > +
> > +    this._instance = null;
> 
> move this into the if()
> 
addressed.
> @@ +50,5 @@
> > +    if (this._isInActivity(PROVIDER_ACTIVITY_CHECKING)) {
> > +      return;
> > +    }
> > +
> > +    this._setActivity(PROVIDER_ACTIVITY_CHECKING);
> 
> Instead calling _setActibity and _isInActivity in all these methods, you can
> do:
> 
> _runIfNotInActivity: /* please choose a better name */ function(aActivity,
> aCb) {
>   if (!this._isInActivity(aActivity)) {
>     this._setActivity(aActivity);
>     aCb();
>   }
> }
> 
> then use this method in checkForUpdate, startDownload, stopDownload and
> applyUpdate.
> 
addressed.
> @@ +124,5 @@
> > +      total: aTotal,
> > +    });
> > +  },
> > +
> > +  onUpdateReady: function() {
> 
> can we say that "onUpdateReady" should be received only when
> _isInActivity(PROVIDER_ACTIVITY_DOWNLOADING) returns true?
> If this is true, can we do something similar to what I suggested before?
> 
> _runIfActiveAndInAction: function(aAction, aCb) { /* a better name please :)
> */ ...
> 
> and then convert onUpdateReady, onUpdateAvailable, onProgress and so on in
> this way:
> 
>   this._runIfActiveAndInAction(function() {
>     ppm.broadcastAsyncMessage("SystemUpdate:OnUpdateReady", {
>       uuid: this.id
>     });
>     this._unsetActivity(PROVIDER_ACTIVITY_DOWNLOADING);
>   }.bind(this));
> 
> @@ +133,5 @@
> > +    ppmm.broadcastAsyncMessage("SystemUpdate:OnUpdateReady", {
> > +      uuid: this.id,
> > +    });
> > +
> > +    this._unsetActivity(PROVIDER_ACTIVITY_DOWNLOADING);
> 
addressed.
> I really think we need this check I proposed before because otherwise you
> can have some race conditions. Think about this (and tell me if I'm write):
> 
> 1. startDownload()
> 2. here we start the download
> 3. stopDownload()
> 4. for some race conditions you receive the onUpdateReady <!-- we should
> ignore this because we are not in DOWNLOADING state.
> 
thanks for reminding. I fixed by your suggestion.
> @@ +137,5 @@
> > +    this._unsetActivity(PROVIDER_ACTIVITY_DOWNLOADING);
> > +  },
> > +
> > +  onError: function(aErrMsg) {
> > +    if (!SystemUpdateService._isActiveProviderId(this.id)) {
> 
> Also onError should be managed only if we are in some kind of state. Correct?
> 
onError is a mediator and invoked by the update provider whenever there is an error case that needs to notify to Gaia layer.

So, I cannot expect which state should the onError callback will be invoked.

Is it good for you?
> @@ +185,5 @@
> > +
> > +  init: function() {
> > +    debug("init");
> > +
> > +    this.messages = ["SystemUpdate:GetProviders",
> 
> you don't need to do 'this.messages' right? I mean, you don't use
> this.messages anywhere else.
> Just do: let messages = [ ...
> 
addressed.
> @@ +229,5 @@
> > +      }
> > +    }
> > +  },
> > +
> > +  addProvider: function(aClassId, aContractId, aName) {
> 
> can you add some kind of check about aClassId, aContractId and aName?
> 
> @@ +245,5 @@
> > +    //dynamically add the provider info to list.
> > +    this._providers.push({
> > +        id: aClassId,
> > +        name: aName,
> > +        contractId: aContractId
> 
> So... I don't know if I like this. I propose a different approach and you
> tell me if this makes sense or not.
> Instead having an array of {id, name, contractId}, can we have an array of
> objects. Anytime you receive a addProvider you do a QI to
> nsISystemUpdateProvider in order to check if the incoming object is actually
> a provider. Nothing more.
> 
> In particular, what I don't like of your approach is that, if I call
> addProvider('foobar', 'foobar', foobar') you will realize that this is not a
> valid provider only when _updateActiveProvide is called. And we don't have
> any error handling there. And this makes your code super fragile.
> 
The intention of ｜addProvider｜ is used only for the test-case that we need to inject an XPCOM information(cid, contract id...) "after" device bootup. In normal user scenario, the update providers' information should be loaded by nsICategoryManager when bootup and instantiate the update provider when switching to active.
Therefore, we simply design this api only for test-case usage and instantiate the provider internally in SystemUpdateService.jsm.
Hope my explanation makes sense for you.
Thanks.
> ::: dom/system/nsISystemUpdateProvider.idl
> @@ +26,5 @@
> > +   */
> > +  void onUpdateReady();
> > +
> > +  /**
> > +   * callback for notifying any error while checking/downloading/applying an update package.
> 
> can you have 80chars max per line? Here and in the rest of the file.
> 
addressed.
> ::: dom/system/tests/preload-SystemUpdateManager-jsm.js
> @@ +6,5 @@
> > +
> > +const Cc = Components.classes;
> > +const Ci = Components.interfaces;
> > +const Cm = Components.manager;
> > +const Cu = Components.utils;
> 
> const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
> 
addressed.
> ::: dom/webidl/SystemUpdate.webidl
> @@ +2,5 @@
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +dictionary SystemUpdateProviderInfo {
> > +  DOMString name;
> 
> some default values for these name and uuid?
> 
addressed.
> @@ +11,5 @@
> > +  DOMString type;
> > +  DOMString version;
> > +  DOMString description;
> > +  DOMTimeStamp buildDate;
> > +  unsigned long long size;
> 
> same here.
addressed.

attach the treeherder result for reference:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c97c3527120b

Comment 51

[James Cheng[:JamesCheng]]

Hi Andrea,

Due to the Red-Tai project schedule concern, 

Could you please review the patch when you are available?

It would be a great help for us to meet the schedule.

Thank you very much.

Comment 52

[Andrea Marchesini [:baku]]

Comment on attachment 8628206 [details] [diff] [review]
Part-1-implement-SystemUpdate-API.-r-bak.patch

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

lgtm!

::: dom/system/SystemUpdateManager.js
@@ +57,5 @@
> +          detail: {
> +            packageInfo: json.packageInfo
> +          }
> +        };
> +        let event = new this._window.CustomEvent("updateavailable", Cu.cloneInto(detail, this._window));

80chars max here and in the rest of the file.

::: dom/system/SystemUpdateService.jsm
@@ +45,5 @@
> +    this.id = null;
> +  },
> +
> +  checkForUpdate: function() {
> +    this._execFuncIfNotInActivity(PROVIDER_ACTIVITY_CHECKING, this._instance.checkForUpdate);

80chars.

::: dom/system/nsISystemUpdateProvider.idl
@@ +59,5 @@
> +  DOMString getParameter(in DOMString name);
> +
> +  /**
> +   * NOTE TO IMPLEMENTORS:
> +   *   Need to consider if it is necessary to fire the pending event when registering the listener.

80chars max for all this file.

Comment 53

[James Cheng[:JamesCheng]]

Attachment: [master]Part-1-implement-SystemUpdate-API.-r-bak.patch

Thanks for your reviewing.
Carry r+ from Andrea and fix the nit and rebase

Comment 54

[James Cheng[:JamesCheng]]

Attachment: [v2.2r+]Bug-1037329-Part-1-implement-SystemUpdate-API.-r-bak.patch

Attach the 2.2r+ patch for project Red-Tai.

Comment 55

[James Cheng[:JamesCheng]]

Attachment: [v2.2r+]Bug-1037329-Part-2-enable-SystemUpdate-API-on-b2g.-r.patch

Attach the 2.2r+ patch for project Red-Tai.

Comment 56

[James Cheng[:JamesCheng]]

Hi Wesley,

I would like to discuss with you about the landing procedure for Red-Tai project.

I've already attached the patches for 2.2r+.

Thanks.

Comment 57

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/e006feb54c7f
https://hg.mozilla.org/integration/b2g-inbound/rev/6ad0aee8c595

Comment 58

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e006feb54c7f
https://hg.mozilla.org/mozilla-central/rev/6ad0aee8c595

Comment 59

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Hey James,
I'm on PTO this week and suffering from the lousy internet access.
([e-mail]) is the release manager for gatekeeping 2.2R if you want to know.
Keeping my needinfo for now, and feel free to raise any question.

Comment 60

[James Cheng[:JamesCheng]]

Hi Wesley,

Sorry for disturbing you.

We can have a face to face discussion about the release process when you're back to office since I'm not familiar with the release procedure for red-tai project.

Thank you very much.

Comment 61

[James Cheng[:JamesCheng]]

Attachment: [v2.2r+]Bug-1037329-Part-2-enable-SystemUpdate-API-on-b2g.-r.patch

Update patch for the dependency bug 1184822.

Comment 62

[Josh Cheng [:josh]]

Thank you James,
Waiting for 2.2R approval flag on bug 1185409.

Comment 63

[James Cheng[:JamesCheng]]

Attachment: [v2.2r+] Bug-1037329-Part-1-implement-SystemUpdate-API.-r-bak.patch

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 1130262, partner needs a way to communicate with their own updater.
User impact if declined: if declined, partner cannot use a standard API from Gaia to communicate with partner's gecko updater.
Testing completed: v2.2r+ https://treeherder.mozilla.org/#/jobs?repo=try&revision=2fe8768c7f69
Risk to taking this patch (and alternatives if risky): No.
String or UUID changes made by this patch:none

Comment 64

[James Cheng[:JamesCheng]]

Attachment: [v2.2r+] Bug-1037329-Part-2-enable-SystemUpdate-API-on-b2g.-r.patch

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 1130262, partner needs a way to communicate with their own updater.
User impact if declined: if declined, partner cannot use a standard API from Gaia to communicate with partner's gecko updater.
Testing completed: v2.2r+ https://treeherder.mozilla.org/#/jobs?repo=try&revision=2fe8768c7f69
Risk to taking this patch (and alternatives if risky): No.
String or UUID changes made by this patch:none

Comment 65

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

(In reply to James Cheng[:JamesCheng] from comment #60)
> Hi Wesley,
> 
> Sorry for disturbing you.
> 
> We can have a face to face discussion about the release process when you're
> back to office since I'm not familiar with the release procedure for red-tai
> project.
> 
> Thank you very much.

Not at all. 
Thanks to Josh's help, now we have the flag "approval‑mozilla‑b2g37_v2_2r?".
Once it got +ed, Release Engineering will uplift it to 2.2R.

Comment 66

[Josh Cheng [:josh]]

Comment on attachment 8635869 [details] [diff] [review]
[v2.2r+] Bug-1037329-Part-2-enable-SystemUpdate-API-on-b2g.-r.patch

Approving as this is required for 2.2r device launch.

Hi Ryan,
Will you help to land "approval‑mozilla‑b2g37_v2_2r" uplift requests?
Thanks!

Comment 67

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/e2b94145ee3f
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/98e84779bbd9