Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1158193002:
Enable test_converter.py to import CSS Writing Modes test suites (Closed)
DescriptionEnable test_converter.py to import CSS Writing Modes test suites
This patch supports prefixed values, renamed properties, and renamed values,
in order to import CSS Writing Modes test suites.
In addition, since the "writing-mode" property is unprefixed for SVG but only
the prefixed property is effective in CSS, test_converter.py needs a special
casing for this property.
BUG=492664
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196192
Patch Set 1 #
Total comments: 10
Patch Set 2 : Comments added as per dirk's review #Patch Set 3 : Reverted set to list as one of the tests rely on it #Messages
Total messages: 19 (6 generated)
kojii@chromium.org changed reviewers: + dpranke@chromium.org
dpranke@, I'm looking into importing CSS Writing Modes test suites. Could you PTAL?
dpranke@chromium.org changed reviewers: + tabatkins@chromium.org
The code change basically looks fine, but I have some concerns over the idea behind this change, which I tried to lay out in the comments; I guess I'm not sure why we don't just update blink to support the new properties and values instead? Adding Tab to get another input on this ... https://codereview.chromium.org/1158193002/diff/1/Tools/Scripts/webkitpy/w3c/... File Tools/Scripts/webkitpy/w3c/test_converter.py (right): https://codereview.chromium.org/1158193002/diff/1/Tools/Scripts/webkitpy/w3c/... Tools/Scripts/webkitpy/w3c/test_converter.py:86: 'text-combine-upright': {'all': 'horizontal'} Is there a reason we can't just add support for the w3c's latest property names and values instead of rewriting to use the old ones? https://codereview.chromium.org/1158193002/diff/1/Tools/Scripts/webkitpy/w3c/... Tools/Scripts/webkitpy/w3c/test_converter.py:87: } this section could use some comments describing specifically what each field will do, e.g. prefixed_properties is a list of properties where we will add "-webkit-" on the front of each name. prefixed_values is a map of properties where the name stays the same (does it?) but we prefix each value. renamed_properties is a map where simply adding a prefix isn't good enough. etc. https://codereview.chromium.org/1158193002/diff/1/Tools/Scripts/webkitpy/w3c/... Tools/Scripts/webkitpy/w3c/test_converter.py:120: unprefixed_properties.discard('writing-mode') I don't think I understand this change; if we still want writing-mode to be prefixed, shouldn't we fix CSSProperties.in instead? https://codereview.chromium.org/1158193002/diff/1/Tools/Scripts/webkitpy/w3c/... Tools/Scripts/webkitpy/w3c/test_converter.py:123: return set(prop for prop in prefixed_properties if prop not in unprefixed_properties) why did this change to a set() ?
Thank you for the quick review, Dirk. The basic idea is that I'm not confident enough to unprefix them yet, and as part of the work to unprefix, I'd like to run the tests more regularly. There is one volunteer who runs the tests manually and report bugs, but I'd like to make this process more reliable and visible. For unicode-bidi values, I discussed with aharon@, who was not confident of the quality yet too. It is possible to support new syntax without uprefixing. This would help not doing renamed_properties and renamed_values in this script. However, by looking at the discussion at CSS WG as the co-editor of the spec, I expect another round of renaming or two. This spec was forked to EPUB spec and there are people relying on the forked syntax, so I'd like us to support new syntax when renaming is really stabilized and with a careful planning, with the least number of renaming in our implementation. Does this look reasonable? https://codereview.chromium.org/1158193002/diff/1/Tools/Scripts/webkitpy/w3c/... File Tools/Scripts/webkitpy/w3c/test_converter.py (right): https://codereview.chromium.org/1158193002/diff/1/Tools/Scripts/webkitpy/w3c/... Tools/Scripts/webkitpy/w3c/test_converter.py:86: 'text-combine-upright': {'all': 'horizontal'} On 2015/05/27 22:52:59, Dirk Pranke wrote: > Is there a reason we can't just add support for the w3c's latest property names > and values instead of rewriting to use the old ones? Replied above. https://codereview.chromium.org/1158193002/diff/1/Tools/Scripts/webkitpy/w3c/... Tools/Scripts/webkitpy/w3c/test_converter.py:87: } On 2015/05/27 22:52:59, Dirk Pranke wrote: > this section could use some comments describing specifically what each field > will do, e.g. > > prefixed_properties is a list of properties where we will add "-webkit-" on the > front of each name. > > prefixed_values is a map of properties where the name stays the same (does it?) > but we prefix each value. > > renamed_properties is a map where simply adding a prefix isn't good enough. > > etc. I'll add and re-upload, but allow me to reply as there are other points to discuss. https://codereview.chromium.org/1158193002/diff/1/Tools/Scripts/webkitpy/w3c/... Tools/Scripts/webkitpy/w3c/test_converter.py:120: unprefixed_properties.discard('writing-mode') On 2015/05/27 22:52:59, Dirk Pranke wrote: > I don't think I understand this change; if we still want writing-mode to be > prefixed, shouldn't we fix CSSProperties.in instead? There are two properties, "writing-mode" and "-webkit-writing-mode". The former is in SVG REC, and applies only within SVG context. The latter is in CSS, applies only in HTML context. So both properties exist, but we need to prefix to test CSS writing-mode. In 3-6 months I'm planning to plan unprefixing CSS Writing Modes, and I need to come up with a plan to solve this problem. This is likely to break some content, so I need more investigation and discussion, but I'd like to make sure our layout code is stable enough to unprefix before making decisions on the uprefix plan, and importing tests is to help that. One of the discussion at www-style is here: https://lists.w3.org/Archives/Public/www-style/2015May/0150.html https://codereview.chromium.org/1158193002/diff/1/Tools/Scripts/webkitpy/w3c/... Tools/Scripts/webkitpy/w3c/test_converter.py:123: return set(prop for prop in prefixed_properties if prop not in unprefixed_properties) On 2015/05/27 22:52:59, Dirk Pranke wrote: > why did this change to a set() ? Because line #139 uses "in" operator to check if the property needs prefix. It's not needed before because all properties in regexp should be prefixed before this change. Now we include value prefix etc. too, so we need to check whether to prefix or not.
Allow me to add one more comment that as part of the unprefix plan mentioned above, I'll make sure these are get rid of, so that we can test unprefixed tests in the most recent syntax.
Okay, I think I'm fine w/ the general approach. I still have a question about writing-mode and how it might affect importing SVG tests (assuming they exist). Can you also add a FIXME to clean this up when the values are unprefixed? https://codereview.chromium.org/1158193002/diff/1/Tools/Scripts/webkitpy/w3c/... File Tools/Scripts/webkitpy/w3c/test_converter.py (right): https://codereview.chromium.org/1158193002/diff/1/Tools/Scripts/webkitpy/w3c/... Tools/Scripts/webkitpy/w3c/test_converter.py:120: unprefixed_properties.discard('writing-mode') On 2015/05/27 23:47:38, koji wrote: > On 2015/05/27 22:52:59, Dirk Pranke wrote: > > I don't think I understand this change; if we still want writing-mode to > be > > prefixed, shouldn't we fix CSSProperties.in instead? > > There are two properties, "writing-mode" and "-webkit-writing-mode". The former > is in SVG REC, and applies only within SVG context. The latter is in CSS, > applies only in HTML context. So both properties exist, but we need to prefix to > test CSS writing-mode. Will this likely cause problems if we import some SVG tests, then? https://codereview.chromium.org/1158193002/diff/1/Tools/Scripts/webkitpy/w3c/... Tools/Scripts/webkitpy/w3c/test_converter.py:123: return set(prop for prop in prefixed_properties if prop not in unprefixed_properties) On 2015/05/27 23:47:38, koji wrote: > On 2015/05/27 22:52:59, Dirk Pranke wrote: > > why did this change to a set() ? > > Because line #139 uses "in" operator to check if the property needs prefix. It's > not needed before because all properties in regexp should be prefixed before > this change. Now we include value prefix etc. too, so we need to check whether > to prefix or not. Okay. `in` actually works with both lists and sets, so I think the change is not technically needed, but it's probably a reasonable optimization, so that's fine.
It would be good for Tab to weigh in on this as well ...
On 2015/05/28 01:25:50, Dirk Pranke wrote: > Okay, I think I'm fine w/ the general approach. I still have a question about > writing-mode and how it might affect importing SVG tests (assuming they exist). > > Can you also add a FIXME to clean this up when the values are unprefixed? Right, done. > https://codereview.chromium.org/1158193002/diff/1/Tools/Scripts/webkitpy/w3c/... > File Tools/Scripts/webkitpy/w3c/test_converter.py (right): > > https://codereview.chromium.org/1158193002/diff/1/Tools/Scripts/webkitpy/w3c/... > Tools/Scripts/webkitpy/w3c/test_converter.py:120: > unprefixed_properties.discard('writing-mode') > On 2015/05/27 23:47:38, koji wrote: > > On 2015/05/27 22:52:59, Dirk Pranke wrote: > > > I don't think I understand this change; if we still want > writing-mode to > > be > > > prefixed, shouldn't we fix CSSProperties.in instead? > > > > There are two properties, "writing-mode" and "-webkit-writing-mode". The > former > > is in SVG REC, and applies only within SVG context. The latter is in CSS, > > applies only in HTML context. So both properties exist, but we need to prefix > to > > test CSS writing-mode. > > Will this likely cause problems if we import some SVG tests, then? Yes, I added that comment too. We could switch by file types, but for mixed content, it requires some more work. Parse context more in depth, use values as a hint (values were renamed), or add both properties are ideas from top of my heads. I can work if we were importing SVG tests before we unprefix writing-mode.
On 2015/05/28 01:28:58, Dirk Pranke wrote: > It would be good for Tab to weigh in on this as well ... Right...I'll try to ping him by e-mail.
lgtm
The CQ bit was checked by kojii@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1158193002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/6...)
The CQ bit was checked by kojii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org Link to the patchset: https://codereview.chromium.org/1158193002/#ps40001 (title: "Reverted set to list as one of the tests rely on it")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1158193002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://src.chromium.org/viewvc/blink?view=rev&revision=196192 |
