Bug 67606 – [Qt][WK2] Make TiledDrawingArea request tiles only in the direction the viewport is panned to.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jocelyn Turcotte]

[Qt][WK2] Make TiledDrawingArea request tiles only in the direction the viewport is panned to.

[Jocelyn Turcotte]

Created attachment 106343 [details]
Patch

[Noam Rosenthal]

Comment on attachment 106343 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=106343&action=review

Awesome stuff. Not sure if someone else (kenneth?) wants to take a look at these TBS patches as well, otherwise r=me.

> Source/WebCore/ChangeLog:18
> +        (WebCore::TiledBackingStore::setCoverAreaFocusVector):

The name "focus" has a bit of a keyboard connotation.
How about trajectory? (not sure this is that important, though)

> Source/WebCore/platform/graphics/TiledBackingStore.cpp:305
> +    // Inflates to both sides, so divide inflate delta by 2

Period at end of sentence.

[Jocelyn Turcotte]

Thanks, adding Kenneth in CC, I'll push it in a couple of days in any case.

[Kenneth Rohde Christiansen]

Comment on attachment 106343 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=106343&action=review

>> Source/WebCore/ChangeLog:18
>> +        (WebCore::TiledBackingStore::setCoverAreaFocusVector):
> 
> The name "focus" has a bit of a keyboard connotation.
> How about trajectory? (not sure this is that important, though)

I support Trajectory as well, much better

> Source/WebCore/platform/graphics/TiledBackingStore.cpp:314
> +// A non-null focus vector will shrink the intersection rect to visibleRect plus it's expansion from it's

I think these should be "its" and not "it's = it is"

> Source/WebCore/platform/graphics/TiledBackingStore.cpp:316
> +// e.g. If visibleRect == (10,10)5x5 and coverMultiplier == 3.0:

Why is if upper case after e.g. ?

> Source/WebCore/platform/graphics/TiledBackingStore.cpp:319
> +// and a (1,1) focus vector of will create tiles intersecting (10,10)10x10.

Strange sentence, I don't get it. "of will"?

> Source/WebCore/platform/graphics/TiledBackingStore.cpp:326
> +        float focusVectorMultiplier = (m_coverAreaMultiplier - 1) / 2 / focusVectorNorm;

This line is hard to read, better add some ()'s

> Source/WebCore/platform/graphics/TiledBackingStore.cpp:327
> +        result.move(result.width() * m_coverAreaFocusVector.x() * focusVectorMultiplier, result.height() * m_coverAreaFocusVector.y() * focusVectorMultiplier);

Maybe it would be more clear if you use two helper variables

> Source/WebCore/platform/graphics/TiledBackingStore.h:94
> +    IntRect calculateKeepRect(const IntRect& visibleRect) const;
> +    IntRect calculateCoverRect(const IntRect& visibleRect) const;

in webkit we normally use the nomenclature "compute" instead of calculate

> Source/WebKit/qt/Api/qwebpage.cpp:1229
> +        // Multipliers were changed to only use a float, keep only the height part.

Is that comment useful? or can it be rewritten

> Source/WebKit2/UIProcess/qt/ViewportInteractionEngine.cpp:165
> +    // Make sure that this is emitted before viewportUpdateRequested so that the web process knows where to look for tiles.

I would make it more like // Must be emitted before...

[Viatcheslav Ostapenko]

IMHO, coverRect should be intersected with a keepRect before use - may be instead of intersecting of coverRect with contentsRect in calculateCoverRect, because keepRect is already clipped by contentsRect.
I think without this needless create/remove of tiles could happen at the end of scrolling.

[Jocelyn Turcotte]

(In reply to comment #4)
Thx, changing all this stuff before pushing.

(In reply to comment #5)
As talked on IRC, the computed cover rect should never span out of the keep rect. Adding ASSERT(keepRect.contains(coverRect)) before pushing to ensure/communicate this.

[Jocelyn Turcotte]

Committed r94685: <http://trac.webkit.org/changeset/94685>