W3C Importer: Consolidate and simplify logic for deciding what not to convert.

W3C Importer: Consolidate and simplify logic for deciding what not to convert.

In this CL:
 - Move existing logic related to what files to skip from import_tests()
   and convert_for_webkit() into one method called should_try_to_convert().
 - Change rule so that before trying to convert the mimetype must appear
   to be text/html, text/css, or application/xhtml+xml.

Note that this could be changed to skip .css files too now, since no
.css files are currently being converted on import.

BUG=669452
Committed: https://crrev.com/5de62df3ed22e2354fa705b0ac7a819fc66d8767
Cr-Commit-Position: refs/heads/master@{#436063}

[qyearsley, 2016-12-01 18:58:21]

Description was changed from

==========
W3C Importer: Consolidate and simplify logic for deciding what not to convert.

BUG=669452
==========

to

==========
W3C Importer: Consolidate and simplify logic for deciding what not to convert.

In this CL:
 - Move existing logic related to what files to skip from import_tests() and
convert_for_webkit() into one method called should_try_to_convert().
 - Change rule so that before trying to convert the mimetype must appear to be 
text/html, text/css, or application/xhtml+xml.

Note that this could be changed to skip .css files too now, since no .css files
are currently being converted on import.

BUG=669452
==========

[qyearsley, 2016-12-01 18:58:22]

qyearsley@chromium.org changed reviewers:
+ foolip@chromium.org, rego@igalia.com

[qyearsley, 2016-12-01 21:36:20]

The CQ bit was checked by qyearsley@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 21:36:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Manuel Rego, 2016-12-01 22:00:52]

Really nice you tackle this so quickly!
I'm not an expert on this script, so I'll let others review it
but the patch seems good to me.
Please, remove the lines in LayoutTests/W3CImportExpectations,
related to the special file and the test that uses it.

Regarding the CL description, I believe you should wrap
the lines.

BTW, I've added a couple of minor inline comments too.

https://codereview.chromium.org/2542963002/diff/1/third_party/WebKit/Tools/Sc...
File third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer.py (right):

https://codereview.chromium.org/2542963002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer.py:438: # Only HTML
and XHTML files should be converted.
The comment doesn't match with the code,
it says HTML and XHTML, but the return line also checks if it's a CSS.

https://codereview.chromium.org/2542963002/diff/1/third_party/WebKit/Tools/Sc...
File third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer_unittest.py
(right):

https://codereview.chromium.org/2542963002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer_unittest.py:191:
self.assertFalse(TestImporter.should_try_to_convert({}, 'foo.svgz',
'LayoutTests/imported/csswg-test/x'))
Nit: Maybe add SVG too, which is much more common.
And maybe even an image.

[qyearsley, 2016-12-01 22:21:44]

Description was changed from

==========
W3C Importer: Consolidate and simplify logic for deciding what not to convert.

In this CL:
 - Move existing logic related to what files to skip from import_tests() and
convert_for_webkit() into one method called should_try_to_convert().
 - Change rule so that before trying to convert the mimetype must appear to be 
text/html, text/css, or application/xhtml+xml.

Note that this could be changed to skip .css files too now, since no .css files
are currently being converted on import.

BUG=669452
==========

to

==========
W3C Importer: Consolidate and simplify logic for deciding what not to convert.

In this CL:
 - Move existing logic related to what files to skip from import_tests()
   and convert_for_webkit() into one method called should_try_to_convert().
 - Change rule so that before trying to convert the mimetype must appear
   to be text/html, text/css, or application/xhtml+xml.

Note that this could be changed to skip .css files too now, since no
.css files are currently being converted on import.

BUG=669452
==========

[qyearsley, 2016-12-01 22:27:53]

Thanks for the review :-)

Now formatted the description and uploaded another patchset that responds to the
comments and removes the skips in W3CImportExpectations.

https://codereview.chromium.org/2542963002/diff/1/third_party/WebKit/Tools/Sc...
File third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer.py (right):

https://codereview.chromium.org/2542963002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer.py:438: # Only HTML
and XHTML files should be converted.
On 2016/12/01 at 22:00:52, Manuel Rego wrote:
> The comment doesn't match with the code,
> it says HTML and XHTML, but the return line also checks if it's a CSS.

Done

https://codereview.chromium.org/2542963002/diff/1/third_party/WebKit/Tools/Sc...
File third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer_unittest.py
(right):

