Bug 78509

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

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

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

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

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

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.

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

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?

> > 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.

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

> > 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?

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?

(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.

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.

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