Bug 74380

Created attachment 127479 [details] Patch

Comment on attachment 127479 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=127479&action=review I did a bit of this long file. I think we have maybe enough for another round. I am concerned about introducing deadcode. Lets ask George, Eli and Yong if we can just delete the old link-to-link code (i pointed it out somewhere below). Also I would like to suggest the following: having webpage:xxx() and the associated webpageprivate::xxx() method code to each other. Move some public WebPage methods up in the file, etc. > Source/WebKit/blackberry/Api/WebPage.cpp:263 > +WebPagePrivate::WebPagePrivate(WebPage* webPage, WebPageClient* client, const Platform::IntRect& rect) lets make this ctor receive a WebCore::IntRect (so no prefix) instead of Platform::IntRect > Source/WebKit/blackberry/Api/WebPage.cpp:296 > + , m_inPageSearchManager(new InPageSearchManager(this)) this leaks > Source/WebKit/blackberry/Api/WebPage.cpp:368 > +#if ENABLE_DRT > + delete m_dumpRenderTree; > + m_dumpRenderTree = 0; > +#endif > + > +} extra line > Source/WebKit/blackberry/Api/WebPage.cpp:462 > + m_page->settings()->setDelegateSelectionPaint(true); > + > +} extra line > Source/WebKit/blackberry/Api/WebPage.cpp:752 > + setScrollPosition(IntPoint(0, 0)); nit: intpoint::zero() here > Source/WebKit/blackberry/Api/WebPage.cpp:917 > + // Should only be invoked when text reflow is enabled nit: dot at the end > Source/WebKit/blackberry/Api/WebPage.cpp:1232 > + if (!m_mainFrame->contentRenderer()) > + return IntSize(); > + return IntSize(m_mainFrame->contentRenderer()->rightAbsoluteVisibleOverflow(), m_mainFrame->contentRenderer()->bottomAbsoluteVisibleOverflow()); nit: add an empty line here for consistency > Source/WebKit/blackberry/Api/WebPage.cpp:1342 > +#if !OS(QNX) > + else { > + // On handset, if a web page would fit in landscape mode use mobile. > + int maxMobileWidth = Platform::Graphics::Screen::landscapeWidth(); > + > + if (contentsSize().width() > maxMobileWidth > + || absoluteVisibleOverflowSize().width() > maxMobileWidth) > + needsLayout = setViewMode(Desktop); > + else > + needsLayout = setViewMode(Mobile); > + } > +#else > + else > + needsLayout = setViewMode(Desktop); > +#endif please ask arvid, adam or eli if we really want to stick with this !QNX block. > Source/WebKit/blackberry/Api/WebPage.cpp:1641 > + return mapToTransformed(IntRect(IntPoint(0, 0), size)).size(); intpoint:zero() > Source/WebKit/blackberry/Api/WebPage.cpp:1646 > + return mapFromTransformed(IntRect(IntPoint(0, 0), size)).size(); ditto > Source/WebKit/blackberry/Api/WebPage.cpp:1657 > + rect.intersect(IntRect(IntPoint(0, 0), transformedContentsSize())); ditto > Source/WebKit/blackberry/Api/WebPage.cpp:1732 > + // FIXME: Temp solution. We'll get back to this. yeah? > Source/WebKit/blackberry/Api/WebPage.cpp:1758 > +Platform::NetworkStreamFactory* WebPagePrivate::networkStreamFactory() > +{ > + return m_client->networkStreamFactory(); > +} > + > + double lines > Source/WebKit/blackberry/Api/WebPage.cpp:1938 > + WTF::String url = stripLeadingAndTrailingHTMLSpaces(imageElement->getAttribute(HTMLNames::srcAttr).string()); remove the WTF:: prefix > Source/WebKit/blackberry/Api/WebPage.cpp:1942 > + WTF::String alt = imageElement->altText(); ditto > Source/WebKit/blackberry/Api/WebPage.cpp:2318 > +class ComplicatedDistance { > +public: > + > + enum DistanceType { > + DistanceNone, > + DistanceApart, > + DistanceIntersected, > + DistanceOverlapped, > + }; > + > + DistanceType m_type; > + int m_dist; > + > + ComplicatedDistance() : m_type(DistanceNone), m_dist(0) { } > + > + void calcDistance(int baseStart, int baseEnd, int targetStart, int targetEnd) > + { > + m_dist = targetEnd <= baseStart ? targetEnd - baseStart - 1 > + : targetStart >= baseEnd ? targetStart - baseEnd + 1 > + : 0; > + if (m_dist) { > + m_type = DistanceApart; > + return; > + } > + > + m_dist = targetStart == baseStart > + ? 0 > + : targetStart < baseStart > + ? targetEnd >= baseEnd ? 0 : baseStart - targetEnd > + : targetEnd <= baseEnd ? 0 : baseEnd - targetStart; > + if (m_dist) { > + m_type = DistanceIntersected; > + return; > + } > + > + m_dist = targetStart - baseStart + targetEnd - baseEnd; > + m_type = DistanceOverlapped; > + } > + > + bool isPositive() const { return m_dist >= 0; } > + bool isNegative() const { return m_dist <= 0; } > + > + bool operator<(const ComplicatedDistance& o) const > + { > + return m_type != o.m_type > + ? m_type > o.m_type > + : m_type == DistanceIntersected > + ? abs(m_dist) > abs(o.m_dist) > + : abs(m_dist) < abs(o.m_dist); > + } > + bool operator==(const ComplicatedDistance& o) const > + { > + return m_type == o.m_type && m_dist == o.m_dist; > + } > +}; > + > +static inline bool checkDirection(Platform::ScrollDirection dir, const ComplicatedDistance& delta, Platform::ScrollDirection negDir, Platform::ScrollDirection posDir) > +{ > + return dir == negDir ? delta.isNegative() : dir == posDir ? delta.isPositive() : false; > +} > + > +static inline bool areNodesPerpendicularlyNavigatable(Platform::ScrollDirection dir, const IntRect& baseRect, const IntRect& targetRect) > +{ > + ComplicatedDistance distX, distY; > + distX.calcDistance(baseRect.x(), baseRect.maxX(), targetRect.x(), targetRect.maxX()); > + distY.calcDistance(baseRect.y(), baseRect.maxY(), targetRect.y(), targetRect.maxY()); > + > + return distY < distX ? dir == Platform::ScrollUp || dir == Platform::ScrollDown > + : dir == Platform::ScrollLeft || dir == Platform::ScrollRight; > +} > + > +struct NodeDistance { > + ComplicatedDistance m_dist; > + ComplicatedDistance m_perpendicularDist; > + IntRect m_rect; > + > + bool isCloser(const NodeDistance& o, Platform::ScrollDirection dir) const > + { > + return m_dist.m_type == o.m_dist.m_type > + ? (m_dist == o.m_dist ? m_perpendicularDist < o.m_perpendicularDist > + : m_dist < o.m_dist ? m_perpendicularDist.m_type >= o.m_perpendicularDist.m_type > + || !areNodesPerpendicularlyNavigatable(dir, o.m_rect, m_rect) > + : m_perpendicularDist.m_type > o.m_perpendicularDist.m_type > + && areNodesPerpendicularlyNavigatable(dir, m_rect, o.m_rect)) > + : m_dist.m_type > o.m_dist.m_type; > + } > +}; > + > +static inline bool tellDistance(Platform::ScrollDirection dir, const IntRect& baseRect, NodeDistance& nodeDist) > +{ > + ComplicatedDistance distX, distY; > + distX.calcDistance(baseRect.x(), baseRect.maxX(), nodeDist.m_rect.x(), nodeDist.m_rect.maxX()); > + distY.calcDistance(baseRect.y(), baseRect.maxY(), nodeDist.m_rect.y(), nodeDist.m_rect.maxY()); > + > + bool horizontal = distY < distX; > + if (horizontal ? checkDirection(dir, distX, Platform::ScrollLeft, Platform::ScrollRight) : checkDirection(dir, distY, Platform::ScrollUp, Platform::ScrollDown)) { > + nodeDist.m_dist = horizontal ? distX : distY; > + nodeDist.m_perpendicularDist = horizontal ? distY : distX; > + return true; > + } > + return false; > +} this is unused link-to-link code. lets clean this up. > Source/WebKit/blackberry/Api/WebPage.cpp:2479 > +bool WebPagePrivate::moveToNextField(Platform::ScrollDirection dir, int desiredScrollAmount) > +{ > + Frame* frame = focusedOrMainFrame(); > + if (!frame) > + return false; > + > + return moveToNextField(frame, dir, desiredScrollAmount); > +} > + > +bool WebPagePrivate::moveToNextField(Frame* frame, Platform::ScrollDirection dir, int desiredScrollAmount) > +{ > + Document* doc = frame->document(); > + FrameView* view = frame->view(); > + if (!doc || !view || view->needsLayout()) > + return false; > + > + // The layout needs to be up-to-date to determine if a node is focusable. > + doc->updateLayoutIgnorePendingStylesheets(); > + > + Node* origFocusedNode = doc->focusedNode(); > + > + Node* startNode = m_page->focusController()->findFocusableNode(FocusDirectionForward, doc, origFocusedNode, 0); > + > + // Last node? get back to beginning. > + if (!startNode && origFocusedNode) > + startNode = m_page->focusController()->findFocusableNode(FocusDirectionForward, doc, 0, 0); > + > + if (!startNode || startNode == origFocusedNode) > + return false; > + > + Node* origRadioGroup = origFocusedNode > + && ((origFocusedNode->hasTagName(HTMLNames::inputTag) > + && static_cast<HTMLInputElement*>(origFocusedNode)->isRadioButton())) > + ? origFocusedNode->parentNode() : 0; > + > + IntRect visibleRect = IntRect(IntPoint(), actualVisibleSize()); > + // Constrain the rect if this is a subframe. > + if (view->parent()) > + visibleRect.intersect(getRecursiveVisibleWindowRect(view)); > + > + IntRect baseRect; > + if (origRadioGroup) > + baseRect = getNodeWindowRect(origRadioGroup); > + else if (origFocusedNode) > + baseRect = getNodeWindowRect(origFocusedNode); > + else { > + baseRect = visibleRect; > + switch (dir) { > + case Platform::ScrollLeft: > + baseRect.setX(baseRect.maxX()); > + case Platform::ScrollRight: > + baseRect.setWidth(0); > + break; > + case Platform::ScrollUp: > + baseRect.setY(baseRect.maxY()); > + case Platform::ScrollDown: > + baseRect.setHeight(0); > + break; > + } > + } > + > + Node* bestChoice = 0; > + NodeDistance bestChoiceDistance; > + Node* lastCandidate = 0; > + IntRect bestChoiceRect = baseRect; > + Node* cur = startNode; > + > + do { > + if (cur->isFocusable()) { > + Node* curRadioGroup = (cur->hasTagName(HTMLNames::inputTag) > + && static_cast<HTMLInputElement*>(cur)->isRadioButton()) > + ? cur->parentNode() : 0; > + if (!curRadioGroup || static_cast<HTMLInputElement*>(cur)->checked()) { > + IntRect nodeRect = curRadioGroup ? getNodeWindowRect(curRadioGroup) : getNodeWindowRect(cur); > + // Note, if for some reason the nodeRect is 0x0, intersects will not match. This shouldn't be a > + // problem as we don't want to focus on a non-visible node. > + if (visibleRect.intersects(nodeRect)) { > + NodeDistance nodeDistance; > + nodeDistance.m_rect = nodeRect; > + if (tellDistance(dir, baseRect, nodeDistance)) { > + if (!bestChoice > + || nodeDistance.isCloser(bestChoiceDistance, dir)) { > + bestChoice = cur; > + bestChoiceDistance = nodeDistance; > + bestChoiceRect = nodeRect; > + } > + } else > + lastCandidate = cur; > + } > + } > + } > + > + cur = m_page->focusController()->findFocusableNode(FocusDirectionForward, doc, cur, 0); > + > + // Last node? go back > + if (!cur && origFocusedNode) > + cur = m_page->focusController()->findFocusableNode(FocusDirectionForward, doc, 0, 0); > + } while (cur && cur != origFocusedNode && cur != startNode); > + > + if (!bestChoice && !origFocusedNode) { > + // In the case that previously no node is on focus. > + bestChoice = lastCandidate; > + } > + > + if (bestChoice) { > + // FIXME: Temporarily disable automatical scrolling for HTML email. > +#if 0 > + if (Document* nodeDoc = bestChoice->document()) { > + if (FrameView* nodeView = nodeDoc->view()) > + nodeView->scrollRectIntoViewRecursively(bestChoice->getRect()); > + } > +#endif > + > + if (bestChoice->isElementNode()) > + static_cast<Element*>(bestChoice)->focus(true); > + else > + m_page->focusController()->setFocusedNode(bestChoice, bestChoice->document()->frame()); > + > + // Trigger a repaint of the node rect as it frequently isn't updated due to JavaScript being > + // disabled. See RIM Bug #1242. > + m_backingStore->d->repaint(bestChoiceRect, true /* contentChanged */, false /* immediate */); > + > + return true; > + } > + > + if (Frame* parent = frame->tree()->parent()) { > + if (moveToNextField(parent, dir, desiredScrollAmount)) > + return true; > + } > + > + return false; > +} Same here. > Source/WebKit/blackberry/Api/WebPage.cpp:2546 > +static inline int distanceBetweenPoints(IntPoint p1, IntPoint p2) > +{ > + // Change int to double, because (dy * dy) can cause int overflow in reality, e.g, (-46709 * -46709). > + double dx = static_cast<double>(p1.x() - p2.x()); > + double dy = static_cast<double>(p1.y() - p2.y()); > + return sqrt((dx * dx) + (dy * dy)); > +} same here. > Source/WebKit/blackberry/Api/WebPage.cpp:2888 > + renderer->style()->isLeftToRightDirection() ? setScrollPosition(topLeftPoint) : setScrollPosition(IntPoint(reflowedRect.x() + reflowedRect.width() - actualVisibleSize().width(), topLeftPoint.y())); this is a long line. > Source/WebKit/blackberry/Api/WebPage.cpp:2921 > +WebPage::WebPage(WebPageClient* client, const WebString& pageGroupName, const Platform::IntRect& rect) > +{ > + globalInitialize(); > + d = new WebPagePrivate(this, client, rect); > + d->init(pageGroupName); > +} lets move this code up in the file. > Source/WebKit/blackberry/Api/WebPage.cpp:2938 > + > +void WebPage::destroy() > +{ > + // TODO: need to verify if this call needs to be made before calling > + // WebPage::destroyWebPageCompositor() > + d->m_backingStore->d->suspendScreenAndBackingStoreUpdates(); > + ditto > Source/WebKit/blackberry/Api/WebPage.cpp:2971 > +WebPage::~WebPage() > +{ > + delete d; > + d = 0; > +} > + > +WebPageClient* WebPage::client() const > +{ > + return d->m_client; > +} ditto > Source/WebKit/blackberry/Api/WebPage.cpp:3175 > +void WebPage::setCaretHighlightStyle(Platform::CaretHighlightStyle style) > +{ > +} lets be sure it is not dead code.

