Bug 98558 - Re-enable WebFrameTest.DivScrollIntoEditableTest
Summary: Re-enable WebFrameTest.DivScrollIntoEditableTest
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Barth
Depends on: 108166
  Show dependency treegraph
Reported: 2012-10-05 14:36 PDT by WebKit Review Bot
Modified: 2013-01-29 11:54 PST (History)
5 users (show)

See Also:

Patch (5.37 KB, patch)
2013-01-28 21:43 PST, Xianzhu Wang
no flags Details | Formatted Diff | Diff
Patch (comments updated) (5.38 KB, patch)
2013-01-28 21:49 PST, Xianzhu Wang
no flags Details | Formatted Diff | Diff
Use EXPECT_NEAR to tolerate inaccuracy (7.79 KB, patch)
2013-01-29 09:51 PST, Xianzhu Wang
no flags Details | Formatted Diff | Diff
Patch (12.48 KB, patch)
2013-01-29 10:49 PST, Xianzhu Wang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description WebKit Review Bot 2012-10-05 14:36:23 PDT
Re-enable WebFrameTest.DivScrollIntoEditableTest
Requested by abarth on #webkit.
Comment 1 Xianzhu Wang 2013-01-28 21:43:37 PST
Created attachment 185153 [details]
Comment 2 Xianzhu Wang 2013-01-28 21:49:59 PST
Created attachment 185154 [details]
Patch (comments updated)
Comment 3 Xianzhu Wang 2013-01-28 21:52:08 PST
The original comment above the function read "This test depends on code that is compiled conditionally." I wonder what the macro is and what the original problem was.
Comment 4 Adam Barth 2013-01-28 22:01:40 PST
I don't remember, but re-enabling it is likely to just cause the same failures.  Perhaps we should run a try job to try to figure out what the issue was?
Comment 5 Xianzhu Wang 2013-01-28 22:23:01 PST
I modified the test at several places to make it pass in my environment locally (chromium-linux). Will cr-linux ews run webkit_unit_tests?
Comment 6 Adam Barth 2013-01-28 22:23:53 PST
> Will cr-linux ews run webkit_unit_tests?

Yes, but only on linux.  Do you know if it passes on cr-android as well?
Comment 7 Xianzhu Wang 2013-01-28 22:49:25 PST
Tried the test with the patch on chromium-android. It passed.

The tested code is all in WebViewImpl::scrollFocusedNodeIntoRect(). The conditionally compiled code seems not to cause problem now, given that the test has been modified to be consistent with the current code.
Comment 8 Adam Barth 2013-01-28 23:06:22 PST
Comment on attachment 185154 [details]
Patch (comments updated)

OK.  Let's land this and see what happens!
Comment 9 WebKit Review Bot 2013-01-28 23:41:59 PST
Comment on attachment 185154 [details]
Patch (comments updated)

Clearing flags on attachment: 185154

Committed r141064: <http://trac.webkit.org/changeset/141064>
Comment 10 WebKit Review Bot 2013-01-28 23:42:02 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 WebKit Review Bot 2013-01-29 01:06:17 PST
Re-opened since this is blocked by bug 108166
Comment 12 Keishi Hattori 2013-01-29 01:08:21 PST
Test seems to be failing on WinXP so I am rolling the patch out.


tests\WebFrameTest.cpp(974): error: Value of: webView->mainFrame()->scrollOffset().width
Actual: 494
Expected: hScroll
Which is: 495
Comment 13 Build Bot 2013-01-29 02:59:19 PST
Comment on attachment 185154 [details]
Patch (comments updated)

Attachment 185154 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16151142

New failing tests:
Comment 14 Xianzhu Wang 2013-01-29 09:51:58 PST
Created attachment 185256 [details]
Use EXPECT_NEAR to tolerate inaccuracy
Comment 15 Xianzhu Wang 2013-01-29 10:35:56 PST
Comment on attachment 185256 [details]
Use EXPECT_NEAR to tolerate inaccuracy

Just found that though the changed test passes, it misses a case to test. Will update the patch soon.
Comment 16 Xianzhu Wang 2013-01-29 10:49:09 PST
Created attachment 185264 [details]
Comment 17 Xianzhu Wang 2013-01-29 11:54:37 PST
The test will be rewritten in bug 107599. We are obsoleting webkit scaling so we don't need the original test any more.