Closed Bug 1454568 Opened 2 years ago Closed Last year

Support [infallible] in xpidl on interface and domobject attributes

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: Nika, Assigned: Nika)

References

Details

Attachments

(1 file, 1 obsolete file)

This would make some callers nicer to use. The second patch on this bug cleans up a getter on nsIWebNavigation.

I didn't bother implementing the convenience wrappers for rust, as it felt unnecessary.
Comment on attachment 8968418 [details] [diff] [review]
Part 2: Switch nsIWebNavigation::sessionHistory to use webidl in xpidl, and [infallible]

https://searchfox.org/mozilla-central/rev/a30a60eadcff99d2a043241955d215dbeccad876/toolkit/components/remotebrowserutils/RemoteWebNavigation.js#30 implements nsIWebNavigation in JS, so the builtinclass bit there doesn't look right to me...

It was kinda ok (though rather fishy) before because its sessionHistory getter just throws, so it just returned null from GetSessionHistory() if anyone does call it from C++...
Attachment #8968418 - Flags: review?(bzbarsky) → review-
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #3)
> Comment on attachment 8968418 [details] [diff] [review]
> Part 2: Switch nsIWebNavigation::sessionHistory to use webidl in xpidl, and
> [infallible]
> 
> https://searchfox.org/mozilla-central/rev/
> a30a60eadcff99d2a043241955d215dbeccad876/toolkit/components/
> remotebrowserutils/RemoteWebNavigation.js#30 implements nsIWebNavigation in
> JS, so the builtinclass bit there doesn't look right to me...
> 
> It was kinda ok (though rather fishy) before because its sessionHistory
> getter just throws, so it just returned null from GetSessionHistory() if
> anyone does call it from C++...

Yup, I just noticed that too >.< oops.

Consider patch 2 waived...
Attachment #8968418 - Attachment is obsolete: true
Comment on attachment 8968417 [details] [diff] [review]
Part 1: Support [infallible] on interfaces and domobjects in xpidl

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

I remember thinking at one point that we didn't want to do this, but for reasons that I cannot remember.

::: xpcom/idl-parser/xpidl/header.py
@@ +382,5 @@
> +
> +            if a.realtype.kind != 'builtin':
> +                assert realtype.endswith(' *'), "bad infallible type"
> +                tmpl = attr_refcnt_infallible_tmpl
> +                realtype = realtype[:-2] # strip trailing pointer

This code is so gross.

::: xpcom/idl-parser/xpidl/rust.py
@@ +245,5 @@
>  
>          name = attributeParamName(m)
>  
> +        if getter and m.infallible and m.realtype.kind == 'builtin':
> +            # NOTE: We don't support non-builtin infallible getters in Rust code.

Does this mean we should really be doing:

if getter and m.infallible:
  if m.realtype.kind != 'builtin':
    raise xpidl.RustNoncompat(...)

?  I guess that would mean that we couldn't actually use infallible getters on non-builtin object types, or the Rust generator would complain?  So we just have this comment here to remind people that the Rust generator has this limitation, correct?
Attachment #8968417 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd [:froydnj] from comment #5)
> ::: xpcom/idl-parser/xpidl/header.py
> @@ +382,5 @@
> > +
> > +            if a.realtype.kind != 'builtin':
> > +                assert realtype.endswith(' *'), "bad infallible type"
> > +                tmpl = attr_refcnt_infallible_tmpl
> > +                realtype = realtype[:-2] # strip trailing pointer
> 
> This code is so gross.

Yeah, couldn't think of a nicer way to write it without making a lot of other changes D:.

> 
> ::: xpcom/idl-parser/xpidl/rust.py
> @@ +245,5 @@
> >  
> >          name = attributeParamName(m)
> >  
> > +        if getter and m.infallible and m.realtype.kind == 'builtin':
> > +            # NOTE: We don't support non-builtin infallible getters in Rust code.
> 
> Does this mean we should really be doing:
> 
> if getter and m.infallible:
>   if m.realtype.kind != 'builtin':
>     raise xpidl.RustNoncompat(...)
> 
> ?  I guess that would mean that we couldn't actually use infallible getters
> on non-builtin object types, or the Rust generator would complain?  So we
> just have this comment here to remind people that the Rust generator has
> this limitation, correct?

Yup. We only turn off the infallible wrapper in rust.
Pushed by nika@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e59e16b56c1e
Support [infallible] on interfaces and domobjects in xpidl, r=froydnj
https://hg.mozilla.org/mozilla-central/rev/e59e16b56c1e
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.