Bug 106666 - [EFL][Qt][WebGL] Share the common code between GraphicsSurfaceGLX and X11WindowResources.
Summary: [EFL][Qt][WebGL] Share the common code between GraphicsSurfaceGLX and X11Wind...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Kalyan
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-01-11 07:49 PST by Kalyan
Modified: 2013-02-09 07:11 PST (History)
12 users (show)

See Also:


Attachments
WIP (25.96 KB, patch)
2013-01-17 07:01 PST, Kalyan
no flags Details | Formatted Diff | Diff
patch (29.12 KB, patch)
2013-01-17 08:42 PST, Kalyan
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
patchv3 (30.72 KB, patch)
2013-01-17 17:40 PST, Kalyan
no flags Details | Formatted Diff | Diff
patch (59.30 KB, patch)
2013-02-03 13:43 PST, Kalyan
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
WIP (57.78 KB, patch)
2013-02-03 17:09 PST, Kalyan
no flags Details | Formatted Diff | Diff
patch (57.80 KB, patch)
2013-02-04 12:30 PST, Kalyan
no flags Details | Formatted Diff | Diff
patch (57.38 KB, patch)
2013-02-04 12:32 PST, Kalyan
no flags Details | Formatted Diff | Diff
patchv2 (57.35 KB, patch)
2013-02-04 13:58 PST, Kalyan
buildbot: commit-queue-
Details | Formatted Diff | Diff
patchv3 (58.30 KB, patch)
2013-02-06 12:37 PST, Kalyan
kenneth: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
patch (58.30 KB, patch)
2013-02-08 00:53 PST, Kalyan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kalyan 2013-01-11 07:49:00 PST
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
Comment 1 Kalyan 2013-01-17 07:01:32 PST
Created attachment 183180 [details]
WIP
Comment 2 Kalyan 2013-01-17 08:42:45 PST
Created attachment 183196 [details]
patch
Comment 3 Early Warning System Bot 2013-01-17 08:48:21 PST
Comment on attachment 183196 [details]
patch

Attachment 183196 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15906947
Comment 4 Early Warning System Bot 2013-01-17 08:48:40 PST
Comment on attachment 183196 [details]
patch

Attachment 183196 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15938158
Comment 5 Zeno Albisser 2013-01-17 08:58:51 PST
I'm looking at the issues reported by Qt EWS.
Comment 6 Kalyan 2013-01-17 17:40:20 PST
Created attachment 183329 [details]
patchv3
Comment 7 Kalyan 2013-01-17 17:59:30 PST
Comment on attachment 183329 [details]
patchv3

Will upload a rebased patch
Comment 8 Kalyan 2013-02-03 13:43:00 PST
Created attachment 186267 [details]
patch
Comment 9 Kenneth Rohde Christiansen 2013-02-03 13:53:53 PST
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
Comment 10 Early Warning System Bot 2013-02-03 13:56:35 PST
Comment on attachment 186267 [details]
patch

Attachment 186267 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16351753
Comment 11 Early Warning System Bot 2013-02-03 13:57:52 PST
Comment on attachment 186267 [details]
patch

Attachment 186267 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16360705
Comment 12 Build Bot 2013-02-03 14:46:02 PST
Comment on attachment 186267 [details]
patch

Attachment 186267 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16346882
Comment 13 Kalyan 2013-02-03 17:09:38 PST
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.
Comment 14 Kalyan 2013-02-03 17:11:01 PST
(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.
Comment 15 Zeno Albisser 2013-02-04 02:30:31 PST
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!
Comment 16 Kalyan 2013-02-04 12:30:28 PST
Created attachment 186437 [details]
patch
Comment 17 Kalyan 2013-02-04 12:31:21 PST
Comment on attachment 186437 [details]
patch

will upload a rebased patch
Comment 18 Kalyan 2013-02-04 12:32:29 PST
Created attachment 186439 [details]
patch
Comment 19 Kalyan 2013-02-04 13:58:03 PST
Created attachment 186453 [details]
patchv2

review fixes
Comment 20 Kalyan 2013-02-04 13:58:50 PST
(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
Comment 21 Build Bot 2013-02-04 17:31:49 PST
Comment on attachment 186453 [details]
patchv2

Attachment 186453 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16375439
Comment 22 Zeno Albisser 2013-02-06 06:59:05 PST
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?
Comment 23 Kalyan 2013-02-06 07:39:08 PST
(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.
Comment 24 Zeno Albisser 2013-02-06 08:10:48 PST
(In reply to comment #23)

LGTM
Comment 25 Kalyan 2013-02-06 12:37:53 PST
Created attachment 186896 [details]
patchv3

Rebased patch
Comment 26 WebKit Review Bot 2013-02-07 06:16:06 PST
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
Comment 27 Kalyan 2013-02-08 00:53:09 PST
Created attachment 187260 [details]
patch
Comment 28 WebKit Review Bot 2013-02-09 07:11:46 PST
Comment on attachment 187260 [details]
patch

Clearing flags on attachment: 187260

Committed r142355: <http://trac.webkit.org/changeset/142355>
Comment 29 WebKit Review Bot 2013-02-09 07:11:52 PST
All reviewed patches have been landed.  Closing bug.