Bug 66383 – [Qt][WK2] Add a basic engine to control the content of the viewport

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Benjamin Poulain]

Currently the content can move freely in the viewport, that should be fixed.

The content should
-stay in the viewport boundaries
-respect the zoom boundaries

This is the base to build kinetic scrolling, the animations, etc

[Kenneth Rohde Christiansen]

Please make it possible to change the visible viewport (useful when showing virtual keyboard etc) and add a proper bounce back (from zoom as well).

QWebInteractionEngine :-) ?

[Benjamin Poulain]

Created attachment 104195 [details]
Patch

[WebKit Review Bot]

Attachment 104195 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/UIProcess/API/qt/qtouchwebview_p.h:38:  _q_viewportRectUpdated is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/qt/ViewportMotionEngine.cpp:21:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebKit2/UIProcess/API/qt/qtouchwebview.cpp:51:  QTouchWebViewPrivate::_q_viewportRectUpdated is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:149:  QTouchWebPagePrivate::_q_commitScaleChange is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/qt/qtouchwebpage_p.h:42:  _q_commitScaleChange is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 5 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Benjamin Poulain]

Don't worry about the style bot...

[Benjamin Poulain]

(In reply to comment #1)
> Please make it possible to change the visible viewport (useful when showing virtual keyboard etc) and add a proper bounce back (from zoom as well).

Changing the visible viewport->we should add it when we need it.

Proper bounce back->follow up patch. I wanted something simple and improve little by little.

[Kenneth Rohde Christiansen]

Comment on attachment 104195 [details]
Patch

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

Just a few initial comments

> Source/WebKit2/ChangeLog:9
> +        Add ViewportMotionEngine to handle the content of the viewport. The class make
> +        sure the QTouchWebPage stays in the viewport and in the boundaries.

With the description is sounds like like it deals with the interaction and not just the motion.

>> Source/WebKit2/UIProcess/API/qt/qtouchwebpage_p.h:42
>> +    void _q_commitScaleChange();
> 
> _q_commitScaleChange is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]

We should fix the style bot to ignore _q_ :-)

> Source/WebKit2/UIProcess/API/qt/qtouchwebview.cpp:67
> +    ViewportMotionEngine::Constraints newConstraints;

Nice name... though it doesnt really apply to the devicePixelRatio. Btw do we need that on the ui side at all?

> Source/WebKit2/UIProcess/qt/ViewportMotionEngine.cpp:40
> +// Updating content value causes the notify signals to be sent.

Send from where? Can that be made more clear?

> Source/WebKit2/UIProcess/qt/ViewportMotionEngine.cpp:44
> +// The guard make sure the signal contentChangedInViewport() is sent if necessary.

I am not sure that I like that signal name

> Source/WebKit2/UIProcess/qt/ViewportMotionEngine.cpp:52
> +        , originalPosition(viewportMotionEngine->m_content->pos())

original? Isnt it more like previousPosition. Original sounds like initial.

currentPosition?

> Source/WebKit2/UIProcess/qt/ViewportMotionEngine.cpp:201
> +void ViewportMotionEngine::updateContentIfNecessary()

That name is a bit confusing... it sounds like relayouting/repaining the contents. updateContentPropertiesIfNecessary?  Why not if *Needed*

> Source/WebKit2/UIProcess/qt/ViewportMotionEngine.cpp:233
> +void ViewportMotionEngine::animateContentPositionToBoundaries()

How does this work in combination with scale? ie. bounceback? Could they be dealt with similarity? maybe one code path

> Source/WebKit2/UIProcess/qt/ViewportMotionEngine.h:71
> +    void contentChangedInViewport();

It is not really the content that changed, but some interaction was done with the content. It is also more the item containing the content that was manipulated.

Ah I see further down that I might be wrong and this responds to both changes in the content size but also user manipulation. Finding a good name for this is hard.

So basically here we have contentSizeChanged and contentRepresentationChanged.

