WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
48450
[Qt] Extend the Platform Plugin to define the padding of HitTestResult
https://bugs.webkit.org/show_bug.cgi?id=48450
Summary
[Qt] Extend the Platform Plugin to define the padding of HitTestResult
Andre Pedralho
Reported
Wednesday, October 27, 2010 7:58:51 PM UTC
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.
Attachments
Adding the TouchAdjusts extension in the Platform Plugin.
(8.16 KB, patch)
2010-10-28 12:47 PDT
,
Andre Pedralho
kenneth
: review-
Details
Formatted Diff
Diff
Updating last patch according to Kenneth suggestions.
(7.57 KB, patch)
2010-10-28 15:13 PDT
,
Andre Pedralho
tonikitoo
: commit-queue-
Details
Formatted Diff
Diff
Patch updated with Antonio's and Simon's suggestions.
(8.83 KB, patch)
2010-10-29 13:21 PDT
,
Andre Pedralho
no flags
Details
Formatted Diff
Diff
Patch updated according to Luiz suggestions.
(8.97 KB, patch)
2010-11-02 16:12 PDT
,
Andre Pedralho
kenneth
: review-
kenneth
: commit-queue-
Details
Formatted Diff
Diff
Sorry, removing one of the ChangeLogs entries.
(8.09 KB, patch)
2010-11-02 16:27 PDT
,
Andre Pedralho
kenneth
: review+
Details
Formatted Diff
Diff
Renaming extension name from TouchInteraction to TouchModifier and making its implementation simpler.
(7.30 KB, patch)
2010-11-03 07:59 PDT
,
Andre Pedralho
no flags
Details
Formatted Diff
Diff
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
Andre Pedralho
Comment 1
Thursday, October 28, 2010 8:47:44 PM UTC
Created
attachment 72219
[details]
Adding the TouchAdjusts extension in the Platform Plugin.
Kenneth Rohde Christiansen
Comment 2
Thursday, October 28, 2010 9:30:53 PM UTC
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.
Kenneth Rohde Christiansen
Comment 3
Thursday, October 28, 2010 9:31:25 PM UTC
Btw, nice to have you back hacking on webkit!
Andre Pedralho
Comment 4
Thursday, October 28, 2010 11:13:06 PM UTC
Created
attachment 72241
[details]
Updating last patch according to Kenneth suggestions.
Andre Pedralho
Comment 5
Thursday, October 28, 2010 11:41:27 PM UTC
(In reply to
comment #3
)
> Btw, nice to have you back hacking on webkit!
It's nice to be back!
Antonio Gomes
Comment 6
Friday, October 29, 2010 5:37:45 AM UTC
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?
Simon Hausmann
Comment 7
Friday, October 29, 2010 4:14:10 PM UTC
Nice! When you change the platform plugin interface, please also change the version string.
Andre Pedralho
Comment 8
Friday, October 29, 2010 6:48:23 PM UTC
(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?
Andre Pedralho
Comment 9
Friday, October 29, 2010 9:21:24 PM UTC
Created
attachment 72379
[details]
Patch updated with Antonio's and Simon's suggestions.
Luiz Agostini
Comment 10
Friday, October 29, 2010 10:16:07 PM UTC
> > 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.
Luiz Agostini
Comment 11
Friday, October 29, 2010 10:18:36 PM UTC
(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.
Antonio Gomes
Comment 12
Saturday, October 30, 2010 6:40:19 AM UTC
(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.
Andre Pedralho
Comment 13
Wednesday, November 3, 2010 12:12:02 AM UTC
Created
attachment 72761
[details]
Patch updated according to Luiz suggestions.
Kenneth Rohde Christiansen
Comment 14
Wednesday, November 3, 2010 12:22:22 AM UTC
Comment on
attachment 72761
[details]
Patch updated according to Luiz suggestions. r- due to double entry in the ChangeLog
Andre Pedralho
Comment 15
Wednesday, November 3, 2010 12:27:08 AM UTC
Created
attachment 72764
[details]
Sorry, removing one of the ChangeLogs entries.
Kenneth Rohde Christiansen
Comment 16
Wednesday, November 3, 2010 12:28:40 AM UTC
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.
Kenneth Rohde Christiansen
Comment 17
Wednesday, November 3, 2010 12:30:34 AM UTC
> > 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.
Kenneth Rohde Christiansen
Comment 18
Wednesday, November 3, 2010 12:32:19 AM UTC
Comment on
attachment 72764
[details]
Sorry, removing one of the ChangeLogs entries. r+, but please consider my suggestions before setting cq+
Andre Pedralho
Comment 19
Wednesday, November 3, 2010 12:59:46 AM UTC
(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?
Kenneth Rohde Christiansen
Comment 20
Wednesday, November 3, 2010 8:58:10 AM UTC
(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
Andre Pedralho
Comment 21
Wednesday, November 3, 2010 3:59:23 PM UTC
Created
attachment 72822
[details]
Renaming extension name from TouchInteraction to TouchModifier and making its implementation simpler.
WebKit Commit Bot
Comment 22
Thursday, November 4, 2010 4:55:42 AM UTC
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
>
WebKit Commit Bot
Comment 23
Thursday, November 4, 2010 4:55:49 AM UTC
All reviewed patches have been landed. Closing bug.
Ademar Reis
Comment 24
Friday, November 5, 2010 4:25:06 PM UTC
Revision
r71303
cherry-picked into qtwebkit-2.1 with commit 78ff335 <
http://gitorious.org/webkit/qtwebkit/commit/78ff335
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug