Bug 39645

Summary: [chromium] Fix zoom tests after r60104
Product: WebKit Reporter: Tony Chang <tony>
Component: New BugsAssignee: Tony Chang <tony>
Status: RESOLVED FIXED    
Severity: Normal CC: levin
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
Patch levin: review+

Description Tony Chang 2010-05-24 22:10:56 PDT
[chromium] Fix zoom tests after r60104d
Comment 1 Tony Chang 2010-05-24 22:11:19 PDT
Created attachment 56971 [details]
Patch
Comment 2 Tony Chang 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.
Comment 3 David Levin 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?
Comment 4 Adam Barth 2010-05-24 23:38:43 PDT
Comment on attachment 56971 [details]
Patch

I'm not an expert here, but this looks reasonable.
Comment 5 Tony Chang 2010-05-24 23:41:32 PDT
Created attachment 56978 [details]
Patch
Comment 6 Tony Chang 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.
Comment 7 David Levin 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. :)
Comment 8 Tony Chang 2010-05-24 23:54:56 PDT
Committed r60131: <http://trac.webkit.org/changeset/60131>
Comment 9 Tony Chang 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!