Bug 78047 – [Qt][WK2] QQuickWebView::event should lookup faster which events QQuickWebPage can handle

• Component: New Bugs
• Status: RESOLVED FIXED

[Rafael Brandao]

[Qt][WK2] QQuickWebView::event should lookup faster which events QQuickWebPage can handle

[Rafael Brandao]

Created attachment 125940 [details]
Patch

[Rafael Brandao]

This is how the assembly code for QQuickWebView::event was before this patch:

=> 0x00007ffff60a5260 <+0>:     mov    %rbx,-0x10(%rsp)
   0x00007ffff60a5265 <+5>:     mov    %rbp,-0x8(%rsp)
   0x00007ffff60a526a <+10>:    sub    $0x18,%rsp
   0x00007ffff60a526e <+14>:    movzwl 0x10(%rsi),%eax
   0x00007ffff60a5272 <+18>:    mov    %rdi,%rbp
   0x00007ffff60a5275 <+21>:    mov    %rsi,%rbx
   0x00007ffff60a5278 <+24>:    mov    0x20(%rdi),%rdx
   0x00007ffff60a527c <+28>:    cmp    $0x3f,%eax
   0x00007ffff60a527f <+31>:    jg     0x7ffff60a52c8 <_ZN13QQuickWebView5eventEP6QEvent+104>
   0x00007ffff60a5281 <+33>:    cmp    $0x3c,%eax
   0x00007ffff60a5284 <+36>:    jl     0x7ffff60a52e8 <_ZN13QQuickWebView5eventEP6QEvent+136>
   0x00007ffff60a5286 <+38>:    mov    0x90(%rdx),%rdi
   0x00007ffff60a528d <+45>:    callq  0x7ffff60a3160 <_ZNK13QQuickWebPage12eventHandlerEv>
   0x00007ffff60a5292 <+50>:    mov    %rbx,%rsi
   0x00007ffff60a5295 <+53>:    mov    %rax,%rdi
   0x00007ffff60a5298 <+56>:    callq  0x7ffff61143f0 <_ZN21QtWebPageEventHandler11handleEventEP6QEvent>
   0x00007ffff60a529d <+61>:    test   %al,%al
   0x00007ffff60a529f <+63>:    jne    0x7ffff60a5318 <_ZN13QQuickWebView5eventEP6QEvent+184>
   0x00007ffff60a52a1 <+65>:    movzwl 0x10(%rbx),%eax
   0x00007ffff60a52a5 <+69>:    cmp    $0x53,%eax
   0x00007ffff60a52a8 <+72>:    je     0x7ffff60a5300 <_ZN13QQuickWebView5eventEP6QEvent+160>
   0x00007ffff60a52aa <+74>:    mov    %rbx,%rsi
   0x00007ffff60a52ad <+77>:    mov    %rbp,%rdi
   0x00007ffff60a52b0 <+80>:    mov    0x8(%rsp),%rbx
   0x00007ffff60a52b5 <+85>:    mov    0x10(%rsp),%rbp
   0x00007ffff60a52ba <+90>:    add    $0x18,%rsp
   0x00007ffff60a52be <+94>:    jmpq   0x7ffff5ff4e60 <_ZN10QQuickItem5eventEP6QEvent@plt>
   0x00007ffff60a52c3 <+99>:    nopl   0x0(%rax,%rax,1)
   0x00007ffff60a52c8 <+104>:   cmp    $0x7f,%eax
   0x00007ffff60a52cb <+107>:   jl     0x7ffff60a52a5 <_ZN13QQuickWebView5eventEP6QEvent+69>
   0x00007ffff60a52cd <+109>:   cmp    $0x81,%eax
   0x00007ffff60a52d2 <+114>:   jle    0x7ffff60a5286 <_ZN13QQuickWebView5eventEP6QEvent+38>
   0x00007ffff60a52d4 <+116>:   lea    -0xc2(%rax),%ecx
   0x00007ffff60a52da <+122>:   cmp    $0x2,%ecx
   0x00007ffff60a52dd <+125>:   ja     0x7ffff60a52a5 <_ZN13QQuickWebView5eventEP6QEvent+69>
   0x00007ffff60a52df <+127>:   jmp    0x7ffff60a5286 <_ZN13QQuickWebView5eventEP6QEvent+38>
   0x00007ffff60a52e1 <+129>:   nopl   0x0(%rax)
   0x00007ffff60a52e8 <+136>:   cmp    $0x2,%eax
   0x00007ffff60a52eb <+139>:   jl     0x7ffff60a52a5 <_ZN13QQuickWebView5eventEP6QEvent+69>
   0x00007ffff60a52ed <+141>:   cmp    $0x9,%eax
   0x00007ffff60a52f0 <+144>:   jle    0x7ffff60a5286 <_ZN13QQuickWebView5eventEP6QEvent+38>
   0x00007ffff60a52f2 <+146>:   cmp    $0x1f,%eax
   0x00007ffff60a52f5 <+149>:   jne    0x7ffff60a52a5 <_ZN13QQuickWebView5eventEP6QEvent+69>
   0x00007ffff60a52f7 <+151>:   jmp    0x7ffff60a5286 <_ZN13QQuickWebView5eventEP6QEvent+38>
   0x00007ffff60a52f9 <+153>:   nopl   0x0(%rax)
   0x00007ffff60a5300 <+160>:   xor    %eax,%eax
   0x00007ffff60a5302 <+162>:   mov    0x8(%rsp),%rbx
   0x00007ffff60a5307 <+167>:   mov    0x10(%rsp),%rbp
   0x00007ffff60a530c <+172>:   add    $0x18,%rsp
   0x00007ffff60a5310 <+176>:   retq   
   0x00007ffff60a5311 <+177>:   nopl   0x0(%rax)
   0x00007ffff60a5318 <+184>:   mov    $0x1,%eax
   0x00007ffff60a531d <+189>:   mov    0x8(%rsp),%rbx
   0x00007ffff60a5322 <+194>:   mov    0x10(%rsp),%rbp
   0x00007ffff60a5327 <+199>:   add    $0x18,%rsp
   0x00007ffff60a532b <+203>:   retq

