Bug 135560 – [EFL] Remove non Coordinated Graphics code path from cmake build system after r142169

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Éva Balázsfalvi]

Remove non Coordinated Graphics code path from cmake build system after r142169

[Éva Balázsfalvi]

Created attachment 235971 [details]
Patch

[Éva Balázsfalvi]

After http://trac.webkit.org/changeset/142169, Coordinated Graphics, Tiled Backing Store, Graphics Surface, ...  are mandatory for EFL WK2, so we can remove these guards from cmake files too.

[Gyuyoung Kim]

Comment on attachment 235971 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        * PlatformEfl.cmake:

It would be nice if you write what is changed in each file.

> Tools/Scripts/webkitperl/FeatureList.pm:-398
> -      define => "WTF_USE_TILED_BACKING_STORE", default => isEfl(), value => \$tiledBackingStoreSupport },

USE(TILED_BACKING_STORE) is still being used in source code. So, I'm not sure whether we can remove this here.

> ChangeLog:8
> +        * Source/cmake/OptionsEfl.cmake:

We need to mention what you do this file.

[Éva Balázsfalvi]

(In reply to comment #3)
> (From update of attachment 235971 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=235971&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        * PlatformEfl.cmake:
> 
> > ChangeLog:8
> > +        * Source/cmake/OptionsEfl.cmake:
> 
> We need to mention what you do this file.

I'll add descriptions to the modified files, thanks for pointing it out.

> It would be nice if you write what is changed in each file.
> 
> > Tools/Scripts/webkitperl/FeatureList.pm:-398
> > -      define => "WTF_USE_TILED_BACKING_STORE", default => isEfl(), value => \$tiledBackingStoreSupport },
> 
> USE(TILED_BACKING_STORE) is still being used in source code. So, I'm not sure whether we can remove this here.

If we leave it in the code, the build will fail in case of minimal build. None of the other ports use it, so it's safe to remove it.

[Gyuyoung Kim]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 235971 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=235971&action=review
> > 
> > > Source/WebCore/ChangeLog:10
> > > +        * PlatformEfl.cmake:
> > 
> > > ChangeLog:8
> > > +        * Source/cmake/OptionsEfl.cmake:
> > 
> > We need to mention what you do this file.
> 
> I'll add descriptions to the modified files, thanks for pointing it out.
> 
> > It would be nice if you write what is changed in each file.
> > 
> > > Tools/Scripts/webkitperl/FeatureList.pm:-398
> > > -      define => "WTF_USE_TILED_BACKING_STORE", default => isEfl(), value => \$tiledBackingStoreSupport },
> > 
> > USE(TILED_BACKING_STORE) is still being used in source code. So, I'm not sure whether we can remove this here.
> 
> If we leave it in the code, the build will fail in case of minimal build. None of the other ports use it, so it's safe to remove it.

If so, you can just change isEfl() with 0. Are you sure whether other ports never enable it again ?

> > > -      define => "WTF_USE_TILED_BACKING_STORE", default => 0, value => \$tiledBackingStoreSupport },

[Csaba Osztrogonác]

