Bug 134589 – [GTK] Use the same default options for production builds that previous stable releases

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Carlos Garcia Campos]

We should use the same options for now, later we can discuss whether to change anything or not.

[Carlos Garcia Campos]

Created attachment 234331 [details]
Patch

[Carlos Garcia Campos]

Created attachment 234339 [details]
This should apply now

[Martin Robinson]

Comment on attachment 234339 [details]
This should apply now

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

Looks good, but we need to make accelerated canvas support conditional on CairoGL.

> Source/cmake/OptionsGTK.cmake:68
> +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_ACCELERATED_2D_CANVAS ON)

This should be determined by whether or not you have CairoGL support. The default should probably be off.

> Tools/Scripts/webkitperl/FeatureList.pm:158
> -      define => "ENABLE_ACCELERATED_2D_CANVAS", default => 0, value => \$accelerated2DCanvasSupport },
> +      define => "ENABLE_ACCELERATED_2D_CANVAS", default => isGtk(), value => \$accelerated2DCanvasSupport },

I think this is going to fail unless Cairo with GL support is installed. This isn't the typical installation on most systems.

[Carlos Garcia Campos]

(In reply to comment #3)
> (From update of attachment 234339 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=234339&action=review
> 
> Looks good, but we need to make accelerated canvas support conditional on CairoGL.

You are right.

> > Source/cmake/OptionsGTK.cmake:68
> > +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_ACCELERATED_2D_CANVAS ON)
> 
> This should be determined by whether or not you have CairoGL support. The default should probably be off.

In stable it's on by default when cairo-gl is found in production. We should make sure that options enabled in production are also enabled in developer builds. We can change the default value if it was a mistake in stable, though.

> > Tools/Scripts/webkitperl/FeatureList.pm:158
> > -      define => "ENABLE_ACCELERATED_2D_CANVAS", default => 0, value => \$accelerated2DCanvasSupport },
> > +      define => "ENABLE_ACCELERATED_2D_CANVAS", default => isGtk(), value => \$accelerated2DCanvasSupport },
> 
> I think this is going to fail unless Cairo with GL support is installed. This isn't the typical installation on most systems.

Even if this is on by default here, it will be disabled later by cmake if the requirements are not found (once I add the checks for cairo-gl, I mean)

[Carlos Garcia Campos]

Created attachment 234698 [details]
Updated patch

Make accelerated 2D canvas depend on cairo-gl

[WebKit Commit Bot]

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


ERROR: Source/cmake/FindCairoGL.cmake:38:  Use lowercase command "string"  [command/lowercase] [5]
Total errors found: 1 in 5 files


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

[Carlos Garcia Campos]

Created attachment 234699 [details]
Fix cmake 'coding' style

[Build Bot]

Comment on attachment 234699 [details]
Fix cmake 'coding' style

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

New failing tests:
media/track/add-and-remove-track.html

[Build Bot]

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

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

[Carlos Garcia Campos]

Committed r170964: <http://trac.webkit.org/changeset/170964>

[Alberto Garcia]

(In reply to comment #4)
> > > +      define => "ENABLE_ACCELERATED_2D_CANVAS", default => isGtk(), value => \$accelerated2DCanvasSupport },
> >
> > I think this is going to fail unless Cairo with GL support is installed. This isn't the typical installation on most systems.
>
> Even if this is on by default here, it will be disabled later by cmake if the requirements are not found (once I add the checks for cairo-gl, I mean)

That doesn't seem to be the case, see bug 136576.

[Csaba Osztrogonác]

Comment on attachment 234699 [details]
Fix cmake 'coding' style

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

> Source/cmake/OptionsGTK.cmake:106
> +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_FTPDIR OFF)

Just out of curiosity? Is there a concrete reason for disabling FTPDIR?
It is enabled everywhere long long time ago. 

I started thinking if we should remove this guard, 
but this change made me unsure.

[Carlos Garcia Campos]

(In reply to comment #12)
> Comment on attachment 234699 [details]
> Fix cmake 'coding' style
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=234699&action=review
> 
> > Source/cmake/OptionsGTK.cmake:106
> > +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_FTPDIR OFF)
> 
> Just out of curiosity? Is there a concrete reason for disabling FTPDIR?

I have no idea.