Bug 132512

Summary: [iOS][WK2] Support disabling speculative tiling
Product: WebKit Reporter: Benjamin Poulain <benjamin>
Component: New BugsAssignee: Benjamin Poulain <benjamin>
Status: RESOLVED FIXED    
Severity: Normal CC: thorton
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch thorton: review+

Description Benjamin Poulain 2014-05-02 20:44:18 PDT
[iOS][WK2] Support speculative tiling
Comment 1 Benjamin Poulain 2014-05-02 20:48:11 PDT
Created attachment 230736 [details]
Patch
Comment 2 Tim Horton 2014-05-03 11:12:51 PDT
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 3 Simon Fraser (smfr) 2014-05-03 11:27:58 PDT
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?
Comment 4 Benjamin Poulain 2014-05-03 17:52:16 PDT
(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.
Comment 5 Benjamin Poulain 2014-05-03 18:02:51 PDT
(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.
Comment 6 Tim Horton 2014-05-03 18:24:34 PDT
(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.
Comment 7 Benjamin Poulain 2014-05-03 18:49:58 PDT
Committed r168233: <http://trac.webkit.org/changeset/168233>