Bug 66383 - [Qt][WK2] Add a basic engine to control the content of the viewport
Summary: [Qt][WK2] Add a basic engine to control the content of the viewport
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Benjamin Poulain
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks:
 
Reported: 2011-08-17 09:20 PDT by Benjamin Poulain
Modified: 2011-08-19 07:56 PDT (History)
4 users (show)

See Also:


Attachments
Patch (46.07 KB, patch)
2011-08-17 11:05 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (46.78 KB, patch)
2011-08-18 04:38 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (46.88 KB, patch)
2011-08-18 04:50 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (47.08 KB, patch)
2011-08-18 06:08 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (47.37 KB, patch)
2011-08-18 06:52 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (47.36 KB, patch)
2011-08-18 07:37 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Poulain 2011-08-17 09:20:08 PDT
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
Comment 1 Kenneth Rohde Christiansen 2011-08-17 09:26:50 PDT
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 :-) ?
Comment 2 Benjamin Poulain 2011-08-17 11:05:59 PDT
Created attachment 104195 [details]
Patch
Comment 3 WebKit Review Bot 2011-08-17 11:08:36 PDT
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.
Comment 4 Benjamin Poulain 2011-08-17 11:09:11 PDT
Don't worry about the style bot...
Comment 5 Benjamin Poulain 2011-08-17 11:22:44 PDT
(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.
Comment 6 Kenneth Rohde Christiansen 2011-08-17 11:36:50 PDT
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 ? :-)
Comment 7 Benjamin Poulain 2011-08-17 14:45:14 PDT
(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.
Comment 8 Benjamin Poulain 2011-08-18 04:38:13 PDT
Created attachment 104322 [details]
Patch
Comment 9 WebKit Review Bot 2011-08-18 04:40:44 PDT
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.
Comment 10 Kenneth Rohde Christiansen 2011-08-18 04:45:29 PDT
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.
Comment 11 Benjamin Poulain 2011-08-18 04:46:38 PDT
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)) {
Comment 12 Benjamin Poulain 2011-08-18 04:50:48 PDT
Created attachment 104325 [details]
Patch
Comment 13 WebKit Review Bot 2011-08-18 04:58:49 PDT
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.
Comment 14 Jocelyn Turcotte 2011-08-18 05:44:51 PDT
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.
Comment 15 Kenneth Rohde Christiansen 2011-08-18 05:46:56 PDT
> > 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.
Comment 16 Benjamin Poulain 2011-08-18 06:03:58 PDT
(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().
Comment 17 Benjamin Poulain 2011-08-18 06:08:21 PDT
Created attachment 104331 [details]
Patch
Comment 18 Kenneth Rohde Christiansen 2011-08-18 06:09:10 PDT
> 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?
Comment 19 WebKit Review Bot 2011-08-18 06:11:22 PDT
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.
Comment 20 Benjamin Poulain 2011-08-18 06:52:16 PDT
Created attachment 104332 [details]
Patch
Comment 21 WebKit Review Bot 2011-08-18 06:55:50 PDT
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.
Comment 22 Benjamin Poulain 2011-08-18 07:37:00 PDT
Created attachment 104340 [details]
Patch
Comment 23 WebKit Review Bot 2011-08-18 07:40:19 PDT
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.
Comment 24 Andreas Kling 2011-08-19 05:39:03 PDT
Comment on attachment 104340 [details]
Patch

r=me. Let's move this forward and worry about perfect signal names later.
Comment 25 Benjamin Poulain 2011-08-19 07:37:45 PDT
Comment on attachment 104340 [details]
Patch

Thanks for the reviews Andreas, Jocelyn and Kenneth.
Comment 26 WebKit Review Bot 2011-08-19 07:56:40 PDT
Comment on attachment 104340 [details]
Patch

Clearing flags on attachment: 104340

Committed r93407: <http://trac.webkit.org/changeset/93407>
Comment 27 WebKit Review Bot 2011-08-19 07:56:46 PDT
All reviewed patches have been landed.  Closing bug.