Bug 42089 - reported size of image is changed when page is zoomed in (or out)
Summary: reported size of image is changed when page is zoomed in (or out)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Normal
Assignee: Peter Kasting
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-07-12 09:52 PDT by zebul666
Modified: 2012-08-30 05:39 PDT (History)
9 users (show)

See Also:


Attachments
test case (1.38 KB, application/x-gzip)
2010-07-12 09:52 PDT, zebul666
no flags Details
patch v1 (9.25 KB, patch)
2010-08-23 17:36 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
patch v1.1 (9.19 KB, patch)
2010-08-23 17:44 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
patch v1.2 (10.46 KB, patch)
2010-08-30 13:14 PDT, Peter Kasting
simon.fraser: review-
simon.fraser: commit-queue-
Details | Formatted Diff | Diff
patch v2.0 (10.15 KB, patch)
2010-08-30 14:19 PDT, Peter Kasting
simon.fraser: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
patch v3.0 (11.27 KB, patch)
2010-08-31 18:25 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
patch v3.1 (17.19 KB, patch)
2010-09-01 12:56 PDT, Peter Kasting
simon.fraser: review+
simon.fraser: commit-queue-
Details | Formatted Diff | Diff
patch v3.2 (17.19 KB, patch)
2010-09-01 13:38 PDT, Peter Kasting
simon.fraser: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
patch v3.3 (6.24 KB, patch)
2010-09-02 18:03 PDT, Peter Kasting
pkasting: commit-queue+
Details | Formatted Diff | Diff
patch v3.4 (16.96 KB, patch)
2010-09-02 18:04 PDT, Peter Kasting
commit-queue: commit-queue-
Details | Formatted Diff | Diff
patch v3.5 (16.79 KB, patch)
2010-09-03 14:20 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description zebul666 2010-07-12 09:52:34 PDT
Created attachment 61241 [details]
test case

The size of an image reported by javascript changes when you zoom the page with ctrl-mouse up down.

This has been tested on Safari 5.0 (7533.16) (windows xp sp3) and Chromium 5.0.375.99 (0) (linux)
Firefox 3.6.6 (linux) and Opera 10.60 (6386) (linux) and Internet Explorer 8 don't behave that way; i.e. there is no change in image property size (height, width).

I spotted the bug when using the canvas element and resizing (zooming in) with CTRL-mouse up on my page.

You can find attached img.html that shows the bug with only an image if you resize it with ctrl-mouse up. Also I found that the reported size is not even accurate because if I take a screenshot and use a pixel ruler, I found a size greater than the one reported every time.

the canvas.html shows the problem on a canvas element

but the root of the problem is the reported size of an image if zoomed in or out. Should it change or not ? If not it's a bug.
Comment 1 Peter Kasting 2010-08-23 14:19:35 PDT
I'm planning to fix this, as well as the problem where the wrong image size is reported in the page title when viewing a standalone zoomed image.
Comment 2 Peter Kasting 2010-08-23 17:36:20 PDT
Created attachment 65187 [details]
patch v1

This fixes the issues reported on this bug, and also reports the correct (unzoomed) image size in the page title when viewing a standalone image.
Comment 3 Peter Kasting 2010-08-23 17:37:44 PDT
Hyatt - I wrote the testcase to also highlight how inaccurate this patch is for zoom factors < 1.
Comment 4 WebKit Review Bot 2010-08-23 17:41:56 PDT
Attachment 65187 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/html/HTMLImageElement.cpp:276:  render_box is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/html/HTMLImageElement.cpp:299:  render_box is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Peter Kasting 2010-08-23 17:44:21 PDT
Created attachment 65190 [details]
patch v1.1

