Bug 90316

This is not ready for review yet.

Created attachment 150266 [details] Patch

Hi Yusuf, what is the status of this patch?

I need to add a test and get rid of some of the comments, but I will get back to this by the end of this week or next week. (In reply to comment #3) > Hi Yusuf, what is the status of this patch?

Comment on attachment 150266 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=150266&action=review > Source/WebKit/chromium/src/WebViewImpl.cpp:189 > +static const float deviceDpiUnit = 160; There's a constant for this in ViewportArguments.h.

Created attachment 159213 [details] Patch

Comment on attachment 159213 [details] Patch Attachment 159213 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13514997

Comment on attachment 159213 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159213&action=review > Source/WebKit/chromium/src/WebViewImpl.cpp:1071 > + const float deviceDpiScale = screenHorizontalDPI(mainFrameImpl()->frameView()) / deviceDpiUnit; Actually, it should be fine again to use deviceScaleFactor() for this. Please switch it back and delete deviceDpiUnit. > Source/WebKit/chromium/src/WebViewImpl.cpp:3796 > + m_doubleTapZoomInEffect = false; It looks like this will fail to work in animated zooms. The new page scale will come back to you asynchronously in this function. So you are setting it to true, then milliseconds later false again. One workaround would be to remember what scale you've sent down and only set to false if it's different. > Source/WebKit/chromium/tests/WebFrameTest.cpp:393 > +void simulateDoubleTap(WebViewImpl* webViewImpl, WebPoint& point, WebPoint& scroll, float& scale, bool& isAnchor) You should be able to reuse startPageScaleAnimation() given that it's instantaneous when the time is 0. > Source/WebKit/chromium/tests/WebFrameTest.cpp:432 > + EXPECT_FLOAT_EQ(deviceDpiScale, scale); What does the div have to do with deviceDpiScale? Is it just because you arranged the sizes that way?

(In reply to comment #8) > (From update of attachment 159213 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=159213&action=review > > > Source/WebKit/chromium/src/WebViewImpl.cpp:1071 > > + const float deviceDpiScale = screenHorizontalDPI(mainFrameImpl()->frameView()) / deviceDpiUnit; > > Actually, it should be fine again to use deviceScaleFactor() for this. Please switch it back and delete deviceDpiUnit. Yeah, we're trying to get rid of the 160 constant from as many places as possible and operate in terms of scale factors whenever possible (instead of fake DPI values).

Comment on attachment 159213 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159213&action=review Sounds like we'll need to iterate a bit on this patch. > Source/WebKit/chromium/src/WebViewImpl.cpp:698 > + case WebInputEvent::GestureDoubleTap: { > + animateZoomAroundPoint(WebPoint(event.x, event.y), DoubleTap); > + return true; > + } Why don't we want to give WebCore a chance to handle this gesture first via handleGestureEvent ? I would expect us to call animateZoomAroundPoint as the default event handler after WebCore has processed it. Maybe I don't understand how gesture events work in enough detail... > Source/WebKit/chromium/src/WebViewImpl.cpp:1061 > + // be visible). TODO(johnme): Revisit this if it isn't always true. TODO(johnme): -> FIXME: (WebKit uses FIXME rather than TODO and doesn't list user names in the comments.) > Source/WebKit/chromium/src/WebViewImpl.cpp:1143 > + scroll = clampOffsetAtScale(scroll, scale); Four-space indent.

Created attachment 165880 [details] Patch

Created attachment 165882 [details] Patch

Attachment 165882 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/ChangeLog', u'Sourc..." exit_code: 1 Source/WebKit/chromium/src/WebViewImpl.cpp:1133: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit/chromium/src/WebViewImpl.cpp:1134: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit/chromium/src/WebViewImpl.cpp:1135: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit/chromium/src/WebViewImpl.cpp:1136: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit/chromium/src/WebViewImpl.cpp:1137: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit/chromium/src/WebViewImpl.cpp:1140: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit/chromium/src/WebViewImpl.cpp:1142: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit/chromium/tests/WebFrameTest.cpp:319: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit/chromium/tests/WebFrameTest.cpp:350: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit/chromium/tests/WebFrameTest.cpp:352: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit/chromium/tests/WebFrameTest.cpp:393: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 11 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 165882 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165882&action=review Seems good in general, just a few comments. > Source/WebKit/chromium/src/WebViewImpl.cpp:1102 > + const float minMaxScale = m_minimumPageScaleFactor * doubleTapZoomMinMax; Rename "minMaxScale" to "legibleScale", "minMaxScale" to "defaultScaleWhenAlreadyLegible", and "doubleTapZoomMinMax" to "doubleTapZoomAlreadyLegibleRatio". The "already legible" stuff is also worth a brief comment. I'd write: // When we compute that text is already legible at or near minimum scale, we still want // to provide the user some kind of feedback for their double-tap gesture. In that // case, zoom the small amount represented by "defaultScaleWhenAlreadyLegible". > Source/WebKit/chromium/tests/WebFrameTest.cpp:-34 > - No need to delete this whitespace. > Source/WebKit/chromium/tests/WebFrameTest.cpp:394 > + webViewImpl->computeScaleAndScrollForHitRect(rect, WebViewImpl::DoubleTap, scale, scroll, isAnchor); Instead of doing this, could you call animateZoomAroundPoint() to exercise more of the code? Use some test trickery to set doubleTapZoomAnimationDurationInSeconds to zero in the test environment; then it will end up just calling setPageScaleFactor() like you're doing here.

(In reply to comment #14) > Rename "minMaxScale" to "legibleScale", "minMaxScale" to "defaultScaleWhenAlreadyLegible", and "doubleTapZoomMinMax" to "doubleTapZoomAlreadyLegibleRatio". Sorry, for the first one, I meant rename "maxScale" to "legibleScale".

Created attachment 166041 [details] Patch

Comment on attachment 166041 [details] Patch Attachment 166041 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14031956

Created attachment 166045 [details] Patch

LGTM. Adam, could you review this?

Comment on attachment 166045 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166045&action=review > Source/WebKit/chromium/src/WebViewImpl.cpp:1076 > + float adjustedDpiScale = deviceScaleFactor(); What's the point of this variable? > Source/WebKit/chromium/src/WebViewImpl.cpp:1079 > + float legibleScale = adjustedDpiScale; > + if (adjustedDpiScale < defaultScaleWhenAlreadyLegible) This can just be: float legibleScale = deviceScaleFactor(); if (legibleScale < defaultScaleWhenAlreadyLegible) [...] > Source/WebKit/chromium/src/WebViewImpl.cpp:1214 > + WebPoint pt = point; pt <--- please use complete words. If you just need to convert the type, you might call this variable webPoint. > Source/WebKit/chromium/src/WebViewImpl.h:564 > - void computeScaleAndScrollForHitRect(const WebRect& hitRect, AutoZoomType, float& scale, WebPoint& scroll); > + void computeScaleAndScrollForHitRect(const WebRect& hitRect, AutoZoomType, float& scale, WebPoint& scroll, bool &isAnchor); bool & -> bool& > Source/WebKit/chromium/src/WebViewImpl.h:571 > + // Exposed for testing purposes. > + void shouldUseAnimateDoubleTapTimeZero(bool); Typically we put that information in the name of the function so we can tell when non-testing code tries to call it: void shouldUseAnimateDoubleTapTimeZeroForTesting(bool) > Source/WebKit/chromium/tests/WebFrameTest.cpp:346 > + WebViewImpl* webViewImpl = static_cast<WebViewImpl*>(FrameTestHelpers::createWebViewAndLoad(m_baseURL + "get_scale_for_auto_zoom_into_div_test.html", true, 0, &client)); > + webViewImpl->enableFixedLayoutMode(true); > + webViewImpl->setDeviceScaleFactor(2.0f); > + > + webViewImpl->resize(WebSize(viewportWidth, viewportHeight)); > + webViewImpl->setPageScaleFactorLimits(0.01, 4); > + webViewImpl->mainFrameImpl()->frameView()->layout(); Do we need to do all of this through the Impl class? We should do as much through the API as possible.

Comment on attachment 166045 [details] Patch I'm not super excited about adding more bools to WebViewImpl. That class is super, super complicated already. Mostly the comments below are just nitpicks. The patch looks fine in general.

Created attachment 166081 [details] Patch

Comment on attachment 166081 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166081&action=review > Source/WebKit/chromium/src/WebViewImpl.cpp:751 > + m_client->cancelScheduledContentIntents(); > + animateZoomAroundPoint(WebPoint(event.x, event.y), DoubleTap); > + return true; So, we don't call handleGestureEvent event anymore? Also, we don't call m_client->didHandleGestureEvent either. At the least, I think we should call m_client->didHandleGestureEvent with eventSwallowed == true. > Source/WebKit/chromium/tests/WebFrameTest.cpp:346 > + WebViewImpl* webViewImpl = static_cast<WebViewImpl*>(FrameTestHelpers::createWebViewAndLoad(m_baseURL + "get_scale_for_auto_zoom_into_div_test.html", true, 0, &client)); > + webViewImpl->enableFixedLayoutMode(true); > + webViewImpl->setDeviceScaleFactor(2.0f); > + > + webViewImpl->resize(WebSize(viewportWidth, viewportHeight)); > + webViewImpl->setPageScaleFactorLimits(0.01, 4); > + webViewImpl->mainFrameImpl()->frameView()->layout(); I'm pretty sure some of these things can be done through the API. Maybe you responded to my earlier comment and I missed it?

Sorry, Adam. I seem to have missed your previous comment. Checking to see how I can do some of the initialization through the API. (In reply to comment #23) > (From update of attachment 166081 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=166081&action=review > > > Source/WebKit/chromium/src/WebViewImpl.cpp:751 > > + m_client->cancelScheduledContentIntents(); > > + animateZoomAroundPoint(WebPoint(event.x, event.y), DoubleTap); > > + return true; > > So, we don't call handleGestureEvent event anymore? Also, we don't call m_client->didHandleGestureEvent either. At the least, I think we should call m_client->didHandleGestureEvent with eventSwallowed == true. > > > Source/WebKit/chromium/tests/WebFrameTest.cpp:346 > > + WebViewImpl* webViewImpl = static_cast<WebViewImpl*>(FrameTestHelpers::createWebViewAndLoad(m_baseURL + "get_scale_for_auto_zoom_into_div_test.html", true, 0, &client)); > > + webViewImpl->enableFixedLayoutMode(true); > > + webViewImpl->setDeviceScaleFactor(2.0f); > > + > > + webViewImpl->resize(WebSize(viewportWidth, viewportHeight)); > > + webViewImpl->setPageScaleFactorLimits(0.01, 4); > > + webViewImpl->mainFrameImpl()->frameView()->layout(); > > I'm pretty sure some of these things can be done through the API. Maybe you responded to my earlier comment and I missed it?

Created attachment 166100 [details] Patch

Looks like we haven't made these changes downstream yet. Fixing the patch with your recommendations. (In reply to comment #23) > (From update of attachment 166081 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=166081&action=review > > > Source/WebKit/chromium/src/WebViewImpl.cpp:751 > > + m_client->cancelScheduledContentIntents(); > > + animateZoomAroundPoint(WebPoint(event.x, event.y), DoubleTap); > > + return true; > > So, we don't call handleGestureEvent event anymore? Also, we don't call m_client->didHandleGestureEvent either. At the least, I think we should call m_client->didHandleGestureEvent with eventSwallowed == true. > I'm pretty sure some of these things can be done through the API. Maybe you responded to my earlier comment and I missed it?

Comment on attachment 166100 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166100&action=review This looks fine. In general, we prefer for tests to use the API whenever possible so that our tests don't impose constraints on how WebCore evolves. I'm deferring to aelias for the actual subject matter of the patch. Most of my review comments have been about the structure of the code. > Source/WebKit/chromium/src/WebViewImpl.cpp:751 > + eventSwallowed = true; I guess the model here is that we're always going to swallow the double-tap gesture and never pass it along. The other option is always pass it along and then call animateZoomAroundPoint if no one else swallows the event. It's not entirely clear to me which is better. I guess we can try this for now and then starting passing the event along later. > Source/WebKit/chromium/src/WebViewImpl.h:570 > + // Exposed for testing purposes. This comment isn't really needed anymore. > Source/WebKit/chromium/tests/WebFrameTest.cpp:329 > + webViewImpl->mainFrameImpl()->frameView()->layout(); How does this differ from WebView::layout? I'm not sur eI understand why we need to pierce the API here.

Created attachment 166273 [details] Patch

Comment on attachment 166273 [details] Patch Thanks for make the tests use more of the API.

Comment on attachment 166273 [details] Patch Rejecting attachment 166273 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 ERROR: /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://queues.webkit.org/results/14062399

I don't know what's causing that error, but it sure is annoying. :(

Comment on attachment 166273 [details] Patch Rejecting attachment 166273 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 ERROR: /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://queues.webkit.org/results/14070325

Do I need to change the ChangeLog to something else than I currently have? (In reply to comment #32) > (From update of attachment 166273 [details]) > Rejecting attachment 166273 [details] from commit-queue. > > Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 > > ERROR: /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). > > Full output: http://queues.webkit.org/results/14070325

Comment on attachment 166273 [details] Patch It looks fine to me. Let's whack it again.

Comment on attachment 166273 [details] Patch Clearing flags on attachment: 166273 Committed r129928: <http://trac.webkit.org/changeset/129928>

All reviewed patches have been landed. Closing bug.

(In reply to comment #36) > All reviewed patches have been landed. Closing bug. fyi, I changed EXPECT_EQ(true/false, ...) to EXPECT_TRUE/FALSE(...) as clang was unhappy about the former: http://trac.webkit.org/changeset/130141