Bug 83895 – [Qt] Clean up how the interaction engine is making use of ViewportAttributes

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Kenneth Rohde Christiansen]

SSIA

[Kenneth Rohde Christiansen]

Created attachment 137087 [details]
Patch

[zalan]

lgtm. discussed on irc.

[Early Warning System Bot]

Comment on attachment 137087 [details]
Patch

Attachment 137087 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12399333

[Jocelyn Turcotte]

Comment on attachment 137087 [details]
Patch

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

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:628
> +    // FIXME: Revise this.
> +    interactionEngine->reset();

Could you elaborate?

> Source/WebKit2/UIProcess/API/qt/qwebviewportinfo.cpp:96
> +void QWebViewportInfo::didUpdateViewportConstraints(const WebCore::ViewportAttributes& attr)

I don't think we should expose this in the public API

> Source/WebKit2/UIProcess/API/qt/qwebviewportinfo_p.h:70
> +    WebCore::ViewportAttributes attributes;

This should go in a new QWebViewportInfoPrivate

[Kenneth Rohde Christiansen]

Comment on attachment 137087 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:628
>> +    interactionEngine->reset();
> 
> Could you elaborate?

It is the same reset you wanted to remove but left there. So we need to revise it at a later time.

>> Source/WebKit2/UIProcess/API/qt/qwebviewportinfo.cpp:96
>> +void QWebViewportInfo::didUpdateViewportConstraints(const WebCore::ViewportAttributes& attr)
> 
> I don't think we should expose this in the public API

This is not public API as it is not exposed to QML nor to a public header file.

>> Source/WebKit2/UIProcess/API/qt/qwebviewportinfo_p.h:70
>> +    WebCore::ViewportAttributes attributes;
> 
> This should go in a new QWebViewportInfoPrivate

If we are going to create a QWebViewportInfoPrivate I think that should be a separate patch.

[Kenneth Rohde Christiansen]

I do not know why this doesnt build, maybe it needs a clean build. It compiles locally and I had this problem before I changed experimental.pri

[Jocelyn Turcotte]

(In reply to comment #5)
> > Could you elaborate?
> 
> It is the same reset you wanted to remove but left there. So we need to revise it at a later time.

Sorry I meant in the comment.

> 
> >> Source/WebKit2/UIProcess/API/qt/qwebviewportinfo.cpp:96
> >> +void QWebViewportInfo::didUpdateViewportConstraints(const WebCore::ViewportAttributes& attr)
> > 
> > I don't think we should expose this in the public API
> 
> This is not public API as it is not exposed to QML nor to a public header file.
> 

Well it's private public API, so this would prevent anybody from doing "QT += webkit-private" in their project as they would also need all the referenced WebCore headers, which we won't distribute. All the other QML API headers already support this.

The private class is also just to make sure that no WebCore symbols are in those private public headers.


(In reply to comment #6)
> I do not know why this doesnt build, maybe it needs a clean build. It compiles locally and I had this problem before I changed experimental.pri

It doesn't build because moc doesn't include "config.h", so you need to "#include "moc_qwebviewportinfo.cpp" at the end of qwebviewportinfo.cpp. But I think you won't need it if you add the _p_p.h file.

[Kenneth Rohde Christiansen]

Created attachment 137324 [details]
Patch

[Rafael Brandao]

Comment on attachment 137324 [details]
Patch

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

> Source/WebKit2/ChangeLog:8
> +        Refactor now the interaction engine is using the ViewportAttributes

Wonder if you mean "how" instead of "now" here.

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:635
> +    // This needs to be revised at some point.

I believe the "FIXME" already marks the following code as "should be revised at some point", so this paragragh may be omitted.

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:640
> +    interactionEngine->setAllowsUserScaling(!!attr.userScalable);

Hm, what? !!attr.userScalable? Why not just "attr.userScalable"?

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:97
> +    return itemScale / m_devicePixelRatio;

Is it possible that a division by zero occurs here?

[Simon Hausmann]

Comment on attachment 137324 [details]
Patch

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

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.h:54
> +    bool hadUserInteraction() { return m_hadUserInteraction; }

Missing const.

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.h:130
> +    bool m_allowsUserScaling;
> +    qreal m_minimumScale;
> +    qreal m_maximumScale;
> +    qreal m_devicePixelRatio;

I see these variables being added, but it seems they're only "initialized" when the message about the updated viewport attributes arrives from the web process. Shouldn't they be initialized in the constructor?

[Kenneth Rohde Christiansen]

> I see these variables being added, but it seems they're only "initialized" when the message about the updated viewport attributes arrives from the web process. Shouldn't they be initialized in the constructor?

Indeed, I will add them to the reset() method instead.

[Kenneth Rohde Christiansen]

> Wonder if you mean "how" instead of "now" here.

OOps!

> > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:635
> > +    // This needs to be revised at some point.
> 
> I believe the "FIXME" already marks the following code as "should be revised at some point", so this paragragh may be omitted.

Im fine with that.

> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:640
> > +    interactionEngine->setAllowsUserScaling(!!attr.userScalable);
> 
> Hm, what? !!attr.userScalable? Why not just "attr.userScalable"?

It is a float. This is the common way to convert it.

> > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:97
> > +    return itemScale / m_devicePixelRatio;
> 
> Is it possible that a division by zero occurs here?

No, a ratio can never be zero.

[Kenneth Rohde Christiansen]

Created attachment 137333 [details]
Patch

[Kenneth Rohde Christiansen]

Landed in 114247.