[webkit-reviews] review denied: [Bug 41842] Add feature detection support for NRWT : [Attachment 65116] proposed_patch_v5

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Gabor Rapcsanyi
<rgabor at inf.u-szeged.hu>'s request for review:
Bug 41842: Add feature detection support for NRWT
https://bugs.webkit.org/show_bug.cgi?id=41842

Attachment 65116: proposed_patch_v5
https://bugs.webkit.org/attachment.cgi?id=65116&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
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!