[webkit-reviews] review granted: [Bug 111429] [Qt][MiniBrowser] Improve the update of touch indicators : [Attachment 191473] Patch

[bugzilla-daemon at webkit.org]

Jocelyn Turcotte <jocelyn.turcotte at digia.com> has granted Andras Becsi
<abecsi at webkit.org>'s request for review:
Bug 111429: [Qt][MiniBrowser] Improve the update of touch indicators
https://bugs.webkit.org/show_bug.cgi?id=111429

Attachment 191473: Patch
https://bugs.webkit.org/attachment.cgi?id=191473&action=review

------- Additional Comments from Jocelyn Turcotte <jocelyn.turcotte at digia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=191473&action=review


> Tools/MiniBrowser/qt/BrowserWindow.cpp:79
> +    m_activeMockComponents.reserve(3);

I think that at this level it won't have any noticeable benefit, if any.

>>>> Tools/MiniBrowser/qt/BrowserWindow.cpp:122
>>>> +	  if (touchPoints.isEmpty()) {
>>> 
>>> Is it possible that we get a partial list of touchpoints where some IDs
would be missing?
>> 
>> The problem is not missing IDs, it's just that not all events have released
state.
>> 
>> We receive an empty list because of a released control key an this results
in problems if you test gesture chains, since not all the states are released
by the time the control key is released.
>> 
>> Maybe this makes more sense in context with:
>> https://bugs.webkit.org/show_bug.cgi?id=111430
> 
> To also answer your actual question :)
> It looks like we always receive all the possible ID because touch points are
updated for all combinations.
> But I can verify that if you have concerns.

Ok I see what you mean, it's passed that way explicitely by
MiniBrowserApplication::sendTouchEvent.
I was wondering if it was worth checking for missing ids in the list as a
general solution instead of checking the only subcase that interests us:
"isEmpty()".

An explicit way would be to have a clearVisualMockTouchPoints method that does
this. Just an idea, both are fine to me so r=me.