Bug 106666 – [EFL][Qt][WebGL] Share the common code between GraphicsSurfaceGLX and X11WindowResources.

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Kalyan]

X11WindowResources contains the X related code which is used by both of our GLX and EGL implementaion. GraphicsSurfaceGLX also has similar code which could be shared by both the classes. This would avoid any code duplication and need to fix bugs in both places. Same case with GLXConfigSelector

[Kalyan]

Created attachment 183180 [details]
WIP

[Kalyan]

Created attachment 183196 [details]
patch

[Early Warning System Bot]

Comment on attachment 183196 [details]
patch

Attachment 183196 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15906947

[Early Warning System Bot]

Comment on attachment 183196 [details]
patch

Attachment 183196 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15938158

[Zeno Albisser]

I'm looking at the issues reported by Qt EWS.

[Kalyan]

Created attachment 183329 [details]
patchv3

[Kalyan]

Comment on attachment 183329 [details]
patchv3

Will upload a rebased patch

[Kalyan]

Created attachment 186267 [details]
patch

[Kenneth Rohde Christiansen]

Comment on attachment 186267 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=186267&action=review

> Source/WebCore/platform/graphics/surfaces/glx/X11Wrapper.cpp:174
> +}
> +#endif
> +
> +#endif

I hate nested ifdefs. Can they be avoided?

> Source/WebCore/platform/graphics/surfaces/glx/X11Wrapper.h:54
> +// Used for handling XError.
> +static bool validOperation = true;
> +static int handleXPixmapCreationError(Display*, XErrorEvent* event)
> +{
> +    if (event->error_code == BadMatch || event->error_code == BadWindow || event->error_code == BadAlloc) {
> +        validOperation = false;
> +
> +        switch (event->error_code) {
> +        case BadMatch:

why is this in the header?

> Source/WebCore/platform/graphics/surfaces/glx/X11Wrapper.h:96
> +        // XSync must be called to ensure that current errors are handled by the original handler.
> +        XSync(X11Wrapper::nativeDisplay(), false);
> +        m_previousErrorHandler = XSetErrorHandler(handleXPixmapCreationError);
> +    }
> +

This is more than a simple implementation, so why is it in the header

[Early Warning System Bot]

Comment on attachment 186267 [details]
patch

Attachment 186267 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16351753

[Early Warning System Bot]

Comment on attachment 186267 [details]
patch

Attachment 186267 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16360705

[Build Bot]

Comment on attachment 186267 [details]
patch

Attachment 186267 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16346882

[Kalyan]

Created attachment 186280 [details]
WIP

Looking into the build issue with Qt. Will mark it for review once I get to resolve it but feel free to give any feedback.

[Kalyan]

(In reply to comment #9)
> (From update of attachment 186267 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186267&action=review
> 
> > Source/WebCore/platform/graphics/surfaces/glx/X11Wrapper.cpp:174
> > +}
> > +#endif
> > +
> > +#endif
> 
> I hate nested ifdefs. Can they be avoided?

Yes, done.
 
> why is this in the header?
> > Source/WebCore/platform/graphics/surfaces/glx/X11Wrapper.h:96
> > +
> 
> This is more than a simple implementation, so why is it in the header

Moved the implementation to cpp file.

[Zeno Albisser]

Comment on attachment 186280 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=186280&action=review

> Source/WebCore/platform/graphics/surfaces/glx/GLXConfigSelector.h:159
> +    {

This looks like duplicate code. Would it be possible to have something like:
GLXFBConfig findMatchingConfig(const int attributes[], int depth, VisualID id = 0)
?

> Source/WebCore/platform/graphics/surfaces/glx/X11Wrapper.h:47
> +

nit: I don't really like the name X11Wrapper. It sounds highly generic and looks rather like a Helper or a convenience class to me than a Wrapper.
How about X11OffScreenHelper, or just X11Helper ?


All in all, i would say this is a very nice piece of work! :)
Thanks a lot!

[Kalyan]

Created attachment 186437 [details]
patch

[Kalyan]

Comment on attachment 186437 [details]
patch

will upload a rebased patch

[Kalyan]

Created attachment 186439 [details]
patch

[Kalyan]

Created attachment 186453 [details]
patchv2

review fixes

[Kalyan]

(In reply to comment #15)
> (From update of attachment 186280 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186280&action=review

> > Source/WebCore/platform/graphics/surfaces/glx/X11Wrapper.h:47
> > +

done

> nit: I don't really like the name X11Wrapper. It sounds highly generic and looks rather like a Helper or a convenience class to me than a Wrapper.
> How about X11OffScreenHelper, or just X11Helper ?
> 
 renamed it as X11Helper

[Build Bot]

Comment on attachment 186453 [details]
patchv2

Attachment 186453 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16375439

[Zeno Albisser]

Comment on attachment 186453 [details]
patchv2

View in context: https://bugs.webkit.org/attachment.cgi?id=186453&action=review

> Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:-330
> -        XFree(configs);

don't you need to free the configs received from m_configSelector->surfaceclientConfig(..) somewhere again?

[Kalyan]

(In reply to comment #22)
> (From update of attachment 186453 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186453&action=review
> 
> > Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:-330
> > -        XFree(configs);
> 
> don't you need to free the configs received from m_configSelector->surfaceclientConfig(..) somewhere again?

findMatchingConfig(uses OwnPtrX11) in GLXConfigSelector should take care of freeing XResources automatically once it goes out of scope. We make a temp copy(on stack) of the selected config.

[Zeno Albisser]

(In reply to comment #23)

LGTM

[Kalyan]

Created attachment 186896 [details]
patchv3

Rebased patch

[WebKit Review Bot]

Comment on attachment 186896 [details]
patchv3

Rejecting attachment 186896 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-04', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 186896, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
rce/WebKit/chromium/webkit/media/crypto/ppapi/cdm --revision 173055 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
56>At revision 173055.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/16426166

[Kalyan]

Created attachment 187260 [details]
patch

[WebKit Review Bot]

Comment on attachment 187260 [details]
patch

Clearing flags on attachment: 187260

Committed r142355: <http://trac.webkit.org/changeset/142355>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.