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

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied  review:
Bug 41842: Add feature detection support for NRWT
https://bugs.webkit.org/show_bug.cgi?id=41842

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

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