We should just use the flag that is there to mark the whole area dirty, and then calculate the full rectangle at the point of recache instead.
Created attachment 58274 [details] Using the updateAll flag that was there all along.
Comment on attachment 58274 [details] Using the updateAll flag that was there all along. > @@ -458,10 +458,14 @@ void GraphicsLayerQtImpl::paint(QPainter* painter, const QStyleOptionGraphicsIte > switch (m_currentContent.contentType) { > case HTMLContentType: > if (m_state.drawsContent) { > - QPixmap backingStore; > - // We might need to recache, in case we try to paint and the cache was purged (e.g. if it was full). > - if (!QPixmapCache::find(m_backingStoreKey, &backingStore) || backingStore.size() != m_size.toSize()) > - backingStore = recache(QRegion(m_state.contentsRect)); > + QRegion region; > + if( m_pendingContent.updateAll ) > + region |= boundingRect().toRect(); > + else > + region |= m_pendingContent.regionToUpdate; > + m_pendingContent.updateAll = false; > + m_pendingContent.regionToUpdate = QRegion(); > + const QPixmap backingStore = recache(region); > painter->drawPixmap(0, 0, backingStore); This part doesn't seem correct - if we got to this point where we paint and there's nothing in the cache, we might need to paint more than the dirty region. This might occur, for example, when the cache was purged because it was full and some other item obscuring this one has moved away. TtThi
Created attachment 58502 [details] Version of the same idea, fixed a few issues. Sam, does this solve the same problem?
(In reply to comment #2) > (From update of attachment 58274 [details]) > > @@ -458,10 +458,14 @@ void GraphicsLayerQtImpl::paint(QPainter* painter, const QStyleOptionGraphicsIte > > switch (m_currentContent.contentType) { > > case HTMLContentType: > > if (m_state.drawsContent) { > > - QPixmap backingStore; > > - // We might need to recache, in case we try to paint and the cache was purged (e.g. if it was full). > > - if (!QPixmapCache::find(m_backingStoreKey, &backingStore) || backingStore.size() != m_size.toSize()) > > - backingStore = recache(QRegion(m_state.contentsRect)); > > + QRegion region; > > + if( m_pendingContent.updateAll ) > > + region |= boundingRect().toRect(); > > + else > > + region |= m_pendingContent.regionToUpdate; > > + m_pendingContent.updateAll = false; > > + m_pendingContent.regionToUpdate = QRegion(); > > + const QPixmap backingStore = recache(region); > > painter->drawPixmap(0, 0, backingStore); > > This part doesn't seem correct - if we got to this point where we paint and there's nothing in the cache, we might need to paint more than the dirty region. This might occur, for example, when the cache was purged because it was full and some other item obscuring this one has moved away. > I'm not sure I understand. When something is dirty in webkit we'll get the setNeedsDisplay(). Thus the pendingContent updateAll/regionToUpdate knows what is dirty. If graphicsview needs to paint we'll get a paint for ourselves and our children - inside of recache() it will determine if the cache'd pixmap is already gone and if so it will still paint QRegion(0,0,m_size.width(),m_size.height()). Looking it up in ::paint and in ::recache seems unnecessary to me. > > > > TtThi
Oh, I see what you're saying. My bad. Your patch is right though it should follow the webkit coding style before it can be accepted :) No'am
Created attachment 58556 [details] Proposed patch with proper coding style
Created attachment 58577 [details] Same patch - some cleanup There was some code duplication and style issues that I'm cleaning up with this patch. Sam, if it works for you you can resubmit or I can mark mine for review, whatever works for you.
Created attachment 58578 [details] Patch (See above)
This patch will do for me, I like that recache() doesn't require the same (semi-redundant) region. However, it seems dubious (to me) that you clear out the regionToUpdate and updateAll members in recache, and thus have to call update first. Maybe just as well to pass a flag to recache that tells it to do the update at the end? In any event this looks to achieve the same as my proposed patch, thanks. //Sam
Created attachment 58808 [details] Putting up my patch and No'am's changes for review now that I finally tested it
Created attachment 59432 [details] Rediff against trunk
Attachment 59432 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:686: Extra space after ( in function call [whitespace/parens] [4] WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:686: Extra space before ) [whitespace/parens] [2] Total errors found: 2 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 59440 [details] ...and fix style issue.
Comment on attachment 59440 [details] ...and fix style issue. WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:170 + QPixmap recache(bool); In the WebKit project we are trying to move away from using bools as arguments where it is not obvious what they mean. recache(false) does that mean to not recache? probably not. Use an enum instead. WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:348 + // Nothing to render Add dot at end of comment. WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:343 + m_pendingContent.updateAll = false; All what? all regions? everything?
We're rewriting AC in a cross-platform manner. New features won't be added to GraphicsLayerQt.