[webkit-qt] API review, comments needed!

[Joseph Ligman]

Hi,

I started working on the issues with QWebFrame::scrollRecursively, and  
am looking for some suggestions.

This API was introduced because the clients of QWebFrame were not able  
to scroll element's with a CSS overflow property. Initially, I tried  
to implement QWebElement::scroll to fix this, but reviewer's didn't  
like that idea.  ScrollRecursively seemed to sound ok at that time.

The hit test position using the last mouse position is  a bug.  For  
this to work, the client has to continually update the mouse position  
while scrolling and this can produce unexpected behavior.  I've been  
planning on adding this position as a parameter to the new API, does  
anyone have any other ideas?

Since the name is confusing, I was thinking about changing this to  
bool QWebFrame::ScrollFrameWithParent(int dx, int dy, QPoint  
overflowHitTestPosition);  and also, I added the bool return type so  
the client could easily check if scrolling happened, but I'm not sure  
this is necessary (if we keep the bool fixing the documentation is no  
problem)


Thanks.
Joe

On Mar 5, 2010, at 11:16 AM, tonikitoo (Antonio Gomes) wrote:

> On Fri, Mar 5, 2010 at 11:16 AM, Simon Hausmann
> <simon.hausmann at nokia.com> wrote:
>> Hi,
>>
>> we just went through the diff of the header files between Qt 4.6's  
>> WebKit and
>> the trunk. It's a relatively small diff, but nevertheless taking  
>> another look
>> at it revealed a few interesting issues. Here are the comments and  
>> questions
>> we collected. Please comment :)
>>
>>
>> QWebFrame::scrollRecursively:
>>
>>    * Implicit dependency on last mouse move event is bad. The  
>> position should
>> become an explicit function parameter.
>>
>>    * The use of "recursion" in the name is confusing, it suggests  
>> recursive
>> behaviour to child frames. However the behaviour is to continue  
>> scrolling the
>> parent frames.
>>
>>    * The documentation does not explain what the return value is.
>>
>>    * The auto test only checks successful scrolling. It never tests  
>> the case
>> of the function
>>      returning false.
>
> I agree and even I asked for that comment to be added in the bug in
> https://bugs.webkit.org/show_bug.cgi?id=32668#c13
> _______________________________________________
> webkit-qt mailing list
> webkit-qt at lists.webkit.org
> http://lists.webkit.org/mailman/listinfo.cgi/webkit-qt