Bug 72030 – [Qt] the QQuickWebView should support double-tap to zoom

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Kenneth Rohde Christiansen]

SSIA

[Kenneth Rohde Christiansen]

Created attachment 114510 [details]
Patch

[Zeno Albisser]

Comment on attachment 114510 [details]
Patch

looks awesome to me :)

[Kenneth Rohde Christiansen]

Created attachment 114515 [details]
Rebased

[Andreas Kling]

Comment on attachment 114515 [details]
Rebased

View in context: https://bugs.webkit.org/attachment.cgi?id=114515&action=review

r=me with some cool stories, bro.

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:251
> +    if (scrollAnimationActive() || scaleAnimationActive())
> +        return;
> +
> +    if (!targetArea.isValid())
> +        return;

Nit: I'd put the !targetArea.isValid() check first, since it's inline.

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:253
> +    int margin = 10; // We want at least a little bit or margin.

This variable should be const.

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:273
> +    QRectF endPosRange = computePosRangeForItemAtScale(endItemScale);

endPositionRange would be a better name I think.
Actually, thinking about it, I personally prefer "final" to "end" in this context, but maybe "end" is an existing QtQuick convention. Regardless, NABD.

[Kenneth Rohde Christiansen]

(In reply to comment #4)
> (From update of attachment 114515 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=114515&action=review
> 
> r=me with some cool stories, bro.
> 
> > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:251
> > +    if (scrollAnimationActive() || scaleAnimationActive())
> > +        return;
> > +
> > +    if (!targetArea.isValid())
> > +        return;
> 
> Nit: I'd put the !targetArea.isValid() check first, since it's inline.

Good point

> > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:253
> > +    int margin = 10; // We want at least a little bit or margin.
> 
> This variable should be const.

Sure
 
> > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:273
> > +    QRectF endPosRange = computePosRangeForItemAtScale(endItemScale);
> 
> endPositionRange would be a better name I think.

It is, but Pos was choosed to be similar to the QQuick API :-) setPosRange() *hint*

> Actually, thinking about it, I personally prefer "final" to "end" in this context, but maybe "end" is an existing QtQuick convention. Regardless, NABD.

Yeah that is used all over in the API (and in our code)

[Kenneth Rohde Christiansen]

Comment on attachment 114515 [details]
Rebased

Landed in 100125