Closed
Bug 903873
Opened 11 years ago
Closed 11 years ago
Convert Web Activities to WebIDL
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: mounir, Assigned: ehsan.akhgari)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
7.80 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•11 years ago
|
Assignee | ||
Comment 1•11 years ago
|
||
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•11 years ago
|
Assignee: nobody → ehsan
Assignee | ||
Updated•11 years ago
|
Attachment #8393885 -
Flags: review?(bzbarsky)
Comment 2•11 years ago
|
||
> 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•11 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. :-)
Comment 4•11 years ago
|
||
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•11 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?
Comment 6•11 years ago
|
||
> 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 | ||
Comment 7•11 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?
Comment 8•11 years ago
|
||
I don't think we should, no. What you have here is fine at first glance; I'll read more carefully tomorrow.
Comment 9•11 years ago
|
||
Comment on attachment 8393885 [details] [diff] [review]
Finish porting Web Activities to WebIDL; r=bzbarsky
r=me
Attachment #8393885 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 10•11 years ago
|
||
Comment 11•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•