Add feature detection support for NRWT

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Gabor Rapcsanyi]

Now the skipped tests for disabled features are hard-coded in WebKitPort.
Add a method for checking supported features (similar to ORWT) and use this functionality on the Qt port.

[Gabor Rapcsanyi]

Created attachment 60859 [details]
proposed patch

[Eric Seidel (no email)]

Comment on attachment 60859 [details]
proposed patch

Please split this into smaller functions.  That's too much python in one function for my little brain.

Shouldn't symbols_for_tests be a class-level define anyway?

[Dirk Pranke]

Comment on attachment 60859 [details]
proposed patch

> Index: WebKitTools/ChangeLog
> ===================================================================
> --- WebKitTools/ChangeLog	(revision 62768)
> +++ WebKitTools/ChangeLog	(working copy)
> @@ -1,3 +1,13 @@
> [phone]  Gabor Rapcsanyi  <[e-mail]>
> +
> +        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  <[e-mail]>
>  
>          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 <[e-mail]>, 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.

[Gabor Rapcsanyi]

Created attachment 61033 [details]
proposed_patch_v2

I have renamed the variables that Dirk suggested.

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

This .copy() need because I remove the elements from this dictionary.

As I saw in ORWT all port use these symbols.
Just cygwin use another way to find supported features.

[Gabor Rapcsanyi]

