Bug 57798

Created attachment 88153 [details] Patch

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

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

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

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 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.

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 on attachment 88235 [details] Proposed Patch. Fixed problem with missing file r- given mine and Jocelyn's comments

(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.

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.

(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.

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

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

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

Comment on attachment 111727 [details] Merged reviewed patch with latest code Clearing flags on attachment: 111727 Committed r97945: <http://trac.webkit.org/changeset/97945>

All reviewed patches have been landed. Closing bug.