[Chromium] Add method to WebViewImpl to extract zoom/scroll params for gesture events on touch devices
Created attachment 116127 [details] Patch
Created attachment 116128 [details] Patch
Comment on attachment 116128 [details] Patch Attachment 116128 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10475352
Created attachment 116153 [details] Patch
Comment on attachment 116153 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116153&action=review > Source/WebCore/ChangeLog:8 > + No new tests. (OOPS!) this is a problem. there's a reasonable amount of non-trivial logic in here and it needs to be covered by some tests > Source/WebCore/platform/PlatformGestureEvent.h:59 > + , m_scale(0.0f) we just say '0' for constants like this in WebKit. See http://www.webkit.org/coding/coding-style.html > Source/WebCore/platform/PlatformGestureEvent.h:85 > + void setDeltaX(float deltaX) { m_deltaX = deltaX; } > + void setDeltaY(float deltaY) { m_deltaY = deltaY; } > + void setScale(float scale) { m_scale = scale; } > + this is somewhat weird - most of these PlatformXEvent types are wrappers around native events and only provide getters. where do we need to manipulate a PlatformGestureEvent directly? > Source/WebKit/chromium/src/WebViewImpl.cpp:193 > +static const float kMinDoubleTapZoomInRatio = 1.33f; > +static const float kMinScaleDifference = 0.01f; trailing f's are not very useful here where are these numbers coming from? can you add some comments indicating how these values were arrived at and what the process would be to change them? > Source/WebKit/chromium/src/WebViewImpl.cpp:792 > + for (unsigned int i = 0; i < gestureEvents->size(); i++) { double space here between 'int' and 'i' we don't typically use 'unsigned int' as a type for iteration. 'unsigned' or size_t please > Source/WebKit/chromium/src/WebViewImpl.cpp:794 > + float scale = 1.f; kill the .1f > Source/WebKit/chromium/src/WebViewImpl.cpp:816 > + IntPoint point = mainFrameImpl()->frameView()->windowToContents(WebCore::IntPoint(rect.x, rect.y)); the WebCore:: prefix isn't needed here > Source/WebKit/chromium/src/WebViewImpl.cpp:828 > + IntRect rect = node->Node::getRect(); // workaround ContainerNode::getRect transformation bug what bug is this talking about? can you add a link to it and a FIXME? > Source/WebKit/chromium/src/WebViewImpl.cpp:839 > + return 0.0f; kill the .0f > Source/WebKit/chromium/src/WebViewImpl.cpp:843 > + minScale = std::max(minScale, (float) m_size.width / (contentSize.width / pageScaleFactor())); drop the std:: > Source/WebKit/chromium/src/WebViewImpl.cpp:848 > +void WebViewImpl::getScaleAndScrollForHitRect(const WebRect& hitRect, bool forDoubleTap, float* scale, WebPoint* scroll) is it valid for 'scale' to be NULL? if not can you make this a reference instead of a pointer? same for scroll > Source/WebKit/chromium/src/WebViewImpl.cpp:865 > + const float minZoomInRatio = forDoubleTap ? kMinDoubleTapZoomInRatio : 1.0f; .0f :( > Source/WebKit/chromium/src/WebViewImpl.cpp:875 > + *scale *= static_cast<float>(m_size.width) / rect.width; are you sure this is the correct rounding? > Source/WebKit/chromium/src/WebViewImpl.cpp:876 > + *scale = std::min(std::max(*scale, minScale), maxScale); drop std:: > Source/WebKit/chromium/src/WebViewImpl.cpp:910 > + rect.y = std::max((float) rect.y, hitRect.y + kTouchPointPadding - screenHeight); drop std:: > Source/WebKit/chromium/src/WebViewImpl.cpp:917 > + rect.x = std::max((float) rect.x, hitRect.x + kTouchPointPadding - screenWidth); drop the std::. there's a using namespace std; at the top of this file > Source/WebKit/chromium/src/WebViewImpl.h:478 > + // Returns the bounding box of the block type node touched by the input > + // point with the padding. > + WebRect getBlockBounds(const WebRect&, bool ignoreClipping); we don't use the 'get' prefix very much in WebKit. a more idomatic name for this might be 'calculateBlockBounds', or perhaps 'compute' to be consistent with the following function
Created attachment 125365 [details] Patch
(In reply to comment #6) > Created an attachment (id=125365) [details] > Patch Please hold off review... still working on writing tests
Andrei, John, could you take a look at this latest patch? This upstreams code that you originally worked on.
Created attachment 125605 [details] Patch
Comment on attachment 125605 [details] Patch I only have some style nits: View in context: https://bugs.webkit.org/attachment.cgi?id=125605&action=review > Source/WebKit/chromium/ChangeLog:13 > + (WebKit::WebViewImpl::WidenRectWithinPageBounds): any reason why this starts with a capital letter? > Source/WebKit/chromium/src/WebViewImpl.cpp:818 > + PlatformGestureEvent& oldPge = gestureEvents->at(i); What's the "old" prefix meant to say? > Source/WebKit/chromium/src/WebViewImpl.cpp:914 > +void WebViewImpl::computeScaleAndScrollForHitRect(const WebRect& hitRect, bool forDoubleTap, float& scale, WebPoint& scroll) Here and in some other places: WebKit frowns on bool args. Can we use an enum? https://lists.webkit.org/pipermail/webkit-dev/2010-December/015192.html > Source/WebKit/chromium/src/WebViewImpl.cpp:934 > + const float maxScale = deviceScaleFactor() * 1 /* font scale factor */; Do we need the trailing comment? > Source/WebKit/chromium/src/WebViewImpl.cpp:946 > + static_cast<int>(minimumMargin * rect.width / m_size.width)); Weird indent...should be 8 spaces I think?
Created attachment 125650 [details] Patch
Created attachment 125654 [details] Patch
LGTM (In reply to comment #12) > Created an attachment (id=125654) [details] > Patch
Created attachment 125659 [details] Patch
Comment on attachment 125659 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=125659&action=review I thought that the WebKit gesture recognizer in chromium was deprecated and due to be deleted soon - I'm very surprised to see new code using it. What's the story? > Source/WebCore/platform/PlatformGestureEvent.h:47 > + PlatformGestureEvent(Type type, const IntPoint& position, const IntPoint& globalPosition, double timestamp, float deltaX, float deltaY, bool shiftKey, bool ctrlKey, bool altKey, bool metaKey, float scale = 0.0f) we don't use qualifiers like ".0f" in WebKit. See "Floating point literals" in http://www.webkit.org/coding/coding-style.html what does it mean for a gesture event to have a scale? is this different from the page scale when the event happens? > Source/WebKit/chromium/src/WebViewImpl.cpp:202 > +static const float kDoubleTapZoomContentDefaultMargin = 5.0; > +static const float kDoubleTapZoomContentMinimumMargin = 2.0; don't append the .0s here
Comment on attachment 125659 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=125659&action=review > Source/WebKit/chromium/src/WebViewImpl.h:504 > + enum BoostedTextType { > + NormalText, > + BoostedText, > + }; this looks like an unrelated change
Created attachment 125679 [details] Patch
(In reply to comment #15) > (From update of attachment 125659 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=125659&action=review > > I thought that the WebKit gesture recognizer in chromium was deprecated and due to be deleted soon - I'm very surprised to see new code using it. What's the story? You are correct.. I should be putting this under ENABLE(GESTURE_EVENTS) rather than GESTURE_RECOGNIZER. Fixed. > > > Source/WebCore/platform/PlatformGestureEvent.h:47 > > + PlatformGestureEvent(Type type, const IntPoint& position, const IntPoint& globalPosition, double timestamp, float deltaX, float deltaY, bool shiftKey, bool ctrlKey, bool altKey, bool metaKey, float scale = 0.0f) > > we don't use qualifiers like ".0f" in WebKit. See "Floating point literals" in http://www.webkit.org/coding/coding-style.html Done. > > what does it mean for a gesture event to have a scale? is this different from the page scale when the event happens? For certain gestures such as Double tap or pinch to zoom, one of the attributes is scale. > > > Source/WebKit/chromium/src/WebViewImpl.cpp:202 > > +static const float kDoubleTapZoomContentDefaultMargin = 5.0; > > +static const float kDoubleTapZoomContentMinimumMargin = 2.0; > > don't append the .0s here Done.
(In reply to comment #16) > (From update of attachment 125659 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=125659&action=review > > > Source/WebKit/chromium/src/WebViewImpl.h:504 > > + enum BoostedTextType { > > + NormalText, > > + BoostedText, > > + }; > > this looks like an unrelated change It is used in the computation of block bounds to zoom to.
Created attachment 125707 [details] Patch
Comment on attachment 125707 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=125707&action=review > Source/WebCore/platform/PlatformGestureEvent.h:82 > + float m_scale; I am concerned about this. startPageScaleAnimation (the ultimate target of what you're constructing) needs a point (existing field) and a scale (deltaX by convention) > Source/WebKit/chromium/src/WebViewImpl.cpp:632 > + if (event.type == WebInputEvent::GestureDoubleTap) { I think this could be better. In particular, the end goal is for if this WGE is a doubletap, compute the arguments needed for startPageScaleAnimation forward these arguments to the impl thread else construct a PlatformGestureEvent and sent it to the EventHandler::handleGestureEvent Since https://bugs.webkit.org/show_bug.cgi?id=77872 is still underway, this code for the time being should being construct a PlatformGestureEvent as would be consumed by ScrollAnimatorNone::zoom > Source/WebKit/chromium/src/WebViewImpl.cpp:906 > +void WebViewImpl::computeScaleAndScrollForHitRect(const WebRect& hitRect, AutoZoomType zoomType, float& scale, WebPoint& scroll) is there any way to make this whole function run on the impl thread? it would be a huge gain I think. > Source/WebKit/chromium/src/WebViewImpl.cpp:994 > +bool WebViewImpl::dispatchGestureEvent(PlatformGestureEvent& pge) because dispatchGestureEvent implies something specific in EventHandler, I'd prefer a different name. > Source/WebKit/chromium/src/WebViewImpl.h:553 > + // Returns the bounding box of the block type node touched by the input Aren't comments like these are atypical in WebCore? nit: need a period at the end.
I also have concerns about the event handling stuff, I think we should avoid adding fields of any kind to the event object. I wrote more detailed comments at https://bugs.webkit.org/show_bug.cgi?id=77872 . I think the event stuff could be removed from this CL and dealt with separately. Another minor concern: the "+ 12" margin in the unit test seems too specific and brittle. How about for things involving the margin, use EXPECT_NEAR(pageWidth / divWidth, scale, 0.1)? That would be more robust to minor changes in the margin logic.
(In reply to comment #21) > > Source/WebKit/chromium/src/WebViewImpl.cpp:906 > > +void WebViewImpl::computeScaleAndScrollForHitRect(const WebRect& hitRect, AutoZoomType zoomType, float& scale, WebPoint& scroll) > > is there any way to make this whole function run on the impl thread? it would be a huge gain I think. Unfortunately, no. The impl thread only knows about the layer tree, whereas this needs detailed information from the render tree. We could precompute the rects and push it across the thread boundary, but this will be nontrivial to get right (Android browser did it and they spent a long time struggling with corner cases).
(In reply to comment #23) > (In reply to comment #21) > > > Source/WebKit/chromium/src/WebViewImpl.cpp:906 > > > +void WebViewImpl::computeScaleAndScrollForHitRect(const WebRect& hitRect, AutoZoomType zoomType, float& scale, WebPoint& scroll) > > > > is there any way to make this whole function run on the impl thread? it would be a huge gain I think. > > Unfortunately, no. The impl thread only knows about the layer tree, whereas this needs detailed information from the render tree. We could precompute the rects and push it across the thread boundary, but this will be nontrivial to get right (Android browser did it and they spent a long time struggling with corner cases). Yes.. moving this to the impl thread would be very hard. We might be able to get everything but computeBlockBounds on to the impl thread but I think that would be useless because computeBlockBounds is the only expensive operation here.
Created attachment 125731 [details] Patch
Hi All.... I have removed the gesture events related code from this CL. I think this code can be added in https://bugs.webkit.org/show_bug.cgi?id=77872 which actually adds support for the double tap gesture. PTAL.
ping
> ping https://lists.webkit.org/pipermail/webkit-dev/2012-January/019023.html
LGTM except the 12.0 number in the unit test I mentioned earlier.
(In reply to comment #29) > LGTM except the 12.0 number in the unit test I mentioned earlier. Hi Alexandre... the margins are all very well defined. The logic of including the margins is also deterministic. So I am not sure why the test would be brittle. Wouldnt it be better to have these parameters tested out exactly?
It's not a huge deal. But personally, I prefer to avoid overspecific tests, particularly ones with nonobvious, semi-arbitrary parameters. If someone made a minor change that changed the margin to e.g. 14.0, that's not necessarily incorrect and it's unnecessary for the test to go red.
(In reply to comment #31) > It's not a huge deal. But personally, I prefer to avoid overspecific tests, particularly ones with nonobvious, semi-arbitrary parameters. If someone made a minor change that changed the margin to e.g. 14.0, that's not necessarily incorrect and it's unnecessary for the test to go red. I think it is fair to require that legit changes to parameters also update the test. The test is there to protect against unrelated/unintentional changes. For example, it would be bad if the block bounds to zoom to are off by 5 pixels due to some unrelated CL. I guess this is a matter of personal opinion.. I'll remove it if other also have hard feelings against :)
(In reply to comment #32) > (In reply to comment #31) > > It's not a huge deal. But personally, I prefer to avoid overspecific tests, particularly ones with nonobvious, semi-arbitrary parameters. If someone made a minor change that changed the margin to e.g. 14.0, that's not necessarily incorrect and it's unnecessary for the test to go red. > > I think it is fair to require that legit changes to parameters also update the test. The test is there to protect against unrelated/unintentional changes. For example, it would be bad if the block bounds to zoom to are off by 5 pixels due to some unrelated CL. > I guess this is a matter of personal opinion.. I'll remove it if other also have hard feelings against :) @jamesr: I think everyone is ok with the current patch.. please review.
This all looks good to me. varunjain@ could you add a bug for the changes needed to ::gestureEvent so that detail doesn't get overlooked?
LGTM, thanks.
Comment on attachment 125731 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=125731&action=review I'm glad you have tests. However, there's a lot more logic here than tests and not all of it is very clear. > Source/WebKit/chromium/src/WebViewImpl.cpp:202 > +static const int kTouchPointPadding = 32; > +static const float kMinScaleDifference = 0.01; > +static const float kDoubleTapZoomContentDefaultMargin = 5; > +static const float kDoubleTapZoomContentMinimumMargin = 2; this is incorrect naming style for WebKit - in WebKit, constants are named just like normal variables (no "k" prefix) > Source/WebKit/chromium/src/WebViewImpl.cpp:835 > + // Find the most boosted text node intersected by the hit test (if any). this looks like a great candidate for being moved into its own function and tested separately > Source/WebKit/chromium/src/WebViewImpl.cpp:943 > + // Fit block to screen, respecting limits. > + if (textType == NormalText) > + scale = maxScale; > + else { > + scale *= static_cast<float>(m_size.width) / rect.width; > + scale = min(scale, maxScale); > + } I'm not sure why this logic makes sense - wouldn't you want to fix the block the same way no matter what the text size is? I'm also pretty sure this bit of logic is not covered by your tests. > Source/WebKit/chromium/src/WebViewImpl.cpp:985 > + rect.x = max((float) rect.x, hitRect.x + kTouchPointPadding - screenWidth); we never use c-style casts in webkit. use static_cast<> if you need to, or max<float>() to avoid it > Source/WebKit/chromium/tests/WebFrameTest.cpp:155 > +TEST_F(WebFrameTest, DivAutoZoomParamsTest) there's a lot more logic in WebViewImpl than there are test cases here. I can't find any tests for the logic to find the text with the largest font size, for example. > Source/WebKit/chromium/tests/WebFrameTest.cpp:184 > + EXPECT_FLOAT_EQ(pageWidth / (divWidth + 12.0 /* margin */), scale); where does 12 come from? that's a pretty bizarre number to have in the middle of this test and I can't find it anywhere else
Created attachment 126521 [details] Patch
(In reply to comment #36) > (From update of attachment 125731 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=125731&action=review > > I'm glad you have tests. However, there's a lot more logic here than tests and not all of it is very clear. added more tests. > > > Source/WebKit/chromium/src/WebViewImpl.cpp:202 > > +static const int kTouchPointPadding = 32; > > +static const float kMinScaleDifference = 0.01; > > +static const float kDoubleTapZoomContentDefaultMargin = 5; > > +static const float kDoubleTapZoomContentMinimumMargin = 2; > > this is incorrect naming style for WebKit - in WebKit, constants are named just like normal variables (no "k" prefix) Done. > > > Source/WebKit/chromium/src/WebViewImpl.cpp:835 > > + // Find the most boosted text node intersected by the hit test (if any). > > this looks like a great candidate for being moved into its own function and tested separately extracted it out to another function. But I want to keep it private, so I cannot test this separately. But it should not matter because the new tests I added cover all of this code. > > > Source/WebKit/chromium/src/WebViewImpl.cpp:943 > > + // Fit block to screen, respecting limits. > > + if (textType == NormalText) > > + scale = maxScale; > > + else { > > + scale *= static_cast<float>(m_size.width) / rect.width; > > + scale = min(scale, maxScale); > > + } > > I'm not sure why this logic makes sense - wouldn't you want to fix the block the same way no matter what the text size is? I think the goal here is that if the hit rect contains a text node that has unscaled fonts then we want to zoom to device scale factor atleast so that we are guaranteed to get legible font size. If the rect already contains boosted fonts then we simply zoom enough to bring the div width equal to page width. > > I'm also pretty sure this bit of logic is not covered by your tests. Added tests to cover specifically this part. > > > Source/WebKit/chromium/src/WebViewImpl.cpp:985 > > + rect.x = max((float) rect.x, hitRect.x + kTouchPointPadding - screenWidth); > > we never use c-style casts in webkit. use static_cast<> if you need to, or max<float>() to avoid it Done. > > > Source/WebKit/chromium/tests/WebFrameTest.cpp:155 > > +TEST_F(WebFrameTest, DivAutoZoomParamsTest) > > there's a lot more logic in WebViewImpl than there are test cases here. I can't find any tests for the logic to find the text with the largest font size, for example. Added test. > > > Source/WebKit/chromium/tests/WebFrameTest.cpp:184 > > + EXPECT_FLOAT_EQ(pageWidth / (divWidth + 12.0 /* margin */), scale); > > where does 12 come from? that's a pretty bizarre number to have in the middle of this test and I can't find it anywhere else Removed the margin and using EXPECT_NEAR as aelias suggested :)
(In reply to comment #34) > This all looks good to me. > > varunjain@ could you add a bug for the changes needed to ::gestureEvent so that detail doesn't get overlooked? created https://bugs.webkit.org/show_bug.cgi?id=78363
Comment on attachment 126521 [details] Patch Attachment 126521 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11447746
Created attachment 126531 [details] Patch
(In reply to comment #40) > (From update of attachment 126521 [details]) > Attachment 126521 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/11447746 new patch fixes build failure
I'm not sure how we plan to proceed with the font boosting code yet and would like to consult with some people first about what our plan is. Without some idea of what the big picture is I don't feel comfortable approving bits and pieces of it. If you think there's a useful subset of this patch without that feel free to post it and I'd be happy to review. Otherwise this will have to wait.
We probably want to handle the font boosting related code separately. It's likely going to need some reworking before we land it.
Comment on attachment 126531 [details] Patch Clearing review? flag from this
Created attachment 127023 [details] Patch
(In reply to comment #43) > I'm not sure how we plan to proceed with the font boosting code yet and would like to consult with some people first about what our plan is. Without some idea of what the big picture is I don't feel comfortable approving bits and pieces of it. > > If you think there's a useful subset of this patch without that feel free to post it and I'd be happy to review. Otherwise this will have to wait. Hi James.. I have removed the font scaling related bits. PTAL.
(In reply to comment #47) > (In reply to comment #43) > > I'm not sure how we plan to proceed with the font boosting code yet and would like to consult with some people first about what our plan is. Without some idea of what the big picture is I don't feel comfortable approving bits and pieces of it. > > > > If you think there's a useful subset of this patch without that feel free to post it and I'd be happy to review. Otherwise this will have to wait. > > Hi James.. > I have removed the font scaling related bits. PTAL. @jamesr: friendly reminder.. PTAL.
Comment on attachment 127023 [details] Patch OK
Comment on attachment 127023 [details] Patch Clearing flags on attachment: 127023 Committed r108027: <http://trac.webkit.org/changeset/108027>
All reviewed patches have been landed. Closing bug.