Summary: | [iOS][WK2] Support disabling speculative tiling | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Benjamin Poulain <benjamin> | ||||
Component: | New Bugs | Assignee: | Benjamin Poulain <benjamin> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | thorton | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Benjamin Poulain
2014-05-02 20:44:18 PDT
Created attachment 230736 [details]
Patch
Comment on attachment 230736 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=230736&action=review > Source/WebCore/ChangeLog:9 > + Move ScrollView::setScrollVelocity() and ScrollView::computeCoverageRect() to FrameView. > + When speculative tiling is disabled, return an unmodified exposed rect. You could leave it in ScrollView, override in FrameView and check m_speculativeTilingEnabled before calling to the superclass. But, was there a reason it was in ScrollView itself in the first place? > Source/WebCore/page/FrameView.cpp:4125 > +FloatRect FrameView::computeCoverageRect(double horizontalMargin, double verticalMargin) const It still makes me sad that this is separate from the one in TileController (that this logic is spread out). Comment on attachment 230736 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=230736&action=review > Source/WebCore/page/FrameView.h:694 > + double m_horizontalVelocity; > + double m_verticalVelocity; > + double m_scaleChangeRate; > + double m_lastVelocityUpdateTime; Shame to bloat all FrameViews with this, when only the main one will care. Maybe move it into a unique_ptr'd object? (In reply to comment #3) > (From update of attachment 230736 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=230736&action=review > > > Source/WebCore/page/FrameView.h:694 > > + double m_horizontalVelocity; > > + double m_verticalVelocity; > > + double m_scaleChangeRate; > > + double m_lastVelocityUpdateTime; > > Shame to bloat all FrameViews with this, when only the main one will care. Maybe move it into a unique_ptr'd object? It is not worth optimizing memory for document-level classes. There are never enough objects that it causes issues. (In reply to comment #2) > (From update of attachment 230736 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=230736&action=review > > > Source/WebCore/ChangeLog:9 > > + Move ScrollView::setScrollVelocity() and ScrollView::computeCoverageRect() to FrameView. > > + When speculative tiling is disabled, return an unmodified exposed rect. > > You could leave it in ScrollView, override in FrameView and check m_speculativeTilingEnabled before calling to the superclass. But, was there a reason it was in ScrollView itself in the first place? It was on ScrollView because it is related to the exposedRect() concept. > > Source/WebCore/page/FrameView.cpp:4125 > > +FloatRect FrameView::computeCoverageRect(double horizontalMargin, double verticalMargin) const > > It still makes me sad that this is separate from the one in TileController (that this logic is spread out). Maybe I am misunderstanding something here: isn't TileController generic for any tiled graphic layer? I agree the code does not really belong on ScrollView/FrameView, but I was under the impression the final home for this would be the compositor or something like that. (In reply to comment #5) > (In reply to comment #2) > > (From update of attachment 230736 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=230736&action=review > > > > > Source/WebCore/ChangeLog:9 > > > + Move ScrollView::setScrollVelocity() and ScrollView::computeCoverageRect() to FrameView. > > > + When speculative tiling is disabled, return an unmodified exposed rect. > > > > You could leave it in ScrollView, override in FrameView and check m_speculativeTilingEnabled before calling to the superclass. But, was there a reason it was in ScrollView itself in the first place? > > It was on ScrollView because it is related to the exposedRect() concept. Right. But no actual reason to keep it there. OK! > > > Source/WebCore/page/FrameView.cpp:4125 > > > +FloatRect FrameView::computeCoverageRect(double horizontalMargin, double verticalMargin) const > > > > It still makes me sad that this is separate from the one in TileController (that this logic is spread out). > > Maybe I am misunderstanding something here: isn't TileController generic for any tiled graphic layer? Sure, but e.g. TileController::computeTileCoverageRect does math that is special for scrolling tiled layers (etc.), based on information pushed down to it. > I agree the code does not really belong on ScrollView/FrameView, but I was under the impression the final home for this would be the compositor or something like that. Maybe! I think you should land this as-is, in any case. Committed r168233: <http://trac.webkit.org/changeset/168233> |