[webkit-reviews] review denied: [Bug 66838] Extract reference links from reftest test file : [Attachment 105129] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Aug 24 22:53:28 PDT 2011
Shinichiro Hamaji <hamaji at chromium.org> has denied Ai Makabi
<makabi at google.com>'s request for review:
Bug 66838: Extract reference links from reftest test file
https://bugs.webkit.org/show_bug.cgi?id=66838
Attachment 105129: Patch
https://bugs.webkit.org/attachment.cgi?id=105129&action=review
------- Additional Comments from Shinichiro Hamaji <hamaji at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=105129&action=review
This patch looks good, but putting r- for now as there are some nitpicks.
If you didn't yet, please run ./Tools/Scripts/check-webkit-style to see if
there are style issues.
> Tools/Scripts/webkitpy/layout_tests/reftests/extract_reference_link.py:42
> + if attrs["rel"] == "match":
I think this will raise an error (KeyError) if the input html contains <link>
without rel attribute.
> Tools/Scripts/webkitpy/layout_tests/reftests/extract_reference_link.py:48
> +def get_reference_link(html):
Could you write a docstring for this? I think it should explain the argument
(is this a string or a file?) and the return value (a tuple)
>
Tools/Scripts/webkitpy/layout_tests/reftests/extract_reference_link_unittest.py
:28
> +import extract_reference_link
from webkitpy.layout_tests.reftests import extract_reference_link ?
I guess this won't pass ./Tools/Scripts/test-webkit-scripts , but I'm not sure.
>
Tools/Scripts/webkitpy/layout_tests/reftests/extract_reference_link_unittest.py
:29
> +
BTW, we usually put imports in alphabetical order.
>
Tools/Scripts/webkitpy/layout_tests/reftests/extract_reference_link_unittest.py
:61
> + ["red-box-notref.xht", "red-box-notref.xht"])
It might be nice if we have another test function which exercises error cases.
For example, I'd test <link> without rel, unclosed <link> (what will happen if
the input is broken as an HTML...?), etc.
More information about the webkit-reviews
mailing list