Tiled backing store area is too big. It calculated like this: keepRect.inflateX(visibleRect.width() * (m_keepAreaMultiplier.width() - 1.f)); keepRect.inflateY(visibleRect.height() * (m_keepAreaMultiplier.height() - 1.f)); and original intention was to get rect of size keepAreaMultiplier * visibleRect, but inflateX and inflateY methods defined like this: void inflateX(float dx) { m_location.setX(m_location.x() - dx); m_size.setWidth(m_size.width() + dx + dx); } void inflateY(float dy) { m_location.setY(m_location.y() - dy); m_size.setHeight(m_size.height() + dy + dy); } so, they add delta to width/height 2 times, so in real area is calculated like width + width * (ratio - 1) * 2 = width * (2 * ratio - 1)
Created attachment 82694 [details] Divide inflate delta by 2 to fix area calculation.
Created attachment 82696 [details] Fix changelog.
Dropping [Qt] prefix as this is not a Qt-specific bug.
Comment on attachment 82696 [details] Fix changelog. The WK2 TiledDrawingAreaProxy has the same logic, should the change not be done there, too?
Created attachment 82723 [details] Fix also webkit2 tile area calculation.
Comment on attachment 82723 [details] Fix also webkit2 tile area calculation. View in context: https://bugs.webkit.org/attachment.cgi?id=82723&action=review > Source/WebCore/ChangeLog:8 > + Tiled backing store area is too big. > + Error in area calculcation causes size of backing store > + up to 6 times bigger than viewport with default multipliers. > + https://bugs.webkit.org/show_bug.cgi?id=54587 Why dont you explain how it is calculating wrong and why it wouldnt be better to change the default multipliers? > Source/WebCore/platform/graphics/TiledBackingStore.cpp:217 > - keepRect.inflateX(visibleRect.width() * (m_keepAreaMultiplier.width() - 1.f)); > - keepRect.inflateY(visibleRect.height() * (m_keepAreaMultiplier.height() - 1.f)); > + keepRect.inflateX(visibleRect.width() * (m_keepAreaMultiplier.width() - 1.f) / 2); > + keepRect.inflateY(visibleRect.height() * (m_keepAreaMultiplier.height() - 1.f) / 2); I dont understand why this is right. It is called a multiplier. So if we have size 10, and a multiplier of two, we would expect a result of 20. ie 10 + 10 * 1 (10.inflate(10 * (2 - 1))
(In reply to comment #6) > (From update of attachment 82723 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=82723&action=review > > > Source/WebCore/ChangeLog:8 > > + Tiled backing store area is too big. > > + Error in area calculcation causes size of backing store > > + up to 6 times bigger than viewport with default multipliers. > > + https://bugs.webkit.org/show_bug.cgi?id=54587 > > Why dont you explain how it is calculating wrong and why it wouldnt be better to change the default multipliers? > > > Source/WebCore/platform/graphics/TiledBackingStore.cpp:217 > > - keepRect.inflateX(visibleRect.width() * (m_keepAreaMultiplier.width() - 1.f)); > > - keepRect.inflateY(visibleRect.height() * (m_keepAreaMultiplier.height() - 1.f)); > > + keepRect.inflateX(visibleRect.width() * (m_keepAreaMultiplier.width() - 1.f) / 2); > > + keepRect.inflateY(visibleRect.height() * (m_keepAreaMultiplier.height() - 1.f) / 2); > > I dont understand why this is right. It is called a multiplier. So if we have size 10, and a multiplier of two, we would expect a result of 20. ie 10 + 10 * 1 (10.inflate(10 * (2 - 1)) Kenneth, have you looked at Inflate function? I've posted it into explanation. ;) It adds delta 2 times (m_size.width() + dx + dx). So, if you have width 20 and multiplier 2, than you will pass delta to inflate as 10 * (2 - 1) = 10. Inside inflate will do width + dx + dx , so instead of 20 you are getting 30. If you have multiplier 4.5 as in wk2, you will pass delta as 10 * (4.5 - 1) = 35 and after inflate you will get 10 + 35 + 35 = 80 instead of 45. Make it easy - put a breakpoint, calculate what do you expect and check what you get.
(In reply to comment #7) > (In reply to comment #6) > > (From update of attachment 82723 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=82723&action=review > > > > > Source/WebCore/ChangeLog:8 > > > + Tiled backing store area is too big. > > > + Error in area calculcation causes size of backing store > > > + up to 6 times bigger than viewport with default multipliers. > > > + https://bugs.webkit.org/show_bug.cgi?id=54587 > > > > Why dont you explain how it is calculating wrong and why it wouldnt be better to change the default multipliers? > > > > > Source/WebCore/platform/graphics/TiledBackingStore.cpp:217 > > > - keepRect.inflateX(visibleRect.width() * (m_keepAreaMultiplier.width() - 1.f)); > > > - keepRect.inflateY(visibleRect.height() * (m_keepAreaMultiplier.height() - 1.f)); > > > + keepRect.inflateX(visibleRect.width() * (m_keepAreaMultiplier.width() - 1.f) / 2); > > > + keepRect.inflateY(visibleRect.height() * (m_keepAreaMultiplier.height() - 1.f) / 2); > > > > I dont understand why this is right. It is called a multiplier. So if we have size 10, and a multiplier of two, we would expect a result of 20. ie 10 + 10 * 1 (10.inflate(10 * (2 - 1)) > > Kenneth, have you looked at Inflate function? I've posted it into explanation. ;) > It adds delta 2 times (m_size.width() + dx + dx). > So, if you have width 20 and multiplier 2, than you will pass delta to inflate as 10 * (2 - 1) = 10. Inside inflate will do width + dx + dx , so instead of 20 you are getting 30. > If you have multiplier 4.5 as in wk2, you will pass delta as 10 * (4.5 - 1) = 35 and after inflate you will get 10 + 35 + 35 = 80 instead of 45. > Make it easy - put a breakpoint, calculate what do you expect and check what you get. Better not use inflate then :-) and just use setWidth instead! etc
> Better not use inflate then :-) and just use setWidth instead! etc Ok. As you wish! ;)
> Better not use inflate then :-) and just use setWidth instead! etc Oh! No! That's not right. Rect position also needs to be adjusted. IMHO, so far inflate is the cleanest solution.
Ok, then lets add a comment in the code. Apparently it tricked more than one person.
Comment on attachment 82723 [details] Fix also webkit2 tile area calculation. View in context: https://bugs.webkit.org/attachment.cgi?id=82723&action=review r=me with the comment >> Source/WebCore/platform/graphics/TiledBackingStore.cpp:217 >> + keepRect.inflateY(visibleRect.height() * (m_keepAreaMultiplier.height() - 1.f) / 2); > > I dont understand why this is right. It is called a multiplier. So if we have size 10, and a multiplier of two, we would expect a result of 20. ie 10 + 10 * 1 (10.inflate(10 * (2 - 1)) // Inflates to both sides, so divide by 2.
Created attachment 82801 [details] Add comment why delta should be divided by 2
Comment on attachment 82801 [details] Add comment why delta should be divided by 2 Clearing flags on attachment: 82801 Committed r79054: <http://trac.webkit.org/changeset/79054>
All reviewed patches have been landed. Closing bug.