And now:

=> 0x00007ffff60a4b50 <+0>:     mov    %rbx,-0x10(%rsp)
   0x00007ffff60a4b55 <+5>:     mov    %rbp,-0x8(%rsp)
   0x00007ffff60a4b5a <+10>:    sub    $0x18,%rsp
   0x00007ffff60a4b5e <+14>:    movzwl 0x10(%rsi),%eax
   0x00007ffff60a4b62 <+18>:    mov    %rdi,%rbp
   0x00007ffff60a4b65 <+21>:    mov    %rsi,%rbx
   0x00007ffff60a4b68 <+24>:    mov    0x20(%rdi),%rdx
   0x00007ffff60a4b6c <+28>:    cmp    $0xff,%eax
   0x00007ffff60a4b71 <+33>:    jg     0x7ffff60a4b83 <_ZN13QQuickWebView5eventEP6QEvent+51>
   0x00007ffff60a4b73 <+35>:    lea    0x1d0b7e6(%rip),%rsi        # 0x7ffff7db0360 <_ZL20s_eventHandledByPage>
   0x00007ffff60a4b7a <+42>:    movslq %eax,%rcx
   0x00007ffff60a4b7d <+45>:    cmpb   $0x0,(%rsi,%rcx,1)
   0x00007ffff60a4b81 <+49>:    jne    0x7ffff60a4ba8 <_ZN13QQuickWebView5eventEP6QEvent+88>
   0x00007ffff60a4b83 <+51>:    cmp    $0x53,%eax
   0x00007ffff60a4b86 <+54>:    je     0x7ffff60a4be0 <_ZN13QQuickWebView5eventEP6QEvent+144>
   0x00007ffff60a4b88 <+56>:    mov    %rbx,%rsi
   0x00007ffff60a4b8b <+59>:    mov    %rbp,%rdi
   0x00007ffff60a4b8e <+62>:    mov    0x8(%rsp),%rbx
   0x00007ffff60a4b93 <+67>:    mov    0x10(%rsp),%rbp
   0x00007ffff60a4b98 <+72>:    add    $0x18,%rsp
   0x00007ffff60a4b9c <+76>:    jmpq   0x7ffff5ff4e60 <_ZN10QQuickItem5eventEP6QEvent@plt>
   0x00007ffff60a4ba1 <+81>:    nopl   0x0(%rax)
   0x00007ffff60a4ba8 <+88>:    mov    0x90(%rdx),%rdi
   0x00007ffff60a4baf <+95>:    callq  0x7ffff60a3160 <_ZNK13QQuickWebPage12eventHandlerEv>
   0x00007ffff60a4bb4 <+100>:   mov    %rbx,%rsi
   0x00007ffff60a4bb7 <+103>:   mov    %rax,%rdi
   0x00007ffff60a4bba <+106>:   callq  0x7ffff61144a0 <_ZN21QtWebPageEventHandler11handleEventEP6QEvent>
   0x00007ffff60a4bbf <+111>:   test   %al,%al
   0x00007ffff60a4bc1 <+113>:   je     0x7ffff60a4b88 <_ZN13QQuickWebView5eventEP6QEvent+56>
   0x00007ffff60a4bc3 <+115>:   mov    $0x1,%eax
   0x00007ffff60a4bc8 <+120>:   mov    0x8(%rsp),%rbx
   0x00007ffff60a4bcd <+125>:   mov    0x10(%rsp),%rbp
   0x00007ffff60a4bd2 <+130>:   add    $0x18,%rsp
   0x00007ffff60a4bd6 <+134>:   retq   
   0x00007ffff60a4bd7 <+135>:   nopw   0x0(%rax,%rax,1)
   0x00007ffff60a4be0 <+144>:   xor    %eax,%eax
   0x00007ffff60a4be2 <+146>:   mov    0x8(%rsp),%rbx
   0x00007ffff60a4be7 <+151>:   mov    0x10(%rsp),%rbp
   0x00007ffff60a4bec <+156>:   add    $0x18,%rsp
   0x00007ffff60a4bf0 <+160>:   retq

