Summary: | [EFL][EGL] Implement TransportSurface client. | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kalyan <kalyan.kondapally> | ||||||||||||||
Component: | WebKit EFL | Assignee: | Kalyan <kalyan.kondapally> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | buildbot, cmarcelo, commit-queue, lucas.de.marchi, luiz, noam, rniwa, sergio, webkit.review.bot, zeno | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | PC | ||||||||||||||||
OS: | Linux | ||||||||||||||||
Bug Depends on: | |||||||||||||||||
Bug Blocks: | 105659 | ||||||||||||||||
Attachments: |
|
Description
Kalyan
2013-04-07 10:16:01 PDT
Created attachment 196799 [details]
patch
Comment on attachment 196799 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=196799&action=review > Source/WebCore/ChangeLog:12 > + > + This patch implements the client support for Transport > + Surface. > + You should say why? and what that means... like improves blah blah on X11 or so... > Source/WebCore/platform/graphics/surfaces/egl/EGLConfigSelector.cpp:182 > + EGLint numConfigs, i, visualId; We normally use separate lines > Source/WebCore/platform/graphics/surfaces/egl/EGLConfigSelector.cpp:200 > + int alphaRequired = m_attributes & GLPlatformSurface::SupportAlpha ? 8 : 0; alphaChannelsRequired? > Source/WebCore/platform/graphics/surfaces/egl/EGLHelper.h:51 > + enum DisplayType { > + NativeDisplay = 0x00, > + CurrentDisplay = 0x01 > + }; so why cant the current display be native? > Source/WebCore/platform/graphics/surfaces/egl/EGLSurface.cpp:184 > + if (m_surface != EGL_NO_SURFACE) { > + > + if (!eglBindTexImage(EGLHelper::eglDisplay(), m_surface, EGL_BACK_BUFFER)) > + destroy(); > + } > + } why not combine? Created attachment 200732 [details]
patchv2
(In reply to comment #2) > (From update of attachment 196799 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=196799&action=review > > Source/WebCore/platform/graphics/surfaces/egl/EGLConfigSelector.cpp:200 > > + int alphaRequired = m_attributes & GLPlatformSurface::SupportAlpha ? 8 : 0; > > alphaChannelsRequired? done > > Source/WebCore/platform/graphics/surfaces/egl/EGLHelper.h:51 > > + enum DisplayType { > > + NativeDisplay = 0x00, > > + CurrentDisplay = 0x01 > > + }; > > so why cant the current display be native? Removed its usage. It was to avoid opening new display connections.Changed it so that we check for any valid display connection before opening a new one. Attachment 200732 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/surfaces/efl/GLTransportSurface.cpp', u'Source/WebCore/platform/graphics/surfaces/efl/GLTransportSurface.h', u'Source/WebCore/platform/graphics/surfaces/efl/GraphicsSurfaceCommon.cpp', u'Source/WebCore/platform/graphics/surfaces/egl/EGLConfigSelector.cpp', u'Source/WebCore/platform/graphics/surfaces/egl/EGLConfigSelector.h', u'Source/WebCore/platform/graphics/surfaces/egl/EGLHelper.cpp', u'Source/WebCore/platform/graphics/surfaces/egl/EGLHelper.h', u'Source/WebCore/platform/graphics/surfaces/egl/EGLSurface.cpp', u'Source/WebCore/platform/graphics/surfaces/egl/EGLSurface.h', u'Source/WebCore/platform/graphics/surfaces/egl/EGLXSurface.cpp', u'Source/WebCore/platform/graphics/surfaces/egl/EGLXSurface.h', u'Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.cpp', u'Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.h', u'Source/WebCore/platform/graphics/surfaces/glx/X11Helper.cpp', u'Source/WebCore/platform/graphics/surfaces/glx/X11Helper.h']" exit_code: 1
Source/WebCore/platform/graphics/surfaces/egl/EGLHelper.cpp:130: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3]
Source/WebCore/platform/graphics/surfaces/egl/EGLHelper.cpp:151: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3]
Source/WebCore/platform/graphics/surfaces/egl/EGLXSurface.cpp:189: Missing space before { [whitespace/braces] [5]
Source/WebCore/platform/graphics/surfaces/egl/EGLXSurface.cpp:268: Missing space before { [whitespace/braces] [5]
Source/WebCore/platform/graphics/surfaces/egl/EGLHelper.h:50: Use 0 instead of NULL. [readability/null] [5]
Total errors found: 5 in 16 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 200732 [details]
patchv2
will upload a new patch after fixing style issues
Created attachment 200765 [details]
patchv3
Comment on attachment 200765 [details] patchv3 Attachment 200765 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/373073 New failing tests: fast/frames/crash-remove-iframe-during-object-beforeload.html Created attachment 200812 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.2
Created attachment 203327 [details]
patchv4
Comment on attachment 203327 [details] patchv4 View in context: https://bugs.webkit.org/attachment.cgi?id=203327&action=review > Source/WebCore/platform/graphics/surfaces/egl/EGLHelper.cpp:154 > + tempHandle = eglCreateImageKHR(display, EGL_NO_CONTEXT, target, clientBuffer, attributes); double space before eglCreate... > Source/WebCore/platform/graphics/surfaces/egl/EGLXSurface.cpp:150 > + if (!handle) > + return; > + > + m_handle = handle; why is it better to have m_handle be undefined? why not always do m_handle = handle then return after? > Source/WebCore/platform/graphics/surfaces/egl/EGLXSurface.cpp:173 > + m_totalBytes = m_size.width() * m_size.height() * 4; > +#if USE(OPENGL_ES_2) i would have a newline after m_totalByes = > Source/WebCore/platform/graphics/surfaces/egl/EGLXSurface.cpp:175 > + static bool bgraSupported = GLPlatformContext::supportsGLExtension("GL_EXT_texture_format_BGRA8888"); two spaces before variable name > Source/WebCore/platform/graphics/surfaces/egl/EGLXSurface.cpp:214 > + // Fallback to use XImage in case EGLImage and TextureToPixmap are not supported. Have you tested these code paths? > Source/WebCore/platform/graphics/surfaces/egl/EGLXSurface.cpp:216 > + m_image = XGetImage(NativeWrapper::nativeDisplay(), m_handle, 0, 0, m_size.width(), m_size.height(), AllPlanes, ZPixmap); > +#if USE(OPENGL_ES_2) newline after m_image = > Source/WebCore/platform/graphics/surfaces/egl/EGLXSurface.cpp:221 > +#endif newline after (In reply to comment #11) > (From update of attachment 203327 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=203327&action=review > > > Source/WebCore/platform/graphics/surfaces/egl/EGLXSurface.cpp:150 > > + if (!handle) > > + return; > > + > > + m_handle = handle; > > why is it better to have m_handle be undefined? why not always do m_handle = handle then return after? > It doesn't make any difference, as the client creation should fail in the case when handle is not valid. > > Source/WebCore/platform/graphics/surfaces/egl/EGLXSurface.cpp:214 > > + // Fallback to use XImage in case EGLImage and TextureToPixmap are not supported. > > Have you tested these code paths? Yes, I did test them. Created attachment 203332 [details]
patchv5
Comment on attachment 203332 [details] patchv5 Clearing flags on attachment: 203332 Committed r150958: <http://trac.webkit.org/changeset/150958> All reviewed patches have been landed. Closing bug. |