Bug 108735 – [CAIRO][EFL][QT][WebGL] Share common code in GraphicsContext3D.

• Status: RESOLVED
• Resolution: WONTFIX
• Component: WebKit EFL

[Kalyan]

Currently all the three ports have almost the same code related to Gl Resources (initialization ,deletion etc) in GraphicsContext3D. All the platform layer stuff is handled in Private part. ImageExtractor code could also be shared between efl and cairo atleast.

[Kalyan]

Created attachment 186201 [details]
WIP

[Kalyan]

Created attachment 186205 [details]
WIP

[Kalyan]

Created attachment 186242 [details]
WIP

Now we have the common code in GraphicsContext3DCommon.cpp. I placed it under texmap for now. Cairo specific part is still present in GraphicsContext3DCairo and qt specific in GraphicsContext3DQt.cpp  PlatformLayer(GraphicsContext3DPrivate) specific code exists as before.

[Zeno Albisser]

Comment on attachment 186242 [details]
WIP

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

> Source/WebCore/GNUmakefile.list.am:6416
> +       Source/WebCore/platform/graphics/texmap/GraphicsContext3DCommon.cpp

indentation?

> Source/WebCore/GNUmakefile.list.am:6474
> +       Source/WebCore/platform/graphics/texmap/GraphicsContext3DPlatformLayer.h \

indentation?

> Source/WebCore/GNUmakefile.list.am:6483
> +       Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayer.h

whitespace change only?

> Source/WebCore/platform/graphics/cairo/GraphicsContext3DCairo.cpp:68
> +        // 2. For texImage2D with HTMLCanvasElement input in which Alpha is already Premultiplied in this port,

whitespace change only? ... in some more spaces as well. May be check your editor settings.

I think this looks generally okay. :)

[Kalyan]

Created attachment 187131 [details]
WIP

[Martin Robinson]

Comment on attachment 187131 [details]
WIP

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

