[webkit-reviews] review denied: [Bug 103959] Coordinated Graphics: Refactor TiledBackingStore code in CoordinatedGraphicsLayer. : [Attachment 177449] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Dec 4 02:47:57 PST 2012


Jocelyn Turcotte <jocelyn.turcotte at digia.com> has denied Huang Dongsung
<luxtella at company100.net>'s request for review:
Bug 103959: Coordinated Graphics: Refactor TiledBackingStore code in
CoordinatedGraphicsLayer.
https://bugs.webkit.org/show_bug.cgi?id=103959

Attachment 177449: Patch
https://bugs.webkit.org/attachment.cgi?id=177449&action=review

------- Additional Comments from Jocelyn Turcotte <jocelyn.turcotte at digia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=177449&action=review


> Source/WebCore/platform/graphics/TiledBackingStore.cpp:546
> +    if (!m_allowInternalCommit)
> +	   return;

createTiles() currently do the work in more than one step. It first creates all
the tiles with the same manhatan distance to the viewport center, then start a
timer and do the next distance, etc. This is done so that the user can see the
rendered content for the viewport before we start rendering out-of-screen
tiles.
By disabling the timer completely this will have the effect of bypassing the
next timer and the cover rect will never be completely covered.
Ideally what we want is that tiles get created, that this schedules a layer
flush, and that the next tile distance tiles are rendered in a new frame.

>
Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.
cpp:766
> +	   m_trajectoryVector = FloatPoint();

Maybe it would be better to set it explicitely on the TiledBackingStore and let
coverWithTilesIfNeeded read the member instead of getting it through a
parameter. Else please name it something like m_pendingTrajectoryVector to
suggest that this is used temporarily.

>
Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.
h:210
> -    bool m_inUpdateMode : 1;
> +    bool m_inCoordinateMode : 1;

Update is much clearer IMO

Please be careful when refactoring this kind of code as it is covered by almost
no automated test. I personally don't see the value of this kind of change if
it is going to cause more problems than it fixes.


More information about the webkit-reviews mailing list