There is a bug where <script src="../../../foo.js"></script> gets rewritten as "../../foo.js"></script> when its relative path is updated. The offending LOC is Line 175 in test_converter.py
Created attachment 239587 [details] Patch
Comment on attachment 239587 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=239587&action=review > Tools/ChangeLog:7 > + This patch fixes a bug in test_converter.py where src paths weren't > + getting rewritten properly. It also adds some tests for this case. i think you need to write a better description, it looks like this patch not only fixes src path rewriting, but also adds support for rewriting source paths in style elements. > Tools/Scripts/webkitpy/w3c/test_converter.py:46 > + Nit: This newline is just noise in the diff. > Tools/Scripts/webkitpy/w3c/test_converter.py:154 > + return self.convert_reference_relpaths(converted[1]) > + it looks like you accidentally duplicated this return statement? > Tools/Scripts/webkitpy/w3c/test_converter.py:175 > - converted = self.convert_reference_relpaths(attr_value) > + converted = re.sub(attr_value, self.convert_reference_relpaths(attr_value), converted) I don't think I understand how this fixed the issue. Can you explain? > Tools/Scripts/webkitpy/w3c/test_importer.py:279 > + Nit: This looks line another stray line.
Created attachment 239821 [details] Patch
Comment on attachment 239821 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=239821&action=review You need to rebase the patch, it no longer applies. Also, see below. > Tools/Scripts/webkitpy/w3c/test_converter.py:170 > - for attr_name, attr_value in attrs: > + for attr in attrs: Why the switch here? It seems much more readable to use atter_name and attr_value instead of attr[0] and attr[1]. > Tools/Scripts/webkitpy/w3c/test_converter.py:171 > if attr_name == 'src': attr_name isn't defined.
Comment on attachment 239821 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=239821&action=review >> Tools/Scripts/webkitpy/w3c/test_converter.py:170 >> + for attr in attrs: > > Why the switch here? It seems much more readable to use atter_name and attr_value instead of attr[0] and attr[1]. I mentioned this in my previous patch. I changed this to be more consistent with the other blocks in this function. If you think attr_name/value are better, I can change the other blocks in this function so they are all consistent. what do you think? >> Tools/Scripts/webkitpy/w3c/test_converter.py:171 >> if attr_name == 'src': > > attr_name isn't defined. oops, missed that. I'll fix in next patch.
(In reply to comment #5) > (From update of attachment 239821 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=239821&action=review > > >> Tools/Scripts/webkitpy/w3c/test_converter.py:170 > >> + for attr in attrs: > > > > Why the switch here? It seems much more readable to use atter_name and attr_value instead of attr[0] and attr[1]. > > I mentioned this in my previous patch. I changed this to be more consistent with the other blocks in this function. If you think attr_name/value are better, I can change the other blocks in this function so they are all consistent. what do you think? I must have missed that. I think it's always better to have names than random indexes. Indexes are too easy to mess up and are much harder to read. I think it would be better to fix the other blocks in a followup patch, just so that this one can stay focused on the problem at hand. > > >> Tools/Scripts/webkitpy/w3c/test_converter.py:171 > >> if attr_name == 'src': > > > > attr_name isn't defined. > > oops, missed that. I'll fix in next patch. I think you should stick with the names, so might as well leave it, but change how the iteration works. :-)
Created attachment 239898 [details] Patch
Comment on attachment 239898 [details] Patch Unfortunately, now the test is broken: [1029/1431] webkitpy.w3c.test_converter_unittest.W...Test.test_convert_attributes_if_needed failed: Traceback (most recent call last): File "/Users/bjonesbe/Code/webkit/svn/Tools/Scripts/webkitpy/w3c/test_converter_unittest.py", line 318, in test_convert_attributes_if_needed self.verify_reference_relative_paths(converted, test_reference_support_info) File "/Users/bjonesbe/Code/webkit/svn/Tools/Scripts/webkitpy/w3c/test_converter_unittest.py", line 358, in verify_reference_relative_paths self.assertTrue(expected_tag in converted[1], 'relative path ' + path + ' was not converted correcty') AssertionError: relative path ../../some-script.js was not converted correcty
Created attachment 239903 [details] Patch
Comment on attachment 239903 [details] Patch r=me
Comment on attachment 239903 [details] Patch Clearing flags on attachment: 239903 Committed r174748: <http://trac.webkit.org/changeset/174748>
All reviewed patches have been landed. Closing bug.