Closed Bug 899322 Opened 6 years ago Closed 5 years ago

Convert the mozApps API to use webidl

Categories

(Core Graveyard :: DOM: Apps, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla34

People

(Reporter: fabrice, Assigned: fabrice)

References

Details

Attachments

(1 file, 3 obsolete files)

No description provided.
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
(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?
Wish I could, but I'm about to run away for a month on (un)PTO :(
Depends on: 923919
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?
Flags: needinfo?(mcaceres)
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....
(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.
Flags: needinfo?(mcaceres)
Blocks: SH-APIs
Attached patch wip (obsolete) — Splinter Review
First wip, let's see how much is broken on try:
https://tbpl.mozilla.org/?tree=Try&rev=756cc2ab68c4
Assignee: nobody → fabrice
Blocks: 981845
Depends on: 1007878
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.
Flags: needinfo?(fabrice)
Attached patch wip (obsolete) — Splinter Review
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.
Attachment #821481 - Attachment is obsolete: true
Flags: needinfo?(fabrice)
(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?
> because Promise<Sequence<MozInterAppMessagePort>> is not valid webidl.

It's Promise<sequence<MozInterAppMessagePort>>, no?
(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__.
(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?
Flags: needinfo?(continuation)
See bug 963382.  I can fix that when I get back, or someone else can pick it up in the meantime.
Flags: needinfo?(continuation)
Depends on: 963382
Attached patch apps-webidl.patch (obsolete) — Splinter Review
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.
Attachment #8468132 - Attachment is obsolete: true
Attachment #8470397 - Flags: review?(mar.castelluccio)
(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].
(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
(In reply to Fabrice Desré [:fabrice] from comment #20)
> Ok, but then I'm also blocked on bug 96338

Wrong bug number?
Bug 963382.  Somebody cleared his cache too early.
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?
Attachment #8470397 - Flags: review?(mar.castelluccio) → review+
> 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.
(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.
Attached patch update patchSplinter Review
Updated to use DOMTimeStamp, and fixed the indentation issues.

Bobby, can you check the dom/webidl parts?
Attachment #8470397 - Attachment is obsolete: true
Attachment #8471337 - Flags: review?(bobbyholley)
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]?
Attachment #8471337 - Flags: review?(bobbyholley) → review+
Depends on: 1053033
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?
(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!
(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.  :-)
(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.
(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.
(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?
(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.
https://hg.mozilla.org/mozilla-central/rev/de62aa662be4
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Depends on: 1056156
Depends on: 1056226
Depends on: 1063852
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.