The main motivation to make this change is that QQuickWebView::event is used a lot and the lookup would grow logarithmically by the number of events handled by QQuickWebPage when we could do this in O(1) instead.

Adding some people to CC... What you think of this?

[Noam Rosenthal]

Comment on attachment 125940 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=125940&action=review

Why use those raw bool arrays instead of the nice abstraction called BitVector?

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:96
> +static inline bool shouldPageHandleEvent(const QEvent::Type& event)

why const&? it's an enum, just pass by value.

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1255
> +    } else if (ev->type() == QEvent::InputMethod)

Why remove the comment?

[Igor Trindade Oliveira]

What valgrind/cachegrind says about this patch?

(In reply to comment #2)
> This is how the assembly code for QQuickWebView::event was before this patch:
> 
> => 0x00007ffff60a5260 <+0>:     mov    %rbx,-0x10(%rsp)
>    0x00007ffff60a5265 <+5>:     mov    %rbp,-0x8(%rsp)
>    0x00007ffff60a526a <+10>:    sub    $0x18,%rsp
>    0x00007ffff60a526e <+14>:    movzwl 0x10(%rsi),%eax
>    0x00007ffff60a5272 <+18>:    mov    %rdi,%rbp
>    0x00007ffff60a5275 <+21>:    mov    %rsi,%rbx
>    0x00007ffff60a5278 <+24>:    mov    0x20(%rdi),%rdx
>    0x00007ffff60a527c <+28>:    cmp    $0x3f,%eax
>    0x00007ffff60a527f <+31>:    jg     0x7ffff60a52c8 <_ZN13QQuickWebView5eventEP6QEvent+104>
>    0x00007ffff60a5281 <+33>:    cmp    $0x3c,%eax
>    0x00007ffff60a5284 <+36>:    jl     0x7ffff60a52e8 <_ZN13QQuickWebView5eventEP6QEvent+136>
>    0x00007ffff60a5286 <+38>:    mov    0x90(%rdx),%rdi
>    0x00007ffff60a528d <+45>:    callq  0x7ffff60a3160 <_ZNK13QQuickWebPage12eventHandlerEv>
>    0x00007ffff60a5292 <+50>:    mov    %rbx,%rsi
>    0x00007ffff60a5295 <+53>:    mov    %rax,%rdi
>    0x00007ffff60a5298 <+56>:    callq  0x7ffff61143f0 <_ZN21QtWebPageEventHandler11handleEventEP6QEvent>
>    0x00007ffff60a529d <+61>:    test   %al,%al
>    0x00007ffff60a529f <+63>:    jne    0x7ffff60a5318 <_ZN13QQuickWebView5eventEP6QEvent+184>
>    0x00007ffff60a52a1 <+65>:    movzwl 0x10(%rbx),%eax
>    0x00007ffff60a52a5 <+69>:    cmp    $0x53,%eax
>    0x00007ffff60a52a8 <+72>:    je     0x7ffff60a5300 <_ZN13QQuickWebView5eventEP6QEvent+160>
>    0x00007ffff60a52aa <+74>:    mov    %rbx,%rsi
>    0x00007ffff60a52ad <+77>:    mov    %rbp,%rdi
>    0x00007ffff60a52b0 <+80>:    mov    0x8(%rsp),%rbx
>    0x00007ffff60a52b5 <+85>:    mov    0x10(%rsp),%rbp
>    0x00007ffff60a52ba <+90>:    add    $0x18,%rsp
>    0x00007ffff60a52be <+94>:    jmpq   0x7ffff5ff4e60 <_ZN10QQuickItem5eventEP6QEvent@plt>
>    0x00007ffff60a52c3 <+99>:    nopl   0x0(%rax,%rax,1)
>    0x00007ffff60a52c8 <+104>:   cmp    $0x7f,%eax
>    0x00007ffff60a52cb <+107>:   jl     0x7ffff60a52a5 <_ZN13QQuickWebView5eventEP6QEvent+69>
>    0x00007ffff60a52cd <+109>:   cmp    $0x81,%eax
>    0x00007ffff60a52d2 <+114>:   jle    0x7ffff60a5286 <_ZN13QQuickWebView5eventEP6QEvent+38>
>    0x00007ffff60a52d4 <+116>:   lea    -0xc2(%rax),%ecx
>    0x00007ffff60a52da <+122>:   cmp    $0x2,%ecx
>    0x00007ffff60a52dd <+125>:   ja     0x7ffff60a52a5 <_ZN13QQuickWebView5eventEP6QEvent+69>
>    0x00007ffff60a52df <+127>:   jmp    0x7ffff60a5286 <_ZN13QQuickWebView5eventEP6QEvent+38>
>    0x00007ffff60a52e1 <+129>:   nopl   0x0(%rax)
>    0x00007ffff60a52e8 <+136>:   cmp    $0x2,%eax
>    0x00007ffff60a52eb <+139>:   jl     0x7ffff60a52a5 <_ZN13QQuickWebView5eventEP6QEvent+69>
>    0x00007ffff60a52ed <+141>:   cmp    $0x9,%eax
>    0x00007ffff60a52f0 <+144>:   jle    0x7ffff60a5286 <_ZN13QQuickWebView5eventEP6QEvent+38>
>    0x00007ffff60a52f2 <+146>:   cmp    $0x1f,%eax
>    0x00007ffff60a52f5 <+149>:   jne    0x7ffff60a52a5 <_ZN13QQuickWebView5eventEP6QEvent+69>
>    0x00007ffff60a52f7 <+151>:   jmp    0x7ffff60a5286 <_ZN13QQuickWebView5eventEP6QEvent+38>
>    0x00007ffff60a52f9 <+153>:   nopl   0x0(%rax)
>    0x00007ffff60a5300 <+160>:   xor    %eax,%eax
>    0x00007ffff60a5302 <+162>:   mov    0x8(%rsp),%rbx
>    0x00007ffff60a5307 <+167>:   mov    0x10(%rsp),%rbp
>    0x00007ffff60a530c <+172>:   add    $0x18,%rsp
>    0x00007ffff60a5310 <+176>:   retq   
>    0x00007ffff60a5311 <+177>:   nopl   0x0(%rax)
>    0x00007ffff60a5318 <+184>:   mov    $0x1,%eax
>    0x00007ffff60a531d <+189>:   mov    0x8(%rsp),%rbx
>    0x00007ffff60a5322 <+194>:   mov    0x10(%rsp),%rbp
>    0x00007ffff60a5327 <+199>:   add    $0x18,%rsp
>    0x00007ffff60a532b <+203>:   retq
> 
> And now:
> 
> => 0x00007ffff60a4b50 <+0>:     mov    %rbx,-0x10(%rsp)
>    0x00007ffff60a4b55 <+5>:     mov    %rbp,-0x8(%rsp)
>    0x00007ffff60a4b5a <+10>:    sub    $0x18,%rsp
>    0x00007ffff60a4b5e <+14>:    movzwl 0x10(%rsi),%eax
>    0x00007ffff60a4b62 <+18>:    mov    %rdi,%rbp
>    0x00007ffff60a4b65 <+21>:    mov    %rsi,%rbx
>    0x00007ffff60a4b68 <+24>:    mov    0x20(%rdi),%rdx
>    0x00007ffff60a4b6c <+28>:    cmp    $0xff,%eax
>    0x00007ffff60a4b71 <+33>:    jg     0x7ffff60a4b83 <_ZN13QQuickWebView5eventEP6QEvent+51>
>    0x00007ffff60a4b73 <+35>:    lea    0x1d0b7e6(%rip),%rsi        # 0x7ffff7db0360 <_ZL20s_eventHandledByPage>
>    0x00007ffff60a4b7a <+42>:    movslq %eax,%rcx
>    0x00007ffff60a4b7d <+45>:    cmpb   $0x0,(%rsi,%rcx,1)
>    0x00007ffff60a4b81 <+49>:    jne    0x7ffff60a4ba8 <_ZN13QQuickWebView5eventEP6QEvent+88>
>    0x00007ffff60a4b83 <+51>:    cmp    $0x53,%eax
>    0x00007ffff60a4b86 <+54>:    je     0x7ffff60a4be0 <_ZN13QQuickWebView5eventEP6QEvent+144>
>    0x00007ffff60a4b88 <+56>:    mov    %rbx,%rsi
>    0x00007ffff60a4b8b <+59>:    mov    %rbp,%rdi
>    0x00007ffff60a4b8e <+62>:    mov    0x8(%rsp),%rbx
>    0x00007ffff60a4b93 <+67>:    mov    0x10(%rsp),%rbp
>    0x00007ffff60a4b98 <+72>:    add    $0x18,%rsp
>    0x00007ffff60a4b9c <+76>:    jmpq   0x7ffff5ff4e60 <_ZN10QQuickItem5eventEP6QEvent@plt>
>    0x00007ffff60a4ba1 <+81>:    nopl   0x0(%rax)
>    0x00007ffff60a4ba8 <+88>:    mov    0x90(%rdx),%rdi
>    0x00007ffff60a4baf <+95>:    callq  0x7ffff60a3160 <_ZNK13QQuickWebPage12eventHandlerEv>
>    0x00007ffff60a4bb4 <+100>:   mov    %rbx,%rsi
>    0x00007ffff60a4bb7 <+103>:   mov    %rax,%rdi
>    0x00007ffff60a4bba <+106>:   callq  0x7ffff61144a0 <_ZN21QtWebPageEventHandler11handleEventEP6QEvent>
>    0x00007ffff60a4bbf <+111>:   test   %al,%al
>    0x00007ffff60a4bc1 <+113>:   je     0x7ffff60a4b88 <_ZN13QQuickWebView5eventEP6QEvent+56>
>    0x00007ffff60a4bc3 <+115>:   mov    $0x1,%eax
>    0x00007ffff60a4bc8 <+120>:   mov    0x8(%rsp),%rbx
>    0x00007ffff60a4bcd <+125>:   mov    0x10(%rsp),%rbp
>    0x00007ffff60a4bd2 <+130>:   add    $0x18,%rsp
>    0x00007ffff60a4bd6 <+134>:   retq   
>    0x00007ffff60a4bd7 <+135>:   nopw   0x0(%rax,%rax,1)
>    0x00007ffff60a4be0 <+144>:   xor    %eax,%eax
>    0x00007ffff60a4be2 <+146>:   mov    0x8(%rsp),%rbx
>    0x00007ffff60a4be7 <+151>:   mov    0x10(%rsp),%rbp
>    0x00007ffff60a4bec <+156>:   add    $0x18,%rsp
>    0x00007ffff60a4bf0 <+160>:   retq
> 
> The main motivation to make this change is that QQuickWebView::event is used a lot and the lookup would grow logarithmically by the number of events handled by QQuickWebPage when we could do this in O(1) instead.
> 
> Adding some people to CC... What you think of this?

[Alexis Menard (darktears)]

Comment on attachment 125940 [details]
Patch

Wait wait wait, what is the improvement here? How much faster is that? I want to see numbers, assembly is not enough. This is a huge hack for maybe nothing. I don't remember seeing ::event being a bottleneck while working in Qt (but I maybe wrong). We're not yet in the state of micro-optimizing our code. I believe that callgrind will show much more bottleneck than this one.

[Alexis Menard (darktears)]

(In reply to comment #5)
> (From update of attachment 125940 [details])
> Wait wait wait, what is the improvement here? How much faster is that? I want to see numbers, assembly is not enough. This is a huge hack for maybe nothing. I don't remember seeing ::event being a bottleneck while working in Qt (but I maybe wrong). We're not yet in the state of micro-optimizing our code. I believe that callgrind will show much more bottleneck than this one.

Also what is the balance here memory usage of this map vs the tiny optimization?

One way to optimize the assembly code here is to group each case to avoid big hole (which is the case today) starting let say from the smallest value to the highest, that would help the compiler to maybe generate a jump table for this switch/case and maybe lead to worst case O(n) best case O(1).

[Rafael Brandao]

(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 125940 [details] [details])
> > Wait wait wait, what is the improvement here? How much faster is that? I want to see numbers, assembly is not enough. This is a huge hack for maybe nothing. I don't remember seeing ::event being a bottleneck while working in Qt (but I maybe wrong). We're not yet in the state of micro-optimizing our code. I believe that callgrind will show much more bottleneck than this one.
> 
> Also what is the balance here memory usage of this map vs the tiny optimization?
> 
> One way to optimize the assembly code here is to group each case to avoid big hole (which is the case today) starting let say from the smallest value to the highest, that would help the compiler to maybe generate a jump table for this switch/case and maybe lead to worst case O(n) best case O(1).

Ok I'll look into numbers for it (valgrind/cachegrind is an unknown tool for me yet). Still it looks wrong to me that we are not already doing this fast enough, but I believe you may be right, this may not be a bottleneck. In the worse case we can skip this task and fix at some point later (when we get into that stage of micro-optimization), but I can only say more about it tomorrow. Thanks for the feedback! :-)

[Simon Hausmann]

Comment on attachment 125940 [details]
Patch

I think the best solution is to get rid of this code altogether. With and without your patch we do a type switch _three_ times. Once in QtQuick, which calls our specific mouseFooEvent, touchEvent, etc. handlers in QQuickWebView. We throw away that result and dispatch once more in QQuickWebView::event, just to throw that away again and do another type dispatch in QtWebPageEventHandler. Let's get rid of our own type dispatch and simply change for example void QQuickWebView::mouseReleaseEvent(QMouseEvent* event) to:

void QQuickWebView::mouseReleaseEvent(QMouseEvent* event)
{
    d->pageView->eventHandler()->mouseReleaseEvent(event);
}

Am I missing something?

[Andras Becsi]

(In reply to comment #8)
> (From update of attachment 125940 [details])
> I think the best solution is to get rid of this code altogether.

I agree with Simon on this, AFAIR the function originates from the time where we did not have the page event handler yet and the item receiving the events was the page, not the view.

[Rafael Brandao]

(In reply to comment #8)
> (From update of attachment 125940 [details])
> I think the best solution is to get rid of this code altogether. With and without your patch we do a type switch _three_ times. Once in QtQuick, which calls our specific mouseFooEvent, touchEvent, etc. handlers in QQuickWebView. We throw away that result and dispatch once more in QQuickWebView::event, just to throw that away again and do another type dispatch in QtWebPageEventHandler. Let's get rid of our own type dispatch and simply change for example void QQuickWebView::mouseReleaseEvent(QMouseEvent* event) to:
> 
> void QQuickWebView::mouseReleaseEvent(QMouseEvent* event)
> {
>     d->pageView->eventHandler()->mouseReleaseEvent(event);
> }
> 
> Am I missing something?

Good point Simon, it seems more reasonable.

[Simon Hausmann]

Created attachment 128710 [details]
Patch

[Alexis Menard (darktears)]

Comment on attachment 128710 [details]
Patch

LGTM. Nice cleanup.

[Kenneth Rohde Christiansen]

Comment on attachment 128710 [details]
Patch

Nice!

[Rafael Brandao]

Comment on attachment 128710 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=128710&action=review

nice :)

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1292
> +    d->pageView->eventHandler()->handleInputMethodEvent(event);

Can you confirm if we're still propagating this event? I recall that I've tried to reproduce it when I was looking into this but I couldn't find out. If we are not using it, maybe we should just remove it.

[Alexis Menard (darktears)]

Comment on attachment 128710 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=128710&action=review

>> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1292
>> +    d->pageView->eventHandler()->handleInputMethodEvent(event);
> 
> Can you confirm if we're still propagating this event? I recall that I've tried to reproduce it when I was looking into this but I couldn't find out. If we are not using it, maybe we should just remove it.

It is used whenever an input method exists in the app :D.

[Simon Hausmann]

Created attachment 129437 [details]
Patch

[Simon Hausmann]

(In reply to comment #16)
> Created an attachment (id=129437) [details]
> Patch

New patch. I landed the original patch but had to roll it out again, because many WK2 tests failed. It turned out that this is caused by WTR sending events directly to the QQuickItem instead of sending them through the QQuickCanvas. The latter dispatches the events directly to QQuickItem::fooEvent(QFooEvent*), _not_ going through ::event. I'll change WTR to send the events through the canvas, like it would happen in a "real" application.

[Simon Hausmann]

Committed r109326: <http://trac.webkit.org/changeset/109326>

[Simon Hausmann]

Comment on attachment 129437 [details]
Patch

Clearning review. Strange that webkit-patch didn't do it this time around :)