Bug 78509 – [BlackBerry] Upstream touch handling related classes

• Status: RESOLVED
• Resolution: FIXED
• Component: UI Events

[Antonio Gomes]

It mostly accounts for TouchEventHandler.cpp|h and FatFingers.cpp|h

[Antonio Gomes]

Created attachment 127393 [details]
patch_1/3 v1 - TouchEventHandler

[Antonio Gomes]

Created attachment 127395 [details]
patch_2/3 v1 - FatFingers

[Antonio Gomes]

Created attachment 127396 [details]
patch_1/3 v2 - TouchEventHandler

[Antonio Gomes]

Created attachment 127397 [details]
patch_2/3 v2 - FatFingers

[Antonio Gomes]

Created attachment 127398 [details]
(landed r107990) patch_3/3 v1 - InRegionScrollableArea

[Rob Buis]

Comment on attachment 127396 [details]
patch_1/3 v2 - TouchEventHandler

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

Looks good, still soem issues.

> Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.cpp:74
> +{

I think ASSERT(element); needs to be done here and not in the above two methods, assuming those are used by elementExpectsMouseEvents only.

> Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.cpp:96
> +    // Check for plugin

Add period :)

> Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.cpp:236
> +            // Create  MouseReleased Event.

Double spacing.

> Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.cpp:239
> +            m_lastFatFingersResult.reset(); // reset the fat finger result as its no longer valid when a user's finger is not on the screen.

reset -> Reset

> Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.cpp:242
> +            if (spellLength) {

Can combine these two, also just 'unsigned' is enough and actually is what spellCheck returns.

> Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.cpp:244
> +                unsigned int start = end - spellLength;

Can be just unsigned.

> Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.cpp:282
> +    IntPoint newContentPos = IntPoint(rect.x() + rect.width(), rect.y() + rect.height() / 2); // midway of right edge

No sentence...

> Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.cpp:297
> +        // Send the mouse move event

No proper sentences.

> Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.cpp:301
> +        // Then send the MousePressed event

Lacks period.

> Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.cpp:312
> +    bool isArea = elementUnderFatFinger->hasTagName(HTMLNames::areaTag);

Can be moved below the if.

> Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.cpp:316
> +    // renderer.

Is this the right position for this comment?

> Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.cpp:374
> +    // FIXME: We can get more precise on the MAP case by calculating the rect with HTMLAreaElement::computeRect().

MAP case?

> Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.h:2
> + * Copyright (C) 2010, 2011 Research In Motion Limited. All rights reserved.

Once you start changing this, 2012 can be added.

> Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.h:30
> +class IntPoint;

Maybe not needed since we include it?

> Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.h:31
> +class Element;

Does not seem used.

> Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.h:44
> +    bool handleTouchPoint(BlackBerry::Platform::TouchPoint&);

BlackBerry:: prefix can be dropped in this header.

[Antonio Gomes]

Created attachment 127416 [details]
(landed r107972) patch_1/3 v3 -TouchEventHandler

[Rob Buis]

Comment on attachment 127397 [details]
patch_2/3 v2 - FatFingers

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

Looks good, still some issues.

> Source/WebKit/blackberry/WebKitSupport/FatFingers.cpp:2
> + * Copyright (C) 2010, 2011 Research In Motion Limited. All rights reserved.

2012 should be added.

> Source/WebKit/blackberry/WebKitSupport/FatFingers.cpp:45
> +#include "WebPage_p.h"

Is above minimal?

> Source/WebKit/blackberry/WebKitSupport/FatFingers.cpp:56
> +using WTF::Vector;

WTF does the above itself, can be removed.

> Source/WebKit/blackberry/WebKitSupport/FatFingers.cpp:124
> +    Element* element = toElement(node);

Not sure if temp var is needed here.

> Source/WebKit/blackberry/WebKitSupport/FatFingers.cpp:185
> +        // If we are looking for a Clickable Element and we found one, we can quite early.

quite -> quit.

> Source/WebKit/blackberry/WebKitSupport/FatFingers.cpp:200
> +    // force blit to make the fat fingers rects show up

Not a proper sentence.

> Source/WebKit/blackberry/WebKitSupport/FatFingers.cpp:330
> +        bool isElement = curNode->isElementNode();

isElement maybe not needed.

> Source/WebKit/blackberry/WebKitSupport/FatFingers.h:2
> + * Copyright (C) 2010, 2011 Research In Motion Limited. All rights reserved.

2012 should be added.

> Source/WebKit/blackberry/WebKitSupport/FatFingers.h:23
> +#include "RenderLayer.h"

Is this one needed? Also it is in the cpp as well.

> Source/WebKit/blackberry/WebKitSupport/FatFingers.h:41
> +namespace BlackBerry {

You can remove BlackBerry:: prefix throughout this header.

> Source/WebKit/blackberry/WebKitSupport/FatFingers.h:100
> +    const FatFingersResult findBestPoint();

Can this be const?

> Source/WebKit/blackberry/WebKitSupport/FatFingers.h:109
> +#endif

DEBUG_FAT_FINGERS could be moved into .cpp if these were just local static vars in the .cpp.

> Source/WebKit/blackberry/WebKitSupport/FatFingers.h:112
> +

No need for empty line.

> Source/WebKit/blackberry/WebKitSupport/FatFingers.h:135
> +    void setSuccessfulFatFingersResult(FatFingersResult&, WebCore::Node*, const WebCore::IntPoint&);

Can this be const?

[Antonio Gomes]

> > Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.cpp:74
> > +{
> 
> I think ASSERT(element); needs to be done here and not in the above two methods, assuming those are used by elementExpectsMouseEvents only.

Nice! I added in all of them instead.

> > Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.cpp:96
> > +    // Check for plugin
> 
> Add period :)

Removed the unneeded comment instead.
 
> > Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.cpp:236
> > +            // Create  MouseReleased Event.
> 
> Double spacing.

Fixed.

 
> > Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.cpp:239
> > +            m_lastFatFingersResult.reset(); // reset the fat finger result as its no longer valid when a user's finger is not on the screen.
> 
> reset -> Reset

Fixed.

> > Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.cpp:242
> > +            if (spellLength) {
> 
> Can combine these two, also just 'unsigned' is enough and actually is what spellCheck returns.
> > Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.cpp:244
> > +                unsigned int start = end - spellLength;
> 
> Can be just unsigned.

Changed them all to unsigned. As a matter of self-documenting the method return value I left the spellLength, unless you feel strong about remove it..

> > Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.cpp:282
> > +    IntPoint newContentPos = IntPoint(rect.x() + rect.width(), rect.y() + rect.height() / 2); // midway of right edge
> 
> No sentence...

Dropped the comment.

> > Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.cpp:297
> > +        // Send the mouse move event
> 
> No proper sentences.

Ditto.

> > Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.cpp:301
> > +        // Then send the MousePressed event
> 
> Lacks period.

Ditto.

> > Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.cpp:312
> > +    bool isArea = elementUnderFatFinger->hasTagName(HTMLNames::areaTag);
> 
> Can be moved below the if.

Done.

> > Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.cpp:316
> > +    // renderer.
> 
> Is this the right position for this comment?

No, fixed.

> > Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.cpp:374
> > +    // FIXME: We can get more precise on the MAP case by calculating the rect with HTMLAreaElement::computeRect().
> 
> MAP case?

:%s/MAP/<map>/g

> > Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.h:2
> > + * Copyright (C) 2010, 2011 Research In Motion Limited. All rights reserved.
> 
> Once you start changing this, 2012 can be added.

Done.

> > Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.h:30
> > +class IntPoint;
> 
> Maybe not needed since we include it?
> 
> > Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.h:31
> > +class Element;
> 
> Does not seem used

Removed both.

> > Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.h:44
> > +    bool handleTouchPoint(BlackBerry::Platform::TouchPoint&);
> 
> BlackBerry:: prefix can be dropped in this header.

True, removed throughout the files.

[Antonio Gomes]

Created attachment 127418 [details]
(landed r107978) patch_2/3 v3 - FatFingers

[Antonio Gomes]

> > Source/WebKit/blackberry/WebKitSupport/FatFingers.cpp:2
> > + * Copyright (C) 2010, 2011 Research In Motion Limited. All rights reserved.
> 
> 2012 should be added.

Fixed.

> > Source/WebKit/blackberry/WebKitSupport/FatFingers.cpp:45
> > +#include "WebPage_p.h"
> 
> Is above minimal?

No, removed one unneeded: RenderLayer.h

> > Source/WebKit/blackberry/WebKitSupport/FatFingers.cpp:56
> > +using WTF::Vector;
> 
> WTF does the above itself, can be removed.

Ok.

> > Source/WebKit/blackberry/WebKitSupport/FatFingers.cpp:124
> > +    Element* element = toElement(node);
> 
> Not sure if temp var is needed here.

Left here, but let me know if you feel strong about removing it.

> > Source/WebKit/blackberry/WebKitSupport/FatFingers.cpp:185
> > +        // If we are looking for a Clickable Element and we found one, we can quite early.
> 
> quite -> quit.

Fixed.

> > Source/WebKit/blackberry/WebKitSupport/FatFingers.cpp:200
> > +    // force blit to make the fat fingers rects show up
> 
> Not a proper sentence.

Fixed.

> > Source/WebKit/blackberry/WebKitSupport/FatFingers.cpp:330
> > +        bool isElement = curNode->isElementNode();
> 
> isElement maybe not needed.

It is used in two spots, so I left for now. Let me know if you feel strong about it.

> > Source/WebKit/blackberry/WebKitSupport/FatFingers.h:2
> > + * Copyright (C) 2010, 2011 Research In Motion Limited. All rights reserved.
> 
> 2012 should be added.

Done.

> > Source/WebKit/blackberry/WebKitSupport/FatFingers.h:23
> > +#include "RenderLayer.h"
> 
> Is this one needed? Also it is in the cpp as well.

Needed here, but removed from the .cpp

> > Source/WebKit/blackberry/WebKitSupport/FatFingers.h:41
> > +namespace BlackBerry {
> 
> You can remove BlackBerry:: prefix throughout this header.

Cool done.

> > Source/WebKit/blackberry/WebKitSupport/FatFingers.h:100
> > +    const FatFingersResult findBestPoint();
> 
> Can this be const?

No.

> > Source/WebKit/blackberry/WebKitSupport/FatFingers.h:109
> > +#endif
> 
> DEBUG_FAT_FINGERS could be moved into .cpp if these were just local static vars in the .cpp.

They are used in BackingStore.cpp
 
> > Source/WebKit/blackberry/WebKitSupport/FatFingers.h:112
> > +
> 
> No need for empty line.

Done.I

> > Source/WebKit/blackberry/WebKitSupport/FatFingers.h:135
> > +    void setSuccessfulFatFingersResult(FatFingersResult&, WebCore::Node*, const WebCore::IntPoint&);
> 
> Can this be const?

Maybe, but a setter being 'const' might look even more strange?

[Rob Buis]

Comment on attachment 127416 [details]
(landed r107972) patch_1/3 v3 -TouchEventHandler

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

Looks good, with some stuff to do before landing.

> Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.cpp:25
> +#include "FatFingers.h"

Already included in header.

> Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.cpp:48
> +#include "WebSettings.h"

Before landing please check this is minimal number of includes.

> Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.h:22
> +#include "ChromeClient.h"

Not needed?

[Antonio Gomes]

(In reply to comment #12)
> (From update of attachment 127416 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127416&action=review
> 
> Looks good, with some stuff to do before landing.
> 
> > Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.cpp:25
> > +#include "FatFingers.h"
> 
> Already included in header.

Removed.


> > Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.cpp:48
> > +#include "WebSettings.h"
> 
> Before landing please check this is minimal number of includes.

Removed

> > Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.h:22
> > +#include "ChromeClient.h"
> 
> Not needed?

It is needed.

[Rob Buis]

Comment on attachment 127418 [details]
(landed r107978) patch_2/3 v3 - FatFingers

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

Looks good.

> Source/WebKit/blackberry/WebKitSupport/FatFingers.cpp:44
> +#include "WebPage_p.h"

Is this minimal?

> Source/WebKit/blackberry/WebKitSupport/FatFingers.cpp:54
> +using WTF::String;

Probably not needed.

> Source/WebKit/blackberry/WebKitSupport/FatFingers.cpp:315
> +    // Iterate over the list of nodes (and subrects of nodes where possible), for each saving the intersection of the bounding box with the finger rect

Lacks period.

> Source/WebKit/blackberry/WebKitSupport/FatFingers.cpp:416
> +        // Iterate through all words, breaking at whitespace, to find the bounding box of each word

Lacks period.

[Rob Buis]

Comment on attachment 127398 [details]
(landed r107990) patch_3/3 v1 - InRegionScrollableArea

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

> Source/WebKit/blackberry/WebKitSupport/InRegionScrollableArea.cpp:2
> + * Copyright (C) 2011 Research In Motion Limited. All rights reserved.

Could add 2012.

> Source/WebKit/blackberry/WebKitSupport/InRegionScrollableArea.cpp:67
> +        IntRect transformedWindowRect = IntRect(IntPoint(0, 0), m_webPage->transformedViewportSize());

Could use IntPoint::zero