Bug 72909 - [Chromium] Add method to WebViewImpl to extract zoom/scroll params for gesture events on touch devices
Summary: [Chromium] Add method to WebViewImpl to extract zoom/scroll params for gestur...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Varun Jain
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-21 13:35 PST by Varun Jain
Modified: 2012-02-16 21:34 PST (History)
12 users (show)

See Also:


Attachments
Patch (11.59 KB, patch)
2011-11-21 13:37 PST, Varun Jain
no flags Details | Formatted Diff | Diff
Patch (10.70 KB, patch)
2011-11-21 13:52 PST, Varun Jain
no flags Details | Formatted Diff | Diff
Patch (10.63 KB, patch)
2011-11-21 16:06 PST, Varun Jain
no flags Details | Formatted Diff | Diff
Patch (15.57 KB, patch)
2012-02-03 10:53 PST, Varun Jain
no flags Details | Formatted Diff | Diff
Patch (18.52 KB, patch)
2012-02-06 02:47 PST, Varun Jain
no flags Details | Formatted Diff | Diff
Patch (18.97 KB, patch)
2012-02-06 08:18 PST, Varun Jain
no flags Details | Formatted Diff | Diff
Patch (19.18 KB, patch)
2012-02-06 08:31 PST, Varun Jain
no flags Details | Formatted Diff | Diff
Patch (19.28 KB, patch)
2012-02-06 08:56 PST, Varun Jain
no flags Details | Formatted Diff | Diff
Patch (19.33 KB, patch)
2012-02-06 11:41 PST, Varun Jain
no flags Details | Formatted Diff | Diff
Patch (20.74 KB, patch)
2012-02-06 14:20 PST, Varun Jain
no flags Details | Formatted Diff | Diff
Patch (15.54 KB, patch)
2012-02-06 16:51 PST, Varun Jain
no flags Details | Formatted Diff | Diff
Patch (19.23 KB, patch)
2012-02-10 09:13 PST, Varun Jain
no flags Details | Formatted Diff | Diff
Patch (19.25 KB, patch)
2012-02-10 10:17 PST, Varun Jain
no flags Details | Formatted Diff | Diff
Patch (14.41 KB, patch)
2012-02-14 12:55 PST, Varun Jain
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Varun Jain 2011-11-21 13:35:27 PST
[Chromium] Add method to WebViewImpl to extract zoom/scroll params for gesture events on touch devices
Comment 1 Varun Jain 2011-11-21 13:37:46 PST
Created attachment 116127 [details]
Patch
Comment 2 Varun Jain 2011-11-21 13:52:52 PST
Created attachment 116128 [details]
Patch
Comment 3 WebKit Review Bot 2011-11-21 13:59:25 PST
Comment on attachment 116128 [details]
Patch

