Convert Web Activities to WebIDL

RESOLVED FIXED in mozilla31

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: mounir, Assigned: Ehsan)

Tracking

(Blocks: 1 bug)

Trunk
mozilla31
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Comment hidden (empty)
Blocks: 929542
(Assignee)

Updated

4 years ago
Blocks: 981845
(Assignee)

Updated

4 years ago
Depends on: 984132
(Assignee)

Updated

4 years ago
No longer blocks: 866528
Depends on: 866528
(Assignee)

Comment 1

4 years ago
Created attachment 8393885 [details] [diff] [review]
Finish porting Web Activities to WebIDL; r=bzbarsky

This patch ports over the ActivityRequestHandler interface.

Unfortunately the original design of the interface specifies that
ActivityRequestHandler.source should be of type ActivityOptions which is
a dictionary.  This is not expressible in WebIDL, so for now we're just
using the type 'object' for that.
(Assignee)

Updated

4 years ago
Assignee: nobody → ehsan
(Assignee)

Updated

4 years ago
Attachment #8393885 - Flags: review?(bzbarsky)
> This is not expressible in WebIDL

It depends.  It looks like the current API is just badly designed in that it returns a new object every time you call the getter.  Are we OK with returning the same object every time the getter is called?  I guess that should be a followup bug.  :(
(Assignee)

Comment 3

4 years ago
(In reply to comment #2)
> > This is not expressible in WebIDL
> 
> It depends.  It looks like the current API is just badly designed in that it
> returns a new object every time you call the getter.  Are we OK with returning
> the same object every time the getter is called?  I guess that should be a
> followup bug.  :(

A quick look at the gaia code seems to suggest that the existing callers do not care about the object identity.  If you want, I can file  follow-up, but I'm not sure what I would put into it.  :-)
That this should be a dictionary type in the IDL and should be a [Cached, Pure] property.  And ask the people who own the code whether it should be [Frozen] too.  ;)
(Assignee)

Comment 5

4 years ago
(In reply to comment #4)
> That this should be a dictionary type in the IDL and should be a [Cached, Pure]
> property.  And ask the people who own the code whether it should be [Frozen]
> too.  ;)

Wait, out of curiosity, can [Cached, Pure] properties be a dictionary?  If yes, do you mind explaining the rationale please?
> Wait, out of curiosity, can [Cached, Pure] properties be a dictionary?

Yes.

The reason WebIDL doesn't allow dictionary attributes is that converting a WebIDL dictionary to JS creates a new object each time.  So allowing them would mean that people would create APIs like this one, where each time you get the property you get a new object, which is viewed as an antipattern.

Sequences have the same issue, which is why WebIDL does not allow sequence-valued attributes.  But as Jonas pointed out in https://www.w3.org/Bugs/Public/show_bug.cgi?id=23682 its reasonable to want attributes whose value is the same array (or object, for the dictionary case) every time.  You can already do it if you declare the attribute as an "object" and then manually cache the value in the callee, but that's moderately annoying.

What our [Cached] extension to WebIDL implements is that the binding itself caches the value.  So the underlying getter is called once, then the binding keeps returning the same object without calling the getter.

Obviously this is only OK for cases when the getter has no side-effects, which is why we only allow [Cached] on attributes that are [Pure] or [Constant].
(Assignee)

Updated

4 years ago
Depends on: 985812
(Assignee)

Comment 7

4 years ago
Thanks for the great explanation, filed bug 985812.

Just as a clarification, do you think we should wait on that bug before landing this?
I don't think we should, no.  What you have here is fine at first glance; I'll read more carefully tomorrow.
Comment on attachment 8393885 [details] [diff] [review]
Finish porting Web Activities to WebIDL; r=bzbarsky

r=me
Attachment #8393885 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/f8613d9d677d
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.