Created attachment 279546 [details] [review]
Gio::Permission wrapper.

Created attachment 279547 [details] [review]
Gio::SimplePermission wrapper

--- Permission
The description of g_permission_impl_update() says: "You should never call this
function except from a #GPermission implementation."
Don't you think Gio::Permission::impl_update() should be protected instead of
public?

The new _CONVERSION in convert_gio.m4 shall be
_CONVERSION(`GAsyncResult*',`const Glib::RefPtr<AsyncResult>&',
`Glib::wrap($3, true)')
Note 'true', i.e. take a copy. There are such conversions in tlsconnection.hg,
tlsdatabase.hg and tlsinteraction.hg.
There is no _CONVERSION with Glib::wrap($3, true) in convert_gio.m4.
It's probably safer to keep it like that, and add the new _CONVERSION in
permission.hg instead. When a function returns a Glib::RefPtr<Something>,
take_copy == true is replaced by the refreturn parameter in _WRAP_METHOD.
A _CONVERSION with "built-in" take_copy in convert_gio.m4 can have unexpected
effects in other classes in the future.

Is it necessary to hand-code acquire_async() and release_async()?
In tlsconnection.hg _WRAP_METHOD generates code for handshake_async().
The generated code in tlsconnection.cc looks equivalent to what you've written
in permission.ccg.

"{?}" has no effect in _WRAP_VFUNC, and it's not needed there.
Functions wrapped with _WRAP_METHOD are called from C++ to C. Virtual functions
are normally called the other way: If a virtual function is overridden in a
derived class, the overridden function is called from the C code (from glib).
It's always called with all parameters (because you can't overload functions in
C), but e.g. 'GCancellable* cancellable' can be 0.

I'm almost certain that the conversions
  _CONVERSION(`GCancellable*', `Glib::RefPtr<Cancellable>', `Glib::wrap($3)')
  _CONVERSION(`GCancellable*', `const Glib::RefPtr<Cancellable>&',
   `Glib::wrap($3)')
in convert_gio.m4 are wrong when used by _WRAP_VFUNC.
You should redefine at least the last one, which is the one used,
in permission.hg to
  #m4 _CONVERSION(`GCancellable*', `const Glib::RefPtr<Cancellable>&',
      `Glib::wrap($3, true)')
There is such a _CONVERSION in bufferedinputstream.hg.
I think this is wrong in all other giomm files that use Cancellable in virtual
functions.
It's safest to put this conversion, and all _WRAP_VFUNC at the end of the file.
Then the conversion can't affect other functions.
(Keep the virtual functions protected.)

I suppose you intend to add
  _WRAP_VFUNC(void acquire_async(...
  _WRAP_VFUNC(void release_async(...
You can look at
  _WRAP_VFUNC(void handshake_async(...
in tlsconnection.hg for tips how to do it.
(I don't understand why one of the parameters there has a default value.
It's useless in a wrapped virtual function for the reason that {?} is useless.)
 
--- SimplePermission
"* Calling Permission::request or Permission::release will result in errors."
There are some errors in gsimplepermission.c, but you can make it better in
simplepermission.hg.
1. "request" should be "acquire".
2. Put parentheses after function names, to make it almost obvious that they
   are functions.
Result: "* Calling Permission::acquire() or Permission::release() will result
in errors."

--- General comments
@newin directives in .hg files usually show when the class or function first
appeared in glibmm, in this case @newin{2,42}. Sometimes, like here, they
appeared earlier in glib. The result is inconsistent, because much of the
documentation is copied from glib by gmmproc. In such automatically generated
documentation, the @newin directives show when the entity first appeared in
glib.

What's the line before the copyright notice good for? Do you know? I mean
// -*- Mode: C++; indent-tabs-mode: nil; c-basic-offset: 2 -*-

Does an editor use this info? I've noticed that many files contain such a line.
If you don't know who uses this info, and no one else can explain it, you can
delete it. Or keep it in case of doubt, it does no harm.

The copyright text is inconsistent in glibmm/gio/src files. "giomm Development
Team" or "gtkmm Development Team"? I recommend "giomm Development Team", but
it's probably unimportant.

Gio::Permission shall have a protected default constructor.
  protected:
    _CTOR_DEFAULT

It's needed in derived classes such as:
  class MyPermission : public Gio::Permission
  {
    protected:
      MyPermission() {}
    public:
      static Glib::RefPtr<MyPermission> create()
      { return Glib::RefPtr<MyPermission>(new MyPermission()); }

Thank you very much for your feedback. I have some comments:
- I had to include gio/gio.h in permission_p.h so that GAsyncResultCallback is defined prior to be used when declaring the vfuncs.
- The line before the copyright notice I think it is a so-called mode line. That particular one is only useful for emacs users; I use vim so that it makes no difference for me. Since probably we have some emacs users among the development team, I'm reluctant to remove it. However, I will do whatever you want me to do; I'm not happy with it since it hardcodes emacs-lisp syntax into a file that has nothing to do with emacs though.

Created attachment 280546 [details] [review]
Gio::Permission wrapper.

Created attachment 280572 [details] [review]
Gio::Permission wrapper.

Created attachment 280573 [details] [review]
Gio::SimplePermission wrapper.

It looks much better now. Just a few rather unimportant comments.

Why did you delete the @newin directive in the description of class Permission?

Why _IGNORE(g_permission_impl_update)? It's not ignored.

I noticed now that in TlsConnection::handshake_async() the 'slot' parameter
comes before the optional 'cancellable' parameter, although 'cancellable' comes
before 'callback' in g_tls_connection_handshake_async(). The parameters have
been swapped in the same way in the corresponding _WRAP_VFUNC.
That's a good idea: Put the compulsory parameters first. If you like, you can
do the same in acquire_async() and release_async().

What about the emacs mode lines? I have no definit opinion. I've posted a
question on gtkmm-list,
https://mail.gnome.org/archives/gtkmm-list/2014-July/msg00031.html
Let's see if I get any comments. Meanwhile, do as you like with those lines.

Thank you. Please find below my comments:

- I though you wanted me to remove the @newin directives because Permission is being added to glibmm a few versions later than GPermission entered glib. Now I realize that the only  problem is that I set 2.42 (with in fact is the next version) instead of 2.26, where GPermission actually entered glib. Sorry.

- Yes, I put g_permission_impl_updated under the protected section; I don't know why I also tried to ignore it.

- Agreed. I didn't notice. Although this results in an inconsistency wrt C API; well, I think it is a good idea anyways.

Regarding the modelines... I didn't responded to your email because I was waiting for more experienced people to respond. My opinion is that since the modeline is not exhaustively included in every source file, maybe it is better to not include it in newly written files.

Created attachment 280671 [details] [review]
Gio::Permission wrapper.

(In reply to comment #10)
> realize that the only  problem is that I set 2.42 (with in fact is the next
> version) instead of 2.26, where GPermission actually entered glib. Sorry.

No, you didn't. You set @newin{2,26}, but I would have set @newin{2,42} if I
had written permission.hg and simplepermission.hg.

> - Agreed. I didn't notice. Although this results in an inconsistency wrt C API;
> well, I think it is a good idea anyways.

In tlsconnection.hg the parameters are swapped in the corresponding vfuncs too.
Translated to permission.hg:

  _WRAP_VFUNC(void acquire_async(const SlotAsyncReady& slot{callback},
    const Glib::RefPtr<Cancellable>& cancellable{.}), acquire_async,
    slot_name slot, slot_callback SignalProxy_async_callback)

  _WRAP_VFUNC(void release_async(const SlotAsyncReady& slot{callback},
    const Glib::RefPtr<Cancellable>& cancellable{.}), release_async,
    slot_name slot, slot_callback SignalProxy_async_callback)

This is a matter of taste. Do you want Permission::acquire_async_vfunc() to
resemble Permission::acquire_async() or the vfunc GPermission::acquire_async?


You don't have too attach more patches with Gio::Permission and
SimplePermission before you push them. If you wish, you can push them as they
are now. Even if you make more modifications, they will be small.