Bug 98558

Summary:

Re-enable WebFrameTest.DivScrollIntoEditableTest

Product:

WebKit

Reporter:

WebKit Review Bot <webkit.review.bot>

Component:

New Bugs

Assignee:

Adam Barth <abarth>

Status:

RESOLVED WONTFIX

Severity:

Normal

CC:

abarth, buildbot, keishi, rniwa, wangxianzhu

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

108166

Bug Blocks:

Attachments:

Description	Flags
Patch	none
Patch (comments updated)	none
Use EXPECT_NEAR to tolerate inaccuracy	none
Patch	none

WebKit Review Bot

Reported 2012-10-05 14:36:23 PDT

Re-enable WebFrameTest.DivScrollIntoEditableTest Requested by abarth on #webkit.

Attachments
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
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Xianzhu Wang

Comment 1 2013-01-28 21:43:37 PST

Created attachment 185153 [details] Patch

Xianzhu Wang

Comment 2 2013-01-28 21:49:59 PST

Created attachment 185154 [details] Patch (comments updated)

Xianzhu Wang

Comment 3 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.

Adam Barth

Comment 4 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?

Xianzhu Wang

Comment 5 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?

Adam Barth

Comment 6 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?

Xianzhu Wang

Comment 7 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.

Adam Barth

Comment 8 2013-01-28 23:06:22 PST

Comment on attachment 185154 [details] Patch (comments updated) OK. Let's land this and see what happens!

WebKit Review Bot

Comment 9 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>

WebKit Review Bot

Comment 10 2013-01-28 23:42:02 PST

All reviewed patches have been landed. Closing bug.

WebKit Review Bot

Comment 11 2013-01-29 01:06:17 PST

Re-opened since this is blocked by bug 108166

Keishi Hattori

Comment 12 2013-01-29 01:08:21 PST

Test seems to be failing on WinXP so I am rolling the patch out. http://chromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20XP/builds/2419/steps/webkit_unit_tests/logs/DivScrollIntoEditableTest WebFrameTest.DivScrollIntoEditableTest: tests\WebFrameTest.cpp(974): error: Value of: webView->mainFrame()->scrollOffset().width Actual: 494 Expected: hScroll Which is: 495

Build Bot

Comment 13 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: http/tests/inspector/appcache/appcache-manifest-with-non-existing-file.html

Xianzhu Wang

Comment 14 2013-01-29 09:51:58 PST

Created attachment 185256 [details] Use EXPECT_NEAR to tolerate inaccuracy

Xianzhu Wang

Comment 15 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.

Xianzhu Wang

Comment 16 2013-01-29 10:49:09 PST

Created attachment 185264 [details] Patch

Xianzhu Wang

Comment 17 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.

Note You need to log in before you can comment on or make changes to this bug.