Bug 112756 – [chromium] Improve mobile device rotation behavior

• Component: New Bugs
• Status: RESOLVED WONTFIX

[chromium] Improve mobile device rotation behavior

Created attachment 193954 [details]
Patch

Created attachment 193956 [details]
Patch

[Alexandre Elias]

Comment on attachment 193956 [details]
Patch

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.

Comment on attachment 193956 [details]
Patch

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.

Got it.

>> Source/WebKit/chromium/src/WebViewImpl.cpp:224
>> +class PointAnchor {
> 
> This name is a bit vague, how about calling it "HitTestAnchor"?

Agreed.

>> 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.

"size" was used as an early-out if the screen is empty, but I suppose that's redundant with subsequent bounds checks. Revmoed.

"point" is used when the anchor is not the actual point of interest; e.g., we care about the scroll offset (top left of the view), but we want the anchor at the top center of the view.

>> 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.

Sure.

>> 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?

Nothing, really.  However, I can see the merits in maintaining a point of interest (scroll offset) at the origin as the screen is resized.  This won't necessarily happen if we remove this check.  Thoughts?

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

Done.

>> Source/WebKit/chromium/src/WebViewImpl.cpp:279
>> +        if (node && !node->isTextNode()) {
> 
> What's special about text nodes?  Shouldn't we anchor to any node?

Much of this code is a transliteration from the original Android WebView code, and I tried to maintain similar logic.  In their case, I imagine text reflow was the primary reason to anchor to a text node on resize.  I can remove this.

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

Gone.

>> 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().

Right.  Again, this code was a rough transliteration from Android, but that logic makes more sense.

>> 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.

Right.

>> 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?

Currently, these points coincide, but that's only because I have the anchor set to the top left, the same as the scroll offset.  If we shift the anchor to the top center, these will be different.

>> Source/WebKit/chromium/src/WebViewImpl.cpp:314
>> +    float m_xPercentInDoc, m_yPercentInDoc;
> 
> Please use WebCore::FloatPoint for these.

Done.

>> 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 :).

I've had much better results anchoring to the top left.  This may simply be a function of 

1) When I rotate the screen, it's typically when I'm reading and 
2) I read in a left-to-right, top-to-bottom language.  

Do we have any more formal guidelines on this?

>> 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.

Done. setPageScaleFactor already clamps the offset, so it's really only necessary for the manual updateMainFrameScrollPosition.

Created attachment 194931 [details]
Patch

Created attachment 195987 [details]
Patch

Alright, the latest update preserves behavior for desktop sites, while properly re-scaling mobile sites that lack fixed page scale limits.

[Peter Beverloo]

Resolving as WontFix given that Chromium moved to Blink.