899322 - Convert the mozApps API to use webidl

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Comment 1

[Marcos Caceres [:marcosc]]

Here is a start... by no means done or correct. I just made it up from reading the docs online. 


partial interface Navigator{
   readonly attribute Apps mozApps;
}

interface Apps{
  readonly attribute ??? mgmt;
  DOMRequest install(DOMString url, optional Receipts receipts);
  DOMRequest getSelf();
  DOMRequest getInstalled(); 
}

dictionary Receipts{
  DOMString[] receipts;
}

interface App{
  readonly attribute Manifest manifest; 
  readonly attribute DOMString manifestURL;
  readonly attribute DOMString origin;
  readonly attribute DOMString installOrigin;
  //not sure if this is a Date or number? 
  readonly attribute Date installTime;
  void launch()
  DOMRequest checkForUpdate()
}

//not sure how any of the following are represented...
interface Manifest{
    activities
    appcache_path
    csp
    default_locale
    description
    developer
    fullscreen
    icons
    installs_allowed_from
    launch_path
    locales
    messages
    name
    orientation
    permissions
    redirects
    type
    version
}

The docs where not clear what

Comment 2

[Marco Castelluccio [:marco]]

(In reply to Marcos Caceres [:marcosc] from comment #1)
> Here is a start... by no means done or correct. I just made it up from
> reading the docs online. 

Are you taking the bug?

Comment 3

[Marcos Caceres [:marcosc]]

Wish I could, but I'm about to run away for a month on (un)PTO :(

Comment 4

[Boris Zbarsky [:bzbarsky]]

Why does this depend on bug 923919?  That is, why are IDL arrays involved here in any way?  Does something store the exact object passed in Receipts?

Comment 5

[Boris Zbarsky [:bzbarsky]]

My initial reading of dom/apps/src/Webapps.js suggests that the answer to my last question is "no", which means that Receipts can use s sequence<DOMString>.  Assuming that's the right code, of course.

Also, I believe Date is being deprecated in favor of timestamps or something....

Comment 6

[Marcos Caceres [:marcosc]]

(In reply to Boris Zbarsky [:bz] from comment #5)
> Also, I believe Date is being deprecated in favor of timestamps or
> something....

Yes, that is correct: one should return the numerical representation of the date and not a Date object.

Comment 7

[[:fabrice] Fabrice Desré]

Attachment: wip

First wip, let's see how much is broken on try:
https://tbpl.mozilla.org/?tree=Try&rev=756cc2ab68c4

Comment 9

[Bobby Holley (:bholley)]

Fabrice - per our discussion on Monday, it's time to make this happen. It should be very straightforward - let me know if you run into anything that isn't.

Comment 10

[[:fabrice] Fabrice Desré]

Attachment: wip

This patch passes all the dom/apps tests locally. The remaining piece to do is the conversion of manifests to webidl dictionaries. I also had to keep a Promise<any> because Promise<Sequence<MozInterAppMessagePort>> is not valid webidl.

Comment 11

[[:fabrice] Fabrice Desré]

try run is at https://tbpl.mozilla.org/?tree=Try&rev=261131a69d67

Comment 12

[Bobby Holley (:bholley)]

(In reply to Fabrice Desré [:fabrice] from comment #10)
> This patch passes all the dom/apps tests locally

\o/

> The remaining piece to do
> is the conversion of manifests to webidl dictionaries. I also had to keep a
> Promise<any> because Promise<Sequence<MozInterAppMessagePort>> is not valid
> webidl.

Hm, I'd think it should be. Can you file a bug and ni Boris?

Comment 13

[Boris Zbarsky [:bzbarsky]]

> because Promise<Sequence<MozInterAppMessagePort>> is not valid webidl.

It's Promise<sequence<MozInterAppMessagePort>>, no?

Comment 14

[[:fabrice] Fabrice Desré]

(In reply to On vacation Aug 5-18.  Please do not request review. from comment #13)
> > because Promise<Sequence<MozInterAppMessagePort>> is not valid webidl.
> 
> It's Promise<sequence<MozInterAppMessagePort>>, no?

You're right! /me facepalms

So the only api change I had to do is to move an attribute which would have been |readonly sequence<DOMString> receipts| to and explicit getter |sequence<DOMString> getReceipts()|. My understanding is that I would need the attribute to be [Cached, Pure] but since the value can change I need to access ClearCachedReceiptsValue from JS. But it seems that this method is not available on __DOM_IMPL__.

Comment 15

[Bobby Holley (:bholley)]

(In reply to Fabrice Desré [:fabrice] from comment #14)
> My understanding is that I would need
> the attribute to be [Cached, Pure] but since the value can change I need to
> access ClearCachedReceiptsValue from JS. But it seems that this method is
> not available on __DOM_IMPL__.

Andrew, do you know how this is supposed to work with JS-implemented WebIDL?

Comment 16

[Boris Zbarsky [:bzbarsky]]

See bug 963382.  I can fix that when I get back, or someone else can pick it up in the meantime.

Comment 17

[[:fabrice] Fabrice Desré]

Attachment: apps-webidl.patch

I'd like to move on with this version, where we don't use dictionaries for the manifests because we can't use dictionaries as attributes in the interfaces.

That would mean that we'd need to use methodless interfaces which looks really weird to me. That will probably not even round trip properly as json objects.

Marco, there's a try build here. Ignore the gaia failures, I have a patch but I need to figure out how to coordinate landings.

Comment 18

[[:fabrice] Fabrice Desré]

https://tbpl.mozilla.org/?tree=Try&rev=9a32a1ce7475

Comment 19

[Bobby Holley (:bholley)]

(In reply to Fabrice Desré [:fabrice] from comment #17)
> Created attachment 8470397 [details] [diff] [review]
> apps-webidl.patch
> 
> I'd like to move on with this version, where we don't use dictionaries for
> the manifests because we can't use dictionaries as attributes in the
> interfaces.

You definitely can. You just need to mark it [Cached, Pure].

Comment 20

[[:fabrice] Fabrice Desré]

(In reply to Bobby Holley (:bholley) from comment #19)
> (In reply to Fabrice Desré [:fabrice] from comment #17)
> > Created attachment 8470397 [details] [diff] [review]
> > apps-webidl.patch
> > 
> > I'd like to move on with this version, where we don't use dictionaries for
> > the manifests because we can't use dictionaries as attributes in the
> > interfaces.
> 
> You definitely can. You just need to mark it [Cached, Pure].

Ok, but then I'm also blocked on bug 96338

Comment 21

[Bobby Holley (:bholley)]

(In reply to Fabrice Desré [:fabrice] from comment #20)
> Ok, but then I'm also blocked on bug 96338

Wrong bug number?

Comment 22

[Andrew McCreight [:mccr8]]

Bug 963382.  Somebody cleared his cache too early.

Comment 23

[Marco Castelluccio [:marco]]

Comment on attachment 8470397 [details] [diff] [review]
apps-webidl.patch

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

Looks good to me, but you may want to ask someone with better understanding of WebIDL to review this too.

::: dom/apps/src/Webapps.js
@@ +328,5 @@
>    },
>  
>    get installTime() {
> +    let value = this._proxy.installTime;
> +    return (value instanceof Date) ? value : new Date(value);

What about comment 6?

@@ +738,3 @@
>      let window = this._window;
>      DOMApplicationRegistry.getAll((aApps) => {
> +     Services.DOMRequest.fireSuccessAsync(request,

What is the change here?

::: dom/apps/tests/test_install_receipts.html
@@ +207,5 @@
>  
>  </script>
>  </pre>
>  </body>
> +</html>

What is the change here?

Comment 24

[Andrew McCreight [:mccr8]]

> Looks good to me, but you may want to ask someone with better understanding of WebIDL to review this too.
Yes, webidl changes need DOM peer review.

Comment 25

[[:fabrice] Fabrice Desré]

(In reply to Marco Castelluccio [:marco] from comment #23)
> Comment on attachment 8470397 [details] [diff] [review]
  
> >    get installTime() {
> > +    let value = this._proxy.installTime;
> > +    return (value instanceof Date) ? value : new Date(value);
> 
> What about comment 6?

Ha right, I'll move to  DOMTimeStamp that is used by other interfaces.

> @@ +738,3 @@
> >      let window = this._window;
> >      DOMApplicationRegistry.getAll((aApps) => {
> > +     Services.DOMRequest.fireSuccessAsync(request,
> 
> What is the change here?

None, it seems I messed up the indentation...

> ::: dom/apps/tests/test_install_receipts.html
> @@ +207,5 @@
> >  
> >  </script>
> >  </pre>
> >  </body>
> > +</html>
> 
> What is the change here?

There was a "No newline at end of file" that splinter is not showing.

Comment 26

[[:fabrice] Fabrice Desré]

Attachment: update patch

Updated to use DOMTimeStamp, and fixed the indentation issues.

Bobby, can you check the dom/webidl parts?

Comment 27

[Bobby Holley (:bholley)]

Comment on attachment 8471337 [details] [diff] [review]
update patch

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

r=bholley with those comments addressed. Thanks for doing this!

::: dom/webidl/Apps.webidl
@@ +23,5 @@
> +
> +[JSImplementation="@mozilla.org/webapps/application;1", ChromeOnly]
> +interface DOMApplication : EventTarget {
> +  readonly attribute any manifest;       // Attributes can't be dictionaries!
> +  readonly attribute any updateManifest; // Attributes can't be dictionaries!

This should be fine modulo bug 963382 right? Please adjust the comment and make sure we have a bug on file to fix this.

@@ +75,5 @@
> +
> +    // Receipts handling functions.
> +    DOMRequest addReceipt(DOMString? receipt);
> +    DOMRequest removeReceipt(DOMString? receipt);
> +    DOMRequest replaceReceipt(DOMString? oldReceipt, DOMString? newReceipt);

Hm, do we really want all of these to be nullable? Are you sure you don't want them to be optional instead? This would seem like kinda bad API design...

@@ +79,5 @@
> +    DOMRequest replaceReceipt(DOMString? oldReceipt, DOMString? newReceipt);
> +};
> +
> +[JSImplementation="@mozilla.org/webapps/manager;1", ChromeOnly]
> +interface DOMApplicationsManager : EventTarget {

Should this perhaps be [NoInterfaceObject]?

Comment 28

[(no longer active)]

Comment on attachment 8471337 [details] [diff] [review]
update patch

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

::: dom/webidl/Apps.webidl
@@ +78,5 @@
> +    DOMRequest removeReceipt(DOMString? receipt);
> +    DOMRequest replaceReceipt(DOMString? oldReceipt, DOMString? newReceipt);
> +};
> +
> +[JSImplementation="@mozilla.org/webapps/manager;1", ChromeOnly]

I think you want [CheckPermissions="webapps-manage"] instead of [ChromeOnly] here.

::: dom/webidl/MozApplicationEvent.webidl
@@ +6,3 @@
>  
>  [Constructor(DOMString type, optional MozApplicationEventInit eventInitDict)]
>  interface MozApplicationEvent : Event

DOMApplication is [ChromeOnly]...  Should we mark this interface [ChromeOnly] as well?

Comment 29

[[:fabrice] Fabrice Desré]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #28)

> > +[JSImplementation="@mozilla.org/webapps/manager;1", ChromeOnly]
> 
> I think you want [CheckPermissions="webapps-manage"] instead of [ChromeOnly]
> here.

We don't want to leak the mgmt constructor on content, hence the ChromeOnly. But I can add CheckPermissions too.

> ::: dom/webidl/MozApplicationEvent.webidl
> @@ +6,3 @@
> >  
> >  [Constructor(DOMString type, optional MozApplicationEventInit eventInitDict)]
> >  interface MozApplicationEvent : Event
> 
> DOMApplication is [ChromeOnly]...  Should we mark this interface
> [ChromeOnly] as well?

Yep, I think so. Good catch!

We can all thank hg brokenness that prevented me to land last night!

Comment 30

[(no longer active)]

(In reply to Fabrice Desré [:fabrice] from comment #29)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #28)
> 
> > > +[JSImplementation="@mozilla.org/webapps/manager;1", ChromeOnly]
> > 
> > I think you want [CheckPermissions="webapps-manage"] instead of [ChromeOnly]
> > here.
> 
> We don't want to leak the mgmt constructor on content, hence the ChromeOnly.
> But I can add CheckPermissions too.

You should use [ChromeConstructor] for that purpose, I think.

> > ::: dom/webidl/MozApplicationEvent.webidl
> > @@ +6,3 @@
> > >  
> > >  [Constructor(DOMString type, optional MozApplicationEventInit eventInitDict)]
> > >  interface MozApplicationEvent : Event
> > 
> > DOMApplication is [ChromeOnly]...  Should we mark this interface
> > [ChromeOnly] as well?
> 
> Yep, I think so. Good catch!
> 
> We can all thank hg brokenness that prevented me to land last night!

We owe many thanks to the hg brokenness for this and other reasons indeed.  :-)

Comment 31

[Bobby Holley (:bholley)]

(In reply to Fabrice Desré [:fabrice] from comment #29)
> You should use [ChromeConstructor] for that purpose, I think.

Can you explain the reasoning for this?

> > >  
> > >  [Constructor(DOMString type, optional MozApplicationEventInit eventInitDict)]
> > >  interface MozApplicationEvent : Event
> > 
> > DOMApplication is [ChromeOnly]...  Should we mark this interface
> > [ChromeOnly] as well?

To make sure I understand correctly - this interface was already exposed before this patch, right? It must be, otherwise we would have turned the tests orange.

Comment 32

[(no longer active)]

(In reply to Bobby Holley (:bholley) from comment #31)
> (In reply to Fabrice Desré [:fabrice] from comment #29)
> > You should use [ChromeConstructor] for that purpose, I think.
> 
> Can you explain the reasoning for this?

It doesn't make sense to have an interface exposed to the Web which has a member of a type that you can't access in Web content.  The availability criteria for the two should match, which is why I'm suggesting making this interface available to the Web content.

> > > >  
> > > >  [Constructor(DOMString type, optional MozApplicationEventInit eventInitDict)]
> > > >  interface MozApplicationEvent : Event
> > > 
> > > DOMApplication is [ChromeOnly]...  Should we mark this interface
> > > [ChromeOnly] as well?
> 
> To make sure I understand correctly - this interface was already exposed
> before this patch, right? It must be, otherwise we would have turned the
> tests orange.

Yes, and it is exposed to the Web.

Comment 33

[Bobby Holley (:bholley)]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #32)
> (In reply to Bobby Holley (:bholley) from comment #31)
> It doesn't make sense to have an interface exposed to the Web which has a
> member of a type that you can't access in Web content.  The availability
> criteria for the two should match, which is why I'm suggesting making this
> interface available to the Web content.

Ok. So we expose the interface object for the benefit of instanceof etc, but make it non-constructable.

> > To make sure I understand correctly - this interface was already exposed
> > before this patch, right? It must be, otherwise we would have turned the
> > tests orange.
> 
> Yes, and it is exposed to the Web.

So the point here is that this was a mistake?

Comment 34

[(no longer active)]

(In reply to Bobby Holley (:bholley) from comment #33)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #32)
> > (In reply to Bobby Holley (:bholley) from comment #31)
> > It doesn't make sense to have an interface exposed to the Web which has a
> > member of a type that you can't access in Web content.  The availability
> > criteria for the two should match, which is why I'm suggesting making this
> > interface available to the Web content.
> 
> Ok. So we expose the interface object for the benefit of instanceof etc, but
> make it non-constructable.

Yes, exactly.  (Sorry for being cryptic before. :)

> > > To make sure I understand correctly - this interface was already exposed
> > > before this patch, right? It must be, otherwise we would have turned the
> > > tests orange.
> > 
> > Yes, and it is exposed to the Web.
> 
> So the point here is that this was a mistake?

My point is that we should be consistent at least.  But yeah, I _think_ that was a mistake.  I'd be happy to defer to Fabrice on that, he knows the history of why that was done before better than I do.

Comment 35

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/b2g-inbound/rev/de62aa662be4

Comment 36

[Ryan VanderMeulen [:RyanVM]]

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