(In reply to comment #2)
> (From update of attachment 60859 [details])
> Please split this into smaller functions.  That's too much python in one function for my little brain.
> 
> Shouldn't symbols_for_tests be a class-level define anyway?

If you want I can put symbols_for_tests to a separate function as Dirk said.

[Gabor Rapcsanyi]

So, do you have any other suggestion for this patch?

[Dirk Pranke]

Comment on attachment 61033 [details]
proposed_patch_v2

Maybe I'm missing something, but I still don't think you need the copy on line 241 of webkit.py. There's no case where you would want to delete the directories from skipped_directories and then add them back in, and so there's no point in looking at the keys in directories_for_symbol over and over again if you've deleted them. There's no correctness issue, but it's unnecessary (and slower).

Otherwise, LGTM (but I'm not a reviewer).

[Gabor Rapcsanyi]

(In reply to comment #7)
> (From update of attachment 61033 [details])
> Maybe I'm missing something, but I still don't think you need the copy on line 241 of webkit.py. There's no case where you would want to delete the directories from skipped_directories and then add them back in, and so there's no point in looking at the keys in directories_for_symbol over and over again if you've deleted them. There's no correctness issue, but it's unnecessary (and slower).
> 
> Otherwise, LGTM (but I'm not a reviewer).

Without of this .copy() the python throws this:
File "WebKitTools/Scripts/webkitpy/layout_tests/port/webkit.py", line 241, in directories_to_skip
    for feature_symbol in directories_for_symbol:
RuntimeError: dictionary changed size during iteration

[Eric Seidel (no email)]

Comment on attachment 61033 [details]
proposed_patch_v2

OK.  Seems we should implement this for the other webkit platforms as well.

directories_to_skip is a bad name, since it's about skipping directories for disabled features.

[Eric Seidel (no email)]

Comment on attachment 61033 [details]
proposed_patch_v2

With this implemented, we can also now remove some hacks in:
http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/layout_tests/port/webkit.py#L220

Yeah, this is not the right layer.

Some platform-specific function should do the symbol search and return an array of symbols. Then webkit.py should do the lookup in the list of conversions.

The ports don't really have to do anything.  Except right now port and platform abstractions are mashed totether, so the port will ahve to act as the platform here, and do the search.

I would just implement this all in webkit.py, except for the library path lookup of course.

This is close, but not quite right.

Also, this needs unit tests.

[Dirk Pranke]

(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 61033 [details] [details])
> > Maybe I'm missing something, but I still don't think you need the copy on line 241 of webkit.py. There's no case where you would want to delete the directories from skipped_directories and then add them back in, and so there's no point in looking at the keys in directories_for_symbol over and over again if you've deleted them. There's no correctness issue, but it's unnecessary (and slower).
> > 
> > Otherwise, LGTM (but I'm not a reviewer).
> 
> Without of this .copy() the python throws this:
> File "WebKitTools/Scripts/webkitpy/layout_tests/port/webkit.py", line 241, in directories_to_skip
>     for feature_symbol in directories_for_symbol:
> RuntimeError: dictionary changed size during iteration

Ah, right, you're mutating the list while iterating over it. My mistake :)

[Dirk Pranke]

(In reply to comment #10)
> (From update of attachment 61033 [details])
> With this implemented, we can also now remove some hacks in:
> http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/layout_tests/port/webkit.py#L220
> 
> Yeah, this is not the right layer.
> 
> Some platform-specific function should do the symbol search and return an array of symbols. Then webkit.py should do the lookup in the list of conversions.
> 
> The ports don't really have to do anything.  Except right now port and platform abstractions are mashed totether, so the port will ahve to act as the platform here, and do the search.
> 
> I would just implement this all in webkit.py, except for the library path lookup of course.
> 
> This is close, but not quite right.
>

Eric, I'm not sure I agree with (or at least understand) you here. This list of directories to skip can vary by both platform and port (webkit mac vs. webkit win, webkit win vs. chromium win), and the mechanism for figuring out the presence of a given feature can vary by either mechanism as well. So I'm not sure what you mean by "the ports don't really have to do anything"?
 
> Also, this needs unit tests.

This is probably true :)

[Dirk Pranke]

(In reply to comment #9)
> (From update of attachment 61033 [details])
> OK.  Seems we should implement this for the other webkit platforms as well.
> 
> directories_to_skip is a bad name, since it's about skipping directories for disabled features.

I think you can look at this a couple of different ways. One way to look at this is that all it is is a list of directories to skip, but the caller doesn't know why the port wants to skip them.

Another way is to make this a "features to disable" (or skip) call -- not directories -- and make the features->directory mapping generic (or at least a different, overridable call), and that would suggest a different name.

A third way is to think of this is as "directories to skip because the features are disabled". Of course, that would be a really long method name, even by Eric's standards. It's not obvious to me what a better  name than "directories_to_skip" would be.

I'm actually fine with any of the three approaches. The second is the most well-factored, but it might be overkill. 

Whatever you do pick, there should be a docstring describing the contract, though.

[Gabor Rapcsanyi]

Created attachment 63993 [details]
proposed_patch_v3

There is no platform layer in NRWT. The WebKitPort (webkit.py) inherits from the BasePort (base.py). And the ports like QtPort (qt.py) inherits from WebKitPort. So we have to do the symbol search and etc. in webkit.py as you wrote.
I have changed the patch, now its checks Windows feature support. If other ports want to use on Linux, they just have to define the _path_to_library method like in Qt. On Win platform they have to define _path_to_driver and the Mac platform doesn't use feature detection.
If its ok for you I will make a unit test for it.

[Dirk Pranke]

Comment on attachment 63993 [details]
proposed_patch_v3

> Index: WebKitTools/ChangeLog
> ===================================================================
> --- WebKitTools/ChangeLog	(revision 65059)
> +++ WebKitTools/ChangeLog	(working copy)
> @@ -1,3 +1,14 @@
> [phone]  Gabor Rapcsanyi  <[e-mail]>
> +
> +        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  <[e-mail]>
>  
>          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 <[e-mail]>, 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.

[Gabor Rapcsanyi]

Created attachment 64576 [details]
proposed_patch_v4

I have made the changes what Dirk suggest, and add a unittest for it.

[Gabor Rapcsanyi]

(In reply to comment #15)
> (From update of attachment 63993 [details])
> 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]

This return [] is necessary because it puts all directory to skipped_directories if it doesn't find the symbols. So if the ports doesn't declare _path_to_library() method, all of the directories will be skipped without of this.

[Dirk Pranke]

(In reply to comment #17)
> This return [] is necessary because it puts all directory to skipped_directories if it doesn't find the symbols. So if the ports doesn't declare _path_to_library() method, all of the directories will be skipped without of this.

Ah, right. Thanks!

[David Levin]

Comment on attachment 64576 [details]
proposed_patch_v4

Ok, I see nothing that looks wrong and I trust Dirk's opinion that this is solid.

[Eric Seidel (no email)]

Comment on attachment 64576 [details]
proposed_patch_v4

WebKitTools/Scripts/webkitpy/layout_tests/port/base.py:662
 +      def _path_to_library(self):
This should really be _path_to_webcore, since these symbols are searched for in WebCore, not WebKit on ports which don't build all-as one.

WebKitTools/Scripts/webkitpy/layout_tests/port/webkit.py:228
 +              symbol_list = os.popen(driver_path + " --print-supported-features 2>&1").readlines()
We actually want to move to this model for all ports.

WebKitTools/Scripts/webkitpy/layout_tests/port/webkit.py:221
 +      def _get_supported_features_symbols(self):
This name doesnt' make a lot of sense since it's not symbols on win32.  Seems like we should have a separate path for ports which use symbols vs. supported features, no?

WebKitTools/Scripts/webkitpy/layout_tests/port/webkit.py:239
 +          if sys.platform in ('cygwin', 'win32'):
Yeah, these are tottally separate code paths.  Lets not conflate them.

One unit test is not sufficient for this complexity of code.

This looks fine in general.  But please separate out the runtime list lookup vs. the symbol lookup. Basically we should try running "print-supported-features" and if that erros out, fall back to nm on all platforms (would be my suggestion).

Thank you so much for looking at this!

[Gabor Rapcsanyi]

(In reply to comment #20)
> (From update of attachment 64576 [details])
> WebKitTools/Scripts/webkitpy/layout_tests/port/base.py:662
>  +      def _path_to_library(self):
> This should really be _path_to_webcore, since these symbols are searched for in WebCore, not WebKit on ports which don't build all-as one.
> 
> WebKitTools/Scripts/webkitpy/layout_tests/port/webkit.py:228
>  +              symbol_list = os.popen(driver_path + " --print-supported-features 2>&1").readlines()
> We actually want to move to this model for all ports.
> 
> WebKitTools/Scripts/webkitpy/layout_tests/port/webkit.py:221
>  +      def _get_supported_features_symbols(self):
> This name doesnt' make a lot of sense since it's not symbols on win32.  Seems like we should have a separate path for ports which use symbols vs. supported features, no?
> 
> WebKitTools/Scripts/webkitpy/layout_tests/port/webkit.py:239
>  +          if sys.platform in ('cygwin', 'win32'):
> Yeah, these are tottally separate code paths.  Lets not conflate them.
> 
> One unit test is not sufficient for this complexity of code.
> 
> This looks fine in general.  But please separate out the runtime list lookup vs. the symbol lookup. Basically we should try running "print-supported-features" and if that erros out, fall back to nm on all platforms (would be my suggestion).
> 
> Thank you so much for looking at this!

Hi Eric!

This fall-back idea sounds good but on Qt and Gtk ports the DRT ignores this switch, gets going and blocks NRWT if we execute it, like if there only were a - switch.
If we want to do this I see two ways. Change all port's DRTs which are blocking with this switch, or make a more complex explicit DRT execution routine in a separate function.
What do you think about this?

[Eric Seidel (no email)]

(In reply to comment #21)
> This fall-back idea sounds good but on Qt and Gtk ports the DRT ignores this switch, gets going and blocks NRWT if we execute it, like if there only were a - switch.

Interesting.  I would have expected DRT to fail when passed switches it didn't recognize.  I figured most port would use get_opt which does all that standard behavior.

> If we want to do this I see two ways. Change all port's DRTs which are blocking with this switch, or make a more complex explicit DRT execution routine in a separate function.

In that case, I would just have a _runtime_feature_list() function which runs DRT --print-supported-features by default, catches any errors and returns None.  Then on ports which don't support it, but don't know how to error, we can override to return None manually.  How does that sound?

> What do you think about this?

I think making fancy fallback/decision logic is less important than simply separating the code paths.  The nm codepath will eventually be removed once all ports know how to do the --print-supported-features.

--print-supported-features (runtime feature detection) is important for us to move to, because it allows us to make a per-machine decision instead of a per-port decision.  For example, if the machine is not in PST, we can skip date-sensitive tests.  If the machine has a broken copy of some library, we can skip tests which depend on said library.  Both of these types of skips are currently hacked into the project in other ways.

Again, thank you very much for all your time on this.  Making NRWT better is *very* important as ORWT is way past needing replacement.

[Gabor Rapcsanyi]

Created attachment 65116 [details]
proposed_patch_v5

I have applied the changes that Eric suggested.

[Eric Seidel (no email)]

Comment on attachment 65116 [details]
proposed_patch_v5

WebKitTools/Scripts/webkitpy/layout_tests/port/base.py:662
 +      def _path_to_webcore(self):
This should probably be _path_to_webcore_library or _path_to_symboled_webcore_library?
Maybe it makes sense given how _path_to_driver is named?

WebKitTools/Scripts/webkitpy/layout_tests/port/base.py:663
 +          """Returns the full path to WebCore."""
to a built copy of WebCore?

WebKitTools/Scripts/webkitpy/layout_tests/port/webkit.py:226
 +          if "SupportedFeatures:" in feature_list:
I'm not sure I fully understand what this is going to return, but I guess the unit tests will tell me...

WebKitTools/Scripts/webkitpy/layout_tests/port/webkit.py:233
 +          symbol_list = os.popen("nm " + webcore_path).readlines()
I think you mean "nm", webcore_path instead of +  That will correctly handle spaces in webcore_path

WebKitTools/Scripts/webkitpy/layout_tests/port/webkit.py:236
 +      def _get_directories_for_features(self):
We don't normally use "get" in function names in WebKit.  I don't think PEP8 does either.

WebKitTools/Scripts/webkitpy/layout_tests/port/webkit.py:271
 +          skipped_directories = []
nit: I would have moved this next to the later loop, since it isn't used before the early return.

WebKitTools/Scripts/webkitpy/layout_tests/port/webkit.py:276
 +              found_feature = [feature_line for feature_line in feature_list if feature in feature_line]
No need to have found_feature as a local.  Just:
if not feature in feature_list:
would do the same, no?


WebKitTools/Scripts/webkitpy/layout_tests/port/webkit.py:279
 +          return skipped_directories
In fact, if you wanted to use fancy list comprehensions, you could just do this:

return sum([directories[feature] for feature in directories.keys() if feature not in feature_list])

Perhaps with better names that would be less confusing.  I'm not sure.

WebKitTools/Scripts/webkitpy/layout_tests/port/webkit.py:259
 +      def _skipped_directories_for_unsupported_features(self):
I don't think this really needs to be limited to directories.  It could just be skipped_tests_for_unsupported_features or skipped_tests_for_disabled_features

WebKitTools/Scripts/webkitpy/layout_tests/port/webkit_unittest.py:52
 +          all_directory = reduce(operator.add, directories_for_symbols.values())
This is the same as sum(), but reduce(operatore.add might be more clear.  either way is fine.

I had difficulty understanding your unit tests.  I probably need to just look again, but maybe sharing more code between them would make them easier to parse.

Either way, I think this needs one more round.

Thank you again for all your hard work on this!

[Gabor Rapcsanyi]

Created attachment 65280 [details]
proposed_patch_v6

[Gabor Rapcsanyi]

(In reply to comment #24)
> (From update of attachment 65116 [details])
> WebKitTools/Scripts/webkitpy/layout_tests/port/base.py:662
>  +      def _path_to_webcore(self):
> This should probably be _path_to_webcore_library or _path_to_symboled_webcore_library?
> Maybe it makes sense given how _path_to_driver is named?
> 
> WebKitTools/Scripts/webkitpy/layout_tests/port/base.py:663
>  +          """Returns the full path to WebCore."""
> to a built copy of WebCore?

corrected


> 
> WebKitTools/Scripts/webkitpy/layout_tests/port/webkit.py:226
>  +          if "SupportedFeatures:" in feature_list:
> I'm not sure I fully understand what this is going to return, but I guess the unit tests will tell me...
> 

On Win port the DRT prints that first and after print the supported features. Other port's DRTs should do the same when they implement this feature.
If we don't do this check and it returns with an error message all directories will be skipped from directories_for_features.


> WebKitTools/Scripts/webkitpy/layout_tests/port/webkit.py:233
>  +          symbol_list = os.popen("nm " + webcore_path).readlines()
> I think you mean "nm", webcore_path instead of +  That will correctly handle spaces in webcore_path
> 

It throws invalid argument error with that comma.


> WebKitTools/Scripts/webkitpy/layout_tests/port/webkit.py:236
>  +      def _get_directories_for_features(self):
> We don't normally use "get" in function names in WebKit.  I don't think PEP8 does either.
> 
> WebKitTools/Scripts/webkitpy/layout_tests/port/webkit.py:271
>  +          skipped_directories = []
> nit: I would have moved this next to the later loop, since it isn't used before the early return.
> 

corrected


> WebKitTools/Scripts/webkitpy/layout_tests/port/webkit.py:276
>  +              found_feature = [feature_line for feature_line in feature_list if feature in feature_line]
> No need to have found_feature as a local.  Just:
> if not feature in feature_list:
> would do the same, no?
> 
> 
> WebKitTools/Scripts/webkitpy/layout_tests/port/webkit.py:279
>  +          return skipped_directories
> In fact, if you wanted to use fancy list comprehensions, you could just do this:
> 
> return sum([directories[feature] for feature in directories.keys() if feature not in feature_list])
> 
> Perhaps with better names that would be less confusing.  I'm not sure.
> 

corrected


> WebKitTools/Scripts/webkitpy/layout_tests/port/webkit.py:259
>  +      def _skipped_directories_for_unsupported_features(self):
> I don't think this really needs to be limited to directories.  It could just be skipped_tests_for_unsupported_features or skipped_tests_for_disabled_features
> 

renamed


> WebKitTools/Scripts/webkitpy/layout_tests/port/webkit_unittest.py:52
>  +          all_directory = reduce(operator.add, directories_for_symbols.values())
> This is the same as sum(), but reduce(operatore.add might be more clear.  either way is fine.
> 
> I had difficulty understanding your unit tests.  I probably need to just look again, but maybe sharing more code between them would make them easier to parse.
> 

I have changed it. Now its more readable and better for unit testing.

[Eric Seidel (no email)]

Comment on attachment 65280 [details]
proposed_patch_v6

So exciting!  Thank you.

[WebKit Commit Bot]

Comment on attachment 65280 [details]
proposed_patch_v6

Rejecting patch 65280 from commit-queue.

Failed to run "['WebKitTools/Scripts/test-webkitpy']" exit_code: 1
Last 500 characters of output:
.py", line 269, in _skipped_tests_for_unsupported_features
    feature_list = self._supported_symbol_list()
  File "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/layout_tests/port/webkit.py", line 234, in _supported_symbol_list
    symbol_list = ' '.join(os.popen("nm " + webcore_library_path).readlines())
TypeError: cannot concatenate 'str' and 'NoneType' objects

----------------------------------------------------------------------
Ran 545 tests in 9.058s

FAILED (errors=1)

Full output: http://queues.webkit.org/results/3903061

[Andras Becsi]

Committed r66727: <http://trac.webkit.org/changeset/66727>

[Gabor Rapcsanyi]

Comment on attachment 65280 [details]
proposed_patch_v6

clearing flags
landed patch after fix