RESOLVED FIXED 39645
[chromium] Fix zoom tests after r60104 
https://bugs.webkit.org/show_bug.cgi?id=39645
Summary [chromium] Fix zoom tests after r60104
Tony Chang
Reported 2010-05-24 22:10:56 PDT
[chromium] Fix zoom tests after r60104d
Attachments
Patch (1.26 KB, patch)
2010-05-24 22:11 PDT, Tony Chang 		no flags
DetailsFormatted DiffDiff
Patch (1.22 KB, patch)
2010-05-24 23:41 PDT, Tony Chang 		levin: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Tony Chang
Comment 1 2010-05-24 22:11:19 PDT
Created attachment 56971 [details] Patch
Tony Chang
Comment 2 2010-05-24 22:12:28 PDT
In http://trac.webkit.org/changeset/60111 , I tried to fix the build break caused by http://trac.webkit.org/changeset/60104. The fix was wrong and causes some zoom related layout tests to fail. This should fix those tests.
David Levin
Comment 3 2010-05-24 22:28:13 PDT
I'm sure you know what you are doing :) but there isn't enough in here to help me understand why this is correct. I started to try to figure it all out again but you probably already did this, so hopefully you can quickly explain, why did r60104 change things in a way that requires this change?
Adam Barth
Comment 4 2010-05-24 23:38:43 PDT
Comment on attachment 56971 [details] Patch I'm not an expert here, but this looks reasonable.
Tony Chang
Comment 5 2010-05-24 23:41:32 PDT
Created attachment 56978 [details] Patch
Tony Chang
Comment 6 2010-05-24 23:45:58 PDT
(In reply to comment #3) > I'm sure you know what you are doing :) but there isn't enough in here to help me understand why this is correct. > > I started to try to figure it all out again but you probably already did this, so hopefully you can quickly explain, why did r60104 change things in a way that requires this change? Sorry, I should have included an explanation. In writing it, I was able to make this a little simpler. m_zoomFactor moved from Frame to FrameView. FrameView has separate notions of page zoom and text zoom. In my hurried compile fix, I assumed that m_zoomFactor was for page zoom, but it's not always. I guess in tests, we default to text zoom factor, but the fix is to read the variable directly rather than checking to see if we're in text zoom or page zoom mode.
David Levin
Comment 7 2010-05-24 23:48:33 PDT
Comment on attachment 56978 [details] Patch Would have been nice to have some of the explanation in the ChangeLog, but ok. :)
Tony Chang
Comment 8 2010-05-24 23:54:56 PDT
Committed r60131: <http://trac.webkit.org/changeset/60131>
Tony Chang
Comment 9 2010-05-24 23:55:46 PDT
(In reply to comment #7) > (From update of attachment 56978 [details]) > Would have been nice to have some of the explanation in the ChangeLog, but ok. :) I copied some of the explanation into the ChangeLog. Thanks for the fast review!
Note You need to log in before you can comment on or make changes to this bug.