[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 #3 from Dirk Pranke <dpranke at chromium.org>  2010-07-08 13:33:08 PST ---
(From update of attachment 60859)
> Index: WebKitTools/ChangeLog
> ===================================================================
> --- WebKitTools/ChangeLog	(revision 62768)
> +++ WebKitTools/ChangeLog	(working copy)
> @@ -1,3 +1,13 @@
> +2010-07-08  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/qt.py:
> +        * Scripts/webkitpy/layout_tests/port/webkit.py:
> +
>  2010-07-07  Daniel Bates  <dbates at rim.com>
>  
>          Reviewed by Dumitru Daniliuc.
> Index: WebKitTools/Scripts/webkitpy/layout_tests/port/qt.py
> ===================================================================
> --- WebKitTools/Scripts/webkitpy/layout_tests/port/qt.py	(revision 62768)
> +++ WebKitTools/Scripts/webkitpy/layout_tests/port/qt.py	(working copy)
> @@ -93,6 +93,11 @@ class QtPort(WebKitPort):
>      def _path_to_driver(self):
>          return self._build_path('bin/DumpRenderTree')
>  
> +    def _tests_for_disabled_features(self):
> +        lib_path = self._build_path('lib/libQtWebKit.so')
> +        disabled_tests = webkit.WebKitPort._tests_for_disabled_features(self)
> +        return disabled_tests + self._check_for_unsupported_features(lib_path)
> +
>      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 62768)
> +++ 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,36 @@ class WebKitPort(base.Port):
>              "platform/win",
>          ]
>  
> +    def _check_for_unsupported_features(self, lib_path):
> +        # FIXME: Implement this for Windows.
> +        if sys.platform in ('cygwin', 'win32'):
> +            return []
> +
> +        nm_command = "nm"
> +        symbols_for_tests = {
> +            "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"],
> +        }
> +
> +        skipped_directories = []
> +        for feature in symbols_for_tests:
> +            skipped_directories += symbols_for_tests[feature]
> +
> +        out = os.popen(nm_command + " " + lib_path)
> +        for line in out.readlines():
> +            for feature in symbols_for_tests.copy():
> +                if feature in line:
> +                    for dict in symbols_for_tests[feature]:
> +                        skipped_directories.remove(dict)
> +                    del symbols_for_tests[feature]
> +
> +        return skipped_directories
> +
>      def _tests_for_disabled_features(self):
>          # FIXME: This should use the feature detection from
>          # webkitperl/features.pm to match run-webkit-tests.

WebKitTools/Scripts/webkitpy/layout_tests/port/webkit.py:221
 +      def _check_for_unsupported_features(self, lib_path):

I'd probably rename this directories_to_skip().

WebKitTools/Scripts/webkitpy/layout_tests/port/webkit.py:227
 +          symbols_for_tests = {

I'd probably rename this something like directories_by_symbol or directories_for_symbol

WebKitTools/Scripts/webkitpy/layout_tests/port/webkit.py:239
 +              skipped_directories += symbols_for_tests[feature]

And here I'd be tempted to do something like:

    skipped_directories = reduce(operator.add, directories_by_symbol.values())

although that might be an obscurely functional thing to do; the longer version is:

    skipped_directories = []
    for dirs in directories_by_symbol.values():
        skipped_directories += dirs

WebKitTools/Scripts/webkitpy/layout_tests/port/webkit.py:243
 +              for feature in symbols_for_tests.copy():

I don't think you need the copy() here.

WebKitTools/Scripts/webkitpy/layout_tests/port/webkit.py:247
 +                      del symbols_for_tests[feature]

I'd probably rename 'line' to 'library_symbol' and 'feature' to 'feature_symbol'.

I'd definitely rename 'dict' to 'dir' or 'directory', since it's a directory (a string), not a dict.

Otherwise, this looks good to me. I disagree with Eric that the function is too long; if anything, I think splitting up the logic might make it harder to follow. I could be convinced that symbols_for_tests should be returned from a separate function, if we thought different ports would have different lists, but I wouldn't refactor that until it was needed. One could argue that splitting the routine up might make it easier to test (since you don't have unit tests), but I'm not sure that unit tests would actually test anything interesting, so I wouldn't buy into the argument.

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