[chromium] Make the GLES2 texture map generic and teach ImageSkia and ImageBufferSkia about GLES2
Created attachment 63005 [details] Patch
Comment on attachment 63005 [details] Patch WebCore/platform/graphics/chromium/GLES2Canvas.cpp:476 + GLES2Texture* GLES2Canvas::createTexture(const void* ptr, GLES2Texture::Format format, int width, int height) ewwww... it is really easy to have casting errors when passing class pointers to a function expecting a void*. this can be especially nasty when multiple inheritance is involved. we've been burned by things like this before. what do you think about defining a common base class that all types that can be passed here must implement? or, how about a struct/union that has member pointers for each supported pointer type? WebCore/platform/graphics/chromium/GLES2Canvas.h:52 + typedef HashMap<const void *, RefPtr<GLES2Texture> > TextureHashMap; nit: no space between "void" and "*". there are several others like this one. otherwise, this LGTM
(In reply to comment #2) > (From update of attachment 63005 [details]) > WebCore/platform/graphics/chromium/GLES2Canvas.cpp:476 > + GLES2Texture* GLES2Canvas::createTexture(const void* ptr, GLES2Texture::Format format, int width, int height) > ewwww... it is really easy to have casting errors when passing class pointers > to a function expecting a void*. this can be especially nasty when multiple > inheritance is involved. we've been burned by things like this before. > what do you think about defining a common base class that all types that > can be passed here must implement? or, how about a struct/union that has > member pointers for each supported pointer type? or even simpler: provide overrides of createTexture and getTexture for each supported input pointer type.
(In reply to comment #2) > (From update of attachment 63005 [details]) > WebCore/platform/graphics/chromium/GLES2Canvas.cpp:476 > + GLES2Texture* GLES2Canvas::createTexture(const void* ptr, GLES2Texture::Format format, int width, int height) > ewwww... it is really easy to have casting errors when passing class pointers > to a function expecting a void*. this can be especially nasty when multiple > inheritance is involved. we've been burned by things like this before. > what do you think about defining a common base class that all types that > can be passed here must implement? or, how about a struct/union that has > member pointers for each supported pointer type? Since this change is my fault, I'll try to justify it: The texture hash map only uses the pointer as an opaque key. It's never dereferenced, nor is it cast to anything else. So there's no danger of casting errors. The only danger of a caller passing in a pointer of the wrong type would be that no texture would be found, or a newly-created texture would be associated with the wrong pointer. But errors like that would be quickly noticed due to bad performance. The original reason was for constness: there was a const_cast in ImageSkia.cpp I wanted to get rid of, and the compiler was giving me grief with "const NativeImagePtr" as the hash key. I think I would have had to define a NativeImagePtrConst for each platform. Another reason for making it opaque is that it would discourage anyone from making any member/fn calls dependent on the platform-specific NativeImagePtr. I was also concerned that NativeImagePtr might not work in a hash map on all platforms.
(In reply to comment #4) ... > Since this change is my fault, I'll try to justify it: > > The texture hash map only uses the pointer as an opaque key. It's never dereferenced, nor is it cast to anything else. So there's no danger of casting errors. The only danger of a caller passing in a pointer of the wrong type would be that no texture would be found, or a newly-created texture would be associated with the wrong pointer. But errors like that would be quickly noticed due to bad performance. Hmm... that's exactly the kind of thing I was hoping we could avoid. The result is not a crash implicating a certain piece of code, instead it is just degraded performance. In the past, code like this led to memory leaks that were hard to track down. I'm just saying that the pattern is to be avoided. > The original reason was for constness: there was a const_cast in ImageSkia.cpp I wanted to get rid of, and the compiler was giving me grief with "const NativeImagePtr" as the hash key. I think I would have had to define a NativeImagePtrConst for each platform. > > Another reason for making it opaque is that it would discourage anyone from making any member/fn calls dependent on the platform-specific NativeImagePtr. I was also concerned that NativeImagePtr might not work in a hash map on all platforms. I see. I did not realize that there were other reasons to hide the fact that it was a NativeImagePtr from this code.
Comment on attachment 63005 [details] Patch R=me but please consider alternatives that avoid risky void*. CQ- for the style issues mentioned earlier.
Created attachment 63100 [details] Patch
As it turns out NativeImagePtr works fine as a hash key, so switched back to using that. The only possible issue is that on WinCE NativeImagePtr is typedef'd to RefPtr instead of a raw pointer. However, I can't imagine this code ever running on WinCE anyway so I think it's fine to ignore that for now.
Comment on attachment 63100 [details] Patch OK, cool. R=me
(In reply to comment #8) > As it turns out NativeImagePtr works fine as a hash key, so switched back to using that. The only possible issue is that on WinCE NativeImagePtr is typedef'd to RefPtr instead of a raw pointer. However, I can't imagine this code ever running on WinCE anyway so I think it's fine to ignore that for now. Great. My bad -- I was led astray by the const NativeImageSkia* in ImageSkia (which as James pointed out to me, is unnecessary -- non-const works fine here). Just FYI, the problem I had with HashMap turned out to be that "const NativeImagePtr" actually maps to "NativeImageSkia* const", not "const NativeImageSkia*", and the DefaultHash function used by HashMap has no specialization for T * const. This could be fixed many ways, but keeping it non-const is probably the easiest.
Comment on attachment 63100 [details] Patch Clearing flags on attachment: 63100 Committed r64374: <http://trac.webkit.org/changeset/64374>
All reviewed patches have been landed. Closing bug.