lets also fix this: /Devel/webkit/Source/WebKit/blackberry/Api/WebPage.cpp: In member function 'bool BlackBerry::WebKit::WebPage::touchEvent(const BlackBerry::Platform::TouchEvent&)': /Devel/webkit/Source/WebKit/blackberry/Api/WebPage.cpp:3778:12: warning: enumeration value 'TouchCancel' not handled in switch [-Wswitch] /Devel/webkit/Source/WebKit/blackberry/Api/WebPage.cpp:3790:45: warning: comparison between signed and unsigned integer expressions [-Wsign-compare] /Devel/RIM/webkit/Source/WebKit/blackberry/Api/WebPage.cpp:3791:16: warning: enumeration value 'TouchStationary' not handled in switch [-Wswitch]

Comment on attachment 127479 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=127479&action=review Partial review 3000 - 5000 > Source/WebKit/blackberry/Api/WebPage.cpp:3237 > + FloatSize currentPPI = Platform::Graphics::Screen::pixelsPerInch(-1); What is -1 here? > Source/WebKit/blackberry/Api/WebPage.cpp:3340 > + m_backingStore->d->orientationChanged(); // Updates tile geometry and creates visible tile buffer Lacks period. > Source/WebKit/blackberry/Api/WebPage.cpp:3539 > + IntRect viewportRect = IntRect(IntPoint(0, 0), transformedActualVisibleSize); IntPoint::zero > Source/WebKit/blackberry/Api/WebPage.cpp:3541 > + = enclosingIntRect(rotationMatrix.inverse().mapRect(FloatRect(viewportRect))); Can be one line I think. > Source/WebKit/blackberry/Api/WebPage.cpp:3708 > + d->showVirtualKeyboard(visible); what if we just do d->m_client->inputSetNavigationMode(visible); here? That way we can remove showVirtualKeyboard... > Source/WebKit/blackberry/Api/WebPage.cpp:3804 > + No need for empty line. > Source/WebKit/blackberry/Api/WebPage.cpp:3869 > +bool WebPagePrivate::dispatchTouchEventToFullScreenPlugin(PluginView* plugin, const Platform::TouchEvent& event) Should be closer to dispatchMouseEventToFullScreenPlugin. > Source/WebKit/blackberry/Api/WebPage.cpp:4059 > + Node* ownerNode = static_cast<Node*>(frame->ownerElement()); Is the static_cast needed? An Element is a Node too. > Source/WebKit/blackberry/Api/WebPage.cpp:4237 > +void WebPage::spellCheckingEnabled(bool enabled) setSpellcheckingEnabled? > Source/WebKit/blackberry/Api/WebPage.cpp:4306 > + d->mapFromTransformed(startPoint); How about: bool invalidPoint = IntPoint(startPoint) == DOMSupport::InvalidPoint; IntPoint start = invalidPoint ? DOMSupport::InvalidPoint : d->mapFromTransformed(startPoint); > Source/WebKit/blackberry/Api/WebPage.cpp:4310 > + d->mapFromTransformed(endPoint); Ditto. > Source/WebKit/blackberry/Api/WebPage.cpp:4396 > + if (!frame->view()) Can be combined with if above. > Source/WebKit/blackberry/Api/WebPage.cpp:4425 > + if (!box->hasOverflowClip()) Ditto. > Source/WebKit/blackberry/Api/WebPage.cpp:4461 > +void pushBackInRegionScrollable(std::vector<Platform::ScrollViewBase>& vector, InRegionScrollableArea scroller, WebPagePrivate* webPage) static? > Source/WebKit/blackberry/Api/WebPage.cpp:4512 > + // propagate scroll to its parent scrollable. Comment can be on two lines. > Source/WebKit/blackberry/Api/WebPage.cpp:4539 > + // IF the minimum font size is ginormous, we may still want the scroll position to be 0,0 Lacks period. > Source/WebKit/blackberry/Api/WebPage.cpp:4545 > + // Should only be invoked when text reflow is enabled Ditto. > Source/WebKit/blackberry/Api/WebPage.cpp:4574 > +void WebPagePrivate::toggleTextReflowIfTextReflowEnabledOnlyForBlockZoom(bool shouldEnableTextReflow) Seriously? What about toggleTextReflowIfEnabledForBlockZoomOnly? > Source/WebKit/blackberry/Api/WebPage.cpp:4594 > + double newScale = 0.0; Can be just 0. > Source/WebKit/blackberry/Api/WebPage.cpp:4819 > + double scale = 1.0; Just 1 is ok here I think. > Source/WebKit/blackberry/Api/WebPage.cpp:4856 > + Why empty line? > Source/WebKit/blackberry/Api/WebPage.cpp:4878 > + return d->m_inPageSearchManager->findNextString(WTF::String::fromUTF8(text), forward); WTF:: not needed.

