Summary: | [Qt] Implement GraphicsSurfaceToken to replace uint64_t as token type. | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Zeno Albisser <zeno> | ||||||||||||
Component: | WebKit2 | Assignee: | Zeno Albisser <zeno> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | dino, gtk-ews, gustavo, noam, philn, webkit.review.bot, xan.lopez | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 98147 | ||||||||||||||
Attachments: |
|
Description
Zeno Albisser
2012-10-05 02:17:04 PDT
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> |