Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2380713004:
[GeometryInterface] Add setMatrixValue(transfromList) function. (Closed)
|
Created:
4 years, 2 months ago by Hwanseung Lee Modified:
4 years, 1 month ago CC:
blink-reviews, blink-reviews-dom_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, Hwanseung Lee(hs1217.lee), rwlbuis, sof Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[GeometryInterface] Add setMatrixValue(transfromList) function.
1. if transformList is the empty string,
set in to the string "matrix(1, 0, 0, 1, 0, 0)".
2. Parse transformList by following the syntax description[1].
if parsing failed, throw SyntaxError.
3. Set is2D to false if the <transform-list> consists of any 3D
Transform functions. Otherwise set is2D to true.
4. Transform all <transform-function>s to 4x4 matrices.
5. Post-multiply all matrices from left to right.
6. return the current matrix.
spec list:
https://drafts.fxtf.org/geometry/#dom-dommatrix-setmatrixvalue
[1] = https://drafts.csswg.org/css-transforms-1/#svg-syntax
BUG=388780, 645884
Committed: https://crrev.com/1b84ec0976538570d0407d95677038679225dd0e
Cr-Commit-Position: refs/heads/master@{#428352}
Patch Set 1 #
Total comments: 1
Patch Set 2 : remove dependency. #
Total comments: 2
Patch Set 3 : rebase. #Patch Set 4 : rebase & check relative length unit. #
Total comments: 21
Patch Set 5 : [GeometryInterface] Add setMatrixValue(transfromList) function. #
Total comments: 24
Patch Set 6 : [GeometryInterface] Add setMatrixValue(transfromList) function. #
Total comments: 16
Patch Set 7 : [GeometryInterface] Add setMatrixValue(transfromList) function. #
Total comments: 8
Patch Set 8 : [GeometryInterface] Add setMatrixValue(transfromList) function. #Patch Set 9 : [GeometryInterface] Add setMatrixValue(transfromList) function. #
Total comments: 4
Patch Set 10 : [GeometryInterface] Add setMatrixValue(transfromList) function. #Patch Set 11 : [GeometryInterface] Add setMatrixValue(transfromList) function. #Messages
Total messages: 67 (39 generated)
Description was changed from ========== [GeometryInterface] Add setMatrixValue(transfromList) function. 1. if transformList is the empty string, set in to the string "matrix(1, 0, 0, 1, 0, 0)". 2. Parse transformList by following the syntax description[1]. if parsing failed, throw SyntaxError. 3. Set is2D to false if the <transform-list> consists of any 3D Transform functions. Otherwise set is2D to true. 4. Transform all <transform-function>s to 4x4 matrices. 5. Post-multiply all matrices from left to right. 6. return the current matrix. spec list: https://drafts.fxtf.org/geometry/#dom-dommatrix-setmatrixvalue [1] = https://drafts.csswg.org/css-transforms-1/#svg-syntax BUG=388780, 645884 ========== to ========== [GeometryInterface] Add setMatrixValue(transfromList) function. 1. if transformList is the empty string, set in to the string "matrix(1, 0, 0, 1, 0, 0)". 2. Parse transformList by following the syntax description[1]. if parsing failed, throw SyntaxError. 3. Set is2D to false if the <transform-list> consists of any 3D Transform functions. Otherwise set is2D to true. 4. Transform all <transform-function>s to 4x4 matrices. 5. Post-multiply all matrices from left to right. 6. return the current matrix. spec list: https://drafts.fxtf.org/geometry/#dom-dommatrix-setmatrixvalue [1] = https://drafts.csswg.org/css-transforms-1/#svg-syntax BUG=388780, 645884 ==========
hs1217.lee@samsung.com changed reviewers: + dominicc@chromium.org, jinho.bang@samsung.com
The CQ bit was checked by jinho.bang@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
@dominicc, zino. PTAL. this CL is based on rotateAxisAngle CL(https://codereview.chromium.org/2365153002/). so if you weren't review rotateAxisAngle CL, please review rotateAxisAngle CL first. and if rotateAxisAngle CL look good to you, please review this CL immediately. thank you.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2380713004/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-setMatrixValue.html (right): https://codereview.chromium.org/2380713004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-setMatrixValue.html:60: expectedMatrix.rotateAxisAngleSelf(0, 0, 1, 90); patch_set 1 is failed in webkit_test. because rotateAxisAngleSelf is not merged at server. when cherry-pick rotateAxisAngleSelf CL & this CL and run webkit_test in local PC, all test passed.
On 2016/09/29 at 23:15:55, hs1217.lee wrote: > https://codereview.chromium.org/2380713004/diff/1/third_party/WebKit/LayoutTe... > File third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-setMatrixValue.html (right): > > https://codereview.chromium.org/2380713004/diff/1/third_party/WebKit/LayoutTe... > third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-setMatrixValue.html:60: expectedMatrix.rotateAxisAngleSelf(0, 0, 1, 90); > patch_set 1 is failed in webkit_test. > because rotateAxisAngleSelf is not merged at server. > when cherry-pick rotateAxisAngleSelf CL & this CL and run webkit_test in local PC, > all test passed. OK. I think there's a way you can make a patch depend on another patch but I can't find it in the UI... Maybe the style team should review this patch; it looks OK but I don't know these APIs well. Maybe shanestephens can suggest someone?
hs1217.lee@samsung.com changed reviewers: + shanestephens@google.com
@shanestephens PTAL. could review about CSS parsing at setMatrixValue method? setMatrixValue method is located DOMMatrix.cpp. or could recommend other appropriate reviewer? thank you.
hs1217.lee@samsung.com changed reviewers: + chrishtr@chromium.org - shanestephens@google.com
@chrishtr PTAL. could review about CSS parsing at setMatrixValue method? setMatrixValue method is located DOMMatrix.cpp. or could recommend other appropriate reviewer?
hs1217.lee@samsung.com changed reviewers: + meade@chromium.org - chrishtr@chromium.org
@meade PTAL. could review about CSS parsing at setMatrixValue method? setMatrixValue method is located DOMMatrix.cpp. or could recommend other appropriate reviewer?
The CQ bit was checked by meade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
meade@chromium.org changed reviewers: + timloh@chromium.org
All your bots are red? I left a comment, but I think Tim would be a better person to review this code, also I don't have OWNERS. https://codereview.chromium.org/2380713004/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-setMatrixValue.html (right): https://codereview.chromium.org/2380713004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-setMatrixValue.html:9: var expectedMatrix = new DOMMatrix([1, 2, 3, 4, 5, 6]); This seems suspicious - from reading your CL description, it should be [1,0,0,1,0,0]? Likewise in the rest of the tests in this file, your expectedMatrix is [1,2,3,4,5,6], which doesn't seem right.
@Eddy. trybot was failed. because these day, blink code was reformatted. so this code need to rebase. i did rebase this CL. @tim PTAL. Thank you. https://codereview.chromium.org/2380713004/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-setMatrixValue.html (right): https://codereview.chromium.org/2380713004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-setMatrixValue.html:9: var expectedMatrix = new DOMMatrix([1, 2, 3, 4, 5, 6]); On 2016/10/13 04:18:50, Eddy wrote: > This seems suspicious - from reading your CL description, it should be > [1,0,0,1,0,0]? > > Likewise in the rest of the tests in this file, your expectedMatrix is > [1,2,3,4,5,6], which doesn't seem right. Done.
The CQ bit was checked by meade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
It doesn't look like step 2 in the spec text (pasted at the bottom of this comment) is correctly followed. It's not clear to me whether 'none' is or isn't supposed to be accepted. The prose here explicitly suggests it should be allowed, although it's actually a <transform-list>. I think the intention is that 'none' is allowed but this needs to be followed up on the spec. The code needs to reject any lengths which are not absolute length units, but it currently only rejects percentages (actually percentages are not <length>s, please follow up on the spec so it's properly handled there). This means that we're going to accept values like 5em or 10vmin when we should be rejecting them. The logic for this is probably going to be useful for the CSS Properties & Values API so it should probably be somewhere shared (maybe in TransformBuilder?) ------------------- Parse transformList by following the syntax description in “Syntax of the SVG ‘transform’ attribute” [CSS3-TRANSFORMS] to a <transform-list>. If parsing is not successful, or any <transform-function> has <length> values without absolute length units, or any keyword other than none is used, throw a SyntaxError exception.
dominicc@chromium.org changed reviewers: - dominicc@chromium.org
On 2016/10/14 02:19:01, Timothy Loh wrote: > It doesn't look like step 2 in the spec text (pasted at the bottom of this > comment) is correctly followed. > > It's not clear to me whether 'none' is or isn't supposed to be accepted. The > prose here explicitly suggests it should be allowed, although it's actually a > <transform-list>. I think the intention is that 'none' is allowed but this needs > to be followed up on the spec. > > The code needs to reject any lengths which are not absolute length units, but it > currently only rejects percentages (actually percentages are not <length>s, > please follow up on the spec so it's properly handled there). This means that > we're going to accept values like 5em or 10vmin when we should be rejecting > them. The logic for this is probably going to be useful for the CSS Properties & > Values API so it should probably be somewhere shared (maybe in > TransformBuilder?) > > ------------------- > > Parse transformList by following the syntax description in “Syntax of the SVG > ‘transform’ attribute” [CSS3-TRANSFORMS] to a <transform-list>. If parsing is > not successful, or any <transform-function> has <length> values without absolute > length units, or any keyword other than none is used, throw a SyntaxError > exception. i did add code to check relative length unit. could you review again?
The CQ bit was checked by meade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2380713004/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-setMatrixValue.html (right): https://codereview.chromium.org/2380713004/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-setMatrixValue.html:197: }, "can't parsing without absolute unit."); Nit: English grammar. Should be "Can't parse without absolute unit." https://codereview.chromium.org/2380713004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/resolver/TransformBuilder.cpp (right): https://codereview.chromium.org/2380713004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/TransformBuilder.cpp:105: bool TransformBuilder::checkHavingRelativeLengthUnit(const CSSValue& inValue) { In other places we've used "hasRelativeLengths". Please keep naming consistent. https://codereview.chromium.org/2380713004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/TransformBuilder.cpp:105: bool TransformBuilder::checkHavingRelativeLengthUnit(const CSSValue& inValue) { Perhaps this should take a CSSValueList? Then you can call the argument valueList. https://codereview.chromium.org/2380713004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/TransformBuilder.cpp:107: DCHECK_EQ(toCSSIdentifierValue(inValue).getValueID(), CSSValueNone); I don't understand what this DCHECK is for... Do you care what the valueID of the input was if you're going to return false here? https://codereview.chromium.org/2380713004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/DOMMatrix.cpp (right): https://codereview.chromium.org/2380713004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/DOMMatrix.cpp:231: inputString = "matrix(1, 0, 0, 1, 0, 0)"; You can use DEFINE_STATIC_LOCAL to avoid allocating this multiple times. https://codereview.chromium.org/2380713004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/DOMMatrix.cpp:234: } This can be shortened to just 3 lines. String inputString = string; if (string.isEmpty()) inputString = identityMatrix2D; https://codereview.chromium.org/2380713004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/DOMMatrix.cpp:241: "Failed to parse '" + string + "'."); This shouldn't continue after throwing the exception right? Need to return nullptr in this if statement. https://codereview.chromium.org/2380713004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/DOMMatrix.cpp:248: "relative length unit can't support."); Nit: Incorrect English grammar. Should be something like "Relative lengths not supported". Same comment as above about returning nullptr after throwing the exception. https://codereview.chromium.org/2380713004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/DOMMatrix.cpp:262: "size, which is not supported."); Same comment about returning nullptr after throwing an exception. https://codereview.chromium.org/2380713004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/DOMMatrix.cpp:271: } else { // There is something there but parsing failed. This structure would be easier to read (exit early on errors and have one less indentation level) if it was something more like const CSSValue* value = CSSParser::parseSingleValue(CSSPropertyTransform, inputString); if (!value) { exceptionState.throwDOMException(SyntaxError, ...); return nullptr; } if (value->isIdentifierValue(value)) { exceptionState.throwDOMException(SyntaxError, ...); return nullptr; } ... etc
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
@Eddy i fixed it follow your comment. https://codereview.chromium.org/2380713004/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-setMatrixValue.html (right): https://codereview.chromium.org/2380713004/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-setMatrixValue.html:197: }, "can't parsing without absolute unit."); On 2016/10/17 03:22:28, Eddy wrote: > Nit: English grammar. Should be "Can't parse without absolute unit." Done. https://codereview.chromium.org/2380713004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/resolver/TransformBuilder.cpp (right): https://codereview.chromium.org/2380713004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/TransformBuilder.cpp:105: bool TransformBuilder::checkHavingRelativeLengthUnit(const CSSValue& inValue) { On 2016/10/17 03:22:28, Eddy wrote: > In other places we've used "hasRelativeLengths". Please keep naming consistent. Done. https://codereview.chromium.org/2380713004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/TransformBuilder.cpp:105: bool TransformBuilder::checkHavingRelativeLengthUnit(const CSSValue& inValue) { On 2016/10/17 03:22:28, Eddy wrote: > Perhaps this should take a CSSValueList? Then you can call the argument > valueList. Done. https://codereview.chromium.org/2380713004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/TransformBuilder.cpp:107: DCHECK_EQ(toCSSIdentifierValue(inValue).getValueID(), CSSValueNone); On 2016/10/17 03:22:28, Eddy wrote: > I don't understand what this DCHECK is for... Do you care what the valueID of > the input was if you're going to return false here? i did remove this code because parameter was changed to CSSVauleList. https://codereview.chromium.org/2380713004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/TransformBuilder.cpp:107: DCHECK_EQ(toCSSIdentifierValue(inValue).getValueID(), CSSValueNone); On 2016/10/17 03:22:28, Eddy wrote: > I don't understand what this DCHECK is for... Do you care what the valueID of > the input was if you're going to return false here? i did remove this it when change to CSSVauleList. https://codereview.chromium.org/2380713004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/DOMMatrix.cpp (right): https://codereview.chromium.org/2380713004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/DOMMatrix.cpp:231: inputString = "matrix(1, 0, 0, 1, 0, 0)"; On 2016/10/17 03:22:28, Eddy wrote: > You can use DEFINE_STATIC_LOCAL to avoid allocating this multiple times. > Done. https://codereview.chromium.org/2380713004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/DOMMatrix.cpp:234: } On 2016/10/17 03:22:28, Eddy wrote: > This can be shortened to just 3 lines. > > String inputString = string; > if (string.isEmpty()) > inputString = identityMatrix2D; Done. https://codereview.chromium.org/2380713004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/DOMMatrix.cpp:241: "Failed to parse '" + string + "'."); On 2016/10/17 03:22:28, Eddy wrote: > This shouldn't continue after throwing the exception right? Need to return > nullptr in this if statement. Done. https://codereview.chromium.org/2380713004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/DOMMatrix.cpp:248: "relative length unit can't support."); On 2016/10/17 03:22:28, Eddy wrote: > Nit: Incorrect English grammar. Should be something like "Relative lengths not > supported". > > Same comment as above about returning nullptr after throwing the exception. Done. https://codereview.chromium.org/2380713004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/DOMMatrix.cpp:262: "size, which is not supported."); On 2016/10/17 03:22:28, Eddy wrote: > Same comment about returning nullptr after throwing an exception. Done. https://codereview.chromium.org/2380713004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/DOMMatrix.cpp:271: } else { // There is something there but parsing failed. On 2016/10/17 03:22:28, Eddy wrote: > This structure would be easier to read (exit early on errors and have one less > indentation level) if it was something more like > > > const CSSValue* value = CSSParser::parseSingleValue(CSSPropertyTransform, > inputString); > if (!value) { > exceptionState.throwDOMException(SyntaxError, ...); > return nullptr; > } > > if (value->isIdentifierValue(value)) { > exceptionState.throwDOMException(SyntaxError, ...); > return nullptr; > } > > ... etc Done.
lgtm You'll still need an OWNER to review, since I don't have OWNERS.
The CQ bit was checked by meade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2380713004/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-setMatrixValue.html (right): https://codereview.chromium.org/2380713004/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-setMatrixValue.html:106: var matrix2d = new DOMMatrix(); Why the name 'matrix2d'? I think just calling everything 'matrix' would be better. https://codereview.chromium.org/2380713004/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-setMatrixValue.html:143: test(() => { This test would be more readable if you just used a single DOMMatrix object. Also it'd be nice to test that the value 'initial' is disallowed. https://codereview.chromium.org/2380713004/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-setMatrixValue.html:146: matrix2d.setMatrixValue("none"); As I said before, I'm not sure that 'none' is supposed to be rejected. I think we should be accepting 'none', but please file a spec bug for clarification as 'none' is not actually a '<transform-list>'. https://codereview.chromium.org/2380713004/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-setMatrixValue.html:151: matrix2d.setMatrixValue("notExist() =>"); Is the "=>" intentional here? https://codereview.chromium.org/2380713004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/resolver/TransformBuilder.cpp (right): https://codereview.chromium.org/2380713004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/TransformBuilder.cpp:109: for (size_t i = 0; i < transformValue->length(); i++) { This can probably be a for ( : ) loop too. https://codereview.chromium.org/2380713004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/TransformBuilder.cpp:113: if (CSSPrimitiveValue::isRelativeUnit( I think this isn't sufficient and won't handle cases like calc(10px + 1em) correctly. At least add a test (you can check in an expected.txt file with the failure) for it and a TODO here. https://codereview.chromium.org/2380713004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/resolver/TransformBuilder.h (right): https://codereview.chromium.org/2380713004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/TransformBuilder.h:48: static bool hasRelativeLengths(const CSSValueList& inValueList); inValueList is a weird name, I'd just call it valueList. I realise you're copying the style of the other function here but I think we should just give this a better name. You also shouldn't put a name in the function declaration where it doesn't add value (i.e. here). https://codereview.chromium.org/2380713004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/DOMMatrix.cpp (right): https://codereview.chromium.org/2380713004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/DOMMatrix.cpp:222: static inline PassRefPtr<ComputedStyle> createInitialStyle() { Just use ComputedStyle::initialStyle() instead of creating a new one. https://codereview.chromium.org/2380713004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/DOMMatrix.cpp:231: String inputString = string; This naming is weird, if you have inputString and string I would expect the argument to be inputString. https://codereview.chromium.org/2380713004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/DOMMatrix.cpp:265: // Convert transform operations to a TransformationMatrix. This can fail This comment is misplaced, the conversion is 12 lines below, and also incorrect because the conversion doesn't actually fail. https://codereview.chromium.org/2380713004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/DOMMatrix.cpp:275: m_is2D = false; Shouldn't this be m_is2D = !operations.has3DOperation(); Because otherwise if you have a 3d matrix and call setMatrixValue with a 2d transform list the flag won't be correctly set. Please have some sort of test for this.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
@Timothy i fix code follow your comment. PTAL. thank you. https://codereview.chromium.org/2380713004/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-setMatrixValue.html (right): https://codereview.chromium.org/2380713004/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-setMatrixValue.html:106: var matrix2d = new DOMMatrix(); On 2016/10/19 02:59:06, Timothy Loh wrote: > Why the name 'matrix2d'? I think just calling everything 'matrix' would be > better. Done. https://codereview.chromium.org/2380713004/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-setMatrixValue.html:143: test(() => { On 2016/10/19 02:59:06, Timothy Loh wrote: > This test would be more readable if you just used a single DOMMatrix object. > Also it'd be nice to test that the value 'initial' is disallowed. Done. https://codereview.chromium.org/2380713004/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-setMatrixValue.html:146: matrix2d.setMatrixValue("none"); On 2016/10/19 02:59:06, Timothy Loh wrote: > As I said before, I'm not sure that 'none' is supposed to be rejected. I think > we should be accepting 'none', but please file a spec bug for clarification as > 'none' is not actually a '<transform-list>'. i fixed code to accept 'none' and i add test code about 'none' https://codereview.chromium.org/2380713004/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-setMatrixValue.html:151: matrix2d.setMatrixValue("notExist() =>"); On 2016/10/19 02:59:06, Timothy Loh wrote: > Is the "=>" intentional here? no. it is mistake. i remove it. https://codereview.chromium.org/2380713004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/resolver/TransformBuilder.cpp (right): https://codereview.chromium.org/2380713004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/TransformBuilder.cpp:109: for (size_t i = 0; i < transformValue->length(); i++) { On 2016/10/19 02:59:08, Timothy Loh wrote: > This can probably be a for ( : ) loop too. Done. https://codereview.chromium.org/2380713004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/TransformBuilder.cpp:113: if (CSSPrimitiveValue::isRelativeUnit( On 2016/10/19 02:59:08, Timothy Loh wrote: > I think this isn't sufficient and won't handle cases like calc(10px + 1em) > correctly. At least add a test (you can check in an expected.txt file with the > failure) for it and a TODO here. i add some code to prevent calc() function. i will fix it at next CL. so i add TODO comment. https://codereview.chromium.org/2380713004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/resolver/TransformBuilder.h (right): https://codereview.chromium.org/2380713004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/TransformBuilder.h:48: static bool hasRelativeLengths(const CSSValueList& inValueList); On 2016/10/19 02:59:08, Timothy Loh wrote: > inValueList is a weird name, I'd just call it valueList. I realise you're > copying the style of the other function here but I think we should just give > this a better name. You also shouldn't put a name in the function declaration > where it doesn't add value (i.e. here). Done. https://codereview.chromium.org/2380713004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/DOMMatrix.cpp (right): https://codereview.chromium.org/2380713004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/DOMMatrix.cpp:222: static inline PassRefPtr<ComputedStyle> createInitialStyle() { On 2016/10/19 02:59:08, Timothy Loh wrote: > Just use ComputedStyle::initialStyle() instead of creating a new one. Done. https://codereview.chromium.org/2380713004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/DOMMatrix.cpp:231: String inputString = string; On 2016/10/19 02:59:08, Timothy Loh wrote: > This naming is weird, if you have inputString and string I would expect the > argument to be inputString. Done. https://codereview.chromium.org/2380713004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/DOMMatrix.cpp:265: // Convert transform operations to a TransformationMatrix. This can fail On 2016/10/19 02:59:08, Timothy Loh wrote: > This comment is misplaced, the conversion is 12 lines below, and also incorrect > because the conversion doesn't actually fail. Done. https://codereview.chromium.org/2380713004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/DOMMatrix.cpp:275: m_is2D = false; On 2016/10/19 02:59:08, Timothy Loh wrote: > Shouldn't this be > > m_is2D = !operations.has3DOperation(); > > Because otherwise if you have a 3d matrix and call setMatrixValue with a 2d > transform list the flag won't be correctly set. Please have some sort of test > for this. Done.
Almost there. https://codereview.chromium.org/2380713004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/DOMMatrix.cpp (right): https://codereview.chromium.org/2380713004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/DOMMatrix.cpp:222: static inline PassRefPtr<ComputedStyle> createInitialStyle() { On 2016/10/22 14:44:56, Hwanseung Lee wrote: > On 2016/10/19 02:59:08, Timothy Loh wrote: > > Just use ComputedStyle::initialStyle() instead of creating a new one. > > Done. I meant that we shouldn't need a createInitialStyle() function at all, can't we call ComputedStyle::initialStyle() where you're currently calling createInitialStyle()? https://codereview.chromium.org/2380713004/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-setMatrixValue.html (right): https://codereview.chromium.org/2380713004/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-setMatrixValue.html:8: var matrix = new DOMMatrix(); I would create two DOMMatrix objects at the start (one 2D and one 3D, both with garbage values), and then in each test initialise the local matrix to the type it won't end up with. For example the first test makes a 2D matrix so initialise it to the 3D matrix. Doing this will allow the test to catch more failure cases like not correctly setting the is2D flag or not initialising the matrix values. https://codereview.chromium.org/2380713004/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-setMatrixValue.html:157: }, "can't parse not exist keyword."); I would write something like "CSS-wide keywords are disallowed". My intention in suggesting to test "initial" is that in CSS, the keyword (along with several others) is allowed as a value for any CSS property. setMatrixValue takes almost the identical set of things as the transform property, but it excludes the css-wide keywords. https://codereview.chromium.org/2380713004/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-setMatrixValue.html:164: unintentional blank line? https://codereview.chromium.org/2380713004/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-setMatrixValue.html:204: // TODO(hs1217.lee) : should be throw exception. This case shouldn't throw an exception, right? So it should be another top level test instead of being part of the exception tests. https://codereview.chromium.org/2380713004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/resolver/TransformBuilder.cpp (right): https://codereview.chromium.org/2380713004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/TransformBuilder.cpp:109: for (Member<const blink::CSSValue> item : *transformValue) { I think you can write for (const CSSValue* item :
@Timothy PTAL. Thank you. https://codereview.chromium.org/2380713004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/DOMMatrix.cpp (right): https://codereview.chromium.org/2380713004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/DOMMatrix.cpp:222: static inline PassRefPtr<ComputedStyle> createInitialStyle() { On 2016/10/24 04:18:30, Timothy Loh wrote: > On 2016/10/22 14:44:56, Hwanseung Lee wrote: > > On 2016/10/19 02:59:08, Timothy Loh wrote: > > > Just use ComputedStyle::initialStyle() instead of creating a new one. > > > > Done. > > I meant that we shouldn't need a createInitialStyle() function at all, can't we > call ComputedStyle::initialStyle() where you're currently calling > createInitialStyle()? Done. https://codereview.chromium.org/2380713004/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-setMatrixValue.html (right): https://codereview.chromium.org/2380713004/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-setMatrixValue.html:8: var matrix = new DOMMatrix(); On 2016/10/24 04:18:30, Timothy Loh wrote: > I would create two DOMMatrix objects at the start (one 2D and one 3D, both with > garbage values), and then in each test initialise the local matrix to the type > it won't end up with. For example the first test makes a 2D matrix so initialise > it to the 3D matrix. > > Doing this will allow the test to catch more failure cases like not correctly > setting the is2D flag or not initialising the matrix values. i fixed it follow your comment. https://codereview.chromium.org/2380713004/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-setMatrixValue.html:157: }, "can't parse not exist keyword."); On 2016/10/24 04:18:30, Timothy Loh wrote: > I would write something like "CSS-wide keywords are disallowed". My intention in > suggesting to test "initial" is that in CSS, the keyword (along with several > others) is allowed as a value for any CSS property. setMatrixValue takes almost > the identical set of things as the transform property, but it excludes the > css-wide keywords. Done. https://codereview.chromium.org/2380713004/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-setMatrixValue.html:164: On 2016/10/24 04:18:30, Timothy Loh wrote: > unintentional blank line? no it is typo. https://codereview.chromium.org/2380713004/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-setMatrixValue.html:204: // TODO(hs1217.lee) : should be throw exception. On 2016/10/24 04:18:30, Timothy Loh wrote: > This case shouldn't throw an exception, right? So it should be another top level > test instead of being part of the exception tests. Yes, Your opinion is right, I'll move it. https://codereview.chromium.org/2380713004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/resolver/TransformBuilder.cpp (right): https://codereview.chromium.org/2380713004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/TransformBuilder.cpp:109: for (Member<const blink::CSSValue> item : *transformValue) { On 2016/10/24 04:18:30, Timothy Loh wrote: > I think you can write > > for (const CSSValue* item : Done.
The CQ bit was checked by timloh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
https://codereview.chromium.org/2380713004/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-setMatrixValue.html (right): https://codereview.chromium.org/2380713004/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-setMatrixValue.html:8: var matrix = new DOMMatrix(); On 2016/10/24 13:23:37, Hwanseung Lee wrote: > On 2016/10/24 04:18:30, Timothy Loh wrote: > > I would create two DOMMatrix objects at the start (one 2D and one 3D, both > with > > garbage values), and then in each test initialise the local matrix to the type > > it won't end up with. For example the first test makes a 2D matrix so > initialise > > it to the 3D matrix. > > > > Doing this will allow the test to catch more failure cases like not correctly > > setting the is2D flag or not initialising the matrix values. > > i fixed it follow your comment. Sorry if I wasn't too clear, what I meant was that we'd define these matrices outside of the test() calls and then use them for all the tests except the exception test. https://codereview.chromium.org/2380713004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/resolver/TransformBuilder.cpp (right): https://codereview.chromium.org/2380713004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/TransformBuilder.cpp:109: for (Member<const blink::CSSValue> item : *transformValue) { On 2016/10/24 13:23:37, Hwanseung Lee wrote: > On 2016/10/24 04:18:30, Timothy Loh wrote: > > I think you can write > > > > for (const CSSValue* item : > > Done. You don't need to write blink::, this is already in the blink namespace. https://codereview.chromium.org/2380713004/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-setMatrixValue.html (right): https://codereview.chromium.org/2380713004/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-setMatrixValue.html:28: assert_false(actualMatrix2.is2D); I would've expected that setting none makes the matrix become the identity matrix. Can we do that for now? I've filed a spec bug to clarify https://github.com/w3c/fxtf-drafts/issues/55 https://codereview.chromium.org/2380713004/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-setMatrixValue.html:174: var matrix = new DOMMatrix(); You should initialise the values in this matrix and then check at the end of this test that it hasn't been changed. https://codereview.chromium.org/2380713004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/DOMMatrix.cpp (right): https://codereview.chromium.org/2380713004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/DOMMatrix.cpp:255: ComputedStyle::clone(ComputedStyle::initialStyle())); We don't need to clone this at all, just use ComputedStyle::initialStyle() where you are referring to initialStyle. https://codereview.chromium.org/2380713004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/DOMMatrix.cpp:278: m_matrix = TransformationMatrix::create(); It's probably better to just call makeIdentity() instead of allocating a new TransformationMatrix.
@Timothy. PTAL. i did rebase this code from master. and i fixed some code. https://codereview.chromium.org/2380713004/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-setMatrixValue.html (right): https://codereview.chromium.org/2380713004/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-setMatrixValue.html:28: assert_false(actualMatrix2.is2D); On 2016/10/25 04:48:01, Timothy Loh wrote: > I would've expected that setting none makes the matrix become the identity > matrix. Can we do that for now? I've filed a spec bug to clarify > > https://github.com/w3c/fxtf-drafts/issues/55 i fixed a code. result matrix becoming the identity matrix when using 'none'. https://codereview.chromium.org/2380713004/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-setMatrixValue.html:174: var matrix = new DOMMatrix(); On 2016/10/25 04:48:01, Timothy Loh wrote: > You should initialise the values in this matrix and then check at the end of > this test that it hasn't been changed. Done. https://codereview.chromium.org/2380713004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/DOMMatrix.cpp (right): https://codereview.chromium.org/2380713004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/DOMMatrix.cpp:278: m_matrix = TransformationMatrix::create(); On 2016/10/25 04:48:01, Timothy Loh wrote: > It's probably better to just call makeIdentity() instead of allocating a new > TransformationMatrix. Done.
The CQ bit was checked by timloh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I think you missed some comments from my previous reply (they were attached to an earlier patchset).
On 2016/10/27 01:40:37, Timothy Loh wrote: > I think you missed some comments from my previous reply (they were attached to > an earlier patchset). i see. i will check it after leave work.
https://codereview.chromium.org/2380713004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/DOMMatrix.cpp (right): https://codereview.chromium.org/2380713004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/DOMMatrix.cpp:280: exceptionState.throwDOMException( Does this exception ever get thrown? From what I can tell parseSingleValue will either return nullptr, none, or a list. This entire if-else chain I think would be better as if (value->isIdentifierValue()) { DCHECK(toCSSIdentifierValue(value)->getValueID() == CSSValueNone); m_matrix->makeIdentity(); m_is2D = true; return this; } And below we don't need to check value->isValueList() because at that point we will always have a CSSValueList. In general I think it's useful to avoid having expressions which always evaluate to the same value (with the exception of in DCHECKs). These sorts of expressions lead to dead code or suggest that we can get into states which we actually can't get into.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
@Timothy PTAL. thank you. https://codereview.chromium.org/2380713004/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-setMatrixValue.html (right): https://codereview.chromium.org/2380713004/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-setMatrixValue.html:8: var matrix = new DOMMatrix(); On 2016/10/25 04:48:00, Timothy Loh wrote: > On 2016/10/24 13:23:37, Hwanseung Lee wrote: > > On 2016/10/24 04:18:30, Timothy Loh wrote: > > > I would create two DOMMatrix objects at the start (one 2D and one 3D, both > > with > > > garbage values), and then in each test initialise the local matrix to the > type > > > it won't end up with. For example the first test makes a 2D matrix so > > initialise > > > it to the 3D matrix. > > > > > > Doing this will allow the test to catch more failure cases like not > correctly > > > setting the is2D flag or not initialising the matrix values. > > > > i fixed it follow your comment. > > Sorry if I wasn't too clear, what I meant was that we'd define these matrices > outside of the test() calls and then use them for all the tests except the > exception test. actually i don't know your intent correctly. but i fixed a code. i made two matrix that was created in global scope. and it is using in each test function. could you check it? https://codereview.chromium.org/2380713004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/resolver/TransformBuilder.cpp (right): https://codereview.chromium.org/2380713004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/TransformBuilder.cpp:109: for (Member<const blink::CSSValue> item : *transformValue) { On 2016/10/25 04:48:01, Timothy Loh wrote: > On 2016/10/24 13:23:37, Hwanseung Lee wrote: > > On 2016/10/24 04:18:30, Timothy Loh wrote: > > > I think you can write > > > > > > for (const CSSValue* item : > > > > Done. > > You don't need to write blink::, this is already in the blink namespace. Done. https://codereview.chromium.org/2380713004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/DOMMatrix.cpp (right): https://codereview.chromium.org/2380713004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/DOMMatrix.cpp:255: ComputedStyle::clone(ComputedStyle::initialStyle())); On 2016/10/25 04:48:01, Timothy Loh wrote: > We don't need to clone this at all, just use ComputedStyle::initialStyle() where > you are referring to initialStyle. Done. https://codereview.chromium.org/2380713004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/DOMMatrix.cpp (right): https://codereview.chromium.org/2380713004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/DOMMatrix.cpp:280: exceptionState.throwDOMException( On 2016/10/27 02:38:42, Timothy Loh wrote: > Does this exception ever get thrown? From what I can tell parseSingleValue will > either return nullptr, none, or a list. This entire if-else chain I think would > be better as > > if (value->isIdentifierValue()) { > DCHECK(toCSSIdentifierValue(value)->getValueID() == CSSValueNone); > m_matrix->makeIdentity(); > m_is2D = true; > return this; > } > > And below we don't need to check value->isValueList() because at that point we > will always have a CSSValueList. > > In general I think it's useful to avoid having expressions which always evaluate > to the same value (with the exception of in DCHECKs). These sorts of expressions > lead to dead code or suggest that we can get into states which we actually can't > get into. when we don't check value->isValueList(), it will be make assert when using "initial" keyword. (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/css/resol...). so i did not remove else statement. did you have good idea? and i used && instead of DCHECK(). how do you think about it?
lgtm but please update the code/test based on my comments below. https://codereview.chromium.org/2380713004/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-setMatrixValue.html (right): https://codereview.chromium.org/2380713004/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-setMatrixValue.html:8: var matrix = new DOMMatrix(); On 2016/10/27 16:24:47, Hwanseung Lee wrote: > On 2016/10/25 04:48:00, Timothy Loh wrote: > > On 2016/10/24 13:23:37, Hwanseung Lee wrote: > > > On 2016/10/24 04:18:30, Timothy Loh wrote: > > > > I would create two DOMMatrix objects at the start (one 2D and one 3D, both > > > with > > > > garbage values), and then in each test initialise the local matrix to the > > type > > > > it won't end up with. For example the first test makes a 2D matrix so > > > initialise > > > > it to the 3D matrix. > > > > > > > > Doing this will allow the test to catch more failure cases like not > > correctly > > > > setting the is2D flag or not initialising the matrix values. > > > > > > i fixed it follow your comment. > > > > Sorry if I wasn't too clear, what I meant was that we'd define these matrices > > outside of the test() calls and then use them for all the tests except the > > exception test. > > > actually i don't know your intent correctly. > but i fixed a code. > i made two matrix that was created in global scope. > and it is using in each test function. could you check it? Something like this is fine, but you need to create new matrices in each test, e.g. var actualMatrix1 = new DOMMatrix(matrix2d); Otherwise you are modifying the global matrices. Compared to just using a new matrix, this test ensures we are correctly setting the is2D flag and operations which set the identity matrix work correctly. https://codereview.chromium.org/2380713004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/DOMMatrix.cpp (right): https://codereview.chromium.org/2380713004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/DOMMatrix.cpp:280: exceptionState.throwDOMException( On 2016/10/27 16:24:48, Hwanseung Lee wrote: > when we don't check value->isValueList(), it will be make assert when using > "initial" keyword. > (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/css/resol...). > so i did not remove else statement. did you have good idea? My point here is that in the code we should make it clear why and how we reach these error cases. Seeing "if (!value->isValueList())" makes me wonder, are we rejecting valid transform lists which somehow have a different type? So it'd be clearer if we could instead check "if (value->isCSSWideKeyword())". Actually it'd be better to just edit the above check "if (!value)" to be "if (!value || value->isCSSWideKeyword())" since we do the same thing in either case. > and i used && instead of DCHECK(). how do you think about it? I prefer having the DCHECK because it more explicitly indicates that the only CSSIdentifierValue we could have here is 'none'.
@timothy i fixed a some code follow your guide. thank you. https://codereview.chromium.org/2380713004/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-setMatrixValue.html (right): https://codereview.chromium.org/2380713004/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-setMatrixValue.html:8: var matrix = new DOMMatrix(); On 2016/10/28 02:50:04, Timothy Loh wrote: > On 2016/10/27 16:24:47, Hwanseung Lee wrote: > > On 2016/10/25 04:48:00, Timothy Loh wrote: > > > On 2016/10/24 13:23:37, Hwanseung Lee wrote: > > > > On 2016/10/24 04:18:30, Timothy Loh wrote: > > > > > I would create two DOMMatrix objects at the start (one 2D and one 3D, > both > > > > with > > > > > garbage values), and then in each test initialise the local matrix to > the > > > type > > > > > it won't end up with. For example the first test makes a 2D matrix so > > > > initialise > > > > > it to the 3D matrix. > > > > > > > > > > Doing this will allow the test to catch more failure cases like not > > > correctly > > > > > setting the is2D flag or not initialising the matrix values. > > > > > > > > i fixed it follow your comment. > > > > > > Sorry if I wasn't too clear, what I meant was that we'd define these > matrices > > > outside of the test() calls and then use them for all the tests except the > > > exception test. > > > > > > actually i don't know your intent correctly. > > but i fixed a code. > > i made two matrix that was created in global scope. > > and it is using in each test function. could you check it? > > Something like this is fine, but you need to create new matrices in each test, > e.g. > > var actualMatrix1 = new DOMMatrix(matrix2d); > > Otherwise you are modifying the global matrices. Compared to just using a new > matrix, this test ensures we are correctly setting the is2D flag and operations > which set the identity matrix work correctly. Done. https://codereview.chromium.org/2380713004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/DOMMatrix.cpp (right): https://codereview.chromium.org/2380713004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/DOMMatrix.cpp:280: exceptionState.throwDOMException( On 2016/10/28 02:50:04, Timothy Loh wrote: > On 2016/10/27 16:24:48, Hwanseung Lee wrote: > > when we don't check value->isValueList(), it will be make assert when using > > "initial" keyword. > > > (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/css/resol...). > > so i did not remove else statement. did you have good idea? > > My point here is that in the code we should make it clear why and how we reach > these error cases. Seeing "if (!value->isValueList())" makes me wonder, are we > rejecting valid transform lists which somehow have a different type? So it'd be > clearer if we could instead check "if (value->isCSSWideKeyword())". Actually > it'd be better to just edit the above check "if (!value)" to be "if (!value || > value->isCSSWideKeyword())" since we do the same thing in either case. > > > and i used && instead of DCHECK(). how do you think about it? > > I prefer having the DCHECK because it more explicitly indicates that the only > CSSIdentifierValue we could have here is 'none'. Done.
The CQ bit was checked by hs1217.lee@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from meade@chromium.org, timloh@chromium.org Link to the patchset: https://codereview.chromium.org/2380713004/#ps200001 (title: "[GeometryInterface] Add setMatrixValue(transfromList) function.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [GeometryInterface] Add setMatrixValue(transfromList) function. 1. if transformList is the empty string, set in to the string "matrix(1, 0, 0, 1, 0, 0)". 2. Parse transformList by following the syntax description[1]. if parsing failed, throw SyntaxError. 3. Set is2D to false if the <transform-list> consists of any 3D Transform functions. Otherwise set is2D to true. 4. Transform all <transform-function>s to 4x4 matrices. 5. Post-multiply all matrices from left to right. 6. return the current matrix. spec list: https://drafts.fxtf.org/geometry/#dom-dommatrix-setmatrixvalue [1] = https://drafts.csswg.org/css-transforms-1/#svg-syntax BUG=388780, 645884 ========== to ========== [GeometryInterface] Add setMatrixValue(transfromList) function. 1. if transformList is the empty string, set in to the string "matrix(1, 0, 0, 1, 0, 0)". 2. Parse transformList by following the syntax description[1]. if parsing failed, throw SyntaxError. 3. Set is2D to false if the <transform-list> consists of any 3D Transform functions. Otherwise set is2D to true. 4. Transform all <transform-function>s to 4x4 matrices. 5. Post-multiply all matrices from left to right. 6. return the current matrix. spec list: https://drafts.fxtf.org/geometry/#dom-dommatrix-setmatrixvalue [1] = https://drafts.csswg.org/css-transforms-1/#svg-syntax BUG=388780, 645884 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== [GeometryInterface] Add setMatrixValue(transfromList) function. 1. if transformList is the empty string, set in to the string "matrix(1, 0, 0, 1, 0, 0)". 2. Parse transformList by following the syntax description[1]. if parsing failed, throw SyntaxError. 3. Set is2D to false if the <transform-list> consists of any 3D Transform functions. Otherwise set is2D to true. 4. Transform all <transform-function>s to 4x4 matrices. 5. Post-multiply all matrices from left to right. 6. return the current matrix. spec list: https://drafts.fxtf.org/geometry/#dom-dommatrix-setmatrixvalue [1] = https://drafts.csswg.org/css-transforms-1/#svg-syntax BUG=388780, 645884 ========== to ========== [GeometryInterface] Add setMatrixValue(transfromList) function. 1. if transformList is the empty string, set in to the string "matrix(1, 0, 0, 1, 0, 0)". 2. Parse transformList by following the syntax description[1]. if parsing failed, throw SyntaxError. 3. Set is2D to false if the <transform-list> consists of any 3D Transform functions. Otherwise set is2D to true. 4. Transform all <transform-function>s to 4x4 matrices. 5. Post-multiply all matrices from left to right. 6. return the current matrix. spec list: https://drafts.fxtf.org/geometry/#dom-dommatrix-setmatrixvalue [1] = https://drafts.csswg.org/css-transforms-1/#svg-syntax BUG=388780, 645884 Committed: https://crrev.com/1b84ec0976538570d0407d95677038679225dd0e Cr-Commit-Position: refs/heads/master@{#428352} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/1b84ec0976538570d0407d95677038679225dd0e Cr-Commit-Position: refs/heads/master@{#428352} |
