new-run-webkit-tests: fix feature detection, skipped platform lists on mac
Created attachment 87922 [details] Patch
Comment on attachment 87922 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=87922&action=review > Tools/Scripts/webkitpy/layout_tests/port/mac.py:134 > + def _path_to_webcore_library(self): > + return self._build_path('WebCore.framework/Versions/A/WebCore') This method does not appear to be used in this patch. > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:186 > + entries = self._filesystem.glob(self._webkit_baseline_path('*')) Doesn't this return a super huge list? This doesn't seem like the right algorithm. You want to enumerate the directories and then ask whether they're in the baseline search path, not enumerate all the things in those directories. > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:187 > + platform_prefix = self.name().split('-', 1)[0] This should be a helpful function instead of munging the string here.
(In reply to comment #2) > (From update of attachment 87922 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=87922&action=review > > > Tools/Scripts/webkitpy/layout_tests/port/mac.py:134 > > + def _path_to_webcore_library(self): > > + return self._build_path('WebCore.framework/Versions/A/WebCore') > > This method does not appear to be used in this patch. > > > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:186 > > + entries = self._filesystem.glob(self._webkit_baseline_path('*')) > > Doesn't this return a super huge list? This doesn't seem like the right algorithm. You want to enumerate the directories and then ask whether they're in the baseline search path, not enumerate all the things in those directories. > The glob() returns LayoutTests/platform/*, nor platform/*/*. It's a small list and the isdir call filters for the directories. > > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:187 > > + platform_prefix = self.name().split('-', 1)[0] > > This should be a helpful function instead of munging the string here. Will add.
(In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 87922 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=87922&action=review > > > > > Tools/Scripts/webkitpy/layout_tests/port/mac.py:134 > > > + def _path_to_webcore_library(self): > > > + return self._build_path('WebCore.framework/Versions/A/WebCore') > > > > This method does not appear to be used in this patch. > > > > > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:186 > > > + entries = self._filesystem.glob(self._webkit_baseline_path('*')) > > > > Doesn't this return a super huge list? This doesn't seem like the right algorithm. You want to enumerate the directories and then ask whether they're in the baseline search path, not enumerate all the things in those directories. > > > > The glob() returns LayoutTests/platform/*, nor platform/*/*. It's a small list and the isdir call filters for the directories. > > > > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:187 > > > + platform_prefix = self.name().split('-', 1)[0] > > > > This should be a helpful function instead of munging the string here. > > Will add. Er, actually, this was left over from an earlier, incorrect patch and isn't even needed. Deleted.
Created attachment 87941 [details] update w/ feedback from abarth
Comment on attachment 87941 [details] update w/ feedback from abarth View in context: https://bugs.webkit.org/attachment.cgi?id=87941&action=review > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:186 > + basename = self._filesystem.basename(entry) Shouldn't this be inside the if since the variable is only used inside the if?
Comment on attachment 87941 [details] update w/ feedback from abarth View in context: https://bugs.webkit.org/attachment.cgi?id=87941&action=review > Tools/Scripts/webkitpy/layout_tests/port/mac.py:134 > + def _path_to_webcore_library(self): > + return self._build_path('WebCore.framework/Versions/A/WebCore') I would have made this change in a separate patch, but i think it's fine to add it to this patch.
(In reply to comment #6) > (From update of attachment 87941 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=87941&action=review > > > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:186 > > + basename = self._filesystem.basename(entry) > > Shouldn't this be inside the if since the variable is only used inside the if? Sure, it can be. I will fix this when I land it. Thanks for the review.
Committed r82753: <http://trac.webkit.org/changeset/82753>
http://trac.webkit.org/changeset/82759 might have broken SnowLeopard Intel Release (WebKit2 Tests) The following tests are not passing: inspector/styles/styles-add-blank-property.html plugins/windowless_plugin_paint_test.html