> Source/WebKit2/UIProcess/qt/ViewportMotionEngine.h:97
> +    enum UserInteractionFlag {

why not just "UserInteractionFlag { NoUserInteraction, UserPositionInteraction, UserScaleInteraction;  ...

> Source/WebKit2/UIProcess/qt/ViewportMotionEngine.h:99
> +        UserHasMovedContent = 1,

HasPositionedContent?

> Source/WebKit2/UIProcess/qt/ViewportMotionEngine.h:108
> +inline bool operator==(const ViewportMotionEngine::Constraints& lhs, const ViewportMotionEngine::Constraints& rhs)

a, b ? :-)

[Benjamin Poulain]

(In reply to comment #6)
> > Source/WebKit2/ChangeLog:9
> > +        Add ViewportMotionEngine to handle the content of the viewport. The class make
> > +        sure the QTouchWebPage stays in the viewport and in the boundaries.
> 
> With the description is sounds like like it deals with the interaction and not just the motion.

Fair. Let's go for "ViewportInteractionEngine" unless someone come up with a better name during the night.

[Benjamin Poulain]

Created attachment 104322 [details]
Patch

[WebKit Review Bot]

Attachment 104322 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/UIProcess/API/qt/qtouchwebview_p.h:38:  _q_viewportRectUpdated is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/qt/qtouchwebview.cpp:51:  QTouchWebViewPrivate::_q_viewportRectUpdated is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/qt/ViewportInteractionEngine.cpp:21:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:149:  QTouchWebPagePrivate::_q_commitScaleChange is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/qt/qtouchwebpage_p.h:42:  _q_commitScaleChange is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 5 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kenneth Rohde Christiansen]

Comment on attachment 104322 [details]
Patch

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

> Source/WebKit2/ChangeLog:8
> +        Add ViewportMotionEngine to handle the content of the viewport. The class make

Wrong name

> Source/WebKit2/UIProcess/API/qt/qtouchwebview_p.h:46
> +    ViewportInteractionEngine interactionEngine;

Isn't this very Qt specific? Should it be prefixed with Qt ?

> Source/WebKit2/UIProcess/qt/ViewportInteractionEngine.h:109
> +    return (a.initialScale == b.initialScale && a.minimumScale == b.minimumScale && a.maximumScale == b.maximumScale
> +            && a.isUserScalable == b.isUserScalable);

I guess the ()'s are not needed. I would put each && on a separate line.

[Benjamin Poulain]

Second attempt.



> Please make it possible to change the visible viewport (useful when showing virtual keyboard etc) and add a proper bounce back (from zoom as well).

> > Source/WebKit2/UIProcess/qt/ViewportMotionEngine.cpp:233
> > +void ViewportMotionEngine::animateContentPositionToBoundaries()
> 
> How does this work in combination with scale? ie. bounceback? Could they be dealt with similarity? maybe one code path

Yep I should do that but that will be for a follow up.


> > Source/WebKit2/UIProcess/qt/ViewportMotionEngine.cpp:44
> > +// The guard make sure the signal contentChangedInViewport() is sent if necessary.
> 
> I am not sure that I like that signal name

The trick here is this class is written entirely without references to WebKit, it is manipulating the QSGItem directly.

So the content is never "pixel values" in this context.

> > Source/WebKit2/UIProcess/qt/ViewportMotionEngine.cpp:201
> > +void ViewportMotionEngine::updateContentIfNecessary()
> 
> That name is a bit confusing... it sounds like relayouting/repaining the contents. updateContentPropertiesIfNecessary?  Why not if *Needed*

Ditto. (fixed the IfNeeded though).



