[Webkit-unassigned] [Bug 66838] Extract reference links from reftest test file

[bugzilla-daemon at webkit.org]

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


Shinichiro Hamaji <hamaji at chromium.org> changed:

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




--- Comment #2 from Shinichiro Hamaji <hamaji at chromium.org>  2011-08-24 22:53:29 PST ---
(From update of attachment 105129)
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.

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