Summary: | [EFL][WebGL] Minor Refactor in GraphicsContext3DEfl. | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kalyan <kalyan.kondapally> | ||||||||||
Component: | WebKit EFL | Assignee: | Kalyan <kalyan.kondapally> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | dino, kenneth, lucas.de.marchi, noam, webkit.review.bot, zeno | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | Linux | ||||||||||||
Bug Depends on: | 104652 | ||||||||||||
Bug Blocks: | 103105 | ||||||||||||
Attachments: |
|
Description
Kalyan
2012-12-10 06:46:36 PST
Created attachment 178564 [details]
patch
Created attachment 178574 [details]
patch
Attachment 178574 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:150: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Total errors found: 1 in 10 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 178574 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=178574&action=review > Source/WebCore/ChangeLog:9 > + > + With this patch we use the same FBO for canvas and backbuffer of offscreen surface. > + Also, removes some unused code. You should explain why that is a good things etc. What it really means > Source/WebCore/ChangeLog:23 > + * platform/graphics/efl/GraphicsContext3DEfl.cpp: > + (WebCore::GraphicsContext3D::createGraphicsSurfaces): > + * platform/graphics/efl/GraphicsContext3DPrivate.cpp: > + (GraphicsContext3DPrivate::canvasResized): > + (GraphicsContext3DPrivate::copyToGraphicsSurface): > + * platform/graphics/efl/GraphicsContext3DPrivate.h: > + (GraphicsContext3DPrivate): > + * platform/graphics/opengl/GLDefs.h: > + * platform/graphics/opengl/GLPlatformContext.h: > + * platform/graphics/opengl/GLPlatformSurface.h: > + * platform/graphics/surfaces/glx/GLXSurface.h: Please add more info about your changes here. Especially when you refactor > Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:236 > + m_private->canvasResized(); didResizeCanvas() seems more consistent with other webcore code > Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:146 > + // We should copy the full buffer, and not respect the > + // current scissor bounds. keep on one line > Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:147 > + // FIXME::It would be more efficient to track the state of the scissor test. not two ::, just "FIXME: " >> Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:150 >> + // m_context->disable(GraphicsContext3D::SCISSOR_TEST); > > Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Dont ever commit outcommented code! > Source/WebCore/platform/graphics/opengl/GLPlatformSurface.h:56 > + // Creates FBO used by the surface. > + // Buffers can be bound to this FBO. one line! > Source/WebCore/platform/graphics/opengl/GLPlatformSurface.h:57 > + void initialize(GLuint *frameBufferId); * placement! > Source/WebCore/platform/graphics/opengl/GLPlatformSurface.h:70 > - // a)Blits texture contents to back buffer. > + // a)Blits back buffer contents to front buffer. > // b)Calls Swap Buffers. > - // c)Sets current FBO as bindFboId and binds texture to bindTexture. > - virtual void updateContents(const uint32_t texture, const IntRect& sourceRect, const GLuint bindFboId, const uint32_t bindTexture); > + // c)Sets current FBO as bindFboId. space after ) Created attachment 178581 [details]
proposedpatch
patch for review
Comment on attachment 178581 [details]
proposedpatch
r- given my other comments
Created attachment 178628 [details]
patchv3
(In reply to comment #4) > (From update of attachment 178574 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=178574&action=review > > > Source/WebCore/ChangeLog:9 > > + > > + With this patch we use the same FBO for canvas and backbuffer of offscreen surface. > > + Also, removes some unused code. > > You should explain why that is a good things etc. What it really means > > > Source/WebCore/ChangeLog:23 > > + * platform/graphics/efl/GraphicsContext3DEfl.cpp: > > + (WebCore::GraphicsContext3D::createGraphicsSurfaces): > > + * platform/graphics/efl/GraphicsContext3DPrivate.cpp: > > + (GraphicsContext3DPrivate::canvasResized): > > + (GraphicsContext3DPrivate::copyToGraphicsSurface): > > + * platform/graphics/efl/GraphicsContext3DPrivate.h: > > + (GraphicsContext3DPrivate): > > + * platform/graphics/opengl/GLDefs.h: > > + * platform/graphics/opengl/GLPlatformContext.h: > > + * platform/graphics/opengl/GLPlatformSurface.h: > > + * platform/graphics/surfaces/glx/GLXSurface.h: > > Please add more info about your changes here. Especially when you refactor > > > Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:236 > > + m_private->canvasResized(); > > didResizeCanvas() seems more consistent with other webcore code > > > Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:146 > > + // We should copy the full buffer, and not respect the > > + // current scissor bounds. > > keep on one line > > > Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:147 > > + // FIXME::It would be more efficient to track the state of the scissor test. > > not two ::, just "FIXME: " > > >> Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:150 > >> + // m_context->disable(GraphicsContext3D::SCISSOR_TEST); > > > > Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] > > Dont ever commit outcommented code! > > > Source/WebCore/platform/graphics/opengl/GLPlatformSurface.h:56 > > + // Creates FBO used by the surface. > > + // Buffers can be bound to this FBO. > > one line! > > > Source/WebCore/platform/graphics/opengl/GLPlatformSurface.h:57 > > + void initialize(GLuint *frameBufferId); > > * placement! > > > Source/WebCore/platform/graphics/opengl/GLPlatformSurface.h:70 > > - // a)Blits texture contents to back buffer. > > + // a)Blits back buffer contents to front buffer. > > // b)Calls Swap Buffers. > > - // c)Sets current FBO as bindFboId and binds texture to bindTexture. > > - virtual void updateContents(const uint32_t texture, const IntRect& sourceRect, const GLuint bindFboId, const uint32_t bindTexture); > > + // c)Sets current FBO as bindFboId. > > space after ) done. Comment on attachment 178628 [details] patchv3 Clearing flags on attachment: 178628 Committed r137211: <http://trac.webkit.org/changeset/137211> All reviewed patches have been landed. Closing bug. This seems to break all of webgl for me! |