<rdar://problem/9002109>
Created attachment 93473 [details] Patch
Comment on attachment 93473 [details] Patch Not a WebKit patch.
Created attachment 93478 [details] Patch
The content of attachment 93473 [details] has been deleted by William Siegrist <wsiegrist@apple.com> who provided the following reason: deleted by request of jonlee The token used to delete this attachment was generated at 2011-05-13 10:39:09 PST.
Comment on attachment 93478 [details] Patch Attachment 93478 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/8693246
Comment on attachment 93478 [details] Patch Attachment 93478 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/8695180
Comment on attachment 93478 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=93478&action=review r- because this needs some renaming work > Source/WebCore/dom/Document.cpp:425 > + , m_countWheelEventHandlers(0) 'count' reads like a verb. I'd call it 'm_numWheelEventHandlers' or 'm_wheelEventHandlersCount' > Source/WebCore/dom/Document.h:1101 > + unsigned countWheelEventHandlers() const { return m_countWheelEventHandlers; } This needs a new name too. > Source/WebCore/page/ChromeClient.h:324 > + virtual void updateCountWheelEventHandlers(unsigned) = 0; This sounds like you want the callee to update their count. I think it's really 'numWheelEventHandlersChanged()', no? > Source/WebCore/page/EventHandler.cpp:-2144 > - view = m_frame->view(); > - if (!view) > - return false; > - Why this change? > Source/WebCore/page/Frame.cpp:1122 > +static unsigned sumTotalWheelEventHandlers(const Frame* frame) Awkwardly named. > Source/WebCore/page/Frame.cpp:1132 > + for (Frame* child = frame->tree()->firstChild(); child; child = child->tree()->nextSibling()) > + count += sumTotalWheelEventHandlers(child); You could walk the entire tree via the FrameTree rather than recursing. > Source/WebCore/page/Frame.cpp:1137 > +void Frame::recalculateTotalWheelEventHandlers() const Odd that a 'recalculate' method is const. This should be more like notifyChomeClientOfNewWheelEventHandlerCount() or something. > Source/WebCore/page/Frame.h:211 > + // Should only be called on the main frame of a page. > + void recalculateTotalWheelEventHandlers() const; I don't see an ASSERT that matches the comment. > Source/WebCore/platform/mac/ScrollAnimatorMac.mm:791 > + // after a gesture begins, we go through Sentence case please. > Source/WebCore/platform/mac/ScrollAnimatorMac.mm:805 > + if ((wheelEvent.deltaX() > 0 && > + !m_scrollableArea->shouldRubberBandInDirection(ScrollLeft) && > + (!m_scrollableArea->horizontalScrollbar() || > + m_scrollableArea->scrollPosition(m_scrollableArea->horizontalScrollbar()) <= m_scrollableArea->minimumScrollPosition().x())) > + || > + (wheelEvent.deltaX() < 0 && > + !m_scrollableArea->shouldRubberBandInDirection(ScrollRight) && > + (!m_scrollableArea->horizontalScrollbar() || > + m_scrollableArea->scrollPosition(m_scrollableArea->horizontalScrollbar()) >= m_scrollableArea->maximumScrollPosition().x()))) { > + ScrollAnimator::handleWheelEvent(wheelEvent); Ouch. Some inline helper functions would make this hugely more readable. > Source/WebKit2/UIProcess/WebPageProxy.h:229 > + bool shouldWaitForWheelEvents(); Should be const. > Source/WebKit2/UIProcess/WebPageProxy.h:616 > + void updateCountWheelEventHandlers(unsigned count) { m_wheelEventHandlerCount = count; } updateWheelEventHandlersCount() > Source/WebKit2/UIProcess/WebPageProxy.messages.in:65 > + UpdateCountWheelEventHandlers(unsigned count) Rename > Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:770 > + default: > + return true; The default clause is defeating the compiler's ability to warn if not all values are handled.
(In reply to comment #7) > > Source/WebCore/page/EventHandler.cpp:-2144 > > - view = m_frame->view(); > > - if (!view) > > - return false; > > - > > Why this change? Earlier in the method this check is already performed. Unless there's a possibility that the frame will lose the view by the time we hit this line, I don't think we have to check twice. -- I am noting all of your renaming suggestions, and will upload a new patch shortly.
(In reply to comment #8) > (In reply to comment #7) > > > > Source/WebCore/page/EventHandler.cpp:-2144 > > > - view = m_frame->view(); > > > - if (!view) > > > - return false; > > > - > > > > Why this change? > > Earlier in the method this check is already performed. Unless there's a possibility that the frame will lose the view by the time we hit this line, I don't think we have to check twice. You have to be really careful in code that allows arbitrary JS to be executed. This code is probably here because the event handler can run JS which results in the frame getting destroyed.
Comment on attachment 93478 [details] Patch Attachment 93478 [details] did not pass cr-mac-ews (chromium): Output: http://queues.webkit.org/results/8699104
Comment on attachment 93478 [details] Patch Attachment 93478 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8694234
Comment on attachment 93478 [details] Patch Attachment 93478 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/8695211
Comment on attachment 93478 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=93478&action=review Noticed the word coalesced spelled with two l's as "coalesced" in a few places. > Source/WebCore/page/ChromeClient.h:173 > + virtual bool shouldRubberBandInDirection(ScrollDirection) = 0; These will need to be reimplemented for many platforms. That’s why the build of every platform except for Mac is failing. >> Source/WebCore/page/ChromeClient.h:324 >> + virtual void updateCountWheelEventHandlers(unsigned) = 0; > > This sounds like you want the callee to update their count. I think it's really 'numWheelEventHandlersChanged()', no? These will need to be reimplemented for many platforms. That’s why the build of every platform except for Mac is failing. This name should probably be setWheelEventHandlerCount.
Created attachment 93516 [details] Patch Including Simon's and Darin's suggestions, as well as making additional spelling corrections to variable names.
Attachment 93516 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:766: An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] Total errors found: 1 in 47 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 93517 [details] Patch
Comment on attachment 93517 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=93517&action=review > Source/WebCore/platform/mac/ScrollAnimatorMac.mm:775 > +static inline bool horizontalScrollPositionIsPastMinimum(ScrollableArea* scrollableArea) > +{ > + return scrollableArea->scrollPosition(scrollableArea->horizontalScrollbar()) <= scrollableArea->minimumScrollPosition().x(); > +} > + > +static inline bool horizontalScrollPositionIsPastMaximum(ScrollableArea* scrollableArea) > +{ > + return scrollableArea->scrollPosition(scrollableArea->horizontalScrollbar()) >= scrollableArea->maximumScrollPosition().x(); > +} It seems reasonable to add these to ScrollableArea
Created attachment 93558 [details] Patch Moved method to ScrollableArea, and added some additional logic to fix some regressions found in the rubber banding behavior.
Comment on attachment 93558 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=93558&action=review Looking really good, but lets go another around. > Source/WebCore/page/Frame.cpp:1130 > + for (const Frame* frame = this; frame; frame = frame->tree()->traverseNext()) > + if (frame->document()) > + count += frame->document()->wheelEventHandlerCount(); This should have braces around for-loop. > Source/WebCore/page/FrameView.cpp:314 > + scrollAnimator()->didAddHorizontalScrollbar(scrollbar); This should call ScrollView::didAddHorizontalScrollbar(scrollbar) instead of calling the animator directly. > Source/WebCore/page/FrameView.cpp:319 > + scrollAnimator()->willRemoveHorizontalScrollbar(scrollbar); Same here. > Source/WebKit2/UIProcess/API/C/WKPage.h:291 > +WK_EXPORT bool WKPageShouldWaitForWheelEvents(WKPageRef page); I would rather this have a name and functionality that would work for all builds, including those without gesture events. Perhaps, something along the lines of horizontal scrolls being noops...
Created attachment 93569 [details] Patch Incorporated Sam's suggestions. Opted for "WebPageProxy::willHandleHorizontalScrollEvents" to relate it to the "didNotHandleWheelEvent" callback. Also, removed preprocessor so that it works for all builds.
Created attachment 93599 [details] Patch
Comment on attachment 93599 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=93599&action=review > Source/WebCore/platform/ScrollableArea.h:137 > + bool isHorizontalScrollerPinnedToLeft() const { return !horizontalScrollbar() || scrollPosition(horizontalScrollbar()) <= minimumScrollPosition().x(); } > + bool isHorizontalScrollerPinnedToRight() const { return !horizontalScrollbar() || scrollPosition(horizontalScrollbar()) >= maximumScrollPosition().x(); } > + 'left' and 'right' are loaded terms in a mixed writing-direction world. I'd rename these to isHorizontalScrollerAtMinimum() and isHorizontalScrollerAtMaximum or something. > Source/WebCore/platform/mac/ScrollAnimatorMac.h:149 > + int m_aggregateScroll; Not sure what 'aggregate' means here. Would 'cumulative' be a better term to use? Also, if this is only ever in X, the name should reflect that.
(In reply to comment #22) > (From update of attachment 93599 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=93599&action=review > > > Source/WebCore/platform/ScrollableArea.h:137 > > + bool isHorizontalScrollerPinnedToLeft() const { return !horizontalScrollbar() || scrollPosition(horizontalScrollbar()) <= minimumScrollPosition().x(); } > > + bool isHorizontalScrollerPinnedToRight() const { return !horizontalScrollbar() || scrollPosition(horizontalScrollbar()) >= maximumScrollPosition().x(); } > > + > > 'left' and 'right' are loaded terms in a mixed writing-direction world. I'd rename these to isHorizontalScrollerAtMinimum() and isHorizontalScrollerAtMaximum or something. Done. > > > Source/WebCore/platform/mac/ScrollAnimatorMac.h:149 > > + int m_aggregateScroll; > > Not sure what 'aggregate' means here. Would 'cumulative' be a better term to use? > > Also, if this is only ever in X, the name should reflect that. Renamed to m_cumulativeHorizontalScroll.
Created attachment 93611 [details] Patch
Comment on attachment 93611 [details] Patch rs=me
http://trac.webkit.org/changeset/86584
This looks like it's causing crashes downstream in Chromium: http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=http%2Ftests%2Fsecurity%2FaboutBlank%2Fwindow-open-self-about-blank.html&showExpectations=true&group=%40ToT%20-%20chromium.org Are you sure that willRemoveHorizontalScrollbar() is always guaranteed to be called exactly once for each call to didAddHorizontalScrollbar()? Not sure whether this is a bug in the Chromium platform layer calling this routine too often, or an invalid assumption in the patch?
Fix to regression failures: http://trac.webkit.org/changeset/86597
I'm on r87310, but still experience the same crash described in comment #27 while running some DRT tests on the EFL port, such as tables/mozilla/core/one_row.html, fast/dom/createElementNS.html or fast/dom/clone-node-form-elements-with-attr.html.
If a bug fix is causing problems or crashes, reopening the bug is not the way to report it. That will just confuse people about the state of the patch or bug. It makes sense to reopen the bug if we rolled out the patch, but otherwise, please file a new bug about the new problem and add comments to the old bug.
(In reply to comment #30) > If a bug fix is causing problems or crashes, reopening the bug is not the way to report it. That will just confuse people about the state of the patch or bug. It makes sense to reopen the bug if we rolled out the patch, but otherwise, please file a new bug about the new problem and add comments to the old bug. Opened that corresponding bug report: https://bugs.webkit.org/show_bug.cgi?id=64954
Comment on attachment 93611 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=93611&action=review > Source/WebCore/dom/Document.h:1103 > + // Used to keep track of horizontal scrollbars and onmousewheel event handlers only. The "horizontal scrollbars" part sounds wrong to me. The code looks like this is just the number of onmousewheel events, and nothing else?