Comment on attachment 127479 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=127479&action=review Lines 5001-EOF > Source/WebKit/blackberry/Api/WebPage.cpp:5092 > + BlackBerry::WebKit::clearDatabase(d->m_page->groupName()); BlackBerry::WebKit prefixes can be removed. > Source/WebKit/blackberry/Api/WebPage.cpp:5172 > + if (nodeImpl) { Can do if (Node* nodeImpl = node.impl()) { here > Source/WebKit/blackberry/Api/WebPage.cpp:5182 > + if (nodeImpl) { Ditto. > Source/WebKit/blackberry/Api/WebPage.cpp:5184 > + if (style) Ditto. > Source/WebKit/blackberry/Api/WebPage.cpp:5191 > +WTF::String WebPagePrivate::findPatternStringForUrl(const KURL& url) const WTF prefix not needed. > Source/WebKit/blackberry/Api/WebPage.cpp:5202 > + return WTF::String(); Ditto. > Source/WebKit/blackberry/Api/WebPage.cpp:5220 > +} Do we need this? > Source/WebKit/blackberry/Api/WebPage.cpp:5295 > + MediaPlayerPrivate::notifyAppActivatedEvent(false); Could combine to MediaPlayerPrivate::notifyAppActivatedEvent(activationState == ActivationActive) > Source/WebKit/blackberry/Api/WebPage.cpp:5328 > + for (; it != last; ++it) These three lines are repeated a lot. How about making a macro for it so it only uses one line? > Source/WebKit/blackberry/Api/WebPage.cpp:5333 > +{ Should done be marked as unused? Or maybe remove it. > Source/WebKit/blackberry/Api/WebPage.cpp:5391 > +} Do we really need enablePasswordEcho/disablePasswordEcho? Clients can just use settings. > Source/WebKit/blackberry/Api/WebPage.cpp:5592 > + const IntRect windowRect = IntRect(IntPoint(0, 0), viewportSize()); IntPoint::zero > Source/WebKit/blackberry/Api/WebPage.cpp:5914 > + if (doc) Can combine above two lines. > Source/WebKit/blackberry/Api/WebPage_p.h:36 > +#include <wtf/OwnPtr.h> Are all these includes needed? For instance WebPage is both included and declared as forward class reference. > Source/WebKit/blackberry/Api/WebPage_p.h:60 > +class TransformationMatrix; This can be cleaned up a lot, for instance SurfaceOpenVG is not used at all.

Comment on attachment 127479 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=127479&action=review >> Source/WebKit/blackberry/Api/WebPage.cpp:3237 >> + FloatSize currentPPI = Platform::Graphics::Screen::pixelsPerInch(-1); > > What is -1 here? It means we don't pass the actual angle, just get the current orientation which is used to calculate the current PPI. >> Source/WebKit/blackberry/Api/WebPage.cpp:3708 >> + d->showVirtualKeyboard(visible); > > what if we just do d->m_client->inputSetNavigationMode(visible); here? That way we can remove showVirtualKeyboard... Don't think so, as we are using showVirtualKeyboard() in PluginViewPrivate::showKeyboard(bool value) as a virtual function. >> Source/WebKit/blackberry/Api/WebPage.cpp:3869 >> +bool WebPagePrivate::dispatchTouchEventToFullScreenPlugin(PluginView* plugin, const Platform::TouchEvent& event) > > Should be closer to dispatchMouseEventToFullScreenPlugin. This is close to the above caller WebPage::touchEvent(const Platform::TouchEvent& event). Do we really need to move it to the place close to dispatchMouseEventToFullScreenPlugin? >> Source/WebKit/blackberry/Api/WebPage.cpp:4237 >> +void WebPage::spellCheckingEnabled(bool enabled) > > setSpellcheckingEnabled? Tend to agree. >> Source/WebKit/blackberry/Api/WebPage.cpp:4461 >> +void pushBackInRegionScrollable(std::vector<Platform::ScrollViewBase>& vector, InRegionScrollableArea scroller, WebPagePrivate* webPage) > > static? Will add static. >> Source/WebKit/blackberry/Api/WebPage.cpp:4574 >> +void WebPagePrivate::toggleTextReflowIfTextReflowEnabledOnlyForBlockZoom(bool shouldEnableTextReflow) > > Seriously? What about toggleTextReflowIfEnabledForBlockZoomOnly? Will change. >> Source/WebKit/blackberry/Api/WebPage.cpp:5092 >> + BlackBerry::WebKit::clearDatabase(d->m_page->groupName()); > > BlackBerry::WebKit prefixes can be removed. As matter of fact, removing BlackBerry::WebKit before clearLocalStorage() can cause compiling error, as the compiler always choose WebPage::clearLocalStorage() instead of the one with a parameter. >> Source/WebKit/blackberry/Api/WebPage.cpp:5220 >> +} > > Do we need this? Don't think we need, will kill it. >> Source/WebKit/blackberry/Api/WebPage.cpp:5391 >> +} > > Do we really need enablePasswordEcho/disablePasswordEcho? Clients can just use settings. Seems still need them, introduced in PR#108802. >> Source/WebKit/blackberry/Api/WebPage_p.h:36 >> +#include <wtf/OwnPtr.h> > > Are all these includes needed? For instance WebPage is both included and declared as forward class reference. Will clean up more. >> Source/WebKit/blackberry/Api/WebPage_p.h:60 >> +class TransformationMatrix; > > This can be cleaned up a lot, for instance SurfaceOpenVG is not used at all. Will clean up more.

Comment on attachment 127479 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=127479&action=review >> Source/WebKit/blackberry/Api/WebPage.cpp:5328 >> + for (; it != last; ++it) > > These three lines are repeated a lot. How about making a macro for it so it only uses one line? Not sure about this, may be we can do it in a follow up patch if there is any better ideas.

Created attachment 128226 [details] Patch Update the fixes based on Antonio and Rob's comments except the one that associate WebPage and WebPagePrivate methods which would be done in another patch. The local diff of the fixes is here webkit/1dcf848d6d

Comment on attachment 128226 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128226&action=review > Source/WebKit/blackberry/Api/WebPage_p.h:68 > +typedef HashMap<const WebCore::Frame*, BackingStoreClient*> BackingStoreClientForFrameMap; I think we could move it to within the class, close to where it is used? > Source/WebKit/blackberry/Api/WebPage_p.h:209 > + int reflowWidth() const > + { > + // Note: to make this reflow width transform invariant just use > + // transformedActualVisibleSize() here instead! > + return actualVisibleSize().width(); > + } move it to .cpp file or make it one-liner like the others above it for consistency > Source/WebKit/blackberry/Api/WebPage_p.h:299 > + bool didLayoutExceedMaximumIterations() const > + { > + // ZoomToFitOnLoad can lead to a large recursion depth in FrameView::layout() as we attempt > + // to determine the zoom scale factor so as to have the content of the page fit within the > + // area of the frame. From observation, we can bail out after a recursion depth of 10 and > + // still have reasonable results. > + return m_nestedLayoutFinishedCount > 10; ditto > Source/WebKit/blackberry/Api/WebPage_p.h:303 > + } > + > + > + void clearFocusNode(); too many blank lines > Source/WebKit/blackberry/Api/WebPage_p.h:483 > + double m_lastUserEventTimestamp; // Used to detect user scrolling. I am sure it is not needed anymore, lets clean it up later. > Source/WebKit/blackberry/Api/WebPage_p.h:518 > + bool m_updateDelegatedOverlaysDispatched; > + > +}; lets drop this extra new line

Created attachment 128258 [details] Patch Updated the patch based on Antonio's comments except the m_lastUserEventTimestamp. Also replaced plugin views for loop with FOR_EACH_PLUGINVIEW macro.

Comment on attachment 128258 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128258&action=review > Source/WebKit/blackberry/Api/WebPage.cpp:160 > +{ better hide s_visibleWebPages inside this function with DEFINE_STATIC_LOCAL. > Source/WebKit/blackberry/Api/WebPage.cpp:168 > +static double maximumBlockZoomScale = 3.0; // This scale can be clamped by the maximumScale set for the page. Can use just 3. > Source/WebKit/blackberry/Api/WebPage.cpp:172 > +const double delayedZoomInterval = 0.0; Can use just 0. > Source/WebKit/blackberry/Api/WebPage.cpp:421 > + m_page->setCustomHTMLTokenizerTimeDelay(0.300); Can use just 0.3 > Source/WebKit/blackberry/Api/WebPage.cpp:969 > + const IntRect viewportRect = IntRect(IntPoint(0, 0), transformedViewportSize()); Inpoint::zero > Source/WebKit/blackberry/Api/WebPage.cpp:2197 > + return IntRect(IntPoint(0, 0), view->contentsSize()); IntPoint::zero > Source/WebKit/blackberry/Api/WebPage_p.h:192 > + double currentScale() const; Really seems like an inline candidate! > Source/WebKit/blackberry/Api/WebPage_p.h:196 > + double minimumScale() const; We could inline methods like this, would make WebPage.cpp smaller.

Comment on attachment 128258 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128258&action=review > Source/WebKit/blackberry/Api/WebPage.cpp:2314 > +Node* WebPagePrivate::bestChildNodeForClickRect(Node* parentNode, const IntRect& clickRect) make it a local static. > Source/WebKit/blackberry/Api/WebPage.cpp:2323 > + while (node) { for (; node; node = node->nextSibling()) { if (!clickRect.intersecsts(rect)) continue; align the block below accordingly } > Source/WebKit/blackberry/Api/WebPage.cpp:2498 > + if (frame) { early return here (using 'continue') align the block below it accordingly > Source/WebKit/blackberry/Api/WebPage.cpp:2533 > + if (!tnode->hasTagName(HTMLNames::imgTag) && !tnode->hasTagName(HTMLNames::inputTag) && !tnode->hasTagName(HTMLNames::textareaTag)) wrap it with { }

Created attachment 128304 [details] Patch Updated the patch based on Rob and Antonio's comments. Leave the name changing of spellCheckingEnabled in another patch.

Comment on attachment 128304 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128304&action=review > Source/WebKit/blackberry/Api/WebPage.cpp:3354 > + IntSize size(width, height); > + if (size == d->m_defaultLayoutSize) > + return; > + > + d->setDefaultLayoutSize(size); > + bool needsLayout = d->setViewMode(d->viewMode()); > + if (needsLayout) { > + d->setNeedsLayout(); > + if (!d->isLoading()) > + d->requestLayoutIfNeeded(); > + } this should go within WebPagePrivate::setDefaultLayoutSize, since it mainly only access d's stuff > Source/WebKit/blackberry/Api/WebPage.cpp:3381 > + int clickCount = (d->m_selectionHandler->isSelectionActive() || mouseEvent.type() != Platform::MouseEvent::MouseMove) ? 1 : 0; unneeded () > Source/WebKit/blackberry/Api/WebPage.cpp:3405 > + false, false, false, false); these are a bunch of false's. What are them about? please add /**/ comments. > Source/WebKit/blackberry/Api/WebPage.cpp:3505 > + EventHandler* eventHandler = m_mainFrame->eventHandler(); > + > + return eventHandler->handleWheelEvent(wheelEvent); unneeded eventHandler var. > Source/WebKit/blackberry/Api/WebPage.cpp:4138 > +// FIXME: Fix it upstream! FIXME: Fix RenderBox::canBeScrolledAndHasScrollableArea method instead. > Source/WebKit/blackberry/Api/WebPage.cpp:4361 > + // Uncomment this to return in order to see the blocks being selected. > +// d->m_client->zoomChanged(isMinZoomed(), isMaxZoomed(), isAtInitialZoom(), currentZoomLevel()); > +// return true; it is already under a debug macro, so does it still need to be commented out code? What does this do if we leave it uncomment? ...or at least align it properly! :-) > Source/WebKit/blackberry/Api/WebPage.cpp:4469 > + if (oldScale == d->minimumScale() || (distanceBetweenPoints(roundTransformedPoint(d->mapToTransformed(d->scrollPosition())), roundTransformedPoint(d->m_finalBlockPoint)) < minimumDisplacement && abs(newScale - oldScale) / oldScale < 0.10)) { 0.1 looks so magic here. > Source/WebKit/blackberry/Api/WebPage.cpp:4636 > +void WebPage::pauseInDebugger() > + > +{ blank line here > Source/WebKit/blackberry/Api/WebPage.cpp:4695 > + if (!d->m_mainFrame) > + return 0; > + JSC::Bindings::RootObject *root = d->m_mainFrame->script()->bindingRootObject(); add a new line here > Source/WebKit/blackberry/Api/WebPage.cpp:4698 > + if (!root) > + return 0; > + JSC::ExecState *exec = root->globalObject()->globalExec(); ditto > Source/WebKit/blackberry/Api/WebPage.cpp:4714 > +#if ENABLE(WEBDOM) > +WebDOMDocument WebPage::document() const > +{ > + if (!d->m_mainFrame) > + return WebDOMDocument(); > + return WebDOMDocument(d->m_mainFrame->document()); > +} > +#endif lets group this one together with the other WEBDOM block below. > Source/WebKit/blackberry/Api/WebPage.cpp:4762 > +void WebPage::clearHistory() > +{ > + // Don't clear the back-forward list as we might like to keep it. > +} > + :? > Source/WebKit/blackberry/Api/WebPage.cpp:4952 > + HashSet<PluginView*>::const_iterator it = d->m_pluginViews.begin(); > + HashSet<PluginView*>::const_iterator last = d->m_pluginViews.end(); > + > +#if ENABLE(VIDEO) > + MediaPlayerPrivate::notifyAppActivatedEvent(activationState == ActivationActive); > +#endif > + > + for (; it != last; ++it) { FOR_EACH_PLUGINVIEW here too? > Source/WebKit/blackberry/Api/WebPage.cpp:4964 > + ASSERT(0); ASSERT_NOT_REACHED() > Source/WebKit/blackberry/Api/WebPage.cpp:5053 > +Frame* WebPage::mainFrame() const > +{ > + return d->m_mainFrame; > +} I do not think this should be a public API. > Source/WebKit/blackberry/Api/WebPage.cpp:5462 > +// MARK: WebSettingsDelegate. lets remove this comment. > Source/WebKit/blackberry/Api/WebPage.cpp:5468 > +void WebPagePrivate::didChangeSettings(WebSettings* webSettings) > +{ > + Settings* coreSettings = m_page->settings(); > + m_page->setGroupName(webSettings->pageGroupName()); > + coreSettings->setXSSAuditorEnabled(webSettings->xssAuditorEnabled()); I wish we could move this whole method out of here in a follow up. > Source/WebKit/blackberry/Api/WebPage.cpp:5555 > + static IntSize size; > + > + if (size.isEmpty()) drop blank line > Source/WebKit/blackberry/Api/WebPage.cpp:5566 > + if (Document* doc = d->m_page->focusController()->focusedOrMainFrame()->document()) > + return doc->queryCommandValue(query); > + return ""; add a new line here > Source/WebKit/blackberry/Api/WebPage.h:222 > +#if defined(ENABLE_WEBDOM) && ENABLE_WEBDOM > + WebDOMDocument document() const; > +#endif move it down together with the other WEBDOM stuff.

Created attachment 128516 [details] Patch Updated the patch based on Antonio's comments. WebPage::mainFrame() related clean up will be done in a follow up patch.

Comment on attachment 128516 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128516&action=review > Source/WebKit/blackberry/Api/WebPage.cpp:4156 > + ASSERT(layer); > + > + if (layer->parent()) follow up: remove blank line > Source/WebKit/blackberry/Api/WebPage.cpp:4207 > + RenderLayer* layer = renderer->enclosingLayer(); > + > + do { ditto. > Source/WebKit/blackberry/Api/WebPage.cpp:4233 > + } > + > + } while (layer = parentLayer(layer)); ditto > Source/WebKit/blackberry/Api/WebPage.cpp:4541 > +Platform::IntRect WebPage::focusNodeRect() > +{ > + return d->focusNodeRect(); > +} lets also move it to Private in a followup

Comment on attachment 128516 [details] Patch Clearing flags on attachment: 128516 Committed r108718: <http://trac.webkit.org/changeset/108718>

All reviewed patches have been landed. Closing bug.

Clean up more in a follow up patch.

Created attachment 129099 [details] Patch A follow patch. Libwebview patch is here libwebview/68b25bd6f0b4ea3c43fb79a4d02137a7a14b8cc7

Comment on attachment 129099 [details] Patch Clearing flags on attachment: 129099 Committed r109051: <http://trac.webkit.org/changeset/109051>

All reviewed patches have been landed. Closing bug.