Attachment 116128 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10475352
Comment 4 Varun Jain 2011-11-21 16:06:15 PST
Created attachment 116153 [details]
Patch
Comment 5 James Robinson 2011-11-27 11:03:41 PST
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
Comment 6 Varun Jain 2012-02-03 10:53:12 PST
Created attachment 125365 [details]
Patch
Comment 7 Varun Jain 2012-02-03 10:56:29 PST
(In reply to comment #6)
> Created an attachment (id=125365) [details]
> Patch

Please hold off review... still working on writing tests
Comment 8 Alexandre Elias 2012-02-03 11:24:42 PST
Andrei, John, could you take a look at this latest patch?  This upstreams code that you originally worked on.
Comment 9 Varun Jain 2012-02-06 02:47:18 PST
Created attachment 125605 [details]
Patch
Comment 10 Andrei Popescu 2012-02-06 06:11:54 PST
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?
Comment 11 Varun Jain 2012-02-06 08:18:28 PST
Created attachment 125650 [details]
Patch
Comment 12 Varun Jain 2012-02-06 08:31:25 PST
Created attachment 125654 [details]
Patch
Comment 13 Andrei Popescu 2012-02-06 08:36:09 PST
LGTM

(In reply to comment #12)
> Created an attachment (id=125654) [details]
> Patch
Comment 14 Varun Jain 2012-02-06 08:56:26 PST
Created attachment 125659 [details]
Patch
Comment 15 James Robinson 2012-02-06 10:55:50 PST
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 16 James Robinson 2012-02-06 10:57:21 PST
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
Comment 17 Varun Jain 2012-02-06 11:41:36 PST
Created attachment 125679 [details]
Patch
Comment 18 Varun Jain 2012-02-06 11:43:37 PST
(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.
Comment 19 Varun Jain 2012-02-06 11:44:13 PST
(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.
Comment 20 Varun Jain 2012-02-06 14:20:53 PST
Created attachment 125707 [details]
Patch
Comment 21 Robert Kroeger 2012-02-06 15:15:03 PST
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.
Comment 22 Alexandre Elias 2012-02-06 15:47:35 PST
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.
Comment 23 Alexandre Elias 2012-02-06 15:52:27 PST
(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).
Comment 24 Varun Jain 2012-02-06 16:20:52 PST
(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.
Comment 25 Varun Jain 2012-02-06 16:51:19 PST
Created attachment 125731 [details]
Patch
Comment 26 Varun Jain 2012-02-06 16:54:03 PST
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.
Comment 27 Varun Jain 2012-02-07 13:26:08 PST
ping
Comment 29 Alexandre Elias 2012-02-07 13:32:51 PST
LGTM except the 12.0 number in the unit test I mentioned earlier.
Comment 30 Varun Jain 2012-02-07 13:45:18 PST
(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?
Comment 31 Alexandre Elias 2012-02-07 13:53:57 PST
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.
Comment 32 Varun Jain 2012-02-07 14:37:08 PST
(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 :)
Comment 33 Varun Jain 2012-02-08 10:21:20 PST
(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.
Comment 34 Robert Kroeger 2012-02-08 11:20:40 PST
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?
Comment 35 Alexandre Elias 2012-02-08 11:24:44 PST
LGTM, thanks.
Comment 36 James Robinson 2012-02-08 11:42:07 PST
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
Comment 37 Varun Jain 2012-02-10 09:13:30 PST
Created attachment 126521 [details]
Patch
Comment 38 Varun Jain 2012-02-10 09:19:14 PST
(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 :)
Comment 39 Varun Jain 2012-02-10 09:23:20 PST
(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 40 WebKit Review Bot 2012-02-10 09:51:19 PST
Comment on attachment 126521 [details]
Patch

Attachment 126521 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11447746
Comment 41 Varun Jain 2012-02-10 10:17:25 PST
Created attachment 126531 [details]
Patch
Comment 42 Varun Jain 2012-02-10 10:18:25 PST
(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
Comment 43 James Robinson 2012-02-13 23:57:52 PST
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.
Comment 44 Adam Barth 2012-02-14 00:07:04 PST
We probably want to handle the font boosting related code separately.  It's likely going to need some reworking before we land it.
Comment 45 James Robinson 2012-02-14 10:47:19 PST
Comment on attachment 126531 [details]
Patch

Clearing review? flag from this
Comment 46 Varun Jain 2012-02-14 12:55:06 PST
Created attachment 127023 [details]
Patch
Comment 47 Varun Jain 2012-02-14 12:57:02 PST
(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.
Comment 48 Varun Jain 2012-02-16 10:03:02 PST
(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 49 James Robinson 2012-02-16 20:01:49 PST
Comment on attachment 127023 [details]
Patch

OK
Comment 50 WebKit Review Bot 2012-02-16 21:34:12 PST
Comment on attachment 127023 [details]
Patch

Clearing flags on attachment: 127023

Committed r108027: <http://trac.webkit.org/changeset/108027>
Comment 51 WebKit Review Bot 2012-02-16 21:34:20 PST
All reviewed patches have been landed.  Closing bug.