Created attachment 166341 [details] Patch

Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

Comment on attachment 166341 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166341&action=review Can we test this patch? > Source/WebKit/chromium/src/WebViewImpl.cpp:2640 > + IntRect boxrect = focusedNode->document()->view()->contentsToWindow(pixelSnappedIntRect(focusedNode->Node::boundingBox())); Rather than having a comment explaining what this variable represents, we prefer to put that information into the name: boxrect -> textboxRect > Source/WebKit/chromium/src/WebViewImpl.cpp:2656 > + IntRect docBox = boxrect; docBox -> ? textboxRectInDocumentCoordinates? > Source/WebKit/chromium/src/WebViewImpl.cpp:2659 > + IntRect docCaret = caret; docCaret -> ? carentInDocumentCoordinates? Should we have a function on document()->view() called windowToContents? It seems strange to manually do this work here. > Source/WebKit/chromium/src/WebViewImpl.cpp:2699 > + if (needAnimation) > + startPageScaleAnimation(newOffset, false, newScale, scrollAndScaleAnimationDurationInSeconds); We don't actually need to do anything if needAnimation is false?

Comment on attachment 166341 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166341&action=review Yusuf has offered to write unit tests for this code, he'll be following up. >> Source/WebKit/chromium/src/WebViewImpl.cpp:2699 >> + startPageScaleAnimation(newOffset, false, newScale, scrollAndScaleAnimationDurationInSeconds); > > We don't actually need to do anything if needAnimation is false? Correct. It means we've decided the textbox is already onscreen and legible.

Comment on attachment 166341 [details] Patch Marking r- pending unit tests.

Created attachment 167173 [details] Patch

Created attachment 167198 [details] Patch

Comment on attachment 167198 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167198&action=review LGTM, minor nit: > Source/WebKit/chromium/src/WebViewImpl.cpp:2658 > + IntRect carentInDocumentCoordinates = caret; Spelling: no "n" in "caret".

Created attachment 167206 [details] Patch

Comment on attachment 167206 [details] Patch Attachment 167206 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14178319 New failing tests: WebFrameTest.DivScrollIntoEditableTest

Created attachment 167349 [details] Patch

Looks like I had misnamed one of the variables while making the name changes Adam requested on the previous patch. The test should be passing now.

Comment on attachment 167349 [details] Patch Thanks.

Comment on attachment 167349 [details] Patch Clearing flags on attachment: 167349 Committed r130545: <http://trac.webkit.org/changeset/130545>

All reviewed patches have been landed. Closing bug.

Fixed Chromium Windows build in http://trac.webkit.org/changeset/130550. The change introduced a warning: 6>tests\WebFrameTest.cpp(507): warning C4305: 'initializing' : truncation from 'double' to 'float'

Re-opened since this is blocked by bug 98556

The failing unit test was disabled in http://trac.webkit.org/changeset/130552. Re-enabling it is tracked by: https://bugs.webkit.org/show_bug.cgi?id=98558