[Webkit-unassigned] [Bug 112756] [chromium] Improve mobile device rotation behavior

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=112756





--- Comment #3 from Alexandre Elias <aelias at chromium.org>  2013-03-20 21:38:40 PST ---
(From update of attachment 193956)
View in context: https://bugs.webkit.org/attachment.cgi?id=193956&action=review

> Source/WebKit/chromium/src/WebViewImpl.cpp:218
> +// point (in CSS pixels). The anchor point tracks a graphical node in the

nit: remove the word "graphical", that's not usual WebKit jargon.  It's a DOM node.

> Source/WebKit/chromium/src/WebViewImpl.cpp:224
> +class PointAnchor {

This name is a bit vague, how about calling it "HitTestAnchor"?

> Source/WebKit/chromium/src/WebViewImpl.cpp:226
> +    PointAnchor(IntPoint point, IntPoint anchor, IntSize size, IntSize viewSize, EventHandler* eventHandler)

Please delete "IntPoint point" and "IntSize size" as you aren't using them for anything.

> Source/WebKit/chromium/src/WebViewImpl.cpp:266
> +    void initialize(IntSize size, IntSize viewSize, EventHandler* eventHandler)

Please put this code directly in the constructor, private "initialize()" is not an idiom in chromium or webkit.

> Source/WebKit/chromium/src/WebViewImpl.cpp:268
> +        // A point at the screen origin will always be preserved

Why?  What's special about the screen origin?

> Source/WebKit/chromium/src/WebViewImpl.cpp:273
> +        if (!size.width() || !size.height() || !viewSize.width() || !viewSize.height())

Use IntSize::IsEmpty()

> Source/WebKit/chromium/src/WebViewImpl.cpp:279
> +        if (node && !node->isTextNode()) {

What's special about text nodes?  Shouldn't we anchor to any node?

> Source/WebKit/chromium/src/WebViewImpl.cpp:289
> +        IntRect bounds = node->Node::pixelSnappedBoundingBox();

If possible, please switch to using boundingBox() and LayoutRect to avoid losing precision.

> Source/WebKit/chromium/src/WebViewImpl.cpp:290
> +        // sites like nytimes.com insert a non-standard tag <nyt_text>

No need to mention this example.

> Source/WebKit/chromium/src/WebViewImpl.cpp:293
> +        if (!bounds.width()) {

You really want a for loop going up the tree until you find a non-zero size node, right?  Also, you should check both the width and height using IsEmpty().

> Source/WebKit/chromium/src/WebViewImpl.cpp:304
> +        m_xPercentInDoc = !bounds.width()  ? 0 : (float) (m_anchor.x() - bounds.x()) / bounds.width();

Take a look at the operators in FloatPoint.h for how to do these on both x and y in one function.

> Source/WebKit/chromium/src/WebViewImpl.cpp:307
> +        m_xPercentInView = (float) (m_anchor.x() - m_point.x()) / viewSize.width();

You're always setting m_anchor and m_point to the same value.  Is this unnecessary complexity?

> Source/WebKit/chromium/src/WebViewImpl.cpp:314
> +    float m_xPercentInDoc, m_yPercentInDoc;

Please use WebCore::FloatPoint for these.

> Source/WebKit/chromium/src/WebViewImpl.cpp:1808
> +    // At some point, it may become desirable to make the top center of the view

Let's change it to the top center now, we'll never get around to it otherwise :).

> Source/WebKit/chromium/src/WebViewImpl.cpp:1847
> +        newScrollPoint = clampOffsetAtScale(newScrollPoint, oldPageScaleFactor);

Using oldPageScaleFactor here is weird and makes the clamp somewhat pointless.  You may want to put this clamp below scaleMultiplier calculation and use the final value.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.