Fix style issues
Comment 6 James Robinson 2010-08-30 12:57:43 PDT
Patch appears to be missing expected results.  Otherwise, looks pretty good to me (although I'm not a reviewer).
Comment 7 Peter Kasting 2010-08-30 13:14:45 PDT
Created attachment 65949 [details]
patch v1.2

Oops.  Forgot to svn add that file.
Comment 8 Simon Fraser (smfr) 2010-08-30 13:20:50 PDT
Comment on attachment 65949 [details]
patch v1.2

Looks like the expected result is wrong, and it would be better with a script-based test that prints PASS or FAIL.
Comment 9 Peter Kasting 2010-08-30 14:19:43 PDT
Created attachment 65953 [details]
patch v2.0

New testcase which prints PASS/FAIL as well as the zoom level on each oval.
Comment 10 WebKit Commit Bot 2010-08-31 01:01:36 PDT
Comment on attachment 65953 [details]
patch v2.0

Rejecting patch 65953 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--wait-for-httpd', '--ignore-tests', 'compositing,media', '--quiet']" exit_code: 1
Running build-dumprendertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 20875 test cases.
fast/images/zoomed-img-size.html -> failed

Exiting early after 1 failures. 8135 tests run.
186.55s total testing time

8134 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
1 test case (<1%) had stderr output

Full output: http://queues.webkit.org/results/3843193
Comment 11 Peter Kasting 2010-08-31 18:05:53 PDT
The reason the current patch fails is because on Windows, the effective zoom of 5% is stored as 0.050000001, and when adjustForAbsoluteZoom() divides 1 by that, it gets a number very close to, but not quite, 20.

I don't know what happens on Mac.

I think it would be good to know why Mac and Windows differ.  But I also think adjustForAbsoluteZoom() should copy the rounding code from CSSPrimitiveValue::computeLengthInt() and bump values away from 0 by 0.01 before rounding.  I wonder how many layout tests this breaks :(
Comment 12 Peter Kasting 2010-08-31 18:25:40 PDT
Created attachment 66156 [details]
patch v3.0

Here's the patch with the rounding code added to adjustForAbsoluteZoom().
Comment 13 Peter Kasting 2010-09-01 10:30:10 PDT
James and I tried to step through Mac to see why it differed from Windows.  We found that the assembly instruction that did a single-precision floating point divide of "1.0 / 0.050000007" produced a result of "20.0".  I don't know why.
Comment 14 James Robinson 2010-09-01 10:36:59 PDT
$ cat test.cc
#include <stdio.h>
int main() {
    unsigned bitpattern = 0x3d4ccccd;
    float f = *reinterpret_cast<float*>(&bitpattern);
    printf("f = %.15f, (int)(1.0f / f) = %d\n", f, (int)(1.0f / f));
    double d = f;
    printf("d = %.15f, (int)(1.0 / f) = %d\n", d, (int)(1.0 / d));
    return 0;
}
jamesr@jamesr:~$ ./test
f = 0.050000000745058, (int)(1.0f / f) = 20
d = 0.050000000745058, (int)(1.0 / f) = 19

so it appears that the divide is a single-precision FP divide on a mac (it compiles to a DIVSS in fact), but that it might be happening in double precision on other platforms.  Will have to check the disassembly on a platform that produces 19 to know for sure.

Either way, a fudge factor seems prudent.
Comment 15 Simon Fraser (smfr) 2010-09-01 10:40:53 PDT
Would be nice to share 

    result += result < 0 ? -0.01 : +0.01;

    if (result > INT_MAX || result < INT_MIN)
        return 0;
    return static_cast<int>(result);

everywhere it occurs, perhaps with some templatized code.
Comment 16 Peter Kasting 2010-09-01 12:56:36 PDT
Created attachment 66250 [details]
patch v3.1

This factors out the rounding code into a template function so it can be shared among all callers.
Comment 17 Simon Fraser (smfr) 2010-09-01 13:21:41 PDT
Comment on attachment 66250 [details]
patch v3.1

> Index: WebCore/css/CSSPrimitiveValue.cpp
> ===================================================================
> --- WebCore/css/CSSPrimitiveValue.cpp	(revision 66611)
> +++ WebCore/css/CSSPrimitiveValue.cpp	(working copy)
> @@ -45,6 +45,15 @@ using namespace WTF;
>  
>  namespace WebCore {
>  
> +template<typename T, T max, T min> inline T roundForImpreciseConversion(double result)
> +{
> +    // Dimension calculations are imprecise, often resulting in values of e.g.
> +    // 44.99998.  We need to go ahead and round if we're really close to the
> +    // next integer value.
> +    result += (result < 0) ? -0.01 : +0.01;
> +    return (result > max || result < min) ? 0 : static_cast<T>(result);
> +}

Won't this cause template instantiation for each combination of, T, min and max? Maybe min and max would
be better as arguments to the function.

Also 'result' is an odd name for an input paramter.

r=me
Comment 18 Peter Kasting 2010-09-01 13:38:37 PDT
Created attachment 66257 [details]
patch v3.2

This changes the name "result" to "value", which is what nearby functions use.

Template arguments will cause one extra instantiation of the function.  OTOH, they might result in better code (since the compiler will have the literal values to compare), and I think the callers read slightly more clearly than with function arguments.  (Stroustrup also tends to use template args for function bounds.)  So in the end I have a very weak preference for template args.  If you feel strongly, I'll change them.
Comment 19 WebKit Commit Bot 2010-09-01 21:57:52 PDT
Comment on attachment 66257 [details]
patch v3.2

Rejecting patch 66257 from commit-queue.

Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Simon Fraser', u'--force']" exit_code: 1
Last 500 characters of output:
pp
Hunk #2 FAILED at 314.
1 out of 2 hunks FAILED -- saving rejects to file WebCore/css/CSSPrimitiveValue.cpp.rej
patching file WebCore/css/CSSPrimitiveValue.h
patching file WebCore/html/HTMLImageElement.cpp
patching file WebCore/loader/ImageDocument.cpp
patching file WebCore/rendering/RenderObject.h
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/fast/images/zoomed-img-size-expected.txt
patching file LayoutTests/fast/images/zoomed-img-size.html

Full output: http://queues.webkit.org/results/3966030
Comment 20 Peter Kasting 2010-09-02 18:03:30 PDT
Created attachment 66448 [details]
patch v3.3

Fixed to apply cleanly
Comment 21 Peter Kasting 2010-09-02 18:04:06 PDT
Created attachment 66449 [details]
patch v3.4

Hmm, last upload incomplete
Comment 22 WebKit Commit Bot 2010-09-03 00:46:44 PDT
Comment on attachment 66449 [details]
patch v3.4

Rejecting patch 66449 from commit-queue.

Failed to run "['WebKitTools/Scripts/build-webkit', '--debug']" exit_code: 1
Last 500 characters of output:
/Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/i386/XSLTProcessor.o /Users/eseidel/Projects/CommitQueue/WebCore/xml/XSLTProcessor.cpp normal i386 c++ com.apple.compilers.gcc.4_2
	Distributed-CompileC /Users/eseidel/Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/i386/HTMLParserScheduler.o /Users/eseidel/Projects/CommitQueue/WebCore/html/parser/HTMLParserScheduler.cpp normal i386 c++ com.apple.compilers.gcc.4_2
(409 failures)


Full output: http://queues.webkit.org/results/3956088
Comment 23 Peter Kasting 2010-09-03 14:20:04 PDT
Created attachment 66540 [details]
patch v3.5

Blind attempt at fixing strange compile error.
Comment 24 WebKit Commit Bot 2010-09-03 16:04:51 PDT
Comment on attachment 66540 [details]
patch v3.5

Clearing flags on attachment: 66540

Committed r66776: <http://trac.webkit.org/changeset/66776>
Comment 25 WebKit Commit Bot 2010-09-03 16:04:57 PDT
All reviewed patches have been landed.  Closing bug.
Comment 26 WebKit Review Bot 2010-09-03 18:18:09 PDT
http://trac.webkit.org/changeset/66776 might have broken SnowLeopard Intel Release (Tests)
Comment 27 nedyalko.andreev 2012-08-30 05:39:07 PDT
This appears to be broken again in version Chrome 21.0.1180.83
Just tried the image test case and zoom-out changes the width and height