Summary: | [Qt][WK2] Add a basic engine to control the content of the viewport | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Benjamin Poulain <benjamin> | ||||||||||||||
Component: | WebKit Qt | Assignee: | Benjamin Poulain <benjamin> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | jturcotte, kenneth, kling, webkit.review.bot | ||||||||||||||
Priority: | P2 | Keywords: | Qt, QtTriaged | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Attachments: |
|
Description
Benjamin Poulain
2011-08-17 09:20:08 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 :-) ? Created attachment 104195 [details]
Patch
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.
Don't worry about the style bot... (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 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 ? :-) (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. Created attachment 104322 [details]
Patch
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 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. 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)) { Created attachment 104325 [details]
Patch
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.
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.
> > 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.
(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(). Created attachment 104331 [details]
Patch
> 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?
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.
Created attachment 104332 [details]
Patch
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.
Created attachment 104340 [details]
Patch
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 on attachment 104340 [details]
Patch
r=me. Let's move this forward and worry about perfect signal names later.
Comment on attachment 104340 [details]
Patch
Thanks for the reviews Andreas, Jocelyn and Kenneth.
Comment on attachment 104340 [details] Patch Clearing flags on attachment: 104340 Committed r93407: <http://trac.webkit.org/changeset/93407> All reviewed patches have been landed. Closing bug. |