Bug 57798 - Tiled Backing Store does not regenerate tiles when it should
Summary: Tiled Backing Store does not regenerate tiles when it should
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-04 15:51 PDT by Carol Szabo
Modified: 2011-10-20 00:22 PDT (History)
10 users (show)

See Also:


Attachments
Patch (6.58 KB, patch)
2011-04-04 16:31 PDT, Carol Szabo
no flags Details | Formatted Diff | Diff
Proposed Patch. Fixed problem with missing file (7.35 KB, patch)
2011-04-05 08:00 PDT, Carol Szabo
no flags Details | Formatted Diff | Diff
Alternative Patch, to satisfy Kenneth and Joceylin's judgement (8.05 KB, patch)
2011-05-29 22:58 PDT, Carol Szabo
no flags Details | Formatted Diff | Diff
Proposed Patch, per Jocelyn's suggestion. (8.49 KB, patch)
2011-05-31 20:35 PDT, Carol Szabo
darin: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Merged reviewed patch with latest code (8.58 KB, patch)
2011-10-19 23:15 PDT, Carol Szabo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carol Szabo 2011-04-04 15:51:12 PDT
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.
Comment 1 Carol Szabo 2011-04-04 16:31:57 PDT
Created attachment 88153 [details]
Patch
Comment 2 Early Warning System Bot 2011-04-04 16:45:07 PDT
Attachment 88153 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8331137
Comment 3 Carol Szabo 2011-04-05 08:00:01 PDT
Created attachment 88235 [details]
Proposed Patch. Fixed problem with missing file
Comment 4 Eric Seidel (no email) 2011-05-23 15:02:38 PDT
One of the CC'd shoudl easily be able to review this three-week-old patch. :)
Comment 5 Kenneth Rohde Christiansen 2011-05-23 15:05:29 PDT
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.
Comment 6 Kenneth Rohde Christiansen 2011-05-23 15:09:00 PDT
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.
Comment 7 Jocelyn Turcotte 2011-05-24 00:58:30 PDT
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.
Comment 8 Kenneth Rohde Christiansen 2011-05-24 02:04:39 PDT
Comment on attachment 88235 [details]
Proposed Patch. Fixed problem with missing file

r- given mine and Jocelyn's comments
Comment 9 Carol Szabo 2011-05-29 21:15:27 PDT
(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.
Comment 10 Carol Szabo 2011-05-29 22:58:49 PDT
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.
Comment 11 Jocelyn Turcotte 2011-05-30 04:28:30 PDT
(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.
Comment 12 Carol Szabo 2011-05-31 20:35:56 PDT
Created attachment 95536 [details]
Proposed Patch, per Jocelyn's suggestion.

Implemented Jocelyn's suggestion.
Comment 13 WebKit Review Bot 2011-10-17 16:40:43 PDT
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
Comment 14 Carol Szabo 2011-10-19 23:15:30 PDT
Created attachment 111727 [details]
Merged reviewed patch with latest code
Comment 15 WebKit Review Bot 2011-10-20 00:21:57 PDT
Comment on attachment 111727 [details]
Merged reviewed patch with latest code

Clearing flags on attachment: 111727

Committed r97945: <http://trac.webkit.org/changeset/97945>
Comment 16 WebKit Review Bot 2011-10-20 00:22:03 PDT
All reviewed patches have been landed.  Closing bug.