> Source/WebCore/platform/graphics/texmap/GraphicsContext3DCommon.cpp:1
> +/*

The TextureMapper sits logically on top of GraphicsContext3D, so storing GraphicsContext3D in the texmap directory may not be the most accurate. Perhaps somewhere in opengl?

[Kalyan]

(In reply to comment #6)
> (From update of attachment 187131 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=187131&action=review
> The TextureMapper sits logically on top of GraphicsContext3D, so storing GraphicsContext3D in the texmap directory may not be the most accurate. Perhaps somewhere in opengl?

Agree, this was temporary. Any suggestions for the file name :) I already see GraphicsContext3DOpenGLCommon.cpp under opengl folder

[Kenneth Rohde Christiansen]

What about OpenGLShared... seems more common in the webkit project

[Kalyan]

Created attachment 187162 [details]
patch

[Kalyan]

(In reply to comment #8)
> What about OpenGLShared... seems more common in the webkit project

K, I moved the Common part under opengl and renamed it as SharedGraphicsContext3DOpenGL. Does that sound k or any other proposals are welcome. Other option we have is to include this in GraphicsContext3DOpenGLCommon.cpp and if def it for three ports.

[Kalyan]

Created attachment 187957 [details]
patch

[WebKit Review Bot]

Attachment 187957 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/PlatformEfl.cmake', u'Source/WebCore/Target.pri', u'Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp', u'Source/WebCore/platform/graphics/opengl/SharedGraphicsContext3DOpenGL.cpp', u'Source/WebCore/platform/graphics/qt/GraphicsContext3DPrivate.cpp', u'Source/WebCore/platform/graphics/qt/GraphicsContext3DPrivate.h', u'Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp', u'Source/WebCore/platform/graphics/surfaces/GraphicsSurface.h', u'Source/WebCore/platform/graphics/surfaces/GraphicsSurfaceToken.h', u'Source/WebCore/platform/graphics/texmap/GraphicsContext3DPlatformLayer.cpp', u'Source/WebCore/platform/graphics/texmap/GraphicsContext3DPlatformLayer.h', u'Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayer.h', u'Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.h']" exit_code: 1
Source/WebCore/platform/graphics/qt/GraphicsContext3DPrivate.cpp:103:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 187957 [details]
patch

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

[Early Warning System Bot]

Comment on attachment 187957 [details]
patch

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

[EFL EWS Bot]

Comment on attachment 187957 [details]
patch

Attachment 187957 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16454508

[Kalyan]

Created attachment 188292 [details]
patchv3

second try with ews

[Early Warning System Bot]

Comment on attachment 188292 [details]
patchv3

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

[Early Warning System Bot]

Comment on attachment 188292 [details]
patchv3

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

[Kalyan]

Created attachment 188330 [details]
patchv4

qtbuild break fix

[WebKit Review Bot]

Attachment 188330 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/PlatformEfl.cmake', u'Source/WebCore/Target.pri', u'Source/WebCore/platform/graphics/GraphicsContext3D.h', u'Source/WebCore/platform/graphics/cairo/GraphicsContext3DCairo.cpp', u'Source/WebCore/platform/graphics/cairo/GraphicsContext3DPrivate.cpp', u'Source/WebCore/platform/graphics/cairo/GraphicsContext3DPrivate.h', u'Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp', u'Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp', u'Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.h', u'Source/WebCore/platform/graphics/opengl/SharedGraphicsContext3DOpenGL.cpp', u'Source/WebCore/platform/graphics/qt/GraphicsContext3DPrivate.cpp', u'Source/WebCore/platform/graphics/qt/GraphicsContext3DPrivate.h', u'Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp', u'Source/WebCore/platform/graphics/surfaces/GraphicsSurface.cpp', u'Source/WebCore/platform/graphics/surfaces/GraphicsSurface.h', u'Source/WebCore/platform/graphics/surfaces/GraphicsSurfaceToken.h', u'Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceQt.cpp', u'Source/WebCore/platform/graphics/texmap/GraphicsContext3DPlatformLayer.cpp', u'Source/WebCore/platform/graphics/texmap/GraphicsContext3DPlatformLayer.h', u'Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayer.h', u'Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.h']" exit_code: 1
Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceQt.cpp:26:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kalyan]

Created attachment 188333 [details]
patchv5

[Kenneth Rohde Christiansen]

Comment on attachment 188333 [details]
patchv5

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

> Source/WebCore/ChangeLog:13
> +        that the common code is shared by all three
> +        ports.

why newline?

> Source/WebCore/GNUmakefile.list.am:1309
>  	$(WebCore)/Modules/webaudio/OfflineAudioCompletionEvent.idl \
> -    $(WebCore)/Modules/webaudio/OscillatorNode.idl \
> +	$(WebCore)/Modules/webaudio/OscillatorNode.idl \
>  	$(WebCore)/Modules/webaudio/AnalyserNode.idl \
>  	$(WebCore)/Modules/webaudio/WaveShaperNode.idl \
> -    $(WebCore)/Modules/webaudio/WaveTable.idl \
> +	$(WebCore)/Modules/webaudio/WaveTable.idl \
>  	$(WebCore)/Modules/webdatabase/DOMWindowWebDatabase.idl \

This seems very unrelated

> Source/WebCore/platform/graphics/cairo/GraphicsContext3DCairo.cpp:5
> + * Copyright (C) 2012 Intel Corporation.

Dont we write All rights reserved?

[Kalyan]

Created attachment 188398 [details]
patchv6

[Kalyan]

(In reply to comment #22)
> (From update of attachment 188333 [details])
> >  	$(WebCore)/Modules/webaudio/OfflineAudioCompletionEvent.idl \
> > -    $(WebCore)/Modules/webaudio/OscillatorNode.idl \
> > +	$(WebCore)/Modules/webaudio/OscillatorNode.idl \
> >  	$(WebCore)/Modules/webaudio/AnalyserNode.idl \
> >  	$(WebCore)/Modules/webaudio/WaveShaperNode.idl \
> > -    $(WebCore)/Modules/webaudio/WaveTable.idl \
> > +	$(WebCore)/Modules/webaudio/WaveTable.idl \
> >  	$(WebCore)/Modules/webdatabase/DOMWindowWebDatabase.idl \
> 
> This seems very unrelated

Cleaned it up as I was making changes to the file. I will try to separate them into another patch.

> 
> Dont we write All rights reserved?

Fixed.

[Kalyan]

Comment on attachment 188398 [details]
patchv6

removing request for review and marking the patch obsolete.

I will try to break down the patch into smaller ones and create separate issues for each part.

[Michael Catanzaro]

Closing this bug because the EFL port has been removed from trunk.

If you feel this bug applies to a different upstream WebKit port and was closed in error, please either update the title and reopen the bug, or leave a comment to request this.