After some discussion on bug #44088 it was decided that the padding values used by the HitTestResult should be defined in the Platform Plugin. This way different platforms will be able to set its own preferred value to the HitTestResult rect.
Created attachment 72219 [details] Adding the TouchAdjusts extension in the Platform Plugin.
Comment on attachment 72219 [details] Adding the TouchAdjusts extension in the Platform Plugin. I like to have this more like the Haptics. Thus call the Extension for Touch, and just make the plugin implement something like: class QWebTouchInteraction : public QObject { enum PaddingDirection { ... } virtual int hitTestPaddingForTouch(const PaddingDirection) const = 0; } I dislike the Adjust* and the setHitTestPadding is unneeded.
Btw, nice to have you back hacking on webkit!
Created attachment 72241 [details] Updating last patch according to Kenneth suggestions.
(In reply to comment #3) > Btw, nice to have you back hacking on webkit! It's nice to be back!
Comment on attachment 72241 [details] Updating last patch according to Kenneth suggestions. View in context: https://bugs.webkit.org/attachment.cgi?id=72241&action=review Looks great! > WebKit/qt/Api/qwebkitplatformplugin.h:113 > + enum PaddingDirection { > + Up, Right, Down, Left > + }; Is not it against the webkit style? > WebKit/qt/ChangeLog:15 > + (TouchAdjuster::setHitTestPadding): you do not have the setter anymore, and it is not called touchadjuster neither. > WebKit/qt/examples/platformplugin/WebPlugin.cpp:254 > + return new TouchInteraction(); who deletes it?
Nice! When you change the platform plugin interface, please also change the version string.
(In reply to comment #6) > (From update of attachment 72241 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=72241&action=review > > Looks great! > > > WebKit/qt/Api/qwebkitplatformplugin.h:113 > > + enum PaddingDirection { > > + Up, Right, Down, Left > > + }; > > Is not it against the webkit style? > Sorry, I just followed the style of the other extensions. > > WebKit/qt/ChangeLog:15 > > + (TouchAdjuster::setHitTestPadding): > > you do not have the setter anymore, and it is not called touchadjuster neither. > Ops! Forget to update Changelog and commit header. > > WebKit/qt/examples/platformplugin/WebPlugin.cpp:254 > > + return new TouchInteraction(); > > who deletes it? Ok! I'll add a private QObject* m_extension variable and delete it in the plugin destructor. Other suggestion?
Created attachment 72379 [details] Patch updated with Antonio's and Simon's suggestions.
> > WebKit/qt/examples/platformplugin/WebPlugin.cpp:254 > > + return new TouchInteraction(); > > who deletes it? Ok! I'll add a private QObject* m_extension variable and delete it in the plugin destructor. Other suggestion? The user of the plugin should responsible for destroying the TouchInteraction object. createExtension caller should be the owner of the returned object.
(In reply to comment #10) > > > WebKit/qt/examples/platformplugin/WebPlugin.cpp:254 > > > + return new TouchInteraction(); > > > > who deletes it? > Ok! I'll add a private QObject* m_extension variable and delete it in the plugin destructor. Other suggestion? > The user of the plugin should responsible for destroying the TouchInteraction object. createExtension caller should be the owner of the returned object.
(In reply to comment #11) > > (In reply to comment #10) > > > > WebKit/qt/examples/platformplugin/WebPlugin.cpp:254 > > > > + return new TouchInteraction(); > > > > > > who deletes it? > > Ok! I'll add a private QObject* m_extension variable and delete it in the plugin destructor. Other suggestion? > > > > The user of the plugin should responsible for destroying the TouchInteraction object. createExtension caller should be the owner of the returned object. Pedralho, please go for what luiz suggested. Other than that, looks good to me.
Created attachment 72761 [details] Patch updated according to Luiz suggestions.
Comment on attachment 72761 [details] Patch updated according to Luiz suggestions. r- due to double entry in the ChangeLog
Created attachment 72764 [details] Sorry, removing one of the ChangeLogs entries.
Comment on attachment 72761 [details] Patch updated according to Luiz suggestions. View in context: https://bugs.webkit.org/attachment.cgi?id=72761&action=review > WebKit/qt/examples/platformplugin/WebPlugin.cpp:226 > +unsigned TouchInteraction::hitTestPaddingForTouch(const PaddingDirection direction) const > +{ > + switch (direction) { > + case Down: > + return bottomPadding; > + case Left: > + return leftPadding; > + case Right: > + return rightPadding; > + case Up: > + return topPadding; > + default: > + Q_ASSERT(0); > + return 0; > + } > +} I don't understand why you make such a complicated example, but well :-) I would just have done return 10; // Use 10 as padding in each direction. > WebKit/qt/examples/platformplugin/WebPlugin.h:102 > +{ > + Q_OBJECT > +public: > + TouchInteraction() > + : bottomPadding(10) > + , leftPadding(10) > + , rightPadding(10) > + , topPadding(10) > + {} > + > + unsigned hitTestPaddingForTouch(const PaddingDirection) const; > + > +private: > + unsigned bottomPadding; > + unsigned leftPadding; > + unsigned rightPadding; > + unsigned topPadding; This seems a bit overengineered... in real life you mostly want the same padding in each direction, but maybe a bit more padding in the top, but that could easily be implemented just in the histTestPaddingForTouch method like if (direction == PaddingDirection::Up) return 15; return 8; or similar. > WebKit/qt/examples/platformplugin/qwebkitplatformplugin.h:123 > + Touch Is Touch a good name here... maybe TouchInteraction ? I'm just thinking out loud... maybe others have comments.
> > if (direction == PaddingDirection::Up) > return 15; > return 8; I think that something like this would make for a better, more clear example. Anyways, that is the whole point of an example - to be simple and easy to follow and understand.
Comment on attachment 72764 [details] Sorry, removing one of the ChangeLogs entries. r+, but please consider my suggestions before setting cq+
(In reply to comment #16) > (From update of attachment 72761 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=72761&action=review > > I don't understand why you make such a complicated example, but well :-) I would just have done > > return 10; // Use 10 as padding in each direction. > > This seems a bit overengineered... in real life you mostly want the same padding in each direction, but maybe a bit more padding in the top, but that could easily be implemented just in the histTestPaddingForTouch method > > like > > if (direction == PaddingDirection::Up) > return 15; > return 8; > > or similar. > // Use 10 as padding in each direction but Up. if (direction == QWebTouchInteraction::Up) return 15; return 10; Better? > > WebKit/qt/examples/platformplugin/qwebkitplatformplugin.h:123 > > + Touch > > Is Touch a good name here... maybe TouchInteraction ? I'm just thinking out loud... maybe others have comments. TouchInteraction is a better name but it is also the class name, then it conflicts. Suggestions?
(In reply to comment #19) > (In reply to comment #16) > > (From update of attachment 72761 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=72761&action=review > > > > I don't understand why you make such a complicated example, but well :-) I would just have done > > > > return 10; // Use 10 as padding in each direction. > > > > This seems a bit overengineered... in real life you mostly want the same padding in each direction, but maybe a bit more padding in the top, but that could easily be implemented just in the histTestPaddingForTouch method > > > > like > > > > if (direction == PaddingDirection::Up) > > return 15; > > return 8; > > > > or similar. > > > > // Use 10 as padding in each direction but Up. > if (direction == QWebTouchInteraction::Up) > return 15; > return 10; > > Better? yes > > > > WebKit/qt/examples/platformplugin/qwebkitplatformplugin.h:123 > > > + Touch > > > > Is Touch a good name here... maybe TouchInteraction ? I'm just thinking out loud... maybe others have comments. > TouchInteraction is a better name but it is also the class name, then it conflicts. Suggestions? You could call the class TouchModifier
Created attachment 72822 [details] Renaming extension name from TouchInteraction to TouchModifier and making its implementation simpler.
Comment on attachment 72822 [details] Renaming extension name from TouchInteraction to TouchModifier and making its implementation simpler. Clearing flags on attachment: 72822 Committed r71303: <http://trac.webkit.org/changeset/71303>
All reviewed patches have been landed. Closing bug.
Revision r71303 cherry-picked into qtwebkit-2.1 with commit 78ff335 <http://gitorious.org/webkit/qtwebkit/commit/78ff335>