Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2271223002:
Convert LayoutTests/svg/dom/SVGLength*.html js-tests.js tests to testharness.js based tests. (Closed)
|
Created:
4 years, 4 months ago by Shanmuga Pandi Modified:
4 years, 3 months ago CC:
blink-reviews, chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionConvert LayoutTests/svg/dom/SVGLength*.html js-tests.js tests to testharness.js based tests.
BUG=636710
Committed: https://crrev.com/7d9f4bebeb65115c81918032278174ad01858d33
Cr-Commit-Position: refs/heads/master@{#415965}
Patch Set 1 #
Total comments: 13
Patch Set 2 : Align with review comments #
Total comments: 39
Patch Set 3 : created subtests #
Total comments: 14
Patch Set 4 : Added testharness based setup() #
Total comments: 7
Patch Set 5 : Aligned with review comments #Patch Set 6 : moved length to local #Messages
Total messages: 28 (6 generated)
shanmuga.m@samsung.com changed reviewers: + srirama.m@samsung.com
PTAL!
https://codereview.chromium.org/2271223002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/svg/dom/SVGLength-px-with-context.html (right): https://codereview.chromium.org/2271223002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/svg/dom/SVGLength-px-with-context.html:8: { nit: move it to previous line. https://codereview.chromium.org/2271223002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/svg/dom/SVGLength-px-with-context.html:10: var divElement = document.createElement("div"); same comments as in previous test to use queryselector. https://codereview.chromium.org/2271223002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/svg/dom/SVGLength-px-with-context.html:27: { nit:move it to previous line. https://codereview.chromium.org/2271223002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/svg/dom/SVGLength-px-with-context.html:31: document.getElementById("description").appendChild(divElement); same here as well. https://codereview.chromium.org/2271223002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/svg/dom/SVGLength-px.html (right): https://codereview.chromium.org/2271223002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/svg/dom/SVGLength-px.html:8: { nit: move it to previous line. https://codereview.chromium.org/2271223002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/svg/dom/SVGLength-px.html:12: document.getElementById("description").appendChild(divElement); you can directly use querySelector("p") and remove id attribute. Also store the element and use it below also. https://codereview.chromium.org/2271223002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/svg/dom/SVGLength.html (right): https://codereview.chromium.org/2271223002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/svg/dom/SVGLength.html:30: assert_throws(null, function() { length.convertToSpecifiedUnits(); }); can you check for TypeError instead of null as in the original test, here and few more places?
PTAL!! https://codereview.chromium.org/2271223002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/svg/dom/SVGLength-px-with-context.html (right): https://codereview.chromium.org/2271223002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/svg/dom/SVGLength-px-with-context.html:8: { On 2016/08/25 05:18:51, Srirama wrote: > nit: move it to previous line. Done. https://codereview.chromium.org/2271223002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/svg/dom/SVGLength-px-with-context.html:10: var divElement = document.createElement("div"); On 2016/08/25 05:18:51, Srirama wrote: > same comments as in previous test to use queryselector. Done. https://codereview.chromium.org/2271223002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/svg/dom/SVGLength-px-with-context.html:27: { On 2016/08/25 05:18:51, Srirama wrote: > nit:move it to previous line. Done. https://codereview.chromium.org/2271223002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/svg/dom/SVGLength-px-with-context.html:31: document.getElementById("description").appendChild(divElement); On 2016/08/25 05:18:51, Srirama wrote: > same here as well. Done. https://codereview.chromium.org/2271223002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/svg/dom/SVGLength-px.html (right): https://codereview.chromium.org/2271223002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/svg/dom/SVGLength-px.html:12: document.getElementById("description").appendChild(divElement); On 2016/08/25 05:18:51, Srirama wrote: > you can directly use querySelector("p") and remove id attribute. Also store the > element and use it below also. Done. https://codereview.chromium.org/2271223002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/svg/dom/SVGLength.html (right): https://codereview.chromium.org/2271223002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/svg/dom/SVGLength.html:30: assert_throws(null, function() { length.convertToSpecifiedUnits(); }); On 2016/08/25 05:18:52, Srirama wrote: > can you check for TypeError instead of null as in the original test, here and > few more places? Done.
lgtm
shanmuga.m@samsung.com changed reviewers: + foolip@chromium.org, fs@opera.com
PTAL!!
https://codereview.chromium.org/2271223002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/svg/dom/SVGLength-px-with-context.html (right): https://codereview.chromium.org/2271223002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength-px-with-context.html:2: <title>Checks SVGLength - converting from px to all other unit types</title> Like for the other -px test, but change 'detached' to 'attached' or 'in document' or something like that. https://codereview.chromium.org/2271223002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength-px-with-context.html:7: function calculateDPI() { Shouldn't need this. https://codereview.chromium.org/2271223002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength-px-with-context.html:37: test(function() { Should be possible to split this into several (sub)tests like the other -px test https://codereview.chromium.org/2271223002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/svg/dom/SVGLength-px.html (right): https://codereview.chromium.org/2271223002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength-px.html:2: <title>Checks SVGLength - converting from px to all other unit types</title> 'SVGLength, converting from 'px' to other units (detached)' https://codereview.chromium.org/2271223002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength-px.html:5: <p></p> Per below we don't need this. https://codereview.chromium.org/2271223002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength-px.html:8: // Crude hack to determine the DPI, instead of hardcoding our 96 dpi here. Actually, I think hardcoding 96 is exactly what we want to do... so this function can be dropped. https://codereview.chromium.org/2271223002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength-px.html:26: test(function() { It'd probably make sense to split this into several tests (one per convertToSpecifiedUnits essentially), and then move the svgElement setup to a setup(). (And create one new SVGLength for each test.) https://codereview.chromium.org/2271223002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength-px.html:49: assert_throws(null, function() { length.convertToSpecifiedUnits(SVGLength.SVG_LENGTHTYPE_PERCENTAGE); }); NotSupportedError https://codereview.chromium.org/2271223002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength-px.html:59: assert_throws(null, function() { length.convertToSpecifiedUnits(SVGLength.SVG_LENGTHTYPE_EMS); }); NotSupportedError https://codereview.chromium.org/2271223002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength-px.html:69: assert_throws(null, function() { length.convertToSpecifiedUnits(SVGLength.SVG_LENGTHTYPE_EXS); }); NotSupportedError https://codereview.chromium.org/2271223002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/svg/dom/SVGLength.html (right): https://codereview.chromium.org/2271223002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength.html:2: <title>Checks the SVGLength API</title> Make this just 'SVGLength interface'. https://codereview.chromium.org/2271223002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength.html:10: // Check initial length values End sentences with full stop ("."). https://codereview.chromium.org/2271223002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength.html:24: assert_throws(null, function() { length.convertToSpecifiedUnits(SVGLength.SVG_LENGTHTYPE_UNKNOWN); }); 'NotSupportedError'? (same for the following five) https://codereview.chromium.org/2271223002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength.html:34: assert_equals(length.unitType, SVGLength.SVG_LENGTHTYPE_PX); You can drop one of these two asserts of unitType. (Or replace with a valueAsString assert.) https://codereview.chromium.org/2271223002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength.html:37: assert_throws(null, function() { length.newValueSpecifiedUnits(SVGLength.SVG_LENGTHTYPE_UNKNOWN, 4); }); 'NotSupportedError' (+two more) https://codereview.chromium.org/2271223002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength.html:55: assert_throws(null, function() { length.newValueSpecifiedUnits('aString', 4); }); NotSupportedError (+2) https://codereview.chromium.org/2271223002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength.html:64: assert_equals(length.unitType, SVGLength.SVG_LENGTHTYPE_PX); Drop one of the unitType asserts. (Or replace with a valueAsString assert.) https://codereview.chromium.org/2271223002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength.html:67: assert_throws(null, function() { length.valueAsString = '10deg'; }); SyntaxError https://codereview.chromium.org/2271223002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength.html:81: assert_throws(null, function() { length.valueAsString = ',5 em'; }); SyntaxError https://codereview.chromium.org/2271223002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength.html:87: assert_throws(null, function() { length.valueAsString = null; }); SyntaxError https://codereview.chromium.org/2271223002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength.html:108: Drop blank line.
https://codereview.chromium.org/2271223002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/svg/dom/SVGLength-px-with-context.html (right): https://codereview.chromium.org/2271223002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength-px-with-context.html:7: function calculateDPI() { On 2016/08/25 10:16:51, fs wrote: > Shouldn't need this. Observed strange issue if we remove calculateDPI() and replaced with cssPixelsPerInch = 96; , especially var cssPixelsPerInch = divElement.offsetHeight; Tests related percentage and ems, exs are failing if we remove that. Looks like "divElement.offsetHeight" is triggering updateStyle and layout and bcz of that it works earlier.
https://codereview.chromium.org/2271223002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/svg/dom/SVGLength-px-with-context.html (right): https://codereview.chromium.org/2271223002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength-px-with-context.html:7: function calculateDPI() { On 2016/08/25 at 14:02:54, Shanmuga Pandi wrote: > On 2016/08/25 10:16:51, fs wrote: > > Shouldn't need this. > > Observed strange issue if we remove calculateDPI() and replaced with cssPixelsPerInch = 96; > , especially var cssPixelsPerInch = divElement.offsetHeight; > > Tests related percentage and ems, exs are failing if we remove that. > Looks like "divElement.offsetHeight" is triggering updateStyle and layout and bcz of that it works earlier. Yes, that may very well be the case. SVGLengthTearOff likely needs to make sure layout is up-to-date for the context element before performing the computation.
PTAL!! https://codereview.chromium.org/2271223002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/svg/dom/SVGLength-px-with-context.html (right): https://codereview.chromium.org/2271223002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength-px-with-context.html:7: function calculateDPI() { On 2016/08/25 14:12:27, fs wrote: > On 2016/08/25 at 14:02:54, Shanmuga Pandi wrote: > > On 2016/08/25 10:16:51, fs wrote: > > > Shouldn't need this. > > > > Observed strange issue if we remove calculateDPI() and replaced with > cssPixelsPerInch = 96; > > , especially var cssPixelsPerInch = divElement.offsetHeight; > > > > Tests related percentage and ems, exs are failing if we remove that. > > Looks like "divElement.offsetHeight" is triggering updateStyle and layout and > bcz of that it works earlier. > > Yes, that may very well be the case. SVGLengthTearOff likely needs to make sure > layout is up-to-date for the context element before performing the computation. Right. I added TODO. I will try to make a separate patch for that. https://codereview.chromium.org/2271223002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength-px-with-context.html:37: test(function() { On 2016/08/25 10:16:51, fs wrote: > Should be possible to split this into several (sub)tests like the other -px test Done. https://codereview.chromium.org/2271223002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/svg/dom/SVGLength-px.html (right): https://codereview.chromium.org/2271223002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength-px.html:2: <title>Checks SVGLength - converting from px to all other unit types</title> On 2016/08/25 10:16:51, fs wrote: > 'SVGLength, converting from 'px' to other units (detached)' Done. https://codereview.chromium.org/2271223002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength-px.html:5: <p></p> On 2016/08/25 10:16:51, fs wrote: > Per below we don't need this. Done. https://codereview.chromium.org/2271223002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength-px.html:8: // Crude hack to determine the DPI, instead of hardcoding our 96 dpi here. On 2016/08/25 10:16:51, fs wrote: > Actually, I think hardcoding 96 is exactly what we want to do... so this > function can be dropped. Done. https://codereview.chromium.org/2271223002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength-px.html:26: test(function() { On 2016/08/25 10:16:51, fs wrote: > It'd probably make sense to split this into several tests (one per > convertToSpecifiedUnits essentially), and then move the svgElement setup to a > setup(). (And create one new SVGLength for each test.) Done. https://codereview.chromium.org/2271223002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength-px.html:49: assert_throws(null, function() { length.convertToSpecifiedUnits(SVGLength.SVG_LENGTHTYPE_PERCENTAGE); }); On 2016/08/25 10:16:51, fs wrote: > NotSupportedError Done. https://codereview.chromium.org/2271223002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/svg/dom/SVGLength.html (right): https://codereview.chromium.org/2271223002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength.html:2: <title>Checks the SVGLength API</title> On 2016/08/25 10:16:52, fs wrote: > Make this just 'SVGLength interface'. Done. https://codereview.chromium.org/2271223002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength.html:10: // Check initial length values On 2016/08/25 10:16:52, fs wrote: > End sentences with full stop ("."). Done. https://codereview.chromium.org/2271223002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength.html:24: assert_throws(null, function() { length.convertToSpecifiedUnits(SVGLength.SVG_LENGTHTYPE_UNKNOWN); }); On 2016/08/25 10:16:52, fs wrote: > 'NotSupportedError'? (same for the following five) Done. https://codereview.chromium.org/2271223002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength.html:34: assert_equals(length.unitType, SVGLength.SVG_LENGTHTYPE_PX); On 2016/08/25 10:16:52, fs wrote: > You can drop one of these two asserts of unitType. (Or replace with a > valueAsString assert.) Done. https://codereview.chromium.org/2271223002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength.html:37: assert_throws(null, function() { length.newValueSpecifiedUnits(SVGLength.SVG_LENGTHTYPE_UNKNOWN, 4); }); On 2016/08/25 10:16:52, fs wrote: > 'NotSupportedError' (+two more) Done. https://codereview.chromium.org/2271223002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength.html:55: assert_throws(null, function() { length.newValueSpecifiedUnits('aString', 4); }); On 2016/08/25 10:16:52, fs wrote: > NotSupportedError (+2) Done. https://codereview.chromium.org/2271223002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength.html:64: assert_equals(length.unitType, SVGLength.SVG_LENGTHTYPE_PX); On 2016/08/25 10:16:52, fs wrote: > Drop one of the unitType asserts. (Or replace with a valueAsString assert.) Done. https://codereview.chromium.org/2271223002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength.html:67: assert_throws(null, function() { length.valueAsString = '10deg'; }); On 2016/08/25 10:16:52, fs wrote: > SyntaxError Done. https://codereview.chromium.org/2271223002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength.html:108: On 2016/08/25 10:16:52, fs wrote: > Drop blank line. Done.
https://codereview.chromium.org/2271223002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/svg/dom/SVGLength-px-with-context.html (right): https://codereview.chromium.org/2271223002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength-px-with-context.html:23: // TODO(shanmuga.m) : Remove calculateDPI() hack and use hardcoded value 96. Instead of this, could you just add something that forces a layout, and then stick the TODO on that? https://codereview.chromium.org/2271223002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength-px-with-context.html:48: } Everything above here could go in a (testharness) setup(). https://codereview.chromium.org/2271223002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength-px-with-context.html:57: }, "Check initial value to be 2px"); Same naming strategy here. Also, this particular test doesn't seem very useful (we have this covered I'm fairly sure.) https://codereview.chromium.org/2271223002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/svg/dom/SVGLength-px.html (right): https://codereview.chromium.org/2271223002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength-px.html:9: function setup() { Use testharness setup(): setup(function() { window.svgElement = document.createElementNS(...); }); also, don't create a new 'length' instance for each test and keep that local. If a helper is useful to create a "well-defined" SVGLength then feel free to use that. https://codereview.chromium.org/2271223002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength-px.html:21: }, "Check initial value to be 2px"); I'd suggest the following format for testnames: document.title + ', <something>' So "document.title + ', initial value'" here and "document.title + ', unitless'" for the next one et.c I don't this first test is particularly useful though, so it could just as well be dropped.
Patchset #4 (id:60001) has been deleted
PTAL! https://codereview.chromium.org/2271223002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/svg/dom/SVGLength-px-with-context.html (right): https://codereview.chromium.org/2271223002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength-px-with-context.html:23: // TODO(shanmuga.m) : Remove calculateDPI() hack and use hardcoded value 96. On 2016/08/26 09:03:06, fs wrote: > Instead of this, could you just add something that forces a layout, and then > stick the TODO on that? Done. https://codereview.chromium.org/2271223002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/svg/dom/SVGLength-px.html (right): https://codereview.chromium.org/2271223002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength-px.html:9: function setup() { On 2016/08/26 09:03:07, fs wrote: > Use testharness setup(): > > setup(function() { > window.svgElement = document.createElementNS(...); > }); > > also, don't create a new 'length' instance for each test and keep that local. If > a helper is useful to create a "well-defined" SVGLength then feel free to use > that. Done. https://codereview.chromium.org/2271223002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength-px.html:21: }, "Check initial value to be 2px"); On 2016/08/26 09:03:07, fs wrote: > I'd suggest the following format for testnames: > > document.title + ', <something>' > > So "document.title + ', initial value'" here and "document.title + ', unitless'" > for the next one et.c > > I don't this first test is particularly useful though, so it could just as well > be dropped. Done.
https://codereview.chromium.org/2271223002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/svg/dom/SVGLength-px.html (right): https://codereview.chromium.org/2271223002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength-px.html:9: function setup() { On 2016/09/01 at 11:59:56, Shanmuga Pandi wrote: > On 2016/08/26 09:03:07, fs wrote: > > Use testharness setup(): > > > > setup(function() { > > window.svgElement = document.createElementNS(...); > > }); > > > > also, don't create a new 'length' instance for each test and keep that local. If > > a helper is useful to create a "well-defined" SVGLength then feel free to use > > that. > > Done. Doesn't look done to me. Having as clean a slate as possible for each test is an advantage - the fact that we need an SVGSVGElement as a "factory" is the only "global" thing we need to deal with here. https://codereview.chromium.org/2271223002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/svg/dom/SVGLength-px-with-context.html (right): https://codereview.chromium.org/2271223002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength-px-with-context.html:52: referenceValue = Number(2 / svgWidth * 100).toFixed(5); var referenceValue https://codereview.chromium.org/2271223002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength-px-with-context.html:62: referenceValue = Number(2 / fontSize).toFixed(6); Ditto. https://codereview.chromium.org/2271223002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength-px-with-context.html:72: referenceValue = Number(2 / xHeight).toFixed(1); Ditto. https://codereview.chromium.org/2271223002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength-px-with-context.html:82: referenceValue = Number(2 * 2.54 / cssPixelsPerInch).toFixed(7); Ditto. https://codereview.chromium.org/2271223002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength-px-with-context.html:92: referenceValue = Number(2 * 25.4 / cssPixelsPerInch).toFixed(6); Ditto. https://codereview.chromium.org/2271223002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength-px-with-context.html:102: referenceValue = Number(2 / cssPixelsPerInch).toFixed(7); Etc...
Please take a look.. Thank you for reviewing... https://codereview.chromium.org/2271223002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/svg/dom/SVGLength-px.html (right): https://codereview.chromium.org/2271223002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength-px.html:9: function setup() { On 2016/09/01 12:31:12, fs wrote: > On 2016/09/01 at 11:59:56, Shanmuga Pandi wrote: > > On 2016/08/26 09:03:07, fs wrote: > > > Use testharness setup(): > > > > > > setup(function() { > > > window.svgElement = document.createElementNS(...); > > > }); > > > > > > also, don't create a new 'length' instance for each test and keep that > local. If > > > a helper is useful to create a "well-defined" SVGLength then feel free to > use > > > that. > > > > Done. > > Doesn't look done to me. Having as clean a slate as possible for each test is an > advantage - the fact that we need an SVGSVGElement as a "factory" is the only > "global" thing we need to deal with here. Could you please check this now.? https://codereview.chromium.org/2271223002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/svg/dom/SVGLength-px-with-context.html (right): https://codereview.chromium.org/2271223002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength-px-with-context.html:52: referenceValue = Number(2 / svgWidth * 100).toFixed(5); On 2016/09/01 12:31:12, fs wrote: > var referenceValue Done.
https://codereview.chromium.org/2271223002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/svg/dom/SVGLength-px.html (right): https://codereview.chromium.org/2271223002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength-px.html:9: function setup() { On 2016/09/01 at 13:18:17, Shanmuga Pandi wrote: > On 2016/09/01 12:31:12, fs wrote: > > On 2016/09/01 at 11:59:56, Shanmuga Pandi wrote: > > > On 2016/08/26 09:03:07, fs wrote: > > > > Use testharness setup(): > > > > > > > > setup(function() { > > > > window.svgElement = document.createElementNS(...); > > > > }); > > > > > > > > also, don't create a new 'length' instance for each test and keep that > > local. If > > > > a helper is useful to create a "well-defined" SVGLength then feel free to > > use > > > > that. > > > > > > Done. > > > > Doesn't look done to me. Having as clean a slate as possible for each test is an > > advantage - the fact that we need an SVGSVGElement as a "factory" is the only > > "global" thing we need to deal with here. > > Could you please check this now.? Still the same, did you forget to upload? What I mean would be: setup(... { window.svgElement = ...; }); test(... { var length = svgElement.createSVGLength(); ... }, ...);
https://codereview.chromium.org/2271223002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/svg/dom/SVGLength-px.html (right): https://codereview.chromium.org/2271223002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength-px.html:9: function setup() { On 2016/09/01 13:22:54, fs wrote: > On 2016/09/01 at 13:18:17, Shanmuga Pandi wrote: > > On 2016/09/01 12:31:12, fs wrote: > > > On 2016/09/01 at 11:59:56, Shanmuga Pandi wrote: > > > > On 2016/08/26 09:03:07, fs wrote: > > > > > Use testharness setup(): > > > > > > > > > > setup(function() { > > > > > window.svgElement = document.createElementNS(...); > > > > > }); > > > > > > > > > > also, don't create a new 'length' instance for each test and keep that > > > local. If > > > > > a helper is useful to create a "well-defined" SVGLength then feel free > to > > > use > > > > > that. > > > > > > > > Done. > > > > > > Doesn't look done to me. Having as clean a slate as possible for each test > is an > > > advantage - the fact that we need an SVGSVGElement as a "factory" is the > only > > > "global" thing we need to deal with here. > > > > Could you please check this now.? > > Still the same, did you forget to upload? What I mean would be: > > setup(... { > window.svgElement = ...; > }); > > test(... { > var length = svgElement.createSVGLength(); > ... > }, ...); Ok. There was some misunderstanding. I will upload it again. Should I modify the SVGLength-px-context.html as well like this. ?
https://codereview.chromium.org/2271223002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/svg/dom/SVGLength-px.html (right): https://codereview.chromium.org/2271223002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength-px.html:9: function setup() { On 2016/09/01 14:00:36, Shanmuga Pandi wrote: > On 2016/09/01 13:22:54, fs wrote: > > On 2016/09/01 at 13:18:17, Shanmuga Pandi wrote: > > > On 2016/09/01 12:31:12, fs wrote: > > > > On 2016/09/01 at 11:59:56, Shanmuga Pandi wrote: > > > > > On 2016/08/26 09:03:07, fs wrote: > > > > > > Use testharness setup(): > > > > > > > > > > > > setup(function() { > > > > > > window.svgElement = document.createElementNS(...); > > > > > > }); > > > > > > > > > > > > also, don't create a new 'length' instance for each test and keep that > > > > local. If > > > > > > a helper is useful to create a "well-defined" SVGLength then feel free > > to > > > > use > > > > > > that. > > > > > > > > > > Done. > > > > > > > > Doesn't look done to me. Having as clean a slate as possible for each test > > is an > > > > advantage - the fact that we need an SVGSVGElement as a "factory" is the > > only > > > > "global" thing we need to deal with here. > > > > > > Could you please check this now.? > > > > Still the same, did you forget to upload? What I mean would be: > > > > setup(... { > > window.svgElement = ...; > > }); > > > > test(... { > > var length = svgElement.createSVGLength(); > > ... > > }, ...); > Done. Please check now. Thanks
lgtm https://codereview.chromium.org/2271223002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/svg/dom/SVGLength-px.html (right): https://codereview.chromium.org/2271223002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength-px.html:9: function setup() { On 2016/09/01 at 14:00:36, Shanmuga Pandi wrote: > On 2016/09/01 13:22:54, fs wrote: > > On 2016/09/01 at 13:18:17, Shanmuga Pandi wrote: > > > On 2016/09/01 12:31:12, fs wrote: > > > > On 2016/09/01 at 11:59:56, Shanmuga Pandi wrote: > > > > > On 2016/08/26 09:03:07, fs wrote: > > > > > > Use testharness setup(): > > > > > > > > > > > > setup(function() { > > > > > > window.svgElement = document.createElementNS(...); > > > > > > }); > > > > > > > > > > > > also, don't create a new 'length' instance for each test and keep that > > > > local. If > > > > > > a helper is useful to create a "well-defined" SVGLength then feel free > > to > > > > use > > > > > > that. > > > > > > > > > > Done. > > > > > > > > Doesn't look done to me. Having as clean a slate as possible for each test > > is an > > > > advantage - the fact that we need an SVGSVGElement as a "factory" is the > > only > > > > "global" thing we need to deal with here. > > > > > > Could you please check this now.? > > > > Still the same, did you forget to upload? What I mean would be: > > > > setup(... { > > window.svgElement = ...; > > }); > > > > test(... { > > var length = svgElement.createSVGLength(); > > ... > > }, ...); > > Ok. There was some misunderstanding. I will upload it again. > Should I modify the SVGLength-px-context.html as well like this. ? No, that one is fine as is because it needs to be associated with an element.
On 2016/09/01 14:14:58, fs wrote: > lgtm > > https://codereview.chromium.org/2271223002/diff/40001/third_party/WebKit/Layo... > File third_party/WebKit/LayoutTests/svg/dom/SVGLength-px.html (right): > > https://codereview.chromium.org/2271223002/diff/40001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/svg/dom/SVGLength-px.html:9: function setup() { > On 2016/09/01 at 14:00:36, Shanmuga Pandi wrote: > > On 2016/09/01 13:22:54, fs wrote: > > > On 2016/09/01 at 13:18:17, Shanmuga Pandi wrote: > > > > On 2016/09/01 12:31:12, fs wrote: > > > > > On 2016/09/01 at 11:59:56, Shanmuga Pandi wrote: > > > > > > On 2016/08/26 09:03:07, fs wrote: > > > > > > > Use testharness setup(): > > > > > > > > > > > > > > setup(function() { > > > > > > > window.svgElement = document.createElementNS(...); > > > > > > > }); > > > > > > > > > > > > > > also, don't create a new 'length' instance for each test and keep > that > > > > > local. If > > > > > > > a helper is useful to create a "well-defined" SVGLength then feel > free > > > to > > > > > use > > > > > > > that. > > > > > > > > > > > > Done. > > > > > > > > > > Doesn't look done to me. Having as clean a slate as possible for each > test > > > is an > > > > > advantage - the fact that we need an SVGSVGElement as a "factory" is the > > > only > > > > > "global" thing we need to deal with here. > > > > > > > > Could you please check this now.? > > > > > > Still the same, did you forget to upload? What I mean would be: > > > > > > setup(... { > > > window.svgElement = ...; > > > }); > > > > > > test(... { > > > var length = svgElement.createSVGLength(); > > > ... > > > }, ...); > > > > Ok. There was some misunderstanding. I will upload it again. > > Should I modify the SVGLength-px-context.html as well like this. ? > > No, that one is fine as is because it needs to be associated with an element. Thank you for reviewing and your patience :)
The CQ bit was checked by shanmuga.m@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from srirama.m@samsung.com Link to the patchset: https://codereview.chromium.org/2271223002/#ps120001 (title: "moved length to local")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/09/01 at 14:31:59, shanmuga.m wrote: > On 2016/09/01 14:14:58, fs wrote: > > lgtm > > > > https://codereview.chromium.org/2271223002/diff/40001/third_party/WebKit/Layo... > > File third_party/WebKit/LayoutTests/svg/dom/SVGLength-px.html (right): > > > > https://codereview.chromium.org/2271223002/diff/40001/third_party/WebKit/Layo... > > third_party/WebKit/LayoutTests/svg/dom/SVGLength-px.html:9: function setup() { > > On 2016/09/01 at 14:00:36, Shanmuga Pandi wrote: > > > On 2016/09/01 13:22:54, fs wrote: > > > > On 2016/09/01 at 13:18:17, Shanmuga Pandi wrote: > > > > > On 2016/09/01 12:31:12, fs wrote: > > > > > > On 2016/09/01 at 11:59:56, Shanmuga Pandi wrote: > > > > > > > On 2016/08/26 09:03:07, fs wrote: > > > > > > > > Use testharness setup(): > > > > > > > > > > > > > > > > setup(function() { > > > > > > > > window.svgElement = document.createElementNS(...); > > > > > > > > }); > > > > > > > > > > > > > > > > also, don't create a new 'length' instance for each test and keep > > that > > > > > > local. If > > > > > > > > a helper is useful to create a "well-defined" SVGLength then feel > > free > > > > to > > > > > > use > > > > > > > > that. > > > > > > > > > > > > > > Done. > > > > > > > > > > > > Doesn't look done to me. Having as clean a slate as possible for each > > test > > > > is an > > > > > > advantage - the fact that we need an SVGSVGElement as a "factory" is the > > > > only > > > > > > "global" thing we need to deal with here. > > > > > > > > > > Could you please check this now.? > > > > > > > > Still the same, did you forget to upload? What I mean would be: > > > > > > > > setup(... { > > > > window.svgElement = ...; > > > > }); > > > > > > > > test(... { > > > > var length = svgElement.createSVGLength(); > > > > ... > > > > }, ...); > > > > > > Ok. There was some misunderstanding. I will upload it again. > > > Should I modify the SVGLength-px-context.html as well like this. ? > > > > No, that one is fine as is because it needs to be associated with an element. > > Thank you for reviewing and your patience :) You're most welcome =)
Message was sent while issue was closed.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Convert LayoutTests/svg/dom/SVGLength*.html js-tests.js tests to testharness.js based tests. BUG=636710 ========== to ========== Convert LayoutTests/svg/dom/SVGLength*.html js-tests.js tests to testharness.js based tests. BUG=636710 Committed: https://crrev.com/7d9f4bebeb65115c81918032278174ad01858d33 Cr-Commit-Position: refs/heads/master@{#415965} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/7d9f4bebeb65115c81918032278174ad01858d33 Cr-Commit-Position: refs/heads/master@{#415965} |
