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

[bugzilla-daemon at webkit.org]

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


Eric Seidel <eric at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #65116|review?                     |review-
               Flag|                            |




--- Comment #24 from Eric Seidel <eric at webkit.org>  2010-08-23 10:26:55 PST ---
(From update of attachment 65116)
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!

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