Up to now, we were using a uint64_t as a token to identify a GraphicsSurface. For mac and linux this seemed to be straight forward and good enough, even though not perfectly well designed. With the upcomping GraphicsSurface implementation for Windows, 64bit will not be sufficient for a token. A token will have to contain two native HANDLES which are of type (void*). On a 64bit build we would then need 128bits. We should therefore wrap any kind of native handles/windowid/IOSurfaceRef into a specific GraphicsSurfaceToken class.
Created attachment 167294 [details] patch for review.
Created attachment 167306 [details] patch for review. - rebased.
Comment on attachment 167306 [details] patch for review. - rebased. Attachment 167306 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/14178454
Comment on attachment 167306 [details] patch for review. - rebased. Attachment 167306 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14169743
Created attachment 167320 [details] patch for review. - fixed compilation without use_graphics_surface.
Comment on attachment 167320 [details] patch for review. - fixed compilation without use_graphics_surface. This patch will need another iteration. - I'll post an update asap.
Created attachment 167331 [details] patch for review.
Comment on attachment 167331 [details] patch for review. View in context: https://bugs.webkit.org/attachment.cgi?id=167331&action=review > Source/WebCore/ChangeLog:7 > + This is necessary in order to implement GraphicsSurface for Windows as well. I would remove "as well" > Source/WebCore/ChangeLog:9 > + WindowID (Linux/GLX), two IOSurfaceIDs (Mac) or in future two HANDLEs (Windows). in *the* future > Source/WebCore/platform/graphics/surfaces/GraphicsSurfaceToken.h:41 > +#if OS(LINUX) > + GraphicsSurfaceToken(uint32_t windowID = 0) > + : frontBufferHandle(windowID) > + { } So this covers linux without X11?
Comment on attachment 167331 [details] patch for review. View in context: https://bugs.webkit.org/attachment.cgi?id=167331&action=review >> Source/WebCore/ChangeLog:9 >> + WindowID (Linux/GLX), two IOSurfaceIDs (Mac) or in future two HANDLEs (Windows). > > in *the* future I would tend to agree, but it seems it could actually be correct according to that: http://itre.cis.upenn.edu/~myl/languagelog/archives/004201.html Well, I'd be more comfortable with a comma before and after. Maybe it's just us not being accustomed to british english that much...
Comment on attachment 167331 [details] patch for review. View in context: https://bugs.webkit.org/attachment.cgi?id=167331&action=review >> Source/WebCore/ChangeLog:7 >> + This is necessary in order to implement GraphicsSurface for Windows as well. > > I would remove "as well" I can change that. >>> Source/WebCore/ChangeLog:9 >>> + WindowID (Linux/GLX), two IOSurfaceIDs (Mac) or in future two HANDLEs (Windows). >> >> in *the* future > > I would tend to agree, but it seems it could actually be correct according to that: http://itre.cis.upenn.edu/~myl/languagelog/archives/004201.html > Well, I'd be more comfortable with a comma before and after. > Maybe it's just us not being accustomed to british english that much... And I can change that as well. >> Source/WebCore/platform/graphics/surfaces/GraphicsSurfaceToken.h:41 >> + { } > > So this covers linux without X11? Linux without GLX/X11 will not use GRAPHICS_SURFACE in the first place. So it's not an issue.
Comment on attachment 167331 [details] patch for review. View in context: https://bugs.webkit.org/attachment.cgi?id=167331&action=review >>> Source/WebCore/platform/graphics/surfaces/GraphicsSurfaceToken.h:41 >>> + { } >> >> So this covers linux without X11? > > Linux without GLX/X11 will not use GRAPHICS_SURFACE in the first place. So it's not an issue. Well adding a has X11 to the ifdef would make that more clear. So how will this be done for EGL?
Comment on attachment 167331 [details] patch for review. View in context: https://bugs.webkit.org/attachment.cgi?id=167331&action=review >>>> Source/WebCore/platform/graphics/surfaces/GraphicsSurfaceToken.h:41 >>>> + { } >>> >>> So this covers linux without X11? >> >> Linux without GLX/X11 will not use GRAPHICS_SURFACE in the first place. So it's not an issue. > > Well adding a has X11 to the ifdef would make that more clear. So how will this be done for EGL? Isn't that something we should worry about, once we are implementing a GraphicsSurface for EGL? I mean, it could be that a single uint32_t is perfectly viable for that case. Or it could be that we need two uint32_t to identify a front and a back buffer, as it is necessary for Mac. Or it could also be that we even need two uint64_t as we will need on Windows (ANGLE/EGL). But I do not think it makes sense to try predicting all that before even prototyping such a solution for EGL. Let's take a step at the time.
Comment on attachment 167331 [details] patch for review. View in context: https://bugs.webkit.org/attachment.cgi?id=167331&action=review >>>>> Source/WebCore/platform/graphics/surfaces/GraphicsSurfaceToken.h:41 >>>>> + { } >>>> >>>> So this covers linux without X11? >>> >>> Linux without GLX/X11 will not use GRAPHICS_SURFACE in the first place. So it's not an issue. >> >> Well adding a has X11 to the ifdef would make that more clear. So how will this be done for EGL? > > Isn't that something we should worry about, once we are implementing a GraphicsSurface for EGL? I mean, it could be that a single uint32_t is perfectly viable for that case. Or it could be that we need two uint32_t to identify a front and a back buffer, as it is necessary for Mac. Or it could also be that we even need two uint64_t as we will need on Windows (ANGLE/EGL). > But I do not think it makes sense to try predicting all that before even prototyping such a solution for EGL. Let's take a step at the time. Still, HAVE(GLX) instead of OS(LINUX) makes this more readable and I fail to see the argument against that.
Comment on attachment 167331 [details] patch for review. View in context: https://bugs.webkit.org/attachment.cgi?id=167331&action=review >>>>>> Source/WebCore/platform/graphics/surfaces/GraphicsSurfaceToken.h:41 >>>>>> + { } >>>>> >>>>> So this covers linux without X11? >>>> >>>> Linux without GLX/X11 will not use GRAPHICS_SURFACE in the first place. So it's not an issue. >>> >>> Well adding a has X11 to the ifdef would make that more clear. So how will this be done for EGL? >> >> Isn't that something we should worry about, once we are implementing a GraphicsSurface for EGL? I mean, it could be that a single uint32_t is perfectly viable for that case. Or it could be that we need two uint32_t to identify a front and a back buffer, as it is necessary for Mac. Or it could also be that we even need two uint64_t as we will need on Windows (ANGLE/EGL). >> But I do not think it makes sense to try predicting all that before even prototyping such a solution for EGL. Let's take a step at the time. > > Still, HAVE(GLX) instead of OS(LINUX) makes this more readable and I fail to see the argument against that. How about context? - We distinguish between DARWIN and GLX? I think it is more readable to distinguish between DARWIN and LINUX. Also i think it is not necessary to forcefully disable a code path that we do not even know yet, if it is going to be wrong for EGL. But I am not going to argue too hard about it, it is just a single line after all. - So I will change it. Anything else?
Comment on attachment 167331 [details] patch for review. View in context: https://bugs.webkit.org/attachment.cgi?id=167331&action=review Some nitpicks... otherwise all good :) > Source/WebCore/platform/graphics/texmap/TextureMapperBackingStore.cpp:37 > +void TextureMapperSurfaceBackingStore::setGraphicsSurface(GraphicsSurfaceToken graphicsSurfaceToken, const IntSize& surfaceSize, uint32_t frontBuffer) const &? > Source/WebCore/platform/graphics/texmap/TextureMapperBackingStore.h:53 > + void setGraphicsSurface(GraphicsSurfaceToken, const IntSize& surfaceSize, uint32_t frontBuffer); const &?
Created attachment 167345 [details] patch for review.
Committed r131036: <http://trac.webkit.org/changeset/131036>