Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2260773002:
Convert image size and zoom level test to testharness.js (Closed)
DescriptionConvert image size and zoom level test to testharness.js
1.Remove zoomed-offset-size-expected.txt.
2.Simply code to test whether image size is changed when
zoom level is altered.
BUG=637859
Committed: https://crrev.com/0fd18b4775d4038af7512dfc72b2bc17425faabb
Cr-Commit-Position: refs/heads/master@{#413146}
Patch Set 1 #Patch Set 2 : Convert getSize to testSize. #
Total comments: 7
Patch Set 3 : Changed code as per review comments. #Patch Set 4 : Add new test expectations. #
Messages
Total messages: 17 (5 generated)
Description was changed from ========== Convert image size and zoom level test to testharness.js 1.Remove zoomed-offset-size-expected.txt. 2.Simply code to test whether image size is changed when zoom level is altered. BUG=637859 ========== to ========== Convert image size and zoom level test to testharness.js 1.Remove zoomed-offset-size-expected.txt. 2.Simply code to test whether image size is changed when zoom level is altered. BUG=637859 ==========
siva.gunturi@samsung.com changed reviewers: + fs@opera.com, srirama.m@samsung.com
@fs, srirama ptal..
https://codereview.chromium.org/2260773002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/images/zoomed-offset-size.html (right): https://codereview.chromium.org/2260773002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/images/zoomed-offset-size.html:9: document.body.zoom = 1.0; I think the intention of this was to reset the zoom after the test had completed, which is why it was set as the last thing. (The zoom should be 1 initially already, you could assert that if you like.) See below though. https://codereview.chromium.org/2260773002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/images/zoomed-offset-size.html:13: document.body.zoom = zoomLevel; I'm not aware of any 'zoom' property on HTMLBodyElement - so this does look like it does what it set out to? Maybe this should set the 'zoom' CSS property as inline style on <body>? https://codereview.chromium.org/2260773002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/images/zoomed-offset-size.html:21: } You can drop the {}
https://codereview.chromium.org/2260773002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/images/zoomed-offset-size.html (right): https://codereview.chromium.org/2260773002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/images/zoomed-offset-size.html:9: document.body.zoom = 1.0; On 2016/08/19 08:24:04, fs wrote: > I think the intention of this was to reset the zoom after the test had > completed, which is why it was set as the last thing. (The zoom should be 1 > initially already, you could assert that if you like.) > > See below though. Done. https://codereview.chromium.org/2260773002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/images/zoomed-offset-size.html:13: document.body.zoom = zoomLevel; On 2016/08/19 08:24:03, fs wrote: > I'm not aware of any 'zoom' property on HTMLBodyElement - so this does look like > it does what it set out to? Maybe this should set the 'zoom' CSS property as > inline style on <body>? Ok, i tried using document.body.style.zoom, instead of document.body.zoom, with this the image size is getting changed and it asserts. https://codereview.chromium.org/2260773002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/images/zoomed-offset-size.html:21: } On 2016/08/19 08:24:04, fs wrote: > You can drop the {} Done.
https://codereview.chromium.org/2260773002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/images/zoomed-offset-size.html (right): https://codereview.chromium.org/2260773002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/images/zoomed-offset-size.html:13: document.body.zoom = zoomLevel; On 2016/08/19 at 09:21:21, sivag wrote: > On 2016/08/19 08:24:03, fs wrote: > > I'm not aware of any 'zoom' property on HTMLBodyElement - so this does look like > > it does what it set out to? Maybe this should set the 'zoom' CSS property as > > inline style on <body>? > > Ok, i tried using document.body.style.zoom, instead of document.body.zoom, with this the image size is getting changed and it asserts. Ok, well it kind of figures I guess =). How many of the assertions are failing? (You could add the zoom level to the assertion descriptions to see which one fails.)
On 2016/08/19 09:38:18, fs wrote: > https://codereview.chromium.org/2260773002/diff/20001/third_party/WebKit/Layo... > File third_party/WebKit/LayoutTests/fast/images/zoomed-offset-size.html (right): > > https://codereview.chromium.org/2260773002/diff/20001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/fast/images/zoomed-offset-size.html:13: > document.body.zoom = zoomLevel; > On 2016/08/19 at 09:21:21, sivag wrote: > > On 2016/08/19 08:24:03, fs wrote: > > > I'm not aware of any 'zoom' property on HTMLBodyElement - so this does look > like > > > it does what it set out to? Maybe this should set the 'zoom' CSS property as > > > inline style on <body>? > > > > Ok, i tried using document.body.style.zoom, instead of document.body.zoom, > with this the image size is getting changed and it asserts. > > Ok, well it kind of figures I guess =). How many of the assertions are failing? > (You could add the zoom level to the assertion descriptions to see which one > fails.) when i printed 0.90 works fine and it fails at 0.95.
On 2016/08/19 09:38:18, fs wrote: > https://codereview.chromium.org/2260773002/diff/20001/third_party/WebKit/Layo... > File third_party/WebKit/LayoutTests/fast/images/zoomed-offset-size.html (right): > > https://codereview.chromium.org/2260773002/diff/20001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/fast/images/zoomed-offset-size.html:13: > document.body.zoom = zoomLevel; > On 2016/08/19 at 09:21:21, sivag wrote: > > On 2016/08/19 08:24:03, fs wrote: > > > I'm not aware of any 'zoom' property on HTMLBodyElement - so this does look > like > > > it does what it set out to? Maybe this should set the 'zoom' CSS property as > > > inline style on <body>? > > > > Ok, i tried using document.body.style.zoom, instead of document.body.zoom, > with this the image size is getting changed and it asserts. > > Ok, well it kind of figures I guess =). How many of the assertions are failing? > (You could add the zoom level to the assertion descriptions to see which one > fails.) CONSOLE MESSAGE: line 13: 0.9 CONSOLE MESSAGE: line 13: 0.95 This is a testharness.js-based test. FAIL Image should return a size of 250x166 regardless of zoom level. assert_equals: expected 250 but got 251 Harness: the test ran to completion.
On 2016/08/19 at 09:48:18, siva.gunturi wrote: > On 2016/08/19 09:38:18, fs wrote: > > https://codereview.chromium.org/2260773002/diff/20001/third_party/WebKit/Layo... > > File third_party/WebKit/LayoutTests/fast/images/zoomed-offset-size.html (right): > > > > https://codereview.chromium.org/2260773002/diff/20001/third_party/WebKit/Layo... > > third_party/WebKit/LayoutTests/fast/images/zoomed-offset-size.html:13: > > document.body.zoom = zoomLevel; > > On 2016/08/19 at 09:21:21, sivag wrote: > > > On 2016/08/19 08:24:03, fs wrote: > > > > I'm not aware of any 'zoom' property on HTMLBodyElement - so this does look > > like > > > > it does what it set out to? Maybe this should set the 'zoom' CSS property as > > > > inline style on <body>? > > > > > > Ok, i tried using document.body.style.zoom, instead of document.body.zoom, > > with this the image size is getting changed and it asserts. > > > > Ok, well it kind of figures I guess =). How many of the assertions are failing? > > (You could add the zoom level to the assertion descriptions to see which one > > fails.) > > CONSOLE MESSAGE: line 13: 0.9 > CONSOLE MESSAGE: line 13: 0.95 > This is a testharness.js-based test. > FAIL Image should return a size of 250x166 regardless of zoom level. assert_equals: expected 250 but got 251 > Harness: the test ran to completion. I guess what we should do for now is just check in the new expectations (i.e with the failure.) Don't know if we should go as far as to make a separate test() for each zoom-level, but that would allow tracking regressions with finer granularity. (This issue doesn't appear to be very image specific, so likely just a generic issue with zoom=0.95.)
On 2016/08/19 10:40:30, fs wrote: > On 2016/08/19 at 09:48:18, siva.gunturi wrote: > > On 2016/08/19 09:38:18, fs wrote: > > > > https://codereview.chromium.org/2260773002/diff/20001/third_party/WebKit/Layo... > > > File third_party/WebKit/LayoutTests/fast/images/zoomed-offset-size.html > (right): > > > > > > > https://codereview.chromium.org/2260773002/diff/20001/third_party/WebKit/Layo... > > > third_party/WebKit/LayoutTests/fast/images/zoomed-offset-size.html:13: > > > document.body.zoom = zoomLevel; > > > On 2016/08/19 at 09:21:21, sivag wrote: > > > > On 2016/08/19 08:24:03, fs wrote: > > > > > I'm not aware of any 'zoom' property on HTMLBodyElement - so this does > look > > > like > > > > > it does what it set out to? Maybe this should set the 'zoom' CSS > property as > > > > > inline style on <body>? > > > > > > > > Ok, i tried using document.body.style.zoom, instead of document.body.zoom, > > > with this the image size is getting changed and it asserts. > > > > > > Ok, well it kind of figures I guess =). How many of the assertions are > failing? > > > (You could add the zoom level to the assertion descriptions to see which one > > > fails.) > > > > CONSOLE MESSAGE: line 13: 0.9 > > CONSOLE MESSAGE: line 13: 0.95 > > This is a testharness.js-based test. > > FAIL Image should return a size of 250x166 regardless of zoom level. > assert_equals: expected 250 but got 251 > > Harness: the test ran to completion. > > I guess what we should do for now is just check in the new expectations (i.e > with the failure.) Don't know if we should go as far as to make a separate > test() for each zoom-level, but that would allow tracking regressions with finer > granularity. (This issue doesn't appear to be very image specific, so likely > just a generic issue with zoom=0.95.) Done, I added the new test expectation.
lgtm
The CQ bit was checked by siva.gunturi@samsung.com
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 ========== Convert image size and zoom level test to testharness.js 1.Remove zoomed-offset-size-expected.txt. 2.Simply code to test whether image size is changed when zoom level is altered. BUG=637859 ========== to ========== Convert image size and zoom level test to testharness.js 1.Remove zoomed-offset-size-expected.txt. 2.Simply code to test whether image size is changed when zoom level is altered. BUG=637859 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Convert image size and zoom level test to testharness.js 1.Remove zoomed-offset-size-expected.txt. 2.Simply code to test whether image size is changed when zoom level is altered. BUG=637859 ========== to ========== Convert image size and zoom level test to testharness.js 1.Remove zoomed-offset-size-expected.txt. 2.Simply code to test whether image size is changed when zoom level is altered. BUG=637859 Committed: https://crrev.com/0fd18b4775d4038af7512dfc72b2bc17425faabb Cr-Commit-Position: refs/heads/master@{#413146} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/0fd18b4775d4038af7512dfc72b2bc17425faabb Cr-Commit-Position: refs/heads/master@{#413146} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
