Bug 170238

Created attachment 305727 [details] Patch

Comment on attachment 305727 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305727&action=review Nice! > Source/WebCore/PAL/ChangeLog:20 > + The address of the managed handle can be retrieved through the address-of > + operator. An implicit conversion operator is also added. This allows > + frictionless use of GCrypt::Handle<> objects with existing libgcrypt APIs. It's also not how resources handles work in WebKit or the standard library. We add a .get() method to get a pointer to the underlying object. So does the standard library. Why not follow this convention? > Source/WebCore/PAL/ChangeLog:24 > + the managed handle. The raw handle value is also retrieveable through > + the handle() method. Do you really prefer .handle() to .get()? > Source/WebCore/PAL/ChangeLog:41 > + std::unique_ptr<> could be used, but it could also be mis-used. I find > + a mini-class with an interface that's specific to libgcrypt API > + interactions to be preferrable to a std::unique_ptr<> with a custom > + deleter. Yes, although I would have expected GCrypt::Handle to be implemented in terms of std::unique_ptr. Any particular reason you decided not to do that? > Source/WebCore/PAL/pal/crypto/gcrypt/Handle.h:31 > +namespace GCrypt { This is the top-level namespace? Surely it should be put under PAL as well? I know it's going to be annoying to type PAL::GCrypt everywhere we use it, but that's a generic problem of PAL and not something to be solved here. > Source/WebCore/PAL/pal/crypto/gcrypt/Handle.h:66 > + T release() Why does it return a T and not a T*? I guess the handle is intended to be copied by value mostly? > Source/WebCore/PAL/pal/crypto/gcrypt/Handle.h:75 > + T handle() const { return m_handle; } Same question here. I suppose it does make a lot of sense to call this .handle() instead of .get() if it's going to be returning a value and not a pointer. > Source/WebCore/PAL/pal/crypto/gcrypt/Handle.h:76 > + operator T() const { return m_handle; } The implicit conversion makes me a little nervous, since it does not match our pattern for smart pointers, but I suppose this is really a different type of object, so I think it's OK. Certainly it would be more convenient to use. > Source/WebCore/crypto/gcrypt/CryptoAlgorithmHMACGCrypt.cpp:38 > +#include <PAL/pal/crypto/gcrypt/Handle.h> No, this isn't OK. We don't use absolute paths to include directories in WebKit. I'd actually prefer if we did, but we're not going to change that today. You'll need to add PAL/pal/crypto/gcrypt to the include path.

Comment on attachment 305727 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305727&action=review >> Source/WebCore/PAL/ChangeLog:41 >> + deleter. > > Yes, although I would have expected GCrypt::Handle to be implemented in terms of std::unique_ptr. Any particular reason you decided not to do that? I wanted to limit how the Handle class could be used. Handle e.g. doesn't allow moving the managed resource to other objects, but we'll see if that actually makes sense. std::unique_ptr<> manages pointers, while with libgcrypt all is done through handles. There's difference in the two concepts, but the truth is that libgcrypt in most cases, if not all, sets up the handle type as a simple alias to a pointer type. E.g. gcry_mac_hd_t is an alias to struct gcry_mac_handle. But still, IMO using std::unique_ptr<struct gcry_mac_handle> borders on API misuse. That said, looking at std::unique_ptr<> documentation, it might be possible to somehow use handles through it by specifying the `pointer` type on the Deleter object, which would then be an alias to the handle type. The other possibility is to take the handle type and then set up a Handle<T> type alias against std::unique_ptr<std::remove_pointer<T>, Deleter<T>>, making the assumption that all the handle types wrapped in Handle are originally type aliases to pointers. I'll revisit this later. >> Source/WebCore/PAL/pal/crypto/gcrypt/Handle.h:31 >> +namespace GCrypt { > > This is the top-level namespace? Surely it should be put under PAL as well? I know it's going to be annoying to type PAL::GCrypt everywhere we use it, but that's a generic problem of PAL and not something to be solved here. I forgot about it. I'll add it inside the PAL namespace. >> Source/WebCore/PAL/pal/crypto/gcrypt/Handle.h:66 >> + T release() > > Why does it return a T and not a T*? I guess the handle is intended to be copied by value mostly? Correct. >> Source/WebCore/PAL/pal/crypto/gcrypt/Handle.h:76 >> + operator T() const { return m_handle; } > > The implicit conversion makes me a little nervous, since it does not match our pattern for smart pointers, but I suppose this is really a different type of object, so I think it's OK. Certainly it would be more convenient to use. I'll revisit this as well. It is convenient, but also a bit out of the ordinary. >> Source/WebCore/crypto/gcrypt/CryptoAlgorithmHMACGCrypt.cpp:38 >> +#include <PAL/pal/crypto/gcrypt/Handle.h> > > No, this isn't OK. We don't use absolute paths to include directories in WebKit. I'd actually prefer if we did, but we're not going to change that today. You'll need to add PAL/pal/crypto/gcrypt to the include path. I failed to spot the standard PAL header inclusion policy in other files. I'll fix it.

Created attachment 305762 [details] Patch for landing

Comment on attachment 305762 [details] Patch for landing Clearing flags on attachment: 305762 Committed r214550: <http://trac.webkit.org/changeset/214550>

All reviewed patches have been landed. Closing bug.