https://codereview.chromium.org/2542963002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer_unittest.py:191:
self.assertFalse(TestImporter.should_try_to_convert({}, 'foo.svgz',
'LayoutTests/imported/csswg-test/x'))
On 2016/12/01 at 22:00:52, Manuel Rego wrote:
> Nit: Maybe add SVG too, which is much more common.
> And maybe even an image.

Done

[qyearsley, 2016-12-01 22:28:08]

The CQ bit was checked by qyearsley@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 22:28:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-01 23:26:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-01 23:26:49]

Dry run: This issue passed the CQ dry run.

[kojii, 2016-12-02 09:32:04]

lgtm, thank you for working on this.

[qyearsley, 2016-12-02 16:06:43]

The CQ bit was checked by qyearsley@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 16:07:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-02 16:11:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-02 16:11:10]

Dry run: This issue passed the CQ dry run.

[qyearsley, 2016-12-02 16:49:17]

The CQ bit was checked by qyearsley@chromium.org

[commit-bot: I haz the power, 2016-12-02 16:49:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-02 16:54:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-02 16:54:39]

Failed to apply patch for
third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_converter.py:
While running git apply --index -p1;
  error: patch failed:
third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_converter.py:45
  error: third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_converter.py: patch
does not apply

Patch:       third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_converter.py
Index: third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_converter.py
diff --git a/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_converter.py
b/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_converter.py
index
261a00505d7358d8764cac32a227d38423aaec4f..547e08e996c56ace4add564954150ab88247c215
100644
--- a/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_converter.py
+++ b/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_converter.py
@@ -45,13 +45,9 @@ def convert_for_webkit(new_path, filename,
reference_support_info, host=Host()):
         reference_support_info: Dict of information about a related reference
HTML, if any.
 
     Returns:
-        A pair of (list of modified CSS properties, modified text) if the file
-        should be modified; None, if the file is not modified.
+        A pair of (list of modified CSS properties, modified text).
     """
     # Conversion is not necessary for any tests in wpt now; see
http://crbug.com/654081.
-    if re.search(r'[/\\]imported[/\\]wpt[/\\]', new_path):
-        return None
-
     contents = host.filesystem.read_binary_file(filename)
     converter = _W3CTestConverter(new_path, filename, reference_support_info,
host)
     if filename.endswith('.css'):

[qyearsley, 2016-12-02 17:03:03]

The CQ bit was checked by qyearsley@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 17:03:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-02 20:11:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-02 20:11:38]

Dry run: This issue passed the CQ dry run.

[qyearsley, 2016-12-02 22:35:41]

The CQ bit was checked by qyearsley@chromium.org

[qyearsley, 2016-12-02 22:35:41]

The patchset sent to the CQ was uploaded after l-g-t-m from kojii@chromium.org
Link to the patchset: https://codereview.chromium.org/2542963002/#ps60001
(title: "Rebase")

[commit-bot: I haz the power, 2016-12-02 22:36:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-02 22:46:08]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1480718141051390,
"parent_rev": "abaa98e97c181515bea42382b8ea51e936dd1569", "commit_rev":
"052f39725ef2c52147f8775a69c9adc296c477fc"}

[commit-bot: I haz the power, 2016-12-02 22:46:39]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-12-02 22:49:55]

Description was changed from

==========
W3C Importer: Consolidate and simplify logic for deciding what not to convert.

In this CL:
 - Move existing logic related to what files to skip from import_tests()
   and convert_for_webkit() into one method called should_try_to_convert().
 - Change rule so that before trying to convert the mimetype must appear
   to be text/html, text/css, or application/xhtml+xml.

Note that this could be changed to skip .css files too now, since no
.css files are currently being converted on import.

BUG=669452
==========

to

==========
W3C Importer: Consolidate and simplify logic for deciding what not to convert.

In this CL:
 - Move existing logic related to what files to skip from import_tests()
   and convert_for_webkit() into one method called should_try_to_convert().
 - Change rule so that before trying to convert the mimetype must appear
   to be text/html, text/css, or application/xhtml+xml.

Note that this could be changed to skip .css files too now, since no
.css files are currently being converted on import.

BUG=669452

Committed: https://crrev.com/5de62df3ed22e2354fa705b0ac7a819fc66d8767
Cr-Commit-Position: refs/heads/master@{#436063}
==========

[commit-bot: I haz the power, 2016-12-02 22:49:56]

Patchset 4 (id:??) landed as
https://crrev.com/5de62df3ed22e2354fa705b0ac7a819fc66d8767
Cr-Commit-Position: refs/heads/master@{#436063}