Summary: | Add a new mode where fixed elements don't constrain their positions during a rubber band | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Beth Dakin <bdakin> | ||||||||||
Component: | Layout and Rendering | Assignee: | Beth Dakin <bdakin> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | andersca, bdakin, cmarcelo, commit-queue, dino, eoconnor, esprehn+autocc, glenn, jamesr, jonlee, kondapallykalyan, luiz, sam, simon.fraser, thorton, tonikitoo | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Beth Dakin
2013-11-12 19:36:16 PST
Created attachment 216766 [details]
Patch
Comment on attachment 216766 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=216766&action=review > Source/WebCore/page/FrameView.cpp:1546 > +IntSize FrameView::scrollOffsetForFixedPosition(const IntRect& visibleContentRect, const IntSize& totalContentsSize, const IntPoint& scrollPosition, const IntPoint& scrollOrigin, float frameScaleFactor, bool fixedElementsLayoutRelativeToFrame, bool shouldConstrainPosition, int headerHeight, int footerHeight) Wonder if we should enumify shouldConstrainPosition. The arguments to this function are getting out of hand, too :( > Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:269 > + return true; The last line of both cases is shared > Source/WebCore/page/scrolling/ScrollingStateScrollingNode.cpp:234 > + m_scrollingStateTree->setHasChangedProperties(true); Weird, I would expect setPropertyChanged to do this, no? > Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h:109 > + bool m_setShouldConstrainScrollPositionForFixedElements; I don't think this member should have the word 'set' in it? > Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.h:129 > + void setNodeShouldConstrainScrollPositionForFixedElements(bool, ScrollingStateScrollingNode*); setShouldConstrainScollPositionForFixedElementsForNode sounds ridiculous but is more consistent with the other functions and the argument order :( Comment on attachment 216766 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=216766&action=review >> Source/WebCore/page/FrameView.cpp:1546 >> +IntSize FrameView::scrollOffsetForFixedPosition(const IntRect& visibleContentRect, const IntSize& totalContentsSize, const IntPoint& scrollPosition, const IntPoint& scrollOrigin, float frameScaleFactor, bool fixedElementsLayoutRelativeToFrame, bool shouldConstrainPosition, int headerHeight, int footerHeight) > > Wonder if we should enumify shouldConstrainPosition. The arguments to this function are getting out of hand, too :( I think we should. > Source/WebCore/page/FrameView.cpp:1569 > + shouldConstrainPosition = !renderView->compositor().mainFrameBackingIsTiledWithMargin(); I'd expect this to check frameViewShouldConstrainScrollPositionForFixedElements() or maybe a Setting, rather than tiggering off mainFrameBackingIsTiledWithMargin, which seems more like a consequence of the behavior rather than the main trigger. > Source/WebCore/page/scrolling/ScrollingCoordinator.h:172 > + bool frameViewShouldConstrainScrollPositionForFixedElements(FrameView*); const? > Source/WebCore/page/scrolling/ScrollingStateScrollingNode.h:114 > + bool shouldConstrainScrollPositionForFixedElements() const { return m_shouldConstrainScrollPositionForFixedElements; } > + void setShouldConstrainScrollPositionForFixedElements(bool); This is a bit of a mouthful. How about "fixedElementsMoveOnRubberband()" or something. > Source/WebCore/rendering/RenderLayerCompositor.cpp:2522 > +bool RenderLayerCompositor::mainFrameBackingIsTiledWithMargin() const > +{ > + // FIXME: Implement. https://bugs.webkit.org/show_bug.cgi?id=124216 > + return false; > +} Not sure it's right for RenderLayerCompositor to be the "owner" of the behavior. Seems like a ScrollView/FrameView thing, initialized from a Setting. (In reply to comment #2) > (From update of attachment 216766 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=216766&action=review > > > Source/WebCore/page/FrameView.cpp:1546 > > +IntSize FrameView::scrollOffsetForFixedPosition(const IntRect& visibleContentRect, const IntSize& totalContentsSize, const IntPoint& scrollPosition, const IntPoint& scrollOrigin, float frameScaleFactor, bool fixedElementsLayoutRelativeToFrame, bool shouldConstrainPosition, int headerHeight, int footerHeight) > > Wonder if we should enumify shouldConstrainPosition. The arguments to this function are getting out of hand, too :( > Okay. > > Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:269 > > + return true; > > The last line of both cases is shared > Will share. > > Source/WebCore/page/scrolling/ScrollingStateScrollingNode.cpp:234 > > + m_scrollingStateTree->setHasChangedProperties(true); > > Weird, I would expect setPropertyChanged to do this, no? > Hrm, it doesn't appear to. This seems due for a clean-up. > > Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h:109 > > + bool m_setShouldConstrainScrollPositionForFixedElements; > > I don't think this member should have the word 'set' in it? > Ah yes, good catch. > > Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.h:129 > > + void setNodeShouldConstrainScrollPositionForFixedElements(bool, ScrollingStateScrollingNode*); > > setShouldConstrainScollPositionForFixedElementsForNode sounds ridiculous but is more consistent with the other functions and the argument order :( :-( yeah. Will change. Created attachment 216856 [details]
Patch
This should address all of Tim's and Simon's comments.
Attachment 216856 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/page/FrameView.cpp', u'Source/WebCore/page/FrameView.h', u'Source/WebCore/page/scrolling/ScrollingStateScrollingNode.cpp', u'Source/WebCore/page/scrolling/ScrollingStateScrollingNode.h', u'Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp', u'Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h', u'Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.h', u'Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.mm', u'Source/WebCore/page/scrolling/mac/ScrollingTreeScrollingNodeMac.mm', u'Source/WebCore/platform/ScrollTypes.h', u'Source/WebCore/rendering/RenderLayerCompositor.cpp']" exit_code: 1
Source/WebCore/page/FrameView.h:236: The parameter name "behaviorForFixed" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 1 in 12 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 216861 [details]
Patch
This should make stylebot happy.
Created attachment 216863 [details]
Patch
Thanks Tim! http://trac.webkit.org/changeset/159300 |