Bug 57798 – Tiled Backing Store does not regenerate tiles when it should

• Component: Platform
• Status: RESOLVED FIXED

[Carol Szabo]

If TiledBackingStore::adjustVisibleRect is called when the contents of the Frame/Layer is empty (or possible even when contents is not empty, but smaller in size then the frame that shows it, tiles are generated to cover the contents - (0, 0) sized tiles in case of empty contents, but not the full visible frame.
When the contents changes to occupy the full visible area, tiles are not regenerated, thus the full document cannot be displayed.
See attachment for a fragment of Qt test code that exhibits the behavior, but the problem is not limited to Qt.

[Carol Szabo]

Created attachment 88153 [details]
Patch

[Early Warning System Bot]

Attachment 88153 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8331137

[Carol Szabo]

Created attachment 88235 [details]
Proposed Patch. Fixed problem with missing file

[Eric Seidel (no email)]

One of the CC'd shoudl easily be able to review this three-week-old patch. :)

[Kenneth Rohde Christiansen]

Jocelyn has been maintaining our WebKit2 version of the tiled backing store, so I'm cc'ing him. I am also cc'ing the original author Antti.

[Kenneth Rohde Christiansen]

Comment on attachment 88235 [details]
Proposed Patch. Fixed problem with missing file

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

> Source/WebCore/platform/graphics/TiledBackingStore.cpp:158
> -    IntRect visibleRect = mapFromContents(m_client->tiledBackingStoreVisibleRect());
> +    IntRect visibleRect = mapFromContents(intersection(m_client->tiledBackingStoreVisibleRect(), m_client->tiledBackingStoreContentsRect()));

Wouldn't it be better to make the tiledBackingStoreVisibleRect() take the contents rect into account? That would make this change quite local.

[Jocelyn Turcotte]

The change looks good to me overall, I think we might have this bug in WebKit2 as well.

I agree with Kenneth that this intersection would best be made at only one place in the code.

Since you are already changing it, greendiv could instead be named to resizedDiv, grownDiv or something. I'm not sure it really matters that it's green.

[Kenneth Rohde Christiansen]

Comment on attachment 88235 [details]
Proposed Patch. Fixed problem with missing file

r- given mine and Jocelyn's comments

[Carol Szabo]

(In reply to comment #8)
> (From update of attachment 88235 [details])
> r- given mine and Jocelyn's comments

Jocelyn and Kenneth,
Let me disagree with your review.
First, if the intersection were to be moved into tiledBackingStoreVisibleRect() there would be one intersection for every tiledBackingStore client (currently at least 2: WebCore::Frame and WebKit::QtGraphicsLayerImpl), thus there is no saving in code size, second, the TiledBackingStoreClient interface would be less rich since one would no longer be able to retrieve the size of the visible area of the backing store client, if that would ever be needed for some purpose (such as optimization of tile recreation vs. memory usage) the value returned by tiledBackingStoreVisibleRect would be clipped to contentSize always.

Second, It is absolutely essential that the div is green. That is what all the  tests that use this resource test against, this particular test does not even execute the function that resizes the div, because it does not matter for this test. It is true that resizableFrom50%To150%GreenDivOnWhiteBackground.html would be more descriptive, but I tried to stick with a reasonably short name.
If I remember correctly it is essential that this Div causes the scrollbars to be shown in some of the tests that use it, but for this particular test it only matters that it is green and not red.
I urge you guys to reconsider this review.

[Carol Szabo]

Created attachment 95325 [details]
Alternative Patch, to satisfy Kenneth and Joceylin's judgement

I have attached a new patch just in case my previous comment did not convince you that the patch you reviewed negatively is actually a better solution to this problem. I did not mark this new patch for review yet as I still believe that my original patch was better.

[Jocelyn Turcotte]

(In reply to comment #9)
> Let me disagree with your review.
> First, if the intersection were to be moved into tiledBackingStoreVisibleRect() there would be one intersection for every tiledBackingStore client (currently at least 2: WebCore::Frame and WebKit::QtGraphicsLayerImpl), thus there is no saving in code size, second, the TiledBackingStoreClient interface would be less rich since one would no longer be able to retrieve the size of the visible area of the backing store client, if that would ever be needed for some purpose (such as optimization of tile recreation vs. memory usage) the value returned by tiledBackingStoreVisibleRect would be clipped to contentSize always.

I agree, what about introducing a TiledBackingStore::visibleRect() that would do this logic?
My point is just to avoid duplicate logic since those two hunks need to be synchronized. The reason why an intersection is needed there might not be obvious just by reading the code, it's easier to break.

> Second, It is absolutely essential that the div is green. That is what all the  tests that use this resource test against, this particular test does not even execute the function that resizes the div, because it does not matter for this test. It is true that resizableFrom50%To150%GreenDivOnWhiteBackground.html would be more descriptive, but I tried to stick with a reasonably short name.
> If I remember correctly it is essential that this Div causes the scrollbars to be shown in some of the tests that use it, but for this particular test it only matters that it is green and not red.
> I urge you guys to reconsider this review.

Sure, greendiv is also fine if you prefer it.

[Carol Szabo]

Created attachment 95536 [details]
Proposed Patch, per Jocelyn's suggestion.

Implemented Jocelyn's suggestion.

[WebKit Review Bot]

Comment on attachment 95536 [details]
Proposed Patch, per Jocelyn's suggestion.

Rejecting attachment 95536 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
e Source/WebKit/qt/tests/qgraphicswebview/tst_qgraphicswebview.cpp.rej
patching file Source/WebKit/qt/tests/qgraphicswebview/tst_qgraphicswebview.qrc
patching file Source/WebKit/qt/tests/qgraphicswebview/resources/56929.html
rm 'Source/WebKit/qt/tests/qgraphicswebview/resources/56929.html'
patching file Source/WebKit/qt/tests/qgraphicswebview/resources/greendiv.html

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Darin Adler', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/10120027

[Carol Szabo]

Created attachment 111727 [details]
Merged reviewed patch with latest code

[WebKit Review Bot]

Comment on attachment 111727 [details]
Merged reviewed patch with latest code

Clearing flags on attachment: 111727

Committed r97945: <http://trac.webkit.org/changeset/97945>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.