Bug 97958

Summary: [chromium] Support zooming focused node for mobile devices
Product: WebKit Reporter: Alexandre Elias <aelias>
Component: New BugsAssignee: Alexandre Elias <aelias>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, aelias, dglazkov, dpranke, fishd, jamesr, jchaffraix, johnme, olilan, tkent+wkapi, webkit.review.bot, yusufo
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 98556    
Bug Blocks: 91648    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Alexandre Elias
Reported 2012-09-28 18:19:07 PDT
[chromium] Support zooming focused node for mobile devices
Attachments
Patch (9.26 KB, patch)
2012-09-28 18:20 PDT, Alexandre Elias 		no flags
DetailsFormatted DiffDiff
Patch (15.02 KB, patch)
2012-10-04 14:28 PDT, yusufo 		no flags
DetailsFormatted DiffDiff
Patch (15.03 KB, patch)
2012-10-04 16:26 PDT, yusufo 		no flags
DetailsFormatted DiffDiff
Patch (15.03 KB, patch)
2012-10-04 16:53 PDT, yusufo 		no flags
DetailsFormatted DiffDiff
Patch (15.01 KB, patch)
2012-10-05 10:43 PDT, yusufo 		no flags
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Alexandre Elias
Comment 1 2012-09-28 18:20:28 PDT
Created attachment 166341 [details] Patch
WebKit Review Bot
Comment 2 2012-09-28 18:24:58 PDT
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.
Adam Barth
Comment 3 2012-10-01 10:51:54 PDT
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?
Alexandre Elias
Comment 4 2012-10-01 13:51:26 PDT
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.
Adam Barth
Comment 5 2012-10-01 13:59:41 PDT
Comment on attachment 166341 [details] Patch Marking r- pending unit tests.
yusufo
Comment 6 2012-10-04 14:28:31 PDT
Created attachment 167173 [details] Patch
yusufo
Comment 7 2012-10-04 16:26:55 PDT
Created attachment 167198 [details] Patch
Alexandre Elias
Comment 8 2012-10-04 16:45:48 PDT
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".
yusufo
Comment 9 2012-10-04 16:53:46 PDT
Created attachment 167206 [details] Patch
WebKit Review Bot
Comment 10 2012-10-04 20:51:57 PDT
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
yusufo
Comment 11 2012-10-05 10:43:45 PDT
Created attachment 167349 [details] Patch
yusufo
Comment 12 2012-10-05 10:45:13 PDT
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.
Adam Barth
Comment 13 2012-10-05 13:24:17 PDT
Comment on attachment 167349 [details] Patch Thanks.
WebKit Review Bot
Comment 14 2012-10-05 13:30:50 PDT
Comment on attachment 167349 [details] Patch Clearing flags on attachment: 167349 Committed r130545: <http://trac.webkit.org/changeset/130545>
WebKit Review Bot
Comment 15 2012-10-05 13:30:54 PDT
All reviewed patches have been landed. Closing bug.
Julien Chaffraix
Comment 16 2012-10-05 14:14:23 PDT
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'
WebKit Review Bot
Comment 17 2012-10-05 14:31:36 PDT
Re-opened since this is blocked by bug 98556
Julien Chaffraix
Comment 18 2012-10-05 14:40:51 PDT
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
Note You need to log in before you can comment on or make changes to this bug.