(In reply to comment #5)
> > > USE(TILED_BACKING_STORE) is still being used in source code. So, I'm not sure whether we can remove this here.
> > 
> > If we leave it in the code, the build will fail in case of minimal build. None of the other ports use it, so it's safe to remove it.
> 
> If so, you can just change isEfl() with 0. Are you sure whether other ports never enable it again ?

Let's see all options one by one:
- default => isEfl() : It means it is enabled on EFL by default, but 
  --minimal option would disable this mandatory feature. It isn't good.
- default => 0 : It means it would be disabled always on EFL - on minimal
  and on normal build too. It isn't good.
- Deleting it from FeatureList.pm would cause problem if we would like
  to make TILED_BACKING_STORE optional. But it isn't optional at all.
  It is mandatory on EFL and not used / not work on other ports.

[Ryuan Choi]

(In reply to comment #6)
> (In reply to comment #5)
> > > > USE(TILED_BACKING_STORE) is still being used in source code. So, I'm not sure whether we can remove this here.
> > > 
> > > If we leave it in the code, the build will fail in case of minimal build. None of the other ports use it, so it's safe to remove it.
> > 
> > If so, you can just change isEfl() with 0. Are you sure whether other ports never enable it again ?
> 
> Let's see all options one by one:
> - default => isEfl() : It means it is enabled on EFL by default, but 
>   --minimal option would disable this mandatory feature. It isn't good.
> - default => 0 : It means it would be disabled always on EFL - on minimal
>   and on normal build too. It isn't good.
> - Deleting it from FeatureList.pm would cause problem if we would like
>   to make TILED_BACKING_STORE optional. But it isn't optional at all.
>   It is mandatory on EFL and not used / not work on other ports.

Hi,
If I am right, TILED_BACKING_STORE can't be an optional nowadays because it's meaningless without Coordinated Graphics.
And if other port want to use it, it can be used like other port specific features, WTF_USE_SOUP and WTF_USE_CROSS_PLATFORM_CONTEXT_MENUS.

So, removing it from FeatureList.pm is fine to me.

FYI,
If other port want to use TILED_BACKING_STORE, there are only two options.
(1) use Coordinated Graphics.
(2) implement missing parts such as TiledBackingStoreBackend, implementation class of Tile and painting path for the pure TiledBackingStore.

I roughly listed missing parts about option (2), but it's to use TiledBackingStore on the WebKit1.
If someone want to use it on the WebKit2, it require much more modules.

[Ryuan Choi]

Comment on attachment 235971 [details]
Patch

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

> Source/cmake/OptionsEfl.cmake:207
> +set(WTF_USE_TEXTURE_MAPPER 1)

With your patch, it looks not required anymore.

> Source/cmake/OptionsEfl.cmake:216
> +set(USE_GRAPHICS_SURFACE 1)

It's already not required.

[Gyuyoung Kim]

(In reply to comment #6)
> (In reply to comment #5)
> > > > USE(TILED_BACKING_STORE) is still being used in source code. So, I'm not sure whether we can remove this here.
> > > 
> > > If we leave it in the code, the build will fail in case of minimal build. None of the other ports use it, so it's safe to remove it.
> > 
> > If so, you can just change isEfl() with 0. Are you sure whether other ports never enable it again ?
> 
> Let's see all options one by one:
> - default => isEfl() : It means it is enabled on EFL by default, but 
>   --minimal option would disable this mandatory feature. It isn't good.
> - default => 0 : It means it would be disabled always on EFL - on minimal
>   and on normal build too. It isn't good.
> - Deleting it from FeatureList.pm would cause problem if we would like
>   to make TILED_BACKING_STORE optional. But it isn't optional at all.
>   It is mandatory on EFL and not used / not work on other ports.

Ossy, I see. I wondered whether other ports will use it future. However, it looks GTK port won't use it. Looks good if you fix Ryuan's comments as well.

[Éva Balázsfalvi]

Created attachment 237308 [details]
Patch

[Ryuan Choi]

Comment on attachment 237308 [details]
Patch

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

> Source/WebCore/PlatformEfl.cmake:387
> +)

nit, wrong indentation

> Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.h:28
> -#endif
>  
>  #include "GLPlatformContext.h"

How about removing unnecessary empty line

> Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.h:-36
>  class GraphicsContext3DPrivate
> -#if USE(TEXTURE_MAPPER_GL)
>          : public TextureMapperPlatformLayer
> -#endif

How about making it one line?

> Source/WebCore/platform/graphics/surfaces/efl/GraphicsSurfaceCommon.cpp:29
>  

How about removing unnecessary line?

[Éva Balázsfalvi]

(In reply to comment #11)
> (From update of attachment 237308 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=237308&action=review
> 
> > Source/WebCore/PlatformEfl.cmake:387
> > +)
> 
> nit, wrong indentation
> 
> > Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.h:28
> > -#endif
> >  
> >  #include "GLPlatformContext.h"
> 
> How about removing unnecessary empty line
> 
> > Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.h:-36
> >  class GraphicsContext3DPrivate
> > -#if USE(TEXTURE_MAPPER_GL)
> >          : public TextureMapperPlatformLayer
> > -#endif
> 
> How about making it one line?
> 
> > Source/WebCore/platform/graphics/surfaces/efl/GraphicsSurfaceCommon.cpp:29
> >  
> 
> How about removing unnecessary line?

Thanks for noticing. Corrected them already.

[Éva Balázsfalvi]

Created attachment 237311 [details]
Patch

[Build Bot]

Comment on attachment 237311 [details]
Patch

Attachment 237311 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5660119270424576

New failing tests:
compositing/checkerboard.html
animations/3d/transform-origin-vs-functions.html
http/tests/appcache/abort-cache-onchecking.html
animations/animation-controller-drt-api.html
canvas/philip/tests/2d.clearRect+fillRect.basic.html
accessibility/alt-tag-on-image-with-nonimage-role.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.html

[Build Bot]

Created attachment 237323 [details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-13  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Ryuan Choi]

(In reply to comment #15)
> Created an attachment (id=237323) [details]
> Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2
> 
> The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
> Bot: webkit-ews-13  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

I don't think that this bug touches mac related code so this is surely false warning.

[WebKit Commit Bot]

Comment on attachment 237311 [details]
Patch

Clearing flags on attachment: 237311

Committed r173112: <http://trac.webkit.org/changeset/173112>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.