> > Source/WebKit2/UIProcess/qt/ViewportMotionEngine.h:97
> > +    enum UserInteractionFlag {
> 
> why not just "UserInteractionFlag { NoUserInteraction, UserPositionInteraction, UserScaleInteraction;  ...
> 
> > Source/WebKit2/UIProcess/qt/ViewportMotionEngine.h:99
> > +        UserHasMovedContent = 1,
> 
> HasPositionedContent?

I think the current enums are a lot easier to read. E.g.:
    if (!(m_userInteractionFlags & UserHasMovedContent)) {
VS
    if (!(m_userInteractionFlags & UserPositionInteraction)) {

[Benjamin Poulain]

Created attachment 104325 [details]
Patch

[WebKit Review Bot]

Attachment 104325 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/UIProcess/API/qt/qtouchwebview_p.h:38:  _q_viewportRectUpdated is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/qt/qtouchwebview.cpp:51:  QTouchWebViewPrivate::_q_viewportRectUpdated is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/qt/ViewportInteractionEngine.cpp:21:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:149:  QTouchWebPagePrivate::_q_commitScaleChange is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/qt/qtouchwebpage_p.h:42:  _q_commitScaleChange is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 5 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jocelyn Turcotte]

Random comments:

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

> Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:37
> +    setTransformOrigin(TopLeft);

A comment explaining the purpose could help future readers.

> Source/WebKit2/UIProcess/qt/QtPanGestureRecognizer.cpp:78
> +        m_viewportInteractionEngine->panGestureRequestScroll(offsetSinceLastEvent.x(), offsetSinceLastEvent.y());

If the viewport interaction will be the one responsible of translating "forces" into scroll offset, then I'd prefer having its input called RequestUpdate like with the pinch recognizer.

> Source/WebKit2/UIProcess/qt/ViewportInteractionEngine.cpp:31
> +static inline QRectF visibleRectInContentCoordinate(const QSGItem* item, const QSGItem* viewport)
> +{
> +    const QRectF viewportInItemCoordinate = item->mapRectFromItem(viewport, viewport->boundingRect());
> +    const QRectF visibleArea = item->boundingRect().intersected(viewportInItemCoordinate);

item -> content?

> Source/WebKit2/UIProcess/qt/ViewportInteractionEngine.cpp:35
> +static inline QRectF contentRectInViewportCoordinate(const QSGItem* item, const QSGItem* viewport)

ditto?

> Source/WebKit2/UIProcess/qt/ViewportInteractionEngine.cpp:40
> +// Updating content value causes the notify signals to be sent by the content item itself.

"values" or "a content value".

> Source/WebKit2/UIProcess/qt/ViewportInteractionEngine.cpp:112
> +    if (m_content->scale() != previousScale)
> +        emit commitScaleChange();

This may cause nasty side effects until we have suspend working, but I guess we can live with it.

> Source/WebKit2/UIProcess/qt/ViewportInteractionEngine.cpp:207
> +    emit commitScaleChange();

Shouldn't you check that you aren't currently between pinch start and pinch end here?
Just explain me if I'm missing something.

> Source/WebKit2/UIProcess/qt/ViewportInteractionEngine.h:63
> +    void pinchGestureRequestUpdate(const QPointF&, qreal);

Would be nice not to strip the argument names in the header here, they aren't obvious from the method name and argument types. Readability nitpicks.

> Source/WebKit2/UIProcess/qt/ViewportInteractionEngine.h:69
> +    void contentChangedInViewport();

Saying that the content changed is ambiguous, this sounds like the page layout changed.
contentTransformChanged wouldn't include the size changed, and contentGeometryChanged wouldn't include the scale change.
Could also be contentViewChanged or something along this.

[Kenneth Rohde Christiansen]

> > Source/WebKit2/UIProcess/qt/ViewportInteractionEngine.h:69
> > +    void contentChangedInViewport();
> 
> Saying that the content changed is ambiguous, this sounds like the page layout changed.
> contentTransformChanged wouldn't include the size changed, and contentGeometryChanged wouldn't include the scale change.
> Could also be contentViewChanged or something along this.

I totally agree with this being confusing/ambiguous.

[Benjamin Poulain]

(In reply to comment #14)
> > Source/WebKit2/UIProcess/qt/ViewportInteractionEngine.cpp:112
> > +    if (m_content->scale() != previousScale)
> > +        emit commitScaleChange();
> 
> This may cause nasty side effects until we have suspend working, but I guess we can live with it.

I am not aware of this. What is the problem?


> > Source/WebKit2/UIProcess/qt/ViewportInteractionEngine.cpp:207
> > +    emit commitScaleChange();
> 
> Shouldn't you check that you aren't currently between pinch start and pinch end here?
> Just explain me if I'm missing something.

The trick is ViewportInteractionEngine::contentScaleChanged() is never called by us, it is called in response to the signals from QTouchWebPage.

The two slots contentGeometryChanged() and contentScaleChanged() are there to maintain consistency of the viewport in the case "someone"(TM) set the values for us.

One example is the page resize, which is out of the control of the viewport.

So since the events already happened, we also emit the signals to keep the state consistency of the client.

> > Source/WebKit2/UIProcess/qt/ViewportInteractionEngine.h:69
> > +    void contentChangedInViewport();
> 
> Saying that the content changed is ambiguous, this sounds like the page layout changed.
> contentTransformChanged wouldn't include the size changed, and contentGeometryChanged wouldn't include the scale change.
> Could also be contentViewChanged or something along this.

So my argument was the class is handling SGItem, not something with actual content.

But we can use a better name if someone find one. As you said, contentTransformChanged() and contentGeometryChanged() don't cut it, and I don't really the value of contentViewChanged() VS contentChangedInViewport().

[Benjamin Poulain]

Created attachment 104331 [details]
Patch

[Kenneth Rohde Christiansen]

> But we can use a better name if someone find one. As you said, contentTransformChanged() and contentGeometryChanged() don't cut it, and I don't really the value of contentViewChanged() VS contentChangedInViewport().

contentTransformAndGeometryChanged?

[WebKit Review Bot]

Attachment 104331 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/UIProcess/API/qt/qtouchwebview_p.h:38:  _q_viewportRectUpdated is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/qt/qtouchwebview.cpp:51:  QTouchWebViewPrivate::_q_viewportRectUpdated is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/qt/ViewportInteractionEngine.cpp:21:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:152:  QTouchWebPagePrivate::_q_commitScaleChange is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/qt/qtouchwebpage_p.h:42:  _q_commitScaleChange is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 5 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Benjamin Poulain]

Created attachment 104332 [details]
Patch

[WebKit Review Bot]

Attachment 104332 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/UIProcess/API/qt/qtouchwebview_p.h:38:  _q_viewportRectUpdated is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/qt/qtouchwebview.cpp:51:  QTouchWebViewPrivate::_q_viewportRectUpdated is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/qt/ViewportInteractionEngine.cpp:21:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:152:  QTouchWebPagePrivate::_q_commitScaleChange is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/qt/qtouchwebpage_p.h:42:  _q_commitScaleChange is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 5 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Benjamin Poulain]

Created attachment 104340 [details]
Patch

[WebKit Review Bot]

Attachment 104340 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/UIProcess/API/qt/qtouchwebview_p.h:38:  _q_viewportRectUpdated is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/qt/qtouchwebview.cpp:51:  QTouchWebViewPrivate::_q_viewportRectUpdated is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/qt/ViewportInteractionEngine.cpp:21:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:152:  QTouchWebPagePrivate::_q_commitScaleChange is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/qt/qtouchwebpage_p.h:42:  _q_commitScaleChange is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 5 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andreas Kling]

Comment on attachment 104340 [details]
Patch

r=me. Let's move this forward and worry about perfect signal names later.

[Benjamin Poulain]

Comment on attachment 104340 [details]
Patch

Thanks for the reviews Andreas, Jocelyn and Kenneth.

[WebKit Review Bot]

Comment on attachment 104340 [details]
Patch

Clearing flags on attachment: 104340

Committed r93407: <http://trac.webkit.org/changeset/93407>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.