[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 #26 from Gabor Rapcsanyi <rgabor at inf.u-szeged.hu>  2010-08-24 09:06:25 PST ---
(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.

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