Bug 66838

Created attachment 105129 [details] Patch

Comment on attachment 105129 [details] Patch 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.

Created attachment 105742 [details] Patch

Comment on attachment 105129 [details] Patch Thank you for the review. View in context: https://bugs.webkit.org/attachment.cgi?id=105129&action=review >> 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. Done. >> 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) Done. >> 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. Done. >> Tools/Scripts/webkitpy/layout_tests/reftests/extract_reference_link_unittest.py:29 >> + > > BTW, we usually put imports in alphabetical order. Done. >> 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. Done.

I guess EWSs are purple because of the empty __init__.py. Would you add a comment in __init__.py please?

Created attachment 105752 [details] Patch

Done. (In reply to comment #5) > I guess EWSs are purple because of the empty __init__.py. Would you add a comment in __init__.py please?

Comment on attachment 105752 [details] Patch Looks good. Thanks for addressing my comments.

Comment on attachment 105752 [details] Patch Clearing flags on attachment: 105752 Committed r94160: <http://trac.webkit.org/changeset/94160>

All reviewed patches have been landed. Closing bug.