[Webkit-unassigned] [Bug 41842] Add feature detection support for NRWT

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=41842





--- Comment #15 from Dirk Pranke <dpranke at chromium.org>  2010-08-11 13:03:19 PST ---
(From update of attachment 63993)
> Index: WebKitTools/ChangeLog
> ===================================================================
> --- WebKitTools/ChangeLog	(revision 65059)
> +++ WebKitTools/ChangeLog	(working copy)
> @@ -1,3 +1,14 @@
> +2010-08-10  Gabor Rapcsanyi  <rgabor at inf.u-szeged.hu>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Add feature detection support to NRWT.
> +        https://bugs.webkit.org/show_bug.cgi?id=41842
> +
> +        * Scripts/webkitpy/layout_tests/port/base.py:
> +        * Scripts/webkitpy/layout_tests/port/qt.py:
> +        * Scripts/webkitpy/layout_tests/port/webkit.py:
> +
>  2010-08-09  Antonio Gomes  <tonikitoo at webkit.org>
>  
>          Reviewed by Ariya Hidayat.
> Index: WebKitTools/Scripts/webkitpy/layout_tests/port/base.py
> ===================================================================
> --- WebKitTools/Scripts/webkitpy/layout_tests/port/base.py	(revision 65057)
> +++ WebKitTools/Scripts/webkitpy/layout_tests/port/base.py	(working copy)
> @@ -659,6 +659,10 @@ class Port(object):
>          """Returns the full path to the test driver (DumpRenderTree)."""
>          raise NotImplementedError('Port.path_to_driver')
>  
> +    def _path_to_library(self):
> +        """Returns the full path to library of WebKit."""
> +        raise NotImplementedError('Port.path_to_library')
> +
>      def _path_to_helper(self):
>          """Returns the full path to the layout_test_helper binary, which
>          is used to help configure the system for the test run, or None
> Index: WebKitTools/Scripts/webkitpy/layout_tests/port/qt.py
> ===================================================================
> --- WebKitTools/Scripts/webkitpy/layout_tests/port/qt.py	(revision 65057)
> +++ WebKitTools/Scripts/webkitpy/layout_tests/port/qt.py	(working copy)
> @@ -93,6 +93,9 @@ class QtPort(WebKitPort):
>      def _path_to_driver(self):
>          return self._build_path('bin/DumpRenderTree')
>  
> +    def _path_to_library(self):
> +        return self._build_path('lib/libQtWebKit.so')
> +
>      def setup_environ_for_server(self):
>          env = webkit.WebKitPort.setup_environ_for_server(self)
>          env['QTWEBKIT_PLUGIN_PATH'] = self._build_path('lib/plugins')
> Index: WebKitTools/Scripts/webkitpy/layout_tests/port/webkit.py
> ===================================================================
> --- WebKitTools/Scripts/webkitpy/layout_tests/port/webkit.py	(revision 65057)
> +++ WebKitTools/Scripts/webkitpy/layout_tests/port/webkit.py	(working copy)
> @@ -1,5 +1,6 @@
>  #!/usr/bin/env python
>  # Copyright (C) 2010 Google Inc. All rights reserved.
> +# Copyright (C) 2010 Gabor Rapcsanyi <rgabor at inf.u-szeged.hu>, University of Szeged
>  #
>  # Redistribution and use in source and binary forms, with or without
>  # modification, are permitted provided that the following conditions are
> @@ -217,6 +218,48 @@ class WebKitPort(base.Port):
>              "platform/win",
>          ]
>  
> +    def get_supported_features_symbol(self):

I'd probably call this "get_supported_features_symbol_list" or "_symbols", since you're returning potentially multiple things. Also, this should have a docstring if you intend for it to be overridable; if you don't, it should probably have a leading underscore in the name. 

Given that in the non-win32 case you actually return *all* symbols and not just ones used to indicate supported features, some sort of docstring or comment about that being okay might be a good idea as well.

> +        library_path = self._path_to_library()
> +        driver_path = self._path_to_driver()
> +
> +        if sys.platform in ('cygwin', 'win32') and driver_path:
> +            symbol_list = os.popen(driver_path + " --print-supported-features 2>&1").readlines()
> +        elif library_path:
> +            symbol_list = os.popen("nm " + library_path).readlines()
> +        else:
> +            symbol_list = []
> +
> +        return symbol_list
> +
> +    def get_directories_for_symbol(self):
> +        if sys.platform in ('cygwin', 'win32'):
> +            directories_for_symbol = {
> +                "Accelerated Compositing": ["compositing"],
> +                "3D Rendering": ["animations/3d", "transforms/3d"],
> +            }
> +        else:
> +            directories_for_symbol = {
> +                "MathMLElement": ["mathml"],
> +                "GraphicsLayer": ["compositing"],
> +                "WebCoreHas3DRendering": ["animations/3d", "transforms/3d"],
> +                "WebGLShader": ["fast/canvas/webgl"],
> +                "WMLElement": ["http/tests/wml", "fast/wml", "wml"],
> +                "parseWCSSInputProperty": ["fast/wcss"],
> +                "isXHTMLMPDocument": ["fast/xhtmlmp"],
> +            }
> +        return directories_for_symbol
> +
> +    def skipped_directories_for_unsupported_features(self):

Still needs a docstring.

> +        symbol_list = self.get_supported_features_symbol()

This is nitpicking, but you could add:

    if not symbol_list:
        return []

here just to reduce the indentation levels. On the other hand, when would this ever be null? It looks like the rest of the code works fine even if this list is null, so this check could be an unnecessary (and slightly confusing) optimization.


> +        directories_for_symbol = self.get_directories_for_symbol()
> +        skipped_directories = []
> +        if symbol_list:
> +            for feature_symbol in directories_for_symbol:
> +                symbol = [library_symbol for library_symbol in symbol_list if feature_symbol in library_symbol]

Maybe call this found_symbols to be a little clearer?

Otherwise this looks fine.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.